fix(position): stamp the broadcast cadence only when a position packet was actually sent (#11751)

* fix(position): stamp the broadcast cadence only when a position packet was actually sent

* fix(position): honor the router verdict and always fall back to nodeinfo when no position goes out

* fix(position): snapshot Time::getMillis() for the throttle stamps and trim the added comments

* fix(position): consume the radio-generation change only on a position send that went out

* refactor(position): share one smart-broadcast path and drop the duplicated fresh-position guard

* fix(position): persist smart broadcasts to the transmit history

src/modules/PositionModule.cpp

@@ -9,6 +9,7 @@
 #include "Router.h"
 #include "TransmitHistory.h"
 #include "TypeConversions.h"
+#include "UptimeClock.h"
 #include "airtime.h"
 #include "configuration.h"
 #include "gps/GPSLog.h"
@@ -291,7 +292,7 @@ meshtastic_MeshPacket *PositionModule::allocReply()
 
     meshtastic_MeshPacket *reply = allocPositionPacket(precision);
     if (reply) {
-        lastSentReply = millis(); // Track when we sent this reply
+        lastSentReply = Time::getMillis(); // Track when we sent this reply
     }
     return reply;
 }
@@ -396,19 +397,21 @@ meshtastic_MeshPacket *PositionModule::allocAtakPli()
     return mp;
 }
 
-void PositionModule::sendOurPosition()
+bool PositionModule::sendOurPosition()
 {
     bool requestReplies = currentGeneration != radioGeneration;
-    currentGeneration = radioGeneration;
 
     // If we changed channels, ask everyone else for their latest info
     uint8_t positionChannel;
     if (findPositionChannel(positionChannel)) {
         LOG_INFO("Send pos@%x:6 to mesh (wantReplies=%d)", localPosition.timestamp, requestReplies);
-        sendOurPosition(NODENUM_BROADCAST, requestReplies, positionChannel);
-        return;
+        if (!sendOurPosition(NODENUM_BROADCAST, requestReplies, positionChannel))
+            return false;
+        currentGeneration = radioGeneration; // only a send that went out consumes the channel change
+        return true;
     }
     LOG_INFO("Skip pos@%x:6 broadcast; position sharing disabled on all channels", localPosition.timestamp);
+    return false;
 }
 
 // Position broadcasts are opt-in per channel in 2.8, but our own position still plays to the
@@ -431,11 +434,11 @@ bool PositionModule::sendOurPositionToPhone()
     return true;
 }
 
