From cadece73ac99b225a7ef856d95fec44bfe9e90a0 Mon Sep 17 00:00:00 2001 From: Benjamin Faershtein <119711889+RCGV1@users.noreply.github.com> Date: Sat, 1 Aug 2026 00:36:43 -0700 Subject: [PATCH] Improve acknowledged unicast retry reliability --- src/mesh/NextHopRouter.cpp | 15 ++- src/mesh/NextHopRouter.h | 17 +++- src/mesh/ReliableRouter.cpp | 6 +- test/test_nexthop_routing/test_main.cpp | 118 ++++++++++++++++++++++++ test/test_packet_signing/test_main.cpp | 27 ++++++ 5 files changed, 172 insertions(+), 11 deletions(-) diff --git a/src/mesh/NextHopRouter.cpp b/src/mesh/NextHopRouter.cpp index 0a64a1f1a98..372ef204b71 100644 --- a/src/mesh/NextHopRouter.cpp +++ b/src/mesh/NextHopRouter.cpp @@ -55,12 +55,18 @@ PendingPacket::PendingPacket(meshtastic_MeshPacket *p, uint8_t numRetransmission { packet = p; this->numRetransmissions = numRetransmissions - 1; // We subtract one, because we assume the user just did the first send + this->initialNumRetransmissions = this->numRetransmissions; } /** * Send a packet */ ErrorCode NextHopRouter::send(meshtastic_MeshPacket *p) +{ + return sendWithNextHop(p, true); +} + +ErrorCode NextHopRouter::sendWithNextHop(meshtastic_MeshPacket *p, bool trackRetransmission) { // Add any messages _we_ send to the seen message list (so we will ignore all retransmissions we see) p->relay_node = nodeDB->getLastByteOfNodeNum(getNodeNum()); // First set the relayer to us @@ -71,7 +77,8 @@ ErrorCode NextHopRouter::send(meshtastic_MeshPacket *p) // If it's from us, ReliableRouter already handles retransmissions if want_ack is set. If a next hop is set and hop limit is // not 0 or want_ack is set, start retransmissions - if ((!isFromUs(p) || !p->want_ack) && p->next_hop != NO_NEXT_HOP_PREFERENCE && (p->hop_limit > 0 || p->want_ack)) { + if (trackRetransmission && (!isFromUs(p) || !p->want_ack) && p->next_hop != NO_NEXT_HOP_PREFERENCE && + (p->hop_limit > 0 || p->want_ack)) { if (auto *copy = packetPool.allocCopy(*p)) startRetransmission(copy); // start retransmission for relayed packet } @@ -351,7 +358,7 @@ bool NextHopRouter::stopRetransmission(GlobalPacketId key) auto p = old->packet; /* Only when we already transmitted a packet via LoRa, we will cancel the packet in the Tx queue to avoid canceling a transmission if it was ACKed super fast via MQTT */ - if (old->numRetransmissions < NUM_RELIABLE_RETX - 1) { + if (old->numRetransmissions < old->initialNumRetransmissions) { // We only cancel it if we are the original sender or if we're not a router(_late) if (isFromUs(p) || roleAllowsCancelingFromTxQueue(p)) { // remove the 'original' (identified by originator and packet->id) from the txqueue and free it @@ -461,13 +468,13 @@ int32_t NextHopRouter::doRetransmissions() } } else { if (auto *copy = packetPool.allocCopy(*p.packet)) { - if (NextHopRouter::send(copy) == ERRNO_SHOULD_RELEASE) + if (sendWithNextHop(copy, false) == ERRNO_SHOULD_RELEASE) packetPool.release(copy); } } #else if (auto *copy = packetPool.allocCopy(*p.packet)) { - if (NextHopRouter::send(copy) == ERRNO_SHOULD_RELEASE) + if (sendWithNextHop(copy, false) == ERRNO_SHOULD_RELEASE) packetPool.release(copy); } #endif diff --git a/src/mesh/NextHopRouter.h b/src/mesh/NextHopRouter.h index 3a19191fe31..26cda830aec 100644 --- a/src/mesh/NextHopRouter.h +++ b/src/mesh/NextHopRouter.h @@ -39,6 +39,9 @@ struct PendingPacket { /** Starts at NUM_RETRANSMISSIONS -1 and counts down. Once zero it will be removed from the list */ uint8_t numRetransmissions = 0; + /** Initial remaining retry count, used to detect whether a retry has fired. */ + uint8_t initialNumRetransmissions = 0; + PendingPacket() {} explicit PendingPacket(meshtastic_MeshPacket *p, uint8_t numRetransmissions); }; @@ -77,8 +80,8 @@ class GlobalPacketIdHashFunction Namely, in the PacketHistory, we keep track of (up to 3) relayers of a packet. When the ACK is delivered back to us via a node that also relayed the original packet, we use that node as next hop for the destination from then on. This makes sure that only when there’s a two-way connection, we assign a next hop. Both the ReliableRouter and NextHopRouter will do retransmissions (the - NextHopRouter only 1 time). For the final retry, if no one actually relayed the packet, it will reset the next hop in order to - fall back to the FloodingRouter again. Note that thus also intermediate hops will do a single retransmission if the intended + NextHopRouter only a small number of times). For the final retry, if no one actually relayed the packet, it will reset the next + hop in order to fall back to the FloodingRouter again. Intermediate hops also do bounded retransmissions if the intended next-hop didn’t relay, in order to fix changes in the middle of the route. */ class NextHopRouter : public FloodingRouter @@ -109,10 +112,12 @@ class NextHopRouter : public FloodingRouter return min(d, r); } - // The number of retransmissions intermediate nodes will do (actually 1 less than this) - constexpr static uint8_t NUM_INTERMEDIATE_RETX = 2; - // The number of retransmissions the original sender will do + // Total attempts for directed hop-level delivery, including the initial send. + constexpr static uint8_t NUM_INTERMEDIATE_RETX = 3; + // Existing reliable broadcast budget, including the initial send. constexpr static uint8_t NUM_RELIABLE_RETX = 3; + // Total attempts for acknowledged unicast from the originating node. + constexpr static uint8_t NUM_RELIABLE_UNICAST_ATTEMPTS = 5; // M3: bounded RAM route-health table (reuse-oldest eviction, like PacketHistory) constexpr static uint8_t ROUTE_HEALTH_MAX = 32; // ~12B/slot -> ~384B @@ -155,6 +160,8 @@ class NextHopRouter : public FloodingRouter */ PendingPacket *startRetransmission(meshtastic_MeshPacket *p, uint8_t numReTx = NUM_INTERMEDIATE_RETX); + ErrorCode sendWithNextHop(meshtastic_MeshPacket *p, bool trackRetransmission); + // Return true if we're allowed to cancel a packet in the txQueue (so we may never transmit it even once) bool roleAllowsCancelingFromTxQueue(const meshtastic_MeshPacket *p); diff --git a/src/mesh/ReliableRouter.cpp b/src/mesh/ReliableRouter.cpp index fce6b8a32e2..967bf5492a2 100644 --- a/src/mesh/ReliableRouter.cpp +++ b/src/mesh/ReliableRouter.cpp @@ -24,8 +24,10 @@ ErrorCode ReliableRouter::send(meshtastic_MeshPacket *p) auto copy = packetPool.allocCopy(*p); DEBUG_HEAP_AFTER("ReliableRouter::send", copy); - if (copy) - startRetransmission(copy, NUM_RELIABLE_RETX); + if (copy) { + const uint8_t totalAttempts = isBroadcast(p->to) ? NUM_RELIABLE_RETX : NUM_RELIABLE_UNICAST_ATTEMPTS; + startRetransmission(copy, totalAttempts); + } } /* If we have pending retransmissions, add the airtime of this packet to it, because during that time we cannot receive an diff --git a/test/test_nexthop_routing/test_main.cpp b/test/test_nexthop_routing/test_main.cpp index 7d3dd9eec8f..3bd9cd8d291 100644 --- a/test/test_nexthop_routing/test_main.cpp +++ b/test/test_nexthop_routing/test_main.cpp @@ -11,6 +11,7 @@ #include "TestUtil.h" #include +#include "airtime.h" #include "configuration.h" #include "gps/RTC.h" #include "mesh/Default.h" @@ -93,6 +94,44 @@ class NextHopRouterTestShim : public NextHopRouter using NextHopRouter::relayOpaquePacket; using Router::shouldDecrementHopLimit; // protected in Router + PendingPacket *trackForTest(const meshtastic_MeshPacket &packet, uint8_t totalAttempts) + { + auto *copy = packetPool.allocCopy(packet); + TEST_ASSERT_NOT_NULL(copy); + return startRetransmission(copy, totalAttempts); + } + + PendingPacket *trackWithDefaultBudgetForTest(const meshtastic_MeshPacket &packet) + { + auto *copy = packetPool.allocCopy(packet); + TEST_ASSERT_NOT_NULL(copy); + return startRetransmission(copy); + } + + bool stopForTest(NodeNum from, PacketId id) { return stopRetransmission(from, id); } + + meshtastic_MeshPacket *pendingPacketForTest(NodeNum from, PacketId id) + { + PendingPacket *entry = findPendingPacket(from, id); + return entry ? entry->packet : nullptr; + } + + void fireNextRetryForTest(NodeNum from, PacketId id) + { + PendingPacket *entry = findPendingPacket(from, id); + TEST_ASSERT_NOT_NULL(entry); + entry->nextTxMsec = 0; + doRetransmissions(); + } + + void markOneRetryFiredForTest(NodeNum from, PacketId id) + { + PendingPacket *entry = findPendingPacket(from, id); + TEST_ASSERT_NOT_NULL(entry); + TEST_ASSERT_GREATER_THAN_UINT8(0, entry->numRetransmissions); + --entry->numRetransmissions; + } + void resetRouteHealthForTest() { for (auto &h : routeHealth) @@ -112,6 +151,7 @@ class MockRadioInterface : public RadioInterface sendCount++; lastHopLimit = p->hop_limit; lastHopStart = p->hop_start; + sentNextHops.push_back(p->next_hop); if (declineAll || p->to == NODENUM_BROADCAST_NO_LORA) return ERRNO_SHOULD_RELEASE; @@ -126,10 +166,18 @@ class MockRadioInterface : public RadioInterface return 0; } + bool cancelSending(NodeNum, PacketId) override + { + cancelCount++; + return true; + } + int sendCount = 0; + uint32_t cancelCount = 0; bool declineAll = false; uint8_t lastHopLimit = 0; uint8_t lastHopStart = 0; + std::vector sentNextHops; }; static MockNodeDB *mockNodeDB = nullptr; @@ -472,6 +520,68 @@ static meshtastic_MeshPacket makeRebroadcastCandidate(NodeNum to) return p; } +void test_pending_does_not_cancel_radio_queue_before_first_retry(void) +{ + MockRadioInterface *mockIface = installMockIface(); + meshtastic_MeshPacket p = makeRebroadcastCandidate(0x33333333); + p.from = kLocalNode; + p.id = 0x51000001; + shim->trackForTest(p, 5); + + TEST_ASSERT_TRUE(shim->stopForTest(kLocalNode, p.id)); + TEST_ASSERT_EQUAL_UINT32(0, mockIface->cancelCount); +} + +void test_pending_cancels_radio_queue_after_first_retry_for_any_budget(void) +{ + MockRadioInterface *mockIface = installMockIface(); + meshtastic_MeshPacket p = makeRebroadcastCandidate(0x33333333); + p.from = kLocalNode; + p.id = 0x51000002; + shim->trackForTest(p, 5); + shim->markOneRetryFiredForTest(kLocalNode, p.id); + + TEST_ASSERT_TRUE(shim->stopForTest(kLocalNode, p.id)); + TEST_ASSERT_EQUAL_UINT32(1, mockIface->cancelCount); +} + +void test_directed_hop_tracks_three_total_attempts(void) +{ + installMockIface(); + meshtastic_MeshPacket p = makeRebroadcastCandidate(0x33333333); + p.id = 0x51530003; + + PendingPacket *entry = shim->trackWithDefaultBudgetForTest(p); + TEST_ASSERT_NOT_NULL(entry); + TEST_ASSERT_EQUAL_UINT8(3, entry->initialNumRetransmissions + 1); + TEST_ASSERT_TRUE(shim->stopForTest(p.from, p.id)); +} + +void test_intermediate_three_attempts_preserve_record_and_flood_last(void) +{ + MockRadioInterface *mockIface = installMockIface(); + constexpr NodeNum dest = 0x33333333; + mockNodeDB->addNode(dest, 2, true, 60, meshtastic_Config_DeviceConfig_Role_CLIENT, false, false, 0xAB); + mockNodeDB->addNode(0x000007AB, 0, true, 60); + + meshtastic_MeshPacket p = makeRebroadcastCandidate(dest); + p.id = 0x51530004; + p.next_hop = 0xAB; + PendingPacket *entry = shim->trackWithDefaultBudgetForTest(p); + TEST_ASSERT_NOT_NULL(entry); + meshtastic_MeshPacket *trackedPacket = entry->packet; + + shim->fireNextRetryForTest(p.from, p.id); + TEST_ASSERT_EQUAL_UINT32(1, mockIface->sentNextHops.size()); + TEST_ASSERT_EQUAL_HEX8(0xAB, mockIface->sentNextHops[0]); + TEST_ASSERT_EQUAL_PTR(trackedPacket, shim->pendingPacketForTest(p.from, p.id)); + + shim->fireNextRetryForTest(p.from, p.id); + TEST_ASSERT_EQUAL_UINT32(2, mockIface->sentNextHops.size()); + TEST_ASSERT_EQUAL_HEX8(NO_NEXT_HOP_PREFERENCE, mockIface->sentNextHops[1]); + TEST_ASSERT_TRUE(shim->stopForTest(p.from, p.id)); +} + // Control: proves the NO_LORA case below turns on the `to` field alone. void test_rebroadcast_normal_broadcast_is_relayed(void) { @@ -549,6 +659,8 @@ void test_event_mode_hop_behavior(void) void setup() { initializeTestEnvironment(); + AirTime testAirTime; + airTime = &testAirTime; UNITY_BEGIN(); mockNodeDB = new MockNodeDB(); @@ -594,6 +706,12 @@ void setup() RUN_TEST(test_hoplimit_decrement_on_colliding_favorites); RUN_TEST(test_hoplimit_decrement_when_resolved_not_favorite); + printf("\n=== pending retransmission bookkeeping ===\n"); + RUN_TEST(test_pending_does_not_cancel_radio_queue_before_first_retry); + RUN_TEST(test_pending_cancels_radio_queue_after_first_retry_for_any_budget); + RUN_TEST(test_directed_hop_tracks_three_total_attempts); + RUN_TEST(test_intermediate_three_attempts_preserve_record_and_flood_last); + printf("\n=== rebroadcast of NODENUM_BROADCAST_NO_LORA ===\n"); RUN_TEST(test_rebroadcast_normal_broadcast_is_relayed); RUN_TEST(test_rebroadcast_no_lora_broadcast_is_not_relayed); diff --git a/test/test_packet_signing/test_main.cpp b/test/test_packet_signing/test_main.cpp index c3abb7bc957..0e9f1b7cfb0 100644 --- a/test/test_packet_signing/test_main.cpp +++ b/test/test_packet_signing/test_main.cpp @@ -173,6 +173,11 @@ class AuthPipelineRouter : public ReliableRouter PendingPacket *entry = findPendingPacket(from, id); return entry ? entry->nextTxMsec : 0; } + uint8_t pendingTotalAttempts(NodeNum from, PacketId id) + { + PendingPacket *entry = findPendingPacket(from, id); + return entry ? entry->initialNumRetransmissions + 1 : 0; + } size_t pendingCount() const { return pending.size(); } void clearPending() { @@ -1508,6 +1513,26 @@ void test_C14_duty_cycle_limited_reliable_send_remains_pending(void) initRegion(); } +void test_C15_reliable_unicast_tracks_five_total_attempts(void) +{ + meshtastic_MeshPacket p = makeDecoded(LOCAL_NODE, REMOTE_NODE, meshtastic_PortNum_ROUTING_APP, SMALL_PAYLOAD); + p.id = 0x51530001; + p.want_ack = true; + + TEST_ASSERT_EQUAL(ERRNO_OK, pipelineRouter->send(packetPool.allocCopy(p))); + TEST_ASSERT_EQUAL_UINT8(5, pipelineRouter->pendingTotalAttempts(LOCAL_NODE, p.id)); +} + +void test_C16_reliable_broadcast_keeps_three_total_attempts(void) +{ + meshtastic_MeshPacket p = makeDecoded(LOCAL_NODE, NODENUM_BROADCAST, meshtastic_PortNum_ROUTING_APP, SMALL_PAYLOAD); + p.id = 0x51530002; + p.want_ack = true; + + TEST_ASSERT_EQUAL(ERRNO_OK, pipelineRouter->send(packetPool.allocCopy(p))); + TEST_ASSERT_EQUAL_UINT8(3, pipelineRouter->pendingTotalAttempts(LOCAL_NODE, p.id)); +} + // C5: the packet survives (C4) but the identity claim inside it must not land - the pubkey guard // can't tell a signer from an impersonator replaying its (public) key. Only the write is refused. void test_N5_unsigned_unicast_nodeinfo_from_signer_does_not_change_name(void) @@ -1976,6 +2001,8 @@ void setup() RUN_TEST(test_C12_exact_authenticated_replay_reuses_verdict_without_collision_bypass); RUN_TEST(test_C13_failed_initial_reliable_send_does_not_retry); RUN_TEST(test_C14_duty_cycle_limited_reliable_send_remains_pending); + RUN_TEST(test_C15_reliable_unicast_tracks_five_total_attempts); + RUN_TEST(test_C16_reliable_broadcast_keeps_three_total_attempts); printf("\n=== Group N: NodeInfoModule authentication ===\n"); RUN_TEST(test_N1_unsigned_nodeinfo_from_signer_dropped); RUN_TEST(test_N2_signed_nodeinfo_from_signer_not_dropped);