Fix brighness not being applied properly in nightlight mode#5625
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR refactors how transitions and brightness are finalized in LED state updates. When no transition is active, ChangesLED transition finalization refactor
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)wled00/led.cpp┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.16][ERROR]: unable to find a config; path Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
wled00/led.cpp (1)
234-235: 💤 Low valueNightlight brightness fix is correct; minor optimization opportunity.
The
applyFinalBri()call successfully prevents transitions from blocking brightness updates during nightlight fade. This fixes the reported issue where repeatedcolorUpdated()calls every 100ms would restart transitions, preventingbrifrom changing.However, when
strip.getTransition() == 0,stateUpdated()(called bycolorUpdated()) already invokesapplyFinalBri()at line 128, so line 235 results in a redundant second call. While harmless, you could optimize by guarding the call:if (strip.getTransition() > 0) applyFinalBri();This ensures the bypass only happens when transitions are enabled. Given the 100ms rate limit and nightlight-only scope, the current unconditional approach is acceptable and keeps the fix simple.
♻️ Optional optimization to avoid redundant call
colorUpdated(CALL_MODE_NO_NOTIFY); -applyFinalBri(); // colorUpdated() re-starts transition (if enabled) every 100ms, preventing brightness from changing correctly (fix for `#5620`) +if (strip.getTransition() > 0) applyFinalBri(); // bypass transition restart to allow brightness updates (fix for `#5620`)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/led.cpp` around lines 234 - 235, colorUpdated() followed by an unconditional applyFinalBri() causes a redundant call when no transition is active because stateUpdated() already calls applyFinalBri(); update the code around colorUpdated(CALL_MODE_NO_NOTIFY) so you only call applyFinalBri() when a transition is enabled (check strip.getTransition() > 0) to avoid the duplicate call while preserving the nightlight fix; reference colorUpdated(), applyFinalBri(), strip.getTransition(), and stateUpdated() when locating the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@wled00/led.cpp`:
- Around line 234-235: colorUpdated() followed by an unconditional
applyFinalBri() causes a redundant call when no transition is active because
stateUpdated() already calls applyFinalBri(); update the code around
colorUpdated(CALL_MODE_NO_NOTIFY) so you only call applyFinalBri() when a
transition is enabled (check strip.getTransition() > 0) to avoid the duplicate
call while preserving the nightlight fix; reference colorUpdated(),
applyFinalBri(), strip.getTransition(), and stateUpdated() when locating the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 12f2b97d-bcac-40fd-922f-ae044ec9bc59
📒 Files selected for processing (1)
wled00/led.cpp
|
in order to properly fix it, transitions should be disabled in nighlight mode. @blazoncek has fixed it but the change is complex and I did not yet fully work through that change to port it upstream. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wled00/led.cpp`:
- Around line 234-237: The issue is that the code unconditionally restores the
saved transition duration after calling colorUpdated(), but that function can
trigger callbacks through UsermodManager::onStateChange() that might modify the
transition value. To fix this, only restore the saved transition duration if the
current transition value is still 0 when checked right before the
strip.setTransition(transitionduration) call, indicating no callback has changed
it. This ensures you do not overwrite intentional transition changes made during
the colorUpdated(CALL_MODE_NO_NOTIFY) call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6f3e0bc8-001d-4383-b47e-0e9d1a4b0cbd
📒 Files selected for processing (1)
wled00/led.cpp
| uint16_t transitionduration = strip.getTransition(); | ||
| strip.setTransition(0); // temporary disable transition and set color & brightness directly, (fix for #5620) | ||
| colorUpdated(CALL_MODE_NO_NOTIFY); | ||
| strip.setTransition(transitionduration); // restore transition time to previous value. Note: this is a dirty hack and needs proper fix by disabling transitions completely in nightlight mode |
There was a problem hiding this comment.
Avoid clobbering transition updates made during colorUpdated().
colorUpdated(CALL_MODE_NO_NOTIFY) flows into stateUpdated(), which calls UsermodManager::onStateChange(callMode). Unconditionally restoring the saved transition right after that can overwrite transition changes made in that callback path.
Suggested patch
- uint16_t transitionduration = strip.getTransition();
+ const uint16_t previousTransition = strip.getTransition();
strip.setTransition(0); // temporary disable transition and set color & brightness directly, (fix for `#5620`)
colorUpdated(CALL_MODE_NO_NOTIFY);
- strip.setTransition(transitionduration); // restore transition time to previous value. Note: this is a dirty hack and needs proper fix by disabling transitions completely in nightlight mode
+ // Restore only if no callback changed transition state during colorUpdated().
+ if (strip.getTransition() == 0) {
+ strip.setTransition(previousTransition);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uint16_t transitionduration = strip.getTransition(); | |
| strip.setTransition(0); // temporary disable transition and set color & brightness directly, (fix for #5620) | |
| colorUpdated(CALL_MODE_NO_NOTIFY); | |
| strip.setTransition(transitionduration); // restore transition time to previous value. Note: this is a dirty hack and needs proper fix by disabling transitions completely in nightlight mode | |
| const uint16_t previousTransition = strip.getTransition(); | |
| strip.setTransition(0); // temporary disable transition and set color & brightness directly, (fix for `#5620`) | |
| colorUpdated(CALL_MODE_NO_NOTIFY); | |
| // Restore only if no callback changed transition state during colorUpdated(). | |
| if (strip.getTransition() == 0) { | |
| strip.setTransition(previousTransition); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wled00/led.cpp` around lines 234 - 237, The issue is that the code
unconditionally restores the saved transition duration after calling
colorUpdated(), but that function can trigger callbacks through
UsermodManager::onStateChange() that might modify the transition value. To fix
this, only restore the saved transition duration if the current transition value
is still 0 when checked right before the strip.setTransition(transitionduration)
call, indicating no callback has changed it. This ensures you do not overwrite
intentional transition changes made during the colorUpdated(CALL_MODE_NO_NOTIFY)
call.
|
I changed this into a hack-fix which is good for now but needs proper rework of how nightlight mode works. |
* Fix brighness not being applied properly in nightlight mode * hacky fix also for color fade, needs proper fix later
|
cherry picked to 16_x |
2 participants
fix for #5620
Summary by CodeRabbit