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
enhancement
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).
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