Use single topic for MQTT client status (LWT, birth, etc.) (#435)

* Update test for new MQTT client status approach

* Refactor About helper to support outputing JSON

* Use single topic for client status

* Web UI: Use single topic for client status

* Add platformio autogenerated file

* increase MQTT buffer from 200 -> 250 to make room for larger LWT messages

.gitignore

@@ -2,6 +2,7 @@
 .piolibdeps
 .clang_complete
 .gcc-flags.json
+.sconsign.dblite
 /web/node_modules
 /web/build
 /dist/*.bin

lib/MQTT/MqttClient.cpp

@@ -6,7 +6,11 @@
 #include <ArduinoJson.h>
 #include <WiFiClient.h>
 #include <MiLightRadioConfig.h>
-#include <AboutStringHelper.h>
+#include <AboutHelper.h>
+
+static const char* STATUS_CONNECTED = "connected";
+static const char* STATUS_DISCONNECTED = "disconnected_clean";
+static const char* STATUS_LWT_DISCONNECTED = "disconnected_unclean";
 
 MqttClient::MqttClient(Settings& settings, MiLightClient*& milightClient)
   : milightClient(milightClient),
@@ -21,6 +25,8 @@ MqttClient::MqttClient(Settings& settings, MiLightClient*& milightClient)
 }
 
 MqttClient::~MqttClient() {
+  String aboutStr = generateConnectionStatusMessage(STATUS_DISCONNECTED);
+  mqttClient->publish(settings.mqttClientStatusTopic.c_str(), aboutStr.c_str(), true);
   mqttClient->disconnect();
   delete this->domain;
 }
@@ -52,15 +58,15 @@ bool MqttClient::connect() {
     Serial.println(F("MqttClient - connecting"));
 #endif
 
-  if (settings.mqttUsername.length() > 0 && settings.mqttLwtTopic.length() > 0) {
+  if (settings.mqttUsername.length() > 0 && settings.mqttClientStatusTopic.length() > 0) {
     return mqttClient->connect(
       nameBuffer,
       settings.mqttUsername.c_str(),
       settings.mqttPassword.c_str(),
-      settings.mqttLwtTopic.c_str(),
+      settings.mqttClientStatusTopic.c_str(),
       2,
       true,
-      settings.mqttLwtMessage.c_str()
+      generateConnectionStatusMessage(STATUS_LWT_DISCONNECTED).c_str()
     );
   } else if (settings.mqttUsername.length() > 0) {
     return mqttClient->connect(
@@ -68,13 +74,13 @@ bool MqttClient::connect() {
       settings.mqttUsername.c_str(),
       settings.mqttPassword.c_str()
     );
-  } else if (settings.mqttLwtTopic.length() > 0) {
+  } else if (settings.mqttClientStatusTopic.length() > 0) {
     return mqttClient->connect(
       nameBuffer,
-      settings.mqttLwtTopic.c_str(),
+      settings.mqttClientStatusTopic.c_str(),
       2,
       true,
-      settings.mqttLwtMessage.c_str()
+      generateConnectionStatusMessage(STATUS_LWT_DISCONNECTED).c_str()
     );
   } else {
     return mqttClient->connect(nameBuffer);
@@ -82,9 +88,9 @@ bool MqttClient::connect() {
 }
 
 void MqttClient::sendBirthMessage() {
-  if (settings.mqttBirthTopic.length() > 0) {
-    String aboutStr = AboutStringHelper::generateAboutString(true);
-    mqttClient->publish(settings.mqttBirthTopic.c_str(), aboutStr.c_str());
+  if (settings.mqttClientStatusTopic.length() > 0) {
+    String aboutStr = generateConnectionStatusMessage(STATUS_CONNECTED);
+    mqttClient->publish(settings.mqttClientStatusTopic.c_str(), aboutStr.c_str(), true);
   }
 }
 
@@ -229,3 +235,17 @@ inline void MqttClient::bindTopicString(
   topicPattern.replace(":group_id", String(groupId));
   topicPattern.replace(":device_type", remoteConfig.name);
 }
+
+String MqttClient::generateConnectionStatusMessage(const char* connectionStatus) {
+  DynamicJsonBuffer buffer;
+  JsonObject& status = buffer.createObject();
+  status["status"] = connectionStatus;
+
+  // Fill other fields
+  AboutHelper::generateAboutObject(status, true);
+
+  String response;
+  status.printTo(response);
+
+  return response;
+}

lib/MQTT/MqttClient.h

@@ -49,6 +49,8 @@ private:
     const uint16_t deviceId,
     const uint16_t groupId
   );
+
+  static String generateConnectionStatusMessage(const char* status);
 };
 
 #endif

lib/Settings/AboutHelper.cpp

@@ -0,0 +1,29 @@
+#include <AboutHelper.h>
+#include <ArduinoJson.h>
+#include <Settings.h>
+#include <ESP8266WiFi.h>
+
+String AboutHelper::generateAboutString(bool abbreviated) {
+  DynamicJsonBuffer buffer;
+  JsonObject& response = buffer.createObject();
+
+  generateAboutObject(response, abbreviated);
+
+  String body;
+  response.printTo(body);
+
+  return body;
+}
+
+void AboutHelper::generateAboutObject(JsonObject& obj, bool abbreviated) {
+  obj["firmware"] = QUOTE(FIRMWARE_NAME);
+  obj["version"] = QUOTE(MILIGHT_HUB_VERSION);
+  obj["ip_address"] = WiFi.localIP().toString();
+  obj["reset_reason"] = ESP.getResetReason();
+
+  if (! abbreviated) {
+    obj["variant"] = QUOTE(FIRMWARE_VARIANT);
+    obj["free_heap"] = ESP.getFreeHeap();
+    obj["arduino_version"] = ESP.getCoreVersion();
+  }
+}

lib/Settings/AboutStringHelper.h

@@ -1,11 +1,13 @@
 #include <Arduino.h>
+#include <ArduinoJson.h>
 
 #ifndef _ABOUT_STRING_HELPER_H
 #define _ABOUT_STRING_HELPER_H
 
-class AboutStringHelper {
+class AboutHelper {
 public:
   static String generateAboutString(bool abbreviated = false);
+  static void generateAboutObject(JsonObject& obj, bool abbreviated = false);
 };
 
 #endif

lib/Settings/AboutStringHelper.cpp

@@ -1,25 +0,0 @@
-#include <AboutStringHelper.h>
-#include <ArduinoJson.h>
-#include <Settings.h>
-#include <ESP8266WiFi.h>
-
-String AboutStringHelper::generateAboutString(bool abbreviated) {
-  DynamicJsonBuffer buffer;
-  JsonObject& response = buffer.createObject();
-
-  response["firmware"] = QUOTE(FIRMWARE_NAME);
-  response["version"] = QUOTE(MILIGHT_HUB_VERSION);
-  response["ip_address"] = WiFi.localIP().toString();
-  response["reset_reason"] = ESP.getResetReason();
-
-  if (! abbreviated) {
-    response["variant"] = QUOTE(FIRMWARE_VARIANT);
-    response["free_heap"] = ESP.getFreeHeap();
-    response["arduino_version"] = ESP.getCoreVersion();
-  }
-
-  String body;
-  response.printTo(body);
-
-  return body;
-}

lib/Settings/Settings.cpp

@@ -98,9 +98,7 @@ void Settings::patch(JsonObject& parsedSettings) {
     this->setIfPresent(parsedSettings, "mqtt_topic_pattern", mqttTopicPattern);
     this->setIfPresent(parsedSettings, "mqtt_update_topic_pattern", mqttUpdateTopicPattern);
     this->setIfPresent(parsedSettings, "mqtt_state_topic_pattern", mqttStateTopicPattern);
-    this->setIfPresent(parsedSettings, "mqtt_lwt_topic", mqttLwtTopic);
-    this->setIfPresent(parsedSettings, "mqtt_lwt_message", mqttLwtMessage);
-    this->setIfPresent(parsedSettings, "mqtt_birth_topic", mqttBirthTopic);
+    this->setIfPresent(parsedSettings, "mqtt_client_status_topic", mqttClientStatusTopic);
     this->setIfPresent(parsedSettings, "discovery_port", discoveryPort);
     this->setIfPresent(parsedSettings, "listen_repeats", listenRepeats);
     this->setIfPresent(parsedSettings, "state_flush_interval", stateFlushInterval);
@@ -216,9 +214,7 @@ void Settings::serialize(Stream& stream, const bool prettyPrint) {
   root["mqtt_topic_pattern"] = this->mqttTopicPattern;
   root["mqtt_update_topic_pattern"] = this->mqttUpdateTopicPattern;
   root["mqtt_state_topic_pattern"] = this->mqttStateTopicPattern;
-  root["mqtt_lwt_topic"] = this->mqttLwtTopic;
-  root["mqtt_lwt_message"] = this->mqttLwtMessage;
-  root["mqtt_birth_topic"] = this->mqttBirthTopic;
+  root["mqtt_client_status_topic"] = this->mqttClientStatusTopic;
   root["discovery_port"] = this->discoveryPort;
   root["listen_repeats"] = this->listenRepeats;
   root["state_flush_interval"] = this->stateFlushInterval;

lib/Settings/Settings.h

@@ -167,9 +167,7 @@ public:
   String mqttTopicPattern;
   String mqttUpdateTopicPattern;
   String mqttStateTopicPattern;
-  String mqttLwtTopic;
-  String mqttLwtMessage;
-  String mqttBirthTopic;
+  String mqttClientStatusTopic;
   size_t stateFlushInterval;
   size_t mqttStateRateLimit;
   size_t packetRepeatThrottleThreshold;

lib/WebServer/MiLightHttpServer.cpp

@@ -6,7 +6,7 @@
 #include <MiLightRadioConfig.h>
 #include <string.h>
 #include <TokenIterator.h>
-#include <AboutStringHelper.h>
+#include <AboutHelper.h>
 #include <index.html.gz.h>
 
 void MiLightHttpServer::begin() {
@@ -120,19 +120,7 @@ void MiLightHttpServer::onGroupDeleted(GroupDeletedHandler handler) {
 }
 
 void MiLightHttpServer::handleAbout() {
-  // DynamicJsonBuffer buffer;
-  // JsonObject& response = buffer.createObject();
-
-  // response["version"] = QUOTE(MILIGHT_HUB_VERSION);
-  // response["variant"] = QUOTE(FIRMWARE_VARIANT);
-  // response["free_heap"] = ESP.getFreeHeap();
-  // response["arduino_version"] = ESP.getCoreVersion();
-  // response["reset_reason"] = ESP.getResetReason();
-
-  // String body;
-  // response.printTo(body);
-
-  server.send(200, APPLICATION_JSON, AboutStringHelper::generateAboutString());
+  server.send(200, APPLICATION_JSON, AboutHelper::generateAboutString());
 }
 
 void MiLightHttpServer::handleGetRadioConfigs() {

platformio.ini

@@ -24,7 +24,7 @@ lib_deps_external =
   CircularBuffer@~1.2.0
 extra_scripts =
   pre:.build_web.py
-build_flags = !python .get_version.py -DMQTT_MAX_PACKET_SIZE=200 -DHTTP_UPLOAD_BUFLEN=128 -D FIRMWARE_NAME=milight-hub -Idist -Ilib/DataStructures
+build_flags = !python .get_version.py -DMQTT_MAX_PACKET_SIZE=250 -DHTTP_UPLOAD_BUFLEN=128 -D FIRMWARE_NAME=milight-hub -Idist -Ilib/DataStructures
 # -D STATE_DEBUG
 # -D DEBUG_PRINTF
 # -D MQTT_DEBUG

test/remote/spec/mqtt_spec.rb

@@ -50,20 +50,27 @@ RSpec.describe 'State' do
     end
   end
 
-  context 'birth and LWT' do
+  context 'client status topic' do
     # Unfortunately, no way to easily simulate an unclean disconnect, so only test birth
-    it 'should send birth message when configured' do
-      birth_topic = "#{@topic_prefix}birth"
+    it 'should send client status messages when configured' do
+      status_topic = "#{@topic_prefix}client_status"
 
       @client.put(
         '/settings',
-        mqtt_birth_topic: birth_topic
+        mqtt_client_status_topic: status_topic
       )
 
-      seen_birth = false
+      # Clear any retained messages
+      @mqtt_client.publish(status_topic, nil)
 
-      @mqtt_client.on_message(birth_topic) do |topic, message|
-        seen_birth = true
+      seen_statuses = Set.new
+      required_statuses = %w(connected disconnected_clean)
+
+      @mqtt_client.on_message(status_topic, 20) do |topic, message|
+        message = JSON.parse(message)
+
+        seen_statuses << message['status']
+        required_statuses.all? { |x| seen_statuses.include?(x) }
       end
 
       # Force MQTT reconnect by updating settings
@@ -71,7 +78,7 @@ RSpec.describe 'State' do
 
       @mqtt_client.wait_for_listeners
 
-      expect(seen_birth).to be(true)
+      expect(seen_statuses).to include(*required_statuses)
     end
   end
 
@@ -128,9 +135,6 @@ RSpec.describe 'State' do
     end
 
     it 'should respect the state update interval' do
-      # Wait for MQTT to reconnect
-      @mqtt_client.on_message("#{@topic_prefix}birth") { |x, y| true }
-
       # Disable updates to prevent the negative effects of spamming commands
       @client.put(
         '/settings',
@@ -139,8 +143,6 @@ RSpec.describe 'State' do
         packet_repeats: 1
       )
 
-      @mqtt_client.wait_for_listeners
-
       # Set initial state
       @client.patch_state({status: 'ON', level: 0}, @id_params)
 

web/src/js/script.js

@@ -15,7 +15,7 @@ $(function() {
             input.val(log);
         }
     });
-  });  
+  });
 });
 
 var UNIT_PARAMS = {
@@ -134,13 +134,13 @@ var UI_FIELDS = [ {
     type: "string",
     tab: "tab-mqtt"
   }, {
-    tag: "mqtt_topic_pattern", 
+    tag: "mqtt_topic_pattern",
     friendly: "MQTT topic pattern",
     help: "Pattern for MQTT topics to listen on. Example: lights/:device_id/:device_type/:group_id. See README for further details",
     type: "string",
     tab: "tab-mqtt"
   }, {
-    tag:   "mqtt_update_topic_pattern", 
+    tag:   "mqtt_update_topic_pattern",
     friendly: "MQTT update topic pattern",
     help: "Pattern to publish MQTT updates. Packets that are received from other devices, and packets that are sent from this device will " +
     "result in updates being sent",
@@ -153,37 +153,25 @@ var UI_FIELDS = [ {
     type: "string",
     tab: "tab-mqtt"
   }, {
-    tag:   "mqtt_username", 
+    tag:   "mqtt_username",
     friendly: "MQTT user name",
     help: "User name to log in to MQTT server",
     type: "string",
     tab: "tab-mqtt"
   }, {
-    tag:   "mqtt_password", 
+    tag:   "mqtt_password",
     friendly: "MQTT password",
     help: "Password to log into MQTT server",
     type: "string",
     tab: "tab-mqtt"
   }, {
-    tag:   "mqtt_lwt_topic", 
-    friendly: "MQTT LWT Topic",
-    help: "Topic to use for LWT message (leave blank to disable LWT)",
-    type: "string",
-    tab: "tab-mqtt"
-  }, {
-    tag:   "mqtt_lwt_message", 
-    friendly: "MQTT LWT Message",
-    help: "LWT Message - sent when client disconnects uncleanly",
+    tag:   "mqtt_client_status_topic",
+    friendly: "MQTT Client Status Topic",
+    help: "Connection status messages will be published to this topic.  This includes LWT and birth.  See README for further detail.",
     type: "string",
     tab: "tab-mqtt"
   }, {
-    tag:   "mqtt_birth_topic", 
-    friendly: "MQTT Birth Topic",
-    help: "Birth Topic - JSON blob with system details will be sent to this topic upon connection",
-    type: "string",
-    tab: "tab-mqtt"
-  }, {
-    tag:   "radio_interface_type", 
+    tag:   "radio_interface_type",
     friendly: "Radio interface type",
     help: "2.4 GHz radio model. Only change this if you know you're not using an NRF24L01!",
     type: "option_buttons",
@@ -193,7 +181,7 @@ var UI_FIELDS = [ {
     },
     tab: "tab-radio"
   }, {
-    tag:   "rf24_power_level", 
+    tag:   "rf24_power_level",
     friendly: "nRF24 Power Level",
     help: "Power level for nRF24L01",
     type: "option_buttons",
@@ -205,7 +193,7 @@ var UI_FIELDS = [ {
     },
     tab: "tab-radio"
   }, {
-    tag:   "rf24_listen_channel", 
+    tag:   "rf24_listen_channel",
     friendly: "nRF24 Listen Channel",
     help: "Which channels to listen for messages on the nRF24",
     type: "option_buttons",
@@ -216,7 +204,7 @@ var UI_FIELDS = [ {
     },
     tab: "tab-radio"
   }, {
-    tag:   "rf24_channels", 
+    tag:   "rf24_channels",
     friendly: "nRF24 Send Channels",
     help: "Which channels to send messages on for the nRF24.  Using fewer channels speeds up sends.",
     type: "option_buttons",
@@ -237,14 +225,14 @@ var UI_FIELDS = [ {
     type: "string",
     tab: "tab-wifi"
   }, {
-    tag:   "state_flush_interval", 
+    tag:   "state_flush_interval",
     friendly: "State flush interval",
     help: "Minimum number of milliseconds between flushing state to flash. " +
     "Set to 0 to disable delay and immediately persist state to flash",
     type: "string",
     tab: "tab-setup"
   }, {
-    tag:   "mqtt_state_rate_limit", 
+    tag:   "mqtt_state_rate_limit",
     friendly: "MQTT state rate limit",
     help: "Minimum number of milliseconds between MQTT updates of bulb state (defaults to 500)",
     type: "string",
@@ -259,7 +247,7 @@ var UI_FIELDS = [ {
     type: "string",
     tab: "tab-radio"
   }, {
-    tag:   "packet_repeat_throttle_sensitivity", 
+    tag:   "packet_repeat_throttle_sensitivity",
     friendly: "Packet repeat throttle sensitivity",
     help: "Controls how packet repeats are throttled. " +
     "Higher values cause packets to be throttled up and down faster " +
@@ -267,7 +255,7 @@ var UI_FIELDS = [ {
     type: "string",
     tab: "tab-radio"
   }, {
-    tag:   "packet_repeat_minimum", 
+    tag:   "packet_repeat_minimum",
     friendly: "Packet repeat minimum",
     help: "Controls how far throttling can decrease the number " +
     "of repeated packets (defaults to 3)",
@@ -280,7 +268,7 @@ var UI_FIELDS = [ {
     type: "group_state_fields",
     tab: "tab-mqtt"
   }, {
-    tag:   "enable_automatic_mode_switching", 
+    tag:   "enable_automatic_mode_switching",
     friendly: "Enable automatic mode switching",
     help: "For RGBWW bulbs (using RGB+CCT or FUT089), enables automatic switching between modes in order to affect changes to " +
     "temperature and saturation when otherwise it would not work",
@@ -319,7 +307,7 @@ var UI_FIELDS = [ {
     friendly: "Flash count on packets",
     help: "Number of times the LED will flash when packets are changing",
     type: "string",
-    tab: "tab-led"    
+    tab: "tab-led"
   }
 ];
 
@@ -928,7 +916,7 @@ $(function() {
     allowEmptyOption: true,
     render: {
       option: function(data, escape) {
-        // Mousedown selects an option -- prevent event from bubbling up to select option 
+        // Mousedown selects an option -- prevent event from bubbling up to select option
         // when delete button is clicked.
         var deleteBtn = $('<span class="selectize-delete"><a href="#"><i class="glyphicon glyphicon-trash"></i></a></span>')
           .mousedown(function(e) {
@@ -1028,7 +1016,7 @@ $(function() {
     });
     settings += "</div>";
   });
-  
+
   // UDP gateways tab
   settings += '<div class="tab-pane fade ' + tabClass + '" id="tab-udp-gateways">';
   settings += $('#gateway-servers-modal .modal-body').remove().html();
@@ -1060,7 +1048,7 @@ $(function() {
           }
 
           return a;
-        }, 
+        },
         {
           // Make sure the value is always an array, even if a single item is selected
           rf24_channels: []
@@ -1081,7 +1069,7 @@ $(function() {
     }
 
     $('#settings-modal').modal('hide');
-    
+
     return false;
   });
 
@@ -1126,5 +1114,5 @@ $(function() {
             input.val(log);
         }
     });
-  });  
+  });
 });