Add FSEQ + FPP usermods for local playback#5641
Conversation
Introduce a new FSEQ usermod package: adds a local .fseq playback effect and an FPP-compatible "FPP Connect" usermod. Files added include the FSEQ player implementation (fseq_player.{h,cpp}), effect glue (fseq_effect.h), web UI manager, XLZ unzip support, SD adapter compatibility shim, registration, and library.json (depends on bitbank2/unzipLIB). Features: per-segment local playback, optional outgoing FPP sync, incoming FPP discovery/control (START/SYNC/PING/BLANK/STOP), FPP-style HTTP endpoints, deferred XLZ unpacking on uploads, and an SD management web UI. Also includes small updates to existing SD/WLED integration files to hook the new usermods into the build/runtime.
WalkthroughAdds an SD-backed FSEQ playback engine, an FPP-compatible network usermod with UDP/HTTP endpoints and upload handling, a browser SD manager UI, XLZ archive unpacking, and effect integration into WLED (new usermod IDs and realtime mode). ChangesFSEQ Playback System
FPP Network Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 |
coderabbitai
Bot
added
enhancement
connectivity
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
usermods/FSEQ/README.md (2)
199-199: 💤 Low valueMinor style improvement: avoid adverb repetition.
The phrase "scans the SD root only - extracts only the first file" repeats "only" twice in close succession. Consider rephrasing for smoother reading, e.g., "scans the SD root exclusively" or "extracts just the first file".
🤖 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/FSEQ/README.md` at line 199, Edit the README phrase "scans the SD root only - extracts only the first file" to avoid repeating "only"; change one occurrence to a synonym such as "scans the SD root exclusively - extracts just the first file" or "scans the SD root only - extracts just the first file" so the sentence reads smoothly (update the line containing "scans the SD root only - extracts only the first file").
299-299: 💤 Low valueSimplify wording for clarity.
"do not behave exactly the same" could be simplified to "behave differently" for more concise documentation.
🤖 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/FSEQ/README.md` at line 299, Replace the sentence "Browser upload and FPP upload do **not** behave exactly the same for `.xlz` files." with a more concise wording such as "Browser upload and FPP upload behave differently for `.xlz` files." — locate the exact string in the README and update it in place, preserving the `.xlz` code formatting and surrounding punctuation.usermods/FSEQ/fseq_player.cpp (1)
1-5: ⚡ Quick winMake
wled.hthe first include in this.cppfile.This keeps translation-unit setup consistent with the project convention and avoids subtle include-order regressions.
♻️ Proposed change
-#include "fseq_player.h" -#include "usermod_fseq.h" -#include "wled.h" -#include "sd_adapter_compat.h" +#include "wled.h" +#include "fseq_player.h" +#include "usermod_fseq.h" +#include "sd_adapter_compat.h" `#include` <Arduino.h>As per coding guidelines
Include "wled.h" as the primary project header in C++.🤖 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/FSEQ/fseq_player.cpp` around lines 1 - 5, Move the project primary header include so "wled.h" is the first include in this translation unit: open fseq_player.cpp and reorder the include directives so that `#include` "wled.h" appears before "fseq_player.h", "usermod_fseq.h", "sd_adapter_compat.h" and <Arduino.h>, preserving all existing includes and keeping their relative order otherwise; this aligns with the project convention and prevents include-order regressions.usermods/FSEQ/fseq_player.h (1)
4-9: ⚡ Quick winReplace these header macros with
constexprconstants.These values live in a public header, so
#defineleaks global names into every includer and drops type safety. A typedconstexprkeeps the same behavior without polluting the preprocessor namespace.As per coding guidelines: "Prefer
constexprover#definefor compile-time constants in C++."🤖 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/FSEQ/fseq_player.h` around lines 4 - 9, Replace the preprocessor macros with typed constexpr variables: remove the `#define` entries for RECORDING_REPEAT_LOOP and RECORDING_REPEAT_DEFAULT in fseq_player.h and declare them as constexpr (preferably inline constexpr) int constants with the same values (e.g. -1 and 0) so they retain compile-time semantics without polluting the preprocessor namespace; update any includes/usages as needed to use the new constexpr names (RECORDING_REPEAT_LOOP, RECORDING_REPEAT_DEFAULT).usermods/FSEQ/xlz_unzip.cpp (1)
141-141: ⚡ Quick winPrefer WLED allocators for the unzip buffer.
Line 141 uses
malloc, but this code should use WLED allocators to keep memory placement consistent and reduce heap-fragmentation risk on embedded targets.♻️ Suggested change
- uint8_t* buffer = static_cast<uint8_t*>(malloc(XLZ_BUFFER_SIZE)); + uint8_t* buffer = static_cast<uint8_t*>(d_malloc(XLZ_BUFFER_SIZE)); if (!buffer) { DEBUG_PRINTLN(F("[XLZ] Failed to allocate unzip buffer")); out.close(); SD_ADAPTER.remove(outputPath.c_str()); zip.closeCurrentFile(); return false; } @@ - free(buffer); + free(buffer);As per coding guidelines: “Use d_malloc() (DRAM-preferred) / p_malloc() (PSRAM-preferred) for allocation in C++.”
🤖 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/FSEQ/xlz_unzip.cpp` at line 141, Replace the direct malloc call that allocates the unzip buffer with the WLED allocator recommended for this target: instead of uint8_t* buffer = static_cast<uint8_t*>(malloc(XLZ_BUFFER_SIZE)); use the appropriate WLED allocator (d_malloc for DRAM-preferred or p_malloc for PSRAM-preferred) to allocate XLZ_BUFFER_SIZE for the variable buffer; ensure you keep the uint8_t* buffer variable and free it with the matching deallocator used in the codebase.
🤖 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/FSEQ/fseq_effect.h`:
- Around line 67-79: The code currently uses fileCount as the playback reload
key which causes a reload whenever any file is added/removed; instead persist
and compare the last-resolved filename/path for the segment and only reload when
that filename actually changes. Modify the logic around
fileCount/selectedIndex/currentFileName (where
FSEQ_getFileNameByIndex(selectedIndex, currentFileName) is called and
SEGMENT.custom3 is used) to read a per-segment storedLastFileName, update that
stored value when FSEQ_getFileNameByIndex succeeds, and trigger reload only if
storedLastFileName != currentFileName (do the same replacement in the other
occurrence referenced around the second block). Ensure the storedLastFileName
survives between calls for the same segment (e.g., attach it to the segment
state) and remove reliance on fileCount for reload decisions.
- Around line 134-152: The code only sends a START when transports change but
never sends a STOP for transports that were just disabled; update the branch
around needsStart in the FSEQ effect sync logic to detect when
_fseq_lastSendMulticast[segId] != sendSyncMulticast or
_fseq_lastSendBroadcast[segId] != sendSyncBroadcast and for each transport that
was previously true but is now false call FPP_sendEffectSyncMessage with the
STOP opcode (same params as the START/periodic send) before sending the START
for the newly enabled transport; ensure you update
_fseq_lastSendMulticast[segId] and _fseq_lastSendBroadcast[segId] only after
emitting STOP(s) and the subsequent START so receivers on the disabled transport
get the STOP.
In `@usermods/FSEQ/usermod_fpp.h`:
- Around line 789-805: Add an inactivity timeout cleanup so stale upload state
(uploadStream, currentUploadFile, uploadSessionActive, currentUploadFileName) is
cleared when a client disconnected mid-transfer: before returning the 409 in the
index==0 branch (and the other upload branches referenced), check if
uploadSessionActive && (millis() - uploadStartTime) > UPLOAD_INACTIVITY_MS; if
true, close and delete the WriteBufferingStream (uploadStream), close and remove
the partial file via SD_ADAPTER.remove(currentUploadFileName.c_str()), set
currentUploadFile to nullptr, delete uploadStream, set uploadSessionActive=false
and reset currentUploadFileName/uploadStartTime, then continue to open a fresh
file; apply the same timeout/cleanup logic to the other upload handling blocks
(the ones around the other referenced ranges) to prevent persistent 409s.
- Around line 161-168: The current rememberSyncProgress() (called from
processUdpPacket()) directly mutates shared playback/status fields
(lastFppSyncSeconds, lastFppSyncMillis, lastSequenceName via
cacheSequenceName()) without synchronization; change this by introducing a
single loop-consumed queue or command snapshot mechanism so all mutations to FPP
sync/status state happen in loop() under a single serialized context (use the
existing fppMux or a new queue). Update rememberSyncProgress(),
buildSystemStatusJSON(), getStableStatusElapsedSeconds(), and
clearPlaybackStatusCache() callers to either enqueue update commands (for
UDP/HTTP handlers) or read from an immutable snapshot object provided by loop(),
and have loop() dequeue/apply updates to lastFppSyncSeconds, lastFppSyncMillis,
lastStatusElapsed*, and lastSequenceName (via cacheSequenceName()) to ensure
consistent, serialized state changes.
In `@usermods/FSEQ/web_ui_manager.cpp`:
- Around line 389-390: The code prepends a slash to the incoming filename but
does not block directory traversal or backslash variants; update the validation
where the variable filename is normalized (the site that currently does if
(!filename.startsWith("/")) filename = "/" + filename;) and the other place
handling delete/upload paths to first URL-decode and canonicalize the path, then
reject any inputs containing "..", "\" or "/"-backslash mixes (and
percent-encoded traversal like "%2e%2e"), or that resolve outside the intended
upload directory; return/emit an error response on invalid input instead of
using the unsafe filename.
- Around line 8-11: The upload code currently ignores the return value of
File::write which allows short/partial writes to be treated as success; modify
the write calls that use UploadContext::file (the occurrences around the blocks
noted at lines ~374-381 and ~409) to check the returned byte count against the
expected bytes written, and if it is less than expected or write() returns an
error set UploadContext::error = true, abort the upload, close the FILE handle,
delete/unlink the partial file, and ensure the request returns a proper failure
response; also ensure any subsequent logic checks UploadContext::error before
marking the upload as successful.
- Around line 128-133: The code uses title.innerHTML = '<span
class="code">#'+f.index+'</span> '+f.name in the data.fseqFiles loop which risks
XSS because f.name is user-controlled; change this to build DOM nodes and set
textContent for the filename (e.g., create the span for '#'+f.index via a
textNode or span.textContent and append a separate text node or element for
f.name using textContent) so no untrusted string is assigned to innerHTML;
update the loop that handles data.fseqFiles and remove direct innerHTML usage
(reference: data.fseqFiles.forEach, title.innerHTML).
In `@usermods/sd_card/sd_card.cpp`:
- Around line 32-35: The defaults for configPinSourceSelect/Clock/Poci/Pico were
changed to -1 but the snapshot variables (uint8_t oldPin...) used for change
detection convert -1 to 255, causing false change detection; fix by making the
snapshot variables use a signed type matching the config pins (e.g., change the
uint8_t oldPin... variables to int8_t oldPin...) or otherwise ensure the
comparison uses a signed cast so comparisons between
configPinSourceSelect/configPinSourceClock/configPinPoci/configPinPico and
oldPin... are done as signed values; update the variables used in the
change-detection comparisons (the oldPin... identifiers and any comparisons
around where reinit is triggered) so -1 is handled correctly.
---
Nitpick comments:
In `@usermods/FSEQ/fseq_player.cpp`:
- Around line 1-5: Move the project primary header include so "wled.h" is the
first include in this translation unit: open fseq_player.cpp and reorder the
include directives so that `#include` "wled.h" appears before "fseq_player.h",
"usermod_fseq.h", "sd_adapter_compat.h" and <Arduino.h>, preserving all existing
includes and keeping their relative order otherwise; this aligns with the
project convention and prevents include-order regressions.
In `@usermods/FSEQ/fseq_player.h`:
- Around line 4-9: Replace the preprocessor macros with typed constexpr
variables: remove the `#define` entries for RECORDING_REPEAT_LOOP and
RECORDING_REPEAT_DEFAULT in fseq_player.h and declare them as constexpr
(preferably inline constexpr) int constants with the same values (e.g. -1 and 0)
so they retain compile-time semantics without polluting the preprocessor
namespace; update any includes/usages as needed to use the new constexpr names
(RECORDING_REPEAT_LOOP, RECORDING_REPEAT_DEFAULT).
In `@usermods/FSEQ/README.md`:
- Line 199: Edit the README phrase "scans the SD root only - extracts only the
first file" to avoid repeating "only"; change one occurrence to a synonym such
as "scans the SD root exclusively - extracts just the first file" or "scans the
SD root only - extracts just the first file" so the sentence reads smoothly
(update the line containing "scans the SD root only - extracts only the first
file").
- Line 299: Replace the sentence "Browser upload and FPP upload do **not**
behave exactly the same for `.xlz` files." with a more concise wording such as
"Browser upload and FPP upload behave differently for `.xlz` files." — locate
the exact string in the README and update it in place, preserving the `.xlz`
code formatting and surrounding punctuation.
In `@usermods/FSEQ/xlz_unzip.cpp`:
- Line 141: Replace the direct malloc call that allocates the unzip buffer with
the WLED allocator recommended for this target: instead of uint8_t* buffer =
static_cast<uint8_t*>(malloc(XLZ_BUFFER_SIZE)); use the appropriate WLED
allocator (d_malloc for DRAM-preferred or p_malloc for PSRAM-preferred) to
allocate XLZ_BUFFER_SIZE for the variable buffer; ensure you keep the uint8_t*
buffer variable and free it with the matching deallocator used in the codebase.
🪄 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: fe641aa8-23d1-4a5b-b071-dc98bba3811a
📒 Files selected for processing (16)
usermods/FSEQ/README.mdusermods/FSEQ/fseq_effect.husermods/FSEQ/fseq_player.cppusermods/FSEQ/fseq_player.husermods/FSEQ/library.jsonusermods/FSEQ/register_usermod.cppusermods/FSEQ/sd_adapter_compat.husermods/FSEQ/usermod_fpp.husermods/FSEQ/usermod_fseq.husermods/FSEQ/web_ui_manager.cppusermods/FSEQ/web_ui_manager.husermods/FSEQ/xlz_unzip.cppusermods/FSEQ/xlz_unzip.husermods/sd_card/sd_card.cppwled00/const.hwled00/json.cpp
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
usermods/FSEQ/fseq_player.cpp (1)
38-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle mixed-case
.fseqsuffixes.This only accepts two exact spellings, so valid files like
/show.Fseqnever enter the index and cannot be selected by index/name lookup. Please switch to a real case-insensitive suffix check and reuse it in the SD UI filter so both paths stay aligned.🐛 Proposed fix
+static bool endsWithIgnoreCase(const String &value, const char *suffix) { + const size_t n = strlen(suffix); + if (value.length() < n) return false; + return value.substring(value.length() - n).equalsIgnoreCase(suffix); +} + bool isFseqFileName(const String &name) { - return name.endsWith(".fseq") || name.endsWith(".FSEQ"); + return endsWithIgnoreCase(name, ".fseq"); }🤖 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/FSEQ/fseq_player.cpp` around lines 38 - 40, The isFseqFileName function currently only matches two exact spellings; change it to perform a case-insensitive suffix check by normalizing the input (e.g., toLowerCase) and checking endsWith(".fseq") inside isFseqFileName, and update the SD UI file-filter code to call and reuse isFseqFileName instead of duplicating its logic so mixed-case variants like "Fseq" or "fSeQ" are accepted consistently across indexing and the UI.usermods/FSEQ/web_ui_manager.cpp (2)
336-339:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the same case-insensitive FSEQ check here.
This filter repeats the same exact-case suffix test as the indexer. Mixed-case names like
/show.Fseqwill still land inotherFileseven though they are valid FSEQ recordings. Share one ignore-case helper between the cache and this endpoint.🤖 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/FSEQ/web_ui_manager.cpp` around lines 336 - 339, The filtering logic in web_ui_manager.cpp builds a filename String called name and uses name.endsWith(".fseq") || name.endsWith(".FSEQ"), which is case-sensitive and inconsistent with the indexer; replace this with a single case-insensitive check (e.g., compare name.toLowerCase().endsWith(".fseq") or call the shared helper used by the indexer such as isFseqFile/IsFseqSuffix) so mixed-case filenames like "/show.Fseq" are recognized as FSEQ; update the branch around the name variable (the block that creates otherFiles.createNestedObject()) to use that shared helper or toLowerCase() check to avoid duplicating logic.
235-242:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon’t show “Upload complete” when /api/sd/upload returns a 4xx/5xx
xhr.onloadruns when the request finishes even for HTTP error statuses, but the UI currently always reports success. Gate the success path onxhr.status(and showxhr.responseTexton failure).🐛 Proposed fix
xhr.onload=function(){ - statusText.innerText='Upload complete'; - loadSDList(); + if (xhr.status >= 200 && xhr.status < 300) { + statusText.innerText='Upload complete'; + loadSDList(); + } else { + statusText.innerText=xhr.responseText||('Upload failed ('+xhr.status+')'); + } setTimeout(()=>{ progressBar.style.width='0%'; statusText.innerText=''; },2000); }; + + xhr.onerror=function(){ + statusText.innerText='Upload failed'; + };🤖 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/FSEQ/web_ui_manager.cpp` around lines 235 - 242, xhr.onload currently treats any finished request as success; update xhr.onload to check xhr.status (e.g., success if 200–299) before setting statusText to 'Upload complete' and calling loadSDList(), otherwise set statusText to the error text (xhr.responseText or a short fallback) to indicate failure and avoid showing success; also ensure progressBar reset behavior still runs (on success or after showing the error) so the UI clears consistently.usermods/FSEQ/xlz_unzip.cpp (1)
243-251:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAccount for the existing destination file in the free-space check.
unpackCurrentFile()deletesfinalOutputPathbefore writing, but this precheck ignores the bytes that would be reclaimed. Replacing an existing.fseqcan therefore fail spuriously on a nearly full card even when the overwrite would fit.🐛 Proposed fix
+ uint64_t existingOutputSize = 0; + if (SD_ADAPTER.exists(finalOutputPath.c_str())) { + File existing = SD_ADAPTER.open(finalOutputPath.c_str(), FILE_READ); + if (existing) { + existingOutputSize = existing.size(); + existing.close(); + } + } + const uint64_t totalBytes = SD_ADAPTER.totalBytes(); const uint64_t usedBytes = SD_ADAPTER.usedBytes(); - const uint64_t freeBytes = (totalBytes >= usedBytes) ? (totalBytes - usedBytes) : 0; + const uint64_t freeBytes = + (totalBytes >= usedBytes) ? (totalBytes - usedBytes + existingOutputSize) + : existingOutputSize; if (fileInfo.uncompressed_size > freeBytes) {🤖 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/FSEQ/xlz_unzip.cpp` around lines 243 - 251, The free-space precheck in unpackCurrentFile() uses fileInfo.uncompressed_size vs freeBytes from SD_ADAPTER but ignores that finalOutputPath is deleted before writing, so an existing file's bytes would be reclaimed; update the check to account for the current destination file size by querying the filesystem for finalOutputPath (e.g., get its size if it exists) and treat effectiveFree = freeBytes + existingFileSize (or reduce required bytes by existingFileSize) before comparing to fileInfo.uncompressed_size; keep the existing DEBUG_PRINTF, call g_xlzZip.closeZIP() on failure as before.usermods/FSEQ/fseq_effect.h (1)
96-118:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't cache the filename until the segment actually starts playing.
If
loadRecordingForSegment()fails once, Line 108 still recordscurrentFileName, sofileChangedstays false forever and this segment never retries the load until the user changes selection.🔧 Localized fix
_fseq_lastIndex[segId] = selectedIndex; _fseq_lastLoop[segId] = loop; - _fseq_lastFileName[segId] = currentFileName; FSEQPlayer::setSegmentLooping(segId, loop); if (!FSEQPlayer::isSegmentPlaying(segId)) { + _fseq_lastFileName[segId] = String(); stopFseqSyncIfNeeded(segId, currentFileName); _fseq_wasPlaying[segId] = false; _fseq_syncStartMs[segId] = 0; _fseq_lastSyncMs[segId] = 0; SEGMENT.fill(BLACK); return; } + + _fseq_lastFileName[segId] = currentFileName;🤖 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/FSEQ/fseq_effect.h` around lines 96 - 118, The code caches currentFileName into _fseq_lastFileName[segId] unconditionally even when loadRecordingForSegment() fails, preventing future retry; fix by moving the assignment of _fseq_lastFileName[segId] = currentFileName (and any state that should reflect a successful load) so it only happens after the segment actually starts playing (e.g., after calling FSEQPlayer::loadRecordingForSegment(...) and verifying FSEQPlayer::isSegmentPlaying(segId) or another definitive success signal), leaving _fseq_lastFileName unchanged on load failure so fileChanged will be true and the load will be retried later.
🧹 Nitpick comments (1)
usermods/FSEQ/xlz_unzip.cpp (1)
153-170: ⚡ Quick winUse
delay(1)in these long extraction loops.These loops can run for large archives, and
yield()is not the watchdog-safe pattern this repo expects on ESP32. Replacing both call sites withdelay(1)keeps extraction cooperative during long runs.As per coding guidelines, “ESP32 timing: don’t assume `yield()` feeds watchdog; if using custom loops/tasks, use `delay(1)` rather than `yield()`.”♻️ Proposed fix
- yield(); + delay(1); @@ - yield(); + delay(1);Also applies to: 325-326
🤖 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/FSEQ/xlz_unzip.cpp` around lines 153 - 170, The long extraction loop using yield() (see zip.readCurrentFile, out.write and the written variable in xlz_unzip.cpp) must use delay(1) to be watchdog-safe on ESP32; replace the yield() call inside the loop with delay(1) and also change the other yield() call mentioned around lines 325-326 to delay(1) so long-running decompression stays cooperative with the ESP32 watchdog.
🤖 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/FSEQ/fseq_player.h`:
- Around line 4-5: The constants static constexpr int16_t RECORDING_REPEAT_LOOP
and RECORDING_REPEAT_DEFAULT are defined before any includes so int16_t may be
undefined; move these two definitions below the header includes (e.g., after
`#include` "wled.h" or other existing includes) so the fixed-width integer type is
available, keeping the same names and values.
In `@usermods/FSEQ/usermod_fpp.h`:
- Around line 716-733: The bug is that uploadSessionActive is being used for two
different meanings (XLZ-pending vs upload-in-progress) causing XLZ uploads to be
left pending and never unpacked; introduce a separate boolean (e.g.,
uploadInProgress or uploadAcceptingUploads) to represent whether new uploads are
allowed/ongoing and keep uploadSessionActive reserved for "xlzPendingScan" or
rename uploadSessionActive to xlzPendingScan usage; update
finishSuccessfulFppUpload (and the code paths that set/clear uploadSessionActive
like the non-.xlz branch) to set xlzPendingScan = true for ".xlz" and only touch
the new uploadInProgress flag for upload lifecycle, and change the upload
acceptance check (the code that currently inspects open handles /
uploadSessionActive) to consult the new uploadInProgress flag so an XLZ pending
state does not cancel/disable the pending unpack.
- Around line 682-714: cleanupFppUploadState and cleanupStaleFppUploadIfNeeded
race with the AsyncWebServer POST handler because uploadStream,
currentUploadFile, currentUploadFileName, uploadSessionActive, xlzPendingScan
and lastUploadActivity are accessed from multiple FreeRTOS tasks; protect all
accesses and mutations of these shared members by introducing a single mutex (or
single-owner queue/event handoff) and lock it in cleanupFppUploadState,
cleanupStaleFppUploadIfNeeded, the /fpp POST handler (where
uploadStream/currentUploadFile are written/flushed), and any loop-side checks
that read uploadSessionActive/xlzPendingScan/lastUploadActivity so reads are
consistent. Also fix the XLZ lifecycle in finishSuccessfulFppUpload by
explicitly clearing xlzPendingScan when the uploaded file is not an .xlz (or by
setting/clearing uploadSessionActive/xlzPendingScan under the same lock) so a
non-.xlz upload cannot leave xlzPendingScan true and prevent the loop scan
condition (uploadSessionActive && xlzPendingScan) from ever running.
---
Outside diff comments:
In `@usermods/FSEQ/fseq_effect.h`:
- Around line 96-118: The code caches currentFileName into
_fseq_lastFileName[segId] unconditionally even when loadRecordingForSegment()
fails, preventing future retry; fix by moving the assignment of
_fseq_lastFileName[segId] = currentFileName (and any state that should reflect a
successful load) so it only happens after the segment actually starts playing
(e.g., after calling FSEQPlayer::loadRecordingForSegment(...) and verifying
FSEQPlayer::isSegmentPlaying(segId) or another definitive success signal),
leaving _fseq_lastFileName unchanged on load failure so fileChanged will be true
and the load will be retried later.
In `@usermods/FSEQ/fseq_player.cpp`:
- Around line 38-40: The isFseqFileName function currently only matches two
exact spellings; change it to perform a case-insensitive suffix check by
normalizing the input (e.g., toLowerCase) and checking endsWith(".fseq") inside
isFseqFileName, and update the SD UI file-filter code to call and reuse
isFseqFileName instead of duplicating its logic so mixed-case variants like
"Fseq" or "fSeQ" are accepted consistently across indexing and the UI.
In `@usermods/FSEQ/web_ui_manager.cpp`:
- Around line 336-339: The filtering logic in web_ui_manager.cpp builds a
filename String called name and uses name.endsWith(".fseq") ||
name.endsWith(".FSEQ"), which is case-sensitive and inconsistent with the
indexer; replace this with a single case-insensitive check (e.g., compare
name.toLowerCase().endsWith(".fseq") or call the shared helper used by the
indexer such as isFseqFile/IsFseqSuffix) so mixed-case filenames like
"/show.Fseq" are recognized as FSEQ; update the branch around the name variable
(the block that creates otherFiles.createNestedObject()) to use that shared
helper or toLowerCase() check to avoid duplicating logic.
- Around line 235-242: xhr.onload currently treats any finished request as
success; update xhr.onload to check xhr.status (e.g., success if 200–299) before
setting statusText to 'Upload complete' and calling loadSDList(), otherwise set
statusText to the error text (xhr.responseText or a short fallback) to indicate
failure and avoid showing success; also ensure progressBar reset behavior still
runs (on success or after showing the error) so the UI clears consistently.
In `@usermods/FSEQ/xlz_unzip.cpp`:
- Around line 243-251: The free-space precheck in unpackCurrentFile() uses
fileInfo.uncompressed_size vs freeBytes from SD_ADAPTER but ignores that
finalOutputPath is deleted before writing, so an existing file's bytes would be
reclaimed; update the check to account for the current destination file size by
querying the filesystem for finalOutputPath (e.g., get its size if it exists)
and treat effectiveFree = freeBytes + existingFileSize (or reduce required bytes
by existingFileSize) before comparing to fileInfo.uncompressed_size; keep the
existing DEBUG_PRINTF, call g_xlzZip.closeZIP() on failure as before.
---
Nitpick comments:
In `@usermods/FSEQ/xlz_unzip.cpp`:
- Around line 153-170: The long extraction loop using yield() (see
zip.readCurrentFile, out.write and the written variable in xlz_unzip.cpp) must
use delay(1) to be watchdog-safe on ESP32; replace the yield() call inside the
loop with delay(1) and also change the other yield() call mentioned around lines
325-326 to delay(1) so long-running decompression stays cooperative with the
ESP32 watchdog.
🪄 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: f6d4dbf1-a4cb-429e-834c-cd709eb7ee38
📒 Files selected for processing (8)
usermods/FSEQ/README.mdusermods/FSEQ/fseq_effect.husermods/FSEQ/fseq_player.cppusermods/FSEQ/fseq_player.husermods/FSEQ/usermod_fpp.husermods/FSEQ/web_ui_manager.cppusermods/FSEQ/xlz_unzip.cppusermods/sd_card/sd_card.cpp
✅ Files skipped from review due to trivial changes (1)
- usermods/FSEQ/README.md
| static constexpr int16_t RECORDING_REPEAT_LOOP = -1; | ||
| static constexpr int16_t RECORDING_REPEAT_DEFAULT = 0; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba usermods/FSEQ/fseq_player.h | sed -n '1,12p'Repository: wled/WLED
Length of output: 96
🏁 Script executed:
#!/bin/bash
set -euo pipefail
awk '{printf "%4d\t%s\n", NR, $0}' usermods/FSEQ/fseq_player.h | sed -n '1,25p'Repository: wled/WLED
Length of output: 822
Move RECORDING_REPEAT_* below the includes in usermods/FSEQ/fseq_player.h
int16_t is used at lines 4–5 before any header (including #include "wled.h") is included, so the header isn’t self-contained and can break when included first in a translation unit.
🔧 Proposed fix
-static constexpr int16_t RECORDING_REPEAT_LOOP = -1;
-static constexpr int16_t RECORDING_REPEAT_DEFAULT = 0;
-
`#include` "wled.h"
`#include` "sd_adapter_compat.h"
+
+static constexpr int16_t RECORDING_REPEAT_LOOP = -1;
+static constexpr int16_t RECORDING_REPEAT_DEFAULT = 0;🧰 Tools
🪛 Clang (14.0.6)
[error] 4-4: unknown type name 'constexpr'
(clang-diagnostic-error)
[error] 4-4: expected ';' after top level declarator
(clang-diagnostic-error)
[error] 5-5: unknown type name 'constexpr'
(clang-diagnostic-error)
[error] 5-5: expected ';' after top level declarator
(clang-diagnostic-error)
🤖 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/FSEQ/fseq_player.h` around lines 4 - 5, The constants static
constexpr int16_t RECORDING_REPEAT_LOOP and RECORDING_REPEAT_DEFAULT are defined
before any includes so int16_t may be undefined; move these two definitions
below the header includes (e.g., after `#include` "wled.h" or other existing
includes) so the fixed-width integer type is available, keeping the same names
and values.
| void cleanupFppUploadState(bool removePartialFile) { | ||
| const String partialFile = currentUploadFileName; | ||
|
|
||
| if (uploadStream) { | ||
| delete uploadStream; | ||
| uploadStream = nullptr; | ||
| } | ||
|
|
||
| if (currentUploadFile) { | ||
| currentUploadFile.close(); | ||
| } | ||
|
|
||
| if (removePartialFile && partialFile.length() > 0) { | ||
| SD_ADAPTER.remove(partialFile.c_str()); | ||
| DEBUG_PRINTF("[FPP] Removed partial upload: %s\n", partialFile.c_str()); | ||
| } | ||
|
|
||
| currentUploadFile = File(); | ||
| currentUploadFileName = ""; | ||
| uploadStartTime = 0; | ||
| lastUploadActivity = 0; | ||
| uploadSessionActive = false; | ||
| xlzPendingScan = false; | ||
| } | ||
|
|
||
| void cleanupStaleFppUploadIfNeeded() { | ||
| if (!uploadStream && !currentUploadFile) return; | ||
| if (lastUploadActivity == 0) return; | ||
| if (millis() - lastUploadActivity < uploadInactivityTimeout) return; | ||
|
|
||
| DEBUG_PRINTLN(F("[FPP] Upload timed out; cleaning partial state")); | ||
| cleanupFppUploadState(true); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For ESPAsyncWebServer on ESP32, does the HTTP upload body callback run outside the Arduino loop() context, and should state shared between that callback and loop() be protected with a mutex or serialized queue?
💡 Result:
Yes: the HTTP upload/body callbacks are not run in the Arduino sketch’s loop context on ESP32; they execute from the AsyncTCP/AsyncWebServer callback task (i.e., a separate FreeRTOS task), so they can run concurrently with loop. Evidence: AsyncTCP is documented/configured as a separate task that runs the network event queue and “executes server callbacks” (CONFIG_ASYNC_TCP_RUNNING_CORE / async_tcp task), not in loop [1][2]. A GitHub issue discussing callback context on ESP32 explicitly notes “loop is running in a task … and the ESPAsyncWebserver is running in a separate task” [3]. Should shared state between that callback and loop be protected (mutex or serialized queue)? - Treat it as concurrent access and protect any non-atomic shared variables you read/write from both contexts. The GitHub issue confirms you “have to consider concurrency access” and only safe cases are atomic/primitive types with only simple reads/writes (no read-modify-write), and same-core assumptions [3]. - If you need to move structured data or do read-modify-write, use a synchronization mechanism (mutex, critical section) or a serialized queue/event-transfer pattern so loop is the sole consumer. The general safe pattern is to have the callback quickly store incoming data into a queue and return, then have loop process it. Practical guidance: 1) Keep the upload/body callback short; accumulate data carefully. The library states handleBody/handleUpload callbacks are called multiple times while parsing [4][5]. 2) For communication to loop, prefer a queue or “set flags + copy plain primitive data” approach; only mutex-guard complex structures or read-modify-write operations. 3) You do not need a mutex for values that are truly atomic and are only ever written or read (no RMW) from both contexts [3]. Bottom line: The callback runs outside loop, so assume concurrency and synchronize shared state unless you can prove it’s atomic/plain and only accessed with simple read/write operations [3][1].
Citations:
- 1: https://components.espressif.com/components/esp32async/asynctcp/versions/3.3.3~1?language=en
- 2: ESP32Async/AsyncTCP@2c8ad8b
- 3: Callback context on ESP32? me-no-dev/ESPAsyncWebServer#657
- 4: https://github.com/ESP32Async/ESPAsyncWebServer/tree/429428353dcdb45f48c98adb596fb44b218a202e
- 5: https://github.com/me-no-dev/ESPAsyncWebServer/blob/master/README.ESP32Async.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="usermods/FSEQ/usermod_fpp.h"
# Show the referenced functions/snippets with line numbers
sed -n '640,760p' "$FILE" | cat -n
echo "-----"
sed -n '880,1045p' "$FILE" | cat -n
echo "-----"
sed -n '1045,1185p' "$FILE" | cat -n
echo "-----"
# Find all uses of the lifecycle flags
rg -n "uploadSessionActive|xlzPendingScan|cleanupStaleFppUploadIfNeeded|cleanupFppUploadState|finishSuccessfulFppUpload" "$FILE"
echo "-----"
# Identify the /fpp upload callback and whether it touches shared upload state
rg -n "(/fpp|fpp).*upload|on\\(.*upload|handle\\w*Upload|upload\\w*\\(" "$FILE" || true
# Print nearby server route setup if found
rg -n "server\\.on\\(|AsyncWebServer|AsyncWebServerRequest|AsyncWebServer|AsyncWebHandler|server" "$FILE" | head -n 80Repository: wled/WLED
Length of output: 17479
Fix concurrent upload-state access and XLZ pending lifecycle in FSEQ usermod.
cleanupStaleFppUploadIfNeeded()/cleanupFppUploadState()(loop-side, lines 682-714) can delete/closeuploadStreamandcurrentUploadFilewhile the/fppAsyncWebServer POST handler (lines 931-1009) is simultaneously writing/flushing them—ESPAsyncWebServer callbacks run outside Arduinoloop()in a separate FreeRTOS task. Add proper serialization (e.g., mutex or a single-owner queue/event handoff) for the shared upload members (uploadStream,currentUploadFile,currentUploadFileName,uploadSessionActive,xlzPendingScan,lastUploadActivity).finishSuccessfulFppUpload()(lines 716-733) setsxlzPendingScan = trueonly for.xlz, but the non-.xlzpath never clearsxlzPendingScan. Since the/fpphandler only rejects whenuploadStream || currentUploadFileis set (not when an XLZ is pending), starting a new non-.xlzupload can setuploadSessionActive = falseand leavexlzPendingScan = true, so the pending XLZ never runs the loop condition (uploadSessionActive && xlzPendingScan, lines 1128-1136).
🤖 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/FSEQ/usermod_fpp.h` around lines 682 - 714, cleanupFppUploadState
and cleanupStaleFppUploadIfNeeded race with the AsyncWebServer POST handler
because uploadStream, currentUploadFile, currentUploadFileName,
uploadSessionActive, xlzPendingScan and lastUploadActivity are accessed from
multiple FreeRTOS tasks; protect all accesses and mutations of these shared
members by introducing a single mutex (or single-owner queue/event handoff) and
lock it in cleanupFppUploadState, cleanupStaleFppUploadIfNeeded, the /fpp POST
handler (where uploadStream/currentUploadFile are written/flushed), and any
loop-side checks that read uploadSessionActive/xlzPendingScan/lastUploadActivity
so reads are consistent. Also fix the XLZ lifecycle in finishSuccessfulFppUpload
by explicitly clearing xlzPendingScan when the uploaded file is not an .xlz (or
by setting/clearing uploadSessionActive/xlzPendingScan under the same lock) so a
non-.xlz upload cannot leave xlzPendingScan true and prevent the loop scan
condition (uploadSessionActive && xlzPendingScan) from ever running.
| void finishSuccessfulFppUpload(const String &uploadedFile) { | ||
| String lowerName = uploadedFile; | ||
| lowerName.toLowerCase(); | ||
|
|
||
| lastUploadFinished = millis(); | ||
| lastUploadActivity = lastUploadFinished; | ||
| currentUploadFileName = ""; | ||
| currentUploadFile = File(); | ||
| uploadStartTime = 0; | ||
|
|
||
| if (lowerName.endsWith(".xlz")) { | ||
| xlzPendingScan = true; | ||
| uploadSessionActive = true; | ||
| } else { | ||
| FSEQ_invalidateFileIndexCache(); | ||
| uploadSessionActive = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
uploadSessionActive is now doing two incompatible jobs.
After a successful .xlz upload, uploadSessionActive means “waiting for unzip”, but Line 939 still accepts a new upload because it only checks open handles. If that later upload finishes as a normal file, Line 731 clears uploadSessionActive while leaving xlzPendingScan set, so the earlier XLZ can no longer satisfy the Line 1128 processing condition and never gets unpacked.
🔧 Minimal containment fix
- if (uploadStream || currentUploadFile) {
+ if (uploadStream || currentUploadFile || xlzPendingScan) {
request->send(409, "text/plain", "Upload already in progress");
return;
}Also applies to: 936-974, 1128-1136
🤖 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/FSEQ/usermod_fpp.h` around lines 716 - 733, The bug is that
uploadSessionActive is being used for two different meanings (XLZ-pending vs
upload-in-progress) causing XLZ uploads to be left pending and never unpacked;
introduce a separate boolean (e.g., uploadInProgress or uploadAcceptingUploads)
to represent whether new uploads are allowed/ongoing and keep
uploadSessionActive reserved for "xlzPendingScan" or rename uploadSessionActive
to xlzPendingScan usage; update finishSuccessfulFppUpload (and the code paths
that set/clear uploadSessionActive like the non-.xlz branch) to set
xlzPendingScan = true for ".xlz" and only touch the new uploadInProgress flag
for upload lifecycle, and change the upload acceptance check (the code that
currently inspects open handles / uploadSessionActive) to consult the new
uploadInProgress flag so an XLZ pending state does not cancel/disable the
pending unpack.
This is a user module designed to meet all expectations. The FSEQ player is now available as an effect and supports multi-segment playback. For me, it’s the best solution that meets all needs. Hope this is okay now. |
|
@coderabbitai This PR is an evolution of #5395 (by the same user). In order to optimize the review
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 154773 🏁 Script executed: Length of output: 3118 🏁 Script executed: Length of output: 4399
Issues from PR
|
#5395 finding |
Status in #5641 |
|---|---|
web_ui_manager.bak backup file committed |
Removed — no .bak files present |
channel_count (uint32_t) silently truncated to uint16_t packetLength |
Fixed — const uint32_t packetLength = state.file_header.channel_count throughout |
VLA char frame_data[buffer_size] (non-standard C++) |
Gone — replaced by gFseqFrameBuffer shared static buffer |
Duplicate usermod IDs (both returning USERMOD_ID_SD_CARD) |
Fixed — USERMOD_ID_FSEQ = 59, USERMOD_ID_FPP = 60 |
Double-playback: both UsermodFseq::loop() and UsermodFPP::loop() calling frame render |
Fixed — UsermodFseq::loop() is intentionally empty; rendering is driven by the effect engine |
Off-by-one ++index > lastLed in pixel write loop |
Fixed — new code uses if (++index >= maxLeds) break |
clearLastPlayback() iterating 65 535 LEDs when playbackLedStop == uint16_t(-1) |
Obsolete — new per-segment clearPlaybackState(state) resets struct fields, no LED iteration |
README /api/sd/list and /api/fseq/list incorrectly described as HTML pages |
Fixed — new README explicitly documents both as JSON endpoints |
Loop button updating playBtn state instead of loopBtn |
Obsolete — completely new Web UI implementation |
Issues from PR #5395 → ⚠️ Still applicable in #5641
1. Dead parameter in skipRemainingFrameData (minor)
fseq_player.cpp lines 198–208:
bool FSEQPlayer::skipRemainingFrameData(PlaybackState &state, uint32_t bytesToSkip) {
if (bytesToSkip == 0) return true;
(void)bytesToSkip; // ← parameter is discarded immediately
const uint32_t newOffset = state.frame_data_offset + state.file_header.channel_count;
...The bytesToSkip parameter is accepted but never used — the seek always jumps one full channel_count forward from frame_data_offset. This is functionally correct in today's callers (they always want to skip to the next frame boundary), but the misleading signature is dead code worth cleaning up.
2. Playback start-time seeding before the first frame (from April 17 review of #5395)
scheduleNextFrame computes state.next_time = state.playback_start_time + ((state.frame + 1U) * step). This is a cleaner design than the old now-based approach, but it still requires playback_start_time to be seeded with a fresh millis() value inside loadRecordingIntoState before the first scheduleNextFrame call. This should be verified; if the elapsed-seconds anchor path (secondsElapsed > 0) seeds it correctly but the zero-start path does not call millis(), the first frame will have stale timing.
3. Architectural recommendations from @willmmiles (PR #5395 discussion)
The discussion between @willmmiles, @DedeHai, and @netmindz reached a consensus direction:
- FPP override should honour
useMainSegmentOnlyandrealtimeRespectLedMaps - A future "live-mode preset slot" hook would let users customise entry/exit behaviour (brightness, transitions, segment assignments)
- Today's implementation overrides segments directly without consulting those settings
This is not a blocker for this PR but is worth calling out as a known gap for a follow-up.
New issues visible only in #5641 (not present in #5395)
One new concern introduced by the rewrite is worth flagging:
gFseqFrameBuffer is a shared static buffer
processFrameDataForSegment reads file data into gFseqFrameBuffer and then copies it into the segment. If WLED ever calls the FSEQ effect for two different segments in the same render pass (which is allowed when multiple segments share the same effect mode), both calls will use the same buffer. Segment A's tail end could be overwritten by Segment B's first read before the pixel writes complete. This is a potential data-corruption issue for multi-segment FSEQ setups, which is one of the headline features of this PR.
Summary for the reviewer: the most actionable items still pending are the skipRemainingFrameData dead-parameter cleanup, the playback_start_time seeding verification, and the gFseqFrameBuffer shared-buffer safety for multi-segment use. Everything critical from the #5395 review has been resolved or superseded by the rewrite.
tpm2 still missing as I see. The former attempts of integrating tpm2 playback did not work with larger numbers of leds (which is the standard case for a matrix use case). |
3 participants
Introduce a new FSEQ usermod package: adds a local .fseq playback effect and an FPP-compatible "FPP Connect" usermod. Files added include the FSEQ player implementation (fseq_player.{h,cpp}), effect glue (fseq_effect.h), web UI manager, XLZ unzip support, SD adapter compatibility shim, registration, and library.json (depends on bitbank2/unzipLIB). Features: per-segment local playback, optional outgoing FPP sync, incoming FPP discovery/control (START/SYNC/PING/BLANK/STOP), FPP-style HTTP endpoints, deferred XLZ unpacking on uploads, and an SD management web UI. Also includes small updates to existing SD/WLED integration files to hook the new usermods into the build/runtime.
Summary by CodeRabbit
New Features
Documentation