Calculate hops correctly even when hop_start==0

src/graphics/niche/InkHUD/Applets/User/Positions/PositionsApplet.cpp

@@ -1,6 +1,7 @@
 #ifdef MESHTASTIC_INCLUDE_INKHUD
 
 #include "./PositionsApplet.h"
+#include "NodeDB.h"
 
 using namespace NicheGraphics;
 
@@ -49,8 +50,8 @@ ProcessMessage InkHUD::PositionsApplet::handleReceived(const meshtastic_MeshPack
     if (!hasPosition)
         return ProcessMessage::CONTINUE;
 
-    bool hasHopsAway = (mp.hop_start != 0 && mp.hop_limit <= mp.hop_start); // From NodeDB::updateFrom
-    uint8_t hopsAway = mp.hop_start - mp.hop_limit;
+    const int8_t hopsAway = getHopsAway(mp);
+    const bool hasHopsAway = hopsAway >= 0;
 
     // Determine if the position packet would change anything on-screen
     // -----------------------------------------------------------------

src/mesh/MeshModule.cpp

@@ -195,7 +195,7 @@ void MeshModule::callModules(meshtastic_MeshPacket &mp, RxSource src)
             // but opted NOT TO.  Because it is not a good idea to let remote nodes 'probe' to find out which PSKs were "good" vs
             // bad.
             routingModule->sendAckNak(meshtastic_Routing_Error_NO_RESPONSE, getFrom(&mp), mp.id, mp.channel,
-                                      routingModule->getHopLimitForResponse(mp.hop_start, mp.hop_limit));
+                                      routingModule->getHopLimitForResponse(mp));
         }
     }
 
@@ -235,7 +235,7 @@ void setReplyTo(meshtastic_MeshPacket *p, const meshtastic_MeshPacket &to)
     assert(p->which_payload_variant == meshtastic_MeshPacket_decoded_tag); // Should already be set by now
     p->to = getFrom(&to);    // Make sure that if we are sending to the local node, we use our local node addr, not 0
     p->channel = to.channel; // Use the same channel that the request came in on
-    p->hop_limit = routingModule->getHopLimitForResponse(to.hop_start, to.hop_limit);
+    p->hop_limit = routingModule->getHopLimitForResponse(to);
 
     // No need for an ack if we are just delivering locally (it just generates an ignored ack)
     p->want_ack = (to.from != 0) ? to.want_ack : false;

src/mesh/MeshService.cpp

