-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Improve Boot preset handling #5573
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+11
−3
Merged
Changes from 8 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
f0e182b
Fix boot preset live data override handling
smitty078 88994e9
Merge branch 'wled:main' into boot-preset-handling
smitty078 83065c1
Initial implementation to apply the same improvements to boot playlist
smitty078 c8f50e6
Add TODO for boot preset and playlist scenarios
smitty078 0cd7097
Changes to implementation based on discussion.
smitty078 cfe7bad
call releaseJSONBufferLock() before returning so that the JSON buffer…
smitty078 b185a02
Merge branch 'wled:main' into boot-preset-handling
smitty078 8793db3
revert whitespace correction to avoid conflicts with other PRs
smitty078 722d6e8
Numerous changes based on conversation
smitty078 6641724
Consistency check to eliminate unneccesary conflicts
smitty078 a1a22ec
Merge branch 'wled:main' into boot-preset-handling-new
smitty078 f5ef3f9
Final conflict resolution
smitty078 741037a
Fix missing semi-colon and another error. Thanks coderabbit
smitty078 8e501d9
Do not reassign callMode, revert whitespace error
smitty078 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]}}, wheren_waitingpoints to the waiting state preset, andn_fallbackis the "fallback after live mode ends" preset.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.