From 1486028b6b99d61cece3f12247236d693aee4553 Mon Sep 17 00:00:00 2001 From: Jean-Francois Penven <67962328+jepenven-silabs@users.noreply.github.com> Date: Tue, 21 Nov 2023 10:13:51 -0500 Subject: [PATCH] [ICD] Add Keep active during MDNS resolution ICD Check-In Sender (#30552) * Fix 30492 * apply comments * fix last comments --- src/app/FailSafeContext.cpp | 4 +- src/app/icd/BUILD.gn | 1 + src/app/icd/ICDCheckInSender.cpp | 19 +++--- src/app/icd/ICDManager.cpp | 60 +++++++++++++------ src/app/icd/ICDManager.h | 6 +- src/app/icd/ICDNotifier.h | 15 ++++- src/app/server/CommissioningWindowManager.cpp | 2 +- src/app/tests/TestICDManager.cpp | 2 +- src/messaging/ExchangeContext.cpp | 4 +- src/messaging/ReliableMessageMgr.cpp | 3 +- 10 files changed, 77 insertions(+), 39 deletions(-) diff --git a/src/app/FailSafeContext.cpp b/src/app/FailSafeContext.cpp index 2d886b17b34bc0..647401e7f6b5d4 100644 --- a/src/app/FailSafeContext.cpp +++ b/src/app/FailSafeContext.cpp @@ -56,9 +56,7 @@ void FailSafeContext::SetFailSafeArmed(bool armed) #if CHIP_CONFIG_ENABLE_ICD_SERVER if (IsFailSafeArmed() != armed) { - ICDListener::KeepActiveFlags activeRequest = ICDListener::KeepActiveFlags::kFailSafeArmed; - armed ? ICDNotifier::GetInstance().BroadcastActiveRequestNotification(activeRequest) - : ICDNotifier::GetInstance().BroadcastActiveRequestWithdrawal(activeRequest); + ICDNotifier::GetInstance().BroadcastActiveRequest(ICDListener::KeepActiveFlag::kFailSafeArmed, armed); } #endif mFailSafeArmed = armed; diff --git a/src/app/icd/BUILD.gn b/src/app/icd/BUILD.gn index 8b8f10fd5c1587..99e8a23b0b699f 100644 --- a/src/app/icd/BUILD.gn +++ b/src/app/icd/BUILD.gn @@ -55,6 +55,7 @@ source_set("sender") { public_deps = [ ":cluster", + ":notifier", "${chip_root}/src/credentials:credentials", "${chip_root}/src/lib/address_resolve:address_resolve", "${chip_root}/src/protocols/secure_channel", diff --git a/src/app/icd/ICDCheckInSender.cpp b/src/app/icd/ICDCheckInSender.cpp index bd2b86bb24fdd3..25637ea1d821dc 100644 --- a/src/app/icd/ICDCheckInSender.cpp +++ b/src/app/icd/ICDCheckInSender.cpp @@ -17,6 +17,8 @@ #include "ICDCheckInSender.h" +#include "ICDNotifier.h" + #include #include @@ -37,15 +39,17 @@ ICDCheckInSender::ICDCheckInSender(Messaging::ExchangeManager * exchangeManager) void ICDCheckInSender::OnNodeAddressResolved(const PeerId & peerId, const AddressResolve::ResolveResult & result) { - mResolveInProgress = false; + if (CHIP_NO_ERROR != SendCheckInMsg(result.address)) + { + ChipLogError(AppServer, "Failed to send the ICD Check-In message"); + } - VerifyOrReturn(CHIP_NO_ERROR != SendCheckInMsg(result.address), - ChipLogError(AppServer, "Failed to send the ICD Check-In message")); + ICDNotifier::GetInstance().BroadcastActiveRequestWithdrawal(ICDListener::KeepActiveFlag::kCheckInInProgress); } void ICDCheckInSender::OnNodeAddressResolutionFailed(const PeerId & peerId, CHIP_ERROR reason) { - mResolveInProgress = false; + ICDNotifier::GetInstance().BroadcastActiveRequestWithdrawal(ICDListener::KeepActiveFlag::kCheckInInProgress); ChipLogProgress(AppServer, "Node Address resolution failed for ICD Check-In with Node ID " ChipLogFormatX64, ChipLogValueX64(peerId.GetNodeId())); } @@ -55,12 +59,13 @@ CHIP_ERROR ICDCheckInSender::SendCheckInMsg(const Transport::PeerAddress & addr) System::PacketBufferHandle buffer = MessagePacketBuffer::New(CheckinMessage::sMinPayloadSize); VerifyOrReturnError(!buffer.IsNull(), CHIP_ERROR_NO_MEMORY); - MutableByteSpan output{ buffer->Start(), buffer->DataLength() }; + MutableByteSpan output{ buffer->Start(), buffer->MaxDataLength() }; // TODO retrieve Check-in counter CounterType counter = 0; ReturnErrorOnFailure(CheckinMessage::GenerateCheckinMessagePayload(mKey, counter, ByteSpan(), output)); + buffer->SetDataLength(static_cast(output.size())); VerifyOrReturnError(mExchangeManager->GetSessionManager() != nullptr, CHIP_ERROR_INTERNAL); @@ -88,13 +93,11 @@ CHIP_ERROR ICDCheckInSender::RequestResolve(ICDMonitoringEntry & entry, FabricTa memcpy(mKey.AsMutable(), entry.key.As(), sizeof(Crypto::Aes128KeyByteArray)); - // TODO #30492 - // Device must stay active during MDNS resolution CHIP_ERROR err = AddressResolve::Resolver::Instance().LookupNode(request, mAddressLookupHandle); if (err == CHIP_NO_ERROR) { - mResolveInProgress = true; + ICDNotifier::GetInstance().BroadcastActiveRequestNotification(ICDListener::KeepActiveFlag::kCheckInInProgress); } return err; diff --git a/src/app/icd/ICDManager.cpp b/src/app/icd/ICDManager.cpp index 3d75b0d4dcf74c..fd744a25764402 100644 --- a/src/app/icd/ICDManager.cpp +++ b/src/app/icd/ICDManager.cpp @@ -40,9 +40,8 @@ using namespace chip::app; using namespace chip::app::Clusters; using namespace chip::app::Clusters::IcdManagement; -uint8_t ICDManager::OpenExchangeContextCount = 0; static_assert(UINT8_MAX >= CHIP_CONFIG_MAX_EXCHANGE_CONTEXTS, - "ICDManager::OpenExchangeContextCount cannot hold count for the max exchange count"); + "ICDManager::mOpenExchangeContextCount cannot hold count for the max exchange count"); void ICDManager::Init(PersistentStorageDelegate * storage, FabricTable * fabricTable, Crypto::SymmetricKeystore * symmetricKeystore) { @@ -98,7 +97,6 @@ bool ICDManager::SupportsFeature(Feature feature) bool success = (Attributes::FeatureMap::Get(kRootEndpointId, &featureMap) == EMBER_ZCL_STATUS_SUCCESS); return success ? ((featureMap & to_underlying(feature)) != 0) : false; #else - return ((mFeatureMap & to_underlying(feature)) != 0); #endif // !CONFIG_BUILD_FOR_HOST_UNIT_TEST } @@ -171,7 +169,7 @@ void ICDManager::UpdateOperationState(OperationalState state) CHIP_ERROR err = DeviceLayer::ConnectivityMgr().SetPollingInterval(slowPollInterval); if (err != CHIP_NO_ERROR) { - ChipLogError(AppServer, "Failed to set Polling Interval: err %" CHIP_ERROR_FORMAT, err.Format()); + ChipLogError(AppServer, "Failed to set Slow Polling Interval: err %" CHIP_ERROR_FORMAT, err.Format()); } } else if (state == OperationalState::ActiveMode) @@ -201,7 +199,7 @@ void ICDManager::UpdateOperationState(OperationalState state) CHIP_ERROR err = DeviceLayer::ConnectivityMgr().SetPollingInterval(GetFastPollingInterval()); if (err != CHIP_NO_ERROR) { - ChipLogError(AppServer, "Failed to set Polling Interval: err %" CHIP_ERROR_FORMAT, err.Format()); + ChipLogError(AppServer, "Failed to set Fast Polling Interval: err %" CHIP_ERROR_FORMAT, err.Format()); } postObserverEvent(ObserverEventType::EnterActiveMode); @@ -274,44 +272,70 @@ void ICDManager::OnKeepActiveRequest(KeepActiveFlags request) { assertChipStackLockedByCurrentThread(); - if (request == KeepActiveFlags::kExchangeContextOpen) + VerifyOrReturn(request < KeepActiveFlagsValues::kInvalidFlag); + + if (request.Has(KeepActiveFlag::kExchangeContextOpen)) { // There can be multiple open exchange contexts at the same time. // Keep track of the requests count. - this->OpenExchangeContextCount++; - this->SetKeepActiveModeRequirements(request, true /* state */); + this->mOpenExchangeContextCount++; } - else /* !kExchangeContextOpen */ + + if (request.Has(KeepActiveFlag::kCheckInInProgress)) { - // Only 1 request per type (kCommissioningWindowOpen, kFailSafeArmed) - // set requirement directly - this->SetKeepActiveModeRequirements(request, true /* state */); + // There can be multiple check-in at the same time. + // Keep track of the requests count. + this->mCheckInRequestCount++; } + + this->SetKeepActiveModeRequirements(request, true /* state */); } void ICDManager::OnActiveRequestWithdrawal(KeepActiveFlags request) { assertChipStackLockedByCurrentThread(); - if (request == KeepActiveFlags::kExchangeContextOpen) + VerifyOrReturn(request < KeepActiveFlagsValues::kInvalidFlag); + + if (request.Has(KeepActiveFlag::kExchangeContextOpen)) { // There can be multiple open exchange contexts at the same time. // Keep track of the requests count. - if (this->OpenExchangeContextCount > 0) + if (this->mOpenExchangeContextCount > 0) { - this->OpenExchangeContextCount--; + this->mOpenExchangeContextCount--; } else { ChipLogError(DeviceLayer, "The ICD Manager did not account for ExchangeContext closure"); } - if (this->OpenExchangeContextCount == 0) + if (this->mOpenExchangeContextCount == 0) { - this->SetKeepActiveModeRequirements(request, false /* state */); + this->SetKeepActiveModeRequirements(KeepActiveFlag::kExchangeContextOpen, false /* state */); } } - else /* !kExchangeContextOpen */ + + if (request.Has(KeepActiveFlag::kCheckInInProgress)) + { + // There can be multiple open exchange contexts at the same time. + // Keep track of the requests count. + if (this->mCheckInRequestCount > 0) + { + this->mCheckInRequestCount--; + } + else + { + ChipLogError(DeviceLayer, "The ICD Manager did not account for Check-In Sender start"); + } + + if (this->mCheckInRequestCount == 0) + { + this->SetKeepActiveModeRequirements(KeepActiveFlag::kCheckInInProgress, false /* state */); + } + } + + if (request.Has(KeepActiveFlag::kCommissioningWindowOpen) || request.Has(KeepActiveFlag::kFailSafeArmed)) { // Only 1 request per type (kCommissioningWindowOpen, kFailSafeArmed) // remove requirement directly diff --git a/src/app/icd/ICDManager.h b/src/app/icd/ICDManager.h index 75001f8f71e0ca..573a20011f7ffc 100644 --- a/src/app/icd/ICDManager.h +++ b/src/app/icd/ICDManager.h @@ -118,6 +118,7 @@ class ICDManager : public ICDListener static void OnIdleModeDone(System::Layer * aLayer, void * appState); static void OnActiveModeDone(System::Layer * aLayer, void * appState); + /** * @brief Callback function called shortly before the device enters idle mode to allow checks to be made. This is currently only * called once to prevent entering in a loop if some events re-trigger this check (for instance if a check for subscription @@ -126,7 +127,8 @@ class ICDManager : public ICDListener */ static void OnTransitionToIdle(System::Layer * aLayer, void * appState); - static uint8_t OpenExchangeContextCount; + uint8_t mOpenExchangeContextCount = 0; + uint8_t mCheckInRequestCount = 0; private: // SIT ICDs should have a SlowPollingThreshold shorter than or equal to 15s (spec 9.16.1.5) @@ -139,7 +141,7 @@ class ICDManager : public ICDListener static constexpr uint32_t kMinActiveModeDuration = 300; static constexpr uint16_t kMinActiveModeThreshold = 300; - BitFlags mKeepActiveFlags{ 0 }; + KeepActiveFlags mKeepActiveFlags{ 0 }; // Initialize mOperationalState to ActiveMode so the init sequence at bootup triggers the IdleMode behaviour first. OperationalState mOperationalState = OperationalState::ActiveMode; diff --git a/src/app/icd/ICDNotifier.h b/src/app/icd/ICDNotifier.h index 9f6032c8616903..4344b72a1d3ac3 100644 --- a/src/app/icd/ICDNotifier.h +++ b/src/app/icd/ICDNotifier.h @@ -18,6 +18,7 @@ #include #include +#include class ICDListener; @@ -38,11 +39,13 @@ static_assert(ICD_MAX_NOTIFICATION_SUBSCRIBERS > 0, "At least 1 Subscriber is re class ICDListener { public: - enum class KeepActiveFlags : uint8_t + enum class KeepActiveFlagsValues : uint8_t { kCommissioningWindowOpen = 0x01, kFailSafeArmed = 0x02, - kExchangeContextOpen = 0x03, + kExchangeContextOpen = 0x04, + kCheckInInProgress = 0x08, + kInvalidFlag = 0x10, // Move up when adding more flags }; enum class ICDManagementEvents : uint8_t @@ -51,6 +54,9 @@ class ICDListener kStayActiveRequestReceived = 0x02, }; + using KeepActiveFlags = BitFlags; + using KeepActiveFlag = KeepActiveFlagsValues; + virtual ~ICDListener() {} /** @@ -101,6 +107,11 @@ class ICDNotifier void BroadcastActiveRequestWithdrawal(ICDListener::KeepActiveFlags request); void BroadcastICDManagementEvent(ICDListener::ICDManagementEvents event); + inline void BroadcastActiveRequest(ICDListener::KeepActiveFlags request, bool notify) + { + (notify) ? BroadcastActiveRequestNotification(request) : BroadcastActiveRequestWithdrawal(request); + } + static ICDNotifier & GetInstance() { return sICDNotifier; } private: diff --git a/src/app/server/CommissioningWindowManager.cpp b/src/app/server/CommissioningWindowManager.cpp index 33eaeedb1eb619..cfa201c57a3757 100644 --- a/src/app/server/CommissioningWindowManager.cpp +++ b/src/app/server/CommissioningWindowManager.cpp @@ -544,7 +544,7 @@ void CommissioningWindowManager::UpdateWindowStatus(CommissioningWindowStatusEnu { mWindowStatus = aNewStatus; #if CHIP_CONFIG_ENABLE_ICD_SERVER - app::ICDListener::KeepActiveFlags request = app::ICDListener::KeepActiveFlags::kCommissioningWindowOpen; + app::ICDListener::KeepActiveFlags request = app::ICDListener::KeepActiveFlag::kCommissioningWindowOpen; if (mWindowStatus != CommissioningWindowStatusEnum::kWindowNotOpen) { app::ICDNotifier::GetInstance().BroadcastActiveRequestNotification(request); diff --git a/src/app/tests/TestICDManager.cpp b/src/app/tests/TestICDManager.cpp index 7e47996dff78fa..ed6ecb7dc625b4 100644 --- a/src/app/tests/TestICDManager.cpp +++ b/src/app/tests/TestICDManager.cpp @@ -165,7 +165,7 @@ class TestICDManager static void TestKeepActivemodeRequests(nlTestSuite * aSuite, void * aContext) { TestContext * ctx = static_cast(aContext); - typedef ICDListener::KeepActiveFlags ActiveFlag; + typedef ICDListener::KeepActiveFlag ActiveFlag; ICDNotifier notifier = ICDNotifier::GetInstance(); // Setting a requirement will transition the ICD to active mode. diff --git a/src/messaging/ExchangeContext.cpp b/src/messaging/ExchangeContext.cpp index 1b93bff5a62e62..c9d912daf88091 100644 --- a/src/messaging/ExchangeContext.cpp +++ b/src/messaging/ExchangeContext.cpp @@ -326,7 +326,7 @@ ExchangeContext::ExchangeContext(ExchangeManager * em, uint16_t ExchangeId, cons SetAutoRequestAck(!session->IsGroupSession()); #if CHIP_CONFIG_ENABLE_ICD_SERVER - app::ICDNotifier::GetInstance().BroadcastActiveRequestNotification(app::ICDListener::KeepActiveFlags::kExchangeContextOpen); + app::ICDNotifier::GetInstance().BroadcastActiveRequestNotification(app::ICDListener::KeepActiveFlag::kExchangeContextOpen); #endif #if defined(CHIP_EXCHANGE_CONTEXT_DETAIL_LOGGING) @@ -345,7 +345,7 @@ ExchangeContext::~ExchangeContext() VerifyOrDie(mFlags.Has(Flags::kFlagClosed)); #if CHIP_CONFIG_ENABLE_ICD_SERVER - app::ICDNotifier::GetInstance().BroadcastActiveRequestWithdrawal(app::ICDListener::KeepActiveFlags::kExchangeContextOpen); + app::ICDNotifier::GetInstance().BroadcastActiveRequestWithdrawal(app::ICDListener::KeepActiveFlag::kExchangeContextOpen); #endif // CHIP_CONFIG_ENABLE_ICD_SERVER // Ideally, in this scenario, the retransmit table should diff --git a/src/messaging/ReliableMessageMgr.cpp b/src/messaging/ReliableMessageMgr.cpp index 53d0487b7b826b..4ddf1801da0ade 100644 --- a/src/messaging/ReliableMessageMgr.cpp +++ b/src/messaging/ReliableMessageMgr.cpp @@ -38,7 +38,6 @@ #include #if CHIP_CONFIG_ENABLE_ICD_SERVER -#include // nogncheck #include // nogncheck #endif @@ -260,7 +259,7 @@ System::Clock::Timestamp ReliableMessageMgr::GetBackoff(System::Clock::Timestamp // Implement: // "An ICD sender SHOULD increase t to also account for its own sleepy interval // required to receive the acknowledgment" - mrpBackoffTime += app::ICDManager::GetFastPollingInterval(); + mrpBackoffTime += CHIP_DEVICE_CONFIG_ICD_FAST_POLL_INTERVAL; #endif mrpBackoffTime += CHIP_CONFIG_MRP_RETRY_INTERVAL_SENDER_BOOST;