WIP: Fix packet loss in the ESP-NOW and RS232 bridges

docs/cli_commands.md

@@ -1123,6 +1123,34 @@ region save
 
 ---
 
+#### View the bridge frame counters
+**Usage:**
+- `get bridge.rxstats`
+- `get bridge.txstats`
+- `set bridge.stats reset`
+
+Counts every way a frame can be lost, so a message-count mismatch between two
+bridged nodes can be attributed rather than guessed at. Compare the sending
+node's TX count against the receiving node's `RX in` to separate loss on the
+link from local drops.
+
+Common RX fields: `in` frames received, `ok` queued to the mesh, `dup` already
+seen, `nopar` not a valid mesh packet, `pool` packet pool was empty (frame
+retried, not lost), `qfull` lost because the main loop was not draining fast
+enough, `hwm` deepest the receive queue has been.
+
+**ESP-NOW** adds `bad` (wrong magic/checksum, usually another network's secret)
+on RX, and on TX reports `ok` frames the radio confirmed, `dup` already bridged,
+`big` too large for an ESP-NOW frame, `rty` retransmissions, `ref` radio refused
+the frame, `tmo` send callback never arrived, `fail` abandoned after every
+attempt, `qfull` transmit queue full, `hwm` transmit queue depth.
+
+**RS232** adds `crc` (checksum failures), `len` (bad length field) and `noise`
+(bytes skipped while hunting for a frame header) on RX, which together indicate
+a marginal cable or a baud-rate mismatch. Its TX side reports `dup` and `big`.
+
+---
+
 #### View the bootloader version (nRF52 only)
 **Usage:** `get bootloader.ver`
 

examples/simple_repeater/MyMesh.h

@@ -249,6 +249,23 @@ class MyMesh : public mesh::Mesh, public CommonCLICallbacks {
   }
 #endif
 
+#if defined(WITH_BRIDGE)
+  bool getBridgeRxStats(char* dest, size_t dest_size) override {
+    bridge.getRxStats(dest, dest_size);
+    return true;
+  }
+
+  bool getBridgeTxStats(char* dest, size_t dest_size) override {
+    bridge.getTxStats(dest, dest_size);
+    return true;
+  }
+
+  bool resetBridgeStats() override {
+    bridge.resetStats();
+    return true;
+  }
+#endif
+
   // To check if there is pending work
   bool hasPendingWork() const;
 

platformio.ini

@@ -51,6 +51,10 @@ build_src_filter =
   +<helpers/*.cpp>
   +<helpers/radiolib/*.cpp>
   +<helpers/bridges/BridgeBase.cpp>
+  +<helpers/bridges/BridgeCodec.cpp>
+  +<helpers/bridges/BridgeFrameQueue.cpp>
+  +<helpers/bridges/BridgeSerialFramer.cpp>
+  +<helpers/bridges/BridgeTxQueue.cpp>
   +<helpers/ui/MomentaryButton.cpp>
 
 ; ----------------- ESP32 ---------------------
@@ -163,11 +167,17 @@ build_flags = -std=c++17
   -I src
   -I test/mocks
 test_build_src = yes
-test_filter = test_utils
+test_filter =
+  test_utils
+  test_bridge
 build_src_filter =
   -<*>
   +<../src/Utils.cpp>
   +<../src/Packet.cpp>
+  +<../src/helpers/bridges/BridgeCodec.cpp>
+  +<../src/helpers/bridges/BridgeSerialFramer.cpp>
+  +<../src/helpers/bridges/BridgeFrameQueue.cpp>
+  +<../src/helpers/bridges/BridgeTxQueue.cpp>
 lib_deps =
   google/googletest @ 1.17.0
 

src/helpers/CommonCLI.cpp

@@ -297,7 +297,7 @@ void CommonCLI::handleCommand(uint32_t sender_timestamp, char* command, char* re
       StrHelper::strncpy(_prefs->password, &command[9], sizeof(_prefs->password));
       savePrefs();
       sprintf(reply, "password now: ");
-      StrHelper::strncpy(&reply[14], _prefs->password, 160-15);   // echo back just to let admin know for sure!!
+      StrHelper::strncpy(&reply[14], _prefs->password, MAX_REPLY_LEN-15);   // echo back just to let admin know for sure!!
     } else if (memcmp(command, "clear stats", 11) == 0) {
       _callbacks->clearStats();
       strcpy(reply, "(OK - stats reset)");
@@ -750,6 +750,9 @@ void CommonCLI::handleSetCmd(uint32_t sender_timestamp, char* command, char* rep
     _prefs->bridge_pkt_src = memcmp(&config[14], "rx", 2) == 0;
     savePrefs();
     strcpy(reply, "OK");
+  } else if (memcmp(config, "bridge.stats reset", 18) == 0) {
+    // zero the counters so a measurement can start from a known point
+    strcpy(reply, _callbacks->resetBridgeStats() ? "OK" : "Bridge stats not supported");
 #endif
 #ifdef WITH_RS232_BRIDGE
   } else if (memcmp(config, "bridge.baud ", 12) == 0) {
@@ -795,7 +798,7 @@ void CommonCLI::handleSetCmd(uint32_t sender_timestamp, char* command, char* rep
     };
   } else {
     strcpy(reply, "unknown config: ");
-    StrHelper::strncpy(&reply[16], config, 160-17);
+    StrHelper::strncpy(&reply[16], config, MAX_REPLY_LEN-17);
   }
 }
 
@@ -910,6 +913,18 @@ void CommonCLI::handleGetCmd(uint32_t sender_timestamp, char* command, char* rep
     sprintf(reply, "> %d", (uint32_t)_prefs->bridge_delay);
   } else if (memcmp(config, "bridge.source", 13) == 0) {
     sprintf(reply, "> %s", _prefs->bridge_pkt_src ? "logRx" : "logTx");
+  } else if (memcmp(config, "bridge.rxstats", 14) == 0) {
+    // Every way the bridge can lose a frame, so a mismatch between two bridged
+    // nodes can be attributed instead of guessed at.
+    strcpy(reply, "> ");
+    if (!_callbacks->getBridgeRxStats(&reply[2], MAX_REPLY_LEN - 2)) {
+      strcpy(reply, "Bridge stats not supported");
+    }
+  } else if (memcmp(config, "bridge.txstats", 14) == 0) {
+    strcpy(reply, "> ");
+    if (!_callbacks->getBridgeTxStats(&reply[2], MAX_REPLY_LEN - 2)) {
+      strcpy(reply, "Bridge stats not supported");
+    }
 #endif
 #ifdef WITH_RS232_BRIDGE
   } else if (memcmp(config, "bridge.baud", 11) == 0) {

src/helpers/CommonCLI.h

@@ -111,11 +111,36 @@ class CommonCLICallbacks {
     // no op by default
   };
 
+  /**
+   * Writes the bridge's receive-side counters into dest.
+   * @return false if this build has no bridge that reports statistics
+   */
+  virtual bool getBridgeRxStats(char* dest, size_t dest_size) {
+    return false;
+  };
+
+  /** Writes the bridge's transmit-side counters into dest. */
+  virtual bool getBridgeTxStats(char* dest, size_t dest_size) {
+    return false;
+  };
+
+  /** Zeroes the bridge's counters. @return false if unsupported */
+  virtual bool resetBridgeStats() {
+    return false;
+  };
+
   virtual bool setRxBoostedGain(bool enable) {
     return false; // CommonCLI reports unsupported if not overridden by wrapper
   };
 };
 
