[BUG] Fix ReadClient to use the right parameters when computing the l…

…iveness timeout (#22699) (#22760)

* Fix ReadClient to use the right parameters when computing the liveness
timeout

This fixes the logic in ReadClient to use the local IDLE interval when
computing the liveness timeout instead of the IDLE interval of our peer.

Testing:

Using the REPL and the liveness timer print, confirmed the fixes are
indeed applied.

Restyled by whitespace

Restyled by clang-format

Build fixes

* Review feedback

Co-authored-by: Jerry Johns <johnsj@google.com>

src/app/ReadClient.cpp

@@ -30,6 +30,7 @@
 #include <assert.h>
 #include <lib/core/CHIPTLVTypes.h>
 #include <lib/support/FibonacciUtils.h>
+#include <messaging/ReliableMessageMgr.h>
 
 namespace chip {
 namespace app {
@@ -798,7 +799,19 @@ CHIP_ERROR ReadClient::RefreshLivenessCheckTimer()
     else
     {
         VerifyOrReturnError(mReadPrepareParams.mSessionHolder, CHIP_ERROR_INCORRECT_STATE);
-        timeout = System::Clock::Seconds16(mMaxInterval) + mReadPrepareParams.mSessionHolder->GetAckTimeout();
+
+        //
+        // To calculate the duration we're willing to wait for a report to come to us, we take into account the maximum interval of
+        // the subscription AND the time it takes for the report to make it to us in the worst case. This latter bit involves
+        // computing the Ack timeout from the publisher for the ReportData message being sent to us using our IDLE interval as the
+        // basis for that computation.
+        //
+        // TODO: We need to find a good home for this logic that will correctly compute this based on transport. For now, this will
+        // suffice since we don't use TCP as a transport currently and subscriptions over BLE aren't really a thing.
+        //
+        auto publisherTransmissionTimeout =
+            GetLocalMRPConfig().ValueOr(GetDefaultMRPConfig()).mIdleRetransTimeout * (CHIP_CONFIG_RMP_DEFAULT_MAX_RETRANS + 1);
+        timeout = System::Clock::Seconds16(mMaxInterval) + publisherTransmissionTimeout;
     }
 
     // EFR32/MBED/INFINION/K32W's chrono count return long unsinged, but other platform returns unsigned

src/messaging/ReliableMessageProtocolConfig.cpp

@@ -31,6 +31,23 @@ namespace chip {
 
 using namespace System::Clock::Literals;
 
+#if CONFIG_BUILD_FOR_HOST_UNIT_TEST
+static Optional<System::Clock::Timeout> idleRetransTimeoutOverride   = NullOptional;
+static Optional<System::Clock::Timeout> activeRetransTimeoutOverride = NullOptional;
+
+void OverrideLocalMRPConfig(System::Clock::Timeout idleRetransTimeout, System::Clock::Timeout activeRetransTimeout)
+{
+    idleRetransTimeoutOverride.SetValue(idleRetransTimeout);
+    activeRetransTimeoutOverride.SetValue(activeRetransTimeout);
+}
+
+void ClearLocalMRPConfigOverride()
+{
+    activeRetransTimeoutOverride.ClearValue();
+    idleRetransTimeoutOverride.ClearValue();
+}
+#endif
+
 ReliableMessageProtocolConfig GetDefaultMRPConfig()
 {
     // Default MRP intervals are defined in spec <2.11.3. Parameters and Constants>
@@ -55,6 +72,18 @@ Optional<ReliableMessageProtocolConfig> GetLocalMRPConfig()
     }
 #endif
 
+#if CONFIG_BUILD_FOR_HOST_UNIT_TEST
+    if (idleRetransTimeoutOverride.HasValue())
+    {
+        config.mIdleRetransTimeout = idleRetransTimeoutOverride.Value();
+    }
+
+    if (activeRetransTimeoutOverride.HasValue())
+    {
+        config.mActiveRetransTimeout = activeRetransTimeoutOverride.Value();
+    }
+#endif
+
     return (config == GetDefaultMRPConfig()) ? Optional<ReliableMessageProtocolConfig>::Missing()
                                              : Optional<ReliableMessageProtocolConfig>::Value(config);
 }

src/messaging/ReliableMessageProtocolConfig.h

@@ -156,4 +156,24 @@ ReliableMessageProtocolConfig GetDefaultMRPConfig();
  */
 Optional<ReliableMessageProtocolConfig> GetLocalMRPConfig();
 
+#if CONFIG_BUILD_FOR_HOST_UNIT_TEST
+
+/**
+ * @brief
+ *
+ * Overrides the local idle and active retransmission timeout parameters (which are usually set through compile
+ * time defines). This is reserved for tests that need the ability to set these at runtime to make certain test scenarios possible.
+ *
+ */
+void OverrideLocalMRPConfig(System::Clock::Timeout idleRetransTimeout, System::Clock::Timeout activeRetransTimeout);
+
+/**
+ * @brief
+ *
+ * Disables the overrides set previously in OverrideLocalMRPConfig().
+ *
+ */
+void ClearLocalMRPConfigOverride();
+#endif
+
 } // namespace chip

src/messaging/tests/MessagingContext.cpp

@@ -95,6 +95,11 @@ void MessagingContext::ShutdownAndRestoreExisting(MessagingContext & existing)
     existing.mTransport->SetSessionManager(&existing.GetSecureSessionManager());
 }
 
+using namespace System::Clock::Literals;
+
+constexpr chip::System::Clock::Timeout MessagingContext::kResponsiveIdleRetransTimeout;
+constexpr chip::System::Clock::Timeout MessagingContext::kResponsiveActiveRetransTimeout;
+
 void MessagingContext::SetMRPMode(MRPMode mode)
 {
     if (mode == MRPMode::kDefault)
@@ -103,17 +108,37 @@ void MessagingContext::SetMRPMode(MRPMode mode)
         mSessionAliceToBob->AsSecureSession()->SetRemoteMRPConfig(GetDefaultMRPConfig());
         mSessionCharlieToDavid->AsSecureSession()->SetRemoteMRPConfig(GetDefaultMRPConfig());
         mSessionDavidToCharlie->AsSecureSession()->SetRemoteMRPConfig(GetDefaultMRPConfig());
+
+#if CONFIG_BUILD_FOR_HOST_UNIT_TEST
+        ClearLocalMRPConfigOverride();
+#else
+        //
+        // A test is calling this function assuming the overrides above are going to work
+        // when in fact, they won't because the compile flag is not set correctly.
+        //
+        VerifyOrDie(false);
+#endif
     }
     else
     {
-        mSessionBobToAlice->AsSecureSession()->SetRemoteMRPConfig(
-            ReliableMessageProtocolConfig(System::Clock::Milliseconds32(10), System::Clock::Milliseconds32(10)));
-        mSessionAliceToBob->AsSecureSession()->SetRemoteMRPConfig(
-            ReliableMessageProtocolConfig(System::Clock::Milliseconds32(10), System::Clock::Milliseconds32(10)));
-        mSessionCharlieToDavid->AsSecureSession()->SetRemoteMRPConfig(
-            ReliableMessageProtocolConfig(System::Clock::Milliseconds32(10), System::Clock::Milliseconds32(10)));
-        mSessionDavidToCharlie->AsSecureSession()->SetRemoteMRPConfig(
-            ReliableMessageProtocolConfig(System::Clock::Milliseconds32(10), System::Clock::Milliseconds32(10)));
+#if CONFIG_BUILD_FOR_HOST_UNIT_TEST
+        OverrideLocalMRPConfig(MessagingContext::kResponsiveIdleRetransTimeout, MessagingContext::kResponsiveActiveRetransTimeout);
+#else
+        //
+        // A test is calling this function assuming the overrides above are going to work
+        // when in fact, they won't because the compile flag is not set correctly.
+        //
+        VerifyOrDie(false);
+#endif
+
+        mSessionBobToAlice->AsSecureSession()->SetRemoteMRPConfig(ReliableMessageProtocolConfig(
+            MessagingContext::kResponsiveIdleRetransTimeout, MessagingContext::kResponsiveActiveRetransTimeout));
+        mSessionAliceToBob->AsSecureSession()->SetRemoteMRPConfig(ReliableMessageProtocolConfig(
+            MessagingContext::kResponsiveIdleRetransTimeout, MessagingContext::kResponsiveActiveRetransTimeout));
+        mSessionCharlieToDavid->AsSecureSession()->SetRemoteMRPConfig(ReliableMessageProtocolConfig(
+            MessagingContext::kResponsiveIdleRetransTimeout, MessagingContext::kResponsiveActiveRetransTimeout));
+        mSessionDavidToCharlie->AsSecureSession()->SetRemoteMRPConfig(ReliableMessageProtocolConfig(
+            MessagingContext::kResponsiveIdleRetransTimeout, MessagingContext::kResponsiveActiveRetransTimeout));
     }
 }
 

src/messaging/tests/MessagingContext.h

@@ -85,6 +85,12 @@ class MessagingContext : public PlatformMemoryUser
                          //      i.e IDLE = 10ms, ACTIVE = 10ms
     };
 
+    //
+    // See above for a description of the values used.
+    //
+    static constexpr System::Clock::Timeout kResponsiveIdleRetransTimeout   = System::Clock::Milliseconds32(10);
+    static constexpr System::Clock::Timeout kResponsiveActiveRetransTimeout = System::Clock::Milliseconds32(10);
+
     MessagingContext() :
         mInitialized(false), mAliceAddress(Transport::PeerAddress::UDP(GetAddress(), CHIP_PORT + 1)),
         mBobAddress(Transport::PeerAddress::UDP(GetAddress(), CHIP_PORT))