From 41366a92b4a4018a555036920c4eb90ebc088a86 Mon Sep 17 00:00:00 2001 From: Jean-Francois Penven Date: Fri, 17 Nov 2023 08:20:20 -0500 Subject: [PATCH 1/3] Fix 30492 --- src/app/FailSafeContext.cpp | 4 +- src/app/icd/BUILD.gn | 1 + src/app/icd/ICDCheckInSender.cpp | 19 ++++--- src/app/icd/ICDManager.cpp | 50 ++++++++++++++----- src/app/icd/ICDManager.h | 4 +- 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, 72 insertions(+), 32 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 65aca79acda778..f77494f78547f6 100644 --- a/src/app/icd/ICDManager.cpp +++ b/src/app/icd/ICDManager.cpp @@ -41,6 +41,7 @@ using namespace chip::app::Clusters; using namespace chip::app::Clusters::IcdManagement; uint8_t ICDManager::OpenExchangeContextCount = 0; +uint8_t ICDManager::CheckInRequestCount = 0; static_assert(UINT8_MAX >= CHIP_CONFIG_MAX_EXCHANGE_CONTEXTS, "ICDManager::OpenExchangeContextCount cannot hold count for the max exchange count"); @@ -98,7 +99,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 +171,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 +201,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,26 +274,32 @@ 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 */); } - 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->CheckInRequestCount++; } + + 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. @@ -308,10 +314,30 @@ void ICDManager::OnActiveRequestWithdrawal(KeepActiveFlags request) if (this->OpenExchangeContextCount == 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->CheckInRequestCount > 0) + { + this->CheckInRequestCount--; + } + else + { + ChipLogError(DeviceLayer, "The ICD Manager did not account for Check-In Sender start"); + } + + if (this->CheckInRequestCount == 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 29ff0722c7e54d..9611d8fbaaa49b 100644 --- a/src/app/icd/ICDManager.h +++ b/src/app/icd/ICDManager.h @@ -119,6 +119,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 @@ -128,6 +129,7 @@ class ICDManager : public ICDListener static void OnTransitionToIdle(System::Layer * aLayer, void * appState); static uint8_t OpenExchangeContextCount; + static uint8_t CheckInRequestCount; private: // SIT ICDs should have a SlowPollingThreshold shorter than or equal to 15s (spec 9.16.1.5) @@ -140,7 +142,7 @@ class ICDManager : public ICDListener static constexpr uint32_t kMinActiveModeDuration = 300; static constexpr uint16_t kMinActiveModeThreshold = 300; - BitFlags mKeepActiveFlags{ 0 }; + KeepActiveFlags mKeepActiveFlags{ 0 }; OperationalState mOperationalState = OperationalState::IdleMode; ICDMode mICDMode = ICDMode::SIT; 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 693d1bc5630f73..78c5b321766b57 100644 --- a/src/app/tests/TestICDManager.cpp +++ b/src/app/tests/TestICDManager.cpp @@ -163,7 +163,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 e22bd4db24a517..f50ed85d83d3b1 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 @@ -259,7 +258,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; From b317ed0705b0bb0f14508c2148610cc346f4c8ff Mon Sep 17 00:00:00 2001 From: Jean-Francois Penven Date: Fri, 17 Nov 2023 12:41:59 -0500 Subject: [PATCH 2/3] apply comments --- src/app/icd/ICDManager.cpp | 2 -- src/app/icd/ICDManager.h | 4 ++-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/app/icd/ICDManager.cpp b/src/app/icd/ICDManager.cpp index f77494f78547f6..f36397be6c2465 100644 --- a/src/app/icd/ICDManager.cpp +++ b/src/app/icd/ICDManager.cpp @@ -40,8 +40,6 @@ using namespace chip::app; using namespace chip::app::Clusters; using namespace chip::app::Clusters::IcdManagement; -uint8_t ICDManager::OpenExchangeContextCount = 0; -uint8_t ICDManager::CheckInRequestCount = 0; static_assert(UINT8_MAX >= CHIP_CONFIG_MAX_EXCHANGE_CONTEXTS, "ICDManager::OpenExchangeContextCount cannot hold count for the max exchange count"); diff --git a/src/app/icd/ICDManager.h b/src/app/icd/ICDManager.h index 9611d8fbaaa49b..fc667f14210df5 100644 --- a/src/app/icd/ICDManager.h +++ b/src/app/icd/ICDManager.h @@ -128,8 +128,8 @@ class ICDManager : public ICDListener */ static void OnTransitionToIdle(System::Layer * aLayer, void * appState); - static uint8_t OpenExchangeContextCount; - static uint8_t CheckInRequestCount; + uint8_t OpenExchangeContextCount; + uint8_t CheckInRequestCount; private: // SIT ICDs should have a SlowPollingThreshold shorter than or equal to 15s (spec 9.16.1.5) From c5a1f1247017f282832eba0a901d628d7bb2ad0b Mon Sep 17 00:00:00 2001 From: jepenven-silabs Date: Mon, 20 Nov 2023 08:31:52 -0500 Subject: [PATCH 3/3] fix last comments --- src/app/icd/ICDManager.cpp | 18 +++++++++--------- src/app/icd/ICDManager.h | 4 ++-- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/app/icd/ICDManager.cpp b/src/app/icd/ICDManager.cpp index f36397be6c2465..d7b1aacd113977 100644 --- a/src/app/icd/ICDManager.cpp +++ b/src/app/icd/ICDManager.cpp @@ -41,7 +41,7 @@ using namespace chip::app::Clusters; using namespace chip::app::Clusters::IcdManagement; 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) { @@ -278,14 +278,14 @@ void ICDManager::OnKeepActiveRequest(KeepActiveFlags request) { // There can be multiple open exchange contexts at the same time. // Keep track of the requests count. - this->OpenExchangeContextCount++; + this->mOpenExchangeContextCount++; } if (request.Has(KeepActiveFlag::kCheckInInProgress)) { // There can be multiple check-in at the same time. // Keep track of the requests count. - this->CheckInRequestCount++; + this->mCheckInRequestCount++; } this->SetKeepActiveModeRequirements(request, true /* state */); @@ -301,16 +301,16 @@ void ICDManager::OnActiveRequestWithdrawal(KeepActiveFlags request) { // 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(KeepActiveFlag::kExchangeContextOpen, false /* state */); } @@ -320,16 +320,16 @@ void ICDManager::OnActiveRequestWithdrawal(KeepActiveFlags request) { // There can be multiple open exchange contexts at the same time. // Keep track of the requests count. - if (this->CheckInRequestCount > 0) + if (this->mCheckInRequestCount > 0) { - this->CheckInRequestCount--; + this->mCheckInRequestCount--; } else { ChipLogError(DeviceLayer, "The ICD Manager did not account for Check-In Sender start"); } - if (this->CheckInRequestCount == 0) + if (this->mCheckInRequestCount == 0) { this->SetKeepActiveModeRequirements(KeepActiveFlag::kCheckInInProgress, false /* state */); } diff --git a/src/app/icd/ICDManager.h b/src/app/icd/ICDManager.h index fc667f14210df5..877a2ae3f25603 100644 --- a/src/app/icd/ICDManager.h +++ b/src/app/icd/ICDManager.h @@ -128,8 +128,8 @@ class ICDManager : public ICDListener */ static void OnTransitionToIdle(System::Layer * aLayer, void * appState); - uint8_t OpenExchangeContextCount; - uint8_t CheckInRequestCount; + 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)