Reliable ACKs for DMs (#8165)

* RoutingModule::sendAckNak takes ackWantsAck arg to set want_ack on the ACK itself

* Use reliable delivery for traceroute requests (which will be copied to traceroute responses by setReplyTo)

* Update ReliableRouter::sniffReceived to use ReliableRouter::shouldSuccessAckWithWantAck

* Use isFromUs

* Update MockRoutingModule::sendAckNak to include ackWantsAck argument (currently ignored)

---------

Co-authored-by: Ben Meadors <benmmeadors@gmail.com>

src/mesh/PhoneAPI.cpp

@@ -710,6 +710,13 @@ bool PhoneAPI::handleToRadioPacket(meshtastic_MeshPacket &p)
         // sendNotification(meshtastic_LogRecord_Level_WARNING, p.id, "Text messages can only be sent once every 2 seconds");
         return false;
     }
+
+    // Upgrade traceroute requests from phone to use reliable delivery, matching TraceRouteModule
+    if (p.decoded.portnum == meshtastic_PortNum_TRACEROUTE_APP && !isBroadcast(p.to)) {
+        // Use reliable delivery for traceroute requests (which will be copied to traceroute responses by setReplyTo)
+        p.want_ack = true;
+    }
+
     lastPortNumToRadio[p.decoded.portnum] = millis();
     service->handleToRadio(p);
     return true;

src/mesh/ReliableRouter.cpp

@@ -103,10 +103,20 @@ void ReliableRouter::sniffReceived(const meshtastic_MeshPacket *p, const meshtas
                     /* A response may be set to want_ack for retransmissions, but we don't need to ACK a response if it received
                       an implicit ACK already. If we received it directly or via NextHopRouter, only ACK with a hop limit of 0 to
                       make sure the other side stops retransmitting. */
-                    if (!p->decoded.request_id && !p->decoded.reply_id) {
+
+                    if (shouldSuccessAckWithWantAck(p)) {
+                        // 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);
+                    } 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) {
+                        // 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.
                         sendAckNak(meshtastic_Routing_Error_NONE, getFrom(p), p->id, p->channel, 0);
                     }
                 } else if (p->which_payload_variant == meshtastic_MeshPacket_encrypted_tag && p->channel == 0 &&
@@ -152,4 +162,36 @@ void ReliableRouter::sniffReceived(const meshtastic_MeshPacket *p, const meshtas
 
     // handle the packet as normal
     isBroadcast(p->to) ? FloodingRouter::sniffReceived(p, c) : NextHopRouter::sniffReceived(p, c);
+}
+
+/**
+ * If we ACK this packet, should we set want_ack=true on the ACK for reliable delivery of the ACK packet?
+ */
+bool ReliableRouter::shouldSuccessAckWithWantAck(const meshtastic_MeshPacket *p)
+{
+    // Don't ACK-with-want-ACK outgoing packets
+    if (isFromUs(p))
+        return false;
+
+    // Only ACK-with-want-ACK if the original packet asked for want_ack
+    if (!p->want_ack)
+        return false;
+
+    // Only ACK-with-want-ACK packets to us (not broadcast)
+    if (!isToUs(p))
+        return false;
+
+    // Special case for text message DMs:
+    bool isTextMessage =
+        (p->which_payload_variant == meshtastic_MeshPacket_decoded_tag) &&
+        IS_ONE_OF(p->decoded.portnum, meshtastic_PortNum_TEXT_MESSAGE_APP, meshtastic_PortNum_TEXT_MESSAGE_COMPRESSED_APP);
+
+    if (isTextMessage) {
+        // If it's a non-broadcast text message, and the original asked for want_ack,
+        // let's send an ACK that is itself want_ack to improve reliability of confirming delivery back to the sender.
+        // This should include all DMs regardless of whether or not reply_id is set.
+        return true;
+    }
+
+    return false;
 }

src/mesh/ReliableRouter.h

@@ -31,4 +31,10 @@ class ReliableRouter : public NextHopRouter
      * We hook this method so we can see packets before FloodingRouter says they should be discarded
      */
     virtual bool shouldFilterReceived(const meshtastic_MeshPacket *p) override;
+
+  private:
+    /**
+     * Should this packet be ACKed with a want_ack for reliable delivery?
+     */
+    bool shouldSuccessAckWithWantAck(const meshtastic_MeshPacket *p);
 };

src/mesh/Router.cpp

@@ -198,9 +198,10 @@ meshtastic_MeshPacket *Router::allocForSending()
 /**
  * Send an ack or a nak packet back towards whoever sent idFrom
  */
