[SED] Take SED polling intervals into account in all MRP contexts (#1…

…4417) Currently, only DNS-SD code takes SED polling intervals into account when advertising CRA and CRI records. PASE and CASE sessions and other contexts, on the other hand, always use the default MRP configuration. Add a function for determining the current local MRP config and use it in all necessary contexts.
project-chip · Jun 26, 2023 · af337bc · af337bc
1 parent feda4f9
commit af337bc
Show file tree

Hide file tree

Showing 18 changed files with 53 additions and 59 deletions.
diff --git a/src/app/DeviceProxy.h b/src/app/DeviceProxy.h
@@ -68,7 +68,7 @@ class DLL_EXPORT DeviceProxy
 
     virtual uint8_t GetNextSequenceNumber() = 0;
 
-    ReliableMessageProtocolConfig mMRPConfig = gDefaultMRPConfig;
+    ReliableMessageProtocolConfig mMRPConfig = GetLocalMRPConfig();
 };
 
 } // namespace chip
diff --git a/src/app/server/CommissioningWindowManager.cpp b/src/app/server/CommissioningWindowManager.cpp
@@ -182,7 +182,7 @@ CHIP_ERROR CommissioningWindowManager::OpenCommissioningWindow()
         ReturnErrorOnFailure(SetTemporaryDiscriminator(mECMDiscriminator));
         ReturnErrorOnFailure(
             mPairingSession.WaitForPairing(mECMPASEVerifier, mECMIterations, ByteSpan(mECMSalt, mECMSaltLength), mECMPasscodeID,
-                                           keyID, Optional<ReliableMessageProtocolConfig>::Value(gDefaultMRPConfig), this));
+                                           keyID, Optional<ReliableMessageProtocolConfig>::Value(GetLocalMRPConfig()), this));
     }
     else
     {
@@ -192,7 +192,7 @@ CHIP_ERROR CommissioningWindowManager::OpenCommissioningWindow()
         ReturnErrorOnFailure(mPairingSession.WaitForPairing(
             pinCode, kSpake2p_Iteration_Count,
             ByteSpan(reinterpret_cast<const uint8_t *>(kSpake2pKeyExchangeSalt), strlen(kSpake2pKeyExchangeSalt)), keyID,
-            Optional<ReliableMessageProtocolConfig>::Value(gDefaultMRPConfig), this));
+            Optional<ReliableMessageProtocolConfig>::Value(GetLocalMRPConfig()), this));
     }
 
     ReturnErrorOnFailure(StartAdvertisement());

diff --git a/src/app/server/Dnssd.cpp b/src/app/server/Dnssd.cpp
@@ -262,7 +262,7 @@ CHIP_ERROR DnssdServer::AdvertiseOperational()
                                                  .SetPeerId(fabricInfo.GetPeerId())
                                                  .SetMac(mac)
                                                  .SetPort(GetSecuredPort())
-                                                 .SetMRPConfig(gDefaultMRPConfig)
+                                                 .SetMRPConfig(GetLocalMRPConfig())
                                                  .SetTcpSupported(Optional<bool>(INET_CONFIG_ENABLE_TCP_ENDPOINT))
                                                  .EnableIpV4(true);
 
