From 9058ccecf9c5257513ed91add49b5f96738bb083 Mon Sep 17 00:00:00 2001 From: Eric Severance Date: Tue, 30 Dec 2025 13:31:35 -0800 Subject: [PATCH] Calculate hops correctly even when hop_start==0 (#9120) * Calculate hops correctly even when hop_start==0. * Use the same type (int8_t) in the loop, avoiding signed/unsigned mismatches. * Clarify defaultIfUnknown is returned for encrypted packets. --- .../User/Positions/PositionsApplet.cpp | 5 +++-- src/mesh/MeshModule.cpp | 4 ++-- src/mesh/MeshService.cpp | 7 ++---- src/mesh/NextHopRouter.cpp | 5 ++--- src/mesh/NodeDB.cpp | 22 +++++++++++++++++-- src/mesh/NodeDB.h | 4 ++++ src/mesh/ReliableRouter.cpp | 11 +++++----- src/mesh/Router.cpp | 3 +-- src/modules/NeighborInfoModule.cpp | 2 +- src/modules/RoutingModule.cpp | 9 ++++---- src/modules/RoutingModule.h | 2 +- src/modules/TraceRouteModule.cpp | 9 ++++---- src/serialization/MeshPacketSerializer.cpp | 10 +++++---- .../MeshPacketSerializer_nRF52.cpp | 10 +++++---- 14 files changed, 63 insertions(+), 40 deletions(-) diff --git a/src/graphics/niche/InkHUD/Applets/User/Positions/PositionsApplet.cpp b/src/graphics/niche/InkHUD/Applets/User/Positions/PositionsApplet.cpp index 88bed998d..ad0f9fc47 100644 --- a/src/graphics/niche/InkHUD/Applets/User/Positions/PositionsApplet.cpp +++ b/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 // ----------------------------------------------------------------- diff --git a/src/mesh/MeshModule.cpp b/src/mesh/MeshModule.cpp index c5748a560..83b64a873 100644 --- a/src/mesh/MeshModule.cpp +++ b/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; diff --git a/src/mesh/MeshService.cpp b/src/mesh/MeshService.cpp index 297404747..368642bf8 100644 --- a/src/mesh/MeshService.cpp +++ b/src/mesh/MeshService.cpp @@ -93,11 +93,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); diff --git a/src/mesh/NextHopRouter.cpp b/src/mesh/NextHopRouter.cpp index afdb4d096..5230e5b85 100644 --- a/src/mesh/NextHopRouter.cpp +++ b/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); diff --git a/src/mesh/NodeDB.cpp b/src/mesh/NodeDB.cpp index 9052ee17c..3c408f01f 100644 --- a/src/mesh/NodeDB.cpp +++ b/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(); } diff --git a/src/mesh/NodeDB.h b/src/mesh/NodeDB.h index 6fd8deb87..817e31617 100644 --- a/src/mesh/NodeDB.h +++ b/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, diff --git a/src/mesh/ReliableRouter.cpp b/src/mesh/ReliableRouter.cpp index 7619fc106..2b9b17183 100644 --- a/src/mesh/ReliableRouter.cpp +++ b/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 diff --git a/src/mesh/Router.cpp b/src/mesh/Router.cpp index ad0c0be6f..92bf2b420 100644 --- a/src/mesh/Router.cpp +++ b/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 } diff --git a/src/modules/NeighborInfoModule.cpp b/src/modules/NeighborInfoModule.cpp index 936a7b44a..2cd8ec5ed 100644 --- a/src/modules/NeighborInfoModule.cpp +++ b/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, diff --git a/src/modules/RoutingModule.cpp b/src/modules/RoutingModule.cpp index 662f5379a..e9e1fc786 100644 --- a/src/modules/RoutingModule.cpp +++ b/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 diff --git a/src/modules/RoutingModule.h b/src/modules/RoutingModule.h index 5d4b9596f..2ac42f447 100644 --- a/src/modules/RoutingModule.h +++ b/src/modules/RoutingModule.h @@ -20,7 +20,7 @@ class RoutingModule : public ProtobufModule 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; diff --git a/src/modules/TraceRouteModule.cpp b/src/modules/TraceRouteModule.cpp index 87a2f1bd2..41dc02cd1 100644 --- a/src/modules/TraceRouteModule.cpp +++ b/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,10 +360,10 @@ 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; @@ -370,7 +371,7 @@ void TraceRouteModule::insertUnknownHops(meshtastic_MeshPacket &p, meshtastic_Ro } // 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; diff --git a/src/serialization/MeshPacketSerializer.cpp b/src/serialization/MeshPacketSerializer.cpp index b31d2dc2e..a12972cb0 100644 --- a/src/serialization/MeshPacketSerializer.cpp +++ b/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); diff --git a/src/serialization/MeshPacketSerializer_nRF52.cpp b/src/serialization/MeshPacketSerializer_nRF52.cpp index 353c710a1..41f505b94 100644 --- a/src/serialization/MeshPacketSerializer_nRF52.cpp +++ b/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;