Track cmDNS-default state to restore Device-Name-based hostname#5565
Track cmDNS-default state to restore Device-Name-based hostname#5565
Conversation
PR wled#5424 made getWLEDhostname() prefer cmDNS over the legacy serverDescription-based hostname, but cmDNS is still overwritten at boot with "wled-<mac6>" when it holds the "x" default sentinel. The new hostname check (strcmp_P against DEFAULT_MDNS_NAME) sees the auto-filled value and treats it as user-configured, so users who never customized the mDNS field — and previously got "wled-<DeviceName>" as their network hostname — now get "wled-<mac6>" instead. Track whether the auto-fill was applied via a new cmDNSisDefault flag, and use that in getWLEDhostname() instead of inspecting cmDNS contents. The flag is set at boot from the comparison against DEFAULT_MDNS_NAME and cleared when the user saves the WiFi settings form. All other consumers (MDNS.begin, ArduinoOTA.setHostname, captive portal, MQTT, bus_manager, improv) keep using the auto-filled cmDNS unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
WalkthroughA new global boolean flag ChangesmDNS Default-State Tracking
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/wled.cpp (1)
513-515:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
cmDNSisDefaultis still at its global initial value (true) whengetWLEDhostname()is called here — regression for custom-mDNS users.
cmDNSisDefaultis not computed until lines 537–538, which come after this ESP32 hostname block. At line 514,!cmDNSisDefaultis alwaysfalse(initial value), sogetWLEDhostname()unconditionally falls through toprepareHostname()(Device-Name path) regardless of whether the user has explicitly set a custom mDNS name.Before this PR, the same call used
strcmp_P(cmDNS, DEFAULT_MDNS_NAME)and correctly detected custom values. This is a regression: on ESP32 withWiFi.persistent(true), the auto-reconnect triggered byWiFi.mode(WIFI_STA)at line 519 may register the wrong DHCP hostname beforeinitConnection()corrects it.Both prerequisites for the computation —
escapedMac(line 471) andcmDNSloaded from config (line 484) — are available before this block, so lines 535–538 can be moved up.🐛 Proposed fix — move the flag initialisation before the ESP32 hostname block
+ // fill in unique mdns default; remember whether we did so the hostname + // logic can distinguish auto-filled vs. user-set values + cmDNSisDefault = (strcmp(cmDNS, DEFAULT_MDNS_NAME) == 0); + if (cmDNSisDefault) sprintf_P(cmDNS, PSTR("wled-%*s"), 6, escapedMac.c_str() + 6); + `#ifndef` ESP8266 WiFi.setScanMethod(WIFI_ALL_CHANNEL_SCAN); WiFi.persistent(true); // ... char hostname[64] = {'\0'}; getWLEDhostname(hostname, sizeof(hostname), true); WiFi.setHostname(hostname); `#else` WiFi.persistent(false); `#endif` ... - // fill in unique mdns default; remember whether we did so the hostname - // logic can distinguish auto-filled vs. user-set values - cmDNSisDefault = (strcmp(cmDNS, DEFAULT_MDNS_NAME) == 0); - if (cmDNSisDefault) sprintf_P(cmDNS, PSTR("wled-%*s"), 6, escapedMac.c_str() + 6);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/wled.cpp` around lines 513 - 515, cmDNSisDefault is still true when getWLEDhostname(hostname, ...) is called, causing custom mDNS names to be ignored; move the computation that sets cmDNSisDefault (which relies on escapedMac and cmDNS already available) to before the ESP32 hostname block so getWLEDhostname sees the correct flag. Specifically, compute/update cmDNSisDefault (using cmDNS and DEFAULT_MDNS_NAME comparison) before calling getWLEDhostname/ WiFi.setHostname, ensuring prepareHostname is only used when appropriate and preventing WiFi.mode(WIFI_STA) from registering the wrong DHCP hostname prior to initConnection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/wled.cpp`:
- Around line 535-539: The config serialization currently writes the runtime
cmDNS directly (id[F("mdns")] = cmDNS;) which will persist auto-filled values
and break cmDNSisDefault detection on reboot; update the serialization in
cfg.cpp so that when cmDNSisDefault is true it writes DEFAULT_MDNS_NAME (the
sentinel) instead of the generated cmDNS, or alternatively write an explicit
cmDNSisDefault boolean flag into the config alongside mdns so the loader can
restore the flag; reference cmDNS, cmDNSisDefault, DEFAULT_MDNS_NAME and the
mdns serialization site (id[F("mdns")]) when making the change.
---
Outside diff comments:
In `@wled00/wled.cpp`:
- Around line 513-515: cmDNSisDefault is still true when
getWLEDhostname(hostname, ...) is called, causing custom mDNS names to be
ignored; move the computation that sets cmDNSisDefault (which relies on
escapedMac and cmDNS already available) to before the ESP32 hostname block so
getWLEDhostname sees the correct flag. Specifically, compute/update
cmDNSisDefault (using cmDNS and DEFAULT_MDNS_NAME comparison) before calling
getWLEDhostname/ WiFi.setHostname, ensuring prepareHostname is only used when
appropriate and preventing WiFi.mode(WIFI_STA) from registering the wrong DHCP
hostname prior to initConnection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 94f6f2cc-45ba-4eba-b906-b6df9fc15805
📒 Files selected for processing (4)
wled00/set.cppwled00/util.cppwled00/wled.cppwled00/wled.h
| // fill in unique mdns default; remember whether we did so the hostname | ||
| // logic can distinguish auto-filled vs. user-set values | ||
| cmDNSisDefault = (strcmp(cmDNS, DEFAULT_MDNS_NAME) == 0); | ||
| if (cmDNSisDefault) sprintf_P(cmDNS, PSTR("wled-%*s"), 6, escapedMac.c_str() + 6); | ||
| #ifndef WLED_DISABLE_MQTT |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how cmDNS is serialised in cfg.cpp (write path)
rg -n "cmDNS\|mdns" wled00/cfg.cppRepository: wled/WLED
Length of output: 1863
🏁 Script executed:
#!/bin/bash
# Verify how cmDNS is serialised in cfg.cpp (write path)
rg -n "cmDNS\|mdns" wled00/cfg.cppRepository: wled/WLED
Length of output: 35
🏁 Script executed:
#!/bin/bash
# Find files related to config serialization
find . -type f -name "*cfg*" -o -name "*config*" | grep -E "\.(cpp|h|hpp)$" | head -20Repository: wled/WLED
Length of output: 98
🏁 Script executed:
#!/bin/bash
# Search for cmDNS in cfg.cpp with context
rg -A5 -B5 "cmDNS" wled00/cfg.cppRepository: wled/WLED
Length of output: 842
The serialization path in cfg.cpp does not guard against persisting auto-filled cmDNS values, defeating this fix on reboot.
The serialization code at id[F("mdns")] = cmDNS; writes the current cmDNS value directly to config without checking cmDNSisDefault. After the auto-fill on line 538 (cmDNS = "wled-<mac>" with cmDNSisDefault = true), the next serializeConfigToFS() call persists the runtime-generated value to cfg.json. On subsequent boots, loading cmDNS = "wled-<mac>" from config causes strcmp(cmDNS, DEFAULT_MDNS_NAME) to evaluate false, making cmDNSisDefault = false and breaking hostname selection logic.
The fix requires either:
- Modifying the serialization in
cfg.cppto persist the sentinel value (DEFAULT_MDNS_NAME) whencmDNSisDefaultis true, or - Persisting the
cmDNSisDefaultflag itself to recover the intended behavior on reboot.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/wled.cpp` around lines 535 - 539, The config serialization currently
writes the runtime cmDNS directly (id[F("mdns")] = cmDNS;) which will persist
auto-filled values and break cmDNSisDefault detection on reboot; update the
serialization in cfg.cpp so that when cmDNSisDefault is true it writes
DEFAULT_MDNS_NAME (the sentinel) instead of the generated cmDNS, or
alternatively write an explicit cmDNSisDefault boolean flag into the config
alongside mdns so the loader can restore the flag; reference cmDNS,
cmDNSisDefault, DEFAULT_MDNS_NAME and the mdns serialization site
(id[F("mdns")]) when making the change.
|
Closing - this PR tries to improve backwards compatibility; however it also adds new problems:
While the backwards compatibility problem should receive further investigation, I think this PR does not provide a future-proof solution, sorry. |
2 participants
PR #5424 made
getWLEDhostname()prefercmDNSover the legacyserverDescription-based hostname, butcmDNSis still overwritten at boot with "wled-" when it holds the "x" default sentinel. The new hostname check (strcmp_P againstDEFAULT_MDNS_NAME) sees the auto-filled value and treats it as user-configured, so users who never customized the mDNS field — and previously gotwled-<DeviceName>as their network hostname — now getwled-<mac6>instead.Track whether the auto-fill was applied via a new cmDNSisDefault flag, and use that in
getWLEDhostname(). The flag is set at boot from the comparison againstDEFAULT_MDNS_NAMEand cleared when the user saves the WiFi settings form. All other consumers (MDNS.begin, ArduinoOTA.setHostname, captive portal, MQTT, bus_manager, improv) keep using the auto-filledcmDNSunchanged.Fixes: #5563
Summary by CodeRabbit
Improved internal tracking of mDNS hostname configuration to better distinguish between auto-generated and user-configured values, enabling more accurate settings management going forward.