Improve Boot preset handling#5573
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a boot-time preset/playlist sequencing pass in setup, tightens when the ChangesBoot preset sequencing and init-mode gating
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wled00/presets.cpp`:
- Around line 197-200: The guard currently lets any fdo["ps"] value through for
CALL_MODE_INIT (so string cycling specs like "1~5~" reach deserializeState);
restrict the CALL_MODE_INIT exception to numeric preset values by adding a type
check (e.g. fdo["ps"].is<int>() or appropriate numeric type) to the existing
condition (mirror the pattern used for CALL_MODE_BUTTON_PRESET which checks
fdo["ps"].is<const char *>()), so only numeric "ps" values are preserved for
deserializeState() and non-numeric strings are removed with fdo.remove("ps") to
avoid unintended playlist/cycling behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8125ad27-4651-45d0-adf2-70d57c1158bb
📒 Files selected for processing (2)
wled00/presets.cppwled00/wled.cpp
|
would a playlist not be able to do the same? |
Thanks for the feedback! Hence why I’m still in draft mode - I’m not ready to say this is review or merge ready just yet.
|
TODO: set up logic handling inside handlePresets() and anywhere else required to implement behavior that is described in comments here.
Add TODO comments for handling boot presets and playlists.
willmmiles
left a comment
willmmiles
left a comment
There was a problem hiding this comment.
Re performance changes: I do expect that the latency of how long it takes the main loop to get around to applying the boot preset will be highly dependent on environment and wifi setup. In fact, one down side of the proposed new approach is that it may lead to more devices showing visible stutters and hiccups during the connection sequence, as the light "starts up" earlier. There's undoubtedly room for improvement there -- I suspect some of the execution order details in the current code are workarounds for issues with older platforms that may not still apply. I don't think we should spend too much time worrying about this (either way) at the moment.
Re ps vs playlists: there's a very real sense in which the functionality of "ps" and "playlist" overlap: "ps" could (almost) be implemented as a single-entry "playlist" with 0 delay. In this context, both approaches require extra work at setup() time -- but realistically we probably should handle both, if only so that we don't have to document a bunch of special cases.
In practice I think this works out to something like:
if (bootPreset > 0) {
handlePreset(); // execute boot preset
handlePlaylist(); // if boot preset set a playlist, process it
handlePreset(); // if either of the above set a preset for deferred execution, execute it now
}
With the reports about brownouts which may be related to faster LED turnon time (I measured roughly 1.2s vs 2.5s in 0.15) making it more consistent would be a welcome improvement, albeit this may cause some transitional friction with existing setups that have insufficient power stability |
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
wled00/presets.cpp (1)
213-214:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestrict the boot-time
psexception to numeric preset IDs.
shouldAllowLoadRequeststill preserves any bootpsvalue whenloris present, so cycling strings like"1~5~"can still reachdeserializeState()during setup. That contradicts the intended single numeric handoff.🛡️ Proposed fix
- const bool shouldAllowLoadRequest = (isBootPreset && presetWillSetLor) || (tmpMode == CALL_MODE_BUTTON_PRESET && fdo["ps"].is<const char *>() && strchr(fdo["ps"].as<const char *>(),'~') != strrchr(fdo["ps"].as<const char *>(),'~')); + const bool shouldAllowLoadRequest = ((isBootPreset && presetWillSetLor && fdo["ps"].is<int>()) || isButtonException);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/presets.cpp` around lines 213 - 214, The boot-time exception currently lets any fdo["ps"] through when isBootPreset && presetWillSetLor, which allows non-numeric strings like "1~5~" to reach deserializeState(); change the shouldAllowLoadRequest logic in the block around shouldAllowLoadRequest/presetWillSetLor so the isBootPreset branch only permits ps values that are strictly numeric (e.g., contains only digits or is recognized as an integer by the JSON API) — keep the existing tmpMode == CALL_MODE_BUTTON_PRESET check and the fdo["ps"] existence check, but add a numeric validation of fdo["ps"] before allowing the boot-time handoff to deserializeState().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wled00/presets.cpp`:
- Around line 191-196: The branch that returns when shouldAllowPresetApply is
false currently exits after calling requestJSONBufferLock(JSON_LOCK_PRESET_LOAD)
and clearing presetToApply, which both drops the pending preset and leaves the
JSON lock held; change this path to first release the JSON lock (call
releaseJSONBufferLock(JSON_LOCK_PRESET_LOAD) or the matching unlock function)
and requeue the blocked preset instead of clearing it (restore/push
presetToApply back into the pending queue or do not clear it) before returning
so the preset is not lost and the JSON lock is not left locked; update the logic
around shouldAllowPresetApply, requestJSONBufferLock(JSON_LOCK_PRESET_LOAD) and
presetToApply to perform unlock + requeue prior to the early return.
- Around line 211-213: The two const bool declarations isButtonException and
shouldAllowLoadRequest are missing trailing semicolons; add a semicolon at the
end of each declaration line (the lines that compute isButtonException using
tmpMode, CALL_MODE_BUTTON_PRESET, fdo["ps"].as<const char *>(), strchr and
strrchr, and the similar shouldAllowLoadRequest expression) so both variable
definitions terminate properly.
---
Duplicate comments:
In `@wled00/presets.cpp`:
- Around line 213-214: The boot-time exception currently lets any fdo["ps"]
through when isBootPreset && presetWillSetLor, which allows non-numeric strings
like "1~5~" to reach deserializeState(); change the shouldAllowLoadRequest logic
in the block around shouldAllowLoadRequest/presetWillSetLor so the isBootPreset
branch only permits ps values that are strictly numeric (e.g., contains only
digits or is recognized as an integer by the JSON API) — keep the existing
tmpMode == CALL_MODE_BUTTON_PRESET check and the fdo["ps"] existence check, but
add a numeric validation of fdo["ps"] before allowing the boot-time handoff to
deserializeState().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c0bed3d1-01f4-47c8-8a7c-b593e291c16f
📒 Files selected for processing (3)
wled00/presets.cppwled00/wled.cppwled00/wled.h
… doesn't get accidentally locked during setup
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
reminds me of another PR that is still on our stack: PR 5370 suggests to keep presets code active while in realtime mode, so that users are able to use presets assigned to physicals buttons. I think these two PR's should be considered together, or maybe merged into one PR. |
I took a brief look at this PR. I do think mine and this one go well together but I have some concerns about implementation of the other PR. I understand exactly what the author did and why - and in fact my very first instinct on this PR was to move handlePreset() and handlePlaylist() outside the realtime gate in the main loop, but ultimately I decided messing with the main loop flow was fairly dangerous, which is why I opted to add calls to setup() and some safety logic inside of handlePreset(). I think the same could be applied to the other PR by adding handlePreset(), handlePlaylist(), handlePreset() inside of handleButton() or, probably more appropriately, in the handle IO loop (which I have not looked at yet) then the same safety logic inside handlePreset() that is added with this PR can be applied to IO calls. Since I’m not the author of the other PR, I didn’t deep dive any further than this and I also don’t want to step on his toes. TL;DR I agree that other PR makes sense along side mine. I’m not certain the implementations are compatible and think there’s an opportunity to share logic. The main concern is the potential effects of altering the main loop behavior. |
| // is this the boot preset and will the preset set lor | ||
| const bool isBootPreset = (tmpMode==CALL_MODE_INIT || tmpPreset==bootPreset); | ||
| const bool presetWillSetLor = (!fdo["lor"].isNull() && fdo["lor"].as<int>() > REALTIME_OVERRIDE_NONE); | ||
|
|
||
| // During setup, only allow the boot preset itself or safe boot-preset chains. | ||
| const bool shouldAllowPresetApply = ( | ||
| setupComplete || isBootPreset || | ||
| (currentPreset == bootPreset && realtimeOverride > REALTIME_OVERRIDE_NONE) || | ||
| (currentPreset == bootPreset && currentPlaylist > 0 && presetWillSetLor) | ||
| ); | ||
| if (shouldAllowPresetApply) { | ||
| presetToApply = 0; //clear request for preset | ||
| callModeToApply = 0; | ||
| } else { | ||
| releaseJSONBufferLock(); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
This check is inappropriate and should be removed. We should not limit which presets are applied at boot. Supporting setting the boot preset to something like {"ps": "1~5r"}, so it essentially randomly selects a preset between 1 and 5 at boot, would be a useful feature.
There was a problem hiding this comment.
Existing path:
boot preset is set -> realtime mode starts -> main loop queues boot preset -> realtime mode ends -> boot preset is applied
New path:
boot preset is set -> if boot preset overrides realtime mode, boot preset is applied immediately, if not, the behavior above applies (this is why it is gated like this)
Proposed change:
Same as "New path" if boot preset sets realtime override
If boot preset does not set realtime override:
boot preset is applied -> realtime starts -> transition from boot preset to realtime
This could make sense, but it introduces an unexpected transition from the boot preset to the realtime data. For a user that wants to boot into realtime data, there is no longer a way to achieve this without turning off boot preset, which means that there is no preset state in the queue for when/if realtime ends.
This is specifically why I gated it the way that it did. Perhaps the documentation is a little unclear - it's not that the boot preset is never getting applied when it does not set lor, it's that it remains queued until realtime is ended, which preserves the previously existing behavior.
There was a problem hiding this comment.
boot preset is set -> realtime mode starts -> main loop queues boot preset
This is a nondeterministic race, and this outcome is not and cannot be guaranteed -- whether the selected boot preset is applied by the main loop before or after processing the first event that triggers real-time mode will be sensitive to local network timing and subtle code timing changes in WLED or the platform toolkit we depend on. I don't think we should encourage users to bet on races, regardless of how often they might win in their particular configurations.
Always applying the boot preset in setup() has the major advantage of being strictly deterministic about the starting state. (As well as offering a guarantee that the "first light" state will be exactly the user preset state instead of only "off" or "yellow" -- this has been a source of confusion in the past!)
If someone wants the behavior of "start up and wait for a little bit to allow real-time mode to start, then fall back to preset N", I think this could be implemented by setting a boot preset with a little playlist, eg. {"playlist":{"ps":[n_waiting, n_fallback], "dur": [ wait_time, 1]}}, where n_waiting points to the waiting state preset, and n_fallback is the "fallback after live mode ends" preset.
There was a problem hiding this comment.
I tend to agree with you. I believe I overthought this in an attempt to preserve existing behavior as much as possible rather than go all in on new behavior. The reality is that the new behavior is better, and that's the whole point. Check my latest commit.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wled00/json.cpp`:
- Line 523: The assignment that remaps callMode currently lacks a terminating
semicolon; update the statement that checks "if (callMode == CALL_MODE_INIT)
callMode = CALL_MODE_DIRECT_CHANGE" by adding a trailing semicolon before the
comment so the expression becomes a valid C++ statement (ensure you modify the
occurrence handling CALL_MODE_INIT -> CALL_MODE_DIRECT_CHANGE within json.cpp
where callMode is remapped).
In `@wled00/presets.cpp`:
- Line 285: The function deletePreset in presets.cpp ends with an extra stray
character "w" after its closing brace ("}w"), which breaks compilation; remove
the trailing "w" so the function ends with a single closing brace "}", and scan
the surrounding lines in the deletePreset implementation to ensure no other
accidental characters remain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8d18151e-40b4-497e-bc41-7b3ce9f4aca8
📒 Files selected for processing (3)
wled00/json.cppwled00/presets.cppwled00/wled.cpp
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
willmmiles
left a comment
willmmiles
left a comment
There was a problem hiding this comment.
It always warms my heart when the necessary patch turns out to be so minimal after all is said and done. Nice job.
Thanks, and thank you for being an attentive, helpful maintainer. Too often maintainers are simply jerks or don't care or both. The team and the project here make me want to continue to contribute. |
4 participants
Problem
Previously, boot preset application depended on the normal loop path.
handlePresets()in the main loop is gated behind realtime mode checks:if (!realtimeMode || realtimeOverride || (realtimeMode && useMainSegmentOnly))If realtime mode became active before the first normal preset processing pass, the boot preset could be skipped or only partially applied. This was especially problematic for boot presets intended to set
LiveDataOverride(lor).Solution
This PR adds
handlePresets()andhandlePlaylist()calls duringsetup().The startup-time calls are intentionally placed:
UsermodManager::setup()serializeConfigToFS())This placement avoids calling usermod JSON handlers before setup while ensuring boot presets are processed deterministically during startup.
Behavior Changes
Earlier and More Consistent “LEDs On” Timing
For users utilizing a boot preset with
on:true, LEDs may now turn on earlier during boot.Because the boot preset is now applied earlier in the startup sequence, lights may illuminate faster than before. During testing, this range varied greatly with a range of 100ms faster to almost 2s faster.
This is particularly beneficial for users powering controllers from a physical wall switch who expect lights to turn on immediately when power is applied.
Users who intentionally relied on the previous delayed or inconsistent timing — particularly setups sensitive to brownouts or power fluctuations — may need to adjust any related delays accordingly.
While the previous behavior could vary depending on timing and realtime state, the new behavior should provide more consistent startup timing across reboots and power-on events on the controller.
Improvements to Boot Preset Capabilities
lorduring startup.psExample:
{ "lor": 2, "ps": 250 }This is useful for a boot preset to establish realtime override behavior first and then immediately advance to another preset.
Summary by CodeRabbit