TMM: address review, fix dedup tests, inject fake time, fix fingerprint-0

Copilot review (PR #10706):
- preloadNextHopsFromNodeDB() now returns bool; runOnce only latches
  nextHopPreloaded once the preload actually ran (retries if nodeDB wasn't
  ready), instead of skipping it forever.
- Remove the empty `#if HAS_VARIABLE_HOPS` blocks in the test.

Test correctness:
- Three more position-dedup tests were missing installWellKnownPrimaryChannel()
  (dropsDuplicate/allowsMoved were fixed earlier; allowsDuplicateAfterInterval,
  cacheFlush, priorRateState were not) — without the well-known-channel gate the
  dedup path never runs, so their STOP assertions failed.

Fake-time injection (no more real sleeps):
- Add TrafficManagementModule::s_testNowMs + nowMs(), mirroring HopScalingModule;
  route all TMM tick/time reads through nowMs(). Tests advance a virtual clock via
  s_testNowMs instead of testDelay() sleeping real 5-6 min across a tick — the
  suite drops from ~15 min to ~30 s. Production behaviour is unchanged (nowMs()
  inlines to millis()).

Fingerprint-0 fix:
- computePositionFingerprint() never returns 0 now (remap 0 -> 0xFF, mirroring
  getLastByteOfNodeNum), so a real position that hashes to 0 doesn't collide with
  the "no position seen" sentinel and its duplicates dedup correctly.

test_traffic_management: 34/34 green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: NomDeTom

src/modules/TrafficManagementModule.cpp

@@ -390,7 +390,7 @@ TrafficManagementModule::NodeInfoPayloadEntry *TrafficManagementModule::findOrCr
     NodeInfoPayloadEntry *empty = nullptr;
     NodeInfoPayloadEntry *lru = nullptr;
     uint32_t lruAge = 0;
-    const uint32_t now = millis();
+    const uint32_t now = clockMs();
 
     for (uint16_t i = 0; i < nodeInfoTargetEntries(); i++) {
         NodeInfoPayloadEntry &e = nodeInfoPayload[i];
@@ -466,7 +466,7 @@ void TrafficManagementModule::cacheNodeInfoPacket(const meshtastic_MeshPacket &m
         // richer context than "just the user protobuf" when PSRAM is present.
         // This path is intentionally independent from NodeInfoModule/NodeDB.
         entry->user = user;
-        entry->lastObservedMs = millis();
+        entry->lastObservedMs = clockMs();
         entry->lastObservedRxTime = mp.rx_time;
         entry->sourceChannel = mp.channel;
         entry->hasDecodedBitfield = mp.decoded.has_bitfield;
@@ -528,11 +528,11 @@ uint8_t TrafficManagementModule::getNextHopHint(NodeNum dest)
 #endif
 }
 
-void TrafficManagementModule::preloadNextHopsFromNodeDB()
+bool TrafficManagementModule::preloadNextHopsFromNodeDB()
 {
 #if TRAFFIC_MANAGEMENT_CACHE_SIZE > 0
     if (!cache || !nodeDB)
-        return;
+        return false; // prerequisites not ready yet — caller should retry on a later pass
 
     uint16_t seeded = 0;
     concurrency::LockGuard guard(&cacheLock);
@@ -552,6 +552,9 @@ void TrafficManagementModule::preloadNextHopsFromNodeDB()
     }
 
     TM_LOG_INFO("Preloaded %u next-hop hints from NodeDB", static_cast<unsigned>(seeded));
+    return true;
+#else
+    return true; // nothing to preload on a cache-less build; don't keep retrying
 #endif
 }
 
@@ -607,7 +610,12 @@ uint8_t TrafficManagementModule::computePositionFingerprint(int32_t lat_truncate
     uint8_t latBits = (static_cast<uint32_t>(lat_truncated) >> shift) & ((1u << bitsToTake) - 1);
     uint8_t lonBits = (static_cast<uint32_t>(lon_truncated) >> shift) & ((1u << bitsToTake) - 1);
 
-    return static_cast<uint8_t>((latBits << 4) | lonBits);
+    const uint8_t fp = static_cast<uint8_t>((latBits << 4) | lonBits);
+    // 0 is the "no position seen" sentinel for pos_fingerprint, so a real position that happens to
+    // hash to 0 must not collide with it (otherwise its duplicates would never dedup). Remap 0 -> 0xFF,
+    // mirroring NodeDB::getLastByteOfNodeNum()'s 0 -> 0xFF idiom. Cost: the 0x00 bucket merges into
+    // 0xFF (one extra collision in 256 — negligible; the fingerprint already collides every 16 cells).
+    return fp ? fp : 0xFF;
 }
 
 // =============================================================================
@@ -632,7 +640,7 @@ ProcessMessage TrafficManagementModule::handleReceived(const meshtastic_MeshPack
     incrementStat(&stats.packets_inspected);
 
     const auto &cfg = moduleConfig.traffic_management;
-    const uint32_t nowMs = millis();
+    const uint32_t nowMs = TrafficManagementModule::clockMs();
 
     // -------------------------------------------------------------------------
     // Undecoded Packet Handling
@@ -811,15 +819,15 @@ int32_t TrafficManagementModule::runOnce()
         return INT32_MAX;
 
 #if TRAFFIC_MANAGEMENT_CACHE_SIZE > 0
-    const uint32_t nowMs = millis();
+    const uint32_t nowMs = TrafficManagementModule::clockMs();
 
     // Warm-start the next-hop cache from persisted NodeInfoLite hints once nodeDB
     // is populated. Done here (not in the constructor) so nodeDB has finished
     // loading. Takes its own lock, so call before acquiring the sweep guard below.
-    if (!nextHopPreloaded) {
-        preloadNextHopsFromNodeDB();
+    // Only latch the one-shot guard once the preload actually ran; if nodeDB wasn't
+    // ready yet, retry on the next maintenance pass instead of skipping it forever.
+    if (!nextHopPreloaded && preloadNextHopsFromNodeDB())
         nextHopPreloaded = true;
-    }
 
     // Free-running tick counters (no epoch needed).
     // TTL expressed in ticks: pos 4× interval (default 44 ticks ≈ 4.4h),
@@ -843,7 +851,7 @@ int32_t TrafficManagementModule::runOnce()
     // Sweep cache and clear expired entries
     uint16_t activeEntries = 0;
     uint16_t expiredEntries = 0;
-    const uint32_t sweepStartMs = millis();
+    const uint32_t sweepStartMs = TrafficManagementModule::clockMs();
 
     const auto &cfg = moduleConfig.traffic_management;
     concurrency::LockGuard guard(&cacheLock);
@@ -900,7 +908,7 @@ int32_t TrafficManagementModule::runOnce()
 
     TM_LOG_DEBUG("Maintenance: %u active, %u expired, %u/%u slots, %lums elapsed", activeEntries, expiredEntries,
                  static_cast<unsigned>(activeEntries), static_cast<unsigned>(cacheSize()),
-                 static_cast<unsigned long>(millis() - sweepStartMs));
+                 static_cast<unsigned long>(TrafficManagementModule::clockMs() - sweepStartMs));
 
 #if defined(ARCH_ESP32) && defined(BOARD_HAS_PSRAM)
     if (nodeInfoPayload) {
@@ -1056,7 +1064,7 @@ bool TrafficManagementModule::shouldRespondToNodeInfo(const meshtastic_MeshPacke
         reply->decoded.bitfield |= BITFIELD_OK_TO_MQTT_MASK;
 
     if (hasCachedUser && cachedLastObservedMs != 0) {
-        uint32_t ageMs = millis() - cachedLastObservedMs;
+        uint32_t ageMs = clockMs() - cachedLastObservedMs;
         TM_LOG_DEBUG("NodeInfo PSRAM hit node=0x%08x age=%lu ms src_ch=%u req_ch=%u rx_time=%lu", p->to,
                      static_cast<unsigned long>(ageMs), static_cast<unsigned>(cachedSourceChannel),
                      static_cast<unsigned>(p->channel), static_cast<unsigned long>(cachedLastObservedRxTime));

src/modules/TrafficManagementModule.h

@@ -52,7 +52,9 @@ class TrafficManagementModule : public MeshModule, private concurrency::OSThread
     // Warm-start the next-hop cache from persisted NodeInfoLite hints so confirmed
     // hops survive later hot-store (NodeDB) eviction. Idempotent; runs once after
     // nodeDB is populated (lazily on first maintenance pass).
-    void preloadNextHopsFromNodeDB();
+    // @return true if it actually ran (prereqs met / nothing to do); false if
+    //   prerequisites (cache, nodeDB) weren't ready yet, so the caller should retry.
+    bool preloadNextHopsFromNodeDB();
 
     /**
      * Check if this packet should have its hops exhausted.
@@ -64,6 +66,17 @@ class TrafficManagementModule : public MeshModule, private concurrency::OSThread
         return exhaustRequested && exhaustRequestedFrom == getFrom(&mp) && exhaustRequestedId == mp.id;
     }
 
+    // Injectable monotonic clock (ms). All TMM time reads go through clockMs() so unit tests can
+    // advance a virtual timebase instead of sleeping real seconds across the 6 min/360 s tick.
+    // Mirrors HopScalingModule::s_testNowMs. Writable from tests as TrafficManagementModule::s_testNowMs;
+    // ignored in production (clockMs() returns millis()).
+    inline static uint32_t s_testNowMs = 0;
+#ifdef PIO_UNIT_TESTING
+    static uint32_t clockMs() { return s_testNowMs; }
+#else
+    static uint32_t clockMs() { return millis(); }
+#endif
+
   protected:
     ProcessMessage handleReceived(const meshtastic_MeshPacket &mp) override;
     bool wantPacket(const meshtastic_MeshPacket *p) override { return true; }
@@ -149,9 +162,9 @@ class TrafficManagementModule : public MeshModule, private concurrency::OSThread
     static constexpr uint32_t kRateTimeTickMs = 300'000UL;   // 5 min/tick
     static constexpr uint32_t kUnknownTimeTickMs = 60'000UL; // 1 min/tick
 
-    static uint8_t currentPosTick() { return static_cast<uint8_t>(millis() / kPosTimeTickMs); }
-    static uint8_t currentRateTick() { return static_cast<uint8_t>((millis() / kRateTimeTickMs) & 0x0F); }
-    static uint8_t currentUnknownTick() { return static_cast<uint8_t>((millis() / kUnknownTimeTickMs) & 0x0F); }
+    static uint8_t currentPosTick() { return static_cast<uint8_t>(clockMs() / kPosTimeTickMs); }
+    static uint8_t currentRateTick() { return static_cast<uint8_t>((clockMs() / kRateTimeTickMs) & 0x0F); }
+    static uint8_t currentUnknownTick() { return static_cast<uint8_t>((clockMs() / kUnknownTimeTickMs) & 0x0F); }
     // =========================================================================
     // Position Fingerprint
     // =========================================================================

test/test_traffic_management/test_main.cpp

@@ -12,8 +12,6 @@
 #if HAS_TRAFFIC_MANAGEMENT
 
 #include "airtime.h"
-#if HAS_VARIABLE_HOPS
-#endif
 #include "mesh/CryptoEngine.h"
 #include "mesh/MeshService.h"
 #include "mesh/NodeDB.h"
@@ -181,6 +179,10 @@ static void resetTrafficConfig()
     mockNodeDB->resetNodes();
     mockNodeDB->clearCachedNode();
     nodeDB = mockNodeDB;
+
+    // Virtual clock base (1 h in, so tick subtraction never underflows). Tests advance time by
+    // bumping TrafficManagementModule::s_testNowMs instead of sleeping real seconds across a tick.
+    TrafficManagementModule::s_testNowMs = 3600000;
 }
 
 static meshtastic_MeshPacket makeDecodedPacket(meshtastic_PortNum port, NodeNum from, NodeNum to = NODENUM_BROADCAST)
@@ -738,8 +740,6 @@ static void test_tm_alterReceived_exhaustsRelayedTelemetryBroadcast(void)
     TEST_ASSERT_EQUAL_UINT32(1, stats.hop_exhausted_packets);
 }
 
-#if HAS_VARIABLE_HOPS
-#endif // HAS_VARIABLE_HOPS
 /**
  * Verify hop exhaustion skips unicast and local-origin packets.
  * Important to avoid mutating traffic that should retain normal forwarding behavior.
@@ -776,8 +776,9 @@ static void test_tm_positionDedup_allowsDuplicateAfterIntervalExpires(void)
 {
     moduleConfig.traffic_management.position_dedup_enabled = true;
     moduleConfig.traffic_management.position_precision_bits = 16;
-    // 360 s = 1 pos-tick (kPosTimeTickMs); testDelay advances past one tick period.
+    // 360 s = 1 pos-tick (kPosTimeTickMs); advance the virtual clock past one tick period.
     moduleConfig.traffic_management.position_min_interval_secs = 360;
+    installWellKnownPrimaryChannel(); // dedup only runs on a well-known channel (gate in handleReceived)
     TrafficManagementModuleTestShim module;
 
     meshtastic_MeshPacket first = makePositionPacket(kRemoteNode, 374221234, -1220845678);
@@ -786,7 +787,7 @@ static void test_tm_positionDedup_allowsDuplicateAfterIntervalExpires(void)
 
     ProcessMessage r1 = module.handleReceived(first);
     ProcessMessage r2 = module.handleReceived(second);
-    testDelay(360001); // advance past one 6-min pos-tick
+    TrafficManagementModule::s_testNowMs += 360001; // advance past one 6-min pos-tick (virtual clock)
     ProcessMessage r3 = module.handleReceived(third);
     meshtastic_TrafficManagementStats stats = module.getStats();
 
@@ -900,6 +901,7 @@ static void test_tm_positionDedup_cacheFlush_doesNotDropFirstPacketAfterFlush(vo
     moduleConfig.traffic_management.position_dedup_enabled = true;
     moduleConfig.traffic_management.position_precision_bits = 16;
     moduleConfig.traffic_management.position_min_interval_secs = 300;
+    installWellKnownPrimaryChannel(); // dedup only runs on a well-known channel (gate in handleReceived)
     TrafficManagementModuleTestShim module;
 
     meshtastic_MeshPacket first = makePositionPacket(kRemoteNode, 374221234, -1220845678);
@@ -930,6 +932,7 @@ static void test_tm_positionDedup_priorRateState_doesNotDropFirstFingerprintZero
     moduleConfig.traffic_management.rate_limit_enabled = true;
     moduleConfig.traffic_management.rate_limit_window_secs = 60;
     moduleConfig.traffic_management.rate_limit_max_packets = 10;
+    installWellKnownPrimaryChannel(); // dedup only runs on a well-known channel (gate in handleReceived)
     TrafficManagementModuleTestShim module;
 
     meshtastic_MeshPacket telemetry = makeDecodedPacket(meshtastic_PortNum_TELEMETRY_APP, kRemoteNode);
@@ -954,15 +957,15 @@ static void test_tm_positionDedup_priorRateState_doesNotDropFirstFingerprintZero
 static void test_tm_rateLimit_resetsAfterWindowExpires(void)
 {
     moduleConfig.traffic_management.rate_limit_enabled = true;
-    // 300 s = 1 rate-tick (kRateTimeTickMs); testDelay advances past one tick period.
+    // 300 s = 1 rate-tick (kRateTimeTickMs); advance the virtual clock past one tick period.
     moduleConfig.traffic_management.rate_limit_window_secs = 300;
     moduleConfig.traffic_management.rate_limit_max_packets = 1;
     TrafficManagementModuleTestShim module;
     meshtastic_MeshPacket packet = makeDecodedPacket(meshtastic_PortNum_TELEMETRY_APP, kRemoteNode);
 
     ProcessMessage r1 = module.handleReceived(packet);
     ProcessMessage r2 = module.handleReceived(packet);
-    testDelay(300001); // advance past one 5-min rate-tick
+    TrafficManagementModule::s_testNowMs += 300001; // advance past one 5-min rate-tick (virtual clock)
     ProcessMessage r3 = module.handleReceived(packet);
     meshtastic_TrafficManagementStats stats = module.getStats();
 
@@ -984,7 +987,7 @@ static void test_tm_unknownPackets_resetAfterWindowExpires(void)
 
     ProcessMessage r1 = module.handleReceived(packet);
     ProcessMessage r2 = module.handleReceived(packet);
-    testDelay(300001); // advance past 5 unknown-ticks (kUnknownWindowTicks=5 × 60s)
+    TrafficManagementModule::s_testNowMs += 300001; // advance past 5 unknown-ticks (5 × 60s) (virtual clock)
     ProcessMessage r3 = module.handleReceived(packet);
     meshtastic_TrafficManagementStats stats = module.getStats();
 
@@ -1225,8 +1228,6 @@ static void test_tm_nextHop_keptAliveAcrossMaintenanceSweep(void)
 
     TEST_ASSERT_EQUAL_UINT8(0x42, module.getNextHopHint(kTargetNode));
 }
-#if HAS_VARIABLE_HOPS
-#endif // HAS_VARIABLE_HOPS
 } // namespace
 
 void setUp(void)
@@ -1264,8 +1265,6 @@ TM_TEST_ENTRY void setup()
     RUN_TEST(test_tm_nodeinfo_directResponse_psramMissDoesNotFallbackToNodeDb);
 #endif
     RUN_TEST(test_tm_alterReceived_exhaustsRelayedTelemetryBroadcast);
-#if HAS_VARIABLE_HOPS
-#endif
     RUN_TEST(test_tm_alterReceived_skipsLocalAndUnicast);
     RUN_TEST(test_tm_positionDedup_allowsDuplicateAfterIntervalExpires);
     RUN_TEST(test_tm_positionDedup_intervalZero_neverDrops);
@@ -1287,8 +1286,6 @@ TM_TEST_ENTRY void setup()
     RUN_TEST(test_tm_nextHop_servedAfterNodeDbRoll);
     RUN_TEST(test_tm_nextHop_preloadDoesNotClobberLearned);
     RUN_TEST(test_tm_nextHop_keptAliveAcrossMaintenanceSweep);
-#if HAS_VARIABLE_HOPS
-#endif
     exit(UNITY_END());
 }