Handle repeated packet after potentially canceling previous Tx (#5330)

* Revert "Fix sending duplicate packets to PhoneAPI/MQTT (#5315)"

This reverts commit 40bc04b521.

* Handle repeated packet after potentially canceling previous Tx

src/mesh/FloodingRouter.cpp

@@ -29,6 +29,17 @@ bool FloodingRouter::shouldFilterReceived(const meshtastic_MeshPacket *p)
             if (Router::cancelSending(p->from, p->id))
                 txRelayCanceled++;
         }
+
+        /* If the original transmitter is doing retransmissions (hopStart equals hopLimit) for a reliable transmission, e.g., when
+        the ACK got lost, we will handle the packet again to make sure it gets an ACK to its packet. */
+        bool isRepeated = p->hop_start > 0 && p->hop_start == p->hop_limit;
+        if (isRepeated) {
+            LOG_DEBUG("Repeated reliable tx");
+            if (!perhapsRebroadcast(p) && isToUs(p) && p->want_ack) {
+                sendAckNak(meshtastic_Routing_Error_NONE, getFrom(p), p->id, p->channel, 0);
+            }
+        }
+
         return true;
     }
 
@@ -41,14 +52,8 @@ bool FloodingRouter::isRebroadcaster()
            config.device.rebroadcast_mode != meshtastic_Config_DeviceConfig_RebroadcastMode_NONE;
 }
 
