Update Percent effect to add optional % of time modes #4904
Conversation
I updated the checkboxes on Percent effect to support three new modes to automatically illuminate a % of the LEDs in a segment: - % of minute - % of hour - % of day This allows you to create dynamic effects based on these timescales. e.g. custom abstract clock displays. Personally I did this because I am creating a visual daily 'clock' for my kids with physical markers on it for "dad is done with work" and "Bed time" etc. I added minute/hour at the suggestion of others, but now this makes it suitable for true clock functionality which is nice -- and with much more flexibility than the other 'clock' mode option. This also deprecates the previous 'use single color' checkbox. I deemed this to be unnecessary since it is easy to just set the palette to 'color 1' which achieves the exact same effect.
WalkthroughAdds optional time-based progress rendering to the Percent FX mode (minute/hour/day) that maps elapsed time to lit LEDs; refactors the non-time path to recompute normalized percent and simplify per-LED palette/unlit filling; updates the mode metadata string to expose the new time options. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
freakintoddles2
changed the title
|
@DedeHai here's the PR I mentioned, and a vid of it in action Screen_Recording_2025-08-28_at_2.07.18_PM.mov |
![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
🧹 Nitpick comments (2)
wled00/FX.cpp (2)
3894-3897: Update the function header comment to match new 0–200 behaviorThe comment still says “Intensity values from 0-100,” but the code now supports 0–200 for the mirrored phase. Update the doc to avoid user confusion.
Apply this diff:
- * Intensity values from 0-100 turn on the leds. + * Intensity: + * 0–100 -> grow from 0% to 100% + * 101–200 -> mirror: shrink from 100% back toward 0%
3940-3966: Smoothing step can overshoot on speed=255; cap to segment lengthWith speed=255, size=255. On small segments this can jump SEGENV.aux1 far past active_leds in one frame. Consider clamping size to SEGLEN (or a fraction thereof) to keep transitions smooth across all lengths.
Example:
- unsigned size = (1 + ((SEGMENT.speed * SEGLEN) >> 11)); - if (SEGMENT.speed == 255) size = 255; + unsigned size = (1 + ((SEGMENT.speed * SEGLEN) >> 11)); + if (SEGMENT.speed == 255) size = SEGLEN; // avoid overshoot on short segments
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
wled00/FX.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/{*.cpp,!(html_*)*.h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ source and header files
Files:
wled00/FX.cpp
🧠 Learnings (3)
📓 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
🧬 Code graph analysis (1)
wled00/FX.cpp (1)
wled00/src/dependencies/time/Time.cpp (6)
second(77-80)second(77-77)minute(72-75)minute(72-72)hour(49-52)hour(49-49)
⏰ 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). (9)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
🔇 Additional comments (1)
wled00/FX.cpp (1)
3978-3979: Confirm UI metadata/backward compatibility for repurposed checkboxesSwitching from “One color” to “% of Minute/Hour/Day” changes checkbox semantics and saved presets. Verify:
- the new labels appear in the UI as intended,
- existing presets using “One color” won’t silently flip on a time mode,
- JSON API metadata consumers tolerate the new fields.
I can help write a quick preset-migration note or guard in the effect to ignore time flags if a legacy preset sets palette==“color 1”.
|
I like it! I am just not too sure the percent effect should be repurposed like this i.e. maybe this should be an effect in its own right. I think percent is quite an old effect and it must stay backwards compatible, meaning repurposing a checkmark is not a good idea or an update to 0.16 will break peoples presets. We should try to avoid that as much as possible. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
wled00/FX.cpp (4)
3900-3913: Make time-mode selection explicit and mutually exclusiveIf multiple checks are set, check3 silently overrides the rest. Make the priority explicit and avoid redundant assignments.
- if (SEGMENT.check1 || SEGMENT.check2 || SEGMENT.check3) { - uint32_t period = 60U; // Minute default - uint32_t elapsed = (uint32_t)second(localTime); - - if (SEGMENT.check2) { // Hour - period = 3600U; - elapsed = (uint32_t)minute(localTime) * 60U + (uint32_t)second(localTime); - } - if (SEGMENT.check3) { // Day - period = 86400U; - elapsed = (uint32_t)hour(localTime) * 3600U - + (uint32_t)minute(localTime) * 60U - + (uint32_t)second(localTime); - } + if (SEGMENT.check1 || SEGMENT.check2 || SEGMENT.check3) { + uint32_t period, elapsed; + if (SEGMENT.check3) { // Day + period = 86400U; + elapsed = (uint32_t)hour(localTime) * 3600U + + (uint32_t)minute(localTime) * 60U + + (uint32_t)second(localTime); + } else if (SEGMENT.check2) { // Hour + period = 3600U; + elapsed = (uint32_t)minute(localTime) * 60U + (uint32_t)second(localTime); + } else { // Minute + period = 60U; + elapsed = (uint32_t)second(localTime); + }
3899-3903: Document unsynced-time behavior to avoid “fixes” laterAdd a note that 00:00:00 (unsynced RTC) intentionally renders full for one tick.
// --- Time modes (if any of Minute/Hour/Day checks are active) --- + // Note: when localTime is not yet synced (00:00:00), we intentionally show a full bar + // for one tick to avoid an “empty” rollover frame (clock semantics).
3928-3936: Avoid double pass over SEGLEN in time pathFill background once, then paint lit portion.
- // lit portion - for (unsigned i = 0; i < lit; i++) { - SEGMENT.setPixelColor(i, SEGMENT.color_from_palette(i, true, PALETTE_SOLID_WRAP, 0)); - } - // unlit portion - for (unsigned i = lit; i < SEGLEN; i++) { - SEGMENT.setPixelColor(i, SEGCOLOR(1)); - } + SEGMENT.fill(SEGCOLOR(1)); + for (unsigned i = 0; i < lit; i++) { + SEGMENT.setPixelColor(i, SEGMENT.color_from_palette(i, true, PALETTE_SOLID_WRAP, 0)); + }
3939-3945: Initialize smoothing accumulator on first callPrevent inherited state when switching into Percent mode.
- // Normal percent mode (when all time options are unchecked) + // Normal percent mode (when all time options are unchecked) + if (SEGENV.call == 0) SEGENV.aux1 = 0;Also applies to: 3967-3975
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
wled00/FX.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/{*.cpp,!(html_*)*.h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ source and header files
Files:
wled00/FX.cpp
🧠 Learnings (6)
📓 Common learnings
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”.
Learnt from: freakintoddles2
PR: wled/WLED#4904
File: wled00/FX.cpp:3901-3913
Timestamp: 2025-09-02T19:36:49.513Z
Learning: In FX.cpp mode_percent time-based options (% of Minute/Hour/Day), do not guard against invalid RTC; showing a full bar with unsynced time is intentional to avoid confusing UX.
📚 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-09-02T19:36:49.513Z
Learnt from: freakintoddles2
PR: wled/WLED#4904
File: wled00/FX.cpp:3901-3913
Timestamp: 2025-09-02T19:36:49.513Z
Learning: In FX.cpp mode_percent time-based options (% of Minute/Hour/Day), do not guard against invalid RTC; showing a full bar with unsynced time is intentional to avoid confusing UX.
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-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-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
🧬 Code graph analysis (1)
wled00/FX.cpp (1)
wled00/src/dependencies/time/Time.cpp (6)
second(77-80)second(77-77)minute(72-75)minute(72-72)hour(49-52)hour(49-49)
⏰ 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). (17)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- 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 (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
🔇 Additional comments (1)
wled00/FX.cpp (1)
3915-3926: LGTM on clock rollover semanticsThe inclusive mapping with full bar at rollover and minimum one lit otherwise matches the intended “clock” UX.
| static const char _data_FX_MODE_PERCENT[] PROGMEM = "Percent@,% of fill,,,,% of Minute,% of Hour,% of Day;!,!;!"; | ||
|
|
There was a problem hiding this comment.
Don’t break existing Percent presets: restore old metadata, add a new alias for time modes
Repurposing check1/check2/check3 here will flip on time-based behavior for users who previously saved “One color” in Percent, breaking presets on upgrade. Keep Percent unchanged and expose the new clock modes as a separate alias (same implementation) to preserve backward compatibility.
Apply this diff to keep old Percent UI and add a new metadata string for the alias:
-static const char _data_FX_MODE_PERCENT[] PROGMEM = "Percent@,% of fill,,,,% of Minute,% of Hour,% of Day;!,!;!";
+static const char _data_FX_MODE_PERCENT[] PROGMEM = "Percent@,% of fill,,,,One color;!,!;!";
+static const char _data_FX_MODE_PERCENT_TIME[] PROGMEM = "Percent (Time)@,% of fill,,,,% of Minute,% of Hour,% of Day;!,!;!";Then register the alias alongside the original (outside this hunk, in WS2812FX::setupEffectData right after addEffect(FX_MODE_PERCENT, …)):
// keep the original Percent
addEffect(FX_MODE_PERCENT, &mode_percent, _data_FX_MODE_PERCENT);
// add a new effect ID at the end for time-based Percent (no code duplication)
addEffect(255, &mode_percent, _data_FX_MODE_PERCENT_TIME);This preserves legacy presets while making the new time-based options discoverable.
🤖 Prompt for AI Agents
In wled00/FX.cpp around lines 3978-3979, the change repurposed the Percent
metadata which will break existing saved Percent presets; restore the original
_data_FX_MODE_PERCENT string exactly as it was and add a new metadata string
(e.g. _data_FX_MODE_PERCENT_TIME) containing the time-mode fields, then do not
change Percent's behavior or its check1/check2/check3 usage. After the existing
addEffect(FX_MODE_PERCENT, &mode_percent, _data_FX_MODE_PERCENT) call in
WS2812FX::setupEffectData, register an alias by calling addEffect with a new
unused effect ID (example 255) that points to the same mode handler but uses the
new _data_FX_MODE_PERCENT_TIME metadata so the time-based options are
discoverable without altering legacy presets.
2 participants
I updated the checkboxes on Percent effect to support three new modes to automatically illuminate a % of the LEDs in a segment:
This allows you to create dynamic effects based on these timescales. e.g. custom abstract clock displays. Personally I did this because I am creating a visual daily 'clock' for my kids with physical markers on it for "dad is done with work" and "Bed time" etc. I added minute/hour at the suggestion of others, but now this makes it suitable for true clock functionality which is nice -- and with much more flexibility than the other 'clock' mode option.
The effect is designed to fill the segment during the last 'tick' of the time period. At rollover (e.g. second 60/0) it intentionally shows SEGLEN lit for one tick to represent the completed period, then move to 1 LED at second 1. This avoids a visually empty segment at rollover and matches the cadence of a ticking hand (…58, 59, 60/0 full, 1, 2…).
This also deprecates/repurposes the previous 'use single color' checkbox. This appeared to be completely redundant and unnecessary since it is easy to just set the palette to 'color 1' which achieves the exact same effect.
Summary by CodeRabbit
New Features
Documentation