From b8929c15fe137bf6860d76ca413c0633f78ef056 Mon Sep 17 00:00:00 2001 From: mikecarper Date: Fri, 23 Oct 2026 18:57:57 -0700 Subject: [PATCH] Keep ESP-NOW TX seen keys instead of duplicate packet snapshots --- src/helpers/SimpleMeshTables.h | 33 ++++++--- src/helpers/bridges/ESPNowBridge.cpp | 15 ++-- src/helpers/bridges/ESPNowBridge.h | 5 +- test/fixtures/espnow_wifi_handoff.cpp | 4 +- test/test_bridge_recovery.py | 69 +++++++++++++++++++ test/test_espnow_radio_lifecycle.py | 4 +- .../test_simple_mesh_tables.cpp | 58 ++++++++++++++++ test/test_wifi_ap_lr_safety.py | 4 +- 8 files changed, 171 insertions(+), 21 deletions(-) diff --git a/src/helpers/SimpleMeshTables.h b/src/helpers/SimpleMeshTables.h index 772c3ea9c..0e1914bc8 100644 --- a/src/helpers/SimpleMeshTables.h +++ b/src/helpers/SimpleMeshTables.h @@ -53,6 +53,15 @@ inline bool routeHashPrefixesOverlap(const uint8_t* a, uint8_t a_len, class SimpleMeshTables : public mesh::MeshTables { public: + // A queued transport can retire an unsent admission after its Packet pool + // entry is gone. Keep the complete hash and its separate ACK-cache domain. + struct SeenKey { + uint8_t hash[MAX_HASH_SIZE] = {}; + bool dedicated_ack = false; + }; + static_assert(sizeof(SeenKey) == MAX_HASH_SIZE + 1, + "A seen key must not retain packet data or alignment padding"); + // Recent rows belong to the mesh loop, including CLI requests marshaled from // network tasks. They must not be read or changed concurrently by an ISR or // worker. Byte access avoids unaligned uint32_t loads in the compact rows; @@ -411,26 +420,32 @@ public: } } - void clear(const mesh::Packet* packet) override { - if (isDedicatedAckPacket(packet)) { - uint8_t hash[MAX_HASH_SIZE]; - packet->calculatePacketHash(hash); - clearAckHash(hash); + SeenKey seenKey(const mesh::Packet& packet) const { + SeenKey key; + packet.calculatePacketHash(key.hash); + key.dedicated_ack = isDedicatedAckPacket(&packet); + return key; + } + + void clearSeenKey(const SeenKey& key) { + if (key.dedicated_ack) { + clearAckHash(key.hash); return; } - uint8_t hash[MAX_HASH_SIZE]; - packet->calculatePacketHash(hash); - uint8_t* sp = _hashes; for (int i = 0; i < MAX_PACKET_HASHES; i++, sp += MAX_HASH_SIZE) { - if (memcmp(hash, sp, MAX_HASH_SIZE) == 0) { + if (memcmp(key.hash, sp, MAX_HASH_SIZE) == 0) { memset(sp, 0, MAX_HASH_SIZE); break; } } } + void clear(const mesh::Packet* packet) override { + clearSeenKey(seenKey(*packet)); + } + uint32_t getNumDirectDups() const { return _direct_dups; } uint32_t getNumFloodDups() const { return _flood_dups; } diff --git a/src/helpers/bridges/ESPNowBridge.cpp b/src/helpers/bridges/ESPNowBridge.cpp index 77016ec6a..b75106f92 100644 --- a/src/helpers/bridges/ESPNowBridge.cpp +++ b/src/helpers/bridges/ESPNowBridge.cpp @@ -304,7 +304,8 @@ void ESPNowBridge::end() { uint8_t tx_index = _tx_tail; for (uint8_t i = 0; i < _tx_count; ++i) { const QueuedTransmit& queued_tx = _tx_queue[tx_index]; - if (!queued_tx.started && !queued_tx.diagnostic) _seen_packets.clear(&queued_tx.packet); + if (!queued_tx.started && !queued_tx.diagnostic) + _seen_packets.clearSeenKey(queued_tx.seen_key); tx_index = static_cast((tx_index + 1) % TX_QUEUE_DEPTH); } portENTER_CRITICAL(&_tx_mux); @@ -385,12 +386,16 @@ bool ESPNowBridge::queueTransmitFrames( } } + // Hash the caller-owned snapshot before entering the callback-shared queue + // lock. SHA work never extends an ESP-NOW critical section. + SimpleMeshTables::SeenKey seen_key; + if (!diagnostic) seen_key = _seen_packets.seenKey(packet); bool queued = false; portENTER_CRITICAL(&_tx_mux); if (_tx_count < TX_QUEUE_DEPTH) { QueuedTransmit& queued_tx = _tx_queue[_tx_head]; queued_tx.frames = frames; - if (!diagnostic) queued_tx.packet = packet; + queued_tx.seen_key = seen_key; queued_tx.next_frame = 0; queued_tx.started = false; queued_tx.start_retries = 0; @@ -417,7 +422,7 @@ void ESPNowBridge::pumpTransmitQueue() { bool completed = false; int completed_status = ESP_NOW_SEND_SUCCESS; bool clear_unsent_seen = false; - mesh::Packet unsent_packet; + SimpleMeshTables::SeenKey unsent_key; uint32_t dropped = 0; const uint32_t now = millis(); bool timed_out = false; @@ -527,7 +532,7 @@ void ESPNowBridge::pumpTransmitQueue() { _tx_retry_at = now + TX_RETRY_DELAY_MS; } else { if (!queued_tx.started && !queued_tx.diagnostic) { - unsent_packet = queued_tx.packet; + unsent_key = queued_tx.seen_key; clear_unsent_seen = true; } _tx_tail = static_cast((_tx_tail + 1) % TX_QUEUE_DEPTH); @@ -539,7 +544,7 @@ void ESPNowBridge::pumpTransmitQueue() { portEXIT_CRITICAL(&_tx_mux); // Admission marks the packet to suppress duplicate queue entries. Restore // the prior behavior when even its first esp_now_send() cannot start. - if (clear_unsent_seen) _seen_packets.clear(&unsent_packet); + if (clear_unsent_seen) _seen_packets.clearSeenKey(unsent_key); BRIDGE_DEBUG_PRINTLN("TX failed to start, error=%d\n", (int)result); } else { portENTER_CRITICAL(&_tx_mux); diff --git a/src/helpers/bridges/ESPNowBridge.h b/src/helpers/bridges/ESPNowBridge.h index 0174007c3..38e55ceb5 100644 --- a/src/helpers/bridges/ESPNowBridge.h +++ b/src/helpers/bridges/ESPNowBridge.h @@ -104,12 +104,15 @@ private: // instead of transmitting an orphan fragment. struct QueuedTransmit { mesh::espnow::ESPNowRawFrames frames; - mesh::Packet packet; + SimpleMeshTables::SeenKey seen_key; uint8_t next_frame; bool started; uint8_t start_retries; bool diagnostic; }; + static_assert(sizeof(QueuedTransmit) <= sizeof(mesh::espnow::ESPNowRawFrames) + + sizeof(SimpleMeshTables::SeenKey) + 8, + "ESP-NOW TX must retain encoded frames without a second packet copy"); QueuedTransmit _tx_queue[TX_QUEUE_DEPTH]; portMUX_TYPE _tx_mux = portMUX_INITIALIZER_UNLOCKED; volatile uint8_t _tx_head; diff --git a/test/fixtures/espnow_wifi_handoff.cpp b/test/fixtures/espnow_wifi_handoff.cpp index f63db1330..fc34b873b 100644 --- a/test/fixtures/espnow_wifi_handoff.cpp +++ b/test/fixtures/espnow_wifi_handoff.cpp @@ -168,10 +168,10 @@ public: int _tx_mux=0, _tx_head=0, _tx_tail=0, _tx_count=0, _tx_waiting=0, _tx_callback_done=0; int _tx_callback_status=0, _tx_dropped=0, _tx_dropped_reported=0; mesh::espnow::ESPNowRawReassembler _raw_reassembler; - struct QueuedTransmit { bool started=false, diagnostic=false; int packet=0; }; + struct QueuedTransmit { bool started=false, diagnostic=false; int seen_key=0; }; static constexpr int TX_QUEUE_DEPTH=2; QueuedTransmit _tx_queue[TX_QUEUE_DEPTH]; - struct { void clear(const int*) {} } _seen_packets; + struct { void clearSeenKey(const int&) {} } _seen_packets; static void recv_cb() {} static void send_cb() {} uint32_t _tx_started_at = 0, _tx_retry_at = 0; diff --git a/test/test_bridge_recovery.py b/test/test_bridge_recovery.py index eaa7229b1..9f2fa2e6d 100644 --- a/test/test_bridge_recovery.py +++ b/test/test_bridge_recovery.py @@ -91,11 +91,15 @@ struct NodePrefs { char bridge_secret[32]="fleet-test"; }; struct Seen { + struct SeenKey {uint8_t hash[8]={};bool dedicated_ack=false;}; std::vector ids; + SeenKey seenKey(const mesh::Packet& p)const{SeenKey key;key.hash[0]=p.data[0];return key;} + void clearSeenKey(const SeenKey& key){ids.erase(std::remove(ids.begin(),ids.end(),key.hash[0]),ids.end());} bool wasSeen(const mesh::Packet* p){return std::find(ids.begin(),ids.end(),p->data[0])!=ids.end();} void markSeen(const mesh::Packet* p){if(!wasSeen(p))ids.push_back(p->data[0]);} void clear(const mesh::Packet* p){ids.erase(std::remove(ids.begin(),ids.end(),p->data[0]),ids.end());} }; +using SimpleMeshTables=Seen; class BridgeBase { public: @INGRESS_HOOK@ @@ -196,6 +200,71 @@ class BridgeRecoveryTests(unittest.TestCase): result = subprocess.run([str(exe)], capture_output=True, text=True) self.assertEqual(result.returncode, 0, result.stderr) + def test_seen_keys_keep_all_six_frames_and_survive_caller_packet_reuse(self): + self.run_fixture(r''' +int main(){ + Backend backend;backend.wifi=true;mesh::wireless::control().begin(backend); + sdk_initialized=sdk_started=true;sdk_mode=WIFI_STA; + NodePrefs prefs;mesh::PacketManager mgr;mesh::RTCClock rtc; + ESPNowBridge bridge(&prefs,&mgr,&rtc);bridge.begin(); + std::array originals; + std::array reusable; + for(unsigned i=0;i<6;++i){ + originals[i]=packet(uint8_t(0x11+i),255);reusable[i]=originals[i]; + bridge.sendPacket(&reusable[i]); + assert(bridge._tx_count==int(i+1)); + assert(bridge._tx_queue[i].seen_key.hash[0]==originals[i].data[0]); + memset(reusable[i].data,0x90+i,sizeof reusable[i].data); + } + auto overflow=packet(0x27);bridge.sendPacket(&overflow); + assert(bridge._tx_count==6&&!bridge._seen_packets.wasSeen(&overflow)); + bridge.end(); + assert(bridge._seen_packets.ids.empty()&&send_calls==0); // exact original admissions retired + bridge.begin(); + for(auto& p:originals)bridge.sendPacket(&p); + bridge.loop(); + for(unsigned i=0;i<6;++i){ + mesh::espnow::ESPNowRawFrames expected; + assert(mesh::espnow::encodeEspNowRawFrames(originals[i].data,255,expected)); + assert(expected.count==2&&sent[2*i]==std::vector(expected.data[0],expected.data[0]+expected.lengths[0])); + complete(bridge); + assert(sent[2*i+1]==std::vector(expected.data[1],expected.data[1]+expected.lengths[1])); + complete(bridge); + } + assert(bridge._tx_count==0&&sent.size()==12); + bridge.end(); + for(auto& p:originals)assert(bridge._seen_packets.wasSeen(&p)); // completed entries stay seen +} +''', versions=(40400,50200,50500)) + + def test_seen_cleanup_retries_and_timeout_distinguish_never_sent_entries(self): + self.run_fixture(r''' +int main(){ + Backend backend;backend.wifi=true;mesh::wireless::control().begin(backend); + sdk_initialized=sdk_started=true;sdk_mode=WIFI_STA; + NodePrefs prefs;mesh::PacketManager mgr;mesh::RTCClock rtc; + ESPNowBridge bridge(&prefs,&mgr,&rtc);bridge.begin(); + auto original=packet(0x15,255),caller=original; + bridge.sendPacket(&caller);memset(caller.data,0x95,sizeof caller.data); + send_result=ESP_ERR_ESPNOW_NO_MEM; + for(unsigned i=0;i<4;++i){bridge.loop();now_ms+=20;} + assert(bridge._tx_count==0&&!bridge._seen_packets.wasSeen(&original)); + // Once a first fragment was accepted, an immediate sibling rejection cannot + // clear the logical packet's admission, even if the caller reused its pool entry. + send_result=ESP_OK;bridge.sendPacket(&original);bridge.loop(); + assert(bridge._tx_count==1&&bridge._tx_queue[bridge._tx_tail].started); + send_result=ESP_ERR_INVALID_ARG;complete(bridge); + assert(bridge._tx_count==0&&bridge._seen_packets.wasSeen(&original)); + auto active=packet(0x21,255),waiting=packet(0x25,255); + send_result=ESP_OK;bridge.sendPacket(&active);bridge.sendPacket(&waiting);bridge.loop(); + assert(bridge._tx_count==2&&bridge._tx_waiting); + now_ms+=1000;bridge.loop(); // lost callback tears down before restarting the session + assert(bridge._initialized&&bridge._tx_count==0); + assert(bridge._seen_packets.wasSeen(&active)&&!bridge._seen_packets.wasSeen(&waiting)); + bridge.end(); +} +''', versions=(40400,50200,50500)) + def test_ota_identical_retries_cross_espnow_and_uart_without_permanent_dedup(self): self.run_fixture(r''' int main(){ diff --git a/test/test_espnow_radio_lifecycle.py b/test/test_espnow_radio_lifecycle.py index 8c71e62cb..55f80c5ca 100644 --- a/test/test_espnow_radio_lifecycle.py +++ b/test/test_espnow_radio_lifecycle.py @@ -225,10 +225,10 @@ public: int _tx_callback_status=0,_tx_dropped=0,_tx_dropped_reported=0; uint32_t _tx_started_at=0,_tx_retry_at=0;bool _sdk_teardown_failed=false; mesh::espnow::ESPNowRawReassembler _raw_reassembler; - struct QueuedTransmit {bool started=false,diagnostic=false;int packet=0;}; + struct QueuedTransmit {bool started=false,diagnostic=false;int seen_key=0;}; static constexpr int TX_QUEUE_DEPTH=2; QueuedTransmit _tx_queue[TX_QUEUE_DEPTH]; - struct {void clear(const int*){}} _seen_packets; + struct {void clearSeenKey(const int&){}} _seen_packets; static void recv_cb(){}static void send_cb(){} void begin();void end(); }; diff --git a/test/test_mesh_tables/test_simple_mesh_tables.cpp b/test/test_mesh_tables/test_simple_mesh_tables.cpp index 42d911b3d..5a4b5d344 100644 --- a/test/test_mesh_tables/test_simple_mesh_tables.cpp +++ b/test/test_mesh_tables/test_simple_mesh_tables.cpp @@ -124,6 +124,64 @@ static Packet makeMultipartAckPacket(uint32_t crc, uint8_t remaining = 1, bool d return p; } +TEST(SimpleMeshTables, SeenKeyRetainsEveryHashByteAndAckCacheDomain) { + SimpleMeshTables table; + Packet ordinary = makeFloodPacket(0x43); + Packet plain_ack = makeAckPacket(0x10203040); + Packet multipart_ack = makeMultipartAckPacket(0x10203040); + for (Packet* packet : {&ordinary, &plain_ack, &multipart_ack}) { + table.markSeen(packet); + auto key = table.seenKey(*packet); + uint8_t expected[MAX_HASH_SIZE]; + packet->calculatePacketHash(expected); + EXPECT_EQ(0, memcmp(expected, key.hash, sizeof(expected))); + EXPECT_EQ(packet != &ordinary, key.dedicated_ack); + auto wrong_domain = key; + wrong_domain.dedicated_ack = !wrong_domain.dedicated_ack; + table.clearSeenKey(wrong_domain); + EXPECT_TRUE(table.wasSeen(packet)); + table.clearSeenKey(key); + EXPECT_FALSE(table.wasSeen(packet)); + } +} + +TEST(SimpleMeshTables, SeenKeyClassifiesShortAndNonAckMultipartAsOrdinary) { + SimpleMeshTables table; + Packet short_ack = makeAckPacket(0x10203040); + short_ack.payload_len = 3; + Packet short_multipart = makeMultipartAckPacket(0x10203040); + short_multipart.payload_len = 4; + Packet other_multipart = makeMultipartAckPacket(0x10203040); + other_multipart.payload[0] = PAYLOAD_TYPE_RAW_CUSTOM; + for (Packet* packet : {&short_ack, &short_multipart, &other_multipart}) { + auto key = table.seenKey(*packet); + EXPECT_FALSE(key.dedicated_ack); + table.markSeen(packet); + EXPECT_TRUE(table.wasSeen(packet)); + table.clearSeenKey(key); + EXPECT_FALSE(table.wasSeen(packet)); + } +} + +TEST(SimpleMeshTables, SeenKeySurvivesPacketReuseAndTracePathChanges) { + SimpleMeshTables table; + Packet original = makeFloodPacket(0x51); + original.header = ROUTE_TYPE_FLOOD | (PAYLOAD_TYPE_TRACE << PH_TYPE_SHIFT); + original.path_len = 3; + Packet reused = original; + table.markSeen(&original); + const auto key = table.seenKey(reused); + reused.path_len = 4; // TRACE's hash also includes its current path length. + table.markSeen(&reused); + EXPECT_TRUE(table.wasSeen(&original)); + EXPECT_TRUE(table.wasSeen(&reused)); + table.clearSeenKey(key); + EXPECT_FALSE(table.wasSeen(&original)); + EXPECT_TRUE(table.wasSeen(&reused)); + table.clear(&reused); // Packet callers retain the same public operation. + EXPECT_FALSE(table.wasSeen(&reused)); +} + // -- wasSeen: pure query ------------------------------------------------------- TEST(SimpleMeshTables, WasSeen_ReturnsFalseForUnseen) { diff --git a/test/test_wifi_ap_lr_safety.py b/test/test_wifi_ap_lr_safety.py index 24bf7474d..561a6ee84 100644 --- a/test/test_wifi_ap_lr_safety.py +++ b/test/test_wifi_ap_lr_safety.py @@ -21,10 +21,10 @@ public: mesh::bridge::diag::Stats _stats; std::atomic _debug_dirty{false}; mesh::espnow::ESPNowRawReassembler _raw_reassembler; - struct QueuedTransmit {bool started=false,diagnostic=false;int packet=0;}; + struct QueuedTransmit {bool started=false,diagnostic=false;int seen_key=0;}; static constexpr int TX_QUEUE_DEPTH=2; QueuedTransmit _tx_queue[TX_QUEUE_DEPTH]; - struct {void clear(const int*){}} _seen_packets; + struct {void clearSeenKey(const int&){}} _seen_packets; static void recv_cb(){}static void send_cb(){} void begin();void end(); void recordError(mesh::bridge::diag::Stage,int32_t);void emitDebug();