From dd9347be62d651026bca15769569901b7185f990 Mon Sep 17 00:00:00 2001 From: nomdetom Date: Wed, 8 Jul 2026 18:35:09 +0100 Subject: [PATCH 1/9] Tolerate a configurable number of heard repeats before cancelling our own rebroadcast FloodingRouter previously cancelled its own scheduled rebroadcast the instant it heard any duplicate of a packet from another node. Add a small per-(sender, id) ring buffer that counts duplicates heard so far, and a compile-time per-portnum threshold (getDupeCancelThreshold) controlling how many repeats to tolerate before giving up - default of 1 preserves existing behavior everywhere. Enable it for TEXT_MESSAGE_APP/TEXT_MESSAGE_COMPRESSED_APP (threshold 2): no ACK/retry safety net for broadcast chat, and this applies to DM text messages too since NextHopRouter::shouldFilterReceived calls the same inherited perhapsCancelDupe/getDupeCancelThreshold with no addressing-based branching. Adds test/test_flooding_router covering the ring buffer, the default/text- message thresholds, and the perhapsCancelDupe threshold-gating integration. clod helped too --- src/mesh/FloodingRouter.cpp | 80 ++++++- src/mesh/FloodingRouter.h | 26 +++ test/native-suite-count | 2 +- test/test_flooding_router/test_main.cpp | 282 ++++++++++++++++++++++++ 4 files changed, 385 insertions(+), 5 deletions(-) create mode 100644 test/test_flooding_router/test_main.cpp diff --git a/src/mesh/FloodingRouter.cpp b/src/mesh/FloodingRouter.cpp index 13f98299f93..2fba8f427f9 100644 --- a/src/mesh/FloodingRouter.cpp +++ b/src/mesh/FloodingRouter.cpp @@ -9,6 +9,72 @@ #include "modules/TraceRouteModule.h" #endif +// Compile-time, per-portnum threshold for how many duplicate rebroadcasts of a packet we must hear +// before giving up on our own scheduled rebroadcast of it. The default of 1 (used for any portnum +// not given a case below, and for packets we can't decode - see below) preserves the historical +// behavior: cancel our own rebroadcast as soon as we hear the first duplicate. Raising a portnum's +// threshold makes this node more persistent for that traffic type, continuing to retransmit even +// after hearing some other nodes already repeat it - at the cost of extra airtime for packets +// that are, on balance, probably going to reach their destination anyway. +// +// This same switch applies uniformly to broadcasts and DMs: NextHopRouter::shouldFilterReceived +// calls this same (inherited, non-overridden) getDupeCancelThreshold() when it hears a duplicate +// it wasn't explicitly asked to relay - both in the fallback-to-flooding case and the general +// "duplicate heard, we're not the assigned next hop" case - so a DM of a listed portnum gets the +// same extra tolerance as a broadcast of that portnum, with no separate to/from-based gating needed. +// +// This is a compile-time table (not a runtime/protobuf config) because the desired threshold is +// expected to vary per packet type rather than being a single device-wide knob; edit it to tune +// for a given deployment. +uint8_t FloodingRouter::getDupeCancelThreshold(const meshtastic_MeshPacket *p) +{ + // Portnum is only visible once a packet is decoded (i.e. we hold the channel key); packets we + // can't decrypt always get the default of 1, since we have no per-portnum signal to act on. + if (p->which_payload_variant != meshtastic_MeshPacket_decoded_tag) + return 1; + + switch (p->decoded.portnum) { + case meshtastic_PortNum_TEXT_MESSAGE_APP: + case meshtastic_PortNum_TEXT_MESSAGE_COMPRESSED_APP: + // User-visible chat, broadcast or DM: no ACK/retry safety net for broadcasts, and no other + // portnum-specific reason to prefer airtime savings over delivery odds. Tolerate one repeat + // heard from another node before giving up on our own rebroadcast. + return 2; + default: + return 1; + } +} + +uint8_t FloodingRouter::registerDupeHeard(NodeNum sender, PacketId id) +{ + for (auto &entry : dupeCounts) { + if (entry.id == id && entry.sender == sender) { + if (entry.count < UINT8_MAX) + entry.count++; + return entry.count; + } + } + // Not tracked yet: claim the next ring slot, evicting whatever packet (if any) was there. + DupeCountEntry &slot = dupeCounts[dupeCountsNextSlot]; + dupeCountsNextSlot = (dupeCountsNextSlot + 1) % DUPE_COUNT_TRACKER_SIZE; + slot.sender = sender; + slot.id = id; + slot.count = 1; + return slot.count; +} + +void FloodingRouter::clearDupeCount(NodeNum sender, PacketId id) +{ + for (auto &entry : dupeCounts) { + if (entry.id == id && entry.sender == sender) { + entry.sender = 0; + entry.id = 0; + entry.count = 0; + return; + } + } +} + FloodingRouter::FloodingRouter() {} /** @@ -138,10 +204,16 @@ bool FloodingRouter::roleAllowsCancelingDupe(const meshtastic_MeshPacket *p) void FloodingRouter::perhapsCancelDupe(const meshtastic_MeshPacket *p) { if (p->transport_mechanism == meshtastic_MeshPacket_TransportMechanism_TRANSPORT_LORA && roleAllowsCancelingDupe(p)) { - // cancel rebroadcast of this message *if* there was already one, unless we're a router! - // But only LoRa packets should be able to trigger this. - if (Router::cancelSending(p->from, p->id)) - txRelayCanceled++; + // Wait for the configured number of duplicate rebroadcasts (see dupeCancelThresholds) before + // giving up on our own rebroadcast; below that, note the duplicate and keep waiting. + uint8_t dupesHeard = registerDupeHeard(p->from, p->id); + if (dupesHeard >= getDupeCancelThreshold(p)) { + // cancel rebroadcast of this message *if* there was already one, unless we're a router! + // But only LoRa packets should be able to trigger this. + if (Router::cancelSending(p->from, p->id)) + txRelayCanceled++; + clearDupeCount(p->from, p->id); + } } if (config.device.role == meshtastic_Config_DeviceConfig_Role_ROUTER_LATE && iface) { iface->clampToLateRebroadcastWindow(getFrom(p), p->id); diff --git a/src/mesh/FloodingRouter.h b/src/mesh/FloodingRouter.h index e8a2e9685fa..d446cdff01d 100644 --- a/src/mesh/FloodingRouter.h +++ b/src/mesh/FloodingRouter.h @@ -75,4 +75,30 @@ class FloodingRouter : public Router // Return true if we are a rebroadcaster bool isRebroadcaster(); + + // How many duplicate rebroadcasts of a packet we require to hear (see the per-portnum switch in + // FloodingRouter.cpp) before giving up on our own scheduled rebroadcast of it. Virtual solely so + // tests can override it to inject a threshold without needing a real portnum case (see + // test/test_flooding_router). + virtual uint8_t getDupeCancelThreshold(const meshtastic_MeshPacket *p); + + // Tracks how many duplicates we've heard so far, per (sender, id), for packets we currently + // have one queued to rebroadcast ourselves. Bounded, ephemeral ring buffer - not a persistent + // record like PacketHistory: entries are only meaningful while our own rebroadcast is still + // pending, and naturally get evicted/reused as the ring wraps. + uint8_t registerDupeHeard(NodeNum sender, PacketId id); + + // Clears tracking state for a (sender, id) once we've acted on it (cancelled our rebroadcast), + // so a reused ring slot can't cause a stale hit against an unrelated future packet. + void clearDupeCount(NodeNum sender, PacketId id); + + private: + static constexpr uint8_t DUPE_COUNT_TRACKER_SIZE = 8; + struct DupeCountEntry { + NodeNum sender = 0; + PacketId id = 0; + uint8_t count = 0; + }; + DupeCountEntry dupeCounts[DUPE_COUNT_TRACKER_SIZE]; + uint8_t dupeCountsNextSlot = 0; }; \ No newline at end of file diff --git a/test/native-suite-count b/test/native-suite-count index 8f92bfdd497..e522732c77e 100644 --- a/test/native-suite-count +++ b/test/native-suite-count @@ -1 +1 @@ -35 +38 diff --git a/test/test_flooding_router/test_main.cpp b/test/test_flooding_router/test_main.cpp new file mode 100644 index 00000000000..3eeecb40c54 --- /dev/null +++ b/test/test_flooding_router/test_main.cpp @@ -0,0 +1,282 @@ +// Unit tests for FloodingRouter's duplicate-rebroadcast bookkeeping: +// - registerDupeHeard/clearDupeCount: the small ring buffer counting how many times we've +// heard a duplicate of a packet we have queued to rebroadcast ourselves. +// - getDupeCancelThreshold: the compile-time, per-portnum threshold (see FloodingRouter.cpp) +// controlling how many duplicates we tolerate before giving up on our own rebroadcast. +// - perhapsCancelDupe: the integration of the two - only actually gives up (and clears its +// tracking state) once the configured threshold has been reached. +// +// perhapsCancelDupe() is safe to call directly here even with no RadioInterface: Router:: +// cancelSending() is iface-null-safe (just returns false), so the queue-cancellation side effect +// is a no-op - what we can and do observe is the dupe-count bookkeeping around it. + +#include "MeshTypes.h" // before TestUtil.h: provides NodeNum etc. +#include "TestUtil.h" +#include + +#include "configuration.h" +#include "mesh/FloodingRouter.h" + +static constexpr NodeNum kSender1 = 0x000005AB; +static constexpr NodeNum kSender2 = 0x000006CD; +static constexpr PacketId kId1 = 0x1001; +static constexpr PacketId kId2 = 0x2002; + +// --------------------------------------------------------------------------- +// Test shim - re-exposes the protected dupe-tracking helpers, and allows overriding +// getDupeCancelThreshold so tests don't depend on any real portnum having a non-default case. +// Nulls cryptLock before it's rebuilt so the Router base can be (re)constructed (same pattern as +// test_nexthop_routing's NextHopRouterTestShim / test_mqtt's MockRouter). +// --------------------------------------------------------------------------- +class FloodingRouterTestShim : public FloodingRouter +{ + public: + FloodingRouterTestShim() : FloodingRouter() + { + delete cryptLock; + cryptLock = nullptr; + } + + using FloodingRouter::clearDupeCount; + using FloodingRouter::getDupeCancelThreshold; + using FloodingRouter::perhapsCancelDupe; + using FloodingRouter::registerDupeHeard; + + // Test-only override: bypasses the compile-time portnum switch so tests can exercise + // perhapsCancelDupe's threshold-gating without needing a real portnum case. + uint8_t testThreshold = 1; + uint8_t getDupeCancelThreshold(const meshtastic_MeshPacket *p) override { return testThreshold; } + + protected: + // FloodingRouter::perhapsRebroadcast is pure virtual; not exercised by these tests. + bool perhapsRebroadcast(const meshtastic_MeshPacket *p) override { return false; } +}; + +// A second, minimal shim that does NOT override getDupeCancelThreshold, for testing the real +// compile-time default implementation directly (FloodingRouter itself is abstract - perhapsRebroadcast +// is pure virtual - so it can't be constructed on its own). +class FloodingRouterPlainShim : public FloodingRouter +{ + public: + FloodingRouterPlainShim() : FloodingRouter() + { + delete cryptLock; + cryptLock = nullptr; + } + + using FloodingRouter::getDupeCancelThreshold; + + protected: + bool perhapsRebroadcast(const meshtastic_MeshPacket *p) override { return false; } +}; + +static FloodingRouterTestShim *shim = nullptr; +static FloodingRouterPlainShim *plainShim = nullptr; + +static meshtastic_MeshPacket makeDupePacket(NodeNum from, PacketId id) +{ + meshtastic_MeshPacket p = meshtastic_MeshPacket_init_zero; + p.from = from; + p.id = id; + p.transport_mechanism = meshtastic_MeshPacket_TransportMechanism_TRANSPORT_LORA; + p.which_payload_variant = meshtastic_MeshPacket_encrypted_tag; // undecodable by default + return p; +} + +void setUp(void) +{ + config.device.role = meshtastic_Config_DeviceConfig_Role_CLIENT; + shim->testThreshold = 1; + // Clear any tracking state a previous test may have left behind for our fixed test keys. + shim->clearDupeCount(kSender1, kId1); + shim->clearDupeCount(kSender2, kId2); +} + +void tearDown(void) {} + +// =========================================================================== +// Group 1 - registerDupeHeard / clearDupeCount (the ring buffer) +// =========================================================================== + +void test_registerDupeHeard_starts_at_one(void) +{ + TEST_ASSERT_EQUAL_UINT8(1, shim->registerDupeHeard(kSender1, kId1)); +} + +void test_registerDupeHeard_increments_on_repeat(void) +{ + TEST_ASSERT_EQUAL_UINT8(1, shim->registerDupeHeard(kSender1, kId1)); + TEST_ASSERT_EQUAL_UINT8(2, shim->registerDupeHeard(kSender1, kId1)); + TEST_ASSERT_EQUAL_UINT8(3, shim->registerDupeHeard(kSender1, kId1)); +} + +void test_registerDupeHeard_independent_per_packet(void) +{ + TEST_ASSERT_EQUAL_UINT8(1, shim->registerDupeHeard(kSender1, kId1)); + TEST_ASSERT_EQUAL_UINT8(2, shim->registerDupeHeard(kSender1, kId1)); + // A different (sender, id) pair tracks its own independent count. + TEST_ASSERT_EQUAL_UINT8(1, shim->registerDupeHeard(kSender2, kId2)); + TEST_ASSERT_EQUAL_UINT8(2, shim->registerDupeHeard(kSender2, kId2)); + // The first pair's count is untouched by the second's activity. + TEST_ASSERT_EQUAL_UINT8(3, shim->registerDupeHeard(kSender1, kId1)); +} + +void test_clearDupeCount_resets_tracking(void) +{ + shim->registerDupeHeard(kSender1, kId1); + shim->registerDupeHeard(kSender1, kId1); + shim->clearDupeCount(kSender1, kId1); + // After clearing, the next duplicate heard for this packet starts fresh at 1. + TEST_ASSERT_EQUAL_UINT8(1, shim->registerDupeHeard(kSender1, kId1)); +} + +void test_clearDupeCount_of_untracked_packet_is_noop(void) +{ + // Clearing a (sender, id) that was never registered must not crash or disturb other entries. + shim->registerDupeHeard(kSender1, kId1); + shim->clearDupeCount(0xFFFFFFFF, 0xFFFFFFFF); + TEST_ASSERT_EQUAL_UINT8(2, shim->registerDupeHeard(kSender1, kId1)); +} + +void test_registerDupeHeard_ring_eviction_does_not_crash(void) +{ + // DUPE_COUNT_TRACKER_SIZE is 8 (see FloodingRouter.h); track more distinct packets than that + // and confirm the ring buffer just evicts older entries rather than misbehaving. + for (uint32_t i = 0; i < 20; i++) { + uint8_t count = shim->registerDupeHeard(kSender1, 0x10000 + i); + TEST_ASSERT_EQUAL_UINT8(1, count); // each is a fresh (sender, id) pair + } +} + +// =========================================================================== +// Group 2 - getDupeCancelThreshold (compile-time per-portnum default) +// =========================================================================== + +void test_default_threshold_is_one_for_undecoded_packet(void) +{ + meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); + p.which_payload_variant = meshtastic_MeshPacket_encrypted_tag; + TEST_ASSERT_EQUAL_UINT8(1, plainShim->getDupeCancelThreshold(&p)); +} + +void test_default_threshold_is_one_for_unlisted_portnum(void) +{ + meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); + p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + p.decoded.portnum = meshtastic_PortNum_POSITION_APP; // no case for this portnum + TEST_ASSERT_EQUAL_UINT8(1, plainShim->getDupeCancelThreshold(&p)); +} + +void test_text_message_threshold_is_two(void) +{ + meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); + p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + p.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP; + TEST_ASSERT_EQUAL_UINT8(2, plainShim->getDupeCancelThreshold(&p)); +} + +void test_text_message_compressed_threshold_is_two(void) +{ + meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); + p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + p.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_COMPRESSED_APP; + TEST_ASSERT_EQUAL_UINT8(2, plainShim->getDupeCancelThreshold(&p)); +} + +void test_text_message_threshold_applies_regardless_of_addressing(void) +{ + // getDupeCancelThreshold only inspects portnum, not p->to - so a DM (to a specific node) + // gets the same threshold as a broadcast for the same portnum. This is what lets + // NextHopRouter::shouldFilterReceived's call to the same inherited perhapsCancelDupe/ + // getDupeCancelThreshold extend the tolerance to DM text messages too, with no separate + // addressing-based gate. + meshtastic_MeshPacket dm = makeDupePacket(kSender1, kId1); + dm.to = 0x12345678; // a specific node, not NODENUM_BROADCAST + dm.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + dm.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP; + TEST_ASSERT_EQUAL_UINT8(2, plainShim->getDupeCancelThreshold(&dm)); +} + +// =========================================================================== +// Group 3 - perhapsCancelDupe (integration: threshold gates the cancel + clears tracking) +// =========================================================================== + +void test_perhapsCancelDupe_clears_tracking_once_threshold_reached(void) +{ + shim->testThreshold = 1; // historical behavior: cancel (and clear) on the very first duplicate + meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); + + shim->perhapsCancelDupe(&p); + + // Threshold (1) was reached on the first call, so tracking was cleared - the next duplicate + // heard for this packet starts a fresh count, rather than continuing to accumulate. + TEST_ASSERT_EQUAL_UINT8(1, shim->registerDupeHeard(kSender1, kId1)); +} + +void test_perhapsCancelDupe_waits_for_configured_repeats(void) +{ + shim->testThreshold = 3; // this node should tolerate hearing 2 repeats before giving up + meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); + + shim->perhapsCancelDupe(&p); // 1st duplicate heard -> internal count 1, below threshold + + // Directly registering one more duplicate both (a) advances the count to 2 for the next + // step, and (b) proves tracking was NOT cleared by the call above: a cleared entry would + // restart at 1, not continue to 2. + TEST_ASSERT_EQUAL_UINT8(2, shim->registerDupeHeard(kSender1, kId1)); + + shim->perhapsCancelDupe(&p); // 3rd duplicate heard -> internal count 3, reaches threshold + // Threshold reached inside the call above, so tracking was cleared - starts fresh at 1. + TEST_ASSERT_EQUAL_UINT8(1, shim->registerDupeHeard(kSender1, kId1)); +} + +void test_perhapsCancelDupe_router_role_never_tracks_or_cancels(void) +{ + // ROUTER never cancels its own rebroadcast (roleAllowsCancelingDupe() gates it out entirely), + // so it should also never bother tracking a dupe count for it. + config.device.role = meshtastic_Config_DeviceConfig_Role_ROUTER; + shim->testThreshold = 1; + meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); + + shim->perhapsCancelDupe(&p); + shim->perhapsCancelDupe(&p); + + // Since roleAllowsCancelingDupe() short-circuits before registerDupeHeard() is ever called, + // this key was never tracked - the first direct registerDupeHeard() call starts fresh at 1. + TEST_ASSERT_EQUAL_UINT8(1, shim->registerDupeHeard(kSender1, kId1)); +} + +// =========================================================================== + +void setup() +{ + initializeTestEnvironment(); + UNITY_BEGIN(); + + shim = new FloodingRouterTestShim(); + plainShim = new FloodingRouterPlainShim(); + + printf("\n=== registerDupeHeard / clearDupeCount (ring buffer) ===\n"); + RUN_TEST(test_registerDupeHeard_starts_at_one); + RUN_TEST(test_registerDupeHeard_increments_on_repeat); + RUN_TEST(test_registerDupeHeard_independent_per_packet); + RUN_TEST(test_clearDupeCount_resets_tracking); + RUN_TEST(test_clearDupeCount_of_untracked_packet_is_noop); + RUN_TEST(test_registerDupeHeard_ring_eviction_does_not_crash); + + printf("\n=== getDupeCancelThreshold (compile-time default) ===\n"); + RUN_TEST(test_default_threshold_is_one_for_undecoded_packet); + RUN_TEST(test_default_threshold_is_one_for_unlisted_portnum); + RUN_TEST(test_text_message_threshold_is_two); + RUN_TEST(test_text_message_compressed_threshold_is_two); + RUN_TEST(test_text_message_threshold_applies_regardless_of_addressing); + + printf("\n=== perhapsCancelDupe (threshold gating) ===\n"); + RUN_TEST(test_perhapsCancelDupe_clears_tracking_once_threshold_reached); + RUN_TEST(test_perhapsCancelDupe_waits_for_configured_repeats); + RUN_TEST(test_perhapsCancelDupe_router_role_never_tracks_or_cancels); + + exit(UNITY_END()); +} + +void loop() {} From 32b2167d29f583255cd71de6bc030e882ce7bb88 Mon Sep 17 00:00:00 2001 From: nomdetom Date: Wed, 8 Jul 2026 19:04:11 +0100 Subject: [PATCH 2/9] Suppress extra-repeat tolerance when the mesh is busy or dense Add meshTooBusyForExtraRepeats() to FloodingRouter: forces getDupeCancelThreshold() back down to 1 (cancel on first duplicate heard) whenever channel utilization is above 10%, TX air utilization is above 4%, or HopScalingModule estimates more than 10 active direct neighbors - regardless of what the per-portnum table says. Spending extra airtime on repeat-tolerant rebroadcasts isn't worth it once the mesh is already congested or dense enough that the packet is probably getting through some other way. Adds a test-only HopScalingModule::setLastPerHopCountsForTest() setter (mirrors the existing PIO_UNIT_TESTING accessors) so tests can inject a specific direct-neighbor count without driving the full histogram/rollover machinery. Extends test/test_flooding_router with the busy-channel-util, busy-air-util-tx, and busy-direct-node-count gate cases (including at-threshold boundary checks), plus a regression test confirming the gate has no effect on portnums that were never extended past 1. clod helped too --- src/mesh/FloodingRouter.cpp | 49 ++++++++- src/modules/HopScalingModule.h | 4 + test/test_flooding_router/test_main.cpp | 140 ++++++++++++++++++++++++ 3 files changed, 191 insertions(+), 2 deletions(-) diff --git a/src/mesh/FloodingRouter.cpp b/src/mesh/FloodingRouter.cpp index 2fba8f427f9..2debfcd4eab 100644 --- a/src/mesh/FloodingRouter.cpp +++ b/src/mesh/FloodingRouter.cpp @@ -1,14 +1,45 @@ #include "FloodingRouter.h" #include "MeshTypes.h" #include "NodeDB.h" +#include "airtime.h" #include "configuration.h" #include "mesh-pb-constants.h" #include "meshUtils.h" +#include "modules/HopScalingModule.h" #include "modules/TextMessageModule.h" #if !MESHTASTIC_EXCLUDE_TRACEROUTE #include "modules/TraceRouteModule.h" #endif +namespace +{ +// Above any of these, the mesh is busy/dense enough that extra-repeat tolerance (see +// getDupeCancelThreshold) would just be adding airtime to an already-congested channel for +// little extra delivery benefit - so we fall back to the historical "cancel on first duplicate" +// behavior regardless of what the portnum table says. +constexpr float BUSY_CHANNEL_UTIL_PERCENT = 10.0f; +constexpr float BUSY_AIR_UTIL_TX_PERCENT = 4.0f; +constexpr uint16_t BUSY_DIRECT_ACTIVE_NODES = 10; + +// True if channel/air utilization or estimated direct-neighbor density indicates the mesh is +// too busy to spend extra airtime on repeat-tolerant rebroadcasts. +bool meshTooBusyForExtraRepeats() +{ + if (airTime && airTime->channelUtilizationPercent() > BUSY_CHANNEL_UTIL_PERCENT) + return true; + if (airTime && airTime->utilizationTXPercent() > BUSY_AIR_UTIL_TX_PERCENT) + return true; +#if HAS_VARIABLE_HOPS + // getLastPerHopCounts().perHop[0] is HopScalingModule's estimate of active (heard within its + // rolling window) direct (hop_away == 0) neighbors - the same "active and direct" notion used + // for its own hop-limit recommendation. + if (hopScalingModule && hopScalingModule->getLastPerHopCounts().perHop[0] > BUSY_DIRECT_ACTIVE_NODES) + return true; +#endif + return false; +} +} // namespace + // Compile-time, per-portnum threshold for how many duplicate rebroadcasts of a packet we must hear // before giving up on our own scheduled rebroadcast of it. The default of 1 (used for any portnum // not given a case below, and for packets we can't decode - see below) preserves the historical @@ -26,6 +57,9 @@ // This is a compile-time table (not a runtime/protobuf config) because the desired threshold is // expected to vary per packet type rather than being a single device-wide knob; edit it to tune // for a given deployment. +// +// Regardless of the table below, meshTooBusyForExtraRepeats() (channel/air utilization or direct- +// neighbor density too high) always forces the threshold back down to 1 - see its definition above. uint8_t FloodingRouter::getDupeCancelThreshold(const meshtastic_MeshPacket *p) { // Portnum is only visible once a packet is decoded (i.e. we hold the channel key); packets we @@ -33,16 +67,27 @@ uint8_t FloodingRouter::getDupeCancelThreshold(const meshtastic_MeshPacket *p) if (p->which_payload_variant != meshtastic_MeshPacket_decoded_tag) return 1; + uint8_t threshold; switch (p->decoded.portnum) { case meshtastic_PortNum_TEXT_MESSAGE_APP: case meshtastic_PortNum_TEXT_MESSAGE_COMPRESSED_APP: // User-visible chat, broadcast or DM: no ACK/retry safety net for broadcasts, and no other // portnum-specific reason to prefer airtime savings over delivery odds. Tolerate one repeat // heard from another node before giving up on our own rebroadcast. - return 2; + threshold = 2; + break; default: - return 1; + threshold = 1; + break; } + + // A busy/dense mesh overrides any portnum-specific extra tolerance: not worth spending more + // airtime on repeats when the channel is already congested or there are many direct neighbors + // to reach anyway. + if (threshold > 1 && meshTooBusyForExtraRepeats()) + return 1; + + return threshold; } uint8_t FloodingRouter::registerDupeHeard(NodeNum sender, PacketId id) diff --git a/src/modules/HopScalingModule.h b/src/modules/HopScalingModule.h index 70d87428a52..b16130f8656 100644 --- a/src/modules/HopScalingModule.h +++ b/src/modules/HopScalingModule.h @@ -204,6 +204,10 @@ class HopScalingModule : private concurrency::OSThread uint16_t getHashSeed() const { return hashSeed; } /// Expose hashNodeId for tests that need to compute which node IDs pass a given denominator. uint16_t hashNodeIdPublic(uint32_t nodeId) const { return hashNodeId(nodeId); } + /// Directly inject the last-rollover per-hop counts, bypassing the histogram/rollHour() + /// machinery. For tests (e.g. FloodingRouter's dupe-repeat gate) that only need a specific + /// getLastPerHopCounts() result and don't care how it was derived. + void setLastPerHopCountsForTest(const PerHopCounts &counts) { lastPerHopCounts = counts; } #endif protected: diff --git a/test/test_flooding_router/test_main.cpp b/test/test_flooding_router/test_main.cpp index 3eeecb40c54..dfa25b19d2a 100644 --- a/test/test_flooding_router/test_main.cpp +++ b/test/test_flooding_router/test_main.cpp @@ -14,8 +14,10 @@ #include "TestUtil.h" #include +#include "airtime.h" #include "configuration.h" #include "mesh/FloodingRouter.h" +#include "modules/HopScalingModule.h" static constexpr NodeNum kSender1 = 0x000005AB; static constexpr NodeNum kSender2 = 0x000006CD; @@ -73,6 +75,62 @@ class FloodingRouterPlainShim : public FloodingRouter static FloodingRouterTestShim *shim = nullptr; static FloodingRouterPlainShim *plainShim = nullptr; +// --------------------------------------------------------------------------- +// Scoped helpers to simulate a busy mesh for meshTooBusyForExtraRepeats() (see FloodingRouter.cpp). +// Install a global airTime/hopScalingModule reporting the given condition for the enclosing scope, +// restoring the previous pointer on destruction. Mirrors test_traffic_management's ScopedBusyAirTime. +// --------------------------------------------------------------------------- +class ScopedChannelUtil +{ + public: + explicit ScopedChannelUtil(float percent) : previous(airTime) + { + // channelUtilizationPercent() == sum(channelUtilization[]) / (PERIODS * 10 * 1000) * 100 + busy.channelUtilization[0] = static_cast(percent / 100.0f * CHANNEL_UTILIZATION_PERIODS * 10 * 1000); + airTime = &busy; + } + ~ScopedChannelUtil() { airTime = previous; } + + private: + AirTime busy; + AirTime *previous; +}; + +class ScopedAirUtilTx +{ + public: + explicit ScopedAirUtilTx(float percent) : previous(airTime) + { + // utilizationTXPercent() == sum(utilizationTX[]) / MS_IN_HOUR * 100 + busy.utilizationTX[0] = static_cast(percent / 100.0f * MS_IN_HOUR); + airTime = &busy; + } + ~ScopedAirUtilTx() { airTime = previous; } + + private: + AirTime busy; + AirTime *previous; +}; + +#if HAS_VARIABLE_HOPS +class ScopedDirectActiveNodes +{ + public: + explicit ScopedDirectActiveNodes(uint16_t count) : previous(hopScalingModule) + { + HopScalingModule::PerHopCounts counts; + counts.perHop[0] = count; + busy.setLastPerHopCountsForTest(counts); + hopScalingModule = &busy; + } + ~ScopedDirectActiveNodes() { hopScalingModule = previous; } + + private: + HopScalingModule busy; + HopScalingModule *previous; +}; +#endif + static meshtastic_MeshPacket makeDupePacket(NodeNum from, PacketId id) { meshtastic_MeshPacket p = meshtastic_MeshPacket_init_zero; @@ -246,6 +304,77 @@ void test_perhapsCancelDupe_router_role_never_tracks_or_cancels(void) TEST_ASSERT_EQUAL_UINT8(1, shim->registerDupeHeard(kSender1, kId1)); } +// =========================================================================== +// Group 4 - meshTooBusyForExtraRepeats gate (channel/air util, direct-neighbor density) +// =========================================================================== + +void test_busy_channel_util_suppresses_text_message_threshold(void) +{ + ScopedChannelUtil busy(11.0f); // above the 10% BUSY_CHANNEL_UTIL_PERCENT gate + meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); + p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + p.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP; + TEST_ASSERT_EQUAL_UINT8(1, plainShim->getDupeCancelThreshold(&p)); +} + +void test_channel_util_at_gate_does_not_suppress(void) +{ + ScopedChannelUtil notBusy(10.0f); // exactly at the threshold, not above it + meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); + p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + p.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP; + TEST_ASSERT_EQUAL_UINT8(2, plainShim->getDupeCancelThreshold(&p)); +} + +void test_busy_air_util_tx_suppresses_text_message_threshold(void) +{ + ScopedAirUtilTx busy(5.0f); // above the 4% BUSY_AIR_UTIL_TX_PERCENT gate + meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); + p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + p.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP; + TEST_ASSERT_EQUAL_UINT8(1, plainShim->getDupeCancelThreshold(&p)); +} + +void test_air_util_tx_at_gate_does_not_suppress(void) +{ + ScopedAirUtilTx notBusy(4.0f); // exactly at the threshold, not above it + meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); + p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + p.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP; + TEST_ASSERT_EQUAL_UINT8(2, plainShim->getDupeCancelThreshold(&p)); +} + +#if HAS_VARIABLE_HOPS +void test_busy_direct_node_count_suppresses_text_message_threshold(void) +{ + ScopedDirectActiveNodes busy(11); // above the 10-node BUSY_DIRECT_ACTIVE_NODES gate + meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); + p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + p.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP; + TEST_ASSERT_EQUAL_UINT8(1, plainShim->getDupeCancelThreshold(&p)); +} + +void test_direct_node_count_at_gate_does_not_suppress(void) +{ + ScopedDirectActiveNodes notBusy(10); // exactly at the threshold, not above it + meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); + p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + p.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP; + TEST_ASSERT_EQUAL_UINT8(2, plainShim->getDupeCancelThreshold(&p)); +} +#endif + +void test_busy_mesh_does_not_affect_already_unextended_portnums(void) +{ + // A portnum whose threshold is already 1 (the default floor) has nothing for the busy-mesh + // gate to suppress - confirm it stays at 1, not some other value, while busy. + ScopedChannelUtil busy(50.0f); + meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); + p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + p.decoded.portnum = meshtastic_PortNum_POSITION_APP; + TEST_ASSERT_EQUAL_UINT8(1, plainShim->getDupeCancelThreshold(&p)); +} + // =========================================================================== void setup() @@ -276,6 +405,17 @@ void setup() RUN_TEST(test_perhapsCancelDupe_waits_for_configured_repeats); RUN_TEST(test_perhapsCancelDupe_router_role_never_tracks_or_cancels); + printf("\n=== meshTooBusyForExtraRepeats gate ===\n"); + RUN_TEST(test_busy_channel_util_suppresses_text_message_threshold); + RUN_TEST(test_channel_util_at_gate_does_not_suppress); + RUN_TEST(test_busy_air_util_tx_suppresses_text_message_threshold); + RUN_TEST(test_air_util_tx_at_gate_does_not_suppress); +#if HAS_VARIABLE_HOPS + RUN_TEST(test_busy_direct_node_count_suppresses_text_message_threshold); + RUN_TEST(test_direct_node_count_at_gate_does_not_suppress); +#endif + RUN_TEST(test_busy_mesh_does_not_affect_already_unextended_portnums); + exit(UNITY_END()); } From 4bcdc4b4e999bdcdd122ef1128348fa85aa81d0e Mon Sep 17 00:00:00 2001 From: nomdetom Date: Wed, 8 Jul 2026 23:30:19 +0100 Subject: [PATCH 3/9] split into separate module --- src/mesh/FloodingRouter.cpp | 120 +----- src/mesh/FloodingRouter.h | 30 +- src/modules/Modules.cpp | 7 + src/modules/RepeatScalingModule.cpp | 148 +++++++ src/modules/RepeatScalingModule.h | 61 +++ test/native-suite-count | 2 +- test/test_flooding_router/test_main.cpp | 368 +++-------------- test/test_repeat_scaling_module/test_main.cpp | 380 ++++++++++++++++++ 8 files changed, 650 insertions(+), 466 deletions(-) create mode 100644 src/modules/RepeatScalingModule.cpp create mode 100644 src/modules/RepeatScalingModule.h create mode 100644 test/test_repeat_scaling_module/test_main.cpp diff --git a/src/mesh/FloodingRouter.cpp b/src/mesh/FloodingRouter.cpp index 2debfcd4eab..ca811c759c3 100644 --- a/src/mesh/FloodingRouter.cpp +++ b/src/mesh/FloodingRouter.cpp @@ -1,125 +1,15 @@ #include "FloodingRouter.h" #include "MeshTypes.h" #include "NodeDB.h" -#include "airtime.h" #include "configuration.h" #include "mesh-pb-constants.h" #include "meshUtils.h" -#include "modules/HopScalingModule.h" +#include "modules/RepeatScalingModule.h" #include "modules/TextMessageModule.h" #if !MESHTASTIC_EXCLUDE_TRACEROUTE #include "modules/TraceRouteModule.h" #endif -namespace -{ -// Above any of these, the mesh is busy/dense enough that extra-repeat tolerance (see -// getDupeCancelThreshold) would just be adding airtime to an already-congested channel for -// little extra delivery benefit - so we fall back to the historical "cancel on first duplicate" -// behavior regardless of what the portnum table says. -constexpr float BUSY_CHANNEL_UTIL_PERCENT = 10.0f; -constexpr float BUSY_AIR_UTIL_TX_PERCENT = 4.0f; -constexpr uint16_t BUSY_DIRECT_ACTIVE_NODES = 10; - -// True if channel/air utilization or estimated direct-neighbor density indicates the mesh is -// too busy to spend extra airtime on repeat-tolerant rebroadcasts. -bool meshTooBusyForExtraRepeats() -{ - if (airTime && airTime->channelUtilizationPercent() > BUSY_CHANNEL_UTIL_PERCENT) - return true; - if (airTime && airTime->utilizationTXPercent() > BUSY_AIR_UTIL_TX_PERCENT) - return true; -#if HAS_VARIABLE_HOPS - // getLastPerHopCounts().perHop[0] is HopScalingModule's estimate of active (heard within its - // rolling window) direct (hop_away == 0) neighbors - the same "active and direct" notion used - // for its own hop-limit recommendation. - if (hopScalingModule && hopScalingModule->getLastPerHopCounts().perHop[0] > BUSY_DIRECT_ACTIVE_NODES) - return true; -#endif - return false; -} -} // namespace - -// Compile-time, per-portnum threshold for how many duplicate rebroadcasts of a packet we must hear -// before giving up on our own scheduled rebroadcast of it. The default of 1 (used for any portnum -// not given a case below, and for packets we can't decode - see below) preserves the historical -// behavior: cancel our own rebroadcast as soon as we hear the first duplicate. Raising a portnum's -// threshold makes this node more persistent for that traffic type, continuing to retransmit even -// after hearing some other nodes already repeat it - at the cost of extra airtime for packets -// that are, on balance, probably going to reach their destination anyway. -// -// This same switch applies uniformly to broadcasts and DMs: NextHopRouter::shouldFilterReceived -// calls this same (inherited, non-overridden) getDupeCancelThreshold() when it hears a duplicate -// it wasn't explicitly asked to relay - both in the fallback-to-flooding case and the general -// "duplicate heard, we're not the assigned next hop" case - so a DM of a listed portnum gets the -// same extra tolerance as a broadcast of that portnum, with no separate to/from-based gating needed. -// -// This is a compile-time table (not a runtime/protobuf config) because the desired threshold is -// expected to vary per packet type rather than being a single device-wide knob; edit it to tune -// for a given deployment. -// -// Regardless of the table below, meshTooBusyForExtraRepeats() (channel/air utilization or direct- -// neighbor density too high) always forces the threshold back down to 1 - see its definition above. -uint8_t FloodingRouter::getDupeCancelThreshold(const meshtastic_MeshPacket *p) -{ - // Portnum is only visible once a packet is decoded (i.e. we hold the channel key); packets we - // can't decrypt always get the default of 1, since we have no per-portnum signal to act on. - if (p->which_payload_variant != meshtastic_MeshPacket_decoded_tag) - return 1; - - uint8_t threshold; - switch (p->decoded.portnum) { - case meshtastic_PortNum_TEXT_MESSAGE_APP: - case meshtastic_PortNum_TEXT_MESSAGE_COMPRESSED_APP: - // User-visible chat, broadcast or DM: no ACK/retry safety net for broadcasts, and no other - // portnum-specific reason to prefer airtime savings over delivery odds. Tolerate one repeat - // heard from another node before giving up on our own rebroadcast. - threshold = 2; - break; - default: - threshold = 1; - break; - } - - // A busy/dense mesh overrides any portnum-specific extra tolerance: not worth spending more - // airtime on repeats when the channel is already congested or there are many direct neighbors - // to reach anyway. - if (threshold > 1 && meshTooBusyForExtraRepeats()) - return 1; - - return threshold; -} - -uint8_t FloodingRouter::registerDupeHeard(NodeNum sender, PacketId id) -{ - for (auto &entry : dupeCounts) { - if (entry.id == id && entry.sender == sender) { - if (entry.count < UINT8_MAX) - entry.count++; - return entry.count; - } - } - // Not tracked yet: claim the next ring slot, evicting whatever packet (if any) was there. - DupeCountEntry &slot = dupeCounts[dupeCountsNextSlot]; - dupeCountsNextSlot = (dupeCountsNextSlot + 1) % DUPE_COUNT_TRACKER_SIZE; - slot.sender = sender; - slot.id = id; - slot.count = 1; - return slot.count; -} - -void FloodingRouter::clearDupeCount(NodeNum sender, PacketId id) -{ - for (auto &entry : dupeCounts) { - if (entry.id == id && entry.sender == sender) { - entry.sender = 0; - entry.id = 0; - entry.count = 0; - return; - } - } -} - FloodingRouter::FloodingRouter() {} /** @@ -249,15 +139,13 @@ bool FloodingRouter::roleAllowsCancelingDupe(const meshtastic_MeshPacket *p) void FloodingRouter::perhapsCancelDupe(const meshtastic_MeshPacket *p) { if (p->transport_mechanism == meshtastic_MeshPacket_TransportMechanism_TRANSPORT_LORA && roleAllowsCancelingDupe(p)) { - // Wait for the configured number of duplicate rebroadcasts (see dupeCancelThresholds) before - // giving up on our own rebroadcast; below that, note the duplicate and keep waiting. - uint8_t dupesHeard = registerDupeHeard(p->from, p->id); - if (dupesHeard >= getDupeCancelThreshold(p)) { + // RepeatScalingModule owns the "how many duplicates should we tolerate before giving up" + // decision (and its bookkeeping/logging) - see RepeatScalingModule::shouldCancelDupe. + if (repeatScalingModule && repeatScalingModule->shouldCancelDupe(p)) { // cancel rebroadcast of this message *if* there was already one, unless we're a router! // But only LoRa packets should be able to trigger this. if (Router::cancelSending(p->from, p->id)) txRelayCanceled++; - clearDupeCount(p->from, p->id); } } if (config.device.role == meshtastic_Config_DeviceConfig_Role_ROUTER_LATE && iface) { diff --git a/src/mesh/FloodingRouter.h b/src/mesh/FloodingRouter.h index d446cdff01d..16231781803 100644 --- a/src/mesh/FloodingRouter.h +++ b/src/mesh/FloodingRouter.h @@ -70,35 +70,11 @@ class FloodingRouter : public Router // the same packet bool roleAllowsCancelingDupe(const meshtastic_MeshPacket *p); - /* Call when receiving a duplicate packet to check whether we should cancel a packet in the Tx queue */ + /* Call when receiving a duplicate packet to check whether we should cancel a packet in the Tx queue. + * The "how many duplicates should we tolerate before giving up" decision (and its bookkeeping/ + * logging) is delegated to RepeatScalingModule - see perhapsCancelDupe's implementation. */ void perhapsCancelDupe(const meshtastic_MeshPacket *p); // Return true if we are a rebroadcaster bool isRebroadcaster(); - - // How many duplicate rebroadcasts of a packet we require to hear (see the per-portnum switch in - // FloodingRouter.cpp) before giving up on our own scheduled rebroadcast of it. Virtual solely so - // tests can override it to inject a threshold without needing a real portnum case (see - // test/test_flooding_router). - virtual uint8_t getDupeCancelThreshold(const meshtastic_MeshPacket *p); - - // Tracks how many duplicates we've heard so far, per (sender, id), for packets we currently - // have one queued to rebroadcast ourselves. Bounded, ephemeral ring buffer - not a persistent - // record like PacketHistory: entries are only meaningful while our own rebroadcast is still - // pending, and naturally get evicted/reused as the ring wraps. - uint8_t registerDupeHeard(NodeNum sender, PacketId id); - - // Clears tracking state for a (sender, id) once we've acted on it (cancelled our rebroadcast), - // so a reused ring slot can't cause a stale hit against an unrelated future packet. - void clearDupeCount(NodeNum sender, PacketId id); - - private: - static constexpr uint8_t DUPE_COUNT_TRACKER_SIZE = 8; - struct DupeCountEntry { - NodeNum sender = 0; - PacketId id = 0; - uint8_t count = 0; - }; - DupeCountEntry dupeCounts[DUPE_COUNT_TRACKER_SIZE]; - uint8_t dupeCountsNextSlot = 0; }; \ No newline at end of file diff --git a/src/modules/Modules.cpp b/src/modules/Modules.cpp index 546651c5bc2..d7712d2bcb2 100644 --- a/src/modules/Modules.cpp +++ b/src/modules/Modules.cpp @@ -50,6 +50,9 @@ #if HAS_VARIABLE_HOPS #include "modules/HopScalingModule.h" #endif +#if !MESHTASTIC_EXCLUDE_REPEATSCALING +#include "modules/RepeatScalingModule.h" +#endif #include "modules/TextMessageModule.h" #if !MESHTASTIC_EXCLUDE_TRACEROUTE #include "modules/TraceRouteModule.h" @@ -143,6 +146,10 @@ void setupModules() hopScalingModule = new HopScalingModule(); #endif +#if !MESHTASTIC_EXCLUDE_REPEATSCALING + repeatScalingModule = new RepeatScalingModule(); +#endif + #if !MESHTASTIC_EXCLUDE_ADMIN adminModule = new AdminModule(); #endif diff --git a/src/modules/RepeatScalingModule.cpp b/src/modules/RepeatScalingModule.cpp new file mode 100644 index 00000000000..de4e4431a31 --- /dev/null +++ b/src/modules/RepeatScalingModule.cpp @@ -0,0 +1,148 @@ +#include "RepeatScalingModule.h" +#include "DebugConfiguration.h" +#include "airtime.h" +#include "configuration.h" +#include "modules/HopScalingModule.h" + +RepeatScalingModule *repeatScalingModule; + +namespace +{ +// Above any of these, the mesh is busy/dense enough that extra-repeat tolerance (see +// RepeatScalingModule::getDupeCancelThreshold) would just be adding airtime to an already-congested +// channel for little extra delivery benefit - so we fall back to the historical "cancel on first +// duplicate" behavior regardless of what the portnum table says. +constexpr float BUSY_CHANNEL_UTIL_PERCENT = 10.0f; +constexpr float BUSY_AIR_UTIL_TX_PERCENT = 4.0f; +constexpr uint16_t BUSY_DIRECT_ACTIVE_NODES = 10; + +// True if channel/air utilization or estimated direct-neighbor density indicates the mesh is +// too busy to spend extra airtime on repeat-tolerant rebroadcasts. Logs which specific condition +// (if any) tripped, so the reason a packet fell back to the historical threshold is visible. +bool meshTooBusyForExtraRepeats() +{ + if (airTime && airTime->channelUtilizationPercent() > BUSY_CHANNEL_UTIL_PERCENT) { + LOG_DEBUG("[REPEATSCALE] Mesh busy: chUtil=%.1f%% > %.1f%%", airTime->channelUtilizationPercent(), + BUSY_CHANNEL_UTIL_PERCENT); + return true; + } + if (airTime && airTime->utilizationTXPercent() > BUSY_AIR_UTIL_TX_PERCENT) { + LOG_DEBUG("[REPEATSCALE] Mesh busy: airUtilTX=%.1f%% > %.1f%%", airTime->utilizationTXPercent(), + BUSY_AIR_UTIL_TX_PERCENT); + return true; + } +#if HAS_VARIABLE_HOPS + // getLastPerHopCounts().perHop[0] is HopScalingModule's estimate of active (heard within its + // rolling window) direct (hop_away == 0) neighbors - the same "active and direct" notion used + // for its own hop-limit recommendation. + if (hopScalingModule && hopScalingModule->getLastPerHopCounts().perHop[0] > BUSY_DIRECT_ACTIVE_NODES) { + LOG_DEBUG("[REPEATSCALE] Mesh busy: directActiveNodes=%u > %u", hopScalingModule->getLastPerHopCounts().perHop[0], + BUSY_DIRECT_ACTIVE_NODES); + return true; + } +#endif + return false; +} +} // namespace + +// Compile-time, per-portnum threshold for how many duplicate rebroadcasts of a packet we must hear +// before giving up on our own scheduled rebroadcast of it. The default of 1 (used for any portnum +// not given a case below, and for packets we can't decode - see below) preserves the historical +// behavior: cancel our own rebroadcast as soon as we hear the first duplicate. Raising a portnum's +// threshold makes this node more persistent for that traffic type, continuing to retransmit even +// after hearing some other nodes already repeat it - at the cost of extra airtime for packets +// that are, on balance, probably going to reach their destination anyway. +// +// This same switch applies uniformly to broadcasts and DMs: NextHopRouter::shouldFilterReceived +// calls the same (inherited, non-overridden) FloodingRouter::perhapsCancelDupe -> shouldCancelDupe +// path when it hears a duplicate it wasn't explicitly asked to relay - both in the fallback-to- +// flooding case and the general "duplicate heard, we're not the assigned next hop" case - so a DM +// of a listed portnum gets the same extra tolerance as a broadcast of that portnum, with no +// separate to/from-based gating needed. +// +// This is a compile-time table (not a runtime/protobuf config) because the desired threshold is +// expected to vary per packet type rather than being a single device-wide knob; edit it to tune +// for a given deployment. +// +// Regardless of the table below, meshTooBusyForExtraRepeats() (channel/air utilization or direct- +// neighbor density too high) always forces the threshold back down to 1 - see its definition above. +uint8_t RepeatScalingModule::getDupeCancelThreshold(const meshtastic_MeshPacket *p) +{ + // Portnum is only visible once a packet is decoded (i.e. we hold the channel key); packets we + // can't decrypt always get the default of 1, since we have no per-portnum signal to act on. + if (p->which_payload_variant != meshtastic_MeshPacket_decoded_tag) + return 1; + + uint8_t threshold; + switch (p->decoded.portnum) { + case meshtastic_PortNum_TEXT_MESSAGE_APP: + case meshtastic_PortNum_TEXT_MESSAGE_COMPRESSED_APP: + // User-visible chat, broadcast or DM: no ACK/retry safety net for broadcasts, and no other + // portnum-specific reason to prefer airtime savings over delivery odds. Tolerate one repeat + // heard from another node before giving up on our own rebroadcast. + threshold = 2; + break; + default: + threshold = 1; + break; + } + + // A busy/dense mesh overrides any portnum-specific extra tolerance: not worth spending more + // airtime on repeats when the channel is already congested or there are many direct neighbors + // to reach anyway. + if (threshold > 1 && meshTooBusyForExtraRepeats()) { + LOG_DEBUG("[REPEATSCALE] portnum=%d wanted threshold=%u but mesh is busy; falling back to 1", p->decoded.portnum, + threshold); + return 1; + } + + return threshold; +} + +uint8_t RepeatScalingModule::registerDupeHeard(NodeNum sender, PacketId id) +{ + for (auto &entry : dupeCounts) { + if (entry.id == id && entry.sender == sender) { + if (entry.count < UINT8_MAX) + entry.count++; + return entry.count; + } + } + // Not tracked yet: claim the next ring slot, evicting whatever packet (if any) was there. + DupeCountEntry &slot = dupeCounts[dupeCountsNextSlot]; + dupeCountsNextSlot = (dupeCountsNextSlot + 1) % DUPE_COUNT_TRACKER_SIZE; + slot.sender = sender; + slot.id = id; + slot.count = 1; + return slot.count; +} + +void RepeatScalingModule::clearDupeCount(NodeNum sender, PacketId id) +{ + for (auto &entry : dupeCounts) { + if (entry.id == id && entry.sender == sender) { + entry.sender = 0; + entry.id = 0; + entry.count = 0; + return; + } + } +} + +bool RepeatScalingModule::shouldCancelDupe(const meshtastic_MeshPacket *p) +{ + const uint8_t threshold = getDupeCancelThreshold(p); + const uint8_t dupesHeard = registerDupeHeard(p->from, p->id); + const int32_t portnum = (p->which_payload_variant == meshtastic_MeshPacket_decoded_tag) ? p->decoded.portnum : -1; + + if (dupesHeard >= threshold) { + LOG_INFO("[REPEATSCALE] Giving up own rebroadcast of 0x%08x from=0x%08x portnum=%d: heard %u/%u duplicate(s)", p->id, + p->from, portnum, dupesHeard, threshold); + clearDupeCount(p->from, p->id); + return true; + } + + LOG_DEBUG("[REPEATSCALE] Tolerating duplicate %u/%u of 0x%08x from=0x%08x portnum=%d: keeping own rebroadcast queued", + dupesHeard, threshold, p->id, p->from, portnum); + return false; +} diff --git a/src/modules/RepeatScalingModule.h b/src/modules/RepeatScalingModule.h new file mode 100644 index 00000000000..e3bd3b3e6a5 --- /dev/null +++ b/src/modules/RepeatScalingModule.h @@ -0,0 +1,61 @@ +#pragma once + +#include "MeshTypes.h" +#include "mesh/mesh-pb-constants.h" + +/** + * RepeatScalingModule owns the "how many duplicate rebroadcasts of a packet we ourselves have + * queued to rebroadcast should we tolerate before giving up" decision (see + * FloodingRouter::perhapsCancelDupe, its sole caller). + * + * The historical behavior is to cancel our own queued rebroadcast as soon as we hear the very + * first duplicate from another node. This module allows some packet types (see + * getDupeCancelThreshold in RepeatScalingModule.cpp) to instead tolerate a configurable number of + * heard duplicates first, trading a little extra airtime for better delivery odds - unless the + * mesh is already busy/dense, in which case it always falls back to the historical behavior. + */ +class RepeatScalingModule +{ + public: + RepeatScalingModule() = default; + virtual ~RepeatScalingModule() = default; + + // Call each time we hear a duplicate rebroadcast of a packet we ourselves have queued to + // rebroadcast. Tracks the running count for (p->from, p->id) and logs the decision. Returns + // true once the configured per-portnum threshold has been reached, in which case the caller + // should cancel its own queued rebroadcast; tracking for the packet is cleared as soon as + // this returns true, so a reused ring slot can't leak into an unrelated future packet. + // Virtual solely so tests of FloodingRouter's role-gating can substitute a test double instead + // of driving the real threshold/ring-buffer logic (which is tested directly in + // test/test_repeat_scaling_module). + virtual bool shouldCancelDupe(const meshtastic_MeshPacket *p); + + protected: + // How many duplicate rebroadcasts of a packet we require to hear (see the per-portnum switch + // in RepeatScalingModule.cpp) before giving up on our own scheduled rebroadcast of it. Virtual + // solely so tests can override it to inject a threshold without needing a real portnum case + // (see test/test_repeat_scaling_module). + virtual uint8_t getDupeCancelThreshold(const meshtastic_MeshPacket *p); + + // Tracks how many duplicates we've heard so far, per (sender, id), for packets we currently + // have one queued to rebroadcast ourselves. Bounded, ephemeral ring buffer - not a persistent + // record like PacketHistory: entries are only meaningful while our own rebroadcast is still + // pending, and naturally get evicted/reused as the ring wraps. + uint8_t registerDupeHeard(NodeNum sender, PacketId id); + + // Clears tracking state for a (sender, id) once we've acted on it (cancelled our rebroadcast), + // so a reused ring slot can't cause a stale hit against an unrelated future packet. + void clearDupeCount(NodeNum sender, PacketId id); + + private: + static constexpr uint8_t DUPE_COUNT_TRACKER_SIZE = 8; + struct DupeCountEntry { + NodeNum sender = 0; + PacketId id = 0; + uint8_t count = 0; + }; + DupeCountEntry dupeCounts[DUPE_COUNT_TRACKER_SIZE]; + uint8_t dupeCountsNextSlot = 0; +}; + +extern RepeatScalingModule *repeatScalingModule; diff --git a/test/native-suite-count b/test/native-suite-count index e522732c77e..a2720097dcc 100644 --- a/test/native-suite-count +++ b/test/native-suite-count @@ -1 +1 @@ -38 +39 diff --git a/test/test_flooding_router/test_main.cpp b/test/test_flooding_router/test_main.cpp index dfa25b19d2a..ad866f7d01d 100644 --- a/test/test_flooding_router/test_main.cpp +++ b/test/test_flooding_router/test_main.cpp @@ -1,34 +1,27 @@ -// Unit tests for FloodingRouter's duplicate-rebroadcast bookkeeping: -// - registerDupeHeard/clearDupeCount: the small ring buffer counting how many times we've -// heard a duplicate of a packet we have queued to rebroadcast ourselves. -// - getDupeCancelThreshold: the compile-time, per-portnum threshold (see FloodingRouter.cpp) -// controlling how many duplicates we tolerate before giving up on our own rebroadcast. -// - perhapsCancelDupe: the integration of the two - only actually gives up (and clears its -// tracking state) once the configured threshold has been reached. +// Unit tests for FloodingRouter::perhapsCancelDupe's role-gating and its delegation to +// RepeatScalingModule (the "how many duplicates should we tolerate before giving up" decision +// and its bookkeeping/logging now lives there - see test/test_repeat_scaling_module). // // perhapsCancelDupe() is safe to call directly here even with no RadioInterface: Router:: // cancelSending() is iface-null-safe (just returns false), so the queue-cancellation side effect -// is a no-op - what we can and do observe is the dupe-count bookkeeping around it. +// is a no-op - what we can and do observe is whether/how many times RepeatScalingModule was +// consulted at all. #include "MeshTypes.h" // before TestUtil.h: provides NodeNum etc. #include "TestUtil.h" #include -#include "airtime.h" #include "configuration.h" #include "mesh/FloodingRouter.h" -#include "modules/HopScalingModule.h" +#include "modules/RepeatScalingModule.h" static constexpr NodeNum kSender1 = 0x000005AB; -static constexpr NodeNum kSender2 = 0x000006CD; static constexpr PacketId kId1 = 0x1001; -static constexpr PacketId kId2 = 0x2002; // --------------------------------------------------------------------------- -// Test shim - re-exposes the protected dupe-tracking helpers, and allows overriding -// getDupeCancelThreshold so tests don't depend on any real portnum having a non-default case. -// Nulls cryptLock before it's rebuilt so the Router base can be (re)constructed (same pattern as -// test_nexthop_routing's NextHopRouterTestShim / test_mqtt's MockRouter). +// Test shim - re-exposes the protected perhapsCancelDupe. Nulls cryptLock before it's rebuilt so +// the Router base can be (re)constructed (same pattern as test_nexthop_routing's +// NextHopRouterTestShim / test_mqtt's MockRouter). // --------------------------------------------------------------------------- class FloodingRouterTestShim : public FloodingRouter { @@ -39,97 +32,29 @@ class FloodingRouterTestShim : public FloodingRouter cryptLock = nullptr; } - using FloodingRouter::clearDupeCount; - using FloodingRouter::getDupeCancelThreshold; using FloodingRouter::perhapsCancelDupe; - using FloodingRouter::registerDupeHeard; - - // Test-only override: bypasses the compile-time portnum switch so tests can exercise - // perhapsCancelDupe's threshold-gating without needing a real portnum case. - uint8_t testThreshold = 1; - uint8_t getDupeCancelThreshold(const meshtastic_MeshPacket *p) override { return testThreshold; } protected: // FloodingRouter::perhapsRebroadcast is pure virtual; not exercised by these tests. bool perhapsRebroadcast(const meshtastic_MeshPacket *p) override { return false; } }; -// A second, minimal shim that does NOT override getDupeCancelThreshold, for testing the real -// compile-time default implementation directly (FloodingRouter itself is abstract - perhapsRebroadcast -// is pure virtual - so it can't be constructed on its own). -class FloodingRouterPlainShim : public FloodingRouter +// Test double standing in for the real RepeatScalingModule: records how many times +// FloodingRouter consulted it, and returns a fixed, test-controlled decision. +class CountingRepeatScalingModule : public RepeatScalingModule { public: - FloodingRouterPlainShim() : FloodingRouter() + int callCount = 0; + bool decision = false; + bool shouldCancelDupe(const meshtastic_MeshPacket *p) override { - delete cryptLock; - cryptLock = nullptr; + callCount++; + return decision; } - - using FloodingRouter::getDupeCancelThreshold; - - protected: - bool perhapsRebroadcast(const meshtastic_MeshPacket *p) override { return false; } }; static FloodingRouterTestShim *shim = nullptr; -static FloodingRouterPlainShim *plainShim = nullptr; - -// --------------------------------------------------------------------------- -// Scoped helpers to simulate a busy mesh for meshTooBusyForExtraRepeats() (see FloodingRouter.cpp). -// Install a global airTime/hopScalingModule reporting the given condition for the enclosing scope, -// restoring the previous pointer on destruction. Mirrors test_traffic_management's ScopedBusyAirTime. -// --------------------------------------------------------------------------- -class ScopedChannelUtil -{ - public: - explicit ScopedChannelUtil(float percent) : previous(airTime) - { - // channelUtilizationPercent() == sum(channelUtilization[]) / (PERIODS * 10 * 1000) * 100 - busy.channelUtilization[0] = static_cast(percent / 100.0f * CHANNEL_UTILIZATION_PERIODS * 10 * 1000); - airTime = &busy; - } - ~ScopedChannelUtil() { airTime = previous; } - - private: - AirTime busy; - AirTime *previous; -}; - -class ScopedAirUtilTx -{ - public: - explicit ScopedAirUtilTx(float percent) : previous(airTime) - { - // utilizationTXPercent() == sum(utilizationTX[]) / MS_IN_HOUR * 100 - busy.utilizationTX[0] = static_cast(percent / 100.0f * MS_IN_HOUR); - airTime = &busy; - } - ~ScopedAirUtilTx() { airTime = previous; } - - private: - AirTime busy; - AirTime *previous; -}; - -#if HAS_VARIABLE_HOPS -class ScopedDirectActiveNodes -{ - public: - explicit ScopedDirectActiveNodes(uint16_t count) : previous(hopScalingModule) - { - HopScalingModule::PerHopCounts counts; - counts.perHop[0] = count; - busy.setLastPerHopCountsForTest(counts); - hopScalingModule = &busy; - } - ~ScopedDirectActiveNodes() { hopScalingModule = previous; } - - private: - HopScalingModule busy; - HopScalingModule *previous; -}; -#endif +static CountingRepeatScalingModule *counting = nullptr; static meshtastic_MeshPacket makeDupePacket(NodeNum from, PacketId id) { @@ -137,242 +62,66 @@ static meshtastic_MeshPacket makeDupePacket(NodeNum from, PacketId id) p.from = from; p.id = id; p.transport_mechanism = meshtastic_MeshPacket_TransportMechanism_TRANSPORT_LORA; - p.which_payload_variant = meshtastic_MeshPacket_encrypted_tag; // undecodable by default + p.which_payload_variant = meshtastic_MeshPacket_encrypted_tag; return p; } void setUp(void) { config.device.role = meshtastic_Config_DeviceConfig_Role_CLIENT; - shim->testThreshold = 1; - // Clear any tracking state a previous test may have left behind for our fixed test keys. - shim->clearDupeCount(kSender1, kId1); - shim->clearDupeCount(kSender2, kId2); + counting->callCount = 0; + counting->decision = false; + repeatScalingModule = counting; } void tearDown(void) {} // =========================================================================== -// Group 1 - registerDupeHeard / clearDupeCount (the ring buffer) +// perhapsCancelDupe: role-gating and delegation to RepeatScalingModule // =========================================================================== -void test_registerDupeHeard_starts_at_one(void) -{ - TEST_ASSERT_EQUAL_UINT8(1, shim->registerDupeHeard(kSender1, kId1)); -} - -void test_registerDupeHeard_increments_on_repeat(void) -{ - TEST_ASSERT_EQUAL_UINT8(1, shim->registerDupeHeard(kSender1, kId1)); - TEST_ASSERT_EQUAL_UINT8(2, shim->registerDupeHeard(kSender1, kId1)); - TEST_ASSERT_EQUAL_UINT8(3, shim->registerDupeHeard(kSender1, kId1)); -} - -void test_registerDupeHeard_independent_per_packet(void) -{ - TEST_ASSERT_EQUAL_UINT8(1, shim->registerDupeHeard(kSender1, kId1)); - TEST_ASSERT_EQUAL_UINT8(2, shim->registerDupeHeard(kSender1, kId1)); - // A different (sender, id) pair tracks its own independent count. - TEST_ASSERT_EQUAL_UINT8(1, shim->registerDupeHeard(kSender2, kId2)); - TEST_ASSERT_EQUAL_UINT8(2, shim->registerDupeHeard(kSender2, kId2)); - // The first pair's count is untouched by the second's activity. - TEST_ASSERT_EQUAL_UINT8(3, shim->registerDupeHeard(kSender1, kId1)); -} - -void test_clearDupeCount_resets_tracking(void) -{ - shim->registerDupeHeard(kSender1, kId1); - shim->registerDupeHeard(kSender1, kId1); - shim->clearDupeCount(kSender1, kId1); - // After clearing, the next duplicate heard for this packet starts fresh at 1. - TEST_ASSERT_EQUAL_UINT8(1, shim->registerDupeHeard(kSender1, kId1)); -} - -void test_clearDupeCount_of_untracked_packet_is_noop(void) -{ - // Clearing a (sender, id) that was never registered must not crash or disturb other entries. - shim->registerDupeHeard(kSender1, kId1); - shim->clearDupeCount(0xFFFFFFFF, 0xFFFFFFFF); - TEST_ASSERT_EQUAL_UINT8(2, shim->registerDupeHeard(kSender1, kId1)); -} - -void test_registerDupeHeard_ring_eviction_does_not_crash(void) -{ - // DUPE_COUNT_TRACKER_SIZE is 8 (see FloodingRouter.h); track more distinct packets than that - // and confirm the ring buffer just evicts older entries rather than misbehaving. - for (uint32_t i = 0; i < 20; i++) { - uint8_t count = shim->registerDupeHeard(kSender1, 0x10000 + i); - TEST_ASSERT_EQUAL_UINT8(1, count); // each is a fresh (sender, id) pair - } -} - -// =========================================================================== -// Group 2 - getDupeCancelThreshold (compile-time per-portnum default) -// =========================================================================== - -void test_default_threshold_is_one_for_undecoded_packet(void) +void test_perhapsCancelDupe_consults_repeatScalingModule_for_client_role(void) { meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); - p.which_payload_variant = meshtastic_MeshPacket_encrypted_tag; - TEST_ASSERT_EQUAL_UINT8(1, plainShim->getDupeCancelThreshold(&p)); -} - -void test_default_threshold_is_one_for_unlisted_portnum(void) -{ - meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); - p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; - p.decoded.portnum = meshtastic_PortNum_POSITION_APP; // no case for this portnum - TEST_ASSERT_EQUAL_UINT8(1, plainShim->getDupeCancelThreshold(&p)); -} - -void test_text_message_threshold_is_two(void) -{ - meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); - p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; - p.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP; - TEST_ASSERT_EQUAL_UINT8(2, plainShim->getDupeCancelThreshold(&p)); -} - -void test_text_message_compressed_threshold_is_two(void) -{ - meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); - p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; - p.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_COMPRESSED_APP; - TEST_ASSERT_EQUAL_UINT8(2, plainShim->getDupeCancelThreshold(&p)); -} - -void test_text_message_threshold_applies_regardless_of_addressing(void) -{ - // getDupeCancelThreshold only inspects portnum, not p->to - so a DM (to a specific node) - // gets the same threshold as a broadcast for the same portnum. This is what lets - // NextHopRouter::shouldFilterReceived's call to the same inherited perhapsCancelDupe/ - // getDupeCancelThreshold extend the tolerance to DM text messages too, with no separate - // addressing-based gate. - meshtastic_MeshPacket dm = makeDupePacket(kSender1, kId1); - dm.to = 0x12345678; // a specific node, not NODENUM_BROADCAST - dm.which_payload_variant = meshtastic_MeshPacket_decoded_tag; - dm.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP; - TEST_ASSERT_EQUAL_UINT8(2, plainShim->getDupeCancelThreshold(&dm)); -} - -// =========================================================================== -// Group 3 - perhapsCancelDupe (integration: threshold gates the cancel + clears tracking) -// =========================================================================== - -void test_perhapsCancelDupe_clears_tracking_once_threshold_reached(void) -{ - shim->testThreshold = 1; // historical behavior: cancel (and clear) on the very first duplicate - meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); - shim->perhapsCancelDupe(&p); - - // Threshold (1) was reached on the first call, so tracking was cleared - the next duplicate - // heard for this packet starts a fresh count, rather than continuing to accumulate. - TEST_ASSERT_EQUAL_UINT8(1, shim->registerDupeHeard(kSender1, kId1)); -} - -void test_perhapsCancelDupe_waits_for_configured_repeats(void) -{ - shim->testThreshold = 3; // this node should tolerate hearing 2 repeats before giving up - meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); - - shim->perhapsCancelDupe(&p); // 1st duplicate heard -> internal count 1, below threshold - - // Directly registering one more duplicate both (a) advances the count to 2 for the next - // step, and (b) proves tracking was NOT cleared by the call above: a cleared entry would - // restart at 1, not continue to 2. - TEST_ASSERT_EQUAL_UINT8(2, shim->registerDupeHeard(kSender1, kId1)); - - shim->perhapsCancelDupe(&p); // 3rd duplicate heard -> internal count 3, reaches threshold - // Threshold reached inside the call above, so tracking was cleared - starts fresh at 1. - TEST_ASSERT_EQUAL_UINT8(1, shim->registerDupeHeard(kSender1, kId1)); + TEST_ASSERT_EQUAL_INT(1, counting->callCount); } -void test_perhapsCancelDupe_router_role_never_tracks_or_cancels(void) +void test_perhapsCancelDupe_router_role_never_consults_repeatScalingModule(void) { // ROUTER never cancels its own rebroadcast (roleAllowsCancelingDupe() gates it out entirely), - // so it should also never bother tracking a dupe count for it. + // so it should also never bother asking RepeatScalingModule about it. config.device.role = meshtastic_Config_DeviceConfig_Role_ROUTER; - shim->testThreshold = 1; meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); shim->perhapsCancelDupe(&p); shim->perhapsCancelDupe(&p); - // Since roleAllowsCancelingDupe() short-circuits before registerDupeHeard() is ever called, - // this key was never tracked - the first direct registerDupeHeard() call starts fresh at 1. - TEST_ASSERT_EQUAL_UINT8(1, shim->registerDupeHeard(kSender1, kId1)); -} - -// =========================================================================== -// Group 4 - meshTooBusyForExtraRepeats gate (channel/air util, direct-neighbor density) -// =========================================================================== - -void test_busy_channel_util_suppresses_text_message_threshold(void) -{ - ScopedChannelUtil busy(11.0f); // above the 10% BUSY_CHANNEL_UTIL_PERCENT gate - meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); - p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; - p.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP; - TEST_ASSERT_EQUAL_UINT8(1, plainShim->getDupeCancelThreshold(&p)); + TEST_ASSERT_EQUAL_INT(0, counting->callCount); } -void test_channel_util_at_gate_does_not_suppress(void) +void test_perhapsCancelDupe_non_lora_transport_never_consults_repeatScalingModule(void) { - ScopedChannelUtil notBusy(10.0f); // exactly at the threshold, not above it + // Only LoRa-transport packets can trigger a cancel; other transports should never be routed + // through RepeatScalingModule. meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); - p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; - p.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP; - TEST_ASSERT_EQUAL_UINT8(2, plainShim->getDupeCancelThreshold(&p)); -} + p.transport_mechanism = meshtastic_MeshPacket_TransportMechanism_TRANSPORT_INTERNAL; -void test_busy_air_util_tx_suppresses_text_message_threshold(void) -{ - ScopedAirUtilTx busy(5.0f); // above the 4% BUSY_AIR_UTIL_TX_PERCENT gate - meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); - p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; - p.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP; - TEST_ASSERT_EQUAL_UINT8(1, plainShim->getDupeCancelThreshold(&p)); -} + shim->perhapsCancelDupe(&p); -void test_air_util_tx_at_gate_does_not_suppress(void) -{ - ScopedAirUtilTx notBusy(4.0f); // exactly at the threshold, not above it - meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); - p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; - p.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP; - TEST_ASSERT_EQUAL_UINT8(2, plainShim->getDupeCancelThreshold(&p)); + TEST_ASSERT_EQUAL_INT(0, counting->callCount); } -#if HAS_VARIABLE_HOPS -void test_busy_direct_node_count_suppresses_text_message_threshold(void) +void test_perhapsCancelDupe_does_not_crash_when_repeatScalingModule_says_cancel(void) { - ScopedDirectActiveNodes busy(11); // above the 10-node BUSY_DIRECT_ACTIVE_NODES gate + // With no RadioInterface, Router::cancelSending() is a safe no-op - this just confirms the + // true-branch (attempting the cancel) doesn't crash and still only consults the module once. + counting->decision = true; meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); - p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; - p.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP; - TEST_ASSERT_EQUAL_UINT8(1, plainShim->getDupeCancelThreshold(&p)); -} -void test_direct_node_count_at_gate_does_not_suppress(void) -{ - ScopedDirectActiveNodes notBusy(10); // exactly at the threshold, not above it - meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); - p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; - p.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP; - TEST_ASSERT_EQUAL_UINT8(2, plainShim->getDupeCancelThreshold(&p)); -} -#endif + shim->perhapsCancelDupe(&p); -void test_busy_mesh_does_not_affect_already_unextended_portnums(void) -{ - // A portnum whose threshold is already 1 (the default floor) has nothing for the busy-mesh - // gate to suppress - confirm it stays at 1, not some other value, while busy. - ScopedChannelUtil busy(50.0f); - meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); - p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; - p.decoded.portnum = meshtastic_PortNum_POSITION_APP; - TEST_ASSERT_EQUAL_UINT8(1, plainShim->getDupeCancelThreshold(&p)); + TEST_ASSERT_EQUAL_INT(1, counting->callCount); } // =========================================================================== @@ -383,38 +132,13 @@ void setup() UNITY_BEGIN(); shim = new FloodingRouterTestShim(); - plainShim = new FloodingRouterPlainShim(); - - printf("\n=== registerDupeHeard / clearDupeCount (ring buffer) ===\n"); - RUN_TEST(test_registerDupeHeard_starts_at_one); - RUN_TEST(test_registerDupeHeard_increments_on_repeat); - RUN_TEST(test_registerDupeHeard_independent_per_packet); - RUN_TEST(test_clearDupeCount_resets_tracking); - RUN_TEST(test_clearDupeCount_of_untracked_packet_is_noop); - RUN_TEST(test_registerDupeHeard_ring_eviction_does_not_crash); - - printf("\n=== getDupeCancelThreshold (compile-time default) ===\n"); - RUN_TEST(test_default_threshold_is_one_for_undecoded_packet); - RUN_TEST(test_default_threshold_is_one_for_unlisted_portnum); - RUN_TEST(test_text_message_threshold_is_two); - RUN_TEST(test_text_message_compressed_threshold_is_two); - RUN_TEST(test_text_message_threshold_applies_regardless_of_addressing); - - printf("\n=== perhapsCancelDupe (threshold gating) ===\n"); - RUN_TEST(test_perhapsCancelDupe_clears_tracking_once_threshold_reached); - RUN_TEST(test_perhapsCancelDupe_waits_for_configured_repeats); - RUN_TEST(test_perhapsCancelDupe_router_role_never_tracks_or_cancels); + counting = new CountingRepeatScalingModule(); - printf("\n=== meshTooBusyForExtraRepeats gate ===\n"); - RUN_TEST(test_busy_channel_util_suppresses_text_message_threshold); - RUN_TEST(test_channel_util_at_gate_does_not_suppress); - RUN_TEST(test_busy_air_util_tx_suppresses_text_message_threshold); - RUN_TEST(test_air_util_tx_at_gate_does_not_suppress); -#if HAS_VARIABLE_HOPS - RUN_TEST(test_busy_direct_node_count_suppresses_text_message_threshold); - RUN_TEST(test_direct_node_count_at_gate_does_not_suppress); -#endif - RUN_TEST(test_busy_mesh_does_not_affect_already_unextended_portnums); + printf("\n=== perhapsCancelDupe (role-gating + delegation) ===\n"); + RUN_TEST(test_perhapsCancelDupe_consults_repeatScalingModule_for_client_role); + RUN_TEST(test_perhapsCancelDupe_router_role_never_consults_repeatScalingModule); + RUN_TEST(test_perhapsCancelDupe_non_lora_transport_never_consults_repeatScalingModule); + RUN_TEST(test_perhapsCancelDupe_does_not_crash_when_repeatScalingModule_says_cancel); exit(UNITY_END()); } diff --git a/test/test_repeat_scaling_module/test_main.cpp b/test/test_repeat_scaling_module/test_main.cpp new file mode 100644 index 00000000000..8f749f09ed5 --- /dev/null +++ b/test/test_repeat_scaling_module/test_main.cpp @@ -0,0 +1,380 @@ +// Unit tests for RepeatScalingModule, which decides how many duplicate rebroadcasts of a packet +// we have queued to rebroadcast ourselves we should tolerate before giving up: +// - registerDupeHeard/clearDupeCount: the small ring buffer counting how many times we've +// heard a duplicate of a packet we have queued to rebroadcast ourselves. +// - getDupeCancelThreshold: the compile-time, per-portnum threshold (see +// RepeatScalingModule.cpp) controlling how many duplicates we tolerate before giving up. +// - meshTooBusyForExtraRepeats gate: channel/air utilization or direct-neighbor density +// overriding any portnum-specific extra tolerance. +// - shouldCancelDupe: the integration of all of the above - only actually gives up (and clears +// its tracking state) once the configured threshold has been reached. + +#include "MeshTypes.h" // before TestUtil.h: provides NodeNum etc. +#include "TestUtil.h" +#include + +#include "airtime.h" +#include "configuration.h" +#include "modules/HopScalingModule.h" +#include "modules/RepeatScalingModule.h" + +static constexpr NodeNum kSender1 = 0x000005AB; +static constexpr NodeNum kSender2 = 0x000006CD; +static constexpr PacketId kId1 = 0x1001; +static constexpr PacketId kId2 = 0x2002; + +// --------------------------------------------------------------------------- +// Test shim - re-exposes the protected dupe-tracking helpers, and allows overriding +// getDupeCancelThreshold so tests don't depend on any real portnum having a non-default case. +// --------------------------------------------------------------------------- +class RepeatScalingModuleTestShim : public RepeatScalingModule +{ + public: + using RepeatScalingModule::clearDupeCount; + using RepeatScalingModule::registerDupeHeard; + + // Test-only override: bypasses the compile-time portnum switch so tests can exercise + // shouldCancelDupe's threshold-gating without needing a real portnum case. + uint8_t testThreshold = 1; + uint8_t getDupeCancelThreshold(const meshtastic_MeshPacket *p) override { return testThreshold; } +}; + +// A second, minimal shim that does NOT override getDupeCancelThreshold, for testing the real +// compile-time default implementation directly. +class RepeatScalingModulePlainShim : public RepeatScalingModule +{ + public: + using RepeatScalingModule::getDupeCancelThreshold; +}; + +static RepeatScalingModuleTestShim *shim = nullptr; +static RepeatScalingModulePlainShim *plainShim = nullptr; + +// --------------------------------------------------------------------------- +// Scoped helpers to simulate a busy mesh for meshTooBusyForExtraRepeats() (see +// RepeatScalingModule.cpp). Install a global airTime/hopScalingModule reporting the given +// condition for the enclosing scope, restoring the previous pointer on destruction. Mirrors +// test_traffic_management's ScopedBusyAirTime. +// --------------------------------------------------------------------------- +class ScopedChannelUtil +{ + public: + explicit ScopedChannelUtil(float percent) : previous(airTime) + { + // channelUtilizationPercent() == sum(channelUtilization[]) / (PERIODS * 10 * 1000) * 100 + busy.channelUtilization[0] = static_cast(percent / 100.0f * CHANNEL_UTILIZATION_PERIODS * 10 * 1000); + airTime = &busy; + } + ~ScopedChannelUtil() { airTime = previous; } + + private: + AirTime busy; + AirTime *previous; +}; + +class ScopedAirUtilTx +{ + public: + explicit ScopedAirUtilTx(float percent) : previous(airTime) + { + // utilizationTXPercent() == sum(utilizationTX[]) / MS_IN_HOUR * 100 + busy.utilizationTX[0] = static_cast(percent / 100.0f * MS_IN_HOUR); + airTime = &busy; + } + ~ScopedAirUtilTx() { airTime = previous; } + + private: + AirTime busy; + AirTime *previous; +}; + +#if HAS_VARIABLE_HOPS +class ScopedDirectActiveNodes +{ + public: + explicit ScopedDirectActiveNodes(uint16_t count) : previous(hopScalingModule) + { + HopScalingModule::PerHopCounts counts; + counts.perHop[0] = count; + busy.setLastPerHopCountsForTest(counts); + hopScalingModule = &busy; + } + ~ScopedDirectActiveNodes() { hopScalingModule = previous; } + + private: + HopScalingModule busy; + HopScalingModule *previous; +}; +#endif + +static meshtastic_MeshPacket makeDupePacket(NodeNum from, PacketId id) +{ + meshtastic_MeshPacket p = meshtastic_MeshPacket_init_zero; + p.from = from; + p.id = id; + p.transport_mechanism = meshtastic_MeshPacket_TransportMechanism_TRANSPORT_LORA; + p.which_payload_variant = meshtastic_MeshPacket_encrypted_tag; // undecodable by default + return p; +} + +void setUp(void) +{ + shim->testThreshold = 1; + // Clear any tracking state a previous test may have left behind for our fixed test keys. + shim->clearDupeCount(kSender1, kId1); + shim->clearDupeCount(kSender2, kId2); +} + +void tearDown(void) {} + +// =========================================================================== +// Group 1 - registerDupeHeard / clearDupeCount (the ring buffer) +// =========================================================================== + +void test_registerDupeHeard_starts_at_one(void) +{ + TEST_ASSERT_EQUAL_UINT8(1, shim->registerDupeHeard(kSender1, kId1)); +} + +void test_registerDupeHeard_increments_on_repeat(void) +{ + TEST_ASSERT_EQUAL_UINT8(1, shim->registerDupeHeard(kSender1, kId1)); + TEST_ASSERT_EQUAL_UINT8(2, shim->registerDupeHeard(kSender1, kId1)); + TEST_ASSERT_EQUAL_UINT8(3, shim->registerDupeHeard(kSender1, kId1)); +} + +void test_registerDupeHeard_independent_per_packet(void) +{ + TEST_ASSERT_EQUAL_UINT8(1, shim->registerDupeHeard(kSender1, kId1)); + TEST_ASSERT_EQUAL_UINT8(2, shim->registerDupeHeard(kSender1, kId1)); + // A different (sender, id) pair tracks its own independent count. + TEST_ASSERT_EQUAL_UINT8(1, shim->registerDupeHeard(kSender2, kId2)); + TEST_ASSERT_EQUAL_UINT8(2, shim->registerDupeHeard(kSender2, kId2)); + // The first pair's count is untouched by the second's activity. + TEST_ASSERT_EQUAL_UINT8(3, shim->registerDupeHeard(kSender1, kId1)); +} + +void test_clearDupeCount_resets_tracking(void) +{ + shim->registerDupeHeard(kSender1, kId1); + shim->registerDupeHeard(kSender1, kId1); + shim->clearDupeCount(kSender1, kId1); + // After clearing, the next duplicate heard for this packet starts fresh at 1. + TEST_ASSERT_EQUAL_UINT8(1, shim->registerDupeHeard(kSender1, kId1)); +} + +void test_clearDupeCount_of_untracked_packet_is_noop(void) +{ + // Clearing a (sender, id) that was never registered must not crash or disturb other entries. + shim->registerDupeHeard(kSender1, kId1); + shim->clearDupeCount(0xFFFFFFFF, 0xFFFFFFFF); + TEST_ASSERT_EQUAL_UINT8(2, shim->registerDupeHeard(kSender1, kId1)); +} + +void test_registerDupeHeard_ring_eviction_does_not_crash(void) +{ + // DUPE_COUNT_TRACKER_SIZE is 8 (see RepeatScalingModule.h); track more distinct packets than + // that and confirm the ring buffer just evicts older entries rather than misbehaving. + for (uint32_t i = 0; i < 20; i++) { + uint8_t count = shim->registerDupeHeard(kSender1, 0x10000 + i); + TEST_ASSERT_EQUAL_UINT8(1, count); // each is a fresh (sender, id) pair + } +} + +// =========================================================================== +// Group 2 - getDupeCancelThreshold (compile-time per-portnum default) +// =========================================================================== + +void test_default_threshold_is_one_for_undecoded_packet(void) +{ + meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); + p.which_payload_variant = meshtastic_MeshPacket_encrypted_tag; + TEST_ASSERT_EQUAL_UINT8(1, plainShim->getDupeCancelThreshold(&p)); +} + +void test_default_threshold_is_one_for_unlisted_portnum(void) +{ + meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); + p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + p.decoded.portnum = meshtastic_PortNum_POSITION_APP; // no case for this portnum + TEST_ASSERT_EQUAL_UINT8(1, plainShim->getDupeCancelThreshold(&p)); +} + +void test_text_message_threshold_is_two(void) +{ + meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); + p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + p.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP; + TEST_ASSERT_EQUAL_UINT8(2, plainShim->getDupeCancelThreshold(&p)); +} + +void test_text_message_compressed_threshold_is_two(void) +{ + meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); + p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + p.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_COMPRESSED_APP; + TEST_ASSERT_EQUAL_UINT8(2, plainShim->getDupeCancelThreshold(&p)); +} + +void test_text_message_threshold_applies_regardless_of_addressing(void) +{ + // getDupeCancelThreshold only inspects portnum, not p->to - so a DM (to a specific node) + // gets the same threshold as a broadcast for the same portnum. This is what lets + // NextHopRouter::shouldFilterReceived's call to the same inherited + // FloodingRouter::perhapsCancelDupe -> RepeatScalingModule::shouldCancelDupe path extend the + // tolerance to DM text messages too, with no separate addressing-based gate. + meshtastic_MeshPacket dm = makeDupePacket(kSender1, kId1); + dm.to = 0x12345678; // a specific node, not NODENUM_BROADCAST + dm.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + dm.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP; + TEST_ASSERT_EQUAL_UINT8(2, plainShim->getDupeCancelThreshold(&dm)); +} + +// =========================================================================== +// Group 3 - meshTooBusyForExtraRepeats gate (channel/air util, direct-neighbor density) +// =========================================================================== + +void test_busy_channel_util_suppresses_text_message_threshold(void) +{ + ScopedChannelUtil busy(11.0f); // above the 10% BUSY_CHANNEL_UTIL_PERCENT gate + meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); + p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + p.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP; + TEST_ASSERT_EQUAL_UINT8(1, plainShim->getDupeCancelThreshold(&p)); +} + +void test_channel_util_at_gate_does_not_suppress(void) +{ + ScopedChannelUtil notBusy(10.0f); // exactly at the threshold, not above it + meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); + p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + p.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP; + TEST_ASSERT_EQUAL_UINT8(2, plainShim->getDupeCancelThreshold(&p)); +} + +void test_busy_air_util_tx_suppresses_text_message_threshold(void) +{ + ScopedAirUtilTx busy(5.0f); // above the 4% BUSY_AIR_UTIL_TX_PERCENT gate + meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); + p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + p.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP; + TEST_ASSERT_EQUAL_UINT8(1, plainShim->getDupeCancelThreshold(&p)); +} + +void test_air_util_tx_at_gate_does_not_suppress(void) +{ + ScopedAirUtilTx notBusy(4.0f); // exactly at the threshold, not above it + meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); + p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + p.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP; + TEST_ASSERT_EQUAL_UINT8(2, plainShim->getDupeCancelThreshold(&p)); +} + +#if HAS_VARIABLE_HOPS +void test_busy_direct_node_count_suppresses_text_message_threshold(void) +{ + ScopedDirectActiveNodes busy(11); // above the 10-node BUSY_DIRECT_ACTIVE_NODES gate + meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); + p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + p.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP; + TEST_ASSERT_EQUAL_UINT8(1, plainShim->getDupeCancelThreshold(&p)); +} + +void test_direct_node_count_at_gate_does_not_suppress(void) +{ + ScopedDirectActiveNodes notBusy(10); // exactly at the threshold, not above it + meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); + p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + p.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP; + TEST_ASSERT_EQUAL_UINT8(2, plainShim->getDupeCancelThreshold(&p)); +} +#endif + +void test_busy_mesh_does_not_affect_already_unextended_portnums(void) +{ + // A portnum whose threshold is already 1 (the default floor) has nothing for the busy-mesh + // gate to suppress - confirm it stays at 1, not some other value, while busy. + ScopedChannelUtil busy(50.0f); + meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); + p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + p.decoded.portnum = meshtastic_PortNum_POSITION_APP; + TEST_ASSERT_EQUAL_UINT8(1, plainShim->getDupeCancelThreshold(&p)); +} + +// =========================================================================== +// Group 4 - shouldCancelDupe (integration: threshold gates the cancel + clears tracking) +// =========================================================================== + +void test_shouldCancelDupe_clears_tracking_once_threshold_reached(void) +{ + shim->testThreshold = 1; // historical behavior: give up (and clear) on the very first duplicate + meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); + + TEST_ASSERT_TRUE(shim->shouldCancelDupe(&p)); + + // Threshold (1) was reached on the first call, so tracking was cleared - the next duplicate + // heard for this packet starts a fresh count, rather than continuing to accumulate. + TEST_ASSERT_EQUAL_UINT8(1, shim->registerDupeHeard(kSender1, kId1)); +} + +void test_shouldCancelDupe_waits_for_configured_repeats(void) +{ + shim->testThreshold = 3; // this node should tolerate hearing 2 repeats before giving up + meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); + + TEST_ASSERT_FALSE(shim->shouldCancelDupe(&p)); // 1st duplicate heard -> internal count 1, below threshold + + // Directly registering one more duplicate both (a) advances the count to 2 for the next + // step, and (b) proves tracking was NOT cleared by the call above: a cleared entry would + // restart at 1, not continue to 2. + TEST_ASSERT_EQUAL_UINT8(2, shim->registerDupeHeard(kSender1, kId1)); + + TEST_ASSERT_TRUE(shim->shouldCancelDupe(&p)); // 3rd duplicate heard -> internal count 3, reaches threshold + // Threshold reached inside the call above, so tracking was cleared - starts fresh at 1. + TEST_ASSERT_EQUAL_UINT8(1, shim->registerDupeHeard(kSender1, kId1)); +} + +// =========================================================================== + +void setup() +{ + initializeTestEnvironment(); + UNITY_BEGIN(); + + shim = new RepeatScalingModuleTestShim(); + plainShim = new RepeatScalingModulePlainShim(); + + printf("\n=== registerDupeHeard / clearDupeCount (ring buffer) ===\n"); + RUN_TEST(test_registerDupeHeard_starts_at_one); + RUN_TEST(test_registerDupeHeard_increments_on_repeat); + RUN_TEST(test_registerDupeHeard_independent_per_packet); + RUN_TEST(test_clearDupeCount_resets_tracking); + RUN_TEST(test_clearDupeCount_of_untracked_packet_is_noop); + RUN_TEST(test_registerDupeHeard_ring_eviction_does_not_crash); + + printf("\n=== getDupeCancelThreshold (compile-time default) ===\n"); + RUN_TEST(test_default_threshold_is_one_for_undecoded_packet); + RUN_TEST(test_default_threshold_is_one_for_unlisted_portnum); + RUN_TEST(test_text_message_threshold_is_two); + RUN_TEST(test_text_message_compressed_threshold_is_two); + RUN_TEST(test_text_message_threshold_applies_regardless_of_addressing); + + printf("\n=== meshTooBusyForExtraRepeats gate ===\n"); + RUN_TEST(test_busy_channel_util_suppresses_text_message_threshold); + RUN_TEST(test_channel_util_at_gate_does_not_suppress); + RUN_TEST(test_busy_air_util_tx_suppresses_text_message_threshold); + RUN_TEST(test_air_util_tx_at_gate_does_not_suppress); +#if HAS_VARIABLE_HOPS + RUN_TEST(test_busy_direct_node_count_suppresses_text_message_threshold); + RUN_TEST(test_direct_node_count_at_gate_does_not_suppress); +#endif + RUN_TEST(test_busy_mesh_does_not_affect_already_unextended_portnums); + + printf("\n=== shouldCancelDupe (threshold gating) ===\n"); + RUN_TEST(test_shouldCancelDupe_clears_tracking_once_threshold_reached); + RUN_TEST(test_shouldCancelDupe_waits_for_configured_repeats); + + exit(UNITY_END()); +} + +void loop() {} From 2355d5b3ccc3a8ce0ad0c0733585c053d36bf575 Mon Sep 17 00:00:00 2001 From: nomdetom Date: Wed, 8 Jul 2026 23:59:42 +0100 Subject: [PATCH 4/9] fixed latent bug --- src/mesh/FloodingRouter.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/mesh/FloodingRouter.cpp b/src/mesh/FloodingRouter.cpp index ca811c759c3..378d8401fbc 100644 --- a/src/mesh/FloodingRouter.cpp +++ b/src/mesh/FloodingRouter.cpp @@ -140,8 +140,10 @@ void FloodingRouter::perhapsCancelDupe(const meshtastic_MeshPacket *p) { if (p->transport_mechanism == meshtastic_MeshPacket_TransportMechanism_TRANSPORT_LORA && roleAllowsCancelingDupe(p)) { // RepeatScalingModule owns the "how many duplicates should we tolerate before giving up" - // decision (and its bookkeeping/logging) - see RepeatScalingModule::shouldCancelDupe. - if (repeatScalingModule && repeatScalingModule->shouldCancelDupe(p)) { + // decision (and its bookkeeping/logging) - see RepeatScalingModule::shouldCancelDupe. When + // the module is compiled out (MESHTASTIC_EXCLUDE_REPEATSCALING), fall back to the historical + // behavior of cancelling on the first duplicate rather than never cancelling at all. + if (!repeatScalingModule || repeatScalingModule->shouldCancelDupe(p)) { // cancel rebroadcast of this message *if* there was already one, unless we're a router! // But only LoRa packets should be able to trigger this. if (Router::cancelSending(p->from, p->id)) From e135e362e7132ccf2fdac3ed6577a61fbd820113 Mon Sep 17 00:00:00 2001 From: nomdetom Date: Thu, 9 Jul 2026 10:56:48 +0100 Subject: [PATCH 5/9] decrypt first, dummy --- src/mesh/NextHopRouter.cpp | 10 +++++ src/modules/RepeatScalingModule.cpp | 57 +++++++++++++++++++++++++---- src/modules/RepeatScalingModule.h | 13 +++++++ 3 files changed, 72 insertions(+), 8 deletions(-) diff --git a/src/mesh/NextHopRouter.cpp b/src/mesh/NextHopRouter.cpp index f33de779cc0..61f605967af 100644 --- a/src/mesh/NextHopRouter.cpp +++ b/src/mesh/NextHopRouter.cpp @@ -8,6 +8,7 @@ #include "modules/TrafficManagementModule.h" #endif #include "NodeDB.h" +#include "modules/RepeatScalingModule.h" NextHopRouter::NextHopRouter() {} @@ -174,6 +175,15 @@ bool NextHopRouter::perhapsRebroadcast(const meshtastic_MeshPacket *p) return true; LOG_INFO("Rebroadcast received message coming from %x", p->relay_node); + // Cache this packet's portnum against (p->from, p->id) so that when a duplicate of + // it arrives from the air later - still encrypted to us, since only this decoded + // copy ever gets decoded - RepeatScalingModule::getDupeCancelThreshold has a + // per-portnum signal to act on instead of always seeing an undecodable packet. + if (repeatScalingModule) + repeatScalingModule->noteScheduled( + p->from, p->id, + p->which_payload_variant == meshtastic_MeshPacket_decoded_tag ? p->decoded.portnum : -1); + // If exhausting hops, force hop_limit = 0 regardless of other logic if (exhaustHops) { tosend->hop_limit = 0; diff --git a/src/modules/RepeatScalingModule.cpp b/src/modules/RepeatScalingModule.cpp index de4e4431a31..161116506ae 100644 --- a/src/modules/RepeatScalingModule.cpp +++ b/src/modules/RepeatScalingModule.cpp @@ -68,13 +68,24 @@ bool meshTooBusyForExtraRepeats() // neighbor density too high) always forces the threshold back down to 1 - see its definition above. uint8_t RepeatScalingModule::getDupeCancelThreshold(const meshtastic_MeshPacket *p) { - // Portnum is only visible once a packet is decoded (i.e. we hold the channel key); packets we - // can't decrypt always get the default of 1, since we have no per-portnum signal to act on. - if (p->which_payload_variant != meshtastic_MeshPacket_decoded_tag) - return 1; + int32_t portnum; + if (p->which_payload_variant == meshtastic_MeshPacket_decoded_tag) { + portnum = p->decoded.portnum; + } else { + // Portnum is only visible once a packet is decoded (i.e. we hold the channel key). A + // duplicate heard over the air is, in the overwhelmingly common case, still encrypted to + // us here - only the one copy that made it through Router::handleReceived ever gets + // decoded. Fall back to the portnum we cached for this same (sender, id) when we + // ourselves scheduled a rebroadcast of it (see noteScheduled()); if we never scheduled + // one (or its ring slot was since evicted/cleared), we have no per-portnum signal to act + // on, so use the default of 1. + portnum = lookupNotedPortnum(p->from, p->id); + if (portnum < 0) + return 1; + } uint8_t threshold; - switch (p->decoded.portnum) { + switch (portnum) { case meshtastic_PortNum_TEXT_MESSAGE_APP: case meshtastic_PortNum_TEXT_MESSAGE_COMPRESSED_APP: // User-visible chat, broadcast or DM: no ACK/retry safety net for broadcasts, and no other @@ -91,8 +102,7 @@ uint8_t RepeatScalingModule::getDupeCancelThreshold(const meshtastic_MeshPacket // airtime on repeats when the channel is already congested or there are many direct neighbors // to reach anyway. if (threshold > 1 && meshTooBusyForExtraRepeats()) { - LOG_DEBUG("[REPEATSCALE] portnum=%d wanted threshold=%u but mesh is busy; falling back to 1", p->decoded.portnum, - threshold); + LOG_DEBUG("[REPEATSCALE] portnum=%d wanted threshold=%u but mesh is busy; falling back to 1", portnum, threshold); return 1; } @@ -114,6 +124,7 @@ uint8_t RepeatScalingModule::registerDupeHeard(NodeNum sender, PacketId id) slot.sender = sender; slot.id = id; slot.count = 1; + slot.portnum = -1; // no noteScheduled() call preceded this - no portnum signal available return slot.count; } @@ -124,16 +135,46 @@ void RepeatScalingModule::clearDupeCount(NodeNum sender, PacketId id) entry.sender = 0; entry.id = 0; entry.count = 0; + entry.portnum = -1; + return; + } + } +} + +void RepeatScalingModule::noteScheduled(NodeNum sender, PacketId id, int32_t portnum) +{ + for (auto &entry : dupeCounts) { + if (entry.id == id && entry.sender == sender) { + entry.portnum = portnum; return; } } + // Not tracked yet: claim the next ring slot, evicting whatever packet (if any) was there. + // count starts at 0 (as opposed to registerDupeHeard's 1) because scheduling our own + // rebroadcast is not itself a heard duplicate. + DupeCountEntry &slot = dupeCounts[dupeCountsNextSlot]; + dupeCountsNextSlot = (dupeCountsNextSlot + 1) % DUPE_COUNT_TRACKER_SIZE; + slot.sender = sender; + slot.id = id; + slot.count = 0; + slot.portnum = portnum; +} + +int32_t RepeatScalingModule::lookupNotedPortnum(NodeNum sender, PacketId id) const +{ + for (auto &entry : dupeCounts) { + if (entry.id == id && entry.sender == sender) + return entry.portnum; + } + return -1; } bool RepeatScalingModule::shouldCancelDupe(const meshtastic_MeshPacket *p) { const uint8_t threshold = getDupeCancelThreshold(p); const uint8_t dupesHeard = registerDupeHeard(p->from, p->id); - const int32_t portnum = (p->which_payload_variant == meshtastic_MeshPacket_decoded_tag) ? p->decoded.portnum : -1; + const int32_t portnum = + (p->which_payload_variant == meshtastic_MeshPacket_decoded_tag) ? p->decoded.portnum : lookupNotedPortnum(p->from, p->id); if (dupesHeard >= threshold) { LOG_INFO("[REPEATSCALE] Giving up own rebroadcast of 0x%08x from=0x%08x portnum=%d: heard %u/%u duplicate(s)", p->id, diff --git a/src/modules/RepeatScalingModule.h b/src/modules/RepeatScalingModule.h index e3bd3b3e6a5..38a3a653ca4 100644 --- a/src/modules/RepeatScalingModule.h +++ b/src/modules/RepeatScalingModule.h @@ -30,6 +30,14 @@ class RepeatScalingModule // test/test_repeat_scaling_module). virtual bool shouldCancelDupe(const meshtastic_MeshPacket *p); + // Call when we schedule our own rebroadcast of a packet (i.e. we successfully decoded it and + // queued it to send). Caches its portnum against (sender, id) so that later, when a duplicate + // of this same packet arrives from the air - still encrypted to us, since only the copy we + // process through Router::handleReceived ever gets decoded - getDupeCancelThreshold() has a + // per-portnum signal to act on instead of always falling back to the undecodable-packet + // default. portnum should be -1 if the packet we're scheduling could not be decoded either. + void noteScheduled(NodeNum sender, PacketId id, int32_t portnum); + protected: // How many duplicate rebroadcasts of a packet we require to hear (see the per-portnum switch // in RepeatScalingModule.cpp) before giving up on our own scheduled rebroadcast of it. Virtual @@ -47,12 +55,17 @@ class RepeatScalingModule // so a reused ring slot can't cause a stale hit against an unrelated future packet. void clearDupeCount(NodeNum sender, PacketId id); + // Returns the portnum cached by noteScheduled() for (sender, id), or -1 if we never scheduled + // a rebroadcast of it (or its ring slot has since been evicted/cleared). + int32_t lookupNotedPortnum(NodeNum sender, PacketId id) const; + private: static constexpr uint8_t DUPE_COUNT_TRACKER_SIZE = 8; struct DupeCountEntry { NodeNum sender = 0; PacketId id = 0; uint8_t count = 0; + int32_t portnum = -1; }; DupeCountEntry dupeCounts[DUPE_COUNT_TRACKER_SIZE]; uint8_t dupeCountsNextSlot = 0; From ab8fa5c466e0cee35cebc40d66100ee9471abba1 Mon Sep 17 00:00:00 2001 From: nomdetom Date: Thu, 9 Jul 2026 12:33:56 +0100 Subject: [PATCH 6/9] more tests --- src/modules/RepeatScalingModule.cpp | 70 +++-- test/test_repeat_scaling_module/test_main.cpp | 247 +++++++++++++++++- 2 files changed, 291 insertions(+), 26 deletions(-) diff --git a/src/modules/RepeatScalingModule.cpp b/src/modules/RepeatScalingModule.cpp index 161116506ae..ca603c11487 100644 --- a/src/modules/RepeatScalingModule.cpp +++ b/src/modules/RepeatScalingModule.cpp @@ -47,24 +47,32 @@ bool meshTooBusyForExtraRepeats() // Compile-time, per-portnum threshold for how many duplicate rebroadcasts of a packet we must hear // before giving up on our own scheduled rebroadcast of it. The default of 1 (used for any portnum -// not given a case below, and for packets we can't decode - see below) preserves the historical -// behavior: cancel our own rebroadcast as soon as we hear the first duplicate. Raising a portnum's -// threshold makes this node more persistent for that traffic type, continuing to retransmit even -// after hearing some other nodes already repeat it - at the cost of extra airtime for packets -// that are, on balance, probably going to reach their destination anyway. +// not given a case below) preserves the historical behavior: cancel our own rebroadcast as soon as +// we hear the first duplicate. Raising a portnum's threshold makes this node more persistent for +// that traffic type, continuing to retransmit even after hearing some other nodes already repeat +// it - at the cost of extra airtime for packets that are, on balance, probably going to reach their +// destination anyway. // // This same switch applies uniformly to broadcasts and DMs: NextHopRouter::shouldFilterReceived // calls the same (inherited, non-overridden) FloodingRouter::perhapsCancelDupe -> shouldCancelDupe // path when it hears a duplicate it wasn't explicitly asked to relay - both in the fallback-to- // flooding case and the general "duplicate heard, we're not the assigned next hop" case - so a DM // of a listed portnum gets the same extra tolerance as a broadcast of that portnum, with no -// separate to/from-based gating needed. +// separate to/from-based gating needed. In practice this only fires for a *decodable* DM - a DM +// not addressed to us can never be decoded by us, so it falls to the next_hop-gated fallback below. // // This is a compile-time table (not a runtime/protobuf config) because the desired threshold is // expected to vary per packet type rather than being a single device-wide knob; edit it to tune // for a given deployment. // -// Regardless of the table below, meshTooBusyForExtraRepeats() (channel/air utilization or direct- +// When the portnum can't be determined at all (packet still encrypted and no noteScheduled() cache +// hit - see below), the portnum switch is skipped and the threshold instead falls back to a +// next_hop-based gate: NO_NEXT_HOP_PREFERENCE (flood-relayed, e.g. a broadcast or a DM with no known +// route) gets the same tolerance as a decoded text message, since it has the same "uncertain +// single-path delivery" shape; a specific next_hop byte (directed route known) does not, since +// delivery there is already backed by the sender's end-to-end ACK/retry. +// +// Regardless of which path above is taken, meshTooBusyForExtraRepeats() (channel/air utilization or direct- // neighbor density too high) always forces the threshold back down to 1 - see its definition above. uint8_t RepeatScalingModule::getDupeCancelThreshold(const meshtastic_MeshPacket *p) { @@ -76,29 +84,43 @@ uint8_t RepeatScalingModule::getDupeCancelThreshold(const meshtastic_MeshPacket // duplicate heard over the air is, in the overwhelmingly common case, still encrypted to // us here - only the one copy that made it through Router::handleReceived ever gets // decoded. Fall back to the portnum we cached for this same (sender, id) when we - // ourselves scheduled a rebroadcast of it (see noteScheduled()); if we never scheduled - // one (or its ring slot was since evicted/cleared), we have no per-portnum signal to act - // on, so use the default of 1. + // ourselves scheduled a rebroadcast of it (see noteScheduled()); a PKI DM not addressed to + // us can never be decoded by us at all (Router::perhapsDecode's PKI branch requires + // isToUs(p)), so this cache will never have an entry for one. portnum = lookupNotedPortnum(p->from, p->id); - if (portnum < 0) - return 1; } uint8_t threshold; - switch (portnum) { - case meshtastic_PortNum_TEXT_MESSAGE_APP: - case meshtastic_PortNum_TEXT_MESSAGE_COMPRESSED_APP: - // User-visible chat, broadcast or DM: no ACK/retry safety net for broadcasts, and no other - // portnum-specific reason to prefer airtime savings over delivery odds. Tolerate one repeat - // heard from another node before giving up on our own rebroadcast. - threshold = 2; - break; - default: - threshold = 1; - break; + if (portnum >= 0) { + switch (portnum) { + case meshtastic_PortNum_TEXT_MESSAGE_APP: + case meshtastic_PortNum_TEXT_MESSAGE_COMPRESSED_APP: + // User-visible chat, broadcast or DM: no ACK/retry safety net for broadcasts, and no + // other portnum-specific reason to prefer airtime savings over delivery odds. Tolerate + // one repeat heard from another node before giving up on our own rebroadcast. + threshold = 2; + break; + default: + threshold = 1; + break; + } + } else { + // Portnum is genuinely unknowable here (most commonly a relayed PKI DM not addressed to + // us). next_hop is a plaintext MeshPacket header field - unlike portnum, it's visible + // regardless of whether we can decrypt the payload - so use it as a coarser proxy signal. + // NO_NEXT_HOP_PREFERENCE means this packet is being flood-relayed (every broadcast, or a DM + // with no directed route known yet): the same "uncertain single-path delivery" shape that + // justifies extra tolerance for a decoded text message. A specific next_hop byte means a + // directed route is already known; per perhapsRebroadcast's own gate we'd only reach this + // point as that assigned relay or in flooding mode, so a duplicate heard here is more + // likely a relay-byte collision than genuine flood redundancy, and delivery is already + // backed by the sender's end-to-end ACK/retry - no benefit to extra tolerance. + threshold = (p->next_hop == NO_NEXT_HOP_PREFERENCE) ? 2 : 1; + LOG_DEBUG("[REPEATSCALE] portnum unknown for 0x%08x from=0x%08x; next_hop=0x%x -> threshold=%u", p->id, p->from, + p->next_hop, threshold); } - // A busy/dense mesh overrides any portnum-specific extra tolerance: not worth spending more + // A busy/dense mesh overrides any extra tolerance decided above: not worth spending more // airtime on repeats when the channel is already congested or there are many direct neighbors // to reach anyway. if (threshold > 1 && meshTooBusyForExtraRepeats()) { diff --git a/test/test_repeat_scaling_module/test_main.cpp b/test/test_repeat_scaling_module/test_main.cpp index 8f749f09ed5..a886913c62d 100644 --- a/test/test_repeat_scaling_module/test_main.cpp +++ b/test/test_repeat_scaling_module/test_main.cpp @@ -44,7 +44,9 @@ class RepeatScalingModuleTestShim : public RepeatScalingModule class RepeatScalingModulePlainShim : public RepeatScalingModule { public: + using RepeatScalingModule::clearDupeCount; using RepeatScalingModule::getDupeCancelThreshold; + using RepeatScalingModule::registerDupeHeard; }; static RepeatScalingModuleTestShim *shim = nullptr; @@ -123,6 +125,8 @@ void setUp(void) // Clear any tracking state a previous test may have left behind for our fixed test keys. shim->clearDupeCount(kSender1, kId1); shim->clearDupeCount(kSender2, kId2); + plainShim->clearDupeCount(kSender1, kId1); + plainShim->clearDupeCount(kSender2, kId2); } void tearDown(void) {} @@ -185,10 +189,14 @@ void test_registerDupeHeard_ring_eviction_does_not_crash(void) // Group 2 - getDupeCancelThreshold (compile-time per-portnum default) // =========================================================================== -void test_default_threshold_is_one_for_undecoded_packet(void) +void test_default_threshold_is_one_for_undecoded_directed_packet(void) { + // Portnum unknowable (encrypted, no noteScheduled cache hit) falls back to the next_hop gate + // (see Group 2b below) - a directed route (specific next_hop byte, not flooding) keeps the + // historical default of 1. meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); p.which_payload_variant = meshtastic_MeshPacket_encrypted_tag; + p.next_hop = 0x42; // a specific, directed relay byte - not NO_NEXT_HOP_PREFERENCE TEST_ASSERT_EQUAL_UINT8(1, plainShim->getDupeCancelThreshold(&p)); } @@ -230,6 +238,96 @@ void test_text_message_threshold_applies_regardless_of_addressing(void) TEST_ASSERT_EQUAL_UINT8(2, plainShim->getDupeCancelThreshold(&dm)); } +// =========================================================================== +// Group 2b - next_hop gate for packets whose portnum can't be determined at all (most commonly a +// relayed PKI DM not addressed to us, which we can never decode - see Router::perhapsDecode's PKI +// branch, gated on isToUs(p)). next_hop is a plaintext MeshPacket header field, so it's visible +// even here, unlike portnum. +// =========================================================================== + +// A DM (addressed to a specific node, not NODENUM_BROADCAST) with no directed route known yet - +// the realistic shape of a relayed PKI DM we can never decrypt (see the module-level comment on +// Group 2b above). NO_NEXT_HOP_PREFERENCE is what a route-less DM actually carries in this state +// (see NextHopRouter::getNextHop/send), same as any broadcast. +static meshtastic_MeshPacket makeUndecodableFloodedDm(NodeNum from, PacketId id) +{ + meshtastic_MeshPacket p = makeDupePacket(from, id); + p.to = 0x12345678; // a specific node, not NODENUM_BROADCAST + p.next_hop = NO_NEXT_HOP_PREFERENCE; + return p; +} + +void test_undecodable_flooded_packet_gets_text_message_tolerance(void) +{ + // NO_NEXT_HOP_PREFERENCE means this packet is being flood-relayed (every broadcast, or a DM + // with no directed route known yet) - treated the same as a decoded text message. + meshtastic_MeshPacket p = makeUndecodableFloodedDm(kSender1, kId1); + TEST_ASSERT_EQUAL_UINT8(2, plainShim->getDupeCancelThreshold(&p)); +} + +void test_undecodable_directed_packet_keeps_default_threshold(void) +{ + // A specific next_hop byte means a directed route is already known - delivery is backed by + // the sender's end-to-end ACK/retry, so no extra tolerance. + meshtastic_MeshPacket p = makeDupePacket(kSender1, kId1); + p.next_hop = 0x42; + TEST_ASSERT_EQUAL_UINT8(1, plainShim->getDupeCancelThreshold(&p)); +} + +void test_next_hop_gate_is_overridden_by_a_cached_portnum(void) +{ + // A cache hit (see noteScheduled()) always takes priority over the next_hop gate, even for a + // flood-relayed packet whose real, known portnum doesn't warrant extra tolerance. + plainShim->noteScheduled(kSender1, kId1, meshtastic_PortNum_POSITION_APP); + meshtastic_MeshPacket p = makeUndecodableFloodedDm(kSender1, kId1); + TEST_ASSERT_EQUAL_UINT8(1, plainShim->getDupeCancelThreshold(&p)); +} + +void test_next_hop_gate_suppressed_by_busy_channel_util(void) +{ + // Same busy-mesh override that applies to the portnum table also applies to the next_hop gate. + ScopedChannelUtil busy(11.0f); // above the 10% BUSY_CHANNEL_UTIL_PERCENT gate + meshtastic_MeshPacket p = makeUndecodableFloodedDm(kSender1, kId1); + TEST_ASSERT_EQUAL_UINT8(1, plainShim->getDupeCancelThreshold(&p)); +} + +void test_next_hop_gate_tolerated_when_channel_util_low(void) +{ + ScopedChannelUtil notBusy(10.0f); // exactly at the threshold, not above it + meshtastic_MeshPacket p = makeUndecodableFloodedDm(kSender1, kId1); + TEST_ASSERT_EQUAL_UINT8(2, plainShim->getDupeCancelThreshold(&p)); +} + +void test_next_hop_gate_suppressed_by_busy_air_util_tx(void) +{ + ScopedAirUtilTx busy(5.0f); // above the 4% BUSY_AIR_UTIL_TX_PERCENT gate + meshtastic_MeshPacket p = makeUndecodableFloodedDm(kSender1, kId1); + TEST_ASSERT_EQUAL_UINT8(1, plainShim->getDupeCancelThreshold(&p)); +} + +void test_next_hop_gate_tolerated_when_air_util_tx_low(void) +{ + ScopedAirUtilTx notBusy(4.0f); // exactly at the threshold, not above it + meshtastic_MeshPacket p = makeUndecodableFloodedDm(kSender1, kId1); + TEST_ASSERT_EQUAL_UINT8(2, plainShim->getDupeCancelThreshold(&p)); +} + +#if HAS_VARIABLE_HOPS +void test_next_hop_gate_suppressed_by_busy_direct_node_count(void) +{ + ScopedDirectActiveNodes busy(11); // above the 10-node BUSY_DIRECT_ACTIVE_NODES gate + meshtastic_MeshPacket p = makeUndecodableFloodedDm(kSender1, kId1); + TEST_ASSERT_EQUAL_UINT8(1, plainShim->getDupeCancelThreshold(&p)); +} + +void test_next_hop_gate_tolerated_when_direct_node_count_low(void) +{ + ScopedDirectActiveNodes notBusy(10); // exactly at the threshold, not above it + meshtastic_MeshPacket p = makeUndecodableFloodedDm(kSender1, kId1); + TEST_ASSERT_EQUAL_UINT8(2, plainShim->getDupeCancelThreshold(&p)); +} +#endif + // =========================================================================== // Group 3 - meshTooBusyForExtraRepeats gate (channel/air util, direct-neighbor density) // =========================================================================== @@ -334,6 +432,128 @@ void test_shouldCancelDupe_waits_for_configured_repeats(void) TEST_ASSERT_EQUAL_UINT8(1, shim->registerDupeHeard(kSender1, kId1)); } +// =========================================================================== +// Group 5 - noteScheduled + encrypted duplicates (as heard for real over the air) +// +// In production, a duplicate rebroadcast heard from another node is still encrypted to us: only +// the one copy that made it through Router::handleReceived ever gets decoded (see +// NextHopRouter::perhapsRebroadcast, which calls noteScheduled() with that copy's portnum once it +// schedules our own rebroadcast). Group 2's tests construct an already-decoded packet directly, +// which proves the portnum switch itself is correct but - as the "portnum=-1" bug demonstrated - +// does not prove the real, always-encrypted duplicate gets the right threshold. These tests start +// from an encrypted packet, exactly as shouldCancelDupe receives it from FloodingRouter, and rely +// solely on noteScheduled() (the same call NextHopRouter makes) for portnum visibility. +// =========================================================================== + +void test_encrypted_dupe_uses_portnum_noted_when_we_scheduled_our_own_rebroadcast(void) +{ + // Simulates NextHopRouter::perhapsRebroadcast having decoded and scheduled our own copy. + plainShim->noteScheduled(kSender1, kId1, meshtastic_PortNum_TEXT_MESSAGE_APP); + + // The duplicate heard over the air is still encrypted - this is the packet shape + // shouldCancelDupe actually sees in production. Broadcast-addressed (to left at its + // zero-init default, distinct from a DM's specific-node "to" below). + meshtastic_MeshPacket dupe = makeDupePacket(kSender1, kId1); + TEST_ASSERT_TRUE(dupe.which_payload_variant == meshtastic_MeshPacket_encrypted_tag); + + TEST_ASSERT_EQUAL_UINT8(2, plainShim->getDupeCancelThreshold(&dupe)); +} + +void test_encrypted_dm_dupe_uses_portnum_noted_when_we_scheduled_our_own_rebroadcast(void) +{ + // Same as the broadcast case above, but addressed as a DM (see the module-level comment in + // RepeatScalingModule.cpp: NextHopRouter::shouldFilterReceived's dupe path reuses the same + // inherited FloodingRouter::perhapsCancelDupe -> shouldCancelDupe call, so a DM's encrypted + // duplicate must get the same noteScheduled-cached tolerance a broadcast's does). + plainShim->noteScheduled(kSender1, kId1, meshtastic_PortNum_TEXT_MESSAGE_APP); + + meshtastic_MeshPacket dupe = makeDupePacket(kSender1, kId1); + dupe.to = 0x12345678; // a specific node, not NODENUM_BROADCAST + TEST_ASSERT_TRUE(dupe.which_payload_variant == meshtastic_MeshPacket_encrypted_tag); + + TEST_ASSERT_EQUAL_UINT8(2, plainShim->getDupeCancelThreshold(&dupe)); +} + +void test_encrypted_dupe_without_noteScheduled_falls_back_to_next_hop_gate(void) +{ + // We never scheduled our own rebroadcast of this (sender, id) - e.g. we're not the assigned + // next hop, or we never decoded it - so there is no cached portnum at all. This is the + // pre-fix behavior for every encrypted duplicate; now it falls to the next_hop gate (Group 2b) + // instead of unconditionally defaulting to 1 - pin next_hop explicitly so this test isolates + // "no cache" from that gate's own behavior. + meshtastic_MeshPacket dupe = makeDupePacket(kSender2, kId2); + dupe.next_hop = 0x42; // a specific, directed relay byte - not NO_NEXT_HOP_PREFERENCE + TEST_ASSERT_EQUAL_UINT8(1, plainShim->getDupeCancelThreshold(&dupe)); +} + +void test_noteScheduled_is_keyed_per_sender_and_id(void) +{ + plainShim->noteScheduled(kSender1, kId1, meshtastic_PortNum_TEXT_MESSAGE_APP); + + // A different (sender, id) pair must not pick up kSender1/kId1's noted portnum. Pin next_hop + // so the lack of a cache hit (not the next_hop gate) is what's under test here. + meshtastic_MeshPacket unrelated = makeDupePacket(kSender2, kId2); + unrelated.next_hop = 0x42; + TEST_ASSERT_EQUAL_UINT8(1, plainShim->getDupeCancelThreshold(&unrelated)); + + meshtastic_MeshPacket dupe = makeDupePacket(kSender1, kId1); + TEST_ASSERT_EQUAL_UINT8(2, plainShim->getDupeCancelThreshold(&dupe)); +} + +void test_noteScheduled_updates_existing_entry(void) +{ + // registerDupeHeard may have already claimed a ring slot for this (sender, id) - e.g. we + // heard a duplicate before deciding to (re)schedule our own rebroadcast of it (hop-limit + // upgrade). noteScheduled must update that slot's portnum in place, not just no-op. + plainShim->registerDupeHeard(kSender1, kId1); + plainShim->noteScheduled(kSender1, kId1, meshtastic_PortNum_TEXT_MESSAGE_APP); + + meshtastic_MeshPacket dupe = makeDupePacket(kSender1, kId1); + TEST_ASSERT_EQUAL_UINT8(2, plainShim->getDupeCancelThreshold(&dupe)); +} + +void test_clearDupeCount_also_forgets_noted_portnum(void) +{ + plainShim->noteScheduled(kSender1, kId1, meshtastic_PortNum_TEXT_MESSAGE_APP); + plainShim->clearDupeCount(kSender1, kId1); + + // Once cleared (e.g. shouldCancelDupe already gave up on this packet), a stale portnum must + // not leak into whatever unrelated packet reuses this ring slot next. Pin next_hop so the + // cleared cache (not the next_hop gate) is what's under test here. + meshtastic_MeshPacket dupe = makeDupePacket(kSender1, kId1); + dupe.next_hop = 0x42; + TEST_ASSERT_EQUAL_UINT8(1, plainShim->getDupeCancelThreshold(&dupe)); +} + +void test_shouldCancelDupe_end_to_end_with_encrypted_duplicate(void) +{ + // Full path through the real (non-overridden) threshold logic, starting from an encrypted + // duplicate: this is the regression test for the "portnum=-1" bug. Before the noteScheduled + // cache existed, this packet always fell back to threshold 1 and cancelled on the very first + // heard duplicate, regardless of portnum. + RepeatScalingModulePlainShim endToEnd; + endToEnd.noteScheduled(kSender1, kId1, meshtastic_PortNum_TEXT_MESSAGE_APP); + + meshtastic_MeshPacket dupe = makeDupePacket(kSender1, kId1); + TEST_ASSERT_FALSE(endToEnd.shouldCancelDupe(&dupe)); // 1st duplicate: tolerated, threshold is 2 + TEST_ASSERT_TRUE(endToEnd.shouldCancelDupe(&dupe)); // 2nd duplicate: threshold reached, cancel +} + +void test_shouldCancelDupe_end_to_end_with_encrypted_dm_duplicate(void) +{ + // Same end-to-end regression test as above, but for a DM (to a specific node rather than + // NODENUM_BROADCAST). Confirms the noteScheduled cache extends the same tolerance to a DM's + // encrypted duplicate that it does to a broadcast's - matching how NextHopRouter routes both + // through the same inherited shouldCancelDupe call with no separate addressing-based gate. + RepeatScalingModulePlainShim endToEnd; + endToEnd.noteScheduled(kSender1, kId1, meshtastic_PortNum_TEXT_MESSAGE_APP); + + meshtastic_MeshPacket dupe = makeDupePacket(kSender1, kId1); + dupe.to = 0x12345678; // a specific node, not NODENUM_BROADCAST + TEST_ASSERT_FALSE(endToEnd.shouldCancelDupe(&dupe)); // 1st duplicate: tolerated, threshold is 2 + TEST_ASSERT_TRUE(endToEnd.shouldCancelDupe(&dupe)); // 2nd duplicate: threshold reached, cancel +} + // =========================================================================== void setup() @@ -353,12 +573,25 @@ void setup() RUN_TEST(test_registerDupeHeard_ring_eviction_does_not_crash); printf("\n=== getDupeCancelThreshold (compile-time default) ===\n"); - RUN_TEST(test_default_threshold_is_one_for_undecoded_packet); + RUN_TEST(test_default_threshold_is_one_for_undecoded_directed_packet); RUN_TEST(test_default_threshold_is_one_for_unlisted_portnum); RUN_TEST(test_text_message_threshold_is_two); RUN_TEST(test_text_message_compressed_threshold_is_two); RUN_TEST(test_text_message_threshold_applies_regardless_of_addressing); + printf("\n=== next_hop gate (undecodable packets, e.g. a relayed PKI DM) ===\n"); + RUN_TEST(test_undecodable_flooded_packet_gets_text_message_tolerance); + RUN_TEST(test_undecodable_directed_packet_keeps_default_threshold); + RUN_TEST(test_next_hop_gate_is_overridden_by_a_cached_portnum); + RUN_TEST(test_next_hop_gate_suppressed_by_busy_channel_util); + RUN_TEST(test_next_hop_gate_tolerated_when_channel_util_low); + RUN_TEST(test_next_hop_gate_suppressed_by_busy_air_util_tx); + RUN_TEST(test_next_hop_gate_tolerated_when_air_util_tx_low); +#if HAS_VARIABLE_HOPS + RUN_TEST(test_next_hop_gate_suppressed_by_busy_direct_node_count); + RUN_TEST(test_next_hop_gate_tolerated_when_direct_node_count_low); +#endif + printf("\n=== meshTooBusyForExtraRepeats gate ===\n"); RUN_TEST(test_busy_channel_util_suppresses_text_message_threshold); RUN_TEST(test_channel_util_at_gate_does_not_suppress); @@ -374,6 +607,16 @@ void setup() RUN_TEST(test_shouldCancelDupe_clears_tracking_once_threshold_reached); RUN_TEST(test_shouldCancelDupe_waits_for_configured_repeats); + printf("\n=== noteScheduled + encrypted duplicates (as heard for real over the air) ===\n"); + RUN_TEST(test_encrypted_dupe_uses_portnum_noted_when_we_scheduled_our_own_rebroadcast); + RUN_TEST(test_encrypted_dm_dupe_uses_portnum_noted_when_we_scheduled_our_own_rebroadcast); + RUN_TEST(test_encrypted_dupe_without_noteScheduled_falls_back_to_next_hop_gate); + RUN_TEST(test_noteScheduled_is_keyed_per_sender_and_id); + RUN_TEST(test_noteScheduled_updates_existing_entry); + RUN_TEST(test_clearDupeCount_also_forgets_noted_portnum); + RUN_TEST(test_shouldCancelDupe_end_to_end_with_encrypted_duplicate); + RUN_TEST(test_shouldCancelDupe_end_to_end_with_encrypted_dm_duplicate); + exit(UNITY_END()); } From 52289192f4685bd0320a5f3f63e09e320c814b23 Mon Sep 17 00:00:00 2001 From: nomdetom Date: Thu, 9 Jul 2026 15:19:05 +0100 Subject: [PATCH 7/9] moredebug --- src/mesh/RadioLibInterface.cpp | 7 +++++++ src/modules/RepeatScalingModule.cpp | 15 ++++++++++++--- src/modules/RepeatScalingModule.h | 6 ++++++ 3 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/mesh/RadioLibInterface.cpp b/src/mesh/RadioLibInterface.cpp index 5a9b292cda8..7efd912ce1e 100644 --- a/src/mesh/RadioLibInterface.cpp +++ b/src/mesh/RadioLibInterface.cpp @@ -11,6 +11,7 @@ #if !MESHTASTIC_EXCLUDE_BEACON #include "modules/MeshBeaconModule.h" #endif +#include "modules/RepeatScalingModule.h" #include #include @@ -789,6 +790,12 @@ bool RadioLibInterface::startSend(meshtastic_MeshPacket *txp) enableInterrupt(isrTxLevel0); lastTxStart = millis(); printPacket("Started Tx", txp); + if (repeatScalingModule) { + uint8_t dupesTolerated = repeatScalingModule->getToleratedDupeCount(txp->from, txp->id); + if (dupesTolerated > 0) + LOG_DEBUG("[REPEATSCALE] Transmitting 0x%08x from=0x%08x after tolerating %u duplicate(s)", txp->id, + txp->from, dupesTolerated); + } #ifdef LED_LORA digitalWrite(LED_LORA, LED_STATE_ON); #endif diff --git a/src/modules/RepeatScalingModule.cpp b/src/modules/RepeatScalingModule.cpp index ca603c11487..9882a2c5a15 100644 --- a/src/modules/RepeatScalingModule.cpp +++ b/src/modules/RepeatScalingModule.cpp @@ -62,8 +62,7 @@ bool meshTooBusyForExtraRepeats() // not addressed to us can never be decoded by us, so it falls to the next_hop-gated fallback below. // // This is a compile-time table (not a runtime/protobuf config) because the desired threshold is -// expected to vary per packet type rather than being a single device-wide knob; edit it to tune -// for a given deployment. +// not yet worked out. // // When the portnum can't be determined at all (packet still encrypted and no noteScheduled() cache // hit - see below), the portnum switch is skipped and the threshold instead falls back to a @@ -191,6 +190,15 @@ int32_t RepeatScalingModule::lookupNotedPortnum(NodeNum sender, PacketId id) con return -1; } +uint8_t RepeatScalingModule::getToleratedDupeCount(NodeNum sender, PacketId id) const +{ + for (auto &entry : dupeCounts) { + if (entry.id == id && entry.sender == sender) + return entry.count; + } + return 0; +} + bool RepeatScalingModule::shouldCancelDupe(const meshtastic_MeshPacket *p) { const uint8_t threshold = getDupeCancelThreshold(p); @@ -205,7 +213,8 @@ bool RepeatScalingModule::shouldCancelDupe(const meshtastic_MeshPacket *p) return true; } - LOG_DEBUG("[REPEATSCALE] Tolerating duplicate %u/%u of 0x%08x from=0x%08x portnum=%d: keeping own rebroadcast queued", + LOG_DEBUG("[REPEATSCALE] Tolerated duplicate %u/%u of 0x%08x from=0x%08x portnum=%d: will still transmit our own " + "rebroadcast", dupesHeard, threshold, p->id, p->from, portnum); return false; } diff --git a/src/modules/RepeatScalingModule.h b/src/modules/RepeatScalingModule.h index 38a3a653ca4..95a8406a990 100644 --- a/src/modules/RepeatScalingModule.h +++ b/src/modules/RepeatScalingModule.h @@ -38,6 +38,12 @@ class RepeatScalingModule // default. portnum should be -1 if the packet we're scheduling could not be decoded either. void noteScheduled(NodeNum sender, PacketId id, int32_t portnum); + // Returns how many duplicates have been heard (and tolerated) so far for (sender, id), or 0 if + // none have (including if we never scheduled/heard anything for it at all). Meant for logging + // at the point of actual transmission (see RadioLibInterface::startSend) - if a packet reaches + // that point at all, any duplicates counted here were tolerated, not cancelled on. + uint8_t getToleratedDupeCount(NodeNum sender, PacketId id) const; + protected: // How many duplicate rebroadcasts of a packet we require to hear (see the per-portnum switch // in RepeatScalingModule.cpp) before giving up on our own scheduled rebroadcast of it. Virtual From 045ac61be6d0a5c4ae7e3f1d3e963fca15fb262d Mon Sep 17 00:00:00 2001 From: nomdetom Date: Thu, 9 Jul 2026 22:11:19 +0100 Subject: [PATCH 8/9] nitpicks --- src/modules/RepeatScalingModule.cpp | 113 ++++++------------ src/modules/RepeatScalingModule.h | 44 ++----- test/test_repeat_scaling_module/test_main.cpp | 53 ++------ 3 files changed, 64 insertions(+), 146 deletions(-) diff --git a/src/modules/RepeatScalingModule.cpp b/src/modules/RepeatScalingModule.cpp index 9882a2c5a15..e0bb3f9c584 100644 --- a/src/modules/RepeatScalingModule.cpp +++ b/src/modules/RepeatScalingModule.cpp @@ -6,19 +6,34 @@ RepeatScalingModule *repeatScalingModule; +// Design notes for getDupeCancelThreshold()'s policy (kept here rather than inline): +// +// Historically we cancel our own queued rebroadcast the instant we hear one duplicate from another +// node. This module lets selected packet types instead tolerate a few heard duplicates first, +// trading a little extra airtime for better delivery odds. Thresholds live in a compile-time +// per-portnum switch (not runtime config) because the right values aren't yet settled. +// +// The switch applies uniformly to broadcasts and DMs - NextHopRouter routes duplicates through the +// same FloodingRouter::perhapsCancelDupe -> shouldCancelDupe path - so a DM of a listed portnum +// gets the same tolerance as a broadcast of it, and only ever for a *decodable* DM (a DM not +// addressed to us can't be decoded by us). +// +// When the portnum can't be determined (packet still encrypted, no noteScheduled() cache hit), we +// fall back to a next_hop gate: NO_NEXT_HOP_PREFERENCE (flood-relayed) gets text-message tolerance +// since it has the same uncertain single-path shape; a specific next_hop does not, as delivery +// there is already backed by the sender's end-to-end ACK/retry. +// +// Either way, meshTooBusyForExtraRepeats() forces the threshold back to 1 on a busy/dense mesh. + namespace { -// Above any of these, the mesh is busy/dense enough that extra-repeat tolerance (see -// RepeatScalingModule::getDupeCancelThreshold) would just be adding airtime to an already-congested -// channel for little extra delivery benefit - so we fall back to the historical "cancel on first -// duplicate" behavior regardless of what the portnum table says. +// Thresholds above which the mesh is busy/dense enough that extra repeats aren't worth the airtime. constexpr float BUSY_CHANNEL_UTIL_PERCENT = 10.0f; constexpr float BUSY_AIR_UTIL_TX_PERCENT = 4.0f; constexpr uint16_t BUSY_DIRECT_ACTIVE_NODES = 10; -// True if channel/air utilization or estimated direct-neighbor density indicates the mesh is -// too busy to spend extra airtime on repeat-tolerant rebroadcasts. Logs which specific condition -// (if any) tripped, so the reason a packet fell back to the historical threshold is visible. +// True if channel/air utilization or direct-neighbor density says the mesh is too busy for extra +// repeats. Logs which condition tripped. bool meshTooBusyForExtraRepeats() { if (airTime && airTime->channelUtilizationPercent() > BUSY_CHANNEL_UTIL_PERCENT) { @@ -32,9 +47,7 @@ bool meshTooBusyForExtraRepeats() return true; } #if HAS_VARIABLE_HOPS - // getLastPerHopCounts().perHop[0] is HopScalingModule's estimate of active (heard within its - // rolling window) direct (hop_away == 0) neighbors - the same "active and direct" notion used - // for its own hop-limit recommendation. + // perHop[0] is HopScalingModule's estimate of active direct (hop_away == 0) neighbors. if (hopScalingModule && hopScalingModule->getLastPerHopCounts().perHop[0] > BUSY_DIRECT_ACTIVE_NODES) { LOG_DEBUG("[REPEATSCALE] Mesh busy: directActiveNodes=%u > %u", hopScalingModule->getLastPerHopCounts().perHop[0], BUSY_DIRECT_ACTIVE_NODES); @@ -45,58 +58,17 @@ bool meshTooBusyForExtraRepeats() } } // namespace -// Compile-time, per-portnum threshold for how many duplicate rebroadcasts of a packet we must hear -// before giving up on our own scheduled rebroadcast of it. The default of 1 (used for any portnum -// not given a case below) preserves the historical behavior: cancel our own rebroadcast as soon as -// we hear the first duplicate. Raising a portnum's threshold makes this node more persistent for -// that traffic type, continuing to retransmit even after hearing some other nodes already repeat -// it - at the cost of extra airtime for packets that are, on balance, probably going to reach their -// destination anyway. -// -// This same switch applies uniformly to broadcasts and DMs: NextHopRouter::shouldFilterReceived -// calls the same (inherited, non-overridden) FloodingRouter::perhapsCancelDupe -> shouldCancelDupe -// path when it hears a duplicate it wasn't explicitly asked to relay - both in the fallback-to- -// flooding case and the general "duplicate heard, we're not the assigned next hop" case - so a DM -// of a listed portnum gets the same extra tolerance as a broadcast of that portnum, with no -// separate to/from-based gating needed. In practice this only fires for a *decodable* DM - a DM -// not addressed to us can never be decoded by us, so it falls to the next_hop-gated fallback below. -// -// This is a compile-time table (not a runtime/protobuf config) because the desired threshold is -// not yet worked out. -// -// When the portnum can't be determined at all (packet still encrypted and no noteScheduled() cache -// hit - see below), the portnum switch is skipped and the threshold instead falls back to a -// next_hop-based gate: NO_NEXT_HOP_PREFERENCE (flood-relayed, e.g. a broadcast or a DM with no known -// route) gets the same tolerance as a decoded text message, since it has the same "uncertain -// single-path delivery" shape; a specific next_hop byte (directed route known) does not, since -// delivery there is already backed by the sender's end-to-end ACK/retry. -// -// Regardless of which path above is taken, meshTooBusyForExtraRepeats() (channel/air utilization or direct- -// neighbor density too high) always forces the threshold back down to 1 - see its definition above. +// Per-portnum duplicate-tolerance threshold; see the design notes above for the full rationale. uint8_t RepeatScalingModule::getDupeCancelThreshold(const meshtastic_MeshPacket *p) { - int32_t portnum; - if (p->which_payload_variant == meshtastic_MeshPacket_decoded_tag) { - portnum = p->decoded.portnum; - } else { - // Portnum is only visible once a packet is decoded (i.e. we hold the channel key). A - // duplicate heard over the air is, in the overwhelmingly common case, still encrypted to - // us here - only the one copy that made it through Router::handleReceived ever gets - // decoded. Fall back to the portnum we cached for this same (sender, id) when we - // ourselves scheduled a rebroadcast of it (see noteScheduled()); a PKI DM not addressed to - // us can never be decoded by us at all (Router::perhapsDecode's PKI branch requires - // isToUs(p)), so this cache will never have an entry for one. - portnum = lookupNotedPortnum(p->from, p->id); - } + const int32_t portnum = resolvePortnum(p); uint8_t threshold; if (portnum >= 0) { switch (portnum) { case meshtastic_PortNum_TEXT_MESSAGE_APP: case meshtastic_PortNum_TEXT_MESSAGE_COMPRESSED_APP: - // User-visible chat, broadcast or DM: no ACK/retry safety net for broadcasts, and no - // other portnum-specific reason to prefer airtime savings over delivery odds. Tolerate - // one repeat heard from another node before giving up on our own rebroadcast. + // User-visible chat: no broadcast ACK/retry safety net, so tolerate one heard repeat. threshold = 2; break; default: @@ -104,24 +76,13 @@ uint8_t RepeatScalingModule::getDupeCancelThreshold(const meshtastic_MeshPacket break; } } else { - // Portnum is genuinely unknowable here (most commonly a relayed PKI DM not addressed to - // us). next_hop is a plaintext MeshPacket header field - unlike portnum, it's visible - // regardless of whether we can decrypt the payload - so use it as a coarser proxy signal. - // NO_NEXT_HOP_PREFERENCE means this packet is being flood-relayed (every broadcast, or a DM - // with no directed route known yet): the same "uncertain single-path delivery" shape that - // justifies extra tolerance for a decoded text message. A specific next_hop byte means a - // directed route is already known; per perhapsRebroadcast's own gate we'd only reach this - // point as that assigned relay or in flooding mode, so a duplicate heard here is more - // likely a relay-byte collision than genuine flood redundancy, and delivery is already - // backed by the sender's end-to-end ACK/retry - no benefit to extra tolerance. + // Portnum unknowable (undecodable packet): fall back to the plaintext next_hop header. threshold = (p->next_hop == NO_NEXT_HOP_PREFERENCE) ? 2 : 1; LOG_DEBUG("[REPEATSCALE] portnum unknown for 0x%08x from=0x%08x; next_hop=0x%x -> threshold=%u", p->id, p->from, p->next_hop, threshold); } - // A busy/dense mesh overrides any extra tolerance decided above: not worth spending more - // airtime on repeats when the channel is already congested or there are many direct neighbors - // to reach anyway. + // A busy/dense mesh overrides any extra tolerance decided above. if (threshold > 1 && meshTooBusyForExtraRepeats()) { LOG_DEBUG("[REPEATSCALE] portnum=%d wanted threshold=%u but mesh is busy; falling back to 1", portnum, threshold); return 1; @@ -139,13 +100,13 @@ uint8_t RepeatScalingModule::registerDupeHeard(NodeNum sender, PacketId id) return entry.count; } } - // Not tracked yet: claim the next ring slot, evicting whatever packet (if any) was there. + // Not tracked yet: claim the next ring slot, evicting whatever was there. DupeCountEntry &slot = dupeCounts[dupeCountsNextSlot]; dupeCountsNextSlot = (dupeCountsNextSlot + 1) % DUPE_COUNT_TRACKER_SIZE; slot.sender = sender; slot.id = id; slot.count = 1; - slot.portnum = -1; // no noteScheduled() call preceded this - no portnum signal available + slot.portnum = -1; // no noteScheduled() preceded this return slot.count; } @@ -170,9 +131,8 @@ void RepeatScalingModule::noteScheduled(NodeNum sender, PacketId id, int32_t por return; } } - // Not tracked yet: claim the next ring slot, evicting whatever packet (if any) was there. - // count starts at 0 (as opposed to registerDupeHeard's 1) because scheduling our own - // rebroadcast is not itself a heard duplicate. + // Not tracked yet: claim the next ring slot. count starts at 0 (not 1) since scheduling our + // own rebroadcast is not itself a heard duplicate. DupeCountEntry &slot = dupeCounts[dupeCountsNextSlot]; dupeCountsNextSlot = (dupeCountsNextSlot + 1) % DUPE_COUNT_TRACKER_SIZE; slot.sender = sender; @@ -190,6 +150,12 @@ int32_t RepeatScalingModule::lookupNotedPortnum(NodeNum sender, PacketId id) con return -1; } +int32_t RepeatScalingModule::resolvePortnum(const meshtastic_MeshPacket *p) const +{ + return (p->which_payload_variant == meshtastic_MeshPacket_decoded_tag) ? p->decoded.portnum + : lookupNotedPortnum(p->from, p->id); +} + uint8_t RepeatScalingModule::getToleratedDupeCount(NodeNum sender, PacketId id) const { for (auto &entry : dupeCounts) { @@ -203,8 +169,7 @@ bool RepeatScalingModule::shouldCancelDupe(const meshtastic_MeshPacket *p) { const uint8_t threshold = getDupeCancelThreshold(p); const uint8_t dupesHeard = registerDupeHeard(p->from, p->id); - const int32_t portnum = - (p->which_payload_variant == meshtastic_MeshPacket_decoded_tag) ? p->decoded.portnum : lookupNotedPortnum(p->from, p->id); + const int32_t portnum = resolvePortnum(p); // for logging only if (dupesHeard >= threshold) { LOG_INFO("[REPEATSCALE] Giving up own rebroadcast of 0x%08x from=0x%08x portnum=%d: heard %u/%u duplicate(s)", p->id, diff --git a/src/modules/RepeatScalingModule.h b/src/modules/RepeatScalingModule.h index 95a8406a990..4dc6bd5154c 100644 --- a/src/modules/RepeatScalingModule.h +++ b/src/modules/RepeatScalingModule.h @@ -20,52 +20,32 @@ class RepeatScalingModule RepeatScalingModule() = default; virtual ~RepeatScalingModule() = default; - // Call each time we hear a duplicate rebroadcast of a packet we ourselves have queued to - // rebroadcast. Tracks the running count for (p->from, p->id) and logs the decision. Returns - // true once the configured per-portnum threshold has been reached, in which case the caller - // should cancel its own queued rebroadcast; tracking for the packet is cleared as soon as - // this returns true, so a reused ring slot can't leak into an unrelated future packet. - // Virtual solely so tests of FloodingRouter's role-gating can substitute a test double instead - // of driving the real threshold/ring-buffer logic (which is tested directly in - // test/test_repeat_scaling_module). + // Note a heard duplicate for (p->from, p->id); returns true (and clears tracking) once the + // per-portnum threshold is reached, meaning the caller should cancel its own rebroadcast. + // Virtual so FloodingRouter role-gating tests can substitute a double. virtual bool shouldCancelDupe(const meshtastic_MeshPacket *p); - // Call when we schedule our own rebroadcast of a packet (i.e. we successfully decoded it and - // queued it to send). Caches its portnum against (sender, id) so that later, when a duplicate - // of this same packet arrives from the air - still encrypted to us, since only the copy we - // process through Router::handleReceived ever gets decoded - getDupeCancelThreshold() has a - // per-portnum signal to act on instead of always falling back to the undecodable-packet - // default. portnum should be -1 if the packet we're scheduling could not be decoded either. + // Cache the portnum of a rebroadcast we've scheduled, so later encrypted duplicates of it can + // still be classified by getDupeCancelThreshold(). Pass -1 if it couldn't be decoded. void noteScheduled(NodeNum sender, PacketId id, int32_t portnum); - // Returns how many duplicates have been heard (and tolerated) so far for (sender, id), or 0 if - // none have (including if we never scheduled/heard anything for it at all). Meant for logging - // at the point of actual transmission (see RadioLibInterface::startSend) - if a packet reaches - // that point at all, any duplicates counted here were tolerated, not cancelled on. + // Duplicates heard (and tolerated) so far for (sender, id), or 0. For logging at TX time. uint8_t getToleratedDupeCount(NodeNum sender, PacketId id) const; protected: - // How many duplicate rebroadcasts of a packet we require to hear (see the per-portnum switch - // in RepeatScalingModule.cpp) before giving up on our own scheduled rebroadcast of it. Virtual - // solely so tests can override it to inject a threshold without needing a real portnum case - // (see test/test_repeat_scaling_module). + // Duplicates to tolerate before cancelling our own rebroadcast. Virtual so tests can inject a + // threshold without relying on a real portnum case. virtual uint8_t getDupeCancelThreshold(const meshtastic_MeshPacket *p); - // Tracks how many duplicates we've heard so far, per (sender, id), for packets we currently - // have one queued to rebroadcast ourselves. Bounded, ephemeral ring buffer - not a persistent - // record like PacketHistory: entries are only meaningful while our own rebroadcast is still - // pending, and naturally get evicted/reused as the ring wraps. + // Ephemeral ring buffer of per-(sender, id) heard-duplicate counts (not persistent state). uint8_t registerDupeHeard(NodeNum sender, PacketId id); - - // Clears tracking state for a (sender, id) once we've acted on it (cancelled our rebroadcast), - // so a reused ring slot can't cause a stale hit against an unrelated future packet. void clearDupeCount(NodeNum sender, PacketId id); - - // Returns the portnum cached by noteScheduled() for (sender, id), or -1 if we never scheduled - // a rebroadcast of it (or its ring slot has since been evicted/cleared). int32_t lookupNotedPortnum(NodeNum sender, PacketId id) const; private: + // Decoded portnum if available, else the one cached by noteScheduled() (or -1). + int32_t resolvePortnum(const meshtastic_MeshPacket *p) const; + static constexpr uint8_t DUPE_COUNT_TRACKER_SIZE = 8; struct DupeCountEntry { NodeNum sender = 0; diff --git a/test/test_repeat_scaling_module/test_main.cpp b/test/test_repeat_scaling_module/test_main.cpp index a886913c62d..c58d92072e6 100644 --- a/test/test_repeat_scaling_module/test_main.cpp +++ b/test/test_repeat_scaling_module/test_main.cpp @@ -1,13 +1,5 @@ -// Unit tests for RepeatScalingModule, which decides how many duplicate rebroadcasts of a packet -// we have queued to rebroadcast ourselves we should tolerate before giving up: -// - registerDupeHeard/clearDupeCount: the small ring buffer counting how many times we've -// heard a duplicate of a packet we have queued to rebroadcast ourselves. -// - getDupeCancelThreshold: the compile-time, per-portnum threshold (see -// RepeatScalingModule.cpp) controlling how many duplicates we tolerate before giving up. -// - meshTooBusyForExtraRepeats gate: channel/air utilization or direct-neighbor density -// overriding any portnum-specific extra tolerance. -// - shouldCancelDupe: the integration of all of the above - only actually gives up (and clears -// its tracking state) once the configured threshold has been reached. +// Unit tests for RepeatScalingModule: its ring-buffer dupe tracking, per-portnum threshold, +// busy-mesh gate, and the shouldCancelDupe integration of all three. #include "MeshTypes.h" // before TestUtil.h: provides NodeNum etc. #include "TestUtil.h" @@ -23,18 +15,14 @@ static constexpr NodeNum kSender2 = 0x000006CD; static constexpr PacketId kId1 = 0x1001; static constexpr PacketId kId2 = 0x2002; -// --------------------------------------------------------------------------- -// Test shim - re-exposes the protected dupe-tracking helpers, and allows overriding -// getDupeCancelThreshold so tests don't depend on any real portnum having a non-default case. -// --------------------------------------------------------------------------- +// Re-exposes the protected helpers and overrides getDupeCancelThreshold so threshold-gating can be +// tested without depending on a real portnum case. class RepeatScalingModuleTestShim : public RepeatScalingModule { public: using RepeatScalingModule::clearDupeCount; using RepeatScalingModule::registerDupeHeard; - // Test-only override: bypasses the compile-time portnum switch so tests can exercise - // shouldCancelDupe's threshold-gating without needing a real portnum case. uint8_t testThreshold = 1; uint8_t getDupeCancelThreshold(const meshtastic_MeshPacket *p) override { return testThreshold; } }; @@ -52,12 +40,8 @@ class RepeatScalingModulePlainShim : public RepeatScalingModule static RepeatScalingModuleTestShim *shim = nullptr; static RepeatScalingModulePlainShim *plainShim = nullptr; -// --------------------------------------------------------------------------- -// Scoped helpers to simulate a busy mesh for meshTooBusyForExtraRepeats() (see -// RepeatScalingModule.cpp). Install a global airTime/hopScalingModule reporting the given -// condition for the enclosing scope, restoring the previous pointer on destruction. Mirrors -// test_traffic_management's ScopedBusyAirTime. -// --------------------------------------------------------------------------- +// Scoped helpers that install a global airTime/hopScalingModule reporting a busy condition for the +// enclosing scope, restoring the previous pointer on destruction (mirrors ScopedBusyAirTime). class ScopedChannelUtil { public: @@ -239,16 +223,12 @@ void test_text_message_threshold_applies_regardless_of_addressing(void) } // =========================================================================== -// Group 2b - next_hop gate for packets whose portnum can't be determined at all (most commonly a -// relayed PKI DM not addressed to us, which we can never decode - see Router::perhapsDecode's PKI -// branch, gated on isToUs(p)). next_hop is a plaintext MeshPacket header field, so it's visible -// even here, unlike portnum. +// Group 2b - next_hop gate for undecodable packets (e.g. a relayed PKI DM not addressed to us). +// next_hop is a plaintext header field, so it's visible even when portnum isn't. // =========================================================================== -// A DM (addressed to a specific node, not NODENUM_BROADCAST) with no directed route known yet - -// the realistic shape of a relayed PKI DM we can never decrypt (see the module-level comment on -// Group 2b above). NO_NEXT_HOP_PREFERENCE is what a route-less DM actually carries in this state -// (see NextHopRouter::getNextHop/send), same as any broadcast. +// A route-less DM (specific node, NO_NEXT_HOP_PREFERENCE) - the shape of a relayed PKI DM we can't +// decrypt, carrying the same next_hop as any broadcast. static meshtastic_MeshPacket makeUndecodableFloodedDm(NodeNum from, PacketId id) { meshtastic_MeshPacket p = makeDupePacket(from, id); @@ -433,16 +413,9 @@ void test_shouldCancelDupe_waits_for_configured_repeats(void) } // =========================================================================== -// Group 5 - noteScheduled + encrypted duplicates (as heard for real over the air) -// -// In production, a duplicate rebroadcast heard from another node is still encrypted to us: only -// the one copy that made it through Router::handleReceived ever gets decoded (see -// NextHopRouter::perhapsRebroadcast, which calls noteScheduled() with that copy's portnum once it -// schedules our own rebroadcast). Group 2's tests construct an already-decoded packet directly, -// which proves the portnum switch itself is correct but - as the "portnum=-1" bug demonstrated - -// does not prove the real, always-encrypted duplicate gets the right threshold. These tests start -// from an encrypted packet, exactly as shouldCancelDupe receives it from FloodingRouter, and rely -// solely on noteScheduled() (the same call NextHopRouter makes) for portnum visibility. +// Group 5 - noteScheduled + encrypted duplicates (as heard for real over the air). +// Unlike Group 2 (which builds already-decoded packets), these start from an encrypted packet and +// rely solely on noteScheduled() for portnum visibility - the real production path. // =========================================================================== void test_encrypted_dupe_uses_portnum_noted_when_we_scheduled_our_own_rebroadcast(void) From 0aa53fd1369ec32833e0ce311972c351d48ff56b Mon Sep 17 00:00:00 2001 From: nomdetom Date: Thu, 9 Jul 2026 22:15:22 +0100 Subject: [PATCH 9/9] cppcheck --- src/modules/RepeatScalingModule.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/modules/RepeatScalingModule.cpp b/src/modules/RepeatScalingModule.cpp index e0bb3f9c584..d7d7f7f8dc3 100644 --- a/src/modules/RepeatScalingModule.cpp +++ b/src/modules/RepeatScalingModule.cpp @@ -143,7 +143,7 @@ void RepeatScalingModule::noteScheduled(NodeNum sender, PacketId id, int32_t por int32_t RepeatScalingModule::lookupNotedPortnum(NodeNum sender, PacketId id) const { - for (auto &entry : dupeCounts) { + for (const auto &entry : dupeCounts) { if (entry.id == id && entry.sender == sender) return entry.portnum; } @@ -158,7 +158,7 @@ int32_t RepeatScalingModule::resolvePortnum(const meshtastic_MeshPacket *p) cons uint8_t RepeatScalingModule::getToleratedDupeCount(NodeNum sender, PacketId id) const { - for (auto &entry : dupeCounts) { + for (const auto &entry : dupeCounts) { if (entry.id == id && entry.sender == sender) return entry.count; }