+/**
+ * Size of the reply buffer every handleCommand() caller provides. The serial
+ * console uses char[160] and remote admin carves 161 bytes out of a 166 byte
+ * frame, so 160 is what a reply can safely assume.
+ */
+#define MAX_REPLY_LEN  160
+
 class CommonCLI {
   mesh::RTCClock* _rtc;
   NodePrefs* _prefs;

src/helpers/bridges/BridgeBase.cpp

@@ -15,20 +15,51 @@ const char *BridgeBase::getLogDateTime() {
   return tmp;
 }
 
-uint16_t BridgeBase::fletcher16(const uint8_t *data, size_t len) {
-  uint8_t sum1 = 0, sum2 = 0;
+void BridgeBase::resetBaseStats() {
+  _rx_delivered = 0;
+  _rx_duplicates = 0;
+  _rx_no_packet = 0;
+  _rx_unparsed = 0;
+  _tx_duplicates = 0;
+  _tx_oversized = 0;
+}
 
-  for (size_t i = 0; i < len; i++) {
-    sum1 = (sum1 + data[i]) % 255;
-    sum2 = (sum2 + sum1) % 255;
-  }
+void BridgeBase::drainRxFrames(BridgeFrameQueue &queue, uint8_t max_frames, uint8_t *scratch,
+                               size_t scratch_cap) {
+  for (uint8_t drained = 0; drained < max_frames && !queue.isEmpty(); drained++) {
+    // Take a pool slot first: if the pool is empty the frame stays queued for a
+    // later loop, costing latency instead of a message.
+    mesh::Packet *pkt = _mgr->allocNew();
+    if (pkt == nullptr) {
+      _rx_no_packet++;
+      break;
+    }
 
-  return (sum2 << 8) | sum1;
-}
+    const size_t frame_len = queue.pop(scratch, scratch_cap);
+    if (frame_len == 0) {
+      _mgr->free(pkt);
+      break;
+    }
 
-bool BridgeBase::validateChecksum(const uint8_t *data, size_t len, uint16_t received_checksum) {
-  uint16_t calculated_checksum = fletcher16(data, len);
-  return received_checksum == calculated_checksum;
+    size_t blob_len = 0;
+    const uint8_t *blob = unwrapFrame(scratch, frame_len, blob_len);
+    if (blob == nullptr) {
+      _mgr->free(pkt);  // the override counted why
+      continue;
+    }
+
+    BRIDGE_DEBUG_PRINTLN("RX, len=%d\n", (int)blob_len);
+
+    // readFrom() takes a uint8_t length, and a mesh packet can never be longer
+    // than MAX_TRANS_UNIT + 1 anyway.
+    if (blob_len > 0 && blob_len <= 255 && pkt->readFrom(blob, (uint8_t)blob_len)) {
+      onPacketReceived(pkt);  // takes ownership
+    } else {
+      BRIDGE_DEBUG_PRINTLN("RX failed to parse packet\n");
+      _rx_unparsed++;
+      _mgr->free(pkt);
+    }
+  }
 }
 
 void BridgeBase::handleReceivedPacket(mesh::Packet *packet) {
@@ -43,7 +74,9 @@ void BridgeBase::handleReceivedPacket(mesh::Packet *packet) {
     _seen_packets.markSeen(packet);
     // bridge_delay provides a buffer to prevent immediate processing conflicts in the mesh network.
     _mgr->queueInbound(packet, millis() + _prefs->bridge_delay);
+    _rx_delivered++;
   } else {
     _mgr->free(packet);
+    _rx_duplicates++;
   }
 }

src/helpers/bridges/BridgeBase.h

@@ -3,20 +3,24 @@
 #include "helpers/AbstractBridge.h"
 #include "helpers/CommonCLI.h"
 #include "helpers/SimpleMeshTables.h"
+#include "helpers/bridges/BridgeFrameQueue.h"
 
 #include <RTClib.h>
 
 /**
  * @brief Base class implementing common bridge functionality
  *
  * This class provides common functionality used by different bridge implementations
- * like packet tracking, checksum calculation, timestamping, and duplicate detection.
+ * like packet tracking, timestamping, and duplicate detection.
  *
  * Features:
- * - Fletcher-16 checksum calculation for data integrity
  * - Packet duplicate detection using SimpleMeshTables
  * - Common timestamp formatting for debug logging
  * - Shared packet management and queuing logic
+ * - Counters shared by every bridge, behind `get bridge.rxstats`/`bridge.txstats`
+ *
+ * The wire framing itself lives in BridgeCodec/BridgeSerialFramer, which stay
+ * free of the Arduino and mesh headers so they can be unit tested natively.
  */
 class BridgeBase : public AbstractBridge {
 public:
@@ -29,26 +33,6 @@ class BridgeBase : public AbstractBridge {
    */
   bool isRunning() const override;
 
-  /**
-   * @brief Common magic number used by all bridge implementations for packet identification
-   *
-   * This magic number is placed at the beginning of bridge packets to identify
-   * them as mesh bridge packets and provide frame synchronization.
-   */
-  static constexpr uint16_t BRIDGE_PACKET_MAGIC = 0xC03E;
-
-  /**
-   * @brief Common field sizes used by bridge implementations
-   *
-   * These constants define the size of common packet fields used across bridges.
-   * BRIDGE_MAGIC_SIZE is used by all bridges for packet identification.
-   * BRIDGE_LENGTH_SIZE is used by bridges that need explicit length fields (like RS232).
-   * BRIDGE_CHECKSUM_SIZE is used by all bridges for Fletcher-16 checksums.
-   */
-  static constexpr uint16_t BRIDGE_MAGIC_SIZE = sizeof(BRIDGE_PACKET_MAGIC);
-  static constexpr uint16_t BRIDGE_LENGTH_SIZE = sizeof(uint16_t);
-  static constexpr uint16_t BRIDGE_CHECKSUM_SIZE = sizeof(uint16_t);
-
 protected:
   /** Tracks bridge state */
   bool _initialized = false;
@@ -65,6 +49,24 @@ class BridgeBase : public AbstractBridge {
   /** Tracks seen packets to prevent loops in broadcast communications */
   SimpleMeshTables _seen_packets;
 
+  /** Packets handed to the mesh inbound queue */
+  uint32_t _rx_delivered = 0;
+
+  /** Packets dropped on arrival because this node had already seen them */
+  uint32_t _rx_duplicates = 0;
+
+  /** Frames left queued because the mesh packet pool was empty */
+  uint32_t _rx_no_packet = 0;
+
+  /** Frames that arrived intact but were not a valid mesh packet */
+  uint32_t _rx_unparsed = 0;
+
+  /** Packets not bridged because this node had already sent them */
+  uint32_t _tx_duplicates = 0;
+
+  /** Packets too large to frame for this transport */
+  uint32_t _tx_oversized = 0;
+
   /**
    * @brief Constructs a BridgeBase instance
    *
@@ -84,27 +86,44 @@ class BridgeBase : public AbstractBridge {
    */
   const char *getLogDateTime();
 
+  /** Zeroes the counters owned by this class. Call from the bridge's resetStats(). */
+  void resetBaseStats();
+
   /**
-   * @brief Calculate Fletcher-16 checksum
+   * @brief Strip the transport's framing off a received frame.
    *
-   * Based on: https://en.wikipedia.org/wiki/Fletcher%27s_checksum
-   * Used to verify data integrity of received packets
+   * Called by drainRxFrames() once per frame. The default treats the frame as a
+   * bare mesh packet blob, which is what a transport that carries no framing of
+   * its own (RS232, whose framer has already unwrapped it) needs.
    *
-   * @param data Pointer to data to calculate checksum for
-   * @param len Length of data in bytes
-   * @return Calculated Fletcher-16 checksum
+   * @param frame      the frame as popped from the receive queue
+   * @param frame_len  length of @p frame
+   * @param blob_len   set to the length of the returned blob
+   * @return pointer to the mesh packet blob, or nullptr to drop the frame. An
+   *         override returning nullptr is expected to count its own reason.
    */
-  static uint16_t fletcher16(const uint8_t *data, size_t len);
+  virtual const uint8_t *unwrapFrame(const uint8_t *frame, size_t frame_len, size_t &blob_len) {
+    blob_len = frame_len;
+    return frame;
+  }
 
   /**
-   * @brief Validate received checksum against calculated checksum
+   * @brief Turn queued frames into mesh packets, bounded per call.
+   *
+   * Shared by every bridge because the ownership rules are subtle: a pool slot is
+   * taken *before* the frame is popped, so an exhausted pool leaves the frame
+   * queued for a later loop instead of destroying it, and every path that does
+   * not hand the packet to the mesh frees it exactly once.
    *
-   * @param data Pointer to data to validate
-   * @param len Length of data in bytes
-   * @param received_checksum Checksum received with data
-   * @return true if checksum is valid, false otherwise
+   * @param queue       frames waiting to be delivered
+   * @param max_frames  frames to convert before returning, so one bridge cannot
+   *                    monopolise the loop
+   * @param scratch     buffer a frame is popped into; must hold the largest frame
+   *                    the queue accepts
+   * @param scratch_cap capacity of @p scratch
    */
-  bool validateChecksum(const uint8_t *data, size_t len, uint16_t received_checksum);
+  void drainRxFrames(BridgeFrameQueue &queue, uint8_t max_frames, uint8_t *scratch,
+                     size_t scratch_cap);
 
   /**
    * @brief Common packet handling for received packets