[SED] Review switching between polling modes (#14418)

* [SED] Review switching between polling modes Sleepy End Device polling mode switching behaves incorrectly in some scenarios. Request fast-polling mode when commissioning window over IP (not BLE) is open. Refactor the switching code in ExchangeContext to make sure that a single exchange can request fast-polling mode only once and that the request is always withdrawn. Also, request the fast-polling while waiting for ACK. * Review comment
project-chip · Jul 21, 2023 · ad20bdb · ad20bdb
1 parent 4648451
commit ad20bdb
Show file tree

Hide file tree

Showing 4 changed files with 80 additions and 33 deletions.
diff --git a/src/app/server/CommissioningWindowManager.cpp b/src/app/server/CommissioningWindowManager.cpp
@@ -22,6 +22,8 @@
 #include <lib/support/CodeUtils.h>
 #include <platform/CHIPDeviceLayer.h>
 
+using namespace chip::app::Clusters;
+
 namespace {
 
 // As per specifications (Section 13.3), Nodes SHALL exit commissioning mode after 20 failed commission attempts.
@@ -259,7 +261,7 @@ CHIP_ERROR CommissioningWindowManager::OpenEnhancedCommissioningWindow(uint16_t
 
 void CommissioningWindowManager::CloseCommissioningWindow()
 {
-    if (mWindowStatus != app::Clusters::AdministratorCommissioning::CommissioningWindowStatus::kWindowNotOpen)
+    if (mWindowStatus != AdministratorCommissioning::CommissioningWindowStatus::kWindowNotOpen)
     {
         ChipLogProgress(AppServer, "Closing pairing window");
         Cleanup();
@@ -273,10 +275,18 @@ CHIP_ERROR CommissioningWindowManager::StartAdvertisement()
     DeviceLayer::ConfigurationMgr().NotifyOfAdvertisementStart();
 #endif
 
+#if CHIP_DEVICE_CONFIG_ENABLE_SED
+    if (!mIsBLE && mWindowStatus == AdministratorCommissioning::CommissioningWindowStatus::kWindowNotOpen)
+    {
+        DeviceLayer::ConnectivityMgr().RequestSEDFastPollingMode(true);
+    }
+#endif
+
     if (mIsBLE)
     {
         ReturnErrorOnFailure(chip::DeviceLayer::ConnectivityMgr().SetBLEAdvertisingEnabled(true));
     }
+
     if (mAppDelegate != nullptr)
     {
         mAppDelegate->OnPairingWindowOpened();
@@ -285,22 +295,18 @@ CHIP_ERROR CommissioningWindowManager::StartAdvertisement()
     Dnssd::CommissioningMode commissioningMode;
     if (mUseECM)
     {
-        mWindowStatus     = app::Clusters::AdministratorCommissioning::CommissioningWindowStatus::kEnhancedWindowOpen;
+        mWindowStatus     = AdministratorCommissioning::CommissioningWindowStatus::kEnhancedWindowOpen;
         commissioningMode = Dnssd::CommissioningMode::kEnabledEnhanced;
     }
     else
     {
-        mWindowStatus     = app::Clusters::AdministratorCommissioning::CommissioningWindowStatus::kBasicWindowOpen;
+        mWindowStatus     = AdministratorCommissioning::CommissioningWindowStatus::kBasicWindowOpen;
         commissioningMode = Dnssd::CommissioningMode::kEnabledBasic;
     }
 
     // reset all advertising, indicating we are in commissioningMode
     app::DnssdServer::Instance().StartServer(commissioningMode);
 
-#if CHIP_DEVICE_CONFIG_ENABLE_SED
-    DeviceLayer::ConnectivityMgr().RequestSEDFastPollingMode(true);
-#endif
-
     return CHIP_NO_ERROR;
 }
 
@@ -311,12 +317,15 @@ CHIP_ERROR CommissioningWindowManager::StopAdvertisement(bool aShuttingDown)
     mServer->GetExchangeManager().UnregisterUnsolicitedMessageHandlerForType(Protocols::SecureChannel::MsgType::PBKDFParamRequest);
     mPairingSession.Clear();
 
-    mWindowStatus = app::Clusters::AdministratorCommissioning::CommissioningWindowStatus::kWindowNotOpen;
-
 #if CHIP_DEVICE_CONFIG_ENABLE_SED
-    DeviceLayer::ConnectivityMgr().RequestSEDFastPollingMode(false);
+    if (!mIsBLE && mWindowStatus != AdministratorCommissioning::CommissioningWindowStatus::kWindowNotOpen)
+    {
+        DeviceLayer::ConnectivityMgr().RequestSEDFastPollingMode(false);
+    }
 #endif
 
+    mWindowStatus = AdministratorCommissioning::CommissioningWindowStatus::kWindowNotOpen;
+
     // If aShuttingDown, don't try to change our DNS-SD advertisements.
     if (!aShuttingDown)
     {

diff --git a/src/messaging/ExchangeContext.cpp b/src/messaging/ExchangeContext.cpp
@@ -87,24 +87,30 @@ void ExchangeContext::SetResponseTimeout(Timeout timeout)
 #if CONFIG_DEVICE_LAYER && CHIP_DEVICE_CONFIG_ENABLE_SED
 void ExchangeContext::UpdateSEDPollingMode()
 {
-    SessionHandle sessionHandle              = GetSessionHandle();
-    Transport::Session::SessionType sessType = sessionHandle->GetSessionType();
+    Transport::PeerAddress address;
 
-    // During PASE session, which happen on BLE, the session is kUnauthenticated
-    // So AsSecureSession() ends up faulting the system
-    if (sessType != Transport::Session::SessionType::kUnauthenticated)
+    switch (GetSessionHandle()->GetSessionType())
     {
-        if (sessionHandle->AsSecureSession()->GetPeerAddress().GetTransportType() != Transport::Type::kBle)
-        {
-            if (!IsResponseExpected() && !IsSendExpected() && (mExchangeMgr->GetNumActiveExchanges() == 1))
-            {
-                chip::DeviceLayer::ConnectivityMgr().RequestSEDFastPollingMode(false);
-            }
-            else
-            {
-                chip::DeviceLayer::ConnectivityMgr().RequestSEDFastPollingMode(true);
-            }
-        }
+    case Transport::Session::SessionType::kSecure:
+        address = GetSessionHandle()->AsSecureSession()->GetPeerAddress();
+        break;
+    case Transport::Session::SessionType::kUnauthenticated:
+        address = GetSessionHandle()->AsUnauthenticatedSession()->GetPeerAddress();
+        break;
+    default:
+        return;
+    }
+
+    VerifyOrReturn(address.GetTransportType() != Transport::Type::kBle);
+    UpdateSEDPollingMode(IsResponseExpected() || IsSendExpected() || IsMessageNotAcked());
+}
+
+void ExchangeContext::UpdateSEDPollingMode(bool fastPollingMode)
+{
+    if (fastPollingMode != IsRequestingFastPollingMode())
+    {
+        SetRequestingFastPollingMode(fastPollingMode);
+        DeviceLayer::ConnectivityMgr().RequestSEDFastPollingMode(fastPollingMode);
     }
 }
 #endif
@@ -287,6 +293,11 @@ ExchangeContext::~ExchangeContext()
     VerifyOrDie(mExchangeMgr != nullptr && GetReferenceCount() == 0);
     VerifyOrDie(!IsAckPending());
 
+#if CONFIG_DEVICE_LAYER && CHIP_DEVICE_CONFIG_ENABLE_SED
+    // Make sure that the exchange withdraws the request for Sleepy End Device fast-polling mode.
+    UpdateSEDPollingMode(false);
+#endif
+
     // Ideally, in this scenario, the retransmit table should
     // be clear of any outstanding messages for this context and
     // the boolean parameter passed to DoClose() should not matter.

diff --git a/src/messaging/ExchangeContext.h b/src/messaging/ExchangeContext.h
@@ -245,16 +245,23 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext,
     void MessageHandled();
 
     /**
-     * Updates Sleepy End Device polling interval in the following way:
+     * Updates Sleepy End Device polling mode in the following way:
      * - does nothing for exchanges over Bluetooth LE
-     * - set IDLE polling mode if all conditions are met:
-     *   - device doesn't expect getting response nor sending message
-     *   - there is no other active exchange than the current one
-     * - set ACTIVE polling mode if any of the conditions is met:
-     *   - device expects getting response or sending message
-     *   - there is another active exchange
+     * - requests fast-polling (active) mode if there are more messages,
+     *   including MRP acknowledgements, expected to be sent or received on
+     *   this exchange.
+     * - withdraws the request for fast-polling (active) mode, otherwise.
      */
     void UpdateSEDPollingMode();
+
+    /**
+     * Requests or withdraws the request for Sleepy End Device fast-polling mode
+     * based on the argument value.
+     *
+     * Note that the device switches to the slow-polling (idle) mode if no
+     * exchange nor other component requests the fast-polling mode.
+     */
+    void UpdateSEDPollingMode(bool fastPollingMode);
 };
 
 } // namespace Messaging

diff --git a/src/messaging/ReliableMessageContext.h b/src/messaging/ReliableMessageContext.h
@@ -153,6 +153,12 @@ class ReliableMessageContext
     /// Set whether there is a message hasn't been acknowledged.
     void SetMessageNotAcked(bool messageNotAcked);
 
+    /// Set if this exchange is requesting Sleepy End Device fast-polling mode
+    void SetRequestingFastPollingMode(bool fastPollingMode);
+
+    /// Determine whether this exchange is requesting Sleepy End Device fast-polling mode
+    bool IsRequestingFastPollingMode() const;
+
     /**
      * Get the reliable message manager that corresponds to this reliable
      * message context.
@@ -194,8 +200,12 @@ class ReliableMessageContext
 
         /// When set, signifies that we are currently in the middle of HandleMessage.
         kFlagHandlingMessage = (1u << 9),
+
         /// When set, we have had Close() or Abort() called on us already.
         kFlagClosed = (1u << 10),
+
+        /// When set, signifies that the exchange is requesting Sleepy End Device fast-polling mode.
+        kFlagFastPollingMode = (1u << 11),
     };
 
     BitFlags<Flags> mFlags; // Internal state flags
@@ -258,6 +268,11 @@ inline bool ReliableMessageContext::HasPiggybackAckPending() const
     return mFlags.Has(Flags::kFlagAckMessageCounterIsValid);
 }
 
+inline bool ReliableMessageContext::IsRequestingFastPollingMode() const
+{
+    return mFlags.Has(Flags::kFlagFastPollingMode);
+}
+
 inline void ReliableMessageContext::SetAutoRequestAck(bool autoReqAck)
 {
     mFlags.Set(Flags::kFlagAutoRequestAck, autoReqAck);
@@ -283,5 +298,10 @@ inline void ReliableMessageContext::SetMessageNotAcked(bool messageNotAcked)
     mFlags.Set(Flags::kFlagMesageNotAcked, messageNotAcked);
 }
 
+inline void ReliableMessageContext::SetRequestingFastPollingMode(bool fastPollingMode)
+{
+    mFlags.Set(Flags::kFlagFastPollingMode, fastPollingMode);
+}
+
 } // namespace Messaging
 } // namespace chip