From 5e7f3c29bd4f6c2ec3922907110119fbbf103274 Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Fri, 1 Nov 2019 23:11:41 +0300 Subject: [PATCH 1/6] mqtt: reduce debug log pressure, return result of mqttSend --- code/espurna/config/prototypes.h | 4 ++-- code/espurna/mqtt.ino | 29 +++++++++++++++++++---------- 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/code/espurna/config/prototypes.h b/code/espurna/config/prototypes.h index 48953e94..3faf1945 100644 --- a/code/espurna/config/prototypes.h +++ b/code/espurna/config/prototypes.h @@ -204,8 +204,8 @@ String mqttTopic(const char * magnitude, unsigned int index, bool is_set); String mqttMagnitude(char * topic); -void mqttSendRaw(const char * topic, const char * message, bool retain); -void mqttSendRaw(const char * topic, const char * message); +bool mqttSendRaw(const char * topic, const char * message, bool retain); +bool mqttSendRaw(const char * topic, const char * message); void mqttSend(const char * topic, const char * message, bool force, bool retain); void mqttSend(const char * topic, const char * message, bool force); diff --git a/code/espurna/mqtt.ino b/code/espurna/mqtt.ino index d5777e0b..54b01a1d 100644 --- a/code/espurna/mqtt.ino +++ b/code/espurna/mqtt.ino @@ -640,25 +640,34 @@ String mqttTopic(const char * magnitude, unsigned int index, bool is_set) { // ----------------------------------------------------------------------------- -void mqttSendRaw(const char * topic, const char * message, bool retain) { +bool mqttSendRaw(const char * topic, const char * message, bool retain) { - if (_mqtt.connected()) { + if (!_mqtt.connected()) return false; + + const unsigned int packetId( #if MQTT_LIBRARY == MQTT_LIBRARY_ASYNCMQTTCLIENT - unsigned int packetId = _mqtt.publish(topic, _mqtt_qos, retain, message); - DEBUG_MSG_P(PSTR("[MQTT] Sending %s => %s (PID %d)\n"), topic, message, packetId); + _mqtt.publish(topic, _mqtt_qos, retain, message) #elif MQTT_LIBRARY == MQTT_LIBRARY_ARDUINOMQTT - _mqtt.publish(topic, message, retain, _mqtt_qos); - DEBUG_MSG_P(PSTR("[MQTT] Sending %s => %s\n"), topic, message); + _mqtt.publish(topic, message, retain, _mqtt_qos) #elif MQTT_LIBRARY == MQTT_LIBRARY_PUBSUBCLIENT - _mqtt.publish(topic, message, retain); - DEBUG_MSG_P(PSTR("[MQTT] Sending %s => %s\n"), topic, message); + _mqtt.publish(topic, message, retain) #endif + ); + + const size_t message_len = strlen(message); + if (message_len > 128) { + DEBUG_MSG_P(PSTR("[MQTT] Sending %s => (%u bytes) (PID %u)\n"), topic, message_len, packetId); + } else { + DEBUG_MSG_P(PSTR("[MQTT] Sending %s => %s (PID %u)\n"), topic, message, packetId); } + + return (packetId > 0); + } -void mqttSendRaw(const char * topic, const char * message) { - mqttSendRaw (topic, message, _mqtt_retain); +bool mqttSendRaw(const char * topic, const char * message) { + return mqttSendRaw (topic, message, _mqtt_retain); } void mqttSend(const char * topic, const char * message, bool force, bool retain) { From 5bef606f2ab8974dcdf525e0339d910a03396fe7 Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Fri, 1 Nov 2019 23:13:51 +0300 Subject: [PATCH 2/6] ha: delay discovery until next loop - stack-like discovery struct to store pending mqtt topic and message - use separate json objects for sensor and switch data (different solution for the #1957) --- code/espurna/config/prototypes.h | 1 + code/espurna/homeassistant.ino | 164 ++++++++++++++++++++++++------- 2 files changed, 131 insertions(+), 34 deletions(-) diff --git a/code/espurna/config/prototypes.h b/code/espurna/config/prototypes.h index 3faf1945..27f7f7c1 100644 --- a/code/espurna/config/prototypes.h +++ b/code/espurna/config/prototypes.h @@ -196,6 +196,7 @@ void i2c_read_buffer(uint8_t address, uint8_t * buffer, size_t len); #endif using mqtt_callback_f = std::function; +using mqtt_msg_t = std::pair; void mqttRegister(mqtt_callback_f callback); diff --git a/code/espurna/homeassistant.ino b/code/espurna/homeassistant.ino index 49798791..436abea2 100644 --- a/code/espurna/homeassistant.ino +++ b/code/espurna/homeassistant.ino @@ -8,10 +8,12 @@ Copyright (C) 2017-2019 by Xose Pérez #if HOMEASSISTANT_SUPPORT +#include +#include #include -bool _haEnabled = false; -bool _haSendFlag = false; +bool _ha_enabled = false; +bool _ha_send_flag = false; // ----------------------------------------------------------------------------- // UTILS @@ -52,6 +54,10 @@ const String switchType("light"); const String switchType("switch"); #endif +// ----------------------------------------------------------------------------- +// Shared context object to store entity and entity registry data +// ----------------------------------------------------------------------------- + struct ha_config_t { static const size_t DEFAULT_BUFFER_SIZE = 2048; @@ -84,6 +90,87 @@ struct ha_config_t { const String version; }; +// ----------------------------------------------------------------------------- +// MQTT discovery +// ----------------------------------------------------------------------------- + +struct ha_discovery_t { + + constexpr static const unsigned long SEND_TIMEOUT = 1000; + + ha_discovery_t() { + #if SENSOR_SUPPORT + _messages.reserve(magnitudeCount() + relayCount()); + #else + _messages.reserve(relayCount()); + #endif + } + + // TODO: is this expected behaviour? + void add(String& topic, String& message) { + _messages.emplace_back(std::move(topic), std::move(message)); + } + + // We don't particulary care about the order since names have indexes? + // If we ever do, use iterators to reference elems and pop the String contents instead + mqtt_msg_t& next() { + return _messages.back(); + } + + void pop() { + _messages.pop_back(); + } + + const bool empty() const { + return !_messages.size(); + } + + void prepareSwitches(ha_config_t& config); + #if SENSOR_SUPPORT + void prepareMagnitudes(ha_config_t& config); + #endif + + Ticker timer; + std::vector _messages; + +}; + +std::unique_ptr _ha_discovery = nullptr; + +void _haSendDiscovery() { + + if (!_ha_discovery) return; + + if (_ha_discovery->empty()) { + return; + } + + const unsigned long ts = millis(); + do { + if (_ha_discovery->empty()) break; + + auto& message = _ha_discovery->next(); + if (!mqttSendRaw(message.first.c_str(), message.second.c_str())) { + break; + } + _ha_discovery->pop(); + // XXX: should not reach this timeout, most common case is the break above + } while (millis() - ts < ha_discovery_t::SEND_TIMEOUT); + + mqttSendStatus(); + + if (_ha_discovery->empty()) { + _ha_discovery = nullptr; + } else { + // 2.3.0: Ticker callback arguments are not preserved and once_ms_scheduled is missing + // We need to use global discovery object to reschedule it + // Otherwise, this would've been shared_ptr from _haSend + _ha_discovery->timer.once_ms(ha_discovery_t::SEND_TIMEOUT, []() { + schedule_function(_haSendDiscovery); + }); + } + +} // ----------------------------------------------------------------------------- // SENSORS @@ -99,7 +186,10 @@ void _haSendMagnitude(unsigned char i, JsonObject& config) { config["unit_of_measurement"] = magnitudeUnits(type); } -void _haSendMagnitudes(ha_config_t& config) { +void ha_discovery_t::prepareMagnitudes(ha_config_t& config) { + + // Note: because none of the keys are erased, use a separate object to avoid accidentally sending switch data + JsonObject& root = config.jsonBuffer.createObject(); for (unsigned char i=0; i(); + + // Prepare all of the messages and send them in the scheduled function later + _ha_discovery->prepareSwitches(config); #if SENSOR_SUPPORT - _haSendMagnitudes(config); + _ha_discovery->prepareMagnitudes(config); #endif - _haSendFlag = false; + _ha_send_flag = false; + schedule_function(_haSendDiscovery); } void _haConfigure() { - bool enabled = getSetting("haEnabled", HOMEASSISTANT_ENABLED).toInt() == 1; - _haSendFlag = (enabled != _haEnabled); - _haEnabled = enabled; + const bool enabled = getSetting("haEnabled", HOMEASSISTANT_ENABLED).toInt() == 1; + _ha_send_flag = (enabled != _ha_enabled); + _ha_enabled = enabled; _haSend(); } @@ -430,7 +526,7 @@ void haSetup() { // On MQTT connect check if we have something to send mqttRegister([](unsigned int type, const char * topic, const char * payload) { if (type == MQTT_CONNECT_EVENT) _haSend(); - if (type == MQTT_DISCONNECT_EVENT) _haSendFlag = false; + if (type == MQTT_DISCONNECT_EVENT) _ha_send_flag = false; }); // Main callbacks From 7adedc8992196c792af8c744a2bbb1ccc0987e30 Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Fri, 1 Nov 2019 23:16:05 +0300 Subject: [PATCH 3/6] ws: use std::pair instead of custom struct --- code/espurna/ws.ino | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/code/espurna/ws.ino b/code/espurna/ws.ino index 132a596c..51087464 100644 --- a/code/espurna/ws.ino +++ b/code/espurna/ws.ino @@ -204,13 +204,7 @@ bool _wsAuth(AsyncWebSocketClient * client) { #if DEBUG_WEB_SUPPORT -struct ws_debug_msg_t { - ws_debug_msg_t(const char* prefix, const char* message) : - prefix(prefix), message(message) - {} - String prefix; - String message; -}; +using ws_debug_msg_t = std::pair; struct ws_debug_t { @@ -257,8 +251,8 @@ struct ws_debug_t { JsonArray& pre = weblog.createNestedArray("pre"); for (auto& message : messages) { - pre.add(message.prefix.c_str()); - msg.add(message.message.c_str()); + pre.add(message.first.c_str()); + msg.add(message.second.c_str()); } wsSend(root); From 9956cd835b4f5561fcb43b1d24e810156f4ea208 Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Fri, 1 Nov 2019 23:18:57 +0300 Subject: [PATCH 4/6] ha: send discovery on reconnect --- code/espurna/homeassistant.ino | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code/espurna/homeassistant.ino b/code/espurna/homeassistant.ino index 436abea2..4155526c 100644 --- a/code/espurna/homeassistant.ino +++ b/code/espurna/homeassistant.ino @@ -526,7 +526,7 @@ void haSetup() { // On MQTT connect check if we have something to send mqttRegister([](unsigned int type, const char * topic, const char * payload) { if (type == MQTT_CONNECT_EVENT) _haSend(); - if (type == MQTT_DISCONNECT_EVENT) _ha_send_flag = false; + if (type == MQTT_DISCONNECT_EVENT) _ha_send_flag = _ha_enabled; }); // Main callbacks From e8acf33a69478a30a7694c502a5ece5e3629ab7a Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Fri, 1 Nov 2019 23:43:48 +0300 Subject: [PATCH 5/6] ha: more cancelation reasons --- code/espurna/homeassistant.ino | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/code/espurna/homeassistant.ino b/code/espurna/homeassistant.ino index 4155526c..93379a73 100644 --- a/code/espurna/homeassistant.ino +++ b/code/espurna/homeassistant.ino @@ -97,8 +97,11 @@ struct ha_config_t { struct ha_discovery_t { constexpr static const unsigned long SEND_TIMEOUT = 1000; + constexpr static const unsigned char SEND_RETRY = 5; - ha_discovery_t() { + ha_discovery_t() : + _retry(SEND_RETRY) + { #if SENSOR_SUPPORT _messages.reserve(magnitudeCount() + relayCount()); #else @@ -106,6 +109,10 @@ struct ha_discovery_t { #endif } + ~ha_discovery_t() { + DEBUG_MSG_P(PSTR("[HA] Discovery %s\n"), empty() ? "OK" : "FAILED"); + } + // TODO: is this expected behaviour? void add(String& topic, String& message) { _messages.emplace_back(std::move(topic), std::move(message)); @@ -125,6 +132,11 @@ struct ha_discovery_t { return !_messages.size(); } + bool retry() { + if (!_retry) return false; + return --_retry; + } + void prepareSwitches(ha_config_t& config); #if SENSOR_SUPPORT void prepareMagnitudes(ha_config_t& config); @@ -132,6 +144,7 @@ struct ha_discovery_t { Ticker timer; std::vector _messages; + unsigned char _retry; }; @@ -141,7 +154,12 @@ void _haSendDiscovery() { if (!_ha_discovery) return; - if (_ha_discovery->empty()) { + const bool connected = mqttConnected(); + const bool retry = _ha_discovery->retry(); + const bool empty = _ha_discovery->empty(); + + if (!connected || !retry || empty) { + _ha_discovery = nullptr; return; } From c414ba1251fe5f1e01e85f251f5f864bb4756221 Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Fri, 1 Nov 2019 23:47:55 +0300 Subject: [PATCH 6/6] ha: log autodiscovery -> discovery --- code/espurna/homeassistant.ino | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code/espurna/homeassistant.ino b/code/espurna/homeassistant.ino index 93379a73..f551644d 100644 --- a/code/espurna/homeassistant.ino +++ b/code/espurna/homeassistant.ino @@ -407,7 +407,7 @@ void _haSend() { // Are we still trying to send discovery messages? if (_ha_discovery) return; - DEBUG_MSG_P(PSTR("[HA] Sending autodiscovery MQTT message\n")); + DEBUG_MSG_P(PSTR("[HA] Preparing MQTT discovery message(s)...\n")); // Get common device config / context object ha_config_t config;