Hostname unification 2: The Force Push Strikes Back#4893
Hostname unification 2: The Force Push Strikes Back#4893
Conversation
…hostname to "nw" key in config JSON - add new config key in "nw" for mDNS (bool) - remove `prepareHostname()` function - update mDNS calls - add hostname update for Ethernet - move WiFi configuration to cfg.json - rename LED and WiFi to Hardware and Network in settings - reorder network settings
WalkthroughThis PR centralizes hostname and mDNS handling: replaces cmDNS with hostName and adds mDNSenabled; updates resolution, MQTT, OTA, MDNS init/update, and captive portal. Config schema shifts hostname/mDNS to nw.*, adds defaults, and applies WiFi/Ethernet settings. UI pages and XML reflect new fields; utility APIs updated and hardened. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
netmindz
added
connectivity
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
wled00/mqtt.cpp (1)
1-243: Remove leftover cmDNS reference and convert MDNS.queryHost calls to resolveHostname
- wled00/wled_eeprom.cpp:83: replace the
cmDNSargument inwith the updated hostname storage key or removereadStringFromEEPROM(96, cmDNS, 32);cmDNSentirely.- wled00/network.cpp:277: change
toclnt = MDNS.queryHost(mDNSname.c_str());clnt = resolveHostname(mDNSname);- wled00/mqtt.cpp:222: change
tomqttIP = MDNS.queryHost(mqttMDNS.c_str());mqttIP = resolveHostname(mqttMDNS);- wled00/bus_manager.cpp:733: change
toclnt = MDNS.queryHost(_hostname);clnt = resolveHostname(_hostname);- No
prepareHostname(...)calls with a single argument remain.wled00/bus_manager.cpp (2)
728-737: Use resolveHostname() to normalize names and avoid duplicated mDNS logic.This path doesn’t lowercase or strip “.local” like other code. Call the shared resolver for consistent behavior and implicit fallback to WiFi.hostByName.
Apply:
#ifdef ARDUINO_ARCH_ESP32 void BusNetwork::resolveHostname() { static unsigned long nextResolve = 0; if (Network.isConnected() && millis() > nextResolve && _hostname.length() > 0) { nextResolve = millis() + 600000; // resolve only every 10 minutes - IPAddress clnt; - if (mDNSenabled) clnt = MDNS.queryHost(_hostname); - else WiFi.hostByName(_hostname.c_str(), clnt); + IPAddress clnt = resolveHostname(_hostname, mDNSenabled); if (clnt != IPAddress()) _client = clnt; } } #endifIf resolveHostname() isn’t visible here, include its declaration:
#include "bus_wrapper.h" +#include "fcn_declare.h" // resolveHostname()
877-906: Syntax error: missing semicolon after u++.This breaks compilation.
Apply:
- u++ + u++;wled00/set.cpp (1)
35-41: Potentially inverted client SSID reconnect check.Unchanged code but likely wrong: it reconnects when SSID is equal. Consider flipping to reconnect only when changed or previously empty.
Proposed fix (context only):
- if (strlen(oldSSID) == 0 || !strncmp(multiWiFi[n].clientSSID, oldSSID, 32)) { + if (strlen(oldSSID) == 0 || strncmp(multiWiFi[n].clientSSID, oldSSID, 32) != 0) { forceReconnect = true; }
🧹 Nitpick comments (6)
wled00/data/style.css (2)
67-69: Consolidate invalid-field styling and fix indentation to tabsYou now style input:invalid here and again on Line 82. Keep a single rule and use tabs per repo guideline for data web files.
Apply within this hunk:
-.err, input:invalid { - color: #f00; -} +.err, input:invalid { + color: #f00; +}
82-84: Remove duplicate input:invalid ruleThis duplicates the styling added on Line 67. Drop the later block.
-input:invalid { - color: #f00; -}wled00/mqtt.cpp (1)
216-230: Unify MQTT host resolution via resolveHostname() (honors mDNSenabled and normalizes .local).Avoid duplicating mDNS logic and string munging here. Use the new resolveHostname() helper so behavior matches the rest of the stack and you still fall back cleanly.
Apply:
#ifdef ARDUINO_ARCH_ESP32 - String mqttMDNS = mqttServer; - mqttMDNS.toLowerCase(); // make sure we have a lowercase hostname - int pos = mqttMDNS.indexOf(F(".local")); - if (pos > 0) mqttMDNS.remove(pos); // remove .local domain if present (and anything following it) - if (mDNSenabled && mqttMDNS.length() > 0 && mqttMDNS.indexOf('.') < 0) { // if mDNS is enabled and server does not have domain - mqttIP = MDNS.queryHost(mqttMDNS.c_str()); - if (mqttIP != IPAddress()) // if MDNS resolved the hostname - mqtt->setServer(mqttIP, mqttPort); - else - mqtt->setServer(mqttServer, mqttPort); - } else + IPAddress ip = resolveHostname(String(mqttServer), mDNSenabled); + if (ip != IPAddress()) { + mqtt->setServer(ip, mqttPort); + } else #endif mqtt->setServer(mqttServer, mqttPort);wled00/improv.cpp (1)
209-211: snprintf size: pass full buffer size for clarity.Using sizeof(vString) instead of sizeof(vString)-1 is simpler; snprintf already reserves space for the NUL.
Apply:
- char vString[33]; - snprintf_P(vString, sizeof(vString)-1, PSTR("%s/%i"), versionString, VERSION); + char vString[33]; + snprintf_P(vString, sizeof(vString), PSTR("%s/%i"), versionString, VERSION);wled00/data/settings_ui.htm (1)
221-225: Minor UX polish for “Device name”.Consider disabling autocorrect and spellcheck to avoid browser meddling.
Apply:
- Device name: <input type="text" name="DS" minlength="2" maxlength="32"><br> + Device name: <input type="text" name="DS" minlength="2" maxlength="32" autocomplete="off" spellcheck="false"><br>wled00/xml.cpp (1)
277-277: Consider making the comment more actionable.The comment mentions using
WiFi.softAPmacAddress()but doesn't explain when or why. Consider expanding it.- // WiFi.softAPmacAddress() for AP MAC address + // TODO: Consider displaying WiFi.softAPmacAddress() when AP is inactive to show AP MAC for reference
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (16)
wled00/bus_manager.cpp(2 hunks)wled00/cfg.cpp(4 hunks)wled00/data/settings.htm(1 hunks)wled00/data/settings_ui.htm(2 hunks)wled00/data/settings_wifi.htm(5 hunks)wled00/data/style.css(1 hunks)wled00/fcn_declare.h(2 hunks)wled00/improv.cpp(1 hunks)wled00/mqtt.cpp(1 hunks)wled00/network.cpp(3 hunks)wled00/set.cpp(3 hunks)wled00/util.cpp(1 hunks)wled00/wled.cpp(4 hunks)wled00/wled.h(1 hunks)wled00/wled_server.cpp(1 hunks)wled00/xml.cpp(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
wled00/{*.cpp,!(html_*)*.h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ source and header files
Files:
wled00/wled_server.cppwled00/fcn_declare.hwled00/improv.cppwled00/mqtt.cppwled00/wled.cppwled00/set.cppwled00/bus_manager.cppwled00/wled.hwled00/cfg.cppwled00/network.cppwled00/xml.cppwled00/util.cpp
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/settings.htmwled00/data/style.csswled00/data/settings_ui.htmwled00/data/settings_wifi.htm
🧠 Learnings (3)
📚 Learning: 2025-08-28T05:32:32.719Z
Learnt from: CR
PR: wled/WLED#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-28T05:32:32.719Z
Learning: Applies to wled00/html_*.h : Do not edit generated embedded web files (wled00/html_*.h)
Applied to files:
wled00/wled_server.cppwled00/set.cppwled00/wled.h
📚 Learning: 2025-04-26T12:51:04.993Z
Learnt from: KrX3D
PR: wled/WLED#4664
File: wled00/syslog.cpp:50-51
Timestamp: 2025-04-26T12:51:04.993Z
Learning: In the WLED codebase, syslog configuration variables (syslogHost, syslogPort, syslogProtocol, syslogFacility, syslogSeverity) are defined as global variables in wled.h and are accessed directly in the SyslogPrinter implementation, rather than being class members.
Applied to files:
wled00/wled.h
📚 Learning: 2025-04-18T22:27:58.634Z
Learnt from: KrX3D
PR: wled/WLED#4237
File: usermods/INA219_v2/INA219_v2.cpp:265-276
Timestamp: 2025-04-18T22:27:58.634Z
Learning: When implementing MQTT message handling in WLED usermods, use `strstr(topic, "/specific/path")` instead of `strcmp_P(topic, PSTR("/specific/path"))` to properly match topics that include the device prefix. The full MQTT topic typically follows the pattern `<mqttDeviceTopic>/specific/path`.
Applied to files:
wled00/cfg.cpp
🧬 Code graph analysis (5)
wled00/fcn_declare.h (3)
wled00/bus_manager.cpp (2)
resolveHostname(728-737)resolveHostname(728-728)wled00/network.cpp (2)
resolveHostname(268-283)resolveHostname(268-268)wled00/util.cpp (2)
prepareHostname(131-149)prepareHostname(131-131)
wled00/wled.cpp (1)
wled00/util.cpp (2)
prepareHostname(131-149)prepareHostname(131-131)
wled00/set.cpp (1)
wled00/util.cpp (4)
prepareHostname(131-149)prepareHostname(131-131)isAsterisksOnly(151-159)isAsterisksOnly(151-151)
wled00/cfg.cpp (1)
wled00/util.cpp (2)
prepareHostname(131-149)prepareHostname(131-131)
wled00/xml.cpp (1)
wled00/util.cpp (8)
printSetFormValue(111-113)printSetFormValue(111-111)printSetFormValue(118-120)printSetFormValue(118-118)printSetFormCheckbox(108-110)printSetFormCheckbox(108-108)printSetClassElementHTML(122-124)printSetClassElementHTML(122-122)
🔇 Additional comments (35)
wled00/data/settings.htm (1)
41-42: LGTM: label updates only; routes unchangedThe button text changes read better and keep existing endpoints.
Also applies to: 47-47
wled00/fcn_declare.h (2)
257-257: LGTM: resolveHostname() declaration matches intended behaviorDefault parameter keeps MDNS optional at call sites.
392-392: prepareHostname callers correctly pass maxLen
All three usages in wled.cpp, set.cpp, and cfg.cpp now explicitly provide the maxLen argument; no further updates needed.wled00/mqtt.cpp (2)
216-230: Heads-up: .local without mDNS will likely fail to resolve.If users set an MQTT server like foo.local while mDNS is disabled, AsyncMqttClient will try standard DNS (and fail). Consider warning in UI or documenting this, or automatically strip “.local” only when mDNSenabled is true.
221-221: LGTM: mDNS gating.Switching the gate from cmDNS length to mDNSenabled is correct and matches the PR’s intent.
wled00/bus_manager.cpp (1)
31-31: LGTM: new global gate.Extern mDNSenabled aligns this module with the new global switch.
wled00/improv.cpp (1)
212-213: LGTM: prefer hostName when serverDescription is default.This matches the new hostname model and removes cmDNS coupling.
wled00/data/settings_ui.htm (1)
156-159: LGTM: Save gating simplified.Dropping ST from the submit condition aligns with the UI changes that removed ST.
wled00/set.cpp (2)
85-86: LGTM: bounds-safe AP password handling.Using isAsterisksOnly() and sizeof-based strlcpy is correct.
296-297: LGTM: unsigned loop/index.Matches intent and avoids UB with negative comparisons.
wled00/xml.cpp (4)
193-193: Good defensive programming with buffer size declaration.Adding an explicit buffer size for the local variable improves safety and clarity.
216-217: LGTM! Hostname and mDNS settings properly migrated.The change from
cmDNStohostNameand addition ofmDNSenabledcheckbox aligns well with the PR's hostname unification objective.
270-273: Good: Added null check for AP IP display.The addition of
apActivecheck prevents displaying invalid AP information when the AP is not active, improving data accuracy.
273-273: Buffer-safe formatting withsnprintf.Using
snprintfwith explicit buffer size is the correct approach for safe string formatting.wled00/wled.cpp (4)
144-144: Appropriate mDNS update gating on ESP8266.Good practice to only update mDNS when it's enabled, avoiding unnecessary processing.
426-427: Hostname initialization logic looks correct.The code properly checks if hostname equals the default and generates a unique one if needed. The size calculation
sizeof(hostName)-1ensures null termination space.
500-500: ArduinoOTA hostname correctly updated.The OTA hostname now uses the unified
hostNamevariable, maintaining consistency across the system.
713-717: mDNS initialization properly gated.The mDNS service is correctly initialized only when
mDNSenabledis true, with proper cleanup (MDNS.end()) before reinitializing.wled00/util.cpp (2)
126-131: Well-documented function signature change.The addition of comprehensive documentation explaining the function's behavior, constraints, and parameters is excellent. The
maxLenparameter addition improves safety by preventing buffer overflows.
132-148: Robust hostname generation with proper bounds checking.The implementation correctly:
- Respects the
maxLenboundary- Generates a MAC-based default hostname
- Sanitizes
serverDescriptioncharacters appropriately- Strips trailing hyphens
- Ensures null termination
wled00/network.cpp (4)
268-283: New hostname resolution function is well-implemented.The function properly handles both mDNS and DNS resolution with appropriate fallbacks. Good defensive programming with network connectivity check.
285-291: Verify the intent of the empty string logic.The function sets the string to empty if all MAC bytes are zero. This seems intentional but could be surprising behavior.
Is this intended to handle uninitialized or invalid MAC addresses? If so, consider adding a comment to clarify this behavior:
void fillMAC2Str(char *str, const uint8_t *mac) { sprintf_P(str, PSTR("%02x%02x%02x%02x%02x%02x"), MAC2STR(mac)); byte nul = 0; for (int i = 0; i < 6; i++) nul |= *mac++; // do we have 0 - if (!nul) str[0] = '\0'; // empty string + if (!nul) str[0] = '\0'; // empty string for all-zero MAC (uninitialized/invalid) }
407-407: Preprocessor directive style consistency.Changed from
#if defined(...)to#ifdef ...for consistency.
417-417: Ethernet hostname correctly set.The Ethernet hostname is now consistently set to the unified
hostNamevariable.wled00/data/settings_wifi.htm (6)
6-6: UI title updated to reflect broader scope.The change from "WiFi Settings" to "Network Settings" accurately reflects that this page now handles WiFi, Ethernet, and ESP-NOW configuration.
137-137: Good use of callback for dynamic UI updates.Loading the settings script with a callback to
genUrlensures the UI updates after settings are loaded.
147-147: Verify ESP-NOW and Ethernet incompatibility handling.The code disables Ethernet when ESP-NOW is enabled, but should we also disable ESP-NOW when Ethernet is selected?
Consider bidirectional enforcement:
function tE() { // keep the hidden input with MAC addresses, only toggle visibility of the list UI gId('rlc').style.display = d.Sf.RE.checked ? 'block' : 'none'; if (d.Sf.RE.checked) d.Sf.ETH.selectedIndex = 0; // disable Ethernet if ESPNOW is enabled }Also update the ETH select onchange:
-<select name="ETH" onchange="if(this.selectedIndex!=0)d.Sf.RE.checked=false;"> +<select name="ETH" onchange="if(this.selectedIndex!=0){d.Sf.RE.checked=false;tE();}">This ensures the ESP-NOW UI is properly hidden when Ethernet is selected.
180-183: Live URL preview implementation is helpful.The
genUrl()function provides immediate feedback to users about their hostname configuration and mDNS availability.
195-195: Good hostname validation pattern.The pattern
[a-zA-Z0-9_\-]*correctly restricts hostnames to valid characters. However, consider if empty hostnames should be allowed.Should we enforce a minimum length to prevent empty hostnames?
-<input type="text" name="CM" minlength="2" maxlength="32" pattern="[a-zA-Z0-9_\-]*" oninput="genUrl()"><br> +<input type="text" name="CM" minlength="2" maxlength="32" pattern="[a-zA-Z0-9_\-]+" required oninput="genUrl()"><br>
211-211: Improved AP SSID validation.Good addition of the pattern attribute to restrict AP SSID to valid characters. The empty option for disabling AP is clearly indicated.
wled00/cfg.cpp (5)
160-169: Good implementation of WiFi hostname configurationThe WiFi hostname is properly applied using the unified
hostNamevariable for both ESP32 and ESP8266 platforms. This ensures consistency across different hardware platforms.
658-660: MQTT default topic generation uses MAC-based patternThe automatic generation of MQTT client ID and device topic correctly uses the device's MAC address, which aligns well with the hostname unification approach where MAC-based identifiers are used as fallbacks.
871-879: Serialization correctly implements new schemaThe serialization properly writes
serverDescriptiontoid.nameandhostNametonw.name, withmDNSenabledwritten tonw.mdns. This correctly implements the new unified hostname schema.
69-69: Size usage is correct: alexaInvocationName[33]
sizeof(alexaInvocationName)yields 33, matching its definition in wled00/wled.h.
163-164: txPower is properly constrained and cast to wifi_power_t; no changes requiredThe value is clamped between WIFI_POWER_2dBm and WIFI_POWER_19_5dBm at line 157 before being passed to WiFi.setTxPower at line 164.
| getStringFromJson(serverDescription, id["name"], sizeof(serverDescription)); | ||
| // legacy behaviour | ||
| getStringFromJson(hostName, id[F("mdns")], sizeof(hostName)); | ||
| if (strlen(hostName) == 0) { | ||
| mDNSenabled = false; // if no host name is set, disable mDNS | ||
| prepareHostname(hostName, sizeof(hostName)-1); | ||
| } | ||
|
|
There was a problem hiding this comment.
Potential issue with legacy hostname migration logic
The current implementation reads from id["mdns"] into hostName for legacy compatibility, but if that field is empty, it disables mDNS and generates a new hostname. This could unexpectedly disable mDNS for users who had it enabled but didn't have an explicit hostname set in their legacy configuration.
Consider preserving the existing mDNS enabled state when migrating legacy configurations:
- // legacy behaviour
- getStringFromJson(hostName, id[F("mdns")], sizeof(hostName));
- if (strlen(hostName) == 0) {
- mDNSenabled = false; // if no host name is set, disable mDNS
- prepareHostname(hostName, sizeof(hostName)-1);
- }
+ // legacy behaviour - preserve existing mDNS state
+ const char* legacyHostname = id[F("mdns")];
+ if (legacyHostname != nullptr) {
+ getStringFromJson(hostName, legacyHostname, sizeof(hostName));
+ }
+ // Generate hostname if empty (from either legacy or new location)
+ if (strlen(hostName) == 0) {
+ prepareHostname(hostName, sizeof(hostName)-1);
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In wled00/cfg.cpp around lines 60 to 67, the legacy migration currently zeroes
mDNSenabled when id["mdns"] yields an empty hostName; instead, preserve the
existing mDNSenabled flag and only change it if the legacy config explicitly
indicates mDNS should be disabled. Update the logic to: read hostName from
id["mdns"], if hostName is empty call prepareHostname(...) to generate a
hostname but do not set mDNSenabled = false; only set mDNSenabled = false when
the legacy field explicitly signals mDNS should be disabled (or when another
explicit legacy flag indicates disablement).
| CJSON(mDNSenabled, nw[F("mdns")]); | ||
| getStringFromJson(hostName, nw["name"], sizeof(hostName)); |
There was a problem hiding this comment.
Order of operations issue: mDNS flag might be overwritten
The mDNS enabled flag is set on line 74 from the new location nw["mdns"], but the legacy code on lines 62-66 might have already set it to false. This could cause the new value to override the legacy migration logic unexpectedly.
Consider reordering to handle legacy migration after reading the new values:
JsonObject nw = doc["nw"];
- CJSON(mDNSenabled, nw[F("mdns")]);
- getStringFromJson(hostName, nw["name"], sizeof(hostName));
+ // Read new format first
+ bool hasNewHostname = nw["name"] != nullptr;
+ if (hasNewHostname) {
+ getStringFromJson(hostName, nw["name"], sizeof(hostName));
+ CJSON(mDNSenabled, nw[F("mdns")]);
+ }
+ // Only apply legacy if new format not present
+ else if (id[F("mdns")] != nullptr) {
+ getStringFromJson(hostName, id[F("mdns")], sizeof(hostName));
+ // Don't force mDNS disabled for legacy configs
+ }
+ // Generate hostname if still empty
+ if (strlen(hostName) == 0) {
+ prepareHostname(hostName, sizeof(hostName)-1);
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In wled00/cfg.cpp around lines 62-75, the new mDNS flag is read from nw["mdns"]
on line 74 which can be unintentionally overridden by the earlier legacy
migration block (lines 62-66); move the legacy migration logic to run after you
read the new values (i.e., after the CJSON/getStringFromJson lines) or,
alternatively, keep reading nw["mdns"] first and then apply the legacy check to
explicitly override mDNSenabled when legacy storage indicates it should be
disabled; update the surrounding comment to clarify the order and ensure the
final mDNSenabled value reflects legacy migration when applicable.
| strlcpy(hostName, request->arg(F("CM")).c_str(), sizeof(hostName)); | ||
| if (strlen(hostName) == 0) prepareHostname(hostName, sizeof(hostName)-1); | ||
| #ifdef ARDUINO_ARCH_ESP32 | ||
| #ifdef WLED_USE_ETHERNET | ||
| ETH.setHostname(hostName); | ||
| #endif | ||
| WiFi.setHostname(hostName); | ||
| #else | ||
| WiFi.hostname(hostName); | ||
| #endif | ||
| mDNSenabled = request->hasArg(F("MD")); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Always sanitize hostName and trigger reconnect when it changes.
Currently prepareHostname() runs only if CM is empty, so user-supplied values can slip through with illegal chars/length. Also, hostname changes should set forceReconnect so stacks pick them up.
Apply:
- strlcpy(hostName, request->arg(F("CM")).c_str(), sizeof(hostName));
- if (strlen(hostName) == 0) prepareHostname(hostName, sizeof(hostName)-1);
+ char oldHost[sizeof(hostName)]; strlcpy(oldHost, hostName, sizeof(oldHost));
+ strlcpy(hostName, request->arg(F("CM")).c_str(), sizeof(hostName));
+ // Sanitize and bound length regardless of user input
+ prepareHostname(hostName, sizeof(hostName)-1);
+ if (strcmp(oldHost, hostName) != 0) forceReconnect = true;
#ifdef ARDUINO_ARCH_ESP32
#ifdef WLED_USE_ETHERNET
ETH.setHostname(hostName);
#endif
WiFi.setHostname(hostName);
#else
WiFi.hostname(hostName);
#endif
mDNSenabled = request->hasArg(F("MD"));📝 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.
| strlcpy(hostName, request->arg(F("CM")).c_str(), sizeof(hostName)); | |
| if (strlen(hostName) == 0) prepareHostname(hostName, sizeof(hostName)-1); | |
| #ifdef ARDUINO_ARCH_ESP32 | |
| #ifdef WLED_USE_ETHERNET | |
| ETH.setHostname(hostName); | |
| #endif | |
| WiFi.setHostname(hostName); | |
| #else | |
| WiFi.hostname(hostName); | |
| #endif | |
| mDNSenabled = request->hasArg(F("MD")); | |
| // Always sanitize hostName and trigger reconnect on change | |
| char oldHost[sizeof(hostName)]; strlcpy(oldHost, hostName, sizeof(oldHost)); | |
| strlcpy(hostName, request->arg(F("CM")).c_str(), sizeof(hostName)); | |
| // Sanitize and bound length regardless of user input | |
| prepareHostname(hostName, sizeof(hostName)-1); | |
| if (strcmp(oldHost, hostName) != 0) forceReconnect = true; | |
| #ifdef ARDUINO_ARCH_ESP32 | |
| #ifdef WLED_USE_ETHERNET | |
| ETH.setHostname(hostName); | |
| #endif | |
| WiFi.setHostname(hostName); | |
| #else | |
| WiFi.hostname(hostName); | |
| #endif | |
| mDNSenabled = request->hasArg(F("MD")); |
🤖 Prompt for AI Agents
In wled00/set.cpp around lines 66 to 77, always sanitize the user-supplied
hostName and trigger a reconnect when it changes: call prepareHostname(hostName,
sizeof(hostName)) (or an appropriate sanitizer/truncator) for both empty and
non-empty CM values instead of only when CM is empty, compare the sanitized
hostName against the current/previous hostname and if different set
forceReconnect = true so network stacks pick up the change, and then call the
appropriate platform hostname APIs (ETH/WiFi) with the sanitized value.
| char oldSSID[33]; strcpy(oldSSID, apSSID); | ||
| strlcpy(apSSID, request->arg(F("AS")).c_str(), 33); | ||
| strlcpy(apSSID, request->arg(F("AS")).c_str(), sizeof(apSSID)); | ||
| if (!strcmp(oldSSID, apSSID) && apActive) forceReconnect = true; | ||
| apHide = request->hasArg(F("AH")); |
There was a problem hiding this comment.
Fix AP SSID change detection (current logic flips condition).
The code forces reconnect when SSID is unchanged.
Apply:
- if (!strcmp(oldSSID, apSSID) && apActive) forceReconnect = true;
+ if (strcmp(oldSSID, apSSID) != 0 && apActive) forceReconnect = true;📝 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.
| char oldSSID[33]; strcpy(oldSSID, apSSID); | |
| strlcpy(apSSID, request->arg(F("AS")).c_str(), 33); | |
| strlcpy(apSSID, request->arg(F("AS")).c_str(), sizeof(apSSID)); | |
| if (!strcmp(oldSSID, apSSID) && apActive) forceReconnect = true; | |
| apHide = request->hasArg(F("AH")); | |
| char oldSSID[33]; strcpy(oldSSID, apSSID); | |
| strlcpy(apSSID, request->arg(F("AS")).c_str(), sizeof(apSSID)); | |
| if (strcmp(oldSSID, apSSID) != 0 && apActive) forceReconnect = true; | |
| apHide = request->hasArg(F("AH")); |
🤖 Prompt for AI Agents
In wled00/set.cpp around lines 79 to 82, the logic that sets forceReconnect is
inverted so it triggers when the SSID is unchanged; change the comparison to
detect a difference and only force reconnect when the SSID actually changed
while AP is active (i.e., replace the current equality check with a non-zero
strcmp/strncmp result combined with apActive) so forceReconnect is set only when
oldSSID and apSSID differ and apActive is true.
|
|
||
| String hostH = request->getHeader(F("Host"))->value(); | ||
| if (!isIp(hostH) && hostH.indexOf(F("wled.me")) < 0 && hostH.indexOf(cmDNS) < 0 && hostH.indexOf(':') < 0) { | ||
| if (!isIp(hostH) && hostH.indexOf(F("wled.me")) < 0 && hostH.indexOf(hostName) < 0 && hostH.indexOf(':') < 0) { |
There was a problem hiding this comment.
Captive-portal false negative when hostName is empty; also consider case-insensitive match
If hostName == "", hostH.indexOf(hostName) returns 0 and bypasses the captive portal erroneously. Add an emptiness guard; optionally normalize case.
Minimal fix:
- if (!isIp(hostH) && hostH.indexOf(F("wled.me")) < 0 && hostH.indexOf(hostName) < 0 && hostH.indexOf(':') < 0) {
+ if (!isIp(hostH) && hostH.indexOf(F("wled.me")) < 0 && (hostName[0] == 0 || hostH.indexOf(hostName) < 0) && hostH.indexOf(':') < 0) {(Optional) case-insensitive match:
- if (!isIp(hostH) && hostH.indexOf(F("wled.me")) < 0 && (hostName[0] == 0 || hostH.indexOf(hostName) < 0) && hostH.indexOf(':') < 0) {
+ String hostHL = hostH; hostHL.toLowerCase();
+ String ownHL = String(hostName); ownHL.toLowerCase();
+ if (!isIp(hostH) && hostHL.indexOf(F("wled.me")) < 0 && (ownHL.length() == 0 || hostHL.indexOf(ownHL) < 0) && hostH.indexOf(':') < 0) {🤖 Prompt for AI Agents
In wled00/wled_server.cpp around line 237, the host check treats an empty
hostName as matching (hostH.indexOf(hostName) == 0), causing false negatives for
captive-portal; also matches are case-sensitive. Fix by first checking hostName
is non-empty before calling indexOf, and optionally normalize both hostH and
hostName to the same case (e.g., toLowerCase) before testing. Update the
conditional to require hostName.length() > 0 (or !hostName.isEmpty()) before
evaluating hostH.indexOf(hostName) and/or compare with case-insensitive
variants.
| WLED_GLOBAL char hostName[33] _INIT(MDNS_NAME); // mDNS address (*.local, replaced by wled-XXXXXX if default is used) | ||
| WLED_GLOBAL bool mDNSenabled _INIT(true); // use mDNS (default is true, can be changed in web UI) | ||
| WLED_GLOBAL char apSSID[33] _INIT(""); // AP off by default (unless setup) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Good: unified hostName + mDNSenabled
Defaults and sizes look consistent with hostname usage elsewhere.
To ensure no leftover symbols cause build issues (per PR note about cmDNS in wled_eeprom.cpp), run:
🏁 Script executed:
#!/bin/bash
# Find any remaining cmDNS references
rg -nP '\bcmDNS\b' -C2Length of output: 392
Update EEPROM handlers to use hostName instead of residual cmDNS
In wled00/wled_eeprom.cpp (around line 83), the EEPROM read/write calls still reference cmDNS. Replace these with hostName (adjusting the size parameter to 33) to match the new declaration and avoid undefined-symbol build errors.
|
Hey! This pull request has been open for quite some time without any new comments now. It will be closed automatically in a week if no further activity occurs. |
github-actions
bot
added
the
stale
softhack007
added
waiting for feedback
github-actions
bot
removed
the
stale
| #ifdef ARDUINO_ARCH_ESP32 | ||
| int tx = request->arg(F("TX")).toInt(); | ||
| txPower = min(max(tx, (int)WIFI_POWER_2dBm), (int)WIFI_POWER_19_5dBm); | ||
| WiFi.setTxPower(wifi_power_t(txPower)); |
There was a problem hiding this comment.
@coderabbitai please consult the source code and documentation of arduino-esp32 v2.0.18, and verify if moving WiFi.setTxPower() and WiFi.setSleep() here is still ensuring that wifi commands happen in the correct sequence. Analyze the program flow to justify your conclusions.
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
arduino-esp32 v2.0.18 WiFi setTxPower setSleep sequence requirements esp_wifi
💡 Result:
For arduino-esp32 v2.0.18, WiFi.setTxPower(...) ultimately calls the ESP-IDF API esp_wifi_set_max_tx_power(...), whose requirement is:
- Call only after the Wi‑Fi driver is started (i.e., after
esp_wifi_start()), per ESP-IDF: “Set maximum transmitting power after WiFi start.” [1] - If you call it before Wi‑Fi is started, ESP-IDF will fail (commonly
ESP_ERR_WIFI_NOT_STARTED/ESP_ERR_WIFI_NOT_START). This is also discussed in Espressif’s tracker. [2]
WiFi.setSleep(...) maps to ESP-IDF power-save control (esp_wifi_set_ps(...)), which sets the current Wi‑Fi power save type (default is WIFI_PS_MIN_MODEM). [3]
Practical sequencing (Arduino)
Safe, version-agnostic order in Arduino sketches:
WiFi.mode(WIFI_STA)(orWIFI_AP,WIFI_AP_STA)WiFi.begin(ssid, pass)(this is what causes the underlying driver start)- After that, call:
WiFi.setTxPower(...)WiFi.setSleep(...)
You do not need to wait for WL_CONNECTED to set TX power; you just need Wi‑Fi to be started.
BLE/NimBLE interaction note (common pitfall)
If you are using NimBLE-Arduino, disabling Wi‑Fi modem sleep (e.g. WiFi.setSleep(false) / esp_wifi_set_ps(WIFI_PS_NONE)) before initializing NimBLE has been reported to crash; initialize BLE first or delay changing Wi‑Fi power-save settings. [4]
Sources: ESP-IDF Wi‑Fi API docs for esp_wifi_set_max_tx_power / esp_wifi_set_ps [1][3]; Espressif issue discussion of esp_wifi_set_max_tx_power requiring esp_wifi_start() [2]; NimBLE-Arduino crash report when disabling Wi‑Fi modem sleep before BLE init [4].
🌐 Web query:
arduino-esp32 WiFiClass setTxPower setSleep source code WiFi started requirement
💡 Result:
-
WiFi.setTxPower(...)(WiFiGenericClass::setTxPower) checks that WiFi is already started (either STA or AP). If neither is started, it warns and returnsfalse; otherwise it callsesp_wifi_set_max_tx_power(power). (github.com)- This matches the underlying ESP-IDF behavior:
esp_wifi_set_max_tx_power()fails ifesp_wifi_start()hasn’t been called (errorESP_ERR_WIFI_NOT_START). (github.com)
- This matches the underlying ESP-IDF behavior:
-
WiFi.setSleep(...)(WiFiGenericClass::setSleep) is STA-centric:setSleep(bool)just maps tosetSleep(enabled ? WIFI_PS_MIN_MODEM : WIFI_PS_NONE). (github.com)setSleep(wifi_ps_type_t)does not require WiFi to be started to “succeed”: if!WiFi.STA.started()it only stores the requested mode in_sleepEnabledand returnstrue(it does not callesp_wifi_set_psyet). (github.com)- Once STA is started, it applies the setting by calling
esp_wifi_set_ps(sleepType); if that call fails it returnsfalse. (github.com)
So, in arduino-esp32: Tx power requires WiFi started (STA or AP) to do anything, while sleep can be set before WiFi starts but won’t be applied to the modem until STA has started. (github.com)
Citations:
- 1: https://github.com/espressif/arduino-esp32/blob/master/libraries/WiFi/src/WiFiGeneric.cpp
- 2: Bug in esp_wifi_set_country and esp_wifi_set_max_tx_power effectively forbids usage of the ESP32-WROOM-32 module in many countries (IDFGH-2034) espressif/esp-idf#4223
- 3: https://github.com/espressif/arduino-esp32/blob/master/libraries/WiFi/src/WiFiGeneric.cpp
- 4: https://github.com/espressif/arduino-esp32/blob/master/libraries/WiFi/src/WiFiGeneric.cpp
- 5: https://github.com/espressif/arduino-esp32/blob/master/libraries/WiFi/src/WiFiGeneric.cpp
- 6: https://github.com/espressif/arduino-esp32/blob/master/libraries/WiFi/src/WiFiGeneric.cpp
Now let me look at the broader program flow in WLED — particularly cfg.cpp and network.cpp/wled.cpp — to understand the full sequencing picture.
🏁 Script executed:
#!/bin/bash
# Check where setTxPower and setSleep are called across the codebase
rg -n "setTxPower|setSleep|setPhyMode|wifi_set_sleep_type|initConnection|forceReconnect" --type=cpp -A2 -B2
# Also check wled.cpp for initConnection and connection sequence
rg -n "initConnection\|forceReconnect\|WiFi\.begin\|WiFi\.mode\|WIFI_STA\|esp_wifi_start" wled00/wled.cpp wled00/network.cpp -A3 -B1
Length of output: 7982
@softhack007 Here is a detailed analysis of the sequencing after consulting the arduino-esp32 v2.0.18 source and tracing the WLED program flow:
API-level requirements (arduino-esp32 v2.0.18)
WiFi.setTxPower() ultimately calls esp_wifi_set_max_tx_power(), which requires the Wi‑Fi driver to already be started. If called before start, it fails with ESP_ERR_WIFI_NOT_STARTED. The arduino-esp32 wrapper also guards this: it checks whether STA or AP is started and returns false silently if neither is active.
WiFi.setSleep() behaves differently: if STA is not yet started, it only stores the requested mode in _sleepEnabled without calling esp_wifi_set_ps. Once STA is started, it then applies the stored setting.
Three call sites — different correctness profiles
1. set.cpp handleSettingsSet() — correct ✅
The HTTP POST handler is only reachable when the device is serving web requests, which means WiFi (STA or AP) is already running. Both WiFi.setTxPower() and WiFi.setSleep() will succeed and take effect immediately on the live interface. This is the valid use case.
2. cfg.cpp lines 162–164 — silent failure for setTxPower ⚠️
// wled00/cfg.cpp ~162
WiFi.setSleep(!noWifiSleep);
WiFi.setHostname(hostName);
WiFi.setTxPower(wifi_power_t(txPower)); // ← called before WiFi is started at bootdeserializeConfig() runs during setup(), well before any WiFi.begin() or WiFi.softAP(). At that point:
setTxPower()will silently returnfalse— the ESP-IDF driver is not started, sotxPowerfrom the config is never applied on the initial STA connection path viainitConnection().setSleep()only stores the value internally and applies it once STA starts — so that part is ultimately fine.
3. Coverage gap after forceReconnect in STA-only mode ⚠️
handleSettingsSet correctly applies setTxPower immediately. However, several conditions in the same function set forceReconnect = true (e.g. SSID change, channel change), which causes initConnection() to be called in the next handleConnection() loop. Looking at initConnection() (line 619, wled.cpp), there is no WiFi.setTxPower() call there. initAP() does call it (line 594) after softAP(), providing coverage when AP is active — but for a pure STA reconnect (no AP), if the reconnect path involves a full esp_wifi_stop() + esp_wifi_start() cycle (e.g. mode change), the TX power setting would not be re-applied.
Summary table
| Call site | WiFi started? | setTxPower result |
setSleep result |
|---|---|---|---|
cfg.cpp startup |
❌ No | Silent no-op | Stored, applied later ✓ |
set.cpp handleSettingsSet |
✅ Yes (HTTP reachable) | Applied immediately ✓ | Applied immediately ✓ |
initAP() (existing) |
✅ Yes (after softAP) | Applied ✓ | Not called |
initConnection() |
✅ Yes | Not called |
Not called |
Bottom line: The placement in set.cpp is correct for the live-change scenario. The call in cfg.cpp at startup will silently fail for setTxPower since WiFi isn't started yet. The safer approach would be to also call WiFi.setTxPower() inside initConnection() after WiFi.begin(), so it is reliably applied both on initial boot and after any forced reconnect in STA-only mode.
🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/bus_manager.cpp:549-556
Timestamp: 2026-02-11T16:20:02.872Z
Learning: ESP-IDF V5 requires a complete rewrite of the LEDC (PWM/analog LED) subsystem in WLED. The framework now manages LEDC channels internally (unlike V4 where WLED managed them directly), and dithering functionality may not work as it did before. Direct LEDC struct access (e.g., `LEDC.channel_group[gr].channel[ch].duty.duty`) used in the current implementation will need to be replaced with proper HAL API calls. This affects `wled00/bus_manager.cpp` BusPwm class and related pin manager LEDC allocation code.
Learnt from: blazoncek
Repo: wled/WLED PR: 4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-02-11T17:17:04.657Z
Learning: In WLED code reviews, when C-style functions or variables are added or modified, check if they are globally visible (not static). If the function/variable is not declared in any global header file (like fcn_declare.h), suggest either: (1) making it static (visible only within the translation unit) with a forward declaration if needed, or (2) adding a comment to clarify that the function/variable is intentionally global. This helps reduce global namespace pollution and enables better compiler optimizations.
Learnt from: DedeHai
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-02-21T15:07:26.737Z
Learning: On ESP32 with ESP-IDF V4, there is an APLL (Audio PLL) resource conflict between Ethernet (when using internal RMII clock on GPIO 0/16/17) and I2S PDM microphones. In commit 98138a02e (Nov 2022, PR `#2907`), the audio reactive usermod was changed to set `use_apll = true` for PDM microphones in `usermods/audioreactive/audio_source.h` line 228. This causes Ethernet initialization to fail because both EMAC and I2S PDM cannot share the APLL simultaneously. This was not an issue in v0.15.3 because APLL was not enabled for PDM mode at that time. The fix is to disable APLL for PDM mode when Ethernet is enabled, either through runtime detection or a compile-time flag.
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, verify that file operations (especially file.open()) respect LittleFS filename limitations. Assume default WLED configuration with LittleFS default filename limit of 255 bytes. Do not assume extreme configuration values like WLED_MAX_SEGNAME_LEN = 512 which would not be standard configurations.
Learnt from: softhack007
Repo: wled/WLED PR: 5355
File: wled00/util.cpp:635-638
Timestamp: 2026-02-07T16:06:08.677Z
Learning: PSRAM-related compilation guards should enable PSRAM code only for ESP32 variants that actually include PSRAM: ESP32-C61, ESP32-C5, and ESP32-P4. Exclude ESP32-C3, ESP32-C6, and ESP8266 from these guards. Apply this rule across the codebase (not just wled00/util.cpp) by reviewing and updating PSRAM guards/macros in all relevant files (C/C++ headers and sources).
There was a problem hiding this comment.
@coderabbitai please extend your analysis to new call sites of WiFi.setHostname(). Is the hostname set in the correct sequence to always ensure that the default esp-xxxxxx hostname will not be used?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all WiFi.setHostname and WiFi.hostname calls with context
rg -n "WiFi\.setHostname|WiFi\.hostname|setHostname" --type=cpp -A3 -B3
# Check initConnection sequence in wled.cpp
rg -n "initConnection\|WiFi\.begin\|WiFi\.mode\|setHostname\|hostName\|prepareHostname" wled00/wled.cpp -A2 -B1
# Check network.cpp for hostname and WiFi.begin
rg -n "WiFi\.begin\|setHostname\|hostName\|initConnection\|WiFi\.mode" wled00/network.cpp -A2 -B1
# Check cfg.cpp for hostname setting sequence
rg -n "setHostname\|hostName\|WiFi\.begin\|deserializeConfig\|WiFi\.mode" wled00/cfg.cpp -A2 -B1
Length of output: 1931
🏁 Script executed:
#!/bin/bash
# Get initConnection and related WiFi setup sequence
rg -n "initConnection|WiFi\.begin|WiFi\.mode|WiFi\.disconnect|setHostname|hostName|prepareHostname|forceReconnect" wled00/wled.cpp -A3 -B2
# Get initConnection from network.cpp if defined there
rg -n "initConnection|WiFi\.begin|WiFi\.mode|WiFi\.disconnect|setHostname|hostName" wled00/network.cpp -A3 -B2
# Also check the setup() sequence to understand order: deserializeConfig vs initConnection
rg -n "deserializeConfig\|initConnection\|setup\b" wled00/wled.cpp -A1 -B1
Length of output: 3928
@softhack007 Here is the extended analysis covering all new WiFi.setHostname() call sites:
All WiFi.setHostname() / WiFi.hostname() call sites in this PR
| Location | Condition | WiFi state at call time |
|---|---|---|
cfg.cpp line 163 |
deserializeConfig() at boot |
Before WiFi.mode() ❌ |
set.cpp line 72 |
User saves WiFi settings (HTTP) | WiFi already running ✅ |
network.cpp line 417 |
ARDUINO_EVENT_ETH_CONNECTED event |
After ETH connected ✅ |
The critical gap: initConnection() has no setHostname()
The old code set the hostname inside initConnection(), between WiFi.mode() and WiFi.begin() — the required sequence for arduino-esp32. This PR removed that call entirely.
initConnection() (wled.cpp line 619) now looks like:
WiFi.disconnect(true); // line 634
...
WiFi.mode(WIFI_STA); // line 659 ← interface (re)initialized
...
WiFi.begin(ssid, pass); // line 666 ← DHCP discover sent — hostname already defaulted to esp-XXXXXX
// ← WiFi.setHostname() is GONE from here
Issue 1 — Boot path: cfg.cpp line 163 sets hostname too early
deserializeConfig() runs in setup() before WiFi.mode(WIFI_STA) is called at wled.cpp line 463. On arduino-esp32 v2.x, WiFi.setHostname() called before WiFi.mode() has no STA interface to write to. When WiFi.mode(WIFI_STA) is subsequently called, the underlying esp_netif is (re)initialized with the SDK default (esp-XXXXXX), wiping the value set earlier.
Issue 2 — Every reconnect: initConnection() never re-asserts the hostname
initConnection() is called on initial boot connection, on forceReconnect, and on disconnect recovery (wled.cpp lines 761, 784, 799, 813). Each time, WiFi.mode() may re-initialize the STA interface, but no WiFi.setHostname() follows it before WiFi.begin(). The DHCP request goes out with esp-XXXXXX.
Issue 3 — set.cpp live change is correct but insufficient
set.cpp line 72 correctly updates the hostname while WiFi is live. However, it also sets forceReconnect = true (e.g. on SSID change), which triggers initConnection() — where the hostname is no longer re-set. So even a live change that forces reconnect may result in esp-XXXXXX being advertised to the DHCP server after the reconnect.
Recommended fix
WiFi.setHostname() needs to be re-added to initConnection(), placed after WiFi.disconnect(true) but before WiFi.begin():
// inside initConnection(), after WiFi.disconnect(true) and before WiFi.begin()
`#ifdef` ARDUINO_ARCH_ESP32
WiFi.setHostname(hostName);
`#else`
WiFi.hostname(hostName);
`#endif`The call in cfg.cpp line 163 can remain for ESP8266 (where WiFi.hostname() is stored and applied at begin() time and is more forgiving about ordering), but on ESP32 it is ineffective before WiFi.mode() and should not be the sole call site.
✏️ Learnings added
Learnt from: softhack007
Repo: wled/WLED PR: 4893
File: wled00/set.cpp:95-95
Timestamp: 2026-03-14T20:56:46.543Z
Learning: In WLED (arduino-esp32), WiFi.setHostname() must be called AFTER WiFi.mode() but BEFORE WiFi.begin() to prevent the default `esp-XXXXXX` hostname from being used in DHCP requests. Calling it before WiFi.mode() (e.g. in deserializeConfig() at boot) has no effect because the STA interface does not yet exist. The correct place is inside initConnection() in wled00/wled.cpp, after WiFi.disconnect(true) and before WiFi.begin(). On ESP8266, WiFi.hostname() is more forgiving about ordering but should still be placed in initConnection() for consistency.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, verify that file operations (especially file.open()) respect LittleFS filename limitations. Assume default WLED configuration with LittleFS default filename limit of 255 bytes. Do not assume extreme configuration values like WLED_MAX_SEGNAME_LEN = 512 which would not be standard configurations.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, file operations (especially file.open()) should be checked to ensure they respect LittleFS filename limitations. The default LittleFS filename limit is 255 bytes (LFS_NAME_MAX). Reviews should assume default WLED configuration defines and not extreme edge-case values (e.g., WLED_MAX_SEGNAME_LEN = 512 would not be standard). File paths should be validated to stay within the 255-byte limit.
Learnt from: blazoncek
Repo: wled/WLED PR: 4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/rest_json_client.cpp:40-46
Timestamp: 2025-08-29T00:22:34.115Z
Learning: For ESP32/ESP8266 HTTP requests in WLED, set timeouts on the underlying WiFiClient (using client.setTimeout()) before calling http_.begin(), rather than using HTTPClient::setTimeout(). This pattern is used successfully in the Klipper usermod to prevent connection hangs.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-08T00:57:36.134Z
Learning: In WLED (wled00/cfg.cpp), `deserializeConfig()` is called with an empty JsonObject when cfg.json doesn't exist on fresh install. Any value read without the ArduinoJSON `|` fallback operator or CJSON macro will override correct constructor defaults with 0/null. Known affected values: `strip.setTargetFps(hw_led["fps"])` at line 179 (sets FPS to 0/unlimited instead of WLED_FPS=42). Fix: `strip.setTargetFps(hw_led["fps"] | WLED_FPS)`. The gamma issue (gammaCorrectCol/Bri) had the same root cause and was fixed in commit d1d9dec402 (Jan 2026) using inline `| default` fallbacks.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-08T00:57:36.134Z
Learning: In WLED (wled00/cfg.cpp), `deserializeConfigFromFS()` only sets `needsSave=true` (triggering a write of cfg.json) when usermod settings are present in the JSON (lines 758-761). On a fresh install with no usermods, `needsSave=false` so `serializeConfigToFS()` is never called at first boot. Correct defaults are never written to cfg.json until the user manually saves. Structural fix: set `needsSave = needsSave || !cfgExists` where `cfgExists = WLED_FS.exists(s_cfg_json)` checked before reading the file.
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR `#4798`, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-02-11T17:17:04.657Z
Learning: In WLED code reviews, when C-style functions or variables are added or modified, check if they are globally visible (not static). If the function/variable is not declared in any global header file (like fcn_declare.h), suggest either: (1) making it static (visible only within the translation unit) with a forward declaration if needed, or (2) adding a comment to clarify that the function/variable is intentionally global. This helps reduce global namespace pollution and enables better compiler optimizations.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/bus_manager.cpp:549-556
Timestamp: 2026-02-11T16:20:02.872Z
Learning: ESP-IDF V5 requires a complete rewrite of the LEDC (PWM/analog LED) subsystem in WLED. The framework now manages LEDC channels internally (unlike V4 where WLED managed them directly), and dithering functionality may not work as it did before. Direct LEDC struct access (e.g., `LEDC.channel_group[gr].channel[ch].duty.duty`) used in the current implementation will need to be replaced with proper HAL API calls. This affects `wled00/bus_manager.cpp` BusPwm class and related pin manager LEDC allocation code.
Learnt from: DedeHai
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-02-21T15:07:26.737Z
Learning: On ESP32 with ESP-IDF V4, there is an APLL (Audio PLL) resource conflict between Ethernet (when using internal RMII clock on GPIO 0/16/17) and I2S PDM microphones. In commit 98138a02e (Nov 2022, PR `#2907`), the audio reactive usermod was changed to set `use_apll = true` for PDM microphones in `usermods/audioreactive/audio_source.h` line 228. This causes Ethernet initialization to fail because both EMAC and I2S PDM cannot share the APLL simultaneously. This was not an issue in v0.15.3 because APLL was not enabled for PDM mode at that time. The fix is to disable APLL for PDM mode when Ethernet is enabled, either through runtime detection or a compile-time flag.
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Learnt from: softhack007
Repo: wled/WLED PR: 5355
File: wled00/util.cpp:635-638
Timestamp: 2026-02-07T16:06:08.677Z
Learning: PSRAM-related compilation guards should enable PSRAM code only for ESP32 variants that actually include PSRAM: ESP32-C61, ESP32-C5, and ESP32-P4. Exclude ESP32-C3, ESP32-C6, and ESP8266 from these guards. Apply this rule across the codebase (not just wled00/util.cpp) by reviewing and updating PSRAM guards/macros in all relevant files (C/C++ headers and sources).
There was a problem hiding this comment.
The overall logic of the PR - to bring all kinds of "hostnames" (Alexa, MQTT, mDNS, webUI name, etc) into one looks good to me - however oi did not inspect all the details. Maybe the Alexa name should still stay separate, not sure about the MQTT name either.
The new call sites of WiFi methods need further analysis and clarification - especially for esp32 (all variants). The esp32 framework seems to be less "forgiving", and a silently failing WiFi function call may lead to regression / unexpected behaviours that will be hard to track down once this PR got merged.
See flow analysis of the rabbit
Edit: a clearer summary of WiFi.setHostName() usage constrains on esp32 is in #5424 (comment)
|
@coderabbitai please create a summary of user-visible changes from this PR, including an overview of previously separate "host names" that are unified by this PR. |
|
Tip For best results, initiate chat on the files or code changes.
PR
|
| Old variable | Role | Where it was used |
|---|---|---|
cmDNS[33] |
mDNS/Bonjour advertised name (e.g. wled-abc123.local) |
mDNS, OTA, captive portal check |
serverDescription |
Human-readable device name in the UI/INFO panel | UI display, Improv |
| WiFi DHCP hostname | Sent to DHCP server during connection | Derived ad-hoc in initConnection() from serverDescription each reconnect |
| Ethernet hostname | Set when ETH connected | Re-derived per-connection via prepareHostname() from serverDescription |
| OTA hostname | Used for OTA discovery | Was set from cmDNS |
| MQTT client ID / device topic | MQTT identification | Had no automatic default — could be empty |
These were configured via two separate UI fields (Server description and mDNS name) with unclear relationship, and the WiFi DHCP hostname was never directly exposed or reliably applied (see also the ESP32 WiFi.setHostname() ordering subtlety noted in our learnings).
✅ What Is Unified Now
All hostname-related identity is now driven by a single hostName[33] variable:
hostNameis generated once at boot byprepareHostname(), using the new "Device name" (serverDescription) field as input — converting it to a valid DNS label (alphanumeric + hyphens, no leading/trailing hyphens).- If no device name is set, it falls back to a MAC-based default (
wled-XXXXXX). - A separate mDNS toggle (
mDNSenabled) now independently controls whether the.localmDNS advertisement is active, without affecting the hostname itself. hostNameis now used consistently for: mDNS, WiFi DHCP hostname, Ethernet hostname, OTA hostname, and the captive portal host-header check.- MQTT client ID now auto-defaults to
WLED-<MAC>and MQTT device topic towled/<MAC>if left empty.
🖥️ UI / Settings Changes
Main settings page — button labels renamed:
- "WiFi Setup" → "Network Setup"
- "LED Preferences" → "Hardware Setup"
- "Time & Macros" → "Time & Scheduler"
Network Setup page (formerly WiFi Setup):
- Page title changed to "Network Settings"
- New Hostname field (CM) — shows the computed hostname, with a live URL preview (e.g.
http://wled-abc123.local/) - New mDNS checkbox (MD) — enables/disables
.localmDNS advertisement independently - Client IP indicator added
- AP section restructured with stricter input validation (empty SSID = no AP)
- Ethernet type selector with multiple vendor options added
- ESP-NOW section reorganized; a warning is shown that ESP-NOW is incompatible with Ethernet
- ESP-NOW remote MAC management UI added
UI / General Settings page:
- "Server description" field renamed to "Device name" (now requires at least 2 characters)
- Sync toggle (ST) control removed from the UI settings page
Config JSON schema change (relevant for anyone parsing cfg.json directly):
id.mdnsandid.namefields removed; replaced bynw.name(hostname) andnw.mdns(mDNS enabled flag)- Legacy
id.mdnsis still read for backward compatibility on import
Summary
In short: the PR gives the device a single, consistent identity — you set a "Device name", and that name is used everywhere (mDNS, DHCP, OTA, Ethernet) in a sanitized form. The Network Setup page is the new home for hostname and mDNS configuration, with a live preview so the resulting .local URL is immediately visible.
🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 4893
File: wled00/set.cpp:95-95
Timestamp: 2026-03-14T20:56:46.543Z
Learning: In WLED (arduino-esp32), WiFi.setHostname() must be called AFTER WiFi.mode() but BEFORE WiFi.begin() to prevent the default `esp-XXXXXX` hostname from being used in DHCP requests. Calling it before WiFi.mode() (e.g. in deserializeConfig() at boot) has no effect because the STA interface does not yet exist. The correct place is inside initConnection() in wled00/wled.cpp, after WiFi.disconnect(true) and before WiFi.begin(). On ESP8266, WiFi.hostname() is more forgiving about ordering but should still be placed in initConnection() for consistency.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-14T21:24:02.836Z
Learning: In WLED (arduino-esp32 v2.x), WiFi.setHostname() must be called BEFORE WiFi.begin() and, crucially, BEFORE WiFi.mode(WIFI_STA). The hostname stored by setHostname() is only physically applied to the netif during a WiFi mode *transition* to WIFI_STA inside WiFiGenericClass::mode(). If the device is already in WIFI_STA mode (e.g., on a reconnect), calling WiFi.mode(WIFI_STA) is a no-op (mode == current mode), so the hostname will never be applied and the default ESP name will be used in DHCP. The correct pattern is: WiFi.disconnect(true) → WiFi.mode(WIFI_MODE_NULL) → WiFi.setHostname(hostname) → WiFi.mode(WIFI_STA) → WiFi.begin(). The WiFi.mode(WIFI_MODE_NULL) step is essential on reconnects to force a genuine mode transition.
4 participants
Hi, as promised, the cleaner version. See #4751 and #4888.
One thing: there's still one
cmDNSreference inwled00/wled_eeprom.cpp:83:29:. This did not break compilation for me, probably due toWLED_ADD_EEPROM_SUPPORTnot being defined. I'm not sure what the correct fix would be.Summary by CodeRabbit