Add TwinkleFadeIn effect #4728
Add TwinkleFadeIn effect #4728
Conversation
WalkthroughThe Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (3)
wled00/FX.cpp (3)
2537-2540: Prefer an enum over multiple booleans for effect variants
twinklefox_one_twinkle()now accepts two feature flags (cat,instantOff).
The possible combinations are implicitly:
- vanilla –
false,false- twinklecat –
true,false- twinklefadein –
false,true- undefined / nonsense –
true,trueUsing two independent booleans makes the (4)-case possible and obscures intent at the call-site.
Consider replacing them with a single typed enum:-enum class TwinkleKind : uint8_t { Vanilla, Cat, FadeIn }; - -static CRGB twinklefox_one_twinkle(uint32_t ms, - uint8_t salt, - TwinkleKind kind = TwinkleKind::Vanilla); +// callers read much clearer: twinklefox_one_twinkle(ms, salt, TwinkleKind::FadeIn)This eliminates invalid states and improves readability.
2598-2601:twinklefox_basesignature driftThe new
instantOffparameter is forwarded correctly, but the function comment block still documents only thecatvariant.
Update the comment to avoid future confusion and include a short description of the new behaviour.
2672-2676: Mode registration string does not mention “Fade-in” behaviour
_data_FX_MODE_TWINKLEFADEINcurrently copies the metadata from the other modes.
To help end-users recognise the difference in the UI, append a short hint (e.g. “fade-in”) in the effect name segment:-static const char _data_FX_MODE_TWINKLEFADEIN[] PROGMEM = "Twinklefadein@!,Twinkle rate,,,,Cool;!,!;!"; +static const char _data_FX_MODE_TWINKLEFADEIN[] PROGMEM = "Twinklefadein (fade-in)@!,Twinkle rate,,,,Cool;!,!;!";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/FX.cpp(6 hunks)wled00/FX.h(1 hunks)
🔇 Additional comments (3)
wled00/FX.h (1)
376-376:✅ Verification successful
New FX mode identifier appended
You’ve addedFX_MODE_TWINKLEFADEINat ID 218 as per the chosen scheme. Ensure this new mode is actually registered insetupEffectData()within FX.cpp so it’s available at runtime.
🏁 Script executed:
#!/bin/bash # Verify the new mode is registered in FX.cpp grep -R "addEffect( *FX_MODE_TWINKLEFADEIN" -n wled00/FX.cppLength of output: 154
New FX mode identifier appended and registered
The addedFX_MODE_TWINKLEFADEINat ID 218 is properly registered insetupEffectData()in wled00/FX.cpp (line 10674). No further changes required—ready to merge.wled00/FX.cpp (2)
2558-2562: Brightness curve forinstantOffis only half implementedFor the fade-in / instant-off branch you set
bright = ph;– a strictly rising linear ramp.
Whenphresets to 0 on the next cycle the LED will indeed turn off instantly, but:
- The ramp peaks at
255only at the very end of the 0-255 phase, so the LED spends most of its cycle very dim.- The visual result is noticeably dull compared to the other variants that use a steeper attack.
You likely want a faster rise (e.g.
bright = ph * 3for 0-85) and an immediate drop to 0 afterwards, mirroring the twinklecat decay shape:-} else if (instantOff) { //fade in, instant off - bright = ph; +} else if (instantOff) { //fade in, instant off + if (ph < 86) { + bright = ph * 3; // rapid fade-in + } else { + bright = 0; // instant off for the rest of the cycle + }Please verify the visual behaviour on hardware – the current linear ramp may look under-powered.
10671-10675: Verify mode ID andMODE_COUNTupdate
addEffect(FX_MODE_TWINKLEFADEIN, …)assumes thatFX_MODE_TWINKLEFADEINis unique and thatMODE_COUNThas been bumped correspondingly inFX.h.
Please double-check for collisions with recently added modes (218/219 are getting crowded).
If multiple PRs land out of order an ID clash will cause subtle runtime issues.
|
If it is a small and simple change, why not update existing effect with one of the custom checkboxes? |
|
To expand: FX ids are at somewhat of a premium these days. I also think it'd be better to add an "invert" checkbox to TwinkleCat vs giving it a new name. |
|
Thank you for the feedback. That makes a lot more sense to use a checkbox instead of a whole new effect. (Which also implies that "TwinkleCat" itself could have been a checkbox. But that's a whole other issue.) I've converted the function param to be a checkbox called "Reverse" and committed that. Note: I did use AI (cursor) to assist me, as I am unfamiliar with the PROGMEM mode string syntax, so please let me know if that is incorrect. |
wled00/FX.h
Outdated
| @@ -373,6 +373,7 @@ extern byte realtimeMode; // used in getMappedPixelIndex() | |||
| #define FX_MODE_PS1DSONICBOOM 215 | |||
| #define FX_MODE_PS1DSPRINGY 216 | |||
| #define FX_MODE_PARTICLEGALAXY 217 | |||
|
|
|||
wled00/FX.cpp
Outdated
| @@ -2665,7 +2667,7 @@ uint16_t mode_twinklecat() | |||
| { | |||
| return twinklefox_base(true); | |||
| } | |||
| static const char _data_FX_MODE_TWINKLECAT[] PROGMEM = "Twinklecat@!,Twinkle rate,,,,Cool;!,!;!"; | |||
| static const char _data_FX_MODE_TWINKLECAT[] PROGMEM = "Twinklecat@!,Twinkle rate,,,,Cool;!,!;!,Reverse"; | |||
There was a problem hiding this comment.
check2 is after cooling checkbox:
"Twinklecat@!,Twinkle rate,,,,Cool,Reverse;!,!;!";
please make sure to test your code so it does what you want it to.
There was a problem hiding this comment.
I plan on testing it. Thanks for the updates.
revert extra line
|
I just compiled and tested, and it's working as expected. For future reference, is there a way to edit the icons used for the checkboxes? |
the icons are fixed but you can choose the checkmark number |
|
Not had chance to test, but the code changes are looking much more compact :) |
This adds a new effect which is the reverse of Twinklecat: Twinklefadein (See Issue #3738)
Twinklecat instantly turns on random lights and then slowly fades them out. This effect slowly fades in random lights, and then instantly turns them off.
It is a very simple and small change, but has not been tested.
The only questions I had were about the FX numbering scheme:
I opted for option #1, but can easily adjust it if needed.
Thanks
Summary by CodeRabbit