@@ -341,7 +341,7 @@ CHIP_ERROR DnssdServer::Advertise(bool commissionableNode, chip::Dnssd::Commissi
     advertiseParameters.SetRotatingDeviceId(chip::Optional<const char *>::Value(rotatingDeviceIdHexBuffer));
 #endif
 
-    advertiseParameters.SetMRPConfig(gDefaultMRPConfig).SetTcpSupported(Optional<bool>(INET_CONFIG_ENABLE_TCP_ENDPOINT));
+    advertiseParameters.SetMRPConfig(GetLocalMRPConfig()).SetTcpSupported(Optional<bool>(INET_CONFIG_ENABLE_TCP_ENDPOINT));
 
     if (mode != chip::Dnssd::CommissioningMode::kEnabledEnhanced)
     {

diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp
@@ -144,7 +144,7 @@ CHIP_ERROR DeviceController::Init(ControllerInitParams params)
         .idAllocator    = &mIDAllocator,
         .fabricTable    = params.systemState->Fabrics(),
         .clientPool     = &mCASEClientPool,
-        .mrpLocalConfig = Optional<ReliableMessageProtocolConfig>::Value(mMRPConfig),
+        .mrpLocalConfig = Optional<ReliableMessageProtocolConfig>::Value(GetLocalMRPConfig()),
     };
 
     CASESessionManagerConfig sessionManagerConfig = {
@@ -894,7 +894,7 @@ CHIP_ERROR DeviceCommissioner::EstablishPASEConnection(NodeId remoteDeviceId, Re
     VerifyOrExit(exchangeCtxt != nullptr, err = CHIP_ERROR_INTERNAL);
 
     err = device->GetPairing().Pair(params.GetPeerAddress(), params.GetSetupPINCode(), keyID,
-                                    Optional<ReliableMessageProtocolConfig>::Value(mMRPConfig), exchangeCtxt, this);
+                                    Optional<ReliableMessageProtocolConfig>::Value(GetLocalMRPConfig()), exchangeCtxt, this);
     SuccessOrExit(err);
 
     // Immediately persist the updated mNextKeyID value

diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h
@@ -394,8 +394,6 @@ class DLL_EXPORT DeviceController : public SessionRecoveryDelegate
 
     uint16_t mVendorId;
 
-    ReliableMessageProtocolConfig mMRPConfig = gDefaultMRPConfig;
-
     //////////// SessionRecoveryDelegate Implementation ///////////////
     void OnFirstMessageDeliveryFailed(const SessionHandle & session) override;
 

diff --git a/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp b/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp
@@ -158,19 +158,7 @@ class AdvertiserMinMdns : public ServiceAdvertiser,
                                    char ** txtFields, size_t & numTxtFields)
     {
         auto optionalMrp = params.GetMRPConfig();
-        // TODO: Issue #5833 - MRP retry intervals should be updated on the poll period value change or device type change.
-#if CHIP_DEVICE_CONFIG_ENABLE_SED
-        chip::DeviceLayer::ConnectivityManager::SEDPollingConfig sedPollingConfig;
-        ReturnErrorOnFailure(chip::DeviceLayer::ConnectivityMgr().GetSEDPollingConfig(sedPollingConfig));
-        // Increment default MRP retry intervals by SED poll period to be on the safe side
-        // and avoid unnecessary retransmissions.
-        if (optionalMrp.HasValue())
-        {
-            auto mrp = optionalMrp.Value();
-            optionalMrp.SetValue(ReliableMessageProtocolConfig(mrp.mIdleRetransTimeout + sedPollingConfig.SlowPollingIntervalMS,
-                                                               mrp.mActiveRetransTimeout + sedPollingConfig.FastPollingIntervalMS));
-        }
-#endif
+
         if (optionalMrp.HasValue())
         {
             auto mrp = optionalMrp.Value();

diff --git a/src/lib/dnssd/Discovery_ImplPlatform.cpp b/src/lib/dnssd/Discovery_ImplPlatform.cpp
@@ -236,18 +236,6 @@ CHIP_ERROR CopyTextRecordValue(char * buffer, size_t bufferLen, const chip::Opti
 
     auto retryInterval = isIdle ? optional.Value().mIdleRetransTimeout : optional.Value().mActiveRetransTimeout;
 
-    // TODO: Issue #5833 - MRP retry intervals should be updated on the poll period value
-    // change or device type change.
-    // TODO: Is this really the best place to set these? Seems like it should be passed
-    // in with the correct values and set one level up from here.
-#if CHIP_DEVICE_CONFIG_ENABLE_SED
-    chip::DeviceLayer::ConnectivityManager::SEDPollingConfig sedPollingConfig;
-    ReturnErrorOnFailure(chip::DeviceLayer::ConnectivityMgr().GetSEDPollingConfig(sedPollingConfig));
-    // Increment default MRP retry intervals by SED poll period to be on the safe side
-    // and avoid unnecessary retransmissions.
-    retryInterval += isIdle ? sedPollingConfig.SlowPollingIntervalMS : sedPollingConfig.FastPollingIntervalMS;
-#endif
-
     if (retryInterval > kMaxRetryInterval)
     {
         ChipLogProgress(Discovery, "MRP retry interval %s value exceeds allowed range of 1 hour, using maximum available",

diff --git a/src/lib/dnssd/Resolver.h b/src/lib/dnssd/Resolver.h
@@ -56,8 +56,9 @@ struct ResolvedNodeData
 
     ReliableMessageProtocolConfig GetMRPConfig() const
     {
-        return ReliableMessageProtocolConfig(GetMrpRetryIntervalIdle().ValueOr(gDefaultMRPConfig.mIdleRetransTimeout),
-                                             GetMrpRetryIntervalActive().ValueOr(gDefaultMRPConfig.mActiveRetransTimeout));
+        const ReliableMessageProtocolConfig defaultConfig = GetLocalMRPConfig();
+        return ReliableMessageProtocolConfig(GetMrpRetryIntervalIdle().ValueOr(defaultConfig.mIdleRetransTimeout),
+                                             GetMrpRetryIntervalActive().ValueOr(defaultConfig.mActiveRetransTimeout));
     }
     Optional<System::Clock::Milliseconds32> GetMrpRetryIntervalIdle() const { return mMrpRetryIntervalIdle; }
     Optional<System::Clock::Milliseconds32> GetMrpRetryIntervalActive() const { return mMrpRetryIntervalActive; }

diff --git a/src/messaging/ReliableMessageProtocolConfig.cpp b/src/messaging/ReliableMessageProtocolConfig.cpp
@@ -24,13 +24,31 @@
 
 #include <messaging/ReliableMessageProtocolConfig.h>
 
+#include <platform/CHIPDeviceLayer.h>
 #include <system/SystemClock.h>
 
 namespace chip {
 
 using namespace System::Clock::Literals;
 
-const ReliableMessageProtocolConfig gDefaultMRPConfig(CHIP_CONFIG_MRP_DEFAULT_IDLE_RETRY_INTERVAL,
-                                                      CHIP_CONFIG_MRP_DEFAULT_ACTIVE_RETRY_INTERVAL);
+ReliableMessageProtocolConfig GetLocalMRPConfig()
+{
+    ReliableMessageProtocolConfig config(CHIP_CONFIG_MRP_DEFAULT_IDLE_RETRY_INTERVAL,
+                                         CHIP_CONFIG_MRP_DEFAULT_ACTIVE_RETRY_INTERVAL);
+
+#if CHIP_DEVICE_CONFIG_ENABLE_SED
+    DeviceLayer::ConnectivityManager::SEDPollingConfig sedPollingConfig;
+
+    if (DeviceLayer::ConnectivityMgr().GetSEDPollingConfig(sedPollingConfig) == CHIP_NO_ERROR)
+    {
+        // Increase default MRP retry intervals by SED polling intervals. That is, intervals for
+        // which the device can be at sleep and not be able to receive any messages).
+        config.mIdleRetransTimeout += sedPollingConfig.SlowPollingIntervalMS;
+        config.mActiveRetransTimeout += sedPollingConfig.FastPollingIntervalMS;
+    }
+#endif
+
+    return config;
+}
 
 } // namespace chip
diff --git a/src/messaging/ReliableMessageProtocolConfig.h b/src/messaging/ReliableMessageProtocolConfig.h
@@ -117,6 +117,6 @@ struct ReliableMessageProtocolConfig
     System::Clock::Milliseconds32 mActiveRetransTimeout;
 };
 
-extern const ReliableMessageProtocolConfig gDefaultMRPConfig;
+ReliableMessageProtocolConfig GetLocalMRPConfig();
 
 } // namespace chip
diff --git a/src/messaging/tests/MessagingContext.cpp b/src/messaging/tests/MessagingContext.cpp
@@ -119,13 +119,13 @@ void MessagingContext::ExpireSessionBobToFriends()
 
 Messaging::ExchangeContext * MessagingContext::NewUnauthenticatedExchangeToAlice(Messaging::ExchangeDelegate * delegate)
 {
-    return mExchangeManager.NewContext(mSessionManager.CreateUnauthenticatedSession(mAliceAddress, gDefaultMRPConfig).Value(),
+    return mExchangeManager.NewContext(mSessionManager.CreateUnauthenticatedSession(mAliceAddress, GetLocalMRPConfig()).Value(),
                                        delegate);
 }
 
 Messaging::ExchangeContext * MessagingContext::NewUnauthenticatedExchangeToBob(Messaging::ExchangeDelegate * delegate)
 {
-    return mExchangeManager.NewContext(mSessionManager.CreateUnauthenticatedSession(mBobAddress, gDefaultMRPConfig).Value(),
+    return mExchangeManager.NewContext(mSessionManager.CreateUnauthenticatedSession(mBobAddress, GetLocalMRPConfig()).Value(),
                                        delegate);
 }
 

diff --git a/src/messaging/tests/TestReliableMessageProtocol.cpp b/src/messaging/tests/TestReliableMessageProtocol.cpp
@@ -1347,8 +1347,8 @@ void CheckLostStandaloneAck(nlTestSuite * inSuite, void * inContext)
 int InitializeTestCase(void * inContext)
 {
     TestContext & ctx = *static_cast<TestContext *>(inContext);
-    ctx.GetSessionAliceToBob()->AsSecureSession()->SetMRPConfig(gDefaultMRPConfig);
-    ctx.GetSessionBobToAlice()->AsSecureSession()->SetMRPConfig(gDefaultMRPConfig);
+    ctx.GetSessionAliceToBob()->AsSecureSession()->SetMRPConfig(GetLocalMRPConfig());
+    ctx.GetSessionBobToAlice()->AsSecureSession()->SetMRPConfig(GetLocalMRPConfig());
     return SUCCESS;
 }
 

diff --git a/src/protocols/secure_channel/CASEServer.cpp b/src/protocols/secure_channel/CASEServer.cpp
@@ -72,7 +72,7 @@ CHIP_ERROR CASEServer::InitCASEHandshake(Messaging::ExchangeContext * ec)
 
     // Setup CASE state machine using the credentials for the current fabric.
     ReturnErrorOnFailure(GetSession().ListenForSessionEstablishment(
-        mSessionKeyId, mFabrics, this, Optional<ReliableMessageProtocolConfig>::Value(gDefaultMRPConfig)));
+        mSessionKeyId, mFabrics, this, Optional<ReliableMessageProtocolConfig>::Value(GetLocalMRPConfig())));
 
     // Hand over the exchange context to the CASE session.
     ec->SetDelegate(&GetSession());

diff --git a/src/transport/GroupSession.h b/src/transport/GroupSession.h
@@ -47,8 +47,9 @@ class GroupSession : public Session
 
     const ReliableMessageProtocolConfig & GetMRPConfig() const override
     {
+        static const ReliableMessageProtocolConfig cfg(GetLocalMRPConfig());
         VerifyOrDie(false);
-        return gDefaultMRPConfig;
+        return cfg;
     }
 
     System::Clock::Milliseconds32 GetAckTimeout() const override

diff --git a/src/transport/PairingSession.h b/src/transport/PairingSession.h
@@ -198,7 +198,7 @@ class DLL_EXPORT PairingSession
 
     Optional<uint16_t> mPeerSessionId;
 
-    ReliableMessageProtocolConfig mMRPConfig = gDefaultMRPConfig;
+    ReliableMessageProtocolConfig mMRPConfig = GetLocalMRPConfig();
 };
 
 } // namespace chip
diff --git a/src/transport/SessionManager.cpp b/src/transport/SessionManager.cpp
@@ -440,7 +440,7 @@ void SessionManager::RefreshSessionOperationalData(const SessionHandle & session
 void SessionManager::MessageDispatch(const PacketHeader & packetHeader, const Transport::PeerAddress & peerAddress,
                                      System::PacketBufferHandle && msg)
 {
-    Optional<SessionHandle> optionalSession = mUnauthenticatedSessions.FindOrAllocateEntry(peerAddress, gDefaultMRPConfig);
+    Optional<SessionHandle> optionalSession = mUnauthenticatedSessions.FindOrAllocateEntry(peerAddress, GetLocalMRPConfig());
     if (!optionalSession.HasValue())
     {
         ChipLogError(Inet, "UnauthenticatedSession exhausted");

diff --git a/src/transport/tests/TestPairingSession.cpp b/src/transport/tests/TestPairingSession.cpp
@@ -108,8 +108,8 @@ void PairingSessionTryDecodeMissingMRPParams(nlTestSuite * inSuite, void * inCon
     NL_TEST_ASSERT(inSuite, reader.Next() == CHIP_NO_ERROR);
     NL_TEST_ASSERT(inSuite, session.DecodeMRPParametersIfPresent(TLV::ContextTag(2), reader) == CHIP_NO_ERROR);
 
-    NL_TEST_ASSERT(inSuite, session.GetMRPConfig().mIdleRetransTimeout == gDefaultMRPConfig.mIdleRetransTimeout);
-    NL_TEST_ASSERT(inSuite, session.GetMRPConfig().mActiveRetransTimeout == gDefaultMRPConfig.mActiveRetransTimeout);
+    NL_TEST_ASSERT(inSuite, session.GetMRPConfig().mIdleRetransTimeout == GetLocalMRPConfig().mIdleRetransTimeout);
+    NL_TEST_ASSERT(inSuite, session.GetMRPConfig().mActiveRetransTimeout == GetLocalMRPConfig().mActiveRetransTimeout);
 }
 
 // Test Suite

diff --git a/src/transport/tests/TestPeerConnections.cpp b/src/transport/tests/TestPeerConnections.cpp
@@ -71,7 +71,7 @@ void TestBasicFunctionality(nlTestSuite * inSuite, void * inContext)
 
     // Node ID 1, peer key 1, local key 2
     auto optionalSession = connections.CreateNewSecureSession(kPeer1SessionType, 2, kPeer1NodeId, kPeer1CATs, 1,
-                                                              0 /* fabricIndex */, gDefaultMRPConfig);
+                                                              0 /* fabricIndex */, GetLocalMRPConfig());
     NL_TEST_ASSERT(inSuite, optionalSession.HasValue());
     NL_TEST_ASSERT(inSuite, optionalSession.Value()->AsSecureSession()->GetSecureSessionType() == kPeer1SessionType);
     NL_TEST_ASSERT(inSuite, optionalSession.Value()->AsSecureSession()->GetPeerNodeId() == kPeer1NodeId);
@@ -80,7 +80,7 @@ void TestBasicFunctionality(nlTestSuite * inSuite, void * inContext)
 
     // Node ID 2, peer key 3, local key 4
     optionalSession = connections.CreateNewSecureSession(kPeer2SessionType, 4, kPeer2NodeId, kPeer2CATs, 3, 0 /* fabricIndex */,
-                                                         gDefaultMRPConfig);
+                                                         GetLocalMRPConfig());
     NL_TEST_ASSERT(inSuite, optionalSession.HasValue());
     NL_TEST_ASSERT(inSuite, optionalSession.Value()->AsSecureSession()->GetSecureSessionType() == kPeer2SessionType);
     NL_TEST_ASSERT(inSuite, optionalSession.Value()->AsSecureSession()->GetPeerNodeId() == kPeer2NodeId);
@@ -90,7 +90,7 @@ void TestBasicFunctionality(nlTestSuite * inSuite, void * inContext)
 
     // Insufficient space for new connections. Object is max size 2
     optionalSession = connections.CreateNewSecureSession(kPeer3SessionType, 6, kPeer3NodeId, kPeer3CATs, 5, 0 /* fabricIndex */,
-                                                         gDefaultMRPConfig);
+                                                         GetLocalMRPConfig());
     NL_TEST_ASSERT(inSuite, !optionalSession.HasValue());
     System::Clock::Internal::SetSystemClockForTesting(realClock);
 }
@@ -104,15 +104,15 @@ void TestFindByKeyId(nlTestSuite * inSuite, void * inContext)
 
     // Node ID 1, peer key 1, local key 2
     auto optionalSession = connections.CreateNewSecureSession(kPeer1SessionType, 2, kPeer1NodeId, kPeer1CATs, 1,
-                                                              0 /* fabricIndex */, gDefaultMRPConfig);
+                                                              0 /* fabricIndex */, GetLocalMRPConfig());
     NL_TEST_ASSERT(inSuite, optionalSession.HasValue());
 
     NL_TEST_ASSERT(inSuite, !connections.FindSecureSessionByLocalKey(1).HasValue());
     NL_TEST_ASSERT(inSuite, connections.FindSecureSessionByLocalKey(2).HasValue());
 
     // Node ID 2, peer key 3, local key 4
     optionalSession = connections.CreateNewSecureSession(kPeer2SessionType, 4, kPeer2NodeId, kPeer2CATs, 3, 0 /* fabricIndex */,
-                                                         gDefaultMRPConfig);
+                                                         GetLocalMRPConfig());
     NL_TEST_ASSERT(inSuite, optionalSession.HasValue());
 
     NL_TEST_ASSERT(inSuite, !connections.FindSecureSessionByLocalKey(3).HasValue());