-void PositionModule::sendOurPosition(NodeNum dest, bool wantReplies, uint8_t channel)
+bool PositionModule::sendOurPosition(NodeNum dest, bool wantReplies, uint8_t channel)
 {
     if (!config.position.fixed_position && !nodeDB->hasLocalPositionSinceBoot()) {
         LOG_DEBUG("Skip position send; no fresh position since boot");
-        return;
+        return false;
     }
 
     // cancel any not yet sent (now stale) position packets
@@ -448,7 +451,7 @@ void PositionModule::sendOurPosition(NodeNum dest, bool wantReplies, uint8_t cha
     meshtastic_MeshPacket *p = allocPositionPacket(precision);
     if (p == nullptr) {
         LOG_DEBUG("allocPositionPacket returned a nullptr");
-        return;
+        return false;
     }
 
     p->to = dest;
@@ -463,7 +466,12 @@ void PositionModule::sendOurPosition(NodeNum dest, bool wantReplies, uint8_t cha
     if (channel > 0)
         p->channel = channel;
 
-    service->sendToMesh(p, RX_SRC_LOCAL, true);
+    // Rejected (full TX queue, duty cycle abort) means released unsent: never stamp the cadence.
+    ErrorCode res = service->sendToMesh(p, RX_SRC_LOCAL, true);
+    if (res != ERRNO_OK && res != ERRNO_SHOULD_RELEASE) {
+        LOG_WARN("Position send rejected by router: 0x%x", res);
+        return false;
+    }
 
     if (IS_ONE_OF(config.device.role, meshtastic_Config_DeviceConfig_Role_TRACKER,
                   meshtastic_Config_DeviceConfig_Role_TAK_TRACKER) &&
@@ -481,6 +489,8 @@ void PositionModule::sendOurPosition(NodeNum dest, bool wantReplies, uint8_t cha
         LOG_DEBUG("Start next execution in 5s, then sleep");
         setIntervalFromNow(FIVE_SECONDS_MS);
     }
+
+    return true;
 }
 
 #define RUNONCE_INTERVAL 5000;
@@ -540,7 +550,7 @@ int32_t PositionModule::runOnce()
     if (node == nullptr)
         return RUNONCE_INTERVAL;
 
-    uint32_t now = millis();
+    uint32_t now = Time::getMillis();
 
     // Local-only delivery, so it runs regardless of mesh opt-in state or channel utilization.
     // Only send while the queue is empty (phone assumed connected), like telemetry. The cadence
@@ -563,8 +573,6 @@ int32_t PositionModule::runOnce()
         return RUNONCE_INTERVAL;
     }
 
-    bool waitingForFreshPosition = (lastGpsSend == 0) && !config.position.fixed_position && !nodeDB->hasLocalPositionSinceBoot();
-
     // Hold to the 6h floor when fixed_position (every role: pinning yourself forfeits the
     // exception) or when stationary. A real move still goes out early via smart-broadcast below.
     // Not-fixed exceptions: lost-and-found broadcasts freely; trackers judge movement at their
@@ -582,9 +590,7 @@ int32_t PositionModule::runOnce()
         effectiveBroadcastIntervalMs(intervalMs, stationary, (uint32_t)default_position_stationary_broadcast_secs * 1000UL);
 
     if (lastGpsSend == 0 || msSinceLastSend >= effectiveIntervalMs) {
-        if (waitingForFreshPosition) {
-            LOG_DEBUG_GPS("Skip initial position send; no fresh position since boot");
-        } else if (nodeDB->hasValidPosition(node)) {
+        if (nodeDB->hasValidPosition(node) && sendOurPosition()) {
             lastGpsSend = now;
 
             meshtastic_PositionLite selfPos;
@@ -595,7 +601,6 @@ int32_t PositionModule::runOnce()
 
             if (transmitHistory)
                 transmitHistory->setLastSentToMesh(meshtastic_PortNum_POSITION_APP);
-            sendOurPosition();
             if (config.device.role == meshtastic_Config_DeviceConfig_Role_LOST_AND_FOUND) {
                 sendLostAndFoundText();
             }
@@ -604,28 +609,10 @@ int32_t PositionModule::runOnce()
         const meshtastic_NodeInfoLite *node2 = service->refreshLocalMeshNode(); // should guarantee there is now a position
 
         if (nodeDB->hasValidPosition(node2)) {
-            // The minimum time (in seconds) that would pass before we are able to send a new position packet.
-
             meshtastic_PositionLite selfPos;
             if (!nodeDB->copyNodePosition(node->num, selfPos))
                 return RUNONCE_INTERVAL; // Defensive: hasValidPosition should imply this is non-null
-            auto smartPosition = getDistanceTraveledSinceLastSend(selfPos);
-            msSinceLastSend = now - lastGpsSend;
-
-            if (smartPosition.hasTraveledOverThreshold &&
-                Throttle::execute(
-                    &lastGpsSend, minimumTimeThreshold, []() { positionModule->sendOurPosition(); },
-                    []() { LOG_DEBUG_GPS("Skip smart broadcast: time throttled"); })) {
-
-                LOG_DEBUG("Sent smart pos@%x:6 to mesh (distanceTraveled=%fm, minDistanceThreshold=%im, timeElapsed=%ims, "
-                          "minTimeInterval=%ims)",
-                          localPosition.timestamp, smartPosition.distanceTraveled, smartPosition.distanceThreshold,
-                          msSinceLastSend, minimumTimeThreshold);
-
-                // Set the current coords as our last ones, after we've compared distance with current and decided to send
-                lastGpsLatitude = selfPos.latitude_i;
-                lastGpsLongitude = selfPos.longitude_i;
-            }
+            trySmartBroadcast(selfPos, now);
         }
     }
 
@@ -707,6 +694,32 @@ struct SmartPosition PositionModule::getDistanceTraveledSinceLastSend(meshtastic
                          .hasTraveledOverThreshold = distanceTraveled >= distanceTravelThreshold};
 }
 
+void PositionModule::trySmartBroadcast(const meshtastic_PositionLite &selfPos, uint32_t nowMs)
+{
+    auto smartPosition = getDistanceTraveledSinceLastSend(selfPos);
+    if (!smartPosition.hasTraveledOverThreshold)
+        return;
+
+    if (!Throttle::hasElapsed(lastGpsSend, minimumTimeThreshold)) {
+        LOG_DEBUG_GPS("Skip smart broadcast: time throttled");
+        return;
+    }
+
+    uint32_t msSinceLastSend = nowMs - lastGpsSend;
+    if (!sendOurPosition())
+        return;
+
+    lastGpsSend = nowMs;
+    if (transmitHistory)
+        transmitHistory->setLastSentToMesh(meshtastic_PortNum_POSITION_APP);
+    LOG_DEBUG("Sent smart pos@%x:6 to mesh (distanceTraveled=%fm, minDistanceThreshold=%im, timeElapsed=%ims, "
+              "minTimeInterval=%ims)",
+              localPosition.timestamp, smartPosition.distanceTraveled, smartPosition.distanceThreshold, msSinceLastSend,
+              minimumTimeThreshold);
+    lastGpsLatitude = selfPos.latitude_i;
+    lastGpsLongitude = selfPos.longitude_i;
+}
+
 void PositionModule::handleNewPosition()
 {
     const meshtastic_NodeInfoLite *node = nodeDB->getMeshNode(nodeDB->getNodeNum());
@@ -716,21 +729,7 @@ void PositionModule::handleNewPosition()
         meshtastic_PositionLite selfPos;
         if (!nodeDB->copyNodePosition(node->num, selfPos))
             return;
-        auto smartPosition = getDistanceTraveledSinceLastSend(selfPos);
-        uint32_t msSinceLastSend = millis() - lastGpsSend;
-        if (smartPosition.hasTraveledOverThreshold &&
-            Throttle::execute(
-                &lastGpsSend, minimumTimeThreshold, []() { positionModule->sendOurPosition(); },
-                []() { LOG_DEBUG_GPS("Skip smart broadcast: time throttled"); })) {
-            LOG_DEBUG("Sent smart pos@%x:6 to mesh (distanceTraveled=%fm, minDistanceThreshold=%im, timeElapsed=%ims, "
-                      "minTimeInterval=%ims)",
-                      localPosition.timestamp, smartPosition.distanceTraveled, smartPosition.distanceThreshold, msSinceLastSend,
-                      minimumTimeThreshold);
-
-            // Set the current coords as our last ones, after we've compared distance with current and decided to send
-            lastGpsLatitude = selfPos.latitude_i;
-            lastGpsLongitude = selfPos.longitude_i;
-        }
+        trySmartBroadcast(selfPos, Time::getMillis());
     }
 }