Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a global setupComplete flag, marks setup start/end in WLED::setup(), runs boot preset/playlist handling during setup (two-pass for boot presets), and tightens preset-apply and ChangesPreset Initialization and Boot Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
3 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.lor:1orlor:2at the top level of the playlist, or in the first playlist entry.lor:1orlor:2can set a single chained preset using thepsJSON API.Example:
{ "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