Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
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. |
|
With few fixes it builds. MQTT mbedTLS WiFi wled.cpp Compare IPAddress to a value, not to 0 (prevents pointer overload → memcmp on nullptr) and clamp index (prevents out-of-range access cascading into bad compares). @@ void WLED::initConnection()
- if (multiWiFi[selectedWiFi].staticIP != 0U && multiWiFi[selectedWiFi].staticGW != 0U) {
- WiFi.config(multiWiFi[selectedWiFi].staticIP, multiWiFi[selectedWiFi].staticGW, multiWiFi[selectedWiFi].staticSN, dnsAddress);
- } else {
- WiFi.config(IPAddress((uint32_t)0), IPAddress((uint32_t)0), IPAddress((uint32_t)0));
- }
+ if (multiWiFi.empty()) { // guard: handle empty WiFi list safely
+ WiFi.config(IPAddress((uint32_t)0), IPAddress((uint32_t)0), IPAddress((uint32_t)0));
+ } else {
+ if (selectedWiFi >= multiWiFi.size()) selectedWiFi = 0; // guard: ensure valid index
+ if (multiWiFi[selectedWiFi].staticIP != IPAddress((uint32_t)0) &&
+ multiWiFi[selectedWiFi].staticGW != IPAddress((uint32_t)0)) { // guard: compare as IPAddress to avoid pointer overload
+ WiFi.config(multiWiFi[selectedWiFi].staticIP, multiWiFi[selectedWiFi].staticGW, multiWiFi[selectedWiFi].staticSN, dnsAddress);
+ } else {
+ WiFi.config(IPAddress((uint32_t)0), IPAddress((uint32_t)0), IPAddress((uint32_t)0));
+ }
+ }ESP32 MAC fallback (fix Info tab showing 000000000000) @@ (top of file)
+#ifdef ARDUINO_ARCH_ESP32
+#include <esp_mac.h> // fallback MAC read from efuse
+#endif@@ // generate module IDs must be done before AP setup
-escapedMac = WiFi.macAddress();
+escapedMac = WiFi.macAddress();
+#ifdef ARDUINO_ARCH_ESP32
+if (escapedMac == "00:00:00:00:00:00") { // fix: fallback to efuse MAC if WiFi not started
+ uint8_t m[6] = {0};
+ if (esp_read_mac(m, ESP_MAC_WIFI_STA) == ESP_OK) {
+ char buf[18];
+ sprintf(buf, "%02X:%02X:%02X:%02X:%02X:%02X", m[0], m[1], m[2], m[3], m[4], m[5]);
+ escapedMac = buf;
+ }
+}
+#endif
escapedMac.replace(":", "");
escapedMac.toLowerCase();network.cpp
@@ int findWiFi(...)
- if (multiWiFi.size() <= 1) {
- DEBUG_PRINTF_P(PSTR("WiFi: Defaulf SSID (%s) used.\n"), multiWiFi[0].clientSSID);
- return 0;
- }
+ if (multiWiFi.size() <= 1) {
+ if (multiWiFi.empty()) return 0; // guard: handle empty list safely
+ DEBUG_PRINTF_P(PSTR("WiFi: Defaulf SSID (%s) used.\n"), multiWiFi[0].clientSSID);
+ return 0;
+ }
@@ for (size_t n = 0; n < multiWiFi.size(); n++)
- bool foundBSSID = memcmp(multiWiFi[n].bssid, WiFi.BSSID(o), 6) == 0;
+ const uint8_t* _scanBSSID = WiFi.BSSID(o);
+ bool _cfgHasBSSID = false; for (int _i = 0; _i < 6; _i++) _cfgHasBSSID |= multiWiFi[n].bssid[_i];
+ bool foundBSSID = (_scanBSSID && _cfgHasBSSID) && memcmp(multiWiFi[n].bssid, _scanBSSID, 6) == 0; // guard: safe memcmp against nullptr/empty BSSIDUsermods Modified um_manager.cpp: #include "wled.h"
#include <assert.h>
/*
* Registration and management utility for v2 usermods
*
* Discovery paths, in this order:
* 1) Legacy dtors window: .dtors.tbl.usermods.0 ... .99 (older toolchains)
* 2) Dynamic fallback: usermods call registerUsermod(Usermod*) from a ctor
*/
// -------- Legacy dtors window (keep for backward compatibility) --------
static Usermod * const _usermod_table_begin[0]
__attribute__((section(".dtors.tbl.usermods.0"), unused)) = {};
static Usermod * const _usermod_table_end[0]
__attribute__((section(".dtors.tbl.usermods.99"), unused)) = {};
struct UMSpan {
Usermod* const* begin;
Usermod* const* end;
};
static inline UMSpan reg_dtors_span() {
UMSpan s{_usermod_table_begin, _usermod_table_end};
return s;
}
// ----- Dynamic fallback: usermods can push themselves here at boot -----
static Usermod* g_dynMods[16];
static size_t g_dynCnt = 0;
// Weak, so a stronger definition elsewhere (if any) wins.
// Usermods may call this from a constructor to guarantee registration.
extern "C" void registerUsermod(Usermod* m) __attribute__((weak));
extern "C" void registerUsermod(Usermod* m) {
if (m && g_dynCnt < (sizeof(g_dynMods)/sizeof(g_dynMods[0]))) {
g_dynMods[g_dynCnt++] = m;
}
}
// ----- Common iteration helpers -----
// Accept any callable (incl. capturing lambdas/functors)
template<typename Fn>
static inline void forEachMod(Fn&& fn) {
// 1) legacy dtors table
{
UMSpan s = reg_dtors_span();
for (auto p = s.begin; p < s.end; ++p) {
Usermod* m = *p;
if (m) fn(m);
}
}
// 2) dynamic list
for (size_t i = 0; i < g_dynCnt; ++i) {
if (g_dynMods[i]) fn(g_dynMods[i]);
}
}
static inline size_t countMods() {
size_t count = 0;
// dtors window
{
UMSpan s = reg_dtors_span();
count += (size_t)(s.end - s.begin);
}
// dynamic
count += g_dynCnt;
return count;
}
// --------- Usermod Manager methods (iterate via forEachMod) ---------
void UsermodManager::setup() { forEachMod([](Usermod* m){ m->setup(); }); }
void UsermodManager::connected() { forEachMod([](Usermod* m){ m->connected(); }); }
void UsermodManager::loop() { forEachMod([](Usermod* m){ m->loop(); }); }
void UsermodManager::handleOverlayDraw() { forEachMod([](Usermod* m){ m->handleOverlayDraw(); }); }
void UsermodManager::appendConfigData(Print& dest) {
forEachMod([&](Usermod* m){ m->appendConfigData(dest); });
}
bool UsermodManager::handleButton(uint8_t b) {
bool overrideIO = false;
forEachMod([&](Usermod* m){
if (m->handleButton(b)) overrideIO = true;
});
return overrideIO;
}
bool UsermodManager::getUMData(um_data_t **data, uint8_t mod_id) {
bool got = false;
forEachMod([&](Usermod* m){
if (got) return;
if (mod_id > 0 && m->getId() != mod_id) return;
if (m->getUMData(data)) got = true;
});
return got;
}
void UsermodManager::addToJsonState(JsonObject& obj) {
forEachMod([&](Usermod* m){ m->addToJsonState(obj); });
}
void UsermodManager::addToJsonInfo(JsonObject& obj) {
auto um_id_list = obj.createNestedArray("um");
forEachMod([&](Usermod* m){
um_id_list.add(m->getId());
m->addToJsonInfo(obj);
});
}
void UsermodManager::readFromJsonState(JsonObject& obj) {
forEachMod([&](Usermod* m){ m->readFromJsonState(obj); });
}
void UsermodManager::addToConfig(JsonObject& obj) {
forEachMod([&](Usermod* m){ m->addToConfig(obj); });
}
bool UsermodManager::readFromConfig(JsonObject& obj) {
bool allComplete = true;
forEachMod([&](Usermod* m){
if (!m->readFromConfig(obj)) allComplete = false;
});
return allComplete;
}
#ifndef WLED_DISABLE_MQTT
void UsermodManager::onMqttConnect(bool sessionPresent) {
forEachMod([&](Usermod* m){ m->onMqttConnect(sessionPresent); });
}
bool UsermodManager::onMqttMessage(char* topic, char* payload) {
bool handled = false;
forEachMod([&](Usermod* m){
if (!handled && m->onMqttMessage(topic, payload)) handled = true;
});
return handled;
}
#endif // WLED_DISABLE_MQTT
#ifndef WLED_DISABLE_ESPNOW
bool UsermodManager::onEspNowMessage(uint8_t* sender, uint8_t* payload, uint8_t len) {
bool handled = false;
forEachMod([&](Usermod* m){
if (!handled && m->onEspNowMessage(sender, payload, len)) handled = true;
});
return handled;
}
#endif // WLED_DISABLE_ESPNOW
void UsermodManager::onUpdateBegin(bool init) {
forEachMod([&](Usermod* m){ m->onUpdateBegin(init); });
}
void UsermodManager::onStateChange(uint8_t mode) {
forEachMod([&](Usermod* m){ m->onStateChange(mode); });
}
Usermod* UsermodManager::lookup(uint16_t mod_id) {
Usermod* found = nullptr;
forEachMod([&](Usermod* m){
if (!found && m->getId() == mod_id) found = m;
});
return found;
}
size_t UsermodManager::getModCount() {
return countMods();
}
/* Usermod v2 interface shim for oappend */
Print* Usermod::oappend_shim = nullptr;
void Usermod::appendConfigData(Print& settingsScript) {
assert(!oappend_shim);
oappend_shim = &settingsScript;
this->appendConfigData();
oappend_shim = nullptr;
}At the end of usermod_v2_RF433.cpp I added: // --- Registration via dynamic fallback (no custom sections) ---
static RF433Usermod usermod_v2_RF433;
REGISTER_USERMOD(usermod_v2_RF433); // OK to keep for legacy dtors builds
extern "C" void registerUsermod(Usermod*) __attribute__((weak));
__attribute__((constructor, used))
static void __rf433_ctor_register(void) {
if (registerUsermod) registerUsermod(&usermod_v2_RF433);
}NeoPixelBus --- a/src/internal/methods/ESP/ESP32/NeoEsp32RmtXMethod.h
+++ b/src/internal/methods/ESP/ESP32/NeoEsp32RmtXMethod.h
@@ -1,5 +1,7 @@
#pragma once
+// Needed for ESP_* macros that take a log_tag
+#include "esp_log.h"
+static const char* TAG = "NeoEsp32RmtX";And I added a temporary shim in wled00/bus_wrapper.h. Since ESP-IDF 5 switched to the new RMT “driver_v2,” Makuna renamed all of the “N” (legacy/driver_v1) variants to “X” (v2) under internal/methods/ESP/ESP32/* // --- temporary shim for NeoPixelBus CORE3 / RMT driver_v2 ------------------
#if __has_include(<NeoPixelBus.h>)
#define NeoEsp32RmtNWs2812xMethod NeoEsp32RmtXWs2812xMethod
#define NeoEsp32RmtNSk6812Method NeoEsp32RmtXSk6812Method
#define NeoEsp32RmtN400KbpsMethod NeoEsp32RmtX400KbpsMethod
#define NeoEsp32RmtNTm1814Method NeoEsp32RmtXTm1814Method
#define NeoEsp32RmtNTm1829Method NeoEsp32RmtXTm1829Method
#define NeoEsp32RmtNTm1914Method NeoEsp32RmtXTm1914Method
#define NeoEsp32RmtNApa106Method NeoEsp32RmtXApa106Method
#define NeoEsp32RmtNWs2805Method NeoEsp32RmtXWs2805Method
#endif
// --------------------------------------------------------------------------- |
|
Thanks for helping with this @sombree Could you please open a PR against the V5 branch and we can then pull in your fixes that way, maintaining attribution for your work |
…(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.
Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com>
Co-authored-by: netmindz <442066+netmindz@users.noreply.github.com>
in V5, LEDC_CHANNEL_MAX and LEDC_SPEED_MODE_MAX are enum constants, and the compiler does not allow a macro where enums are implicitly treated as int. Solution: change into constexpr with static type cast => still computed by the compiler and does not create any local variable.
…ib_ignore * esp32_idf_V5 has the main build_unflags and lib_ignore * next level (esp32, esp32s3, esp32s2, esp32c3) inherits from esp32_idf_V5 * user buildenvs _only_ inherit from (esp32, esp32s3, esp32s2, esp32c3)
| { | ||
| EA_DEBUGLN("# Responding to description.xml ... #\n"); | ||
| IPAddress localIP = Network.localIP(); | ||
| IPAddress localIP = WLEDNetwork.localIP(); |
There was a problem hiding this comment.
Should we revert this in favor of using a #define @willmmiles ?
plus - re-enabled one S3 and one S2 build - disabled c++20 deprecation warnings - minor indentation fix
… udp.flush() * add esp32dev_V4 for regression checking w.r.t. V4 framework * bring back lost V4 code * udp.flush() is deprecated --> udp.clear()
* disable "temporary shim for NeoPixelBus CORE3" for V4 * better way to prevent compiling NeoEsp32RmtHIMethod
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai take a look at the various migration guides (see #4838 (comment)). Then scan the complete source code in this PR brach for remaining hidden incompatibilities with esp-idf V5.3.x or arduino-esp32 v3.3.x. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 4957 🏁 Script executed: Length of output: 4124 🏁 Script executed: Length of output: 476 🏁 Script executed: Length of output: 1670 🌐 Web query: 💡 Result: Key ESP-IDF v5.3 migration (from v5.2 → v5.3) API / breaking-change items called out in Espressif’s migration guide:
Other v5.3 release-note breaking API changes you may also need to handle (depending on which 5.3.x you upgrade to):
Sources: ESP-IDF v5.3 migration guide pages and ESP-IDF GitHub release notes. [1][2][3][4][5][6][7][8] 🌐 Web query: 💡 Result: There isn’t a separate “2.0 → 3.3” migration guide; Espressif’s documented breaking changes are covered in the “Migration from 2.x to 3.0” guide (and that’s the main jump you must handle), then you should skim 3.1/3.2/3.3 release notes for any additional changes. [1], [2] Breaking changes you’ll hit when moving from Arduino-ESP32 2.0.x → 3.x (incl. 3.3.x)From the official 2.x → 3.0 migration guide: [1]
What’s “3.3-specific” vs “3.0 breakage”?Arduino-ESP32 3.3.0 is a minor line update based on ESP-IDF 5.5.x (roadmap lists 3.3.0 = 2025-08-06, ESP-IDF 5.5), and the GitHub release notes for 3.3.x largely read as features/fixes rather than a new wave of “2.x-style” breaking API removals. [3], [2] Practical guidance: do the migration using the 2.x → 3.0 guide first (that’s where the real breaking changes are), then build/test on your target 3.3.x and review the 3.3.x release notes for anything affecting the subsystems you use (Wi‑Fi/TLS, USB/TinyUSB, UART, LEDC, etc.). [1], [2] Sources: [1] Migration guide 2.x→3.0, [2] arduino-esp32 GitHub releases (3.3.x notes), [3] arduino-esp32 release roadmap (3.3.0 date/IDF base).
🔴 Critical Issues Requiring Immediate Attention1. util.cpp - Chip Identification APIs (Lines 1165-1179)Status: // Lines 1165-1167: Missing includes and unguarded APIs
esp_chip_info_t chip_info;
esp_chip_info(&chip_info);
esp_efuse_mac_get_default((uint8_t*)fp);Problem: These ESP-IDF APIs require proper headers that may not be included. `#ifdef` ESP32
`#include` <esp_chip_info.h> // Add this
`#include` <esp_efuse.h> // Add this
`#if` ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(5, 0, 0)
`#include` <esp_mac.h> // V5 requirement
`#endif`
`#endif`2. deep_sleep Usermod - RTC GPIO APIs (deep_sleep.cpp:214-238)Status: Multiple
Recommendation: Add conditional compilation: `#if` ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(5, 0, 0)
// V5 RTC GPIO API (likely unchanged, but verify)
rtc_gpio_hold_dis((gpio_num_t)wakeupPin);
// ... rest of calls
`#else`
// V4 API (current code)
`#endif`🟡 Moderate Issues - May Cause Warnings/Deprecation3. Heap Monitoring Functions (wled.cpp:275-291, fcn_declare.h:492-493)Status: heap_caps_get_free_size() // Deprecated in IDF 5.x
heap_caps_get_largest_free_block() // Deprecated in IDF 5.xESP-IDF V5.3 Recommendation: These still work but may emit deprecation warnings. Consider future migration to:
4. pwm_outputs Usermod - Missing LEDC GuardsStatus: File: This usermod uses old LEDC APIs ( Fix: Apply the same pattern as `#if` ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(5, 0, 0)
ledcAttach(pin, freq, resolution);
`#else`
ledcSetup(channel, freq, resolution);
ledcAttachPin(pin, channel);
`#endif`5. ADC Deprecated Constant (util.cpp:1177, audioreactive)Status: // Line 1177:
esp_adc_cal_characterize(ADC_UNIT_1, ADC_ATTEN_DB_11, ...);
// audioreactive/audio_source.h:625:
adc1_config_channel_atten(adc1_channel_t(channel), ADC_ATTEN_DB_11);Fix: Replace with: ADC_ATTEN_DB_11 → ADC_ATTEN_DB_12 // Same behavior, new name🟢 Minor/Informational Issues6. WiFi.macAddress() Usage (3 occurrences)Status: ✅ Compatible but has fallback Files: Arduino-ESP32 v3 changed behavior slightly, but the API signature is preserved. The existing fallback code in 7. Serial.setDebugOutput() (wled.cpp:386)Status: ✅ Compatible This API is preserved in Arduino-ESP32 v3.x with the same signature. 8. NeoESP32RmtHI LibraryStatus: ✅ Intentionally disabled Missing ESP_IDF_VERSION guards in
No action needed. 📋 ESP-IDF 5.3 Specific Changes Not Found in CodebaseGood news - WLED doesn't appear to use these deprecated V5.3 APIs: ✅ Bluetooth GAP: No 🎯 Recommended Action PlanPriority 1 (Breaks builds):
Priority 2 (Generates warnings):
Priority 3 (Future-proofing):
✅ What's Already Working WellThe codebase shows good V4/V5 compatibility patterns in:
Overall assessment: The V5 migration is ~90% complete. The remaining issues are concentrated in:
Would you like me to generate specific patch proposals for the Priority 1 issues? 🧠 Learnings used |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/util.cpp (1)
1135-1229:⚠️ Potential issue | 🟠 MajorBlanket IDF v5 exclusion disables all three functions unnecessarily —
computeSHA1andgetDeviceIdcan work with targeted API guards.The
#if ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)guard removescomputeSHA1,generateDeviceFingerprint, andgetDeviceIdon IDF v5, but only two compatibility issues exist:
mbedtls API rename (lines 1147–1149): In IDF v5's mbedtls 3.x, the
_retsuffix was removed. Use conditional compilation to select the correct function names:
- Old (IDF <5):
mbedtls_sha1_starts_ret(),mbedtls_sha1_update_ret(),mbedtls_sha1_finish_ret()- New (IDF ≥5):
mbedtls_sha1_starts(),mbedtls_sha1_update(),mbedtls_sha1_finish()(still returnint)esp_adc_cal.h removal (line 1164): Replaced by
esp_adc/adc_cali.handesp_adc/adc_cali_scheme.hwith a redesigned API; only affectsgenerateDeviceFingerprint.A targeted approach allows
computeSHA1andgetDeviceIdto work on IDF v5 immediately:Suggested fix for computeSHA1
String computeSHA1(const String& input) { `#ifdef` ESP8266 return sha1(input); `#else` unsigned char shaResult[20]; mbedtls_sha1_context ctx; mbedtls_sha1_init(&ctx); + `#if` ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(5, 0, 0) + mbedtls_sha1_starts(&ctx); + mbedtls_sha1_update(&ctx, (const unsigned char*)input.c_str(), input.length()); + mbedtls_sha1_finish(&ctx, shaResult); + `#else` mbedtls_sha1_starts_ret(&ctx); mbedtls_sha1_update_ret(&ctx, (const unsigned char*)input.c_str(), input.length()); mbedtls_sha1_finish_ret(&ctx, shaResult); + `#endif` mbedtls_sha1_free(&ctx); // ... hex conversion unchanged `#endif` }
🤖 Fix all issues with AI agents
In `@platformio.ini`:
- Around line 278-283: The build_unflags assignment is wrong: it uses
${esp32_idf_V5.build_flags} which cancels flags; change the value to
${esp32_idf_V5.build_unflags} so build_unflags references the unflag list from
the esp32_idf_V5 section; update the platformio.ini entry that defines
build_unflags (near the lines with platform, build_flags, lib_deps) to use
${esp32_idf_V5.build_unflags} instead of ${esp32_idf_V5.build_flags}.
- Line 130: The build flag WLED_USE_SHARED_RMT is defined multiple times; remove
the definitions from the common build_flags and keep a single definition in the
esp32_idf_V5 build_flags block (or if you want it to apply everywhere, remove
the duplicates in esp32_idf_V5 and keep it only in common). Edit the
platformio.ini to ensure WLED_USE_SHARED_RMT appears exactly once and only
within the intended scope by deleting the extra entries in the
[common].build_flags or the duplicate(s) in [esp32_idf_V5].build_flags.
- Around line 387-404: The [esp32s3] environment is missing the proper unflags
and lib exclusion: replace the incorrect reference to ${common.build_unflags}
with ${esp32_idf_V5.build_unflags} (so the -Wno-volatile unflag is applied to C
files) and add the same lib_ignore entry used in other ESP32 envs (e.g.
${esp32_idf_V5.lib_ignore}) to exclude NeoESP32RmtHI from S3 builds to avoid RMT
driver conflicts; update the build_unflags and add lib_ignore under the
[esp32s3] block.
In `@wled00/dmx_input.cpp`:
- Around line 95-129: Define a named constant like DMX_PERSONALITY_COUNT and
replace the hardcoded 10 in the static array declaration (currently static
dmx_personality_t personalities[10];), in createPersonalities usage, and in
calls to dmx_driver_install and rdm_register_dmx_personality so all three sites
use DMX_PERSONALITY_COUNT; update any loops or bounds that assume 10 to
reference DMX_PERSONALITY_COUNT and ensure the constant is visible where
dmx_driver_install and rdm_register_dmx_personality are invoked.
- Around line 86-93: In createConfig(), config.software_version_label is set to
DmxVersionString.c_str() which leaves a dangling pointer when the local
std::string DmxVersionString is destroyed; fix by copying the string into the
config's fixed-size char buffer (e.g., use
strncpy(config.software_version_label, DmxVersionString.c_str(),
sizeof(config.software_version_label)-1) and set the final byte to '\0') so the
label remains valid after createConfig() returns; alternatively, if
config.software_version_label is a const char* and you prefer a static lifetime,
replace DmxVersionString with a static std::string or a static char array, but
prefer the strncpy approach to avoid global state.
In `@wled00/network.cpp`:
- Around line 234-241: The ETH.begin(...) call uses the v5/Arduino-ESP32
parameter order and needs a conditional to preserve v4 compatibility; wrap the
call to ETH.begin in a version/build guard (e.g., check CONFIG_IDF_VERSION_MAJOR
or the esp32dev_V4 define) and call ETH.begin with the v5 signature when IDF v5
is detected (using es.eth_type, es.eth_address, es.eth_mdc, es.eth_mdio,
es.eth_power, es.eth_clk_mode) but use the v4 signature when building for
esp32dev_V4/IDF v4 (placing phy_addr/eth_address as the first parameter and
ordering the rest per the v4 API) so compilation succeeds on both IDF versions.
In `@wled00/src/dependencies/espalexa/Espalexa.h`:
- Line 273: The lambda passed to serverAsync->on that handles HTTP_GET still
uses an implicit-capture `[=]`; change its capture to `[this]` to match the
other handlers and avoid implicit `this` capture warnings — i.e. update the
lambda used in the serverAsync->on("/espalexa", HTTP_GET, ...) callback that
assigns to `server` and calls `servePage()` so it captures `this` explicitly
(use `[this]`), leaving the parameter `AsyncWebServerRequest *request`
unchanged.
🧹 Nitpick comments (5)
wled00/src/dependencies/espalexa/Espalexa.h (1)
21-26: Dummy macro value is misleading but currently harmless.The real
ESP_IDF_VERSION_VALencodes as(major << 16) | (minor << 8) | patch, soESP_IDF_VERSION_VAL(5,0,0)=327680, not500. Since the only comparison site (line 389) is guarded bydefined(ARDUINO_ARCH_ESP32), this value is never evaluated on ESP8266, so it won't cause a bug today. But if anyone later adds a version check without the ESP32 guard, the result will be silently wrong.Suggested fix
-#define ESP_IDF_VERSION_VAL(n1,n2,n3) 500 +#define ESP_IDF_VERSION_VAL(n1,n2,n3) ((n1 << 16) | (n2 << 8) | (n3))platformio.ini (2)
504-522:esp32dev_V4unflagsWLED_ENABLE_DMX_INPUTwhich may not exist at that point.Line 510 uses
build_unflagswith-D WLED_ENABLE_DMX_INPUT, butbuild_unflagson line 509 inherits${common.build_unflags}which is empty. The DMX flag comes from${esp32_idf_V4.build_flags}(line 313), so it's being added and unflagged in the same env. This works in PlatformIO but the intent would be clearer by simply not including the flag in the first place via a V4-specific override.Otherwise the V4 regression test environment looks reasonable.
13-39: Several entries indefault_envsare both commented and uncommented.
esp32s3dev_8MB_opiappears uncommented on line 36 and commented on line 31.esp32s3dev_16MB_opiandesp32s3_4M_qspisimilarly appear commented in two places (lines 30+35 and 32+38). This makes it hard to tell which S3 builds are actually active. Consider cleaning up to have each env listed once.wled00/fcn_declare.h (1)
5-10: DummyESP_IDF_VERSION_VALreturns a misleading magic number.The shim defines
ESP_IDF_VERSION_VAL(n1,n2,n3)as500, which doesn't match the real macro's formula((n1<<16)|(n2<<8)|n3). While it currently works because ESP8266 guards short-circuit before evaluating IDF version comparisons, any future direct use on ESP8266 (e.g.,ESP_IDF_VERSION_VAL(1,0,0)) would yield500instead of65536, leading to silently wrong comparisons.Consider matching the real formula or using a clearly-below-everything sentinel like
0:Suggested fix
// dummy macro for 8266 `#ifndef` ARDUINO_ARCH_ESP32 `#ifndef` ESP_IDF_VERSION_VAL -#define ESP_IDF_VERSION_VAL(n1,n2,n3) 500 +#define ESP_IDF_VERSION_VAL(n1,n2,n3) ((n1 << 16) | (n2 << 8) | n3) +#endif +#ifndef ESP_IDF_VERSION +#define ESP_IDF_VERSION 0 `#endif` `#endif`wled00/dmx_input.cpp (1)
100-128: Usesizeof(personalities[0].description)instead of hardcoded32for strncpy buffer size.The
descriptionfield indmx_personality_tis 33 bytes (32 chars + null terminator). While the current code happens to work becausepersonalitiesis zero-initialized and all strings are shorter than 32 characters, usingsizeof(personalities[0].description)would be more defensive and prevent potential issues if string lengths change in the future.
| #else | ||
| // fallback code | ||
| const std::string DmxVersionString = "WLED_V" + std::to_string(VERSION); | ||
| config.software_version_label = DmxVersionString.c_str(); | ||
| #endif | ||
|
|
||
| return config; | ||
| } |
There was a problem hiding this comment.
Dangling pointer: config.software_version_label points to destroyed local std::string.
DmxVersionString is a local std::string inside createConfig(). Assigning .c_str() to config.software_version_label and then returning config leaves the pointer dangling — the string's buffer is freed when the function returns. This is undefined behavior and will likely cause garbage or a crash when the label is read.
Proposed fix — use `strncpy` into a fixed-size field, or make the string `static`
`#else`
// fallback code
- const std::string DmxVersionString = "WLED_V" + std::to_string(VERSION);
- config.software_version_label = DmxVersionString.c_str();
+ static std::string DmxVersionString = "WLED_V" + std::to_string(VERSION);
+ config.software_version_label = DmxVersionString.c_str();
`#endif`🤖 Prompt for AI Agents
In `@wled00/dmx_input.cpp` around lines 86 - 93, In createConfig(),
config.software_version_label is set to DmxVersionString.c_str() which leaves a
dangling pointer when the local std::string DmxVersionString is destroyed; fix
by copying the string into the config's fixed-size char buffer (e.g., use
strncpy(config.software_version_label, DmxVersionString.c_str(),
sizeof(config.software_version_label)-1) and set the final byte to '\0') so the
label remains valid after createConfig() returns; alternatively, if
config.software_version_label is a const char* and you prefer a static lifetime,
replace DmxVersionString with a static std::string or a static char array, but
prefer the strncpy approach to avoid global state.
| static dmx_personality_t personalities[10]; | ||
|
|
||
| static void createPersonalities() | ||
| { | ||
| // Initialize personalities array | ||
| strncpy(personalities[0].description, "SINGLE_RGB", 32); | ||
| personalities[0].footprint = 3; | ||
|
|
||
| strncpy(personalities[1].description, "SINGLE_DRGB", 32); | ||
| personalities[1].footprint = 4; | ||
|
|
||
| strncpy(personalities[2].description, "EFFECT", 32); | ||
| personalities[2].footprint = 15; | ||
|
|
||
| strncpy(personalities[3].description, "MULTIPLE_RGB", 32); | ||
| personalities[3].footprint = std::min(512, int(strip.getLengthTotal()) * 3); | ||
|
|
||
| strncpy(personalities[4].description, "MULTIPLE_DRGB", 32); | ||
| personalities[4].footprint = std::min(512, int(strip.getLengthTotal()) * 3 + 1); | ||
|
|
||
| strncpy(personalities[5].description, "MULTIPLE_RGBW", 32); | ||
| personalities[5].footprint = std::min(512, int(strip.getLengthTotal()) * 4); | ||
|
|
||
| strncpy(personalities[6].description, "EFFECT_W", 32); | ||
| personalities[6].footprint = 18; | ||
|
|
||
| strncpy(personalities[7].description, "EFFECT_SEGMENT", 32); | ||
| personalities[7].footprint = std::min(512, strip.getSegmentsNum() * 15); | ||
|
|
||
| strncpy(personalities[8].description, "EFFECT_SEGMENT_W", 32); | ||
| personalities[8].footprint = std::min(512, strip.getSegmentsNum() * 18); | ||
|
|
||
| strncpy(personalities[9].description, "PRESET", 32); | ||
| personalities[9].footprint = 1; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Magic number 10 repeated for personality count — extract a named constant.
The literal 10 appears in the static array size (Line 95), dmx_driver_install (Line 151), and rdm_register_dmx_personality (Line 167). If one site is updated but not the others, it silently breaks. A single constant keeps them in sync.
Proposed refactor
+static constexpr size_t DMX_PERSONALITY_COUNT = 10;
+
-static dmx_personality_t personalities[10];
+static dmx_personality_t personalities[DMX_PERSONALITY_COUNT];Then use DMX_PERSONALITY_COUNT in dmx_driver_install and rdm_register_dmx_personality calls as well.
Also applies to: 148-151, 167-167
🤖 Prompt for AI Agents
In `@wled00/dmx_input.cpp` around lines 95 - 129, Define a named constant like
DMX_PERSONALITY_COUNT and replace the hardcoded 10 in the static array
declaration (currently static dmx_personality_t personalities[10];), in
createPersonalities usage, and in calls to dmx_driver_install and
rdm_register_dmx_personality so all three sites use DMX_PERSONALITY_COUNT;
update any loops or bounds that assume 10 to reference DMX_PERSONALITY_COUNT and
ensure the constant is visible where dmx_driver_install and
rdm_register_dmx_personality are invoked.
to check if 8266 code was broken accidentally
|
@coderabbitai i need a concise summary of open points in this PR. |
|
Tip For best results, initiate chat on the files or code changes.
🔧 Core WLED Code Issues1. util.cpp - Device Identification FunctionsStatus: ❌ Broken in V5 builds (conditionally compiled out)
2. ADC Deprecated ConstantsStatus:
🏗️ Build System & Tooling3. PlatformIO InstabilityStatus:
4. Usermod CI BuildsStatus: ⏸️ Generating ~100 extra builds
5. Platform Package DecisionStatus: 🤔 Strategy discussion needed
🔌 Usermods Compatibility6. pwm_outputs UsermodStatus: ❌ Missing LEDC API guards
7. deep_sleep UsermodStatus:
8. audioreactive UsermodStatus: 🚧 Disabled for V5
9. Usermod Registration MechanismStatus: ✅ Fixed with hybrid approach
📚 External Dependencies10. NeoPixelBus/RMT Driver ConflictsStatus: ✅ Mitigated with build flag
11. mbedtls SHA1 FunctionsStatus: ✅ Fixed with shim
🎯 Action Items SummaryCritical (breaks builds):
Important (warnings/future-proofing): Strategic decisions needed: Would you like me to create GitHub issues for the critical action items, or generate patch proposals for items 🧠 Learnings used |
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