Add Cycle mode to Twinkle #5004
Conversation
WalkthroughAdds a "Cycle mode" branch to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)wled00/**/*.cpp📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧠 Learnings (3)📚 Learning: 2025-09-02T19:42:57.475ZApplied to files:
📚 Learning: 2025-08-26T11:51:21.817ZApplied to files:
📚 Learning: 2025-04-30T05:41:03.633ZApplied to files:
🧬 Code graph analysis (1)wled00/FX.cpp (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
wled00/FX.cpp (1)
678-684: Reinitialize when Cycle mode is toggled at runtimeState init runs only on first effect call. Enabling Cycle mode mid‑effect won’t clear the bitset, counters, or background fill, so the cycle can start in an arbitrary state. Track the prior value of check1 and reinit when it changes to 1.
You can memoize the last toggle in a spare aux field and reinitialize:
- // Initialize on first call - if (SEGENV.call == 0) + // Initialize on first call or when Cycle mode is newly enabled + if (SEGENV.call == 0 || (SEGENV.aux2 != 1)) { SEGENV.aux0 = 0; // Count of LEDs switched in current cycle SEGENV.aux1 = 0; // Direction: 0 = switching to new color, 1 = switching back memset(SEGENV.data, 0, dataSize); // Clear all bits SEGMENT.fill(SEGCOLOR(1)); // Start with secondary color + SEGENV.aux2 = 1; // mark cycle mode initialized } + // Optional: clear the marker when cycle mode is disabled (outside this branch)If aux2 is unavailable in your SEGENV, pick any unused SEGENV field or pack state into one of the existing aux fields. Based on patterns elsewhere (e.g., mode_shimmer), tracking “input state” in a combined word is also acceptable.
Also applies to: 694-701
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/FX.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/FX.cpp
🧠 Learnings (3)
📚 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-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
📚 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
🧬 Code graph analysis (1)
wled00/FX.cpp (2)
wled00/fcn_declare.h (4)
hw_random16(430-430)hw_random16(431-431)hw_random16(432-432)hw_random(427-427)wled00/util.cpp (4)
hw_random(618-622)hw_random(618-618)hw_random(624-630)hw_random(624-624)
⏰ 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). (14)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
🔇 Additional comments (1)
wled00/FX.cpp (1)
763-763: Confirm UI mapping for “Cycle mode” uses SEGMENT.check1The metadata string adds “Cycle mode” as a checkbox label. Please confirm it maps to SEGMENT.check1 for TWINKLE (and that TWINKLE didn’t previously use check1). If TWINKLE had no checkboxes before, this is fine.
wled00/FX.cpp
Outdated
| // Initialize on first call | ||
| if (SEGENV.call == 0) | ||
| if ((SEGENV.call == 0) || (SEGENV.aux1 > 1)) | ||
| { |
There was a problem hiding this comment.
The suggested change was to add a value to the non-existent aux2. This method assumes that the aux1 value was set by hw_random in the original code on line 750 to be some value (hopefully greater than 1).
Even if the data is not initialized, once the cycle completes, it gets initialized and works fine.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
wled00/FX.cpp (1)
668-672: Tiny-segment guard added — good fixEarly return for SEGLEN <= 1 prevents zero-byte allocation and OOB bit access. Matches the prior recommendation.
🧹 Nitpick comments (1)
wled00/FX.cpp (1)
674-737: Cycle mode logic solid; add a starvation fallbackAllocation + 1-bit-per-LED tracking is correct, and init/flip/reset are clean. Rarely, the random-pick loop may exhaust maxAttempts without finding a free bit, stalling progress. Force a flip when that happens.
- while (attempts < maxAttempts) + while (attempts < maxAttempts) { unsigned j = hw_random16(SEGLEN); unsigned index = j >> 3; unsigned bitNum = j & 0x07; if ( ! bitRead(SEGENV.data[index], bitNum)) { // This LED hasn't been switched yet bitWrite(SEGENV.data[index], bitNum, true); SEGENV.aux0++; if (SEGENV.aux1 == 0) { // Switching to primary color SEGMENT.setPixelColor(j, SEGMENT.color_from_palette(j, true, PALETTE_SOLID_WRAP, 0)); } else { // Switching back to secondary color SEGMENT.setPixelColor(j, SEGCOLOR(1)); } break; } attempts++; } + if (attempts >= maxAttempts) { + // Fail-safe: complete the cycle to avoid getting stuck + SEGENV.aux0 = SEGLEN; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/FX.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/FX.cpp
🧠 Learnings (4)
📚 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-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
📚 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
🧬 Code graph analysis (1)
wled00/FX.cpp (3)
usermods/usermod_v2_animartrix/usermod_v2_animartrix.cpp (1)
SEGENV(64-76)wled00/fcn_declare.h (4)
hw_random16(430-430)hw_random16(431-431)hw_random16(432-432)hw_random(427-427)wled00/util.cpp (4)
hw_random(618-622)hw_random(618-618)hw_random(624-630)hw_random(624-624)
⏰ 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). (1)
- GitHub Check: wled_build / Build Environments (usermods)
🔇 Additional comments (1)
wled00/FX.cpp (1)
769-769: Metadata: confirm Intensity is disabled when Cycle mode is ON“Twinkle@!,!,,,,Cycle mode;...,;;m12=0” adds the checkbox, but disabling the Intensity slider typically needs explicit UI mapping. Please verify the UI hides/disables Intensity when SEGMENT.check1 is true.
Run-time check suggestion:
- Toggle Cycle mode in UI and confirm Intensity slider is disabled/ignored.
- If needed, adjust the metadata mapping (e.g., conditional disable flag for Intensity).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
wled00/FX.cpp (2)
706-739: Rejection sampling can stutter near cycle end; consider deterministic permutationThe while loop retries up to SEGLEN*2 to find an unvisited LED. As the bitset fills, misses become frequent, causing uneven timing on long segments. Prefer a per-cycle permutation:
- On init/flip, generate a shuffled index list (e.g., Fisher–Yates) and consume sequentially each tick; or
- Choose a jump step co-prime to SEGLEN (LCG-style stride), visiting each index exactly once without extra memory.
Both guarantee O(1) per frame and eliminate retries. If memory is tight, the co-prime stride variant uses constant space.
- // Switch one random LED that hasn't been switched yet - unsigned attempts = 0; - unsigned maxAttempts = SEGLEN * 2; // Avoid infinite loop - while (attempts < maxAttempts) - { - unsigned j = hw_random16(SEGLEN); - ... - attempts++; - } + // Switch next LED from a precomputed permutation (or stride sequence) + // Example: stride sequence with co-prime 'step' stored in aux2 + uint16_t j = (SEGENV.aux2 + SEGENV.aux0 * SEGENV.custom1 /* step */) % SEGLEN; + // Ensure j is unique per frame by construction; then mark/set it
761-769: Use uint16_t for PRNG to reflect 16‑bit LCG intentPRNG16 is effectively 16-bit. Using uint16_t avoids implicit widening and clarifies intent.
- unsigned PRNG16 = SEGENV.aux1; + uint16_t PRNG16 = (uint16_t)SEGENV.aux1;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/FX.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/FX.cpp
🧠 Learnings (3)
📚 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
📚 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-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
🧬 Code graph analysis (1)
wled00/FX.cpp (2)
wled00/fcn_declare.h (4)
hw_random16(430-430)hw_random16(431-431)hw_random16(432-432)hw_random(427-427)wled00/util.cpp (4)
hw_random(618-622)hw_random(618-618)hw_random(624-630)hw_random(624-624)
⏰ 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). (2)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
🔇 Additional comments (3)
wled00/FX.cpp (3)
669-673: Good tiny-segment guard avoids OOB/zero-alloc pitfallsEarly-return on SEGLEN <= 1 prevents allocateData(0) and bitset indexing. This addresses the prior OOB risk when SEGLEN is 0/1.
684-691: Robust re-init across mode togglesUsing (SEGENV.call == 0) || (SEGENV.aux1 > 1) to reinitialize covers switching from the legacy twinkle (where aux1 held a PRNG seed). Clean and safe.
774-774: Verify UI: is Intensity actually disabled when Cycle mode is ON?Code ignores SEGMENT.intensity in Cycle mode, but the metadata only adds “Cycle mode”; it doesn’t disable the slider. If the intent is to gray out Intensity when Cycle is set, you may need a UI dependency in the mode string or frontend logic.
Would you like a patch suggestion for metadata/UI to hide/disable Intensity when Cycle is checked?
|
thanks for contributing. I think adding 80 lines of code to 25line code effect is not a good idea. it should either be made much simpler or be its own effect. |
|
There was conversation in my previous effect contribution PR about effect IDs being a limited commodity. So I was respecting that and putting this change into the effect that it was most similar to. I don't feel like the code size should be the determining factor of where an effect should go, but rather, which effect it is most similar to and where it fits logically with respect to the actual effect. But I'm also not here to tell the maintainers of the code how to handle their project, just giving my reasoning for why I did what I did. I'll look into making it smaller. Thanks for the feedback. |
|
so you have basically recreated the dissolve effect? why not use that one? |
|
Well, this is embarrassing. I just saw what twinkle was doing and had an idea, and never even looked for a different effect that did a similar thing. Dissolve does behave nearly as I want, and will be good enough. Thanks, and sorry for wasting your time. |
2 participants
This adds a checkbox to the Twinkle effect that enables Cycle mode, where the lights will turn from the background color to the foreground color, and once all the pixels have changed, will randomly change each pixel back to the background color, completing the cycle which then repeats.
This obviously disables the Segment Intensity slider as all the pixels will twinkle to the foreground color before twinkling back to the background color.
All code has been successfully built, tested, and found working as expected on my setup.
Full disclosure, AI was used to assist me in making these changes.
Summary by CodeRabbit