Remove FRAMETIME return value from all FX#5314
Conversation
- all FX now render every frame, no more "speed up" during transitions
WalkthroughCore FX mode functions and WS2812FX interfaces were changed from returning Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 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: 2
🤖 Fix all issues with AI agents
In `@usermods/user_fx/README.md`:
- Around line 83-85: The tutorial snippets call mode_static() when a non-2D
setup is detected but do not stop execution, allowing the flow to continue into
2D-specific logic; update the conditional that checks strip.isMatrix and
SEGMENT.is2D() so that after calling mode_static() you immediately return (e.g.,
wrap mode_static() in a block and add return;) and apply the same change to the
other occurrence referenced around the second snippet (the one at 130-132) so
both examples exit early after the fallback.
In `@wled00/FXparticleSystem.h`:
- Line 23: The header currently defines WLED_DEBUG_PS unconditionally which
forces particle-system debug and extra flash into every build; remove the
default `#define` and make WLED_DEBUG_PS opt-in by guarding debug code behind a
build-time macro (e.g., rely on a compile flag such as -DENABLE_WLED_DEBUG_PS or
an existing global debug macro) so that FXparticleSystem.h only enables debug
when that build flag is set; update references to WLED_DEBUG_PS in the file so
they are inside `#ifdef` ENABLE_WLED_DEBUG_PS (or the chosen macro) and document
the new build flag for opt‑in usage.
🧹 Nitpick comments (3)
usermods/user_fx/README.md (1)
253-253: Clarify how to re‑render when skipping state updates.The new note says to re‑render every frame, but the example still renders only inside the update interval. Consider adding a short hint or snippet showing a “render every call, update conditionally” structure to avoid confusion for readers.
wled00/FXparticleSystem.cpp (2)
1098-1102: Consider nullingPartSyson early exit to avoid dangling pointers.
SEGMENT.deallocateData()invalidates prior PS memory; settingPartSys = nullptrhere makes accidental reuse safer.♻️ Suggested change
if (!strip.isMatrix) { errorFlag = ERR_NOT_IMPL; // TODO: need a better error code if more codes are added SEGMENT.deallocateData(); // deallocate any data to make sure data is null (there is no valid PS in data and data can only be checked for null) + PartSys = nullptr; return false; // only for 2D }
1850-1854: Also nullPartSysin the 1D early-exit path.
This keeps callers from accidentally using a stale pointer afterSEGMENT.deallocateData().♻️ Suggested change
if (SEGLEN == 1) { errorFlag = ERR_NOT_IMPL; // TODO: need a better error code if more codes are added SEGMENT.deallocateData(); // deallocate any data to make sure data is null (there is no valid PS in data and data can only be checked for null) + PartSys = nullptr; return false; // single pixel not supported }
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
usermods/user_fx/README.md (1)
302-309: Update Sinelon example to void-return to match FX changes.
The example still declaresuint16_tand later showsreturn FRAMETIME;, butFX_FALLBACK_STATICexpands toreturn;and the API is now void. Please align the snippet and its narrative to avoid copy-paste compile errors.✏️ Proposed doc fix
-static uint16_t sinelon_base(bool dual, bool rainbow=false) { +static void sinelon_base(bool dual, bool rainbow=false) { if (SEGLEN <= 1) FX_FALLBACK_STATIC; ... - return FRAMETIME; }-uint16_t mode_sinelon(void) { - return sinelon_base(false); -} +void mode_sinelon(void) { + sinelon_base(false); +} -uint16_t mode_sinelon_dual(void) { - return sinelon_base(true); -} +void mode_sinelon_dual(void) { + sinelon_base(true); +} -uint16_t mode_sinelon_rainbow(void) { - return sinelon_base(false, true); -} +void mode_sinelon_rainbow(void) { + sinelon_base(false, true); +}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@wled00/FXparticleSystem.h`:
- Around line 145-147: The comment for updateFire still mentions the removed
renderonly parameter and should be updated to reflect the new always-update
behavior; modify the comment on the updateFire(const uint8_t intensity)
declaration in FXparticleSystem.h to remove the "if renderonly is set, particles
are not updated" clause and instead state that updateFire always updates
particles (and is used to keep fire behavior consistent across frame skips/
transitions), or simply remove the outdated phrase so the comment matches
current behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
usermods/Temperature/Temperature.cpp (1)
3-3: Fix forward declaration return type (compile error).The forward declaration still returns
uint16_twhile the definition isvoid, which is a conflicting type error.🛠️ Proposed fix
-static uint16_t mode_temperature(); +static void mode_temperature();usermods/TetrisAI_v2/TetrisAI_v2.cpp (1)
101-108: Remove return value from void function.
mode_2DTetrisAI()isvoid, but still returns350, which is invalid.🛠️ Proposed fix
if (!strip.isMatrix || !SEGENV.allocateData(sizeof(tetrisai_data))) { // not a 2D set-up SEGMENT.fill(SEGCOLOR(0)); - return 350; + return; }usermods/pixels_dice_tray/led_effects.h (2)
44-66: Fix return-type mismatch after switchingrunning_copytovoid(compile break).
running_copy()is nowvoid, butcheck_roll()still doesreturn running_copy(...)(Line 112) and the mode wrappers (pulse_roll,check_roll, plus theextern mode_*declarations) still useuint16_treturns. With the global FX return removal, this will fail to compile. Update the usermod effects and externs tovoidand stop returning their results.Example shape (outside this hunk, for illustration):
extern void mode_breath(); extern void mode_blends(); extern void mode_glitter(); extern void mode_gravcenter(); static void pulse_roll() { auto roll = GetLastRollForSegment(); if (roll.state != pixels::RollState::ON_FACE) { mode_breath(); return; } mode_blends(); ... } static void check_roll() { auto roll = GetLastRollForSegment(); if (roll.state != pixels::RollState::ON_FACE) { running_copy(SEGCOLOR(0), SEGCOLOR(2)); return; } if (roll.current_face + 1 >= SEGMENT.custom2) { mode_glitter(); return; } mode_gravcenter(); }
68-81: Clampnum_segmentsand avoid<=to prevent out-of-range index.When
roll.current_faceis max,num_segmentscan equalSEGLEN, andi <= num_segmentscallssetPixelColor(SEGLEN, ...). Safer to clamp and use<.✅ Suggested fix
- uint16_t num_segments = float(roll.current_face + 1) / 20.0 * SEGLEN; - for (int i = 0; i <= num_segments; i++) { + uint16_t num_segments = (uint32_t)(roll.current_face + 1) * SEGLEN / 20; + if (num_segments > SEGLEN) num_segments = SEGLEN; + for (int i = 0; i < num_segments; i++) { SEGMENT.setPixelColor(i, SEGCOLOR(0)); }
🤖 Fix all issues with AI agents
In `@usermods/user_fx/README.md`:
- Around line 255-257: Typo: change the misspelled word "fuction" to "function"
in the README paragraph that mentions SEGMENT.call and FRAMETIME; update the
sentence "Also `SEGMENT.call` is still incremented on each fuction call." to
"Also `SEGMENT.call` is still incremented on each function call." to correct the
spelling while keeping the wording and references to FRAMETIME, refresh_ms and
SEGMENT.call intact.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
usermods/pixels_dice_tray/led_effects.h (1)
73-79: Prevent out-of-bounds write when the roll fills the segment.
Ifnum_segments == SEGLEN,i <= num_segmentswrites indexSEGLEN(invalid).🐛 Proposed fix
- uint16_t num_segments = float(roll.current_face + 1) / 20.0 * SEGLEN; - for (int i = 0; i <= num_segments; i++) { + uint16_t num_segments = float(roll.current_face + 1) / 20.0 * SEGLEN; + if (num_segments > SEGLEN) num_segments = SEGLEN; + for (int i = 0; i < num_segments; i++) { SEGMENT.setPixelColor(i, SEGCOLOR(0)); }
🤖 Fix all issues with AI agents
In `@usermods/pixels_dice_tray/led_effects.h`:
- Around line 109-114: In function check_roll() fix the compile error by adding
the missing semicolon after the bare return inside the if branch; locate
check_roll and the offending return within the block that calls
running_copy(SEGCOLOR(0), SEGCOLOR(2)) and terminate that statement with a
semicolon so the function compiles correctly.
In `@usermods/user_fx/README.md`:
- Line 309: The function signature for sinelon_base is missing the explicit
return type; update the declaration "static sinelon_base(bool dual, bool
rainbow=false)" to include the void return type (i.e., change it to "static void
sinelon_base(...)") so it matches the PR's move from uint16_t to void and the
rest of the API; modify the declaration and any matching forward declarations or
definitions of sinelon_base to use the void return type.
- Around line 398-410: The three wrapper functions mode_sinelon,
mode_sinelon_dual, and mode_sinelon_rainbow are missing return type specifiers;
update their declarations to include the proper WLED FX convention (e.g., static
void) before each function name so they match sinelon_base's usage and signature
expectations; ensure you change mode_sinelon(void), mode_sinelon_dual(void), and
mode_sinelon_rainbow(void) to static void mode_sinelon(void), static void
mode_sinelon_dual(void), and static void mode_sinelon_rainbow(void)
respectively.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@usermods/user_fx/README.md`:
- Around line 255-262: The snippet currently uses an early return which prevents
rendering and breaks smooth transitions; instead modify the logic around
SEGENV.step/strip.now so you only skip updating effect state when (strip.now -
SEGENV.step) < FRAMETIME_FIXED but do not return — allow the rest of the
function to run to perform rendering and let SEGMENT.call increment. Concretely,
change the early-return pattern to a conditional branch that bypasses
state-mutating code (state update block) when the interval hasn't elapsed, but
still executes rendering/gradient/brightness/segment output code; keep updating
SEGENV.step only when you actually perform the state update so timing remains
correct.
|
I've not tested all the effects, but approve in principle the change |
|
thanks, its almost exclusively "house-keeping". only the "ICU" FX has fudamental changes. While checking feasiblity of #5135 a few hours ago I actually found that the segmen's internal "next time" could be replaced with "time_offset" then all that is missing is instead of strip.now() use strip.now()+time_offset and #5135 has a base to work with. |
|
@coderabbitai please explain the use of "direct" in |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1283 🏁 Script executed: Length of output: 3220 🏁 Script executed: Length of output: 96
How it worksLooking at the implementation in void WS2812FX::setBrightness(uint8_t b, bool direct) {
// ... brightness update logic ...
BusManager::setBrightness(scaledBri(b));
if (!direct) {
unsigned long t = millis();
if (_segments[0].next_time > t + 22 && t - _lastShow > MIN_SHOW_DELAY) trigger();
}
}When
When
Usage patterns in the codebase
Still useful after this PR?Yes, the
The fixed frame timing from your PR actually makes the behavior more predictable, but the need to optionally trigger immediate updates when changing brightness remains valid. 🧠 Learnings used |
|
@coderabbitai regarding the above comment about |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3304 🏁 Script executed: Length of output: 8711
Current usage patternsFrom the code analysis:
Why per-segment timing is still valuableEven though all segments now update at the same rate (FRAMETIME), they don't necessarily update at the same time:
If you moved to static/strip-levelA single global timer would mean:
RecommendationKeep it per-segment. The memory cost is minimal (4 bytes × num_segments), and it preserves the ability for segments to have independent lifecycles while still rendering at a uniform rate. This design also leaves the door open for your 🧠 Learnings used |
|
@softhack007 would there be any downside to moving the seg.next_time into the strip class and have all segments & strips always update in sync? I can not think of a reason not to do that. |
|
@DedeHai good question.
nowUp (and SEGMENT.now) is already controlled by the same (global) clock, so these might not be needed or could be moved to strip level.
This might be a tricky one. Not sure if |
|
I really have no idea why you keep asking me while at the same time ignoring my comments. |
|
Don't worry, we will find the answer ourselves. |
|
Lines 1725 to 1730 in 129137d
I think the condition on As far as my memory gets me, this was added long ago together with smooth transitions, and we had a problem with the "solid" effect that was only updating 3x per second. I'm still wondering why only --> Simplifying the condition to |
I was under the same impression, at least from a "local" viepoint - not thinking of what happens to DMX / streaming / external control has got me in the past. |
|
@DedeHai I'll ask the rabbit for an extra "confidence check", just to be sure.
@coderabbitai please identify the commit that added the condition "if (_segments[0].next_time > t + 22", and also check for old issue tickets and PR that might explain the intention behind this check. Considering that MIN_SHOW_DELAY is either 8 or 16, and all segments are now progressing at the same speed, do you see user-noticeable impact (transitions, external control, etc) if we simplify the condition as suggested in #5314 (comment) ? Looking at the history that's available to you, what was the initial purpose of the condition? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Unrecoverable error: Request timed out after 300000ms (requestId=70f6895a-34b1-449e-9b9e-e3ee3a280621) 🏁 Script executed: Length of output: 3526 🏁 Script executed: Length of output: 1510 🏁 Script executed: Length of output: 2241 🏁 Script executed: Length of output: 1479 🏁 Script executed: Length of output: 24309
Origin of the conditionThe condition was added in commit 8ccf349 on July 19, 2023, with the message "Always repaint NPB buffer on brightness change". This was part of addressing issue The original problemIssue
Why the
|
|
@DedeHai now we know everything ;-) I've never seen the rabbit work for longer than 10 minutes for answering a question 😅
I think its safest to add a small "margin", like |
|
I checked, no pulsing using ABL. Approved and good to merge from my side. |
4 participants
since for smooth transitions the FPS was forced to FRAMETIME anyway and only a handful of FX actually used it there is no reason to keep this logic. With persistent segment buffers a frame can also be sent out multiple times, even if the FX does not render it (which it should for smooth transitions but it is not a requirement anymore)
FX_FALLBACK_STATIC { mode_static(); return; }return FRAMETIME;and addFX_FALLBACK_STATICreferencesAdditional changes:
Summary by CodeRabbit
Bug Fixes
Refactor
Documentation