@@ -141,21 +141,21 @@ void TestExpireConnections(nlTestSuite * inSuite, void * inContext)
 
     // Node ID 1, peer key 1, local key 2
     auto optionalSession = connections.CreateNewSecureSession(kPeer1SessionType, 2, kPeer1NodeId, kPeer1CATs, 1,
-                                                              0 /* fabricIndex */, gDefaultMRPConfig);
+                                                              0 /* fabricIndex */, GetLocalMRPConfig());
     NL_TEST_ASSERT(inSuite, optionalSession.HasValue());
     optionalSession.Value()->AsSecureSession()->SetPeerAddress(kPeer1Addr);
 
     clock.SetMonotonic(200_ms64);
     // Node ID 2, peer key 3, local key 4
     optionalSession = connections.CreateNewSecureSession(kPeer2SessionType, 4, kPeer2NodeId, kPeer2CATs, 3, 0 /* fabricIndex */,
-                                                         gDefaultMRPConfig);
+                                                         GetLocalMRPConfig());
     NL_TEST_ASSERT(inSuite, optionalSession.HasValue());
     optionalSession.Value()->AsSecureSession()->SetPeerAddress(kPeer2Addr);
 
     // cannot add before expiry
     clock.SetMonotonic(300_ms64);
     optionalSession = connections.CreateNewSecureSession(kPeer3SessionType, 6, kPeer3NodeId, kPeer3CATs, 5, 0 /* fabricIndex */,
-                                                         gDefaultMRPConfig);
+                                                         GetLocalMRPConfig());
     NL_TEST_ASSERT(inSuite, !optionalSession.HasValue());
 
     // at time 300, this expires ip addr 1
