Add FSEQ + FPP usermods for local playback#5641
Open
Danit2 wants to merge 3 commits into
Open
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.
Contributor
WalkthroughAdds an SD-backed FSEQ playback engine and effect, FPP-compatible UDP/HTTP usermod with upload handling and deferred XLZ unpacking, an embedded SD manager web UI, SD backend compatibility glue, and usermod registration plus realtime-mode JSON emission. 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}
Contributor
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
Contributor
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
Comment on lines
+4
to
+5
| static constexpr int16_t RECORDING_REPEAT_LOOP = -1; | ||
| static constexpr int16_t RECORDING_REPEAT_DEFAULT = 0; |
Contributor
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.
Comment on lines
+682
to
+714
| 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); | ||
| } |
Contributor
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.
Comment on lines
+716
to
+733
| 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; | ||
| } | ||
| } |
Contributor
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.
Member
softhack007
reviewed
May 23, 2026
Author
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. |
Member
|
@coderabbitai This PR is an evolution of #5395 (by the same user). In order to optimize the review
|
Contributor
|
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). |
Member
|
@coderabbitai full review |
This comment was marked as outdated.
This comment was marked as outdated.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
usermods/FSEQ/fseq_effect.h (1)
13-15: ⚡ Quick winRemove the unused index/loop caches.
_fseq_lastIndexand_fseq_lastLoopare still initialized and updated, but the current reload logic only reads_fseq_lastFileName. Keeping the extra arrays around makes the state machine harder to follow for no benefit. As per coding guidelines, "Unused or dead code must be justified or removed to keep the codebase clean, maintainable, and readable".Also applies to: 47-49, 90-91, 106-108
🤖 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 13 - 15, Remove the unused index/loop caches _fseq_lastIndex and _fseq_lastLoop: delete their declarations and remove all code that initializes, updates or references these arrays so the reload/state logic only uses _fseq_lastFileName; search for occurrences of _fseq_lastIndex and _fseq_lastLoop (declarations plus any assignments/reads) and eliminate them, and ensure any related comments or tests are updated to reflect that only _fseq_lastFileName is retained for reload tracking.usermods/FSEQ/fseq_player.cpp (1)
198-208: ⚡ Quick winDrop
bytesToSkipor actually use it.The current implementation ignores the parameter and always jumps to the end of the frame. That makes the signature misleading and easy to misuse later.
♻️ Minimal cleanup
-bool FSEQPlayer::skipRemainingFrameData(PlaybackState &state, uint32_t bytesToSkip) { - if (bytesToSkip == 0) return true; - (void)bytesToSkip; - +bool FSEQPlayer::skipRemainingFrameData(PlaybackState &state) { const uint32_t newOffset = state.frame_data_offset + state.file_header.channel_count; if (!state.recordingFile.seek(newOffset)) { DEBUG_PRINTLN("[FSEQ] Failed to skip remaining frame data");🤖 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 198 - 208, The function FSEQPlayer::skipRemainingFrameData currently ignores the bytesToSkip parameter and always skips state.file_header.channel_count bytes; change it to actually use bytesToSkip by computing newOffset = state.frame_data_offset + std::min<uint32_t>(bytesToSkip, state.file_header.channel_count) (remove the (void)bytesToSkip), then call state.recordingFile.seek(newOffset) and keep the existing error handling/clearPlaybackState call if seek fails; reference symbols: skipRemainingFrameData, PlaybackState, bytesToSkip, state.frame_data_offset, state.file_header.channel_count, state.recordingFile.seek, clearPlaybackState.usermods/FSEQ/usermod_fseq.h (1)
55-56: 💤 Low valueAdd brief comments to empty config methods.
The empty
addToConfig()andreadFromConfig()implementations should include a brief comment explaining why no configuration is needed (e.g., "// No user-configurable settings for this usermod").📝 Proposed addition
- void addToConfig(JsonObject &root) override {} - bool readFromConfig(JsonObject &root) override { return true; } + // No user-configurable settings for this usermod + void addToConfig(JsonObject &root) override {} + bool readFromConfig(JsonObject &root) override { return true; }🤖 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_fseq.h` around lines 55 - 56, Add a short explanatory comment inside the empty methods addToConfig(JsonObject &root) and readFromConfig(JsonObject &root) in usermod_fseq.h indicating that there are no user-configurable settings for this usermod (e.g., "// No user-configurable settings for this usermod") so future readers know the emptiness is intentional; update both function bodies (addToConfig and readFromConfig) with that brief comment and keep the return behavior unchanged.
🤖 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.cpp`:
- Around line 34-36: The global gFseqFrameBuffer (of size
FSEQ_SHARED_FRAME_BUFFER_SIZE) must be moved out of file scope and become a
per-playback buffer owned by PlaybackState (or provided via a buffer pointer
parameter) to prevent concurrent render overwrites; add a member like uint8_t[]
frameBuffer (or a pointer and allocator) to the PlaybackState struct/class sized
with FSEQ_SHARED_FRAME_BUFFER_SIZE, remove gFseqFrameBuffer, and update all
usages in fseq_player.cpp (including the regions around the other occurrences
you noted) to reference playbackState->frameBuffer (or the passed-in buffer)
instead of the global; ensure allocation/initialization happens when
PlaybackState is created and lifetime covers rendering, and remove any
assumptions about a single shared buffer in functions that previously referenced
gFseqFrameBuffer.
- Around line 440-444: The size check uses sizeof(state.file_header) which may
include ABI padding; instead compare state.file_size against the serialized FSEQ
header size (20 bytes) used by the readUInt* sequence. Replace the
sizeof(state.file_header) check with a constant or literal 20 and keep the
existing behavior of calling clearPlaybackState(state) and returning on failure
(references: state.file_size, state.file_header, FileHeader,
clearPlaybackState). Ensure any comment notes that the 20 corresponds to the sum
of the explicit readUInt* calls that parse the on-disk header.
In `@usermods/FSEQ/README.md`:
- Around line 104-112: Update the README's discovery behavior to reflect the
actual implementation: change the list to state that discovery is sent only to
the subnet broadcast (not global broadcast) and that pingInterval is 30 seconds
(not 10); reference the implementation variables/functions to verify values
(pingInterval, sendDiscovery/discovery burst logic) and ensure the documented
burst count and burst interval match the code if they differ from what's
currently written.
In `@usermods/FSEQ/register_usermod.cpp`:
- Around line 1-3: Place the primary project header first by moving the `#include`
"wled.h" to the top of register_usermod.cpp, then include the module headers
(`#include` "usermod_fpp.h" and `#include` "usermod_fseq.h") afterward; follow the
WLED convention of primary project header first, then platform/Arduino headers,
then third‑party headers so symbols in usermod_fpp/usermod_fseq are compiled
with the correct project definitions.
In `@usermods/FSEQ/web_ui_manager.cpp`:
- Around line 235-242: xhr.onload currently treats any completed HTTP response
as success; update the xhr.onload handler to check xhr.status (accept 200–299)
before setting statusText to "Upload complete" and calling loadSDList(); for
non-2xx responses set statusText to an error message (include xhr.status/text),
do not call loadSDList(), and ensure progressBar is reset in both success and
failure paths; also wire up xhr.onerror/xhr.onabort to set an error statusText
and reset the progressBar to cover network failures.
- Around line 336-338: The filename check currently only matches ".fseq" and
".FSEQ" and misses mixed-case variants; modify the condition that inspects the
variable name (from file.name() -> name) to perform a case-insensitive suffix
check (e.g. compare name.toLowerCase().endsWith(".fseq") or use a
case-insensitive regex) so any casing like ".Fseq" is treated as an FSEQ and not
sent to otherFiles; update the branch around the name variable where the
startsWith/endsWith logic lives to use this case-insensitive test.
In `@usermods/FSEQ/xlz_unzip.cpp`:
- Around line 11-12: g_xlzZip is a shared global UNZIP instance used by
unpackAndDelete() and processAllPendingXLZ() and is not synchronized, causing
race conditions when two XLZ operations overlap; fix by either making UNZIP
local to each operation (allocate a UNZIP per call in
unpackAndDelete/processAllPendingXLZ) or serialize access with a FreeRTOS
mutex/semaphore (create a static mutex, take it at the start of
unpackAndDelete/processAllPendingXLZ and give it back on all return paths,
including error/cleanup), ensuring all uses of g_xlzZip (and any cleanup paths)
are protected so archive state cannot be clobbered by concurrent tasks.
- Around line 211-259: The code currently uses gotoFirstFile()/gotoNextFile()
and immediately extracts the first entry as the .fseq payload; change this to
iterate over archive entries (using gotoFirstFile(), getFileInfo(...,
entryName,...), then gotoNextFile()) until you find a real regular file whose
sanitized name ends with ".fseq" (or whose uncompressed_size > 0 and is not a
directory entry) before calling unpackCurrentFile and setting finalOutputPath;
skip non-file entries (directories, zero-size metadata) and continue the loop,
calling g_xlzZip.closeZIP() and returning false only if the loop finishes
without finding any valid .fseq entry; keep using sanitizeEntryName,
hasXLZExtension, unpackCurrentFile and ensure finalOutputPath is computed from
the matched entry name.
In `@wled00/json.cpp`:
- Line 767: The line handling REALTIME_MODE_FSEQ uses a tab for indentation;
replace the leading tab before "case REALTIME_MODE_FSEQ: root[\"lm\"] =
F(\"FSEQ\"); break;" with two spaces to conform to the project's 2-space
indentation rule (no tabs), ensuring the "case REALTIME_MODE_FSEQ" line aligns
with other case labels in json.cpp.
---
Nitpick comments:
In `@usermods/FSEQ/fseq_effect.h`:
- Around line 13-15: Remove the unused index/loop caches _fseq_lastIndex and
_fseq_lastLoop: delete their declarations and remove all code that initializes,
updates or references these arrays so the reload/state logic only uses
_fseq_lastFileName; search for occurrences of _fseq_lastIndex and _fseq_lastLoop
(declarations plus any assignments/reads) and eliminate them, and ensure any
related comments or tests are updated to reflect that only _fseq_lastFileName is
retained for reload tracking.
In `@usermods/FSEQ/fseq_player.cpp`:
- Around line 198-208: The function FSEQPlayer::skipRemainingFrameData currently
ignores the bytesToSkip parameter and always skips
state.file_header.channel_count bytes; change it to actually use bytesToSkip by
computing newOffset = state.frame_data_offset + std::min<uint32_t>(bytesToSkip,
state.file_header.channel_count) (remove the (void)bytesToSkip), then call
state.recordingFile.seek(newOffset) and keep the existing error
handling/clearPlaybackState call if seek fails; reference symbols:
skipRemainingFrameData, PlaybackState, bytesToSkip, state.frame_data_offset,
state.file_header.channel_count, state.recordingFile.seek, clearPlaybackState.
In `@usermods/FSEQ/usermod_fseq.h`:
- Around line 55-56: Add a short explanatory comment inside the empty methods
addToConfig(JsonObject &root) and readFromConfig(JsonObject &root) in
usermod_fseq.h indicating that there are no user-configurable settings for this
usermod (e.g., "// No user-configurable settings for this usermod") so future
readers know the emptiness is intentional; update both function bodies
(addToConfig and readFromConfig) with that brief comment and keep the return
behavior unchanged.
🪄 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: c73faaa6-ea83-47b3-bae6-8763e799b71d
📒 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
Comment on lines
+34
to
+36
| // Shared read buffer to avoid large stack allocations. | ||
| // Rendering is done sequentially in the main loop, so one shared buffer is fine. | ||
| static uint8_t gFseqFrameBuffer[FSEQ_SHARED_FRAME_BUFFER_SIZE]; |
Contributor
There was a problem hiding this comment.
Make the frame scratch buffer per playback state.
This global buffer couples all FSEQ renders together. If two segments hit the player in the same render cycle, one read can overwrite the other segment's in-flight frame data. A buffer owned by PlaybackState (or passed in by the caller) avoids cross-segment corruption.
Also applies to: 262-300, 311-351
🤖 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 34 - 36, The global
gFseqFrameBuffer (of size FSEQ_SHARED_FRAME_BUFFER_SIZE) must be moved out of
file scope and become a per-playback buffer owned by PlaybackState (or provided
via a buffer pointer parameter) to prevent concurrent render overwrites; add a
member like uint8_t[] frameBuffer (or a pointer and allocator) to the
PlaybackState struct/class sized with FSEQ_SHARED_FRAME_BUFFER_SIZE, remove
gFseqFrameBuffer, and update all usages in fseq_player.cpp (including the
regions around the other occurrences you noted) to reference
playbackState->frameBuffer (or the passed-in buffer) instead of the global;
ensure allocation/initialization happens when PlaybackState is created and
lifetime covers rendering, and remove any assumptions about a single shared
buffer in functions that previously referenced gFseqFrameBuffer.
Comment on lines
+440
to
+444
| if (state.file_size < sizeof(state.file_header)) { | ||
| DEBUG_PRINTF("Invalid file size: %lu\n", (unsigned long)state.file_size); | ||
| clearPlaybackState(state); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact check and surrounding code
sed -n '400,520p' usermods/FSEQ/fseq_player.cpp | nl -ba | sed -n '1,200p'
# Find FileHeader definition and packing attributes
rg -n "struct FileHeader|FileHeader|file_header" usermods/FSEQ/fseq_player.* usermods/FSEQ/*.h
# Show header struct definition
rg -n "struct FileHeader" usermods/FSEQ -S
FILEHEADER_FILE="$(rg -l "struct FileHeader" usermods/FSEQ | head -n 1)"
echo "FileHeader defined in: $FILEHEADER_FILE"
sed -n '1,220p' "$FILEHEADER_FILE" | nl -ba | sed -n '1,220p'
# Confirm any other usage of sizeof(state.file_header) / file_header size checks
rg -n "sizeof\\(state\\.file_header\\)|sizeof\\(.*file_header\\)" usermods/FSEQ/fseq_player.cpp usermods/FSEQ/fseq_player.* -SRepository: wled/WLED
Length of output: 96
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Surrounding code near the reported check
sed -n '420,500p' usermods/FSEQ/fseq_player.cpp | cat -n
# Locate FileHeader definition and show it (with line numbers)
rg -n --hidden --no-ignore -S "struct FileHeader|class FileHeader|typedef .*FileHeader|FileHeader" usermods/FSEQ | head -n 50
FILEHEADER_FILE="$(rg -l --hidden --no-ignore "struct FileHeader" usermods/FSEQ | head -n 1)"
echo "FileHeader defined in: $FILEHEADER_FILE"
sed -n '1,260p' "$FILEHEADER_FILE" | cat -n
# Check for any explicit packing or size constraints
rg -n "packed|__attribute__\\(\\(packed\\)\\)|static_assert.*FileHeader|sizeof\\(FileHeader\\)" "$FILEHEADER_FILE" usermods/FSEQ -S || true
# Confirm how header is parsed/filled (struct read vs field-by-field)
rg -n -S "file_header|state\\.file_header|read\\(|memcpy|seek|readBytes|fread|sizeof\\(state\\.file_header\\)|sizeof\\(.*file_header\\)" usermods/FSEQ/fseq_player.cpp usermods/FSEQ/fseq_player.* || trueRepository: wled/WLED
Length of output: 22734
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect byte-width of helper reads
rg -n "readUInt(8|16|24|32)\\s*\\(" usermods/FSEQ/fseq_player.cpp
echo "----"
# Show implementations around the first occurrences
sed -n '1,260p' usermods/FSEQ/fseq_player.cpp | cat -nRepository: wled/WLED
Length of output: 10854
Compare against the serialized FSEQ header byte count (20), not sizeof(FileHeader).
state.file_size is checked against sizeof(state.file_header), but the header is consumed from the file via explicit readUInt* calls totaling 20 bytes. FileHeader is an unpacked struct (no packing/size constraint), so sizeof(state.file_header) can include ABI padding and drift from the on-disk layout.
🔧 Safer check
- if (state.file_size < sizeof(state.file_header)) {
+ constexpr uint32_t FSEQ_HEADER_BYTES = 20;
+ if (state.file_size < FSEQ_HEADER_BYTES) {
DEBUG_PRINTF("Invalid file size: %lu\n", (unsigned long)state.file_size);
clearPlaybackState(state);
return;
}🤖 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 440 - 444, The size check uses
sizeof(state.file_header) which may include ABI padding; instead compare
state.file_size against the serialized FSEQ header size (20 bytes) used by the
readUInt* sequence. Replace the sizeof(state.file_header) check with a constant
or literal 20 and keep the existing behavior of calling
clearPlaybackState(state) and returning on failure (references: state.file_size,
state.file_header, FileHeader, clearPlaybackState). Ensure any comment notes
that the 20 corresponds to the sum of the explicit readUInt* calls that parse
the on-disk header.
Comment on lines
+104
to
+112
| After Wi-Fi connects, the usermod sends a short discovery burst and then continues periodic announcements. | ||
|
|
||
| Current behavior: | ||
|
|
||
| - sends discovery to **subnet broadcast** and **global broadcast** | ||
| - sends an initial burst of **5** announcements | ||
| - burst interval: **1 second** | ||
| - regular ping/discovery interval: **10 seconds** | ||
|
|
Contributor
There was a problem hiding this comment.
Update the discovery behavior docs to match the code.
The implementation currently sends discovery only to subnet broadcast, and pingInterval is 30 seconds, not 10. This section will mislead anyone trying to validate network behavior against the README.
🤖 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` around lines 104 - 112, Update the README's
discovery behavior to reflect the actual implementation: change the list to
state that discovery is sent only to the subnet broadcast (not global broadcast)
and that pingInterval is 30 seconds (not 10); reference the implementation
variables/functions to verify values (pingInterval, sendDiscovery/discovery
burst logic) and ensure the documented burst count and burst interval match the
code if they differ from what's currently written.
Comment on lines
+1
to
+3
| #include "usermod_fpp.h" | ||
| #include "usermod_fseq.h" | ||
| #include "wled.h" |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Reorder includes to place wled.h first.
The WLED C++ coding conventions require wled.h as the primary project header, included before other headers.
♻️ Proposed fix
+#include "wled.h"
`#include` "usermod_fpp.h"
`#include` "usermod_fseq.h"
-#include "wled.h"As per coding guidelines: Include "wled.h" as the primary project header first, then platform/Arduino headers, then third-party headers.
📝 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.
Suggested change
| #include "usermod_fpp.h" | |
| #include "usermod_fseq.h" | |
| #include "wled.h" | |
| `#include` "wled.h" | |
| `#include` "usermod_fpp.h" | |
| `#include` "usermod_fseq.h" |
🤖 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/register_usermod.cpp` around lines 1 - 3, Place the primary
project header first by moving the `#include` "wled.h" to the top of
register_usermod.cpp, then include the module headers (`#include` "usermod_fpp.h"
and `#include` "usermod_fseq.h") afterward; follow the WLED convention of primary
project header first, then platform/Arduino headers, then third‑party headers so
symbols in usermod_fpp/usermod_fseq are compiled with the correct project
definitions.
Comment on lines
+235
to
+242
| xhr.onload=function(){ | ||
| statusText.innerText='Upload complete'; | ||
| loadSDList(); | ||
| setTimeout(()=>{ | ||
| progressBar.style.width='0%'; | ||
| statusText.innerText=''; | ||
| },2000); | ||
| }; |
Contributor
There was a problem hiding this comment.
Don't show failed uploads as successful.
xhr.onload runs for any completed HTTP response, including 400/500, but the UI always prints Upload complete and refreshes the list. A rejected or failed upload will therefore look successful in the browser.
Suggested fix
xhr.onload=function(){
- statusText.innerText='Upload complete';
- loadSDList();
- setTimeout(()=>{
- progressBar.style.width='0%';
- statusText.innerText='';
- },2000);
+ if (xhr.status >= 200 && xhr.status < 300) {
+ statusText.innerText='Upload complete';
+ loadSDList();
+ setTimeout(()=>{
+ progressBar.style.width='0%';
+ statusText.innerText='';
+ },2000);
+ } else {
+ statusText.innerText=xhr.responseText || ('Upload failed (' + xhr.status + ')');
+ }
};
+
+ 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 completed HTTP response as success; update the xhr.onload
handler to check xhr.status (accept 200–299) before setting statusText to
"Upload complete" and calling loadSDList(); for non-2xx responses set statusText
to an error message (include xhr.status/text), do not call loadSDList(), and
ensure progressBar is reset in both success and failure paths; also wire up
xhr.onerror/xhr.onabort to set an error statusText and reset the progressBar to
cover network failures.
Comment on lines
+336
to
+338
| String name = file.name(); | ||
| if (!name.startsWith("/")) name = "/" + name; | ||
| if (!(name.endsWith(".fseq") || name.endsWith(".FSEQ"))) { |
Contributor
There was a problem hiding this comment.
Filter .fseq entries case-insensitively here too.
This branch only excludes .fseq and .FSEQ. Mixed-case variants like /show.Fseq will fall into otherFiles even though the indexed FSEQ cache is case-insensitive, so the same file can show up in the wrong section.
Suggested fix
String name = file.name();
if (!name.startsWith("/")) name = "/" + name;
- if (!(name.endsWith(".fseq") || name.endsWith(".FSEQ"))) {
+ const int extPos = name.length() - 5;
+ const bool isFseq = extPos >= 0 && name.substring(extPos).equalsIgnoreCase(".fseq");
+ if (!isFseq) {
JsonObject obj = otherFiles.createNestedObject();
obj["name"] = name;
obj["size"] = (float)file.size() / 1024.0f;🤖 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 - 338, The filename check
currently only matches ".fseq" and ".FSEQ" and misses mixed-case variants;
modify the condition that inspects the variable name (from file.name() -> name)
to perform a case-insensitive suffix check (e.g. compare
name.toLowerCase().endsWith(".fseq") or use a case-insensitive regex) so any
casing like ".Fseq" is treated as an FSEQ and not sent to otherFiles; update the
branch around the name variable where the startsWith/endsWith logic lives to use
this case-insensitive test.
Comment on lines
+211
to
+259
| if (g_xlzZip.gotoFirstFile() != UNZ_OK) { | ||
| DEBUG_PRINTLN(F("[XLZ] Archive contains no files")); | ||
| g_xlzZip.closeZIP(); | ||
| return false; | ||
| } | ||
|
|
||
| const int infoRc = g_xlzZip.getFileInfo(&fileInfo, entryName, sizeof(entryName), | ||
| nullptr, 0, comment, sizeof(comment)); | ||
| if (infoRc != UNZ_OK) { | ||
| DEBUG_PRINTF("[XLZ] getFileInfo() failed: %d\n", infoRc); | ||
| g_xlzZip.closeZIP(); | ||
| return false; | ||
| } | ||
|
|
||
| String safeName = sanitizeEntryName(entryName); | ||
| if (safeName.isEmpty()) { | ||
| DEBUG_PRINTLN(F("[XLZ] Invalid filename inside archive")); | ||
| g_xlzZip.closeZIP(); | ||
| return false; | ||
| } | ||
|
|
||
| finalOutputPath = zipPath; | ||
| if (hasXLZExtension(finalOutputPath)) { | ||
| finalOutputPath.remove(finalOutputPath.length() - 4); | ||
| finalOutputPath += ".fseq"; | ||
| } else { | ||
| finalOutputPath = normalizePath(safeName); | ||
| if (!endsWithIgnoreCase(finalOutputPath, ".fseq")) { | ||
| finalOutputPath += ".fseq"; | ||
| } | ||
| } | ||
|
|
||
| const uint64_t totalBytes = SD_ADAPTER.totalBytes(); | ||
| const uint64_t usedBytes = SD_ADAPTER.usedBytes(); | ||
| const uint64_t freeBytes = (totalBytes >= usedBytes) ? (totalBytes - usedBytes) : 0; | ||
| if (fileInfo.uncompressed_size > freeBytes) { | ||
| DEBUG_PRINTF("[XLZ] Not enough free space. need=%lu free=%lu\n", | ||
| static_cast<unsigned long>(fileInfo.uncompressed_size), | ||
| static_cast<unsigned long>(freeBytes)); | ||
| g_xlzZip.closeZIP(); | ||
| return false; | ||
| } | ||
|
|
||
| DEBUG_PRINTF("[XLZ] Extracting %s -> %s\n", zipPath.c_str(), finalOutputPath.c_str()); | ||
| ok = unpackCurrentFile(g_xlzZip, finalOutputPath, static_cast<uint32_t>(fileInfo.uncompressed_size)); | ||
|
|
||
| const int nextRc = g_xlzZip.gotoNextFile(); | ||
| if (ok && nextRc == UNZ_OK) { | ||
| DEBUG_PRINTLN(F("[XLZ] Warning: archive contains more than one file; only the first file was extracted")); |
Contributor
There was a problem hiding this comment.
Don't assume the first ZIP entry is the FSEQ payload.
Line 211 always extracts whatever gotoFirstFile() returns, and Line 255 writes it out as .fseq without checking that the entry is a regular file or that its name ends in .fseq. Archives with a leading directory entry, metadata file, or wrapper folder will currently produce an empty/wrong output file and then delete the source archive on success. Iterate until you find a real .fseq entry, or fail the archive explicitly if none exists.
🤖 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 211 - 259, The code currently uses
gotoFirstFile()/gotoNextFile() and immediately extracts the first entry as the
.fseq payload; change this to iterate over archive entries (using
gotoFirstFile(), getFileInfo(..., entryName,...), then gotoNextFile()) until you
find a real regular file whose sanitized name ends with ".fseq" (or whose
uncompressed_size > 0 and is not a directory entry) before calling
unpackCurrentFile and setting finalOutputPath; skip non-file entries
(directories, zero-size metadata) and continue the loop, calling
g_xlzZip.closeZIP() and returning false only if the loop finishes without
finding any valid .fseq entry; keep using sanitizeEntryName, hasXLZExtension,
unpackCurrentFile and ensure finalOutputPath is computed from the matched entry
name.
| case REALTIME_MODE_TPM2NET: root["lm"] = F("tpm2.net"); break; | ||
| case REALTIME_MODE_DDP: root["lm"] = F("DDP"); break; | ||
| case REALTIME_MODE_DMX: root["lm"] = F("DMX"); break; | ||
| case REALTIME_MODE_FSEQ: root["lm"] = F("FSEQ"); break; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Replace tab with spaces for indentation.
Line 767 uses a tab character for indentation, which violates the WLED C++ coding convention requiring 2-space indentation with no tabs.
♻️ Proposed fix
- case REALTIME_MODE_FSEQ: root["lm"] = F("FSEQ"); break;
+ case REALTIME_MODE_FSEQ: root["lm"] = F("FSEQ"); break;As per coding guidelines: Use 2-space indentation (no tabs) in C++ files.
📝 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.
Suggested change
| case REALTIME_MODE_FSEQ: root["lm"] = F("FSEQ"); break; | |
| case REALTIME_MODE_FSEQ: root["lm"] = F("FSEQ"); break; |
🤖 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/json.cpp` at line 767, The line handling REALTIME_MODE_FSEQ uses a tab
for indentation; replace the leading tab before "case REALTIME_MODE_FSEQ:
root[\"lm\"] = F(\"FSEQ\"); break;" with two spaces to conform to the project's
2-space indentation rule (no tabs), ensuring the "case REALTIME_MODE_FSEQ" line
aligns with other case labels in json.cpp.
softhack007
reviewed
May 29, 2026
| bool ok = true; | ||
| uint32_t written = 0; | ||
|
|
||
| while (true) { |
Member
There was a problem hiding this comment.
coding style: could you change this loop into do { .... } while !done; style?
Currently the logic is a bit "backwards" - an infinite loop, but there are some exits via break. I think the logic would be easier to follow if you invest a byte for a "done" flag, and make the repetition condition explicit.
(you can keep the error case early-exists via break if you want)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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