Add ability to change discovery prefix on the fly either via WebGUI or mosquitto_sub

[puterboy]

Is your feature request related to a problem? Please describe. I am using OMG_lilygo_rtl_433_esp to capture 433 messages from my various sensors around my house and then post them to my Mosquitto MQTT broker for use in HomeAssistant.

• On the one hand, I need to shut off OpenMQTT Gateway "Auto Discovery" because otherwise HA dutifully recognizes and inserts new devices and sensors for the dozens of devices that my neighbors have (which then get further duplicated whenever they or even I change batteries in various devices).

• On the other hand, when I add a new device myself, I want to generate a discovery message(s) for that device alone so that HA will recognize that sensor -- but I don't want to suck in my neighbors devices at the same time

Describe the solution you'd like I would like to have the discovery prefix be changeable on-the-fly without having to recompile and re-flash firmware. This could done either with the Web GUI (similar to how one can set the MQTT Base Topic from the Configure MQTT menu or by sending a "System Command" via mosquitto_pub as in https://docs.openmqttgateway.com/use/gateway.html

So one could have:

mosquitto_pub -t "home/OpenMQTTGateway/commands/MQTTtoSYS/config" -m '{  "discovery_topic": "homeassistant-staging" }

which would be analogous to how one sets the MQTT_topic:

mosquitto_pub -t "home/OpenMQTTGateway/commands/MQTTtoSYS/config" -m '{  "mqtt_topic": "topic/" }

NOTE: I used discovery_topic rather than say discovery_prefix to be consistent with the variable naming in the source code.

Describe alternatives you've considered I have considered the following 3 workarounds:

1. Recompile and reflash the firmware for my Lilygo device with the Discovery Prefix changed to something other than the default discovery prefix used by HA (which is "homeassistant") and then manually cut and paste my newly discovered device from the new discovery prefix to the HA assistant one (or use mosquitto_sub/mosquitto_pub to do the same)

2. Change either temporarily or permanently the default discovery prefix for HA away from the default discovery prefix for OpenMQTTGateway (which is "homeassistant"). Then delete any unwanted neighbor discovery messages and turn off OpenMQTT Gateway auto-discovery before changing the HA discovery prefix back to its default

3. Turn off HA or shut off MQTT integration polling while I listen for discovery messages from OpenMQTT Gateway and then delete any unwanted newly discovered neighbor devices and turn off auto-discovery before turning back on the HA MQTT integration

However, changing the Discovery prefix on the fly within OpenMQTTGateway seems the easiest and cleanest solution that does the least "harm" since I don't disrupt my HA setup and I can easily change the OpenMQTTGateway prefix back to its default when I am done adding any new devices.

[puterboy]

It would seem adding this would require only trivial changes tot the following 3 files: (Note: my formatting below is pseudo-patch like)

https://github.com/1technophile/OpenMQTTGateway/blob/development/main/config_mqttDiscovery.h

  #ifndef discovery_Topic
  #  define discovery_Topic "homeassistant"
  #endif
...
+ char discovery_topic[parameters_size + 1] = discovery_Topic;

https://github.com/1technophile/OpenMQTTGateway/blob/development/main/main.ino

- const bool is_autodiscovery_subscription = (pattern == "#") || (pattern.startsWith(String(discovery_Topic) + "/"));
+ const bool is_autodiscovery_subscription = (pattern == "#") || (pattern.startsWith(String(discovery_topic) + "/"));
...
  json["mqtt_topic"] = mqtt_topic;
+ json["discovery_topic"] = discovery_topic;
...
       if (json.containsKey("mqtt_topic"))
          strcpy(mqtt_topic, json["mqtt_topic"]);
+     if (json.containsKey("discovery_topic"))
+       strcpy(discovery_topic, json["discovery_topic"]);
...
  WiFiManagerParameter custom_mqtt_topic("topic", "mqtt base topic", mqtt_topic, mqtt_topic_max_size, " minlength='1' maxlength='64' required");
+ WiFiManagerParameter custom_discovery_topic("topic", "discovery topic prefix", discovery_topic, parameters_size, " minlength='1' maxlength='64' required");
...
  wifiManager.addParameter(&custom_mqtt_topic);
+ wifiManager.addParameter(&custom_discovery_topic);
...
  strcpy(mqtt_topic, custom_mqtt_topic.getValue());
  if (mqtt_topic[strlen(mqtt_topic) - 1] != '/' && strlen(mqtt_topic) < parameters_size) {
    strcat(mqtt_topic, "/");
  }
