make per-segment on/off and brightness behave the same as global - #5524
Closed
DedeHai wants to merge 3 commits into
Closed
make per-segment on/off and brightness behave the same as global#5524DedeHai wants to merge 3 commits into
DedeHai wants to merge 3 commits into
Conversation
Contributor
WalkthroughChanges adjust Segment transition handling (startTransition, currentBri, setOpacity), modify WS2812FX::blendSegment non-FADE off-state rendering, and restrict global transition start in stateUpdated() to brightness changes only; also add jsonTransitionOnce one-time transition delay when bri is unchanged. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant LEDController
participant SegmentManager
participant Renderer
Client->>LEDController: update state (bri, on/off, jsonTransitionOnce)
LEDController->>LEDController: if bri != briOld -> set/restart global transition timer
LEDController->>SegmentManager: startTransition(segmentCopy? / blendingStyle)
alt segmentCopy requested but no _oldSegment & non-FADE
SegmentManager->>SegmentManager: stopTransition() (cancel)
else TRANSITION_FADE or _oldSegment exists
SegmentManager->>SegmentManager: create/reset _oldSegment, set _t->_start/_dur/_bri
end
SegmentManager->>Renderer: provide segment state for blending
Renderer->>Renderer: blendSegment(): apply opacity/brightness
Renderer->>Renderer: if per-segment off (non-FADE) -> force BLACK pixel
Renderer-->>Client: output pixels
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/FX_fcn.cpp`:
- Around line 289-313: When isInTransition() returns early the active transition
(_t) isn't refreshed, causing stale progress; update the transition timing and
brightness before any early return: inside the isInTransition() branch of
Segment::startTransition() (the branch that currently returns without creating a
copy or when segmentCopy is false), set _t->_start = millis(), _t->_dur = dur,
and _t->_bri = currentBri() (and optionally reset _t->_prevPaletteBlends) before
returning so rapid per-segment retriggers refresh the active transition state
rather than jump or immediately finish.
In `@wled00/led.cpp`:
- Around line 131-146: When a transition is active do not overwrite the
previous-completed baseline (briOld) with the in-flight interim value (briT);
instead introduce and use a separate requested/target brightness variable (e.g.,
briTarget) to detect intentional brightness changes and only restart global
transitions when bri != briTarget (or when briTarget is updated), keep
transitionActive/transitionStartTime logic tied to that explicit target change,
and remove the commented-out old branch; update references to briOld, briT, bri,
transitionActive, strip.setTransitionMode, transitionStartTime,
jsonTransitionOnce and transitionDelay accordingly so segment-only updates
during an active transition do not reset the global transition timer.
🪄 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: 685a4033-b915-4810-8901-d106ef11acb0
📒 Files selected for processing (2)
wled00/FX_fcn.cppwled00/led.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wled00/FX_fcn.cpp (1)
289-325:⚠️ Potential issue | 🟠 MajorPreserve the in-flight brightness when replacing a no-copy transition.
When Line 295 stops an active non-FADE brightness transition, the replacement transition at Line 325 is initialized from raw
on ? opacity : 0instead of thevisibleBricaptured at Line 290. Mid-transition on/off or effect retargets can still jump to the target brightness instead of continuing from the visible level.Proposed fix
void Segment::startTransition(uint16_t dur, bool segmentCopy) { if (dur == 0 || !isActive()) { if (isInTransition()) _t->_dur = 0; return; } + uint8_t initialBri = on ? opacity : 0; if (isInTransition()) { const uint8_t visibleBri = currentBri(); if (segmentCopy && !_t->_oldSegment) { if (blendingStyle != TRANSITION_FADE) { // transition without a copy means we are in a segment brightness transition // need to cancel the transition to not interfere with the new one (or global brightness change) stopTransition(); + initialBri = visibleBri; } else { // already in FADE transition but segment copy requested and not yet created (for example a SWIPE color change during brightness transition) // create segment copy and restart timer so currentBri() can interpolate correctly _t->_oldSegment = new(std::nothrow) Segment(*this); // store/copy current segment settings @@ _t = new(std::nothrow) Transition(dur); if (_t) { - _t->_bri = on ? opacity : 0; + _t->_bri = initialBri;Based on learnings:
Segment::startTransition()should reset_t->_start = millis(),_t->_dur = dur, and_t->_bri = currentBri()in theisInTransition()branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX_fcn.cpp` around lines 289 - 325, The new Transition allocated in Segment::startTransition() is initialized from raw on ? opacity : 0 after stopping a non-FADE in-flight brightness transition, which loses the currently visible brightness; when replacing a running transition (isInTransition() branch that calls stopTransition()) set the new transition's timing and base brightness to continue from the visible level by assigning _t->_start = millis(), _t->_dur = dur and _t->_bri = currentBri() (or visibleBri) after _t = new(std::nothrow) Transition(dur) so the new _t uses the interpolated brightness rather than on ? opacity : 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/FX_fcn.cpp`:
- Around line 373-377: The non-FADE branch always applies max(curBri, _t->_bri)
when _t->_oldSegment exists, causing brightness to stick when only brightness
changed; modify the logic so the max rule is used only when the old and new "on"
states differ (compare _t->_oldSegment->on vs _t->on or equivalent), otherwise
perform the same interpolation used for FADE (use the prog-based blend formula:
prog * curBri + _t->_bri * (0xFFFFU - prog) divided by 0xFFFFU). Update the
conditional around blendingStyle/ _t->_oldSegment to check the on-state
difference and apply max only in that case.
---
Duplicate comments:
In `@wled00/FX_fcn.cpp`:
- Around line 289-325: The new Transition allocated in
Segment::startTransition() is initialized from raw on ? opacity : 0 after
stopping a non-FADE in-flight brightness transition, which loses the currently
visible brightness; when replacing a running transition (isInTransition() branch
that calls stopTransition()) set the new transition's timing and base brightness
to continue from the visible level by assigning _t->_start = millis(), _t->_dur
= dur and _t->_bri = currentBri() (or visibleBri) after _t = new(std::nothrow)
Transition(dur) so the new _t uses the interpolated brightness rather than on ?
opacity : 0.
🪄 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: 47bc4210-0d1e-4aee-a40a-9afd0be79ae9
📒 Files selected for processing (2)
wled00/FX_fcn.cppwled00/led.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- wled00/led.cpp
Collaborator
Author
|
@coderabbitai please compile a test plan to verify the changes, focus on edge cases that might behave incorrectly. also check if any externally applied changes by sync or home assistent etc. work |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me pull the relevant code sections to write a precise test plan. 🏁 Script executed: Length of output: 2819 🏁 Script executed: Length of output: 15360 🏁 Script executed: Length of output: 15194
Test Plan — PR
|
| # | Scenario | Expected | Changed code path |
|---|---|---|---|
| A1 | Turn one segment OFF from idle (no prior transition) — non-FADE style | Non-FADE transition plays; off-region pixels forced to BLACK via if (segO && !seg->on) c_a = BLACK |
blendSegment() slow-path |
| A2 | Turn one segment ON from idle — non-FADE style | Transition plays; std::max(curBri, _t->_bri) keeps brightness correct from the first frame |
currentBri() |
| A3 | Rapid OFF → ON while non-FADE transition is still running (no _oldSegment yet) |
stopTransition() is now called; new transition restarts cleanly from current visible brightness — no stuck/stale clock |
startTransition() |
| A4 | Rapid ON → OFF while non-FADE transition is still running | Same as A3 — no ghost brightness | startTransition() |
| A5 | Repeat A3/A4 five times quickly (stress) | Each cycle produces a full, clean transition; no permanent dim or full-bright stuck state | startTransition(), currentBri() |
B. setOpacity() — segmentCopy flag change
| # | Scenario | Expected | Changed code path |
|---|---|---|---|
| B1 | Fade opacity slider from 255 → 128 with non-FADE blending style | segmentCopy=false is passed — no segment copy allocated; brightness crossfades using _t->_bri interpolation only |
setOpacity() |
| B2 | Rapid opacity changes mid-transition (non-FADE) | Transition clock restarted each time from currentBri(); no allocation loop / memory exhaustion |
startTransition() restart branch |
| B3 | Opacity change while FADE blending style | Behavior unchanged — FADE branch not affected | setOpacity() |
C. FADE style — new copy-creation branch
| # | Scenario | Expected | Changed code path |
|---|---|---|---|
| C1 | Trigger a SWIPE/color change while a per-segment FADE brightness transition is in progress (no _oldSegment yet) |
_oldSegment is now created, timer reset, _bri set to visible brightness; crossfade is smooth with no color pop |
startTransition() FADE+copy branch |
| C2 | Same as C1 but the newly allocated _oldSegment has isActive() == false |
stopTransition() is called immediately; no orphaned _oldSegment pointer |
startTransition() FADE+copy branch |
D. currentBri() — std::max for non-FADE on/off
| # | Scenario | Expected | Changed code path |
|---|---|---|---|
| D1 | Turn off segment (non-FADE, _oldSegment exists): progress goes 0→0xFFFF |
Brightness is std::max(curBri=0, _t->_bri=old_opacity) = remains at old_opacity until effect clips it to BLACK |
currentBri() |
| D2 | Turn on segment (non-FADE, _oldSegment exists): progress goes 0→0xFFFF |
std::max(curBri=opacity, _t->_bri=0) = opacity from frame 1; no dark flicker at transition start |
currentBri() |
| D3 | Non-FADE style but not an on/off transition (e.g. palette-only change, _oldSegment->on == on) |
Falls back to FADE-blend path ((prog * curBri + _t->_bri * (0xFFFFU - prog)) / 0xFFFFU) |
currentBri() |
E. Global brightness re-targeting (led.cpp)
| # | Scenario | Expected | Changed code path |
|---|---|---|---|
| E1 | Brightness slider 0 → 255 during an ongoing 0 → 128 transition | briOld = briT captures mid-flight value; interpolation restarts from there — no brightness jump |
stateUpdated() |
| E2 | Three rapid brightness changes before any transition completes | Each re-target uses the current briT; final value is reached smoothly |
stateUpdated() |
| E3 | Brightness change while bri == briOld but jsonTransitionOnce = true |
strip.setTransition(transitionDelay) is applied and flag cleared; no transition fires on brightness (it's equal), but the next effect change uses the temporary transition duration |
stateUpdated() |
F. jsonTransitionOnce with equal brightness
| # | Scenario | Expected | Changed code path |
|---|---|---|---|
| F1 | JSON payload sets same brightness + tt field (one-time transition) |
setTransition(transitionDelay) called, jsonTransitionOnce cleared; subsequent color/effect change uses the specified duration |
stateUpdated() |
| F2 | Preset recall with matching brightness | Same as F1 — transition delay applied even though bri didn't change |
stateUpdated() |
G. Multiple segments interaction
| # | Scenario | Expected | Changed code path |
|---|---|---|---|
| G1 | Two non-overlapping segments: one OFF (non-FADE transition), one ON | if (segO && !seg->on) c_a = BLACK only sets the off-segment's pixels to BLACK; on-segment unaffected |
blendSegment() slow-path |
| G2 | Two overlapping segments: both in simultaneous non-FADE on/off transitions | Each segment's pixels independently forced BLACK in their off-state; no cross-segment bleed | blendSegment() slow-path |
| G3 | Single-pixel segment with non-FADE style | blendingStyle is forced to TRANSITION_FADE for width*height==1; std::max branch is never reached |
blendSegment() / currentBri() |
H. External integrations — Home Assistant, Sync, MQTT, Alexa, Presets
| # | Scenario | Expected | Notes |
|---|---|---|---|
| H1 | Home Assistant sends {"on":false,"transition":1} to a single segment via JSON API |
Per-segment off-transition behaves identically to UI toggle; blendSegment() BLACK-forcing applies |
Full HTTP JSON path |
| H2 | Home Assistant sends brightness change mid-transition ({"bri":100}) |
briOld = briT re-targeting applies (same as UI slider); no jump |
stateUpdated() |
| H3 | UDP Sync (CALL_MODE_NOTIFICATION) delivers a brightness change while local transition is active | stateUpdated() is called; transition re-targeting (briOld = briT) still runs because the bri != briOld branch is not gated on callMode |
stateUpdated() |
| H4 | UDP Sync delivers a per-segment on/off change mid-non-FADE transition | startTransition() restart branch fires; stopTransition() called if segmentCopy=true + non-FADE + no _oldSegment |
startTransition() |
| H5 | MQTT cmnd/wled/api sends {"seg":[{"on":false}]} |
Same path as HA JSON; per-segment transition triggers correctly | Full JSON path |
| H6 | Alexa brightness change (0–100% mapped to 0–255) | Hits setOpacity() → now always segmentCopy=false regardless of blend style |
setOpacity() |
| H7 | Preset load that flips multiple segments on/off simultaneously | Each segment calls startTransition() independently; non-FADE segments get stopTransition() + fresh start, FADE segments get copy |
startTransition() |
| H8 | Playlist cycling with jsonTransitionOnce mid-cycle while brightness is stable |
setTransition(transitionDelay) is applied for the effect change; no spurious brightness interpolation |
stateUpdated() |
I. Global on/off vs per-segment on/off interaction in blendSegment()
| # | Scenario | Expected | Notes |
|---|---|---|---|
| I1 | Global bri = 0 (off) while a per-segment non-FADE transition is active | Both the global (briOld==0 || bri==0) condition and the new if (segO && !seg->on) condition may apply; pixels go BLACK, no conflict |
Dual condition in blendSegment() |
| I2 | Global on (briOld=0 → bri>0) while a segment is individually off | Global path sets clipped pixels to BLACK; per-segment !seg->on also sets BLACK — double BLACK is harmless |
blendSegment() slow-path |
| I3 | Segment is individually on but global bri is transitioning 0→255 |
seg->on == true, so !seg->on path is skipped; global fade applies normally |
blendSegment() slow-path |
J. Memory safety
| # | Scenario | Expected | Notes |
|---|---|---|---|
| J1 | stopTransition() called inside startTransition() (non-FADE, segmentCopy, no _oldSegment) followed immediately by the normal fresh allocation path |
_t is deleted, then new Transition(dur) allocates fresh — no double-free, no use-after-free |
startTransition() |
| J2 | new(std::nothrow) Segment(*) for _oldSegment returns nullptr (OOM) in FADE copy branch |
Guard if (_t->_oldSegment) prevents null dereference; stopTransition() is called if allocation fails |
startTransition() FADE copy branch |
Recommended test sequence (manual, on hardware)
- Set a non-FADE blending style (e.g. Fairy Dust or Swipe Right), transition duration ~800 ms.
- Run scenarios A3/A4/A5 — rapidly toggle segment on/off via UI, HA, and MQTT within the transition window.
- Set transition to ~1000 ms and drag the brightness slider repeatedly mid-transition (scenario E1/E2).
- Use a Home Assistant automation to fire on → off → on within 200 ms intervals (covers H1–H4).
- Confirm
briOld = briTre-targeting by loggingbriTover serial during rapid HA brightness commands. - Load a preset with 3+ segments in different on/off states and verify each transitions independently (H7).
- Test with a single-pixel segment to confirm FADE-override path (G3).
🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-25T12:25:11.610Z
Learning: In WLED's `Segment::startTransition()` (wled00/FX_fcn.cpp:284), when `isInTransition()` is true and `_t->_oldSegment` already exists, the function silently returns without updating `_t->_start` or `_t->_bri`. This causes a bug where rapid successive on/off toggles during a non-FADE blending transition (e.g., fairy dust) leave the transition clock stale: by the time of the second re-trigger, elapsed time may already exceed `_dur`, so `updateTransitionProgress()` sets `_progress = 0xFFFF` immediately on the next service tick and `stopTransition()` fires — the blending effect never plays. The fix is to always reset `_t->_start = millis()`, `_t->_dur = dur`, and `_t->_bri = currentBri()` (current visible brightness) in the `isInTransition()` branch, regardless of whether `_oldSegment` exists.
Learnt from: DedeHai
Repo: wled/WLED PR: 4615
File: wled00/FX.cpp:10824-10824
Timestamp: 2026-03-29T06:08:02.547Z
Learning: WLED: In wled00/FX.cpp::mode_slow_transition(), the change-detection logic intentionally compares data->currentCCT to SEGMENT.cct (not data->endCCT). SEGMENT.cct is set to currentCCT at the end of each call; comparing to endCCT would re-initialize the transition on each frame and stall CCT blending. Do not propose changing this.
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR `#4798`, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: softhack007
Repo: wled/WLED PR: 5443
File: wled00/FX_fcn.cpp:1277-1277
Timestamp: 2026-03-24T12:10:32.630Z
Learning: In WLED's `WS2812FX::service()` (wled00/FX_fcn.cpp), the old condition `|| (doShow && seg.mode == FX_MODE_STATIC)` was an **inclusion** guard — it caused FX_MODE_STATIC to render only when another segment had already set doShow=true. It did NOT skip or protect FX_MODE_STATIC from rendering. The PR `#5443` simplification removes this condition, meaning FX_MODE_STATIC now renders on every `timeToShow` tick uniformly. This is intentional and not a regression. Do not flag FX_MODE_STATIC special-casing as missing in future reviews of this function.
Learnt from: softhack007
Repo: wled/WLED PR: 5508
File: wled00/FX.cpp:4137-4142
Timestamp: 2026-04-16T09:50:07.774Z
Learning: WLED overlay behavior: Effects must operate via SEGMENT.* APIs (reads/writes) — global frame reads (strip.getPixelColor*) are reserved for copy_* style post‑processing. Blending between segments is performed later in WS2812FX::blendSegment(); effects should not try to composite with lower layers themselves.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-12-28T14:06:48.772Z
Learning: In WLED post-commit ee9ac947, the rendering pipeline uses per-segment buffers and per-pixel bus updates. Unmapped (0xFFFF) mapping entries are now skipped in WS2812FX::show() (no “clear to black”), which can leave physical gap LEDs with stale/random colors unless they are explicitly cleared. This is a behavior change from pre-0.16 where a full physical buffer was effectively refreshed each frame.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-04-10T11:41:46.737Z
Learning: In the WLED repository (wled/WLED), trailing-whitespace removals or other pure-whitespace "optimizations" must NOT be intermixed with real code/logic changes in the same PR. Reviewers prefer whitespace cleanup to be submitted in a dedicated, separate PR. Reason: intermixing makes diffs harder to read (obscures actual logic changes) and causes trouble when cherry-picking commits. Always flag PRs that mix whitespace-only edits with functional changes and ask the contributor to separate them.
Learnt from: DedeHai
Repo: wled/WLED PR: 4923
File: wled00/FX.cpp:4883-4901
Timestamp: 2025-09-12T17:29:43.826Z
Learning: In WLED’s web UI, only one slider value (e.g., SEGMENT.intensity or SEGMENT.custom1) changes at a time; code relying on this may use simplified change guards, though presets/JSON can still update multiple fields atomically.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-04-10T16:10:42.477Z
Learning: In WLED (wled/WLED), the segments vector (`strip.segments`) is highly volatile: preset loads, effect changes, and JSON state updates in json.cpp can add/remove/reorder segments and change their active status at any time. Segments have no stable persistent identifier — only their ordinal index in the vector, which also changes. Code that tries to maintain a discovery-time binding from an external protocol (e.g., Espalexa/Alexa) to a specific segment using ordinal-among-active-segments as a key will silently map to wrong segments after any preset load. A correct implementation must store the init-time segment-index-to-device mapping explicitly and guard against the segment at that index changing identity after a vector rebuild.
Learnt from: DedeHai
Repo: wled/WLED PR: 4889
File: wled00/json.cpp:310-310
Timestamp: 2026-03-21T18:12:09.437Z
Learning: In WLED's `deserializeSegment()` (wled00/json.cpp), the blend mode field `seg.blendMode` is intentionally written without a post-read clamp (`getVal(elem["bm"], seg.blendMode)`). Out-of-range or unsupported blend mode values are handled safely in `WS2812FX::blendSegment()` (wled00/FX_fcn.cpp), which defaults to mode 0 for any unsupported value via a bounds check against the `BLENDMODES` constant. Do not flag the missing clamp in deserializeSegment as a bug in future reviews.
This comment was marked as duplicate.
This comment was marked as duplicate.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
wled00/FX_fcn.cpp (1)
284-340:startTransition()rework addresses prior feedback — LGTM with one small nit.The
isInTransition()branch now consistently refreshes_t->_start,_t->_dur,_t->_bri, and_t->_prevPaletteBlends(both in the FADE+no-copy-yet path and in the generic “already transitioning” path), and the non-FADE+no-copy-yet case cleanly cancels and falls through to reallocate. This resolves the stale-clock / immediate-stopTransition issue that was flagged on the previous revisions.One minor observation: in the non-FADE+no-copy-yet branch (lines 292–295),
visibleBriis computed but discarded — afterstopTransition()the fall-through path at lines 322–325 sets_t->_bri = on ? opacity : 0. Combined with the newsetOpacity(..., false)behaviour, this means a mode/color change that interrupts an in-flight per-segment opacity fade in a non-FADE style will seed the fresh transition from the target opacity rather than the currently visible brightness, so the user may see a small brightness snap at the moment of interruption. Not a correctness bug; noting in case it was not intentional.Optional tweak to preserve visible brightness across the cancel-and-restart
if (segmentCopy && !_t->_oldSegment) { if (blendingStyle != TRANSITION_FADE) { // transition without a copy means we are in a segment brightness transition // need to cancel the transition to not interfere with the new one (or global brightness change) stopTransition(); + // fall through; seed the new transition from the currently visible brightness + _t = new(std::nothrow) Transition(dur); + if (_t) { + _t->_bri = visibleBri; + _t->_cct = cct; + _t->_palette = palette; + `#ifndef` WLED_SAVE_RAM + loadPalette(_t->_palT, palette); + `#endif` + for (int i=0; i<NUM_COLORS; i++) _t->_colors[i] = colors[i]; + if (segmentCopy) _t->_oldSegment = new(std::nothrow) Segment(*this); + if (_t->_oldSegment && !_t->_oldSegment->isActive()) stopTransition(); + } + return; } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX_fcn.cpp` around lines 284 - 340, The code in Segment::startTransition computes visibleBri but discards it when the non-FADE+no-copy-yet path calls stopTransition(), causing the new Transition to be seeded from on? opacity : 0 and producing a visible snap; preserve the computed visibleBri and use it to initialize the new transition's brightness. Concretely: when taking the branch that calls stopTransition(), save the visibleBri value and after allocating _t = new Transition(dur) (or before assigning _t->_bri) set _t->_bri = visibleBri instead of leaving the existing on ? opacity : 0 behavior (ensure this applies both to the allocate-path and any restart-paths that follow stopTransition()). Reference symbols: Segment::startTransition, visibleBri, stopTransition(), _t = new(std::nothrow) Transition(dur), and _t->_bri.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@wled00/FX_fcn.cpp`:
- Around line 284-340: The code in Segment::startTransition computes visibleBri
but discards it when the non-FADE+no-copy-yet path calls stopTransition(),
causing the new Transition to be seeded from on? opacity : 0 and producing a
visible snap; preserve the computed visibleBri and use it to initialize the new
transition's brightness. Concretely: when taking the branch that calls
stopTransition(), save the visibleBri value and after allocating _t = new
Transition(dur) (or before assigning _t->_bri) set _t->_bri = visibleBri instead
of leaving the existing on ? opacity : 0 behavior (ensure this applies both to
the allocate-path and any restart-paths that follow stopTransition()). Reference
symbols: Segment::startTransition, visibleBri, stopTransition(), _t =
new(std::nothrow) Transition(dur), and _t->_bri.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cd30466f-9e4c-4e12-8cb4-af32575631cb
📒 Files selected for processing (1)
wled00/FX_fcn.cpp
Collaborator
Author
|
performed all tests except the ones involving HA and MQTT and all behave correctly |
Contributor
|
If you want my opinion: This incorrectly solves OP issue. It also changes behaviour of non-segment-copy transitions. |
Collaborator
Author
|
yea, I am not too convinced this is the proper let alone best solution. If you find something better, please keep me posted. |
DedeHai
marked this pull request as draft
April 23, 2026 12:10
Contributor
|
I think I've solved the issue (all of them). Watch my fork for changes after I test them. |
1 task
Collaborator
Author
|
@coderabbitai in |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Locate and outline button.cpp ==='
button_file="$(fd -t f '^button\.cpp$' | head -n1)"
printf 'button_file=%s\n' "$button_file"
ast-grep outline "$button_file" --items all --view expanded || true
echo '=== Guard and lastOnTime context ==='
rg -n -C 12 'lastOnTime|millis\s*\(\)\s*-\s*lastOnTime|>\s*600' "$button_file"
echo '=== Definitions and calls across source ==='
rg -n -C 4 '\blastOnTime\b|button.*off|off.*button' --glob '*.{cpp,h,ino}' .
echo '=== History for this guard ==='
git log --all --oneline -S'lastOnTime' -- "$button_file" | head -30
git log --all --oneline -S'600' -- "$button_file" | head -30
git blame -L "$(rg -n 'millis\s*\(\)\s*-\s*lastOnTime\s*>\s*600' "$button_file" | head -n1 | cut -d: -f1),+1" "$button_file" || true
echo '=== GitHub issue and PR searches ==='
gh search issues --repo wled/WLED --state all --limit 50 'lastOnTime'
gh search issues --repo wled/WLED --state all --limit 50 '"button" "600"'
gh search issues --repo wled/WLED --state all --limit 50 '"button" "off" delay'Length of output: 11480 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Introducing commit and previous implementation ==='
git show --no-ext-diff --format=fuller --stat 1ca55e42a
git show --no-ext-diff --format=fuller -U25 1ca55e42a -- wled00/button.cpp
echo '--- parent handleOnOff ---'
git show 1ca55e42a^:wled00/button.cpp | nl -ba | sed -n '350,405p'
echo '=== Pending-update semantics ==='
rg -n -C 8 '\bneedsUpdate\s*\(' --glob '*.{cpp,h}' wled00
echo '=== GitHub issues referenced by nearby relay code ==='
for n in 346 812 3581 3955; do
echo "--- #$n ---"
gh issue view "$n" --repo wled/WLED --json number,title,state,url,author,createdAt,closedAt,body,comments \
--jq '{number,title,state,url,author:.author.login,createdAt,closedAt,body,comments:[.comments[]|{author:.author.login,body,url}]}'
done
echo '=== GitHub search: exact identifiers and delay reports ==='
for query in \
'repo:wled/WLED lastOnTime' \
'repo:wled/WLED "600" button' \
'repo:wled/WLED button relay "off" delay' \
'repo:wled/WLED relay turns off too early' \
'repo:wled/WLED LEDs do not turn off relay'
do
echo "--- $query ---"
gh api -X GET search/issues -f q="$query" -f per_page=50 \
--jq '.items[] | {number,title,state,pull_request:(has("pull_request")),url,created_at,closed_at}'
doneLength of output: 5464 🏁 Script executed: #!/bin/bash
set -u
echo '=== Commit that introduced the exact condition ==='
git log --all --format='%H %s' -S'millis() - lastOnTime > 600' -- wled00/button.cpp | head -20 || true
git log --all --format='%H %s' -S'lastOnTime > 600' -- wled00/button.cpp | head -20 || true
introducer="$(git log --all --format='%H' -S'millis() - lastOnTime > 600' -- wled00/button.cpp | tail -n1)"
if [ -n "$introducer" ]; then
echo "=== Introducer: $introducer ==="
git show --no-ext-diff --format=fuller -U30 "$introducer" -- wled00/button.cpp || true
fi
echo '=== Current and pre-#5315 shutdown logic ==='
sed -n '355,405p' wled00/button.cpp || true
git show 1ca55e42a^:wled00/button.cpp | sed -n '345,395p' || true
echo '=== needsUpdate declarations, implementation, and loop order ==='
rg -n -C 10 '\bneedsUpdate\s*\(' --glob '*.{cpp,h}' wled00 || true
rg -n -C 10 '\bhandleIO\s*\(|\bhandleTransitions\s*\(|\.service\s*\(' --glob '*.{cpp,h}' wled00/wled.cpp wled00/FX_fcn.cpp || true
echo '=== `#5315` and relay stabilization issue reports ==='
for n in 5315 346 812 3581 3955; do
echo "--- #$n ---"
gh issue view "$n" --repo wled/WLED --comments || true
done
echo '=== Search reported issues and PRs ==='
for query in \
'repo:wled/WLED lastOnTime' \
'repo:wled/WLED "600" button' \
'repo:wled/WLED button relay "off" delay' \
'repo:wled/WLED relay turns off too early' \
'repo:wled/WLED LEDs do not turn off relay'
do
echo "--- $query ---"
gh api -X GET search/issues -f q="$query" -f per_page=50 \
--jq '.items[] | [.number,.state,.title,.html_url] | `@tsv`' || true
doneLength of output: 59259 🏁 Script executed: #!/bin/bash
set -u
echo '=== Follow the file before its 2020 rename ==='
git log --follow --format='%H%x09%ad%x09%s' --date=short -- wled00/button.cpp | tail -80 || true
git log --all --format='%H%x09%ad%x09%s' --date=short -S'lastOnTime' -- . ':!wled00/button.cpp' | head -80 || true
git grep -n -e 'lastOnTime' "$(git rev-list --all | tail -n1)" 2>/dev/null || true
echo '=== Find earliest commit containing the 600 ms off guard across all paths ==='
for commit in $(git rev-list --all --reverse); do
if git grep -q 'lastOnTime > 600' "$commit" -- 2>/dev/null; then
printf 'first_commit=%s\n' "$commit"
git show --no-ext-diff --format=fuller -U25 "$commit" -- 2>/dev/null | head -220
break
fi
done
echo '=== Directly relevant reported issues ==='
for n in 369 5619; do
echo "--- #$n ---"
gh issue view "$n" --repo wled/WLED --comments || true
done
echo '=== PR `#5315` discussion and changed files ==='
gh pr view 5315 --repo wled/WLED --comments || true
gh pr view 5315 --repo wled/WLED --json url,title,body,commits,files \
--jq '{url,title,body,commits:[.commits[]|{oid,messageHeadline,messageBody}],files:[.files[]|{path,additions,deletions}]}' || trueLength of output: 1503 🏁 Script executed: #!/bin/bash
set -u
echo '=== Relevant issue reports ==='
for n in 369 5619; do
echo "--- #$n metadata ---"
gh api "repos/wled/WLED/issues/$n" \
--jq '{number,title,state,html_url,created_at,closed_at,body}' || true
echo "--- #$n comments ---"
gh api "repos/wled/WLED/issues/$n/comments?per_page=100" \
--jq '.[] | {user:.user.login,created_at,html_url,body}' || true
done
echo '=== PR `#5315` metadata and discussion ==='
gh api 'repos/wled/WLED/pulls/5315' \
--jq '{number,title,state,merged_at,html_url,body}' || true
gh api 'repos/wled/WLED/issues/5315/comments?per_page=100' \
--jq '.[] | {user:.user.login,created_at,html_url,body}' || true
gh api 'repos/wled/WLED/pulls/5315/comments?per_page=100' \
--jq '.[] | {user:.user.login,path,position,created_at,html_url,body}' || true
echo '=== Narrow pre-rename history ==='
git log --all --oneline --follow -- wled00/button.cpp 2>/dev/null | tail -25 || true
for path in wled00/wled00.ino wled00.ino wled00/wled.ino; do
echo "--- $path ---"
git log --all --oneline -S'lastOnTime' -- "$path" | head -15 || true
git log --all --oneline -S'lastOnTime > 600' -- "$path" | head -15 || true
doneLength of output: 200362
In } else if ((millis() - lastOnTime > 600 && !strip.needsUpdate()) || forceOff) {WLED executes the normal power-off path only when all these conditions are true:
The 600 ms delay gives the output path a grace interval. The HistoryThe 600 ms condition is old. It already existed with the original relay support before the 2020 source-file rename. PR The PR Related reported issues
The exact reason for selecting 600 ms, instead of another duration, is not documented in the inspected historical commit or discussions. The intent is clear, but the number appears to be a legacy empirical constant. You are interacting with an AI system. |
Member
Not sure this is the primary purpose. The timer check is implemented in button.cpp, and it appears to be very old code.
|
Member
@coderabbitai could this line cause problems when the last "on" command has arrived less than 600ms ago, for example from HyperHDR? Is it possible that the power-off is silently lost? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
button_file="$(fd -t f '^button\.cpp$' | head -n1)"
printf '=== button file: %s ===\n' "$button_file"
ast-grep outline "$button_file" --items all --view expanded
printf '\n=== shutdown state context ===\n'
rg -n -C 18 '\blastOnTime\b|\boffMode\b|\bhandleOnOff\s*\(|\bneedsUpdate\s*\(' "$button_file"
printf '\n=== definitions and call order ===\n'
rg -n -C 8 '\bhandleOnOff\s*\(|\bhandleIO\s*\(|\bneedsUpdate\s*\(' --glob '*.{cpp,h}' wled00
printf '\n=== realtime / HyperHDR input and shutdown interaction ===\n'
rg -n -i -C 8 'hyperhdr|realtime|realtimeMode|realtimeTimeout|exit.*realtime|disable.*realtime|bri\s*=\s*0|setBrightness' --glob '*.{cpp,h}' wled00
printf '\n=== related GitHub reports ===\n'
for query in \
'repo:wled/WLED HyperHDR relay off' \
'repo:wled/WLED HyperHDR power off' \
'repo:wled/WLED HyperHDR lastOnTime' \
'repo:wled/WLED realtime relay turns off'
do
echo "--- $query ---"
gh api -X GET search/issues -f q="$query" -f per_page=30 \
--jq '.items[] | [.number,.state,.title,.html_url] | `@tsv`' || true
doneLength of output: 111006 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Main-loop order ==='
sed -n '45,145p' wled00/wled.cpp
echo '=== Realtime timeout and notification call sites ==='
rg -n -C 10 '\bhandleNotifications\s*\(|\bexitRealtime\s*\(|\brealtimeTimeout\b' --glob '*.{cpp,h}' wled00
echo '=== Trigger lifecycle in service ==='
rg -n -C 10 '_triggered\s*=' wled00/FX_fcn.cpp wled00/FX.h
echo '=== Issue `#5619` ==='
gh api 'repos/wled/WLED/issues/5619' \
--jq '{number,title,state,html_url,created_at,body}'
gh api 'repos/wled/WLED/issues/5619/comments?per_page=100' \
--jq '.[] | {user:.user.login,created_at,html_url,body}'Length of output: 199708
if (strip.getBrightness() && !forceOff) {
lastOnTime = millis();
}Therefore, if HyperHDR has caused For a normal HyperHDR stop sequence:
The off request becomes effectively delayed indefinitely only if later input keeps the output active. Examples are:
In these cases, the logical off state can already be set while the physical relay remains on. The relevant distinction is:
So, the 600 ms condition is a debounce/grace delay. It does not clear or overwrite the off state. It can make the relay appear not to switch off when external realtime activity continues or restarts during that interval. You are interacting with an AI system. |
Collaborator
Author
|
@softhack007 my hunch is the 600ms delay was added to caputre a "long press" from a button/switch as that takes 600ms to register. If that is actually the case, the delay could be skipped if no buttons are currently pressed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix for #5520
Summary by CodeRabbit