Add MDNS enable in Web UI #22602
Add MDNS enable in Web UI #22602
Conversation
This allows the awesome MDNS advertise feature to be easier to discover and enable for novice users.
cristiklein
force-pushed
the
add-mdns-in-ui
branch
from
a2689d5 to
f807614
Compare
There was a problem hiding this comment.
Thanks but your PR is broken on multiple levels: missing languages and guarding with USE_DISCOVERY.
Overall MDNS is a feature that brings most problems than it solves, I'm not sure we want to surface it in the GUI. As it is only for advanced users that know what they are doing, I recommend not to add it.
| @@ -350,6 +350,7 @@ | |||
| #define D_MQTT_TLS_ENABLE "MQTT TLS" | |||
| #define D_HTTP_API "HTTP API" | |||
| #define D_HTTP_API_ENABLE "HTTP API enable" | |||
| #define D_MDNS_ENABLE "MDNS enable" | |||
There was a problem hiding this comment.
The define must be done in every language file, not just English
| @@ -351,6 +351,7 @@ const char HTTP_FORM_OTHER[] PROGMEM = | |||
| "<br>" | |||
| "<label><input id='b3' type='checkbox'%s><b>" D_HTTP_API_ENABLE "</b></label><br>" | |||
| "<label><input id='b1' type='checkbox'%s><b>" D_MQTT_ENABLE "</b></label><br>" | |||
| "<label><input id='b4' type='checkbox'%s><b>" D_MDNS_ENABLE "</b></label><br>" | |||
There was a problem hiding this comment.
This must be guarded by #ifdef USE_DISCOVERY to not show it when MDNS is not present in the build
| @@ -2653,6 +2654,7 @@ void HandleOtherConfiguration(void) { | |||
| WSContentSend_P(HTTP_FORM_OTHER, HtmlEscape(ResponseData()).c_str(), (USER_MODULE == Settings->module) ? PSTR(" checked disabled") : "", | |||
| (Settings->flag5.disable_referer_chk) ? PSTR(" checked") : "", // SetOption128 - Enable HTTP API | |||
| (Settings->flag.mqtt_enabled) ? PSTR(" checked") : "", // SetOption3 - Enable MQTT | |||
| (Settings->flag3.mdns_enabled) ? PSTR(" checked") : "", // SetOption55 - Enable MDNS | |||
There was a problem hiding this comment.
Guard with #ifdef USE_DISCOVERY
| @@ -2706,6 +2708,8 @@ void OtherSaveSettings(void) { | |||
| cmnd += Webserver->hasArg(F("b1")); | |||
| cmnd += F(";" D_CMND_SO "128 "); | |||
| cmnd += Webserver->hasArg(F("b3")); | |||
| cmnd += F(";" D_CMND_SO "55 "); | |||
| cmnd += Webserver->hasArg(F("b4")); | |||
There was a problem hiding this comment.
Guard with #ifdef USE_DISCOVERY
|
There is no mDNS support in the esp8266 precompiled builds and there will be never (horrible faulty and eats up to much flash). From my side a clear no to add in GUI |
|
Thx for your work. As you noticed we hate mDNS as it uses way to much code space and except for Matter it isn't really needed once you've managed to install a simple DNS server in your home environment. |
|
I agree. Thanks folks for taking the time to look at my PR and give me feedback. It was good practice for me and I hope I'll come with a more useful PR next time. (Although, to be honest, the project feels rather finished, so I'm unsure what that could be.) |
Description:
This allows the awesome MDNS advertise feature to be easier to discover and enable for novice users. See screenshot below.
May I use the space in this PR to express the awesomeness of this project. I'm not a stranger to embedded development, but have never worked with PlatformIO or Tasmota before. Few hours later, I can build my own firmware and contribute to this project. Kudos!
Related issue (if applicable): fixes #
Note to review: I didn't find guidance on how to perform testing against Tasmota core ESP8266 V.2.7.8 and Tasmota core ESP32 V.3.1.0.241206. Please provide me with guidance and I'll perform the tests.
Checklist:
NOTE: The code change must pass CI tests. Your PR cannot be merged unless tests pass