@@ -95,11 +95,8 @@ int MeshService::handleFromRadio(const meshtastic_MeshPacket *mp)
     } else if (mp->which_payload_variant == meshtastic_MeshPacket_decoded_tag && !nodeDB->getMeshNode(mp->from)->has_user &&
                nodeInfoModule && !isPreferredRebroadcaster && !nodeDB->isFull()) {
         if (airTime->isTxAllowedChannelUtil(true)) {
-            // Hops used by the request. If somebody in between running modified firmware modified it, ignore it
-            auto hopStart = mp->hop_start;
-            auto hopLimit = mp->hop_limit;
-            uint8_t hopsUsed = hopStart < hopLimit ? config.lora.hop_limit : hopStart - hopLimit;
-            if (hopsUsed > config.lora.hop_limit + 2) {
+            const int8_t hopsUsed = getHopsAway(*mp, config.lora.hop_limit);
+            if (hopsUsed > (int32_t)(config.lora.hop_limit + 2)) {
                 LOG_DEBUG("Skip send NodeInfo: %d hops away is too far away", hopsUsed);
             } else {
                 LOG_INFO("Heard new node on ch. %d, send NodeInfo and ask for response", mp->channel);

src/mesh/NextHopRouter.cpp

@@ -64,7 +64,7 @@ bool NextHopRouter::shouldFilterReceived(const meshtastic_MeshPacket *p)
                 perhapsRebroadcast(p);
             }
         } else {
-            bool isRepeated = p->hop_start > 0 && p->hop_start == p->hop_limit;
+            bool isRepeated = getHopsAway(*p) == 0;
             // If repeated and not in Tx queue anymore, try relaying again, or if we are the destination, send the ACK again
             if (isRepeated) {
                 if (!findInTxQueue(p->from, p->id)) {
@@ -101,8 +101,7 @@ void NextHopRouter::sniffReceived(const meshtastic_MeshPacket *p, const meshtast
                 bool wasAlreadyRelayer = wasRelayer(p->relay_node, p->decoded.request_id, p->to);
                 bool weWereSoleRelayer = false;
                 bool weWereRelayer = wasRelayer(ourRelayID, p->decoded.request_id, p->to, &weWereSoleRelayer);
-                if ((weWereRelayer && wasAlreadyRelayer) ||
-                    (p->hop_start != 0 && p->hop_start == p->hop_limit && weWereSoleRelayer)) {
+                if ((weWereRelayer && wasAlreadyRelayer) || (getHopsAway(*p) == 0 && weWereSoleRelayer)) {
                     if (origTx->next_hop != p->relay_node) { // Not already set
                         LOG_INFO("Update next hop of 0x%x to 0x%x based on ACK/reply (was relayer %d we were sole %d)", p->from,
                                  p->relay_node, wasAlreadyRelayer, weWereSoleRelayer);

src/mesh/NodeDB.cpp

@@ -1549,6 +1549,23 @@ uint32_t sinceReceived(const meshtastic_MeshPacket *p)
     return delta;
 }
 
+int8_t getHopsAway(const meshtastic_MeshPacket &p, int8_t defaultIfUnknown)
+{
+    // Firmware prior to 2.3.0 (585805c) lacked a hop_start field. Firmware version 2.5.0 (bf34329) introduced a
+    // bitfield that is always present. Use the presence of the bitfield to determine if the origin's firmware
+    // version is guaranteed to have hop_start populated. Note that this can only be done for decoded packets as
+    // the bitfield is encrypted under the channel encryption key. For encrypted packets, this returns
+    // defaultIfUnknown when hop_start is 0.
+    if (p.hop_start == 0 && !(p.which_payload_variant == meshtastic_MeshPacket_decoded_tag && p.decoded.has_bitfield))
+        return defaultIfUnknown; // Cannot reliably determine the number of hops.
+
+    // Guard against invalid values.
+    if (p.hop_start < p.hop_limit)
+        return defaultIfUnknown;
+
+    return p.hop_start - p.hop_limit;
+}
+
 #define NUM_ONLINE_SECS (60 * 60 * 2) // 2 hrs to consider someone offline
 
 size_t NodeDB::getNumOnlineMeshNodes(bool localOnly)
@@ -1801,9 +1818,10 @@ void NodeDB::updateFrom(const meshtastic_MeshPacket &mp)
         info->via_mqtt = mp.via_mqtt; // Store if we received this packet via MQTT
 
         // If hopStart was set and there wasn't someone messing with the limit in the middle, add hopsAway
-        if (mp.hop_start != 0 && mp.hop_limit <= mp.hop_start) {
+        const int8_t hopsAway = getHopsAway(mp);
+        if (hopsAway >= 0) {
             info->has_hops_away = true;
-            info->hops_away = mp.hop_start - mp.hop_limit;
+            info->hops_away = hopsAway;
         }
         sortMeshDB();
     }

src/mesh/NodeDB.h

@@ -110,6 +110,10 @@ uint32_t sinceLastSeen(const meshtastic_NodeInfoLite *n);
 /// Given a packet, return how many seconds in the past (vs now) it was received
 uint32_t sinceReceived(const meshtastic_MeshPacket *p);
 
+/// Given a packet, return the number of hops used to reach this node.
+/// Returns defaultIfUnknown if the number of hops couldn't be determined.
+int8_t getHopsAway(const meshtastic_MeshPacket &p, int8_t defaultIfUnknown = -1);
+
 enum LoadFileResult {
     // Successfully opened the file
     LOAD_SUCCESS = 1,

src/mesh/ReliableRouter.cpp

@@ -1,6 +1,7 @@
 #include "ReliableRouter.h"
 #include "Default.h"
 #include "MeshTypes.h"
+#include "NodeDB.h"
 #include "configuration.h"
 #include "memGet.h"
 #include "mesh-pb-constants.h"
@@ -108,12 +109,12 @@ void ReliableRouter::sniffReceived(const meshtastic_MeshPacket *p, const meshtas
                         // If this packet should always be ACKed reliably with want_ack back to the original sender, make sure we
                         // do that unconditionally.
                         sendAckNak(meshtastic_Routing_Error_NONE, getFrom(p), p->id, p->channel,
-                                   routingModule->getHopLimitForResponse(p->hop_start, p->hop_limit), true);
+                                   routingModule->getHopLimitForResponse(*p), true);
                     } else if (!p->decoded.request_id && !p->decoded.reply_id) {
                         // If it's not an ACK or a reply, send an ACK.
                         sendAckNak(meshtastic_Routing_Error_NONE, getFrom(p), p->id, p->channel,
-                                   routingModule->getHopLimitForResponse(p->hop_start, p->hop_limit));
-                    } else if ((p->hop_start > 0 && p->hop_start == p->hop_limit) || p->next_hop != NO_NEXT_HOP_PREFERENCE) {
+                                   routingModule->getHopLimitForResponse(*p));
+                    } else if ((getHopsAway(*p) == 0) || p->next_hop != NO_NEXT_HOP_PREFERENCE) {
                         // If we received the packet directly from the original sender, send a 0-hop ACK since the original sender
                         // won't overhear any implicit ACKs. If we received the packet via NextHopRouter, also send a 0-hop ACK to
                         // stop the immediate relayer's retransmissions.
@@ -123,11 +124,11 @@ void ReliableRouter::sniffReceived(const meshtastic_MeshPacket *p, const meshtas
                            (nodeDB->getMeshNode(p->from) == nullptr || nodeDB->getMeshNode(p->from)->user.public_key.size == 0)) {
                     LOG_INFO("PKI packet from unknown node, send PKI_UNKNOWN_PUBKEY");
                     sendAckNak(meshtastic_Routing_Error_PKI_UNKNOWN_PUBKEY, getFrom(p), p->id, channels.getPrimaryIndex(),
-                               routingModule->getHopLimitForResponse(p->hop_start, p->hop_limit));
+                               routingModule->getHopLimitForResponse(*p));
                 } else {
                     // Send a 'NO_CHANNEL' error on the primary channel if want_ack packet destined for us cannot be decoded
                     sendAckNak(meshtastic_Routing_Error_NO_CHANNEL, getFrom(p), p->id, channels.getPrimaryIndex(),
-                               routingModule->getHopLimitForResponse(p->hop_start, p->hop_limit));
+                               routingModule->getHopLimitForResponse(*p));
                 }
             } else if (p->next_hop == nodeDB->getLastByteOfNodeNum(getNodeNum()) && p->hop_limit > 0) {
                 // No wantAck, but we need to ACK with hop limit of 0 if we were the next hop to stop their retransmissions

src/mesh/Router.cpp

@@ -81,8 +81,7 @@ Router::Router() : concurrency::OSThread("Router"), fromRadioQueue(MAX_RX_FROMRA
 bool Router::shouldDecrementHopLimit(const meshtastic_MeshPacket *p)
 {
     // First hop MUST always decrement to prevent retry issues
-    bool isFirstHop = (p->hop_start != 0 && p->hop_start == p->hop_limit);
-    if (isFirstHop) {
+    if (getHopsAway(*p) == 0) {
         return true; // Always decrement on first hop
     }
 

src/modules/NeighborInfoModule.cpp

@@ -170,7 +170,7 @@ bool NeighborInfoModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp,
         } else {
             LOG_DEBUG("  Ignoring dummy neighbor info packet (single neighbor with nodeId 0, snr 0)");
         }
-    } else if (mp.hop_start != 0 && mp.hop_start == mp.hop_limit) {
+    } else if (getHopsAway(mp) == 0) {
         LOG_DEBUG("Get or create neighbor: %u with snr %f", mp.from, mp.rx_snr);
         // If the hopLimit is the same as hopStart, then it is a neighbor
         getOrCreateNeighbor(mp.from, mp.from, 0,

src/modules/RoutingModule.cpp

@@ -58,12 +58,11 @@ void RoutingModule::sendAckNak(meshtastic_Routing_Error err, NodeNum to, PacketI
     router->sendLocal(p); // we sometimes send directly to the local node
 }
 
-uint8_t RoutingModule::getHopLimitForResponse(uint8_t hopStart, uint8_t hopLimit)
+uint8_t RoutingModule::getHopLimitForResponse(const meshtastic_MeshPacket &mp)
 {
-    if (hopStart != 0) {
-        // Hops used by the request. If somebody in between running modified firmware modified it, ignore it
-        uint8_t hopsUsed = hopStart < hopLimit ? config.lora.hop_limit : hopStart - hopLimit;
-        if (hopsUsed > config.lora.hop_limit) {
+    const int8_t hopsUsed = getHopsAway(mp);
+    if (hopsUsed >= 0) {
+        if (hopsUsed > (int32_t)(config.lora.hop_limit)) {
 // In event mode, we never want to send packets with more than our default 3 hops.
 #if !(EVENTMODE)             // This falls through to the default.
             return hopsUsed; // If the request used more hops than the limit, use the same amount of hops

src/modules/RoutingModule.h

@@ -20,7 +20,7 @@ class RoutingModule : public ProtobufModule<meshtastic_Routing>
                                        uint8_t hopLimit = 0);
 
     // Given the hopStart and hopLimit upon reception of a request, return the hop limit to use for the response
-    uint8_t getHopLimitForResponse(uint8_t hopStart, uint8_t hopLimit);
+    uint8_t getHopLimitForResponse(const meshtastic_MeshPacket &mp);
 
   protected:
     friend class Router;

src/modules/TraceRouteModule.cpp

@@ -1,5 +1,6 @@
 #include "TraceRouteModule.h"
 #include "MeshService.h"
+#include "NodeDB.h"
 #include "graphics/Screen.h"
 #include "graphics/ScreenFonts.h"
 #include "graphics/SharedUIDisplay.h"
@@ -359,18 +360,18 @@ void TraceRouteModule::insertUnknownHops(meshtastic_MeshPacket &p, meshtastic_Ro
     }
 
     // Only insert unknown hops if hop_start is valid
-    if (p.hop_start != 0 && p.hop_limit <= p.hop_start) {
-        uint8_t hopsTaken = p.hop_start - p.hop_limit;
+    const int8_t hopsTaken = getHopsAway(p);
+    if (hopsTaken >= 0) {
         int8_t diff = hopsTaken - *route_count;
-        for (uint8_t i = 0; i < diff; i++) {
+        for (int8_t i = 0; i < diff; i++) {
             if (*route_count < ROUTE_SIZE) {
                 route[*route_count] = NODENUM_BROADCAST; // This will represent an unknown hop
                 *route_count += 1;
             }
         }
         // Add unknown SNR values if necessary
         diff = *route_count - *snr_count;
-        for (uint8_t i = 0; i < diff; i++) {
+        for (int8_t i = 0; i < diff; i++) {
             if (*snr_count < ROUTE_SIZE) {
                 snr_list[*snr_count] = INT8_MIN; // This will represent an unknown SNR
                 *snr_count += 1;

src/serialization/MeshPacketSerializer.cpp

@@ -418,8 +418,9 @@ std::string MeshPacketSerializer::JsonSerialize(const meshtastic_MeshPacket *mp,
         jsonObj["rssi"] = new JSONValue((int)mp->rx_rssi);
     if (mp->rx_snr != 0)
         jsonObj["snr"] = new JSONValue((float)mp->rx_snr);
-    if (mp->hop_start != 0 && mp->hop_limit <= mp->hop_start) {
-        jsonObj["hops_away"] = new JSONValue((unsigned int)(mp->hop_start - mp->hop_limit));
+    const int8_t hopsAway = getHopsAway(*mp);
+    if (hopsAway >= 0) {
+        jsonObj["hops_away"] = new JSONValue((unsigned int)(hopsAway));
         jsonObj["hop_start"] = new JSONValue((unsigned int)(mp->hop_start));
     }
 
@@ -450,8 +451,9 @@ std::string MeshPacketSerializer::JsonSerializeEncrypted(const meshtastic_MeshPa
         jsonObj["rssi"] = new JSONValue((int)mp->rx_rssi);
     if (mp->rx_snr != 0)
         jsonObj["snr"] = new JSONValue((float)mp->rx_snr);
-    if (mp->hop_start != 0 && mp->hop_limit <= mp->hop_start) {
-        jsonObj["hops_away"] = new JSONValue((unsigned int)(mp->hop_start - mp->hop_limit));
+    const int8_t hopsAway = getHopsAway(*mp);
+    if (hopsAway >= 0) {
+        jsonObj["hops_away"] = new JSONValue((unsigned int)(hopsAway));
         jsonObj["hop_start"] = new JSONValue((unsigned int)(mp->hop_start));
     }
     jsonObj["size"] = new JSONValue((unsigned int)mp->encrypted.size);

src/serialization/MeshPacketSerializer_nRF52.cpp

@@ -358,8 +358,9 @@ std::string MeshPacketSerializer::JsonSerialize(const meshtastic_MeshPacket *mp,
         jsonObj["rssi"] = (int)mp->rx_rssi;
     if (mp->rx_snr != 0)
         jsonObj["snr"] = (float)mp->rx_snr;
-    if (mp->hop_start != 0 && mp->hop_limit <= mp->hop_start) {
-        jsonObj["hops_away"] = (unsigned int)(mp->hop_start - mp->hop_limit);
+    const int8_t hopsAway = getHopsAway(*mp);
+    if (hopsAway >= 0) {
+        jsonObj["hops_away"] = (unsigned int)(hopsAway);
         jsonObj["hop_start"] = (unsigned int)(mp->hop_start);
     }
 
@@ -393,8 +394,9 @@ std::string MeshPacketSerializer::JsonSerializeEncrypted(const meshtastic_MeshPa
         jsonObj["rssi"] = (int)mp->rx_rssi;
     if (mp->rx_snr != 0)
         jsonObj["snr"] = (float)mp->rx_snr;
-    if (mp->hop_start != 0 && mp->hop_limit <= mp->hop_start) {
-        jsonObj["hops_away"] = (unsigned int)(mp->hop_start - mp->hop_limit);
+    const int8_t hopsAway = getHopsAway(*mp);
+    if (hopsAway >= 0) {
+        jsonObj["hops_away"] = (unsigned int)(hopsAway);
         jsonObj["hop_start"] = (unsigned int)(mp->hop_start);
     }
     jsonObj["size"] = (unsigned int)mp->encrypted.size;