Add boot_lor usermod - enforces a configured realtime override at startup#5569
Add boot_lor usermod - enforces a configured realtime override at startup#5569
Conversation
Applies a configured realtime override (lor) value after initial network or UDP activity. Useful for API-driven setups (HomeKit, Home Assistant, etc.) where realtime streaming (DDP/E1.31) should not take control at boot. - Optional wait for first UDP packet - Optional additional delay before applying - Reasserts value for a configurable duration to handle startup races
|
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 (1)
WalkthroughAdds a new WLED usermod "boot_lor" that enforces a configured realtimeOverride at boot. Includes PlatformIO manifest, README, C++ usermod implementation with lifecycle/config/JSON hooks, and a new ChangesBoot LOR Usermod
Sequence DiagramsequenceDiagram
participant WLED as WLED System
participant Usermod as BootLor Usermod
participant Network as Network Layer
participant Config as Config System
WLED->>Usermod: setup()
Usermod->>Config: read "boot_lor" config
Network->>Usermod: connected() / WLED_CONNECTED
Usermod->>Usermod: record connected timestamp
Usermod->>Usermod: wait additionalWaitSec
Usermod->>WLED: set realtimeOverride = bootLor
Usermod->>Usermod: record firstAppliedMs, state = applied
Usermod->>Usermod: reassert until assertForSec elapsed
Usermod->>Usermod: set state = finished
Usermod->>Config: add runtime info to info -> u
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
usermods/boot_lor/usermod_boot_lor.cpp (1)
3-18: ⚡ Quick win4-space indentation throughout — guideline requires 2-space.
The entire class body uses 4-space indentation. The C++ coding conventions mandate 2-space indentation (no tabs) across the project.
As per coding guidelines: "Use 2-space indentation in C++ files (no tabs)."
🤖 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 `@usermods/boot_lor/usermod_boot_lor.cpp` around lines 3 - 18, The class BootLorUsermod is indented with 4 spaces throughout; update its formatting to use 2-space indentation everywhere (no tabs) for the class declaration and all member declarations/definitions (e.g., BootLorUsermod, _name, bootLor, waitForUdpPacket, additionalWaitSec, assertForSec, referenceMs, firstAppliedMs, referenceSet, applied, finished) so the file conforms to the project's 2-space C++ indentation guideline.
🤖 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 `@usermods/boot_lor/readme.md`:
- Line 33: Fix the typo in the README text: replace "relaibly" with "reliably"
in the sentence "Using JSON or HTTP API in a boot preset does not relaibly set
lor" so it reads "Using JSON or HTTP API in a boot preset does not reliably set
lor".
- Around line 47-49: Two fenced code blocks open without a language tag (e.g.
the block containing "WiFi connected → first UDP packet received → (optional
delay) → apply lor → assert for N seconds → stop" and the other fenced block
later) which triggers MD040; update each opening fence from ``` to ```text (or
another appropriate language specifier) so the Markdown linter is satisfied and
rendering improves. Locate the blocks by the exact fence content and replace the
three backticks with a language-tagged fence.
In `@usermods/boot_lor/usermod_boot_lor.cpp`:
- Around line 122-124: getId() currently returns USERMOD_ID_RESERVED which is
forbidden; update it to use a proper usermod ID: add a new constant e.g.
USERMOD_ID_59 = 59 to wled00/const.h and change usermod_boot_lor::getId() to
return USERMOD_ID_59; alternatively, if you don't want to claim a permanent slot
now, return the provided generic fallback constant (e.g.
USERMOD_ID_GENERIC_FALLBACK) instead—refer to the getId() method and the
constants in wled00/const.h when making the change.
---
Nitpick comments:
In `@usermods/boot_lor/usermod_boot_lor.cpp`:
- Around line 3-18: The class BootLorUsermod is indented with 4 spaces
throughout; update its formatting to use 2-space indentation everywhere (no
tabs) for the class declaration and all member declarations/definitions (e.g.,
BootLorUsermod, _name, bootLor, waitForUdpPacket, additionalWaitSec,
assertForSec, referenceMs, firstAppliedMs, referenceSet, applied, finished) so
the file conforms to the project's 2-space C++ indentation guideline.
🪄 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: e10ef806-04d4-42ee-abb7-93ee16c710cb
📒 Files selected for processing (3)
usermods/boot_lor/library.jsonusermods/boot_lor/readme.mdusermods/boot_lor/usermod_boot_lor.cpp
There was a problem hiding this comment.
♻️ Duplicate comments (1)
usermods/boot_lor/readme.md (1)
123-125:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language tag to the fenced code block.
Line 123 still opens a fence with plain
; use something liketext to satisfy MD040.Suggested fix
-``` +```text Boot LOR: [bootLor, state, realtimeOverride]</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@usermods/boot_lor/readme.mdaround lines 123 - 125, The fenced code block
starting within the README entry "Boot LOR: [bootLor, state, realtimeOverride]" needs a language tag to satisfy MD040; update the opening fence fromto a tagged fence such as ```text (and keep the closing fence
unchanged) so the block is explicitly marked as plain text.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Duplicate comments:
In@usermods/boot_lor/readme.md:
- Around line 123-125: The fenced code block starting with
in the README entry "Boot LOR: [bootLor, state, realtimeOverride]" needs a language tag to satisfy MD040; update the opening fence fromto a tagged fence such asas plain text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID:
2343374f-a0c9-4604-9942-f7904cfeb196📒 Files selected for processing (3)
usermods/boot_lor/readme.mdusermods/boot_lor/usermod_boot_lor.cppwled00/const.h
…raffic. Additional changes suggested by coderabbit including docstrings
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
usermods/boot_lor/usermod_boot_lor.cpp (2)
164-172: ⚡ Quick winConsider
F()wrappers on literal JSON keys/strings.
"u","Boot LOR","waiting","applied","finished"are string literals that live in RAM on ESP8266. WhileaddToJsonInfois not a hot-loop path, the guideline calls forF("string")on string constants. ArduinoJson'soperator[]andadd()both accept__FlashStringHelper*.As per coding guidelines: "Use F("string") for string constants in C++ to save RAM on ESP8266."
🤖 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 `@usermods/boot_lor/usermod_boot_lor.cpp` around lines 164 - 172, In addToJsonInfo, wrap literal JSON keys and constant strings with F() to store them in flash instead of RAM: use F("u") when indexing root (operator[]), F("Boot LOR") when calling createNestedArray, and F("finished")/F("applied")/F("waiting") when adding the status; keep adding bootLor and realtimeOverride unchanged and ensure JsonArray::add calls accept __FlashStringHelper* (they do) so replace only the string literals in the code paths that reference root["u"], createNestedArray("Boot LOR"), and add("...").
12-12: ⚡ Quick win
_nameshould usePROGMEMstorage per usermod coding guidelines.
static constexpr const char*places the pointer as constexpr but does not guarantee the string data lands in flash on ESP8266. The coding guideline explicitly requiresstatic const char[] PROGMEMfor repeated strings in usermod code.♻️ Proposed fix
- static constexpr const char* _name = "boot_lor"; ///< JSON configuration key for this usermod. + static const char _name[] PROGMEM; ///< JSON configuration key for this usermod.And add outside the class (before the static instance at line 184):
+const char BootLorUsermod::_name[] PROGMEM = "boot_lor";As per coding guidelines: "Store repeated strings as static const char[] PROGMEM in C++ usermod code."
🤖 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 `@usermods/boot_lor/usermod_boot_lor.cpp` at line 12, Replace the constexpr pointer with a PROGMEM-stored C-string: in the class, remove or replace "static constexpr const char* _name" and declare "static const char _name[] PROGMEM;" (use the actual class name, e.g., usermod_boot_lor::_name) and then add the definition outside the class: "const char <ClassName>::_name[] PROGMEM = "boot_lor";" so the string data is placed in flash; ensure the external definition appears before the static instance/registration so the symbol is available at link time.
🤖 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 `@usermods/boot_lor/readme.md`:
- Line 52: Remove all references to the removed option "waitForUdpPacket" in the
README: delete the behavioral line that says "Optionally waits for the first UDP
packet (recommended for DDP setups)", remove the "waitForUdpPacket": true
entries from both config JSON examples, and remove the waitForUdpPacket row from
the options table; replace one of those spots (e.g., under Options or
Recommended settings) with a short note stating the waitForUdpPacket option was
removed and any such key in user configs is ignored to avoid silent no-op
behavior.
---
Nitpick comments:
In `@usermods/boot_lor/usermod_boot_lor.cpp`:
- Around line 164-172: In addToJsonInfo, wrap literal JSON keys and constant
strings with F() to store them in flash instead of RAM: use F("u") when indexing
root (operator[]), F("Boot LOR") when calling createNestedArray, and
F("finished")/F("applied")/F("waiting") when adding the status; keep adding
bootLor and realtimeOverride unchanged and ensure JsonArray::add calls accept
__FlashStringHelper* (they do) so replace only the string literals in the code
paths that reference root["u"], createNestedArray("Boot LOR"), and add("...").
- Line 12: Replace the constexpr pointer with a PROGMEM-stored C-string: in the
class, remove or replace "static constexpr const char* _name" and declare
"static const char _name[] PROGMEM;" (use the actual class name, e.g.,
usermod_boot_lor::_name) and then add the definition outside the class: "const
char <ClassName>::_name[] PROGMEM = "boot_lor";" so the string data is placed in
flash; ensure the external definition appears before the static
instance/registration so the symbol is available at link time.
🪄 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: 0d6d8a9c-d36e-4a9d-94d0-6ae7118d1e20
📒 Files selected for processing (2)
usermods/boot_lor/readme.mdusermods/boot_lor/usermod_boot_lor.cpp
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
smitty078
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Clarified note about realtime streaming in readme.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Thanks for the submission, but it feels like we should fix the underlying issue of not being able to set that value at boot than a usermod messing about with state |
|
@coderabbitai please investigate why setting lor as part of a boot preset does not work 100% of the time |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me dig into the WLED codebase to trace the boot preset and 🏁 Script executed: Length of output: 9667 🏁 Script executed: Length of output: 11749 Let me look deeper at the preset async mechanism and the network/UDP initialization sequence. 🏁 Script executed: Length of output: 10606 🏁 Script executed: Length of output: 2863 🏁 Script executed: Length of output: 4902 🏁 Script executed: Length of output: 14528
Root Cause 1: Async/Deferred Preset Loading (Primary)
bool applyPreset(byte index, byte callMode) {
presetToApply = index; // just sets a flag
callModeToApply = callMode;
return true;
}The actual JSON (including Once WiFi connects and a streaming controller (DDP/E1.31/Hyperion) sends its first packet, Root Cause 2: JSON Buffer Lock Contention Widens the Window
if (presetToApply == 0 || !requestJSONBufferLock(JSON_LOCK_PRESET_LOAD)) return;During the boot period, the JSON buffer is frequently contended (WebSocket send, HTTP responses, etc.), so the boot preset application can be deferred across multiple loop iterations, giving streaming data even more opportunity to race ahead. Root Cause 3:
|
Thanks for the feedback. It’s encouraging that you’re interested in going after a fix in core so that presets can work reliably for this use case. As a (new) user looking for this use-case to work for me, the usermod was the quick fix I needed and thought I’d submit as it may help others looking for the same behavior. As I investigated initially I found a fair number of “setting lor in a preset doesn’t work” type reports around the web that corroborated my own experience. Having no prior knowledge of the codebase, I didn’t initially attempt to look for a fix in core. I’m happy to take a stab at it and/or test a proposed fix as this is my primary use-case currently, so I’m invested in it working. Is the way forward to work towards implementing a proposed fix myself, or await a fix from you/the team? |
@coderabbitai Option 1 would be flat out wrong. Live override is meant to be a dynamic property of the system, so that it can be enabled or disabled along with other all the other effect state. Persisting the value every time it's changed would cause excessive flash wear as the cfg would be constantly rewritten as that feature is turned on and off; the obvious alternative, naiively adding it to the cfg serialization, would have it be persisted only when some other cfg setting is saved, causing the at-boot behaviour to mysteriously change depending on what value it happened to have when some other setting was saved. Probably the correct fix here is to actually apply the boot preset JSON before starting the main loop (instead of using the defer-execution approach we use to manage resource contention). This would also have the benefit of being able to absolutely guarantee user-controlled initialization of the FX state before any frames are rendered. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainYou're absolutely right, and I apologize for the flawed Option 1 suggestion. Let me look at the boot sequence to better understand the synchronous pre-loop approach. 🏁 Script executed: Unrecoverable error: Failed to clone repository
You're right on both counts:
I've updated my understanding for future reviews. ✏️ Learnings added
|
|
I did my own analysis today and I’ve concluded that I believe the best and correct option is to call handlePresets() immediately after applyPreset() in startStrip(), where the boot preset is initially added to the queue. This totally resolves the issue with the only side effect being a single use case: a boot preset that does not set lor:1 or lor:2 is set AND realtime data is received immediate or soon after boot. In the case, the user would observe a preset-> realtime transition in state, which might be undesirable, but should not be unexpected. The solution would be to avoid a boot preset that sets state that interferes with potential realtime data. This is easily enough explained in documentation and is not a behavioral bug. |
|
Would it be appropriate to test this solution and submit a PR for it? Is your preference to have a new PR or to revert and amend this one for sake of discussion that has already occurred? |
4 participants
Applies a configured realtime override (lor) value after initial network activity.
Useful for API-driven setups (HomeKit, Home Assistant, etc.) where realtime streaming (DDP/E1.31) should not take control at boot.
Summary by CodeRabbit
New Features
Documentation
Chores