Merge branch 'master' of https://github.com/tobiasguyer/gaggimate into all_features

Author: tobiasguyer

.clabot

@@ -35,7 +35,8 @@
     "szymonmilewski",
     "stephenwilley",
     "Marbit911",
-    "khanguyen74"
+    "khanguyen74",
+    "Clinteastman"
   ],
   "message": "We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added."
 }

lib/NanoPbComm/src/Endpoint.cpp

@@ -8,7 +8,7 @@ static const char *ENDPOINT_TAG = "Endpoint";
 
 Endpoint::Endpoint(Transport &transport) : _transport(transport) {
     _mutex = xSemaphoreCreateRecursiveMutex();
-    _rxQueue = xQueueCreate(RX_QUEUE_DEPTH, sizeof(gm::Payload));
+    _rxQueue = xQueueCreate(RX_QUEUE_DEPTH, sizeof(DispatchEvent));
     if (_mutex == nullptr || _rxQueue == nullptr)
         ESP_LOGE(ENDPOINT_TAG, "Failed to allocate endpoint resources (out of memory)");
 }
@@ -42,10 +42,16 @@ void Endpoint::begin() {
 
 void Endpoint::dispatchTaskFn(void *arg) {
     auto *self = static_cast<Endpoint *>(arg);
-    gm::Payload payload;
+    DispatchEvent event;
     for (;;) {
-        if (xQueueReceive(self->_rxQueue, &payload, portMAX_DELAY) == pdTRUE)
-            self->dispatch(payload);
+        if (xQueueReceive(self->_rxQueue, &event, portMAX_DELAY) != pdTRUE)
+            continue;
+        if (event.isConnection) {
+            if (self->_connHandler)
+                self->_connHandler(event.connected);
+        } else {
+            self->dispatch(event.payload);
+        }
     }
 }
 
@@ -232,8 +238,11 @@ void Endpoint::handleData(const uint8_t *data, size_t length) {
         if (static_cast<pb_size_t>(uxQueueSpacesAvailable(_rxQueue)) < n) {
             accepted = false;
         } else {
-            for (pb_size_t i = 0; i < n; i++)
-                xQueueSend(_rxQueue, &_rxFrame.payloads[i], 0);
+            for (pb_size_t i = 0; i < n; i++) {
+                DispatchEvent event;
+                event.payload = _rxFrame.payloads[i];
+                xQueueSend(_rxQueue, &event, 0);
+            }
         }
     }
 
@@ -264,9 +273,15 @@ void Endpoint::handleConnection(bool connected) {
     _queue.clear();
     unlock();
 
-    if (_rxQueue)
-        xQueueReset(_rxQueue); // drop any inbound payloads from a previous session
-
-    if (_connHandler)
-        _connHandler(connected); // e.g. push SystemInfo on connect
+    if (_rxQueue) {
+        // Drop queued payloads from the previous session, then serialize the
+        // application connection callback with payload dispatch. A payload
+        // already executing finishes before this event, so it cannot mutate
+        // per-session application state after the callback resets it.
+        xQueueReset(_rxQueue);
+        DispatchEvent event;
+        event.isConnection = true;
+        event.connected = connected;
+        xQueueSend(_rxQueue, &event, 0);
+    }
 }

lib/NanoPbComm/src/Endpoint.h

@@ -27,9 +27,11 @@
  *     typed handlers -- no run-time type erasure.
  *
  * Threading: decode + ACK/dedup + the send pump run on the transport's callback
- * thread, but registered handlers are invoked on a dedicated dispatch task (fed
- * by an inbound payload queue) so slow application callbacks never block the BLE
- * host task. If the inbound queue is full the frame is left un-ACKed, which
+ * thread, but registered handlers and connection callbacks are invoked on a
+ * dedicated dispatch task (fed by an inbound event queue) so slow application
+ * callbacks never block the BLE host task. Serializing both event types also
+ * prevents payload handlers from crossing a connection-session boundary. If
+ * the inbound queue is full the frame is left un-ACKed, which
  * back-pressures the sender into retransmitting. Queue + in-flight state are
  * guarded by a mutex; handlers run with the mutex released, so a handler may
  * call send() re-entrantly.
@@ -122,8 +124,16 @@ class Endpoint {
 
     ConnectionHandler _connHandler = nullptr;
 
-    // Inbound payloads decoded on the transport thread, drained by the dispatch
-    // task so handlers never run on the BLE host task.
+    struct DispatchEvent {
+        gm::Payload payload{};
+        bool isConnection = false;
+        bool connected = false;
+    };
+
+    // Inbound payloads and connection changes originate on the transport
+    // thread, then drain through one dispatch task. A connection change resets
+    // the queue before its event is inserted, so no queued payload from the old
+    // session can run after the new session callback.
     QueueHandle_t _rxQueue = nullptr;
     TaskHandle_t _dispatchTask = nullptr;
 

lib/NanoPbComm/src/GaggiMateClient.h

@@ -7,15 +7,8 @@
 #include <Arduino.h>
 #include <functional>
 
-/**
- * Display-side protocol facade.
- *
- * Owns a BLE client transport + Endpoint and exposes semantic send methods and
- * typed response callbacks. The connect sequence is asynchronous: the link is
- * established via connectToServer(), and the controller's SystemInfo arrives as
- * a pushed message (onSystemInfo) which is when capability-dependent setup
- * should run.
- */
+// Display-side protocol facade: owns transport + Endpoint, exposes semantic sends and typed response callbacks.
+// Connecting is async: capability-dependent setup belongs in onSystemInfo, pushed by the controller after connect.
 class GaggiMateClient {
   public:
     using ConnectionCallback = std::function<void(bool connected)>;
@@ -43,19 +36,18 @@ class GaggiMateClient {
     bool isConnected() const { return _endpoint.isConnected(); }
     void disconnect() { _transport.disconnect(); }
 
-    // BLE round-trip latency (ms) measured by the reliability layer (send -> ACK).
-    // EWMA-smoothed; refreshed at least every ~2s by the keep-alive ping plus on
-    // every control update. hasLatency() is false until the first ACK of a link.
+    // Forget the paired controller so the display can pair to a different one.
+    void clearBonds() { _transport.clearBonds(); }
+
+    // EWMA-smoothed send->ACK round-trip (ms); hasLatency() is false until the first ACK of a link.
     uint32_t getLatencyMs() const { return _endpoint.latencyMs(); }
     uint32_t getLastLatencyMs() const { return _endpoint.lastLatencyMs(); }
     bool hasLatency() const { return _endpoint.hasLatency(); }
 
-    // Tight connection interval (responsive control) while active; relaxed when
-    // idle to give the shared radio back to Wi-Fi.
+    // Tight connection interval while active; relaxed when idle to give the shared radio back to Wi-Fi.
     void setLowLatency(bool active) { _transport.setLowLatency(active); }
 
-    // Native NimBLE client handle, used by ControllerOTA / status RSSI (OTA uses
-    // its own BLE service, independent of this protocol).
+    // Native NimBLE client handle for ControllerOTA / status RSSI (OTA uses its own BLE service).
     NimBLEClient *getClient() const { return _transport.getNativeClient(); }
 
     // Build a payload without sending (compose your own batch, then send()).
@@ -69,8 +61,7 @@ class GaggiMateClient {
     gm::Payload buildAutotune(uint32_t testTime, uint32_t samples, uint32_t heaterWattage);
     gm::Payload buildPressureScale(float scale);
     gm::Payload buildTare();
-    // Pack channel/brightness pairs into one LedControl payload; entries beyond
-    // the schema's per-message cap (LedControl.channels max_count) are dropped.
+    // Pack channel/brightness pairs into one LedControl payload; entries beyond the schema's max_count are dropped.
     gm::Payload buildLedControl(const LedChannelCommand *channels, size_t count);
 
     // Commands (display -> controller)
@@ -85,17 +76,14 @@ class GaggiMateClient {
     void sendPressureScale(float scale);
     void sendThermostatControl(float boilerLowPass, float groupLowPass);
     void tare();
-    // Drive several LED channels in one message (avoids per-channel sends that
-    // the outbound queue would coalesce down to a single channel).
+    // Drive several LED channels in one message; per-channel sends would coalesce down to a single channel.
     void sendLedControl(const LedChannelCommand *channels, size_t count);
 
-    // Send a pre-built payload / batch of payloads (one frame). Compose batches
-    // from build*() helpers -- e.g. the display's delta-based control update.
+    // Send a pre-built payload / batch of payloads (one frame), composed from the build*() helpers.
     void send(const gm::Payload &payload) { _endpoint.send(payload); }
     void sendBatch(const gm::Payload *payloads, size_t count) { _endpoint.sendBatch(payloads, count); }
 
-    // Fired when the connected controller is missing the framed-comms
-    // characteristics (old / incompatible firmware); link is kept for OTA.
+    // Fired when the controller lacks the framed-comms characteristics (old firmware); link is kept for OTA.
     void onIncompatibleController(IncompatibleCallback cb) { _incompatibleCb = std::move(cb); }
 
     // Response registrations (controller -> display)

lib/NanoPbComm/src/GaggiMateComm.h

@@ -3,8 +3,7 @@
 
 #include <cstdint>
 
-// Public protocol vocabulary shared by GaggiMateClient and GaggiMateServer.
-// Firmware code only ever sees these plain types -- never the nanopb structs.
+// Public protocol vocabulary shared by GaggiMateClient and GaggiMateServer; firmware never sees the nanopb structs.
 
 // Pump control mode. Integer values match gaggimate_PumpMode in the schema.
 enum class PumpControlMode : uint8_t {
@@ -19,9 +18,7 @@ enum class BoilerControlMode : uint8_t {
     Pressure = 1,    // setpoint is a target pressure in bar
 };
 
-// Per-component commands, used to drive several components atomically in one
-// frame and to detect changes (callers can compare against the last value sent
-// and only transmit what actually changed).
+// Per-component commands: drive several components atomically in one frame, comparable so callers send only deltas.
 struct BoilerCommand {
     uint8_t index = 0;
     BoilerControlMode mode = BoilerControlMode::Temperature;
@@ -46,27 +43,22 @@ struct RelayCommand {
     bool operator==(const RelayCommand &o) const { return index == o.index && open == o.open; }
     bool operator!=(const RelayCommand &o) const { return !(*this == o); }
 };
-// One LED output's target brightness. Several are packed into a single
-// LedControl message so a multi-channel update can't be split (and coalesced)
-// into separate frames.
+// One LED output's target brightness; packed into a single LedControl message so multi-channel updates can't be split.
 struct LedChannelCommand {
     uint8_t channel = 0;
     uint8_t brightness = 0;
     bool operator==(const LedChannelCommand &o) const { return channel == o.channel && brightness == o.brightness; }
     bool operator!=(const LedChannelCommand &o) const { return !(*this == o); }
 };
 
-// Error codes. Values match the gaggimate.ErrorCode enum and the codes the old
-// string protocol used, so existing firmware comparisons keep working.
+// Error codes; values match gaggimate.ErrorCode and the old string protocol so existing comparisons keep working.
 constexpr int ERROR_CODE_NONE = 0;
 constexpr int ERROR_CODE_COMM_SEND = 1;
 constexpr int ERROR_CODE_COMM_RCV = 2;
 constexpr int ERROR_CODE_PROTO_ERR = 3;
 constexpr int ERROR_CODE_RUNAWAY = 4;
 constexpr int ERROR_CODE_TIMEOUT = 5;
-// Autotune hit its test-duration window without detecting a reaction. The
-// controller skips the NVS PID persist; the display surfaces it without a
-// watchdog-disconnect UX. Distinct from the generic TIMEOUT.
+// Autotune saw no reaction within its test window (distinct from TIMEOUT): no PID persist, no watchdog-disconnect UX.
 constexpr int ERROR_CODE_AUTOTUNE_TIMEOUT = 6;
 
 #endif // GAGGIMATE_COMM_H

lib/NanoPbComm/src/GaggiMateServer.cpp

@@ -10,6 +10,7 @@ void GaggiMateServer::init(const String &deviceName, const String &hardware, con
     setSystemInfo(hardware, version, capabilities);
     registerHandlers();
     _endpoint.onConnection([this](bool connected) {
+        _sentSystemInfoAfterHandshake = false;
         if (connected)
             pushSystemInfo();
     });
@@ -112,10 +113,7 @@ gm::Payload GaggiMateServer::buildError(int code) {
     return p;
 }
 
-// Telemetry (sensor / volumetric / ToF) is sent fire-and-forget: it is
-// high-rate and self-refreshing, so a dropped sample is replaced by the next
-// one. This avoids the constant ACK chatter on the high-rate path. Button /
-// autotune-result / error / system-info stay reliable.
+// Telemetry (sensor / volumetric / ToF) is fire-and-forget: self-refreshing, so skip ACK chatter; the rest stays reliable.
 void GaggiMateServer::sendSensorData(float temperature, float pressure, float puckFlow, float pumpFlow, float puckResistance, float temperature2,
                                      float pumpPower, float heaterPower) {
     _endpoint.sendUnreliable(buildSensorData(temperature, pressure, puckFlow, pumpFlow, puckResistance, temperature2, pumpPower, heaterPower));
@@ -135,6 +133,14 @@ void GaggiMateServer::sendError(int code) { _endpoint.send(buildError(code)); }
 
 void GaggiMateServer::registerHandlers() {
     _endpoint.on(gaggimate_Payload_ping_tag, [this](const gm::Payload &) {
+        // A SystemInfo notification sent synchronously from the BLE subscribe
+        // callback can beat the client's notification handler. Once a ping has
+        // crossed the framed protocol, the link is fully established; resend
+        // SystemInfo once so reliable delivery starts from a usable session.
+        if (!_sentSystemInfoAfterHandshake) {
+            _sentSystemInfoAfterHandshake = true;
+            pushSystemInfo();
+        }
         if (_pingCb)
             _pingCb();
     });

lib/NanoPbComm/src/GaggiMateServer.h

@@ -7,12 +7,8 @@
 #include <Arduino.h>
 #include <functional>
 
-/**
- * Controller-side protocol facade.
- *
- * Owns a BLE server transport + Endpoint and exposes semantic send methods and
- * typed command callbacks. Pushes SystemInfo to the display on connect.
- */
+// Controller-side protocol facade: owns transport + Endpoint, exposes semantic sends and typed command callbacks;
+// pushes SystemInfo to the display on connect.
 class GaggiMateServer {
   public:
     using PingCallback = std::function<void()>;
@@ -37,8 +33,7 @@ class GaggiMateServer {
 
     void setSystemInfo(const String &hardware, const String &version, const gm::DeviceCapabilities &capabilities);
 
-    // Build a payload without sending (compose your own batch, then send()).
-    // sendSensorData reports boiler 0; the wire format supports several boilers.
+    // Build a payload without sending; sendSensorData reports boiler 0 (the wire format supports several).
     gm::Payload buildSensorData(float temperature, float pressure, float puckFlow, float pumpFlow, float puckResistance, float temperature2,
                                 float pumpPower = 0.0f, float heaterPower = 0.0f);
     gm::Payload buildButtonState(uint8_t index, bool pressed);
@@ -56,10 +51,12 @@ class GaggiMateServer {
     void sendTofMeasurement(uint32_t distance);
     void sendError(int code);
 
-    // Drop the current BLE link. The ping watchdog calls this so the display
-    // sees a real disconnect instead of having to interpret an in-band error.
+    // Drop the BLE link; the ping watchdog uses this so the display sees a real disconnect, not an in-band error.
     void disconnect() { _transport.disconnect(); }
 
+    // Forget the paired display and advertise openly again (re-pairing escape hatch, e.g. after a screen swap).
+    void clearBonds() { _transport.clearBonds(); }
+
     // Send a pre-built payload / batch of payloads (one frame).
     void send(const gm::Payload &payload) { _endpoint.send(payload); }
     void sendBatch(const gm::Payload *payloads, size_t count) { _endpoint.sendBatch(payloads, count); }
@@ -87,6 +84,10 @@ class GaggiMateServer {
     BleServerTransport _transport;
     Endpoint _endpoint;
     gm::SystemInfo _systemInfo = gaggimate_SystemInfo_init_zero;
+    // The BLE subscribe callback can run before the client has finished
+    // installing its notification handler. The first received ping is the
+    // application-level proof that the new session is ready in both directions.
+    bool _sentSystemInfoAfterHandshake = false;
 
     PingCallback _pingCb;
     BoilerCallback _boilerCb;
@@ -102,8 +103,7 @@ class GaggiMateServer {
     void registerHandlers();
     void pushSystemInfo();
 
-    // Drives the endpoint send pump / retransmit independently of the
-    // controller's (slow, 250ms) main loop, on the NimBLE core.
+    // Drives the endpoint send pump / retransmit on the NimBLE core, independent of the slow 250ms main loop.
     TaskHandle_t _taskHandle = nullptr;
     static void pumpTask(void *arg);
 };

lib/NanoPbComm/src/Messages.h

@@ -1,14 +1,8 @@
 #ifndef NANOPBCOMM_MESSAGES_H
 #define NANOPBCOMM_MESSAGES_H
 
-// Short, namespaced aliases for the package-prefixed nanopb types so library
-// internals read cleanly (gm::Payload instead of gaggimate_Payload). These are
-// used only inside NanoPbComm -- the high-level GaggiMateClient/Server API
-// exposes plain C++ types, so firmware code never sees nanopb structs.
-//
-// Tag/descriptor/init macros (gaggimate_*_tag, gaggimate_*_msg,
-// gaggimate_*_init_zero) are macros, not types, so they are referenced with
-// their full names where needed.
+// NanoPbComm-internal aliases for nanopb types (gm::Payload vs gaggimate_Payload); firmware never sees nanopb structs.
+// The gaggimate_*_tag/_msg/_init_zero macros are not types, so they keep their full names.
 
 #include "gaggimate.pb.h"
 

lib/NanoPbComm/src/Protocol.h

@@ -7,9 +7,7 @@
 // Shared protocol UUIDs and helpers used by both ends.
 namespace gm_proto {
 
-// BLE service + characteristics. A single TX/RX pair carries framed nanopb
-// datagrams (replacing the old one-characteristic-per-message design). The
-// service UUID is kept so the display scan logic is unchanged.
+// BLE service + chars: one TX/RX pair carries framed nanopb datagrams; the service UUID predates the framed protocol.
 static constexpr const char *SERVICE_UUID = "e75bc5b6-ff6e-4337-9d31-0c128f2e6e68";
 // Controller (server) -> display (client) notifications.
 static constexpr const char *TX_CHAR_UUID = "87654321-4321-8765-4321-cba987654321";
@@ -18,9 +16,7 @@ static constexpr const char *RX_CHAR_UUID = "12345678-1234-5678-1234-123456789ab
 // Legacy read-only system-info characteristic (JSON), kept for external readers.
 static constexpr const char *INFO_CHAR_UUID = "f8d7203b-e00c-48e2-83ba-37ff49cdba74";
 
-// Protocol/schema version. Bump on any breaking change to gaggimate.proto so a
-// display talking to an out-of-date controller (or vice versa) can detect the
-// mismatch. Carried in SystemInfo.protocol_version.
+// Bump on any breaking gaggimate.proto change; carried in SystemInfo.protocol_version for mismatch detection.
 static constexpr uint32_t PROTOCOL_VERSION = 3;
 
 // Outbound priorities (higher wins in the queue).
@@ -31,13 +27,10 @@ enum Priority : uint8_t {
     PRIO_HIGH = 200,    // ping, error
 };
 
-// Per-family device-index space for the coalescing key. Keeps keys dense so the
-// queue's reverse-lookup table stays small.
+// Per-family device-index space for the coalescing key; keeps keys dense so the queue's reverse-lookup stays small.
 static constexpr uint16_t MAX_DEVICES = 8;
 
-// Coalescing key: a (message family, device index) pair so repeated updates for
-// the same component collapse to the latest value. Device-less messages map to
-// index 0. Keys stay below which_content_max * MAX_DEVICES.
+// Coalescing key (family, device index): repeated updates for one component collapse to the latest value.
 inline uint16_t coalescingKey(const gm::Payload &p) {
     uint16_t index = 0;
     switch (p.which_content) {