-void FloodingRouter::sniffReceived(const meshtastic_MeshPacket *p, const meshtastic_Routing *c)
+bool FloodingRouter::perhapsRebroadcast(const meshtastic_MeshPacket *p)
 {
-    bool isAckorReply = (p->which_payload_variant == meshtastic_MeshPacket_decoded_tag) && (p->decoded.request_id != 0);
-    if (isAckorReply && !isToUs(p) && !isBroadcast(p->to)) {
-        // do not flood direct message that is ACKed or replied to
-        LOG_DEBUG("Rxd an ACK/reply not for me, cancel rebroadcast");
-        Router::cancelSending(p->to, p->decoded.request_id); // cancel rebroadcast for this DM
-    }
     if (!isToUs(p) && (p->hop_limit > 0) && !isFromUs(p)) {
         if (p->id != 0) {
             if (isRebroadcaster()) {
@@ -67,6 +72,8 @@ void FloodingRouter::sniffReceived(const meshtastic_MeshPacket *p, const meshtas
                 // Note: we are careful to resend using the original senders node id
                 // We are careful not to call our hooked version of send() - because we don't want to check this again
                 Router::send(tosend);
+
+                return true;
             } else {
                 LOG_DEBUG("No rebroadcast: Role = CLIENT_MUTE or Rebroadcast Mode = NONE");
             }
@@ -74,6 +81,21 @@ void FloodingRouter::sniffReceived(const meshtastic_MeshPacket *p, const meshtas
             LOG_DEBUG("Ignore 0 id broadcast");
         }
     }
+
+    return false;
+}
+
+void FloodingRouter::sniffReceived(const meshtastic_MeshPacket *p, const meshtastic_Routing *c)
+{
+    bool isAckorReply = (p->which_payload_variant == meshtastic_MeshPacket_decoded_tag) && (p->decoded.request_id != 0);
+    if (isAckorReply && !isToUs(p) && !isBroadcast(p->to)) {
+        // do not flood direct message that is ACKed or replied to
+        LOG_DEBUG("Rxd an ACK/reply not for me, cancel rebroadcast");
+        Router::cancelSending(p->to, p->decoded.request_id); // cancel rebroadcast for this DM
+    }
+
+    perhapsRebroadcast(p);
+
     // handle the packet as normal
     Router::sniffReceived(p, c);
-}
+}

src/mesh/FloodingRouter.h

@@ -1,5 +1,6 @@
 #pragma once
 
+#include "PacketHistory.h"
 #include "Router.h"
 
 /**
@@ -25,11 +26,15 @@
   Any entries in recentBroadcasts that are older than X seconds (longer than the
   max time a flood can take) will be discarded.
  */
-class FloodingRouter : public Router
+class FloodingRouter : public Router, protected PacketHistory
 {
   private:
     bool isRebroadcaster();
 
+    /** Check if we should rebroadcast this packet, and do so if needed
+     * @return true if rebroadcasted */
+    bool perhapsRebroadcast(const meshtastic_MeshPacket *p);
+
   public:
     /**
      * Constructor

src/mesh/NodeDB.cpp

@@ -1231,12 +1231,12 @@ meshtastic_NodeInfoLite *NodeDB::getOrCreateMeshNode(NodeNum n)
             int oldestBoringIndex = -1;
             for (int i = 1; i < numMeshNodes; i++) {
                 // Simply the oldest non-favorite node
-                if (!meshNodes->at(i).is_favorite && meshNodes->at(i).last_heard < oldest) {
+                if (!meshNodes->at(i).is_favorite && !meshNodes->at(i).is_ignored && meshNodes->at(i).last_heard < oldest) {
                     oldest = meshNodes->at(i).last_heard;
                     oldestIndex = i;
                 }
                 // The oldest "boring" node
-                if (!meshNodes->at(i).is_favorite && meshNodes->at(i).user.public_key.size == 0 &&
+                if (!meshNodes->at(i).is_favorite && !meshNodes->at(i).is_ignored && meshNodes->at(i).user.public_key.size == 0 &&
                     meshNodes->at(i).last_heard < oldestBoring) {
                     oldestBoring = meshNodes->at(i).last_heard;
                     oldestBoringIndex = i;

src/mesh/PacketHistory.cpp

@@ -16,7 +16,7 @@ PacketHistory::PacketHistory()
 /**
  * Update recentBroadcasts and return true if we have already seen this packet
  */
-bool PacketHistory::wasSeenRecently(const meshtastic_MeshPacket *p, bool withUpdate, bool *isRepeated)
+bool PacketHistory::wasSeenRecently(const meshtastic_MeshPacket *p, bool withUpdate)
 {
     if (p->id == 0) {
         LOG_DEBUG("Ignore message with zero id");
@@ -38,18 +38,6 @@ bool PacketHistory::wasSeenRecently(const meshtastic_MeshPacket *p, bool withUpd
         seenRecently = false;
     }
 
-    /* If the original transmitter is doing retransmissions (hopStart equals hopLimit) for a reliable transmission, e.g., when the
-       ACK got lost, we will handle the packet again to make sure it gets an ACK/response to its packet. */
-    if (seenRecently && p->hop_start > 0 && p->hop_start == p->hop_limit) {
-        if (withUpdate)
-            LOG_DEBUG("Repeated reliable tx");
-
-        if (isRepeated)
-            *isRepeated = true;
-
-        seenRecently = false;
-    }
-
     if (seenRecently) {
         LOG_DEBUG("Found existing packet record for fr=0x%x,to=0x%x,id=0x%x", p->from, p->to, p->id);
     }
@@ -59,7 +47,7 @@ bool PacketHistory::wasSeenRecently(const meshtastic_MeshPacket *p, bool withUpd
             recentPackets.erase(found);     // as unsorted_set::iterator is const (can't update timestamp - so re-insert..)
         }
         recentPackets.insert(r);
-        LOG_DEBUG("Add packet record fr=0x%x, id=0x%x", p->from, p->id);
+        printPacket("Add packet record", p);
     }
 
     // Capacity is reerved, so only purge expired packets if recentPackets fills past 90% capacity

src/mesh/PacketHistory.h

@@ -1,6 +1,6 @@
 #pragma once
 
-#include "NodeDB.h"
+#include "Router.h"
 #include <unordered_set>
 
 /// We clear our old flood record 10 minutes after we see the last of it
@@ -41,5 +41,5 @@ class PacketHistory
      *
      * @param withUpdate if true and not found we add an entry to recentPackets
      */
-    bool wasSeenRecently(const meshtastic_MeshPacket *p, bool withUpdate = true, bool *isRepeated = nullptr);
-};
+    bool wasSeenRecently(const meshtastic_MeshPacket *p, bool withUpdate = true);
+};

src/mesh/PhoneAPI.cpp

@@ -12,7 +12,6 @@
 #include "PhoneAPI.h"
 #include "PowerFSM.h"
 #include "RadioInterface.h"
-#include "Router.h"
 #include "TypeConversions.h"
 #include "main.h"
 #include "xmodem.h"
@@ -657,4 +656,4 @@ int PhoneAPI::onNotify(uint32_t newValue)
     }
 
     return timeout ? -1 : 0; // If we timed out, MeshService should stop iterating through observers as we just removed one
-}
+}

src/mesh/ReliableRouter.cpp

@@ -176,9 +176,9 @@ bool ReliableRouter::stopRetransmission(GlobalPacketId key)
         if (old->numRetransmissions < NUM_RETRANSMISSIONS - 1) {
             // remove the 'original' (identified by originator and packet->id) from the txqueue and free it
             cancelSending(getFrom(p), p->id);
-            // now free the pooled copy for retransmission too
-            packetPool.release(p);
         }
+        // now free the pooled copy for retransmission too
+        packetPool.release(p);
         auto numErased = pending.erase(key);
         assert(numErased == 1);
         return true;

src/mesh/Router.cpp

@@ -622,17 +622,9 @@ void Router::handleReceived(meshtastic_MeshPacket *p, RxSource src)
         // us (because we would be able to decrypt it)
         if (!decoded && moduleConfig.mqtt.encryption_enabled && p->channel == 0x00 && !isBroadcast(p->to) && !isToUs(p))
             p_encrypted->pki_encrypted = true;
