Allow use of presets (physical button) when receiving live data#5370
Allow use of presets (physical button) when receiving live data#5370IsaccoBenedetti wants to merge 10 commits into
Conversation
WalkthroughThis PR adds a feature flag ChangesRealtime Preset Switching
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
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. 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 |
|
Hi, thanks for your contribution to wled development 👍 I think this PR is a bit mis-labeled ... in fact it does not modify the button handling code. Buttons are read and button can be used as always while WLED is in "external control" mode. The PR does address a weakness that presets controlled by button are not effective until "real time mode" has ended. The PR actually changes the preset and playlist logic, by allowing preset changes and playlist activity during realtime mode. That's a behavioural change and I'm not sure we should go that way. Which the proposed change, the order in the main loop gets changed and presets are now handled prior to OTA, HUE and NightLight. To protect previous behaviour, there should be at new option in "sync interfaces" settings, something like "presets and playlist always active" (maybe find a better wording). I think the root cause of the problem that @IsaccoBenedetti wants to address is somewhere else: Lines 451 to 459 in f830ea4
|
|
PS the summary from @coderabbitai is rubbish, it completely missed the point 😄 |
This comment was marked as resolved.
This comment was marked as resolved.
@coderabbitai yes. And also add that consequences of the behavioural change still need investigation. |
|
I'll trigger the regeneration now. 🧠 Learnings used✅ Actions performedSummary regeneration triggered. |
@coderabbitai it's not clear to me what "LO=2" is actually meant for. please inspects utility functions like leds.cpp, set.cpp, cfg.cpp, XML.cpp, json.cpp to find out what this flag is doing. |
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 10090 🏁 Script executed: Length of output: 1632
LO = Live Override (defined in `#define` REALTIME_OVERRIDE_NONE 0
`#define` REALTIME_OVERRIDE_ONCE 1
`#define` REALTIME_OVERRIDE_ALWAYS 2How it works:
Implementation details:
Special behavior: When So LO=2 in a button preset tells WLED: "ignore all incoming realtime data and apply this preset instead" - which is the escape mechanism from realtime mode that this PR enables by allowing 🧠 Learnings used |
|
another option would be to add a checkmark "exit real time mode on button press" in the config and do just that, any button press will exit? |
This comment was marked as duplicate.
This comment was marked as duplicate.
I like your idea 👍 Edit: it's still a good chance to also address the "live mode has infinite timeout" problem - the behaviour is different from all other external control modes, which all use |
|
which one is "live mode"? I only ever used DDP and that one times out after a few seconds. |
Not sure ... its triggered by a json message with Some external tools (hyperion? falcon player?) appear to use this command, but then don't send |
softhack007
changed the title
softhack007
changed the title
|
@coderabbitai please research the internet on protocol specifications related to the "live mode" or "realtime control" supported by WLED. Which protocols are started by a json message Do these pprotocols mandate an "infinite timeout" until |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🌐 Web query: 💡 Result: WLED JSON API
|
This comment was marked as duplicate.
This comment was marked as duplicate.
|
@IsaccoBenedetti maybe the latest discussion / research is a bit off-topic. Can you explain your use case of the button solution?
|
|
Hi @softhack007, and thank you for your review! You're right, maybe I should explain my use case better. Basically, it's one of those classic "wife approval" factors: this wled instance drives a led strip in our living room, which I connected both to a HyperHDR instance (which is this fork of Hyperion: https://github.com/awawa-dev/HyperHDR) and to 2 physical buttons on the wall. To be honest, I feel that since api command LO=2 precise purpose is exactly to break out of realtime mode, any user that adds such a command to a preset will probably expect it to work this way, but I also agree that, since the order of the actions in the main loop is slightly changed now, it may introduce some unexpected consequences, though I haven't noticed any so far. Thus, adding a setting in sync interfaces is probably safer. I can look into it if you'd like, do you have any pointers on how you'd do it, since I'm not familiar with the codebase like you? Let me know what your thoughts are |
|
See also #5288 @softhack007 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
wled00/data/settings_sync.htm (1)
129-130: Reminder: regenerate embedded UI headers after this web UI change.Please run
npm run buildbefore firmware builds so the embedded headers are up to date.
As per coding guidelines: “wled00/data/**: When modifying web UI files, run npm run build to regenerate embedded headers before any firmware build”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/settings_sync.htm` around lines 129 - 130, You added/modified web UI elements in wled00/data/settings_sync.htm (checkboxes named "RLM" and "ROP"); regenerate the embedded UI headers by running the project's UI build step and committing the generated artifacts before doing any firmware build—run `npm run build` in the repository root to rebuild the embedded headers (so the generated header files reflect the changes to settings_sync.htm) and include those updated generated files in the PR.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@wled00/data/settings_sync.htm`:
- Around line 129-130: You added/modified web UI elements in
wled00/data/settings_sync.htm (checkboxes named "RLM" and "ROP"); regenerate the
embedded UI headers by running the project's UI build step and committing the
generated artifacts before doing any firmware build—run `npm run build` in the
repository root to rebuild the embedded headers (so the generated header files
reflect the changes to settings_sync.htm) and include those updated generated
files in the PR.
|
Hi, small update, I added the option in sync interfaces as suggested, and also refactored the main loop to avoid changing the order of actions. |
|
Hi @softhack007, just a quick ping on this. I've been running the new version (with the opt-in setting) on my setup without issues so far. Let me know if there is anything else you'd like me to change or if you need further tests before the merge. I'd be happy to help if I can. PS: I saw your reference to #5288. While it may seem related, I don't think it actually is: that issue is related to how the JSON API handles automatic timeouts when the stream ends, whereas my PR focuses on manual override while the stream is still active. Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
wled00/button.cpp (1)
370-370: 💤 Low valueOptional: simplify the redundant guard term.
Inside this branch
realtimeModeis already known to betrue, so!(realtimeMode && useMainSegmentOnly)reduces to!useMainSegmentOnly. (The current form mirrors the De Morgan negation of the main-loop skip condition, which is why the extrarealtimeMode &&crept in.)♻️ Proposed simplification
- if (realtimeMode && !realtimeOverride && !(realtimeMode && useMainSegmentOnly) && realtimeAllowPresets) { + if (realtimeMode && !realtimeOverride && !useMainSegmentOnly && realtimeAllowPresets) {🤖 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/button.cpp` at line 370, The if-condition uses a redundant term: inside the branch realtimeMode is true, so the subexpression !(realtimeMode && useMainSegmentOnly) should be simplified to !useMainSegmentOnly; update the condition in the if statement that references realtimeMode, realtimeOverride, useMainSegmentOnly, and realtimeAllowPresets to remove the redundant realtimeMode && portion (i.e., replace !(realtimeMode && useMainSegmentOnly) with !useMainSegmentOnly) so the logic is equivalent but clearer.
🤖 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/button.cpp`:
- Around line 368-376: The current comment near the realtime button override
branch is unclear about why handlePresets() is called twice; update the comment
to state that we first apply any queued preset (handlePresets()), then
advance/queue the next preset via handlePlaylist() which sets presetToApply
through applyPresetFromPlaylist(...), and finally call handlePresets() again to
actually apply that newly queued preset — mirroring the handlePresets();
handlePlaylist(); handlePresets(); pattern used for bootPreset in
wled00/wled.cpp; also explicitly note that this presets-first ordering in
realtime mode (when realtimeMode && !realtimeOverride && !useMainSegmentOnly &&
realtimeAllowPresets) is intentional and differs from the main loop order so
playlist advancement cannot override a button-requested preset.
---
Nitpick comments:
In `@wled00/button.cpp`:
- Line 370: The if-condition uses a redundant term: inside the branch
realtimeMode is true, so the subexpression !(realtimeMode && useMainSegmentOnly)
should be simplified to !useMainSegmentOnly; update the condition in the if
statement that references realtimeMode, realtimeOverride, useMainSegmentOnly,
and realtimeAllowPresets to remove the redundant realtimeMode && portion (i.e.,
replace !(realtimeMode && useMainSegmentOnly) with !useMainSegmentOnly) so the
logic is equivalent but clearer.
🪄 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: 50729389-e8c0-4729-a159-4a158ca3927d
📒 Files selected for processing (6)
wled00/button.cppwled00/cfg.cppwled00/data/settings_sync.htmwled00/set.cppwled00/wled.hwled00/xml.cpp
💤 Files with no reviewable changes (3)
- wled00/wled.h
- wled00/set.cpp
- wled00/xml.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- wled00/cfg.cpp
- wled00/data/settings_sync.htm
| // If in realtime mode and presets are explicitly allowed to override (e.g., via buttons), | ||
| // we process them here to avoid messing with the main loop. | ||
| if (realtimeMode && !realtimeOverride && !(realtimeMode && useMainSegmentOnly) && realtimeAllowPresets) { | ||
| handlePresets(); | ||
| if (!presetNeedsSaving()) { | ||
| handlePlaylist(); | ||
| } | ||
| handlePresets(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect main loop ordering/guards for handlePresets, handlePlaylist, presetNeedsSaving
rg -nP --type=cpp -C4 '\bhandlePresets\s*\(\)|\bhandlePlaylist\s*\(\)|\bpresetNeedsSaving\s*\(\)' wled00/wled.cpp
# Show handlePresets implementation to assess idempotency / queued-preset behavior
ast-grep --pattern 'void handlePresets() {
$$$
}'
# Show handlePlaylist implementation (does it queue via applyPreset?)
ast-grep --pattern 'void handlePlaylist() {
$$$
}'Repository: wled/WLED
Length of output: 900
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect the exact button.cpp block around the reported lines
echo "---- button.cpp context (lines ~340-410) ----"
nl -ba wled00/button.cpp | sed -n '340,410p'
# 2) Find definitions/signatures for handlePresets/handlePlaylist
echo "---- locate handlePresets definition ----"
rg -n --type=cpp '\bhandlePresets\s*\(' wled00 -g'*.cpp'
echo "---- locate handlePlaylist definition ----"
rg -n --type=cpp '\bhandlePlaylist\s*\(' wled00 -g'*.cpp'
# 3) Show main loop and bootPreset block rationale in wled.cpp
echo "---- wled.cpp context around first preset/playlist section ----"
nl -ba wled00/wled.cpp | sed -n '80,170p'
echo "---- wled.cpp context around presetNeedsSaving / handlePlaylist / handlePresets ----"
nl -ba wled00/wled.cpp | sed -n '360,440p'
echo "---- wled.cpp context around bootPreset > 0 block ----"
nl -ba wled00/wled.cpp | sed -n '470,530p'
# 4) Print handlePresets/handlePlaylist implementations once located
# (Use the first definition found per function to keep output bounded.)
HP_LINE=$(rg -n --type=cpp -m1 '\bhandlePresets\s*\(' wled00 -g'*.cpp' | cut -d: -f1)
HP_FILE=$(rg -n --type=cpp -m1 '\bhandlePresets\s*\(' wled00 -g'*.cpp' | cut -d: -f1 --output-separator='' | true)
# Can't reliably extract file via cut above because ripgrep output includes "file:line:..."
# Re-run with stable parsing:
HP_MATCH=$(rg -n --type=cpp -m1 '\bhandlePresets\s*\(' wled00 -g'*.cpp')
HP_FILE=$(echo "$HP_MATCH" | cut -d: -f1)
HP_LINE=$(echo "$HP_MATCH" | cut -d: -f2)
HL_MATCH=$(rg -n --type=cpp -m1 '\bhandlePlaylist\s*\(' wled00 -g'*.cpp')
HL_FILE=$(echo "$HL_MATCH" | cut -d: -f1)
HL_LINE=$(echo "$HL_MATCH" | cut -d: -f2)
echo "---- handlePresets implementation snapshot: ${HP_FILE}:${HP_LINE} ----"
nl -ba "$HP_FILE" | sed -n "$((HP_LINE-20)),$((HP_LINE+60))p"
echo "---- handlePlaylist implementation snapshot: ${HL_FILE}:${HL_LINE} ----"
nl -ba "$HL_FILE" | sed -n "$((HL_LINE-20)),$((HL_LINE+60))p"Repository: wled/WLED
Length of output: 160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- button.cpp context (lines ~350-390) ----"
cat -n wled00/button.cpp | sed -n '350,390p'
echo "---- locate handlePresets definition ----"
rg -n --type=cpp '\bhandlePresets\s*\(' wled00 -g'*.cpp'
echo "---- locate handlePlaylist definition ----"
rg -n --type=cpp '\bhandlePlaylist\s*\(' wled00 -g'*.cpp'
echo "---- wled.cpp context around presetNeedsSaving / handlePlaylist / handlePresets ----"
cat -n wled00/wled.cpp | sed -n '80,160p'
echo "---- wled.cpp context around bootPreset > 0 block ----"
cat -n wled00/wled.cpp | sed -n '480,535p'Repository: wled/WLED
Length of output: 8145
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- presets.cpp: handlePresets() implementation (around line ~145) ----"
cat -n wled00/presets.cpp | sed -n '110,240p'
echo "---- playlist.cpp: handlePlaylist() implementation (around line ~151) ----"
cat -n wled00/playlist.cpp | sed -n '120,240p'
echo "---- locate presetNeedsSaving() definition ----"
rg -n --type=cpp '\bpresetNeedsSaving\s*\(' wled00/presets.cpp wled00/*.cpp
echo "---- print region around presetNeedsSaving() definition (if found) ----"
PRESET_NEEDS_LINE=$(rg -n --type=cpp 'presetNeedsSaving\s*\(' wled00/presets.cpp | head -n1 | cut -d: -f2 || true)
PRESET_NEEDS_FILE=$(rg -n --type=cpp 'presetNeedsSaving\s*\(' wled00/presets.cpp | head -n1 | cut -d: -f1 || true)
if [[ -n "${PRESET_NEEDS_LINE:-}" && -n "${PRESET_NEEDS_FILE:-}" ]]; then
cat -n "$PRESET_NEEDS_FILE" | sed -n "$((PRESET_NEEDS_LINE-30)),$((PRESET_NEEDS_LINE+60))p"
else
echo "presetNeedsSaving() definition not found in presets.cpp (showing call sites only)."
fiRepository: wled/WLED
Length of output: 10629
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- presets.cpp: presetNeedsSaving() definition (top of file) ----"
cat -n wled00/presets.cpp | sed -n '1,120p'
echo "---- presets.cpp: call sites where presetNeedsSaving() gates behavior ----"
rg -n --type=cpp '\bpresetNeedsSaving\s*\(' wled00 -g'*.cpp' -g'*.h'Repository: wled/WLED
Length of output: 4701
Clarify the intent of the double handlePresets() call in realtime button override
handlePlaylist()does not apply the next preset directly; it queues one by settingpresetToApplyviaapplyPresetFromPlaylist(...), so the secondhandlePresets()is required to actually apply that queued preset.- Update the comment in
wled00/button.cpp(368-376) to describe the sequencing (“apply queued preset → optionally advance playlist/queue next preset → apply again”), matching the samehandlePresets(); handlePlaylist(); handlePresets();pattern used forbootPreset > 0inwled00/wled.cpp. - Confirm this presets-first ordering in realtime mode is intentionally different from the normal main loop (
handlePlaylist()thenhandlePresets()), so playlist advancement can’t override the button preset when realtime override is enabled.
🤖 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/button.cpp` around lines 368 - 376, The current comment near the
realtime button override branch is unclear about why handlePresets() is called
twice; update the comment to state that we first apply any queued preset
(handlePresets()), then advance/queue the next preset via handlePlaylist() which
sets presetToApply through applyPresetFromPlaylist(...), and finally call
handlePresets() again to actually apply that newly queued preset — mirroring the
handlePresets(); handlePlaylist(); handlePresets(); pattern used for bootPreset
in wled00/wled.cpp; also explicitly note that this presets-first ordering in
realtime mode (when realtimeMode && !realtimeOverride && !useMainSegmentOnly &&
realtimeAllowPresets) is intentional and differs from the main loop order so
playlist advancement cannot override a button-requested preset.
|
Hi, I had a look at PR #5573 and noticed this very useful suggestion from @smitty078 : #5573 (comment) I decided to implement it the way he suggested and I can confirm it's working as expected, without any need to interfere with the main loop. Thanks @smitty078 ! I kept the option in the sync interface section for now, but I feel at this point it should be safe to remove it completely. |
4 participants
Hi, first time contributor here!
Recently I ran into a small issue when using physical buttons and HyperHDR. I'm not sure if this was done by design, but currently when WLED is receiving a live data stream physical button inputs are not processed. This makes it impossible to regain local control over the lights once a live source is active, without opening the webUI.
Initially I tried assigning a preset with the API command LO=2 (Live Override) to a physical button, but it didn't work as expected, because the functions that handle preset logic (handlePresets() and handlePlaylist()) are located within the conditional block that is skipped when realtimeMode is true.
This issue has been previously reported by the community on the WLED Discourse forums, but it seems it never got resolved: Live data override on the physical button.
This PR aims to solve the issue, simply by moving the handlePresets() and handlePlaylist() function calls out of the if (!realtimeMode || ...) block in the main loop.
This way button presses and their associated API commands are now processed on every loop cycle, regardless of the live data state. When a button triggers a preset containing LO=2, the realtimeOverride flag is correctly set, allowing WLED to exit the live stream and apply the desired preset.
I was a bit worried these functions were purposely been left inside the check for performance reasons, so I tested it specifically on an ESP8266 instead of an ESP32 with a HyperHDR stream, but I couldn't notice any lag or jitter at all. It seems solid.
What do you think?
Summary by CodeRabbit
New Features