@@ -173,7 +173,7 @@ void TestExpireConnections(nlTestSuite * inSuite, void * inContext)
     clock.SetMonotonic(300_ms64);
     // Node ID 3, peer key 5, local key 6
     optionalSession = connections.CreateNewSecureSession(kPeer3SessionType, 6, kPeer3NodeId, kPeer3CATs, 5, 0 /* fabricIndex */,
-                                                         gDefaultMRPConfig);
+                                                         GetLocalMRPConfig());
     NL_TEST_ASSERT(inSuite, optionalSession.HasValue());
     optionalSession.Value()->AsSecureSession()->SetPeerAddress(kPeer3Addr);
 
@@ -206,7 +206,7 @@ void TestExpireConnections(nlTestSuite * inSuite, void * inContext)
 
     // Node ID 1, peer key 1, local key 2
     optionalSession = connections.CreateNewSecureSession(kPeer1SessionType, 2, kPeer1NodeId, kPeer1CATs, 1, 0 /* fabricIndex */,
-                                                         gDefaultMRPConfig);
+                                                         GetLocalMRPConfig());
     NL_TEST_ASSERT(inSuite, optionalSession.HasValue());
     NL_TEST_ASSERT(inSuite, connections.FindSecureSessionByLocalKey(2).HasValue());
     NL_TEST_ASSERT(inSuite, connections.FindSecureSessionByLocalKey(4).HasValue());