Adding Shimmer FX #4923
Adding Shimmer FX #4923
Conversation
Sends a shimmer accross the strip at defined (or random) intervals Optional brightness modulators: sine or perlin noise Can be used as an overlay to other effects.
WalkthroughAdds a new 1D "Shimmer" effect implementation, metadata descriptor, and registers it in the FX catalog. Assigns FX mode ID 161 to SHIMMER and comments out the previous 2D mode at that ID. No other effects changed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
✨ Finishing touches
🧪 Generate unit tests
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 Pre-merge checks✅ Passed checks (3 passed)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
wled00/FX.cpp (3)
4909-4911: Guard step calc against rare 32-bit overflow.speed*deltaTime can approach the 32-bit edge on very long strips/slow frames. Do the multiply in 64-bit; cost is negligible.
- int32_t step = 1 + ((speed * deltaTime) >> 5); // subpixels moved this frame. note >>5 as speed is in subpixels/512ms + uint32_t step = 1 + (uint32_t)(((uint64_t)speed * (uint64_t)deltaTime) >> 5); // 64-bit multiply to avoid overflow
4890-4890: Fix the units in the comment for ‘speed’.It’s a Q5 fixed-point “subpixels/ms” value (distance*32/time), not “subpixels/512ms”.
- uint32_t speed = ((traversalDistance << 5) / traversalTime); // subpixels/512ms + uint32_t speed = ((traversalDistance << 5) / traversalTime); // Q5 fixed-point subpixels/ms (distance*32 / time)
4927-4932: Avoid abs() overload ambiguity on 32-bit values.Compute absolute distance explicitly to keep types clear and avoid signed/unsigned surprises.
- uint32_t dist = abs(position - (i << 8)); + int32_t d = position - int32_t(i << 8); + uint32_t dist = d < 0 ? uint32_t(-d) : uint32_t(d);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/FX.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware files
Files:
wled00/FX.cpp
🧠 Learnings (9)
📓 Common learnings
Learnt from: DedeHai
PR: wled/WLED#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.
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
PR: wled/WLED#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:
wled00/FX.cpp
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
PR: wled/WLED#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:
wled00/FX.cpp
📚 Learning: 2025-04-30T05:41:03.633Z
Learnt from: blazoncek
PR: wled/WLED#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:
wled00/FX.cpp
📚 Learning: 2025-09-12T17:29:43.793Z
Learnt from: DedeHai
PR: wled/WLED#4923
File: wled00/FX.cpp:4883-4901
Timestamp: 2025-09-12T17:29:43.793Z
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:
wled00/FX.cpp
📚 Learning: 2025-08-31T03:38:14.114Z
Learnt from: BobLoeffler68
PR: wled/WLED#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:
wled00/FX.cpp
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
PR: wled/WLED#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:
wled00/FX.cpp
📚 Learning: 2025-05-09T18:48:21.296Z
Learnt from: DedeHai
PR: wled/WLED#4682
File: wled00/FX.cpp:9016-9039
Timestamp: 2025-05-09T18:48:21.296Z
Learning: WLED does not support matrices large enough (≥1024 px) for 32-bit overflow in dx*dx + dy*dy to be a practical concern in particle system effects.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2
PR: wled/WLED#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:
wled00/FX.cpp
🧬 Code graph analysis (1)
wled00/FX.cpp (3)
wled00/util.cpp (10)
hw_random(618-622)hw_random(618-618)hw_random(624-630)hw_random(624-624)perlin16(1011-1013)perlin16(1011-1011)perlin16(1015-1017)perlin16(1015-1015)perlin16(1019-1021)perlin16(1019-1019)wled00/wled_math.cpp (2)
sin16_t(65-76)sin16_t(65-65)wled00/colors.cpp (4)
color_fade(67-87)color_fade(67-67)color_blend(11-21)color_blend(11-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
🔇 Additional comments (3)
wled00/FX.cpp (3)
4892-4901: Nice: packed re-init guard avoids sum collisions.Using a 16-bit packed key for intensity/custom1 in SEGENV.aux1 hardens re-init vs. JSON/preset updates. Good call.
4949-4949: UI metadata: ‘Flow’/‘Zebra’ labels don’t match code.Code only uses one checkbox (SEGMENT.check1) to switch between sine “Zebra” and Perlin modulators; the string defines both “Flow” and “Zebra” (two checkboxes), plus Reverse/Sporadic. Consider dropping “Flow” or wiring a second behavior to it to avoid a dangling control in the UI.
If you prefer to simplify now:
-static const char _data_FX_MODE_SHIMMER[] PROGMEM = "Shimmer@Speed,Interval,Size,Granular,Flow,Zebra,Reverse,Sporadic;Fx,Bg,Cx;!;1;pal=15,sx=220,ix=10,c2=0,c3=0"; +static const char _data_FX_MODE_SHIMMER[] PROGMEM = "Shimmer@Speed,Interval,Size,Granular,Zebra,Reverse,Sporadic;Fx,Bg,Cx;!;1;pal=15,sx=220,ix=10,c2=0,c3=0";Else, we can add a “Flow” behavior (e.g., time-scrolling the modulator) and map it to a second check. Happy to draft that if you want.
10880-10881: Effect registration looks correct.SHIMMER is appended among 1D effects and uses its own metadata; no clashes observed.
| uint32_t deltaTime = (strip.now - *lastTime) & 0x7F; // clamp to 127ms to avoid overflows. note: speed*deltaTime can still overflow for segments > ~10k pixels | ||
| *lastTime = strip.now; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clamp deltaTime — don’t bitmask it.
Using “& 0x7F” folds long frames instead of capping them, causing time-warp/jitter after stalls. Saturate instead.
Apply:
- uint32_t deltaTime = (strip.now - *lastTime) & 0x7F; // clamp to 127ms to avoid overflows. note: speed*deltaTime can still overflow for segments > ~10k pixels
- *lastTime = strip.now;
+ uint32_t deltaTime = strip.now - *lastTime;
+ if (deltaTime > 127u) deltaTime = 127u; // cap to 127ms to avoid overflows; preserves elapsed time on long frames
+ *lastTime = strip.now;📝 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.
| uint32_t deltaTime = (strip.now - *lastTime) & 0x7F; // clamp to 127ms to avoid overflows. note: speed*deltaTime can still overflow for segments > ~10k pixels | |
| *lastTime = strip.now; | |
| uint32_t deltaTime = strip.now - *lastTime; | |
| if (deltaTime > 127u) deltaTime = 127u; // cap to 127ms to avoid overflows; preserves elapsed time on long frames | |
| *lastTime = strip.now; |
🤖 Prompt for AI Agents
In wled00/FX.cpp around lines 4903-4905, the code uses "& 0x7F" to limit
deltaTime which folds large intervals instead of capping them; replace the
bitmask with saturation: compute the raw difference (strip.now - *lastTime) into
a uint32_t, then if that value is greater than 127 set deltaTime = 127 else set
deltaTime = the raw value, and finally update *lastTime = strip.now; this
ensures deltaTime is clamped to 127ms rather than wrapped.
There was a problem hiding this comment.
its just a safety precaution, the limit does not engage in normal use.
There was a problem hiding this comment.
Oops, something went wrong! Please try again later. 🐰 💔
|
@DedeHai this effect is a perfect example for use of pixel transparency. Just a thought. EDIT: BTW I was wondering why did you choose 256 subpixel levels? IMO 4 or 8 are enough in every instance, if you want to g crazy then 16 but 256 is an overkill. |
|
a subpixel size of 256 ensures good resolution for short segments and slow speeds. I chose the code structure this way to make it easier to understand, there would be other more convoluted ways like it was in the original PR. If you lower the subpixel steps, min speed is capped for segments < 10 pixels. in what way would using transparency differ from using a black background and "add" overlay? |
IMO It is not easier.
Virtual "pixel" can only occupy at most 2 physical pixel and at least 1 physical pixel. If it occupies 2, then both are semi-transparent. Unless you want blurring in which case there are even more reasons for opacity. |
Sends a shimmer accross the strip at defined (or random) intervals. Optional brightness modulators: sine or perlin noise. Can be used as an overlay to other effects.
based on an idea from @Charming-Lime, see #4905
Summary by CodeRabbit
New Features
Chores