Add clocked playlist scheduling#5663
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds playlist clock-sync and deterministic shuffle: two new PL_OPTION flags, frontend clockSync/deterministic UI and centralized handler, backend deterministic per-cycle ordering and clock-to-cycle mapping, loading/cleanup updates, and serialization of the new flags. ChangesClock sync & deterministic shuffle
Sequence Diagram (high-level) sequenceDiagram
participant Frontend
participant PlaylistManager
participant TokiTime
participant Strip
Frontend->>PlaylistManager: set clockSync / deterministic via plUpdateOptions
PlaylistManager->>TokiTime: request current cycle (getClockSyncPlaylistCycle)
TokiTime-->>PlaylistManager: cycle number + elapsed in cycle
PlaylistManager->>PlaylistManager: mapCycleTimeToPlaylistSlot -> select playlistOrder[index]
PlaylistManager->>Strip: applyPlaylistEntry(selectedEntry)
🎯 4 (Complex) | ⏱️ ~45 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. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/playlist.cpp (1)
151-166:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate clocked entry durations at load time.
loadPlaylist()acceptsclocked:truewithdur == 0, butgetClockedPlaylistState()later rejects any zero-duration entry and the clocked branch just returns. That means API callers or editedpresets.jsoncan load a clocked playlist that never starts advancing, even though the UI normalizes these values before save.Suggested ingress-side normalization
- shuffle = shuffle || playlistObj["r"]; - if (playlistObj[F("clocked")] | false) playlistOptions |= PL_OPTION_CLOCKED; + shuffle = shuffle || playlistObj["r"]; + const bool clocked = playlistObj[F("clocked")] | false; + if (clocked) { + for (byte i = 0; i < playlistLen; i++) { + if (playlistEntries[i].dur == 0) playlistEntries[i].dur = 10000UL; // match UI default: 10.0s + } + playlistOptions |= PL_OPTION_CLOCKED; + } if (shuffle) playlistOptions |= PL_OPTION_SHUFFLE;As per coding guidelines, "Trust Boundary Model — enforce input-validation and bounds-checking rules ONLY at the first untrusted ingress point."
Also applies to: 198-199
🤖 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/playlist.cpp` around lines 151 - 166, loadPlaylist() currently allows clocked playlists to be loaded with zero-duration entries which later causes getClockedPlaylistState() to reject the playlist; enforce validation at load time by normalizing any dur==0 to a safe non-zero fallback (e.g., the default 100 tenths -> 10000 ms) or to the previous valid duration when parsing JsonArray durations and the single dur field, updating playlistEntries[].dur accordingly; modify the code paths that parse playlistObj["dur"] and the durations loop (and the analogous block at the other occurrence around the 198-199 mention) to replace zero durations with the fallback before storing so clocked playlists are always valid after load.
🤖 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/playlist.cpp`:
- Around line 224-255: The clocked-path returns early and therefore skips the
existing rollover/repeat/end-preset handling; update the PL_OPTION_CLOCKED
branch (the block guarded by playlistOptions & PL_OPTION_CLOCKED and using
getClockedPlaylistState, derivedIndex, entryOffset) so that after syncing
playlistIndex/timebase and applying the new preset it still runs the common
end-of-entry logic that handles playlistRepeat decrement, unloading the
playlist, and applying playlistEndPreset (i.e., do not return before the
existing repeat/end handling), or explicitly invoke that same rollover logic
from here (ensuring nightlightActive, doAdvancePlaylist, playlistRepeat and
playlistEndPreset are processed correctly) rather than bypassing it with an
early return.
---
Outside diff comments:
In `@wled00/playlist.cpp`:
- Around line 151-166: loadPlaylist() currently allows clocked playlists to be
loaded with zero-duration entries which later causes getClockedPlaylistState()
to reject the playlist; enforce validation at load time by normalizing any
dur==0 to a safe non-zero fallback (e.g., the default 100 tenths -> 10000 ms) or
to the previous valid duration when parsing JsonArray durations and the single
dur field, updating playlistEntries[].dur accordingly; modify the code paths
that parse playlistObj["dur"] and the durations loop (and the analogous block at
the other occurrence around the 198-199 mention) to replace zero durations with
the fallback before storing so clocked playlists are always valid after load.
🪄 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: 55ae4f6b-0105-46d5-a155-06c8f1cf88ed
📒 Files selected for processing (3)
wled00/const.hwled00/data/index.jswled00/playlist.cpp
@aeoa can you explain how this feature behaves different from time-controlled presets (settings -> time & macros -> Time-Controlled Presets) ? |
softhack007
added
enhancement
AI
Time-Controlled Presets trigger a specific preset / playlist at a specific time of day. |
|
do I understand correctly that what this tries to solve is time drift of different controllers over long periods of time where a playlist plays? like several hours? how much drift did you observe without this PR? |
The actual problem I am trying to solve is this: I have several separate mobile LED installations, each with their own controller but with the same playlist preset set to apply on boot. I want those installation to show the same effect at the same time such that they appear as one connected unit. We cannot rely on the WLED sync feature, because there is no reliable WIFI connection, and the controllers may not all be on the same network. The feature of this PR is that clocked playlists schedule their entries in a deterministic manner relative to wall clock. Say we have a playlist with three presets A, B, and C, each with a duration of 10s. Each controller is started at a different point in time. Until first NTP time fix, the first effect of the playlist is shown as a fallback. Once wall time is available, the playlist determines the correct effect for the current point in time and stays locked to wall time. This also works for shuffled playlists, because the seed for the randomization is also derived from the wall clock. The actual time synchronization is left to the Toki implementation already present in WLED. With NTP, time is only updated about twice a day. Within that time frame, the controller clocks may drift apart by a couple of seconds. Perhaps another PR could make that NTP update interval configurable. |
|
ok got it. |
Did your AI tell you? Because the playlist shuffle code uses the hardware random generator, which is not pseudo-random but a true random number based on entropy. Line 35 in bd0ebf9 Lines 484 to 485 in bd0ebf9 |
Normal playlists still use true randomness through Instead, a separate set of RNG functions are used for clocked playlists: The seed is computed from the playlist cycle, which is computed from the wall time and playlist duration: The seed is then used shuffle an array of playlist entry indices: This is then used in |
@DedeHai i share your view about this feature is "just" for a very specific usecase. However it might be very hard to implement it as a usermod, without re-implementing playlists at usermod level. Maybe its better to have the "clocked playlist steps" encapsulated with a feature flag (like WLED_ENABLE_TIME_IN_PLAYLIST), disabled by default? Any other feature flag name that is not too long should be ok... As a side-effect, we'd need to test that a "timed playlist step" will be ignored by a normal build. |
|
I think there's a valid use case for a feature like this, but I really don't like the implementation by creating a second, independent code path in the playlist logic. This approach will create a maintenance burden for any future playlist features: they'll have to be implemented twice. The fact that you had to consider (FWIW, this is a common problem with AI-generated code architectures -- it's biased towards copy-and-paste and bolting on new logic as an aside, "minimizing changes to the existing code", vs. rethinking the design to accommodate new goals properly.) Instead I suggest breaking down the feature into its primitive components:
The latter two could be implemented in the context of the existing Zero-duration entries should be treated with the existing semantics, ie. treated as terminal when applied. The fallback logic isn't worth the code complexity and weird special cases -- documenting exceptions to the usual behaviour is a huge pain in the butt and makes it hard for people to understand. On the bikeshed front: I don't love "clocked" as a name, either -- the past-tense form makes me think of hardware devices with clock strobe lines. Maybe "real time" or "wall time" or "absolute time". Thoughts? |
|
fully agree with @willmmiles re: naming |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
wled00/playlist.cpp (1)
190-198: 💤 Low valueClock-sync without deterministic shuffle may cause controller desync.
When
clockSyncis enabled butdeterministicis false (orris false), controllers will be on the same cycle position but with different shuffle orders. The PR objectives state "Shuffle for clocked playlists is deterministic per wall-clock playlist cycle" — consider whether enablingclockSyncshould implicitly enable deterministic shuffle when shuffle is active, or at minimum document this behavior.🤖 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/playlist.cpp` around lines 190 - 198, When clockSync is set but deterministic shuffle isn't, controllers can desync because they share cycle position but not shuffle order; update the logic in playlist.cpp (around playlistObj, shuffle, clockSync, deterministicShuffle, and playlistOptions) so that if clockSync is true and shuffle is active you also set deterministicShuffle (i.e., ensure PL_OPTION_DETERMINISTIC_SHUFFLE is enabled whenever PL_OPTION_CLOCK_SYNC and PL_OPTION_SHUFFLE are set), or at minimum add a comment documenting this implicit behavior; adjust the condition that currently sets PL_OPTION_DETERMINISTIC_SHUFFLE to check clockSync as well and set playlistOptions accordingly.
🤖 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.
Nitpick comments:
In `@wled00/playlist.cpp`:
- Around line 190-198: When clockSync is set but deterministic shuffle isn't,
controllers can desync because they share cycle position but not shuffle order;
update the logic in playlist.cpp (around playlistObj, shuffle, clockSync,
deterministicShuffle, and playlistOptions) so that if clockSync is true and
shuffle is active you also set deterministicShuffle (i.e., ensure
PL_OPTION_DETERMINISTIC_SHUFFLE is enabled whenever PL_OPTION_CLOCK_SYNC and
PL_OPTION_SHUFFLE are set), or at minimum add a comment documenting this
implicit behavior; adjust the condition that currently sets
PL_OPTION_DETERMINISTIC_SHUFFLE to check clockSync as well and set
playlistOptions accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e74b0c5e-32e5-4d8b-b5ab-58ebdc1555d4
📒 Files selected for processing (4)
wled00/const.hwled00/data/index.jswled00/fcn_declare.hwled00/playlist.cpp
💤 Files with no reviewable changes (1)
- wled00/fcn_declare.h
|
Thank you for the good feedback @willmmiles. I have reworked the implementation according to your suggestions. Changes:
I have tested this a bit on two ESP32C3, and it seems to do the right thing. I have not looked into the nightlight. Does this go in the right direction? |
|
Looking much better! I don't have time to do a deep review today, unfortunately - I'll give more detailed feedback next week. Some quick thoughts:
|
4 participants
What
Adds an optional
clockedmode for playlists. A clocked playlist derives its active entry from synchronized Toki time, so multiple WLED controllers with matching playlists and presets can stay aligned without continuously communicating with each other.This is useful for setups where controllers may have spotty network connectivity after initial time sync, where relying on frequent live sync packets is undesirable, or where controllers are not on the same local network but can sync to the same time source.
How
When
playlist.clockedis enabled, playlist handling uses absolute Toki time modulo the total finite playlist duration to select the active entry. The selected entry offset is also applied tostrip.timebase, so effects usingstrip.nowcan start from the same relative effect time.Shuffle remains deterministic per wall-clock playlist cycle, so matching controllers derive the same shuffled order.
The playlist editor now includes a
Clock synccheckbox. Manual advance is disabled for clocked playlists because zero-duration entries cannot be placed on a wall-clock timeline.Limitations
Clocked playlists require an existing time source, such as NTP enabled in WLED Time settings. They do not enable or change NTP settings automatically.
Zero-duration/manual-advance entries are unsupported. Nested playlists keep existing WLED behavior; child playlist duration wins once loaded.
Testing
Hardware sanity-tested on two ESP32-C3 during development
Summary by CodeRabbit
New Features
Bug Fixes / Improvements