Fix MQTT HA auto-discovery, add MQTT LWT, expose current pressure and migrate to MQTT async library #390
Conversation
WalkthroughThe MQTT plugin is replaced with a new architecture using PsychicMqttClient instead of the legacy MQTT library. The new implementation adds asynchronous event-driven lifecycle management, Home Assistant auto-discovery support, per-topic debouncing, and heartbeat publishing. Public API is simplified to Changes
Sequence DiagramsequenceDiagram
participant Setup as Setup Phase
participant WiFi as WiFi Monitor
participant MQTT as MQTT Client
participant Controller as Controller
participant HA as Home Assistant
Setup->>Setup: configureFromSettings()
Setup->>Setup: Compute identity from MAC
loop Connection Ready Check
WiFi->>WiFi: Check WiFi status
WiFi->>MQTT: connectIfReady() if ready
end
alt WiFi Connected & Broker Set
MQTT->>MQTT: Initialize PsychicMqttClient
MQTT->>MQTT: Connect to broker
end
MQTT->>MQTT: onConnected()
MQTT->>MQTT: Publish online status (retained)
MQTT->>MQTT: publishDiscovery()
MQTT->>HA: Send discovery payload
MQTT->>MQTT: subscribeHAStatusOnce()
MQTT->>HA: Subscribe to HA status topic
loop Heartbeat & Sensor Updates
Controller->>MQTT: Sensor updates (temp, pressure)
MQTT->>MQTT: Check debounce intervals
alt Debounce elapsed
MQTT->>MQTT: publish() to relative topic
end
alt Heartbeat interval exceeded
MQTT->>MQTT: Publish heartbeat JSON
end
end
alt Disconnection
MQTT->>MQTT: onDisconnected()
MQTT->>MQTT: AutoReconnect behavior
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The refactor involves substantial architectural changes: migration to a new MQTT client library with different API semantics, new event-driven lifecycle management, Home Assistant discovery payload generation, debouncing state machines for multiple sensors, and identity computation from MAC addresses. The changes span multiple files and introduce non-trivial logic across configuration, connection management, publishing policies, and state tracking. While the pattern is cohesive, the density of new functionality and interdependencies between lifecycle methods require careful verification of correctness and integration points. Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/display/plugins/MQTTPlugin.cpp (1)
388-467: Optional: reduce heap churn in HA discovery JSON.
JsonDocument+String payloadcauses dynamic allocations on each publish. ConsiderStaticJsonDocument<N>with a sized buffer andserializeJson(doc, client)into a stackchar buf[], or reuse oneStaticJsonDocumentacross entities. This keeps heap pressure low on ESP32.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
platformio.ini(1 hunks)src/display/plugins/MQTTPlugin.cpp(2 hunks)src/display/plugins/MQTTPlugin.h(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/display/plugins/MQTTPlugin.h (3)
src/display/core/Plugin.h (1)
Plugin(6-12)src/display/plugins/MQTTPlugin.cpp (25)
setup(118-242)setup(118-118)controller(30-30)controller(35-35)controller(42-42)loop(244-257)loop(244-244)configureFromSettings(260-302)configureFromSettings(260-260)connectIfReady(304-329)connectIfReady(304-304)onConnected(331-338)onConnected(331-331)onDisconnected(340-342)onDisconnected(340-340)subscribeHAStatusOnce(344-359)subscribeHAStatusOnce(344-344)publishDiscovery(362-468)publishDiscovery(362-362)publish(470-476)publish(470-470)relativeTopic(43-43)publishBrewState(478-485)publishBrewState(478-478)state(44-44)src/display/core/Controller.cpp (4)
setup(25-73)setup(25-25)loop(203-283)loop(203-203)
src/display/plugins/MQTTPlugin.cpp (3)
src/display/core/Controller.h (1)
pluginManager(128-128)src/display/plugins/WebUIPlugin.cpp (4)
setup(19-46)setup(19-19)loop(48-119)loop(48-48)src/display/core/Controller.cpp (4)
setup(25-73)setup(25-25)loop(203-283)loop(203-203)
🪛 Clang (14.0.6)
src/display/plugins/MQTTPlugin.h
[error] 5-5: 'PsychicMqttClient.h' file not found
(clang-diagnostic-error)
src/display/plugins/MQTTPlugin.cpp
[error] 5-5: 'PsychicMqttClient.h' file not found
(clang-diagnostic-error)
🔇 Additional comments (3)
platformio.ini (1)
57-57: Confirm build uses PlatformIO; header is correct per upstream.The include name
<PsychicMqttClient.h>is correct per the library docs; the Clang “file not found” is likely due to analyzing without PlatformIO’s dependency resolution. Ensure CI compiles via PlatformIO (pio run) or configures the include paths accordingly. (github.com)If CI uses a plain compiler step, consider switching to a PlatformIO job or adding the library’s include path to the analyzer.
src/display/plugins/MQTTPlugin.cpp (2)
244-257: Heartbeat is fine; note it’s not required for MQTT keepalive.The library handles PINGREQ per
setKeepAlive(). Heartbeat is useful for consumers but not necessary to keep the connection alive. No change required; just a heads‑up to avoid assuming it replaces MQTT ping.Confirm that consumers (e.g., HA automations) actually use
.../controller/heartbeat; otherwise consider removing to save traffic.
349-359: HA re‑announce on status works; ensure re‑subscribe semantics are library‑backed.
onTopic()is used insideonConnected(), which is correct if the library re‑subscribes on reconnect. PsychicMqttClient’s docs show#include <PsychicMqttClient.h>and demonstrateonTopic()usage; confirm it persists across reconnects as expected. (github.com)
| #ifndef MQTTPLUGIN_H | ||
| #define MQTTPLUGIN_H | ||
|
|
||
| #include "../core/Plugin.h" | ||
| #include <PsychicMqttClient.h> | ||
| #include <WiFi.h> | ||
|
|
||
| // Debounce for current temperature (QoS 0) | ||
| constexpr uint32_t TEMP_MIN_INTERVAL_MS = 500; | ||
| constexpr float TEMP_MIN_DELTA_C = 0.20f; | ||
|
|
||
| // Debounce for current pressure (QoS 0) | ||
| constexpr uint32_t PRESSURE_MIN_INTERVAL_MS = 500; | ||
| constexpr float PRESSURE_MIN_DELTA_BAR = 0.10f; | ||
|
|
||
| // --- MQTT keepalive & heartbeat policy --- | ||
| // KeepAlive is set to 5 seconds at the client level. | ||
| // We publish a heartbeat more frequently than that to guarantee activity. | ||
| constexpr uint32_t MQTT_KEEPALIVE_S = 5; | ||
|
|
||
| // Heartbeat period = keepalive - margin (margin = 1500 ms). | ||
| // That gives us 3.5 s, safely < 5 s even with some scheduler jitter. | ||
| constexpr uint32_t HEARTBEAT_MARGIN_MS = 1500; | ||
| constexpr uint32_t HEARTBEAT_PERIOD_MS = (MQTT_KEEPALIVE_S * 1000 > HEARTBEAT_MARGIN_MS + 1000) | ||
| ? (MQTT_KEEPALIVE_S * 1000 - HEARTBEAT_MARGIN_MS) | ||
| : 1000; // never go below 1s even if someone tweaks keepalive smaller | ||
|
|
||
| class MQTTPlugin : public Plugin { | ||
| public: | ||
| void setup(Controller *controller, PluginManager *pluginManager) override; | ||
| void loop() override; | ||
|
|
||
| private: | ||
| // lifecycle | ||
| void configureFromSettings(Controller *controller); | ||
| void connectIfReady(); | ||
| void onConnected(); | ||
| void onDisconnected(); | ||
| void subscribeHAStatusOnce(); | ||
|
|
||
| // HA helpers | ||
| void publishDiscovery(Controller *controller); | ||
| void publish(const char *relativeTopic, const char *json, int qos, bool retain = false); | ||
| void publishBrewState(const char *state); | ||
|
|
||
| // identity & topics | ||
| String macUnderscore_; | ||
| String clientId_; // gaggimate_<MAC_> | ||
| String baseTopic_; // gaggimate/<MAC_>/ | ||
| String statusTopic_; // .../status | ||
| String heartbeatTopic_; // .../controller/heartbeat | ||
| String discoveryPrefix_; // default "homeassistant" | ||
|
|
||
| // broker settings (cached at boot) | ||
| String brokerHost_; | ||
| uint16_t brokerPort_ = 1883; | ||
| String username_; | ||
| String password_; | ||
| bool mqttEnabled_ = true; | ||
|
|
||
| // Must persist the URI string we pass to setServer() | ||
| String mqttUri_; | ||
|
|
||
| // state/flags | ||
| Controller *ctrl_ = nullptr; | ||
| bool haStatusSubscribed_ = false; | ||
| bool clientConfigured_ = false; | ||
|
|
||
| // capability cache | ||
| bool hasPressure_ = false; | ||
|
|
||
| // debouncing | ||
| uint32_t lastTempPublishMs_ = 0; | ||
| float lastTemperatureSent_ = NAN; | ||
|
|
||
| uint32_t lastPressurePublishMs_ = 0; | ||
| float lastPressureSent_ = NAN; | ||
|
|
||
| // heartbeat | ||
| uint32_t lastAnyPublishMs_ = 0; | ||
|
|
||
| // async MQTT | ||
| PsychicMqttClient mqtt_; | ||
| }; | ||
|
|
||
| #endif // MQTTPLUGIN_H | ||
|
|
There was a problem hiding this comment.
Remove duplicated header and class from the .cpp (ODR violation, conflicting constants).
The .cpp starts with a full copy of the header (include guards, constants, class). This duplicates the type definition and conflicts with MQTTPlugin.h (e.g., different debounce values), risking ODR violations and undefined behavior. Keep the declaration only in the header; the .cpp should only include the header and provide definitions.
Apply this minimal fix:
-#ifndef MQTTPLUGIN_H
-#define MQTTPLUGIN_H
-#include "../core/Plugin.h"
-#include <PsychicMqttClient.h>
-#include <WiFi.h>
-// Debounce for current temperature (QoS 0)
-constexpr uint32_t TEMP_MIN_INTERVAL_MS = 500;
-constexpr float TEMP_MIN_DELTA_C = 0.20f;
-// Debounce for current pressure (QoS 0)
-constexpr uint32_t PRESSURE_MIN_INTERVAL_MS = 500;
-constexpr float PRESSURE_MIN_DELTA_BAR = 0.10f;
-// --- MQTT keepalive & heartbeat policy ---
-constexpr uint32_t MQTT_KEEPALIVE_S = 5;
-constexpr uint32_t HEARTBEAT_MARGIN_MS = 1500;
-constexpr uint32_t HEARTBEAT_PERIOD_MS = (MQTT_KEEPALIVE_S * 1000 > HEARTBEAT_MARGIN_MS + 1000)
- ? (MQTT_KEEPALIVE_S * 1000 - HEARTBEAT_MARGIN_MS)
- : 1000;
-class MQTTPlugin : public Plugin {
- public:
- void setup(Controller *controller, PluginManager *pluginManager) override;
- void loop() override;
- private:
- // lifecycle
- void configureFromSettings(Controller *controller);
- void connectIfReady();
- void onConnected();
- void onDisconnected();
- void subscribeHAStatusOnce();
- // HA helpers
- void publishDiscovery(Controller *controller);
- void publish(const char *relativeTopic, const char *json, int qos, bool retain = false);
- void publishBrewState(const char *state);
- // identity & topics
- String macUnderscore_;
- String clientId_;
- String baseTopic_;
- String statusTopic_;
- String heartbeatTopic_;
- String discoveryPrefix_;
- // broker settings
- String brokerHost_;
- uint16_t brokerPort_ = 1883;
- String username_;
- String password_;
- bool mqttEnabled_ = true;
- String mqttUri_;
- // state/flags
- Controller *ctrl_ = nullptr;
- bool haStatusSubscribed_ = false;
- bool clientConfigured_ = false;
- // capability cache
- bool hasPressure_ = false;
- // debouncing
- uint32_t lastTempPublishMs_ = 0;
- float lastTemperatureSent_ = NAN;
- uint32_t lastPressurePublishMs_ = 0;
- float lastPressureSent_ = NAN;
- // heartbeat
- uint32_t lastAnyPublishMs_ = 0;
- // async MQTT
- PsychicMqttClient mqtt_;
-};
-#endif // MQTTPLUGIN_H
-
-// ======================= IMPLEMENTATION =======================
+#include "MQTTPlugin.h"After this, the .cpp will use the single set of constants from the header.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #ifndef MQTTPLUGIN_H | |
| #define MQTTPLUGIN_H | |
| #include "../core/Plugin.h" | |
| #include <PsychicMqttClient.h> | |
| #include <WiFi.h> | |
| // Debounce for current temperature (QoS 0) | |
| constexpr uint32_t TEMP_MIN_INTERVAL_MS = 500; | |
| constexpr float TEMP_MIN_DELTA_C = 0.20f; | |
| // Debounce for current pressure (QoS 0) | |
| constexpr uint32_t PRESSURE_MIN_INTERVAL_MS = 500; | |
| constexpr float PRESSURE_MIN_DELTA_BAR = 0.10f; | |
| // --- MQTT keepalive & heartbeat policy --- | |
| // KeepAlive is set to 5 seconds at the client level. | |
| // We publish a heartbeat more frequently than that to guarantee activity. | |
| constexpr uint32_t MQTT_KEEPALIVE_S = 5; | |
| // Heartbeat period = keepalive - margin (margin = 1500 ms). | |
| // That gives us 3.5 s, safely < 5 s even with some scheduler jitter. | |
| constexpr uint32_t HEARTBEAT_MARGIN_MS = 1500; | |
| constexpr uint32_t HEARTBEAT_PERIOD_MS = (MQTT_KEEPALIVE_S * 1000 > HEARTBEAT_MARGIN_MS + 1000) | |
| ? (MQTT_KEEPALIVE_S * 1000 - HEARTBEAT_MARGIN_MS) | |
| : 1000; // never go below 1s even if someone tweaks keepalive smaller | |
| class MQTTPlugin : public Plugin { | |
| public: | |
| void setup(Controller *controller, PluginManager *pluginManager) override; | |
| void loop() override; | |
| private: | |
| // lifecycle | |
| void configureFromSettings(Controller *controller); | |
| void connectIfReady(); | |
| void onConnected(); | |
| void onDisconnected(); | |
| void subscribeHAStatusOnce(); | |
| // HA helpers | |
| void publishDiscovery(Controller *controller); | |
| void publish(const char *relativeTopic, const char *json, int qos, bool retain = false); | |
| void publishBrewState(const char *state); | |
| // identity & topics | |
| String macUnderscore_; | |
| String clientId_; // gaggimate_<MAC_> | |
| String baseTopic_; // gaggimate/<MAC_>/ | |
| String statusTopic_; // .../status | |
| String heartbeatTopic_; // .../controller/heartbeat | |
| String discoveryPrefix_; // default "homeassistant" | |
| // broker settings (cached at boot) | |
| String brokerHost_; | |
| uint16_t brokerPort_ = 1883; | |
| String username_; | |
| String password_; | |
| bool mqttEnabled_ = true; | |
| // Must persist the URI string we pass to setServer() | |
| String mqttUri_; | |
| // state/flags | |
| Controller *ctrl_ = nullptr; | |
| bool haStatusSubscribed_ = false; | |
| bool clientConfigured_ = false; | |
| // capability cache | |
| bool hasPressure_ = false; | |
| // debouncing | |
| uint32_t lastTempPublishMs_ = 0; | |
| float lastTemperatureSent_ = NAN; | |
| uint32_t lastPressurePublishMs_ = 0; | |
| float lastPressureSent_ = NAN; | |
| // heartbeat | |
| uint32_t lastAnyPublishMs_ = 0; | |
| // async MQTT | |
| PsychicMqttClient mqtt_; | |
| }; | |
| #endif // MQTTPLUGIN_H | |
| #include "MQTTPlugin.h" |
🧰 Tools
🪛 Clang (14.0.6)
[error] 5-5: 'PsychicMqttClient.h' file not found
(clang-diagnostic-error)
🤖 Prompt for AI Agents
In src/display/plugins/MQTTPlugin.cpp around lines 1 to 87, the file currently
contains a full copy of the header (include guards, constants and the MQTTPlugin
class) which duplicates MQTTPlugin.h and causes ODR/constant conflicts; remove
the entire duplicated header/class block from the top of the .cpp, replace it
with a single #include "MQTTPlugin.h" (or the correct relative include), and
leave only the method/function definitions in the .cpp so the program uses the
single declaration and constants from the header.
| // state/flags | ||
| Controller *ctrl_ = nullptr; | ||
| bool haStatusSubscribed_ = false; | ||
| bool clientConfigured_ = false; | ||
|
|
||
| // debouncing | ||
| uint32_t lastTempPublishMs_ = 0; | ||
| float lastTemperatureSent_ = NAN; | ||
|
|
||
| uint32_t lastPressurePublishMs_ = 0; | ||
| float lastPressureSent_ = NAN; | ||
|
|
||
| // heartbeat | ||
| uint32_t lastAnyPublishMs_ = 0; | ||
|
|
||
| float lastTemperature = 0; | ||
| // async MQTT | ||
| PsychicMqttClient mqtt_; | ||
| }; |
There was a problem hiding this comment.
Add missing member: hasPressure_ to match implementation.
hasPressure_ is used in MQTTPlugin.cpp but not declared here. Declare it in the header so the class layout matches and compilation doesn’t rely on the accidental class redefinition in the .cpp.
Apply this diff near the other state fields:
// state/flags
Controller *ctrl_ = nullptr;
bool haStatusSubscribed_ = false;
bool clientConfigured_ = false;
+ // capability cache
+ bool hasPressure_ = false;🤖 Prompt for AI Agents
In src/display/plugins/MQTTPlugin.h around lines 64 to 81, the header is missing
the hasPressure_ member used in MQTTPlugin.cpp; add a boolean member declaration
(e.g., bool hasPressure_ = false;) alongside the other state/flags (near ctrl_,
haStatusSubscribed_, clientConfigured_) so the class layout matches the
implementation and the code compiles.
1 participant
First of all I am no programer and this is my first bigger PR to any project. Honestly most of it was done with AI. I hope the code is good enough but in case it is trash feel free to reject it. I am only trying to improve what is bugging me in my capacity and I am sorry in case this is utter trash that wastes your time.
Motivation:
Main features:
Summary by CodeRabbit