Pixelforge backport, UI stability improvements, speedup for UDP real-time #293
Conversation
align with tools/cdata.js from upstream WLED --> prerequisite for pixelforge tool !
replaces the pixel magic tool with a much more feature-rich tool for handling gif images. Also adds a scrolling text interface and the possibility to add more tools with a single button click like the classic pixel magic tool and the pixel-painter tool.
* different call for listing images * different call for deleting images * minor cleanups
📝 WalkthroughWalkthroughAdds PixelForge/PixArt UIs and GIF codec, introduces a Node-based frontend build step and tests, PlatformIO middleware and a post-link usermod validator, enables PixelForge/PixArt build flags across many envs, updates npm tooling, and adds ESP32 mutex/spinlock protections across drawing, realtime, and file operations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant PIO as PlatformIO
participant BuildPy as pio-scripts/build_ui.py
participant Node as Node/npm
participant Compiler as C++ Compiler
participant Middleware as set_metadata.py
participant Linker as Linker (.map)
participant Validator as validate_modules.py
participant Server as WLED Server
participant Browser as Client
PIO->>BuildPy: run pre-build hook
BuildPy->>Node: npm ci
Node-->>BuildPy: deps installed
BuildPy->>Node: npm run build (generate html headers)
Node-->>BuildPy: headers produced
BuildPy-->>PIO: place headers into build dir
PIO->>Compiler: compile sources
Compiler->>Middleware: build wled_metadata.cpp
Middleware->>Compiler: inject CPPDEFINES (WLED_REPO/WLED_VERSION)
Compiler->>Linker: link -> ELF + .map
Linker-->>PIO: .map produced
PIO->>Validator: post-link hook
Validator->>Linker: parse .map for usermod objects
alt all usermods present
Validator-->>PIO: success
else missing objects
Validator-->>PIO: abort build
end
Browser->>Server: GET /pixelforge.htm
Server-->>Browser: gzip pixelforge + common.js (cached)
Browser->>Server: upload / websocket (DDP)
Server-->>Browser: responses/events
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (11)
tools/cdata.js (2)
18-24: Inconsistentnode:prefix usage in imports.Line 19 uses
require("path")while lines 18 and 21 use thenode:prefix (node:fs,node:zlib). For consistency and to explicitly signal built-in modules, consider usingnode:path.🔎 Proposed fix
const fs = require("node:fs"); -const path = require("path"); +const path = require("node:path"); const inline = require("web-resource-inliner"); const zlib = require("node:zlib");
216-228: Potential issue: directory mtime is checked before recursing.On line 220,
isFileNewerThan(filePath, time)is called for all entries including directories. Directory mtimes change when files are added/removed in them, which may cause false positives. Additionally, if the intent is only to check file contents, checking directory mtimes may be misleading.🔎 Proposed fix: Check file type before mtime comparison
function isAnyFileInFolderNewerThan(folderPath, time) { const files = fs.readdirSync(folderPath, { withFileTypes: true }); for (const file of files) { const filePath = path.join(folderPath, file.name); - if (isFileNewerThan(filePath, time)) { - return true; - } if (file.isDirectory() && isAnyFileInFolderNewerThan(filePath, time)) { return true; + } else if (file.isFile() && isFileNewerThan(filePath, time)) { + return true; } } return false; }tools/cdata-test.js (2)
120-123: Flaky assertion: 500ms threshold may be too tight.The 500ms window for checking if a file was "newly created" could cause intermittent test failures on slower CI systems or under heavy load. Consider increasing the threshold or using a different approach (e.g., comparing mtimes before and after the operation).
🔎 Proposed fix
- async function checkIfFileWasNewlyCreated(file) { - const modifiedTime = fs.statSync(file).mtimeMs; - assert(Date.now() - modifiedTime < 500, file + ' was not modified'); + async function checkIfFileWasNewlyCreated(file, beforeTime) { + const modifiedTime = fs.statSync(file).mtimeMs; + assert(modifiedTime > beforeTime, file + ' was not modified'); }Then capture
beforeTime = Date.now()before running the script.
5-6: Inconsistent module import style withcdata.js.The test file uses
require('fs')whilecdata.jsusesrequire('node:fs'). For consistency across the codebase, consider aligning the import style.🔎 Proposed fix
-const fs = require('fs'); -const path = require('path'); +const fs = require('node:fs'); +const path = require('node:path');pio-scripts/set_metadata.py (1)
84-85: Consider adding error handling for package.json parsing.If
package.jsonis malformed or missing theversionfield, this will raise an unhandled exception during the build. Consider wrapping this in a try/except with a sensible fallback.🔎 Proposed fix
-with open("package.json", "r") as package: - WLED_VERSION = json.load(package)["version"] +try: + with open("package.json", "r") as package: + WLED_VERSION = json.load(package).get("version", "unknown") +except (FileNotFoundError, json.JSONDecodeError, KeyError) as e: + print(f"Warning: Could not read version from package.json: {e}") + WLED_VERSION = "unknown"pio-scripts/validate_modules.py (1)
47-50: Minor style nit: double space and unnecessary return.Line 50 has a double space before
env.subst, and the explicitreturn Noneon line 76 is unnecessary (Python implicitly returnsNone). Thesourceandtargetparameters are required by the SCons post-action callback signature, so the static analysis warnings about them being unused are false positives.🔎 Proposed fix
def validate_map_file(source, target, env): """ Validate that all modules appear in the output build """ build_dir = Path(env.subst("$BUILD_DIR")) - map_file_path = build_dir / env.subst("${PROGNAME}.map") + map_file_path = build_dir / env.subst("${PROGNAME}.map")And at line 76:
- return None + # SCons post-action doesn't use return valuewled00/data/pixelforge/pixelforge.htm (3)
540-541: TODO: Image loading can fail with 503 when switching tabs.The comment indicates a known issue where switching tabs can cause 503 errors and unloaded images. This is flagged in the PR objectives as "More testing" being a remaining TODO.
Would you like help implementing a retry mechanism with exponential backoff to handle transient 503 errors more gracefully?
10-10: TODO: Sequential loading and code optimization.This TODO notes pending work for sequential loading of scripts and code size optimization (e.g.,
getURL()from common.js). Consider tracking this in a follow-up issue.Do you want me to open an issue to track these optimizations?
1168-1170: Unhandled promise rejection in fetch chain.The
txtUpfunction catches errors but only logs them. Consider providing user feedback via the existingmsg()toast function for consistency with other error handlers in this file.🔎 Proposed fix
fetch(`${wu}/json/state`,{method:'POST',body:JSON.stringify(j)}) .then(r => { if(r.ok) segLoad(); }) - .catch(console.error); + .catch(e => { console.error(e); msg('Failed to update', 'err'); });wled00/data/pxmagic/pxmagic.htm (2)
1418-1424: External API dependency introduces reliability and privacy concerns.The animation frame extraction relies on an external third-party service (
pixelmagictool.vercel.app). This creates:
- A single point of failure if the service is unavailable
- Potential privacy concerns as user images are sent to an external server
- Dependency on external service availability
Consider documenting this dependency or providing a fallback/local implementation.
850-858: Consider a more informative fallback for missing hostname.The fallback to
"0.0.0.0"when no hostname is available may confuse users. Consider using"localhost"or displaying a clearer error state.🔎 Proposed fix
const host = params.get("hn") ? params.get("hn") : window.location.host ? window.location.host - : "0.0.0.0"; + : "localhost";
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
package.json(2 hunks)pio-scripts/build_ui.py(1 hunks)pio-scripts/set_metadata.py(1 hunks)pio-scripts/validate_modules.py(1 hunks)platformio.ini(1 hunks)tools/cdata-test.js(1 hunks)tools/cdata.js(6 hunks)wled00/data/index.htm(1 hunks)wled00/data/pixelforge/omggif.js(1 hunks)wled00/data/pixelforge/pixelforge.htm(1 hunks)wled00/data/pxmagic/pxmagic.htm(1 hunks)wled00/data/settings.htm(1 hunks)wled00/data/style.css(2 hunks)wled00/wled_server.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**/*.{htm,html,css,js}: Use tabs for indentation in web UI files (.htm/.html/.css/.js) under wled00/data
After editing web UI files, run npm run build to regenerate embedded headers
Manually validate web UI changes via local HTTP server and browser
Files:
wled00/data/pxmagic/pxmagic.htmwled00/data/pixelforge/pixelforge.htmwled00/data/pixelforge/omggif.jswled00/data/index.htmwled00/data/style.csswled00/data/settings.htm
wled00/data/**/*.{htm,html,js,css}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Check the browser console for JavaScript errors after web UI changes
Files:
wled00/data/pxmagic/pxmagic.htmwled00/data/pixelforge/pixelforge.htmwled00/data/pixelforge/omggif.jswled00/data/index.htmwled00/data/style.csswled00/data/settings.htm
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/wled_server.cpp
🧠 Learnings (14)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: netmindz
Repo: MoonModules/WLED-MM PR: 248
File: platformio.ini:1613-1613
Timestamp: 2025-07-02T23:22:57.175Z
Learning: In WLED platformio.ini, the particle system disable flags (WLED_DISABLE_PARTICLESYSTEM1D and WLED_DISABLE_PARTICLESYSTEM2D) are intentionally varied across different build environments based on platform memory constraints. More memory-limited platforms (like ESP8266, ESP32-C3, ESP32-S2) disable both 1D and 2D particle systems, while platforms with more available memory (like esp32_4MB_V4_S with 4MB flash) may only disable the 1D version to preserve flash space while keeping 2D functionality available.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/data/**/*.{htm,html,css,js} : After editing web UI files, run npm run build to regenerate embedded headers
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/data/**/*.{htm,html,css,js} : After editing web UI files, run npm run build to regenerate embedded headers
Applied to files:
wled00/data/pxmagic/pxmagic.htmpackage.jsonwled00/data/pixelforge/pixelforge.htmtools/cdata.js
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/data/**/*.{htm,html,css,js} : Manually validate web UI changes via local HTTP server and browser
Applied to files:
wled00/data/pxmagic/pxmagic.htmwled00/data/pixelforge/pixelforge.htmtools/cdata.jswled00/data/index.htmwled00/data/settings.htm
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/data/**/*.{htm,html,js,css} : Check the browser console for JavaScript errors after web UI changes
Applied to files:
wled00/data/pxmagic/pxmagic.htmwled00/data/pixelforge/pixelforge.htmtools/cdata.jswled00/data/index.htm
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/html_*.h : Do not edit generated embedded web header files: wled00/html_*.h
Applied to files:
wled00/data/pixelforge/pixelforge.htmtools/cdata.jswled00/wled_server.cpp
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Always commit both source web files and the generated wled00/html_*.h files
Applied to files:
tools/cdata.js
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/data/**/*.{htm,html,css,js} : Use tabs for indentation in web UI files (.htm/.html/.css/.js) under wled00/data
Applied to files:
tools/cdata.jswled00/data/settings.htm
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Always build the web UI first before any hardware/firmware build
Applied to files:
tools/cdata.jsplatformio.ini
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Always run npm test before finishing work
Applied to files:
tools/cdata-test.js
📚 Learning: 2025-11-02T22:07:15.689Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-11-02T22:17:26.380Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:17:26.380Z
Learning: In wled00/FX.cpp, the `#ifndef WLED_DISABLE_2D` guard around the Akemi 2D fallback in the `mode_oops()` function (lines 142-146) is intentionally kept. This guard is necessary because when users build with `-D WLED_DISABLE_2D` to remove all 2D effects from the build, the Akemi effect is also not compiled, so the guard prevents calling a non-existent function.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-07-02T23:22:57.175Z
Learnt from: netmindz
Repo: MoonModules/WLED-MM PR: 248
File: platformio.ini:1613-1613
Timestamp: 2025-07-02T23:22:57.175Z
Learning: In WLED platformio.ini, the particle system disable flags (WLED_DISABLE_PARTICLESYSTEM1D and WLED_DISABLE_PARTICLESYSTEM2D) are intentionally varied across different build environments based on platform memory constraints. More memory-limited platforms (like ESP8266, ESP32-C3, ESP32-S2) disable both 1D and 2D particle systems, while platforms with more available memory (like esp32_4MB_V4_S with 4MB flash) may only disable the 1D version to preserve flash space while keeping 2D functionality available.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-11-14T13:55:44.442Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-14T13:55:44.442Z
Learning: In MoonModules/WLED-MM, always flag Variable Length Arrays (VLA) during code reviews, especially when they could overflow available stack space (typically 2K-4K on ESP32/ESP8266). VLAs based on dynamic parameters like segment dimensions, particle counts, or pixel buffers should be highlighted and require justification from the author. Prefer fixed-size arrays, heap allocation, or bounded VLAs with explicit maximum size constraints.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-06-30T17:53:37.232Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 248
File: usermods/audioreactive/audio_reactive.h:199-200
Timestamp: 2025-06-30T17:53:37.232Z
Learning: In WLED AudioReactive usermod, when using ArduinoFFT (enabled via UM_AUDIOREACTIVE_USE_ARDUINO_FFT), it's acceptable and necessary to redefine the global sqrt macro to sqrtf within the conditional compilation block for performance optimization. This is a specific requirement for ArduinoFFT optimization that can provide 10-50% performance improvement on ESP32.
Applied to files:
wled00/wled_server.cpp
🧬 Code graph analysis (2)
tools/cdata-test.js (1)
tools/cdata.js (3)
fs(18-18)path(19-19)files(217-217)
wled00/wled_server.cpp (1)
wled00/file.cpp (2)
handleFileRead(513-558)handleFileRead(513-513)
🪛 Biome (2.1.2)
tools/cdata.js
[error] 249-249: Illegal return statement outside of a function
(parse)
[error] 256-256: Illegal return statement outside of a function
(parse)
wled00/data/pixelforge/omggif.js
[error] 438-438: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 464-464: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 465-465: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 466-466: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 467-467: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 468-468: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 469-469: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 470-470: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 471-471: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 472-472: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 473-473: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 474-474: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 475-475: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 483-483: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 512-512: This code is unreachable
(lint/correctness/noUnreachable)
[error] 32-32: Shouldn't redeclare 'gopts'. Consider to delete it or rename it.
'gopts' is defined here:
(lint/suspicious/noRedeclare)
[error] 32-32: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 32-32: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 448-448: Shouldn't redeclare 'block_size'. Consider to delete it or rename it.
'block_size' is defined here:
(lint/suspicious/noRedeclare)
[error] 477-477: Shouldn't redeclare 'has_local_palette'. Consider to delete it or rename it.
'has_local_palette' is defined here:
(lint/suspicious/noRedeclare)
[error] 487-487: Shouldn't redeclare 'block_size'. Consider to delete it or rename it.
'block_size' is defined here:
(lint/suspicious/noRedeclare)
🪛 Ruff (0.14.8)
pio-scripts/build_ui.py
1-1: Undefined name Import
(F821)
8-8: Undefined name env
(F821)
13-13: Undefined name env
(F821)
16-16: Undefined name env
(F821)
pio-scripts/set_metadata.py
1-1: Undefined name Import
(F821)
23-23: Starting a process with a partial executable path
(S607)
28-28: subprocess call: check for execution of untrusted input
(S603)
28-28: Starting a process with a partial executable path
(S607)
40-40: subprocess call: check for execution of untrusted input
(S603)
40-40: Starting a process with a partial executable path
(S607)
67-67: Consider moving this statement to an else block
(TRY300)
75-75: Do not catch blind exception: Exception
(BLE001)
113-113: Undefined name env
(F821)
pio-scripts/validate_modules.py
47-47: Unused function argument: source
(ARG001)
47-47: Unused function argument: target
(ARG001)
78-78: Undefined name Import
(F821)
79-79: Undefined name env
(F821)
79-79: Undefined name env
(F821)
80-80: Undefined name env
(F821)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Builds (esp8266pro_16MB_M)
- GitHub Check: Builds (esp32c3dev_4MB_M)
- GitHub Check: Builds (adafruit_matrixportal_esp32s3)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: Builds (esp8266_4MB_M)
- GitHub Check: Builds (esp32S3_WROOM-2_M)
- GitHub Check: Builds (seeed_esp32c3_4MB_S)
- GitHub Check: Builds (esp32_4MB_M_eth)
- GitHub Check: Builds (esp32S3_8MB_PSRAM_M)
- GitHub Check: Builds (esp32s2_PSRAM_M)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_M)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp32_16MB_V4_S)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
- GitHub Check: Builds (esp32_pico_4MB_V4_S)
- GitHub Check: Builds (esp32_4MB_V4_S)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_16MB_V4_M_debug)
- GitHub Check: Builds (esp32dev_compat)
🔇 Additional comments (15)
platformio.ini (1)
223-223: LGTM!The pre-build script replacement from
build-html.pytobuild_ui.pyaligns with the PR's objective to upgrade the HTML build system. The new naming convention is consistent with Python standards.pio-scripts/set_metadata.py (1)
1-4: LGTM - PlatformIO-specific constructs are valid.The static analysis warnings about
Importandenvbeing undefined are false positives. These are PlatformIO/SCons-specific constructs that are injected at runtime. The subprocess calls use hardcodedgitcommands with no user input, so the security warnings are also not applicable here.package.json (1)
26-33: Dependency updates are appropriate for the build system upgrade.The migration from
inlinertoweb-resource-inlinerand removal ofzlib(in favor of built-innode:zlib) are sensible changes. The Node.js >=20.0.0 engine requirement aligns with the built-in test runner usage. All specified dependency versions are secure: web-resource-inliner ^7.0.0 has no known vulnerabilities, html-minifier-terser ^7.2.0 has no direct vulnerabilities, clean-css ^5.3.3 is safe to use, and nodemon ^3.1.9 shows no reported security vulnerabilities. Consider updating nodemon to 3.1.10 or later for the latest improvements.pio-scripts/validate_modules.py (1)
78-80: Build integration uses standard SCons/PlatformIO idioms.The
Import("env")is the standard SCons idiom to import the build environment, andenv.Append/env.AddPostActioncorrectly wire up the validation. The Ruff warnings about undefined names are false positives in this PlatformIO context.wled00/data/pixelforge/pixelforge.htm (2)
436-445: Verify the external tool download URL.The initialization constructs
wufrom a query parameter or falls back towindow.location.host, which is the expected pattern for cross-origin tooling. However, line 589 hardcodes an external URL (https://dedehai.github.io/) for downloading additional tools. Please verify this is the intended and trusted source.
324-325: Scrolling Text tab marked as not yet supported in WLED-MM.The comments indicate the Scrolling Text functionality (lines 324-392) is not yet supported in WLED-MM, which aligns with the PR objective "Make scrolling text tab work" being listed as a remaining TODO.
wled00/data/pixelforge/omggif.js (2)
1-28: Third-party library (omggif) vendored with MIT license.This is a well-established GIF encoder/decoder library. The static analysis warnings (variable redeclarations, switch declarations, unreachable code) are characteristic of pre-ES6 JavaScript patterns and are not worth modifying in vendored code.
807-809: **GifReader export is commented out.**The original omggif library exports bothGifWriterandGifReader, but here onlyGifWriteris exported. Sincepixelforge.htmusesGifReaderdirectly (line 639), this works because both classes are in the global scope when loaded via<script>tag. The commented-out line appears intentional for this browser-based usage.wled00/data/style.css (1)
36-48: Button styling improvements look good.The added transition and hover effects improve the user experience with visual feedback. As per coding guidelines, remember to run
npm run buildto regenerate embedded headers and manually validate the changes via local HTTP server.wled00/wled_server.cpp (2)
12-14: LGTM!The conditional include for PixelForge follows the same pattern as the existing PIXART feature flag, maintaining consistency in the codebase.
454-465: LGTM!The PixelForge route handler correctly follows the established pattern used by other static pages (pixart.htm, cpal.htm, simple.htm):
- First attempts to serve from filesystem via
handleFileRead- Checks ETag cache headers for 304 responses
- Falls back to embedded gzipped content with proper cache headers
The use of
static const charin PROGMEM for the path string is appropriate for ESP memory optimization.wled00/data/settings.htm (2)
71-71: LGTM!The
@importdirective correctly references the external stylesheet, improving maintainability by centralizing common styles. This aligns with the broader refactoring to move shared styles tostyle.css.
79-87: LGTM!The button styling simplification to
display: block; padding: 0;with other visual properties moved to the external stylesheet is a clean separation of concerns.wled00/data/index.htm (1)
207-209: LGTM!The PixelForge navigation button follows the established pattern for custom palette navigation, correctly handling both local file protocol and HTTP access via the
locvariable. The addedtitleattributes on all three buttons improve accessibility.wled00/data/pxmagic/pxmagic.htm (1)
10-565: CSS and HTML structure looks well-organized.The embedded CSS uses CSS variables for theming, includes responsive breakpoints, and follows a consistent naming convention. The HTML form structure has proper validation attributes.
| if node_ex is None: | ||
| print('\x1b[0;31;43m' + 'Node.js is not installed or missing from PATH html css js will not be processed check https://kno.wled.ge/advanced/compiling-wled/' + '\x1b[0m') | ||
| exitCode = env.Execute("null") | ||
| exit(exitCode) |
There was a problem hiding this comment.
Problematic "null" command execution.
Executing "null" as a shell command is incorrect—it will fail on most systems (the command doesn't exist). The intent appears to be aborting the build with a non-zero exit code. Consider using Exit(1) from SCons directly or exit(1).
🔎 Proposed fix
+from SCons.Script import Exit
+
node_ex = shutil.which("node")
# Check if Node.js is installed and present in PATH if it failed, abort the build
if node_ex is None:
print('\x1b[0;31;43m' + 'Node.js is not installed or missing from PATH html css js will not be processed check https://kno.wled.ge/advanced/compiling-wled/' + '\x1b[0m')
- exitCode = env.Execute("null")
- exit(exitCode)
+ Exit(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.
| if node_ex is None: | |
| print('\x1b[0;31;43m' + 'Node.js is not installed or missing from PATH html css js will not be processed check https://kno.wled.ge/advanced/compiling-wled/' + '\x1b[0m') | |
| exitCode = env.Execute("null") | |
| exit(exitCode) | |
| from SCons.Script import Exit | |
| node_ex = shutil.which("node") | |
| # Check if Node.js is installed and present in PATH if it failed, abort the build | |
| if node_ex is None: | |
| print('\x1b[0;31;43m' + 'Node.js is not installed or missing from PATH html css js will not be processed check https://kno.wled.ge/advanced/compiling-wled/' + '\x1b[0m') | |
| Exit(1) |
🧰 Tools
🪛 Ruff (0.14.8)
8-8: Undefined name env
(F821)
🤖 Prompt for AI Agents
In pio-scripts/build_ui.py around lines 6 to 9, remove the incorrect
env.Execute("null") call and stop using a non-existent "null" shell command;
instead abort with a proper non-zero exit: call SCons' Exit(1) (from
SCons.Script) if the script is running under SCons, or use sys.exit(1) / exit(1)
to terminate the process with status 1, and keep the existing error print before
that.
| # Install the necessary node packages for the pre-build asset bundling script | ||
| print('\x1b[6;33;42m' + 'Installing node packages' + '\x1b[0m') | ||
| env.Execute("npm ci") |
There was a problem hiding this comment.
Missing error check on npm ci.
The return value of npm ci is not checked. If dependency installation fails, the build will continue and likely fail later with a confusing error.
🔎 Proposed fix
# Install the necessary node packages for the pre-build asset bundling script
print('\x1b[6;33;42m' + 'Installing node packages' + '\x1b[0m')
- env.Execute("npm ci")
+ exitCode = env.Execute("npm ci")
+ if exitCode:
+ print('\x1b[0;31;43m' + 'npm ci failed check https://kno.wled.ge/advanced/compiling-wled/' + '\x1b[0m')
+ exit(exitCode)🧰 Tools
🪛 Ruff (0.14.8)
13-13: Undefined name env
(F821)
🤖 Prompt for AI Agents
In pio-scripts/build_ui.py around lines 11-13, the script calls env.Execute("npm
ci") but doesn’t check its return code; capture the return value (rc =
env.Execute("npm ci")) and if rc is non-zero, print/log an error with context
and exit with a non-zero status (e.g., sys.exit(1) or raise RuntimeError) so the
build stops immediately when npm ci fails.
| // Don't run this script if we're in a test environment | ||
| if (process.env.NODE_ENV === 'test') { | ||
| return; | ||
| } | ||
|
|
||
| console.info(wledBanner); | ||
|
|
||
| if (isAlreadyBuilt("wled00/data") && process.argv[2] !== '--force' && process.argv[2] !== '-f') { | ||
| console.info("Web UI is already built"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Illegal return statements at module level.
The static analysis correctly identifies that return statements at lines 249 and 256 are outside any function, which is technically invalid in strict CommonJS modules and may cause issues in certain Node.js configurations or when bundling.
🔎 Proposed fix: Use process.exit() or conditional execution
// Don't run this script if we're in a test environment
if (process.env.NODE_ENV === 'test') {
- return;
+ // Skip execution in test environment
+} else {
+ console.info(wledBanner);
+
+ if (isAlreadyBuilt("wled00/data") && process.argv[2] !== '--force' && process.argv[2] !== '-f') {
+ console.info("Web UI is already built");
+ } else {
+ writeHtmlGzipped("wled00/data/index.htm", "wled00/html_ui.h", 'index', false);
+ // ... rest of build invocations
+ }
}
-
-console.info(wledBanner);
-
-if (isAlreadyBuilt("wled00/data") && process.argv[2] !== '--force' && process.argv[2] !== '-f') {
- console.info("Web UI is already built");
- return;
-}Alternatively, wrap the main logic in an async IIFE:
+async function main() {
// Don't run this script if we're in a test environment
if (process.env.NODE_ENV === 'test') {
return;
}
console.info(wledBanner);
if (isAlreadyBuilt("wled00/data") && process.argv[2] !== '--force' && process.argv[2] !== '-f') {
console.info("Web UI is already built");
return;
}
// ... build invocations
+}
+
+main().catch(console.error);Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (2.1.2)
[error] 249-249: Illegal return statement outside of a function
(parse)
[error] 256-256: Illegal return statement outside of a function
(parse)
🤖 Prompt for AI Agents
In tools/cdata.js around lines 247 to 257, the code contains module-level return
statements which are illegal in CommonJS modules; replace those returns with
proper early-exit behavior: either wrap the script's main logic in a
function/async IIFE and use return inside that function, or keep top-level flow
and call process.exit(0) for normal early exits (and process.exit(1) on fatal
errors). Update both occurrences (the NODE_ENV test and the "already built"
check) accordingly and ensure any cleanup or logging happens before calling
process.exit.
|
@DedeHai if you find some time, it would be cool if you could check that I've found all the "pitfalls" that need adjustments for 0.14.x and 0.15.x APIs. Edit: how about adding an extra download link for your new VideoLab tool ? WLED-MM supports DDP over webSockets, so i guess it should work. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tools/cdata.js (2)
142-165: Callback-basedinline.htmlnot properly integrated with async/await.The
writeHtmlGzippedfunction is declaredasyncbut uses a callback-based API (inline.html) without wrapping it in a Promise. This means the function returns immediately before the callback completes, which can cause race conditions when multiple files are processed concurrently.
247-257: Illegalreturnstatements at module level.The static analysis correctly identifies that
returnstatements at lines 249 and 256 are outside any function. While Node.js CommonJS modules technically allow this (the module is wrapped in a function internally), it's non-standard and may cause issues when bundling or in strict ESM contexts.
🧹 Nitpick comments (1)
wled00/wled_server.cpp (1)
454-471: Route handlers follow established patterns.The PixelForge route handlers correctly mirror existing patterns for serving static HTML and JavaScript content with proper caching and gzip encoding.
Optional: Remove commented-out code
Line 457 contains a commented-out alternative implementation referencing a non-existent
handleStaticContentfunction. Since the current implementation is working and follows the established pattern, consider removing this dead code for cleaner maintainability.server.on(_pixelforge_htm, HTTP_GET, [](AsyncWebServerRequest *request) { - //handleStaticContent(request, FPSTR(_pixelforge_htm), 200, FPSTR(CONTENT_TYPE_HTML), PAGE_pixelforge, PAGE_pixelforge_length); if (handleFileRead(request, FPSTR(_pixelforge_htm))) return;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tools/cdata.js(6 hunks)wled00/data/common.js(1 hunks)wled00/wled_server.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/wled_server.cpp
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**/*.{htm,html,css,js}: Use tabs for indentation in web UI files (.htm/.html/.css/.js) under wled00/data
After editing web UI files, run npm run build to regenerate embedded headers
Manually validate web UI changes via local HTTP server and browser
Files:
wled00/data/common.js
wled00/data/**/*.{htm,html,js,css}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Check the browser console for JavaScript errors after web UI changes
Files:
wled00/data/common.js
🧠 Learnings (14)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/html_*.h : Do not edit generated embedded web header files: wled00/html_*.h
Applied to files:
wled00/wled_server.cpptools/cdata.js
📚 Learning: 2025-07-02T23:22:57.175Z
Learnt from: netmindz
Repo: MoonModules/WLED-MM PR: 248
File: platformio.ini:1613-1613
Timestamp: 2025-07-02T23:22:57.175Z
Learning: In WLED platformio.ini, the particle system disable flags (WLED_DISABLE_PARTICLESYSTEM1D and WLED_DISABLE_PARTICLESYSTEM2D) are intentionally varied across different build environments based on platform memory constraints. More memory-limited platforms (like ESP8266, ESP32-C3, ESP32-S2) disable both 1D and 2D particle systems, while platforms with more available memory (like esp32_4MB_V4_S with 4MB flash) may only disable the 1D version to preserve flash space while keeping 2D functionality available.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-11-02T22:17:26.380Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:17:26.380Z
Learning: In wled00/FX.cpp, the `#ifndef WLED_DISABLE_2D` guard around the Akemi 2D fallback in the `mode_oops()` function (lines 142-146) is intentionally kept. This guard is necessary because when users build with `-D WLED_DISABLE_2D` to remove all 2D effects from the build, the Akemi effect is also not compiled, so the guard prevents calling a non-existent function.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-11-02T22:07:15.689Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-11-14T13:55:44.442Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-14T13:55:44.442Z
Learning: In MoonModules/WLED-MM, always flag Variable Length Arrays (VLA) during code reviews, especially when they could overflow available stack space (typically 2K-4K on ESP32/ESP8266). VLAs based on dynamic parameters like segment dimensions, particle counts, or pixel buffers should be highlighted and require justification from the author. Prefer fixed-size arrays, heap allocation, or bounded VLAs with explicit maximum size constraints.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-06-30T17:53:37.232Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 248
File: usermods/audioreactive/audio_reactive.h:199-200
Timestamp: 2025-06-30T17:53:37.232Z
Learning: In WLED AudioReactive usermod, when using ArduinoFFT (enabled via UM_AUDIOREACTIVE_USE_ARDUINO_FFT), it's acceptable and necessary to redefine the global sqrt macro to sqrtf within the conditional compilation block for performance optimization. This is a specific requirement for ArduinoFFT optimization that can provide 10-50% performance improvement on ESP32.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/data/**/*.{htm,html,js,css} : Check the browser console for JavaScript errors after web UI changes
Applied to files:
wled00/data/common.jstools/cdata.js
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/data/**/*.{htm,html,css,js} : After editing web UI files, run npm run build to regenerate embedded headers
Applied to files:
tools/cdata.js
📚 Learning: 2025-10-22T21:59:24.331Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Applied to files:
tools/cdata.js
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/data/**/*.{htm,html,css,js} : Manually validate web UI changes via local HTTP server and browser
Applied to files:
tools/cdata.js
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Always commit both source web files and the generated wled00/html_*.h files
Applied to files:
tools/cdata.js
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/data/**/*.{htm,html,css,js} : Use tabs for indentation in web UI files (.htm/.html/.css/.js) under wled00/data
Applied to files:
tools/cdata.js
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Always build the web UI first before any hardware/firmware build
Applied to files:
tools/cdata.js
🧬 Code graph analysis (1)
wled00/wled_server.cpp (1)
wled00/file.cpp (2)
handleFileRead(513-558)handleFileRead(513-513)
🪛 Biome (2.1.2)
tools/cdata.js
[error] 249-249: Illegal return statement outside of a function
(parse)
[error] 256-256: Illegal return statement outside of a function
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Builds (esp32dev_compat)
- GitHub Check: Builds (esp32_pico_4MB_V4_S)
- GitHub Check: Builds (esp32_4MB_M_eth)
- GitHub Check: Builds (esp32c3dev_4MB_M)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_M)
- GitHub Check: Builds (esp32S3_WROOM-2_M)
- GitHub Check: Builds (esp8266_4MB_M)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: Builds (esp32_16MB_V4_M_debug)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp8266_2MB_S)
- GitHub Check: Builds (esp8266pro_16MB_M)
- GitHub Check: Builds (esp32_4MB_M_eth)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
- GitHub Check: Builds (esp32_4MB_V4_S)
- GitHub Check: Builds (esp32_pico_4MB_V4_S)
- GitHub Check: Builds (esp32_16MB_V4_S)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
- GitHub Check: Builds (esp32dev_compat)
🔇 Additional comments (18)
wled00/wled_server.cpp (1)
12-14: LGTM - Consistent conditional include pattern.The conditional include follows the same pattern as PixArt and other optional features, properly guarding the PixelForge headers.
tools/cdata.js (9)
18-27: LGTM!The imports are properly structured using Node.js built-in module prefixes (
node:fs,node:zlib). The module exports forisFileNewerThanandisAnyFileInFolderNewerThanare correctly hoisted since they reference function declarations defined later in the file.
42-61: LGTM!The
generateBuildTimefunction andsingleHeaderconstant provide a clean cache-busting mechanism. The#ifdef/#undef/#definepattern ensures the build timestamp is always fresh, andPROGMEMis correctly used for embedded targets.
98-112: LGTM!The
adoptVersionAndRepofunction correctly handles URL replacement for the WLED-MM fork while preserving upstream credits. ThereplaceAllusage is compatible with the Node 20+ requirement.
167-207: LGTM!The
specToChunkandwriteChunksfunctions are properly implemented with async/await. The sequential processing inwriteChunksis appropriate, and error handling for unknown methods is correctly implemented.
209-228: LGTM with minor note.The helper functions are well-structured for build freshness checks. Note that
isFileNewerThanwill throw if a file is deleted betweenreaddirSyncandstatSynccalls, but this race condition is unlikely during normal build operations.
230-245: LGTM!The
isAlreadyBuiltfunction correctly implements build freshness detection by:
- Finding the oldest output file timestamp
- Checking if any source file,
package.json, or the script itself is newer- Gracefully handling missing output files by triggering a rebuild
Including
__filenamein the check ensures that script changes trigger a rebuild, which is a good practice.
259-278: PixelForge build configuration added correctly.The new
pixelforge.htmbuild entry at line 263 correctly disables CSS inlining (inlineCss = false), which aligns with the PR objectives. The migration ofedit.htmfromwriteHtmlGzippedtowriteChunksis consistent with the build refactoring.
307-312: LGTM!The new
common.jschunk configuration is correctly added with gzip compression and JS minification, consistent with the build system upgrade.
480-490: LGTM!The change to gzip with
js-minifyforiro.jsandrangetouch.jsis a good optimization that will reduce the embedded size of these JavaScript libraries.wled00/data/common.js (8)
10-15: Type guard implementations look correct.The implementations properly handle their respective type checks. Note that
isE(o)expects an object argument and will throw on null/undefined, but this is acceptable given the name implies "isEmpty" for objects.
17-53: Tooltip implementation is functional with minor edge-case caveat.The tooltip logic correctly positions and displays tooltips. The cleanup at lines 45-48 removes all
.tooltipelements rather than tracking the specific tooltip for this element. In typical usage this is fine, but if multiple tooltips could trigger simultaneously, there's potential interference.
106-129: Location and URL helpers handle multiple hosting scenarios correctly.The
getLoc()function properly detects file mode, reverse proxy configurations, and normal hosting. The use ofprompt()at line 112 is acceptable for file mode initial setup, though it blocks the UI. The path manipulation logic correctly handles trailing slashes and nested paths.
130-139: Toast notification implementation is correct.The function properly manages toast display with timeout cleanup and graceful handling of missing DOM elements. The global
timeoutvariable ensures only one toast shows at a time.
140-150: File upload implementation is correct.The function properly constructs FormData, sends via XMLHttpRequest, handles success/error cases, and clears the file input afterward. The return value of
falsesuggests this is designed to be used as an event handler.
151-168: WebSocket connection management is well-implemented.The function elegantly reuses existing WebSocket connections when available and creates new ones when needed. The try-catch at line 154 properly handles cross-origin access restrictions. Setting
binaryTypeto"arraybuffer"at line 164 is essential for the binary DDP protocol used later.
170-210: DDP protocol implementation is correct and well-structured.The function properly implements the DDP (Distributed Display Protocol) framing with:
- Correct packet structure (WLED indicator + 10-byte DDP header + RGB data)
- Proper frame size limits (472 RGB pixels = 1416 bytes data + 10 DDP + 1 WLED + 1 WebSocket = 1428 total)
- Chunking logic with accurate offset calculations
- Push flag set only on the final packet to trigger rendering
- Robust error handling with try-catch
The commented ESP8266 support at line 178 could be helpful if re-enabled in the future.
87-105: GetV() is called without being defined in common.js, but this may be dead code.Line 97 calls
GetV()which is not defined in this file. WhileGetV()is defined as a placeholder in update.htm (injected with values), each settings page defines its own localloadJS()function that shadows this implementation and does not callGetV(). The enhancedloadJS()version in common.js (with preGetV/postGetV parameters) appears to be unused because pages define their own local versions that override it. Verify whether the common.jsloadJS()implementation is actually called in any context—if not, this is dead code; if it is called, ensure GetV() is available globally in those contexts.
| function gId(c) { return d.getElementById(c); } // getElementById | ||
| function cE(e) { return d.createElement(e); } // createElement | ||
| function gEBCN(c) { return d.getElementsByClassName(c); } // getElementsByClassName | ||
| function gN(s) { return d.getElementsByName(s)[0]; } // getElementsByName |
There was a problem hiding this comment.
Add null safety to gN() function.
The function accesses index [0] without checking if any elements exist. If getElementsByName(s) returns an empty NodeList, this will return undefined and cause TypeErrors when callers expect an element.
🔎 Proposed fix
-function gN(s) { return d.getElementsByName(s)[0]; } // getElementsByName
+function gN(s) { const els = d.getElementsByName(s); return els.length > 0 ? els[0] : null; } // getElementsByNameAlternatively, if callers always expect an element to exist, document this assumption with a comment or add a runtime assertion.
📝 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.
| function gN(s) { return d.getElementsByName(s)[0]; } // getElementsByName | |
| function gN(s) { const els = d.getElementsByName(s); return els.length > 0 ? els[0] : null; } // getElementsByName |
🤖 Prompt for AI Agents
In wled00/data/common.js around line 9, gN(s) currently returns
d.getElementsByName(s)[0] without checking for empty results; update it to
handle the case where getElementsByName returns no elements by checking the
NodeList length and returning null (or throwing/asserting if callers require a
guaranteed element). Replace the direct indexing with a safe conditional that
returns the first element when present and a null/undefined (or explicit error)
when not, and if you choose null ensure callers are updated or documented to
handle the null case.
|
I will take a look. let me know if you have found any other issues (the rabbit does not seem too happy, did not read it yet though) |
@DedeHai seems that DDP over webSockets - at least in WLED-MM - still needs some robustness improvements. |
this got lost while backporting upstream code
What are the specific test scenarios? For 0.16 this works pretty well, tested on C3 without an FPS limit and it works flawlessly. |
* protect pixel buffer changes with critical section * protect segment list modification with critical section * make _isServicing and _triggered atomic (move out of packed bitarray) * allow custom timeout for waitUntilIdle - DDP needs a longer timeout * protect strip.show() and effect drawing functions with mutex, to prevent crashes
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/e131.cpp (1)
15-38: Tighten DDP semaphore handling and clean up disabled sequence logicTwo points here:
Unused
lastPushSeqwith disabled filter
lastPushSeq(Line 16) is now only referenced inside the#if 0block (Lines 19–30). As written, it's unused in the compiled code and will typically warn.Easiest fix is to wrap its declaration in the same
#if 0or remove both together until the "fixme" logic is revisited.busDrawMux failure path reintroduces race + mis-give semantics
In
handleDDPPacketyou now do:#if defined(ARDUINO_ARCH_ESP32) if (xSemaphoreTake(busDrawMux, 200) != pdTRUE) { delay(1); } #endif for (uint16_t i = start; i < stop; i++) { setRealtimePixel(...); pixels++; } packets++; #if defined(ARDUINO_ARCH_ESP32) xSemaphoreGive(busDrawMux); #endifIf
xSemaphoreTaketimes out, you still write pixels without synchronization and callxSemaphoreGivewithout having taken the semaphore—the latter pushes a binary semaphore into an inconsistent state. Skip the frame instead when the lock cannot be acquired in time, and onlyGivewhenTakesucceeded:#if defined(ARDUINO_ARCH_ESP32)
- if (xSemaphoreTake(busDrawMux, 200) != pdTRUE) { delay(1);}
- BaseType_t gotLock = xSemaphoreTake(busDrawMux, 200);
- if (gotLock != pdTRUE) {
- delay(1);
- return;
- }
#endif
for (uint16_t i = start; i < stop; i++) {
setRealtimePixel(...);
pixels++;
}
packets++;
#if defined(ARDUINO_ARCH_ESP32)
xSemaphoreGive(busDrawMux);
#endif
🧹 Nitpick comments (4)
wled00/FX_fcn.cpp (3)
1916-1932: Clarify and documentwaitUntilIdle(timeout)semanticsThe new
WS2812FX::waitUntilIdle(unsigned timeout)implementation:
- Forces
timeoutto be at leastMAX_IDLE_WAIT_MS(currently 120ms).- Spins until
_isServicingclears or timeout expires.This is fine for the current call sites (
waitUntilIdle(350)and the helper wrapper), but it means:
- Callers cannot request a “quick check” with a very small timeout; they always pay at least ~120ms when a service() is in progress.
strip_wait_until_idle()(no-arg) will also wait at leastMAX_IDLE_WAIT_MSwhen a frame is in flight.Consider adding a short comment in the header where
waitUntilIdleis declared to document that behavior, and confirm all call sites are comfortable with this minimum wait rather than a custom per‑call value.
59-68:strip_wait_until_idlewrapper relies on default argument and uses dynamicStringTwo minor points about
strip_wait_until_idle(String whoCalledMe):
- It calls
strip.waitUntilIdle();without passing a timeout; this assumes the declaration inFX.hprovides a default argument. If you ever remove the default in the header, this will silently diverge. It’s slightly safer to pass an explicit timeout (e.g.MAX_IDLE_WAIT_MS) to keep call sites self‑documenting.- The
Stringparameter causes a heap allocation every time this helper is used. Given it’s used for debug output only, switching toconst char*(orconst __FlashStringHelper*) would avoid dynamic allocation and fragmentation on constrained MCUs.Example:
-void strip_wait_until_idle(String whoCalledMe) { +void strip_wait_until_idle(const char* whoCalledMe) { ... - DEBUG_PRINTLN(whoCalledMe + String(": strip is still drawing effects.")); - strip.waitUntilIdle(); + DEBUG_PRINT(whoCalledMe); + DEBUG_PRINTLN(F(": strip is still drawing effects.")); + strip.waitUntilIdle(MAX_IDLE_WAIT_MS);
1998-2013: Consider defensive coding for mutex acquire/release semanticsThe mutex guarding in
service()andshow()correctly prevents concurrent drawing on ESP32. The current approach is functionally safe: whenxSemaphoreTakefails, the control flow (continueorreturn) preventsxSemaphoreGivefrom being called, which is correct.However, making the success of the
Takeexplicit would improve clarity and future-proof against refactors:#if defined(ARDUINO_ARCH_ESP32) - if (xSemaphoreTake(busDrawMux, 200) != pdTRUE) { delay(1); continue;} + BaseType_t gotLock = xSemaphoreTake(busDrawMux, 200); + if (gotLock != pdTRUE) { delay(1); continue; } #endif ... #if defined(ARDUINO_ARCH_ESP32) - xSemaphoreGive(busDrawMux); + if (gotLock == pdTRUE) xSemaphoreGive(busDrawMux); #endifApply the same pattern in
show(). Additionally, consider adding a debug log when the 200ms timeout occurs—this would surface stalls during stress testing.wled00/FX.h (1)
1041-1041: Remove commented-out code.The commented-out
setPixelSegmentdeclaration should be removed entirely rather than left as a comment, as it clutters the API surface and creates confusion about its availability.🔎 Suggested cleanup
__attribute__((pure)) getActiveSegsLightCapabilities(bool selectedOnly = false); - //setPixelSegment(uint8_t n);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
wled00/FX.h(3 hunks)wled00/FX_fcn.cpp(10 hunks)wled00/e131.cpp(3 hunks)wled00/udp.cpp(1 hunks)wled00/wled.cpp(1 hunks)wled00/wled.h(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/wled.hwled00/wled.cppwled00/udp.cppwled00/e131.cppwled00/FX_fcn.cppwled00/FX.h
🧠 Learnings (16)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/html_*.h : Do not edit generated embedded web header files: wled00/html_*.h
Applied to files:
wled00/wled.h
📚 Learning: 2025-07-02T23:22:57.175Z
Learnt from: netmindz
Repo: MoonModules/WLED-MM PR: 248
File: platformio.ini:1613-1613
Timestamp: 2025-07-02T23:22:57.175Z
Learning: In WLED platformio.ini, the particle system disable flags (WLED_DISABLE_PARTICLESYSTEM1D and WLED_DISABLE_PARTICLESYSTEM2D) are intentionally varied across different build environments based on platform memory constraints. More memory-limited platforms (like ESP8266, ESP32-C3, ESP32-S2) disable both 1D and 2D particle systems, while platforms with more available memory (like esp32_4MB_V4_S with 4MB flash) may only disable the 1D version to preserve flash space while keeping 2D functionality available.
Applied to files:
wled00/wled.hwled00/wled.cppwled00/e131.cpp
📚 Learning: 2025-11-02T22:07:15.689Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Applied to files:
wled00/wled.hwled00/wled.cppwled00/e131.cppwled00/FX_fcn.cppwled00/FX.h
📚 Learning: 2025-10-22T21:50:25.839Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Applied to files:
wled00/wled.hwled00/wled.cppwled00/e131.cppwled00/FX_fcn.cppwled00/FX.h
📚 Learning: 2025-11-14T13:55:44.442Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-14T13:55:44.442Z
Learning: In MoonModules/WLED-MM, always flag Variable Length Arrays (VLA) during code reviews, especially when they could overflow available stack space (typically 2K-4K on ESP32/ESP8266). VLAs based on dynamic parameters like segment dimensions, particle counts, or pixel buffers should be highlighted and require justification from the author. Prefer fixed-size arrays, heap allocation, or bounded VLAs with explicit maximum size constraints.
Applied to files:
wled00/wled.hwled00/wled.cppwled00/e131.cppwled00/FX_fcn.cppwled00/FX.h
📚 Learning: 2025-06-30T17:53:37.232Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 248
File: usermods/audioreactive/audio_reactive.h:199-200
Timestamp: 2025-06-30T17:53:37.232Z
Learning: In WLED AudioReactive usermod, when using ArduinoFFT (enabled via UM_AUDIOREACTIVE_USE_ARDUINO_FFT), it's acceptable and necessary to redefine the global sqrt macro to sqrtf within the conditional compilation block for performance optimization. This is a specific requirement for ArduinoFFT optimization that can provide 10-50% performance improvement on ESP32.
Applied to files:
wled00/wled.hwled00/wled.cppwled00/FX_fcn.cpp
📚 Learning: 2025-11-02T22:17:26.380Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:17:26.380Z
Learning: In wled00/FX.cpp, the `#ifndef WLED_DISABLE_2D` guard around the Akemi 2D fallback in the `mode_oops()` function (lines 142-146) is intentionally kept. This guard is necessary because when users build with `-D WLED_DISABLE_2D` to remove all 2D effects from the build, the Akemi effect is also not compiled, so the guard prevents calling a non-existent function.
Applied to files:
wled00/wled.hwled00/e131.cppwled00/FX_fcn.cppwled00/FX.h
📚 Learning: 2025-11-24T00:15:38.069Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-24T00:15:38.069Z
Learning: In MoonModules/WLED-MM, when reading ESP32/ESP32-S3 eFuse blocks using esp_efuse_read_field_blob(): EFUSE_BLK0 has only 7 × 32-bit registers (224 bits total), not 8 registers (256 bits). Reading more than 7 words from EFUSE_BLK0 triggers assertion failure in esp_efuse_utility_read_reg(). Use maximum 7 words when reading EFUSE_BLK0 to avoid crashes. This applies to all ESP32 variants (ESP32, ESP32-S2, ESP32-S3, ESP32-C3).
Applied to files:
wled00/wled.hwled00/FX_fcn.cpp
📚 Learning: 2025-10-22T22:24:46.163Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/json.cpp:295-304
Timestamp: 2025-10-22T22:24:46.163Z
Learning: When deriving an upper-bound index from a count in WLED-MM PR #270, use a safe guard: last = count ? count - 1 : 0. Avoid min/max with 0 on unsigned types and never pass (count-1) directly to helpers (e.g., updateVal16, relativeChange, random8) without guarding. Effect IDs are 16-bit; palette IDs remain 8-bit but should use the same guard when count can be 0.
Applied to files:
wled00/e131.cppwled00/FX_fcn.cpp
📚 Learning: 2025-07-12T04:20:14.546Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
Applied to files:
wled00/e131.cppwled00/FX_fcn.cpp
📚 Learning: 2025-10-22T21:59:24.331Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/**/*.{cpp,h} : Use spaces (2 per level) for C++ firmware source and headers in wled00
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-05-04T10:32:30.457Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 237
File: wled00/FXparticleSystem.cpp:678-743
Timestamp: 2025-05-04T10:32:30.457Z
Learning: In the Particle System implementation for WLED, stack allocation is preferred over heap allocation for temporary rendering buffers because there's enough stack memory available and heap allocations can cause memory fragmentation.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-11-14T12:42:22.059Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 240
File: wled00/image_loader.cpp:0-0
Timestamp: 2025-11-14T12:42:22.059Z
Learning: In MoonModules/WLED-MM and embedded systems code, the pattern `unsigned timePassed = millis() - last_time;` is rollover-safe and proven to work correctly even when millis() wraps around at ~49.7 days uptime. This works due to modular arithmetic in unsigned integers. The subtraction yields the correct elapsed time regardless of rollover. This pattern should not be flagged as problematic or refactored to use signed arithmetic.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-10-22T21:50:22.379Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/FX.cpp:11799-11813
Timestamp: 2025-10-22T21:50:22.379Z
Learning: In PR #270 (16-bit effect IDs PoC), the current addEffect behavior of appending and ignoring explicit ids >= _mode.size() is intentional for the proof-of-concept; changes to honor explicit ids are deferred to a follow-up.
Applied to files:
wled00/FX_fcn.cpp
🧬 Code graph analysis (3)
wled00/e131.cpp (2)
usermods/artifx/arti.h (1)
millis(150-153)wled00/udp.cpp (2)
setRealtimePixel(651-668)setRealtimePixel(651-651)
wled00/FX_fcn.cpp (3)
wled00/wled.h (1)
size(195-203)wled00/FX.h (1)
isServicing(1028-1028)usermods/artifx/arti.h (1)
millis(150-153)
wled00/FX.h (1)
wled00/FX_fcn.cpp (4)
waitUntilIdle(1916-1932)waitUntilIdle(1916-1916)getActiveSegsLightCapabilities(2257-2263)getActiveSegsLightCapabilities(2257-2257)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Builds (esp32c3dev_4MB_M)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_16MB_V4_M_debug)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_16MB_V4_M_debug)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp32c3mini_dio_4MB_M)
- GitHub Check: Builds (esp32c3dev_2MB_M)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: Builds (esp8266_4MB_M)
- GitHub Check: Builds (esp32s2_PSRAM_M)
- GitHub Check: Builds (esp32_16MB_V4_S)
- GitHub Check: Builds (esp32S3_8MB_PSRAM_M)
- GitHub Check: Builds (esp32S3_WROOM-2_M)
- GitHub Check: Builds (esp01_1MB_S)
- GitHub Check: Builds (esp32_pico_4MB_V4_S)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
🔇 Additional comments (6)
wled00/wled.h (1)
762-771: Global ESP32 draw semaphore definition looks consistentThe new
busDrawMuxglobal is correctly guarded with#ifdef ARDUINO_ARCH_ESP32and wired throughWLED_GLOBAL, so it should integrate cleanly with the ESP32‑only synchronization you’re adding elsewhere.wled00/udp.cpp (1)
163-166: Finite idle wait before entering realtime mode is reasonableSwitching to
strip.waitUntilIdle(350)gives a bounded (but still generous) wait to letservice()finish before going into realtime. This plays fine with the new timeout‑basedwaitUntilIdleimplementation.wled00/FX_fcn.cpp (2)
1874-1888: ESP32 critical sections around LED buffers look correct and focusedThe new
s_wled_strip_muxcritical sections:
- In
Segment::allocLeds(), wrapfree(ledsrgb)andcallocof the new buffer.- In
WS2812FX::finalizeInit(), wrapfree(Segment::_globalLeds)andpurgeSegments(true)while_globalLedsis being torn down.- In
WS2812FX::fixInvalidSegments(), wrap segment bounds fixes andpurgeSegments().are:
- Short (no
delay(), FS, or network calls inside).- Limited to pointer/size updates that might be touched from multiple cores/tasks.
Given that
suspendStripServiceandbusDrawMuxare also in play, this added hardware‑level lock around global LED buffers is a reasonable extra safeguard and doesn’t introduce obvious deadlock paths.Also applies to: 2494-2524, 120-129
2723-2735: Reallocf is already in use elsewhere in the codebase; defensive approach is appropriateThis code pattern using
reallocfwith acallocfallback is consistent with existing usage inFX_2Dfcn.cppand is appropriate for current WLED-MM targets. Reallocf is part of newlib's standard memory management library (stdlib.h), and modern ESP8266 Arduino cores use newlib 4.0.0. Additionally, ESP8266 support is being phased out in WLED-MM, with focus on ESP32 platforms that all include newlib with reallocf support.The defensive implementation—attempting
reallocfthen falling back tocallocon failure—is already established in the codebase and provides appropriate safety. No additional compatibility changes are required.wled00/FX.h (2)
987-987: LGTM: Backward-compatible API enhancement.The addition of the default
timeoutparameter towaitUntilIdlemaintains backward compatibility while enabling timeout-controlled idle waits, aligning with the ESP32 synchronization improvements in FX_fcn.cpp.
1252-1257: Remove placeholder bitfields or document their necessity.The search found no
sizeof(WS2812FX)calls, EEPROM serialization, or direct layout-dependent code for the WS2812FX class itself. While the bitfield members_isOffRefreshRequiredand_hasWhiteChannelare actively used, the placeholder bits ("bullshit", "bullshit2") appear unnecessary unless they preserve layout for an undocumented reason. If layout preservation is intentional (e.g., for ABI compatibility with compiled libraries), add a comment explaining why. Otherwise, remove the placeholders and commented bitfields to improve code clarity.
Hi @DedeHai, I've tested it with WLED-MM on esp32-S3, with a 192x64 hub75 panel. My last commit was necessary to prevent random crashes, but this could be MM specific. The "Akemi" effect runs with ~40fps on my 192x64 pixel HUB75, so its unlikely that the pixel drawing performance in WLED is the limiting factor. Overall I see low performance of the DDP-over-ws solution when going above 64x32 pixels, using one of your example videos:
I think one problem is the limited esp32 buffer capacity for incoming websocket data, this might hurt us above 64x64 pixels when >8 DDP packets are needed to transmit a single frame. But I've also seen very small DDP packets (~16 pixels) that included a DDP sync flag, especially with higher resolutions. 🤔 not sure if this comes from your tool, or it is just a consequence of network buffer congestion. Another bug: VideoLab has a drop-down for the target segment, but I could not limit video output to a segment with this. The segment dimensions are used, but the normal DDP mode just fills all pixels sequentially. Edit: video playback also gets very unstable (with larger sizes) when I have the webUI open, showing LEDs preview. This might be another "smoking gun" of network buffer congestion. |
|
I am not sure using my barely beta stage tool to benchmark WS speed is a valid test ;) I never tested it above 64x64, for a network benchmark you should (have AI) write a simple tool that just pushes a lot of pixels without much processing to take one variable out of the equation. |
Yes you're right, its not a good test because too many "known unknowns". Actually after some more tinkering, i get the feeling that the botleneck comes from the web browser client. I am using Win11, and my first test was with MS Edge, over my home wifi (fast, but wifi sends non-realtime packets in batches) I've added some extra code to ws.cpp to speed up the part of collecting the DDP data in
@DedeHai could it be that the browser (JS) has some rate limiting for websocket? It looks like my esp32-S3 is not at its limits, but DDP packets are arriving very slowly which leads to strong visual stutter. Just an educated guess, because I have no real clue about javascript 😇 |
free beta testing :-) i really like your idea to stream videos to WLED via DDP, so I'm doing my best to make sure WLED-MM can support your new tool once it stabilizes 😀 |
you and me both :) I know for a fact that the video streaming tool is very demanding on CPU as it is not much optimized for processing speed. I did some optimization but since i am pretty much clueless on what browser functions are fast and what better to avoid I am relying 100% on AI to be accurate about that and that may just be the core issue of the stuttering ;) edit: |
unchecking "Receive UDP realtime" in Sync settings will immediately end realtime display, also over webSockets.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/e131.cpp (1)
19-30: Track the disabled packet filtering logic.The late-packet rejection logic has been disabled with
#if 0. While the FIXME comment acknowledges the issue, this should be tracked formally (e.g., in an issue) to ensure it's addressed before release, especially given the PR discussion about packet loss at higher resolutions.Would you like me to open an issue to track re-enabling this logic with proper multi-packet-per-frame support?
🧹 Nitpick comments (1)
wled00/e131.cpp (1)
94-95: Consider documenting the kill switch behavior.The early exit when
receiveDirectis false provides a mechanism to disable incoming realtime data, but the conditions under whichreceiveDirectbecomes false are not documented here. Adding a brief comment explaining when this flag is cleared would improve code clarity.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/e131.cpp(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/e131.cpp
🧠 Learnings (9)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-14T13:55:44.442Z
Learning: In MoonModules/WLED-MM, always flag Variable Length Arrays (VLA) during code reviews, especially when they could overflow available stack space (typically 2K-4K on ESP32/ESP8266). VLAs based on dynamic parameters like segment dimensions, particle counts, or pixel buffers should be highlighted and require justification from the author. Prefer fixed-size arrays, heap allocation, or bounded VLAs with explicit maximum size constraints.
📚 Learning: 2025-11-02T22:07:15.689Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Applied to files:
wled00/e131.cpp
📚 Learning: 2025-10-22T22:24:46.163Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/json.cpp:295-304
Timestamp: 2025-10-22T22:24:46.163Z
Learning: When deriving an upper-bound index from a count in WLED-MM PR #270, use a safe guard: last = count ? count - 1 : 0. Avoid min/max with 0 on unsigned types and never pass (count-1) directly to helpers (e.g., updateVal16, relativeChange, random8) without guarding. Effect IDs are 16-bit; palette IDs remain 8-bit but should use the same guard when count can be 0.
Applied to files:
wled00/e131.cpp
📚 Learning: 2025-11-02T22:17:26.380Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:17:26.380Z
Learning: In wled00/FX.cpp, the `#ifndef WLED_DISABLE_2D` guard around the Akemi 2D fallback in the `mode_oops()` function (lines 142-146) is intentionally kept. This guard is necessary because when users build with `-D WLED_DISABLE_2D` to remove all 2D effects from the build, the Akemi effect is also not compiled, so the guard prevents calling a non-existent function.
Applied to files:
wled00/e131.cpp
📚 Learning: 2025-10-22T21:50:25.839Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Applied to files:
wled00/e131.cpp
📚 Learning: 2025-07-12T04:20:14.546Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
Applied to files:
wled00/e131.cpp
📚 Learning: 2025-10-22T21:59:24.331Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Applied to files:
wled00/e131.cpp
📚 Learning: 2025-07-02T23:22:57.175Z
Learnt from: netmindz
Repo: MoonModules/WLED-MM PR: 248
File: platformio.ini:1613-1613
Timestamp: 2025-07-02T23:22:57.175Z
Learning: In WLED platformio.ini, the particle system disable flags (WLED_DISABLE_PARTICLESYSTEM1D and WLED_DISABLE_PARTICLESYSTEM2D) are intentionally varied across different build environments based on platform memory constraints. More memory-limited platforms (like ESP8266, ESP32-C3, ESP32-S2) disable both 1D and 2D particle systems, while platforms with more available memory (like esp32_4MB_V4_S with 4MB flash) may only disable the 1D version to preserve flash space while keeping 2D functionality available.
Applied to files:
wled00/e131.cpp
📚 Learning: 2025-11-14T13:55:44.442Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-14T13:55:44.442Z
Learning: In MoonModules/WLED-MM, always flag Variable Length Arrays (VLA) during code reviews, especially when they could overflow available stack space (typically 2K-4K on ESP32/ESP8266). VLAs based on dynamic parameters like segment dimensions, particle counts, or pixel buffers should be highlighted and require justification from the author. Prefer fixed-size arrays, heap allocation, or bounded VLAs with explicit maximum size constraints.
Applied to files:
wled00/e131.cpp
🧬 Code graph analysis (1)
wled00/e131.cpp (1)
wled00/udp.cpp (4)
setRealtimePixel(651-668)setRealtimePixel(651-651)exitRealtime(207-222)exitRealtime(207-207)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Builds (adafruit_matrixportal_esp32s3)
- GitHub Check: Builds (esp8266_4MB_M)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_M)
- GitHub Check: Builds (esp32c3dev_4MB_M)
- GitHub Check: Builds (esp32c3mini_dio_4MB_M)
- GitHub Check: Builds (esp01_1MB_S)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: Builds (esp32S3_8MB_PSRAM_M)
- GitHub Check: Builds (esp8266pro_16MB_M)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp32S3_WROOM-2_M)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp32_16MB_V4_M_debug)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_4MB_M_eth)
- GitHub Check: Builds (esp32_pico_4MB_V4_S)
- GitHub Check: Builds (esp32dev_compat)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_4MB_V4_S)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
wled00/e131.cpp (1)
34-37: Remaining race condition on debug counter reads.While the counter increments (lines 65, 67) are now protected by
busDrawMuxon ESP32, the read-and-reset operations at lines 79-81 remain unprotected. Concurrent calls tohandleDDPPacketfrom async_tcp tasks can still race when reading and resetting these counters, leading to inaccurate debug output.Protect the read-and-reset sequence at lines 79-81 with the same
busDrawMuxsemaphore, or convert these counters tostd::atomic<unsigned>and use.load()/.store()for all accesses.Based on PR objectives describing concurrency concerns with async_tcp tasks, and building upon the past review comment about thread-safety.
🧹 Nitpick comments (1)
wled00/e131.cpp (1)
58-71: Semaphore usage now correct; consider timeout constant and debug logging.The critical issue from the previous review has been resolved—
xSemaphoreGiveis now only called whenxSemaphoreTakesucceeds. However, consider two refinements:
- Use
pdMS_TO_TICKS(200)instead of the raw value200to clarify that the timeout is 200 milliseconds.- When lock acquisition fails, the packet is silently dropped. Consider adding a
WLED_DEBUGmessage to aid troubleshooting under high contention.🔎 Optional refinements
#if defined(ARDUINO_ARCH_ESP32) // WLEDMM acquire drawing permission (wait max 200ms) before setting pixels - if (xSemaphoreTake(busDrawMux, 200) == pdTRUE) { + if (xSemaphoreTake(busDrawMux, pdMS_TO_TICKS(200)) == pdTRUE) { #endif for (uint16_t i = start; i < stop; i++) { setRealtimePixel(i, data[c], data[c+1], data[c+2], ddpChannelsPerLed >3 ? data[c+3] : 0); c += ddpChannelsPerLed; pixels++; } packets ++; #if defined(ARDUINO_ARCH_ESP32) xSemaphoreGive(busDrawMux); // WLEDMM release drawing permissions + } else { + #ifdef WLED_DEBUG + USER_PRINTLN(F("DDP: Failed to acquire busDrawMux, packet dropped")); + #endif } #endifBased on the past review that identified the semaphore misuse.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/e131.cpp(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/e131.cpp
🧠 Learnings (9)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
📚 Learning: 2025-11-02T22:07:15.689Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Applied to files:
wled00/e131.cpp
📚 Learning: 2025-11-02T22:17:26.380Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:17:26.380Z
Learning: In wled00/FX.cpp, the `#ifndef WLED_DISABLE_2D` guard around the Akemi 2D fallback in the `mode_oops()` function (lines 142-146) is intentionally kept. This guard is necessary because when users build with `-D WLED_DISABLE_2D` to remove all 2D effects from the build, the Akemi effect is also not compiled, so the guard prevents calling a non-existent function.
Applied to files:
wled00/e131.cpp
📚 Learning: 2025-10-22T22:24:46.163Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/json.cpp:295-304
Timestamp: 2025-10-22T22:24:46.163Z
Learning: When deriving an upper-bound index from a count in WLED-MM PR #270, use a safe guard: last = count ? count - 1 : 0. Avoid min/max with 0 on unsigned types and never pass (count-1) directly to helpers (e.g., updateVal16, relativeChange, random8) without guarding. Effect IDs are 16-bit; palette IDs remain 8-bit but should use the same guard when count can be 0.
Applied to files:
wled00/e131.cpp
📚 Learning: 2025-10-22T21:50:25.839Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Applied to files:
wled00/e131.cpp
📚 Learning: 2025-07-02T23:22:57.175Z
Learnt from: netmindz
Repo: MoonModules/WLED-MM PR: 248
File: platformio.ini:1613-1613
Timestamp: 2025-07-02T23:22:57.175Z
Learning: In WLED platformio.ini, the particle system disable flags (WLED_DISABLE_PARTICLESYSTEM1D and WLED_DISABLE_PARTICLESYSTEM2D) are intentionally varied across different build environments based on platform memory constraints. More memory-limited platforms (like ESP8266, ESP32-C3, ESP32-S2) disable both 1D and 2D particle systems, while platforms with more available memory (like esp32_4MB_V4_S with 4MB flash) may only disable the 1D version to preserve flash space while keeping 2D functionality available.
Applied to files:
wled00/e131.cpp
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/html_*.h : Do not edit generated embedded web header files: wled00/html_*.h
Applied to files:
wled00/e131.cpp
📚 Learning: 2025-10-22T21:59:24.331Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Applied to files:
wled00/e131.cpp
📚 Learning: 2025-11-14T13:55:44.442Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-14T13:55:44.442Z
Learning: In MoonModules/WLED-MM, always flag Variable Length Arrays (VLA) during code reviews, especially when they could overflow available stack space (typically 2K-4K on ESP32/ESP8266). VLAs based on dynamic parameters like segment dimensions, particle counts, or pixel buffers should be highlighted and require justification from the author. Prefer fixed-size arrays, heap allocation, or bounded VLAs with explicit maximum size constraints.
Applied to files:
wled00/e131.cpp
🧬 Code graph analysis (1)
wled00/e131.cpp (1)
wled00/udp.cpp (4)
setRealtimePixel(651-668)setRealtimePixel(651-651)exitRealtime(207-222)exitRealtime(207-207)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Builds (adafruit_matrixportal_esp32s3)
- GitHub Check: Builds (esp32s2_PSRAM_M)
- GitHub Check: Builds (esp32s2_PSRAM_S)
- GitHub Check: Builds (esp32S3_8MB_PSRAM_M)
- GitHub Check: Builds (esp01_1MB_S)
- GitHub Check: Builds (esp8266pro_16MB_M)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: Builds (esp8266_4MB_M)
- GitHub Check: Builds (esp32c3dev_4MB_M)
- GitHub Check: Builds (esp32_16MB_V4_S)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_16MB_V4_M_debug)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp32_pico_4MB_V4_S)
- GitHub Check: Builds (esp32_4MB_M_eth)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
- GitHub Check: Builds (esp32dev_compat)
🔇 Additional comments (2)
wled00/e131.cpp (2)
19-30: Out-of-sequence filtering intentionally disabled.The disabled code block correctly reflects the need to handle many packets per frame in the DDP-over-WebSockets scenario described in the PR objectives, where >8 packets per frame are common for larger displays.
96-97: ThereceiveDirectimplementation is correct and properly initialized.The variable is declared as a global bool with default value
true, loaded from persistent storage (EEPROM) on startup, and controlled through UI toggles and API endpoints. The kill-switch logic at lines 96-97 follows the established pattern used elsewhere in the codebase (e.g.,udp.cpp:285, 288, 528) and maintains the intended semantics for enabling/disabling UDP realtime reception.
revised mutex and critical section handling for segment drawing - simplified code with macros (no more #if ESP32 ... #endif) - remove some critical sections (prevents interrupt stalling) - added mutex to functions that change the list of segments - added mutexes to all (potential) background drawing code - use recursive mutexes to prevent accidental self-locking of tasks
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
wled00/e131.cpp (1)
34-38: DDP debug counters are still not fully synchronized across tasks.
packets/pixelsare incremented while holdingbusDrawMux, but the debug print and reset happen outside that critical section, so concurrenthandleDDPPacketcalls can still race on these statics (especially with async_tcp tasks on multiple cores). If you care about accurate metrics, you’d need to either read/reset them under the same mutex, or move them behind a separate debug lock; otherwise they should be treated as approximate only.Also applies to: 70-78
🧹 Nitpick comments (4)
wled00/json.cpp (1)
345-414: Image-array drawing critical section looks correct; consider handling lock timeouts explicitly.The new
busDrawMuxguard plusseg.calc_virtualLength()bounds checks make the JSON"i"image path much safer against concurrent drawing and OOB writes. The only thing I’d suggest is optionally logging or counting failures whenesp32SemTake(busDrawMux, 250)times out, instead of silently skipping the update, to make diagnosing contention-induced frame drops easier later.wled00/e131.cpp (1)
57-67: busDrawMux protection for DDP/DMX looks sound; note that lock timeouts silently drop frames.Wrapping DDP and DMX pixel writes in
esp32SemTake(busDrawMux, 200)…esp32SemGive(busDrawMux)is consistent with the new drawing mutex used inservice()/show()and should eliminate concurrent access to the LED buses from realtime and effect paths. If theTaketimes out, the code correctly skips the write rather than callingGiveon an unacquired semaphore.Two behavioral notes to be aware of:
- When contention is high, those 200‑tick timeouts will silently drop DDP/DMX frames; if that becomes an issue, you might want to add a debug counter/log for lock failures.
- The new
if (!receiveDirect) { exitRealtime(); return; }kill‑switch at the top ofhandleE131Packetnow disables all E1.31/ArtNet/DDP updates whenreceiveDirectis false, which seems consistent with “disable UDP realtime”, but it’s worth double‑checking that this matches the existing UI/config expectations.Also applies to: 92-124, 185-218, 361-374
wled00/FX_fcn.cpp (2)
1875-1884: Global LED buffer and service()/show() now share a coherent locking scheme; be aware of silent skips on mutex timeouts.
- The
_globalLedsswap infinalizeInit()is now protected by a short ESP32 critical section (portENTER_CRITICALons_wled_strip_mux) while the pointer is nulled, thenfree(oldGLeds)happens outside. WithsuspendStripServiceset earlier infinalizeInit, this avoids concurrent use of the freed buffer from the main effect loop.waitUntilIdle(unsigned timeout)on ESP32+WLEDMM_PROTECT_SERVICEnow busy‑waits up to at leastMAX_IDLE_WAIT_MS(120ms) whileisServicing(), which bounds the wait and prevents indefinite stalls while still giving the effect loop time to finish a frame.- In
service(), effect execution is guarded byesp32SemTake(busDrawMux, 200)/esp32SemGive(busDrawMux), andshow()wrapsbusses.show()with the same mutex and timeout. This aligns realtime (DDP/DMX), JSON, and effect drawing on a single bus‑level lock, preventing overlapping bus writes.The one behavioral caveat is that if
esp32SemTaketimes out inservice()orshow(), you silently skip that frame (with a smalldelay(1)inservice()). That’s probably the right trade‑off for robustness, but if debugging dropped frames becomes important later, you may want to add a lightweight counter or debug log for lock failures.Also applies to: 1907-1927, 1930-2008, 2139-2152
2718-2737: Check thatreallocfis available on all supported toolchains in deserializeMap().The new
deserializeMap()usesreallocf(customMappingTable, sizeof(uint16_t) * size)to resize the mapping table, with a fallback tocallocif that fails—which is a nice pattern (and you correctly resetcustomMappingTableSizeanderrorFlagdepending on outcomes). However,reallocfis not part of standard C and isn’t guaranteed to exist in all libc/newlib variants.Please double‑check that both your ESP32 and ESP8266 toolchains ship
reallocf; if not, you may need to replace it with plainreallocplus an explicit error‑handling path.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
wled00/FX_fcn.cpp(16 hunks)wled00/e131.cpp(7 hunks)wled00/json.cpp(2 hunks)wled00/wled.cpp(2 hunks)wled00/wled.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wled00/wled.cpp
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/e131.cppwled00/wled.hwled00/FX_fcn.cppwled00/json.cpp
🧠 Learnings (15)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-14T13:55:44.442Z
Learning: In MoonModules/WLED-MM, always flag Variable Length Arrays (VLA) during code reviews, especially when they could overflow available stack space (typically 2K-4K on ESP32/ESP8266). VLAs based on dynamic parameters like segment dimensions, particle counts, or pixel buffers should be highlighted and require justification from the author. Prefer fixed-size arrays, heap allocation, or bounded VLAs with explicit maximum size constraints.
📚 Learning: 2025-11-02T22:07:15.689Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Applied to files:
wled00/e131.cppwled00/wled.hwled00/FX_fcn.cpp
📚 Learning: 2025-11-02T22:17:26.380Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:17:26.380Z
Learning: In wled00/FX.cpp, the `#ifndef WLED_DISABLE_2D` guard around the Akemi 2D fallback in the `mode_oops()` function (lines 142-146) is intentionally kept. This guard is necessary because when users build with `-D WLED_DISABLE_2D` to remove all 2D effects from the build, the Akemi effect is also not compiled, so the guard prevents calling a non-existent function.
Applied to files:
wled00/e131.cppwled00/wled.hwled00/FX_fcn.cpp
📚 Learning: 2025-10-22T21:50:25.839Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Applied to files:
wled00/e131.cppwled00/wled.hwled00/FX_fcn.cpp
📚 Learning: 2025-10-22T22:24:46.163Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/json.cpp:295-304
Timestamp: 2025-10-22T22:24:46.163Z
Learning: When deriving an upper-bound index from a count in WLED-MM PR #270, use a safe guard: last = count ? count - 1 : 0. Avoid min/max with 0 on unsigned types and never pass (count-1) directly to helpers (e.g., updateVal16, relativeChange, random8) without guarding. Effect IDs are 16-bit; palette IDs remain 8-bit but should use the same guard when count can be 0.
Applied to files:
wled00/e131.cppwled00/FX_fcn.cpp
📚 Learning: 2025-11-14T13:55:44.442Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-14T13:55:44.442Z
Learning: In MoonModules/WLED-MM, always flag Variable Length Arrays (VLA) during code reviews, especially when they could overflow available stack space (typically 2K-4K on ESP32/ESP8266). VLAs based on dynamic parameters like segment dimensions, particle counts, or pixel buffers should be highlighted and require justification from the author. Prefer fixed-size arrays, heap allocation, or bounded VLAs with explicit maximum size constraints.
Applied to files:
wled00/e131.cppwled00/wled.hwled00/FX_fcn.cpp
📚 Learning: 2025-10-22T21:59:24.331Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Applied to files:
wled00/e131.cppwled00/wled.hwled00/FX_fcn.cpp
📚 Learning: 2025-07-12T04:20:14.546Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
Applied to files:
wled00/e131.cppwled00/FX_fcn.cpp
📚 Learning: 2025-11-14T12:42:22.059Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 240
File: wled00/image_loader.cpp:0-0
Timestamp: 2025-11-14T12:42:22.059Z
Learning: In MoonModules/WLED-MM and embedded systems code, the pattern `unsigned timePassed = millis() - last_time;` is rollover-safe and proven to work correctly even when millis() wraps around at ~49.7 days uptime. This works due to modular arithmetic in unsigned integers. The subtraction yields the correct elapsed time regardless of rollover. This pattern should not be flagged as problematic or refactored to use signed arithmetic.
Applied to files:
wled00/e131.cppwled00/FX_fcn.cpp
📚 Learning: 2025-07-02T23:22:57.175Z
Learnt from: netmindz
Repo: MoonModules/WLED-MM PR: 248
File: platformio.ini:1613-1613
Timestamp: 2025-07-02T23:22:57.175Z
Learning: In WLED platformio.ini, the particle system disable flags (WLED_DISABLE_PARTICLESYSTEM1D and WLED_DISABLE_PARTICLESYSTEM2D) are intentionally varied across different build environments based on platform memory constraints. More memory-limited platforms (like ESP8266, ESP32-C3, ESP32-S2) disable both 1D and 2D particle systems, while platforms with more available memory (like esp32_4MB_V4_S with 4MB flash) may only disable the 1D version to preserve flash space while keeping 2D functionality available.
Applied to files:
wled00/e131.cppwled00/wled.hwled00/FX_fcn.cpp
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/html_*.h : Do not edit generated embedded web header files: wled00/html_*.h
Applied to files:
wled00/wled.h
📚 Learning: 2025-06-30T17:53:37.232Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 248
File: usermods/audioreactive/audio_reactive.h:199-200
Timestamp: 2025-06-30T17:53:37.232Z
Learning: In WLED AudioReactive usermod, when using ArduinoFFT (enabled via UM_AUDIOREACTIVE_USE_ARDUINO_FFT), it's acceptable and necessary to redefine the global sqrt macro to sqrtf within the conditional compilation block for performance optimization. This is a specific requirement for ArduinoFFT optimization that can provide 10-50% performance improvement on ESP32.
Applied to files:
wled00/wled.h
📚 Learning: 2025-11-24T00:15:38.069Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-24T00:15:38.069Z
Learning: In MoonModules/WLED-MM, when reading ESP32/ESP32-S3 eFuse blocks using esp_efuse_read_field_blob(): EFUSE_BLK0 has only 7 × 32-bit registers (224 bits total), not 8 registers (256 bits). Reading more than 7 words from EFUSE_BLK0 triggers assertion failure in esp_efuse_utility_read_reg(). Use maximum 7 words when reading EFUSE_BLK0 to avoid crashes. This applies to all ESP32 variants (ESP32, ESP32-S2, ESP32-S3, ESP32-C3).
Applied to files:
wled00/wled.hwled00/FX_fcn.cpp
📚 Learning: 2025-05-04T10:32:30.457Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 237
File: wled00/FXparticleSystem.cpp:678-743
Timestamp: 2025-05-04T10:32:30.457Z
Learning: In the Particle System implementation for WLED, stack allocation is preferred over heap allocation for temporary rendering buffers because there's enough stack memory available and heap allocations can cause memory fragmentation.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-10-22T21:50:22.379Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/FX.cpp:11799-11813
Timestamp: 2025-10-22T21:50:22.379Z
Learning: In PR #270 (16-bit effect IDs PoC), the current addEffect behavior of appending and ignoring explicit ids >= _mode.size() is intentional for the proof-of-concept; changes to honor explicit ids are deferred to a follow-up.
Applied to files:
wled00/FX_fcn.cpp
🧬 Code graph analysis (1)
wled00/e131.cpp (1)
wled00/udp.cpp (4)
setRealtimePixel(651-668)setRealtimePixel(651-651)exitRealtime(207-222)exitRealtime(207-207)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: Builds (esp32c3dev_4MB_M)
- GitHub Check: Builds (esp32c3dev_2MB_M)
- GitHub Check: Builds (esp8266_4MB_S)
- GitHub Check: Builds (esp8266_2MB_S)
- GitHub Check: Builds (esp8266_4MB_M)
- GitHub Check: Builds (esp32s2_PSRAM_S)
- GitHub Check: Builds (esp32_16MB_V4_S)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
- GitHub Check: Builds (adafruit_matrixportal_esp32s3)
- GitHub Check: Builds (athom_music_esp32_4MB_M)
- GitHub Check: Builds (esp32_16MB_V4_M_debug)
- GitHub Check: Builds (esp32_pico_4MB_V4_S)
- GitHub Check: Builds (esp32_4MB_V4_S)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_4MB_M_eth)
🔇 Additional comments (4)
wled00/e131.cpp (1)
18-30: DDP late‑packet filter is now compiled out.The
#if 0around the late‑packet rejection logic meanse131SkipOutOfSequenceno longer has any effect for DDP traffic. That matches the comment about having far more than 5–10 packets per frame, but it’s worth keeping in mind that, as‑is, DDP will accept all in‑sequence packets regardless of frame boundary heuristics. If you intend to reintroduce filtering later, consider adding a TODO or comment pointing to the desired new heuristic.wled00/wled.h (1)
769-785: busDrawMux global and esp32SemTake/esp32SemGive macros are well‑structured; ensure early initialization on ESP32.The
SemaphoreHandle_t busDrawMuxplusesp32SemTake/esp32SemGivewrappers give a clean, cross‑platform way to guard drawing, and the dummy macros keep ESP8266 builds simple. Just make surebusDrawMuxis created (e.g. viaxSemaphoreCreateRecursiveMutex()or similar) very early in setup, before any code paths that callesp32SemTake(busDrawMux, …)can run, so that all the new guards inservice(),show(), JSON/DDP/DMX, etc. are working as intended.wled00/FX_fcn.cpp (2)
107-133: allocLeds refactor looks safe; OOM behavior is acceptable but note loss of old buffer.The new
allocLeds()logic (freeingoldLedsRgbthencalloc’ing a larger buffer and updatingledsrgbSize) is simpler and avoids doing heap allocations inside a critical section, which is good. On allocation failure you now end up withledsrgb == nullptr,ledsrgbSize == 0, andERR_LOW_BUFset—which is a reasonable fail‑safe even though any previously allocated (too‑small) buffer is discarded rather than kept as a fallback. That trade‑off is fine given the alternative would be writing past the old buffer.
2346-2363: Using busDrawMux around segment structural changes is a good safeguard.
purgeSegments(),resetSegments(false),makeAutoSegments(true), andfixInvalidSegments()now all takebusDrawMux(with either a finite timeout orportMAX_DELAY) while mutating the_segmentsvector and segment bounds. That’s consistent with the rest of the drawing synchronization and should prevent segment reconfiguration from racing with effect rendering or realtime writers.
fixInvalidSegments()correctly releases the lock before callingpurgeSegments(), so there’s no lock inversion, and re‑acquisition relies on the same mutex. Given the use of a recursive semaphore, any nested calls from the same task would still be safe.Also applies to: 2387-2415, 2418-2472, 2496-2523
softhack007
changed the title
|
regarding the rabbits issues with the pixelmagic tool: I tested the one from pixelforge i.e. the gz file from my repo (which I either pulled from upstream or original source, can't remember) and there were no console errors except when I tried to use it on a 1D strip which I think it does not support. You'd need to test if the version from the "other tools" link works with MM. edit: I was thinking of removing the pixelmagic tool entirely from the WLED build as it is much more convenient for users to just hit that download button but kept it for backwards build compatibility. |
* avoid implicit creation of global vars * removed stray </small> * fix assignment with undefined variables
|
Tip For best results, initiate chat on the files or code changes. @softhack007 Short answer: not reliably on-device; partially yes on a host.
Practical ways to catch/prevent this in WLED-MM:
Bottom line: TSan/ASan won’t catch this; Valgrind can in a host test with --track-fds. On ESP32, the safest path is the code fixes you’re applying plus lightweight debug assertions/instrumentation. 🧠 Learnings used |
|
@softhack007 the VideoLab tool has gotten a big speed bump by scaling down the video input before processing and I added WS queue overflow protection as per your request. Please check it out (available in my repo) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wled00/wled.h (1)
768-791: Consider guarding portENTER_CRITICAL/portEXIT_CRITICAL redefinitions for ESP8266 fallback.The recursive mutex design is correct—heap operations can safely be performed while holding
esp32SemTake/esp32SemGive, unlike spinlock critical sections. However, the unconditional redefinition ofportENTER_CRITICALandportEXIT_CRITICALas no-ops on ESP8266 (lines 789–790) lacks guard conditions. While no current conflicts exist in the codebase, adding#ifndefguards is defensive:+#ifndef portENTER_CRITICAL #define portENTER_CRITICAL(sname) +#endif +#ifndef portEXIT_CRITICAL #define portEXIT_CRITICAL(sname) +#endifThis prevents accidental redefinition conflicts if future ESP8266 SDK updates introduce these macros.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
wled00/FX_fcn.cppwled00/udp.cppwled00/util.cppwled00/wled.h
✅ Files skipped from review due to trivial changes (1)
- wled00/udp.cpp
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/util.cppwled00/wled.hwled00/FX_fcn.cpp
🧠 Learnings (21)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-23T21:29:14.915Z
Learning: In MoonModules/WLED-MM, presets.json writes must be serialized and atomic: guard all write paths with a dedicated file-level mutex (presetFileMux) and use temp-file + rename to replace the file. The “immediate write” branch in presets.cpp can race with async doSaveState() and corrupt presets.json; prefer routing all saves through the async path or enforce the same file mutex + atomic write there.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-14T13:55:44.442Z
Learning: In MoonModules/WLED-MM, always flag Variable Length Arrays (VLA) during code reviews, especially when they could overflow available stack space (typically 2K-4K on ESP32/ESP8266). VLAs based on dynamic parameters like segment dimensions, particle counts, or pixel buffers should be highlighted and require justification from the author. Prefer fixed-size arrays, heap allocation, or bounded VLAs with explicit maximum size constraints.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-23T21:36:41.126Z
Learning: In MoonModules/WLED-MM, presets.json writes must be serialized with a dedicated file-level mutex (presetFileMux) to prevent corruption when the immediate write path in savePreset() races with the async write path in doSaveState(). Use a 2500ms timeout for the mutex to accommodate typical 200KB file writes (~750ms) with safety margin.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-25T00:46:23.500Z
Learning: In MoonModules/WLED-MM file operations using the global file handle `f` in file.cpp, writeObjectToFile() must check `if (doCloseFile) closeFile();` before opening a new file, following the same pattern as readObjectFromFile(). Without this check, opening a new file while a previous file operation has set doCloseFile=true will overwrite the global file handle without flushing buffers, causing data corruption. This is critical for preventing presets.json corruption when async and immediate write paths race.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
Learnt from: netmindz
Repo: MoonModules/WLED-MM PR: 248
File: platformio.ini:1613-1613
Timestamp: 2025-07-02T23:22:57.175Z
Learning: In WLED platformio.ini, the particle system disable flags (WLED_DISABLE_PARTICLESYSTEM1D and WLED_DISABLE_PARTICLESYSTEM2D) are intentionally varied across different build environments based on platform memory constraints. More memory-limited platforms (like ESP8266, ESP32-C3, ESP32-S2) disable both 1D and 2D particle systems, while platforms with more available memory (like esp32_4MB_V4_S with 4MB flash) may only disable the 1D version to preserve flash space while keeping 2D functionality available.
📚 Learning: 2025-12-23T21:29:14.915Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-23T21:29:14.915Z
Learning: In MoonModules/WLED-MM, presets.json writes must be serialized and atomic: guard all write paths with a dedicated file-level mutex (presetFileMux) and use temp-file + rename to replace the file. The “immediate write” branch in presets.cpp can race with async doSaveState() and corrupt presets.json; prefer routing all saves through the async path or enforce the same file mutex + atomic write there.
Applied to files:
wled00/util.cppwled00/wled.hwled00/FX_fcn.cpp
📚 Learning: 2025-12-22T21:14:08.797Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-22T21:14:08.797Z
Learning: In MoonModules/WLED-MM on ESP32 with FreeRTOS, heap operations (malloc, free, realloc) are thread-safe but must NOT be called inside critical sections (portENTER_CRITICAL/portEXIT_CRITICAL) as they can block and involve the scheduler. Use recursive mutexes (xSemaphoreTakeRecursive/Give) instead to protect buffer reallocations. Critical sections (spinlocks) are only appropriate for very short, non-blocking operations.
Applied to files:
wled00/util.cppwled00/wled.hwled00/FX_fcn.cpp
📚 Learning: 2025-12-23T21:36:41.126Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-23T21:36:41.126Z
Learning: In MoonModules/WLED-MM, presets.json writes must be serialized with a dedicated file-level mutex (presetFileMux) to prevent corruption when the immediate write path in savePreset() races with the async write path in doSaveState(). Use a 2500ms timeout for the mutex to accommodate typical 200KB file writes (~750ms) with safety margin.
Applied to files:
wled00/util.cppwled00/wled.hwled00/FX_fcn.cpp
📚 Learning: 2025-12-25T00:46:23.500Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-25T00:46:23.500Z
Learning: In MoonModules/WLED-MM file operations using the global file handle `f` in file.cpp, writeObjectToFile() must check `if (doCloseFile) closeFile();` before opening a new file, following the same pattern as readObjectFromFile(). Without this check, opening a new file while a previous file operation has set doCloseFile=true will overwrite the global file handle without flushing buffers, causing data corruption. This is critical for preventing presets.json corruption when async and immediate write paths race.
Applied to files:
wled00/util.cppwled00/FX_fcn.cpp
📚 Learning: 2025-10-22T21:59:24.331Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Applied to files:
wled00/util.cppwled00/FX_fcn.cpp
📚 Learning: 2025-12-22T20:31:17.991Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-22T20:31:17.991Z
Learning: In MoonModules/WLED-MM on ESP32/ESP8266 platforms with limited heap memory, when reallocating buffers (e.g., LED buffers), always free the old buffer before allocating the new one. Allocating the new buffer first would temporarily require double the memory and could cause malloc failures when available RAM is low. The free-then-allocate pattern is preferred for resource-constrained embedded devices, even if it creates a brief nullptr window that must be protected by proper synchronization.
Applied to files:
wled00/util.cppwled00/wled.hwled00/FX_fcn.cpp
📚 Learning: 2025-11-14T13:55:44.442Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-14T13:55:44.442Z
Learning: In MoonModules/WLED-MM, always flag Variable Length Arrays (VLA) during code reviews, especially when they could overflow available stack space (typically 2K-4K on ESP32/ESP8266). VLAs based on dynamic parameters like segment dimensions, particle counts, or pixel buffers should be highlighted and require justification from the author. Prefer fixed-size arrays, heap allocation, or bounded VLAs with explicit maximum size constraints.
Applied to files:
wled00/util.cppwled00/wled.hwled00/FX_fcn.cpp
📚 Learning: 2025-11-02T22:07:15.689Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Applied to files:
wled00/util.cppwled00/wled.hwled00/FX_fcn.cpp
📚 Learning: 2025-07-12T04:20:14.546Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
Applied to files:
wled00/util.cppwled00/FX_fcn.cpp
📚 Learning: 2025-11-24T00:15:38.069Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-24T00:15:38.069Z
Learning: In MoonModules/WLED-MM, when reading ESP32/ESP32-S3 eFuse blocks using esp_efuse_read_field_blob(): EFUSE_BLK0 has only 7 × 32-bit registers (224 bits total), not 8 registers (256 bits). Reading more than 7 words from EFUSE_BLK0 triggers assertion failure in esp_efuse_utility_read_reg(). Use maximum 7 words when reading EFUSE_BLK0 to avoid crashes. This applies to all ESP32 variants (ESP32, ESP32-S2, ESP32-S3, ESP32-C3).
Applied to files:
wled00/util.cppwled00/wled.hwled00/FX_fcn.cpp
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/html_*.h : Do not edit generated embedded web header files: wled00/html_*.h
Applied to files:
wled00/wled.hwled00/FX_fcn.cpp
📚 Learning: 2025-12-21T22:26:03.261Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
Applied to files:
wled00/wled.hwled00/FX_fcn.cpp
📚 Learning: 2025-07-02T23:22:57.175Z
Learnt from: netmindz
Repo: MoonModules/WLED-MM PR: 248
File: platformio.ini:1613-1613
Timestamp: 2025-07-02T23:22:57.175Z
Learning: In WLED platformio.ini, the particle system disable flags (WLED_DISABLE_PARTICLESYSTEM1D and WLED_DISABLE_PARTICLESYSTEM2D) are intentionally varied across different build environments based on platform memory constraints. More memory-limited platforms (like ESP8266, ESP32-C3, ESP32-S2) disable both 1D and 2D particle systems, while platforms with more available memory (like esp32_4MB_V4_S with 4MB flash) may only disable the 1D version to preserve flash space while keeping 2D functionality available.
Applied to files:
wled00/wled.hwled00/FX_fcn.cpp
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/**/*.{cpp,h} : Use spaces (2 per level) for C++ firmware source and headers in wled00
Applied to files:
wled00/wled.h
📚 Learning: 2025-10-22T21:50:25.839Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Applied to files:
wled00/wled.hwled00/FX_fcn.cpp
📚 Learning: 2025-06-30T17:53:37.232Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 248
File: usermods/audioreactive/audio_reactive.h:199-200
Timestamp: 2025-06-30T17:53:37.232Z
Learning: In WLED AudioReactive usermod, when using ArduinoFFT (enabled via UM_AUDIOREACTIVE_USE_ARDUINO_FFT), it's acceptable and necessary to redefine the global sqrt macro to sqrtf within the conditional compilation block for performance optimization. This is a specific requirement for ArduinoFFT optimization that can provide 10-50% performance improvement on ESP32.
Applied to files:
wled00/wled.hwled00/FX_fcn.cpp
📚 Learning: 2025-11-02T22:17:26.380Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:17:26.380Z
Learning: In wled00/FX.cpp, the `#ifndef WLED_DISABLE_2D` guard around the Akemi 2D fallback in the `mode_oops()` function (lines 142-146) is intentionally kept. This guard is necessary because when users build with `-D WLED_DISABLE_2D` to remove all 2D effects from the build, the Akemi effect is also not compiled, so the guard prevents calling a non-existent function.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-10-22T22:24:46.163Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/json.cpp:295-304
Timestamp: 2025-10-22T22:24:46.163Z
Learning: When deriving an upper-bound index from a count in WLED-MM PR #270, use a safe guard: last = count ? count - 1 : 0. Avoid min/max with 0 on unsigned types and never pass (count-1) directly to helpers (e.g., updateVal16, relativeChange, random8) without guarding. Effect IDs are 16-bit; palette IDs remain 8-bit but should use the same guard when count can be 0.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-11-14T12:42:22.059Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 240
File: wled00/image_loader.cpp:0-0
Timestamp: 2025-11-14T12:42:22.059Z
Learning: In MoonModules/WLED-MM and embedded systems code, the pattern `unsigned timePassed = millis() - last_time;` is rollover-safe and proven to work correctly even when millis() wraps around at ~49.7 days uptime. This works due to modular arithmetic in unsigned integers. The subtraction yields the correct elapsed time regardless of rollover. This pattern should not be flagged as problematic or refactored to use signed arithmetic.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-05-04T10:32:30.457Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 237
File: wled00/FXparticleSystem.cpp:678-743
Timestamp: 2025-05-04T10:32:30.457Z
Learning: In the Particle System implementation for WLED, stack allocation is preferred over heap allocation for temporary rendering buffers because there's enough stack memory available and heap allocations can cause memory fragmentation.
Applied to files:
wled00/FX_fcn.cpp
🧬 Code graph analysis (2)
wled00/util.cpp (1)
usermods/artifx/arti.h (1)
millis(150-153)
wled00/FX_fcn.cpp (1)
wled00/FX.h (2)
is2D(619-619)calc_virtualWidth(747-758)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
- GitHub Check: Builds (esp32_4MB_M_eth)
- GitHub Check: Builds (esp01_1MB_S)
- GitHub Check: Builds (esp8266pro_16MB_M)
- GitHub Check: Builds (esp8266_4MB_M)
- GitHub Check: Builds (athom_music_esp32_4MB_M)
- GitHub Check: Builds (esp32c3dev_4MB_M)
- GitHub Check: Builds (esp32c3mini_dio_4MB_M)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp32c3dev_2MB_M)
- GitHub Check: Builds (esp32S3_8MB_PSRAM_M)
- GitHub Check: Builds (esp32_16MB_V4_S)
- GitHub Check: Builds (esp32s2_PSRAM_S)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp32_pico_4MB_V4_S)
- GitHub Check: Builds (esp32_4MB_M_eth)
- GitHub Check: Builds (esp32dev_compat)
🔇 Additional comments (13)
wled00/util.cpp (2)
225-257: Recursive mutex pattern looks correct for ESP32.The JSON buffer locking implementation properly uses a recursive mutex with a 1800ms timeout that exceeds the
suspendStripServicetimeout (1500ms), preventing deadlocks. The ESP8266 fallback withcan_yield()busy-wait is appropriate for the single-core platform.One minor observation: Lines 240-242 handle the edge case where the mutex was acquired but
jsonBufferLockis still set (indicating another context holds it). Releasing the mutex before returning is correct.
261-271: Release order is correct.Setting
jsonBufferLock = 0before callingesp32SemGive(jsonBufferLockMutex)is the correct order. Any waiting task cannot proceed until it acquires the mutex, so the flag will be visible as cleared when the waiting task resumes.wled00/FX_fcn.cpp (10)
119-133: Atomic pointer swap pattern is correct.The
allocLeds()reallocation follows the proper pattern per learnings:
- Capture old pointer outside critical section
- Set
ledsrgb = nullptrinside spinlock (atomic clear)- Free old buffer outside critical section (heap ops not in critical sections ✓)
- Allocate new buffer outside critical section (heap ops not in critical sections ✓)
- Set
ledsrgb = newLedsRgbinside spinlock (atomic assignment)This addresses the prior review comment about race conditions. The brief
nullptrwindow is acceptable since concurrent readers will see either the old valid pointer,nullptr, or the new valid pointer.
574-596: Graceful failure handling in setUp().The segmentMux acquisition with 2100ms timeout and the fallback behavior (setting
boundsUnchanged = trueand logging) is appropriate. This addresses the prior review concern about infinite waits. The mutex is correctly released before callingmarkForReset()andrefreshLightCapabilities()which may trigger further operations.
1467-1469: Thread-safe dimension calculation in fill().Using
calc_virtualWidth()/calc_virtualHeight()instead of the cached versions ensures correct dimensions whenfill()is called fromjson.cppoutside the normal segment frame context. This prevents stale cached values from causing incorrect fill operations.
1892-1901: Correct atomic swap pattern for _globalLeds.The
_globalLedscleanup follows the same safe pattern asallocLeds(): capture pointer, atomically clear under spinlock, then free outside the critical section. CallingpurgeSegments(true)after ensures segments don't hold stale references.
1929-1944: Flexible timeout for waitUntilIdle().The new
unsigned timeoutparameter allows callers to specify custom wait times while ensuring a minimum ofMAX_IDLE_WAIT_MS(120ms). This is more flexible than the previous fixed wait and aligns with the configurable timeout pattern used elsewhere.
2004-2019: Per-segment mutex acquisition in service() loop.The
busDrawMuxis acquired and released for each segment's effect execution (lines 2006, 2019). While this adds lock/unlock overhead per segment, it allows other high-priority tasks (DDP, WebSockets) to interleave during long effect chains, which aligns with the PR's goal of improving stability with concurrent access.If mutex acquisition fails (line 2006), the
delay(1); continue;pattern silently skips the segment for this frame. This matches the author's stated preference for silent frame drops over log flooding. Consider adding a rate-limited debug counter if visibility into contention is ever needed.
2155-2159: Bus show protected by mutex.The
busDrawMuxprotection aroundbusses.show()prevents race conditions with concurrent effect drawing. The silent early return on mutex timeout aligns with the author's stated design preference to avoid log flooding under high contention.
2354-2370: Segment vector modifications properly protected.Both
purgeSegments()andresetSegments()acquiresegmentMuxbefore modifying the_segmentsvector, preventing iterator invalidation ifservice()is iterating concurrently. The 300ms timeout inpurgeSegments()is appropriate for a quick operation; the 2100ms inresetSegments()allows for longer initialization scenarios.
2507-2536: Correct mutex release before nested call.Line 2530 correctly releases
segmentMuxbefore callingpurgeSegments(), which also acquires the same mutex. Since these are recursive mutexes, nested acquisition would technically work, but releasing first is cleaner and avoids holding the lock longer than necessary.
2429-2482: makeAutoSegments() protected during segment creation.The
segmentMuxprotection (lines 2431-2482) ensures the_segmentsvector isn't modified by other tasks during the auto-segmentation logic. The mutex is released beforefixInvalidSegments()is called (line 2504 is outside the protected block), allowing that function to acquire its own lock independently.wled00/wled.h (1)
771-773: No action needed. The semaphore handles (busDrawMux,segmentMux,jsonBufferLockMutex) are properly created withxSemaphoreCreateRecursiveMutex()inwled.cpplines 480-482 during setup, before any code attempts to use them. Error handling is in place (line 483), and all usage sites check the return value ofesp32SemTake()before proceeding.
…nter * this prevents "open after open", that could lead to file corruption when "f" was still holding some changes in its buffer (written out with f.close())
prevents concurrent presets.json writes from doSaveState() and savePreset()
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wled00/presets.cpp (1)
328-334: Minor: Fix copy-paste error in comment.The comment at line 332 incorrectly references "doSaveState()" but this code is in the savePreset() function. Otherwise, the mutex acquisition is correct and properly guards the immediate write path.
🔎 Proposed fix
- USER_PRINTLN(F("doSaveState(): preset file busy, cannot write")); + USER_PRINTLN(F("savePreset(): preset file busy, cannot write"));
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
wled00/file.cppwled00/presets.cppwled00/wled.cppwled00/wled.h
🚧 Files skipped from review as they are similar to previous changes (1)
- wled00/wled.cpp
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/presets.cppwled00/file.cppwled00/wled.h
🧠 Learnings (15)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-23T21:29:14.927Z
Learning: In MoonModules/WLED-MM, presets.json writes must be serialized and atomic: guard all write paths with a dedicated file-level mutex (presetFileMux) and use temp-file + rename to replace the file. The “immediate write” branch in presets.cpp can race with async doSaveState() and corrupt presets.json; prefer routing all saves through the async path or enforce the same file mutex + atomic write there.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-23T21:36:41.138Z
Learning: In MoonModules/WLED-MM, presets.json writes must be serialized with a dedicated file-level mutex (presetFileMux) to prevent corruption when the immediate write path in savePreset() races with the async write path in doSaveState(). Use a 2500ms timeout for the mutex to accommodate typical 200KB file writes (~750ms) with safety margin.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-14T13:55:44.442Z
Learning: In MoonModules/WLED-MM, always flag Variable Length Arrays (VLA) during code reviews, especially when they could overflow available stack space (typically 2K-4K on ESP32/ESP8266). VLAs based on dynamic parameters like segment dimensions, particle counts, or pixel buffers should be highlighted and require justification from the author. Prefer fixed-size arrays, heap allocation, or bounded VLAs with explicit maximum size constraints.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-25T00:46:23.516Z
Learning: In MoonModules/WLED-MM file operations using the global file handle `f` in file.cpp, writeObjectToFile() must check `if (doCloseFile) closeFile();` before opening a new file, following the same pattern as readObjectFromFile(). Without this check, opening a new file while a previous file operation has set doCloseFile=true will overwrite the global file handle without flushing buffers, causing data corruption. This is critical for preventing presets.json corruption when async and immediate write paths race.
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-22T21:14:08.797Z
Learning: In MoonModules/WLED-MM on ESP32 with FreeRTOS, heap operations (malloc, free, realloc) are thread-safe but must NOT be called inside critical sections (portENTER_CRITICAL/portEXIT_CRITICAL) as they can block and involve the scheduler. Use recursive mutexes (xSemaphoreTakeRecursive/Give) instead to protect buffer reallocations. Critical sections (spinlocks) are only appropriate for very short, non-blocking operations.
Learnt from: netmindz
Repo: MoonModules/WLED-MM PR: 248
File: platformio.ini:1613-1613
Timestamp: 2025-07-02T23:22:57.175Z
Learning: In WLED platformio.ini, the particle system disable flags (WLED_DISABLE_PARTICLESYSTEM1D and WLED_DISABLE_PARTICLESYSTEM2D) are intentionally varied across different build environments based on platform memory constraints. More memory-limited platforms (like ESP8266, ESP32-C3, ESP32-S2) disable both 1D and 2D particle systems, while platforms with more available memory (like esp32_4MB_V4_S with 4MB flash) may only disable the 1D version to preserve flash space while keeping 2D functionality available.
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/data/**/*.{htm,html,css,js} : After editing web UI files, run npm run build to regenerate embedded headers
📚 Learning: 2025-12-23T21:29:14.927Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-23T21:29:14.927Z
Learning: In MoonModules/WLED-MM, presets.json writes must be serialized and atomic: guard all write paths with a dedicated file-level mutex (presetFileMux) and use temp-file + rename to replace the file. The “immediate write” branch in presets.cpp can race with async doSaveState() and corrupt presets.json; prefer routing all saves through the async path or enforce the same file mutex + atomic write there.
Applied to files:
wled00/presets.cppwled00/file.cppwled00/wled.h
📚 Learning: 2025-12-23T21:36:41.138Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-23T21:36:41.138Z
Learning: In MoonModules/WLED-MM, presets.json writes must be serialized with a dedicated file-level mutex (presetFileMux) to prevent corruption when the immediate write path in savePreset() races with the async write path in doSaveState(). Use a 2500ms timeout for the mutex to accommodate typical 200KB file writes (~750ms) with safety margin.
Applied to files:
wled00/presets.cppwled00/file.cppwled00/wled.h
📚 Learning: 2025-12-25T00:46:23.516Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-25T00:46:23.516Z
Learning: In MoonModules/WLED-MM file operations using the global file handle `f` in file.cpp, writeObjectToFile() must check `if (doCloseFile) closeFile();` before opening a new file, following the same pattern as readObjectFromFile(). Without this check, opening a new file while a previous file operation has set doCloseFile=true will overwrite the global file handle without flushing buffers, causing data corruption. This is critical for preventing presets.json corruption when async and immediate write paths race.
Applied to files:
wled00/presets.cppwled00/file.cpp
📚 Learning: 2025-12-25T00:45:39.751Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-25T00:45:39.751Z
Learning: In wled00/file.cpp, writeObjectToFile() must call closeFile() when doCloseFile is true before opening the global File f for a new write. Without this pre-close, a second write can reassign f while the previous write’s handle is still open, risking unflushed buffers and file corruption. Read paths already pre-close; writes need the same guard.
Applied to files:
wled00/presets.cppwled00/file.cpp
📚 Learning: 2025-12-21T22:26:03.261Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
Applied to files:
wled00/wled.h
📚 Learning: 2025-11-02T22:07:15.689Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Applied to files:
wled00/wled.h
📚 Learning: 2025-12-22T21:14:08.797Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-22T21:14:08.797Z
Learning: In MoonModules/WLED-MM on ESP32 with FreeRTOS, heap operations (malloc, free, realloc) are thread-safe but must NOT be called inside critical sections (portENTER_CRITICAL/portEXIT_CRITICAL) as they can block and involve the scheduler. Use recursive mutexes (xSemaphoreTakeRecursive/Give) instead to protect buffer reallocations. Critical sections (spinlocks) are only appropriate for very short, non-blocking operations.
Applied to files:
wled00/wled.h
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/html_*.h : Do not edit generated embedded web header files: wled00/html_*.h
Applied to files:
wled00/wled.h
📚 Learning: 2025-07-02T23:22:57.175Z
Learnt from: netmindz
Repo: MoonModules/WLED-MM PR: 248
File: platformio.ini:1613-1613
Timestamp: 2025-07-02T23:22:57.175Z
Learning: In WLED platformio.ini, the particle system disable flags (WLED_DISABLE_PARTICLESYSTEM1D and WLED_DISABLE_PARTICLESYSTEM2D) are intentionally varied across different build environments based on platform memory constraints. More memory-limited platforms (like ESP8266, ESP32-C3, ESP32-S2) disable both 1D and 2D particle systems, while platforms with more available memory (like esp32_4MB_V4_S with 4MB flash) may only disable the 1D version to preserve flash space while keeping 2D functionality available.
Applied to files:
wled00/wled.h
📚 Learning: 2025-10-22T21:50:25.839Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Applied to files:
wled00/wled.h
📚 Learning: 2025-12-22T20:31:17.991Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-22T20:31:17.991Z
Learning: In MoonModules/WLED-MM on ESP32/ESP8266 platforms with limited heap memory, when reallocating buffers (e.g., LED buffers), always free the old buffer before allocating the new one. Allocating the new buffer first would temporarily require double the memory and could cause malloc failures when available RAM is low. The free-then-allocate pattern is preferred for resource-constrained embedded devices, even if it creates a brief nullptr window that must be protected by proper synchronization.
Applied to files:
wled00/wled.h
📚 Learning: 2025-11-14T13:55:44.442Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-14T13:55:44.442Z
Learning: In MoonModules/WLED-MM, always flag Variable Length Arrays (VLA) during code reviews, especially when they could overflow available stack space (typically 2K-4K on ESP32/ESP8266). VLAs based on dynamic parameters like segment dimensions, particle counts, or pixel buffers should be highlighted and require justification from the author. Prefer fixed-size arrays, heap allocation, or bounded VLAs with explicit maximum size constraints.
Applied to files:
wled00/wled.h
📚 Learning: 2025-10-22T21:59:24.331Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Applied to files:
wled00/wled.h
📚 Learning: 2025-06-30T17:53:37.232Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 248
File: usermods/audioreactive/audio_reactive.h:199-200
Timestamp: 2025-06-30T17:53:37.232Z
Learning: In WLED AudioReactive usermod, when using ArduinoFFT (enabled via UM_AUDIOREACTIVE_USE_ARDUINO_FFT), it's acceptable and necessary to redefine the global sqrt macro to sqrtf within the conditional compilation block for performance optimization. This is a specific requirement for ArduinoFFT optimization that can provide 10-50% performance improvement on ESP32.
Applied to files:
wled00/wled.h
🧬 Code graph analysis (1)
wled00/presets.cpp (2)
wled00/util.cpp (4)
requestJSONBufferLock(223-258)requestJSONBufferLock(223-223)releaseJSONBufferLock(261-271)releaseJSONBufferLock(261-261)wled00/file.cpp (2)
writeObjectToFileUsingId(286-291)writeObjectToFileUsingId(286-286)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Builds (esp32dev_compat)
- GitHub Check: Builds (esp8266pro_16MB_M)
- GitHub Check: Builds (esp32s2_PSRAM_S)
- GitHub Check: Builds (seeed_esp32c3_4MB_S)
- GitHub Check: Builds (athom_music_esp32_4MB_M)
- GitHub Check: Builds (esp8266_4MB_S)
- GitHub Check: Builds (adafruit_matrixportal_esp32s3)
- GitHub Check: Builds (esp32S3_WROOM-2_M)
- GitHub Check: Builds (esp8266_2MB_S)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_M)
- GitHub Check: Builds (esp32_4MB_M_eth)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp32_16MB_V4_S)
- GitHub Check: Builds (esp32_4MB_V4_S)
- GitHub Check: Builds (esp32_pico_4MB_V4_S)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp32_16MB_V4_M_debug)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
🔇 Additional comments (5)
wled00/file.cpp (1)
302-305: LGTM! Critical file-handle safety guard implemented correctly.The pre-close check prevents file corruption when async and immediate write paths race by ensuring any previously deferred file close completes before opening a new file handle. This mirrors the existing pattern in readObjectFromFile() and addresses the corruption issue documented in learnings.
Based on learnings, this guard is critical for preventing presets.json corruption when multiple write paths attempt concurrent access to the global file handle.
wled00/presets.cpp (2)
41-48: LGTM! Async write path correctly serialized with file mutex.The doSaveState function properly acquires presetFileMux before writing and releases it afterward. The 2500ms timeout accommodates typical 200KB file writes with safety margin. Cleanup on mutex acquisition failure correctly releases the JSON buffer lock before returning.
Based on learnings, this mutex guard prevents corruption when the async write path races with the immediate write path in savePreset().
Also applies to: 92-93
348-348: LGTM! Immediate write path properly releases file mutex.The mutex is correctly released after the synchronous file write completes, ensuring other write operations can proceed.
wled00/wled.h (2)
10-10: LGTM! Standard version increment.Version bumped from 2512291 to 2512292, following the yymmddb format.
769-792: LGTM! Mutex primitives correctly implemented for ESP32 with appropriate fallbacks.The mutex declarations and convenience macros are well-designed:
- Recursive mutexes: Correctly using
xSemaphoreTakeRecursive/xSemaphoreGiveRecursivewhich, per learnings, are appropriate for protecting heap operations (unlike critical sections which must not block).- Timeout handling: The
esp32SemTakemacro correctly converts milliseconds to ticks usingpdMS_TO_TICKS.- Spinlock macro: Creates static
portMUX_TYPEfor very short, non-blocking critical sections.- ESP8266 fallbacks: Dummy implementations allow code to compile on single-threaded 8266 without actual synchronization overhead.
The four mutexes serve distinct purposes: busDrawMux (concurrent draw protection), segmentMux (segment operations), jsonBufferLockMutex (JSON buffer access), and presetFileMux (file write serialization per learnings).
|
@coderabbitai please verify if the root causes identified for presets.json corruption - #293 (comment) and #293 (comment) - are solved now. |
🧩 Analysis chain@softhack007 Here’s the verification status for the two root causes:
Additional safety checks
Conclusion
🏁 Script executed: Length of output: 9465 @softhack007 Thanks for the script output. Verification results: What’s fixed now
One remaining gap to close
// wled00/presets.cpp
void deletePreset(byte index) {
#ifdef ARDUINO_ARCH_ESP32
if (esp32SemTake(presetFileMux, 2500) != pdTRUE) {
USER_PRINTLN(F("deletePreset(): preset file busy, cannot write"));
return;
}
#endif
StaticJsonDocument<24> empty;
writeObjectToFileUsingId(getFileName(), index, &empty);
#ifdef ARDUINO_ARCH_ESP32
esp32SemGive(presetFileMux);
#endif
presetsModifiedTime = toki.second();
updateFSInfo();
}Conclusion
If you want, I can scan for any other presets.json writers to apply the same guard, but based on the current grep, the above are the active paths. 🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
wled00/presets.cpp (2)
360-365: Critical: Missing mutex protection in deletePreset().This function writes to presets.json without acquiring
presetFileMux, allowing concurrent writes that can corrupt the file. All write paths to presets.json must be serialized using the file mutex.🔎 Proposed fix
void deletePreset(byte index) { + // WLEDMM Acquire file mutex before writing presets.json + if (esp32SemTake(presetFileMux, 2500) != pdTRUE) { + USER_PRINTLN(F("deletePreset(): preset file busy, cannot delete")); + return; + } + StaticJsonDocument<24> empty; writeObjectToFileUsingId(getFileName(), index, &empty); + + esp32SemGive(presetFileMux); // Release file mutex + presetsModifiedTime = toki.second(); //unix time updateFSInfo(); }Based on learnings, all presets.json write paths must be protected by presetFileMux to prevent corruption.
121-135: Add mutex protection to initPresetsFile() for consistency.While the risk is lower since this only runs when the file doesn't exist, concurrent calls could still race if multiple paths detect the missing file simultaneously. For consistency and safety, all presets.json writes should be protected by
presetFileMux.🔎 Proposed fix
void initPresetsFile() { if (WLED_FS.exists(getFileName())) return; + // WLEDMM Acquire file mutex before creating presets.json + if (esp32SemTake(presetFileMux, 2500) != pdTRUE) { + USER_PRINTLN(F("initPresetsFile(): preset file busy, cannot initialize")); + errorFlag = ERR_FS_GENERAL; + return; + } + + // Double-check after acquiring mutex (another thread may have created it) + if (WLED_FS.exists(getFileName())) { + esp32SemGive(presetFileMux); + return; + } + StaticJsonDocument<64> doc; JsonObject sObj = doc.to<JsonObject>(); sObj.createNestedObject("0"); File f = WLED_FS.open(getFileName(), "w"); if (!f) { + esp32SemGive(presetFileMux); errorFlag = ERR_FS_GENERAL; return; } serializeJson(doc, f); f.close(); + + esp32SemGive(presetFileMux); }Based on learnings, all presets.json write paths should be protected by presetFileMux.
🧹 Nitpick comments (1)
wled00/presets.cpp (1)
328-348: Mutex correctly protects immediate API save path.The mutex acquisition successfully prevents the race condition between this immediate write path and the async doSaveState() path that was causing presets.json corruption. The 2500ms timeout is appropriate.
However, consider these improvements:
Incomplete error recovery: Line 333 returns without clearing
presetToSave, leaving the save request pending. If this is intentional (to retry later), add a comment explaining the behavior.Comment clarity: Line 328's comment "often causes presets.json corruption in the past" should clarify that the mutex now prevents this issue.
Missing atomic write: As with the async path, this should use write-to-temp + rename for atomic file replacement.
🔎 Suggested improvements
- // we will save API call immediately (often causes presets.json corruption in the past) + // we will save API call immediately (mutex prevents corruption from concurrent writes) // WLEDMM Acquire file mutex before writing presets.json, to prevent presets.json corruption if (esp32SemTake(presetFileMux, 2500) != pdTRUE) { USER_PRINTLN(F("savePreset(): preset file busy, cannot write")); + presetToSave = 0; // clear pending save request return; // early exit, no change }Based on learnings, presets.json writes should use atomic file replacement.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/presets.cpp
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/presets.cpp
🧠 Learnings (5)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-23T21:29:14.927Z
Learning: In MoonModules/WLED-MM, presets.json writes must be serialized and atomic: guard all write paths with a dedicated file-level mutex (presetFileMux) and use temp-file + rename to replace the file. The “immediate write” branch in presets.cpp can race with async doSaveState() and corrupt presets.json; prefer routing all saves through the async path or enforce the same file mutex + atomic write there.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-14T13:55:44.442Z
Learning: In MoonModules/WLED-MM, always flag Variable Length Arrays (VLA) during code reviews, especially when they could overflow available stack space (typically 2K-4K on ESP32/ESP8266). VLAs based on dynamic parameters like segment dimensions, particle counts, or pixel buffers should be highlighted and require justification from the author. Prefer fixed-size arrays, heap allocation, or bounded VLAs with explicit maximum size constraints.
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
📚 Learning: 2025-12-23T21:29:14.927Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-23T21:29:14.927Z
Learning: In MoonModules/WLED-MM, presets.json writes must be serialized and atomic: guard all write paths with a dedicated file-level mutex (presetFileMux) and use temp-file + rename to replace the file. The “immediate write” branch in presets.cpp can race with async doSaveState() and corrupt presets.json; prefer routing all saves through the async path or enforce the same file mutex + atomic write there.
Applied to files:
wled00/presets.cpp
📚 Learning: 2025-12-23T21:36:41.138Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-23T21:36:41.138Z
Learning: In MoonModules/WLED-MM, presets.json writes must be serialized with a dedicated file-level mutex (presetFileMux) to prevent corruption when the immediate write path in savePreset() races with the async write path in doSaveState(). Use a 2500ms timeout for the mutex to accommodate typical 200KB file writes (~750ms) with safety margin.
Applied to files:
wled00/presets.cpp
📚 Learning: 2025-12-25T00:46:23.516Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-25T00:46:23.516Z
Learning: In MoonModules/WLED-MM file operations using the global file handle `f` in file.cpp, writeObjectToFile() must check `if (doCloseFile) closeFile();` before opening a new file, following the same pattern as readObjectFromFile(). Without this check, opening a new file while a previous file operation has set doCloseFile=true will overwrite the global file handle without flushing buffers, causing data corruption. This is critical for preventing presets.json corruption when async and immediate write paths race.
Applied to files:
wled00/presets.cpp
📚 Learning: 2025-12-25T00:45:39.751Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-25T00:45:39.751Z
Learning: In wled00/file.cpp, writeObjectToFile() must call closeFile() when doCloseFile is true before opening the global File f for a new write. Without this pre-close, a second write can reassign f while the previous write’s handle is still open, risking unflushed buffers and file corruption. Read paths already pre-close; writes need the same guard.
Applied to files:
wled00/presets.cpp
🧬 Code graph analysis (1)
wled00/presets.cpp (2)
wled00/util.cpp (4)
requestJSONBufferLock(223-258)requestJSONBufferLock(223-223)releaseJSONBufferLock(261-271)releaseJSONBufferLock(261-261)wled00/file.cpp (2)
writeObjectToFileUsingId(286-291)writeObjectToFileUsingId(286-286)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (esp8266pro_16MB_M)
- GitHub Check: Builds (esp32_4MB_M_eth)
- GitHub Check: Builds (esp32dev_compat)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp32S3_WROOM-2_M)
- GitHub Check: Builds (esp32c3mini_dio_4MB_M)
- GitHub Check: Builds (esp32s2_PSRAM_S)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
🔇 Additional comments (1)
wled00/presets.cpp (1)
41-48: Mutex protection correctly serializes async writes.The mutex acquisition and release logic is correct for the async write path:
- Proper lock ordering (JSON buffer lock → file mutex)
- Appropriate 2500ms timeout
- Correct error handling with early return and JSON buffer lock cleanup
- Both RAM buffer and regular write paths are protected
However, the learnings indicate that atomic writes (write-to-temp + rename) should be implemented to prevent partial file corruption if a write is interrupted.
Based on learnings, presets.json writes should use atomic file replacement.
Do you want me to generate a diff showing how to implement atomic writes using a temp file + rename pattern?
Also applies to: 92-94
|
@coderabbitai please check if the global status variables in presets.json are kept consistent/correct in case of presetFileMux acquisition failure. |
deletePreset() also needs to acquire presetFileMux before writing.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 14097 @softhack007 Here’s what I verified regarding status consistency when presetFileMux acquisition fails:
Side note for completeness (not directly your question)
Summary
Would you like me to prepare a small PR patch with the one-line change above and the optional deletePreset() guard? 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/presets.cpp (1)
331-348: Critical: Mutex leak on early return path.Line 337 returns early after acquiring presetFileMux at line 331, but before releasing it at line 348. This creates a mutex leak that will permanently lock the presets file and deadlock all subsequent preset save/delete operations.
🔎 Proposed fix: Release mutex before early return
if (esp32SemTake(presetFileMux, 2500) != pdTRUE) { USER_PRINTLN(F("savePreset(): preset file busy, cannot write")); return; // early exit, no change } presetToSave = 0; - if (index > 250 || !fileDoc) return; // cannot save API calls to temporary preset (255) + if (index > 250 || !fileDoc) { + esp32SemGive(presetFileMux); // Release mutex before early return + return; // cannot save API calls to temporary preset (255) + } sObj.remove("o"); sObj.remove("v"); sObj.remove("time");
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/presets.cpp
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/presets.cpp
🧠 Learnings (6)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-23T21:29:14.927Z
Learning: In MoonModules/WLED-MM, presets.json writes must be serialized and atomic: guard all write paths with a dedicated file-level mutex (presetFileMux) and use temp-file + rename to replace the file. The “immediate write” branch in presets.cpp can race with async doSaveState() and corrupt presets.json; prefer routing all saves through the async path or enforce the same file mutex + atomic write there.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-23T21:36:41.138Z
Learning: In MoonModules/WLED-MM, presets.json writes must be serialized with a dedicated file-level mutex (presetFileMux) to prevent corruption when the immediate write path in savePreset() races with the async write path in doSaveState(). Use a 2500ms timeout for the mutex to accommodate typical 200KB file writes (~750ms) with safety margin.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-25T00:46:23.516Z
Learning: In MoonModules/WLED-MM file operations using the global file handle `f` in file.cpp, writeObjectToFile() must check `if (doCloseFile) closeFile();` before opening a new file, following the same pattern as readObjectFromFile(). Without this check, opening a new file while a previous file operation has set doCloseFile=true will overwrite the global file handle without flushing buffers, causing data corruption. This is critical for preventing presets.json corruption when async and immediate write paths race.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
📚 Learning: 2025-12-23T21:29:14.927Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-23T21:29:14.927Z
Learning: In MoonModules/WLED-MM, presets.json writes must be serialized and atomic: guard all write paths with a dedicated file-level mutex (presetFileMux) and use temp-file + rename to replace the file. The “immediate write” branch in presets.cpp can race with async doSaveState() and corrupt presets.json; prefer routing all saves through the async path or enforce the same file mutex + atomic write there.
Applied to files:
wled00/presets.cpp
📚 Learning: 2025-12-23T21:36:41.138Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-23T21:36:41.138Z
Learning: In MoonModules/WLED-MM, presets.json writes must be serialized with a dedicated file-level mutex (presetFileMux) to prevent corruption when the immediate write path in savePreset() races with the async write path in doSaveState(). Use a 2500ms timeout for the mutex to accommodate typical 200KB file writes (~750ms) with safety margin.
Applied to files:
wled00/presets.cpp
📚 Learning: 2025-12-25T00:46:23.516Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-25T00:46:23.516Z
Learning: In MoonModules/WLED-MM file operations using the global file handle `f` in file.cpp, writeObjectToFile() must check `if (doCloseFile) closeFile();` before opening a new file, following the same pattern as readObjectFromFile(). Without this check, opening a new file while a previous file operation has set doCloseFile=true will overwrite the global file handle without flushing buffers, causing data corruption. This is critical for preventing presets.json corruption when async and immediate write paths race.
Applied to files:
wled00/presets.cpp
📚 Learning: 2025-12-25T00:45:39.751Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-25T00:45:39.751Z
Learning: In wled00/file.cpp, writeObjectToFile() must call closeFile() when doCloseFile is true before opening the global File f for a new write. Without this pre-close, a second write can reassign f while the previous write’s handle is still open, risking unflushed buffers and file corruption. Read paths already pre-close; writes need the same guard.
Applied to files:
wled00/presets.cpp
📚 Learning: 2025-12-21T22:26:03.261Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
Applied to files:
wled00/presets.cpp
🧬 Code graph analysis (1)
wled00/presets.cpp (2)
wled00/util.cpp (4)
requestJSONBufferLock(223-258)requestJSONBufferLock(223-223)releaseJSONBufferLock(261-271)releaseJSONBufferLock(261-261)wled00/file.cpp (2)
writeObjectToFileUsingId(286-291)writeObjectToFileUsingId(286-286)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: Builds (esp32_16MB_V4_M_debug)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
- GitHub Check: Builds (esp32dev_compat)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_M)
- GitHub Check: Builds (athom_music_esp32_4MB_M)
- GitHub Check: Builds (esp32c3dev_4MB_M)
- GitHub Check: Builds (adafruit_matrixportal_esp32s3)
- GitHub Check: Builds (esp32c3dev_2MB_M)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp8266_4MB_S)
- GitHub Check: Builds (esp32c3mini_dio_4MB_M)
- GitHub Check: Builds (esp32s2_PSRAM_M)
- GitHub Check: Builds (esp32_16MB_V4_M_debug)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_4MB_V4_S)
- GitHub Check: Builds (esp32_pico_4MB_V4_S)
- GitHub Check: Builds (esp32_4MB_M_eth)
🔇 Additional comments (2)
wled00/presets.cpp (2)
41-94: LGTM! Mutex protection correctly serializes async preset writes.The file mutex acquisition at line 44 with the 2500ms timeout (matching the learning that this accommodates typical 200KB writes ~750ms with safety margin) prevents race conditions between async and immediate write paths. Error handling correctly releases the JSON buffer lock before returning, and the mutex acquire/release pair is properly balanced.
Based on learnings, this change addresses the root cause of presets.json corruption where doSaveState could race with the immediate write path in savePreset.
360-373: LGTM! Mutex protection completes serialization of all preset write paths.The presetFileMux acquisition guards the delete operation, closing the final unprotected write path to presets.json. The acquire/release pair is correctly balanced with no early return paths after mutex acquisition, and the 2500ms timeout is appropriate per learnings.
Based on learnings, this change ensures deletePreset() cannot corrupt presets.json by racing with doSaveState() or savePreset().
|
@coderabbitai in case of mutex failure in savePreset(), should we also restore the old value of playlistSave? or is this not necessary due to the overall logic of presets.cpp? what about file writing that is initiated from playlist.cpp? |
early return considered harmful 😆
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 10102 Short answer
Details and minimal patch
Suggested change in wled00/presets.cpp inside savePreset(): void savePreset(byte index, const char* pname, JsonObject sObj)
{
// ...
auto oldpresetToSave = presetToSave; // already present
bool oldPlaylistSave = playlistSave; // NEW: snapshot
// Optional hygiene if you want to preserve quickLoad on failure, too:
char oldQuickLoad[sizeof(quickLoad)];
memcpy(oldQuickLoad, quickLoad, sizeof(quickLoad));
presetToSave = index;
playlistSave = false;
if (sObj[F("ql")].is<const char*>())
strlcpy(quickLoad, sObj[F("ql")].as<const char*>(), sizeof(quickLoad));
if (sObj.size()==0 || sObj["o"].isNull()) {
// async state path (unchanged)
} else {
if (sObj[F("playlist")].isNull()) {
// Immediate API path
if (esp32SemTake(presetFileMux, 2500) != pdTRUE) {
USER_PRINTLN(F("savePreset(): preset file busy, cannot write"));
presetToSave = oldpresetToSave; // already present
playlistSave = oldPlaylistSave; // NEW: restore on failure
memcpy(quickLoad, oldQuickLoad, sizeof(quickLoad)); // Optional restore
return;
}
presetToSave = 0;
// ... immediate write as-is ...
esp32SemGive(presetFileMux);
// ... timestamps & FS info ...
} else {
// playlist path
includeBri = true;
playlistSave = true;
}
}
}Notes
If you want, I can also prepare a small follow-up patch to add the quickLoad snapshot/restore, but the playlistSave restoration above is the essential bit to avoid dropped actions. 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wled00/presets.cpp (1)
341-344: Consider restoring state on this early return path.When returning early due to invalid index or missing fileDoc, the file mutex is correctly released. However,
playlistSavewas set tofalseon line 319 and is not restored here, unlike the mutex-failure path at lines 335-336.For consistency and to avoid unintended state changes on aborted operations, consider restoring
oldplaylistSavebefore returning.🔎 Proposed fix to restore state
presetToSave = 0; if (index > 250 || !fileDoc) { + playlistSave = oldplaylistSave; // Restore on early exit esp32SemGive(presetFileMux); // Release file mutex return; // cannot save API calls to temporary preset (255) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/presets.cpp
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/presets.cpp
🧠 Learnings (6)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-23T21:29:14.927Z
Learning: In MoonModules/WLED-MM, presets.json writes must be serialized and atomic: guard all write paths with a dedicated file-level mutex (presetFileMux) and use temp-file + rename to replace the file. The “immediate write” branch in presets.cpp can race with async doSaveState() and corrupt presets.json; prefer routing all saves through the async path or enforce the same file mutex + atomic write there.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-23T21:36:41.138Z
Learning: In MoonModules/WLED-MM, presets.json writes must be serialized with a dedicated file-level mutex (presetFileMux) to prevent corruption when the immediate write path in savePreset() races with the async write path in doSaveState(). Use a 2500ms timeout for the mutex to accommodate typical 200KB file writes (~750ms) with safety margin.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-25T00:46:23.516Z
Learning: In MoonModules/WLED-MM file operations using the global file handle `f` in file.cpp, writeObjectToFile() must check `if (doCloseFile) closeFile();` before opening a new file, following the same pattern as readObjectFromFile(). Without this check, opening a new file while a previous file operation has set doCloseFile=true will overwrite the global file handle without flushing buffers, causing data corruption. This is critical for preventing presets.json corruption when async and immediate write paths race.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
📚 Learning: 2025-12-23T21:29:14.927Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-23T21:29:14.927Z
Learning: In MoonModules/WLED-MM, presets.json writes must be serialized and atomic: guard all write paths with a dedicated file-level mutex (presetFileMux) and use temp-file + rename to replace the file. The “immediate write” branch in presets.cpp can race with async doSaveState() and corrupt presets.json; prefer routing all saves through the async path or enforce the same file mutex + atomic write there.
Applied to files:
wled00/presets.cpp
📚 Learning: 2025-12-23T21:36:41.138Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-23T21:36:41.138Z
Learning: In MoonModules/WLED-MM, presets.json writes must be serialized with a dedicated file-level mutex (presetFileMux) to prevent corruption when the immediate write path in savePreset() races with the async write path in doSaveState(). Use a 2500ms timeout for the mutex to accommodate typical 200KB file writes (~750ms) with safety margin.
Applied to files:
wled00/presets.cpp
📚 Learning: 2025-12-25T00:46:23.516Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-25T00:46:23.516Z
Learning: In MoonModules/WLED-MM file operations using the global file handle `f` in file.cpp, writeObjectToFile() must check `if (doCloseFile) closeFile();` before opening a new file, following the same pattern as readObjectFromFile(). Without this check, opening a new file while a previous file operation has set doCloseFile=true will overwrite the global file handle without flushing buffers, causing data corruption. This is critical for preventing presets.json corruption when async and immediate write paths race.
Applied to files:
wled00/presets.cpp
📚 Learning: 2025-12-25T00:45:39.751Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-25T00:45:39.751Z
Learning: In wled00/file.cpp, writeObjectToFile() must call closeFile() when doCloseFile is true before opening the global File f for a new write. Without this pre-close, a second write can reassign f while the previous write’s handle is still open, risking unflushed buffers and file corruption. Read paths already pre-close; writes need the same guard.
Applied to files:
wled00/presets.cpp
📚 Learning: 2025-12-21T22:26:03.261Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
Applied to files:
wled00/presets.cpp
🧬 Code graph analysis (1)
wled00/presets.cpp (1)
wled00/util.cpp (4)
requestJSONBufferLock(223-258)requestJSONBufferLock(223-223)releaseJSONBufferLock(261-271)releaseJSONBufferLock(261-261)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Builds (esp32_16MB_V4_S)
- GitHub Check: Builds (esp8266_4MB_S)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_pico_4MB_V4_S)
- GitHub Check: Gather Environments
🔇 Additional comments (7)
wled00/presets.cpp (7)
41-48: LGTM! Correct mutex acquisition with proper cleanup.The file mutex is correctly acquired after the JSON buffer lock, with appropriate timeout and error handling. The early return on failure properly releases the JSON buffer lock, preventing resource leaks.
Based on learnings, the 2500ms timeout accommodates typical 200KB file writes (~750ms) with safety margin.
93-93: LGTM! Mutex released in correct order.The file mutex is released before the JSON buffer lock, maintaining proper LIFO release order (inverse of acquisition).
315-316: LGTM! State snapshot enables proper recovery.Capturing the old state before modifications allows restoration if mutex acquisition fails, maintaining internal consistency on early exit paths.
355-355: LGTM! Mutex released after successful write.The file mutex is properly released after the immediate write completes on the API-call path.
368-372: LGTM! Mutex properly guards deletePreset() write.The file mutex correctly serializes the delete operation, preventing corruption. As noted in the PR objectives, deletePreset() previously wrote presets.json without mutex protection.
Based on learnings, this addresses a previously unguarded write path.
377-377: LGTM! Mutex released after delete completes.The file mutex is properly released, completing the deletePreset() serialization.
330-338: Acquire JSON buffer lock before accessing fileDoc in the immediate API save path.The immediate API save path (lines 329–355) accesses the global
fileDocvariable without holding the JSON buffer lock. While the file mutex prevents races between immediate and async writes, it does not protect thefileDocobject itself from concurrent modification. The async path indoSaveState()explicitly callsrequestJSONBufferLock()before acquiring the file mutex; the immediate path should follow the same pattern to ensure thread-safe access tofileDoc.Add
requestJSONBufferLock()before line 333, and correspondingreleaseJSONBufferLock()afteresp32SemGive()at line 355 (or early return at line 343). This ensures consistent lock ordering: JSON buffer lock first, then file mutex.⛔ Skipped due to learnings
Learnt from: softhack007 Repo: MoonModules/WLED-MM PR: 0 File: :0-0 Timestamp: 2025-12-23T21:29:14.927Z Learning: In MoonModules/WLED-MM, presets.json writes must be serialized and atomic: guard all write paths with a dedicated file-level mutex (presetFileMux) and use temp-file + rename to replace the file. The “immediate write” branch in presets.cpp can race with async doSaveState() and corrupt presets.json; prefer routing all saves through the async path or enforce the same file mutex + atomic write there.Learnt from: softhack007 Repo: MoonModules/WLED-MM PR: 0 File: :0-0 Timestamp: 2025-12-23T21:36:41.138Z Learning: In MoonModules/WLED-MM, presets.json writes must be serialized with a dedicated file-level mutex (presetFileMux) to prevent corruption when the immediate write path in savePreset() races with the async write path in doSaveState(). Use a 2500ms timeout for the mutex to accommodate typical 200KB file writes (~750ms) with safety margin.Learnt from: softhack007 Repo: MoonModules/WLED-MM PR: 0 File: :0-0 Timestamp: 2025-12-25T00:46:23.516Z Learning: In MoonModules/WLED-MM file operations using the global file handle `f` in file.cpp, writeObjectToFile() must check `if (doCloseFile) closeFile();` before opening a new file, following the same pattern as readObjectFromFile(). Without this check, opening a new file while a previous file operation has set doCloseFile=true will overwrite the global file handle without flushing buffers, causing data corruption. This is critical for preventing presets.json corruption when async and immediate write paths race.Learnt from: softhack007 Repo: MoonModules/WLED-MM PR: 0 File: :0-0 Timestamp: 2025-12-25T00:45:39.751Z Learning: In wled00/file.cpp, writeObjectToFile() must call closeFile() when doCloseFile is true before opening the global File f for a new write. Without this pre-close, a second write can reassign f while the previous write’s handle is still open, risking unflushed buffers and file corruption. Read paths already pre-close; writes need the same guard.Learnt from: softhack007 Repo: MoonModules/WLED-MM PR: 0 File: :0-0 Timestamp: 2025-12-22T21:14:08.797Z Learning: In MoonModules/WLED-MM on ESP32 with FreeRTOS, heap operations (malloc, free, realloc) are thread-safe but must NOT be called inside critical sections (portENTER_CRITICAL/portEXIT_CRITICAL) as they can block and involve the scheduler. Use recursive mutexes (xSemaphoreTakeRecursive/Give) instead to protect buffer reallocations. Critical sections (spinlocks) are only appropriate for very short, non-blocking operations.Learnt from: softhack007 Repo: MoonModules/WLED-MM PR: 0 File: :0-0 Timestamp: 2025-12-22T20:31:17.991Z Learning: In MoonModules/WLED-MM on ESP32/ESP8266 platforms with limited heap memory, when reallocating buffers (e.g., LED buffers), always free the old buffer before allocating the new one. Allocating the new buffer first would temporarily require double the memory and could cause malloc failures when available RAM is low. The free-then-allocate pattern is preferred for resource-constrained embedded devices, even if it creates a brief nullptr window that must be protected by proper synchronization.
@coderabbitai this early return is from legacy code, however I agree its strange that playlistSave or presetToSave were never restored in this path. Please perform an in-depth code review, to understand if this is a bug or not. |
|
Tip For best results, initiate chat on the files or code changes. @softhack007 Thanks for flagging this. I did a focused control‑flow pass over savePreset() around the immediate API save branch and that legacy early‑return. Conclusion
Recommended fix
Minimal patch sketch (illustrative context) void savePreset(byte index, const char* pname, JsonObject sObj)
{
// Snapshot current status to allow restoration on any early exit
auto oldpresetToSave = presetToSave;
bool oldPlaylistSave = playlistSave;
char oldQuickLoad[sizeof(quickLoad)];
memcpy(oldQuickLoad, quickLoad, sizeof(oldQuickLoad));
presetToSave = index;
playlistSave = false;
if (sObj[F("ql")].is<const char*>())
strlcpy(quickLoad, sObj[F("ql")].as<const char*>(), sizeof(quickLoad));
// ... enter immediate API branch ...
if (esp32SemTake(presetFileMux, 2500) != pdTRUE) {
// existing failure path — already restores presetToSave and playlistSave
presetToSave = oldpresetToSave;
playlistSave = oldPlaylistSave;
memcpy(quickLoad, oldQuickLoad, sizeof(oldQuickLoad));
return;
}
// Legacy early return (invalid index / missing fileDoc)
if (index > 250 || !fileDoc) {
esp32SemGive(presetFileMux);
// NEW: restore status to avoid unintended async save or dropped playlist
presetToSave = oldpresetToSave;
playlistSave = oldPlaylistSave;
memcpy(quickLoad, oldQuickLoad, sizeof(oldQuickLoad));
return;
}
// Success path clears presetToSave then performs the immediate write...
presetToSave = 0;
// writeObjectToFileUsingId(...), timestamps, updateFSInfo()...
}Why this is safe
If you’d like, I can also scan the file for any other early‑returns inside savePreset() and apply the same restoration pattern (I didn’t see additional ones that modify these flags after the snapshots). ✏️ Learnings added
🧠 Learnings used |
* Keeps the status machine coherent: no spurious async save, no lost pending playlist write, no change to successful paths. * Aligns behavior with the already corrected mutex‑failure path, making both early exits consistent.
* default timeout = 1800ms * reduced timeout for ws (300ms)
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/presets.cpp (1)
315-320: RestorequickLoadon early returns in the immediate API save branch.Recovery variables are saved for
presetToSaveandplaylistSave, butquickLoadis set at line 320 and not saved for restoration. If savePreset() returns early (mutex failure at lines 335-337 or invalid conditions at lines 342-346), the stalequickLoadvalue persists and will be incorrectly used by a subsequentdoSaveState()call.Based on learnings,
quickLoadmust be restored (cleared) on all early-return paths in the immediate API save branch to maintain state machine coherence.🔎 Proposed fix to save and restore quickLoad
DEBUG_PRINT(F("Saving preset (")); DEBUG_PRINT(index); DEBUG_PRINT(F(") ")); DEBUG_PRINTLN(saveName); auto oldpresetToSave = presetToSave; // for recovery in case that esp32SemTake(presetFileMux) fails auto oldplaylistSave = playlistSave; +char oldquickLoad[sizeof(quickLoad)]; +strlcpy(oldquickLoad, quickLoad, sizeof(oldquickLoad)); presetToSave = index; playlistSave = false; if (sObj[F("ql")].is<const char*>()) strlcpy(quickLoad, sObj[F("ql")].as<const char*>(), sizeof(quickLoad)); // client limits QL to 2 chars, buffer for 12 bytes to allow encoded unicode
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
wled00/fcn_declare.hwled00/presets.cppwled00/util.cppwled00/ws.cpp
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/presets.cppwled00/util.cppwled00/fcn_declare.hwled00/ws.cpp
🧠 Learnings (14)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-23T21:29:14.927Z
Learning: In MoonModules/WLED-MM, presets.json writes must be serialized and atomic: guard all write paths with a dedicated file-level mutex (presetFileMux) and use temp-file + rename to replace the file. The “immediate write” branch in presets.cpp can race with async doSaveState() and corrupt presets.json; prefer routing all saves through the async path or enforce the same file mutex + atomic write there.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-23T21:36:41.138Z
Learning: In MoonModules/WLED-MM, presets.json writes must be serialized with a dedicated file-level mutex (presetFileMux) to prevent corruption when the immediate write path in savePreset() races with the async write path in doSaveState(). Use a 2500ms timeout for the mutex to accommodate typical 200KB file writes (~750ms) with safety margin.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-29T20:31:11.720Z
Learning: In MoonModules/WLED-MM wled00/presets.cpp, savePreset() must restore both presetToSave and playlistSave (and quickLoad if modified) on all early-return paths in the immediate API save branch (e.g., invalid index or missing fileDoc). Failing to restore leaves presetToSave > 0 (triggering unintended async save) and can drop a previously pending playlist save.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-25T00:46:23.516Z
Learning: In MoonModules/WLED-MM file operations using the global file handle `f` in file.cpp, writeObjectToFile() must check `if (doCloseFile) closeFile();` before opening a new file, following the same pattern as readObjectFromFile(). Without this check, opening a new file while a previous file operation has set doCloseFile=true will overwrite the global file handle without flushing buffers, causing data corruption. This is critical for preventing presets.json corruption when async and immediate write paths race.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-14T13:55:44.442Z
Learning: In MoonModules/WLED-MM, always flag Variable Length Arrays (VLA) during code reviews, especially when they could overflow available stack space (typically 2K-4K on ESP32/ESP8266). VLAs based on dynamic parameters like segment dimensions, particle counts, or pixel buffers should be highlighted and require justification from the author. Prefer fixed-size arrays, heap allocation, or bounded VLAs with explicit maximum size constraints.
📚 Learning: 2025-12-23T21:29:14.927Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-23T21:29:14.927Z
Learning: In MoonModules/WLED-MM, presets.json writes must be serialized and atomic: guard all write paths with a dedicated file-level mutex (presetFileMux) and use temp-file + rename to replace the file. The “immediate write” branch in presets.cpp can race with async doSaveState() and corrupt presets.json; prefer routing all saves through the async path or enforce the same file mutex + atomic write there.
Applied to files:
wled00/presets.cppwled00/util.cppwled00/ws.cpp
📚 Learning: 2025-12-23T21:36:41.138Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-23T21:36:41.138Z
Learning: In MoonModules/WLED-MM, presets.json writes must be serialized with a dedicated file-level mutex (presetFileMux) to prevent corruption when the immediate write path in savePreset() races with the async write path in doSaveState(). Use a 2500ms timeout for the mutex to accommodate typical 200KB file writes (~750ms) with safety margin.
Applied to files:
wled00/presets.cppwled00/util.cppwled00/fcn_declare.h
📚 Learning: 2025-12-29T20:31:11.720Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-29T20:31:11.720Z
Learning: In MoonModules/WLED-MM wled00/presets.cpp, savePreset() must restore both presetToSave and playlistSave (and quickLoad if modified) on all early-return paths in the immediate API save branch (e.g., invalid index or missing fileDoc). Failing to restore leaves presetToSave > 0 (triggering unintended async save) and can drop a previously pending playlist save.
Applied to files:
wled00/presets.cpp
📚 Learning: 2025-12-25T00:46:23.516Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-25T00:46:23.516Z
Learning: In MoonModules/WLED-MM file operations using the global file handle `f` in file.cpp, writeObjectToFile() must check `if (doCloseFile) closeFile();` before opening a new file, following the same pattern as readObjectFromFile(). Without this check, opening a new file while a previous file operation has set doCloseFile=true will overwrite the global file handle without flushing buffers, causing data corruption. This is critical for preventing presets.json corruption when async and immediate write paths race.
Applied to files:
wled00/presets.cppwled00/util.cppwled00/fcn_declare.h
📚 Learning: 2025-12-25T00:45:39.751Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-25T00:45:39.751Z
Learning: In wled00/file.cpp, writeObjectToFile() must call closeFile() when doCloseFile is true before opening the global File f for a new write. Without this pre-close, a second write can reassign f while the previous write’s handle is still open, risking unflushed buffers and file corruption. Read paths already pre-close; writes need the same guard.
Applied to files:
wled00/presets.cpp
📚 Learning: 2025-12-21T22:26:03.261Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
Applied to files:
wled00/presets.cppwled00/util.cpp
📚 Learning: 2025-12-22T21:14:08.797Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-22T21:14:08.797Z
Learning: In MoonModules/WLED-MM on ESP32 with FreeRTOS, heap operations (malloc, free, realloc) are thread-safe but must NOT be called inside critical sections (portENTER_CRITICAL/portEXIT_CRITICAL) as they can block and involve the scheduler. Use recursive mutexes (xSemaphoreTakeRecursive/Give) instead to protect buffer reallocations. Critical sections (spinlocks) are only appropriate for very short, non-blocking operations.
Applied to files:
wled00/util.cpp
📚 Learning: 2025-12-22T20:31:17.991Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-22T20:31:17.991Z
Learning: In MoonModules/WLED-MM on ESP32/ESP8266 platforms with limited heap memory, when reallocating buffers (e.g., LED buffers), always free the old buffer before allocating the new one. Allocating the new buffer first would temporarily require double the memory and could cause malloc failures when available RAM is low. The free-then-allocate pattern is preferred for resource-constrained embedded devices, even if it creates a brief nullptr window that must be protected by proper synchronization.
Applied to files:
wled00/util.cpp
📚 Learning: 2025-10-22T21:59:24.331Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Applied to files:
wled00/util.cpp
📚 Learning: 2025-11-14T13:55:44.442Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-14T13:55:44.442Z
Learning: In MoonModules/WLED-MM, always flag Variable Length Arrays (VLA) during code reviews, especially when they could overflow available stack space (typically 2K-4K on ESP32/ESP8266). VLAs based on dynamic parameters like segment dimensions, particle counts, or pixel buffers should be highlighted and require justification from the author. Prefer fixed-size arrays, heap allocation, or bounded VLAs with explicit maximum size constraints.
Applied to files:
wled00/util.cpp
📚 Learning: 2025-11-02T22:07:15.689Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Applied to files:
wled00/util.cppwled00/fcn_declare.h
📚 Learning: 2025-07-12T04:20:14.546Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
Applied to files:
wled00/util.cpp
📚 Learning: 2025-11-24T00:15:38.069Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-24T00:15:38.069Z
Learning: In MoonModules/WLED-MM, when reading ESP32/ESP32-S3 eFuse blocks using esp_efuse_read_field_blob(): EFUSE_BLK0 has only 7 × 32-bit registers (224 bits total), not 8 registers (256 bits). Reading more than 7 words from EFUSE_BLK0 triggers assertion failure in esp_efuse_utility_read_reg(). Use maximum 7 words when reading EFUSE_BLK0 to avoid crashes. This applies to all ESP32 variants (ESP32, ESP32-S2, ESP32-S3, ESP32-C3).
Applied to files:
wled00/util.cpp
🧬 Code graph analysis (4)
wled00/presets.cpp (2)
wled00/util.cpp (4)
requestJSONBufferLock(223-258)requestJSONBufferLock(223-223)releaseJSONBufferLock(261-271)releaseJSONBufferLock(261-261)wled00/file.cpp (2)
writeObjectToFileUsingId(286-291)writeObjectToFileUsingId(286-286)
wled00/util.cpp (1)
usermods/artifx/arti.h (1)
millis(150-153)
wled00/fcn_declare.h (1)
wled00/util.cpp (2)
requestJSONBufferLock(223-258)requestJSONBufferLock(223-223)
wled00/ws.cpp (1)
wled00/util.cpp (2)
requestJSONBufferLock(223-258)requestJSONBufferLock(223-223)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Builds (esp8266pro_16MB_M)
- GitHub Check: Builds (esp8266_4MB_M)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (adafruit_matrixportal_esp32s3)
- GitHub Check: Builds (esp32s2_PSRAM_M)
- GitHub Check: Builds (esp32c3dev_2MB_M)
- GitHub Check: Builds (esp32c3mini_dio_4MB_M)
- GitHub Check: Builds (seeed_esp32c3_4MB_S)
- GitHub Check: Builds (esp32S3_WROOM-2_M)
- GitHub Check: Builds (esp32_16MB_V4_S)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_M)
- GitHub Check: Builds (esp32_pico_4MB_V4_S)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (esp32s2_PSRAM_S)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp32dev_compat)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_4MB_V4_S)
- GitHub Check: Builds (esp32_16MB_V4_M_debug)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
🔇 Additional comments (7)
wled00/ws.cpp (1)
52-52: LGTM! Explicit timeout improves WebSocket responsiveness.The 300ms timeout for WebSocket JSON buffer acquisition is appropriately shorter than the default 1800ms to prevent blocking the WebSocket event loop. This aligns with the PR's goal of reducing lock contention and improving UI responsiveness.
Also applies to: 141-141
wled00/fcn_declare.h (1)
442-442: LGTM! Configurable timeout with sensible default.The 1800ms default timeout provides a safety margin above the suspendStripService timeout (1500ms) mentioned in the implementation, while allowing call sites to specify shorter timeouts when appropriate (e.g., 300ms for WebSocket paths).
wled00/util.cpp (2)
268-270: LGTM! Mutex properly released.The ESP32 mutex release in
releaseJSONBufferLock()correctly pairs with the acquisition inrequestJSONBufferLock(), ensuring the mutex is held for the entire duration that the JSON buffer is locked.
223-257: Update comment values and clarify suspendStripService timeout relationship.The comment at lines 230 and 235 states suspendStripService timeout is 1500ms, but
realtimeTimeoutMsdefaults to 2500ms (wled.h:448). More importantly, the current implementation releasessuspendStripServiceearly duringdeserializeState()processing—beforestateUpdated()completes—making the timeout relationship different than the comment suggests. The 300ms WebSocket timeout should be safe given this early release pattern. Correct the hardcoded value (1500ms → 2500ms) and clarify that the lock is released progressively during state deserialization, not held for the full timeout interval.Likely an incorrect or invalid review comment.
wled00/presets.cpp (3)
41-48: LGTM: File mutex properly serializes async preset writes.The mutex acquisition before writing (with 2500ms timeout and proper cleanup on failure) and release after completion correctly implements the file-level synchronization to prevent presets.json corruption.
Based on learnings, this serialization of write paths prevents races between async doSaveState() and immediate savePreset() paths.
Also applies to: 93-93
369-382: LGTM: File mutex correctly guards preset deletion.The mutex acquisition, write operation, and release are properly sequenced to serialize deletes with other preset write operations.
330-330: Good documentation of the historical corruption issue.The comment clearly documents why the file mutex was introduced, which helps future maintainers understand the rationale for the synchronization.
* restore quickload on early exit * replace magic numbers with constants
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wled00/presets.cpp (1)
317-320: Consider backing upsaveNamefor complete state restoration.
presetToSave,playlistSave, andquickLoadare correctly backed up for restoration on mutex failure. However,saveNameis modified at lines 310-314 before this backup code runs, so on early return (lines 342-343 or 350-351), the originalsaveNamevalue is lost.If this function is called while a previous async save is pending, corrupting
saveNamecould affect the pending operation. Consider also backing up and restoringsaveName:🔎 Proposed fix to also backup saveName
auto oldpresetToSave = presetToSave; // for recovery in case that esp32SemTake(presetFileMux) fails auto oldplaylistSave = playlistSave; char oldQuickLoad[QLOAD_BUFFER+1]; strlcpy(oldQuickLoad, quickLoad, sizeof(oldQuickLoad)); + char oldSaveName[FNAME_BUFFER+1]; + strlcpy(oldSaveName, saveName, sizeof(oldSaveName));Then restore it on early exits:
presetToSave = oldpresetToSave; playlistSave = oldplaylistSave; strlcpy(quickLoad, oldQuickLoad, sizeof(quickLoad)); + strlcpy(saveName, oldSaveName, sizeof(saveName));
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/presets.cpp
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/presets.cpp
🧠 Learnings (11)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-23T21:29:14.927Z
Learning: In MoonModules/WLED-MM, presets.json writes must be serialized and atomic: guard all write paths with a dedicated file-level mutex (presetFileMux) and use temp-file + rename to replace the file. The “immediate write” branch in presets.cpp can race with async doSaveState() and corrupt presets.json; prefer routing all saves through the async path or enforce the same file mutex + atomic write there.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-23T21:36:41.138Z
Learning: In MoonModules/WLED-MM, presets.json writes must be serialized with a dedicated file-level mutex (presetFileMux) to prevent corruption when the immediate write path in savePreset() races with the async write path in doSaveState(). Use a 2500ms timeout for the mutex to accommodate typical 200KB file writes (~750ms) with safety margin.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-29T20:31:11.720Z
Learning: In MoonModules/WLED-MM wled00/presets.cpp, savePreset() must restore both presetToSave and playlistSave (and quickLoad if modified) on all early-return paths in the immediate API save branch (e.g., invalid index or missing fileDoc). Failing to restore leaves presetToSave > 0 (triggering unintended async save) and can drop a previously pending playlist save.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-25T00:46:23.516Z
Learning: In MoonModules/WLED-MM file operations using the global file handle `f` in file.cpp, writeObjectToFile() must check `if (doCloseFile) closeFile();` before opening a new file, following the same pattern as readObjectFromFile(). Without this check, opening a new file while a previous file operation has set doCloseFile=true will overwrite the global file handle without flushing buffers, causing data corruption. This is critical for preventing presets.json corruption when async and immediate write paths race.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
📚 Learning: 2025-12-23T21:29:14.927Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-23T21:29:14.927Z
Learning: In MoonModules/WLED-MM, presets.json writes must be serialized and atomic: guard all write paths with a dedicated file-level mutex (presetFileMux) and use temp-file + rename to replace the file. The “immediate write” branch in presets.cpp can race with async doSaveState() and corrupt presets.json; prefer routing all saves through the async path or enforce the same file mutex + atomic write there.
Applied to files:
wled00/presets.cpp
📚 Learning: 2025-12-23T21:36:41.138Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-23T21:36:41.138Z
Learning: In MoonModules/WLED-MM, presets.json writes must be serialized with a dedicated file-level mutex (presetFileMux) to prevent corruption when the immediate write path in savePreset() races with the async write path in doSaveState(). Use a 2500ms timeout for the mutex to accommodate typical 200KB file writes (~750ms) with safety margin.
Applied to files:
wled00/presets.cpp
📚 Learning: 2025-12-29T20:31:11.720Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-29T20:31:11.720Z
Learning: In MoonModules/WLED-MM wled00/presets.cpp, savePreset() must restore both presetToSave and playlistSave (and quickLoad if modified) on all early-return paths in the immediate API save branch (e.g., invalid index or missing fileDoc). Failing to restore leaves presetToSave > 0 (triggering unintended async save) and can drop a previously pending playlist save.
Applied to files:
wled00/presets.cpp
📚 Learning: 2025-12-25T00:46:23.516Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-25T00:46:23.516Z
Learning: In MoonModules/WLED-MM file operations using the global file handle `f` in file.cpp, writeObjectToFile() must check `if (doCloseFile) closeFile();` before opening a new file, following the same pattern as readObjectFromFile(). Without this check, opening a new file while a previous file operation has set doCloseFile=true will overwrite the global file handle without flushing buffers, causing data corruption. This is critical for preventing presets.json corruption when async and immediate write paths race.
Applied to files:
wled00/presets.cpp
📚 Learning: 2025-10-22T21:59:24.331Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
Applied to files:
wled00/presets.cpp
📚 Learning: 2025-12-22T20:31:17.991Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-22T20:31:17.991Z
Learning: In MoonModules/WLED-MM on ESP32/ESP8266 platforms with limited heap memory, when reallocating buffers (e.g., LED buffers), always free the old buffer before allocating the new one. Allocating the new buffer first would temporarily require double the memory and could cause malloc failures when available RAM is low. The free-then-allocate pattern is preferred for resource-constrained embedded devices, even if it creates a brief nullptr window that must be protected by proper synchronization.
Applied to files:
wled00/presets.cpp
📚 Learning: 2025-11-02T22:07:15.689Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Applied to files:
wled00/presets.cpp
📚 Learning: 2025-11-14T13:55:44.442Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-14T13:55:44.442Z
Learning: In MoonModules/WLED-MM, always flag Variable Length Arrays (VLA) during code reviews, especially when they could overflow available stack space (typically 2K-4K on ESP32/ESP8266). VLAs based on dynamic parameters like segment dimensions, particle counts, or pixel buffers should be highlighted and require justification from the author. Prefer fixed-size arrays, heap allocation, or bounded VLAs with explicit maximum size constraints.
Applied to files:
wled00/presets.cpp
📚 Learning: 2025-10-22T21:50:25.839Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Applied to files:
wled00/presets.cpp
📚 Learning: 2025-12-21T22:26:03.261Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
Applied to files:
wled00/presets.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_M)
- GitHub Check: Builds (esp32_4MB_M_eth)
- GitHub Check: Builds (esp32dev_compat)
- GitHub Check: Builds (esp32s2_PSRAM_M)
- GitHub Check: Builds (esp32c3mini_dio_4MB_M)
- GitHub Check: Builds (adafruit_matrixportal_esp32s3)
- GitHub Check: Builds (esp8266_4MB_M)
- GitHub Check: Builds (esp32c3dev_2MB_M)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_M)
- GitHub Check: Builds (athom_music_esp32_4MB_M)
- GitHub Check: Builds (seeed_esp32c3_4MB_S)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp32_16MB_V4_S)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp32dev_compat)
- GitHub Check: Builds (esp32_pico_4MB_V4_S)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
🔇 Additional comments (8)
wled00/presets.cpp (8)
15-18: Good use of named constants for buffer sizes.Defining
QLOAD_BUFFERandFNAME_BUFFERas constants and using them consistently for array sizing andstrlcpybounds is a solid improvement for maintainability and prevents off-by-one errors.
43-50: Correct mutex acquisition order and error handling.The sequence is sound: JSON buffer lock is acquired first (line 43), then the file mutex (line 46). On mutex failure, the JSON lock is properly released before returning. The 2500ms timeout provides adequate margin for large preset files. Based on learnings, this aligns with the recommended pattern for serializing preset writes.
94-96: Proper LIFO lock release order.The mutex is released before the JSON buffer lock, maintaining correct reverse acquisition order. This prevents potential deadlock scenarios.
310-314: Consistent buffer size usage.Using
FNAME_BUFFER+1consistently with thesaveNamearray declaration ensures correct bounds forstrlcpy.
334-343: Mutex acquisition and state restoration correctly implemented.The immediate API save path now properly acquires
presetFileMuxbefore writing, addressing the race condition withdoSaveState(). The state restoration on mutex failure (lines 339-341) correctly restorespresetToSave,playlistSave, andquickLoadas requested in past review comments.
345-352: Correct mutex release and state restoration on validation failure.The
index > 250check is appropriate here since API calls cannot be saved to temporary presets (251-255). The mutex is correctly released before returning, and state variables are properly restored. This addresses the concerns from past review comments.
359-365: Mutex correctly scoped around file write operation.The file mutex is held through
initPresetsFile()andwriteObjectToFileUsingId(), then released before updating non-file-related state. This ensures the critical section is as short as necessary while protecting the file write.
375-388: Delete operation correctly protected by file mutex.In MoonModules/WLED-MM, presets.json writes must be serialized and atomic: guard all write paths with a dedicated file-level mutex (presetFileMux). This change properly guards the delete path with
presetFileMux, completing the mutex coverage across all three write paths (doSaveState,savePresetimmediate branch, anddeletePreset). Based on learnings, this is necessary to prevent corruption.
Backport of the new PixelForge by @DedeHai
Additional Improvements
toDo:
#FPS,#AMP,#ERR-> postponed. Back-porting the relevant upstream "scrolling text" part will come later in a separate PR.
-> PixelPaint works, but refuses to accept more than 128 pixels in one dimension
-> PixelMagic has known incompatibilities with WLED-MM (large image presets cause error message)
-> we could add an additional download link for VideoLab (beta)
Known Issues and Caveats
WebUI HTML build system:
#characters. For example, i was working inC:\Users\frank\Arduino\#ESP32\MoonModules\WLED-MM, had to rename#ESP32toESP32to get npm run build workinghtml_*.hfiles, to force re-generationC:\Users\frank\AppData\Local\npm-cache)other
Summary by CodeRabbit
New Features
Tests
Style
Refactor
Chores
Stability
✏️ Tip: You can customize this high-level summary in your review settings.