-
         // After potentially altering it, publish received message to MQTT if we're not the original transmitter of the packet
-        if ((decoded || p_encrypted->pki_encrypted) && moduleConfig.mqtt.enabled && !isFromUs(p) && mqtt) {
-            // Check if it wasn't already seen, then we don't need to handle it again
-            bool isRepeated = false;
-            bool *rptr = &isRepeated;
-            wasSeenRecently(p, false, rptr);
-            if (!isRepeated) {
-                mqtt->onSend(*p_encrypted, *p, p->channel);
-            }
-        }
+        if ((decoded || p_encrypted->pki_encrypted) && moduleConfig.mqtt.enabled && !isFromUs(p) && mqtt)
+            mqtt->onSend(*p_encrypted, *p, p->channel);
 #endif
     }
 
@@ -659,6 +651,13 @@ void Router::perhapsHandleReceived(meshtastic_MeshPacket *p)
         return;
     }
 
+    meshtastic_NodeInfoLite *node = nodeDB->getMeshNode(p->from);
+    if (node != NULL && node->is_ignored) {
+        LOG_DEBUG("Ignore msg, 0x%x is ignored", p->from);
+        packetPool.release(p);
+        return;
+    }
+
     if (p->from == NODENUM_BROADCAST) {
         LOG_DEBUG("Ignore msg from broadcast address");
         packetPool.release(p);

src/mesh/Router.h

@@ -4,7 +4,6 @@
 #include "MemoryPool.h"
 #include "MeshTypes.h"
 #include "Observer.h"
-#include "PacketHistory.h"
 #include "PointerQueue.h"
 #include "RadioInterface.h"
 #include "concurrency/OSThread.h"
@@ -12,7 +11,7 @@
 /**
  * A mesh aware router that supports multiple interfaces.
  */
-class Router : protected concurrency::OSThread, protected PacketHistory
+class Router : protected concurrency::OSThread
 {
   private:
     /// Packets which have just arrived from the radio, ready to be processed by this service and possibly

src/mesh/TypeConversions.cpp

@@ -12,6 +12,7 @@ meshtastic_NodeInfo TypeConversions::ConvertToNodeInfo(const meshtastic_NodeInfo
     info.channel = lite->channel;
     info.via_mqtt = lite->via_mqtt;
     info.is_favorite = lite->is_favorite;
+    info.is_ignored = lite->is_ignored;
 
     if (lite->has_hops_away) {
         info.has_hops_away = true;

src/modules/AdminModule.cpp

@@ -283,6 +283,28 @@ bool AdminModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshta
         }
         break;
     }
+    case meshtastic_AdminMessage_set_ignored_node_tag: {
+        LOG_INFO("Client received set_ignored_node command");
+        meshtastic_NodeInfoLite *node = nodeDB->getMeshNode(r->set_ignored_node);
+        if (node != NULL) {
+            node->is_ignored = true;
+            node->has_device_metrics = false;
+            node->has_position = false;
+            node->user.public_key.size = 0;
+            node->user.public_key.bytes[0] = 0;
+            saveChanges(SEGMENT_DEVICESTATE, false);
+        }
+        break;
+    }
+    case meshtastic_AdminMessage_remove_ignored_node_tag: {
+        LOG_INFO("Client received remove_ignored_node command");
+        meshtastic_NodeInfoLite *node = nodeDB->getMeshNode(r->remove_ignored_node);
+        if (node != NULL) {
+            node->is_ignored = false;
+            saveChanges(SEGMENT_DEVICESTATE, false);
+        }
+        break;
+    }
     case meshtastic_AdminMessage_set_fixed_position_tag: {
         LOG_INFO("Client received set_fixed_position command");
         meshtastic_NodeInfoLite *node = nodeDB->getMeshNode(nodeDB->getNodeNum());

src/modules/RoutingModule.cpp

@@ -28,14 +28,8 @@ bool RoutingModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, mesh
     // FIXME - move this to a non promsicious PhoneAPI module?
     // Note: we are careful not to send back packets that started with the phone back to the phone
     if ((isBroadcast(mp.to) || isToUs(&mp)) && (mp.from != 0)) {
-        // Check if it wasn't already seen, then we don't need to handle it again
-        bool isRepeated = false;
-        bool *rptr = &isRepeated;
-        router->wasSeenRecently(&mp, false, rptr);
-        if (!isRepeated) {
-            printPacket("Delivering rx packet", &mp);
-            service->handleFromRadio(&mp);
-        }
+        printPacket("Delivering rx packet", &mp);
+        service->handleFromRadio(&mp);
     }
 
     return false; // Let others look at this message also if they want