Don't send piggybacked acks when they are not needed.

src/messaging/ExchangeContext.cpp

@@ -237,7 +237,6 @@ ExchangeContext::ExchangeContext(ExchangeManager * em, uint16_t ExchangeId, Secu
 
     SetDropAckDebug(false);
     SetAckPending(false);
-    SetPeerRequestedAck(false);
     SetMsgRcvdFromPeer(false);
     SetAutoRequestAck(true);
 

src/messaging/ExchangeMessageDispatch.cpp

@@ -50,18 +50,14 @@ CHIP_ERROR ExchangeMessageDispatch::SendMessage(SecureSessionHandle session, uin
     payloadHeader.SetExchangeID(exchangeId).SetMessageType(protocol, type).SetInitiator(isInitiator);
 
     // If there is a pending acknowledgment piggyback it on this message.
-    if (reliableMessageContext->HasPeerRequestedAck())
+    if (reliableMessageContext->IsAckPending())
     {
-        payloadHeader.SetAckId(reliableMessageContext->GetPendingPeerAckId());
-
-        // Set AckPending flag to false since current outgoing message is going to serve as the ack on this exchange.
-        reliableMessageContext->SetAckPending(false);
+        payloadHeader.SetAckId(reliableMessageContext->TakePendingPeerAckId());
 
 #if !defined(NDEBUG)
         if (!payloadHeader.HasMessageType(Protocols::SecureChannel::MsgType::StandaloneAck))
         {
-            ChipLogDetail(ExchangeManager, "Piggybacking Ack for MsgId:%08" PRIX32 " with msg",
-                          reliableMessageContext->GetPendingPeerAckId());
+            ChipLogDetail(ExchangeManager, "Piggybacking Ack for MsgId:%08" PRIX32 " with msg", payloadHeader.GetAckId().Value());
         }
 #endif
     }
@@ -115,11 +111,6 @@ CHIP_ERROR ExchangeMessageDispatch::OnMessageReceived(const PayloadHeader & payl
         if (payloadHeader.NeedsAck())
         {
             // An acknowledgment needs to be sent back to the peer for this message on this exchange,
-            // Set the flag in message header indicating an ack requested by peer;
-            msgFlags.Set(MessageFlagValues::kPeerRequestedAck);
-
-            // Also set the flag in the exchange context indicating an ack requested;
-            reliableMessageContext->SetPeerRequestedAck(true);
 
             ReturnErrorOnFailure(reliableMessageContext->HandleNeedsAck(messageId, msgFlags));
         }

src/messaging/Flags.h

@@ -43,16 +43,14 @@ enum class MessageFlagValues : uint32_t
     kMessageEncoded = 0x00001000,
     /**< Indicates that default IPv6 source address selection should be used when sending IPv6 multicast messages. */
     kDefaultMulticastSourceAddress = 0x00002000,
-    /**< Indicates that the sender of the  message requested an acknowledgment. */
-    kPeerRequestedAck = 0x00004000,
     /**< Indicates that the message is a duplicate of a previously received message. */
-    kDuplicateMessage = 0x00008000,
+    kDuplicateMessage = 0x00004000,
     /**< Indicates that the peer's group key message counter is not synchronized. */
-    kPeerGroupMsgIdNotSynchronized = 0x00010000,
+    kPeerGroupMsgIdNotSynchronized = 0x00008000,
     /**< Indicates that the source of the message is the initiator of the CHIP exchange. */
-    kFromInitiator = 0x00020000,
+    kFromInitiator = 0x00010000,
     /**< Indicates that message is being sent/received via the local ephemeral UDP port. */
-    kViaEphemeralUDPPort = 0x00040000,
+    kViaEphemeralUDPPort = 0x00020000,
 };
 
 using MessageFlags = BitFlags<MessageFlagValues>;

src/messaging/ReliableMessageContext.cpp

@@ -62,11 +62,6 @@ bool ReliableMessageContext::IsAckPending() const
     return mFlags.Has(Flags::kFlagAckPending);
 }
 
-bool ReliableMessageContext::HasPeerRequestedAck() const
-{
-    return mFlags.Has(Flags::kFlagPeerRequestedAck);
-}
-
 bool ReliableMessageContext::HasRcvdMsgFromPeer() const
 {
     return mFlags.Has(Flags::kFlagMsgRcvdFromPeer);
@@ -87,11 +82,6 @@ void ReliableMessageContext::SetAckPending(bool inAckPending)
     mFlags.Set(Flags::kFlagAckPending, inAckPending);
 }
 
-void ReliableMessageContext::SetPeerRequestedAck(bool inPeerRequestedAck)
-{
-    mFlags.Set(Flags::kFlagPeerRequestedAck, inPeerRequestedAck);
-}
-
 void ReliableMessageContext::SetDropAckDebug(bool inDropAckDebug)
 {
     mFlags.Set(Flags::kFlagDropAckDebug, inDropAckDebug);
@@ -200,7 +190,8 @@ CHIP_ERROR ReliableMessageContext::HandleNeedsAck(uint32_t MessageId, BitFlags<M
     // Expire any virtual ticks that have expired so all wakeup sources reflect the current time
     GetReliableMessageMgr()->ExpireTicks();
 
-    // If the message IS a duplicate.
+    // If the message IS a duplicate there will never be a response to it, so we
+    // should not wait for one and just immediately send a standalone ack.
     if (MsgFlags.Has(MessageFlagValues::kDuplicateMessage))
     {
 #if !defined(NDEBUG)
@@ -213,7 +204,7 @@ CHIP_ERROR ReliableMessageContext::HandleNeedsAck(uint32_t MessageId, BitFlags<M
         uint32_t tempAckId = mPendingPeerAckId;
 
         // Set the pending ack id.
-        mPendingPeerAckId = MessageId;
+        SetPendingPeerAckId(MessageId);
 
         // Send the Ack for the duplication message in a SecureChannel::StandaloneAck message.
         err = SendStandaloneAckMessage();
@@ -222,8 +213,7 @@ CHIP_ERROR ReliableMessageContext::HandleNeedsAck(uint32_t MessageId, BitFlags<M
         if (wasAckPending)
         {
             // Restore previously pending ack id.
-            mPendingPeerAckId = tempAckId;
-            SetAckPending(true);
+            SetPendingPeerAckId(tempAckId);
         }
 
         SuccessOrExit(err);
@@ -243,11 +233,10 @@ CHIP_ERROR ReliableMessageContext::HandleNeedsAck(uint32_t MessageId, BitFlags<M
         }
 
         // Replace the Pending ack id.
-        mPendingPeerAckId = MessageId;
+        SetPendingPeerAckId(MessageId);
         mNextAckTimeTick =
             static_cast<uint16_t>(CHIP_CONFIG_RMP_DEFAULT_ACK_TIMEOUT_TICK +
                                   GetReliableMessageMgr()->GetTickCounterFromTimeDelta(System::Timer::GetCurrentEpoch()));
-        SetAckPending(true);
     }
 
 exit:
@@ -286,5 +275,11 @@ CHIP_ERROR ReliableMessageContext::SendStandaloneAckMessage()
     return err;
 }
 
+void ReliableMessageContext::SetPendingPeerAckId(uint32_t aPeerAckId)
+{
+    mPendingPeerAckId = aPeerAckId;
+    SetAckPending(true);
+}
+
 } // namespace Messaging
 } // namespace chip

src/messaging/ReliableMessageContext.h

@@ -57,7 +57,16 @@ class ReliableMessageContext
      */
     CHIP_ERROR FlushAcks();
 
-    uint32_t GetPendingPeerAckId() { return mPendingPeerAckId; }
+    /**
+     * Take the pending peer ack id from the context.  This must only be called
+     * when IsAckPending() is true.  After this call, IsAckPending() will be
+     * false; it's the caller's responsibility to send the ack.
+     */
+    uint32_t TakePendingPeerAckId()
+    {
+        SetAckPending(false);
+        return mPendingPeerAckId;
+    }
 
     /**
      *  Get the initial retransmission interval. It would be the time to wait before
@@ -131,33 +140,6 @@ class ReliableMessageContext
      */
     bool IsAckPending() const;
 
-    /**
-     *  Set if an acknowledgment needs to be sent back to the peer on this exchange.
-     *
-     *  @param[in]  inAckPending A Boolean indicating whether (true) or not
-     *                          (false) an acknowledgment should be sent back
-     *                          in response to a received message.
-     */
-    void SetAckPending(bool inAckPending);
-
-    /**
-     *  Determine whether peer requested acknowledgment for at least one message
-     *  on this exchange.
-     *
-     *  @return Returns 'true' if acknowledgment requested, else 'false'.
-     */
-    bool HasPeerRequestedAck() const;
-
-    /**
-     *  Set if an acknowledgment was requested in the last message received
-     *  on this exchange.
-     *
-     *  @param[in]  inPeerRequestedAck A Boolean indicating whether (true) or not
-     *                                 (false) an acknowledgment was requested
-     *                                 in the last received message.
-     */
-    void SetPeerRequestedAck(bool inPeerRequestedAck);
-
     /**
      *  Determine whether at least one message has been received
      *  on this exchange from peer.
@@ -218,13 +200,8 @@ class ReliableMessageContext
         /// When set, signifies that there is an acknowledgment pending to be sent back.
         kFlagAckPending = 0x0020,
 
-        /// When set, signifies that at least one message received on this exchange requested an acknowledgment.
-        /// This flag is read by the application to decide if it needs to request an acknowledgment for the
-        /// response message it is about to send. This flag can also indicate whether peer is using ReliableMessageProtocol.
-        kFlagPeerRequestedAck = 0x0040,
-
         /// When set, signifies that at least one message has been received from peer on this exchange context.
-        kFlagMsgRcvdFromPeer = 0x0080,
+        kFlagMsgRcvdFromPeer = 0x0040,
     };
 
     BitFlags<Flags> mFlags; // Internal state flags
@@ -236,6 +213,19 @@ class ReliableMessageContext
     CHIP_ERROR HandleNeedsAck(uint32_t MessageId, BitFlags<MessageFlagValues> Flags);
     ExchangeContext * GetExchangeContext();
 
+    /**
+     *  Set if an acknowledgment needs to be sent back to the peer on this exchange.
+     *
+     *  @param[in]  inAckPending A Boolean indicating whether (true) or not
+     *                          (false) an acknowledgment should be sent back
+     *                          in response to a received message.
+     */
+    void SetAckPending(bool inAckPending);
+
+    // Set our pending peer ack id and any other state needed to ensure that we
+    // will send that ack at some point.
+    void SetPendingPeerAckId(uint32_t aPeerAckId);
+
 private:
     friend class ReliableMessageMgr;
     friend class ExchangeContext;

src/messaging/ReliableMessageMgr.cpp

@@ -116,7 +116,6 @@ void ReliableMessageMgr::ExecuteActions()
 #endif
                 // Send the Ack in a SecureChannel::StandaloneAck message
                 rc->SendStandaloneAckMessage();
-                rc->SetAckPending(false);
             }
         }
     });