added support for fseq files #4570
Conversation
WalkthroughThis pull request introduces new usermod functionality for processing FSEQ files along with supporting features. It adds documentation, new classes to handle FSEQ and FPP operations, SD card management, and a web-based UI interface. In addition, the registration mechanism for usermods is updated to incorporate the new features while disabling an existing SD card usermod. The changes include comprehensive modifications to both header and source files to support file reading, playback control, UDP network communication, and configuration management. Changes
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Now we have playback of gif files in the main code. Is there any chance this could be extended to support this file type as well or is there too little in common? |
|
@netmindz there is an old PR from @fastbytes #2413 and @constant-flow #2292 for FSEQ/TPM2 file playback we never got around to properly migrate/update for WLED. I am really sorry those old PRs were left hanging but one man just couldn't do everything. I hope this fares better or old PRs are revived. |
|
xLights can now discover WLED using the network discovery feature. 
|
|
No worries if there isn't anything in common. I had a vague memory I'd seen a comment about GIF just being the first to be done then extend, but maybe that was just aspirational rather than any software architecture/design |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
usermods/FSEQ/usermod_fpp.h (3)
17-64: Consider adding error handling inwrite()forWriteBufferingStream.
Currently, the functionwrite()accumulates bytes in_buffer, but it does not check the return value of_upstream.write(). If_upstream.write()fails or returns fewer bytes than expected, data loss may occur without any notification.
67-68: Buffer size may be too large for memory-constrained boards.
FILE_UPLOAD_BUFFER_SIZEis set to563 * 7, which is roughly 4kB of memory. Consider making this configurable or ensuring adequate free heap space to reduce the risk of memory exhaustion on lower-end boards.
82-139: Factor out repeated JSON-building logic.
Functions likebuildSystemInfoJSON(),buildSystemStatusJSON(), andbuildFppdMultiSyncSystemsJSON()appear to have overlapping fields. Consider extracting common fields into a shared helper function to avoid duplicated code.usermods/FSEQ/usermod_fseq.h (1)
281-343: Reduce duplication in SD file management.
UsermodFseqand other modules (e.g.,UsermodFPP) have similar logic for listing SD files and handling uploads. Consider sharing a helper utility or consolidating code to enhance maintainability.wled00/usermods_list.cpp (1)
438-440: Consider removing or documenting the commented-outUsermodSdCard.
If it’s no longer relevant, removing it can reduce confusion. Otherwise, add a comment explaining why it remains disabled.usermods/FSEQ/README.md (2)
65-75: Consider adding memory configuration flags.The PlatformIO configuration looks good but could benefit from additional memory-related flags for ESP32, especially when dealing with file operations.
Consider adding these flags:
[env:esp32dev_V4] build_flags = -D WLED_DEBUG -D WLED_USE_SD_SPI -D WLED_PIN_SCK=18 ; CLK -D WLED_PIN_MISO=19 ; Data Out (POCI) -D WLED_PIN_MOSI=23 ; Data In (PICO) -D WLED_PIN_SS=5 ; Chip Select (CS) -D USERMOD_FSEQ -std=gnu++17 + board_build.partitions = huge_app.csv ; More space for app + board_build.filesystem = littlefs ; Better for frequent small writes
94-99: Fix formatting issues in the Usage section.The Usage section contains hard tabs which should be replaced with spaces for consistent formatting across different editors.
Apply this formatting fix:
-###Usage - • Web Interface: -Access the SD & FSEQ Manager by clicking the Fseq button in the main UI. The interface allows you to view, upload, and delete SD card files as well as control FSEQ playback. - • File Management: -The SD file manager displays all files on the SD card. You can upload new files via the provided form and delete files using the red "Delete" button. - • FSEQ Playback: +### Usage + +* **Web Interface:** + Access the SD & FSEQ Manager by clicking the Fseq button in the main UI. The interface allows you to view, upload, and delete SD card files as well as control FSEQ playback. + +* **File Management:** + The SD file manager displays all files on the SD card. You can upload new files via the provided form and delete files using the red "Delete" button. + +* **FSEQ Playback:**🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
94-94: Hard tabs
Column: 1(MD010, no-hard-tabs)
94-94: Hard tabs
Column: 3(MD010, no-hard-tabs)
96-96: Hard tabs
Column: 1(MD010, no-hard-tabs)
96-96: Hard tabs
Column: 3(MD010, no-hard-tabs)
98-98: Hard tabs
Column: 1(MD010, no-hard-tabs)
98-98: Hard tabs
Column: 3(MD010, no-hard-tabs)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
usermods/FSEQ/README.md(1 hunks)usermods/FSEQ/usermod_fpp.h(1 hunks)usermods/FSEQ/usermod_fseq.h(1 hunks)wled00/data/index.htm(1 hunks)wled00/usermods_list.cpp(4 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
usermods/FSEQ/README.md
94-94: Hard tabs
Column: 1
(MD010, no-hard-tabs)
94-94: Hard tabs
Column: 3
(MD010, no-hard-tabs)
96-96: Hard tabs
Column: 1
(MD010, no-hard-tabs)
96-96: Hard tabs
Column: 3
(MD010, no-hard-tabs)
98-98: Hard tabs
Column: 1
(MD010, no-hard-tabs)
98-98: Hard tabs
Column: 3
(MD010, no-hard-tabs)
🔇 Additional comments (4)
usermods/FSEQ/usermod_fseq.h (1)
64-190: Validate maximum channel count for LED range.
When reading frames,channel_countcan exceed the strip capacity if the file is malformed. Ensure channel data does not write outside the valid LED range. This helps prevent buffer overruns and unexpected behavior.wled00/usermods_list.cpp (2)
14-20: Header inclusions appear consistent.
The conditional includes forUSERMOD_FSEQandUSERMOD_FPPare appropriately isolated, improving modularity.
274-281: Conditional usermod registrations are correct.
UsermodFseq()andUsermodFPP()are selectively added toUsermodManagervia preprocessor checks, keeping the codebase clean and extensible.wled00/data/index.htm (1)
28-29: LGTM! Button implementation follows UI patterns.The new FSEQ button is well-integrated into the existing navigation, using consistent styling and the established
getURL()pattern.
usermods/FSEQ/usermod_fpp.h
Outdated
| server.on("/fpp", HTTP_POST, | ||
| // onRequest callback – po dokončení uploadu | ||
| [this](AsyncWebServerRequest *request) { | ||
| if (uploadStream != nullptr) { | ||
| uploadStream->flush(); | ||
| delete uploadStream; | ||
| uploadStream = nullptr; | ||
| } | ||
| if (currentUploadFile) { | ||
| currentUploadFile.close(); | ||
| } | ||
| currentUploadFileName = ""; | ||
| request->send(200, "text/plain", "Upload complete"); | ||
| }, | ||
| // onUpload callback – nie je použitý (NULL) | ||
| NULL, | ||
| // onBody callback – spracováva raw upload dáta | ||
| [this](AsyncWebServerRequest *request, uint8_t *data, size_t len, size_t index, size_t total) { | ||
| if (index == 0) { | ||
| // Načítanie parametrov uploadu | ||
| DEBUG_PRINTLN("[FPP] Received parameters:"); | ||
| for (uint8_t i = 0; i < request->params(); i++) { | ||
| AsyncWebParameter* p = request->getParam(i); | ||
| DEBUG_PRINTF("[FPP] Param %s = %s\n", p->name().c_str(), p->value().c_str()); | ||
| } | ||
| String fileParam = ""; | ||
| if (request->hasParam("filename")) { | ||
| fileParam = request->arg("filename"); | ||
| } | ||
| DEBUG_PRINTF("[FPP] fileParam = %s\n", fileParam.c_str()); | ||
| if (fileParam != "") { | ||
| currentUploadFileName = fileParam.startsWith("/") ? fileParam : "/" + fileParam; | ||
| } else { | ||
| currentUploadFileName = "/default.fseq"; | ||
| } | ||
| DEBUG_PRINTF("[FPP] Using filename: %s\n", currentUploadFileName.c_str()); | ||
| if (SD.exists(currentUploadFileName.c_str())) { | ||
| SD.remove(currentUploadFileName.c_str()); | ||
| } | ||
| currentUploadFile = SD.open(currentUploadFileName.c_str(), FILE_WRITE); | ||
| if (!currentUploadFile) { | ||
| DEBUG_PRINTLN(F("[FPP] ERROR: Failed to open file for writing")); | ||
| return; | ||
| } | ||
| uploadStream = new WriteBufferingStream(currentUploadFile, FILE_UPLOAD_BUFFER_SIZE); | ||
| uploadStartTime = millis(); | ||
| } | ||
| if (uploadStream != nullptr) { | ||
| uploadStream->write(data, len); | ||
| } | ||
| if (index + len >= total) { | ||
| if (uploadStream != nullptr) { | ||
| uploadStream->flush(); | ||
| delete uploadStream; | ||
| uploadStream = nullptr; | ||
| } | ||
| currentUploadFile.close(); | ||
| currentUploadFileName = ""; | ||
| } | ||
| } | ||
| ); | ||
|
|
||
| server.on("/fpp/connect", HTTP_GET, [this](AsyncWebServerRequest *request) { | ||
| if (!request->hasArg("file")) { | ||
| request->send(400, "text/plain", "Missing 'file' parameter"); | ||
| return; | ||
| } | ||
| String filepath = request->arg("file"); | ||
| if (!filepath.startsWith("/")) { | ||
| filepath = "/" + filepath; | ||
| } | ||
| FSEQFile::loadRecording(filepath.c_str(), 0, strip.getLength()); | ||
| request->send(200, "text/plain", "FPP connect started: " + filepath); | ||
| }); | ||
| server.on("/fpp/stop", HTTP_GET, [this](AsyncWebServerRequest *request) { | ||
| FSEQFile::clearLastPlayback(); | ||
| realtimeLock(10, REALTIME_MODE_INACTIVE); | ||
| request->send(200, "text/plain", "FPP connect stopped"); | ||
| }); | ||
| } | ||
|
|
||
| void loop() { | ||
| if (!udpStarted && (WiFi.status() == WL_CONNECTED)) { | ||
| if (udp.listenMulticast(multicastAddr, udpPort)) { | ||
| udpStarted = true; | ||
| udp.onPacket([this](AsyncUDPPacket packet) { | ||
| sendPingPacket(packet.remoteIP()); | ||
| }); | ||
| } | ||
| } | ||
| FSEQFile::handlePlayRecording(); | ||
| } | ||
|
|
||
| uint16_t getId() { | ||
| return USERMOD_ID_SD_CARD; | ||
| } | ||
| void addToConfig(JsonObject &root) { } | ||
| bool readFromConfig(JsonObject &root) { return true; } | ||
| }; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate file type and size for raw uploads.
The /fpp POST endpoint accepts raw data without any checks on file extension or maximum file size. This may allow malicious or unintended uploads. Consider adding file-type checks or size limits before writing to disk.
usermods/FSEQ/usermod_fseq.h
Outdated
| void FSEQFile::loadRecording(const char* filepath, uint16_t startLed, uint16_t stopLed) { | ||
| if (recordingFile.available()) { | ||
| clearLastPlayback(); | ||
| recordingFile.close(); | ||
| } | ||
| playbackLedStart = startLed; | ||
| playbackLedStop = stopLed; | ||
| if (playbackLedStart == uint16_t(-1) || playbackLedStop == uint16_t(-1)) { | ||
| Segment sg = strip.getSegment(-1); | ||
| playbackLedStart = sg.start; | ||
| playbackLedStop = sg.stop; | ||
| } | ||
| DEBUG_PRINTF("FSEQ load animation on LED %d to %d\n", playbackLedStart, playbackLedStop); | ||
| if (fileOnSD(filepath)) { | ||
| DEBUG_PRINTF("Read file from SD: %s\n", filepath); | ||
| recordingFile = SD_ADAPTER.open(filepath, "rb"); | ||
| } else if (fileOnFS(filepath)) { | ||
| DEBUG_PRINTF("Read file from FS: %s\n", filepath); | ||
| } else { | ||
| DEBUG_PRINTF("File %s not found (%s)\n", filepath, USED_STORAGE_FILESYSTEMS); | ||
| return; | ||
| } | ||
| if ((uint64_t)recordingFile.available() < sizeof(file_header)) { | ||
| DEBUG_PRINTF("Invalid file size: %d\n", recordingFile.available()); | ||
| recordingFile.close(); | ||
| return; | ||
| } | ||
| for (int i = 0; i < 4; i++) { | ||
| file_header.identifier[i] = readUInt8(); | ||
| } | ||
| file_header.channel_data_offset = readUInt16(); | ||
| file_header.minor_version = readUInt8(); | ||
| file_header.major_version = readUInt8(); | ||
| file_header.header_length = readUInt16(); | ||
| file_header.channel_count = readUInt32(); | ||
| file_header.frame_count = readUInt32(); | ||
| file_header.step_time = readUInt8(); | ||
| file_header.flags = readUInt8(); | ||
| printHeaderInfo(); | ||
| if (file_header.identifier[0] != 'P' || file_header.identifier[1] != 'S' || | ||
| file_header.identifier[2] != 'E' || file_header.identifier[3] != 'Q') { | ||
| DEBUG_PRINTF("Error reading FSEQ file %s header, invalid identifier\n", filepath); | ||
| recordingFile.close(); | ||
| return; | ||
| } | ||
| if (((uint64_t)file_header.channel_count * (uint64_t)file_header.frame_count) + file_header.header_length > UINT32_MAX) { | ||
| DEBUG_PRINTF("Error reading FSEQ file %s header, file too long (max 4gb)\n", filepath); | ||
| recordingFile.close(); | ||
| return; | ||
| } | ||
| if (file_header.step_time < 1) { | ||
| DEBUG_PRINTF("Invalid step time %d, using default %d instead\n", file_header.step_time, FSEQ_DEFAULT_STEP_TIME); | ||
| file_header.step_time = FSEQ_DEFAULT_STEP_TIME; | ||
| } | ||
| if (realtimeOverride == REALTIME_OVERRIDE_ONCE) { | ||
| realtimeOverride = REALTIME_OVERRIDE_NONE; | ||
| } | ||
| recordingRepeats = RECORDING_REPEAT_DEFAULT; | ||
| playNextRecordingFrame(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add stricter checks for file size vs. frame_count.
Although basic header checks exist, there is no explicit handling if frame_count is unexpectedly large or the file is truncated. Consider verifying that enough data is present before attempting to read frames.
usermods/FSEQ/README.md
Outdated
| This usermod adds support for playing FSEQ files from an SD card and provides a web interface for managing SD files and FSEQ playback. Follow the instructions below to install and use this usermod. | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding hardware requirements section.
The introduction should specify the required hardware setup, particularly the SD card module specifications and wiring details.
Add a new section after the introduction:
## Hardware Requirements
* ESP32 board (tested with...)
* SD card module (SPI)
* SD card (FAT32 formatted)
### Wiring
| SD Card Module | ESP32 |
|---------------|-----------|
| CS | GPIO5 |
| SCK | GPIO18 |
| MOSI | GPIO23 |
| MISO | GPIO19 |
| VCC | 3.3V |
| GND | GND |
So someone presumable has already written FSEQ support in a fork somewhere if xLights has an upload option? |
|
Since I'm already getting tonnes of mail, let's join the fray. What I'll veto is the change in UI. Please remove Fseq button from UI as this is a usermod. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (15)
usermods/FSEQ/sd_manager.cpp (1)
16-41: Consider separating data retrieval from HTML formatting.The listFiles method mixes file system operations with HTML generation. This violates the single responsibility principle and makes the code less reusable.
-String SDManager::listFiles(const char* dirname) { +String SDManager::listFiles(const char* dirname) { String result = ""; File root = SD_ADAPTER.open(dirname); if(!root) { - result += "<li>Failed to open directory: "; - result += dirname; - result += "</li>"; + result = "ERROR:Failed to open directory: " + String(dirname); return result; } if(!root.isDirectory()){ - result += "<li>Not a directory: "; - result += dirname; - result += "</li>"; + result = "ERROR:Not a directory: " + String(dirname); return result; } File file = root.openNextFile(); while(file) { - result += "<li>"; - result += file.name(); - result += " (" + String(file.size()) + " bytes)</li>"; + if(result.length() > 0) result += "\n"; + result += String(file.name()) + ":" + String(file.size()); file.close(); file = root.openNextFile(); } root.close(); return result; }Then format the result in the WebUIManager class.
usermods/FSEQ/README.md (2)
7-7: Fix bare URL formatting.Bare URLs should be properly formatted as markdown links.
-# SWEB UI http://yourIP/sd/ui +# Web UI + +Access the web interface at: [http://yourIP/sd/ui](http://yourIP/sd/ui)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
7-7: Bare URL used
null(MD034, no-bare-urls)
70-97: Replace hard tabs with spaces and improve formatting.The HTTP endpoints section uses hard tabs instead of spaces and lacks proper markdown formatting.
Convert the list to properly formatted markdown:
-HTTP Endpoints - -The following endpoints are available: - • GET /sd/ui - Description: Returns an HTML page with the SD & FSEQ Manager interface. - Usage: Open this URL in a browser. +## HTTP Endpoints + +The following endpoints are available: + +* **GET /sd/ui** + Description: Returns an HTML page with the SD & FSEQ Manager interface. + Usage: Open this URL in a browser.Apply similar formatting to all endpoints in the list.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
73-73: Hard tabs
Column: 1(MD010, no-hard-tabs)
74-74: Hard tabs
Column: 1(MD010, no-hard-tabs)
75-75: Hard tabs
Column: 1(MD010, no-hard-tabs)
76-76: Hard tabs
Column: 1(MD010, no-hard-tabs)
77-77: Hard tabs
Column: 1(MD010, no-hard-tabs)
78-78: Hard tabs
Column: 1(MD010, no-hard-tabs)
79-79: Hard tabs
Column: 1(MD010, no-hard-tabs)
80-80: Hard tabs
Column: 1(MD010, no-hard-tabs)
81-81: Hard tabs
Column: 1(MD010, no-hard-tabs)
82-82: Hard tabs
Column: 1(MD010, no-hard-tabs)
83-83: Hard tabs
Column: 1(MD010, no-hard-tabs)
84-84: Hard tabs
Column: 1(MD010, no-hard-tabs)
85-85: Hard tabs
Column: 1(MD010, no-hard-tabs)
86-86: Hard tabs
Column: 1(MD010, no-hard-tabs)
87-87: Hard tabs
Column: 1(MD010, no-hard-tabs)
88-88: Hard tabs
Column: 1(MD010, no-hard-tabs)
89-89: Hard tabs
Column: 1(MD010, no-hard-tabs)
90-90: Hard tabs
Column: 1(MD010, no-hard-tabs)
91-91: Hard tabs
Column: 1(MD010, no-hard-tabs)
92-92: Hard tabs
Column: 1(MD010, no-hard-tabs)
93-93: Hard tabs
Column: 1(MD010, no-hard-tabs)
94-94: Hard tabs
Column: 1(MD010, no-hard-tabs)
95-95: Hard tabs
Column: 1(MD010, no-hard-tabs)
96-96: Hard tabs
Column: 1(MD010, no-hard-tabs)
usermods/FSEQ/web_ui_manager.cpp (2)
5-6: Translate comments to English for better international accessibility.Non-English comments may make it difficult for other contributors to understand the code.
-// Registrácia všetkých web endpointov +// Register all web endpoints
8-26: Consider extracting HTML templates to separate files.The current approach embeds a large amount of HTML directly in the C++ code, which makes maintenance difficult. Consider using a templating system or at least moving the HTML to separate string constants.
usermods/FSEQ/fseq_player.cpp (2)
53-55: Implement or remove the stubfileOnFS().
This function always returnsfalse, which may cause confusion. If file playback from the filesystem (SPIFFS/LittleFS) is genuinely unsupported, consider removing it entirely; otherwise, implement the actual checks.
194-199: Consider callingstrip.show()after clearing the last playback.
clearLastPlayback()sets pixels to black but does not callstrip.show(), so the LEDs might remain lit until the next update. This might lead to unexpected visible artifacts.usermods/FSEQ/usermod_fpp.h (4)
17-32: Check for failed writes inWriteBufferingStream.
Insideflush(),_upstream.write(...)may fail or return fewer bytes than expected; the current code doesn’t verify that scenario. You might want to check and handle partial writes or errors to avoid data corruption.
71-71: Clarify the rationale behindFILE_UPLOAD_BUFFER_SIZE.
Using563 * 7is unclear to future maintainers. Consider documenting the reason for this specific buffer size, or switch to a straightforward constant.
434-460: Report file list retrieval errors more precisely.
Line 439 checks ifroot.isDirectory(). If it fails, only a generic error is returned. Consider adding more diagnostic info or verifying card initialization success to help users debug issues with reading the SD card.
462-481: Consider authentication for playback controls.
/fpp/connectand/fpp/stopendpoints are publicly accessible. If unauthorized users can trigger or stop FSEQ playback, it could be problematic in certain deployments. At least document the risk or provide optional authentication.usermods/FSEQ/fseq_player.h (1)
53-55: Remove or utilizecolorChannels.
This static field is never referenced infseq_player.cppexcept for initialization. If the code always assumes RGB (3 channels), consider removing or clarifying future usage.usermods/FSEQ/usermod_fseq.h (3)
58-72: Consider more robust handling of SD initialization failure.
Currently, SD initialization failure just logs an error and continues. If SD is critical for your usermod’s functionality, you may want to disable certain features or provide user feedback when initialization fails, to avoid undefined behavior later.
85-96: Conditionally hide or adjust the Info tab link for uninitialized SD.
When the SD fails to initialize, the link in the Info tab still appears, potentially leading to errors if accessed. Consider making the link dynamic or providing a different message if the SD card is not available.
127-159: Ensure thread safety during SD reinitialization.
While this method deallocates and reallocates pins, there’s no explicit lock mechanism to prevent file I/O during reinit. If this method can be triggered while playback is in progress, you may face data races or partial writes. A simple solution is to suspend playback, perform reinit, then resume.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
usermods/FSEQ/README.md(1 hunks)usermods/FSEQ/fseq_player.cpp(1 hunks)usermods/FSEQ/fseq_player.h(1 hunks)usermods/FSEQ/sd_manager.cpp(1 hunks)usermods/FSEQ/sd_manager.h(1 hunks)usermods/FSEQ/usermod_fpp.h(1 hunks)usermods/FSEQ/usermod_fseq.h(1 hunks)usermods/FSEQ/web_ui_manager.cpp(1 hunks)usermods/FSEQ/web_ui_manager.h(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- usermods/FSEQ/web_ui_manager.h
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
usermods/FSEQ/README.md
7-7: Bare URL used
null
(MD034, no-bare-urls)
73-73: Hard tabs
Column: 1
(MD010, no-hard-tabs)
74-74: Hard tabs
Column: 1
(MD010, no-hard-tabs)
75-75: Hard tabs
Column: 1
(MD010, no-hard-tabs)
76-76: Hard tabs
Column: 1
(MD010, no-hard-tabs)
77-77: Hard tabs
Column: 1
(MD010, no-hard-tabs)
78-78: Hard tabs
Column: 1
(MD010, no-hard-tabs)
79-79: Hard tabs
Column: 1
(MD010, no-hard-tabs)
80-80: Hard tabs
Column: 1
(MD010, no-hard-tabs)
81-81: Hard tabs
Column: 1
(MD010, no-hard-tabs)
82-82: Hard tabs
Column: 1
(MD010, no-hard-tabs)
83-83: Hard tabs
Column: 1
(MD010, no-hard-tabs)
84-84: Hard tabs
Column: 1
(MD010, no-hard-tabs)
85-85: Hard tabs
Column: 1
(MD010, no-hard-tabs)
86-86: Hard tabs
Column: 1
(MD010, no-hard-tabs)
87-87: Hard tabs
Column: 1
(MD010, no-hard-tabs)
88-88: Hard tabs
Column: 1
(MD010, no-hard-tabs)
89-89: Hard tabs
Column: 1
(MD010, no-hard-tabs)
90-90: Hard tabs
Column: 1
(MD010, no-hard-tabs)
91-91: Hard tabs
Column: 1
(MD010, no-hard-tabs)
92-92: Hard tabs
Column: 1
(MD010, no-hard-tabs)
93-93: Hard tabs
Column: 1
(MD010, no-hard-tabs)
94-94: Hard tabs
Column: 1
(MD010, no-hard-tabs)
95-95: Hard tabs
Column: 1
(MD010, no-hard-tabs)
96-96: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (15)
usermods/FSEQ/sd_manager.h (1)
1-22: Implementation looks solid and well-structured.The SDManager class provides a clean interface for SD card operations with appropriate conditional compilation to support both SPI and MMC interfaces. The header file is well-organized with proper header guards.
usermods/FSEQ/sd_manager.cpp (2)
3-10: Implementation correctly handles different SD card interfaces.The begin() method properly initializes the SD card based on the defined interface type (SPI or MMC), with good error handling.
43-45: LGTM: Simple and effective implementation.The deleteFile method is a straightforward and efficient wrapper for the SD adapter's remove function.
usermods/FSEQ/README.md (5)
1-6: Good introduction and project summary.The introduction clearly explains the purpose and key features of the usermod.
15-21: Clear feature list.The features section provides a good summary of the usermod's capabilities.
26-39: Installation instructions are well-documented.The PlatformIO configuration section clearly shows the required build flags.
45-50: Good UI integration instructions.The instructions for adding the FSEQ button to the WLED web interface are clear and straightforward.
1-117: Consider adding hardware requirements section.The introduction should specify the required hardware setup, particularly the SD card module specifications and wiring details.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
7-7: Bare URL used
null(MD034, no-bare-urls)
73-73: Hard tabs
Column: 1(MD010, no-hard-tabs)
74-74: Hard tabs
Column: 1(MD010, no-hard-tabs)
75-75: Hard tabs
Column: 1(MD010, no-hard-tabs)
76-76: Hard tabs
Column: 1(MD010, no-hard-tabs)
77-77: Hard tabs
Column: 1(MD010, no-hard-tabs)
78-78: Hard tabs
Column: 1(MD010, no-hard-tabs)
79-79: Hard tabs
Column: 1(MD010, no-hard-tabs)
80-80: Hard tabs
Column: 1(MD010, no-hard-tabs)
81-81: Hard tabs
Column: 1(MD010, no-hard-tabs)
82-82: Hard tabs
Column: 1(MD010, no-hard-tabs)
83-83: Hard tabs
Column: 1(MD010, no-hard-tabs)
84-84: Hard tabs
Column: 1(MD010, no-hard-tabs)
85-85: Hard tabs
Column: 1(MD010, no-hard-tabs)
86-86: Hard tabs
Column: 1(MD010, no-hard-tabs)
87-87: Hard tabs
Column: 1(MD010, no-hard-tabs)
88-88: Hard tabs
Column: 1(MD010, no-hard-tabs)
89-89: Hard tabs
Column: 1(MD010, no-hard-tabs)
90-90: Hard tabs
Column: 1(MD010, no-hard-tabs)
91-91: Hard tabs
Column: 1(MD010, no-hard-tabs)
92-92: Hard tabs
Column: 1(MD010, no-hard-tabs)
93-93: Hard tabs
Column: 1(MD010, no-hard-tabs)
94-94: Hard tabs
Column: 1(MD010, no-hard-tabs)
95-95: Hard tabs
Column: 1(MD010, no-hard-tabs)
96-96: Hard tabs
Column: 1(MD010, no-hard-tabs)
usermods/FSEQ/web_ui_manager.cpp (1)
199-204: LGTM: FSEQ stop endpoint is properly implemented.The stop endpoint correctly clears the last playback and releases the realtime lock.
usermods/FSEQ/fseq_player.cpp (2)
69-88: Handle partial channel data and unusedcolorChannels.
Currently,processFrameData()reads data in multiples of 3 bytes for RGB, yet it unconditionally treats every pixel as RGB. Iffile_header.channel_countis not a multiple of 3, leftover bytes are silently ignored. Additionally, the staticcolorChannelsvariable is never referenced here.Would you like to confirm if these FSEQ files always use exactly 3 channels per pixel by design, or should we handle other channel arrangements?
129-192: Validate FSEQ file properties more thoroughly.
Currently, the code reads only the header once. Unsafe or malformed values forchannel_countorframe_countcould lead to out-of-range reads. Additionally, consider clampingframeto0if(secondsElapsed * 1000.0f) / step_timeyields a negative index.usermods/FSEQ/fseq_player.h (1)
63-64: File reading fallback infileOnFS().
Similar to the.cppfile,fileOnFSis declared but not actually implemented. If local filesystem playback is not supported, drop the method entirely or clarify the intended behavior.usermods/FSEQ/usermod_fseq.h (3)
36-42: Check for SPI bus conflicts with other usermods.
Defining a globalspiPortand referencingVSPIcan interfere with other usermods that run on the same hardware SPI pins. Ensure that no other part of the firmware is simultaneously initializing or overriding this SPI instance.
74-78: Verify concurrency when reinitializing SD at runtime.
If the user changes SD configuration while FSEQ playback is active, this loop call toFSEQPlayer::handlePlayRecording();might conflict with the reinitialization process. Consider adding synchronization or checks to avoid partial reads from the SD.
173-181: Validate default SPI pin mappings for diverse boards.
The default pin assignments (CS=5, SCK=18, MISO=19, MOSI=23) may not be valid or optimal for all ESP32 boards or setups. Consider adding documentation or checks to ensure safe defaults on various hardware configurations.
usermods/FSEQ/web_ui_manager.cpp
Outdated
| server.on("/fseq/start", HTTP_GET, [](AsyncWebServerRequest *request) { | ||
| if (!request->hasArg("file")) { | ||
| request->send(400, "text/plain", "Missing 'file' parameter"); | ||
| return; | ||
| } | ||
| String filepath = request->arg("file"); | ||
| if (!filepath.startsWith("/")) filepath = "/" + filepath; | ||
| FSEQPlayer::loadRecording(filepath.c_str(), 0, uint16_t(-1), 0.0f); | ||
| request->send(200, "text/plain", "FSEQ started: " + filepath); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add file existence check before starting FSEQ playback.
The FSEQ start endpoint doesn't verify if the specified file exists before attempting to play it.
server.on("/fseq/start", HTTP_GET, [](AsyncWebServerRequest *request) {
if (!request->hasArg("file")) {
request->send(400, "text/plain", "Missing 'file' parameter");
return;
}
String filepath = request->arg("file");
if (!filepath.startsWith("/")) filepath = "/" + filepath;
+
+ // Check if file exists
+ if (!SD_ADAPTER.exists(filepath.c_str())) {
+ request->send(404, "text/plain", "File not found: " + filepath);
+ return;
+ }
+
FSEQPlayer::loadRecording(filepath.c_str(), 0, uint16_t(-1), 0.0f);
request->send(200, "text/plain", "FSEQ started: " + filepath);
});📝 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.
| server.on("/fseq/start", HTTP_GET, [](AsyncWebServerRequest *request) { | |
| if (!request->hasArg("file")) { | |
| request->send(400, "text/plain", "Missing 'file' parameter"); | |
| return; | |
| } | |
| String filepath = request->arg("file"); | |
| if (!filepath.startsWith("/")) filepath = "/" + filepath; | |
| FSEQPlayer::loadRecording(filepath.c_str(), 0, uint16_t(-1), 0.0f); | |
| request->send(200, "text/plain", "FSEQ started: " + filepath); | |
| }); | |
| server.on("/fseq/start", HTTP_GET, [](AsyncWebServerRequest *request) { | |
| if (!request->hasArg("file")) { | |
| request->send(400, "text/plain", "Missing 'file' parameter"); | |
| return; | |
| } | |
| String filepath = request->arg("file"); | |
| if (!filepath.startsWith("/")) filepath = "/" + filepath; | |
| // Check if file exists | |
| if (!SD_ADAPTER.exists(filepath.c_str())) { | |
| request->send(404, "text/plain", "File not found: " + filepath); | |
| return; | |
| } | |
| FSEQPlayer::loadRecording(filepath.c_str(), 0, uint16_t(-1), 0.0f); | |
| request->send(200, "text/plain", "FSEQ started: " + filepath); | |
| }); |
usermods/FSEQ/web_ui_manager.cpp
Outdated
| // Endpoint for deleting files | ||
| server.on("/sd/delete", HTTP_GET, [](AsyncWebServerRequest *request) { | ||
| if (!request->hasArg("path")) { | ||
| request->send(400, "text/plain", "Missing 'path' parameter"); | ||
| return; | ||
| } | ||
| String path = request->arg("path"); | ||
| if (!path.startsWith("/")) path = "/" + path; | ||
| bool res = SD_ADAPTER.remove(path.c_str()); | ||
| String msg = res ? "File deleted" : "Delete failed"; | ||
| request->send(200, "text/plain", msg); | ||
| }); |
There was a problem hiding this comment.
Implement path validation for file deletion.
The delete endpoint doesn't validate paths properly, which could allow deletion of critical files if malicious paths are provided.
server.on("/sd/delete", HTTP_GET, [](AsyncWebServerRequest *request) {
if (!request->hasArg("path")) {
request->send(400, "text/plain", "Missing 'path' parameter");
return;
}
String path = request->arg("path");
if (!path.startsWith("/")) path = "/" + path;
+
+ // Validate path - prevent access to system folders or traversal
+ if (path.indexOf("..") >= 0 || path.startsWith("/system") || path.length() < 2) {
+ request->send(403, "text/plain", "Access denied");
+ return;
+ }
+
bool res = SD_ADAPTER.remove(path.c_str());
String msg = res ? "File deleted" : "Delete failed";
request->send(200, "text/plain", msg);
});📝 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.
| // Endpoint for deleting files | |
| server.on("/sd/delete", HTTP_GET, [](AsyncWebServerRequest *request) { | |
| if (!request->hasArg("path")) { | |
| request->send(400, "text/plain", "Missing 'path' parameter"); | |
| return; | |
| } | |
| String path = request->arg("path"); | |
| if (!path.startsWith("/")) path = "/" + path; | |
| bool res = SD_ADAPTER.remove(path.c_str()); | |
| String msg = res ? "File deleted" : "Delete failed"; | |
| request->send(200, "text/plain", msg); | |
| }); | |
| // Endpoint for deleting files | |
| server.on("/sd/delete", HTTP_GET, [](AsyncWebServerRequest *request) { | |
| if (!request->hasArg("path")) { | |
| request->send(400, "text/plain", "Missing 'path' parameter"); | |
| return; | |
| } | |
| String path = request->arg("path"); | |
| if (!path.startsWith("/")) path = "/" + path; | |
| // Validate path - prevent access to system folders or traversal | |
| if (path.indexOf("..") >= 0 || path.startsWith("/system") || path.length() < 2) { | |
| request->send(403, "text/plain", "Access denied"); | |
| return; | |
| } | |
| bool res = SD_ADAPTER.remove(path.c_str()); | |
| String msg = res ? "File deleted" : "Delete failed"; | |
| request->send(200, "text/plain", msg); | |
| }); |
usermods/FSEQ/web_ui_manager.cpp
Outdated
| // Endpoint for uploadind files | ||
| server.on("/sd/upload", HTTP_POST, [](AsyncWebServerRequest *request) { | ||
| request->send(200, "text/plain", "Upload complete"); | ||
| }, [](AsyncWebServerRequest *request, String filename, size_t index, uint8_t *data, size_t len, bool final) { | ||
| static File uploadFile; | ||
| String path = filename; | ||
| if (!filename.startsWith("/")) path = "/" + filename; | ||
| if(index == 0) { | ||
| DEBUG_PRINTF("[SD] Starting upload for file: %s\n", path.c_str()); | ||
| uploadFile = SD_ADAPTER.open(path.c_str(), FILE_WRITE); | ||
| if (!uploadFile) { | ||
| DEBUG_PRINTF("[SD] Failed to open file for writing: %s\n", path.c_str()); | ||
| } | ||
| } | ||
| if(uploadFile) { | ||
| size_t written = uploadFile.write(data, len); | ||
| DEBUG_PRINTF("[SD] Writing %d bytes to file: %s (written: %d bytes)\n", len, path.c_str(), written); | ||
| } | ||
| if(final) { | ||
| if(uploadFile) { | ||
| uploadFile.close(); | ||
| DEBUG_PRINTF("[SD] Upload complete and file closed: %s\n", path.c_str()); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement proper error handling for file uploads.
The upload handler doesn't return error messages to the client when a file can't be opened for writing. Consider adding an error response mechanism.
server.on("/sd/upload", HTTP_POST, [](AsyncWebServerRequest *request) {
request->send(200, "text/plain", "Upload complete");
}, [](AsyncWebServerRequest *request, String filename, size_t index, uint8_t *data, size_t len, bool final) {
static File uploadFile;
+ static bool uploadError = false;
String path = filename;
if (!filename.startsWith("/")) path = "/" + filename;
if(index == 0) {
DEBUG_PRINTF("[SD] Starting upload for file: %s\n", path.c_str());
+ uploadError = false;
uploadFile = SD_ADAPTER.open(path.c_str(), FILE_WRITE);
if (!uploadFile) {
DEBUG_PRINTF("[SD] Failed to open file for writing: %s\n", path.c_str());
+ uploadError = true;
+ request->client()->text("Error: Failed to open file for writing");
+ return;
}
}
+ if (uploadError) return;
if(uploadFile) {
size_t written = uploadFile.write(data, len);
+ if (written != len) {
+ DEBUG_PRINTF("[SD] Write error: only %d of %d bytes written\n", written, len);
+ uploadError = true;
+ request->client()->text("Error: Failed to write data to file");
+ }
DEBUG_PRINTF("[SD] Writing %d bytes to file: %s (written: %d bytes)\n", len, path.c_str(), written);
}
if(final) {
if(uploadFile) {
uploadFile.close();
DEBUG_PRINTF("[SD] Upload complete and file closed: %s\n", path.c_str());
+ if (!uploadError) {
+ request->client()->text("Upload complete");
+ }
}
}
});📝 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.
| // Endpoint for uploadind files | |
| server.on("/sd/upload", HTTP_POST, [](AsyncWebServerRequest *request) { | |
| request->send(200, "text/plain", "Upload complete"); | |
| }, [](AsyncWebServerRequest *request, String filename, size_t index, uint8_t *data, size_t len, bool final) { | |
| static File uploadFile; | |
| String path = filename; | |
| if (!filename.startsWith("/")) path = "/" + filename; | |
| if(index == 0) { | |
| DEBUG_PRINTF("[SD] Starting upload for file: %s\n", path.c_str()); | |
| uploadFile = SD_ADAPTER.open(path.c_str(), FILE_WRITE); | |
| if (!uploadFile) { | |
| DEBUG_PRINTF("[SD] Failed to open file for writing: %s\n", path.c_str()); | |
| } | |
| } | |
| if(uploadFile) { | |
| size_t written = uploadFile.write(data, len); | |
| DEBUG_PRINTF("[SD] Writing %d bytes to file: %s (written: %d bytes)\n", len, path.c_str(), written); | |
| } | |
| if(final) { | |
| if(uploadFile) { | |
| uploadFile.close(); | |
| DEBUG_PRINTF("[SD] Upload complete and file closed: %s\n", path.c_str()); | |
| } | |
| } | |
| }); | |
| // Endpoint for uploadind files | |
| server.on("/sd/upload", HTTP_POST, [](AsyncWebServerRequest *request) { | |
| request->send(200, "text/plain", "Upload complete"); | |
| }, [](AsyncWebServerRequest *request, String filename, size_t index, uint8_t *data, size_t len, bool final) { | |
| static File uploadFile; | |
| static bool uploadError = false; | |
| String path = filename; | |
| if (!filename.startsWith("/")) path = "/" + filename; | |
| if(index == 0) { | |
| DEBUG_PRINTF("[SD] Starting upload for file: %s\n", path.c_str()); | |
| uploadError = false; | |
| uploadFile = SD_ADAPTER.open(path.c_str(), FILE_WRITE); | |
| if (!uploadFile) { | |
| DEBUG_PRINTF("[SD] Failed to open file for writing: %s\n", path.c_str()); | |
| uploadError = true; | |
| request->client()->text("Error: Failed to open file for writing"); | |
| return; | |
| } | |
| } | |
| if (uploadError) return; | |
| if(uploadFile) { | |
| size_t written = uploadFile.write(data, len); | |
| if (written != len) { | |
| DEBUG_PRINTF("[SD] Write error: only %d of %d bytes written\n", written, len); | |
| uploadError = true; | |
| request->client()->text("Error: Failed to write data to file"); | |
| } | |
| DEBUG_PRINTF("[SD] Writing %d bytes to file: %s (written: %d bytes)\n", len, path.c_str(), written); | |
| } | |
| if(final) { | |
| if(uploadFile) { | |
| uploadFile.close(); | |
| DEBUG_PRINTF("[SD] Upload complete and file closed: %s\n", path.c_str()); | |
| if (!uploadError) { | |
| request->client()->text("Upload complete"); | |
| } | |
| } | |
| } | |
| }); |
usermods/FSEQ/web_ui_manager.cpp
Outdated
| // Výpis súborov na SD karte s tlačidlom Delete a možnosťou uploadu | ||
| server.on("/sd/list", HTTP_GET, [](AsyncWebServerRequest *request) { | ||
| String html = "<html><head><meta charset='utf-8'><title>SD Card Files</title>"; | ||
| html += "<style>"; | ||
| html += "body { font-family: sans-serif; font-size: 24px; color: #00FF00; background-color: #000; margin: 0; padding: 20px; }"; | ||
| html += "h1 { margin-top: 0; }"; | ||
| html += "ul { list-style: none; margin: 0; padding: 0; }"; | ||
| html += "li { margin-bottom: 10px; }"; | ||
| html += "a, button { display: inline-block; font-size: 24px; color: #00FF00; border: 2px solid #00FF00; background-color: transparent; padding: 10px 20px; margin: 5px; text-decoration: none; }"; | ||
| html += "a:hover, button:hover { background-color: #00FF00; color: #000; }"; | ||
| html += ".deleteLink { border-color: #FF0000; color: #FF0000; }"; | ||
| html += ".deleteLink:hover { background-color: #FF0000; color: #000; }"; | ||
| html += ".backLink { border: 2px solid #00FF00; padding: 10px 20px; }"; | ||
| html += "</style></head><body>"; | ||
| html += "<h1>SD Card Files</h1><ul>"; | ||
|
|
||
| SDManager sd; | ||
| File root = SD_ADAPTER.open("/"); | ||
| if(root && root.isDirectory()){ | ||
| File file = root.openNextFile(); | ||
| while(file){ | ||
| String name = file.name(); | ||
| html += "<li>" + name + " (" + String(file.size()) + " bytes) "; | ||
| // Namiesto priameho linku voláme funkciu deleteFile() | ||
| html += "<a href='#' class='deleteLink' onclick=\"deleteFile('" + name + "')\">Delete</a></li>"; | ||
| file.close(); | ||
| file = root.openNextFile(); | ||
| } | ||
| } else { | ||
| html += "<li>Failed to open directory: /</li>"; | ||
| } | ||
| root.close(); | ||
|
|
||
| html += "</ul>"; | ||
| html += "<h2>Upload File</h2>"; | ||
| html += "<form id='uploadForm' enctype='multipart/form-data'>"; | ||
| html += "Select file: <input type='file' name='upload'><br><br>"; | ||
| html += "<input type='submit' value='Upload'>"; | ||
| html += "</form>"; | ||
| html += "<div id='uploadStatus'></div>"; | ||
| html += "<p><a href='/sd/ui' class='backLink'>BACK</a></p>"; | ||
| html += "<script>"; | ||
| html += "document.getElementById('uploadForm').addEventListener('submit', function(e) {"; | ||
| html += " e.preventDefault();"; | ||
| html += " var formData = new FormData(this);"; | ||
| html += " document.getElementById('uploadStatus').innerText = 'Uploading...';"; | ||
| html += " fetch('/sd/upload', { method: 'POST', body: formData })"; | ||
| html += " .then(response => response.text())"; | ||
| html += " .then(data => {"; | ||
| html += " document.getElementById('uploadStatus').innerText = data;"; | ||
| html += " setTimeout(function() { location.reload(); }, 1000);"; | ||
| html += " })"; | ||
| html += " .catch(err => {"; | ||
| html += " document.getElementById('uploadStatus').innerText = 'Upload failed';"; | ||
| html += " });"; | ||
| html += "});"; | ||
| // Upravená funkcia pre mazanie súborov, ktorá spracováva požiadavku asynchrónne | ||
| html += "function deleteFile(filename) {"; | ||
| html += " if (!confirm('Are you sure you want to delete ' + filename + '?')) return;"; | ||
| html += " fetch('/sd/delete?path=' + encodeURIComponent(filename))"; | ||
| html += " .then(response => response.text())"; | ||
| html += " .then(data => {"; | ||
| html += " alert(data);"; | ||
| html += " setTimeout(function() { location.reload(); }, 1000);"; | ||
| html += " })"; | ||
| html += " .catch(err => { alert('Delete failed'); });"; | ||
| html += "}"; | ||
| html += "</script>"; | ||
| html += "</body></html>"; | ||
| request->send(200, "text/html", html); | ||
| }); |
There was a problem hiding this comment.
Add input validation for directory paths.
The file listing functionality doesn't validate or sanitize the directory path, which could lead to potential security issues if malicious inputs are provided.
Add path validation before accessing the SD card:
server.on("/sd/list", HTTP_GET, [](AsyncWebServerRequest *request) {
String html = "<html><head><meta charset='utf-8'><title>SD Card Files</title>";
// ... HTML generation code ...
SDManager sd;
+ // Validate path - only allow root directory for now
+ String dirPath = "/";
+ if (request->hasArg("dir")) {
+ dirPath = request->arg("dir");
+ // Sanitize path - ensure it starts with / and doesn't contain ..
+ if (!dirPath.startsWith("/")) dirPath = "/" + dirPath;
+ if (dirPath.indexOf("..") >= 0) dirPath = "/";
+ }
- File root = SD_ADAPTER.open("/");
+ File root = SD_ADAPTER.open(dirPath.c_str());📝 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.
| // Výpis súborov na SD karte s tlačidlom Delete a možnosťou uploadu | |
| server.on("/sd/list", HTTP_GET, [](AsyncWebServerRequest *request) { | |
| String html = "<html><head><meta charset='utf-8'><title>SD Card Files</title>"; | |
| html += "<style>"; | |
| html += "body { font-family: sans-serif; font-size: 24px; color: #00FF00; background-color: #000; margin: 0; padding: 20px; }"; | |
| html += "h1 { margin-top: 0; }"; | |
| html += "ul { list-style: none; margin: 0; padding: 0; }"; | |
| html += "li { margin-bottom: 10px; }"; | |
| html += "a, button { display: inline-block; font-size: 24px; color: #00FF00; border: 2px solid #00FF00; background-color: transparent; padding: 10px 20px; margin: 5px; text-decoration: none; }"; | |
| html += "a:hover, button:hover { background-color: #00FF00; color: #000; }"; | |
| html += ".deleteLink { border-color: #FF0000; color: #FF0000; }"; | |
| html += ".deleteLink:hover { background-color: #FF0000; color: #000; }"; | |
| html += ".backLink { border: 2px solid #00FF00; padding: 10px 20px; }"; | |
| html += "</style></head><body>"; | |
| html += "<h1>SD Card Files</h1><ul>"; | |
| SDManager sd; | |
| File root = SD_ADAPTER.open("/"); | |
| if(root && root.isDirectory()){ | |
| File file = root.openNextFile(); | |
| while(file){ | |
| String name = file.name(); | |
| html += "<li>" + name + " (" + String(file.size()) + " bytes) "; | |
| // Namiesto priameho linku voláme funkciu deleteFile() | |
| html += "<a href='#' class='deleteLink' onclick=\"deleteFile('" + name + "')\">Delete</a></li>"; | |
| file.close(); | |
| file = root.openNextFile(); | |
| } | |
| } else { | |
| html += "<li>Failed to open directory: /</li>"; | |
| } | |
| root.close(); | |
| html += "</ul>"; | |
| html += "<h2>Upload File</h2>"; | |
| html += "<form id='uploadForm' enctype='multipart/form-data'>"; | |
| html += "Select file: <input type='file' name='upload'><br><br>"; | |
| html += "<input type='submit' value='Upload'>"; | |
| html += "</form>"; | |
| html += "<div id='uploadStatus'></div>"; | |
| html += "<p><a href='/sd/ui' class='backLink'>BACK</a></p>"; | |
| html += "<script>"; | |
| html += "document.getElementById('uploadForm').addEventListener('submit', function(e) {"; | |
| html += " e.preventDefault();"; | |
| html += " var formData = new FormData(this);"; | |
| html += " document.getElementById('uploadStatus').innerText = 'Uploading...';"; | |
| html += " fetch('/sd/upload', { method: 'POST', body: formData })"; | |
| html += " .then(response => response.text())"; | |
| html += " .then(data => {"; | |
| html += " document.getElementById('uploadStatus').innerText = data;"; | |
| html += " setTimeout(function() { location.reload(); }, 1000);"; | |
| html += " })"; | |
| html += " .catch(err => {"; | |
| html += " document.getElementById('uploadStatus').innerText = 'Upload failed';"; | |
| html += " });"; | |
| html += "});"; | |
| // Upravená funkcia pre mazanie súborov, ktorá spracováva požiadavku asynchrónne | |
| html += "function deleteFile(filename) {"; | |
| html += " if (!confirm('Are you sure you want to delete ' + filename + '?')) return;"; | |
| html += " fetch('/sd/delete?path=' + encodeURIComponent(filename))"; | |
| html += " .then(response => response.text())"; | |
| html += " .then(data => {"; | |
| html += " alert(data);"; | |
| html += " setTimeout(function() { location.reload(); }, 1000);"; | |
| html += " })"; | |
| html += " .catch(err => { alert('Delete failed'); });"; | |
| html += "}"; | |
| html += "</script>"; | |
| html += "</body></html>"; | |
| request->send(200, "text/html", html); | |
| }); | |
| // Výpis súborov na SD karte s tlačidlom Delete a možnosťou uploadu | |
| server.on("/sd/list", HTTP_GET, [](AsyncWebServerRequest *request) { | |
| String html = "<html><head><meta charset='utf-8'><title>SD Card Files</title>"; | |
| html += "<style>"; | |
| html += "body { font-family: sans-serif; font-size: 24px; color: #00FF00; background-color: #000; margin: 0; padding: 20px; }"; | |
| html += "h1 { margin-top: 0; }"; | |
| html += "ul { list-style: none; margin: 0; padding: 0; }"; | |
| html += "li { margin-bottom: 10px; }"; | |
| html += "a, button { display: inline-block; font-size: 24px; color: #00FF00; border: 2px solid #00FF00; background-color: transparent; padding: 10px 20px; margin: 5px; text-decoration: none; }"; | |
| html += "a:hover, button:hover { background-color: #00FF00; color: #000; }"; | |
| html += ".deleteLink { border-color: #FF0000; color: #FF0000; }"; | |
| html += ".deleteLink:hover { background-color: #FF0000; color: #000; }"; | |
| html += ".backLink { border: 2px solid #00FF00; padding: 10px 20px; }"; | |
| html += "</style></head><body>"; | |
| html += "<h1>SD Card Files</h1><ul>"; | |
| SDManager sd; | |
| // Validate path - only allow root directory for now | |
| String dirPath = "/"; | |
| if (request->hasArg("dir")) { | |
| dirPath = request->arg("dir"); | |
| // Sanitize path - ensure it starts with / and doesn't contain .. | |
| if (!dirPath.startsWith("/")) dirPath = "/" + dirPath; | |
| if (dirPath.indexOf("..") >= 0) dirPath = "/"; | |
| } | |
| File root = SD_ADAPTER.open(dirPath.c_str()); | |
| if(root && root.isDirectory()){ | |
| File file = root.openNextFile(); | |
| while(file){ | |
| String name = file.name(); | |
| html += "<li>" + name + " (" + String(file.size()) + " bytes) "; | |
| // Namiesto priameho linku voláme funkciu deleteFile() | |
| html += "<a href='#' class='deleteLink' onclick=\"deleteFile('" + name + "')\">Delete</a></li>"; | |
| file.close(); | |
| file = root.openNextFile(); | |
| } | |
| } else { | |
| html += "<li>Failed to open directory: /</li>"; | |
| } | |
| root.close(); | |
| html += "</ul>"; | |
| html += "<h2>Upload File</h2>"; | |
| html += "<form id='uploadForm' enctype='multipart/form-data'>"; | |
| html += "Select file: <input type='file' name='upload'><br><br>"; | |
| html += "<input type='submit' value='Upload'>"; | |
| html += "</form>"; | |
| html += "<div id='uploadStatus'></div>"; | |
| html += "<p><a href='/sd/ui' class='backLink'>BACK</a></p>"; | |
| html += "<script>"; | |
| html += "document.getElementById('uploadForm').addEventListener('submit', function(e) {"; | |
| html += " e.preventDefault();"; | |
| html += " var formData = new FormData(this);"; | |
| html += " document.getElementById('uploadStatus').innerText = 'Uploading...';"; | |
| html += " fetch('/sd/upload', { method: 'POST', body: formData })"; | |
| html += " .then(response => response.text())"; | |
| html += " .then(data => {"; | |
| html += " document.getElementById('uploadStatus').innerText = data;"; | |
| html += " setTimeout(function() { location.reload(); }, 1000);"; | |
| html += " })"; | |
| html += " .catch(err => {"; | |
| html += " document.getElementById('uploadStatus').innerText = 'Upload failed';"; | |
| html += " });"; | |
| html += "});"; | |
| // Upravená funkcia pre mazanie súborov, ktorá spracováva požiadavku asynchrónne | |
| html += "function deleteFile(filename) {"; | |
| html += " if (!confirm('Are you sure you want to delete ' + filename + '?')) return;"; | |
| html += " fetch('/sd/delete?path=' + encodeURIComponent(filename))"; | |
| html += " .then(response => response.text())"; | |
| html += " .then(data => {"; | |
| html += " alert(data);"; | |
| html += " setTimeout(function() { location.reload(); }, 1000);"; | |
| html += " })"; | |
| html += " .catch(err => { alert('Delete failed'); });"; | |
| html += "}"; | |
| html += "</script>"; | |
| html += "</body></html>"; | |
| request->send(200, "text/html", html); | |
| }); |
usermods/FSEQ/fseq_player.cpp
Outdated
| bool FSEQPlayer::isPlaying() { | ||
| return recordingFile && recordingFile.available(); | ||
| } | ||
|
|
||
| void FSEQPlayer::syncPlayback(float secondsElapsed) { | ||
| if (!isPlaying()) { | ||
| DEBUG_PRINTLN("[FSEQ] Sync: Playback not active, cannot sync."); | ||
| return; | ||
| } | ||
|
|
||
| uint32_t expectedFrame = (uint32_t)((secondsElapsed * 1000.0f) / file_header.step_time); | ||
| int32_t diff = (int32_t)expectedFrame - (int32_t)frame; | ||
|
|
||
| if (abs(diff) > 2) { | ||
| frame = expectedFrame; | ||
| uint32_t offset = file_header.channel_data_offset + file_header.channel_count * frame; | ||
| if (recordingFile.seek(offset)) { | ||
| DEBUG_PRINTF("[FSEQ] Sync: Adjusted frame to %lu (diff=%ld)\n", expectedFrame, diff); | ||
| } else { | ||
| DEBUG_PRINTLN("[FSEQ] Sync: Failed to seek to new frame"); | ||
| } | ||
| } else { | ||
| DEBUG_PRINTF("[FSEQ] Sync: No adjustment needed (current frame: %lu, expected: %lu)\n", frame, expectedFrame); | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid seeking out of file range in syncPlayback().
When the calculated expectedFrame exceeds the total frame count, the code attempts a file seek which can fail silently outside valid bounds. Consider clamping expectedFrame to [0, file_header.frame_count - 1].
uint32_t expectedFrame = ...;
+ if (expectedFrame >= file_header.frame_count) {
+ expectedFrame = file_header.frame_count - 1;
+ }
...📝 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.
| bool FSEQPlayer::isPlaying() { | |
| return recordingFile && recordingFile.available(); | |
| } | |
| void FSEQPlayer::syncPlayback(float secondsElapsed) { | |
| if (!isPlaying()) { | |
| DEBUG_PRINTLN("[FSEQ] Sync: Playback not active, cannot sync."); | |
| return; | |
| } | |
| uint32_t expectedFrame = (uint32_t)((secondsElapsed * 1000.0f) / file_header.step_time); | |
| int32_t diff = (int32_t)expectedFrame - (int32_t)frame; | |
| if (abs(diff) > 2) { | |
| frame = expectedFrame; | |
| uint32_t offset = file_header.channel_data_offset + file_header.channel_count * frame; | |
| if (recordingFile.seek(offset)) { | |
| DEBUG_PRINTF("[FSEQ] Sync: Adjusted frame to %lu (diff=%ld)\n", expectedFrame, diff); | |
| } else { | |
| DEBUG_PRINTLN("[FSEQ] Sync: Failed to seek to new frame"); | |
| } | |
| } else { | |
| DEBUG_PRINTF("[FSEQ] Sync: No adjustment needed (current frame: %lu, expected: %lu)\n", frame, expectedFrame); | |
| } | |
| } | |
| bool FSEQPlayer::isPlaying() { | |
| return recordingFile && recordingFile.available(); | |
| } | |
| void FSEQPlayer::syncPlayback(float secondsElapsed) { | |
| if (!isPlaying()) { | |
| DEBUG_PRINTLN("[FSEQ] Sync: Playback not active, cannot sync."); | |
| return; | |
| } | |
| uint32_t expectedFrame = (uint32_t)((secondsElapsed * 1000.0f) / file_header.step_time); | |
| if (expectedFrame >= file_header.frame_count) { | |
| expectedFrame = file_header.frame_count - 1; | |
| } | |
| int32_t diff = (int32_t)expectedFrame - (int32_t)frame; | |
| if (abs(diff) > 2) { | |
| frame = expectedFrame; | |
| uint32_t offset = file_header.channel_data_offset + file_header.channel_count * frame; | |
| if (recordingFile.seek(offset)) { | |
| DEBUG_PRINTF("[FSEQ] Sync: Adjusted frame to %lu (diff=%ld)\n", expectedFrame, diff); | |
| } else { | |
| DEBUG_PRINTLN("[FSEQ] Sync: Failed to seek to new frame"); | |
| } | |
| } else { | |
| DEBUG_PRINTF("[FSEQ] Sync: No adjustment needed (current frame: %lu, expected: %lu)\n", frame, expectedFrame); | |
| } | |
| } |
usermods/FSEQ/fseq_player.cpp
Outdated
| bool FSEQPlayer::stopBecauseAtTheEnd() { | ||
| if (!recordingFile.available()) { | ||
| if (recordingRepeats == RECORDING_REPEAT_LOOP) { | ||
| recordingFile.seek(0); | ||
| } else if (recordingRepeats > 0) { | ||
| recordingFile.seek(0); | ||
| recordingRepeats--; | ||
| DEBUG_PRINTF("Repeat recording again for: %d\n", recordingRepeats); | ||
| } else { | ||
| DEBUG_PRINTLN("Finished playing recording, disabling realtime mode"); | ||
| realtimeLock(10, REALTIME_MODE_INACTIVE); | ||
| recordingFile.close(); | ||
| clearLastPlayback(); | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Risk of incorrect looping on repeated playback.
When the end of the file is reached and recordingRepeats > 0 or RECORDING_REPEAT_LOOP, the code calls seek(0) without re-reading the header, and the frame counter is not reset to zero. This may lead to seeking frames at offsets beyond the header on subsequent loops.
} else if (recordingRepeats > 0) {
recordingFile.seek(0);
+ frame = 0; // Reset the frame to avoid offset misalignment.
recordingRepeats--;
...
}📝 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.
| bool FSEQPlayer::stopBecauseAtTheEnd() { | |
| if (!recordingFile.available()) { | |
| if (recordingRepeats == RECORDING_REPEAT_LOOP) { | |
| recordingFile.seek(0); | |
| } else if (recordingRepeats > 0) { | |
| recordingFile.seek(0); | |
| recordingRepeats--; | |
| DEBUG_PRINTF("Repeat recording again for: %d\n", recordingRepeats); | |
| } else { | |
| DEBUG_PRINTLN("Finished playing recording, disabling realtime mode"); | |
| realtimeLock(10, REALTIME_MODE_INACTIVE); | |
| recordingFile.close(); | |
| clearLastPlayback(); | |
| return true; | |
| } | |
| } | |
| return false; | |
| } | |
| bool FSEQPlayer::stopBecauseAtTheEnd() { | |
| if (!recordingFile.available()) { | |
| if (recordingRepeats == RECORDING_REPEAT_LOOP) { | |
| recordingFile.seek(0); | |
| } else if (recordingRepeats > 0) { | |
| recordingFile.seek(0); | |
| frame = 0; // Reset the frame to avoid offset misalignment. | |
| recordingRepeats--; | |
| DEBUG_PRINTF("Repeat recording again for: %d\n", recordingRepeats); | |
| } else { | |
| DEBUG_PRINTLN("Finished playing recording, disabling realtime mode"); | |
| realtimeLock(10, REALTIME_MODE_INACTIVE); | |
| recordingFile.close(); | |
| clearLastPlayback(); | |
| return true; | |
| } | |
| } | |
| return false; | |
| } |
| server.on("/fpp", HTTP_POST, | ||
| [this](AsyncWebServerRequest *request) { | ||
| if (uploadStream != nullptr) { | ||
| uploadStream->flush(); | ||
| delete uploadStream; | ||
| uploadStream = nullptr; | ||
| } | ||
| if (currentUploadFile) { | ||
| currentUploadFile.close(); | ||
| } | ||
| currentUploadFileName = ""; | ||
| request->send(200, "text/plain", "Upload complete"); | ||
| }, | ||
| NULL, | ||
| [this](AsyncWebServerRequest *request, uint8_t *data, size_t len, size_t index, size_t total) { | ||
| if (index == 0) { | ||
| DEBUG_PRINTLN("[FPP] Received upload parameters:"); | ||
| for (uint8_t i = 0; i < request->params(); i++) { | ||
| AsyncWebParameter* p = request->getParam(i); | ||
| DEBUG_PRINTF("[FPP] Param %s = %s\n", p->name().c_str(), p->value().c_str()); | ||
| } | ||
| String fileParam = ""; | ||
| if (request->hasParam("filename")) { | ||
| fileParam = request->arg("filename"); | ||
| } | ||
| DEBUG_PRINTF("[FPP] fileParam = %s\n", fileParam.c_str()); | ||
| currentUploadFileName = (fileParam != "") ? (fileParam.startsWith("/") ? fileParam : "/" + fileParam) : "/default.fseq"; | ||
| DEBUG_PRINTF("[FPP] Using filename: %s\n", currentUploadFileName.c_str()); | ||
| if (SD.exists(currentUploadFileName.c_str())) { | ||
| SD.remove(currentUploadFileName.c_str()); | ||
| } | ||
| currentUploadFile = SD.open(currentUploadFileName.c_str(), FILE_WRITE); | ||
| if (!currentUploadFile) { | ||
| DEBUG_PRINTLN(F("[FPP] ERROR: Failed to open file for writing")); | ||
| return; | ||
| } | ||
| uploadStream = new WriteBufferingStream(currentUploadFile, FILE_UPLOAD_BUFFER_SIZE); | ||
| uploadStartTime = millis(); | ||
| } | ||
| if (uploadStream != nullptr) { | ||
| uploadStream->write(data, len); | ||
| } | ||
| if (index + len >= total) { | ||
| if (uploadStream != nullptr) { | ||
| uploadStream->flush(); | ||
| delete uploadStream; | ||
| uploadStream = nullptr; | ||
| } | ||
| currentUploadFile.close(); | ||
| currentUploadFileName = ""; | ||
| } | ||
| } | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add safeguards for large or invalid file uploads.
The /fpp endpoint allows raw file uploads without explicit size or type checks. Malicious or accidental large uploads could degrade performance or fill storage. Consider imposing a limit or validating the file extension.
| #include "wled.h" | ||
| #include "../usermods/FSEQ/fseq_player.h" | ||
| #include "../usermods/FSEQ/fseq_player.cpp" | ||
| #include "../usermods/FSEQ/sd_manager.h" | ||
| #include "../usermods/FSEQ/sd_manager.cpp" | ||
| #include "../usermods/FSEQ/web_ui_manager.h" | ||
| #include "../usermods/FSEQ/web_ui_manager.cpp" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid including .cpp files directly in the header.
Including .cpp files here (e.g., #include "../usermods/FSEQ/fseq_player.cpp") can cause multiple definition errors and complicate linking. It’s generally best to include only the corresponding headers in .h files and have .cpp files compiled separately.
-#include "../usermods/FSEQ/fseq_player.cpp"
-#include "../usermods/FSEQ/sd_manager.cpp"
-#include "../usermods/FSEQ/web_ui_manager.cpp"📝 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.
| #include "wled.h" | |
| #include "../usermods/FSEQ/fseq_player.h" | |
| #include "../usermods/FSEQ/fseq_player.cpp" | |
| #include "../usermods/FSEQ/sd_manager.h" | |
| #include "../usermods/FSEQ/sd_manager.cpp" | |
| #include "../usermods/FSEQ/web_ui_manager.h" | |
| #include "../usermods/FSEQ/web_ui_manager.cpp" | |
| #include "wled.h" | |
| #include "../usermods/FSEQ/fseq_player.h" | |
| #include "../usermods/FSEQ/sd_manager.h" | |
| #include "../usermods/FSEQ/web_ui_manager.h" |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
usermods/FSEQ/web_ui_manager.cpp (1)
89-101:⚠️ Potential issueValidate path to prevent directory traversal risk.
The delete endpoint (/api/sd/delete) does not validate user-supplied paths beyond prefixing "/". Malicious requests might use special filenames or subdirectories. Consider rejecting paths containing “..” or system-reserved paths.+ // Basic check to prevent directory traversal or protected paths + if (path.indexOf("..") != -1 || path.startsWith("/system")) { + request->send(403, "text/plain", "Access denied"); + return; + }
🧹 Nitpick comments (6)
usermods/FSEQ/README.md (3)
7-7: Avoid bare URL usage in Markdown.
In Markdown, it’s often preferred to embed links within brackets for better readability and compliance with linters (MD034).- # SWEB UI http://yourIP/sd/ui + # SWEB UI [http://yourIP/sd/ui](http://yourIP/sd/ui)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
7-7: Bare URL used
null(MD034, no-bare-urls)
73-96: Replace hard tabs with spaces for readability.
Your code snippet from lines 73 to 96 appears to use hard tabs, triggering MD010 linting warnings. Converting them to spaces helps ensure consistent formatting.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
73-73: Hard tabs
Column: 1(MD010, no-hard-tabs)
74-74: Hard tabs
Column: 1(MD010, no-hard-tabs)
75-75: Hard tabs
Column: 1(MD010, no-hard-tabs)
76-76: Hard tabs
Column: 1(MD010, no-hard-tabs)
77-77: Hard tabs
Column: 1(MD010, no-hard-tabs)
78-78: Hard tabs
Column: 1(MD010, no-hard-tabs)
79-79: Hard tabs
Column: 1(MD010, no-hard-tabs)
80-80: Hard tabs
Column: 1(MD010, no-hard-tabs)
81-81: Hard tabs
Column: 1(MD010, no-hard-tabs)
82-82: Hard tabs
Column: 1(MD010, no-hard-tabs)
83-83: Hard tabs
Column: 1(MD010, no-hard-tabs)
84-84: Hard tabs
Column: 1(MD010, no-hard-tabs)
85-85: Hard tabs
Column: 1(MD010, no-hard-tabs)
86-86: Hard tabs
Column: 1(MD010, no-hard-tabs)
87-87: Hard tabs
Column: 1(MD010, no-hard-tabs)
88-88: Hard tabs
Column: 1(MD010, no-hard-tabs)
89-89: Hard tabs
Column: 1(MD010, no-hard-tabs)
90-90: Hard tabs
Column: 1(MD010, no-hard-tabs)
91-91: Hard tabs
Column: 1(MD010, no-hard-tabs)
92-92: Hard tabs
Column: 1(MD010, no-hard-tabs)
93-93: Hard tabs
Column: 1(MD010, no-hard-tabs)
94-94: Hard tabs
Column: 1(MD010, no-hard-tabs)
95-95: Hard tabs
Column: 1(MD010, no-hard-tabs)
96-96: Hard tabs
Column: 1(MD010, no-hard-tabs)
1-118: Consider adding a “Hardware Requirements” section.
To make this README more thorough, it’s recommended to describe the required hardware (SD card modules, wiring, etc.) similar to how other usermods provide details for setup.+ ## Hardware Requirements + + - ESP32 board (tested with <model> ...) + - SD card module (SPI or SDMMC, depending on compile flags) + - Formatted SD card + + ### Example Wiring + | Signal | Pin | + | ------ | ---- | + | CS | GPIO5 (or user-configured) | + | SCK | GPIO18 (or user-configured) | + | MOSI | GPIO23 (or user-configured) | + | MISO | GPIO19 (or user-configured) | + | 3.3V | 3.3V | + | GND | GND |🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
7-7: Bare URL used
null(MD034, no-bare-urls)
73-73: Hard tabs
Column: 1(MD010, no-hard-tabs)
74-74: Hard tabs
Column: 1(MD010, no-hard-tabs)
75-75: Hard tabs
Column: 1(MD010, no-hard-tabs)
76-76: Hard tabs
Column: 1(MD010, no-hard-tabs)
77-77: Hard tabs
Column: 1(MD010, no-hard-tabs)
78-78: Hard tabs
Column: 1(MD010, no-hard-tabs)
79-79: Hard tabs
Column: 1(MD010, no-hard-tabs)
80-80: Hard tabs
Column: 1(MD010, no-hard-tabs)
81-81: Hard tabs
Column: 1(MD010, no-hard-tabs)
82-82: Hard tabs
Column: 1(MD010, no-hard-tabs)
83-83: Hard tabs
Column: 1(MD010, no-hard-tabs)
84-84: Hard tabs
Column: 1(MD010, no-hard-tabs)
85-85: Hard tabs
Column: 1(MD010, no-hard-tabs)
86-86: Hard tabs
Column: 1(MD010, no-hard-tabs)
87-87: Hard tabs
Column: 1(MD010, no-hard-tabs)
88-88: Hard tabs
Column: 1(MD010, no-hard-tabs)
89-89: Hard tabs
Column: 1(MD010, no-hard-tabs)
90-90: Hard tabs
Column: 1(MD010, no-hard-tabs)
91-91: Hard tabs
Column: 1(MD010, no-hard-tabs)
92-92: Hard tabs
Column: 1(MD010, no-hard-tabs)
93-93: Hard tabs
Column: 1(MD010, no-hard-tabs)
94-94: Hard tabs
Column: 1(MD010, no-hard-tabs)
95-95: Hard tabs
Column: 1(MD010, no-hard-tabs)
96-96: Hard tabs
Column: 1(MD010, no-hard-tabs)
usermods/FSEQ/web_ui_manager.cpp (1)
275-289: Enhance error feedback during file upload.
Currently, the upload endpoint returns a simple 200 OK for all outcomes. Consider providing more detailed error responses (e.g., file open failures, partial writes). This will help users diagnose issues when uploading large files.usermods/FSEQ/fseq_player.cpp (2)
91-110: Consider robust loop logic after playback ends.
Repetition logic allows resetting to the start of the file for repeated playback, but check if the file is physically shorter than expected. If the file is damaged or incorrectly truncated, repeated resets could lead to indefinite looping on partial data.
213-233: Clamp large frame offsets insyncPlayback().
You already handle adjustments if the difference is more than 2 frames. Ensure extremely large or negative offsets do not cause unexpected file seeks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
usermods/FSEQ/README.md(1 hunks)usermods/FSEQ/fseq_player.cpp(1 hunks)usermods/FSEQ/usermod_fseq.h(1 hunks)usermods/FSEQ/web_ui_manager.cpp(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
usermods/FSEQ/README.md
7-7: Bare URL used
null
(MD034, no-bare-urls)
73-73: Hard tabs
Column: 1
(MD010, no-hard-tabs)
74-74: Hard tabs
Column: 1
(MD010, no-hard-tabs)
75-75: Hard tabs
Column: 1
(MD010, no-hard-tabs)
76-76: Hard tabs
Column: 1
(MD010, no-hard-tabs)
77-77: Hard tabs
Column: 1
(MD010, no-hard-tabs)
78-78: Hard tabs
Column: 1
(MD010, no-hard-tabs)
79-79: Hard tabs
Column: 1
(MD010, no-hard-tabs)
80-80: Hard tabs
Column: 1
(MD010, no-hard-tabs)
81-81: Hard tabs
Column: 1
(MD010, no-hard-tabs)
82-82: Hard tabs
Column: 1
(MD010, no-hard-tabs)
83-83: Hard tabs
Column: 1
(MD010, no-hard-tabs)
84-84: Hard tabs
Column: 1
(MD010, no-hard-tabs)
85-85: Hard tabs
Column: 1
(MD010, no-hard-tabs)
86-86: Hard tabs
Column: 1
(MD010, no-hard-tabs)
87-87: Hard tabs
Column: 1
(MD010, no-hard-tabs)
88-88: Hard tabs
Column: 1
(MD010, no-hard-tabs)
89-89: Hard tabs
Column: 1
(MD010, no-hard-tabs)
90-90: Hard tabs
Column: 1
(MD010, no-hard-tabs)
91-91: Hard tabs
Column: 1
(MD010, no-hard-tabs)
92-92: Hard tabs
Column: 1
(MD010, no-hard-tabs)
93-93: Hard tabs
Column: 1
(MD010, no-hard-tabs)
94-94: Hard tabs
Column: 1
(MD010, no-hard-tabs)
95-95: Hard tabs
Column: 1
(MD010, no-hard-tabs)
96-96: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (3)
usermods/FSEQ/web_ui_manager.cpp (1)
187-188: Investigate concurrency in status polling.
FSEQ status is polled every 500 ms. If large file operations or slow hardware cause extended delays, repeated polls might queue. Consider verifying concurrency safety withincheckFseqStatus()or using an atomic guard if needed.usermods/FSEQ/fseq_player.cpp (1)
69-88: Check for truncated frame data reads.
The loop reads frames in chunks; if the file is truncated mid-frame, partial decoding may occur. Consider verifying if the file read returns fewer bytes than expected.while (index < lastLed && bytes_remaining > 0) { uint16_t length = min(bytes_remaining, buffer_size); size_t actuallyRead = recordingFile.readBytes(frame_data, length); + if (actuallyRead < length) { + DEBUG_PRINTLN("Warning: Unexpected end of file while reading frame data"); + break; + } bytes_remaining -= actuallyRead; ... }usermods/FSEQ/usermod_fseq.h (1)
43-49: Do not include .cpp files within a header.
This can create multiple definition errors and complicate the build. Each.cppfile should be compiled separately, and only the corresponding header should be included here.
| void reinit_SD_SPI() { | ||
| // Deinit SD if needed | ||
| SD_ADAPTER.end(); | ||
| // Reallocate pins | ||
| PinManager::deallocatePin(configPinSourceSelect, PinOwner::UM_SdCard); | ||
| PinManager::deallocatePin(configPinSourceClock, PinOwner::UM_SdCard); | ||
| PinManager::deallocatePin(configPinPoci, PinOwner::UM_SdCard); | ||
| PinManager::deallocatePin(configPinPico, PinOwner::UM_SdCard); | ||
|
|
||
| PinManagerPinType pins[4] = { | ||
| { configPinSourceSelect, true }, | ||
| { configPinSourceClock, true }, | ||
| { configPinPoci, false }, | ||
| { configPinPico, true } | ||
| }; | ||
| if (!PinManager::allocateMultiplePins(pins, 4, PinOwner::UM_SdCard)) { | ||
| DEBUG_PRINTF("[%s] SPI pin allocation failed!\n", FPSTR(_name)); | ||
| return; | ||
| } | ||
|
|
||
| // Reinit SPI with new pins | ||
| spiPort.begin(configPinSourceClock, configPinPoci, configPinPico, configPinSourceSelect); | ||
|
|
||
| // Try to begin SD again | ||
| if (!SD_ADAPTER.begin(configPinSourceSelect, spiPort)) { | ||
| DEBUG_PRINTF("[%s] SPI begin failed!\n", FPSTR(_name)); | ||
| } else { | ||
| DEBUG_PRINTF("[%s] SD SPI reinitialized with new pins\n", FPSTR(_name)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reinitialize SPI if pin reallocation fails.
If allocateMultiplePins() fails (line 145), the code returns prematurely. It might be beneficial to roll back or attempt a fallback to the previous pin configuration for a more robust user experience.
|
I have tested the code, and it works fine for me. However, it would be great if someone could test it on different boards to ensure compatibility. |
This PR adds a usermod for playing FSEQ files from xLights.
The feature is still in development, and further improvements are planned.
Feedback and testing on different ESP32 boards are welcome!
Summary by CodeRabbit