-void Router::sendAckNak(meshtastic_Routing_Error err, NodeNum to, PacketId idFrom, ChannelIndex chIndex, uint8_t hopLimit)
+void Router::sendAckNak(meshtastic_Routing_Error err, NodeNum to, PacketId idFrom, ChannelIndex chIndex, uint8_t hopLimit,
+                        bool ackWantsAck)
 {
-    routingModule->sendAckNak(err, to, idFrom, chIndex, hopLimit);
+    routingModule->sendAckNak(err, to, idFrom, chIndex, hopLimit, ackWantsAck);
 }
 
 void Router::abortSendAndNak(meshtastic_Routing_Error err, meshtastic_MeshPacket *p)

src/mesh/Router.h

@@ -125,7 +125,8 @@ class Router : protected concurrency::OSThread, protected PacketHistory
     /**
      * Send an ack or a nak packet back towards whoever sent idFrom
      */
-    void sendAckNak(meshtastic_Routing_Error err, NodeNum to, PacketId idFrom, ChannelIndex chIndex, uint8_t hopLimit = 0);
+    void sendAckNak(meshtastic_Routing_Error err, NodeNum to, PacketId idFrom, ChannelIndex chIndex, uint8_t hopLimit = 0,
+                    bool ackWantsAck = false);
 
   private:
     /**

src/modules/RoutingModule.cpp

@@ -47,10 +47,14 @@ meshtastic_MeshPacket *RoutingModule::allocReply()
     return NULL;
 }
 
-void RoutingModule::sendAckNak(meshtastic_Routing_Error err, NodeNum to, PacketId idFrom, ChannelIndex chIndex, uint8_t hopLimit)
+void RoutingModule::sendAckNak(meshtastic_Routing_Error err, NodeNum to, PacketId idFrom, ChannelIndex chIndex, uint8_t hopLimit,
+                               bool ackWantsAck)
 {
     auto p = allocAckNak(err, to, idFrom, chIndex, hopLimit);
 
+    // Allow the caller to set want_ack on this ACK packet if it's important that the ACK be delivered reliably
+    p->want_ack = ackWantsAck;
+
     router->sendLocal(p); // we sometimes send directly to the local node
 }
 

src/modules/RoutingModule.h

@@ -13,8 +13,8 @@ class RoutingModule : public ProtobufModule<meshtastic_Routing>
      */
     RoutingModule();
 
-    virtual void sendAckNak(meshtastic_Routing_Error err, NodeNum to, PacketId idFrom, ChannelIndex chIndex,
+    virtual void sendAckNak(meshtastic_Routing_Error err, NodeNum to, PacketId idFrom, ChannelIndex chIndex, uint8_t hopLimit = 0,
-                            uint8_t hopLimit = 0);
+                            bool ackWantsAck = false);
 
     // 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);

src/modules/TraceRouteModule.cpp

@@ -419,6 +419,9 @@ bool TraceRouteModule::startTraceRoute(NodeNum node)
         p->decoded.portnum = meshtastic_PortNum_TRACEROUTE_APP;
         p->decoded.want_response = true;
 
+        // Use reliable delivery for traceroute requests (which will be copied to traceroute responses by setReplyTo)
+        p->want_ack = true;
+
         // Manually encode the RouteDiscovery payload
         p->decoded.payload.size =
             pb_encode_to_bytes(p->decoded.payload.bytes, sizeof(p->decoded.payload.bytes), &meshtastic_RouteDiscovery_msg, &req);
@@ -532,6 +535,9 @@ void TraceRouteModule::launch(NodeNum node)
         p->decoded.portnum = meshtastic_PortNum_TRACEROUTE_APP;
         p->decoded.want_response = true;
 
+        // Use reliable delivery for traceroute requests (which will be copied to traceroute responses by setReplyTo)
+        p->want_ack = true;
+
         p->decoded.payload.size =
             pb_encode_to_bytes(p->decoded.payload.bytes, sizeof(p->decoded.payload.bytes), &meshtastic_RouteDiscovery_msg, &req);
 

test/test_mqtt/MQTT.cpp

@@ -83,8 +83,8 @@ class MockNodeDB : public NodeDB
 class MockRoutingModule : public RoutingModule
 {
   public:
-    void sendAckNak(meshtastic_Routing_Error err, NodeNum to, PacketId idFrom, ChannelIndex chIndex,
+    void sendAckNak(meshtastic_Routing_Error err, NodeNum to, PacketId idFrom, ChannelIndex chIndex, uint8_t hopLimit = 0,
-                    uint8_t hopLimit = 0) override
+                    bool ackWantsAck = false) override
     {
         ackNacks_.emplace_back(err, to, idFrom, chIndex, hopLimit);
     }