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.

Sign up for GitHub

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

Jump to bottom

Avoid repeated calls to rgblight_set() in tight succession when setting lighting layers #18338

Merged
merged 8 commits into from
Nov 10, 2022
Merged

Avoid repeated calls to rgblight_set() in tight succession when setting lighting layers #18338

merged 8 commits into from
Nov 10, 2022

Conversation

spidey3
Copy link
Contributor

@spidey3 spidey3 commented Sep 11, 2022 •
edited

Description

See https://discord.com/channels/440868230475677696/440868230475677698/1018496514470981642.

There is a hypothesis that quickly repeated calls to rgblight_set() can cause problems with certain led drivers on certain boards / MCUs. That situation occurs when:

  • there is code in layer_state_set_user() to select lighting layers based on the currently enabled keymap layers, and...
  • a static rgblight animation mode is displayed, OR
  • RGBLIGHT_LAYERS_OVERRIDE_RGB_OFF is set and rgblight is currently disabled.

Regardless of whether the repeated calls to rgblight_set() cause timing issues, it's much more efficient to avoid them. This PR eliminates the repeated calls to rgblight_set() by deferring the action until the next invocation of rgblight_task().

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 the core label Sep 11, 2022
@spidey3 spidey3 changed the title Defer layer rgblight set Experiment: Defer layer rgblight set Sep 14, 2022
@github-actions github-actions bot added cli qmk cli command dependencies documentation keyboard keymap python translation via Adds via keymap and/or updates keyboard for via support labels Sep 18, 2022
@spidey3 spidey3 changed the base branch from master to develop September 18, 2022 18:18
@spidey3 spidey3 added bug needs testing and removed keyboard keymap documentation python cli qmk cli command translation via Adds via keymap and/or updates keyboard for via support dependencies labels Sep 18, 2022
sigprof
sigprof requested changes Sep 18, 2022
View reviewed changes
Copy link
Contributor

@sigprof sigprof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs some testing, but looks like it should work

quantum/rgblight/rgblight.c Outdated Show resolved Hide resolved
@spidey3 @sigprof
deferred_set_layer_state shouldn't be global
aaaec4d 
Co-authored-by: Sergey Vlasov <sigprof@gmail.com>
@spidey3 spidey3 changed the title Experiment: Defer layer rgblight set Defer layer rgblight set to avoid repeated calls to rgblight_set() in tight succession Sep 18, 2022
@spidey3 spidey3 changed the title Defer layer rgblight set to avoid repeated calls to rgblight_set() in tight succession Avoid repeated calls to rgblight_set() in tight succession when setting lighting layers Sep 18, 2022
@spidey3 spidey3 marked this pull request as ready for review September 18, 2022 22:40
@spidey3 spidey3 requested review from sigprof and removed request for sigprof September 18, 2022 22:40
@spidey3 spidey3 requested a review from a team September 28, 2022 16:27
@drashna drashna requested a review from a team October 25, 2022 05:59
@spidey3 spidey3 added breaking_change Changes that need to wait for a version increment breaking_change_2022q4 labels Nov 9, 2022
@tzarc tzarc merged commit f6baf91 into qmk:develop Nov 10, 2022
@spidey3 spidey3 deleted the defer_layer_rgblight_set branch November 15, 2022 17:36
ramonimbao pushed a commit to ramonimbao/qmk_firmware that referenced this pull request Nov 28, 2022
@spidey3 @sigprof @ramonimbao
Avoid repeated calls to rgblight_set() in tight succession when setti…
ce1bb0c 
…ng lighting layers (qmk#18338)

Co-authored-by: Sergey Vlasov <sigprof@gmail.com>
elpekenin pushed a commit to elpekenin/qmk_firmware that referenced this pull request Dec 7, 2022
@spidey3 @sigprof @elpekenin
Avoid repeated calls to rgblight_set() in tight succession when setti…
7f5fb56 
…ng lighting layers (qmk#18338)

Co-authored-by: Sergey Vlasov <sigprof@gmail.com>
@climent
Copy link
Contributor

climent commented Dec 20, 2022

This PR broke RGBLIGHT for my setup. I do not know if this is a widespread problem, or just particular to my setup, but with both an ELITE-C and a FROOD (rp2040) boards, reverting this PR returns the RGB operation to a working state.

Way it breaks, with this PR:

  • Flashed my atreyu board (https://github.com/climent/qmk_firmware/tree/climent-develop/keyboards/atreyu/keymaps/pcb-rp2040) with either the CONVERT_TO (for rp2040) and without (for the ELITE-C)
  • all operations on layers (RAISE, LOWER and CAPS_LOCK) do not work
  • RGB_* commands cause the LEDs to glow green, and no further changes happen on the LEDs, although restarting the board indicates that the EEPROM was written after a successful operation, as the LEDs now show the new state (for example, running the RGB_HUI changes the HUE to the new value, but it is only visible after reattaching the board).

I tested it by checking out ae94be9, reverting this PR (patching my tree with git diff + patch -p1 < patch), recompiling the code and reflashing the board (both ELITE-C and rp2040 MCUs).

@alinelena alinelena mentioned this pull request Jan 6, 2023
Open
2 tasks
elipsitz added a commit to elipsitz/qmk_firmware that referenced this pull request Mar 12, 2023
@elipsitz
Fix rgblight layers when animations aren't enabled
ae87707 
PR qmk#18338 introduced a change that deferred rgblight_set after a call to
rgblight_set_layer_state to the next invocation of rgblight_task.

However, rgblight_task is a no-op unless RGBLIGHT_USE_TIMER is set,
which only happens automatically if an RGB animation is enabled, or if
RGBLIGHT_LAYER_BLINK is enabled.

If neither of these are enabled, rgblight_set is never called
automatically after rgblight_set_layer_state, so the LED state is never
actually set.

This commit fixes this issue by ensuring that RGBLIGHT_USER_TIMER is set
if rgblight layers are enabled.
@elipsitz elipsitz mentioned this pull request Mar 12, 2023
Merged
14 tasks
kilinrax pushed a commit to kilinrax/qmk_firmware that referenced this pull request Mar 25, 2023
Revert "Avoid repeated calls to rgblight_set() in tight succession wh…
7480558 
…en setting lighting layers (qmk#18338)"

This reverts commit f6baf91.
@sigprof sigprof mentioned this pull request Apr 1, 2023
Closed
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sigprof sigprof sigprof approved these changes

@drashna drashna drashna approved these changes

@tzarc tzarc tzarc approved these changes

Assignees
No one assigned
Labels
breaking_change Changes that need to wait for a version increment bug core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@spidey3 @climent @sigprof @drashna @tzarc