Add clocked playlist scheduling#5663
Conversation
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds playlist clock-sync and deterministic shuffle across the editor, playlist option flags, runtime timing and ordering, loading and cleanup, and JSON serialization. ChangesClock sync and deterministic shuffle
Sequence Diagram(s)sequenceDiagram
participant PlaylistEditor
participant plUpdateOptions
participant PlaylistManager
participant TokiTime
participant Strip
PlaylistEditor->>plUpdateOptions: toggle shuffle / deterministic / clockSync / manual
plUpdateOptions->>PlaylistManager: persist playlist option state
PlaylistManager->>TokiTime: getClockSyncPlaylistCycle
TokiTime-->>PlaylistManager: cycle number + elapsed time
PlaylistManager->>PlaylistManager: mapCycleTimeToPlaylistSlot
PlaylistManager->>Strip: apply selected playlist entry
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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:
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/data/index.js`:
- Line 1599: The toast message in showToast uses a ternary expression with the
wrong precedence, so for s.error < 100 it loses the error code and errstr.
Update the expression in the shown toast logic to parenthesize the conditional
choice first, then concatenate the selected prefix with s.error and errstr so
both paths render the full message correctly.
In `@wled00/playlist.cpp`:
- Around line 95-101: The clock-sync check in playlist timing logic is too
permissive because it only rejects TOKI_TS_NONE, allowing unsynced or inaccurate
sources to drive the schedule. Update the validation in the playlist time
calculation path to require a real wall-clock source, using the Toki time source
check in the playlist code to reject TOKI_TS_UDP and TOKI_TS_BAD and only
proceed for a synced source at or above TOKI_TS_SEC. Keep the existing
absoluteMs, cycleNum, and cycleTime computation unchanged once the source is
validated.
- Around line 247-260: The playlist advance logic in the clock-sync path
currently falls back to local/manual timing when getClockSyncPlaylistCycle()
fails, which can make a clock-synced playlist run unsynchronized. Update the
flow around the clock sync handling in playlist.cpp so that when playlistOptions
includes PL_OPTION_CLOCK_SYNC and clock sync time is unavailable, the playlist
does not enter the normal local-clock advance branch; instead, keep it stalled
or otherwise gated until clockSyncTimeValid becomes true, while preserving the
existing rollover behavior in the clock-sync branch.
🪄 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: da0f5c5d-3316-4e1e-97f0-b4db64f910ab
📒 Files selected for processing (5)
wled00/const.hwled00/data/index.jswled00/fcn_declare.hwled00/playlist.cppwled00/prng.h
💤 Files with no reviewable changes (1)
- wled00/fcn_declare.h
✅ Files skipped from review due to trivial changes (1)
- wled00/const.h
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 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/data/index.js`:
- Line 1599: The toast message in showToast uses a ternary expression with the
wrong precedence, so for s.error < 100 it loses the error code and errstr.
Update the expression in the shown toast logic to parenthesize the conditional
choice first, then concatenate the selected prefix with s.error and errstr so
both paths render the full message correctly.
In `@wled00/playlist.cpp`:
- Around line 95-101: The clock-sync check in playlist timing logic is too
permissive because it only rejects TOKI_TS_NONE, allowing unsynced or inaccurate
sources to drive the schedule. Update the validation in the playlist time
calculation path to require a real wall-clock source, using the Toki time source
check in the playlist code to reject TOKI_TS_UDP and TOKI_TS_BAD and only
proceed for a synced source at or above TOKI_TS_SEC. Keep the existing
absoluteMs, cycleNum, and cycleTime computation unchanged once the source is
validated.
- Around line 247-260: The playlist advance logic in the clock-sync path
currently falls back to local/manual timing when getClockSyncPlaylistCycle()
fails, which can make a clock-synced playlist run unsynchronized. Update the
flow around the clock sync handling in playlist.cpp so that when playlistOptions
includes PL_OPTION_CLOCK_SYNC and clock sync time is unavailable, the playlist
does not enter the normal local-clock advance branch; instead, keep it stalled
or otherwise gated until clockSyncTimeValid becomes true, while preserving the
existing rollover behavior in the clock-sync branch.
🪄 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: da0f5c5d-3316-4e1e-97f0-b4db64f910ab
📒 Files selected for processing (5)
wled00/const.hwled00/data/index.jswled00/fcn_declare.hwled00/playlist.cppwled00/prng.h
💤 Files with no reviewable changes (1)
- wled00/fcn_declare.h
✅ Files skipped from review due to trivial changes (1)
- wled00/const.h
🛑 Comments failed to post (3)
wled00/data/index.js (1)
1599-1599: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Operator precedence drops the error code and message for
s.error < 100.In JS,
+binds tighter than?:, so this parses as(s.error<100) ? 'Error ' : ('Note ' + s.error + ": " + errstr). For any error code below 100 (e.g.1"Denied!"), the toast shows only"Error "— the code anderrstrare lost. Wrap the ternary in parentheses so the prefix selection happens before concatenation.🐛 Proposed fix
- showToast((s.error<100) ? 'Error ': 'Note ' + s.error + ": " + errstr, true); // show "please restart" as a note, all others as errors + showToast(((s.error<100) ? 'Error ' : 'Note ') + s.error + ": " + errstr, true); // show "please restart" as a note, all others as errors📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.showToast(((s.error<100) ? 'Error ' : 'Note ') + s.error + ": " + errstr, true); // show "please restart" as a note, all others as errors🤖 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/data/index.js` at line 1599, The toast message in showToast uses a ternary expression with the wrong precedence, so for s.error < 100 it loses the error code and errstr. Update the expression in the shown toast logic to parenthesize the conditional choice first, then concatenate the selected prefix with s.error and errstr so both paths render the full message correctly.wled00/playlist.cpp (2)
95-101: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require a real wall-clock source for clock sync.
Line 96 only rejects
TOKI_TS_NONE, soTOKI_TS_UDPandTOKI_TS_BADare accepted even though the Toki contract marks them as unsynced or inaccurate. That undermines the “Clock sync” guarantee. Consider requiring at leastTOKI_TS_SECor a stricter threshold.Proposed fix
- if (toki.getTimeSource() == TOKI_TS_NONE) return false; + if (toki.getTimeSource() < TOKI_TS_SEC) return false;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if (!(playlistOptions & PL_OPTION_CLOCK_SYNC) || playlistTotalDur == 0) return false; if (toki.getTimeSource() < TOKI_TS_SEC) return false; Toki::Time tm = toki.getTime(); uint64_t absoluteMs = (uint64_t)tm.sec * 1000ULL + tm.ms; cycleNum = (uint32_t)(absoluteMs / playlistTotalDur); cycleTime = absoluteMs % playlistTotalDur;🤖 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 95 - 101, The clock-sync check in playlist timing logic is too permissive because it only rejects TOKI_TS_NONE, allowing unsynced or inaccurate sources to drive the schedule. Update the validation in the playlist time calculation path to require a real wall-clock source, using the Toki time source check in the playlist code to reject TOKI_TS_UDP and TOKI_TS_BAD and only proceed for a synced source at or above TOKI_TS_SEC. Keep the existing absoluteMs, cycleNum, and cycleTime computation unchanged once the source is validated.
247-260: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don’t silently run clock-sync playlists on local timing.
When clock sync is enabled but
getClockSyncPlaylistCycle()fails, Line 260 falls back to the normal local/manual advance path. That makes a “Clock sync” playlist play unsynchronized until time becomes valid or when total duration is zero.Proposed fix
- if (playlistOptions & PL_OPTION_CLOCK_SYNC) { + bool clockSyncEnabled = playlistOptions & PL_OPTION_CLOCK_SYNC; + if (clockSyncEnabled) { uint32_t cycleNum = 0; if (getClockSyncPlaylistCycle(cycleNum, clockSyncCycleTime)) { clockSyncTimeValid = true; targetCycleNum = cycleNum; if (playlistIndex < 0 || playlistCycleNum != targetCycleNum) needsRollover = true; - // Clock-synced playlists derive the active slot from wall time, so manual "next preset" cannot persist. - doAdvancePlaylist = false; } + // Clock-synced playlists derive the active slot from wall time, so manual "next preset" cannot persist. + doAdvancePlaylist = false; + if (!clockSyncTimeValid) return; } // Derive local-clock/manual advance state - if (!clockSyncTimeValid && ((playlistEntryDur < UINT32_MAX && now - presetCycledTime > playlistEntryDur) || doAdvancePlaylist)) { + if (!clockSyncEnabled && ((playlistEntryDur < UINT32_MAX && now - presetCycledTime > playlistEntryDur) || doAdvancePlaylist)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// Derive wall-clock rollover state bool clockSyncEnabled = playlistOptions & PL_OPTION_CLOCK_SYNC; if (clockSyncEnabled) { uint32_t cycleNum = 0; if (getClockSyncPlaylistCycle(cycleNum, clockSyncCycleTime)) { clockSyncTimeValid = true; targetCycleNum = cycleNum; if (playlistIndex < 0 || playlistCycleNum != targetCycleNum) needsRollover = true; } // Clock-synced playlists derive the active slot from wall time, so manual "next preset" cannot persist. doAdvancePlaylist = false; if (!clockSyncTimeValid) return; } // Derive local-clock/manual advance state if (!clockSyncEnabled && ((playlistEntryDur < UINT32_MAX && now - presetCycledTime > playlistEntryDur) || doAdvancePlaylist)) {🤖 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 247 - 260, The playlist advance logic in the clock-sync path currently falls back to local/manual timing when getClockSyncPlaylistCycle() fails, which can make a clock-synced playlist run unsynchronized. Update the flow around the clock sync handling in playlist.cpp so that when playlistOptions includes PL_OPTION_CLOCK_SYNC and clock sync time is unavailable, the playlist does not enter the normal local-clock advance branch; instead, keep it stalled or otherwise gated until clockSyncTimeValid becomes true, while preserving the existing rollover behavior in the clock-sync branch.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
wled00/playlist.cpp (1)
292-294: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid scanning the playlist on every main-loop iteration.
handlePlaylist()runs from the main loop, and this callsmapCycleTimeToPlaylistSlot()every time clock sync is valid. Cache the current slot window and only remap when the cycle changes, the current entry window no longer containsclockSyncCycleTime, or a deliberate resync interval expires.🤖 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 292 - 294, handlePlaylist() currently remaps the clock sync slot on every main-loop pass whenever clockSyncTimeValid is true, which is too expensive. Update the clock-sync path in handlePlaylist() to cache the current playlist slot/window and reuse it until the cycle time changes, the cached entry no longer covers clockSyncCycleTime, or a resync interval forces refresh. Keep the logic centered around mapCycleTimeToPlaylistSlot() and the current playlist index/offset state so the remap only happens when necessary.
🤖 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/data/index.js`:
- Around line 3113-3117: The playlist editor path in makeP()/refreshPlE() only
backfills missing fields and can leave persisted clock-sync playlists in an
invalid state. Update the JSON load normalization so that when a playlist has
clockSync enabled but all durations are zero, clockSync is cleared or otherwise
normalized before rendering the segment UI. Make this adjustment in the existing
playlist parsing flow around the makeP(p, true) and refreshPlE(p) calls so the
editor reflects the same constraints enforced by playlist.cpp.
In `@wled00/playlist.cpp`:
- Around line 322-324: The playlist serialization in playlist.cpp is saving the
current playlistEntries order even when deterministic shuffle is enabled, which
can persist a cycle-specific shuffled order instead of the canonical one. Update
the playlist-saving path around the playlist object serialization to write an
unshuffled/original entry order for deterministic shuffle, or keep the shuffled
state separate from the stored entries so reloads start from the same canonical
sequence. Use the playlistEntries and playlistOptions logic that sets
deterministic and shuffle flags to locate the fix.
- Around line 207-214: In playlist.cpp, the clockSync handling in the playlist
JSON load path accepts entries with zero duration even though clock-synced
playback cannot use manual-advance slots. Update the playlist parsing logic
around the shuffle/clockSync flags to inspect the loaded entries before setting
PL_OPTION_CLOCK_SYNC, and if any entry has dur == 0, reject or clear clockSync
instead of enabling it. Use the existing playlistObj parsing and playlistOptions
bitmask flow to keep the validation in the untrusted JSON ingress path.
---
Nitpick comments:
In `@wled00/playlist.cpp`:
- Around line 292-294: handlePlaylist() currently remaps the clock sync slot on
every main-loop pass whenever clockSyncTimeValid is true, which is too
expensive. Update the clock-sync path in handlePlaylist() to cache the current
playlist slot/window and reuse it until the cycle time changes, the cached entry
no longer covers clockSyncCycleTime, or a resync interval forces refresh. Keep
the logic centered around mapCycleTimeToPlaylistSlot() and the current playlist
index/offset state so the remap only happens when necessary.
🪄 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: 488b1c0a-3ae5-498a-a8bd-d08980a0a764
📒 Files selected for processing (5)
wled00/const.hwled00/data/index.jswled00/fcn_declare.hwled00/playlist.cppwled00/prng.h
💤 Files with no reviewable changes (1)
- wled00/fcn_declare.h
| if (!plJson[p].deterministic) plJson[p].deterministic = false; | ||
| if (!plJson[p].clockSync) plJson[p].clockSync = false; | ||
| if (isNaN(plJson[p].end)) plJson[p].end = 0; | ||
| gId('seg' +i).innerHTML = makeP(p,true); | ||
| refreshPlE(p); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize persisted clock-sync playlists before rendering.
The PR explicitly says clock-sync playlists do not support zero-duration/manual-advance entries, but this path only backfills keys and then renders the editor. If a playlist is loaded from JSON with clockSync: true and all dur[] == 0, wled00/playlist.cpp will reject clock-sync because playlistTotalDur == 0, while the UI still shows Clock sync as enabled until the user toggles an option. Normalize that combination here (or clear clockSync) before makeP()/refreshPlE().
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 3115-3115: Avoid assigning untrusted data to innerHTML/outerHTML or document.write
Context: gId('seg' +i).innerHTML = makeP(p,true)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(inner-outer-html)
🤖 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/data/index.js` around lines 3113 - 3117, The playlist editor path in
makeP()/refreshPlE() only backfills missing fields and can leave persisted
clock-sync playlists in an invalid state. Update the JSON load normalization so
that when a playlist has clockSync enabled but all durations are zero, clockSync
is cleared or otherwise normalized before rendering the segment UI. Make this
adjustment in the existing playlist parsing flow around the makeP(p, true) and
refreshPlE(p) calls so the editor reflects the same constraints enforced by
playlist.cpp.
| playlist["r"] = (playlistOptions & PL_OPTION_SHUFFLE) != 0; | ||
| playlist[F("deterministic")] = (playlistOptions & PL_OPTION_DETERMINISTIC_SHUFFLE) != 0; | ||
| playlist[F("clockSync")] = (playlistOptions & PL_OPTION_CLOCK_SYNC) != 0; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Persist the canonical order for deterministic shuffle.
The new flags are serialized while playlistEntries may already be shuffled in place. Saving that state preserves a cycle-specific order with deterministic=true; on reload it gets shuffled again, so controllers starting from the original playlist can diverge. Serialize an unshuffled/canonical order for deterministic shuffle, or keep shuffle order separate from entry storage.
🤖 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 322 - 324, The playlist serialization in
playlist.cpp is saving the current playlistEntries order even when deterministic
shuffle is enabled, which can persist a cycle-specific shuffled order instead of
the canonical one. Update the playlist-saving path around the playlist object
serialization to write an unshuffled/original entry order for deterministic
shuffle, or keep the shuffled state separate from the stored entries so reloads
start from the same canonical sequence. Use the playlistEntries and
playlistOptions logic that sets deterministic and shuffle flags to locate the
fix.
|
This might also be something to look into:
|
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