Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughRefactors build and platform compatibility for ESP32/ESP-IDF v5: gates NeoEsp32RmtHI compilation by WLED_USE_SHARED_RMT, renames Network → WLEDNetwork across the codebase, updates LEDC/ETH/UDP/MDNS/NTP APIs for IDF v5, adds SHA-1 shim, DMX RDM personality changes, and updates platformio/requirements. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
I'm not sure if renaming Network is the right approach to fix the conflict @willmmiles |
|
I am also unsure why we are seeing linker errors for things like mbedtls_sha1_starts |
Hm, it looks like it's just a bag of stateless utility functions. Probably it should be a namespace instead of a class. I don't think there's a practical solution to avoid the collision other than renaming, unless the newer core offers these utilities itself. (In fact I'd also suggest pulling them in to wled00/network.cpp , given the use of WLED-specific #defines.) (Best pratice would've been that the upstream libraries put everything they do in a namespace, eg.
I believe that the Tasmota platform omits mbedtls to save flash space, as they've gone over to BearSSL. Looks like they're still exposing the headers though - I don't know if that's an IDF bug or something wrong with their build process. AsyncWebServer needs an SHA1 implementation as it is required for WebSockets. It looks like the upstream AWS folks have switched over to IDFv5 functions and vendored in Espressif's implementation when building on IDFv4. I'll see if I can pull those patches. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
…(ESP-IDF 5.3.4 / arduino-esp32 v3.1.4). Based on wled#4838.
…(ESP-IDF 5.3.4 / arduino-esp32 v3.1.4). Based on wled#4838.
|
ToDO: analyze this build warning Building .pio\build\esp32dev\bootloader.bin
esptool v5.1.2
Creating ESP32 image...
Merged 2 ELF sections.
Successfully created ESP32 image.
wled00/wled_metadata.cpp:6:4: warning: #warning WLED_VERSION was not set - using default value of 'dev' [-Wcpp]
6 | #warning WLED_VERSION was not set - using default value of 'dev'
| ^~~~~~~
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/ota_update.cpp (1)
724-727:⚠️ Potential issue | 🔴 CriticalWrite only the validated bootloader length to flash.
verifyBootloaderImage()updatesbootloaderSize, butbytesToWriteis initialized fromcontext->bytesBuffered. If trailing bytes exist, extra unvalidated data is flashed.Suggested fix
size_t flashOffset = context->bootloaderOffset; - const uint8_t* dataToWrite = context->buffer; - size_t bytesToWrite = context->bytesBuffered; + const uint8_t* dataToWrite = bootloaderData; + size_t bytesToWrite = bootloaderSize; // If validation adjusted the pointer, it means we have a partition table at the start // In this case, we should skip writing the partition table and write bootloader at 0x1000 if (bootloaderData != context->buffer) { // bootloaderData was adjusted - skip partition table in our data size_t partitionTableSize = bootloaderData - context->buffer; - dataToWrite = bootloaderData; - bytesToWrite = bootloaderSize; DEBUG_PRINTF_P(PSTR("Skipping %d bytes of partition table data\n"), partitionTableSize); }Also applies to: 730-735, 738-743
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/ota_update.cpp` around lines 724 - 727, The code writes context->bytesBuffered to flash even after verifyBootloaderImage() may have reduced the validated bootloaderSize, so trailing unvalidated bytes can be flashed; change the write logic in ota_update.cpp (around flashOffset / dataToWrite / bytesToWrite handling) to clamp bytesToWrite to the validated bootloaderSize (use the updated bootloaderSize instead of context->bytesBuffered), and adjust flashOffset/dataToWrite for subsequent writes to ensure only bootloaderSize bytes are written; update the same pattern in the other occurrences around the noted lines (730-735, 738-743) so every write uses min(context->bytesBuffered, bootloaderSize - bytesAlreadyWritten).
♻️ Duplicate comments (2)
wled00/util.cpp (1)
1139-1140:⚠️ Potential issue | 🟡 MinorUnify the compile guard for
computeSHA1()/getDeviceId().Line 1139 uses a different guard than the declaration/call sites, which risks target drift over time. Use the same guard pattern here as in the related declaration/use locations.
Suggested fix
-#if ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0) // ToDO: find a solution to make this work in V5 +#if !defined(ARDUINO_ARCH_ESP32) || (ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)) // TODO: find a solution to make this work in V5🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/util.cpp` around lines 1139 - 1140, The preprocessor guard around the block needs to match the same condition used where computeSHA1() and getDeviceId() are declared/used; update the guard at the shown location (currently `#if` ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)) to the exact same macro expression used at the computeSHA1()/getDeviceId() declaration and call sites so the functions are consistently included/excluded across compilation units (locate the guard used alongside the computeSHA1 and getDeviceId declarations and apply that identical guard here).platformio.ini (1)
531-531:⚠️ Potential issue | 🟡 MinorESP32 V5 envs are still dropping V5-specific
build_unflags.These environments inherit
${common.build_unflags}instead of${esp32_idf_V5.build_unflags}, so V5-specific unflags are not consistently applied.Suggested fix pattern
-build_unflags = ${common.build_unflags} +build_unflags = ${esp32_idf_V5.build_unflags}Apply the same replacement to each affected ESP32 V5 environment listed above.
Also applies to: 545-546, 556-556, 572-572, 589-589, 607-607, 783-783
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platformio.ini` at line 531, Several ESP32 V5 environments are inheriting ${common.build_unflags} instead of the V5-specific ${esp32_idf_V5.build_unflags}, so V5-specific unflags are not applied; update each ESP32 V5 environment entry in platformio.ini to replace build_unflags = ${common.build_unflags} with build_unflags = ${esp32_idf_V5.build_unflags} (apply the same replacement for every ESP32 V5 env that currently references ${common.build_unflags}); look for occurrences of the build_unflags key and the symbols common.build_unflags and esp32_idf_V5.build_unflags to ensure all V5 envs use the V5-specific variable.
🧹 Nitpick comments (1)
usermods/pixels_dice_tray/BLE_REQUIREMENT.md (1)
17-20: Clarify the expected working directory for the copy command.
cp platformio_override.ini.sample ../../../platformio_override.iniis context-sensitive; adding “run fromusermods/pixels_dice_tray” would prevent miscopies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/pixels_dice_tray/BLE_REQUIREMENT.md` around lines 17 - 20, Clarify the working directory for the cp command by updating the instruction around "cp platformio_override.ini.sample ../../../platformio_override.ini" to explicitly state it must be run from the usermods/pixels_dice_tray directory (e.g., "From the usermods/pixels_dice_tray directory, run: cp platformio_override.ini.sample ../../../platformio_override.ini"); reference the exact command string so maintainers can locate and edit that line in BLE_REQUIREMENT.md.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/data/update.htm`:
- Around line 47-50: hideforms() currently hides bootupd and toggles upd but
never displays the "Noupd" / updating state, so uploads can leave a blank page;
update hideforms() to also show the Noupd element (e.g., remove "hide" or add a
visible class to gId('Noupd')) so an “Updating...” state is visible, and make
the same change in the other similar submit handlers that mirror the hideforms
logic (the blocks using gId('bootupd'), toggle('upd') around the submit flow) to
ensure consistent behavior across all submit paths.
In `@wled00/ota_update.cpp`:
- Around line 453-462: The bug is signed-byte handling when converting
bootloaderSHA256Cache bytes to hex in the loop using char b1/b2; change to use
an unsigned byte type (e.g., uint8_t or unsigned char) for the temporary value
and for the nibble operations so right-shifts and masks behave correctly: read
the byte as uint8_t val = (uint8_t)bootloaderSHA256Cache[i], compute high = (val
>> 4) and low = (val & 0x0F), convert each nibble to its ASCII hex char (same +
'0' and +39 adjustment as used) and call result.concat on those chars instead of
relying on signed char b1/b2.
- Around line 415-418: The loop that reads the bootloader into the SHA256
context (using esp_flash_read and sizer.feed) currently ignores read failures
and later marks the bootloader SHA cache valid even on partial reads; modify the
logic so you track a boolean success flag (or return early) and only mark the
cached bootloader SHA as valid if every esp_flash_read returned ESP_OK for all
chunks, otherwise abort hashing and ensure the cache remains invalid (so
getBootloaderSHA256Hex() will trigger a recalculation or report failure); update
the code paths around the loop and where the cache validity flag is set to
depend on this all-reads-succeeded condition.
---
Outside diff comments:
In `@wled00/ota_update.cpp`:
- Around line 724-727: The code writes context->bytesBuffered to flash even
after verifyBootloaderImage() may have reduced the validated bootloaderSize, so
trailing unvalidated bytes can be flashed; change the write logic in
ota_update.cpp (around flashOffset / dataToWrite / bytesToWrite handling) to
clamp bytesToWrite to the validated bootloaderSize (use the updated
bootloaderSize instead of context->bytesBuffered), and adjust
flashOffset/dataToWrite for subsequent writes to ensure only bootloaderSize
bytes are written; update the same pattern in the other occurrences around the
noted lines (730-735, 738-743) so every write uses min(context->bytesBuffered,
bootloaderSize - bytesAlreadyWritten).
---
Duplicate comments:
In `@platformio.ini`:
- Line 531: Several ESP32 V5 environments are inheriting ${common.build_unflags}
instead of the V5-specific ${esp32_idf_V5.build_unflags}, so V5-specific unflags
are not applied; update each ESP32 V5 environment entry in platformio.ini to
replace build_unflags = ${common.build_unflags} with build_unflags =
${esp32_idf_V5.build_unflags} (apply the same replacement for every ESP32 V5 env
that currently references ${common.build_unflags}); look for occurrences of the
build_unflags key and the symbols common.build_unflags and
esp32_idf_V5.build_unflags to ensure all V5 envs use the V5-specific variable.
In `@wled00/util.cpp`:
- Around line 1139-1140: The preprocessor guard around the block needs to match
the same condition used where computeSHA1() and getDeviceId() are declared/used;
update the guard at the shown location (currently `#if` ESP_IDF_VERSION <
ESP_IDF_VERSION_VAL(5, 0, 0)) to the exact same macro expression used at the
computeSHA1()/getDeviceId() declaration and call sites so the functions are
consistently included/excluded across compilation units (locate the guard used
alongside the computeSHA1 and getDeviceId declarations and apply that identical
guard here).
---
Nitpick comments:
In `@usermods/pixels_dice_tray/BLE_REQUIREMENT.md`:
- Around line 17-20: Clarify the working directory for the cp command by
updating the instruction around "cp platformio_override.ini.sample
../../../platformio_override.ini" to explicitly state it must be run from the
usermods/pixels_dice_tray directory (e.g., "From the usermods/pixels_dice_tray
directory, run: cp platformio_override.ini.sample
../../../platformio_override.ini"); reference the exact command string so
maintainers can locate and edit that line in BLE_REQUIREMENT.md.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
package.jsonplatformio.iniusermods/audioreactive/audio_source.husermods/pixels_dice_tray/BLE_REQUIREMENT.mdusermods/pixels_dice_tray/library.jsonwled00/bus_manager.cppwled00/data/update.htmwled00/e131.cppwled00/fcn_declare.hwled00/improv.cppwled00/json.cppwled00/network.cppwled00/ntp.cppwled00/ota_update.cppwled00/ota_update.hwled00/src/dependencies/espalexa/Espalexa.hwled00/udp.cppwled00/util.cppwled00/wled.cppwled00/wled.hwled00/wled_server.cppwled00/xml.cpp
💤 Files with no reviewable changes (2)
- wled00/ota_update.h
- usermods/pixels_dice_tray/library.json
🚧 Files skipped from review as they are similar to previous changes (7)
- wled00/fcn_declare.h
- wled00/improv.cpp
- wled00/json.cpp
- wled00/wled.cpp
- wled00/udp.cpp
- wled00/wled_server.cpp
- wled00/network.cpp
| function hideforms() { | ||
| gId('bootupd').classList.add("hide"); | ||
| toggle('upd'); | ||
| } |
There was a problem hiding this comment.
Show the update progress state when submitting.
hideforms() currently hides/toggles form containers but never shows Noupd, so upload can leave a blank page instead of an “Updating...” state.
Suggested fix
function hideforms() {
- gId('bootupd').classList.add("hide");
- toggle('upd');
+ gId('upd').classList.add("hide");
+ gId('bootupd').classList.add("hide");
+ gId('Noupd').classList.remove("hide");
}Also applies to: 58-59, 77-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/data/update.htm` around lines 47 - 50, hideforms() currently hides
bootupd and toggles upd but never displays the "Noupd" / updating state, so
uploads can leave a blank page; update hideforms() to also show the Noupd
element (e.g., remove "hide" or add a visible class to gId('Noupd')) so an
“Updating...” state is visible, and make the same change in the other similar
submit handlers that mirror the hideforms logic (the blocks using
gId('bootupd'), toggle('upd') around the submit flow) to ensure consistent
behavior across all submit paths.
| for (uint32_t offset = 0; offset < bootloaderSize; offset += chunkSize) { | ||
| size_t readSize = min((size_t)(bootloaderSize - offset), chunkSize); | ||
| if (esp_flash_read(NULL, buffer, BOOTLOADER_OFFSET + offset, readSize) == ESP_OK) { | ||
| mbedtls_sha256_update(&ctx, buffer, readSize); | ||
| sizer.feed(buffer, readSize); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file structure and locate the relevant lines
cd /root/repo && wc -l wled00/ota_update.cppRepository: wled/WLED
Length of output: 113
🏁 Script executed:
# Read the relevant section of the file around lines 415-442
sed -n '400,450p' wled00/ota_update.cppRepository: wled/WLED
Length of output: 1550
🏁 Script executed:
# Get more context by reading the entire function to understand control flow
# First, let's find what function contains these lines
sed -n '350,500p' wled00/ota_update.cpp | head -200Repository: wled/WLED
Length of output: 5010
Mark bootloader SHA256 cache valid only if all flash reads succeed.
Read failures are currently silently ignored. If any read fails, the SHA256 context remains uninitialized or partial, but the cache is still marked valid and persists until invalidated, causing getBootloaderSHA256Hex() to return an invalid digest without recalculation.
Suggested fix
+ bool readOk = true;
for (uint32_t offset = 0; offset < bootloaderSize; offset += chunkSize) {
size_t readSize = min((size_t)(bootloaderSize - offset), chunkSize);
- if (esp_flash_read(NULL, buffer, BOOTLOADER_OFFSET + offset, readSize) == ESP_OK) {
- sizer.feed(buffer, readSize);
+ if (esp_flash_read(NULL, buffer, BOOTLOADER_OFFSET + offset, readSize) != ESP_OK) {
+ readOk = false;
+ break;
+ }
+ sizer.feed(buffer, readSize);
size_t hashLen = readSize;
if (sizer.isSizeKnown()) {
size_t totalSize = sizer.totalSize();
if (totalSize > 0 && totalSize <= BOOTLOADER_SIZE) {
bootloaderSize = totalSize;
if (offset + readSize > totalSize) {
hashLen = (totalSize > offset) ? (totalSize - offset) : 0;
}
}
}
if (hashLen > 0) {
totalHashLen += hashLen;
mbedtls_sha256_update(&ctx, buffer, hashLen);
}
- }
}
+ if (!readOk) {
+ mbedtls_sha256_free(&ctx);
+ bootloaderSHA256CacheValid = false;
+ return;
+ }
mbedtls_sha256_finish(&ctx, bootloaderSHA256Cache);
mbedtls_sha256_free(&ctx);
bootloaderSHA256CacheValid = true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/ota_update.cpp` around lines 415 - 418, The loop that reads the
bootloader into the SHA256 context (using esp_flash_read and sizer.feed)
currently ignores read failures and later marks the bootloader SHA cache valid
even on partial reads; modify the logic so you track a boolean success flag (or
return early) and only mark the cached bootloader SHA as valid if every
esp_flash_read returned ESP_OK for all chunks, otherwise abort hashing and
ensure the cache remains invalid (so getBootloaderSHA256Hex() will trigger a
recalculation or report failure); update the code paths around the loop and
where the cache validity flag is set to depend on this all-reads-succeeded
condition.
Fix signed-byte handling in SHA256 hex conversion. When bootloaderSHA256Cache bytes >= 0x80 are assigned to a signed char, they sign-extend to negative values. Right-shifting negative values produces incorrect results (e.g., 0xFF becomes -1, which shifts incorrectly). This causes wrong hex digit output for non-ASCII hash bytes.
There was a problem hiding this comment.
♻️ Duplicate comments (5)
wled00/data/update.htm (1)
47-50:⚠️ Potential issue | 🟡 MinorShow the “Updating...” state explicitly in
hideforms().The helper hides/toggles forms but does not unhide
Noupd, so both submit paths can render a blank page during upload.Suggested fix
function hideforms() { - gId('bootupd').classList.add("hide"); - toggle('upd'); + gId('upd').classList.add("hide"); + gId('bootupd').classList.add("hide"); + gId('Noupd').classList.remove("hide"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/update.htm` around lines 47 - 50, hideforms() currently hides the bootupd form and toggles upd but never shows the "Updating..." indicator, which can leave the page blank; update hideforms() to explicitly unhide or show the Noupd element (e.g. remove its "hide" class or set it visible) so the "Updating..." state is displayed (refer to hideforms(), gId('bootupd'), toggle('upd') and the Noupd element).platformio.ini (3)
130-130:⚠️ Potential issue | 🟡 MinorDefine
WLED_USE_SHARED_RMTin one scope only.Line [130] defines it globally and Line [337] defines it again for V5. Keep one V5-scoped definition to avoid unintended ESP8266 leakage and config drift.
🛠️ Proposed fix
[common] build_flags = ... - -D WLED_USE_SHARED_RMT ;; Use standard RMT method instead of incompatible NeoEsp32RmtHI - until updated for V5 [esp32_idf_V5] build_flags = -g ... -D WLED_USE_SHARED_RMT ;; ToDO: check if NeoESP32RmtHI is still needed with V5 (see discussion in PR#4838)Also applies to: 337-337
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platformio.ini` at line 130, The WLED_USE_SHARED_RMT define is declared twice causing scope leakage; remove the global definition of WLED_USE_SHARED_RMT (the one outside the V5 environment) and keep only the V5-scoped definition so it applies exclusively when building for V5; search for the symbol WLED_USE_SHARED_RMT in platformio.ini and delete the non-V5/global instance, leaving the V5-specific define in the env:V5 section.
23-23:⚠️ Potential issue | 🟡 MinorKeep test-only Ethernet overrides out of the default V4 regression target.
Line [23] keeps
esp32dev_V4indefault_envs, while Line [518] still applies temporary Ethernet/pin test overrides. This makes the V4 regression job non-baseline.🛠️ Proposed fix
[env:esp32dev_V4] ... - -D WLED_USE_ETHERNET -D RLYPIN=-1 -D BTNPIN=-1 ;; TODO: this is just for testing - remove before merging to mainAlso applies to: 518-519
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platformio.ini` at line 23, The default_envs list includes the test-only target esp32dev_V4 while the temporary Ethernet/pin overrides are applied to that same V4 env (esp32dev_V4) later, making the regression baseline non-baseline; remove esp32dev_V4 from the default_envs value and instead create or use a separate dedicated test env (or move the Ethernet/pin overrides into a non-default env) so that esp32dev_V4 is not part of the default regression targets and the overrides at the esp32dev_V4 block are only applied to the test env.
531-546:⚠️ Potential issue | 🟡 Minor
esp32devandesp32dev_debugshould inherit V5-specificbuild_unflags.Line [531] and Line [545] currently use
${common.build_unflags}. That bypasses${esp32_idf_V5.build_unflags}and can re-enable flags intentionally removed for V5.🛠️ Proposed fix
[env:esp32dev] ... -build_unflags = ${common.build_unflags} +build_unflags = ${esp32_idf_V5.build_unflags} ... [env:esp32dev_debug] ... -build_unflags = ${common.build_unflags} +build_unflags = ${esp32_idf_V5.build_unflags} -D WLED_RELEASE_NAME=\"ESP32\"Based on learnings: "In PlatformIO projects, prefer using build_unflags in the PlatformIO configuration to remove inherited -D defines rather than using GCC's -U option."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platformio.ini` around lines 531 - 546, The env sections esp32dev and esp32dev_debug are inheriting ${common.build_unflags} which skips the V5-specific unflags; replace those occurrences with ${esp32_idf_V5.build_unflags} so both env:esp32dev and env:esp32dev_debug inherit the V5-specific build_unflags (keep other build_unflags entries as-is, e.g., additional -D WLED_RELEASE_NAME=\"ESP32\" in esp32dev_debug).wled00/ota_update.cpp (1)
415-442:⚠️ Potential issue | 🔴 CriticalOnly mark bootloader SHA cache valid when all flash reads succeed.
Line [417] currently skips failed reads, but Line [441] still sets the cache valid. That can return an incorrect digest from partial data.
🛠️ Proposed fix
static void calculateBootloaderSHA256() { + bool readOk = true; // Calculate SHA256 mbedtls_sha256_context ctx; mbedtls_sha256_init(&ctx); mbedtls_sha256_starts(&ctx, 0); // 0 = SHA256 (not SHA224) ... for (uint32_t offset = 0; offset < bootloaderSize; offset += chunkSize) { size_t readSize = min((size_t)(bootloaderSize - offset), chunkSize); - if (esp_flash_read(NULL, buffer, BOOTLOADER_OFFSET + offset, readSize) == ESP_OK) { - sizer.feed(buffer, readSize); + if (esp_flash_read(NULL, buffer, BOOTLOADER_OFFSET + offset, readSize) != ESP_OK) { + readOk = false; + break; + } + sizer.feed(buffer, readSize); ... - if (hashLen > 0) { - totalHashLen += hashLen; - mbedtls_sha256_update(&ctx, buffer, hashLen); - } - } + if (hashLen > 0) { + totalHashLen += hashLen; + mbedtls_sha256_update(&ctx, buffer, hashLen); + } } + if (!readOk) { + mbedtls_sha256_free(&ctx); + bootloaderSHA256CacheValid = false; + return; + } mbedtls_sha256_finish(&ctx, bootloaderSHA256Cache); mbedtls_sha256_free(&ctx); bootloaderSHA256CacheValid = true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/ota_update.cpp` around lines 415 - 442, The loop may skip failed esp_flash_read calls but the code always calls mbedtls_sha256_finish and sets bootloaderSHA256CacheValid; change this so the SHA finish and bootloaderSHA256CacheValid = true only occur when all flash reads succeeded. Add a boolean (e.g., allReadsOk) initialized true, set false if esp_flash_read(...) != ESP_OK, and after the loop call mbedtls_sha256_finish(&ctx, bootloaderSHA256Cache) and set bootloaderSHA256CacheValid = true only if allReadsOk; always mbedtls_sha256_free(&ctx) to avoid leaks even when reads failed. Ensure you reference bootloaderSize, chunkSize, sizer, totalHashLen, bootloaderSHA256Cache, and bootloaderSHA256CacheValid when implementing the change.
🧹 Nitpick comments (1)
wled00/ota_update.cpp (1)
567-568: Use the target-specific offset constant in bootloader context.Line [567] hardcodes
0x1000even thoughBOOTLOADER_OFFSETis already target-aware.♻️ Proposed refactor
- const uint32_t bootloaderOffset = 0x1000; + const uint32_t bootloaderOffset = static_cast<uint32_t>(BOOTLOADER_OFFSET);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/ota_update.cpp` around lines 567 - 568, Replace the hardcoded bootloader offset with the target-aware constant: change the local variable bootloaderOffset (currently set to 0x1000) to use BOOTLOADER_OFFSET so the code respects platform-specific offsets; keep maxBootloaderSize as-is and ensure any subsequent uses/reference of bootloaderOffset in ota_update.cpp continue to compile against the renamed/updated symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@platformio.ini`:
- Line 130: The WLED_USE_SHARED_RMT define is declared twice causing scope
leakage; remove the global definition of WLED_USE_SHARED_RMT (the one outside
the V5 environment) and keep only the V5-scoped definition so it applies
exclusively when building for V5; search for the symbol WLED_USE_SHARED_RMT in
platformio.ini and delete the non-V5/global instance, leaving the V5-specific
define in the env:V5 section.
- Line 23: The default_envs list includes the test-only target esp32dev_V4 while
the temporary Ethernet/pin overrides are applied to that same V4 env
(esp32dev_V4) later, making the regression baseline non-baseline; remove
esp32dev_V4 from the default_envs value and instead create or use a separate
dedicated test env (or move the Ethernet/pin overrides into a non-default env)
so that esp32dev_V4 is not part of the default regression targets and the
overrides at the esp32dev_V4 block are only applied to the test env.
- Around line 531-546: The env sections esp32dev and esp32dev_debug are
inheriting ${common.build_unflags} which skips the V5-specific unflags; replace
those occurrences with ${esp32_idf_V5.build_unflags} so both env:esp32dev and
env:esp32dev_debug inherit the V5-specific build_unflags (keep other
build_unflags entries as-is, e.g., additional -D WLED_RELEASE_NAME=\"ESP32\" in
esp32dev_debug).
In `@wled00/data/update.htm`:
- Around line 47-50: hideforms() currently hides the bootupd form and toggles
upd but never shows the "Updating..." indicator, which can leave the page blank;
update hideforms() to explicitly unhide or show the Noupd element (e.g. remove
its "hide" class or set it visible) so the "Updating..." state is displayed
(refer to hideforms(), gId('bootupd'), toggle('upd') and the Noupd element).
In `@wled00/ota_update.cpp`:
- Around line 415-442: The loop may skip failed esp_flash_read calls but the
code always calls mbedtls_sha256_finish and sets bootloaderSHA256CacheValid;
change this so the SHA finish and bootloaderSHA256CacheValid = true only occur
when all flash reads succeeded. Add a boolean (e.g., allReadsOk) initialized
true, set false if esp_flash_read(...) != ESP_OK, and after the loop call
mbedtls_sha256_finish(&ctx, bootloaderSHA256Cache) and set
bootloaderSHA256CacheValid = true only if allReadsOk; always
mbedtls_sha256_free(&ctx) to avoid leaks even when reads failed. Ensure you
reference bootloaderSize, chunkSize, sizer, totalHashLen, bootloaderSHA256Cache,
and bootloaderSHA256CacheValid when implementing the change.
---
Nitpick comments:
In `@wled00/ota_update.cpp`:
- Around line 567-568: Replace the hardcoded bootloader offset with the
target-aware constant: change the local variable bootloaderOffset (currently set
to 0x1000) to use BOOTLOADER_OFFSET so the code respects platform-specific
offsets; keep maxBootloaderSize as-is and ensure any subsequent uses/reference
of bootloaderOffset in ota_update.cpp continue to compile against the
renamed/updated symbol.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
package.jsonplatformio.iniusermods/audioreactive/audio_source.husermods/pixels_dice_tray/BLE_REQUIREMENT.mdusermods/pixels_dice_tray/library.jsonwled00/bus_manager.cppwled00/data/update.htmwled00/e131.cppwled00/fcn_declare.hwled00/improv.cppwled00/json.cppwled00/network.cppwled00/ntp.cppwled00/ota_update.cppwled00/ota_update.hwled00/src/dependencies/espalexa/Espalexa.hwled00/udp.cppwled00/util.cppwled00/wled.cppwled00/wled.hwled00/wled_server.cppwled00/xml.cpp
💤 Files with no reviewable changes (2)
- usermods/pixels_dice_tray/library.json
- wled00/ota_update.h
🚧 Files skipped from review as they are similar to previous changes (5)
- wled00/json.cpp
- wled00/improv.cpp
- wled00/e131.cpp
- wled00/wled_server.cpp
- wled00/fcn_declare.h
Guard against zero-length binary payloads before dereferencing data[0] or data[1]
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/ws.cpp (1)
87-94:⚠️ Potential issue | 🟠 MajorAdd minimum packet size validation for E1.31 and ArtNet before casting and dereferencing struct fields.
Line 87's guard (
len < offset+1) ensures only 2 bytes minimum, but E1.31/ArtNet packet structures require significantly larger buffers to safely access their fields:
- Art-Net: minimum 18 bytes (accesses
art_opcodeat offset 8-9,art_lengthat offset 16-17 inhandleE131Packet)- E1.31: minimum 125 bytes (accesses
property_values[0]at offset 125 inhandleE131Packet)Currently, lines 90 and 93 forward undersized payloads as raw pointers with no length information passed to
handleE131Packet(). This mirrors the vulnerability already handled properly by the DDP branch (lines 96-100), which validates minimum size before calling the same handler.Add explicit size checks before the E1.31 and ArtNet cases (similar to DDP at line 96):
case BINARY_PROTOCOL_E131: if (len < 125 + offset) return; handleE131Packet((e131_packet_t*)&data[offset], client->remoteIP(), P_E131); break; case BINARY_PROTOCOL_ARTNET: if (len < 18 + offset) return; handleE131Packet((e131_packet_t*)&data[offset], client->remoteIP(), P_ARTNET); break;Malformed short binary frames will otherwise cause out-of-bounds reads when struct fields are accessed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/ws.cpp` around lines 87 - 94, The switch handling BINARY_PROTOCOL_E131 and BINARY_PROTOCOL_ARTNET casts data+offset to e131_packet_t and can read far beyond the current buffer; add explicit minimum-size guards using len, offset and the protocol constants before calling handleE131Packet (mirror the DDP branch): check that len >= offset + 125 for BINARY_PROTOCOL_E131 and len >= offset + 18 for BINARY_PROTOCOL_ARTNET, return early on failure, then call handleE131Packet((e131_packet_t*)&data[offset], client->remoteIP(), P_E131/P_ARTNET) as before.
♻️ Duplicate comments (6)
wled00/ota_update.cpp (1)
415-442:⚠️ Potential issue | 🔴 CriticalPrevent caching an invalid bootloader SHA when flash reads/parsing fail.
Line 417 ignores read failures, and Line 441 still marks the cache as valid. This can cache a digest from partial/invalid input.
💡 Proposed fix
static void calculateBootloaderSHA256() { // Calculate SHA256 mbedtls_sha256_context ctx; mbedtls_sha256_init(&ctx); mbedtls_sha256_starts(&ctx, 0); // 0 = SHA256 (not SHA224) @@ size_t bootloaderSize = BOOTLOADER_SIZE; BootloaderImageSizer sizer; - size_t totalHashLen = 0; + bool readOk = true; for (uint32_t offset = 0; offset < bootloaderSize; offset += chunkSize) { size_t readSize = min((size_t)(bootloaderSize - offset), chunkSize); - if (esp_flash_read(NULL, buffer, BOOTLOADER_OFFSET + offset, readSize) == ESP_OK) { - sizer.feed(buffer, readSize); + if (esp_flash_read(NULL, buffer, BOOTLOADER_OFFSET + offset, readSize) != ESP_OK) { + readOk = false; + break; + } + if (!sizer.feed(buffer, readSize) && sizer.hasError()) { + readOk = false; + break; + } size_t hashLen = readSize; if (sizer.isSizeKnown()) { @@ if (hashLen > 0) { - totalHashLen += hashLen; mbedtls_sha256_update(&ctx, buffer, hashLen); } - } } + if (!readOk) { + mbedtls_sha256_free(&ctx); + bootloaderSHA256CacheValid = false; + return; + } + mbedtls_sha256_finish(&ctx, bootloaderSHA256Cache); mbedtls_sha256_free(&ctx); bootloaderSHA256CacheValid = true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/ota_update.cpp` around lines 415 - 442, The loop currently ignores esp_flash_read failures and always finalizes and marks bootloaderSHA256CacheValid true; change the logic in the function containing the loop so you track overall success (e.g., a bool like bootloaderHashOk) initialized true, set it false if any esp_flash_read call returns non-ESP_OK or if sizer/parsing indicates an error/unexpected size, only call mbedtls_sha256_finish(&ctx, bootloaderSHA256Cache) and set bootloaderSHA256CacheValid = true when bootloaderHashOk is still true; on failure, free the mbedtls context and leave bootloaderSHA256CacheValid false (or clear the cache) so you never cache a digest derived from partial/invalid reads (referencing bootloaderSize, chunkSize, esp_flash_read, sizer, mbedtls_sha256_finish, bootloaderSHA256Cache and bootloaderSHA256CacheValid).wled00/data/update.htm (1)
47-50:⚠️ Potential issue | 🟡 MinorShow the explicit “Updating…” state on submit.
On Line 47-50,
hideforms()still never revealsNoupd, so submit can leave the page with hidden forms but no progress message.Suggested fix
function hideforms() { - gId('bootupd').classList.add("hide"); - toggle('upd'); + gId('upd').classList.add("hide"); + gId('bootupd').classList.add("hide"); + gId('Noupd').classList.remove("hide"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/update.htm` around lines 47 - 50, The hideforms() function hides the boot update form (gId('bootupd')) but does not reveal the explicit "Updating…" progress element, so submissions can leave the UI blank; update hideforms() to also show the progress/message element (e.g., remove the "hide" class from the element with id 'Noupd' or otherwise ensure the "Updating…" indicator is visible) while keeping the existing toggle('upd') behavior so the user sees the explicit updating state during submit.platformio.ini (2)
531-531:⚠️ Potential issue | 🟡 MinorV5 environments should inherit V5-specific
build_unflags.Line 531 uses
${common.build_unflags}in a V5 environment. This drops${esp32_idf_V5.build_unflags}and can reintroduce flags that V5 config explicitly removes.Suggested fix
[env:esp32dev] -build_unflags = ${common.build_unflags} +build_unflags = ${esp32_idf_V5.build_unflags}Apply the same pattern to other V5 envs currently overriding with
${common.build_unflags}.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platformio.ini` at line 531, In V5 environments that set build_unflags to ${common.build_unflags}, change them to include the V5-specific override (e.g., use ${esp32_idf_V5.build_unflags} instead of ${common.build_unflags}) so the V5-specific flags are preserved; search for occurrences of build_unflags = ${common.build_unflags} in V5 env blocks and replace them with build_unflags = ${esp32_idf_V5.build_unflags} (or merge both if needed) to avoid reintroducing flags V5 intends to remove.
518-519:⚠️ Potential issue | 🟡 MinorKeep V4 regression env free of test-only Ethernet overrides.
Line 518 adds test-only Ethernet + pin overrides in
env:esp32dev_V4, and that env is part ofdefault_envs(Line 23). This skews regression coverage away from a baseline V4 build.Suggested fix
[env:esp32dev_V4] - -D WLED_USE_ETHERNET -D RLYPIN=-1 -D BTNPIN=-1 ;; TODO: this is just for testing - remove before merging to mainMove these flags to a dedicated test-only env if needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platformio.ini` around lines 518 - 519, Remove the test-only defines "-D WLED_USE_ETHERNET -D RLYPIN=-1 -D BTNPIN=-1" from the env named env:esp32dev_V4 (which is currently included in default_envs) and instead add a new dedicated test-only environment (e.g., env:esp32dev_V4_test) that contains those defines; update default_envs to keep only baseline env:esp32dev_V4 so regression runs use the clean V4 baseline, and ensure the new test env is not part of default_envs so it only runs when explicitly requested.wled00/util.cpp (1)
1139-1140:⚠️ Potential issue | 🟠 MajorUse a platform-aware ESP-IDF guard here.
Line 1139 uses a raw
ESP_IDF_VERSIONcomparison. In mixed ESP8266/ESP32 builds, this should be guarded by platform first to avoid preprocessor portability issues and keep declaration/definition guards consistent.Suggested fix
-#if ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0) // ToDO: find a solution to make this work in V5 +#if !defined(ARDUINO_ARCH_ESP32) || (ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)) // TODO: find a solution to make this work in V5#!/bin/bash set -euo pipefail echo "Inspect current util.cpp guard:" sed -n '1134,1146p' wled00/util.cpp echo echo "Inspect related declarations in fcn_declare.h:" rg -n -C2 'computeSHA1|getDeviceId|ESP_IDF_VERSION' wled00/fcn_declare.h echo echo "Find ESP_IDF_VERSION guard styles in codebase:" rg -n '#if.*ESP_IDF_VERSION' wled00/ -g '*.{h,cpp}'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/util.cpp` around lines 1139 - 1140, Replace the raw ESP_IDF_VERSION comparison with a platform-aware guard: wrap the existing conditional in a check that the build is for ESP32 and ESP_IDF_VERSION is defined, e.g. use something like "#if defined(ARDUINO_ARCH_ESP32) && defined(ESP_IDF_VERSION) && (ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5,0,0))" (or the project’s ESP32 macro if different) so the block around the computeSHA1/getDeviceId-related code in util.cpp only applies on ESP32 and only when ESP_IDF_VERSION is available; this avoids preprocessor errors in mixed ESP8266/ESP32 builds.wled00/json.cpp (1)
703-705:⚠️ Potential issue | 🟡 MinorGuard
ESP_IDF_VERSIONbefore numeric comparison in the preprocessor condition.Line 703 currently compares
ESP_IDF_VERSIONwithout adefined()guard. If the macro is absent in a build context, it is treated as0, which can accidentally enabledeviceIdemission on targets you intended to exclude.🔧 Proposed fix
-#if !defined(ARDUINO_ARCH_ESP32) || (ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)) // ToDO: find a solution to make this work in V5 +#if !defined(ARDUINO_ARCH_ESP32) || (defined(ESP_IDF_VERSION) && (ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0))) // TODO: find a solution to make this work in V5 root[F("deviceId")] = getDeviceId(); `#endif`Run this to verify macro availability and all related guards:
#!/bin/bash set -euo pipefail echo "=== json.cpp guard ===" sed -n '699,707p' wled00/json.cpp echo echo "=== ESP_IDF_VERSION usages in preprocessor conditions ===" rg -n '#\s*(if|elif).*(ESP_IDF_VERSION|ESP_IDF_VERSION_VAL)' --type=cpp --type=h echo echo "=== Definitions/fallbacks for ESP_IDF_VERSION and ESP_IDF_VERSION_VAL ===" rg -n '#\s*define\s+ESP_IDF_VERSION(_VAL)?\b' --type=cpp --type=h🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/json.cpp` around lines 703 - 705, The preprocessor currently compares ESP_IDF_VERSION without checking if it is defined, which can evaluate to 0 unexpectedly; update the condition that controls emitting root[F("deviceId")]/getDeviceId() to explicitly test defined(ESP_IDF_VERSION) before the numeric comparison (e.g. use !defined(ARDUINO_ARCH_ESP32) || !defined(ESP_IDF_VERSION) || (ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5,0,0))) so the (ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(...)) term is only evaluated when ESP_IDF_VERSION is defined.
🧹 Nitpick comments (1)
wled00/ota_update.cpp (1)
567-568: Use target-aware constant instead of hardcoded bootloader offset.Line 567 hardcodes
0x1000even thoughBOOTLOADER_OFFSETis already defined per target. Reusing the existing constant avoids drift when support for additional targets is enabled.💡 Proposed refactor
struct BootloaderUpdateContext { @@ - const uint32_t bootloaderOffset = 0x1000; + const uint32_t bootloaderOffset = BOOTLOADER_OFFSET; const uint32_t maxBootloaderSize = 0x10000; // 64KB buffer size };Based on learnings: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/ota_update.cpp` around lines 567 - 568, Replace the hardcoded bootloaderOffset value (currently set to 0x1000) with the target-aware BOOTLOADER_OFFSET constant so the build uses the per-target definition; update the declaration that uses bootloaderOffset (and keep maxBootloaderSize as-is) to reference BOOTLOADER_OFFSET (ensuring the type remains uint32_t) so the code no longer contains the magic number and follows target-specific configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/pixels_dice_tray/BLE_REQUIREMENT.md`:
- Around line 17-20: Clarify the working directory for the cp command in
BLE_REQUIREMENT.md: either state the user must run the command from the usermod
directory (usermods/pixels_dice_tray) before running "cp
platformio_override.ini.sample ../../../platformio_override.ini", or replace it
with a repo-root-safe alternative such as copying with explicit paths ("cp
usermods/pixels_dice_tray/platformio_override.ini.sample
platformio_override.ini") so the command works regardless of current directory;
update the text to include one of these options and show the adjusted command.
- Around line 9-12: Update the wording in BLE_REQUIREMENT.md to clarify that
renaming library.json to library.json.disabled only prevents auto-inclusion of
this usermod via in-tree mechanisms controlled by custom_usermods (e.g., the
WLED in-tree usermods list), and does not disable out-of-tree usermods brought
in via library dependencies; explicitly mention custom_usermods affects only
in-tree usermods while lib_deps-supplied/out-of-tree usermods (and thus the
Arduino BLE library when provided as a dependency) can still be active and cause
compilation to pull in BLE symbols on Tasmota Arduino ESP32 builds.
---
Outside diff comments:
In `@wled00/ws.cpp`:
- Around line 87-94: The switch handling BINARY_PROTOCOL_E131 and
BINARY_PROTOCOL_ARTNET casts data+offset to e131_packet_t and can read far
beyond the current buffer; add explicit minimum-size guards using len, offset
and the protocol constants before calling handleE131Packet (mirror the DDP
branch): check that len >= offset + 125 for BINARY_PROTOCOL_E131 and len >=
offset + 18 for BINARY_PROTOCOL_ARTNET, return early on failure, then call
handleE131Packet((e131_packet_t*)&data[offset], client->remoteIP(),
P_E131/P_ARTNET) as before.
---
Duplicate comments:
In `@platformio.ini`:
- Line 531: In V5 environments that set build_unflags to
${common.build_unflags}, change them to include the V5-specific override (e.g.,
use ${esp32_idf_V5.build_unflags} instead of ${common.build_unflags}) so the
V5-specific flags are preserved; search for occurrences of build_unflags =
${common.build_unflags} in V5 env blocks and replace them with build_unflags =
${esp32_idf_V5.build_unflags} (or merge both if needed) to avoid reintroducing
flags V5 intends to remove.
- Around line 518-519: Remove the test-only defines "-D WLED_USE_ETHERNET -D
RLYPIN=-1 -D BTNPIN=-1" from the env named env:esp32dev_V4 (which is currently
included in default_envs) and instead add a new dedicated test-only environment
(e.g., env:esp32dev_V4_test) that contains those defines; update default_envs to
keep only baseline env:esp32dev_V4 so regression runs use the clean V4 baseline,
and ensure the new test env is not part of default_envs so it only runs when
explicitly requested.
In `@wled00/data/update.htm`:
- Around line 47-50: The hideforms() function hides the boot update form
(gId('bootupd')) but does not reveal the explicit "Updating…" progress element,
so submissions can leave the UI blank; update hideforms() to also show the
progress/message element (e.g., remove the "hide" class from the element with id
'Noupd' or otherwise ensure the "Updating…" indicator is visible) while keeping
the existing toggle('upd') behavior so the user sees the explicit updating state
during submit.
In `@wled00/json.cpp`:
- Around line 703-705: The preprocessor currently compares ESP_IDF_VERSION
without checking if it is defined, which can evaluate to 0 unexpectedly; update
the condition that controls emitting root[F("deviceId")]/getDeviceId() to
explicitly test defined(ESP_IDF_VERSION) before the numeric comparison (e.g. use
!defined(ARDUINO_ARCH_ESP32) || !defined(ESP_IDF_VERSION) || (ESP_IDF_VERSION <
ESP_IDF_VERSION_VAL(5,0,0))) so the (ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(...))
term is only evaluated when ESP_IDF_VERSION is defined.
In `@wled00/ota_update.cpp`:
- Around line 415-442: The loop currently ignores esp_flash_read failures and
always finalizes and marks bootloaderSHA256CacheValid true; change the logic in
the function containing the loop so you track overall success (e.g., a bool like
bootloaderHashOk) initialized true, set it false if any esp_flash_read call
returns non-ESP_OK or if sizer/parsing indicates an error/unexpected size, only
call mbedtls_sha256_finish(&ctx, bootloaderSHA256Cache) and set
bootloaderSHA256CacheValid = true when bootloaderHashOk is still true; on
failure, free the mbedtls context and leave bootloaderSHA256CacheValid false (or
clear the cache) so you never cache a digest derived from partial/invalid reads
(referencing bootloaderSize, chunkSize, esp_flash_read, sizer,
mbedtls_sha256_finish, bootloaderSHA256Cache and bootloaderSHA256CacheValid).
In `@wled00/util.cpp`:
- Around line 1139-1140: Replace the raw ESP_IDF_VERSION comparison with a
platform-aware guard: wrap the existing conditional in a check that the build is
for ESP32 and ESP_IDF_VERSION is defined, e.g. use something like "#if
defined(ARDUINO_ARCH_ESP32) && defined(ESP_IDF_VERSION) && (ESP_IDF_VERSION <
ESP_IDF_VERSION_VAL(5,0,0))" (or the project’s ESP32 macro if different) so the
block around the computeSHA1/getDeviceId-related code in util.cpp only applies
on ESP32 and only when ESP_IDF_VERSION is available; this avoids preprocessor
errors in mixed ESP8266/ESP32 builds.
---
Nitpick comments:
In `@wled00/ota_update.cpp`:
- Around line 567-568: Replace the hardcoded bootloaderOffset value (currently
set to 0x1000) with the target-aware BOOTLOADER_OFFSET constant so the build
uses the per-target definition; update the declaration that uses
bootloaderOffset (and keep maxBootloaderSize as-is) to reference
BOOTLOADER_OFFSET (ensuring the type remains uint32_t) so the code no longer
contains the magic number and follows target-specific configuration.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
package.jsonplatformio.iniusermods/audioreactive/audio_source.husermods/pixels_dice_tray/BLE_REQUIREMENT.mdusermods/pixels_dice_tray/library.jsonwled00/bus_manager.cppwled00/data/update.htmwled00/e131.cppwled00/fcn_declare.hwled00/improv.cppwled00/json.cppwled00/network.cppwled00/ntp.cppwled00/ota_update.cppwled00/ota_update.hwled00/src/dependencies/espalexa/Espalexa.hwled00/udp.cppwled00/util.cppwled00/wled.cppwled00/wled.hwled00/wled_server.cppwled00/ws.cppwled00/xml.cpp
💤 Files with no reviewable changes (2)
- usermods/pixels_dice_tray/library.json
- wled00/ota_update.h
🚧 Files skipped from review as they are similar to previous changes (5)
- wled00/e131.cpp
- wled00/improv.cpp
- wled00/bus_manager.cpp
- wled00/wled_server.cpp
- wled00/fcn_declare.h
| The `library.json` file has been renamed to `library.json.disabled` to prevent this usermod from being automatically included in builds that use `custom_usermods = *` (like the `usermods` environment in platformio.ini). | ||
|
|
||
| The Tasmota Arduino ESP32 platform used by WLED does not include Arduino BLE library by default, which causes compilation failures when this usermod is auto-included. | ||
|
|
There was a problem hiding this comment.
Clarify custom_usermods scope to avoid misleading inclusion behavior.
Line 9 currently reads as if this prevents all auto-inclusion paths. In WLED, this only affects in-tree usermods; out-of-tree usermods from lib_deps can still be active. Please add that distinction explicitly.
✏️ Proposed wording tweak
-The `library.json` file has been renamed to `library.json.disabled` to prevent this usermod from being automatically included in builds that use `custom_usermods = *` (like the `usermods` environment in platformio.ini).
+The `library.json` file has been renamed to `library.json.disabled` to prevent this in-tree usermod from being automatically included in builds that use `custom_usermods = *` (like the `usermods` environment in platformio.ini).
+Note: this does not disable out-of-tree usermods pulled via `lib_deps`.Based on learnings: In WLED, custom_usermods controls only in-tree usermods, while out-of-tree usermods from library dependencies remain active.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/pixels_dice_tray/BLE_REQUIREMENT.md` around lines 9 - 12, Update the
wording in BLE_REQUIREMENT.md to clarify that renaming library.json to
library.json.disabled only prevents auto-inclusion of this usermod via in-tree
mechanisms controlled by custom_usermods (e.g., the WLED in-tree usermods list),
and does not disable out-of-tree usermods brought in via library dependencies;
explicitly mention custom_usermods affects only in-tree usermods while
lib_deps-supplied/out-of-tree usermods (and thus the Arduino BLE library when
provided as a dependency) can still be active and cause compilation to pull in
BLE symbols on Tasmota Arduino ESP32 builds.
| 1. **Copy the sample configuration:** | ||
| ```bash | ||
| cp platformio_override.ini.sample ../../../platformio_override.ini | ||
| ``` |
There was a problem hiding this comment.
Specify the command’s expected working directory.
The copy command in Line 19 is path-sensitive and may fail unless users run it from the usermod directory. Please state the required working directory (or provide a repo-root-safe alternative).
✏️ Proposed wording tweak
-1. **Copy the sample configuration:**
+1. **Copy the sample configuration** (run from `usermods/pixels_dice_tray`):
```bash
cp platformio_override.ini.sample ../../../platformio_override.ini
```📝 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.
| 1. **Copy the sample configuration:** | |
| ```bash | |
| cp platformio_override.ini.sample ../../../platformio_override.ini | |
| ``` | |
| 1. **Copy the sample configuration** (run from `usermods/pixels_dice_tray`): |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/pixels_dice_tray/BLE_REQUIREMENT.md` around lines 17 - 20, Clarify
the working directory for the cp command in BLE_REQUIREMENT.md: either state the
user must run the command from the usermod directory (usermods/pixels_dice_tray)
before running "cp platformio_override.ini.sample
../../../platformio_override.ini", or replace it with a repo-root-safe
alternative such as copying with explicit paths ("cp
usermods/pixels_dice_tray/platformio_override.ini.sample
platformio_override.ini") so the command works regardless of current directory;
update the text to include one of these options and show the adjusted command.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
version tag renaming
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
fixes inconsistencies
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
wled00/ws.cpp (1)
89-94:⚠️ Potential issue | 🔴 CriticalAdd minimum-length validation for E1.31/Art-Net before field access.
Lines 90 and 93 cast and pass packet pointers to
handleE131Packet()with insufficient validation. Line 87 only guarantees 2 bytes total, buthandleE131Packet()immediately dereferences packet structure fields without checking buffer size:
- Art-Net: accesses
art_opcode(offset 8),art_universe(offset 14),art_length(offset 16),art_datawithout validating minimum ~18+ bytes- E1.31: accesses
options,property_value_count,property_values[...]without validating minimum ~40+ bytesUnlike the DDP case (line 96-100), which validates
len < 10 + offsetbefore callinghandleE131Packet(), E1.31 and Art-Net have no pre-flight checks. Add protocol-specific minimum-length checks inwsEvent()before line 90/93:
- E1.31: require minimum packet size before accessing property fields
- Art-Net: require minimum ~18 bytes before accessing art_* fields
This prevents out-of-bounds memory access on malformed packets.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/ws.cpp` around lines 89 - 94, In wsEvent(), before casting data+offset and calling handleE131Packet() for BINARY_PROTOCOL_E131 and BINARY_PROTOCOL_ARTNET, add protocol-specific minimum-length checks: for BINARY_PROTOCOL_ARTNET ensure remaining bytes (len - offset) are at least ~18 bytes to cover art_opcode, art_universe, art_length and art_data header; for BINARY_PROTOCOL_E131 ensure remaining bytes meet the E1.31 minimum (~40 bytes) required to safely read options, property_value_count and property_values. Perform these checks using the existing variables (data, len, offset, client) and only call handleE131Packet((e131_packet_t*)&data[offset], client->remoteIP(), P_E131/P_ARTNET) when the validation passes; otherwise drop/log the packet and return. Ensure you reference BINARY_PROTOCOL_E131, BINARY_PROTOCOL_ARTNET, wsEvent, and handleE131Packet when applying the guards.wled00/ota_update.cpp (1)
724-735:⚠️ Potential issue | 🔴 CriticalWrite only the validated bootloader span returned by verification.
At Line 726,
bytesToWritestarts fromcontext->bytesBufferedand only switches to validated size when pointer was shifted. If the pointer is unchanged, trailing uploaded bytes can still be flashed past the verified image boundary.Suggested fix
- size_t flashOffset = context->bootloaderOffset; - const uint8_t* dataToWrite = context->buffer; - size_t bytesToWrite = context->bytesBuffered; + size_t flashOffset = context->bootloaderOffset; + const uint8_t* dataToWrite = bootloaderData; + size_t bytesToWrite = bootloaderSize; // If validation adjusted the pointer, it means we have a partition table at the start // In this case, we should skip writing the partition table and write bootloader at 0x1000 if (bootloaderData != context->buffer) { // bootloaderData was adjusted - skip partition table in our data size_t partitionTableSize = bootloaderData - context->buffer; - dataToWrite = bootloaderData; - bytesToWrite = bootloaderSize; DEBUG_PRINTF_P(PSTR("Skipping %d bytes of partition table data\n"), partitionTableSize); }Also applies to: 738-745
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/ota_update.cpp` around lines 724 - 735, The code uses context->bytesBuffered for bytesToWrite even when bootloaderData wasn't adjusted, risking flashing extra trailing data; change bytesToWrite to the validated bootloader length returned by verification (use bootloaderSize) whenever writing bootloader spans so that bytesToWrite is always the verified span size; update the logic around flashOffset, dataToWrite, bytesToWrite, and bootloaderData in the write path (and the similar block at the later section lines ~738-745) to ensure you only write the validated bootloaderSize instead of context->bytesBuffered when writing the bootloader region.
♻️ Duplicate comments (5)
wled00/data/update.htm (1)
47-50:⚠️ Potential issue | 🟡 MinorUse a deterministic submit state and explicitly show “Updating…”
At Line 49,
toggle('upd')can produce inconsistent visibility, andNoupdis never shown. This can still leave a blank screen while upload is running.Proposed fix
function hideforms() { - gId('bootupd').classList.add("hide"); - toggle('upd'); + gId('upd').classList.add("hide"); + gId('bootupd').classList.add("hide"); + gId('Noupd').classList.remove("hide"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/update.htm` around lines 47 - 50, hideforms currently calls toggle('upd') which yields inconsistent visibility and never shows the Noupd state; change hideforms to set a deterministic submit state by explicitly showing the uploading indicator and hiding the Noupd message instead of toggling: in function hideforms() (and any caller that expects upload-start behavior) remove toggle('upd') and instead call the DOM helpers to gId('upd').classList.remove('hide') and gId('Noupd').classList.add('hide'), set the submit control to disabled and change its label/text to "Updating…" (so users see progress), and ensure the code path that handles upload failure or completion re-enables the submit button and restores visibility (e.g., by re-showing Noupd when appropriate).usermods/pixels_dice_tray/BLE_REQUIREMENT.md (2)
9-12:⚠️ Potential issue | 🟡 MinorClarify that
custom_usermodsonly controls in-tree usermods.Current wording can be read as globally preventing auto-inclusion, but out-of-tree usermods from
lib_depscan still be active.Based on learnings: In WLED,
custom_usermodscontrols only in-tree usermods, while out-of-tree usermods pulled vialib_depsremain active.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/pixels_dice_tray/BLE_REQUIREMENT.md` around lines 9 - 12, Clarify that disabling library.json (renamed to library.json.disabled) only prevents this in-tree usermod from being auto-included via the custom_usermods mechanism, and does not affect out-of-tree usermods pulled via lib_deps; update the BLE_REQUIREMENT.md text to explicitly mention custom_usermods controls only in-tree usermods and that out-of-tree usermods added through lib_deps (or other external dependency mechanisms) can still be included and cause the same Arduino BLE dependency to be required.
17-20:⚠️ Potential issue | 🟡 MinorMake the copy command location-safe (or state required working directory).
cp platformio_override.ini.sample ../../../platformio_override.iniis cwd-dependent; please either specify it must be run fromusermods/pixels_dice_trayor provide a repo-root-safe command.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/pixels_dice_tray/BLE_REQUIREMENT.md` around lines 17 - 20, The copy command in BLE_REQUIREMENT.md is cwd-dependent; update the instruction to either state explicitly that the command must be run from the usermods/pixels_dice_tray directory or replace the example with a repo-root-safe variant that references the sample file location (the current `cp platformio_override.ini.sample ../../../platformio_override.ini` line) so the copy works regardless of working directory; edit the BLE_REQUIREMENT.md entry containing that cp command to include the chosen clarification or safe path form.platformio.ini (1)
130-130:⚠️ Potential issue | 🟡 Minor
WLED_USE_SHARED_RMTis still defined twice and in broader scope than needed.Defining it in both
[common]and[esp32_idf_V5]is redundant and leaks an ESP32-specific switch into non-ESP32 environments.Also applies to: 337-337
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platformio.ini` at line 130, WLED_USE_SHARED_RMT is defined twice and exposed in the global [common] scope; remove the duplicate from the [common] section and keep the define only inside the ESP32-specific profile ([esp32_idf_V5]) so the flag is scoped to ESP32 builds; update any accompanying comment to reference that this define is ESP32-only and verify no other sections rely on the common definition before removing it.wled00/ota_update.cpp (1)
415-442:⚠️ Potential issue | 🟠 MajorOnly set bootloader SHA cache valid when all flash reads succeed.
Line 417 ignores read failures by skipping the chunk, but Line 441 still marks the cache valid. That can publish an invalid digest.
Suggested fix
static void calculateBootloaderSHA256() { // Calculate SHA256 mbedtls_sha256_context ctx; mbedtls_sha256_init(&ctx); mbedtls_sha256_starts(&ctx, 0); // 0 = SHA256 (not SHA224) const size_t chunkSize = 256; alignas(esp_image_header_t) uint8_t buffer[chunkSize]; size_t bootloaderSize = BOOTLOADER_SIZE; BootloaderImageSizer sizer; size_t totalHashLen = 0; + bool readOk = true; for (uint32_t offset = 0; offset < bootloaderSize; offset += chunkSize) { size_t readSize = min((size_t)(bootloaderSize - offset), chunkSize); - if (esp_flash_read(NULL, buffer, BOOTLOADER_OFFSET + offset, readSize) == ESP_OK) { - sizer.feed(buffer, readSize); + if (esp_flash_read(NULL, buffer, BOOTLOADER_OFFSET + offset, readSize) != ESP_OK) { + readOk = false; + break; + } + sizer.feed(buffer, readSize); size_t hashLen = readSize; if (sizer.isSizeKnown()) { size_t totalSize = sizer.totalSize(); if (totalSize > 0 && totalSize <= BOOTLOADER_SIZE) { bootloaderSize = totalSize; if (offset + readSize > totalSize) { hashLen = (totalSize > offset) ? (totalSize - offset) : 0; } } } if (hashLen > 0) { totalHashLen += hashLen; mbedtls_sha256_update(&ctx, buffer, hashLen); } - } } + if (!readOk || totalHashLen == 0) { + mbedtls_sha256_free(&ctx); + bootloaderSHA256CacheValid = false; + return; + } mbedtls_sha256_finish(&ctx, bootloaderSHA256Cache); mbedtls_sha256_free(&ctx); bootloaderSHA256CacheValid = true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/ota_update.cpp` around lines 415 - 442, The loop that reads flash and updates the SHA256 (for .. offset .. bootloaderSize with esp_flash_read and mbedtls_sha256_update) must detect any esp_flash_read failures and abort computing/marking the cache; add a success flag (e.g., readOk) or break on first esp_flash_read != ESP_OK, call mbedtls_sha256_free and avoid mbedtls_sha256_finish/setting bootloaderSHA256CacheValid unless all chunks read successfully (optionally verify totalHashLen matches expected bootloaderSize via sizer.totalSize()), and ensure bootloaderSHA256CacheValid is set to true only when the full read+finish completed without errors.
🧹 Nitpick comments (1)
platformio.ini (1)
172-172: Consider pinning library refs to commit hashes for build reproducibility.Branch-based refs like
#ESP32-C6and#esp-idf-v5-fixescan drift and make builds non-reproducible across time and machines. While the comments indicate these are intentional patches for active workarounds (RMT driver conflicts, ESP-IDF v5 compatibility), consider pinning to specific commit hashes once patches stabilize or are merged upstream. If frequent updates are expected, document the rationale for using branch refs alongside the inline comments already present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platformio.ini` at line 172, The platformio.ini dependency uses branch refs (e.g., the FastLED ref "#ESP32-C6" and other branch refs like "#esp-idf-v5-fixes") which can drift; update these entries to pin to specific commit hashes (replace "#ESP32-C6" and similar branch tags with the corresponding commit SHA once the patch is stable) or, if you must keep branch refs, add an inline comment documenting the reason and expected update cadence for reproducibility; locate and change the dependency lines referencing FastLED and any esp-idf-related library refs to use commit SHAs or add the documentation note.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/bus_manager.cpp`:
- Around line 417-423: In the ESP-IDF >=5 branch where
ledcAttachChannel(_pins[i], _frequency, _depth - (dithering*4), channel) is
called, check its boolean return value and handle failure: if
ledcAttachChannel(...) returns false, mark the bus/channel as not initialized
(set _valid = false), emit an error/log message indicating which channel/pin
failed (use _pins[i] and channel), and abort further initialization for this
instance (break/return) so we don't leave _valid true when PWM wasn't
configured; ensure any cleanup needed for previously attached channels is
performed before exiting initialization.
---
Outside diff comments:
In `@wled00/ota_update.cpp`:
- Around line 724-735: The code uses context->bytesBuffered for bytesToWrite
even when bootloaderData wasn't adjusted, risking flashing extra trailing data;
change bytesToWrite to the validated bootloader length returned by verification
(use bootloaderSize) whenever writing bootloader spans so that bytesToWrite is
always the verified span size; update the logic around flashOffset, dataToWrite,
bytesToWrite, and bootloaderData in the write path (and the similar block at the
later section lines ~738-745) to ensure you only write the validated
bootloaderSize instead of context->bytesBuffered when writing the bootloader
region.
In `@wled00/ws.cpp`:
- Around line 89-94: In wsEvent(), before casting data+offset and calling
handleE131Packet() for BINARY_PROTOCOL_E131 and BINARY_PROTOCOL_ARTNET, add
protocol-specific minimum-length checks: for BINARY_PROTOCOL_ARTNET ensure
remaining bytes (len - offset) are at least ~18 bytes to cover art_opcode,
art_universe, art_length and art_data header; for BINARY_PROTOCOL_E131 ensure
remaining bytes meet the E1.31 minimum (~40 bytes) required to safely read
options, property_value_count and property_values. Perform these checks using
the existing variables (data, len, offset, client) and only call
handleE131Packet((e131_packet_t*)&data[offset], client->remoteIP(),
P_E131/P_ARTNET) when the validation passes; otherwise drop/log the packet and
return. Ensure you reference BINARY_PROTOCOL_E131, BINARY_PROTOCOL_ARTNET,
wsEvent, and handleE131Packet when applying the guards.
---
Duplicate comments:
In `@platformio.ini`:
- Line 130: WLED_USE_SHARED_RMT is defined twice and exposed in the global
[common] scope; remove the duplicate from the [common] section and keep the
define only inside the ESP32-specific profile ([esp32_idf_V5]) so the flag is
scoped to ESP32 builds; update any accompanying comment to reference that this
define is ESP32-only and verify no other sections rely on the common definition
before removing it.
In `@usermods/pixels_dice_tray/BLE_REQUIREMENT.md`:
- Around line 9-12: Clarify that disabling library.json (renamed to
library.json.disabled) only prevents this in-tree usermod from being
auto-included via the custom_usermods mechanism, and does not affect out-of-tree
usermods pulled via lib_deps; update the BLE_REQUIREMENT.md text to explicitly
mention custom_usermods controls only in-tree usermods and that out-of-tree
usermods added through lib_deps (or other external dependency mechanisms) can
still be included and cause the same Arduino BLE dependency to be required.
- Around line 17-20: The copy command in BLE_REQUIREMENT.md is cwd-dependent;
update the instruction to either state explicitly that the command must be run
from the usermods/pixels_dice_tray directory or replace the example with a
repo-root-safe variant that references the sample file location (the current `cp
platformio_override.ini.sample ../../../platformio_override.ini` line) so the
copy works regardless of working directory; edit the BLE_REQUIREMENT.md entry
containing that cp command to include the chosen clarification or safe path
form.
In `@wled00/data/update.htm`:
- Around line 47-50: hideforms currently calls toggle('upd') which yields
inconsistent visibility and never shows the Noupd state; change hideforms to set
a deterministic submit state by explicitly showing the uploading indicator and
hiding the Noupd message instead of toggling: in function hideforms() (and any
caller that expects upload-start behavior) remove toggle('upd') and instead call
the DOM helpers to gId('upd').classList.remove('hide') and
gId('Noupd').classList.add('hide'), set the submit control to disabled and
change its label/text to "Updating…" (so users see progress), and ensure the
code path that handles upload failure or completion re-enables the submit button
and restores visibility (e.g., by re-showing Noupd when appropriate).
In `@wled00/ota_update.cpp`:
- Around line 415-442: The loop that reads flash and updates the SHA256 (for ..
offset .. bootloaderSize with esp_flash_read and mbedtls_sha256_update) must
detect any esp_flash_read failures and abort computing/marking the cache; add a
success flag (e.g., readOk) or break on first esp_flash_read != ESP_OK, call
mbedtls_sha256_free and avoid mbedtls_sha256_finish/setting
bootloaderSHA256CacheValid unless all chunks read successfully (optionally
verify totalHashLen matches expected bootloaderSize via sizer.totalSize()), and
ensure bootloaderSHA256CacheValid is set to true only when the full read+finish
completed without errors.
---
Nitpick comments:
In `@platformio.ini`:
- Line 172: The platformio.ini dependency uses branch refs (e.g., the FastLED
ref "#ESP32-C6" and other branch refs like "#esp-idf-v5-fixes") which can drift;
update these entries to pin to specific commit hashes (replace "#ESP32-C6" and
similar branch tags with the corresponding commit SHA once the patch is stable)
or, if you must keep branch refs, add an inline comment documenting the reason
and expected update cadence for reproducibility; locate and change the
dependency lines referencing FastLED and any esp-idf-related library refs to use
commit SHAs or add the documentation note.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
package.jsonplatformio.iniusermods/audioreactive/audio_source.husermods/pixels_dice_tray/BLE_REQUIREMENT.mdusermods/pixels_dice_tray/library.jsonwled00/bus_manager.cppwled00/data/update.htmwled00/e131.cppwled00/fcn_declare.hwled00/improv.cppwled00/json.cppwled00/network.cppwled00/ntp.cppwled00/ota_update.cppwled00/ota_update.hwled00/src/dependencies/espalexa/Espalexa.hwled00/udp.cppwled00/util.cppwled00/wled.cppwled00/wled.hwled00/wled_server.cppwled00/ws.cppwled00/xml.cpp
💤 Files with no reviewable changes (2)
- usermods/pixels_dice_tray/library.json
- wled00/ota_update.h
🚧 Files skipped from review as they are similar to previous changes (3)
- wled00/improv.cpp
- wled00/udp.cpp
- wled00/src/dependencies/espalexa/Espalexa.h
| #if ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0) | ||
| ledcSetup(channel, _frequency, _depth - (dithering*4)); // with dithering _frequency doesn't really matter as resolution is 8 bit | ||
| ledcAttachPin(_pins[i], channel); | ||
| #else | ||
| ledcAttachChannel(_pins[i], _frequency, _depth - (dithering*4), channel); | ||
| // LEDC timer reset credit @dedehai | ||
| #endif |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, verify the current state of ledcAttachChannel calls
echo "=== All ledcAttachChannel calls in bus_manager.cpp ==="
rg -nP --type=cpp -C3 'ledcAttachChannel' wled00/bus_manager.cpp
echo -e "\n=== Checking for return value checks on ledcAttachChannel ==="
rg -nP --type=cpp 'if\s*\(\s*!?\s*ledcAttachChannel' wled00/bus_manager.cpp
echo -e "\n=== Full BusPwm::begin method context ==="
sed -n '390,450p' wled00/bus_manager.cppRepository: wled/WLED
Length of output: 3598
🏁 Script executed:
#!/bin/bash
# Check ESP-IDF documentation and other LEDC function behavior
echo "=== Check ledcSetup and ledcAttachPin return handling ==="
rg -nP --type=cpp -C2 'ledcSetup|ledcAttachPin' wled00/bus_manager.cpp | head -50
echo -e "\n=== Check if there are any comments or patterns for LEDC error handling ==="
rg -nP --type=cpp -i 'ledc.*fail|ledc.*error' wled00/bus_manager.cppRepository: wled/WLED
Length of output: 898
🏁 Script executed:
#!/bin/bash
# Check the broader context of pin allocation and cleanup patterns
echo "=== Search for PinManager::deallocateLedc and deallocateMultiplePins patterns ==="
rg -nP --type=cpp 'deallocateLedc|deallocateMultiplePins' wled00/bus_manager.cpp
echo -e "\n=== Check existing error handling patterns in BusPwm ==="
rg -nP --type=cpp -B2 'return false|return;' wled00/bus_manager.cpp | head -80Repository: wled/WLED
Length of output: 3190
🌐 Web query:
ESP-IDF v5 ledcAttachChannel return value documentation
💡 Result:
ledcAttachChannel() is not an ESP-IDF API in v5.x; it’s an Arduino-ESP32 helper function. In Arduino-ESP32, its signature is:
bool ledcAttachChannel(uint8_t pin, uint32_t freq, uint8_t resolution, int8_t channel);and the return value is:
true: configuration successfulfalse: an error occurred and the LEDC channel was not configured [1]
In ESP-IDF v5, the LEDC driver APIs instead typically return esp_err_t (e.g., ESP_OK, ESP_ERR_INVALID_ARG, etc.) rather than bool, as shown in the ESP-IDF LEDC documentation (for functions like ledc_set_duty, ledc_set_duty_and_update, etc.) [2].
Sources: Arduino-ESP32 LEDC API reference [1], ESP-IDF v5.0 LEDC API reference [2].
[1] https://docs.espressif.com/projects/arduino-esp32/en/latest/api/ledc.html
[2] https://docs.espressif.com/projects/esp-idf/en/v5.0/esp32/api-reference/peripherals/ledc.html
Check return value of ledcAttachChannel() and handle initialization failure.
Line 421 ignores the boolean return from ledcAttachChannel(). If the attach fails, initialization silently continues and _valid is set to true despite the LEDC channel not being configured, resulting in no PWM output at runtime.
Proposed fix
`#if` ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)
ledcSetup(channel, _frequency, _depth - (dithering*4)); // with dithering _frequency doesn't really matter as resolution is 8 bit
ledcAttachPin(_pins[i], channel);
`#else`
- ledcAttachChannel(_pins[i], _frequency, _depth - (dithering*4), channel);
+ if (!ledcAttachChannel(_pins[i], _frequency, _depth - (dithering*4), channel)) {
+ DEBUGBUS_PRINTF_P(PSTR("LEDC attach failed (pin:%u ch:%u)\n"), _pins[i], channel);
+ for (unsigned j = 0; j < i; j++) ledcDetach(_pins[j]);
+ PinManager::deallocateLedc(_ledcStart, numPins);
+ PinManager::deallocateMultiplePins(pins, numPins, PinOwner::BusPwm);
+ return;
+ }
// LEDC timer reset credit `@dedehai`
`#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/bus_manager.cpp` around lines 417 - 423, In the ESP-IDF >=5 branch
where ledcAttachChannel(_pins[i], _frequency, _depth - (dithering*4), channel)
is called, check its boolean return value and handle failure: if
ledcAttachChannel(...) returns false, mark the bus/channel as not initialized
(set _valid = false), emit an error/log message indicating which channel/pin
failed (use _pins[i] and channel), and abort further initialization for this
instance (break/return) so we don't leave _valid true when PWM wasn't
configured; ensure any cleanup needed for previously attached channels is
performed before exiting initialization.
|
With regards to your own V5 branch @willmmiles , if you have a neater one then we should decide if we replace the current version in the repo with yours before we get too far down the rabbit hole. I'm not precious about the work I did so far, it was more of a discovery of what was broken |
I'm reviewing it in more depth thismorning and I think some kind of combination might be the way. I took very different approaches on fixing a few things like third-party library integration (the FastLED RMT driver conflict should never have happened because we never call that code -- it's actually caused by current versions of pioarduino/tasmota playing games with |
|
I don't want to delay version 16 further by trying to swap to V5 I think we need to get 16 released, create 16_x branch probably at the point of beta We can then merge in V5 into main. I'm keen to avoid different platform versions in the same release, so I would prefer we didn't have the C5, C6 and P4 on V5 and the rest still on V4 This would potentially make main and right nightly builds way less stable than they have been if we merge V5 "too soon", but maybe that if something we just message as best we can to the users? It would certainly be good to have discussion as to what the criteria are for when V5 is our main development version rather than an experimental branch |
I think this we will have to use two platform versions for some time, at least in wled release 17 - so the mainline of MCUs (esp32, s3,s2,c3) will be on V4, while the newer C5, C6 and P4 will be marked "experimental" on V5. If you look at the long list of open points in the V5 and C6-V5 branch, my feeling is we should not expose our users to a new major framework release unless strictly necessary. We did the same with the V4 framework (S3,S2,C3) in wled releases 0.14 and 0.15, and in retrospect I think it was the right decision. Even if it lead to classic esp32 running an "ancient" framework, I think users had almost zero problems and they never really complained. Ok just two issues I can recall - wifi WPA3 support and the "FritzBox" bug, plus limited support for newer ethernet chips. For the FritzBox problem, we pointed people to the never "esp32 V4" builds. |
|
I do really worry with this approach @softhack007 we underestimate the dev effort that is spent trying to work out of an issues we face relate to the new hardware of the new platform. If we wrongly attribute to the hardware, then when we come to update esp32 classic then we are going to hit all those issues all over again. We really have very little development resource and with no automated testing, any code that is dependent on chipset or platform variation is a very significant development and testing cost, which if we are honest I would say is not always tested for the current set of variants, let alone when we add 3 more The ESP-IDF v4.4 release branch reached End-of-Life (EOL) in July 2024 - so we desperately need to catch up! Pushing back to most likely 2027 until we get the vast majority of our users onto a supported platform if we go with a dual platform approach for v16 doesn't seem great |
|
@netmindz yes I agree with your point about development effort and resources. Maybe we need to think about it a bit more. The current V5 branch would also allow for this: make V5 the official platform in 17.0, and keep compiling with V4 as a special developers options mainly for back-to-back testing and to help with porting. My main concern with V5 is really the sheer number of "opps" and small open ends that I found. We can "embrace" V5, but it will force us to fix all these open ends before we can even think about releasing wled 17.0. For sure it's possible to go the V5 way, and it's a good investment of our time because we have to do that any way, so the only question is "when", not "if". |
|
C3 builds are failing after sync/merge with main
@willmmiles any idea? |
Newer IDF v5s have added some extra validations to the linker scripts on RISCV platforms. In some sense, the challenge here is I should hopefully have an improved version ready tonight. |
Ready was a stretch - I'm out of time and it's not fully tested. The build time validations pass but I haven't had a chance to upload it to real devices; I'll resume tomorrow. willmmiles@149acc9 is the key commit. |
Update the code to handle at least being able to compile against V5
Edit (softhack007): espressif has "silently" changed a lot of APIs, and - in many cases - replacements are not behaving the same way as before. We are lucky that a few "legacy APIs" are still available, like the "V4" I2S driver. So even when "it compiles", there may be a lot more adaptations needed before "it really works".
Helpful information and migration guidance
coding guides
We should make sure that the "V5" branch will still compile for "V4" as a backup solution.
When adding code that only works in the new framework, it should be conditionally compiled, like in the examples below
related
📋 Major Open Points Checklist for ESP-IDF V5 Port
🔧 Core WLED Code
PWM/LEDC (Analog LEDs)
NeoEsp32RmtHI
"ToDO: check if NeoESP32RmtHI is still needed with V5 (see discussion in PR#4838)"MQTT
"TODO: remove once we have updated library for V5"Infrared (IR)
Fix device identification functions in
util.cpp(ref)computeSHA1()andgetDeviceId()currently disabled for ESP-IDF ≥5.0esp_chip_info.h,esp_efuse.h,esp_mac.hMigrate
audioreactiveusermod I2S driver (ref)Replace deprecated ADC constant (ref)
ADC_ATTEN_DB_11→ADC_ATTEN_DB_12inutil.cpp:1177🏗️ Build System & Dependencies
Analyze WLED_VERSION build warning (ref)
Fix PlatformIO dependency references (ref)
#bc3af18) with long hashes or .zip URLsfatal: couldn't find remote referrorsDecide on platform package strategy (ref)
Disable Usermod CI for V5 branch (ref)
🔌 Usermods Compatibility
Update
pwm_outputsusermod for V5 (ref)PWM_fan.cpp(already fixed)Add version guards to
deep_sleepusermod (ref)rtc_gpio_*()calls need ESP-IDF version protectiondeep_sleep.cpp✅ Items Already Resolved
bus_manager.cppandPWM_fan.cpp-DESP32_ARDUINO_NO_RGB_BUILTINflag)Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation
Refactor