+ strcpy(discovery_topic, custom_discovery_topic.getValue());
+ size_t dt_len = strlen(discovery_topic);
+ if(if dt_len > 0 && discovery_topic[srtlen(dt_len-1] == '/')
+   discovery_topic[dt_len-1] = '\0'; //Strip trailing '/' if any
...
 if ((SYSdata.containsKey("mqtt_topic") && SYSdata["mqtt_topic"].is<char*>()) ||
        (SYSdata.containsKey("gateway_name") && SYSdata["gateway_name"].is<char*>()) ||
+     (SYSdata.containsKey("discovery_topic") && SYSdata["discovery_topic"].is<char*>()) ||
        (SYSdata.containsKey("gw_pass") && SYSdata["gw_pass"].is<char*>())) {
      if (SYSdata.containsKey("mqtt_topic")) {
        strncpy(mqtt_topic, SYSdata["mqtt_topic"], parameters_size);
      }
      if (SYSdata.containsKey("gateway_name")) {
        strncpy(gateway_name, SYSdata["gateway_name"], parameters_size);
      }
+     if (SYSdata.containsKey("discovery_topic")) {
+       strncpy(discovery_topic, SYSdata["discovery_topic"], parameters_size);
+     }

https://github.com/1technophile/OpenMQTTGateway/blob/development/main/ZmqttDiscovery.ino

- String topic_to_publish = String(discovery_Topic) + "/device_automation/" + String(unique_id) + "/config";
+ String topic_to_publish = String(discovery_topic) + "/device_automation/" + String(unique_id) + "/config";
...
-  String topic = String(discovery_Topic) + "/" + String(sensor_type) + "/" + String(unique_id) + "/config";
+ String topic = String(discovery_topic) + "/" + String(sensor_type) + "/" + String(unique_id) + "/config";
...
- String topic = String(discovery_Topic) + "/" + String(sensor_type) + "/" + String(unique_id) + "/config";
+ String topic = String(discovery_topic) + "/" + String(sensor_type) + "/" + String(unique_id) + "/config";

Couldn't be more trivial :)

If you are willing to add this feature, I will test it and create a proper PR. Let me know...

[1technophile]

Feel free to push a PR, also would be interesting to have the capability into the WebUI

[puterboy]

Trying to understand the WebUI code but if I am reading the code correctly, it seems like it will require only minor additions to config_WebContent.h to extend the web page and to ZWebUI.ino to handle the page.

Though I am a little confused about WEBtoSYS and what that does vs. just directly setting the variables based on the data coming back from the web form.

[puterboy]

https://github.com/1technophile/OpenMQTTGateway/blob/development/main/config_WebContent.h

- // mqtt server (mh), mqtt port (ml), mqtt username (mu), mqtt password (mp), secure connection (sc), server certificate (msc), topic (mt)
+ // mqtt server (mh), mqtt port (ml), mqtt username (mu), mqtt password (mp), secure connection (sc), server certificate (msc), mqtt topic (mt), discovery topic (dt)
...
- const char config_mqtt_body[] = body_header "<fieldset class=\"set1\"><legend><span><b>MQTT Parameters</b></span></legend><form method='get' action='mq'><p><b>MQTT Server</b><br><input id='mh' placeholder=" MQTT_SERVER " value='%s'></p><p><b>MQTT Port</b><br><input id='ml' placeholder=" MQTT_PORT " value='%s'></p><p><b>MQTT Username</b><br><input id='mu' placeholder=" MQTT_USER " value='%s'></p><p><label><b>MQTT Password</b></label><br><input id='mp' type='password' placeholder=\"Password\" ></p><p><b>MQTT Secure Connection</b><br><input id='sc' type='checkbox' %s></p><p><b>Gateway Name</b><br><input id='h' placeholder=" Gateway_Name " value=\"%s\"></p><p><b>MQTT Base Topic</b><br><input id='mt' placeholder='' value='%s'></p><br><button name='save' type='submit' class='button bgrn'>Save</button></form></fieldset>" body_footer_config_menu;

+ const char config_mqtt_body[] = body_header "<fieldset class=\"set1\"><legend><span><b>MQTT Parameters</b></span></legend><form method='get' action='mq'><p><b>MQTT Server</b><br><input id='mh' placeholder=" MQTT_SERVER " value='%s'></p><p><b>MQTT Port</b><br><input id='ml' placeholder=" MQTT_PORT " value='%s'></p><p><b>MQTT Username</b><br><input id='mu' placeholder=" MQTT_USER " value='%s'></p><p><label><b>MQTT Password</b></label><br><input id='mp' type='password' placeholder=\"Password\" ></p><p><b>MQTT Secure Connection</b><br><input id='sc' type='checkbox' %s></p><p><b>Gateway Name</b><br><input id='h' placeholder=" Gateway_Name " value=\"%s\"></p><p><b>MQTT Base Topic</b><br><input id='mt' placeholder='' value='%s'></p><b>MQTT Discovery Topic</b><br><input id='dt' placeholder='' value='%s'></p><br><button name='save' type='submit' class='button bgrn'>Save</button></form></fieldset>" body_footer_config_menu;

https://github.com/1technophile/OpenMQTTGateway/blob/development/main/ZwebUI.ino

  if (server.hasArg("mt")) {
    WEBtoSYS["mqtt_topic"] = server.arg("mt");
    if (strncmp(mqtt_topic, server.arg("mt").c_str(), parameters_size)) {
      update = true;
    }
  }
+ if (server.hasArg("dt")) {
+   WEBtoSYS["discovery_topic"] = server.arg("dt");
+    if (strncmp(discovery_topic, server.arg("dt").c_str(), parameters_size)) {
+      update = true;
+   }
+ }
...
-   // mqtt server (mh), mqtt port (ml), mqtt username (mu), mqtt password (mp), secure connection (sc), server certificate (msc), topic (mt)
+   // mqtt server (mh), mqtt port (ml), mqtt username (mu), mqtt password (mp), secure connection (sc), server certificate (msc), mqtt topic (mt), discovery topic (dt)
...
  #  if MQTT_BROKER_MODE
-  snprintf(buffer, WEB_TEMPLATE_BUFFER_MAX_SIZE, config_mqtt_body, jsonChar, gateway_name, "", "1883", "", "", gateway_name, mqtt_topic);
+  snprintf(buffer, WEB_TEMPLATE_BUFFER_MAX_SIZE, config_mqtt_body, jsonChar, gateway_name, "", "1883", "", "", gateway_name, mqtt_topic, discovery_topic);
  #  else
-   snprintf(buffer, WEB_TEMPLATE_BUFFER_MAX_SIZE, config_mqtt_body, jsonChar, gateway_name, cnt_parameters_array[CNT_DEFAULT_INDEX].mqtt_server, cnt_parameters_array[CNT_DEFAULT_INDEX].mqtt_port, cnt_parameters_array[CNT_DEFAULT_INDEX].mqtt_user, (cnt_parameters_array[CNT_DEFAULT_INDEX].isConnectionSecure ? "checked" : ""), gateway_name, mqtt_topic);
+   snprintf(buffer, WEB_TEMPLATE_BUFFER_MAX_SIZE, config_mqtt_body, jsonChar, gateway_name, cnt_parameters_array[CNT_DEFAULT_INDEX].mqtt_server, cnt_parameters_array[CNT_DEFAULT_INDEX].mqtt_port, cnt_parameters_array[CNT_DEFAULT_INDEX].mqtt_user, (cnt_parameters_array[CNT_DEFAULT_INDEX].isConnectionSecure ? "checked" : ""), gateway_name, mqtt_topic, discovery_topic);

[1technophile]

Trying to understand the WebUI code but if I am reading the code correctly, it seems like it will require only minor additions to config_WebContent.h to extend the web page and to ZWebUI.ino to handle the page.

Correct

Though I am a little confused about WEBtoSYS and what that does vs. just directly setting the variables based on the data coming back from the web form.

WEBtoSYS is a way to pass the parameter to MQTTtoSYS

[1technophile]

I am not sure we should enable to change the discovery message during onboarding, I think a new parameter may confuse the users and generate support.

[puterboy]

OK - does that mean not adding the WiFi manager codelines? (if I am understanding that correctly wifiManager is used to create a temporary AP to log into, right? -- if I am misunderstanding, please point me to the code that is run during onboarding)

BTW, I made a couple of corrections to the above pseudo-patches

[puterboy]

BTW, I think discovery_prefix might be a better name for the new variable version of the declaration discovery_Topic I say this because you already use the variable name discovery_topic for the name of the discovery (sub) topic that sits under what is now the fixed discovery_Topic.

I could of course just set: discovery_prefix = discovery_Topic but ideally, one would renamediscovery_Topic to discovery_Prefix to be consistent between naming of the declaration and its associated variable (Note that even now it is confusing that the declaration discovery_Topic refers to the base parent while the variable discovery_topic refers to the child subtopic)

Note that Home Assistant FWIW uses the term discovery_prefix

In order not to break existing configs, perhaps discovery_Topic could be deprecated in favor of discovery_Prefix for the declaration, but an #ifdef could be added to pull in discovery_Topic if it exists in someone's (legacy) config.

Let me know what you think

[1technophile]

OK - does that mean not adding the WiFi manager codelines?

Yes, Indeed. The onboarding can be done through a web portal. Most of the time, when used on a mobile phone, adding a field to the limited screen may increase the risk of misconfiguration by non-initiated users.

The parameters that are not into WiFi Manager but saved are into SYSConfig_s, it could be added there.

BTW, I think discovery_prefix might be a better name for the new variable version of the declaration discovery_Topic I say this because you already use the variable name discovery_topic for the name of the discovery (sub) topic that sits under what is now the fixed discovery_Topic.

It makes sense

In order not to break existing configs, perhaps discovery_Topic could be deprecated in favor of discovery_Prefix for the declaration, but an #ifdef could be added to pull in discovery_Topic if it exists in someone's (legacy) config.

Agreed

[puterboy]

One (hopefully) final question... I am a bit confused about how & when changes are stored to flash or other non-volatile storage. It seems like for certain variables (e.g, mqtt_topic, gatewa_name, gw_pass, mqtt_topic, etc.) saving is always done when variables are changed via the 'saveConfig()' routine which saves a JSON'ized string of the current variable settings.

On the other hand, other variables (e.g., discovery, ohdiscovery, XtoMQTT, rgbbrightness), use a separate 'preferences' way to save the values to NVS and that only happens when the 'save' flag is set in the mqtt payload.

Why are these changes treated differently? Am I missing something?

[1technophile]

The first approach is due to the use of WiFi Manager for onboarding. After that, parameters were added that did not require adjustment through the onboarding process; they were added through the NVS/preferences approach.

[puterboy]

Merged with Development branch

[puterboy]

Just found a small but critical issue in my patch. In the new stanza in MQTTtoSYS where discovery_prefix is parsed, I added the line:

restartESP = true 

which I inserted so that discovery topics will be generated fresh under the new discovery_prefix HOWEVER, based on how handleMQ in ZwebuUI.ino is coded, this has the side-effect of triggering a reboot for every change on the MQTT Configuration page which in turn creates a problem for the MQTT login variables (e.g., mqtt_server, mqtt_port, mqtt_user, mqtt_pass) since they are only saved as part of a call to setupMQTT() triggered by setting mqttSetupPending which then never happens as it is pre-empted by the reboot which wipes out any changes to those unsaved variables.

It turns out though that setupMQTT() always triggers a reboot anyway in a rather backhanded fashion. Since a reboot is triggered if any element of cnt_parameters is changed which in turn is triggered by any change in any of the MQTT config variables (even ones that are not stored in cnt_parameters. So, changing discovery_prefix will result in a reboot anyway.

So all that needs to be done is to remove the above line from my original PR.

The only downside is that the change is only saved if the MQTT setup and connection are successful which shouldn't be truly necessary -- this is a consequence of the fact that the code is a bit knotted as detailed in the below comments.

[puterboy]

More generally, I don't think any of the variables really require a reboot.

1. The MQTT config variables just require rerunning setupMQTT() and then reconnecting MQTT
2. For discovery_prefix, one could additionally trigger re-posting all discovery topics (as they arrive) by setting connectedOnce = false to simulate a first connection (this is a static variable within the routine handle_autodiscovery so you would just have to make it into a global variable in main.ino)
3. The gateway name should probably be treated similarly to discovery_prefix ('#2') since many of the MQTT topics embed the gateway name somewhere in their name -- i.e, rerun setupMQTT() with connectedOnce = false
4. The gateway password wouldn't need any reboot or reconnection
5. The router SSID and password variables really just need a WiFi disconnect & reconnect (perhaps followed by an explicit MQTT connect if not already automatic in the logic)
6. Finally, the changing of any of the on-volatile variables would trigger a single call to setupMQTT(), contingent on a successful reconnection to WiFi or MQTT as appropriate.

This could all be handled by clearly separating the following 4 actions:

1. Save configurations (needs to be done any time at least one variable stored in non-volatile memory is changed) made contingent on successful completion of # 2 or # 3 as appropriate
2. Rerun MQTT setup and reconnect (needs to be done potentially for all MQTT variable changes)
3. Restart WiFi (needs to be done for wifi SSID/passwd changes)
4. Reboot (Only run after the above and only if necessary - may not be needed at all)

But this is all nice-to-have and unfortunately I don't have time to rewrite the code myself (nor am I particularly good at that)

[puterboy]

BTW, the reason I suggest ultimately a rewrite would be nice is that the code in MQTTtoSYS seems a bit hairy and inconsistent at times -- presumably it was built up over time.

For example, some variables directly and explicitly trigger a saveConfig and/or ESPrestart while others do so only very indirectly via a combination of calling setupMQTT and connecting to MQTT which triggers a save and reboot as a side effect of the fact that other variables that happen to be on the same page are stored in the cnt_parameters_array.

Also, as a result of the above, some variables actually trigger saveConfig twice resulting in duplicated saving and wear-and-tear on the flash -- first via an explicit call to saveConfig and second via setting the mqttSetupPending which in turn calls a subsequent setupMQTT and an eventual further call to saveConfig (as described above)

[puterboy]

Fixed as per above PRs