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

Luna keyboard pet OLED timeout fix #17189

Merged
merged 1 commit into from
Sep 30, 2022
Merged

Conversation

joukewitteveen
Copy link
Contributor

@joukewitteveen joukewitteveen commented May 22, 2022

The old keyboard pet code would cause the screen to flicker after timeout because the animation would keep on running and each new frame would turn the screen on, after which the timeout logic would turn it off (provided the screen was idle for > OLED_TIMEOUT). The same flicker would be triggered by other triggers that turn the screen on, for instance toggling Caps Lock.

Description

The on/off bug is fixed by using the animation idle timer (anim_sleep) also to track whether the screen is off or not. If the screen turns on (for whatever reason), we restart the timer. Also, we only render the keyboard pet when the screen is on, to prevent causing it to turn on.

Implications and Alternatives

The keyboard pet OLED timeout logic duplicates the timeout logic in the OLED driver. There are two more thorough solutions I can think of that make reimplementing the logic unnecessary.

  1. If the wpm counter drops to 0, we could render a single frame image of a sleeping pet. As this image doesn't change, the screen doesn't update and the timeout isn't reset each time the pet is rendered.
  2. Support for OLED_IDLE_BLOCKS_MASK is easy to implement. This macro would specify a OLED_BLOCK_TYPE value and blocks in the mask would not cause the screen to be turned on (and the timeout timer to be reset).

For (2), the oled_on() line in drivers/oled/ssd1306_sh1106.c:oled_render would change to

    if ((OLED_BLOCK_TYPE)1 << update_start) & ~((OLED_BLOCK_TYPE) OLED_IDLE_BLOCKS_MASK) {
        oled_on();
    }

This second option would offer a generic solution for using the screen timeout when a long-running animation is being shown. The downside is that figuring out the correct blocks mask is not very ergonomic. It is also a bit of a misuse of the block-based rendering logic.

As a remnant of looking into this, I had the addition of an assertion in the OLED driver implementation laying around. I added it to this PR. The assertion is violated in code by @7-rate, but indeed: that code is broken since support for fonts with lines of any height other than 8 pixels is not implemented.

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

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 core keymap via Adds via keymap and/or updates keyboard for via support labels May 22, 2022
@joukewitteveen joukewitteveen force-pushed the oled-on-off-bug branch 2 times, most recently from 9512f38 to 14f1744 Compare May 22, 2022 14:19
drivers/oled/ssd1306_sh1106.c Outdated Show resolved Hide resolved
The animation itself turns the screen on, preventing the normal timeout
from ever triggering.
@joukewitteveen
Copy link
Contributor Author

@alexmalott: In your recent #17835 you must have encountered the screen flickering bug. What do you think of the alternative fix proposed here?

@drashna drashna requested a review from a team September 26, 2022 05:27
@spidey3 spidey3 merged commit 8bd73d4 into qmk:develop Sep 30, 2022
ramonimbao pushed a commit to ramonimbao/qmk_firmware that referenced this pull request Nov 28, 2022
The animation itself turns the screen on, preventing the normal timeout
from ever triggering.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keymap via Adds via keymap and/or updates keyboard for via support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants