Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix oled_render to render all dirty blocks. #18887

Merged
merged 1 commit into from
Nov 13, 2022
Merged

Conversation

rvnash
Copy link
Contributor

@rvnash rvnash commented Oct 27, 2022

Description

oled_render is meant to render all dirty changes to the device, but it only renders the first dirty block it finds. In practice this does not usually cause much of a problem because it is called periodically from oled_task, and eventually it works its way through all dirty blocks and they get rendered.

However, if you update the oled very fast, like in an animation, the first blocks are made re-dirty before the oled_task loop can catch up w/ rendering the later blocks. This causes the later blocks to not get rendered.

This change fixes that problem by rendering all dirty blocks in oled_render which is what I believe users of that function would expect it to do.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

  • none

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@github-actions github-actions bot added the core label Oct 27, 2022
@rvnash rvnash marked this pull request as ready for review October 27, 2022 23:40
@zvecr zvecr requested a review from a team November 12, 2022 23:40
@tzarc tzarc merged commit 4c1d8a0 into qmk:develop Nov 13, 2022
@sigprof
Copy link
Contributor

sigprof commented Nov 13, 2022

Actually oled_render() was updating just a single dirty block on purpose — so that updating the display won't result in a large delay between keyboard matrix scans. (This is the poor man's substitute for multithreading that is used in many places in QMK — e.g., RGB matrix calculations are handled that way too.)

Now, if the whole OLED was marked dirty, for a 128×32 OLED just the data for those blocks would occupy 512 bytes, and transferring those 512 bytes over a 400 kHz I2C would take about 12 ms even without taking the command overhead into account; a 128×64 OLED would require more than 25 ms to transfer the whole data at once. However, dividing the 128×32 OLED data into 16 blocks results in 32 bytes per block, which brings the data transfer time below 1 ms (not counting the command overhead again).

One possible fix that won't result in excessive delays during OLED updates is to save the last updated block number between updates and continue from the next block instead of starting the search from the first block every time; this could result in some mix of old and new data during updates, but would probably better than stopping the update on the first dirty block.

@rvnash
Copy link
Contributor Author

rvnash commented Nov 13, 2022

save the last updated block number between updates and continue from the next block

I think that is a good idea ... and have oled_task call the routine which does that. However, I still think an explicit call to oled_render should flush the whole cache. What do you say? Want me to codes that solution?

@sigprof
Copy link
Contributor

sigprof commented Nov 13, 2022

The current quick fix will probably be #19068 (it effectively reverts your PR by default to avoid introducing latency problems, but you can set OLED_UPDATE_PROCESS_LIMIT to a larger value if you prefer that; setting it to OLED_BLOCK_COUNT will make oled_render() update the whole display at once). Note that it does not implement the proposed behavior of continuing the update from the point where the previous iteration stopped instead of always restarting from the beginning; that could be introduced in another PR for the next development cycle.

Note that in many cases the problem that you are attempting to fix is actually caused by some bug in the oled_task_user() implementation — a very common error is writing some OLED update code that draws multiple times over the same place during a single invocation of oled_task_user(), which results in some blocks always being marked dirty. If the render code gets updated to support that (either by drawing everything at once, or by restarting from where the previous iteration had stopped), such bugs would be mostly invisible, except the OLED_TIMEOUT feature won't work (because that timeout gets reset after any OLED content change), which may be harder to notice.

As for changing the behavior for an explicit call of oled_render(), that could make sense in some really special cases (e.g., when you have some code that blocks the main loop for some good reason, but also needs to force the OLED to be updated). This would probably involve refactoring oled_render() to move the actual updating code into a separate function that could take some parameter for the update mode.

ramonimbao pushed a commit to ramonimbao/qmk_firmware that referenced this pull request Nov 28, 2022
elpekenin pushed a commit to elpekenin/qmk_firmware that referenced this pull request Dec 7, 2022
crembz pushed a commit to crembz/qmk_firmware that referenced this pull request Dec 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants