Wled hsv - #5685
Conversation
Defer calling begin() on buses
Add support for ESP32-S3 WROOM-2 (solves wled#4099)
Fix release name macro expansion
Fix release name macro expansion
Fixed point calculation for improved accuracy, dithering in debug builds only. Averaging and optional multiplier can be set as compile flags, example for speed testing with long averaging and a 10x multiplier: -D FPS_CALC_AVG=200 -D FPS_MULTIPLIER=10 The calculation resolution is limited (9.7bit fixed point) so values larger than 200 can hit resolution limit and get stuck before reaching the final value. If WLED_DEBUG is defined, dithering is added to the returned value so sub-frame accuracy is possible in post-processingwithout enabling the multiplier.
dithering is not really needed, the FPS_MULTIPLIER is a much better option.
Avoiding name collisions with the 'delay' function.
Don't generate a response if there's no HTTP request. Fixes wled#4269
Fixes bug introduced by wled#4312.
While not used by most bus types, it's not an optional parameter.
Add report version feature
* Initial plan * Convert PSRAM from bytes to MB in usage reporting JavaScript Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com> * Use 1024*1024 instead of magic number for bytes to MB conversion Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com>
not needed yet, but will make maintenance easier in the future, and avoid confusion.
# Conflicts: # wled00/util.cpp
Dynamic LED type selection, backport to 0.15
fix for wled#4298 - no conflict with DMX output - backport
Shim in Makuna/NeoPixelBus#894 until approved by upstream. Fixes wled#4906 and wled#5136.
Backport fix ESP8266 DMA off-by-one to 0.15
This reverts commit 4995f05.
Includes bonus fix for ESP32 DMA driver, too! Replaces wled#5139.
0.15 - Replace wled#5138 with upstream NeoPixelBus fix
WalkthroughThis PR adds metadata-based OTA validation and recovery flows, refactors ESP32 bus and segment output handling, updates several runtime and UI behaviors, introduces an HSV ticker usermod, and refreshes build, release, and PlatformIO configuration. ChangesCore runtime, OTA, buses, and UI
HSV ticker usermod
Build, release, and documentation
Sequence Diagram(s)sequenceDiagram
participant Usermod
participant footballData
participant OpenLigaDB
participant Presets
Usermod->>footballData: fetch match data
footballData-->>Usermod: match response
Usermod->>OpenLigaDB: fetch fallback league data
OpenLigaDB-->>Usermod: match response
Usermod->>Presets: apply scheduled/live/goal preset
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches⚔️ Resolve merge conflicts
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
wled00/led.cpp (1)
84-89:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove duplicate
strip.trigger()in the zero-transition path.
applyFinalBri()now already callsstrip.trigger(), butstateUpdated()triggers again immediately after it whenstrip.getTransition() == 0. This adds redundant work for the same update.Proposed fix
if (strip.getTransition() == 0) { jsonTransitionOnce = false; transitionActive = false; applyFinalBri(); - strip.trigger(); return; }Also applies to: 133-138
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/led.cpp` around lines 84 - 89, The applyFinalBri() function now calls strip.trigger(), so when stateUpdated() calls applyFinalBri() and then immediately calls strip.trigger() again in the zero-transition path (when strip.getTransition() == 0), it creates a duplicate trigger. Remove the redundant strip.trigger() call in the zero-transition conditional branch within stateUpdated() that executes after applyFinalBri() is called, and also apply the same fix to the similar code block mentioned in lines 133-138.wled00/wled.cpp (2)
403-412:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix dangling
elsein FS init flow (build breaker).
handleBootLoop();is now inserted betweenif (!fsinit) { ... }andelse deEEP();. InWLED_ADD_EEPROM_SUPPORTbuilds this creates an invalidif/elsestructure and breaks compilation.Proposed fix
if (!fsinit) { DEBUGFS_PRINTLN(F("FS failed!")); errorFlag = ERR_FS_BEGIN; - } - - handleBootLoop(); // check for bootloop and take action (requires WLED_FS) - -#ifdef WLED_ADD_EEPROM_SUPPORT - else deEEP(); -#else - initPresetsFile(); -#endif + } else { + handleBootLoop(); // check for bootloop and take action (requires WLED_FS) +#ifdef WLED_ADD_EEPROM_SUPPORT + deEEP(); +#else + initPresetsFile(); +#endif + } updateFSInfo();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/wled.cpp` around lines 403 - 412, The handleBootLoop() call inserted between the if (!fsinit) block and the else deEEP() statement creates a dangling else, which breaks compilation in WLED_ADD_EEPROM_SUPPORT builds. Move the handleBootLoop() call to after the entire if/else/ifdef block structure (after the closing of the WLED_ADD_EEPROM_SUPPORT conditional block) so that the else remains properly paired with its if statement.
808-814:⚠️ Potential issue | 🟠 MajorSet WiFi hostname before
WiFi.begin()to preserve DHCP hostname.
WiFi.setHostname(hostname)is called afterWiFi.begin(...)(lines 813/816), but the proper initialization sequence requires hostname to be set afterWiFi.mode()but beforeWiFi.begin(). This causes DHCP to use the defaultesp-XXXXXXhostname instead of the configured WLED hostname.Move the hostname configuration calls before
WiFi.begin()for both ESP32 and ESP8266 code paths:
- Line 808:
WiFi.begin()should be moved to afterWiFi.setHostname()/WiFi.hostname()- Line 813/816:
WiFi.setHostname()andWiFi.hostname()calls should precedeWiFi.begin()Proposed fix
char hostname[25]; prepareHostname(hostname); - WiFi.begin(multiWiFi[selectedWiFi].clientSSID, multiWiFi[selectedWiFi].clientPass); // no harm if called multiple times `#ifdef` ARDUINO_ARCH_ESP32 + WiFi.setHostname(hostname); + WiFi.begin(multiWiFi[selectedWiFi].clientSSID, multiWiFi[selectedWiFi].clientPass); // no harm if called multiple times WiFi.setTxPower(wifi_power_t(txPower)); WiFi.setSleep(!noWifiSleep); - WiFi.setHostname(hostname); `#else` + WiFi.hostname(hostname); + WiFi.begin(multiWiFi[selectedWiFi].clientSSID, multiWiFi[selectedWiFi].clientPass); // no harm if called multiple times wifi_set_sleep_type((noWifiSleep) ? NONE_SLEEP_T : MODEM_SLEEP_T); - WiFi.hostname(hostname); `#endif`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/wled.cpp` around lines 808 - 814, The WiFi hostname must be configured before WiFi.begin() is called, but currently WiFi.setHostname(hostname) on the ESP32 path and the corresponding WiFi.hostname() call on the ESP8266 path (in the `#else` branch) are being executed after WiFi.begin(). Reorder the code by moving the WiFi.setHostname(hostname) call for ESP32 and the WiFi.hostname() call for ESP8266 to execute before the WiFi.begin() call on line 808. This ensures that DHCP will use the configured hostname instead of the default ESP hostname.Source: Learnings
.github/workflows/release.yml (1)
1-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeclare explicit workflow permissions (least privilege).
This workflow has no explicit
permissions:block. Add scoped permissions (for this flow, typicallycontents: write) to avoid default-token overreach.🔧 Suggested fix
name: WLED Release CI on: push: tags: - '*' + +permissions: + contents: writeAs per coding guidelines:
.github/workflows/*.{yml,yaml}must declare explicitpermissions:blocks scoped to least privilege.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml around lines 1 - 17, The release workflow is missing an explicit permissions block at the top level, which violates the least privilege principle. Add a `permissions:` block after the `on:` section and before the `jobs:` section, specifying `contents: write` permission which is required for the release job to create releases and write to the repository. This ensures the workflow operates with only the minimum necessary permissions instead of using default overreach.Source: Coding guidelines
wled00/FX_fcn.cpp (1)
461-490:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset effect state when grouping or mapping changes.
setGeometry()updatesgrouping,spacing,offset, andmap1D2D, then returns onboundsUnchangedbeforemarkForReset(). Grouping/mapping changes alter virtual geometry, so effects can keep stale buffers/counters sized for the old layout.🐛 Proposed fix sketch
- m12 = constrain(m12, 0, 7); - if (stop && (spc > 0 || m12 != map1D2D)) fill(BLACK); - if (m12 != map1D2D) map1D2D = m12; + m12 = constrain(m12, 0, 7); + const bool groupingUnchanged = grp ? (grouping == grp && spacing == spc) : (grouping == 1 && spacing == 0); + const bool offsetUnchanged = (ofs == UINT16_MAX || offset == ofs); + const bool mappingUnchanged = (m12 == map1D2D); + if (boundsUnchanged && groupingUnchanged && offsetUnchanged && mappingUnchanged) return; + + if (stop && (spc > 0 || !mappingUnchanged)) fill(BLACK); + if (!mappingUnchanged) map1D2D = m12; ... - if (boundsUnchanged) return; markForReset(); + if (boundsUnchanged) return;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/FX_fcn.cpp` around lines 461 - 490, The setGeometry() function returns early when boundsUnchanged is true, which prevents markForReset() from being called when grouping, spacing, offset, or map1D2D have changed. Since these parameters affect virtual geometry and effects may have cached buffers or counters sized for the old layout, markForReset() must be called whenever any of these parameters change, not just when bounds change. Modify the early return condition to also check whether grouping, spacing, offset, or map1D2D have actually changed compared to their previous values, and only return early if neither bounds nor any of these parameters have changed. Alternatively, store the old parameter values, update them, and then conditionally call markForReset() before the early return based on whether any of these parameters differ from their previous values.wled00/fcn_declare.h (1)
115-124:⚠️ Potential issue | 🔴 CriticalFix const-correctness and filter parameter in file API declarations.
The String write wrappers pass
const JsonDocument*to non-const C-string declarations, causing a type mismatch at compile time. The read wrappers accept an optionalfilterparameter but discard it when forwarding to C-string versions.Update declarations in
wled00/fcn_declare.hto match the wrapper signatures:Proposed fix
-bool writeObjectToFileUsingId(const char* file, uint16_t id, JsonDocument* content); -bool writeObjectToFile(const char* file, const char* key, JsonDocument* content); -bool readObjectFromFileUsingId(const char* file, uint16_t id, JsonDocument* dest); -bool readObjectFromFile(const char* file, const char* key, JsonDocument* dest); +bool writeObjectToFileUsingId(const char* file, uint16_t id, const JsonDocument* content); +bool writeObjectToFile(const char* file, const char* key, const JsonDocument* content); +bool readObjectFromFileUsingId(const char* file, uint16_t id, JsonDocument* dest, const JsonDocument* filter = nullptr); +bool readObjectFromFile(const char* file, const char* key, JsonDocument* dest, const JsonDocument* filter = nullptr);Update the read wrapper implementations to forward the
filterparameter:-inline bool readObjectFromFileUsingId(const String &file, uint16_t id, JsonDocument* dest, const JsonDocument* filter = nullptr) { return readObjectFromFileUsingId(file.c_str(), id, dest); }; -inline bool readObjectFromFile(const String &file, const char* key, JsonDocument* dest, const JsonDocument* filter = nullptr) { return readObjectFromFile(file.c_str(), key, dest); }; +inline bool readObjectFromFileUsingId(const String &file, uint16_t id, JsonDocument* dest, const JsonDocument* filter = nullptr) { return readObjectFromFileUsingId(file.c_str(), id, dest, filter); }; +inline bool readObjectFromFile(const String &file, const char* key, JsonDocument* dest, const JsonDocument* filter = nullptr) { return readObjectFromFile(file.c_str(), key, dest, filter); };Update the definitions in
wled00/file.cppaccordingly to accept and handle the new parameters.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/fcn_declare.h` around lines 115 - 124, The const-correctness issue exists because the String wrapper implementations for writeObjectToFileUsingId and writeObjectToFile pass const JsonDocument* to the C-string function declarations which expect non-const JsonDocument*, causing a type mismatch. Additionally, the String wrapper implementations for readObjectFromFileUsingId and readObjectFromFile accept an optional filter parameter but discard it instead of forwarding it. Fix this by updating the C-string function declarations to accept const JsonDocument* for the content parameter in all four functions, add the optional filter parameter to the C-string read declarations (readObjectFromFileUsingId and readObjectFromFile), update the inline wrapper implementations to forward the filter parameter to the underlying C-string versions, and then update the actual function implementations in file.cpp to match these corrected signatures and handle the filter parameter appropriately.wled00/remote.cpp (2)
199-199:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCast sequence bytes before shifting.
incoming->seq[3] << 24promotes the byte to signedint; values >= 128 can shift into the sign bit and invoke undefined behavior. Cast each byte touint32_tbefore shifting.🐛 Proposed fix
- uint32_t cur_seq = incoming->seq[0] | (incoming->seq[1] << 8) | (incoming->seq[2] << 16) | (incoming->seq[3] << 24); + uint32_t cur_seq = (uint32_t)incoming->seq[0] + | ((uint32_t)incoming->seq[1] << 8) + | ((uint32_t)incoming->seq[2] << 16) + | ((uint32_t)incoming->seq[3] << 24);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/remote.cpp` at line 199, The byte values from the incoming->seq array are being promoted to signed int before the left shift operations, which can cause undefined behavior when byte values are >= 128 (sign bit gets set). In the cur_seq assignment line in remote.cpp, cast each incoming->seq element to uint32_t before performing the left shift operations to ensure the shifts operate on unsigned values without sign bit issues. Apply the cast to all four bytes in the expression: incoming->seq[0], incoming->seq[1], incoming->seq[2], and incoming->seq[3].
216-236:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not clear a button that arrives during processing.
remoteJson()can take time due FS/JSON work; if the ESP-NOW callback stores a newESPNowButtonduring that window, Line 236 overwrites it with-1. Atomically copy-and-clear the pending button before processing so newly arrived buttons remain queued for the next loop.🐛 Directional fix
void handleRemote() { - if(ESPNowButton >= 0) { - if (!remoteJson(ESPNowButton)) - switch (ESPNowButton) { + // Use an atomic exchange / short critical section here on ESP32. + const int16_t button = ESPNowButton; + ESPNowButton = -1; + if (button >= 0) { + if (!remoteJson(button)) + switch (button) { case WIZMOTE_BUTTON_ON : setOn(); break; case WIZMOTE_BUTTON_OFF : setOff(); break; case WIZMOTE_BUTTON_ONE : presetWithFallback(1, FX_MODE_STATIC, 0); break; case WIZMOTE_BUTTON_TWO : presetWithFallback(2, FX_MODE_BREATH, 0); break; case WIZMOTE_BUTTON_THREE : presetWithFallback(3, FX_MODE_FIRE_FLICKER, 0); break; case WIZMOTE_BUTTON_FOUR : presetWithFallback(4, FX_MODE_RAINBOW, 0); break; case WIZMOTE_BUTTON_NIGHT : activateNightMode(); break; case WIZMOTE_BUTTON_BRIGHT_UP : brightnessUp(); break; case WIZMOTE_BUTTON_BRIGHT_DOWN : brightnessDown(); break; case WIZ_SMART_BUTTON_ON : setOn(); break; case WIZ_SMART_BUTTON_OFF : setOff(); break; case WIZ_SMART_BUTTON_BRIGHT_UP : brightnessUp(); break; case WIZ_SMART_BUTTON_BRIGHT_DOWN : brightnessDown(); break; default: break; } } - ESPNowButton = -1; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/remote.cpp` around lines 216 - 236, The issue is that ESPNowButton is cleared to -1 at the end of handleRemote() after processing, but remoteJson() can take time and a new button might arrive via the ESP-NOW callback during this window, causing it to be overwritten and lost. To fix this, at the start of the handleRemote() function, before calling remoteJson() or accessing ESPNowButton, atomically copy the current ESPNowButton value to a local variable and immediately reset ESPNowButton to -1. Then use this local variable for all subsequent operations including the remoteJson() call and the switch statement cases. This ensures newly arrived buttons are preserved for the next loop iteration and won't be lost during processing.wled00/bus_manager.cpp (2)
182-207:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a wide temporary before storing estimated current.
busPowerSum * actualMilliampsPerLed * _brieasily exceeds 32 bits for large strips, and assigning the result touint16_t _milliAmpsTotalbefore comparing can wrap belowpowerBudget, disabling the limiter.🐛 Proposed fix
- BusDigital::_milliAmpsTotal = (busPowerSum * actualMilliampsPerLed * _bri) / (765*255); + uint32_t estimatedMilliamps = (uint64_t(busPowerSum) * actualMilliampsPerLed * _bri) / (765ULL * 255ULL); uint8_t newBri = _bri; - if (BusDigital::_milliAmpsTotal > powerBudget) { + if (estimatedMilliamps > powerBudget) { //scale brightness down to stay in current limit - unsigned scaleB = powerBudget * 255 / BusDigital::_milliAmpsTotal; + unsigned scaleB = powerBudget * 255 / estimatedMilliamps; newBri = (_bri * scaleB) / 256 + 1; - BusDigital::_milliAmpsTotal = powerBudget; + BusDigital::_milliAmpsTotal = powerBudget > UINT16_MAX ? UINT16_MAX : powerBudget; //_milliAmpsTotal = (busPowerSum * actualMilliampsPerLed * newBri) / (765*255); + } else { + BusDigital::_milliAmpsTotal = estimatedMilliamps > UINT16_MAX ? UINT16_MAX : estimatedMilliamps; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/bus_manager.cpp` around lines 182 - 207, The calculation of milliamps total where BusDigital::_milliAmpsTotal is assigned the result of (busPowerSum * actualMilliampsPerLed * _bri) / (765*255) can overflow a 32-bit integer for large LED strips before the division is performed. Use a wide temporary variable (such as uint64_t) to store the intermediate multiplication result of busPowerSum * actualMilliampsPerLed * _bri before performing the division and assigning to _milliAmpsTotal. This ensures the full value is calculated before any truncation occurs, preventing the power limiter comparison from failing due to wraparound.
899-925:⚠️ Potential issue | 🔴 CriticalFix the RMT bus counter increment and missing semicolon.
Line 925 is missing a semicolon and causes a syntax error. More critically, the counter increments after all platform-specific checks and continue statements, which means buses skipped by the conditionals at lines 913 and 923 never advance
u. This breaks the 1:1 bus-to-RMT channel mapping documented in the comment at line 917.Move the increment to immediately after the first filter passes so it takes effect before any subsequent checks:
Proposed fix
unsigned rmt = 0; unsigned u = 0; for (auto &bus : busses) { if (bus->getLength()==0 || !bus->isDigital() || bus->is2Pin()) continue; + const unsigned busIndex = u++; `#if` defined(CONFIG_IDF_TARGET_ESP32C3) // 2 RMT, only has 1 I2S but NPB does not support it ATM - if (u > 1) return; - rmt = u; + if (busIndex > 1) return; + rmt = busIndex; `#elif` defined(CONFIG_IDF_TARGET_ESP32S2) // 4 RMT, only has 1 I2S bus, supported in NPB - if (u > 3) return; - rmt = u; + if (busIndex > 3) return; + rmt = busIndex; `#elif` defined(CONFIG_IDF_TARGET_ESP32S3) // 4 RMT, has 2 I2S but NPB does not support them ATM - if (u > 3) return; - rmt = u; + if (busIndex > 3) return; + rmt = busIndex; `#else` unsigned numI2S = !PolyBus::isParallelI2S1Output(); // if using parallel I2S, RMT is used 1st - if (numI2S > u) continue; - if (u > 7 + numI2S) return; - rmt = u - numI2S; + if (busIndex < numI2S) continue; + if (busIndex > 7 + numI2S) return; + rmt = busIndex - numI2S; `#endif` //assumes that bus number to rmt channel mapping stays 1:1 rmt_channel_t ch = static_cast<rmt_channel_t>(rmt); @@ -920,7 +920,6 @@ void BusManager::esp32RMTInvertIdle() { if (lvl == RMT_IDLE_LEVEL_HIGH) lvl = RMT_IDLE_LEVEL_LOW; else if (lvl == RMT_IDLE_LEVEL_LOW) lvl = RMT_IDLE_LEVEL_HIGH; else continue; rmt_set_idle_level(ch, idle_out, lvl); - u++ } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/bus_manager.cpp` around lines 899 - 925, The counter variable `u` is incremented at the end of the loop iteration after the continue statements, which means buses that are skipped by the conditional checks at lines 913 and 923 never increment the counter. This breaks the 1:1 bus-to-RMT channel mapping. Additionally, line 925 is missing a semicolon. Move the `u++` increment to immediately after the first filter condition that checks `bus->getLength()==0 || !bus->isDigital() || bus->is2Pin()` so that the counter increments for all buses that pass the initial filter, regardless of whether they are later skipped by platform-specific checks, and add the missing semicolon to the increment statement.wled00/data/settings_leds.htm (1)
616-627:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix config import after the
chrID()change.Line 616 references
oMaxB, which is no longer defined here, so LED config import throws before loading outputs. The same block also uses raw numericifor form names; bus 10+ is generated withA,B, etc. bychrID(), so those lookups miss the actual fields.🐛 Proposed fix
- for (var i=0; i<oMaxB+maxV; i++) addLEDs(-1); + for (var i=0; i<maxB+maxV; i++) addLEDs(-1); var l = c.hw.led; l.ins.forEach((v,i,a)=>{ addLEDs(1); - for (var j=0; j<v.pin.length; j++) d.getElementsByName(`L${j}${i}`)[0].value = v.pin[j]; - d.getElementsByName("LT"+i)[0].value = v.type; - d.getElementsByName("LS"+i)[0].value = v.start; - d.getElementsByName("LC"+i)[0].value = v.len; - d.getElementsByName("CO"+i)[0].value = v.order; - d.getElementsByName("SL"+i)[0].value = v.skip; - d.getElementsByName("RF"+i)[0].checked = v.ref; - d.getElementsByName("CV"+i)[0].checked = v.rev; + const s = chrID(i); + for (var j=0; j<v.pin.length; j++) d.getElementsByName(`L${j}${s}`)[0].value = v.pin[j]; + d.getElementsByName("LT"+s)[0].value = v.type; + d.getElementsByName("LS"+s)[0].value = v.start; + d.getElementsByName("LC"+s)[0].value = v.len; + d.getElementsByName("CO"+s)[0].value = v.order; + d.getElementsByName("SL"+s)[0].value = v.skip; + d.getElementsByName("RF"+s)[0].checked = v.ref; + d.getElementsByName("CV"+s)[0].checked = v.rev; });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/data/settings_leds.htm` around lines 616 - 627, The config import block references the undefined variable oMaxB on line 616 and uses raw numeric indices for form field name lookups, which breaks for bus indices 10 and above where chrID() converts them to letters. Remove or fix the oMaxB reference that pre-populates LED entries with addLEDs(-1), and update all getElementsByName() calls in the forEach loop to use chrID(i) instead of raw i when constructing field names like L${j}${i}, LT${i}, LS${i}, LC${i}, CO${i}, SL${i}, RF${i}, and CV${i} so that bus 10 gets the correct letter identifier (A, B, etc.) when looking up form fields.
🟠 Major comments (20)
.github/workflows/release.yml-29-29 (1)
29-29:⚠️ Potential issue | 🟠 MajorUpgrade
softprops/action-gh-releaseto v3 before merging.Line 29 uses
@v1, which relies on deprecated Node.js runtimes no longer supported on current GitHub-hosted runners. Upgrade to@v3(uses Node 24), or@v2.6.2if Node 20 compatibility is required.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml at line 29, The softprops/action-gh-release action on line 29 is using version `@v1`, which relies on deprecated Node.js runtimes no longer supported on current GitHub-hosted runners. Update the action reference from `@v1` to `@v3` for Node 24 support, or use `@v2.6.2` if Node 20 compatibility is required for your workflow.Source: Linters/SAST tools
wled00/wled_metadata.cpp-107-109 (1)
107-109:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBound
release_namebefore hashing untrusted metadata.At Line 107,
djb2_hash_runtime(candidate.release_name)assumes a null-terminated string.candidateis copied from untrusted binary bytes, so a missing terminator can make the hash walk past the struct boundary.🔧 Suggested fix
- // Validate hash using runtime function - uint32_t expected_hash = djb2_hash_runtime(candidate.release_name); + // Validate hash using runtime function (bounded to struct field) + const size_t relLen = strnlen(candidate.release_name, WLED_RELEASE_NAME_MAX_LEN); + if (relLen == WLED_RELEASE_NAME_MAX_LEN) { + DEBUG_PRINTF_P(PSTR("Found WLED structure at offset %u but release_name is not null-terminated\n"), offset); + continue; + } + uint32_t expected_hash = djb2_hash_runtime(candidate.release_name);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/wled_metadata.cpp` around lines 107 - 109, The call to djb2_hash_runtime(candidate.release_name) at line 107 assumes release_name is null-terminated, but since candidate is populated from untrusted binary metadata, the string may not have a null terminator and the hash function could read past the struct boundary. Before hashing with djb2_hash_runtime, ensure that candidate.release_name is properly null-terminated by either explicitly adding a null terminator to the field after copying candidate from the binary data, or by using a bounded version of the hash function that takes a length parameter to prevent reading past the struct bounds.wled00/ota_update.cpp-292-299 (1)
292-299:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail hash computation when any bootloader read chunk fails.
At Lines 292-299, failed flash reads are ignored and hashing continues. That can cache an incorrect SHA256 as if it were valid.
🔧 Suggested fix
const size_t chunkSize = 256; uint8_t buffer[chunkSize]; + bool readOk = true; for (uint32_t offset = 0; offset < BOOTLOADER_SIZE; offset += chunkSize) { size_t readSize = min((size_t)(BOOTLOADER_SIZE - offset), chunkSize); `#if` ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(4, 4, 0) - if (esp_flash_read(NULL, buffer, BOOTLOADER_OFFSET + offset, readSize) == ESP_OK) { // use esp_flash_read for V4 framework (-S2, -S3, -C3) + if (esp_flash_read(NULL, buffer, BOOTLOADER_OFFSET + offset, readSize) == ESP_OK) { `#else` - if (spi_flash_read(BOOTLOADER_OFFSET + offset, buffer, readSize) == ESP_OK) { // use spi_flash_read for old V3 framework (legacy esp32) + if (spi_flash_read(BOOTLOADER_OFFSET + offset, buffer, readSize) == ESP_OK) { `#endif` mbedtls_sha256_update(&ctx, buffer, readSize); + } else { + readOk = false; + break; } } + + if (!readOk) { + mbedtls_sha256_free(&ctx); + bootloaderSHA256HexCache = ""; + DEBUG_PRINTLN(F("Failed to read full bootloader for SHA256")); + return; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/ota_update.cpp` around lines 292 - 299, The bootloader read operation at the conditional block (lines checking ESP_IDF_VERSION with esp_flash_read and spi_flash_read) only updates the hash when the read succeeds, but silently continues when it fails. When a flash read returns anything other than ESP_OK, the loop should terminate and the hash computation should fail rather than continue with an incomplete hash. Add error handling that breaks out of the loop or returns an error status whenever either the esp_flash_read or spi_flash_read call fails to return ESP_OK.wled00/usermod_v2_hsv_ticker.h-152-236 (1)
152-236:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMove long HTTPS polling out of the main WLED loop.
loop()can enter synchronous requests with multi-second connect/read windows; a failed fd.org request alone can occupy the main loop for tens of seconds, stalling effects, UI handling, and other services. Use a short-budget state machine or background worker with queued results.Also applies to: 240-258, 621-633
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/usermod_v2_hsv_ticker.h` around lines 152 - 236, The `fdRequest` function contains multiple long-duration synchronous blocking operations with timeouts spanning tens of seconds, which halts the main WLED loop and prevents effects and UI handling from executing. Refactor this function into a non-blocking state machine that maintains state between loop iterations, breaking the connection sequence and body reading into smaller segments with short timeouts, and storing the HTTP response in a buffer or queue that can be checked asynchronously rather than blocking until the entire response is received.wled00/udp.cpp-980-982 (1)
980-982:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the ESP-NOW payload length before reading
data[0].A zero-length ESP-NOW frame reaches this block before the later
len < 3check, sodata[0]can be read out of bounds. As per coding guidelines, ESP-NOW payloads are raw radio bytes and values are trusted only after validation.🛡️ Proposed fix
// handle WiZ Mote data + if (len == 0) { + DEBUG_PRINTLN(F("ESP-NOW empty packet.")); + return; + } if (data[0] == 0x91 || data[0] == 0x81 || data[0] == 0x80) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/udp.cpp` around lines 980 - 982, Before accessing data[0] in the conditional that checks if data[0] equals 0x91, 0x81, or 0x80, add a length guard to ensure len is greater than 0. This prevents out-of-bounds memory access when handling zero-length ESP-NOW frames. Wrap the existing data[0] comparison check with a length validation condition that verifies len > 0 first.Source: Coding guidelines
wled00/usermod_v2_hsv_ticker.h-153-155 (1)
153-155:⚠️ Potential issue | 🟠 MajorReplace
setInsecure()with proper TLS certificate validation for API requests carrying tokens.The usermod disables TLS verification for api.football-data.org (line 154) and OpenLigaDB (line 242) requests. Since these calls transmit authentication tokens, disabling certificate validation creates a MITM vulnerability allowing token interception and response tampering. Use
setCACert()with pinned root CAs for both endpoints. The Google connectivity check at line 581 should also use validation; if needed for compatibility, isolate it from authenticated API calls and document the intent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/usermod_v2_hsv_ticker.h` around lines 153 - 155, The WiFiClientSecure object uses setInsecure() to disable TLS certificate validation, which creates a Man-in-the-Middle vulnerability for authenticated API requests transmitting tokens to api.football-data.org and OpenLigaDB endpoints. Replace all setInsecure() calls with setCACert() method calls that provide pinned root CA certificates for each respective API endpoint to properly validate TLS connections. For the Google connectivity check, apply the same certificate validation unless there is a documented compatibility reason, in which case isolate it from the authenticated API calls and add a clear comment explaining the exception.wled00/usermod_v2_hsv_ticker.h-128-136 (1)
128-136:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate parsed UTC fields before indexing
md.If
sscanf()fails or the API returns an invalid month,md[mo-1]reads out of bounds. Initialize fields, require six parsed values, and range-check the date/time parts before computing days.🛡️ Proposed fix
- int yr,mo,dy,hr,mn,sc2; - sscanf(s,"%d-%d-%dT%d:%d:%d",&yr,&mo,&dy,&hr,&mn,&sc2); - if (yr<2020) return 0; + int yr=0, mo=0, dy=0, hr=0, mn=0, sc2=0; + if (sscanf(s,"%d-%d-%dT%d:%d:%d",&yr,&mo,&dy,&hr,&mn,&sc2) != 6) return 0; + if (yr<2020 || mo<1 || mo>12 || dy<1 || dy>31 || + hr<0 || hr>23 || mn<0 || mn>59 || sc2<0 || sc2>60) return 0; static const int md[]={0,31,59,90,120,151,181,212,243,273,304,334};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/usermod_v2_hsv_ticker.h` around lines 128 - 136, The parseUTC function does not validate the parsed date/time fields before using them, which could cause out-of-bounds array access. Initialize the variables yr, mo, dy, hr, mn, and sc2 to default values, then check that sscanf returns 6 to ensure all six values were successfully parsed. Before indexing the md array with mo-1, validate that mo is within the valid range of 1 to 12, and return 0 if it is outside this range. Apply similar range checking to other date/time components (day, hour, minute, second) to ensure they are within acceptable bounds before proceeding with the days calculation.wled00/udp.cpp-348-350 (1)
348-350:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply the received segment offset when syncing grouping.
Line 350 keeps
selseg.offset, so a notifier’s offset/phase change is ignored wheneverreceiveSegmentOptionsis enabled butreceiveSegmentBoundsis disabled. Use the parsedoffsetwhile preserving local bounds.🐛 Proposed fix
- selseg.setGeometry(selseg.start, selseg.stop, udpIn[5+ofs], udpIn[6+ofs], selseg.offset, selseg.startY, selseg.stopY); + selseg.setGeometry(selseg.start, selseg.stop, udpIn[5+ofs], udpIn[6+ofs], offset, selseg.startY, selseg.stopY);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/udp.cpp` around lines 348 - 350, The setGeometry call in the segment grouping update is preserving the local selseg.offset value, which causes the offset/phase change from the notifier to be ignored when receiveSegmentOptions is enabled but receiveSegmentBounds is disabled. In the setGeometry call for selseg, replace the selseg.offset parameter with the appropriate offset value parsed from the UDP input data (similar to how udpIn[5+ofs] and udpIn[6+ofs] are being used for grouping and spacing parameters), ensuring the received offset overwrites the local offset when syncing.wled00/FX_fcn.cpp-1074-1083 (1)
1074-1083:⚠️ Potential issue | 🟠 MajorUse unsigned literal for 32-bit color masks to avoid undefined shift behavior.
The expression
0xFF << 24shifts a signedintliteral to a value (0xFF000000) exceeding the maximum positive value of signed int32. This is undefined behavior per C++11 standard. The loop runs at i=24 with every pixel fade. Use an unsigned literal instead.Proposed fix
- color &= ~(0xFF<<i); - color |= ((c1 + delta) & 0xFF) << i; + const uint32_t mask = 0xFFUL << i; + color = (color & ~mask) | (uint32_t((c1 + delta) & 0xFF) << i);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/FX_fcn.cpp` around lines 1074 - 1083, The bit shift operations in the color processing loop use a signed int literal that causes undefined behavior when shifted to position 24, exceeding the maximum positive value of a signed int32. Change the hex literal from 0xFF to 0xFFU (or use an equivalent unsigned literal) in all bit shift expressions within the loop, including the expressions involving shifts like (0xFF<<i) and ~(0xFF<<i), to ensure the shifts remain well-defined across all bit positions.wled00/json.cpp-635-636 (1)
635-636:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid exposing a persistent device fingerprint in public info JSON.
deviceIdis deterministic across full flash erase and derived from hardware identifiers ingetDeviceId(). Since/json/infois normally unauthenticated and CORS is permissive, this creates a stable tracking identifier; gate it behind opt-in telemetry/debug output or omit it from the general info response.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/json.cpp` around lines 635 - 636, The line containing `root[F("deviceId")] = getDeviceId();` exposes a persistent device fingerprint in the unauthenticated public info JSON response, creating a stable tracking identifier. Remove this line from the general info response, or alternatively wrap it in a conditional check that only includes it when a telemetry or debug mode flag is explicitly enabled by the user, ensuring the persistent identifier is not exposed to unauthenticated requests.wled00/remote.cpp-4-4 (1)
4-4:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDerive the ESP-NOW bus wait from actual strip timing.
A fixed 24 ms timeout can expire while long clockless updates are still transmitting, so
remoteJson()may still access FS during sendout and cause the glitches this wait is trying to avoid. Base the timeout on bus/strip timing or use a conservative worst-case cap for clockless strips.Based on learnings, per-LED transmission wait timeouts should not hardcode universal assumptions and should derive from actual strip type timing or use a conservative worst-case.
Also applies to: 127-128
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/remote.cpp` at line 4, The ESPNOW_BUSWAIT_TIMEOUT constant is hardcoded to a fixed 24ms value that does not account for long clockless LED strip updates, causing the timeout to expire prematurely and allow remoteJson() to access the filesystem during transmission, resulting in glitches. Replace this fixed timeout with a value derived from actual strip timing or use a conservative worst-case timeout that accounts for the slowest clockless strip types. Additionally, review the other timeout constants mentioned around lines 127-128 and apply the same principle to ensure all bus wait timeouts are based on strip timing rather than universal assumptions.Source: Learnings
wled00/json.cpp-223-235 (1)
223-235:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse last valid indexes for relative
fx/palparsing.
parseNumber()clamps relative string updates againstmaxvinclusively, sostrip.getModeCount()/strip.getPaletteCount()can produce one-past-last IDs for inputs like"~". Keep plain"r"using the exclusive random upper bound, but clamp relative forms tocount - 1.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/json.cpp` around lines 223 - 235, The getVal function calls for parsing relative effect and palette updates are using strip.getModeCount() and strip.getPaletteCount() as maximum values, but parseNumber() clamps relative string updates inclusively against maxv, which can produce one-past-last valid indexes for inputs like "~". In the two getVal calls around line 224-225 (for fx with strip.getModeCount()) and line 232-233 (for pal with strip.getPaletteCount()), subtract 1 from the count values when passing them as the maximum to ensure relative updates are clamped to valid indexes, so use strip.getModeCount() - 1 and strip.getPaletteCount() - 1 respectively.wled00/json.cpp-394-424 (1)
394-424:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not modify segments if servicing did not finish.
The comment says segment mutation must not happen while effects are executing, but after the frame-time timeout this code proceeds even if
strip.isServicing()is still true. Resume and defer/abort the segment update when the wait times out to avoid mutating segment vectors/data under an active effect.🛡️ Proposed guard
while (strip.isServicing() && millis() < waitUntil) delay(1); // wait until frame is over `#ifdef` WLED_DEBUG if (millis() >= waitUntil) DEBUG_PRINTLN(F("JSON: Waited for strip to finish servicing.")); `#endif` + if (strip.isServicing()) { + strip.resume(); + return false; + } if (segVar.is<JsonObject>()) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/json.cpp` around lines 394 - 424, The code must not modify segments if the strip is still servicing after the frame-time wait times out. Add a safety check after the while loop that calls strip.resume() and returns early if strip.isServicing() is still true at that point. This prevents the segment mutation logic (the deserializeSegment calls and subsequent operations on segments) from executing when the timeout expires with strip.isServicing() still returning true, ensuring segment data is not modified while effects are executing.lib/NeoESP32RmtHI/src/NeoEsp32RmtHI.S-58-63 (1)
58-63:⚠️ Potential issue | 🟠 MajorAdd explicit
.balign 16directive before the interrupt stack to ensure ABI-compliant alignment.
_rmt_intr_stackmust be 16-byte aligned per Xtensa C ABI requirements before calling C code at line 168. The.datasection in Xtensa GCC toolchain defaults to 4-byte alignment; without an explicit.balign 16directive, the linker may place the stack at a non-16-byte-aligned address. While the offset calculation (RMT_INTR_STACK_SIZE - 16 = 496) is divisible by 16, it cannot guarantee stack alignment unless the base address is also aligned. This misalignment would violate the ABI contract forcall4and cause crashes in the high-priority ISR path.Proposed fix
.data + .balign 16 _rmt_intr_stack: .space RMT_INTR_STACK_SIZE _rmt_save_ctx: .space REG_SAVE_AREA_SIZEAlso applies to: 154-168
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/NeoESP32RmtHI/src/NeoEsp32RmtHI.S` around lines 58 - 63, The `_rmt_intr_stack` symbol in the `.data` section must be 16-byte aligned per Xtensa C ABI requirements to ensure proper stack alignment when calling C code from the high-priority ISR. The `.data` section defaults to 4-byte alignment in the Xtensa GCC toolchain, which does not guarantee the required 16-byte alignment. Add an explicit `.balign 16` directive immediately before the `_rmt_intr_stack` label definition to ensure the symbol is placed at a 16-byte-aligned address, preventing potential ABI violations and crashes in the interrupt handler execution path.lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h-78-78 (1)
78-78:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
ESP_ERROR_CHECKin destructor can abort the program.If
Uninstallfails for any reason,ESP_ERROR_CHECKwill abort the application. In a destructor, this is unexpected behavior that prevents proper cleanup of other objects.Consider using
ESP_ERROR_CHECK_WITHOUT_ABORTfor consistency with line 76:- ESP_ERROR_CHECK(NeoEsp32RmtHiMethodDriver::Uninstall(_channel.RmtChannelNumber)); + ESP_ERROR_CHECK_WITHOUT_ABORT(NeoEsp32RmtHiMethodDriver::Uninstall(_channel.RmtChannelNumber));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h` at line 78, In the destructor for the class containing the NeoEsp32RmtHiMethodDriver, replace the ESP_ERROR_CHECK macro wrapping the NeoEsp32RmtHiMethodDriver::Uninstall call with ESP_ERROR_CHECK_WITHOUT_ABORT to prevent the program from aborting if the uninstall operation fails. This ensures graceful error handling during object destruction without terminating the entire application unexpectedly.lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h-173-180 (1)
173-180:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing null check after
malloccan cause null pointer dereference.On memory-constrained ESP32 devices,
malloccan returnNULL. Subsequent use of these pointers (e.g.,memcpyinUpdate()) would crash.Consider adding allocation failure handling:
void construct() { _dataEditing = static_cast<uint8_t*>(malloc(_sizeData)); - // data cleared later in Begin() - _dataSending = static_cast<uint8_t*>(malloc(_sizeData)); - // no need to initialize it, it gets overwritten on every send + if (!_dataEditing || !_dataSending) { + // Handle allocation failure - log error or set error state + free(_dataEditing); + free(_dataSending); + _dataEditing = nullptr; + _dataSending = nullptr; + } + // data cleared later in Begin() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h` around lines 173 - 180, Add null pointer checks after each malloc call in the construct() method to handle allocation failures on memory-constrained ESP32 devices. After allocating memory for _dataEditing and _dataSending, check if the returned pointers are NULL and implement appropriate error handling such as logging an error message and either returning false to indicate construction failure or throwing an exception, depending on the class's error handling pattern.wled00/FX.cpp-7318-7328 (1)
7318-7328:⚠️ Potential issue | 🟠 Major | ⚡ Quick winShift the waterfall buffer before writing the new tail pixel.
Line 7320/7322 overwrites
pixels[k]before Line 7327 copiespixels[i+1]left, so the new color is duplicated into bothkandk - 1while the previous tail pixel is dropped.🐛 Proposed fix
unsigned k = SEGLEN-1; - if (samplePeak) { - pixels[k] = (uint32_t)CRGB(CHSV(92,92,92)); - } else { - pixels[k] = color_blend(SEGCOLOR(1), SEGMENT.color_from_palette(pixCol+SEGMENT.intensity, false, PALETTE_SOLID_WRAP, 0), (uint8_t)my_magnitude); - } - SEGMENT.setPixelColor(k, pixels[k]); // loop will not execute if SEGLEN equals 1 for (unsigned i = 0; i < k; i++) { pixels[i] = pixels[i+1]; // shift left SEGMENT.setPixelColor(i, pixels[i]); } + if (samplePeak) { + pixels[k] = (uint32_t)CRGB(CHSV(92,92,92)); + } else { + pixels[k] = color_blend(SEGCOLOR(1), SEGMENT.color_from_palette(pixCol+SEGMENT.intensity, false, PALETTE_SOLID_WRAP, 0), (uint8_t)my_magnitude); + } + SEGMENT.setPixelColor(k, pixels[k]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/FX.cpp` around lines 7318 - 7328, The waterfall buffer is being shifted incorrectly because the new tail pixel is written to pixels[k] before the shift loop executes. When the loop shifts pixels left (pixels[i] = pixels[i+1]), it copies the newly set pixels[k] to pixels[k-1], duplicating the new color and losing the previous tail pixel value. Move the entire pixel assignment block that sets pixels[k] (both the conditional assignment on line 7320 or 7322 and the corresponding SEGMENT.setPixelColor call on line 7325) to execute after the for loop completes, so the buffer shifts first and then the new tail pixel is written to position k.wled00/bus_manager.cpp-812-820 (1)
812-820:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply the shared parallel-I2S buffer adjustment to the first 8 digital buses.
PolyBus::getI()maps the first 8 digital buses to parallel I2S and the later buses to RMT, but this condition adjusts buses afterMAX_RMT. That leaves the actual parallel buses over-counted and subtracts shared-buffer memory from RMT buses instead.🐛 Proposed fix
- if (PolyBus::isParallelI2S1Output() && digitalCount > MAX_RMT) { + if (PolyBus::isParallelI2S1Output() && digitalCount <= 8) { unsigned i2sCommonSize = 3 * bus->getLength() * bus->getNumberOfChannels() * (bus->is16bit()+1); if (i2sCommonSize > maxI2S) maxI2S = i2sCommonSize; busSize -= i2sCommonSize; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/bus_manager.cpp` around lines 812 - 820, The condition `if (PolyBus::isParallelI2S1Output() && digitalCount > MAX_RMT)` incorrectly determines which buses receive the shared parallel-I2S buffer adjustment. Since PolyBus::getI() maps the first 8 digital buses to parallel I2S (not buses after MAX_RMT), the adjustment should be applied to buses within the first 8 digital buses instead. Replace the `digitalCount > MAX_RMT` check with a condition that identifies whether the current bus is among the first 8 digital buses (you may need to track the digital bus index separately), so the i2sCommonSize adjustment is properly subtracted from the actual parallel I2S buses and not from RMT buses.wled00/data/settings_leds.htm-773-775 (1)
773-775:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t add an extra digital bus when parallel I2S is enabled.
When
PRis checked,!d.Sf["PR"].checkedis0, so ESP32/S2 subtract-1and expose one more digital bus than the backend accepts.🐛 Proposed fix
- let maxDB = maxD - ((is32() || isS2() || isS3()) ? (!d.Sf["PR"].checked) * 8 - (!isS3()) : 0); // adjust max digital buses if parallel I2S is not used + let maxDB = maxD; // adjust max digital buses if parallel I2S is not used + if (!d.Sf["PR"].checked && (is32() || isS2() || isS3())) { + maxDB -= 8 - (isS3() ? 0 : 1); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/data/settings_leds.htm` around lines 773 - 775, The maxDB calculation logic has a flaw in how it handles the parallel I2S setting. When d.Sf["PR"] is checked (parallel I2S enabled), !d.Sf["PR"].checked evaluates to 0, which causes the subtraction to become negative and effectively adds an extra digital bus instead of properly limiting it. Fix the conditional expression in the maxDB assignment to ensure that when parallel I2S is enabled (PR is checked), the digital bus limit is not increased. Adjust the logic by either inverting the condition to check d.Sf["PR"].checked directly or restructuring the calculation so the multiplication correctly subtracts the appropriate amount only when parallel I2S is disabled.platformio.ini-204-204 (1)
204-204:⚠️ Potential issue | 🟠 MajorUse the correct USB DFU boot macro spelling:
ARDUINO_USB_DFU_ON_BOOT.Line 204 defines
ARDUINO_DFU_ON_BOOT, while lines 168 and 475 use the correct spellingARDUINO_USB_DFU_ON_BOOT. The shorter form will be ignored, leaving the S3 default unchanged.🐛 Proposed fix
- -DARDUINO_USB_MSC_ON_BOOT=0 -DARDUINO_DFU_ON_BOOT=0 + -DARDUINO_USB_MSC_ON_BOOT=0 -DARDUINO_USB_DFU_ON_BOOT=0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platformio.ini` at line 204, The macro name on line 204 is spelled incorrectly as ARDUINO_DFU_ON_BOOT when it should be ARDUINO_USB_DFU_ON_BOOT to match the correct spelling used elsewhere in the configuration. Replace the incorrect macro name ARDUINO_DFU_ON_BOOT with the correct spelling ARDUINO_USB_DFU_ON_BOOT to ensure the build flag is properly recognized and applied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 60a95dcd-2e4c-43ea-9d67-6cf253f9bbba
⛔ Files ignored due to path filters (3)
HSVTicker_Dokumentation_1.pdfis excluded by!**/*.pdfpackage-lock.jsonis excluded by!**/package-lock.jsontools/AutoCubeMap.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (61)
.github/workflows/release.yml.gitignoreCHANGELOG.mdlib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.hlib/NeoESP32RmtHI/library.jsonlib/NeoESP32RmtHI/src/NeoEsp32RmtHI.Slib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpppackage.jsonpio-scripts/build_ui.pypio-scripts/output_bins.pypio-scripts/set_metadata.pypio-scripts/set_version.pyplatformio.iniplatformio_override.sample.initools/cdata.jsusermods/BME280_v2/usermod_bme280.husermods/audioreactive/audio_reactive.husermods/rgb-rotary-encoder/readme.mdusermods/usermod_rotary_brightness_color/usermod_rotary_brightness_color.husermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.hwled00/FX.cppwled00/FX.hwled00/FX_fcn.cppwled00/bus_manager.cppwled00/bus_manager.hwled00/bus_wrapper.hwled00/button.cppwled00/cfg.cppwled00/const.hwled00/data/common.jswled00/data/index.csswled00/data/index.htmwled00/data/index.jswled00/data/settings_leds.htmwled00/data/settings_sec.htmwled00/data/update.htmwled00/e131.cppwled00/fcn_declare.hwled00/file.cppwled00/hue.cppwled00/json.cppwled00/led.cppwled00/mqtt.cppwled00/network.cppwled00/ota_update.cppwled00/ota_update.hwled00/pin_manager.cppwled00/presets.cppwled00/remote.cppwled00/set.cppwled00/udp.cppwled00/usermod.cppwled00/usermod_v2_hsv_ticker.hwled00/usermods_list.cppwled00/util.cppwled00/wled.cppwled00/wled.hwled00/wled_metadata.cppwled00/wled_metadata.hwled00/wled_server.cppwled00/xml.cpp
💤 Files with no reviewable changes (2)
- pio-scripts/set_version.py
- tools/cdata.js
| bool IsReadyToUpdate() const | ||
| { | ||
| return (ESP_OK == ESP_ERROR_CHECK_WITHOUT_ABORT_SILENT_TIMEOUT(NeoEsp32RmtHiMethodDriver::WaitForTxDone(_channel.RmtChannelNumber, 0))); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find definition of ESP_ERROR_CHECK_WITHOUT_ABORT_SILENT_TIMEOUT
rg -n "ESP_ERROR_CHECK_WITHOUT_ABORT_SILENT_TIMEOUT" --type cpp --type hRepository: wled/WLED
Length of output: 250
🏁 Script executed:
rg "ESP_ERROR_CHECK" --type cpp --type h -iRepository: wled/WLED
Length of output: 1265
🏁 Script executed:
# Check the file context around line 89 for includes and macro definitions
cat -n lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h | head -100Repository: wled/WLED
Length of output: 4445
🏁 Script executed:
# Search for any macro definition or header that might contain this
rg "define.*ABORT.*TIMEOUT" --type cpp --type hRepository: wled/WLED
Length of output: 35
🏁 Script executed:
# Also check if there's a custom macro file in the NeoESP32RmtHI directory
find lib/NeoESP32RmtHI -type f \( -name "*.h" -o -name "*.cpp" \) -exec grep -l "define" {} \;Repository: wled/WLED
Length of output: 147
🏁 Script executed:
# Check the cpp file for any macro definitions
cat -n lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp | head -150Repository: wled/WLED
Length of output: 5974
🏁 Script executed:
# Also search ESP-IDF documentation patterns - check if this is documented anywhere
rg "SILENT_TIMEOUT|WITHOUT_ABORT" --type cpp --type hRepository: wled/WLED
Length of output: 809
ESP_ERROR_CHECK_WITHOUT_ABORT_SILENT_TIMEOUT macro does not exist—compilation will fail.
This macro is undefined in the codebase and is not a standard ESP-IDF macro. It appears only once at line 89 and will cause a compilation error. Likely intended as ESP_ERROR_CHECK_WITHOUT_ABORT, which is used elsewhere in the file (line 76 and others).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h` around lines 87 - 90, The
IsReadyToUpdate() method uses a non-existent macro
ESP_ERROR_CHECK_WITHOUT_ABORT_SILENT_TIMEOUT that will cause a compilation
failure. Replace this undefined macro with the correct
ESP_ERROR_CHECK_WITHOUT_ABORT macro, which is the standard ESP-IDF macro already
used elsewhere in the file and matches the intended functionality of checking
the RmtChannelNumber status.
| if repo: | ||
| cdefs.append(("WLED_REPO", f"\\\"{repo}\\\"")) | ||
|
|
||
| cdefs.append(("WLED_VERSION", WLED_VERSION)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import json
with open("package.json", "r", encoding="utf-8") as f:
v = json.load(f).get("version")
print("package.json version:", v)
PY
# Inspect all WLED_VERSION consumers/producers
rg -n -C2 '\bWLED_VERSION\b' pio-scripts/set_metadata.py wled00/wled_metadata.h wled00/wled_metadata.cpp wled00/wled.h
echo
echo "Expected check:"
echo "- If consumer code assigns WLED_VERSION directly to const char* / string fields, define should be quoted."
echo "- If consumer only stringifies token with preprocessor (#), unquoted may still be acceptable."Repository: wled/WLED
Length of output: 2311
🏁 Script executed:
# Find TOSTRING macro definition
rg -n 'define\s+TOSTRING' --type cpp --type h
# Also check if version string ever has pre-release/beta markers
fd package.json -exec grep -h version {} \; | head -5Repository: wled/WLED
Length of output: 202
🏁 Script executed:
# Find STRINGIFY macro definition
rg -n 'define\s+STRINGIFY' --type cpp --type h
# Also check for any version string patterns in git or historical data
git log --oneline --all -- package.json | head -10Repository: wled/WLED
Length of output: 534
🏁 Script executed:
# Verify current package.json version string and check if it could have hyphens
cat package.json | grep -A1 '"version"'
# Confirm the stringification context in wled_metadata.cpp
rg -B2 -A2 'TOSTRING\(WLED_VERSION\)' wled00/wled_metadata.cppRepository: wled/WLED
Length of output: 475
Add quotes around WLED_VERSION to safely handle pre-release versions with hyphens.
Line 105 appends WLED_VERSION unquoted, while WLED_REPO is explicitly quoted. Although the current version 0.15.3 works unquoted, git history shows pre-release versions like 17.0.0-dev and 16.0-alpha have been used. An unquoted hyphenated version like 0.15.3-dev will break as a C macro token (parsed as 0.15.3 - dev), causing a compile error. The TOSTRING(WLED_VERSION) consumer in wled_metadata.cpp:59 correctly stringifies the token, so quoting is safe and necessary.
Change to: cdefs.append(("WLED_VERSION", f"\\\"{WLED_VERSION}\\\""))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pio-scripts/set_metadata.py` at line 105, The WLED_VERSION value appended to
cdefs on line 105 is unquoted, which will cause C macro compilation errors if a
pre-release version with hyphens (like 0.15.3-dev) is used, since it will be
parsed as a subtraction operation rather than a token. Update the cdefs.append
call for WLED_VERSION to wrap the value with escaped quotes using an f-string,
matching the quoting approach already used for WLED_REPO, so that the TOSTRING
macro in the consumer code can properly stringify the version regardless of
whether it contains hyphens.
| BusDigital::BusDigital(const BusConfig &bc, uint8_t nr, const ColorOrderMap &com) | ||
| : Bus(bc.type, bc.start, bc.autoWhite, bc.count, bc.reversed, (bc.refreshReq || bc.type == TYPE_TM1814)) | ||
| , _skip(bc.skipAmount) //sacrificial pixels | ||
| , _colorOrder(bc.colorOrder) | ||
| , _milliAmpsPerLed(bc.milliAmpsPerLed) | ||
| , _milliAmpsMax(bc.milliAmpsMax) | ||
| , _colorOrderMap(com) | ||
| { | ||
| if (!isDigital(bc.type) || !bc.count) return; | ||
| if (!PinManager::allocatePin(bc.pins[0], true, PinOwner::BusDigital)) return; | ||
| DEBUGBUS_PRINTLN(F("Bus: Creating digital bus.")); | ||
| if (!isDigital(bc.type) || !bc.count) { DEBUGBUS_PRINTLN(F("Not digial or empty bus!")); return; } | ||
| if (!PinManager::allocatePin(bc.pins[0], true, PinOwner::BusDigital)) { DEBUGBUS_PRINTLN(F("Pin 0 allocated!")); return; } | ||
| _frequencykHz = 0U; | ||
| _pins[0] = bc.pins[0]; | ||
| if (is2Pin(bc.type)) { | ||
| if (!PinManager::allocatePin(bc.pins[1], true, PinOwner::BusDigital)) { | ||
| cleanup(); | ||
| DEBUGBUS_PRINTLN(F("Pin 1 allocated!")); | ||
| return; |
There was a problem hiding this comment.
Initialize cleanup-owned handles before any early return.
These constructors can return before _pins, _busPtr, _iType, _ledcStart, or _pin are initialized, but destructors still call cleanup(); Line 123 also calls cleanup() during partial construction. Allocation failure can therefore deallocate or delete garbage handles.
🛡️ Proposed fix
BusDigital::BusDigital(const BusConfig &bc, uint8_t nr, const ColorOrderMap &com)
: Bus(bc.type, bc.start, bc.autoWhite, bc.count, bc.reversed, (bc.refreshReq || bc.type == TYPE_TM1814))
, _skip(bc.skipAmount) //sacrificial pixels
, _colorOrder(bc.colorOrder)
+, _pins{255, 255}
+, _iType(I_NONE)
+, _frequencykHz(0U)
, _milliAmpsPerLed(bc.milliAmpsPerLed)
, _milliAmpsMax(bc.milliAmpsMax)
+, _busPtr(nullptr)
, _colorOrderMap(com)
{
DEBUGBUS_PRINTLN(F("Bus: Creating digital bus."));
if (!isDigital(bc.type) || !bc.count) { DEBUGBUS_PRINTLN(F("Not digial or empty bus!")); return; }
if (!PinManager::allocatePin(bc.pins[0], true, PinOwner::BusDigital)) { DEBUGBUS_PRINTLN(F("Pin 0 allocated!")); return; }
- _frequencykHz = 0U;
_pins[0] = bc.pins[0]; BusPwm::BusPwm(const BusConfig &bc)
: Bus(bc.type, bc.start, bc.autoWhite, 1, bc.reversed, bc.refreshReq) // hijack Off refresh flag to indicate usage of dithering
+#ifdef ARDUINO_ARCH_ESP32
+, _ledcStart(255)
+#endif
{ BusOnOff::BusOnOff(const BusConfig &bc)
: Bus(bc.type, bc.start, bc.autoWhite, 1, bc.reversed)
+, _pin(255)
, _onoffdata(0)
{Also applies to: 417-426, 455-477, 653-663
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wled00/bus_manager.cpp` around lines 109 - 126, Initialize member variables
_pins, _busPtr, _iType, _ledcStart, and _pin at the start of the BusDigital
constructor (and the other constructors mentioned at lines 417-426, 455-477,
653-663) before any code that can cause early returns. The constructors have
multiple early return paths (after pin allocation checks and type validation
checks) that can exit before these member variables are initialized, but the
destructor will still call cleanup() which dereferences these uninitialized
variables, causing undefined behavior. Set these member variables to safe
default values (nullptr, 0, etc.) in the initializer list or at the very
beginning of the constructor body.
| const int newLen = min(strlen(newName), (size_t)WLED_MAX_SEGNAME_LEN); | ||
| if (newLen) { | ||
| if (name) name = static_cast<char*>(realloc(name, newLen+1)); | ||
| else name = static_cast<char*>(malloc(newLen+1)); | ||
| if (name) strlcpy(name, newName, newLen+1); | ||
| name[newLen] = 0; |
There was a problem hiding this comment.
Preserve the old name when allocation fails.
Assigning realloc() directly to name loses the existing pointer on failure, and Line 615 dereferences name even when allocation returned nullptr.
🛡️ Proposed fix
- if (name) name = static_cast<char*>(realloc(name, newLen+1));
- else name = static_cast<char*>(malloc(newLen+1));
- if (name) strlcpy(name, newName, newLen+1);
- name[newLen] = 0;
+ char *newNameBuffer = name ? static_cast<char*>(realloc(name, newLen+1))
+ : static_cast<char*>(malloc(newLen+1));
+ if (!newNameBuffer) return *this;
+ name = newNameBuffer;
+ strlcpy(name, newName, newLen+1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wled00/FX_fcn.cpp` around lines 610 - 615, The code directly assigns the
result of realloc() and malloc() to name, which loses the original pointer if
allocation fails and also leaves name as nullptr. Subsequently, line 616
unconditionally dereferences name with name[newLen] = 0 without checking if
allocation succeeded. To fix this, use a temporary pointer variable to capture
the result of realloc() on line 612 and malloc() on line 614, then only assign
to name if the allocation succeeded (temporary pointer is not nullptr). Ensure
the null terminator assignment on line 616 is only executed when name is valid
by moving it inside or after the allocation success check.
| inline uint16_t groupLength() const { return grouping + spacing; } | ||
| inline uint8_t getLightCapabilities() const { return _capabilities; } | ||
| inline void deactivate() { setGeometry(0,0); } | ||
| inline Segment &clearName() { if (name) free(name); name = nullptr; return *this; } |
There was a problem hiding this comment.
Use delete[] for segment names.
name is allocated with new char[] and destroyed with delete[] elsewhere; clearName() using free() mixes allocators and can corrupt the heap.
🐛 Proposed fix
- inline Segment &clearName() { if (name) free(name); name = nullptr; return *this; }
+ inline Segment &clearName() { if (name) delete[] name; name = nullptr; return *this; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wled00/FX.h` at line 535, The clearName() method in the Segment class is
using free() to deallocate memory that was allocated with new char[], mixing C
and C++ memory management which can corrupt the heap. Replace the free(name)
call with delete[] name to use the correct deallocation operator that matches
the new char[] allocation used elsewhere in the code.
| #if defined(ARDUINO_ARCH_ESP32S3) || defined(ARDUINO_ARCH_ESP32S2) || defined(ARDUINO_ARCH_ESP32C3) | ||
| unsigned long start = millis(); | ||
| while (strip.isUpdating() && millis() - start < FRAMETIME_FIXED) yield(); // wait for strip to finish updating, accessing FS during sendout causes glitches | ||
| #endif |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For Arduino-ESP32 PlatformIO builds, which chip-target macros are guaranteed for ESP32-S2/S3/C3 detection in C/C++: ARDUINO_ARCH_ESP32S2/S3/C3, CONFIG_IDF_TARGET_ESP32S2/S3/C3, or both?
💡 Result:
For Arduino-ESP32 PlatformIO builds, you should use the CONFIG_IDF_TARGET_... macros to detect specific ESP32 variants [1][2][3]. The ARDUINO_ARCH_ESP32 macro is common to the entire ESP32 family and does not differentiate between chips (e.g., there is no ARDUINO_ARCH_ESP32S3) [1]. The CONFIG_IDF_TARGET_ESP32S2, CONFIG_IDF_TARGET_ESP32S3, and CONFIG_IDF_TARGET_ESP32C3 macros are the standard, guaranteed identifiers provided by the underlying ESP-IDF and automatically available when building with the Arduino-ESP32 framework [1][2]. Key points for C/C++ detection: - Use CONFIG_IDF_TARGET_ESP32S2 for ESP32-S2 [1][3]. - Use CONFIG_IDF_TARGET_ESP32S3 for ESP32-S3 [1][3]. - Use CONFIG_IDF_TARGET_ESP32C3 for ESP32-C3 [1][3]. - Avoid ARDUINO_ARCH_ESP32S2/S3/C3 as these are not standard macros defined by the framework [1]. These CONFIG_IDF_TARGET_... macros are defined in the sdkconfig.h file, which is automatically included when you #include <Arduino.h> [1]. They are used throughout the core ESP32 codebase for conditional compilation, making them the most reliable choice for your code [2].
Citations:
- 1: https://community.platformio.org/t/what-is-the-specific-if-defined-arduino-arch-esp32-for-esp32-s2-esp32-s3-esp32-c3/33833
- 2: https://github.com/espressif/arduino-esp32/blob/master/cores/esp32/esp32-hal.h
- 3: https://deepwiki.com/espressif/arduino-esp32/4.1-system-apis-and-peripheral-manager
🏁 Script executed:
# Check if ARDUINO_ARCH_ESP32S* macros are used anywhere in the codebase
echo "=== Searching for ARDUINO_ARCH_ESP32S usage ==="
rg "ARDUINO_ARCH_ESP32[SC]" wled00/ || echo "No matches found"
echo -e "\n=== Searching for CONFIG_IDF_TARGET_ESP32 usage ==="
rg "CONFIG_IDF_TARGET_ESP32" wled00/ | head -20
# Check the actual content around lines 167-170 in presets.cpp
echo -e "\n=== Content of wled00/presets.cpp lines 160-175 ==="
sed -n '160,175p' wled00/presets.cppRepository: wled/WLED
Length of output: 3202
🏁 Script executed:
# Check if WLED defines these ARDUINO_ARCH_* macros anywhere
echo "=== Checking for definitions of ARDUINO_ARCH_ESP32S ==="
rg "`#define` ARDUINO_ARCH_ESP32[SC]" wled00/
# Also check the const.h file to understand the context of its usage
echo -e "\n=== Content of wled00/const.h showing ARDUINO_ARCH_ESP32S usage ==="
grep -n -B 3 -A 3 "ARDUINO_ARCH_ESP32" wled00/const.h | head -40
# Verify that Arduino-ESP32 doesn't define these macros by checking if they are undefined in practice
echo -e "\n=== Search for header includes that might define these ==="
head -50 wled00/wled.h | grep -E "^`#include`|`#if` defined"Repository: wled/WLED
Length of output: 723
🏁 Script executed:
# Double-check: search Arduino-ESP32 documentation patterns in comments or other files
echo "=== Looking for any documentation or comments about macro definitions ==="
rg "ARDUINO_ARCH_ESP32" wled00/ -A 1 -B 1 | head -60
# Check if there's a custom header that might define these
echo -e "\n=== Check wled.h includes and early definitions ==="
head -100 wled00/wled.h | grep -E "`#include`|`#define` ARDUINO"Repository: wled/WLED
Length of output: 3689
Replace with CONFIG_IDF_TARGET_ESP32* macros; the current guard will never activate.
Lines 167–170 use ARDUINO_ARCH_ESP32S3, ARDUINO_ARCH_ESP32S2, and ARDUINO_ARCH_ESP32C3, but these macros are not defined by the Arduino-ESP32 framework. The framework only defines ARDUINO_ARCH_ESP32 (common to all variants). As a result, this guard will always evaluate false and the wait-for-strip-update logic will never execute on S3/S2/C3 boards, allowing the FS-access glitch to occur.
The codebase elsewhere (wled.h, wled.cpp, udp.cpp, etc.) consistently uses CONFIG_IDF_TARGET_ESP32S3, CONFIG_IDF_TARGET_ESP32S2, and CONFIG_IDF_TARGET_ESP32C3, which are the standard, guaranteed identifiers from the underlying ESP-IDF.
Suggested fix
- `#if` defined(ARDUINO_ARCH_ESP32S3) || defined(ARDUINO_ARCH_ESP32S2) || defined(ARDUINO_ARCH_ESP32C3)
+ `#if` defined(CONFIG_IDF_TARGET_ESP32S3) || defined(CONFIG_IDF_TARGET_ESP32S2) || defined(CONFIG_IDF_TARGET_ESP32C3)
unsigned long start = millis();
while (strip.isUpdating() && millis() - start < FRAMETIME_FIXED) yield(); // wait for strip to finish updating, accessing FS during sendout causes glitches
`#endif`Also audit wled00/const.h lines 487–490 and 557–560, which use the same undefined macros for memory buffer size definitions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wled00/presets.cpp` around lines 167 - 170, The preprocessor guard in
presets.cpp using ARDUINO_ARCH_ESP32S3, ARDUINO_ARCH_ESP32S2, and
ARDUINO_ARCH_ESP32C3 will never activate because these macros are not defined by
the Arduino-ESP32 framework. Replace these undefined macros with
CONFIG_IDF_TARGET_ESP32S3, CONFIG_IDF_TARGET_ESP32S2, and
CONFIG_IDF_TARGET_ESP32C3 respectively (which are the standard ESP-IDF
identifiers used elsewhere in the codebase). Additionally, audit wled00/const.h
at lines 487-490 and 557-560 which use the same undefined ARDUINO_ARCH macros
for memory buffer size definitions and apply the same replacement pattern.
|
nope |
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes & Improvements