Spinning Wheel FX in the user_fx usermod #5293
Open
+256
−0
Conversation
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
Contributor
WalkthroughAdds a new Spinning Wheel user FX with per-strip multi‑phase animation (fast spin → slowdown → wobble → stopped), per‑strip state and randomness, palette or color‑wheel color modes, two helper macros, and registers it in UserFxUsermod setup. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
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 @usermods/user_fx/user_fx.cpp:
- Around line 167-171: Comparison of timestamps using addition can overflow;
replace checks like `now >= state[STOP_TIME_IDX] + spin_delay` with a
subtraction-based elapsed-time check to handle wraparound safely. For the `phase
== 3` branch (variables: now, state[STOP_TIME_IDX], spin_delay, needsReset)
change the condition to `now - state[STOP_TIME_IDX] >= spin_delay` (ensure both
operands are uint32_t/compatible), and apply the same pattern to the other
similar checks in this file that add a delay to a stored timestamp (the
comparisons at the other locations referenced in the review) so every `now >=
stored_timestamp + delay` becomes `now - stored_timestamp >= delay`.
🧹 Nitpick comments (1)
usermods/user_fx/user_fx.cpp (1)
111-111: Consider addingstaticfor consistency.The
mode_spinning_wheelfunction lacks thestatickeyword, unlikemode_static(line 11) andmode_diffusionfire(line 22). While this doesn't cause functional issues, addingstaticmaintains consistency and limits the function's linkage scope.-uint16_t mode_spinning_wheel(void) { +static uint16_t mode_spinning_wheel(void) {
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/user_fx/user_fx.cpp
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
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: 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: BobLoeffler68
Repo: wled/WLED PR: 5251
File: usermods/user_fx/user_fx.cpp:146-148
Timestamp: 2026-01-01T07:19:40.244Z
Learning: In the WLED Ants effect (usermods/user_fx/user_fx.cpp), the color alternation logic `antIndex % 3 == 1` is intentional and creates a 1/3 vs 2/3 distribution between SEGCOLOR(0) and SEGCOLOR(2). This unbalanced alternation is by design and should not be changed to a balanced mod 2 alternation.
Learnt from: netmindz
Repo: wled/WLED PR: 4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: netmindz
Repo: wled/WLED PR: 4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
Repo: wled/WLED PR: 4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2026-01-01T07:19:40.244Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5251
File: usermods/user_fx/user_fx.cpp:146-148
Timestamp: 2026-01-01T07:19:40.244Z
Learning: In the WLED Ants effect (usermods/user_fx/user_fx.cpp), the color alternation logic `antIndex % 3 == 1` is intentional and creates a 1/3 vs 2/3 distribution between SEGCOLOR(0) and SEGCOLOR(2). This unbalanced alternation is by design and should not be changed to a balanced mod 2 alternation.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-08-26T11:51:21.817Z
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.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-11-22T12:12:53.486Z
Learnt from: DedeHai
Repo: wled/WLED PR: 5105
File: wled00/FX.h:968-968
Timestamp: 2025-11-22T12:12:53.486Z
Learning: In WLED's WS2812FX class (wled00/FX.h), the _pixels array stores LED colors indexed by unmapped (logical) LED positions, not physical positions. The getPixelColor(n) function intentionally checks if the logical LED at index n maps to a valid physical position via getMappedPixelIndex(n), then accesses _pixels[n] with the unmapped index. If the mapping is invalid (e.g., -1 for gaps in LED maps, or out of bounds), it returns 0 (black). This allows peek preview to correctly show gaps in LED mappings as black pixels. The mapping from logical to physical positions is only applied when writing to the hardware bus, not when reading from the pixel buffer.
<!-- [add_learning]
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-10-20T09:41:50.374Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4995
File: wled00/FX.cpp:5209-5212
Timestamp: 2025-10-20T09:41:50.374Z
Learning: In WLED 2D effects, SEGMENT.length() returns the physical pixel count (upper bound), while SEG_W * SEG_H can be smaller depending on mapping (mirroring/grouping). For per-cell state buffers (e.g., mode_2Dgameoflife in wled00/FX.cpp), allocate using SEGMENT.length() to ensure sufficient capacity and avoid reallocations, while bounding indexing by cols*rows.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-12-28T14:06:48.772Z
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.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-11-27T06:33:11.436Z
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.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2026-01-01T13:18:10.072Z
Learnt from: softhack007
Repo: wled/WLED PR: 5251
File: usermods/user_fx/user_fx.cpp:225-227
Timestamp: 2026-01-01T13:18:10.072Z
Learning: In WLED, when computing time differences between unsigned long timestamps (e.g., strip.now, lastBumpUpdate) for ESP32-C3 (RISC-V), cast the subtraction result to int before converting to float. Use float(int(timestamp1 - timestamp2)) instead of float(timestamp1 - timestamp2) to avoid wraparound issues and ensure correct behavior. Apply this pattern in usermods/user_fx/user_fx.cpp and similar timestamp-difference calculations in related modules where unsigned long subtraction could overflow before conversion.
Applied to files:
usermods/user_fx/user_fx.cpp
🔇 Additional comments (7)
usermods/user_fx/user_fx.cpp (7)
5-8: LGTM!The macro definitions follow established WLED patterns for palette wrapping and virtual strip indexing.
111-144: LGTM!The initialization logic is sound: early exit for invalid segment length, proper state allocation for all virtual strips, and efficient caching of the color scale factor.
146-152: LGTM!The settings change detection using a simple checksum is appropriate for triggering effect restarts. Minor collisions won't cause issues beyond an occasional unnecessary restart.
263-268: LGTM!The LED drawing logic correctly computes position from fixed-point representation, maps position to hue using the cached scaling factor, and properly addresses virtual strips.
271-276: LGTM!The main loop correctly iterates through all virtual strips, passing the appropriate state pointer offset for each. Returning
FRAMETIMEis the standard pattern for frame-rate effects.
277-277: LGTM!The metadata string correctly maps the UI controls to the segment parameters used in the effect code, and the
m12=1option properly enables 2D expansion for virtual strip support.
288-291: LGTM!The new effect is correctly registered following the established pattern, using id=255 for dynamic ID assignment.
Contributor
Author
|
This effect came from an idea under Feature Request: FX: Random Pixel left |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @usermods/user_fx/user_fx.cpp:
- Around line 141-144: SEGENV.aux1 is declared as uint16_t but is being assigned
(255 << 16) / SEGLEN which overflows for SEGLEN < 256; fix by either (A)
widening SEGENV.aux1 to a 32-bit type (e.g., uint32_t) and update all uses to
expect a 32-bit scale, or (B) stop storing the full 32-bit scale and compute the
32-bit division at use time (replace the assignment in the SEGENV.call == 0
block with leaving aux1 as a small cached value or remove it and compute (255U
<< 16) / SEGLEN in the draw routine where aux1 is consumed). Update all
references to SEGENV.aux1 in the drawing code to match the chosen approach (use
32-bit arithmetic if you widened the field, or use a local uint32_t temp for
inline computation if you compute on demand).
🧹 Nitpick comments (1)
usermods/user_fx/user_fx.cpp (1)
177-183: Consider documenting the velocity scaling factor.The multiplier
655(approximately 65536/100) is used for fixed-point velocity scaling. A brief comment would help future maintainers understand the relationship between the UI slider values and actual animation speed.💡 Suggested documentation
// Set velocity + // Velocity is fixed-point (pixels/frame * 65536). Multiplier 655 ≈ 65536/100 + // scales random values to give reasonable animation speeds. if (SEGMENT.check2) { // random speed state[VELOCITY_IDX] = random16(200, 900) * 655;
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/user_fx/user_fx.cpp
🧰 Additional context used
🧠 Learnings (19)
📓 Common learnings
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: 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: BobLoeffler68
Repo: wled/WLED PR: 5251
File: usermods/user_fx/user_fx.cpp:146-148
Timestamp: 2026-01-01T07:19:40.244Z
Learning: In the WLED Ants effect (usermods/user_fx/user_fx.cpp), the color alternation logic `antIndex % 3 == 1` is intentional and creates a 1/3 vs 2/3 distribution between SEGCOLOR(0) and SEGCOLOR(2). This unbalanced alternation is by design and should not be changed to a balanced mod 2 alternation.
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
Repo: wled/WLED PR: 4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2026-01-13T03:07:13.582Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5293
File: usermods/user_fx/user_fx.cpp:167-171
Timestamp: 2026-01-13T03:07:13.582Z
Learning: In WLED user_fx modules (usermods/user_fx/*.cpp and similar), when storing future timestamps, assign stored_time = now + delay is correct and safe with uint32_t wraparound. Only use subtraction-based checks like (now - stored_time) >= delay to determine elapsed time. Do not flag assignments for wraparound handling; only compare elapsed time. This guideline applies to all files in the user_fx module.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2026-01-01T13:18:10.072Z
Learnt from: softhack007
Repo: wled/WLED PR: 5251
File: usermods/user_fx/user_fx.cpp:225-227
Timestamp: 2026-01-01T13:18:10.072Z
Learning: In WLED, when computing time differences between unsigned long timestamps (e.g., strip.now, lastBumpUpdate) for ESP32-C3 (RISC-V), cast the subtraction result to int before converting to float. Use float(int(timestamp1 - timestamp2)) instead of float(timestamp1 - timestamp2) to avoid wraparound issues and ensure correct behavior. Apply this pattern in usermods/user_fx/user_fx.cpp and similar timestamp-difference calculations in related modules where unsigned long subtraction could overflow before conversion.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-08-26T11:51:21.817Z
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.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-11-27T06:33:11.436Z
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.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-11-14T13:37:11.994Z
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.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-11-14T13:37:30.955Z
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.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2026-01-01T07:19:40.244Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5251
File: usermods/user_fx/user_fx.cpp:146-148
Timestamp: 2026-01-01T07:19:40.244Z
Learning: In the WLED Ants effect (usermods/user_fx/user_fx.cpp), the color alternation logic `antIndex % 3 == 1` is intentional and creates a 1/3 vs 2/3 distribution between SEGCOLOR(0) and SEGCOLOR(2). This unbalanced alternation is by design and should not be changed to a balanced mod 2 alternation.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-08-31T03:38:14.114Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-12-01T07:01:16.949Z
Learnt from: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.949Z
Learning: In WLED PR #5134, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2026-01-03T14:38:09.949Z
Learnt from: DedeHai
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-01-03T14:38:09.949Z
Learning: In WLED code reviews, check that FastLED trigonometric function aliases are not used. Replace `sin8()` and `cos8()` with `sin8_t()` and `cos8_t()`. Replace `sin16()` and `cos16()` with `sin16_t()` and `cos16_t()`. For float versions, replace `sinf()` and `cosf()` with `sin_approx()` and `cos_approx()`. See wled_math.cpp for the proper WLED implementations.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, verify that file operations (especially file.open()) respect LittleFS filename limitations. Assume default WLED configuration with LittleFS default filename limit of 255 bytes. Do not assume extreme configuration values like WLED_MAX_SEGNAME_LEN = 512 which would not be standard configurations.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-11-22T12:12:53.486Z
Learnt from: DedeHai
Repo: wled/WLED PR: 5105
File: wled00/FX.h:968-968
Timestamp: 2025-11-22T12:12:53.486Z
Learning: In WLED's WS2812FX class (wled00/FX.h), the _pixels array stores LED colors indexed by unmapped (logical) LED positions, not physical positions. The getPixelColor(n) function intentionally checks if the logical LED at index n maps to a valid physical position via getMappedPixelIndex(n), then accesses _pixels[n] with the unmapped index. If the mapping is invalid (e.g., -1 for gaps in LED maps, or out of bounds), it returns 0 (black). This allows peek preview to correctly show gaps in LED mappings as black pixels. The mapping from logical to physical positions is only applied when writing to the hardware bus, not when reading from the pixel buffer.
<!-- [add_learning]
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-10-20T09:41:50.374Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4995
File: wled00/FX.cpp:5209-5212
Timestamp: 2025-10-20T09:41:50.374Z
Learning: In WLED 2D effects, SEGMENT.length() returns the physical pixel count (upper bound), while SEG_W * SEG_H can be smaller depending on mapping (mirroring/grouping). For per-cell state buffers (e.g., mode_2Dgameoflife in wled00/FX.cpp), allocate using SEGMENT.length() to ensure sufficient capacity and avoid reallocations, while bounding indexing by cols*rows.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-12-28T14:06:48.772Z
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.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-09-12T17:29:43.826Z
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.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-08-08T17:22:37.374Z
Learnt from: netmindz
Repo: wled/WLED PR: 4819
File: wled00/FX.cpp:10654-10655
Timestamp: 2025-08-08T17:22:37.374Z
Learning: In WLED FX.cpp, um_data returned by getAudioData() is intentionally untyped because different usermods provide different payloads, and simulateSound() may supply dummy data when the AudioReactive usermod isn’t built. Strongly typed accessors are not feasible; prefer explicit casts (e.g., samplePeak as uint8_t at u_data[3]) or small helpers that encapsulate these casts.
Applied to files:
usermods/user_fx/user_fx.cpp
🧬 Code graph analysis (1)
usermods/user_fx/user_fx.cpp (1)
usermods/usermod_v2_animartrix/usermod_v2_animartrix.cpp (1)
SEGENV(64-76)
🔇 Additional comments (6)
usermods/user_fx/user_fx.cpp (6)
5-8: LGTM!The macro definitions follow established WLED patterns.
PALETTE_SOLID_WRAPcorrectly checks palette blend modes, andindexToVStripproperly encodes the virtual strip number into the upper bits of the index.
146-152: LGTM!The settings checksum correctly excludes
check1(color mode) since changing the color mode shouldn't restart the animation—it only affects the final color output, not the spin state.
167-171: LGTM!The timestamp handling is correct:
- Line 169 uses subtraction-based elapsed time check for determining if the delay has passed
- Line 207 casts to
int32_tfor comparing against future timestamps, correctly handling wraparound (negative result indicates the target time hasn't been reached yet)Based on learnings, this follows the proper pattern for uint32_t timestamp arithmetic.
Also applies to: 205-210
230-255: LGTM!The wobble phase creates a realistic "bounce back" effect typical of spinning wheel toys—moving back one LED, then forward to the stop position. The timing delays (200ms, 400ms, 300ms) are reasonable for the visual effect.
263-268: LGTM!The drawing logic correctly extracts the integer position from fixed-point, determines the hue based on position, and uses
indexToVStripto address the appropriate virtual strip. The color mode toggle between color wheel and palette provides good user flexibility.
277-277: LGTM!The effect metadata correctly maps the UI controls to the corresponding segment parameters. The default
c3=8provides a reasonable ~5 second delay between spins. Effect registration follows the standard pattern using ID 255 for dynamic assignment.Also applies to: 288-291
…comment about the velocity scaling.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
usermods/user_fx/user_fx.cpp (1)
5-8: Extract these macros to a shared header to eliminate duplication.
PALETTE_SOLID_WRAPandindexToVStripare defined identically inwled00/FX.cppand duplicated here. This pattern also appears inusermods/pixels_dice_tray/led_effects.h. Consider moving them to a common header (e.g.,wled00/FX.h) to maintain consistency across the codebase and reduce duplication risk.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/user_fx/user_fx.cpp
🧰 Additional context used
🧠 Learnings (23)
📓 Common learnings
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: 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: BobLoeffler68
Repo: wled/WLED PR: 5251
File: usermods/user_fx/user_fx.cpp:146-148
Timestamp: 2026-01-01T07:19:40.244Z
Learning: In the WLED Ants effect (usermods/user_fx/user_fx.cpp), the color alternation logic `antIndex % 3 == 1` is intentional and creates a 1/3 vs 2/3 distribution between SEGCOLOR(0) and SEGCOLOR(2). This unbalanced alternation is by design and should not be changed to a balanced mod 2 alternation.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5293
File: usermods/user_fx/user_fx.cpp:167-171
Timestamp: 2026-01-13T03:07:13.582Z
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.
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
Repo: wled/WLED PR: 4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-08-26T11:51:21.817Z
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.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2026-01-01T07:19:40.244Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5251
File: usermods/user_fx/user_fx.cpp:146-148
Timestamp: 2026-01-01T07:19:40.244Z
Learning: In the WLED Ants effect (usermods/user_fx/user_fx.cpp), the color alternation logic `antIndex % 3 == 1` is intentional and creates a 1/3 vs 2/3 distribution between SEGCOLOR(0) and SEGCOLOR(2). This unbalanced alternation is by design and should not be changed to a balanced mod 2 alternation.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2026-01-13T03:07:13.582Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5293
File: usermods/user_fx/user_fx.cpp:167-171
Timestamp: 2026-01-13T03:07:13.582Z
Learning: In WLED user_fx modules (usermods/user_fx/*.cpp and similar), when storing future timestamps, assign stored_time = now + delay is correct and safe with uint32_t wraparound. Only use subtraction-based checks like (now - stored_time) >= delay to determine elapsed time. Do not flag assignments for wraparound handling; only compare elapsed time. This guideline applies to all files in the user_fx module.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2026-01-01T13:18:10.072Z
Learnt from: softhack007
Repo: wled/WLED PR: 5251
File: usermods/user_fx/user_fx.cpp:225-227
Timestamp: 2026-01-01T13:18:10.072Z
Learning: In WLED, when computing time differences between unsigned long timestamps (e.g., strip.now, lastBumpUpdate) for ESP32-C3 (RISC-V), cast the subtraction result to int before converting to float. Use float(int(timestamp1 - timestamp2)) instead of float(timestamp1 - timestamp2) to avoid wraparound issues and ensure correct behavior. Apply this pattern in usermods/user_fx/user_fx.cpp and similar timestamp-difference calculations in related modules where unsigned long subtraction could overflow before conversion.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-11-27T06:33:11.436Z
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.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-11-14T13:37:11.994Z
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.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-11-14T13:37:30.955Z
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.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-08-31T03:38:14.114Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-12-01T07:01:16.949Z
Learnt from: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.949Z
Learning: In WLED PR #5134, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2026-01-03T14:38:09.949Z
Learnt from: DedeHai
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-01-03T14:38:09.949Z
Learning: In WLED code reviews, check that FastLED trigonometric function aliases are not used. Replace `sin8()` and `cos8()` with `sin8_t()` and `cos8_t()`. Replace `sin16()` and `cos16()` with `sin16_t()` and `cos16_t()`. For float versions, replace `sinf()` and `cosf()` with `sin_approx()` and `cos_approx()`. See wled_math.cpp for the proper WLED implementations.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, verify that file operations (especially file.open()) respect LittleFS filename limitations. Assume default WLED configuration with LittleFS default filename limit of 255 bytes. Do not assume extreme configuration values like WLED_MAX_SEGNAME_LEN = 512 which would not be standard configurations.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-04-30T05:41:03.633Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-11-16T19:40:46.260Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4926
File: wled00/FX.cpp:4727-4730
Timestamp: 2025-11-16T19:40:46.260Z
Learning: WLED AuroraWave (wled00/FX.cpp): wave_start and wave_end intentionally use int16_t; segments longer than 32k LEDs are not supported (bounded by MAX_LEDS), so widening to 32-bit is unnecessary.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-12-28T14:06:48.772Z
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.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-11-14T13:37:11.994Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, file operations (especially file.open()) should be checked to ensure they respect LittleFS filename limitations. The default LittleFS filename limit is 255 bytes (LFS_NAME_MAX). Reviews should assume default WLED configuration defines and not extreme edge-case values (e.g., WLED_MAX_SEGNAME_LEN = 512 would not be standard). File paths should be validated to stay within the 255-byte limit.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-11-22T12:12:53.486Z
Learnt from: DedeHai
Repo: wled/WLED PR: 5105
File: wled00/FX.h:968-968
Timestamp: 2025-11-22T12:12:53.486Z
Learning: In WLED's WS2812FX class (wled00/FX.h), the _pixels array stores LED colors indexed by unmapped (logical) LED positions, not physical positions. The getPixelColor(n) function intentionally checks if the logical LED at index n maps to a valid physical position via getMappedPixelIndex(n), then accesses _pixels[n] with the unmapped index. If the mapping is invalid (e.g., -1 for gaps in LED maps, or out of bounds), it returns 0 (black). This allows peek preview to correctly show gaps in LED mappings as black pixels. The mapping from logical to physical positions is only applied when writing to the hardware bus, not when reading from the pixel buffer.
<!-- [add_learning]
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-10-20T09:41:50.374Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4995
File: wled00/FX.cpp:5209-5212
Timestamp: 2025-10-20T09:41:50.374Z
Learning: In WLED 2D effects, SEGMENT.length() returns the physical pixel count (upper bound), while SEG_W * SEG_H can be smaller depending on mapping (mirroring/grouping). For per-cell state buffers (e.g., mode_2Dgameoflife in wled00/FX.cpp), allocate using SEGMENT.length() to ensure sufficient capacity and avoid reallocations, while bounding indexing by cols*rows.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-09-12T17:29:43.826Z
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.
Applied to files:
usermods/user_fx/user_fx.cpp
📚 Learning: 2025-08-08T17:22:37.374Z
Learnt from: netmindz
Repo: wled/WLED PR: 4819
File: wled00/FX.cpp:10654-10655
Timestamp: 2025-08-08T17:22:37.374Z
Learning: In WLED FX.cpp, um_data returned by getAudioData() is intentionally untyped because different usermods provide different payloads, and simulateSound() may supply dummy data when the AudioReactive usermod isn’t built. Strongly typed accessors are not feasible; prefer explicit casts (e.g., samplePeak as uint8_t at u_data[3]) or small helpers that encapsulate these casts.
Applied to files:
usermods/user_fx/user_fx.cpp
🧬 Code graph analysis (1)
usermods/user_fx/user_fx.cpp (1)
usermods/usermod_v2_animartrix/usermod_v2_animartrix.cpp (1)
SEGENV(64-76)
🔇 Additional comments (5)
usermods/user_fx/user_fx.cpp (5)
111-144: LGTM!State management is well-structured:
- Proper allocation with per-strip state arrays
- Clear documentation of state indices with
constexprfor maintainability- Color scale caching in
aux1is a good performance optimization
178-183: Velocity calculations look correct.The fixed-point velocity scaling (
* 655 ≈ * 65536/100) is properly documented in the comment. The maximum value:
- Random speed:
900 * 655 = 589,500fits inuint32_t- Mapped speed:
900 * 655 = 589,500fits inuint32_t
204-255: Phase management and timing logic are well-implemented.Good use of wraparound-safe timing patterns:
- Line 207:
(int32_t)(now - state[SLOWDOWN_TIME_IDX]) >= 0correctly detects when a future timestamp has been reached- Line 234: Elapsed time calculation handles uint32_t wraparound correctly
- Wobble sequence timing (200ms → 400ms → 300ms) provides a natural deceleration feel
277-277: Metadata string is correctly formatted.The effect metadata properly defines:
- Sliders: speed, intensity, and custom3 (with custom1/custom2 skipped via
,,)- Checkboxes: color mode, random speed, random time
- Defaults:
c3=8provides a reasonable ~5.3s spin delay default
288-291: Effect registration follows the established pattern.Using
id=255for user effects is correct—the final ID is assigned bystrip.addEffect().
* Added Color Per Block checkbox. Enabled will set the spinner LEDs to the same color (instead of changing colors depending on the palette and LED position). * Added Sync Restart checkbox. Enabled means that all spinners will restart together (instead of individually) after they have all stopped spinning. * Added resizing the spinner slider (between 1 and 10 LEDs). * Changed how we do random speed and slowdown start time (sliders set to 0 = random). * tweaks here and there
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@usermods/user_fx/user_fx.cpp`:
- Around line 302-308: The code computes numSpinners using floor division which
undercounts when nrOfVStrips() isn't a multiple of spinnerSize; in the block
that checks SEGMENT.check2, change the computation of numSpinners to use ceiling
division (e.g., (SEGMENT.nrOfVStrips() + spinnerSize - 1) / spinnerSize) while
still guarding with max(1, ...); update the calculation around spinnerSize,
stripNr and hue so each partial last block gets its own hue instead of reusing
the first.
Collaborator
|
since you already made a few videos it may be useful to put one or two of them in the opening comment just for future reference. |
Contributor
Author
|
@DedeHai I added 3 video links to the first comment for this PR. Please test and merge when you can. If you think it's good enough for the main core effects (fx.cpp), that would be fine too. It's quite versatile as you will see, so maybe it could be a core FX. I also added a video for the other 3 effects that I've created for the user fx usermod. They are Ants (PR #5251), Morse Code (#5252), and Lava Lamp (#5253). Please test them when you have time and merge if/when you think they are ready. (I think they are ready.) :-) Lava Lamp might actually be good as a core FX as millions of people around the world have had (or know of) the real lava lamps. I'll have another 2D effect coming soon as well for the usermod. Also, none of these have labels (e.g. Effect or Usermod) yet. Am I able to add a label to these PRs? I don't see any way to do it. A couple of my most recent FX have used claude.ai a lot, so they should probably get the "AI" label? |
netmindz
approved these changes
Jan 21, 2026
DedeHai
reviewed
Jan 22, 2026
Collaborator
let's keep it in user FX for now, if moving to main code it would need more polishing and optimized the code size. I did not check in detail on how the logic works and if it is possible to make it more compact but the "state machine" appears to me that it could be optimized by leveraging some coding tricks.
I need to check that and how your implementation compares to my (still unfinished) version of a lava lamp effect.
labels are assigned by maintainers and are just a visual aid to more quickly identify PRs in the list or to indicate what the state of a PR is (like "awaiting testing" ) |
Contributor
Author
Ok, thx.
I did look at your version on Pygame and the blobs were very spread out and didn't act like the lava lamps that I've had. I didn't let it run for very long, so maybe the blobs would merge after a while. In my lava lamp, they are spread out at first, but then after a minute or so, they merge in the middle and then mostly go up and down like a lava lamp.
Okie doke. Thx. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@usermods/user_fx/user_fx.cpp`:
- Around line 5-6: PALETTE_SOLID_WRAP is using strip.paletteBlend which isn’t
synchronized with the UI; change the check to use the global paletteBlend (or
explicitly copy global paletteBlend into strip.paletteBlend during
initialization) so UI changes take effect. Update the macro/definition for
PALETTE_SOLID_WRAP to reference the global paletteBlend variable (instead of
strip.paletteBlend), or ensure code that sets up/updates Strip objects copies
the global paletteBlend into strip.paletteBlend whenever the UI value changes;
touch symbols: PALETTE_SOLID_WRAP, paletteBlend, and strip.paletteBlend.
- Around line 145-147: SEGENV.aux1 and any modulo/index math used when rendering
virtual strips in M12_pBar currently use SEGLEN (full strip area) but must use
the per-virtual-strip length virtualHeight(); replace uses of SEGLEN in the
M12_pBar branch (including the SEGENV.aux1 computation near
random16_set_seed(hw_random16()) and the index/modulo/scaling logic around the
blocks you noted at ~298–310 and ~318–323) with virtualHeight() so indices and
scaling are computed per-strip, not per-area, preventing out-of-bounds
wrapping/clipping.
Comment on lines
145
to
147
| random16_set_seed(hw_random16()); | ||
| SEGENV.aux1 = (255 << 8) / SEGLEN; // Cache the color scaling | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the specific lines mentioned in the review
cat -n usermods/user_fx/user_fx.cpp | sed -n '140,150p; 143,150p; 298,325p'Repository: wled/WLED
Length of output: 2394
🏁 Script executed:
# Find the definition and usage of indexToVStrip
rg -n "indexToVStrip" -B 2 -A 5Repository: wled/WLED
Length of output: 6486
🏁 Script executed:
# Search for virtualHeight and related functions
rg -n "virtualHeight|vHeight\(\)|nrOfVStrips" -B 2 -A 2Repository: wled/WLED
Length of output: 19142
🏁 Script executed:
# Look for how virtual strips are used in effects
rg -n "indexToVStrip" usermods/user_fx/user_fx.cpp -B 3 -A 8Repository: wled/WLED
Length of output: 974
🏁 Script executed:
# Check SEGLEN definition and usage
rg -n "define SEGLEN|#define SEGLEN" -A 2Repository: wled/WLED
Length of output: 231
🏁 Script executed:
# Find setPixelColor implementation and how it handles indexToVStrip encoding
rg -n "void.*setPixelColor\(" wled00/FX_fcn.cpp -A 15 | head -60Repository: wled/WLED
Length of output: 1502
🏁 Script executed:
# Check if there's any mapping logic that interprets the high 16 bits (stripNr encoding)
rg -n "stripNr|high.*16|>>.*16" wled00/FX_fcn.cpp | head -30Repository: wled/WLED
Length of output: 515
🏁 Script executed:
# Verify how the Fire 2012 effect handles virtual strips - does it always use full SEGLEN for each strip?
rg -n "mode_fire_2012" wled00/FX.cpp -A 40 | head -50Repository: wled/WLED
Length of output: 2593
🏁 Script executed:
# Check if there's explicit bounds checking when using setPixelColor with virtual strips
rg -n "setPixelColor.*indexToVStrip|indexToVStrip.*setPixelColor" wled00/FX.cpp -B 2 -A 2 | head -40Repository: wled/WLED
Length of output: 2327
🏁 Script executed:
# Look for documentation or comments about per-strip indexing vs full SEGLEN
rg -n "virtual.*strip|vstrip" wled00/FX.h -B 2 -A 2 | head -40Repository: wled/WLED
Length of output: 533
Use per-strip length instead of SEGLEN when rendering virtual strips in M12_pBar mode.
For M12_pBar (expanding 1D effects to 2D bars), each virtual strip has length virtualHeight(), not SEGLEN (which is virtualWidth() * virtualHeight()). Moduloing and scaling by SEGLEN produces indices beyond the per-strip bounds, causing pixel wrapping or clipping errors.
🛠️ Proposed fix
+ const uint16_t vstripLen = SEGMENT.is2D() ? SEGMENT.virtualHeight() : SEGLEN;
if (SEGENV.call == 0) {
random16_set_seed(hw_random16());
- SEGENV.aux1 = (255 << 8) / SEGLEN; // Cache the color scaling
+ SEGENV.aux1 = (255 << 8) / vstripLen; // Cache the color scaling
}
...
- uint16_t pos = (pos_fixed >> 16) % SEGLEN;
+ uint16_t pos = (pos_fixed >> 16) % vstripLen;
...
- uint16_t drawPos = (pos + y) % SEGLEN;
+ uint16_t drawPos = (pos + y) % vstripLen;Also applies to: 298–310, 318–323
🤖 Prompt for AI Agents
In `@usermods/user_fx/user_fx.cpp` around lines 145 - 147, SEGENV.aux1 and any
modulo/index math used when rendering virtual strips in M12_pBar currently use
SEGLEN (full strip area) but must use the per-virtual-strip length
virtualHeight(); replace uses of SEGLEN in the M12_pBar branch (including the
SEGENV.aux1 computation near random16_set_seed(hw_random16()) and the
index/modulo/scaling logic around the blocks you noted at ~298–310 and ~318–323)
with virtualHeight() so indices and scaling are computed per-strip, not
per-area, preventing out-of-bounds wrapping/clipping.
✅ Addressed in commit 0f5469a
Contributor
Author
There was a problem hiding this comment.
@coderabbitai The vstripLen suggestion is good when using the M12_pBar, but when I use the Pinwheel option, part of the pinwheel is missing.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for M12 mapping mode definitions and usage
rg -n "M12_|mapping.*1D2D|pBar|Pinwheel" wled00/const.h wled00/FX.h -B 2 -A 2
Length of output: 668
🏁 Script executed:
#!/bin/bash
# Look for how other effects handle different mapping modes with virtual strips
rg -n "nrOfVStrips|virtualHeight|mapping" wled00/FX.cpp -B 3 -A 3 | grep -A 5 -B 5 "M12\|pBar\|mapping"
Length of output: 3257
🏁 Script executed:
#!/bin/bash
# Search for the mapping enum or constants
rg -n "enum.*map|MAP.*=|mapping.*mode" wled00/const.h wled00/FX.h -B 1 -A 3
Length of output: 157
🏁 Script executed:
#!/bin/bash
# Look at how fire effects or other 1D-to-2D effects determine strip length
rg -n "mode_fire_2012|Fire 2012" wled00/FX.cpp -A 50 | head -80
Length of output: 3682
@BobLoeffler68 Thank you for catching that! You're absolutely right. My initial suggestion was too narrow—it only considered M12_pBar mode.
The issue is that nrOfVStrips() returns different values depending on the mapping mode:
- M12_pBar (bars): returns
virtualWidth()- creates multiple vertical strips - M12_sPinwheel (pinwheel) and other modes: returns
1- uses a single virtual strip
Here's the corrected fix that handles all mapping modes:
Corrected fix that works for all M12 mapping modes:
unsigned strips = SEGMENT.nrOfVStrips();
constexpr unsigned stateVarsPerStrip = 8;
unsigned dataSize = sizeof(uint32_t) * stateVarsPerStrip;
if (!SEGENV.allocateData(dataSize * strips)) return mode_static();
uint32_t* state = reinterpret_cast<uint32_t*>(SEGENV.data);
+ // Calculate per-strip length based on mapping mode
+ const uint16_t vstripLen = SEGLEN / strips;
+
// state[0] = current position (fixed point: upper 16 bits = position, lower 16 bits = fraction)
...
if (SEGENV.call == 0) {
random16_set_seed(hw_random16());
- SEGENV.aux1 = (255 << 8) / SEGLEN; // Cache the color scaling
+ SEGENV.aux1 = (255 << 8) / vstripLen; // Cache the color scaling
}
...
- uint16_t pos = (pos_fixed >> 16) % SEGLEN;
+ uint16_t pos = (pos_fixed >> 16) % vstripLen;
...
for (int8_t x = 0; x < spinnerSize; x++) {
for (uint8_t y = 0; y < spinnerSize; y++) {
- uint16_t drawPos = (pos + y) % SEGLEN;
+ uint16_t drawPos = (pos + y) % vstripLen;
int16_t drawStrip = stripNr + x;Why this works:
vstripLen = SEGLEN / stripsautomatically calculates the correct length per virtual strip- M12_pBar mode:
strips = virtualWidth(), sovstripLen = (virtualWidth() × virtualHeight()) / virtualWidth() = virtualHeight()✅ - Pinwheel mode:
strips = 1, sovstripLen = SEGLEN / 1 = SEGLEN✅
This matches how Fire 2012 and other effects handle virtual strips—they always use SEGLEN in their loops, which naturally adapts based on how many virtual strips are active.
Sorry for the incomplete initial suggestion! This should now work correctly for both M12_pBar and Pinwheel modes.
🧬 Code Graph Analysis Results
[usermods/user_fx/user_fx.cpp: Macro and usage related to virtual strips mapping]
- Summary: This section defines a macro indexToVStrip(index, stripNr) used to map a LED index within a virtual strip to a specific physical strip. It is then used in the Spinning Wheel drawing logic to set a pixel color on the appropriate virtual/physical combination. Key details:
- Macro parameters:
- index: LED index within the 1D representation of a strip.
- stripNr: The virtual strip number being addressed.
- Return value: A 32-bit value encoding both the LED index and the target virtual strip number, used by SEGMENT.setPixelColor to address the correct LED in a potentially multi-strip (1D/2D) arrangement.
- Usage context: In mode_spinning_wheel, when rendering the spinner, the code computes drawPos (LED position within a strip) and drawStrip (target strip), then calls SEGMENT.setPixelColor(indexToVStrip(drawPos, drawStrip), color) to apply the color to the appropriate LED on the correct strip.
- Important implications: This mapping underpins correct rendering across multi-strip or 2D configurations; mismatches can cause parts of the pinwheel to be missing or misaligned if vstripLen or strip counting is inconsistent.
- Macro parameters:
[usermods/user_fx/user_fx.cpp: mode_spinning_wheel drawing logic using virtual-strip mapping]
- Summary: This portion of the Spinning Wheel implementation handles rendering across virtual strips by calculating positions (pos, spinnerSize), determining color per block, and applying color to the correct LEDs using the virtual-to-physical mapping described above.
- Core behavior:
- Determines per-strip state and spinner parameters (spinnerSize, etc.).
- Computes the current LED position (pos) and color hue for each spinner block.
- Iterates over a spinnerSize x spinnerSize block, computes drawPos and drawStrip for each LED in the block, and uses indexToVStrip(drawPos, drawStrip) to set the LED color.
- Important details:
- The rendering relies on a correct virtual strip layout (nrOfVStrips) and a consistent vstrip length (vstripLen). If vstripLen is misaligned with the actual hardware layout (e.g., when using Pinwheel mode), portions of the spinner can appear missing.
- The logic uses SEGMENT.setPixelColor(indexToVStrip(drawPos, drawStrip), color) to address LEDs across the virtual/physical mapping.
- Potential impact on issues described: A mismatch between vstripLen expectations and the actual hardware arrangement (especially when switching between M12_pBar and Pinwheel options) can produce incomplete rendering of the pinwheel, as observed in the user comment.
- Core behavior:
[Note: If no additional relevant code snippets are found, respond with the above two contexts. If you need exact line-number references, please provide a version with line numbers or request a line-numbered snippet extraction.]
🧠 Learnings used
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: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Learnt from: netmindz
Repo: wled/WLED PR: 4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: netmindz
Repo: wled/WLED PR: 4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5251
File: usermods/user_fx/user_fx.cpp:146-148
Timestamp: 2026-01-01T07:19:40.244Z
Learning: In the WLED Ants effect (usermods/user_fx/user_fx.cpp), the color alternation logic `antIndex % 3 == 1` is intentional and creates a 1/3 vs 2/3 distribution between SEGCOLOR(0) and SEGCOLOR(2). This unbalanced alternation is by design and should not be changed to a balanced mod 2 alternation.
Learnt from: DedeHai
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-01-13T21:23:35.514Z
Learning: In WLED, the global `paletteBlend` variable (wled.h:603) and the `WS2812FX::paletteBlend` member (FX.h:940) are duplicates without synchronization code. The global is loaded/saved in cfg.cpp and set via UI in set.cpp, but never copied to the strip member. This is the only such case in the codebase; other settings are either strip-only members (autoSegments, correctWB, cctFromRgb, isMatrix) or global-only (gammaCorrectCol/Bri/Val, blendingStyle).
Learnt from: blazoncek
Repo: wled/WLED PR: 4995
File: wled00/FX.cpp:5209-5212
Timestamp: 2025-10-20T09:41:50.374Z
Learning: In WLED 2D effects, SEGMENT.length() returns the physical pixel count (upper bound), while SEG_W * SEG_H can be smaller depending on mapping (mirroring/grouping). For per-cell state buffers (e.g., mode_2Dgameoflife in wled00/FX.cpp), allocate using SEGMENT.length() to ensure sufficient capacity and avoid reallocations, while bounding indexing by cols*rows.
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.
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: willmmiles
Repo: wled/WLED PR: 4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
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: 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: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
Contributor
Author
There was a problem hiding this comment.
@coderabbitai I tried your fix, but then I get 2 errors when compiling. Here is one error that pertains to the uint16_t drawPos = (pos + y) % vstripLen; line: usermods/user_fx/user_fx.cpp:321:42: error: use of local variable with automatic storage from containing function
Contributor
There was a problem hiding this comment.
@BobLoeffler68 You're absolutely right! I apologize for the error. The problem is that vstripLen is a local variable in mode_spinning_wheel(), but virtualStrip::runStrip() is a static method that cannot access variables from the containing function's scope.
The fix is to pass vstripLen as a parameter to runStrip():
static uint16_t mode_spinning_wheel(void) {
if (SEGLEN < 1) return mode_static();
unsigned strips = SEGMENT.nrOfVStrips();
constexpr unsigned stateVarsPerStrip = 8;
unsigned dataSize = sizeof(uint32_t) * stateVarsPerStrip;
if (!SEGENV.allocateData(dataSize * strips)) return mode_static();
uint32_t* state = reinterpret_cast<uint32_t*>(SEGENV.data);
// Calculate per-strip length based on mapping mode
const uint16_t vstripLen = SEGLEN / strips;
// state[] index values for easier readability
constexpr unsigned CUR_POS_IDX = 0;
constexpr unsigned VELOCITY_IDX = 1;
constexpr unsigned PHASE_IDX = 2;
constexpr unsigned STOP_TIME_IDX = 3;
constexpr unsigned WOBBLE_STEP_IDX = 4;
constexpr unsigned SLOWDOWN_TIME_IDX = 5;
constexpr unsigned WOBBLE_TIME_IDX = 6;
constexpr unsigned STOP_POS_IDX = 7;
SEGMENT.fill(SEGCOLOR(1));
// Handle random seeding globally (outside the virtual strip)
if (SEGENV.call == 0) {
random16_set_seed(hw_random16());
SEGENV.aux1 = (255 << 8) / vstripLen; // Cache the color scaling
}
// ... rest of setup code ...
struct virtualStrip {
// Add vstripLen as a parameter here
static void runStrip(uint16_t stripNr, uint32_t* state, bool settingsChanged, bool allReadyToRestart, uint16_t vstripLen) {
uint8_t phase = state[PHASE_IDX];
uint32_t now = strip.now;
// ... rest of the function stays the same until we get to the drawing part ...
// Update position (phases 0 and 1 only)
if (phase == 0 || phase == 1) {
pos_fixed += velocity;
state[CUR_POS_IDX] = pos_fixed;
}
// Draw LED for all phases
uint16_t pos = (pos_fixed >> 16) % vstripLen; // Now uses the parameter
uint8_t spinnerSize = map(SEGMENT.custom1, 0, 255, 1, 10);
// ... color calculation ...
// Draw the spinner with configurable size (1-10 LEDs)
for (int8_t x = 0; x < spinnerSize; x++) {
for (uint8_t y = 0; y < spinnerSize; y++) {
uint16_t drawPos = (pos + y) % vstripLen; // Now uses the parameter
int16_t drawStrip = stripNr + x;
// Wrap horizontally if needed, or skip if out of bounds
if (drawStrip >= 0 && drawStrip < (int16_t)SEGMENT.nrOfVStrips()) {
SEGMENT.setPixelColor(indexToVStrip(drawPos, drawStrip), color);
}
}
}
}
};
// Pass vstripLen as a parameter when calling runStrip()
for (unsigned stripNr=0; stripNr<strips; stripNr++) {
uint8_t spinnerSize = map(SEGMENT.custom1, 0, 255, 1, 10);
if (stripNr % spinnerSize == 0) {
virtualStrip::runStrip(stripNr, &state[stripNr * stateVarsPerStrip], settingsChanged, allReadyToRestart, vstripLen);
}
}
return FRAMETIME;
}Summary of changes:
- Added
uint16_t vstripLenas the last parameter tovirtualStrip::runStrip() - Passed
vstripLenwhen callingrunStrip()in the for loop at the bottom - Now
vstripLenis accessible inside the static method as a parameter
This should compile correctly and work for both M12_pBar and Pinwheel modes!
🧠 Learnings used
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: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Learnt from: netmindz
Repo: wled/WLED PR: 4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: netmindz
Repo: wled/WLED PR: 4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5251
File: usermods/user_fx/user_fx.cpp:146-148
Timestamp: 2026-01-01T07:19:40.244Z
Learning: In the WLED Ants effect (usermods/user_fx/user_fx.cpp), the color alternation logic `antIndex % 3 == 1` is intentional and creates a 1/3 vs 2/3 distribution between SEGCOLOR(0) and SEGCOLOR(2). This unbalanced alternation is by design and should not be changed to a balanced mod 2 alternation.
Learnt from: DedeHai
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-01-13T21:23:35.514Z
Learning: In WLED, the global `paletteBlend` variable (wled.h:603) and the `WS2812FX::paletteBlend` member (FX.h:940) are duplicates without synchronization code. The global is loaded/saved in cfg.cpp and set via UI in set.cpp, but never copied to the strip member. This is the only such case in the codebase; other settings are either strip-only members (autoSegments, correctWB, cctFromRgb, isMatrix) or global-only (gammaCorrectCol/Bri/Val, blendingStyle).
Learnt from: blazoncek
Repo: wled/WLED PR: 4995
File: wled00/FX.cpp:5209-5212
Timestamp: 2025-10-20T09:41:50.374Z
Learning: In WLED 2D effects, SEGMENT.length() returns the physical pixel count (upper bound), while SEG_W * SEG_H can be smaller depending on mapping (mirroring/grouping). For per-cell state buffers (e.g., mode_2Dgameoflife in wled00/FX.cpp), allocate using SEGMENT.length() to ensure sufficient capacity and avoid reallocations, while bounding indexing by cols*rows.
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: DedeHai
Repo: wled/WLED PR: 4926
File: wled00/FX.cpp:4727-4730
Timestamp: 2025-11-16T19:40:46.260Z
Learning: WLED AuroraWave (wled00/FX.cpp): wave_start and wave_end intentionally use int16_t; segments longer than 32k LEDs are not supported (bounded by MAX_LEDS), so widening to 32-bit is unnecessary.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
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: 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: 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: DedeHai
Repo: wled/WLED PR: 5040
File: wled00/image_loader.cpp:84-96
Timestamp: 2025-11-14T05:48:44.673Z
Learning: In WLED (wled00/FX_2Dfcn.cpp), the Segment::setPixelColorXY() function performs internal bounds checking against vWidth() and vHeight(), returning early if coordinates are out of bounds. No additional guards are needed when calling this function, even in upscaling loops where coordinates might exceed segment dimensions.
Contributor
Author
There was a problem hiding this comment.
@coderabbitai Yep, that I what I was going to do but wanted to make sure. Also, there are two other lines that were using SEGLEN. Do they need to be changed as well?
uint16_t stop_pos = (pos_fixed >> 16) % SEGLEN;
uint16_t back_pos = (stop_pos == 0) ? SEGLEN - 1 : stop_pos - 1;
Contributor
There was a problem hiding this comment.
@BobLoeffler68 Yes, absolutely! Both of those lines need to be changed to vstripLen for the same reason as the others:
Line 1: uint16_t stop_pos = (pos_fixed >> 16) % SEGLEN;
- This calculates the stop position within a single virtual strip
- It should modulo by the strip length (
vstripLen), not the full segment length - Change to:
uint16_t stop_pos = (pos_fixed >> 16) % vstripLen;
Line 2: uint16_t back_pos = (stop_pos == 0) ? SEGLEN - 1 : stop_pos - 1;
- This calculates "one position backward" from stop_pos, wrapping around to the end of the strip if needed
- When wrapping, it should wrap to the end of the virtual strip (
vstripLen - 1), not the end of the full segment - Change to:
uint16_t back_pos = (stop_pos == 0) ? vstripLen - 1 : stop_pos - 1;
Both of these are inside the virtualStrip::runStrip() function, so they have access to the vstripLen parameter that gets passed in.
Summary of all SEGLEN → vstripLen changes needed in runStrip():
- ✅
uint16_t pos = (pos_fixed >> 16) % vstripLen; - ✅
uint16_t stop_pos = (pos_fixed >> 16) % vstripLen; - ✅
uint16_t back_pos = (stop_pos == 0) ? vstripLen - 1 : stop_pos - 1; - ✅
uint16_t drawPos = (pos + y) % vstripLen;
And at function scope:
5. ✅ const uint16_t vstripLen = SEGLEN / strips; (right after strips definition)
6. ✅ SEGENV.aux1 = (255 << 8) / vstripLen; (in the SEGENV.call == 0 block)
This will ensure all position calculations are relative to individual strip length, not the full segment area! 🎯
🧠 Learnings used
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: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Learnt from: netmindz
Repo: wled/WLED PR: 4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: netmindz
Repo: wled/WLED PR: 4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5251
File: usermods/user_fx/user_fx.cpp:146-148
Timestamp: 2026-01-01T07:19:40.244Z
Learning: In the WLED Ants effect (usermods/user_fx/user_fx.cpp), the color alternation logic `antIndex % 3 == 1` is intentional and creates a 1/3 vs 2/3 distribution between SEGCOLOR(0) and SEGCOLOR(2). This unbalanced alternation is by design and should not be changed to a balanced mod 2 alternation.
Learnt from: DedeHai
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-01-13T21:23:35.514Z
Learning: In WLED, the global `paletteBlend` variable (wled.h:603) and the `WS2812FX::paletteBlend` member (FX.h:940) are duplicates without synchronization code. The global is loaded/saved in cfg.cpp and set via UI in set.cpp, but never copied to the strip member. This is the only such case in the codebase; other settings are either strip-only members (autoSegments, correctWB, cctFromRgb, isMatrix) or global-only (gammaCorrectCol/Bri/Val, blendingStyle).
Learnt from: blazoncek
Repo: wled/WLED PR: 4995
File: wled00/FX.cpp:5209-5212
Timestamp: 2025-10-20T09:41:50.374Z
Learning: In WLED 2D effects, SEGMENT.length() returns the physical pixel count (upper bound), while SEG_W * SEG_H can be smaller depending on mapping (mirroring/grouping). For per-cell state buffers (e.g., mode_2Dgameoflife in wled00/FX.cpp), allocate using SEGMENT.length() to ensure sufficient capacity and avoid reallocations, while bounding indexing by cols*rows.
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: 4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
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: 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: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: DedeHai
Repo: wled/WLED PR: 4926
File: wled00/FX.cpp:4727-4730
Timestamp: 2025-11-16T19:40:46.260Z
Learning: WLED AuroraWave (wled00/FX.cpp): wave_start and wave_end intentionally use int16_t; segments longer than 32k LEDs are not supported (bounded by MAX_LEDS), so widening to 32-bit is unnecessary.
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: 4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Contributor
Author
There was a problem hiding this comment.
@coderabbitai Passing the vstripLen parameter to runStrip() did not work. I just see a full row of LEDs lit up and it never moves.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
@BobLoeffler68 Let me investigate what might be causing the full row of LEDs to light up. I need to see the current implementation to diagnose the issue.
🏁 Script executed:
#!/bin/bash
# Get the current implementation of mode_spinning_wheel to see what's happening
sed -n '/static uint16_t mode_spinning_wheel/,/^static const char _data_FX_MODE_SPINNINGWHEEL/p' usermods/user_fx/user_fx.cpp
Length of output: 8866
@BobLoeffler68 I see the issue! Looking at your current code, the vstripLen changes weren't fully applied (you've reverted back to using SEGLEN everywhere). The "full row lit up" issue likely happened because vstripLen wasn't properly defined at function scope or there was a division issue.
Here's the complete, working solution with proper scope and safety checks:
static uint16_t mode_spinning_wheel(void) {
if (SEGLEN < 1) return mode_static();
unsigned strips = SEGMENT.nrOfVStrips();
if (strips == 0) return mode_static(); // Safety check to prevent division by zero
const uint16_t vstripLen = SEGLEN / strips; // ← Define here at function scope (line ~98)
constexpr unsigned stateVarsPerStrip = 8;
unsigned dataSize = sizeof(uint32_t) * stateVarsPerStrip;
if (!SEGENV.allocateData(dataSize * strips)) return mode_static();
uint32_t* state = reinterpret_cast<uint32_t*>(SEGENV.data);
// state comments...
// state[] index values...
constexpr unsigned CUR_POS_IDX = 0;
constexpr unsigned VELOCITY_IDX = 1;
constexpr unsigned PHASE_IDX = 2;
constexpr unsigned STOP_TIME_IDX = 3;
constexpr unsigned WOBBLE_STEP_IDX = 4;
constexpr unsigned SLOWDOWN_TIME_IDX = 5;
constexpr unsigned WOBBLE_TIME_IDX = 6;
constexpr unsigned STOP_POS_IDX = 7;
SEGMENT.fill(SEGCOLOR(1));
// Handle random seeding globally (outside the virtual strip)
if (SEGENV.call == 0) {
random16_set_seed(hw_random16());
SEGENV.aux1 = (255 << 8) / vstripLen; // ← Change from SEGLEN to vstripLen
}
// ... rest of settings check code stays the same ...
struct virtualStrip {
// ← Add vstripLen as the last parameter
static void runStrip(uint16_t stripNr, uint32_t* state, bool settingsChanged, bool allReadyToRestart, uint16_t vstripLen) {
uint8_t phase = state[PHASE_IDX];
uint32_t now = strip.now;
// ... needsReset and initialization code stays the same until line 168 ...
// In phase == 1 block, change line ~168:
if (velocity < 2000) {
velocity = 0;
state[VELOCITY_IDX] = 0;
phase = 2;
state[PHASE_IDX] = 2;
state[WOBBLE_STEP_IDX] = 0;
uint16_t stop_pos = (pos_fixed >> 16) % vstripLen; // ← Change from SEGLEN
state[STOP_POS_IDX] = stop_pos;
state[WOBBLE_TIME_IDX] = now;
}
// In phase == 2 block, change line ~179:
if (wobble_step == 0 && elapsed >= 200) {
// Move back one LED from stop position
uint16_t back_pos = (stop_pos == 0) ? vstripLen - 1 : stop_pos - 1; // ← Change from SEGLEN
pos_fixed = ((uint32_t)back_pos) << 16;
// ... rest stays same ...
}
// ... rest of phase 2 code stays the same ...
// Update position (phases 0 and 1 only)
if (phase == 0 || phase == 1) {
pos_fixed += velocity;
state[CUR_POS_IDX] = pos_fixed;
}
// Draw LED for all phases - change line ~195:
uint16_t pos = (pos_fixed >> 16) % vstripLen; // ← Change from SEGLEN
uint8_t spinnerSize = map(SEGMENT.custom1, 0, 255, 1, 10);
// ... color calculation stays the same ...
// Draw the spinner - change line ~218:
for (int8_t x = 0; x < spinnerSize; x++) {
for (uint8_t y = 0; y < spinnerSize; y++) {
uint16_t drawPos = (pos + y) % vstripLen; // ← Change from SEGLEN
int16_t drawStrip = stripNr + x;
if (drawStrip >= 0 && drawStrip < (int16_t)SEGMENT.nrOfVStrips()) {
SEGMENT.setPixelColor(indexToVStrip(drawPos, drawStrip), color);
}
}
}
}
};
// At the bottom, pass vstripLen when calling runStrip:
for (unsigned stripNr=0; stripNr<strips; stripNr++) {
uint8_t spinnerSize = map(SEGMENT.custom1, 0, 255, 1, 10);
if (stripNr % spinnerSize == 0) {
virtualStrip::runStrip(stripNr, &state[stripNr * stateVarsPerStrip], settingsChanged, allReadyToRestart, vstripLen); // ← Add vstripLen parameter
}
}
return FRAMETIME;
}Summary of all 6 changes needed:
- Line ~95: Add safety check:
if (strips == 0) return mode_static(); - Line ~96: Define
const uint16_t vstripLen = SEGLEN / strips;(right after strips, at function scope) - Line ~147: Change
SEGENV.aux1 = (255 << 8) / vstripLen; - runStrip signature: Add
uint16_t vstripLenas last parameter - Inside runStrip: Change all 4 uses of
SEGLENtovstripLen:- Line ~168:
uint16_t stop_pos = (pos_fixed >> 16) % vstripLen; - Line ~179:
uint16_t back_pos = (stop_pos == 0) ? vstripLen - 1 : stop_pos - 1; - Line ~195:
uint16_t pos = (pos_fixed >> 16) % vstripLen; - Line ~218:
uint16_t drawPos = (pos + y) % vstripLen;
- Line ~168:
- Line ~332: Pass
vstripLenin the runStrip call
The key was defining vstripLen at function scope (right after strips) so it's accessible everywhere!
🧬 Code Graph Analysis Results
[Relevant snippets from: usermods/user_fx/user_fx.cpp]
Filename: usermods/user_fx/user_fx.cpp
Line range: 131-206
Content (snippet: virtualStrip struct with runStrip function)
struct virtualStrip {
static void runStrip(uint16_t stripNr, uint32_t* state, bool settingsChanged, bool allReadyToRestart) {
uint8_t phase = state[PHASE_IDX];
uint32_t now = strip.now;
// Check for restart conditions
bool needsReset = false;
if (SEGENV.call == 0) {
needsReset = true;
} else if (settingsChanged) {
needsReset = true;
} else if (phase == 3 && state[STOP_TIME_IDX] != 0) {
// If synchronized restart is enabled, only restart when all strips are ready
if (SEGMENT.check3) {
if (allReadyToRestart) {
needsReset = true;
}
} else {
// Normal mode: restart after individual strip delay
uint16_t spin_delay = map(SEGMENT.custom3, 0, 31, 2000, 15000);
if ((now - state[STOP_TIME_IDX]) >= spin_delay) {
needsReset = true;
}
}
}
// Initialize or restart
if (needsReset) {
state[CUR_POS_IDX] = 0;
// Set velocity
uint16_t speed = map(SEGMENT.speed, 0, 255, 300, 800);
if (speed == 300) { // random speed (user selected 0 on speed slider)
state[VELOCITY_IDX] = random16(200, 900) * 655; // fixed-point velocity scaling (approx. 65536/100)
} else {
state[VELOCITY_IDX] = random16(speed - 100, speed + 100) * 655;
}
// Set slowdown start time
uint16_t slowdown = map(SEGMENT.intensity, 0, 255, 3000, 5000);
if (slowdown == 3000) { // random slowdown start time (user selected 0 on intensity slider)
state[SLOWDOWN_TIME_IDX] = now + random16(2000, 6000);
} else {
state[SLOWDOWN_TIME_IDX] = now + random16(slowdown - 1000, slowdown + 1000);
}
state[PHASE_IDX] = 0;
state[STOP_TIME_IDX] = 0;
state[WOBBLE_STEP_IDX] = 0;
state[WOBBLE_TIME_IDX] = 0;
state[STOP_POS_IDX] = 0; // Initialize stop position
phase = 0;
}
uint32_t pos_fixed = state[CUR_POS_IDX];
uint32_t velocity = state[VELOCITY_IDX];
// Phase management
if (phase == 0) {
// Fast spinning phase
if ((int32_t)(now - state[SLOWDOWN_TIME_IDX]) >= 0) {
phase = 1;
state[PHASE_IDX] = 1;
}
} else if (phase == 1) {
// Slowing phase - apply deceleration
uint32_t decel = velocity / 80;
if (decel < 100) decel = 100;
velocity = (velocity > decel) ? velocity - decel : 0;
state[VELOCITY_IDX] = velocity;
// Check if stopped
if (velocity < 2000) {
velocity = 0;
state[VELOCITY_IDX] = 0;
phase = 2;
state[PHASE_IDX] = 2;
state[WOBBLE_STEP_IDX] = 0;
uint16_t stop_pos = (pos_fixed >> 16) % SEGLEN;
state[STOP_POS_IDX] = stop_pos;
state[WOBBLE_TIME_IDX] = now;
}
} else if (phase == 2) {
// Wobble phase (moves the LED back one and then forward one)
uint32_t wobble_step = state[WOBBLE_STEP_IDX];
uint16_t stop_pos = state[STOP_POS_IDX];
uint32_t elapsed = now - state[WOBBLE_TIME_IDX];
if (wobble_step == 0 && elapsed >= 200) {
// Move back one LED from stop position
uint16_t back_pos = (stop_pos == 0) ? SEGLEN - 1 : stop_pos - 1;
pos_fixed = ((uint32_t)back_pos) << 16;
state[CUR_POS_IDX] = pos_fixed;
state[WOBBLE_STEP_IDX] = 1;
state[WOBBLE_TIME_IDX] = now;
} else if (wobble_step == 1 && elapsed >= 400) {
// Move forward to the stop position
pos_fixed = ((uint32_t)stop_pos) << 16;
state[CUR_POS_IDX] = pos_fixed;
state[WOBBLE_STEP_IDX] = 2;
state[WOBBLE_TIME_IDX] = now;
} else if (wobble_step == 2 && elapsed >= 300) {
// Wobble complete, enter stopped phase
phase = 3;
state[PHASE_IDX] = 3;
state[STOP_TIME_IDX] = now;
}
}
// Update position (phases 0 and 1 only)
if (phase == 0 || phase == 1) {
pos_fixed += velocity;
state[CUR_POS_IDX] = pos_fixed;
}
// Draw LED for all phases
uint16_t pos = (pos_fixed >> 16) % SEGLEN;
uint8_t spinnerSize = map(SEGMENT.custom1, 0, 255, 1, 10);
// Calculate color once per spinner block (based on strip number, not position)
uint8_t hue;
if (SEGMENT.check2) {
// Each spinner block gets its own color based on strip number
uint16_t numSpinners = max(1U, (SEGMENT.nrOfVStrips() + spinnerSize - 1) / spinnerSize);
hue = (255 * (stripNr / spinnerSize)) / numSpinners;
} else {
// Color changes with position
hue = (SEGENV.aux1 * pos) >> 8;
}
uint32_t color = SEGMENT.check1 ? SEGMENT.color_wheel(hue) : SEGMENT.color_from_palette(hue, true, PALETTE_SOLID_WRAP, 0);
// Draw the spinner with configurable size (1-10 LEDs)
for (int8_t x = 0; x < spinnerSize; x++) {
for (uint8_t y = 0; y < spinnerSize; y++) {
uint16_t drawPos = (pos + y) % SEGLEN;
int16_t drawStrip = stripNr + x;
// Wrap horizontally if needed, or skip if out of bounds
if (drawStrip >= 0 && drawStrip < (int16_t)SEGMENT.nrOfVStrips()) {
SEGMENT.setPixelColor(indexToVStrip(drawPos, drawStrip), color);
}
}
}
}
};
Filename: usermods/user_fx/user_fx.cpp
Line range: 208-234
Content (snippet: invocation of runStrip in the animation loop)
for (unsigned stripNr=0; stripNr<strips; stripNr++) {
// Only run on strips that are multiples of spinnerSize to avoid overlap
uint8_t spinnerSize = map(SEGMENT.custom1, 0, 255, 1, 10);
if (stripNr % spinnerSize == 0) {
virtualStrip::runStrip(stripNr, &state[stripNr * stateVarsPerStrip], settingsChanged, allReadyToRestart);
}
}
[End of relevant snippets]
🧠 Learnings used
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: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Learnt from: netmindz
Repo: wled/WLED PR: 4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: netmindz
Repo: wled/WLED PR: 4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5251
File: usermods/user_fx/user_fx.cpp:146-148
Timestamp: 2026-01-01T07:19:40.244Z
Learning: In the WLED Ants effect (usermods/user_fx/user_fx.cpp), the color alternation logic `antIndex % 3 == 1` is intentional and creates a 1/3 vs 2/3 distribution between SEGCOLOR(0) and SEGCOLOR(2). This unbalanced alternation is by design and should not be changed to a balanced mod 2 alternation.
Learnt from: DedeHai
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-01-13T21:23:35.514Z
Learning: In WLED, the global `paletteBlend` variable (wled.h:603) and the `WS2812FX::paletteBlend` member (FX.h:940) are duplicates without synchronization code. The global is loaded/saved in cfg.cpp and set via UI in set.cpp, but never copied to the strip member. This is the only such case in the codebase; other settings are either strip-only members (autoSegments, correctWB, cctFromRgb, isMatrix) or global-only (gammaCorrectCol/Bri/Val, blendingStyle).
Learnt from: blazoncek
Repo: wled/WLED PR: 4995
File: wled00/FX.cpp:5209-5212
Timestamp: 2025-10-20T09:41:50.374Z
Learning: In WLED 2D effects, SEGMENT.length() returns the physical pixel count (upper bound), while SEG_W * SEG_H can be smaller depending on mapping (mirroring/grouping). For per-cell state buffers (e.g., mode_2Dgameoflife in wled00/FX.cpp), allocate using SEGMENT.length() to ensure sufficient capacity and avoid reallocations, while bounding indexing by cols*rows.
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: 4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
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: 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: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: DedeHai
Repo: wled/WLED PR: 4926
File: wled00/FX.cpp:4727-4730
Timestamp: 2025-11-16T19:40:46.260Z
Learning: WLED AuroraWave (wled00/FX.cpp): wave_start and wave_end intentionally use int16_t; segments longer than 32k LEDs are not supported (bounded by MAX_LEDS), so widening to 32-bit is unnecessary.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
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: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
Learnt from: DedeHai
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1176-1187
Timestamp: 2025-09-16T18:08:42.848Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), intentionally allowing memory overruns when bus configurations exceed MAX_LED_MEMORY is a deliberate design choice. The trade-off prioritizes creating buses with reduced LED counts over completely failing to create buses, which would cause no LED output and UI failures. This approach forces users to update configurations after migrating to version 0.16 while maintaining basic functionality.
Learnt from: willmmiles
Repo: wled/WLED PR: 4890
File: lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h:173-180
Timestamp: 2025-09-02T01:56:43.841Z
Learning: willmmiles prefers to maintain consistency with upstream NeoPixelBus patterns (like unchecked malloc in construct() methods) rather than diverging until improvements are made upstream first, to minimize maintenance burden and keep the codebase aligned.
Contributor
Author
There was a problem hiding this comment.
@coderabbitai I had not pushed my latest code yet, so you were using old code. I just pushed the latest code with your suggestions, but it still does not work. Please check the code to see why. Thx.
…not working correctly now. Committing and pushing so coderabbitai can check the latest code.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@usermods/user_fx/user_fx.cpp`:
- Around line 118-121: After computing vstripLen = SEGLEN / strips in
user_fx.cpp, add a defensive guard for vstripLen == 0 to avoid later
division/modulo by zero; either clamp strips to at most SEGLEN (e.g. strips =
min<uint16_t>(strips, SEGLEN)) before the division or check immediately after
assignment and return mode_static() (or otherwise handle the error) if vstripLen
== 0. Update the code paths that rely on vstripLen (references: vstripLen,
strips, SEGLEN, and functions/modes that use them such as mode_static()) so no
subsequent division or modulo uses a zero divisor.
🧹 Nitpick comments (1)
usermods/user_fx/user_fx.cpp (1)
333-339: Consider hoistingspinnerSizecalculation outside the loop.The
map()call at line 335 is repeated on every iteration but the result is constant within the frame. Minor optimization opportunity.♻️ Suggested refactor
+ uint8_t spinnerSize = map(SEGMENT.custom1, 0, 255, 1, 10); for (unsigned stripNr=0; stripNr<strips; stripNr++) { - // Only run on strips that are multiples of spinnerSize to avoid overlap - uint8_t spinnerSize = map(SEGMENT.custom1, 0, 255, 1, 10); if (stripNr % spinnerSize == 0) { virtualStrip::runStrip(stripNr, &state[stripNr * stateVarsPerStrip], settingsChanged, allReadyToRestart, vstripLen); } }
Comment on lines
+118
to
+121
| if (strips == 0) return mode_static(); | ||
|
|
||
| const uint16_t vstripLen = SEGLEN / strips; | ||
|
|
Contributor
There was a problem hiding this comment.
Add defensive guard for vstripLen == 0 to prevent division/modulo by zero.
If SEGLEN < strips (unusual but possible with certain configurations), vstripLen becomes 0, causing division by zero at line 150 and modulo by zero at lines 263, 301, 321.
🛡️ Suggested defensive fix
if (strips == 0) return mode_static();
const uint16_t vstripLen = SEGLEN / strips;
+ if (vstripLen == 0) return mode_static();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (strips == 0) return mode_static(); | |
| const uint16_t vstripLen = SEGLEN / strips; | |
| if (strips == 0) return mode_static(); | |
| const uint16_t vstripLen = SEGLEN / strips; | |
| if (vstripLen == 0) return mode_static(); |
🤖 Prompt for AI Agents
In `@usermods/user_fx/user_fx.cpp` around lines 118 - 121, After computing
vstripLen = SEGLEN / strips in user_fx.cpp, add a defensive guard for vstripLen
== 0 to avoid later division/modulo by zero; either clamp strips to at most
SEGLEN (e.g. strips = min<uint16_t>(strips, SEGLEN)) before the division or
check immediately after assignment and return mode_static() (or otherwise handle
the error) if vstripLen == 0. Update the code paths that rely on vstripLen
(references: vstripLen, strips, SEGLEN, and functions/modes that use them such
as mode_static()) so no subsequent division or modulo uses a zero divisor.
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.
This PR will add the Spinning Wheel effect into the new user_fx usermod instead of FX.cpp
Fun ideas on a 32x32 2D matrix:
Kaleidoscope effect
Effect settings: Spinner size 1, enable Color Mode checkbox, select Rainbow (or any) palette
Segment settings: enable X mirror, enable Y mirror, grouping 1, spacing 0, and Expand 1D FX = Bar
Slot machine effect
Effect settings: Spinner size 4, enable Color Mode checkbox, select Rainbow (or any) palette, enable Color per block checkbox (or not)
Segment settings: default settings and Expand 1D FX = Bar
Wheel of Fortune effect
Effect settings: Spinner size 1, enable Color Mode checkbox, select Rainbow (or any) palette
Segment settings: default settings and Expand 1D FX = Pinwheel
Example videos:
https://github.com/user-attachments/assets/65d517e0-ce83-4de2-bb77-cb8f57342fb1 (1D strip)
https://github.com/user-attachments/assets/9b616281-54fc-4177-ad46-739a1fdb8796 (2D kaleidoscope)
https://github.com/user-attachments/assets/036aef22-1cfd-48d7-b205-204b6a77f034 (2D slot machine)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.