spots effect accuracy improvements#5341
Draft
softhack007 wants to merge 3 commits intomainfrom
Draft
Conversation
* avoid gaps on left/light side of strip, by using higher accuracy zoneLength * prevent overflows * ensure that zones calculation are performed in full 32bit * fix wrong slider name in "spots fade"
Contributor
📝 WalkthroughWalkthroughReworks Spots Fade zone geometry and per-pixel math to use fixed-point arithmetic, clamps computed zones to at least one, masks brightness to 8 bits before blending, and updates the effect metadata parameter label from "Spread" to "Speed". Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@wled00/FX.cpp`:
- Around line 2955-2970: The current computation of zoneLen8 uses zones >> 3
which collapses 9–15 to 1 and can make (zones * zoneLen8) >> 3 exceed SEGLEN
causing offset underflow and huge indices; fix by using a true 1/8 fixed‑point
scale (avoid shifts on zones) — compute zoneLen8 as (SEGLEN * 8) / zones (using
a wider integer type to avoid overflow) and compute offset the same way ((SEGLEN
- ((zones * zoneLen8) >> 3)) >> 1); also replace the magic literal 8 with a
named constant like ZONE_FP_SCALE to clarify intent and prevent future errors
(symbols: zoneLen8, offset, zones, SEGLEN).
* use named constants for fixed point scaling * remove buggy "SEGLEN / (zones >>3)" shortcut * remove an unnecessary casts to uint32_t
softhack007
changed the title
softhack007
added
effect
optimization
DedeHai
requested changes
Feb 4, 2026
Collaborator
DedeHai
left a comment
DedeHai
left a comment
There was a problem hiding this comment.
generally an improvement, there are two issues (one introduced, one unsolved):
- on small segments, the threshold can be too high, turning off all spots. something like this fixes that but may introduce "breaking changes" I did only a brief test:
uint16_t maxWave = triwave16(((zoneLen - 1) * 0xFFFF) / zoneLen);
threshold = min(threshold, uint16_t(maxWave - 1)); // guarantee threshold is always reached (prevent "no spots" on short strips)to check: use a segment with 15 LEDs, turn speed slider down and intensity slider up.
- the newly introduced issue is a classic integer math rounding problem: while the spots now spread better, they distribute unevenly. I faced the same dilemma in "PS chase" and used more elaborate code to get both a full spread distribution as well as an even spacing.
| return spots_base(tr); | ||
| } | ||
| static const char _data_FX_MODE_SPOTS_FADE[] PROGMEM = "Spots Fade@Spread,Width,,,,,Overlay;!,!;!"; | ||
| static const char _data_FX_MODE_SPOTS_FADE[] PROGMEM = "Spots Fade@Speed,Width,,,,,Overlay;!,!;!"; |
Collaborator
There was a problem hiding this comment.
while you are at it: rename "Width" to "Density" for both variants?
Member
Author
@DedeHai thanks for your comments 😃 Yes unevenly distributed gaps are a new side-effect. At second look the nested loops with offset re-calcution seem unnecessary, we could also just repeat the tri-wave pattern until the end of strip is reached. Just that the effect might not be centered any more for long "spread" values. I'll think about a better implementation. Edit: maybe this is better
Edit2: 🤔 actually offset calculation is the only "tricky" thing left, because the effect should be centered no matter how long or short the strip really is... |
Collaborator
|
I need to check that code but in general for long er strips with 100 LEDs+ you will not notice a 1 pixel offset, unless its coarse and you want it aligned to furniture but I guess that is more of an edge case. What is much more noticeable is an irregular pattern. Edit: with 24V strips and "zones" now coming up more and more, low LED-count segments are not uncommon (I have 64 on mine) |
Member
Author
|
@DedeHai I'll make a second attempt on this, you're right that unequally spaced spots look ugly.. |
Contributor
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.cpp`:
- Around line 2921-2935: The loop currently computes a uniform zoneLen (SEGLEN /
zones) but computes per-zone start positions using fractional boundaries (offset
+ ((z * zoneLen8) >> ZONELEN_FP_SHIFT)), causing leftover pixels to become gaps;
change the inner loop to derive each zone's width from the next fractional
boundary instead of reusing zoneLen: compute pos for zone z using the existing
fractional formula (pos = offset + ((z * zoneLen8) >> ZONELEN_FP_SHIFT)) and
posNext for z+1 similarly, then set the per-zone width as zoneWidth = posNext -
pos (for the last zone use SEGLEN - pos), and iterate i from 0 to zoneWidth so
spots fill evenly; update uses of zoneLen and any dependent indexing (variables:
zones, zoneLen, zoneLen8, offset, pos) accordingly.
🪄 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: 15da9da6-a7cc-4142-b43b-def235c6c9df
📒 Files selected for processing (1)
wled00/FX.cpp
Comment on lines
+2921
to
+2935
| unsigned maxZones = max(1U, SEGLEN >> 2); // prevents "0 zones" | ||
| unsigned zones = 1U + ((uint32_t(SEGMENT.intensity) * maxZones) >> 8); | ||
| unsigned zoneLen = SEGLEN / zones; | ||
| unsigned zoneLen8 = (SEGLEN * ZONELEN_FP_SCALE) / zones; // zoneLength * 8 (fixed‑point) -> avoids gaps at right/left sides | ||
| unsigned offset = (SEGLEN - ((zones * zoneLen8) >> ZONELEN_FP_SHIFT)) >> 1; | ||
|
|
||
| for (unsigned z = 0; z < zones; z++) | ||
| { | ||
| unsigned pos = offset + z * zoneLen; | ||
| unsigned pos = offset + ((z * zoneLen8) >> ZONELEN_FP_SHIFT); | ||
| for (unsigned i = 0; i < zoneLen; i++) | ||
| { | ||
| unsigned wave = triwave16((i * 0xFFFF) / zoneLen); | ||
| if (wave > threshold) { | ||
| unsigned index = 0 + pos + i; | ||
| unsigned s = (wave - threshold)*255 / (0xFFFF - threshold); | ||
| unsigned index = pos + i; | ||
| unsigned s = ((wave - threshold)*255 / (0xFFFF - threshold)) & 0xFF; // & 0xFF prevents overflow in next line |
Contributor
There was a problem hiding this comment.
Use boundary-derived widths to keep the spots evenly spaced.
Line 2923 still fixes every zone to SEGLEN / zones, while Line 2929 places starts on fractional boundaries. On lengths that do not divide evenly, the leftover pixels turn into visible holes instead of evenly spaced spots. Derive each zone width from the next boundary instead of reusing the truncated integer width.
♻️ Proposed fix
- unsigned zoneLen = SEGLEN / zones;
- unsigned zoneLen8 = (SEGLEN * ZONELEN_FP_SCALE) / zones; // zoneLength * 8 (fixed‑point) -> avoids gaps at right/left sides
- unsigned offset = (SEGLEN - ((zones * zoneLen8) >> ZONELEN_FP_SHIFT)) >> 1;
+ unsigned zoneLen8 = (uint32_t(SEGLEN) * ZONELEN_FP_SCALE) / zones; // zoneLength * 8 (fixed-point) -> avoids gaps at right/left sides
+ unsigned offset = (SEGLEN - ((uint32_t(zones) * zoneLen8) >> ZONELEN_FP_SHIFT)) >> 1;
for (unsigned z = 0; z < zones; z++)
{
- unsigned pos = offset + ((z * zoneLen8) >> ZONELEN_FP_SHIFT);
+ unsigned pos = offset + ((uint32_t(z) * zoneLen8) >> ZONELEN_FP_SHIFT);
+ unsigned nextPos = (z + 1 == zones)
+ ? (SEGLEN - offset)
+ : offset + ((uint32_t(z + 1) * zoneLen8) >> ZONELEN_FP_SHIFT);
+ unsigned zoneLen = nextPos - pos;
for (unsigned i = 0; i < zoneLen; i++)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/FX.cpp` around lines 2921 - 2935, The loop currently computes a
uniform zoneLen (SEGLEN / zones) but computes per-zone start positions using
fractional boundaries (offset + ((z * zoneLen8) >> ZONELEN_FP_SHIFT)), causing
leftover pixels to become gaps; change the inner loop to derive each zone's
width from the next fractional boundary instead of reusing zoneLen: compute pos
for zone z using the existing fractional formula (pos = offset + ((z * zoneLen8)
>> ZONELEN_FP_SHIFT)) and posNext for z+1 similarly, then set the per-zone width
as zoneWidth = posNext - pos (for the last zone use SEGLEN - pos), and iterate i
from 0 to zoneWidth so spots fill evenly; update uses of zoneLen and any
dependent indexing (variables: zones, zoneLen, zoneLen8, offset, pos)
accordingly.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
Member
Author
|
@coderabbitai let's take the chance to clean up your learnings:
|
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me first fetch the relevant files to compare against the retrieved learnings. 🏁 Script executed: Length of output: 29759
🗑️ Remove — covered by
|
| ID | Topic |
|---|---|
| 377d02d0 | mode_dissolveplus: hw_random16(SEGLEN) safe for 1D effects |
| 5c0e30c2 | WS2812FX::service(): FX_MODE_STATIC rendering behavior (PR #5443) |
| e174bc9e | Gamma calls in FX.cpp intentional for effect shaping |
| f885e51b | Timestamp assignment stored_time = now + delay is correct for wraparound |
| 621387da | Ants effect: velocity init range intentional, don't change |
| 517af376 | FMEA approach for reliability/safety mechanism evaluation |
| a17fbaa8 | mode_random_chase(): PRNG advance before prevSeed capture is intentional |
| 1d43d5c1 | Post-commit ee9ac94: 0xFFFF unmapped entries left with stale colors |
| 58837130 | respondModeData(): 256-char lineBuffer limit is intentional |
| a76480ab | Spinning Wheel: color scale must match position modulo range |
| da18ae54 | startTransition(): rapid toggle bug — stale _t->_start |
No other learnings appear expired or outdated based on the current codebase context.
⛔ Learnings removed
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: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
🧠 Learnings used
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5440
File: usermods/user_fx/user_fx.cpp:1304-1313
Timestamp: 2026-03-25T07:03:35.475Z
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: 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: 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: 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-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
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: 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: DedeHai
Repo: wled/WLED PR: 4615
File: wled00/FX.cpp:1814-1817
Timestamp: 2026-03-29T07:49:28.820Z
Learning: WLED wled00/FX.cpp::mode_random_chase(): Advancing the shared PRNG before capturing prevSeed on the first call is intentional to decorrelate initial seeds across effects. Do not suggest moving the snapshot earlier; if needed, add a comment to document this behavior.
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: willmmiles
Repo: wled/WLED PR: 5462
File: wled00/json.cpp:1189-1198
Timestamp: 2026-03-30T15:32:08.847Z
Learning: In WLED's `respondModeData()` (wled00/json.cpp), the 256-character `lineBuffer` limit for effect descriptor strings (getModeData) is an intentional constraint that matches the same 256-char limit used ~6 other places in the codebase. It is not new to this PR and should not be flagged as a regression. Lifting it requires a future refactor (better type system or dynamic buffer).
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5293
File: usermods/user_fx/user_fx.cpp:149-151
Timestamp: 2026-01-24T18:51:09.740Z
Learning: In WLED's Spinning Wheel effect (usermods/user_fx/user_fx.cpp), the color scale calculation (SEGENV.aux1) must match the position modulo range. When position uses `pos = (pos_fixed >> 16) % SEGLEN`, the color scale must be `aux1 = (255 << 8) / SEGLEN` to distribute the palette evenly across the full position range. Using vstripLen for color scaling while position uses SEGLEN causes palette colors to cycle too quickly and appear incomplete.
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.
Member
Author
|
@coderabbitai please double-check if your learning f885e51b is correct. It sounds like the pattern is not safe against unsigned wrap-around. Personally i would prefer the "millis() - last_time > wait" pattern that is also in your learnings. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainGood question — let me check the exact code context that originally generated learning 🏁 Script executed: Length of output: 2097 Let me check how 🏁 Script executed: Length of output: 822
// Assignment (lines 224/226):
state[SLOWDOWN_TIME_IDX] = now + hw_random16(2000, 6000);
// Comparison (line 243):
if ((int32_t)(now - state[SLOWDOWN_TIME_IDX]) >= 0)The The preferred WLED idiom ✏️ Learnings added
⛔ Learnings removed🧠 Learnings used |
Member
Author
|
@coderabbitai please also check your learnings against the new .github/copilot-instructions.md (applicable via .coderabbit.yaml) and check for learnings that are fully covered and could be removed now. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 12988
🗑️ Remove — fully covered by
|
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.
A few improvement for the spots effect, especially relevant for longer strips:
Summary by CodeRabbit
Bug Fixes
Documentation