Skip to content

Add mDNS support for MQTT server #4769

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
blazoncek merged 4 commits into from
Aug 2, 2025
Merged

Add mDNS support for MQTT server #4769

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
79b3bc2
Add mDNS support for MQTT server
blazoncek Jul 12, 2025
24f2306
Strip .local from mDNS resolution
blazoncek Jul 15, 2025
07e303b
Remove anything behind .local and ignore case
blazoncek Jul 15, 2025
ecc3eae
Revert status message change
blazoncek Jul 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions wled00/mqtt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,8 +214,10 @@ bool initMqtt()
mqtt->setServer(mqttIP, mqttPort);
} else {
#ifdef ARDUINO_ARCH_ESP32
if (strlen(cmDNS) > 0 && strchr(mqttServer, '.') == nullptr) { // if mDNS is enabled and server does not have domain
mqttIP = MDNS.queryHost(mqttServer);
String mqttMDNS = mqttServer;
mqttMDNS.replace(F(".local"), ""); // remove .local if present
if (strlen(cmDNS) > 0 && mqttMDNS.length() > 0 && mqttMDNS.indexOf('.') < 0) { // if mDNS is enabled and server does not have domain
Copy link

@schildbach schildbach Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is .replace() limited to the end of strings? Otherwise it would strip in the middle of DNS names too which is probably not wanted.

In general, I'd recommend explicitly checking for .local at the end, and only if present resolve via mDNS (stripping the domain if it isn't done by the mDNS component anyway). It seems the above code just checks for the presence of dots, which is imho not correct. Plain hostnames (without dots) are typically expected to be resolved via DNS (using the search domain).

Copy link

@schildbach schildbach Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the impl from this PR would fail for those who use dots in their mDNS name. Yes, some people do this and I understand this is bending the mDNS spec a bit, but it would be nice to support these cases anyway.

Copy link

@schildbach schildbach Jul 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more comment about .replace(): is it case-insensitive? Both DNS and mDNS names/domains are case-insensitive.

Copy link
Contributor Author

@blazoncek blazoncek Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are welcome to add on to this at a later time. For now just test if it fulfills your needs with .local domain.
You have to understand that I'm doing that in my spare time and I have no need for this feature and limited resources in ESP may need a few compromises.

FYI resolving mDNS host only requires hostname, no domain. Domain is only there to determine if regular DNS is needed or not.

Copy link

@schildbach schildbach Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, and thanks for your time investment. I was just reviewing this PR as you asked me to do.

mqttIP = MDNS.queryHost(mqttMDNS.c_str());
if (mqttIP != IPAddress()) // if MDNS resolved the hostname
mqtt->setServer(mqttIP, mqttPort);
else
Expand Down