Hostname creation cleanup, and a minimal variant or PR#5397 to fix bad DNS name#5424
Hostname creation cleanup, and a minimal variant or PR#5397 to fix bad DNS name#5424softhack007 wants to merge 16 commits intomainfrom
Conversation
Add hostname configuration for WiFi connection based on cmDNS.
* refactor legacy prepareHostname() logic, and split hostname sanitization from hostname creation. * always use the max possible length allowed for a FQDN hostname
* hostname by cmDNS is now included in getWLEDhostname() * ensure that the name is sanitized * avoid using a potentially undefined macro
WalkthroughReplaces legacy hostname preparation with a length-aware API Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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)
📝 Coding Plan
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 |
This comment was marked as resolved.
This comment was marked as resolved.
robustness improvement.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
wled00/util.cpp (1)
157-157: Use named constants for hostname prefix/min-size values.The repeated
5/6values encode protocol semantics (prefix length and minimum useful buffer). Pulling them into constants will keep both helpers aligned.Based on learnings: 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.
Also applies to: 177-183
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/util.cpp` at line 157, Replace the magic numbers used for the hostname prefix and minimum buffer size with named constants so the logic in util.cpp is self-documenting and consistent across helpers: introduce a HOSTNAME_PREFIX (e.g., "wled"), a HOSTNAME_PREFIX_LEN constant (the prefix length), and a HOSTNAME_MIN_LEN constant (minimum useful buffer, currently 6) and use those instead of literal 5/6 in the conditional and the strlcpy call in the hostname-generation function and the related code around the block referenced (lines 177-183) so both helpers reference the same symbolic values.
🤖 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/util.cpp`:
- Line 157: The small-buffer guard in prepareHostname is too permissive: change
the checks that read "if (maxLen < 6)" to "if (maxLen <= 6)" (both occurrences
referenced in the diff around prepareHostname) so that buffers of size 6 are
treated as too small and the function returns early via strlcpy(hostname,
"wled", maxLen); to avoid producing the invalid "wled-" label for maxLen == 6.
---
Nitpick comments:
In `@wled00/util.cpp`:
- Line 157: Replace the magic numbers used for the hostname prefix and minimum
buffer size with named constants so the logic in util.cpp is self-documenting
and consistent across helpers: introduce a HOSTNAME_PREFIX (e.g., "wled"), a
HOSTNAME_PREFIX_LEN constant (the prefix length), and a HOSTNAME_MIN_LEN
constant (minimum useful buffer, currently 6) and use those instead of literal
5/6 in the conditional and the strlcpy call in the hostname-generation function
and the related code around the block referenced (lines 177-183) so both helpers
reference the same symbolic values.
|
@coderabbitai please verify if the new implementation of
|
This comment was marked as duplicate.
This comment was marked as duplicate.
|
Tip For best results, initiate chat on the files or code changes.
All expected improvements are correctly implemented. The implementation is sound and handles all listed edge cases deterministically. 🧠 Learnings used |
|
@coderabbitai there seem to be some "implicit" constraints on where and when the WiFi (and ETH) "setHostname()" method must be called on esp32 boards, in order to avoid that the default DNS name ESP-XXXXXX gets used.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll look at the arduino-esp32 v2.0.18 source code for 🏁 Script executed: Length of output: 11773 Now let me search for the arduino-esp32 v2.0.18 source to understand the
Part 1 — Constraints from the arduino-esp32 v2.x implementationThe fundamental ruleThe official arduino-esp32 documentation states that Why it silently fails in v2.x — the "stored but not applied" trap
The hostname is only physically applied at one location in To work around this, the mode must actually change — e.g., transition through Summary of constraints
Part 2 — Assessment of the PR's call sequencesIssue 1 🔴 —
|
|
I'm changing the PR back to draft, and try the suggestion by the rabbit #5424 (comment) . |
|
Thanks for picking this up @softhack007 |
* follow the rabbit's suggestion to set the hostname before WiFi.mode(STA) * WiFi.mode(WIFI_MODE_NULL) after WiFi.disconnect(), to ensure that all changes are applied on wifi re-start
|
works for me :-) @gsieben please test if this PR solves your problem with |
by checking if the serverDescription also start with "wled".
DedeHai
left a comment
There was a problem hiding this comment.
I still do not use any dns so can't test but the code looks solid, I guess the rabbit checked for off-by-1 errors? those are hard to spot, I checked and found none.
saves 5 bytes of RAM on 8266
Names like WLED0 or WLEDa survive sanitization, but strlen(hostname) == 5 still sends them down the wled-<MAC> fallback path.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
wled00/util.cpp (1)
162-162: Consider extracting duplicate fallback hostname generation.The wled-MAC fallback pattern appears identically at lines 162 and 187. A small helper or inline lambda could reduce duplication.
💡 Optional: Extract to static helper
// Near top of hostname section static void setWledMacFallback(char* hostname, size_t maxLen) { snprintf_P(hostname, maxLen, PSTR("wled-%*s"), 6, escapedMac.c_str() + 6); hostname[maxLen - 1] = '\0'; }Then replace both occurrences with
setWledMacFallback(hostname, maxLen);Also applies to: 187-187
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/util.cpp` at line 162, Duplicate fallback hostname generation using snprintf_P("wled-%*s", 6, escapedMac.c_str() + 6) occurs twice; extract that logic into a small static helper (or inline lambda) such as setWledMacFallback(char* hostname, size_t maxLen) and call it from both places instead of repeating the snprintf_P call, making sure the helper null-terminates hostname (hostname[maxLen-1] = '\0') and uses the same escapedMac reference so behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@wled00/util.cpp`:
- Line 162: Duplicate fallback hostname generation using snprintf_P("wled-%*s",
6, escapedMac.c_str() + 6) occurs twice; extract that logic into a small static
helper (or inline lambda) such as setWledMacFallback(char* hostname, size_t
maxLen) and call it from both places instead of repeating the snprintf_P call,
making sure the helper null-terminates hostname (hostname[maxLen-1] = '\0') and
uses the same escapedMac reference so behavior is unchanged.
|
question: ethernet DNS is still using the legacy hostname, while wifi now prefers the mDNS name. Should we keep that, or align the ethernet path to also prefer the mDNS name?
|
|
Testing: I found out that my wifi router (German telecom "speedport4") allows client DNS name registration:-) so I can test that aspect, and it works. It's the first time that my WLED devices appear with their proper name in my router's network overview 👍 |
avoid "x" = not set (default MDNS)
|
ToDO for a future PR: the mDNS startup code still uses the not-sanitized host name, which can include unicode smileys, invalid characters etc --> will not fix in this PR, to preserve legacy behaviour. |
important: this is not a replacement for #4893. Its a fix that tries to keep the impact on existing functionalities minimal - cleaning up the implicit logic from prepareHostname(), and extracting a generic hostname sanitizer function. Its a potential fix for the 0.15_x branch, too.
@gsieben I hope you don't mind that I took the initiative to evolve your PR. It would be nice if you can test if this PR solves your problem, too.
Summary by CodeRabbit
Bug Fixes
New Features