Skip to content

Conversation

netmindz
Copy link
Member

@netmindz netmindz commented Aug 14, 2025

Update the code to handle at least being able to compile against V5

Copy link
Contributor

coderabbitai bot commented Aug 14, 2025

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch V5

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@netmindz netmindz changed the title V5 ESP-IDF V5 Aug 14, 2025
@netmindz netmindz mentioned this pull request Aug 15, 2025
@netmindz netmindz requested a review from willmmiles August 16, 2025 11:30
@netmindz
Copy link
Member Author

I'm not sure if renaming Network is the right approach to fix the conflict @willmmiles

@netmindz
Copy link
Member Author

I am also unsure why we are seeing linker errors for things like mbedtls_sha1_starts

@netmindz netmindz added this to the 0.17.0 milestone Aug 16, 2025
@willmmiles
Copy link
Member

I'm not sure if renaming Network is the right approach to fix the conflict @willmmiles

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. Arduino::Network or somesuch, but here we are.)

I am also unsure why we are seeing linker errors for things like mbedtls_sha1_starts

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.

@sombree
Copy link

sombree commented Aug 25, 2025

With few fixes it builds.
image

MQTT
With the change below I was able to build it (I don’t have a way to test runtime behavior):
In AsyncMqttClient.hpp line 41, change:
#if ESP32
to
#ifdef ESP32

mbedTLS
Upstream ESPAsyncWebServer made the change here: 57244d47444a3281ba30f4c9da60fb23d05eb495
Applying similar changes to Aircoookie/ESPAsyncWebServer.git#v2.4.2 made it buildable.

WiFi
I had crash related to memcpy and MAC adress was shown as 000000000000. Quick fixes are below.

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

  1. Guard empty Wi-Fi list before accessing multiWiFi[0]
@@ 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;
+  }
  1. Safe BSSID compare (both sides must exist & configured BSSID non-zero)
@@ 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 BSSID

Usermods
I had more issues with usermods—the new toolchain’s linker was GC’ing/orphaning the old section-based registration. I switched registration to a ctor-based dynamic hook (no new sections) and kept legacy dtors for backward compatibility.
I used the RF433 usermod as the example.

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
I also had issues with NeoPixelBus. At boot, IDF runs check_rmt_legacy_driver_conflict() and aborts if both the old (legacy) RMT driver and the new (“driver_ng” / v2) RMT driver are linked into firmware.
I switched to Makuna/NeoPixelBus.git#CORE3 and made a small change:

--- 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
// ---------------------------------------------------------------------------

@netmindz
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants