Hostname unification #4751
Hostname unification #4751
Conversation
WalkthroughThe changes unify and improve hostname and mDNS handling across the codebase, replacing legacy fixed-size buffers with safer Changes
Estimated code review effort3 (30–60 minutes) Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (14)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
🧰 Additional context used🧠 Learnings (8)📓 Common learningswled00/data/settings_ui.htm (2)Learnt from: blazoncek Learnt from: blazoncek
wled00/wled_server.cpp (4)Learnt from: blazoncek Learnt from: blazoncek Learnt from: KrX3D Learnt from: KrX3D wled00/xml.cpp (2)Learnt from: blazoncek Learnt from: blazoncek wled00/improv.cpp (2)Learnt from: blazoncek
Learnt from: blazoncek wled00/wled.cpp (7)Learnt from: blazoncek Learnt from: blazoncek Learnt from: KrX3D Learnt from: blazoncek
Learnt from: KrX3D Learnt from: blazoncek
Learnt from: blazoncek wled00/data/settings_wifi.htm (1)Learnt from: blazoncek wled00/wled.h (7)Learnt from: KrX3D Learnt from: blazoncek
Learnt from: blazoncek Learnt from: KrX3D Learnt from: blazoncek Learnt from: blazoncek Learnt from: KrX3D 🔇 Additional comments (20)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
wled00/network.cpp
Outdated
| char hostname[64]; | ||
| prepareHostname(hostname); | ||
| char hostname[33]; |
There was a problem hiding this comment.
Why the change from 64 to 33 and this seems out of step with other parts of the code where you are using 25
There was a problem hiding this comment.
None of the UI fields are 64 bytes.
|
With this change, can we still have no mDNS but still the user-defined hostname? (used in DHCP request for "regular" DNS) |
Yes, the fallback is UI device name. |
|
Yeah that doesn't quite make sense then. While I accept that not putting anything in there will maintain existing behaviour, I thought that part of the reason for this change is that current behaviour is unclear. Most users would expect the network hostname to be with the network settings, not automatically generated from a setting under UI. So this change to move hostname to be consistent for regular DNS and mDNS is an improvement, but it would be sad if you couldn't see your own regular hostname without being forced to enable mDNS as the only way to disable is to remove the hostname |
|
Then the only option is to add a checkmark for enabling mDNS. |
|
@netmindz @willmmiles @DedeHai @softhack007 @Aircoookie I would like we all agree on the proposed change since it may disrupt some people's network configuration and we should be aware of this change. As a pro for this change it fixes wrong/missing hostname on change or boot. |
|
this is above my networking know-how. I exclusively work with IPs. |
It is not really technical. This PR switches the hostname generation to mDNS name instead of UI name (by default). UI name is used as a fallback when user does not want to use mDNS and empties mDNS name. In current situation UI name is WLED by default and if user does not change that there may be several devices with same hostname. Default mDNS name is a combination of "wled-" and MAC address. |
|
Ah I get it, still don't really understand the background, did not have time yet to read the issues solved with this. Can't really comment on "disrupting network". I do not use names, I assign fixed IPs. All I want to add: it must not be convoluted, like input fields need to be clear. If I change mDNS name, it should not change the host name and vice versa (if that is how it currently is). If any naming settings are linked, they MUST be in the same settings page and synced directly in that page so its obvious. edit: |
|
I'm pulling this to draft as perhaps we need to discuss the following:
IMO hostname and mDNS name should be identical (not necessarily the case with this PR) however device name should not be used in the networking context (it is with this PR if user erases mDNS). I have modified the entire name handling and introduced explicit mDNS selection as a POC in my fork (unpublished ATM). |
…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
| <div id="ESPNOW"> | ||
| Enable ESP-NOW: <input type="checkbox" name="RE" onchange="tE()"><br> | ||
| <i>Listen for events over ESP-NOW<br> | ||
| Keep disabled if not using a remote or wireless sync, increases power consumption.<br></i> |
There was a problem hiding this comment.
name it ESPNow-Sync instead of wireless sync?
|
@willmmiles does the latest push fulfill what we talked about (single hostname across all networking options)? It does not change UI name, though, which remains WLED by default but can be overridden during compile. |
blazoncek
added
the
connectivity
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Style