service cleanup after removing return FRAMETIME#5443
service cleanup after removing return FRAMETIME#5443softhack007 wants to merge 14 commits intomainfrom
Conversation
* Activation "_frametime" is now the same for all effects, so conditionally running effects based on their own frametime is obsolete * Special handling for mode_static is also obsolete
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughCompute a single Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.
Actionable comments posted: 1
🤖 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 1271-1272: Replace the addition-based strict comparison in the
timeToShow assignment with a subtraction-based elapsed-time check consistent
with the pattern used on line 1275: compute elapsed = nowUp - _lastServiceShow
and set timeToShow = (elapsed >= _frametime) || _triggered (ensuring the
equality case triggers a frame); also correct the comment text "all segment are"
to "all segments are". Use the existing variables nowUp, _lastServiceShow,
_frametime, _triggered and keep the same behavior for the trigger override.
as suggested by the rabbit
preserve previous behaviour: strip.now must also be updsted in case of "early return"
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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`:
- Line 1277: The current loop sets doShow and unconditionally calls
_mode[seg.mode]() for every active segment (seg), causing static/frozen segments
to recompute and retransmit identical frames; change the loop to first determine
per-segment whether it truly needs rendering by adding a fast-path flag (e.g.,
needsRender) that is computed from segment state (frozen/static,
lastRenderedFrame/timestamp, or a dirty bit) and only call _mode[seg.mode]()
when needsRender is true, while keeping doShow/needsShow separate so segments
that require only a show() refresh (but no recompute) are handled without
invoking the effect callback; update any uses around doShow and the show() call
accordingly so unchanged segments skip expensive work.
invert the logic - instead of "if (!isActive) continue", make the logic explicit by embedding the rest of the loop body into "if (isActive)". Moved the final "_segment_index++;" into the conditional body, to preserve previous behavior. Warning: the previous behavior might be buggy.
previously "inactive" segment were skipped, leading to SegmentID becoming misaligned with the actual list of segments.
compiler for 8266 complained about "comparing singed to unsigned"
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@DedeHai @willmmiles @netmindz I'm happy now with the cleaned-up result, but not sure if this should go into 16.0 or into 16.1. The Maybe better to keep the code as-is in 16.0 (bugfixes already backported to main) and wait until 16.1 with the full cleanup 🤔 or maybe by "quality manager mindset" is too cautious? What do you think? |
softhack007
added
Awaiting testing
optimization
fixes a corner case: if the segments list only has inactive segments, we should still run strip.show() if requested by strip.trigger().
|
IMHO 16.0 will be a leap forward from 0.15, better pack whatever changes we have into the beta and get it tested instead of waiting and breaking things again in 16.1 |
|
@coderabbitai please do a thorough check if there are any bugs or missing special case handling in service(). Also compile a list of tests to be performed on real hardware to check if all cases are handled properly. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me read the current state of the 🏁 Script executed: Length of output: 23329 Good, I have the full function. Let me also check some key definitions and the git diff to compare old vs new behavior. 🏁 Script executed: Length of output: 7475 Let me check a couple more things — the 🏁 Script executed: Length of output: 8434
Code Analysis:
|
| Old | New |
|---|---|
bool doShow = false |
bool doShow = _triggered |
_segment_index = 0 before loop; _segment_index++ only for active segs |
_segment_index = i for every segment; reset to 0 after loop |
Range-based for (Segment &seg : _segments) |
Index-based for (size_t i = 0; ...) |
if (!seg.isActive()) continue; skipped inactive |
if (seg.isActive()) { ... } wraps active work |
handleTransition() and resetIfRequired() were already outside the isActive() guard in the old code — this is not a new behaviour change.
🐛 Findings
1. doShow = _triggered — Correct, intentional, no bug
With _triggered = true and all segments inactive, show() is now called even with no active segments processed. Per the learning already stored, this is architecturally correct: frozen segments' externally-written pixel buffers must reach the hardware via show(), and strip.trigger() is the flushing mechanism for those.
2. _segment_index bug fix confirmed ✅
In the old code, _segment_index was incremented only for active segments (it lived below the continue in the !isActive() branch). So with segment 0 inactive and segment 1 active, getCurrSegmentId() inside effects for segment 1 would wrongly return 0. The new code fixes this by assigning _segment_index = i at the top of every iteration.
3. FX_MODE_STATIC explicit trigger() in json.cpp — Still present, still valid
json.cpp:254 retains:
if (seg.mode == FX_MODE_STATIC) strip.trigger(); //instant refreshSince service() now renders static segments uniformly at the standard frametime, this trigger() is still useful — it provides an instant response to color changes via the JSON API without waiting for the next frame. Not a regression.
4. MIN_FRAME_DELAY blocks trigger() and FPS_UNLIMITED — Intentional, pre-existing
MIN_FRAME_DELAY is platform-dependent:
- ESP32: 2 ms → max effective trigger rate ≈ 500 Hz
- S2/C3: 3 ms → max ≈ 333 Hz
- ESP8266: 8 ms → max ≈ 125 Hz
The guard at line 1275 (elapsed <= MIN_FRAME_DELAY) applies even when _triggered=true or _targetFps==FPS_UNLIMITED. This is intentional ("keep wifi alive — no matter if triggered or unlimited"). However, a trigger() call is not lost when blocked — _triggered is only cleared at line 1332, so the show fires on the very next eligible service call.
5. Suspend mid-loop handling — Correct ✅
If _suspend is set mid-loop:
breakexits the loop (line 1282)if (doShow && !_suspend)preventsshow()(line 1322)if (!_suspend) _triggered = falsepreserves_triggered(line 1332)_lastServiceShowis not updated → on resume,elapsedwill be large → first post-resume service immediately fires
This means effects that use strip.now for timing may experience a time jump after a long suspend. This is inherent to the design, not introduced by this PR.
6. Minor: Dead comment on line 1291
//if (timeToShow) // removed - if we arrive here, its always showtimeThis is a leftover from an earlier refactor stage. It accurately explains why the condition was removed but could be cleaned up.
7. _segment_index type vs. loop counter type — Safe but worth noting
_segment_index is uint8_t (FX.h:1047), while the loop uses size_t i. The assignment _segment_index = i would truncate if _segments.size() > 255, but MAX_NUM_SEGMENTS ≤ 255 so this is safe in practice.
8. _currentSegment not reset to nullptr after the loop — Pre-existing
After service(), _currentSegment points to the last segO (old segment during transition) or the last active segment. This is pre-existing and not a regression, but if SEGMENT/SEGENV macros were ever accessed outside of isServicing(), it would be a stale pointer.
🧪 Hardware Test List
Timing & FPS
- Default FPS — Normal multi-segment operation; verify frame rate matches configured target (e.g. 42 FPS)
- FPS_UNLIMITED mode — Set target FPS to unlimited; verify no crashes, measure actual achieved FPS vs.
MIN_FRAME_DELAYfloor - Very low FPS (1 FPS) — Confirm effects still update at 1 FPS without stutter
- FPS change at runtime — Change FPS while effects are running; verify no glitch or freeze
strip.trigger() paths
- JSON color change on FX_MODE_STATIC — Change color via REST API; verify LED updates immediately (via
json.cpp:254trigger path) - JSON segment update (non-static effect) — Change effect parameters via JSON; verify
strip.trigger()atjson.cpp:358causes immediate refresh - Brightness change via
led.cpp— Adjust master brightness; verifystrip.trigger()fires and LEDs update promptly - Rapid back-to-back
trigger()calls — Call trigger faster thanMIN_FRAME_DELAY; confirm no shows are dropped (last trigger must always result in a show), and WiFi stays alive
Frozen segments
- Frozen segment +
strip.trigger()— Freeze a segment, write pixels via single-pixel JSON API, call trigger; confirm LEDs update (validatesdoShow = _triggeredpath with frozen/inactive segment) - Frozen segment + NO trigger — Freeze a segment; confirm pixel content persists and does not flicker or go dark
Suspend / Resume
- Suspend during active effects —
strip.suspend()while effects are running; verify LEDs hold last frame and no corruption - Resume after suspend — Resume after a multi-second suspend; verify effects restart cleanly without flash/jump (tests
_lastServiceShownot being updated during suspend) - Rapid suspend/resume cycles — Toggle suspend rapidly; verify no
_isServicingdeadlock (the 2×frametime+100ms wait insuspend()should never timeout under normal load)
Segment index correctness (_segment_index bug fix)
- Mixed active/inactive segments — Create e.g. 4 segments, deactivate segment 0 and segment 2, run effects on 1 and 3; verify
getCurrSegmentId()returns correct index (1 and 3) inside those effects — this is the key regression test for the_segment_indexfix - All segments inactive — Deactivate all segments; confirm no crash and
show()is only called when triggered
Realtime / UDP
- E1.31 / DDP / TPM2.NET — Send realtime data via UDP;
udp.cppusesstrip.trigger()whenuseMainSegmentOnly=true— verify LEDs update correctly and not stale - Realtime +
useMainSegmentOnly=false— Thestrip.show()direct call path inudp.cpp:452; verify this still bypasses service() timing correctly
Transitions
- Effect transition (fade) — Switch effects mid-run; verify blend transition completes cleanly with old/new segment running in parallel
- On/Off segment transitions — Toggle segment
onflag with various blend styles; verify no flash or incorrect blending - Color/CCT transitions — Apply color and CCT changes while effect is running; verify smooth transition
Platform-specific
- ESP8266 — Full regression test;
MIN_FRAME_DELAY=8msmeans trigger-response latency is more noticeable — test with fast JSON updates - ESP32 multi-core — While less of an issue (WLED runs service on one core), stress-test with Wi-Fi heavy load to verify
MIN_FRAME_DELAYguard keeps networking alive
Edge cases
- Single segment, single pixel — Verify no off-by-one in segment geometry with degenerate 1-pixel segment
- Max number of segments — Configure maximum allowed segments simultaneously; verify
_segment_index(uint8_t) doesn't truncate and all effects render handleRandomPalette()with no active segments + trigger — Verify it's called (line 1324 insidedoShowblock) and does not crash with zero active segments
🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 5443
File: wled00/FX_fcn.cpp:1277-1277
Timestamp: 2026-03-24T12:10:29.964Z
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: 5443
File: wled00/FX_fcn.cpp:1277-1277
Timestamp: 2026-03-24T12:13:17.908Z
Learning: In WLED's `WS2812FX::service()` (wled00/FX_fcn.cpp), `seg.freeze` means "do not run the effect function (_mode[seg.mode]())" — it does NOT mean "skip show()". A frozen segment's pixel buffer can still be updated externally (e.g., realtime control or single-pixel JSON API). `strip.trigger()` is the primary mechanism to flush those externally written pixels to the LEDs on the next service tick. Therefore, frozen segments must remain part of the `doShow`/`show()` path, and it is architecturally wrong to exclude frozen segments from `doShow`. Do not suggest skipping frozen segments from the show path in future reviews.
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: 0
File: :0-0
Timestamp: 2026-03-15T13:21:32.008Z
Learning: In WLED code reviews (and generally), when a PR discussion involves "is this worth doing?" uncertainty about a proposed reliability, safety, or data-integrity mechanism (e.g. CRC checks, backups, power-loss protection), suggest performing a software FMEA (Failure Mode and Effects Analysis). The FMEA should: enumerate failure modes for the feared event, assess each mitigation's effectiveness per failure mode, note common-cause/common-mode failures, and rate credibility for the typical WLED use case. This was successfully used in PR `#5421` to evaluate CRC-based config file protection and was well-received by the WLED maintainers (softhack007 explicitly requested this be remembered for future reviews).
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5293
File: usermods/user_fx/user_fx.cpp:167-171
Timestamp: 2026-01-13T03:07:20.560Z
Learning: In WLED effects (usermods/user_fx/user_fx.cpp and similar), when storing future timestamps, assignments like `stored_time = now + delay` are correct and handle uint32_t wraparound properly. Only comparison statements need to use subtraction-based elapsed-time checks (e.g., `(now - stored_time) >= delay`). Do not flag assignment statements as needing fixes for wraparound handling.
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: BobLoeffler68
Repo: wled/WLED PR: 5440
File: usermods/user_fx/user_fx.cpp:1307-1327
Timestamp: 2026-03-24T07:59:25.586Z
Learning: In WLED's Dissolve Plus effect (usermods/user_fx/user_fx.cpp, mode_dissolveplus), when the "Last one" checkbox (check3/lastOneMode) is turned off while the effect is in PHASE_PAUSE_SURVIVOR, the phase is intentionally NOT immediately reset. The pause expires naturally via the normal delay timing (a few hundred ticks), after which the filling phase begins. PHASE_DISSOLVE_SURVIVOR is reset to PHASE_DISSOLVE and PHASE_FILL_SURVIVOR is reset to PHASE_FILL on toggle-off, but PHASE_PAUSE_SURVIVOR is left to expire on its own. Do not flag this as a bug in future reviews.
|
I ran most of the tests suggested by the rabbit, except:
I have found one bug, but that is probably not related to this PR: other than that, all tests passed, including live streaming (used the video tool over WS at 2FPS starget -> still runs at 50FPS). |
|
Thanks for the review and tests 🥇
Looks like both FPS jumps are due to brightness handling, and especially it comes from "responsiveness improvement" hacks from the times when the SOLID effect was running at 3fps fixed, causing brightness/color changes to stutter Lines 1755 to 1758 in d8cb20a MIN_SHOW_DELAY is 3ms, so effectively the strip would jump into "unlimited FPS" mode whenever brightness changes. --> solution: Lines 76 to 82 in d8cb20a --> solution: this is ancient legacy code, don't touch 😉 @coderabbitai I intend to implement the first solution - please check if this would cause new side-effects on other features like transitions, blending, cross-fading, and palette transitions. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 13717 min(FRAMETIME, uint16_t(FRAMETIME_FIXED))" The issue is: So the proposed fix increases the threshold from MIN_SHOW_DELAY (3-15ms) to min(FRAMETIME, FRAMETIME_FIXED). This means:
Wait, that can't be right. Let me re-read more carefully. Oh! I think I misread MIN_SHOW_DELAY. Let me look again: So: if _frametime < 16ms (FPS > ~62), MIN_SHOW_DELAY = 8ms. Otherwise (FPS ≤ 62), MIN_SHOW_DELAY = 15ms. This seems reasonable. At 1 FPS (_frametime = 1000ms), MIN_SHOW_DELAY = 15ms. If last show was > 15ms ago, trigger(). At 1 FPS, the last show is always > 15ms ago (it's 1000ms ago), so trigger always fires on brightness change. This immediately causes one "fast" frame. With the fix
Hmm, this still seems like it would trigger at 1 FPS. Unless the intent is to NOT use min() but rather to use the current _frametime directly? Wait, re-reading the original: "MIN_SHOW_DELAY --> min(FRAMETIME, uint16_t(FRAMETIME_FIXED))" Maybe the fix is not about the trigger threshold, but about something else. Let me look at the actual trigger path again.
if (_triggered || _targetFps == FPS_UNLIMITED) timeToShow = true;So with trigger, the next service() call will run effects and show regardless of frametime. This is the "speedup" — at 1 FPS, the next service() call is in ~2ms (MIN_FRAME_DELAY), which runs effects prematurely. The fix to
So the threshold is always capped at FRAMETIME_FIXED ≈ 23ms. At 1 FPS, the last show was 1000ms ago, so Wait... Maybe I'm misunderstanding the direction of the fix. Let me look at this from the opposite angle. At very low FPS (1-2 FPS), the "speedup" problem is: the brightness change fires trigger(), which forces an immediate frame. With _frametime=1000ms and trigger, the next service() runs effects. This causes an "extra" frame at the wrong time. Maybe the idea is different: instead of increasing the threshold so trigger() is NOT called, the fix is to make the threshold be FRAMETIME (current frametime) so trigger() is only called if we're "close enough to the next frame anyway" (i.e., last show was nearly a full frametime ago). If threshold = min(_frametime, FRAMETIME_FIXED):
Unless the fix is meant to use
Hmm, I'm confused. Let me re-read softhack007's proposal more carefully: "solution: MIN_SHOW_DELAY --> min(FRAMETIME, uint16_t(FRAMETIME_FIXED)) will still improve responsiveness, without making all effects run wild" The current MIN_SHOW_DELAY = 3ms effectively (since it was designed for old code where _frametime was 33ms for static effects). "min(FRAMETIME, FRAMETIME_FIXED)" at standard 42 FPS = min(23, 23) = 23ms. The old min show delay for standard FPS (> 62 FPS): 8ms. For ≤ 62 FPS: 15ms. Oh wait! Look at this again: At UNLIMITED mode: _frametime = MIN_FRAME_DELAY = 2ms. So MIN_SHOW_DELAY = 8ms (since 2 < 16). The problem: MIN_SHOW_DELAY says "trigger if no show for >8ms (or 15ms)" but in reality at unlimited mode, a show happens every ~2ms, so Hmm, so for 1-2 FPS effects, every brightness change causes an immediate frame. With min(FRAMETIME, FRAMETIME_FIXED) = min(1000ms, 23ms) = 23ms as the new threshold... that's still less than the 1000ms between frames, so trigger still fires. Wait. I think I need to look at this from a completely different angle. Maybe "min(FRAMETIME, FRAMETIME_FIXED)" means "don't trigger if the next regular show is within FRAMETIME_FIXED ms". In other words, the condition becomes: No, min(FRAMETIME, FRAMETIME_FIXED) means: the threshold is the smaller of the two. Hmm. Actually wait - if FRAMETIME at 1 FPS = 1000ms and FRAMETIME_FIXED = 23ms, then min = 23ms. At 1 FPS, last show was 500ms ago (on average) → 500ms > 23ms → trigger fires. Still broken. I think maybe the intent is to NOT call trigger() at all from setBrightness(), and instead just let the next frame pick up the brightness change naturally. But that's too responsive-breaking. OR - maybe the fix is to change what trigger() does: instead of forcing timeToShow=true immediately, it should set a "trigger within the next FRAMETIME_FIXED ms" flag. But that's a much bigger change. Let me just take softhack007's proposal at face value and analyze the potential side effects of that specific change (replacing MIN_SHOW_DELAY with min(FRAMETIME, uint16_t(FRAMETIME_FIXED)) in line 1754). I should gather the relevant code and provide the analysis, noting that for very low FPS configurations, the behavior improvement may be limited. Actually, I think I may be overthinking this. Let me read the MIN_SHOW_DELAY definition once more:
This checks if _frametime < 16ms. At the time
Now DedeHai tested at 1-2 FPS and observed speedup. At 1 FPS with MIN_SHOW_DELAY=15ms and last show typically 1000ms ago: trigger always fires → speed up happens. This is the confirmed issue. With fix
Hmm, that doesn't fix it either! Unless the intent is to look at how close we are to the NEXT show, not how long since last show? i.e. "how long until next show = _frametime - (t - _lastShow)". If that's less than FRAMETIME_FIXED, don't trigger. But the code doesn't compute it that way. OK let me look at this from a fresh perspective. Actually, maybe the solution as described only addresses the case where MIN_SHOW_DELAY is very small (like 8ms at high FPS) and not where it's 15ms but the frame is 1000ms long. The problem DedeHai saw at 1-2 FPS may be a distinct/related issue. The fix as proposed prevents the "effects run wild" on unlimited FPS or high FPS, not necessarily 1-2 FPS. I need to look at the actual code paths more carefully. Let me just gather more context and provide the analysis. 🏁 Script executed: Length of output: 11872
Analysis: Side Effects of the
|
| Configured FPS | _frametime |
Old threshold (MIN_SHOW_DELAY) |
New threshold (min(FRAMETIME, FRAMETIME_FIXED)) |
Effect |
|---|---|---|---|---|
| 42 (default) | 23 ms | 15 ms | min(23, 23) = 23 ms |
✅ No more premature trigger (~8 ms early) |
| 60 | 16 ms | 15 ms | min(16, 23) = 16 ms |
✅ Marginal improvement |
| 120 | 8 ms | 8 ms (_frametime < 16) |
min(8, 23) = 8 ms |
↔ No change |
| Unlimited | 2–8 ms | 8 ms | min(2–8, 23) = 2–8 ms |
↔ No change |
| 2 | 500 ms | 15 ms | min(500, 23) = 23 ms |
|
| 1 | 1000 ms | 15 ms | min(1000, 23) = 23 ms |
The fix correctly prevents "premature" triggering at standard FPS by aligning the threshold with the actual frametime. For 1–2 FPS, t - _lastShow is always >> FRAMETIME_FIXED, so trigger still fires on brightness change — that edge case remains a known limitation (as you noted, the led.cpp path is "don't touch" territory).
Impact on transitions, blending, cross-fading, palette transitions
1. Brightness fade transitions (applyBri() → setBrightness(briT) path)
- This IS in the affected path. During a slow
briTfade,handleTime()callsapplyBri()→setBrightness()repeatedly. - With the fix at 42 FPS: brightness renders at exactly 42 FPS rather than triggering premature frames.
BusManager::setBrightness()applies the hardware register immediately regardless;show()pushes it to LEDs at the scheduled cadence. No regression, cleaner cadence.
2. applyFinalBri() (transition duration = 0)
- Calls
strip.trigger()directly, bypassingsetBrightness()'s trigger path entirely. Completely unaffected.
3. Segment effect transitions (fade, fairy dust, etc.)
handleTransition()/updateTransitionProgress()/progress()all usemillis()directly — fully time-based, not frame-count dependent.- An extra or premature show() frame just samples the blend at a slightly different time ratio. Removing premature frames makes this more accurate, not less.
4. On/Off power transitions (with non-FADE blend styles)
- Same time-based
progress()mechanism. Bug DedeHai found (fairy dust broken after rapid on/off during transition) is in the transition state machine — unrelated to this change.
5. Cross-fading / blendingStyle transitions
beginDraw(prog)uses the time-basedprog = seg.progress(). No regression.
6. Palette transitions (handleRandomPalette())
- Called inside
if (doShow) { ... show(); }— runs once per rendered frame, driven by the normal FPS cadence. No regression.
One implementation note
Inside WS2812FX::setBrightness(), the FRAMETIME macro expands to strip.getFrameTime() which is just _frametime. It's cleaner to use _frametime directly inside the class to avoid the macro resolving through the global strip object:
// proposed change in setBrightness():
if (t - _lastShow > std::min(_frametime, (uint16_t)FRAMETIME_FIXED)) trigger();Or using the C macro style consistent with the existing codebase (no std::):
if (t - _lastShow > min(_frametime, (uint16_t)FRAMETIME_FIXED)) trigger();Verdict
Safe to implement. No regressions expected in transitions, blending, cross-fading, or palette transitions. All those subsystems are time-based (not frame-count-based) and are either unaffected or benefit from the more disciplined trigger cadence. The remaining 1–2 FPS edge case is a pre-existing separate issue.
🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 5443
File: wled00/FX_fcn.cpp:1277-1277
Timestamp: 2026-03-24T12:10:29.964Z
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: 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:13:17.908Z
Learning: In WLED's `WS2812FX::service()` (wled00/FX_fcn.cpp), `seg.freeze` means "do not run the effect function (_mode[seg.mode]())" — it does NOT mean "skip show()". A frozen segment's pixel buffer can still be updated externally (e.g., realtime control or single-pixel JSON API). `strip.trigger()` is the primary mechanism to flush those externally written pixels to the LEDs on the next service tick. Therefore, frozen segments must remain part of the `doShow`/`show()` path, and it is architecturally wrong to exclude frozen segments from `doShow`. Do not suggest skipping frozen segments from the show path in future reviews.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5440
File: usermods/user_fx/user_fx.cpp:1307-1327
Timestamp: 2026-03-24T07:59:25.586Z
Learning: In WLED's Dissolve Plus effect (usermods/user_fx/user_fx.cpp, mode_dissolveplus), when the "Last one" checkbox (check3/lastOneMode) is turned off while the effect is in PHASE_PAUSE_SURVIVOR, the phase is intentionally NOT immediately reset. The pause expires naturally via the normal delay timing (a few hundred ticks), after which the filling phase begins. PHASE_DISSOLVE_SURVIVOR is reset to PHASE_DISSOLVE and PHASE_FILL_SURVIVOR is reset to PHASE_FILL on toggle-off, but PHASE_PAUSE_SURVIVOR is left to expire on its own. Do not flag this as a bug in future reviews.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5440
File: usermods/user_fx/user_fx.cpp:1304-1313
Timestamp: 2026-03-25T07:03:31.345Z
Learning: In WLED `mode_dissolveplus` (usermods/user_fx/user_fx.cpp), using `hw_random16(SEGLEN)` to select the survivor pixel index is correct and safe for this 1D-only effect. The 0xFFFF unmapped-entry concern from the physical bus mapping does not apply to 1D segments because virtual indices 0..SEGLEN-1 always map to valid physical LEDs without gaps. Do not flag this as a bug in future reviews of 1D effects.
Learnt from: DedeHai
Repo: wled/WLED PR: 5434
File: wled00/colors.cpp:79-83
Timestamp: 2026-03-21T11:43:36.767Z
Learning: In WLED's `color_fade()` (wled00/colors.cpp), the `video=true` path computes `maxc = (maxc>>2) + 1` and only preserves a channel if its value strictly exceeds that threshold (`r > maxc`, etc.). This means very dark non-zero channels (e.g., r==1 or r==2) can be dropped to zero in video mode. This is intentional: it prevents flickering in corner regions of gradients and is an accepted design compromise. Do not flag this as a bug in future reviews.
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-03-08T00:57:36.134Z
Learning: In WLED (wled00/cfg.cpp), `deserializeConfig()` is called with an empty JsonObject when cfg.json doesn't exist on fresh install. Any value read without the ArduinoJSON `|` fallback operator or CJSON macro will override correct constructor defaults with 0/null. Known affected values: `strip.setTargetFps(hw_led["fps"])` at line 179 (sets FPS to 0/unlimited instead of WLED_FPS=42). Fix: `strip.setTargetFps(hw_led["fps"] | WLED_FPS)`. The gamma issue (gammaCorrectCol/Bri) had the same root cause and was fixed in commit d1d9dec402 (Jan 2026) using inline `| default` fallbacks.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5293
File: usermods/user_fx/user_fx.cpp:167-171
Timestamp: 2026-01-13T03:07:20.560Z
Learning: In WLED effects (usermods/user_fx/user_fx.cpp and similar), when storing future timestamps, assignments like `stored_time = now + delay` are correct and handle uint32_t wraparound properly. Only comparison statements need to use subtraction-based elapsed-time checks (e.g., `(now - stored_time) >= delay`). Do not flag assignment statements as needing fixes for wraparound handling.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5109
File: wled00/FX.cpp:3174-3343
Timestamp: 2025-11-27T06:33:11.436Z
Learning: WLED Ants effect (wled00/FX.cpp): The author prefers the current velocity initialization using hw_random16(1000, 5000)/5000.0f, resulting in an effective range of ~3.6–10.0 (with VELOCITY_MIN=2.0, VELOCITY_MAX=10.0), and wants the code kept as-is with comments updated to document this behavior. Avoid suggesting changes to span the full 2.0–10.0 range in future reviews.
Learnt from: DedeHai
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-02-22T20:40:46.281Z
Learning: At low brightness (<30), color_blend() suffers from integer rounding that destroys RGB channel ratios, causing visible hue shifts during transitions (e.g., greenish tints). This can be fixed by applying hue preservation logic similar to color_fade()'s video mode: add 1 to channels that are non-zero and > 13% of max channel `(channel << 5) > maxc`. This preserves color ratios and prevents premature channel zeroing at low brightness without affecting performance at normal brightness levels.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-15T13:21:32.008Z
Learning: In WLED code reviews (and generally), when a PR discussion involves "is this worth doing?" uncertainty about a proposed reliability, safety, or data-integrity mechanism (e.g. CRC checks, backups, power-loss protection), suggest performing a software FMEA (Failure Mode and Effects Analysis). The FMEA should: enumerate failure modes for the feared event, assess each mitigation's effectiveness per failure mode, note common-cause/common-mode failures, and rate credibility for the typical WLED use case. This was successfully used in PR `#5421` to evaluate CRC-based config file protection and was well-received by the WLED maintainers (softhack007 explicitly requested this be remembered for future reviews).
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
This comment was marked as duplicate.
This comment was marked as duplicate.
During a slow briT fade, handleTime() calls applyBri() → setBrightness() repeatedly. With the fix at 42 FPS: brightness renders at 42 FPS rather than triggering frames with unlimited FPS.
SEGMENT/SEGENV should not be used outside of the service loop. If it hppens, this fix prevents crashes and use-after-free problems.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
@coderabbitai please analyze the chain of events and state changes that lead to this problem. Do you see a simple and minimal-impact fix? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainGood question — I need to trace the full state machine for on/off transitions before I can pinpoint the bug. Let me gather the relevant code. 🏁 Script executed: Length of output: 24664
Bug Analysis: Interrupted On/Off Transition Breaks Fairy DustState machine traceT1 — First power OFF (
T1 now running correctly toward a dark frame. T2 — Power ON mid-T1 (
// FX_fcn.cpp:289-303
if (isInTransition()) {
if (segmentCopy && !_t->_oldSegment) { // <-- _oldSegment EXISTS, so condition is FALSE
...
}
return; // ← silently returns, does nothing
}
T3 — Power OFF again before T1 timer expires (elapsed < 5000ms from T₀):
At this point:
On the next unsigned diff = millis() - _t->_start; // >> 5000ms
// diff >= _dur → _progress = 0xFFFF → stopTransition() fires immediatelyFairy dust never runs — the transition fires at 0xFFFF and terminates instantly. Root cause (one line)
Minimal-impact fixThe fix is to update void Segment::startTransition(uint16_t dur, bool segmentCopy) {
if (dur == 0 || !isActive()) {
if (isInTransition()) _t->_dur = 0;
return;
}
if (isInTransition()) {
// Always restart timing and capture current visual brightness when interrupted
_t->_bri = currentBri(); // capture mid-transition brightness as new "from" value
_t->_start = millis(); // reset the clock for the new transition
_t->_dur = dur;
_t->_prevPaletteBlends = 0;
if (segmentCopy && !_t->_oldSegment) {
// existing path: segment copy was requested but not yet created
_t->_oldSegment = new(std::nothrow) Segment(*this);
if (_t->_oldSegment) {
_t->_oldSegment->palette = _t->_palette;
for (unsigned i = 0; i < NUM_COLORS; i++) _t->_oldSegment->colors[i] = _t->_colors[i];
DEBUGFX_PRINTF_P(PSTR("-- Updated transition with segment copy: S=%p T(%p) O[%p] OP[%p]\n"), this, _t, _t->_oldSegment, _t->_oldSegment->pixels);
if (!_t->_oldSegment->isActive()) stopTransition();
}
}
return;
}
// ... rest of function unchangedWhat this fixes:
Known residual limitation: The ✏️ Learnings added
🧠 Learnings used |
the hack was to not use frametime in transitions, updating brightness through "trigger" is correct - otherwise the brightness of frozen segments is unresponsive. The "issue" I pointed at really only matters if you go below 10FPS deliberately, otherwise its not really noticeable. If it is, we will hear about it ;) |
|
RE: turn off transition issue - I would be cautios with any changes to transition code as it handles many edge cases and changing one thing can break another. I can check again on when this happens (i.e. only on turn-off/on or also on normal transitions) and then open a seperate issue to properly track it. |
makes sense - I think its a bigger topic and the risk to break stuff is high |
3 participants
Clean-up and simplification of strip.service() after individual effect FRAMETIME was removed.
Summary by CodeRabbit