From ad20bdba355591a814c4e300d42d7929489389f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damian=20Kr=C3=B3lik?= <66667989+Damian-Nordic@users.noreply.github.com> Date: Thu, 3 Feb 2022 14:23:55 +0100 Subject: [PATCH] [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 --- src/app/server/CommissioningWindowManager.cpp | 29 ++++++++----- src/messaging/ExchangeContext.cpp | 43 ++++++++++++------- src/messaging/ExchangeContext.h | 21 ++++++--- src/messaging/ReliableMessageContext.h | 20 +++++++++ 4 files changed, 80 insertions(+), 33 deletions(-) diff --git a/src/app/server/CommissioningWindowManager.cpp b/src/app/server/CommissioningWindowManager.cpp index d342a47202df9c..1ce4658343dcfd 100644 --- a/src/app/server/CommissioningWindowManager.cpp +++ b/src/app/server/CommissioningWindowManager.cpp @@ -22,6 +22,8 @@ #include #include +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 index 20d308a53ad936..1bcc0579e621d3 100644 --- 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 index c5ec80a44b7200..23489ec733cc96 100644 --- 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 index ed4a05b0657afd..79d9b4f3cd7043 100644 --- 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 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