Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the 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.
softhack007
added
major
|
Would be great to get your input on this @troyhacks |
@DedeHai do you think we can replace deprecated functions as the rabbit recommends? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@platformio.ini`:
- Around line 540-547: The debug env `[env:esp32dev_debug]` currently overrides
build_unflags and drops inherited unflags from `${esp32_idf_V5.build_unflags}`;
update the `build_unflags` setting in `env:esp32dev_debug` to preserve inherited
unflags by appending or merging `${esp32_idf_V5.build_unflags}` (e.g., include
both the existing `-D WLED_RELEASE_NAME=...` unflag and
`${esp32_idf_V5.build_unflags}`) so you don't re-enable flags previously removed
by the esp32_idf_V5 configuration.
In `@wled00/util.cpp`:
- Around line 1139-1140: The preprocessor guard around computeSHA1() and
getDeviceId() in util.cpp must match the established pattern used in
fcn_declare.h and json.cpp; replace the current simplistic ESP_IDF_VERSION check
with the explicit guard used elsewhere: check for either non-ESP32 builds or
ESP-IDF < 5.0 (i.e., the pattern !defined(ARDUINO_ARCH_ESP32) ||
(ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)) ) so the
declarations/definitions of computeSHA1 and getDeviceId are consistently
enabled/disabled; keep the TODO comment about getDeviceId unavailability on
ESP-IDF v5 intact.
🧹 Nitpick comments (1)
wled00/util.cpp (1)
16-19: Indentation is inconsistent with surrounding includes.Lines 8-15 have no leading whitespace for
#includedirectives, but lines 16-19 have varying indentation. This creates visual inconsistency within the same#elseblock.🧹 Suggested fix to align with surrounding code style
`#include` "mbedtls/sha1.h" // for SHA1 on ESP32 `#include` "esp_efuse.h" - `#include` "esp_chip_info.h" - `#if` ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(5, 0, 0) - `#include` <esp_mac.h> // V5 requirement - `#endif` +#include "esp_chip_info.h" +#if ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(5, 0, 0) +#include <esp_mac.h> // V5 requirement +#endif `#endif`
| #if ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0) // ToDO: find a solution to make this work in V5 | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for ESP_IDF_VERSION_VAL definitions
rg -n "ESP_IDF_VERSION_VAL" --type-add 'header:*.h' -t header
# Check how ESP_IDF_VERSION is used in headers
rg -n "ESP_IDF_VERSION" wled00/ -t cpp -t header -B 2 -A 2
# Check the actual code around lines 1139-1140
sed -n '1130,1150p' wled00/util.cppRepository: wled/WLED
Length of output: 2765
🏁 Script executed:
# Check for version compatibility macros in the codebase
rg -n "defined.*ESP_IDF_VERSION" wled00/Repository: wled/WLED
Length of output: 730
🏁 Script executed:
# Look for any fallback definitions
rg -n "#define.*ESP_IDF_VERSION" wled00/Repository: wled/WLED
Length of output: 206
🏁 Script executed:
sed -n '1139,1250p' wled00/util.cppRepository: wled/WLED
Length of output: 3556
🏁 Script executed:
# Also check if there's similar code in json.cpp mentioned in results
sed -n '700,715p' wled00/json.cppRepository: wled/WLED
Length of output: 858
🏁 Script executed:
# Check fcn_declare.h to see how the same pattern is used there
sed -n '420,440p' wled00/fcn_declare.hRepository: wled/WLED
Length of output: 1395
🏁 Script executed:
# Check if ESP_IDF_VERSION is defined on ESP8266
rg -n "ESP_IDF_VERSION" --type cpp | grep -i "define\|ifdef" | head -20
# Look at the learning about USB CDC which mentions ARDUINO_USB_CDC_ON_BOOT vs explicit CONFIG_IDF_TARGET checks
# This suggests relying on Arduino framework flags rather than IDF checks
# Check what the standard pattern is
rg -n "ARDUINO_ARCH_ESP32" wled00/ -t cpp | head -10
# Verify if ESP8266 includes or defines ESP_IDF_VERSION
fd "board" . | head -5
rg -n "ESP_IDF_VERSION" . --type-add 'board:*.json' | head -5Repository: wled/WLED
Length of output: 2218
🏁 Script executed:
# Search for where ESP_IDF_VERSION might be set for ESP8266
rg -n "ESP_IDF_VERSION\s*=" wled00/
# Check if there's version checking in network.cpp that shows the correct pattern
sed -n '230,240p' wled00/network.cppRepository: wled/WLED
Length of output: 440
🏁 Script executed:
# Check the exact guard pattern in util.cpp at the beginning
head -20 wled00/util.cpp
# Verify what files include or define ESP_IDF_VERSION
rg -n "include.*esp_idf_version" wled00/ -t cpp
# Check if the guard pattern is consistent across files
rg -B1 "computeSHA1\|getDeviceId" wled00/ -t cpp -t header | grep -E "^[^:]*:`#if`"Repository: wled/WLED
Length of output: 718
🏁 Script executed:
# Let's verify: Check if ESP_IDF_VERSION is used without defined() check in util.cpp
sed -n '1,30p' wled00/util.cpp | grep -A5 -B5 "ESP_IDF_VERSION"
# Count unsafe vs safe patterns in the codebase
echo "=== Unsafe patterns (no defined() check): ==="
rg "#if ESP_IDF_VERSION" wled00/ -t cpp | grep -v "defined(ESP_IDF_VERSION)"
echo ""
echo "=== Safe patterns (with defined() or platform check): ==="
rg "#if.*defined.*ARDUINO_ARCH.*ESP_IDF_VERSION" wled00/ -t cpp
rg "#if defined(ESP_IDF_VERSION)" wled00/ -t cppRepository: wled/WLED
Length of output: 2080
Use the established guard pattern for consistency with declarations and other usage sites.
The guard at line 1139 is missing the explicit platform check used elsewhere in the codebase. The same computeSHA1() and getDeviceId() functions are declared in fcn_declare.h:427 and used in json.cpp:703 with an explicit pattern: #if !defined(ARDUINO_ARCH_ESP32) || (ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)). The guard here should match that pattern for consistency and clarity.
Additionally, getDeviceId() remains unavailable on ESP-IDF v5 due to mbedTLS API changes—the TODO indicates this is known but unresolved.
🛠️ 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: Update mbedTLS SHA1 API for ESP-IDF v5 support📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #if 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: Update mbedTLS SHA1 API for ESP-IDF v5 support |
🤖 Prompt for AI Agents
In `@wled00/util.cpp` around lines 1139 - 1140, The preprocessor guard around
computeSHA1() and getDeviceId() in util.cpp must match the established pattern
used in fcn_declare.h and json.cpp; replace the current simplistic
ESP_IDF_VERSION check with the explicit guard used elsewhere: check for either
non-ESP32 builds or ESP-IDF < 5.0 (i.e., the pattern
!defined(ARDUINO_ARCH_ESP32) || (ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0))
) so the declarations/definitions of computeSHA1 and getDeviceId are
consistently enabled/disabled; keep the TODO comment about getDeviceId
unavailability on ESP-IDF v5 intact.
First off, move I2C to IDF calls. That will solve a lot of headaches right off that bat. I'll have more as I think about it. A good test is to comment out Arduino.h and see what breaks. 😁 |
|
Flash size comparison:
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@softhack007 now do it with LTO. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@platformio.ini`:
- Around line 505-525: The esp32dev_V4 regression environment contains test-only
build flags (WLED_USE_ETHERNET and the pin overrides RLYPIN and BTNPIN) which
should be removed from the default_envs to avoid skewing V4 regression results;
remove those flags from the [env:esp32dev_V4] build_flags and create a separate
test-only env (e.g., [env:esp32dev_V4_ethernet_test]) that sets -D
WLED_USE_ETHERNET -D RLYPIN=-1 -D BTNPIN=-1 (and any related lib_ignore entries
such as esp_dmx) so the regression env remains standard while the test env
captures the temporary Ethernet/pin overrides.
In `@wled00/bus_manager.cpp`:
- Around line 421-427: The code currently calls ledcAttach(...) under IDF v5
which auto-allocates channels; change this to explicitly bind the expected
channel index (channel = _ledcStart + i) using ledcAttachChannel (or the v5
equivalent that takes a channel argument) so that the assigned channel matches
the code's later assumptions (references: ledcAttach, channel, _ledcStart,
_pins, and the timer reset and direct register writes around lines noted).
Ensure you compute/assign channel = _ledcStart + i before attaching and replace
the ledcAttach(...) call with ledcAttachChannel(channel, _pins[i],
/*...params...*/) so timer reset and PWM duty writes target the correct channel.
| [env:esp32dev_V4] | ||
| ;; uses V4 framework - for checking that the code still builds in V4 | ||
| board = esp32dev | ||
| platform = ${esp32_idf_V4.platform} | ||
| platform_packages = ${esp32_idf_V4.platform_packages} | ||
| build_unflags = ${common.build_unflags} | ||
| custom_usermods = audioreactive | ||
| build_flags = ${common.build_flags} ${esp32_idf_V4.build_flags} -D WLED_RELEASE_NAME=\"ESP32\" #-D WLED_DISABLE_BROWNOUT_DET | ||
| -D WLED_ENABLE_DMX_INPUT ;; TODO: fix lots of compile errors in dmx_input.cpp | ||
| custom_usermods = ${common.default_usermods} | ||
| build_flags = ${common.build_flags} ${esp32_idf_V4.build_flags} -D WLED_RELEASE_NAME=\"ESP32_V4\" #-D WLED_DISABLE_BROWNOUT_DET | ||
| -DARDUINO_USB_CDC_ON_BOOT=0 ;; this flag is mandatory for "classic ESP32" when building with arduino-esp32 >=2.0.3 | ||
| -D WLED_DISABLE_INFRARED ;; TODO: remove once we have updated for V5 | ||
| -D WLED_USE_SHARED_RMT ;; TODO: disabled NeoEsp32RmtMethodIsr | ||
| -D WLED_USE_ETHERNET -D RLYPIN=-1 -D BTNPIN=-1 ;; TODO: this is just for testing - remove before merging to main | ||
| lib_deps = ${esp32_idf_V4.lib_deps} | ||
| lib_ignore = | ||
| NeoESP32RmtHI ;; TODO: disabled NeoEsp32RmtMethodIsr | ||
| esp_dmx ;; TODO: fix lots of compile errors in dmx_input.cpp | ||
| monitor_filters = esp32_exception_decoder | ||
| board_build.partitions = ${esp32.default_partitions} | ||
| board_build.flash_mode = dio | ||
|
|
There was a problem hiding this comment.
Remove the testing-only Ethernet flags before merge.
WLED_USE_ETHERNET plus pin overrides are marked TODO-only in the V4 regression env, but this env is in default_envs. That can skew V4 regression results away from the standard build. Consider moving these flags to an override or a dedicated test env.
Do you want me to draft a minimal test-only env block or open a tracking issue for this cleanup?
🤖 Prompt for AI Agents
In `@platformio.ini` around lines 505 - 525, The esp32dev_V4 regression
environment contains test-only build flags (WLED_USE_ETHERNET and the pin
overrides RLYPIN and BTNPIN) which should be removed from the default_envs to
avoid skewing V4 regression results; remove those flags from the
[env:esp32dev_V4] build_flags and create a separate test-only env (e.g.,
[env:esp32dev_V4_ethernet_test]) that sets -D WLED_USE_ETHERNET -D RLYPIN=-1 -D
BTNPIN=-1 (and any related lib_ignore entries such as esp_dmx) so the regression
env remains standard while the test env captures the temporary Ethernet/pin
overrides.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
keep same behaviour as in previous V4 code
Update ledcAttach to ledcAttachChannel
for V5 branch binaries
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@wled00/json.cpp`:
- Around line 703-705: The conditional compilation around root[F("deviceId")] =
getDeviceId() causes deviceId to be omitted on ESP32 IDF v5+, so restore a
stable deviceId field in the JSON by either removing the `#if` guard so
root[F("deviceId")] is always set (calling getDeviceId()), or implement a
v5-safe alternative (e.g., getDeviceIdV5() or a fallback value) and assign it to
root[F("deviceId")] when ESP_IDF_VERSION >= 5; ensure you update the code paths
around getDeviceId and the preprocessor condition so downstream consumers always
see a deviceId field (even if it is an explicit fallback/placeholder).
| #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 | ||
| root[F("deviceId")] = getDeviceId(); | ||
| #endif |
There was a problem hiding this comment.
deviceId omitted on ESP32 IDF v5+.
This drops a previously exposed field in info. Please confirm downstream consumers tolerate it or provide an alternative when the V5 SHA1 path is ready.
🤖 Prompt for AI Agents
In `@wled00/json.cpp` around lines 703 - 705, The conditional compilation around
root[F("deviceId")] = getDeviceId() causes deviceId to be omitted on ESP32 IDF
v5+, so restore a stable deviceId field in the JSON by either removing the `#if`
guard so root[F("deviceId")] is always set (calling getDeviceId()), or implement
a v5-safe alternative (e.g., getDeviceIdV5() or a fallback value) and assign it
to root[F("deviceId")] when ESP_IDF_VERSION >= 5; ensure you update the code
paths around getDeviceId and the preprocessor condition so downstream consumers
always see a deviceId field (even if it is an explicit fallback/placeholder).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
platformio.ini (4)
130-130:WLED_USE_SHARED_RMTstill defined in[common]and[esp32_idf_V5].Line 130 defines it in
[common].build_flags(applies to all platforms, including ESP8266 where it's meaningless), and line 336 defines it again in[esp32_idf_V5].build_flags. Since all ESP32 V5 environments inherit from[esp32_idf_V5], the definition in[common]is redundant and should be removed to keep it scoped to ESP32 only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platformio.ini` at line 130, Remove the redundant global definition of WLED_USE_SHARED_RMT from the [common].build_flags and ensure the only occurrence remains in [esp32_idf_V5].build_flags so the flag is scoped to ESP32 V5 environments; update the build_flags in the [common] section (remove the -D WLED_USE_SHARED_RMT entry) and keep the existing -D WLED_USE_SHARED_RMT in the [esp32_idf_V5] section to avoid applying the flag to non-ESP32 targets like ESP8266.
599-611:[env:esp32_wrover]extendsesp32_idf_V5but overridesbuild_unflagswith${common.build_unflags}.Using
extends = esp32_idf_V5gives this env all V5 base settings, but line 606 then overridesbuild_unflagsback to${common.build_unflags}, dropping V5-specific unflags. This should use${esp32_idf_V5.build_unflags}for consistency with the other V5-based environments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platformio.ini` around lines 599 - 611, The env "[env:esp32_wrover]" currently extends esp32_idf_V5 but replaces build_unflags with ${common.build_unflags}, which removes the V5-specific unflags; update the build_unflags entry in the [env:esp32_wrover] block to use ${esp32_idf_V5.build_unflags} instead of ${common.build_unflags} so it inherits the V5 unflags while keeping the common defaults intact (modify the build_unflags symbol in that env).
526-537:build_unflagsshould use V5-specific unflags.
[env:esp32dev]uses${esp32_idf_V5.platform}and${esp32_idf_V5.build_flags}but inherits${common.build_unflags}instead of${esp32_idf_V5.build_unflags}. This drops the-Wno-volatileunflag (line 329) needed for C files in V5 builds. The same issue applies toesp32dev_debug(line 544),esp32dev_8M(line 555),esp32dev_16M(line 571),esp32_eth(line 588),esp32_wrover(line 606), andusermods(line 782).Proposed fix for esp32dev (apply similarly to other envs)
-build_unflags = ${common.build_unflags} +build_unflags = ${esp32_idf_V5.build_unflags}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platformio.ini` around lines 526 - 537, The env [env:esp32dev] (and the other V5-based envs esp32dev_debug, esp32dev_8M, esp32dev_16M, esp32_eth, esp32_wrover, usermods) incorrectly reference ${common.build_unflags} which omits the V5-specific unflags; update the build_unflags assignment in each of those env blocks to use ${esp32_idf_V5.build_unflags} instead of ${common.build_unflags} so the V5-only unflag (e.g. -Wno-volatile) is included for C files when building with esp32_idf_V5.
505-524:⚠️ Potential issue | 🟡 MinorV4 regression env still carries testing-only Ethernet flags.
Line 517 has
-D WLED_USE_ETHERNET -D RLYPIN=-1 -D BTNPIN=-1with a TODO comment. Sinceesp32dev_V4is indefault_envs(line 23), this testing configuration will be included in CI/release builds, which could produce misleading regression results. These should be removed or moved to a separate test environment before merge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platformio.ini` around lines 505 - 524, The esp32dev_V4 environment includes testing-only build flags (-D WLED_USE_ETHERNET, -D RLYPIN=-1, -D BTNPIN=-1) which are pulled into CI because env:esp32dev_V4 is listed in default_envs; remove these flags from the env:esp32dev_V4 block (or move them into a new dedicated test env) and ensure default_envs no longer includes that test-only env so CI/release builds use a clean V4 regression config; update the TODO comment to reflect the moved/removed flags.wled00/util.cpp (1)
1139-1140: Guard pattern inconsistency withfcn_declare.h— already flagged.The guard at line 1139 uses
#if ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)butfcn_declare.handjson.cppuse the safer pattern#if !defined(ARDUINO_ARCH_ESP32) || (ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)). IfESP_IDF_VERSION_VALis not defined on ESP8266, this line would cause a preprocessor error.Proposed 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 in util.cpp using "#if ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)" can error on platforms where ESP_IDF_VERSION_VAL isn't defined; update the guard to the same safe pattern used in fcn_declare.h and json.cpp by changing it to the combined check that first tests ARDUINO_ARCH_ESP32 (e.g., use the pattern "!defined(ARDUINO_ARCH_ESP32) || (ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0))") so the preprocessor won't evaluate ESP_IDF_VERSION_VAL on non-ESP32 builds and the block around util.cpp's conditional compiles consistently with fcn_declare.h and json.cpp.
🧹 Nitpick comments (2)
platformio.ini (2)
13-39: Commented-out and duplicate entries indefault_envscreate confusion.Lines 30-31 comment out
esp32s3dev_16MB_opiandesp32s3dev_8MB_opi, but then lines 35-36 also list them (line 35 commented, line 36 uncommented). Theesp32s3_4M_qspiappears twice (lines 32 and 38), both commented. This duplication makes it hard to tell which envs are intentionally active.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platformio.ini` around lines 13 - 39, The default_envs list contains duplicate and contradictory commented/uncommented entries causing confusion; clean it up by consolidating and uncommenting only the intended environment names (e.g., default_envs, esp32s3dev_16MB_opi, esp32s3dev_8MB_opi, esp32s3_4M_qspi, nodemcuv2_160, esp32dev_V4, etc.), remove duplicate commented lines and ensure each env appears exactly once with the correct comment state and any TODO notes moved inline next to that single entry so it's clear which envs are active vs intentionally disabled.
322-346:[esp32_idf_V5]section looks well-structured — good migration foundation.The V5 section cleanly separates platform, flags, and dependencies. The TODO comments are clear about temporary workarounds. The
ESP32_ARDUINO_NO_RGB_BUILTINflag (line 340) is correctly included per softhack007's recommendation to avoid RMT driver conflicts.One concern: line 343 uses an unpinned branch reference for esp_dmx:
https://github.com/netmindz/esp_dmx/#esp-idf-v5-fixesThis should be pinned to a commit hash for reproducible builds, consistent with how NeoPixelBus is pinned on line 273. Branch references can break if the branch is rebased or force-pushed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platformio.ini` around lines 322 - 346, The esp_dmx dependency in the [esp32_idf_V5] env uses an unpinned branch URL (https://github.com/netmindz/esp_dmx/#esp-idf-v5-fixes) which risks non-reproducible builds; update the lib_deps entry for esp_dmx to pin to a specific commit SHA (replace the branch fragment with the repo@<commit-sha> or use the /commit/<sha> URL format) so builds are deterministic—use the NeoPixelBus pinning pattern already used elsewhere (NeoPixelBus entry) as the template when editing the [esp32_idf_V5] lib_deps line.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@platformio.ini`:
- Line 270: Update the AsyncTCP dependency from 3.4.6 to 3.4.7 in the dependency
declaration (change the line containing "esp32async/AsyncTCP @ 3.4.6"); this
upgrades to the release that restores correct AsyncClient::abort() dispose
semantics and prevents the memory leak when dispose callbacks are not triggered.
Ensure any lockfiles or platform-specific dependency pins are also updated so
the build uses AsyncTCP v3.4.7.
In `@wled00/bus_manager.cpp`:
- Around line 416-423: The code directly manipulates LEDC internals
(LEDC.channel_group[gr].channel[ch].duty.duty and
LEDC.channel_group[gr].channel[ch].hpoint.hpoint) to implement dithering, which
is incompatible with ESP-IDF v5; replace that direct struct access with a
compatibility-safe approach: add an `#if/`#else guard on ESP_IDF_VERSION so that
for v5 and newer you either (a) disable hardware dithering and fall back to
no-dither or a software-dither implementation that updates duty via public APIs
(e.g., ledc_set_duty/ledc_set_duty_and_update or Arduino ledcWrite equivalents)
or (b) use any new HAL/API if available, and for older versions keep the
existing direct approach; update any callers around ledcAttachChannel/ledcDetach
to use the guarded behavior and remove direct references to
LEDC.channel_group[...] so the code compiles and behaves correctly across
ESP-IDF versions.
---
Duplicate comments:
In `@platformio.ini`:
- Line 130: Remove the redundant global definition of WLED_USE_SHARED_RMT from
the [common].build_flags and ensure the only occurrence remains in
[esp32_idf_V5].build_flags so the flag is scoped to ESP32 V5 environments;
update the build_flags in the [common] section (remove the -D
WLED_USE_SHARED_RMT entry) and keep the existing -D WLED_USE_SHARED_RMT in the
[esp32_idf_V5] section to avoid applying the flag to non-ESP32 targets like
ESP8266.
- Around line 599-611: The env "[env:esp32_wrover]" currently extends
esp32_idf_V5 but replaces build_unflags with ${common.build_unflags}, which
removes the V5-specific unflags; update the build_unflags entry in the
[env:esp32_wrover] block to use ${esp32_idf_V5.build_unflags} instead of
${common.build_unflags} so it inherits the V5 unflags while keeping the common
defaults intact (modify the build_unflags symbol in that env).
- Around line 526-537: The env [env:esp32dev] (and the other V5-based envs
esp32dev_debug, esp32dev_8M, esp32dev_16M, esp32_eth, esp32_wrover, usermods)
incorrectly reference ${common.build_unflags} which omits the V5-specific
unflags; update the build_unflags assignment in each of those env blocks to use
${esp32_idf_V5.build_unflags} instead of ${common.build_unflags} so the V5-only
unflag (e.g. -Wno-volatile) is included for C files when building with
esp32_idf_V5.
- Around line 505-524: The esp32dev_V4 environment includes testing-only build
flags (-D WLED_USE_ETHERNET, -D RLYPIN=-1, -D BTNPIN=-1) which are pulled into
CI because env:esp32dev_V4 is listed in default_envs; remove these flags from
the env:esp32dev_V4 block (or move them into a new dedicated test env) and
ensure default_envs no longer includes that test-only env so CI/release builds
use a clean V4 regression config; update the TODO comment to reflect the
moved/removed flags.
In `@wled00/util.cpp`:
- Around line 1139-1140: The preprocessor guard in util.cpp using "#if
ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)" can error on platforms where
ESP_IDF_VERSION_VAL isn't defined; update the guard to the same safe pattern
used in fcn_declare.h and json.cpp by changing it to the combined check that
first tests ARDUINO_ARCH_ESP32 (e.g., use the pattern
"!defined(ARDUINO_ARCH_ESP32) || (ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0,
0))") so the preprocessor won't evaluate ESP_IDF_VERSION_VAL on non-ESP32 builds
and the block around util.cpp's conditional compiles consistently with
fcn_declare.h and json.cpp.
---
Nitpick comments:
In `@platformio.ini`:
- Around line 13-39: The default_envs list contains duplicate and contradictory
commented/uncommented entries causing confusion; clean it up by consolidating
and uncommenting only the intended environment names (e.g., default_envs,
esp32s3dev_16MB_opi, esp32s3dev_8MB_opi, esp32s3_4M_qspi, nodemcuv2_160,
esp32dev_V4, etc.), remove duplicate commented lines and ensure each env appears
exactly once with the correct comment state and any TODO notes moved inline next
to that single entry so it's clear which envs are active vs intentionally
disabled.
- Around line 322-346: The esp_dmx dependency in the [esp32_idf_V5] env uses an
unpinned branch URL (https://github.com/netmindz/esp_dmx/#esp-idf-v5-fixes)
which risks non-reproducible builds; update the lib_deps entry for esp_dmx to
pin to a specific commit SHA (replace the branch fragment with the
repo@<commit-sha> or use the /commit/<sha> URL format) so builds are
deterministic—use the NeoPixelBus pinning pattern already used elsewhere
(NeoPixelBus entry) as the template when editing the [esp32_idf_V5] lib_deps
line.
ℹ️ 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 (16)
package.jsonplatformio.iniwled00/bus_manager.cppwled00/e131.cppwled00/fcn_declare.hwled00/improv.cppwled00/json.cppwled00/network.cppwled00/ntp.cppwled00/src/dependencies/espalexa/Espalexa.hwled00/udp.cppwled00/util.cppwled00/wled.cppwled00/wled.hwled00/wled_server.cppwled00/xml.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- wled00/e131.cpp
- wled00/udp.cpp
- wled00/wled.cpp
- wled00/json.cpp
| [esp32_all_variants] | ||
| lib_deps = | ||
| esp32async/AsyncTCP @ 3.4.7 | ||
| esp32async/AsyncTCP @ 3.4.6 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
AsyncTCP 3.4.6 3.4.7 changelog memory bug ESP32
💡 Result:
- AsyncTCP v3.4.7 (released Aug 2, 2025): Fixes a memory leak introduced in v3.4.6; release notes explicitly say “Please do not use v3.4.6.” The fix is “Restore abort dispose semantics” (PR Updated readme #82). When calling
AsyncClient::abort(), it again enqueues an error event (as it did before PR#79), which prevents leaks caused by missing dispose callbacks. [1][2] - AsyncTCP v3.4.6 (released Jul 30, 2025): main change is “Error/closing stability” (PR limit led power consumption (using estimation) #79)—this is the change that v3.4.7 identifies as having introduced the memory leak. [3][1]
Sources: [1] [2] [3]
🏁 Script executed:
# Search for any references in WLED codebase about AsyncTCP versions or issues
rg -i "asynctcp|3\.4\.[67]" --type md --type txtRepository: wled/WLED
Length of output: 276
AsyncTCP 3.4.6 must be upgraded to 3.4.7 — this version contains a documented memory leak introduced in PR #79.
The official AsyncTCP v3.4.7 release (Aug 2, 2025) explicitly states: "Please do not use v3.4.6." The leak occurs when AsyncClient::abort() is called and dispose callbacks are not triggered. v3.4.7 restores the abort dispose semantics to prevent this. Shipping with 3.4.6 risks memory exhaustion on ESP32 devices.
Fix
- esp32async/AsyncTCP @ 3.4.6
+ esp32async/AsyncTCP @ 3.4.7📝 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.
| esp32async/AsyncTCP @ 3.4.6 | |
| esp32async/AsyncTCP @ 3.4.7 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platformio.ini` at line 270, Update the AsyncTCP dependency from 3.4.6 to
3.4.7 in the dependency declaration (change the line containing
"esp32async/AsyncTCP @ 3.4.6"); this upgrades to the release that restores
correct AsyncClient::abort() dispose semantics and prevents the memory leak when
dispose callbacks are not triggered. Ensure any lockfiles or platform-specific
dependency pins are also updated so the build uses AsyncTCP v3.4.7.
| unsigned channel = _ledcStart + i; | ||
| #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:
# Get the specific lines mentioned in the review
sed -n '410,430p' wled00/bus_manager.cpp
echo "---"
sed -n '580,600p' wled00/bus_manager.cppRepository: wled/WLED
Length of output: 1661
🏁 Script executed:
# Search for ledcAttachChannel and ledcDetach usage patterns in the file
rg -n "ledcAttachChannel|ledcDetach" wled00/bus_manager.cpp -B 2 -A 2Repository: wled/WLED
Length of output: 529
🏁 Script executed:
# Check if there are any direct LEDC struct accesses like LEDC.channel_group
rg -n "LEDC\." wled00/bus_manager.cppRepository: wled/WLED
Length of output: 360
🌐 Web query:
Arduino-ESP32 v3 ledcAttachChannel function signature parameters
💡 Result:
In Arduino-ESP32 v3.x, the function signature is:
bool ledcAttachChannel(uint8_t pin, uint32_t freq, uint8_t resolution, int8_t channel); [1]
Parameters [1]:
- pin (
uint8_t): GPIO to output PWM on - freq (
uint32_t): PWM frequency (Hz) - resolution (
uint8_t): duty resolution in bits (typically 1–14, 1–20 on ESP32) - channel (
int8_t): LEDC channel number to use (manual assignment)
Notes: if the channel is already configured, the passed freq and resolution are ignored; multiple pins attached to the same channel share the same duty cycle. [1]
🌐 Web query:
Arduino-ESP32 v3 ledcDetach function documentation
💡 Result:
In Arduino-ESP32 v3.x, the LEDC “detach” API is:
- Signature:
bool ledcDetach(uint8_t pin); - Purpose: Detaches the specified GPIO pin from the LEDC (PWM) peripheral.
- Return value:
trueon success,falseon error. [1]
Migration note (2.x → 3.0): ledcDetachPin was renamed to ledcDetach, and the API generally moved from channel-based calls toward pin-based calls. [2]
Sources: [1], [2]
🏁 Script executed:
# Find the function containing lines 549-550
sed -n '540,560p' wled00/bus_manager.cpp | head -30Repository: wled/WLED
Length of output: 1196
🏁 Script executed:
# Get broader context around lines 549-550
sed -n '535,560p' wled00/bus_manager.cppRepository: wled/WLED
Length of output: 1315
Critical: Direct LEDC struct access (lines 549–550) incompatible with ESP-IDF v5.
While the ledcAttachChannel and ledcDetach API signatures at lines 416–423 and 587–591 are correctly aligned with Arduino-ESP32 v3, the implementation has a blocking issue: lines 549–550 directly access LEDC struct internals (LEDC.channel_group[gr].channel[ch].duty.duty and LEDC.channel_group[gr].channel[ch].hpoint.hpoint) to implement dithering. This approach is not supported in ESP-IDF v5, where the framework manages LEDC channels internally and no public HAL API exposes dithering control. The current comment states "no HAL exposed function for dithering" — this design dependency must be resolved before ESP-IDF v5 support is viable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/bus_manager.cpp` around lines 416 - 423, The code directly manipulates
LEDC internals (LEDC.channel_group[gr].channel[ch].duty.duty and
LEDC.channel_group[gr].channel[ch].hpoint.hpoint) to implement dithering, which
is incompatible with ESP-IDF v5; replace that direct struct access with a
compatibility-safe approach: add an `#if/`#else guard on ESP_IDF_VERSION so that
for v5 and newer you either (a) disable hardware dithering and fall back to
no-dither or a software-dither implementation that updates duty via public APIs
(e.g., ledc_set_duty/ledc_set_duty_and_update or Arduino ledcWrite equivalents)
or (b) use any new HAL/API if available, and for older versions keep the
existing direct approach; update any callers around ledcAttachChannel/ledcDetach
to use the guarded behavior and remove direct references to
LEDC.channel_group[...] so the code compiles and behaves correctly across
ESP-IDF versions.
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
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Refactor