From 6612770f23e90fd9cd89ecd0ccf58679951774b7 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Mon, 4 Mar 2024 14:28:43 -0500 Subject: [PATCH 01/29] wip --- src/app/InteractionModelEngine.cpp | 21 +++++++++++++++-- src/app/ReadHandler.cpp | 1 + src/app/icd/server/BUILD.gn | 3 +++ src/app/icd/server/ICDManager.cpp | 32 +++++++++++++++++++------- src/app/icd/server/ICDManager.h | 37 ++++++++++++++++++++++++++---- src/app/server/Server.cpp | 18 ++++++++++++++- src/app/server/Server.h | 12 ++++++++++ 7 files changed, 109 insertions(+), 15 deletions(-) diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 81bea8af9b4626..c0f2799d059c1e 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -371,8 +371,25 @@ bool InteractionModelEngine::SubjectHasActiveSubscription(FabricIndex aFabricInd bool InteractionModelEngine::SubjectHasPersistedSubscription(FabricIndex aFabricIndex, NodeId subjectID) { - // TODO(#30281) : Implement persisted sub check and verify how persistent subscriptions affects this at ICDManager::Init - return false; + bool persistedSubMatches = false; + +#if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS + auto * iterator = mpSubscriptionResumptionStorage->IterateSubscriptions(); + SubscriptionResumptionStorage::SubscriptionInfo subscriptionInfo; + + while (iterator->Next(subscriptionInfo)) + { + // TODO(#31873): Persistant subscription only stores the NodeID for now. We cannot check if the CAT matches + if (subscriptionInfo.mFabricIndex == aFabricIndex && subscriptionInfo.mNodeId == subjectID) + { + persistedSubMatches = true; + break; + } + } + iterator->Release(); +#endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS + + return persistedSubMatches; } void InteractionModelEngine::OnDone(CommandResponseSender & apResponderObj) diff --git a/src/app/ReadHandler.cpp b/src/app/ReadHandler.cpp index 63da2a53aa7470..97ba2dc4565ea9 100644 --- a/src/app/ReadHandler.cpp +++ b/src/app/ReadHandler.cpp @@ -821,6 +821,7 @@ void ReadHandler::PersistSubscription() auto * subscriptionResumptionStorage = mManagementCallback.GetInteractionModelEngine()->GetSubscriptionResumptionStorage(); VerifyOrReturn(subscriptionResumptionStorage != nullptr); + // TODO(#31873): We need to store the CAT information to enable better interaction with ICDs SubscriptionResumptionStorage::SubscriptionInfo subscriptionInfo = { .mNodeId = GetInitiatorNodeId(), .mFabricIndex = GetAccessingFabricIndex(), .mSubscriptionId = mSubscriptionId, diff --git a/src/app/icd/server/BUILD.gn b/src/app/icd/server/BUILD.gn index 9a10430a3303ff..e034ac38d5e13e 100644 --- a/src/app/icd/server/BUILD.gn +++ b/src/app/icd/server/BUILD.gn @@ -75,6 +75,8 @@ source_set("manager") { ":configuration-data", ":notifier", ":observer", + + # TODO : can this be in checkin? "${chip_root}/src/app:subscription-manager", "${chip_root}/src/credentials:credentials", "${chip_root}/src/messaging", @@ -84,6 +86,7 @@ source_set("manager") { public_deps += [ ":monitoring-table", ":sender", + "${chip_root}/src/app:app_config", ] } } diff --git a/src/app/icd/server/ICDManager.cpp b/src/app/icd/server/ICDManager.cpp index 931737783dde9f..e586b2259b0727 100644 --- a/src/app/icd/server/ICDManager.cpp +++ b/src/app/icd/server/ICDManager.cpp @@ -204,8 +204,10 @@ void ICDManager::SendCheckInMsgs() #endif // CONFIG_BUILD_FOR_HOST_UNIT_TEST } -bool ICDManager::CheckInMessagesWouldBeSent() +bool ICDManager::CheckInMessagesWouldBeSent(std::function RegistrationVerifier) { + VerifyOrReturnValue(RegistrationVerifier, false); + for (const auto & fabricInfo : *mFabricTable) { uint16_t supported_clients = ICDConfigurationData::GetInstance().GetClientsSupportedPerFabric(); @@ -234,7 +236,7 @@ bool ICDManager::CheckInMessagesWouldBeSent() } // At least one registration would require a Check-In message - VerifyOrReturnValue(mSubInfoProvider->SubjectHasActiveSubscription(entry.fabricIndex, entry.monitoredSubject), true); + VerifyOrReturnValue(RegistrationVerifier(entry.fabricIndex, entry.monitoredSubject), true); } } @@ -242,7 +244,19 @@ bool ICDManager::CheckInMessagesWouldBeSent() return false; } -void ICDManager::TriggerCheckInMessages() +bool ICDManager::CheckInWouldBeSentAtActiveModeVerifier(FabricIndex aFabricIndex, NodeId subjectID) +{ + VerifyOrReturnValue(mSubInfoProvider->SubjectHasActiveSubscription(aFabricIndex, subjectID), true); + +#if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS + // At least one registration has a persisted entry. Do not send Check-In message. + // This is to cover the use-case where the subscription resumption feature is used with the Check-In message. + VerifyOrReturnValue(mSubInfoProvider->SubjectHasPersistedSubscription(aFabricIndex, subjectID), true); +#endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS + return false; +} + +void ICDManager::TriggerCheckInMessages(const std::function & verifier) { VerifyOrReturn(SupportsFeature(Feature::kCheckInProtocolSupport)); @@ -250,9 +264,7 @@ void ICDManager::TriggerCheckInMessages() // If we are already in ActiveMode, Check-In messages have already been sent. VerifyOrReturn(mOperationalState == OperationalState::IdleMode); - // If we don't have any Check-In messages to send, do nothing - VerifyOrReturn(CheckInMessagesWouldBeSent()); - + VerifyOrReturn(CheckInMessagesWouldBeSent(verifier)); UpdateOperationState(OperationalState::ActiveMode); } #endif // CHIP_CONFIG_ENABLE_ICD_CIP @@ -313,12 +325,16 @@ void ICDManager::UpdateOperationState(OperationalState state) { mOperationalState = OperationalState::IdleMode; +#if CHIP_CONFIG_ENABLE_ICD_CIP + std::function verifier = + std::bind(&ICDManager::CheckInWouldBeSentAtActiveModeVerifier, this, std::placeholders::_1, std::placeholders::_2); +#endif // CHIP_CONFIG_ENABLE_ICD_CIP + // When the active mode interval is 0, we stay in idleMode until a notification brings the icd into active mode // unless the device would need to send Check-In messages - // TODO(#30281) : Verify how persistent subscriptions affects this at ICDManager::Init if (ICDConfigurationData::GetInstance().GetActiveModeDuration() > kZero #if CHIP_CONFIG_ENABLE_ICD_CIP - || CheckInMessagesWouldBeSent() + || CheckInMessagesWouldBeSent(verifier) #endif // CHIP_CONFIG_ENABLE_ICD_CIP ) { diff --git a/src/app/icd/server/ICDManager.h b/src/app/icd/server/ICDManager.h index 5e391c1469143c..7858588081f8cc 100644 --- a/src/app/icd/server/ICDManager.h +++ b/src/app/icd/server/ICDManager.h @@ -31,12 +31,16 @@ #include #include #include +#include #include #include #include #include #include +#define CHIP_CONFIG_ENABLE_ICD_CIP 1 +#define CHIP_CONFIG_ENABLE_ICD_LIT 1 + namespace chip { namespace Crypto { using SymmetricKeystore = SessionKeystore; @@ -79,6 +83,15 @@ class ICDManager : public ICDListener ICDModeChange, }; + /** + * @brief Verifier template function + * This type can be used to implement specific verifiers that can be used in + * the CheckInMessagesWouldBeSent function. The goal is to avoid having multiple functions that implement the iterator + * loop with only the check changing. + */ + + using RegistrationVerificationFunction = bool(FabricIndex aFabricIndex, NodeId subjectID); + ICDManager() {} void Init(PersistentStorageDelegate * storage, FabricTable * fabricTable, Crypto::SymmetricKeystore * symmetricKeyStore, Messaging::ExchangeManager * exchangeManager, SubscriptionsInfoProvider * manager); @@ -122,8 +135,10 @@ class ICDManager : public ICDListener /** * @brief Trigger the ICDManager to send Check-In message if necessary + * + * @param[in] algo Verifier function to use to determine if we need to send check-in messages */ - void TriggerCheckInMessages(); + void TriggerCheckInMessages(const std::function & verifier); #endif // CHIP_CONFIG_ENABLE_ICD_CIP #ifdef CONFIG_BUILD_FOR_HOST_UNIT_TEST @@ -166,14 +181,28 @@ class ICDManager : public ICDListener private: #if CHIP_CONFIG_ENABLE_ICD_CIP + /** + * @brief Verifier to determine if a Check-In message would be sent on transition to ActiveMode + * + * @param aFabricIndex client fabric index + * @param subjectID client subject ID + * @return true Check-In message would be sent on transition to ActiveMode. + * @return false Device has an active subscription with the subjectID. + * Device is trying to resume inactive subscriptions with the client. See CHIP_CONFIG_PERSIST_SUBSCRIPTIONS and + * CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION + */ + bool CheckInWouldBeSentAtActiveModeVerifier(FabricIndex aFabricIndex, NodeId subjectID); + /** * @brief Function checks if at least one client registration would require a Check-In message * + * @param[in] RegistrationVerifier function to use to determine if a Check-In message would be sent for a given registration + * * @return true At least one registration would require an Check-In message if we were entering ActiveMode. - * @return false None of the registration would require a Check-In message either because there are no registration or because - * they all have associated subscriptions. + * @return false None of the registration would require a Check-In message either because there are no registration or + * because they all have associated subscriptions. */ - bool CheckInMessagesWouldBeSent(); + bool CheckInMessagesWouldBeSent(std::function RegistrationVerifier); #endif // CHIP_CONFIG_ENABLE_ICD_CIP KeepActiveFlags mKeepActiveFlags{ 0 }; diff --git a/src/app/server/Server.cpp b/src/app/server/Server.cpp index d2983636450691..3d03de4334f921 100644 --- a/src/app/server/Server.cpp +++ b/src/app/server/Server.cpp @@ -446,7 +446,9 @@ void Server::OnPlatformEvent(const DeviceLayer::ChipDeviceEvent & event) // We trigger Check-In messages before resuming subscriptions to avoid doing both. if (!mFailSafeContext.IsFailSafeArmed()) { - mICDManager.TriggerCheckInMessages(); + std::function verifier = + std::bind(&Server::CheckInWouldBeSentAtBootVerifier, this, std::placeholders::_1, std::placeholders::_2); + mICDManager.TriggerCheckInMessages(verifier); } #endif // CHIP_CONFIG_ENABLE_ICD_SERVER && CHIP_CONFIG_ENABLE_ICD_CIP #if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS @@ -519,6 +521,20 @@ void Server::RejoinExistingMulticastGroups() } } +#if CHIP_CONFIG_ENABLE_ICD_CIP +bool Server::CheckInWouldBeSentAtBootVerifier(FabricIndex aFabricIndex, NodeId subjectID) +{ +#if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS + // At least one registration has a persisted entry. Do not send Check-In message. + // This is to cover the use-case where the subscription resumption feature is used with the Check-In message. + VerifyOrReturnValue(chip::app::InteractionModelEngine::GetInstance()->SubjectHasPersistedSubscription(aFabricIndex, subjectID), + true); +#endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS + + return false; +} +#endif // CHIP_CONFIG_ENABLE_ICD_CIP + void Server::GenerateShutDownEvent() { PlatformMgr().ScheduleWork([](intptr_t) { PlatformMgr().HandleServerShuttingDown(); }); diff --git a/src/app/server/Server.h b/src/app/server/Server.h index 302471b8bebb89..83b0a65b18daf1 100644 --- a/src/app/server/Server.h +++ b/src/app/server/Server.h @@ -369,6 +369,18 @@ class Server #if CHIP_CONFIG_ENABLE_ICD_SERVER app::ICDManager & GetICDManager() { return mICDManager; } + +#if CHIP_CONFIG_ENABLE_ICD_CIP + /** + * @brief Verifier to determine if a Check-In message would be sent at Boot up + * + * @param aFabricIndex client fabric index + * @param subjectID client subject ID + * @return true Check-In message would be sent on boot up. + * @return false Device has a persisted subscription with the client. See CHIP_CONFIG_PERSIST_SUBSCRIPTIONS. + */ + bool CheckInWouldBeSentAtBootVerifier(FabricIndex aFabricIndex, NodeId subjectID); +#endif // CHIP_CONFIG_ENABLE_ICD_CIP #endif // CHIP_CONFIG_ENABLE_ICD_SERVER /** From 271a62874bc9b70786cbe1ed40be856e7156225f Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Fri, 15 Mar 2024 23:23:36 +0300 Subject: [PATCH 02/29] Add unit tests to validate NodeID subscriptions --- src/app/ReadHandler.h | 2 + src/app/tests/TestInteractionModelEngine.cpp | 368 ++++++++++++++++++- 2 files changed, 364 insertions(+), 6 deletions(-) diff --git a/src/app/ReadHandler.h b/src/app/ReadHandler.h index c597bac8a58d70..bf7d229e51f346 100644 --- a/src/app/ReadHandler.h +++ b/src/app/ReadHandler.h @@ -70,6 +70,7 @@ class TestReportScheduler; } // namespace reporting class InteractionModelEngine; +class TestInteractionModelEngine; /** * @class ReadHandler @@ -433,6 +434,7 @@ class ReadHandler : public Messaging::ExchangeDelegate // friend class chip::app::reporting::Engine; friend class chip::app::InteractionModelEngine; + friend class TestInteractionModelEngine; // The report scheduler needs to be able to access StateFlag private functions ShouldStartReporting(), CanStartReporting(), // ForceDirtyState() and IsDirty() to know when to schedule a run so it is declared as a friend class. diff --git a/src/app/tests/TestInteractionModelEngine.cpp b/src/app/tests/TestInteractionModelEngine.cpp index fcdc7d53e1594f..bc6517963fd0c0 100644 --- a/src/app/tests/TestInteractionModelEngine.cpp +++ b/src/app/tests/TestInteractionModelEngine.cpp @@ -22,6 +22,7 @@ * */ +#include #include #include #include @@ -38,10 +39,30 @@ #include #include +#if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS +#include +#include +#endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS + #include +namespace { + using TestContext = chip::Test::AppContext; +class NullReadHandlerCallback : public chip::app::ReadHandler::ManagementCallback +{ +public: + void OnDone(chip::app::ReadHandler & apReadHandlerObj) override {} + chip::app::ReadHandler::ApplicationCallback * GetAppCallback() override { return nullptr; } + chip::app::InteractionModelEngine * GetInteractionModelEngine() override + { + return chip::app::InteractionModelEngine::GetInstance(); + } +}; + +} // namespace + namespace chip { namespace app { class TestInteractionModelEngine @@ -49,10 +70,17 @@ class TestInteractionModelEngine public: static void TestAttributePathParamsPushRelease(nlTestSuite * apSuite, void * apContext); static void TestRemoveDuplicateConcreteAttribute(nlTestSuite * apSuite, void * apContext); -#if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION +#if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS + static void TestSubjectHasPersistedSubscription(nlTestSuite * apSuite, void * apContext); +#if CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION static void TestSubscriptionResumptionTimer(nlTestSuite * apSuite, void * apContext); -#endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION +#endif // CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION +#endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS static int GetAttributePathListLength(SingleLinkedListNode * apattributePathParamsList); + static void TestSubjectHasActiveSubscriptionSingleSubOneEntry(nlTestSuite * apSuite, void * apContext); + static void TestSubjectHasActiveSubscriptionSingleSubMultipleEntries(nlTestSuite * apSuite, void * apContext); + static void TestSubjectHasActiveSubscriptionMultipleSubsSingleEntry(nlTestSuite * apSuite, void * apContext); + static void TestSubjectHasActiveSubscriptionMultipleSubsMultipleEntries(nlTestSuite * apSuite, void * apContext); }; int TestInteractionModelEngine::GetAttributePathListLength(SingleLinkedListNode * apAttributePathParamsList) @@ -229,7 +257,326 @@ void TestInteractionModelEngine::TestRemoveDuplicateConcreteAttribute(nlTestSuit InteractionModelEngine::GetInstance()->ReleaseAttributePathList(attributePathParamsList); } -#if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION +void TestInteractionModelEngine::TestSubjectHasActiveSubscriptionSingleSubOneEntry(nlTestSuite * apSuite, void * apContext) +{ + TestContext & ctx = *static_cast(apContext); + NullReadHandlerCallback nullCallback; + InteractionModelEngine * engine = InteractionModelEngine::GetInstance(); + + NodeId bobNodeId = 0x12344321ull; + FabricIndex bobFabricIndex = 1; + + // Create ExchangeContext + Messaging::ExchangeContext * exchangeCtx1 = ctx.NewExchangeToBob(nullptr, false); + NL_TEST_ASSERT(apSuite, exchangeCtx1); + + // InteractionModelEngine init + NL_TEST_ASSERT(apSuite, + CHIP_NO_ERROR == + engine->Init(&ctx.GetExchangeManager(), &ctx.GetFabricTable(), reporting::GetDefaultReportScheduler())); + + // Verify that there are no active subscriptions + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(bobFabricIndex, bobNodeId) == false); + + // Create and setup readHandler 1 + ReadHandler * readHandler1 = engine->GetReadHandlerPool().CreateObject( + nullCallback, exchangeCtx1, ReadHandler::InteractionType::Subscribe, reporting::GetDefaultReportScheduler()); + + // Verify that Bob still doesn't have an active subscription + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(bobFabricIndex, bobNodeId) == false); + + // Set readHandler1 to active + readHandler1->SetStateFlag(ReadHandler::ReadHandlerFlags::ActiveSubscription, true); + + // Verify that Bob still has an active subscription + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(bobFabricIndex, bobNodeId)); + + // Clean up read handlers + engine->GetReadHandlerPool().ReleaseAll(); + + // Verify that there are no active subscriptions + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(bobFabricIndex, bobNodeId) == false); +} + +void TestInteractionModelEngine::TestSubjectHasActiveSubscriptionSingleSubMultipleEntries(nlTestSuite * apSuite, void * apContext) +{ + TestContext & ctx = *static_cast(apContext); + NullReadHandlerCallback nullCallback; + InteractionModelEngine * engine = InteractionModelEngine::GetInstance(); + + NodeId bobNodeId = 0x12344321ull; + FabricIndex bobFabricIndex = 1; + + // Create ExchangeContexts + Messaging::ExchangeContext * exchangeCtx1 = ctx.NewExchangeToBob(nullptr, false); + NL_TEST_ASSERT(apSuite, exchangeCtx1); + + Messaging::ExchangeContext * exchangeCtx2 = ctx.NewExchangeToBob(nullptr, false); + NL_TEST_ASSERT(apSuite, exchangeCtx1); + + // InteractionModelEngine init + NL_TEST_ASSERT(apSuite, + CHIP_NO_ERROR == + engine->Init(&ctx.GetExchangeManager(), &ctx.GetFabricTable(), reporting::GetDefaultReportScheduler())); + + // Verify that both Alice and Bob have no active subscriptions + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(bobFabricIndex, bobNodeId) == false); + + // Create readHandler 1 + engine->GetReadHandlerPool().CreateObject(nullCallback, exchangeCtx1, ReadHandler::InteractionType::Subscribe, + reporting::GetDefaultReportScheduler()); + + // Verify that Bob still doesn't have an active subscription + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(bobFabricIndex, bobNodeId) == false); + + // Create and setup readHandler 2 + ReadHandler * readHandler2 = engine->GetReadHandlerPool().CreateObject( + nullCallback, exchangeCtx2, ReadHandler::InteractionType::Subscribe, reporting::GetDefaultReportScheduler()); + + // Verify that Bob still doesn't have an active subscription + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(bobFabricIndex, bobNodeId) == false); + + // Set readHandler2 to active + readHandler2->SetStateFlag(ReadHandler::ReadHandlerFlags::ActiveSubscription, true); + + // Verify that Bob still doesn't have an active subscription + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(bobFabricIndex, bobNodeId)); + + // Release active ReadHandler + engine->GetReadHandlerPool().ReleaseObject(readHandler2); + + // Verify that there are no active subscriptions + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(bobFabricIndex, bobNodeId) == false); + + // Clean up read handlers + engine->GetReadHandlerPool().ReleaseAll(); +} + +void TestInteractionModelEngine::TestSubjectHasActiveSubscriptionMultipleSubsSingleEntry(nlTestSuite * apSuite, void * apContext) +{ + TestContext & ctx = *static_cast(apContext); + NullReadHandlerCallback nullCallback; + InteractionModelEngine * engine = InteractionModelEngine::GetInstance(); + + NodeId bobNodeId = 0x12344321ull; + FabricIndex bobFabricIndex = 1; + NodeId aliceNodeId = 0x11223344ull; + FabricIndex aliceFabricIndex = 2; + + // Create ExchangeContexts + Messaging::ExchangeContext * exchangeCtx1 = ctx.NewExchangeToBob(nullptr, false); + NL_TEST_ASSERT(apSuite, exchangeCtx1); + + Messaging::ExchangeContext * exchangeCtx2 = ctx.NewExchangeToAlice(nullptr, false); + NL_TEST_ASSERT(apSuite, exchangeCtx2); + + // InteractionModelEngine init + NL_TEST_ASSERT(apSuite, + CHIP_NO_ERROR == + engine->Init(&ctx.GetExchangeManager(), &ctx.GetFabricTable(), reporting::GetDefaultReportScheduler())); + + // Verify that both Alice and Bob have no active subscriptions + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(bobFabricIndex, bobNodeId) == false); + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(aliceFabricIndex, aliceNodeId) == false); + + // Create and setup readHandler 1 + ReadHandler * readHandler1 = engine->GetReadHandlerPool().CreateObject( + nullCallback, exchangeCtx1, ReadHandler::InteractionType::Subscribe, reporting::GetDefaultReportScheduler()); + + // Create and setup readHandler 2 + ReadHandler * readHandler2 = engine->GetReadHandlerPool().CreateObject( + nullCallback, exchangeCtx2, ReadHandler::InteractionType::Subscribe, reporting::GetDefaultReportScheduler()); + + // Verify that Bob still doesn't have an active subscription + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(bobFabricIndex, bobNodeId) == false); + + // Set readHandler1 to active + readHandler1->SetStateFlag(ReadHandler::ReadHandlerFlags::ActiveSubscription, true); + + // Verify that Bob has an active subscription + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(bobFabricIndex, bobNodeId)); + + // Verify that Alice still doesn't have an active subscription + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(aliceFabricIndex, aliceNodeId) == false); + + // Set readHandler2 to active + readHandler2->SetStateFlag(ReadHandler::ReadHandlerFlags::ActiveSubscription, true); + + // Verify that Bob has an active subscription + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(bobFabricIndex, bobNodeId)); + + // Verify that Alice still doesn't have an active subscription + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(aliceFabricIndex, aliceNodeId)); + + // Set readHandler1 to inactive + readHandler1->SetStateFlag(ReadHandler::ReadHandlerFlags::ActiveSubscription, false); + + // Verify that Bob doesn't have an active subscription + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(bobFabricIndex, bobNodeId) == false); + + // Verify that Alice still has an active subscription + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(aliceFabricIndex, aliceNodeId)); + + // Clean up read handlers + engine->GetReadHandlerPool().ReleaseAll(); + + // Verify that both Alice and Bob have no active subscriptions + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(bobFabricIndex, bobNodeId) == false); + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(aliceFabricIndex, aliceNodeId) == false); +} + +void TestInteractionModelEngine::TestSubjectHasActiveSubscriptionMultipleSubsMultipleEntries(nlTestSuite * apSuite, + void * apContext) +{ + TestContext & ctx = *static_cast(apContext); + NullReadHandlerCallback nullCallback; + InteractionModelEngine * engine = InteractionModelEngine::GetInstance(); + + NodeId bobNodeId = 0x12344321ull; + FabricIndex bobFabricIndex = 1; + NodeId aliceNodeId = 0x11223344ull; + FabricIndex aliceFabricIndex = 2; + + // Create ExchangeContexts + Messaging::ExchangeContext * exchangeCtx11 = ctx.NewExchangeToBob(nullptr, false); + NL_TEST_ASSERT(apSuite, exchangeCtx11); + + Messaging::ExchangeContext * exchangeCtx12 = ctx.NewExchangeToBob(nullptr, false); + NL_TEST_ASSERT(apSuite, exchangeCtx12); + + Messaging::ExchangeContext * exchangeCtx21 = ctx.NewExchangeToAlice(nullptr, false); + NL_TEST_ASSERT(apSuite, exchangeCtx21); + + Messaging::ExchangeContext * exchangeCtx22 = ctx.NewExchangeToAlice(nullptr, false); + NL_TEST_ASSERT(apSuite, exchangeCtx22); + + // InteractionModelEngine init + NL_TEST_ASSERT(apSuite, + CHIP_NO_ERROR == + engine->Init(&ctx.GetExchangeManager(), &ctx.GetFabricTable(), reporting::GetDefaultReportScheduler())); + + // Verify that both Alice and Bob have no active subscriptions + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(bobFabricIndex, bobNodeId) == false); + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(aliceFabricIndex, aliceNodeId) == false); + + // Create and setup readHandler 1-1 + engine->GetReadHandlerPool().CreateObject(nullCallback, exchangeCtx11, ReadHandler::InteractionType::Subscribe, + reporting::GetDefaultReportScheduler()); + + // Create and setup readHandler 1-2 + ReadHandler * readHandler12 = engine->GetReadHandlerPool().CreateObject( + nullCallback, exchangeCtx12, ReadHandler::InteractionType::Subscribe, reporting::GetDefaultReportScheduler()); + + // Create and setup readHandler 2-1 + engine->GetReadHandlerPool().CreateObject(nullCallback, exchangeCtx21, ReadHandler::InteractionType::Subscribe, + reporting::GetDefaultReportScheduler()); + + // Create and setup readHandler 2-2 + ReadHandler * readHandler22 = engine->GetReadHandlerPool().CreateObject( + nullCallback, exchangeCtx22, ReadHandler::InteractionType::Subscribe, reporting::GetDefaultReportScheduler()); + + // Verify that both Alice and Bob have no active subscriptions + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(bobFabricIndex, bobNodeId) == false); + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(aliceFabricIndex, aliceNodeId) == false); + + // Set readHandler 1-2 to active + readHandler12->SetStateFlag(ReadHandler::ReadHandlerFlags::ActiveSubscription, true); + + // Verify that Bob has an active subscription + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(bobFabricIndex, bobNodeId)); + + // Verify that Alice still doesn't have an active subscription + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(aliceFabricIndex, aliceNodeId) == false); + + // Set readHandler 2-2 to active + readHandler22->SetStateFlag(ReadHandler::ReadHandlerFlags::ActiveSubscription, true); + + // Verify that Bob has an active subscription + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(bobFabricIndex, bobNodeId)); + + // Verify that Alice still doesn't have an active subscription + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(aliceFabricIndex, aliceNodeId)); + + // Set readHandler1 to inactive + readHandler12->SetStateFlag(ReadHandler::ReadHandlerFlags::ActiveSubscription, false); + + // Verify that Bob doesn't have an active subscription + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(bobFabricIndex, bobNodeId) == false); + + // Verify that Alice still has an active subscription + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(aliceFabricIndex, aliceNodeId)); + + // Clean up read handlers + engine->GetReadHandlerPool().ReleaseAll(); + + // Verify that both Alice and Bob have no active subscriptions + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(bobFabricIndex, bobNodeId) == false); + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(aliceFabricIndex, aliceNodeId) == false); +} + +#if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS + +void TestInteractionModelEngine::TestSubjectHasPersistedSubscription(nlTestSuite * apSuite, void * apContext) +{ + TestContext & ctx = *static_cast(apContext); + CHIP_ERROR err = CHIP_NO_ERROR; + + chip::TestPersistentStorageDelegate storage; + chip::app::SimpleSubscriptionResumptionStorage subscriptionStorage; + + err = subscriptionStorage.Init(&storage); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + + err = InteractionModelEngine::GetInstance()->Init(&ctx.GetExchangeManager(), &ctx.GetFabricTable(), + app::reporting::GetDefaultReportScheduler(), nullptr, &subscriptionStorage); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + + NodeId nodeId1 = 1; + FabricIndex fabric1 = 1; + SubscriptionId sub1 = 1; + NodeId nodeId2 = 2; + FabricIndex fabric2 = 2; + SubscriptionId sub2 = 2; + + SubscriptionResumptionStorage::SubscriptionInfo info1 = { .mNodeId = nodeId1, + .mFabricIndex = fabric1, + .mSubscriptionId = sub1 }; + SubscriptionResumptionStorage::SubscriptionInfo info2 = { .mNodeId = nodeId2, + .mFabricIndex = fabric2, + .mSubscriptionId = sub2 }; + + // Test with no persisted subscriptions - Should return false + NL_TEST_ASSERT(apSuite, InteractionModelEngine::GetInstance()->SubjectHasPersistedSubscription(fabric1, nodeId1) == false); + + // Add one entry + NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == subscriptionStorage.Save(info1)); + + // Verify that entry matches - Should return true + NL_TEST_ASSERT(apSuite, InteractionModelEngine::GetInstance()->SubjectHasPersistedSubscription(fabric1, nodeId1)); + + // Test with absent subscription - Should return false + NL_TEST_ASSERT(apSuite, InteractionModelEngine::GetInstance()->SubjectHasPersistedSubscription(fabric2, nodeId2) == false); + + // Add second entry + NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == subscriptionStorage.Save(info2)); + + // Verify that entry matches - Should return true + NL_TEST_ASSERT(apSuite, InteractionModelEngine::GetInstance()->SubjectHasPersistedSubscription(fabric2, nodeId2)); + NL_TEST_ASSERT(apSuite, InteractionModelEngine::GetInstance()->SubjectHasPersistedSubscription(fabric1, nodeId1)); + + // Remove an entry + NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == subscriptionStorage.Delete(nodeId1, fabric1, sub1)); + + // Test with absent subscription - Should return false + NL_TEST_ASSERT(apSuite, InteractionModelEngine::GetInstance()->SubjectHasPersistedSubscription(fabric1, nodeId1) == false); + + // Clean Up entries + subscriptionStorage.DeleteAll(fabric1); + subscriptionStorage.DeleteAll(fabric2); +} + +#if CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION + void TestInteractionModelEngine::TestSubscriptionResumptionTimer(nlTestSuite * apSuite, void * apContext) { TestContext & ctx = *static_cast(apContext); @@ -259,7 +606,9 @@ void TestInteractionModelEngine::TestSubscriptionResumptionTimer(nlTestSuite * a timeTillNextResubscriptionMs = InteractionModelEngine::GetInstance()->ComputeTimeSecondsTillNextSubscriptionResumption(); NL_TEST_ASSERT(apSuite, timeTillNextResubscriptionMs == CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION_MAX_RETRY_INTERVAL_SECS); } -#endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION + +#endif // CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION +#endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS } // namespace app } // namespace chip @@ -271,9 +620,16 @@ const nlTest sTests[] = { NL_TEST_DEF("TestAttributePathParamsPushRelease", chip::app::TestInteractionModelEngine::TestAttributePathParamsPushRelease), NL_TEST_DEF("TestRemoveDuplicateConcreteAttribute", chip::app::TestInteractionModelEngine::TestRemoveDuplicateConcreteAttribute), -#if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION +#if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS + NL_TEST_DEF("TestSubjectHasPersistedSubscription", chip::app::TestInteractionModelEngine::TestSubjectHasPersistedSubscription), +#if CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION NL_TEST_DEF("TestSubscriptionResumptionTimer", chip::app::TestInteractionModelEngine::TestSubscriptionResumptionTimer), -#endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION +#endif // CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION +#endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS + NL_TEST_DEF("TestSubjectHasActiveSubscriptionSingleSubOneEntry", chip::app::TestInteractionModelEngine::TestSubjectHasActiveSubscriptionSingleSubOneEntry), + NL_TEST_DEF("TestSubjectHasActiveSubscriptionSingleSubMultipleEntries", chip::app::TestInteractionModelEngine::TestSubjectHasActiveSubscriptionSingleSubMultipleEntries), + NL_TEST_DEF("TestSubjectHasActiveSubscriptionMultipleSubsSingleEntry", chip::app::TestInteractionModelEngine::TestSubjectHasActiveSubscriptionMultipleSubsSingleEntry), + NL_TEST_DEF("TestSubjectHasActiveSubscriptionMultipleSubsMultipleEntries", chip::app::TestInteractionModelEngine::TestSubjectHasActiveSubscriptionMultipleSubsMultipleEntries), NL_TEST_SENTINEL() }; // clang-format on From bfef6900952077530bda83229b080601de2459ed Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Sat, 16 Mar 2024 01:37:19 +0300 Subject: [PATCH 03/29] Add CATS unit test --- src/app/tests/TestInteractionModelEngine.cpp | 54 ++++++++++++++++++++ src/messaging/tests/MessagingContext.cpp | 14 +++++ src/messaging/tests/MessagingContext.h | 3 ++ 3 files changed, 71 insertions(+) diff --git a/src/app/tests/TestInteractionModelEngine.cpp b/src/app/tests/TestInteractionModelEngine.cpp index bc6517963fd0c0..cfb775decb85d7 100644 --- a/src/app/tests/TestInteractionModelEngine.cpp +++ b/src/app/tests/TestInteractionModelEngine.cpp @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -81,6 +82,7 @@ class TestInteractionModelEngine static void TestSubjectHasActiveSubscriptionSingleSubMultipleEntries(nlTestSuite * apSuite, void * apContext); static void TestSubjectHasActiveSubscriptionMultipleSubsSingleEntry(nlTestSuite * apSuite, void * apContext); static void TestSubjectHasActiveSubscriptionMultipleSubsMultipleEntries(nlTestSuite * apSuite, void * apContext); + static void TestSubjectHasActiveSubscriptionSubWithCAT(nlTestSuite * apSuite, void * apContext); }; int TestInteractionModelEngine::GetAttributePathListLength(SingleLinkedListNode * apAttributePathParamsList) @@ -514,6 +516,57 @@ void TestInteractionModelEngine::TestSubjectHasActiveSubscriptionMultipleSubsMul NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(aliceFabricIndex, aliceNodeId) == false); } +void TestInteractionModelEngine::TestSubjectHasActiveSubscriptionSubWithCAT(nlTestSuite * apSuite, void * apContext) +{ + TestContext & ctx = *reinterpret_cast(apContext); + InteractionModelEngine * engine = InteractionModelEngine::GetInstance(); + NullReadHandlerCallback nullCallback; + + CASEAuthTag cat = 0x1111'0001; + CASEAuthTag invalidCAT = 0x1112'0001; + CATValues cats = CATValues{ { cat } }; + NodeId valideSubjectId = NodeIdFromCASEAuthTag(cat); + NodeId invalideSubjectId = NodeIdFromCASEAuthTag(invalidCAT); + FabricIndex bobFabricIndex = 1; + + // InteractionModelEngine init + NL_TEST_ASSERT(apSuite, + CHIP_NO_ERROR == + engine->Init(&ctx.GetExchangeManager(), &ctx.GetFabricTable(), reporting::GetDefaultReportScheduler())); + + // Make sure we are using CASE sessions, because there is no defunct-marking for PASE. + ctx.ExpireSessionBobToAlice(); + ctx.ExpireSessionAliceToBob(); + NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == ctx.CreateCASESessionBobToAlice(cats)); + NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == ctx.CreateCASESessionAliceToBob(cats)); + + // Create ExchangeContexts + Messaging::ExchangeContext * exchangeCtx = ctx.NewExchangeToBob(nullptr, false); + NL_TEST_ASSERT(apSuite, exchangeCtx); + + // Create readHandler + ReadHandler * readHandler = engine->GetReadHandlerPool().CreateObject( + nullCallback, exchangeCtx, ReadHandler::InteractionType::Subscribe, reporting::GetDefaultReportScheduler()); + + // Verify there are not active subscriptions + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(bobFabricIndex, valideSubjectId) == false); + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(bobFabricIndex, invalideSubjectId) == false); + + // Set readHandler to active + readHandler->SetStateFlag(ReadHandler::ReadHandlerFlags::ActiveSubscription, true); + + // Verify tthat valid subjectID has an active subscription + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(bobFabricIndex, valideSubjectId)); + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(bobFabricIndex, invalideSubjectId) == false); + + // Clean up read handlers + engine->GetReadHandlerPool().ReleaseAll(); + + // Verify there are not active subscriptions + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(bobFabricIndex, valideSubjectId) == false); + NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(bobFabricIndex, invalideSubjectId) == false); +} + #if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS void TestInteractionModelEngine::TestSubjectHasPersistedSubscription(nlTestSuite * apSuite, void * apContext) @@ -630,6 +683,7 @@ const nlTest sTests[] = NL_TEST_DEF("TestSubjectHasActiveSubscriptionSingleSubMultipleEntries", chip::app::TestInteractionModelEngine::TestSubjectHasActiveSubscriptionSingleSubMultipleEntries), NL_TEST_DEF("TestSubjectHasActiveSubscriptionMultipleSubsSingleEntry", chip::app::TestInteractionModelEngine::TestSubjectHasActiveSubscriptionMultipleSubsSingleEntry), NL_TEST_DEF("TestSubjectHasActiveSubscriptionMultipleSubsMultipleEntries", chip::app::TestInteractionModelEngine::TestSubjectHasActiveSubscriptionMultipleSubsMultipleEntries), + NL_TEST_DEF("TestSubjectHasActiveSubscriptionSubWithCAT", chip::app::TestInteractionModelEngine::TestSubjectHasActiveSubscriptionSubWithCAT), NL_TEST_SENTINEL() }; // clang-format on diff --git a/src/messaging/tests/MessagingContext.cpp b/src/messaging/tests/MessagingContext.cpp index 441492e80d5694..dc41292dc0f313 100644 --- a/src/messaging/tests/MessagingContext.cpp +++ b/src/messaging/tests/MessagingContext.cpp @@ -174,6 +174,13 @@ CHIP_ERROR MessagingContext::CreateCASESessionBobToAlice() CryptoContext::SessionRole::kInitiator); } +CHIP_ERROR MessagingContext::CreateCASESessionBobToAlice(const CATValues & cats) +{ + return mSessionManager.InjectCaseSessionWithTestKey(mSessionBobToAlice, kBobKeyId, kAliceKeyId, GetBobFabric()->GetNodeId(), + GetAliceFabric()->GetNodeId(), mBobFabricIndex, mAliceAddress, + CryptoContext::SessionRole::kInitiator, cats); +} + CHIP_ERROR MessagingContext::CreateSessionAliceToBob() { return mSessionManager.InjectPaseSessionWithTestKey(mSessionAliceToBob, kAliceKeyId, GetBobFabric()->GetNodeId(), kBobKeyId, @@ -187,6 +194,13 @@ CHIP_ERROR MessagingContext::CreateCASESessionAliceToBob() CryptoContext::SessionRole::kResponder); } +CHIP_ERROR MessagingContext::CreateCASESessionAliceToBob(const CATValues & cats) +{ + return mSessionManager.InjectCaseSessionWithTestKey(mSessionAliceToBob, kAliceKeyId, kBobKeyId, GetAliceFabric()->GetNodeId(), + GetBobFabric()->GetNodeId(), mAliceFabricIndex, mBobAddress, + CryptoContext::SessionRole::kResponder, cats); +} + CHIP_ERROR MessagingContext::CreatePASESessionCharlieToDavid() { return mSessionManager.InjectPaseSessionWithTestKey(mSessionCharlieToDavid, kCharlieKeyId, 0xdeadbeef, kDavidKeyId, diff --git a/src/messaging/tests/MessagingContext.h b/src/messaging/tests/MessagingContext.h index 31143a727326be..7ae452b9678a90 100644 --- a/src/messaging/tests/MessagingContext.h +++ b/src/messaging/tests/MessagingContext.h @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -141,8 +142,10 @@ class MessagingContext : public PlatformMemoryUser CHIP_ERROR CreateSessionBobToAlice(); // Creates PASE session CHIP_ERROR CreateCASESessionBobToAlice(); + CHIP_ERROR CreateCASESessionBobToAlice(const CATValues & cats); CHIP_ERROR CreateSessionAliceToBob(); // Creates PASE session CHIP_ERROR CreateCASESessionAliceToBob(); + CHIP_ERROR CreateCASESessionAliceToBob(const CATValues & cats); CHIP_ERROR CreateSessionBobToFriends(); // Creates PASE session CHIP_ERROR CreatePASESessionCharlieToDavid(); CHIP_ERROR CreatePASESessionDavidToCharlie(); From 76412278d5c91a7dd68ec55586c1fe13b60d63a9 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Sat, 16 Mar 2024 01:37:52 +0300 Subject: [PATCH 04/29] Clean up conditionnal checks --- src/app/InteractionModelEngine.cpp | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index c0f2799d059c1e..b3ed0e12601fb7 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -336,16 +336,10 @@ bool InteractionModelEngine::SubjectHasActiveSubscription(FabricIndex aFabricInd { bool isActive = false; mReadHandlers.ForEachActiveObject([aFabricIndex, subjectID, &isActive](ReadHandler * handler) { - if (!handler->IsType(ReadHandler::InteractionType::Subscribe)) - { - return Loop::Continue; - } + VerifyOrReturnValue(handler->IsType(ReadHandler::InteractionType::Subscribe), Loop::Continue); Access::SubjectDescriptor subject = handler->GetSubjectDescriptor(); - if (subject.fabricIndex != aFabricIndex) - { - return Loop::Continue; - } + VerifyOrReturnValue(subject.fabricIndex == aFabricIndex, Loop::Continue); if (subject.authMode == Access::AuthMode::kCase) { @@ -353,13 +347,9 @@ bool InteractionModelEngine::SubjectHasActiveSubscription(FabricIndex aFabricInd { isActive = handler->IsActiveSubscription(); - // Exit loop only if isActive is set to true - // Otherwise keep looking for another subscription that could - // match the subject - if (isActive) - { - return Loop::Break; - } + // Exit loop only if isActive is set to true. + // Otherwise keep looking for another subscription that could match the subject. + VerifyOrReturnValue(!isActive, Loop::Break); } } From 742ec95849d4377963a96b009a79f3d9eba86c6a Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Sun, 17 Mar 2024 19:28:38 +0800 Subject: [PATCH 05/29] finish PR clean up --- src/app/ReadHandler.cpp | 2 +- src/app/icd/server/BUILD.gn | 4 +--- src/app/icd/server/ICDManager.h | 3 --- src/app/server/Server.cpp | 6 +++--- src/app/tests/TestInteractionModelEngine.cpp | 20 ++++++++++++++++++++ 5 files changed, 25 insertions(+), 10 deletions(-) diff --git a/src/app/ReadHandler.cpp b/src/app/ReadHandler.cpp index 97ba2dc4565ea9..142df8015601ca 100644 --- a/src/app/ReadHandler.cpp +++ b/src/app/ReadHandler.cpp @@ -821,7 +821,7 @@ void ReadHandler::PersistSubscription() auto * subscriptionResumptionStorage = mManagementCallback.GetInteractionModelEngine()->GetSubscriptionResumptionStorage(); VerifyOrReturn(subscriptionResumptionStorage != nullptr); - // TODO(#31873): We need to store the CAT information to enable better interaction with ICDs + // TODO(#31873): We need to store the CAT information to enable better interactions with ICDs SubscriptionResumptionStorage::SubscriptionInfo subscriptionInfo = { .mNodeId = GetInitiatorNodeId(), .mFabricIndex = GetAccessingFabricIndex(), .mSubscriptionId = mSubscriptionId, diff --git a/src/app/icd/server/BUILD.gn b/src/app/icd/server/BUILD.gn index e034ac38d5e13e..b1e26cbe6de3fd 100644 --- a/src/app/icd/server/BUILD.gn +++ b/src/app/icd/server/BUILD.gn @@ -75,9 +75,6 @@ source_set("manager") { ":configuration-data", ":notifier", ":observer", - - # TODO : can this be in checkin? - "${chip_root}/src/app:subscription-manager", "${chip_root}/src/credentials:credentials", "${chip_root}/src/messaging", ] @@ -87,6 +84,7 @@ source_set("manager") { ":monitoring-table", ":sender", "${chip_root}/src/app:app_config", + "${chip_root}/src/app:subscription-manager", ] } } diff --git a/src/app/icd/server/ICDManager.h b/src/app/icd/server/ICDManager.h index 7858588081f8cc..8bb2e5b9877f0d 100644 --- a/src/app/icd/server/ICDManager.h +++ b/src/app/icd/server/ICDManager.h @@ -38,9 +38,6 @@ #include #include -#define CHIP_CONFIG_ENABLE_ICD_CIP 1 -#define CHIP_CONFIG_ENABLE_ICD_LIT 1 - namespace chip { namespace Crypto { using SymmetricKeystore = SessionKeystore; diff --git a/src/app/server/Server.cpp b/src/app/server/Server.cpp index 3d03de4334f921..2dbd1e444842b7 100644 --- a/src/app/server/Server.cpp +++ b/src/app/server/Server.cpp @@ -527,11 +527,11 @@ bool Server::CheckInWouldBeSentAtBootVerifier(FabricIndex aFabricIndex, NodeId s #if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS // At least one registration has a persisted entry. Do not send Check-In message. // This is to cover the use-case where the subscription resumption feature is used with the Check-In message. - VerifyOrReturnValue(chip::app::InteractionModelEngine::GetInstance()->SubjectHasPersistedSubscription(aFabricIndex, subjectID), - true); + VerifyOrReturnValue(!chip::app::InteractionModelEngine::GetInstance()->SubjectHasPersistedSubscription(aFabricIndex, subjectID), + false); #endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS - return false; + return true; } #endif // CHIP_CONFIG_ENABLE_ICD_CIP diff --git a/src/app/tests/TestInteractionModelEngine.cpp b/src/app/tests/TestInteractionModelEngine.cpp index cfb775decb85d7..803dc9c5b08d21 100644 --- a/src/app/tests/TestInteractionModelEngine.cpp +++ b/src/app/tests/TestInteractionModelEngine.cpp @@ -259,6 +259,9 @@ void TestInteractionModelEngine::TestRemoveDuplicateConcreteAttribute(nlTestSuit InteractionModelEngine::GetInstance()->ReleaseAttributePathList(attributePathParamsList); } +/** + * @brief Test verifies the SubjectHasActiveSubscription with a single subscription with a single entry + */ void TestInteractionModelEngine::TestSubjectHasActiveSubscriptionSingleSubOneEntry(nlTestSuite * apSuite, void * apContext) { TestContext & ctx = *static_cast(apContext); @@ -300,6 +303,10 @@ void TestInteractionModelEngine::TestSubjectHasActiveSubscriptionSingleSubOneEnt NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(bobFabricIndex, bobNodeId) == false); } +/** + * @brief Test verifies that the SubjectHasActiveSubscription will continue iterating till it fines at least one valid active + * subscription + */ void TestInteractionModelEngine::TestSubjectHasActiveSubscriptionSingleSubMultipleEntries(nlTestSuite * apSuite, void * apContext) { TestContext & ctx = *static_cast(apContext); @@ -354,6 +361,9 @@ void TestInteractionModelEngine::TestSubjectHasActiveSubscriptionSingleSubMultip engine->GetReadHandlerPool().ReleaseAll(); } +/** + * @brief Test validates that the SubjectHasActiveSubscription can support multiple subscriptions from different clients + */ void TestInteractionModelEngine::TestSubjectHasActiveSubscriptionMultipleSubsSingleEntry(nlTestSuite * apSuite, void * apContext) { TestContext & ctx = *static_cast(apContext); @@ -427,6 +437,10 @@ void TestInteractionModelEngine::TestSubjectHasActiveSubscriptionMultipleSubsSin NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(aliceFabricIndex, aliceNodeId) == false); } +/** + * @brief Test validates that the SubjectHasActiveSubscription can find the active subscription even if there are multiple + * subscriptions for each client + */ void TestInteractionModelEngine::TestSubjectHasActiveSubscriptionMultipleSubsMultipleEntries(nlTestSuite * apSuite, void * apContext) { @@ -516,6 +530,9 @@ void TestInteractionModelEngine::TestSubjectHasActiveSubscriptionMultipleSubsMul NL_TEST_ASSERT(apSuite, engine->SubjectHasActiveSubscription(aliceFabricIndex, aliceNodeId) == false); } +/** + * @brief Verifies that SubjectHasActiveSubscription support CATs as a subject-id + */ void TestInteractionModelEngine::TestSubjectHasActiveSubscriptionSubWithCAT(nlTestSuite * apSuite, void * apContext) { TestContext & ctx = *reinterpret_cast(apContext); @@ -569,6 +586,9 @@ void TestInteractionModelEngine::TestSubjectHasActiveSubscriptionSubWithCAT(nlTe #if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS +/** + * @brief Test verifies the SubjectHasPersistedSubscription with single and multiple persisted subscriptions. + */ void TestInteractionModelEngine::TestSubjectHasPersistedSubscription(nlTestSuite * apSuite, void * apContext) { TestContext & ctx = *static_cast(apContext); From bb0cfd8f5d38aa8184ddbdea65477c67e56d500b Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Mon, 18 Mar 2024 14:44:27 +0800 Subject: [PATCH 06/29] Fix conditionnals --- src/app/icd/server/ICDManager.cpp | 2 +- src/app/icd/server/ICDManager.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/icd/server/ICDManager.cpp b/src/app/icd/server/ICDManager.cpp index e586b2259b0727..6445bef4ad98d0 100644 --- a/src/app/icd/server/ICDManager.cpp +++ b/src/app/icd/server/ICDManager.cpp @@ -236,7 +236,7 @@ bool ICDManager::CheckInMessagesWouldBeSent(std::function #if CHIP_CONFIG_ENABLE_ICD_CIP +#include // nogncheck #include // nogncheck #include // nogncheck #endif // CHIP_CONFIG_ENABLE_ICD_CIP -#include #include #include #include From 60f996ccd1b99ca3b57cec9507d83be403225b33 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Mon, 18 Mar 2024 15:05:33 +0800 Subject: [PATCH 07/29] update comment --- src/app/icd/server/ICDManager.h | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/app/icd/server/ICDManager.h b/src/app/icd/server/ICDManager.h index c968326c41c2d7..09633567ebcdf5 100644 --- a/src/app/icd/server/ICDManager.h +++ b/src/app/icd/server/ICDManager.h @@ -82,9 +82,11 @@ class ICDManager : public ICDListener /** * @brief Verifier template function - * This type can be used to implement specific verifiers that can be used in - * the CheckInMessagesWouldBeSent function. The goal is to avoid having multiple functions that implement the iterator - * loop with only the check changing. + * This type can be used to implement specific verifiers that can be used in the CheckInMessagesWouldBeSent function. + * The goal is to avoid having multiple functions that implement the iterator loop with only the check changing. + * + * @return true if at least one Check-In message wuld be sent + * false No Check-In messages would be sent */ using RegistrationVerificationFunction = bool(FabricIndex aFabricIndex, NodeId subjectID); From a0882432baced25c3890e4a377e8c53b77f96c8b Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Mon, 18 Mar 2024 15:08:02 +0800 Subject: [PATCH 08/29] fix typo --- src/app/InteractionModelEngine.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index b3ed0e12601fb7..f6c67e24ea04b9 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -369,7 +369,7 @@ bool InteractionModelEngine::SubjectHasPersistedSubscription(FabricIndex aFabric while (iterator->Next(subscriptionInfo)) { - // TODO(#31873): Persistant subscription only stores the NodeID for now. We cannot check if the CAT matches + // TODO(#31873): Persistent subscription only stores the NodeID for now. We cannot check if the CAT matches if (subscriptionInfo.mFabricIndex == aFabricIndex && subscriptionInfo.mNodeId == subjectID) { persistedSubMatches = true; From ec25e96b7b08cd1f6f2c2e62108075e349145362 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Tue, 19 Mar 2024 06:50:49 +0800 Subject: [PATCH 09/29] Fix build issue rename gn source-set to match header --- src/app/BUILD.gn | 4 ++-- src/app/icd/server/BUILD.gn | 2 +- src/app/icd/server/ICDManager.h | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/app/BUILD.gn b/src/app/BUILD.gn index e2b96e44868c0d..c7ebe3a377df7b 100644 --- a/src/app/BUILD.gn +++ b/src/app/BUILD.gn @@ -122,7 +122,7 @@ source_set("global-attributes") { ] } -source_set("subscription-manager") { +source_set("subscription-info-provider") { sources = [ "SubscriptionsInfoProvider.h" ] public_deps = [ "${chip_root}/src/lib/core" ] @@ -207,7 +207,7 @@ static_library("interaction-model") { ":app_config", ":constants", ":paths", - ":subscription-manager", + ":subscription-info-provider", "${chip_root}/src/app/MessageDef", "${chip_root}/src/app/icd/server:icd-server-config", "${chip_root}/src/app/icd/server:observer", diff --git a/src/app/icd/server/BUILD.gn b/src/app/icd/server/BUILD.gn index b1e26cbe6de3fd..a73784152f482f 100644 --- a/src/app/icd/server/BUILD.gn +++ b/src/app/icd/server/BUILD.gn @@ -75,6 +75,7 @@ source_set("manager") { ":configuration-data", ":notifier", ":observer", + "${chip_root}/src/app:subscription-info-provider", "${chip_root}/src/credentials:credentials", "${chip_root}/src/messaging", ] @@ -84,7 +85,6 @@ source_set("manager") { ":monitoring-table", ":sender", "${chip_root}/src/app:app_config", - "${chip_root}/src/app:subscription-manager", ] } } diff --git a/src/app/icd/server/ICDManager.h b/src/app/icd/server/ICDManager.h index 09633567ebcdf5..9cf68d565b53e1 100644 --- a/src/app/icd/server/ICDManager.h +++ b/src/app/icd/server/ICDManager.h @@ -21,11 +21,11 @@ #include #if CHIP_CONFIG_ENABLE_ICD_CIP -#include // nogncheck #include // nogncheck #include // nogncheck #endif // CHIP_CONFIG_ENABLE_ICD_CIP +#include #include #include #include From bc676fb85f73bbf1a044d16b0af408778ef912a2 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Tue, 19 Mar 2024 09:05:48 +0800 Subject: [PATCH 10/29] fix unit test #ifdef to #if --- src/app/icd/server/ICDManager.h | 2 +- src/app/reporting/ReportScheduler.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/icd/server/ICDManager.h b/src/app/icd/server/ICDManager.h index 9cf68d565b53e1..e9d1138af77ed9 100644 --- a/src/app/icd/server/ICDManager.h +++ b/src/app/icd/server/ICDManager.h @@ -140,7 +140,7 @@ class ICDManager : public ICDListener void TriggerCheckInMessages(const std::function & verifier); #endif // CHIP_CONFIG_ENABLE_ICD_CIP -#ifdef CONFIG_BUILD_FOR_HOST_UNIT_TEST +#if CONFIG_BUILD_FOR_HOST_UNIT_TEST void SetTestFeatureMapValue(uint32_t featureMap) { mFeatureMap = featureMap; }; #endif diff --git a/src/app/reporting/ReportScheduler.h b/src/app/reporting/ReportScheduler.h index 5700eed6dfa025..e1cd348a09415d 100644 --- a/src/app/reporting/ReportScheduler.h +++ b/src/app/reporting/ReportScheduler.h @@ -213,7 +213,7 @@ class ReportScheduler : public ReadHandler::Observer, public ICDStateObserver /// @brief Get the number of ReadHandlers registered in the scheduler's node pool size_t GetNumReadHandlers() const { return mNodesPool.Allocated(); } -#ifdef CONFIG_BUILD_FOR_HOST_UNIT_TEST +#if CONFIG_BUILD_FOR_HOST_UNIT_TEST Timestamp GetMinTimestampForHandler(const ReadHandler * aReadHandler) { ReadHandlerNode * node = FindReadHandlerNode(aReadHandler); From b12eb7a4b5ecef72ac9fa80ad4c7ebb23add195e Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Tue, 19 Mar 2024 09:06:00 +0800 Subject: [PATCH 11/29] Add project config to ESP all-clusters-app to enable the unit test defines --- .../esp32/main/Kconfig.projbuild | 4 ++ .../esp32/main/include/CHIPProjectConfig.h | 38 +++++++++++++++++++ 2 files changed, 42 insertions(+) create mode 100644 examples/all-clusters-app/esp32/main/include/CHIPProjectConfig.h diff --git a/examples/all-clusters-app/esp32/main/Kconfig.projbuild b/examples/all-clusters-app/esp32/main/Kconfig.projbuild index 2cec0c32093734..e1d8805e689326 100644 --- a/examples/all-clusters-app/esp32/main/Kconfig.projbuild +++ b/examples/all-clusters-app/esp32/main/Kconfig.projbuild @@ -59,6 +59,10 @@ menu "Demo" depends on IDF_TARGET_ESP32H2 endchoice + config CHIP_PROJECT_CONFIG + string "CHIP Project Configuration file" + default "main/include/CHIPProjectConfig.h" + choice prompt "Rendezvous Mode" default RENDEZVOUS_MODE_BLE if BT_ENABLED diff --git a/examples/all-clusters-app/esp32/main/include/CHIPProjectConfig.h b/examples/all-clusters-app/esp32/main/include/CHIPProjectConfig.h new file mode 100644 index 00000000000000..c8ceb54ff6e618 --- /dev/null +++ b/examples/all-clusters-app/esp32/main/include/CHIPProjectConfig.h @@ -0,0 +1,38 @@ +/* + * + * Copyright (c) 2023 Project CHIP Authors + * All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * @file + * Example project configuration file for CHIP. + * + * This is a place to put application or project-specific overrides + * to the default configuration values for general CHIP features. + * + */ + +#pragma once + +/** + * @def CONFIG_BUILD_FOR_HOST_UNIT_TEST + * + * @brief Defines whether we're currently building for unit testing, which enables a set of features + * that are only utilized in those tests. This flag should not be enabled on devices. If you have a test + * that uses this flag, either appropriately conditionalize the entire test on this flag, or to exclude + * the compliation of that test source file entirely. + */ +#define CONFIG_BUILD_FOR_HOST_UNIT_TEST CONFIG_BUILD_CHIP_TESTS From be088b4c73b6246dc9c4ee67f506d505d6e3fe23 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Tue, 19 Mar 2024 10:27:54 +0800 Subject: [PATCH 12/29] force define to 1 --- .../all-clusters-app/esp32/main/include/CHIPProjectConfig.h | 2 +- src/app/tests/TestInteractionModelEngine.cpp | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/examples/all-clusters-app/esp32/main/include/CHIPProjectConfig.h b/examples/all-clusters-app/esp32/main/include/CHIPProjectConfig.h index c8ceb54ff6e618..9fff1bd10e67b4 100644 --- a/examples/all-clusters-app/esp32/main/include/CHIPProjectConfig.h +++ b/examples/all-clusters-app/esp32/main/include/CHIPProjectConfig.h @@ -35,4 +35,4 @@ * that uses this flag, either appropriately conditionalize the entire test on this flag, or to exclude * the compliation of that test source file entirely. */ -#define CONFIG_BUILD_FOR_HOST_UNIT_TEST CONFIG_BUILD_CHIP_TESTS +#define CONFIG_BUILD_FOR_HOST_UNIT_TEST 1 diff --git a/src/app/tests/TestInteractionModelEngine.cpp b/src/app/tests/TestInteractionModelEngine.cpp index 803dc9c5b08d21..589fb03e475ef9 100644 --- a/src/app/tests/TestInteractionModelEngine.cpp +++ b/src/app/tests/TestInteractionModelEngine.cpp @@ -23,13 +23,14 @@ */ #include +#include + #include #include #include #include #include #include -#include #include #include #include From 77abb2cba63b7a8a7ae17959959760ffefc4f994 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Tue, 19 Mar 2024 11:45:47 +0800 Subject: [PATCH 13/29] add CHIPCore.h include --- src/app/tests/TestInteractionModelEngine.cpp | 3 ++- src/app/tests/TestReadInteraction.cpp | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/app/tests/TestInteractionModelEngine.cpp b/src/app/tests/TestInteractionModelEngine.cpp index 589fb03e475ef9..550c83a26b7ed3 100644 --- a/src/app/tests/TestInteractionModelEngine.cpp +++ b/src/app/tests/TestInteractionModelEngine.cpp @@ -22,9 +22,10 @@ * */ -#include +// TODO(#32628): Remove the CHIPCore.h header when the esp32 build is correctly fixed #include +#include #include #include #include diff --git a/src/app/tests/TestReadInteraction.cpp b/src/app/tests/TestReadInteraction.cpp index 13ae20862a4c35..4b4a7628f53e8f 100644 --- a/src/app/tests/TestReadInteraction.cpp +++ b/src/app/tests/TestReadInteraction.cpp @@ -22,6 +22,9 @@ * */ +// TODO(#32628): Remove the CHIPCore.h header when the esp32 build is correctly fixed +#include + #include "lib/support/CHIPMem.h" #include #include From 57c0522bbedff766dd785f579a408690cedb0f23 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Tue, 19 Mar 2024 11:58:39 +0800 Subject: [PATCH 14/29] add include after ChipCore --- src/app/tests/TestReadInteraction.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/app/tests/TestReadInteraction.cpp b/src/app/tests/TestReadInteraction.cpp index 4b4a7628f53e8f..d4e4cc976b870a 100644 --- a/src/app/tests/TestReadInteraction.cpp +++ b/src/app/tests/TestReadInteraction.cpp @@ -22,7 +22,8 @@ * */ -// TODO(#32628): Remove the CHIPCore.h header when the esp32 build is correctly fixed +// TODO(#32628): Remove the CHIPCore.h header when the esp32 build is correctly fixed (next two includes) +#include #include #include "lib/support/CHIPMem.h" @@ -1372,8 +1373,7 @@ void TestReadInteraction::TestSetDirtyBetweenChunks(nlTestSuite * apSuite, void public: DirtyingMockDelegate(AttributePathParams (&aReadPaths)[2], int & aNumAttributeResponsesWhenSetDirty, int & aNumArrayItemsWhenSetDirty) : - mReadPaths(aReadPaths), - mNumAttributeResponsesWhenSetDirty(aNumAttributeResponsesWhenSetDirty), + mReadPaths(aReadPaths), mNumAttributeResponsesWhenSetDirty(aNumAttributeResponsesWhenSetDirty), mNumArrayItemsWhenSetDirty(aNumArrayItemsWhenSetDirty) {} From e31e5c4d7a92a5a3cd581838d6c98969a2ff6247 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Tue, 19 Mar 2024 12:02:39 +0800 Subject: [PATCH 15/29] restyle --- src/app/tests/TestReadInteraction.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/app/tests/TestReadInteraction.cpp b/src/app/tests/TestReadInteraction.cpp index d4e4cc976b870a..2db8f0b4685788 100644 --- a/src/app/tests/TestReadInteraction.cpp +++ b/src/app/tests/TestReadInteraction.cpp @@ -1373,7 +1373,8 @@ void TestReadInteraction::TestSetDirtyBetweenChunks(nlTestSuite * apSuite, void public: DirtyingMockDelegate(AttributePathParams (&aReadPaths)[2], int & aNumAttributeResponsesWhenSetDirty, int & aNumArrayItemsWhenSetDirty) : - mReadPaths(aReadPaths), mNumAttributeResponsesWhenSetDirty(aNumAttributeResponsesWhenSetDirty), + mReadPaths(aReadPaths), + mNumAttributeResponsesWhenSetDirty(aNumAttributeResponsesWhenSetDirty), mNumArrayItemsWhenSetDirty(aNumArrayItemsWhenSetDirty) {} From a90901b46218eab20af427f4a19bf8b0294fc303 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Tue, 19 Mar 2024 12:14:44 +0800 Subject: [PATCH 16/29] fix include ordering --- src/app/tests/TestReadInteraction.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/app/tests/TestReadInteraction.cpp b/src/app/tests/TestReadInteraction.cpp index 2db8f0b4685788..b914cd86adaa74 100644 --- a/src/app/tests/TestReadInteraction.cpp +++ b/src/app/tests/TestReadInteraction.cpp @@ -23,8 +23,9 @@ */ // TODO(#32628): Remove the CHIPCore.h header when the esp32 build is correctly fixed (next two includes) -#include #include +// add a line to avoid the reorder +#include #include "lib/support/CHIPMem.h" #include @@ -1373,8 +1374,7 @@ void TestReadInteraction::TestSetDirtyBetweenChunks(nlTestSuite * apSuite, void public: DirtyingMockDelegate(AttributePathParams (&aReadPaths)[2], int & aNumAttributeResponsesWhenSetDirty, int & aNumArrayItemsWhenSetDirty) : - mReadPaths(aReadPaths), - mNumAttributeResponsesWhenSetDirty(aNumAttributeResponsesWhenSetDirty), + mReadPaths(aReadPaths), mNumAttributeResponsesWhenSetDirty(aNumAttributeResponsesWhenSetDirty), mNumArrayItemsWhenSetDirty(aNumArrayItemsWhenSetDirty) {} From 4d1081c4f313ce388eeb34ca6889396cc5d5278f Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Tue, 19 Mar 2024 12:17:41 +0800 Subject: [PATCH 17/29] restyle --- src/app/tests/TestReadInteraction.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/app/tests/TestReadInteraction.cpp b/src/app/tests/TestReadInteraction.cpp index b914cd86adaa74..9ce6e81bad2b8b 100644 --- a/src/app/tests/TestReadInteraction.cpp +++ b/src/app/tests/TestReadInteraction.cpp @@ -1374,7 +1374,8 @@ void TestReadInteraction::TestSetDirtyBetweenChunks(nlTestSuite * apSuite, void public: DirtyingMockDelegate(AttributePathParams (&aReadPaths)[2], int & aNumAttributeResponsesWhenSetDirty, int & aNumArrayItemsWhenSetDirty) : - mReadPaths(aReadPaths), mNumAttributeResponsesWhenSetDirty(aNumAttributeResponsesWhenSetDirty), + mReadPaths(aReadPaths), + mNumAttributeResponsesWhenSetDirty(aNumAttributeResponsesWhenSetDirty), mNumArrayItemsWhenSetDirty(aNumArrayItemsWhenSetDirty) {} From 2f42fd06820dd205fad38de138684e013ddbd828 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Tue, 19 Mar 2024 12:45:39 +0800 Subject: [PATCH 18/29] move include --- src/app/InteractionModelEngine.h | 3 +++ src/app/reporting/ReportScheduler.h | 3 +++ src/app/tests/TestInteractionModelEngine.cpp | 3 --- src/app/tests/TestReadInteraction.cpp | 5 ----- 4 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h index 57d97f7517b085..0a095368f093df 100644 --- a/src/app/InteractionModelEngine.h +++ b/src/app/InteractionModelEngine.h @@ -25,6 +25,9 @@ #pragma once +// TODO(#32628): Remove the CHIPCore.h header when the esp32 build is correctly fixed (next two includes) +#include + #include #include #include diff --git a/src/app/reporting/ReportScheduler.h b/src/app/reporting/ReportScheduler.h index e1cd348a09415d..86bfc0110ac357 100644 --- a/src/app/reporting/ReportScheduler.h +++ b/src/app/reporting/ReportScheduler.h @@ -18,6 +18,9 @@ #pragma once +// TODO(#32628): Remove the CHIPCore.h header when the esp32 build is correctly fixed (next two includes) +#include + #include #include #include diff --git a/src/app/tests/TestInteractionModelEngine.cpp b/src/app/tests/TestInteractionModelEngine.cpp index 550c83a26b7ed3..562cafc9879794 100644 --- a/src/app/tests/TestInteractionModelEngine.cpp +++ b/src/app/tests/TestInteractionModelEngine.cpp @@ -22,9 +22,6 @@ * */ -// TODO(#32628): Remove the CHIPCore.h header when the esp32 build is correctly fixed -#include - #include #include #include diff --git a/src/app/tests/TestReadInteraction.cpp b/src/app/tests/TestReadInteraction.cpp index 9ce6e81bad2b8b..13ae20862a4c35 100644 --- a/src/app/tests/TestReadInteraction.cpp +++ b/src/app/tests/TestReadInteraction.cpp @@ -22,11 +22,6 @@ * */ -// TODO(#32628): Remove the CHIPCore.h header when the esp32 build is correctly fixed (next two includes) -#include -// add a line to avoid the reorder -#include - #include "lib/support/CHIPMem.h" #include #include From d86b5549c4f9ec18991c10b0a9c6ed790d3666d2 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Tue, 19 Mar 2024 13:10:39 +0800 Subject: [PATCH 19/29] add unit test define to qemu build --- src/test_driver/esp32/main/include/CHIPProjectConfig.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/test_driver/esp32/main/include/CHIPProjectConfig.h b/src/test_driver/esp32/main/include/CHIPProjectConfig.h index 3a25601cfd884e..1e4dc9626b5033 100644 --- a/src/test_driver/esp32/main/include/CHIPProjectConfig.h +++ b/src/test_driver/esp32/main/include/CHIPProjectConfig.h @@ -30,4 +30,6 @@ // Enable support functions for parsing command-line arguments #define CHIP_CONFIG_ENABLE_ARG_PARSER 1 +#define CONFIG_BUILD_FOR_HOST_UNIT_TEST 1 + #endif // CHIP_PROJECT_CONFIG_H From cd8efa64ad4b6361cd8d3cb65e1818cf486c77c6 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Tue, 19 Mar 2024 13:32:04 +0800 Subject: [PATCH 20/29] fix esp all-clusters-minimal build proble --- .../esp32/main/Kconfig.projbuild | 4 ++ .../esp32/main/include/CHIPProjectConfig.h | 38 +++++++++++++++++++ 2 files changed, 42 insertions(+) create mode 100644 examples/all-clusters-minimal-app/esp32/main/include/CHIPProjectConfig.h diff --git a/examples/all-clusters-minimal-app/esp32/main/Kconfig.projbuild b/examples/all-clusters-minimal-app/esp32/main/Kconfig.projbuild index 171af4f0ba2c24..9fe8f460e6b0a4 100644 --- a/examples/all-clusters-minimal-app/esp32/main/Kconfig.projbuild +++ b/examples/all-clusters-minimal-app/esp32/main/Kconfig.projbuild @@ -48,6 +48,10 @@ menu "Demo" depends on IDF_TARGET_ESP32C2 endchoice + config CHIP_PROJECT_CONFIG + string "CHIP Project Configuration file" + default "main/include/CHIPProjectConfig.h" + choice prompt "Rendezvous Mode" default RENDEZVOUS_MODE_BLE if BT_ENABLED diff --git a/examples/all-clusters-minimal-app/esp32/main/include/CHIPProjectConfig.h b/examples/all-clusters-minimal-app/esp32/main/include/CHIPProjectConfig.h new file mode 100644 index 00000000000000..9fff1bd10e67b4 --- /dev/null +++ b/examples/all-clusters-minimal-app/esp32/main/include/CHIPProjectConfig.h @@ -0,0 +1,38 @@ +/* + * + * Copyright (c) 2023 Project CHIP Authors + * All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * @file + * Example project configuration file for CHIP. + * + * This is a place to put application or project-specific overrides + * to the default configuration values for general CHIP features. + * + */ + +#pragma once + +/** + * @def CONFIG_BUILD_FOR_HOST_UNIT_TEST + * + * @brief Defines whether we're currently building for unit testing, which enables a set of features + * that are only utilized in those tests. This flag should not be enabled on devices. If you have a test + * that uses this flag, either appropriately conditionalize the entire test on this flag, or to exclude + * the compliation of that test source file entirely. + */ +#define CONFIG_BUILD_FOR_HOST_UNIT_TEST 1 From 58ae2e1d7c3d9ef66b8a10535b65cbf039b49970 Mon Sep 17 00:00:00 2001 From: mkardous-silabs <84793247+mkardous-silabs@users.noreply.github.com> Date: Wed, 20 Mar 2024 08:04:58 +0800 Subject: [PATCH 21/29] Apply suggestions from code review Co-authored-by: Boris Zbarsky --- src/app/icd/server/ICDManager.cpp | 4 ++-- src/app/icd/server/ICDManager.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/app/icd/server/ICDManager.cpp b/src/app/icd/server/ICDManager.cpp index 6445bef4ad98d0..b61d266382a5b9 100644 --- a/src/app/icd/server/ICDManager.cpp +++ b/src/app/icd/server/ICDManager.cpp @@ -249,8 +249,8 @@ bool ICDManager::CheckInWouldBeSentAtActiveModeVerifier(FabricIndex aFabricIndex VerifyOrReturnValue(mSubInfoProvider->SubjectHasActiveSubscription(aFabricIndex, subjectID), true); #if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS - // At least one registration has a persisted entry. Do not send Check-In message. - // This is to cover the use-case where the subscription resumption feature is used with the Check-In message. + // If at least one registration has a persisted entry, do not send Check-In message. + // The resumption of the persisted subscription will serve the same function a check-in would have served. VerifyOrReturnValue(mSubInfoProvider->SubjectHasPersistedSubscription(aFabricIndex, subjectID), true); #endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS return false; diff --git a/src/app/icd/server/ICDManager.h b/src/app/icd/server/ICDManager.h index e9d1138af77ed9..e8b4f957f15b29 100644 --- a/src/app/icd/server/ICDManager.h +++ b/src/app/icd/server/ICDManager.h @@ -85,7 +85,7 @@ class ICDManager : public ICDListener * This type can be used to implement specific verifiers that can be used in the CheckInMessagesWouldBeSent function. * The goal is to avoid having multiple functions that implement the iterator loop with only the check changing. * - * @return true if at least one Check-In message wuld be sent + * @return true if at least one Check-In message would be sent * false No Check-In messages would be sent */ From 07b7a4272c67dc99e30ea0699bed052d119b6517 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Sun, 24 Mar 2024 13:45:43 +0300 Subject: [PATCH 22/29] Fix comment --- src/app/InteractionModelEngine.h | 2 +- src/app/reporting/ReportScheduler.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h index 0a095368f093df..c3fd66ad82487c 100644 --- a/src/app/InteractionModelEngine.h +++ b/src/app/InteractionModelEngine.h @@ -25,7 +25,7 @@ #pragma once -// TODO(#32628): Remove the CHIPCore.h header when the esp32 build is correctly fixed (next two includes) +// TODO(#32628): Remove the CHIPCore.h header when the esp32 build is correctly fixed #include #include diff --git a/src/app/reporting/ReportScheduler.h b/src/app/reporting/ReportScheduler.h index 86bfc0110ac357..f0e6ce3ff82f27 100644 --- a/src/app/reporting/ReportScheduler.h +++ b/src/app/reporting/ReportScheduler.h @@ -18,7 +18,7 @@ #pragma once -// TODO(#32628): Remove the CHIPCore.h header when the esp32 build is correctly fixed (next two includes) +// TODO(#32628): Remove the CHIPCore.h header when the esp32 build is correctly fixed #include #include From 5fe51f328fdaf3ac64a0fb09abd257ad035bd920 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Tue, 26 Mar 2024 14:49:33 -0400 Subject: [PATCH 23/29] Finish refactor of the Sub logic when interacting with the ICDManager --- src/app/BUILD.gn | 1 + src/app/InteractionModelEngine.cpp | 35 +++++-- src/app/InteractionModelEngine.h | 24 ++++- ...bscriptionResumptionSessionEstablisher.cpp | 13 ++- src/app/icd/server/BUILD.gn | 5 + src/app/icd/server/ICDManager.cpp | 82 ++++++++++++++-- src/app/icd/server/ICDManager.h | 13 +++ src/app/server/Server.cpp | 15 ++- src/app/tests/TestICDManager.cpp | 96 +++++++++++++++++-- src/app/tests/TestInteractionModelEngine.cpp | 61 ++++++++++++ 10 files changed, 312 insertions(+), 33 deletions(-) diff --git a/src/app/BUILD.gn b/src/app/BUILD.gn index c7ebe3a377df7b..87508380a08083 100644 --- a/src/app/BUILD.gn +++ b/src/app/BUILD.gn @@ -210,6 +210,7 @@ static_library("interaction-model") { ":subscription-info-provider", "${chip_root}/src/app/MessageDef", "${chip_root}/src/app/icd/server:icd-server-config", + "${chip_root}/src/app/icd/server:manager", "${chip_root}/src/app/icd/server:observer", "${chip_root}/src/app/util:af-types", "${chip_root}/src/app/util:callbacks", diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index f6c67e24ea04b9..c19357b1615514 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -365,8 +365,10 @@ bool InteractionModelEngine::SubjectHasPersistedSubscription(FabricIndex aFabric #if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS auto * iterator = mpSubscriptionResumptionStorage->IterateSubscriptions(); - SubscriptionResumptionStorage::SubscriptionInfo subscriptionInfo; + // Verify that we were able t0 allocate an iterator. If not, assume we have a persisted subscription. + VerifyOrReturnValue(iterator, true); + SubscriptionResumptionStorage::SubscriptionInfo subscriptionInfo; while (iterator->Next(subscriptionInfo)) { // TODO(#31873): Persistent subscription only stores the NodeID for now. We cannot check if the CAT matches @@ -1924,22 +1926,22 @@ CHIP_ERROR InteractionModelEngine::ResumeSubscriptions() // future improvements: https://github.com/project-chip/connectedhomeip/issues/25439 SubscriptionResumptionStorage::SubscriptionInfo subscriptionInfo; - auto * iterator = mpSubscriptionResumptionStorage->IterateSubscriptions(); - int subscriptionsToResume = 0; - uint16_t minInterval = 0; + auto * iterator = mpSubscriptionResumptionStorage->IterateSubscriptions(); + mNumOfSubscriptionToResume = 0; + uint16_t minInterval = 0; while (iterator->Next(subscriptionInfo)) { - subscriptionsToResume++; + mNumOfSubscriptionToResume++; minInterval = std::max(minInterval, subscriptionInfo.mMinInterval); } iterator->Release(); - if (subscriptionsToResume) + if (mNumOfSubscriptionToResume) { #if CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION mSubscriptionResumptionScheduled = true; #endif - ChipLogProgress(InteractionModel, "Resuming %d subscriptions in %u seconds", subscriptionsToResume, minInterval); + ChipLogProgress(InteractionModel, "Resuming %d subscriptions in %u seconds", mNumOfSubscriptionToResume, minInterval); ReturnErrorOnFailure(mpExchangeMgr->GetSessionManager()->SystemLayer()->StartTimer(System::Clock::Seconds16(minInterval), ResumeSubscriptionsTimerCallback, this)); } @@ -2061,5 +2063,24 @@ bool InteractionModelEngine::HasSubscriptionsToResume() } #endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION +#if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS +void InteractionModelEngine::DecrementNumSubscriptionToResume() +{ + VerifyOrReturn(mNumOfSubscriptionToResume > 0); +#if CHIP_CONFIG_ENABLE_ICD_CIP && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION + VerifyOrDie(mICDManager); +#endif // CHIP_CONFIG_ENABLE_ICD_CIP && CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION + + mNumOfSubscriptionToResume--; + +#if CHIP_CONFIG_ENABLE_ICD_CIP && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION + if (!mNumOfSubscriptionToResume) + { + mICDManager->SetBootUpResumeSubscriptionExecuted(); + } +#endif // CHIP_CONFIG_ENABLE_ICD_CIP && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION +} +#endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS + } // namespace app } // namespace chip diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h index c3fd66ad82487c..86a266fbad73a2 100644 --- a/src/app/InteractionModelEngine.h +++ b/src/app/InteractionModelEngine.h @@ -50,6 +50,7 @@ #include #include #include +#include #include #include #include @@ -69,6 +70,10 @@ #include +#if CHIP_CONFIG_ENABLE_ICD_SERVER +#include // nogncheck +#endif // CHIP_CONFIG_ENABLE_ICD_SERVER + namespace chip { namespace app { @@ -130,6 +135,10 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, void Shutdown(); +#if CHIP_CONFIG_ENABLE_ICD_SERVER + void SetICDManager(ICDManager * manager) { mICDManager = manager; }; +#endif // CHIP_CONFIG_ENABLE_ICD_SERVER + Messaging::ExchangeManager * GetExchangeManager(void) const { return mpExchangeMgr; } /** @@ -320,6 +329,10 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, bool SubjectHasPersistedSubscription(FabricIndex aFabricIndex, NodeId subjectID) override; +#if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS + void DecrementNumSubscriptionToResume(); +#endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS + #if CONFIG_BUILD_FOR_HOST_UNIT_TEST // // Get direct access to the underlying read handler pool @@ -605,6 +618,10 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, CommandHandlerInterface * mCommandHandlerList = nullptr; +#if CHIP_CONFIG_ENABLE_ICD_SERVER + ICDManager * mICDManager = nullptr; +#endif // CHIP_CONFIG_ENABLE_ICD_SERVER + ObjectPool mCommandResponderObjs; ObjectPool mTimedHandlers; WriteHandler mWriteHandlers[CHIP_IM_MAX_NUM_WRITE_HANDLER]; @@ -663,12 +680,15 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, #endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION #endif // CONFIG_BUILD_FOR_HOST_UNIT_TEST -#if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION +#if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS + int8_t mNumOfSubscriptionToResume = 0; +#if CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION bool HasSubscriptionsToResume(); uint32_t ComputeTimeSecondsTillNextSubscriptionResumption(); uint32_t mNumSubscriptionResumptionRetries = 0; bool mSubscriptionResumptionScheduled = false; -#endif +#endif // CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION +#endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS FabricTable * mpFabricTable; diff --git a/src/app/SubscriptionResumptionSessionEstablisher.cpp b/src/app/SubscriptionResumptionSessionEstablisher.cpp index 3c6b3969512c89..393ca3d4c6aae5 100644 --- a/src/app/SubscriptionResumptionSessionEstablisher.cpp +++ b/src/app/SubscriptionResumptionSessionEstablisher.cpp @@ -90,15 +90,21 @@ void SubscriptionResumptionSessionEstablisher::HandleDeviceConnected(void * cont AutoDeleteEstablisher establisher(static_cast(context)); SubscriptionResumptionStorage::SubscriptionInfo & subscriptionInfo = establisher->mSubscriptionInfo; InteractionModelEngine * imEngine = InteractionModelEngine::GetInstance(); + + // Decrement the number of subscriptions to resume + imEngine->DecrementNumSubscriptionToResume(); + if (!imEngine->EnsureResourceForSubscription(subscriptionInfo.mFabricIndex, subscriptionInfo.mAttributePaths.AllocatedSize(), subscriptionInfo.mEventPaths.AllocatedSize())) { + // TODO - Should we keep the subscription here? ChipLogProgress(InteractionModel, "no resource for subscription resumption"); return; } ReadHandler * readHandler = imEngine->mReadHandlers.CreateObject(*imEngine, imEngine->GetReportScheduler()); if (readHandler == nullptr) { + // TODO - Should we keep the subscription here? ChipLogProgress(InteractionModel, "no resource for ReadHandler creation"); return; } @@ -118,10 +124,15 @@ void SubscriptionResumptionSessionEstablisher::HandleDeviceConnectionFailure(voi CHIP_ERROR error) { AutoDeleteEstablisher establisher(static_cast(context)); + InteractionModelEngine * imEngine = InteractionModelEngine::GetInstance(); SubscriptionResumptionStorage::SubscriptionInfo & subscriptionInfo = establisher->mSubscriptionInfo; ChipLogError(DataManagement, "Failed to establish CASE for subscription-resumption with error '%" CHIP_ERROR_FORMAT "'", error.Format()); - auto * subscriptionResumptionStorage = InteractionModelEngine::GetInstance()->GetSubscriptionResumptionStorage(); + + // Decrement the number of subscriptions to resume + imEngine->DecrementNumSubscriptionToResume(); + + auto * subscriptionResumptionStorage = imEngine->GetSubscriptionResumptionStorage(); if (!subscriptionResumptionStorage) { ChipLogError(DataManagement, "Failed to get subscription resumption storage"); diff --git a/src/app/icd/server/BUILD.gn b/src/app/icd/server/BUILD.gn index a73784152f482f..45cc4a845c8965 100644 --- a/src/app/icd/server/BUILD.gn +++ b/src/app/icd/server/BUILD.gn @@ -22,6 +22,11 @@ buildconfig_header("icd-server-buildconfig") { header = "ICDServerBuildConfig.h" header_dir = "app/icd/server" + if (chip_enable_icd_lit || chip_enable_icd_checkin || + chip_enable_icd_user_active_mode_trigger) { + assert(chip_enable_icd_lit) + } + if (chip_enable_icd_lit) { assert(chip_enable_icd_checkin && chip_enable_icd_user_active_mode_trigger) } diff --git a/src/app/icd/server/ICDManager.cpp b/src/app/icd/server/ICDManager.cpp index b61d266382a5b9..cec70780c57737 100644 --- a/src/app/icd/server/ICDManager.cpp +++ b/src/app/icd/server/ICDManager.cpp @@ -105,6 +105,10 @@ void ICDManager::Shutdown() mFabricTable = nullptr; mSubInfoProvider = nullptr; mICDSenderPool.ReleaseAll(); + +#if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION + mIsBootUpResumeSubscriptionExecuted = false; +#endif // !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION #endif // CHIP_CONFIG_ENABLE_ICD_CIP } @@ -152,6 +156,7 @@ void ICDManager::SendCheckInMsgs() ICDMonitoringTable table(*mStorage, fabricInfo.GetFabricIndex(), supported_clients /*Table entry limit*/, mSymmetricKeystore); + if (table.IsEmpty()) { continue; @@ -173,8 +178,7 @@ void ICDManager::SendCheckInMsgs() continue; } - bool active = mSubInfoProvider->SubjectHasActiveSubscription(entry.fabricIndex, entry.monitoredSubject); - if (active) + if (!CheckInWouldBeSentAtActiveModeVerifier(entry.fabricIndex, entry.monitoredSubject)) { continue; } @@ -244,17 +248,77 @@ bool ICDManager::CheckInMessagesWouldBeSent(std::functionSubjectHasActiveSubscription(aFabricIndex, subjectID) || + mSubInfoProvider->SubjectHasPersistedSubscription(aFabricIndex, subjectID)); +} +#else +/** + * @brief Implementation for when the persistant subscription is present without the subscription timeout resumption feature. + * Verifier checks that there no active subscriptions. If the boot up subscription resumption has been completed, + * verifier also checks if there are persisted subscriptions. + * + * @note The persistent subscriptions feature tries to resume subscriptions at the highest min interval + * of all the persisted subscriptions. As such, it is possible for the ICD to return to Idle Mode + * until the timer elaspses. We do not when to send Check-In message to clients with persisted subscriptions + * until we have tried to resubscribe. + * + * @param aFabricIndex + * @param subjectID subjectID to check. Can be an opperationnal node id or a CAT + * + * @return true Returns true if the fabricIndex and subjectId combination does not have an active subscription. + * If the boot up susbscription has not been completed, there must not be a persisted subscription either. + * @return false Returns false if the fabricIndex and subjectId combination has an active subscription. + * If the boot up susbscription has not been completed, + * if the fabricIndex and subjectId combination has a persisted subscription. + */ bool ICDManager::CheckInWouldBeSentAtActiveModeVerifier(FabricIndex aFabricIndex, NodeId subjectID) { - VerifyOrReturnValue(mSubInfoProvider->SubjectHasActiveSubscription(aFabricIndex, subjectID), true); + bool wouldSendCheckIn = !(mSubInfoProvider->SubjectHasActiveSubscription(aFabricIndex, subjectID)); -#if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS - // If at least one registration has a persisted entry, do not send Check-In message. - // The resumption of the persisted subscription will serve the same function a check-in would have served. - VerifyOrReturnValue(mSubInfoProvider->SubjectHasPersistedSubscription(aFabricIndex, subjectID), true); -#endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS - return false; + if (!mIsBootUpResumeSubscriptionExecuted) + { + wouldSendCheckIn = wouldSendCheckIn && !mSubInfoProvider->SubjectHasPersistedSubscription(aFabricIndex, subjectID); + } + + return wouldSendCheckIn; +} +#endif // CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION +#else +/** + * @brief Implementation for when neither the persistant subscription and subscription timeout resumption feature are present. + * Verifier checks that there no active sbuscriptions for a given fabricIndex and subjectId combination. + * + * @param aFabricIndex + * @param subjectID subjectID to check. Can be an opperationnal node id or a CAT + * + * @return true Returns true if the fabricIndex and subjectId combination does not have an active subscription. + * @return false Returns false if the fabricIndex and subjectId combination has an active subscription. + */ +bool ICDManager::CheckInWouldBeSentAtActiveModeVerifier(FabricIndex aFabricIndex, NodeId subjectID) +{ + return !(mSubInfoProvider->SubjectHasActiveSubscription(aFabricIndex, subjectID)); } +#endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS void ICDManager::TriggerCheckInMessages(const std::function & verifier) { diff --git a/src/app/icd/server/ICDManager.h b/src/app/icd/server/ICDManager.h index e8b4f957f15b29..b644e2d501eda3 100644 --- a/src/app/icd/server/ICDManager.h +++ b/src/app/icd/server/ICDManager.h @@ -138,10 +138,20 @@ class ICDManager : public ICDListener * @param[in] algo Verifier function to use to determine if we need to send check-in messages */ void TriggerCheckInMessages(const std::function & verifier); + +#if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION + /** + * @brief Set mSubCheckInBootCheckExecuted to true + * Function allows the InteractionModelEngine to notify the ICDManager that the boot up subscription resumption has been + * completed. + */ + void SetBootUpResumeSubscriptionExecuted() { mIsBootUpResumeSubscriptionExecuted = true; }; +#endif // !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION && CHIP_CONFIG_PERSIST_SUBSCRIPTIONS #endif // CHIP_CONFIG_ENABLE_ICD_CIP #if CONFIG_BUILD_FOR_HOST_UNIT_TEST void SetTestFeatureMapValue(uint32_t featureMap) { mFeatureMap = featureMap; }; + bool GetIsBootUpResumeSubscriptionExecuted() { return mIsBootUpResumeSubscriptionExecuted; }; #endif // Implementation of ICDListener functions. @@ -212,6 +222,9 @@ class ICDManager : public ICDListener ObjectPool mStateObserverPool; #if CHIP_CONFIG_ENABLE_ICD_CIP +#if !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION && CHIP_CONFIG_PERSIST_SUBSCRIPTIONS + bool mIsBootUpResumeSubscriptionExecuted = false; +#endif // !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION PersistentStorageDelegate * mStorage = nullptr; FabricTable * mFabricTable = nullptr; Messaging::ExchangeManager * mExchangeManager = nullptr; diff --git a/src/app/server/Server.cpp b/src/app/server/Server.cpp index 2dbd1e444842b7..4a67cf562a94a4 100644 --- a/src/app/server/Server.cpp +++ b/src/app/server/Server.cpp @@ -333,6 +333,10 @@ CHIP_ERROR Server::Init(const ServerInitParams & initParams) &mCASESessionManager, mSubscriptionResumptionStorage); SuccessOrExit(err); +#if CHIP_CONFIG_ENABLE_ICD_SERVER + chip::app::InteractionModelEngine::GetInstance()->SetICDManager(&mICDManager); +#endif // CHIP_CONFIG_ENABLE_ICD_SERVER + // ICD Init needs to be after data model init and InteractionModel Init #if CHIP_CONFIG_ENABLE_ICD_SERVER @@ -453,7 +457,7 @@ void Server::OnPlatformEvent(const DeviceLayer::ChipDeviceEvent & event) #endif // CHIP_CONFIG_ENABLE_ICD_SERVER && CHIP_CONFIG_ENABLE_ICD_CIP #if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS ResumeSubscriptions(); -#endif +#endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS break; #if CHIP_SYSTEM_CONFIG_USE_OPEN_THREAD_ENDPOINT case DeviceEventType::kThreadConnectivityChange: @@ -525,9 +529,9 @@ void Server::RejoinExistingMulticastGroups() bool Server::CheckInWouldBeSentAtBootVerifier(FabricIndex aFabricIndex, NodeId subjectID) { #if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS - // At least one registration has a persisted entry. Do not send Check-In message. - // This is to cover the use-case where the subscription resumption feature is used with the Check-In message. - VerifyOrReturnValue(!chip::app::InteractionModelEngine::GetInstance()->SubjectHasPersistedSubscription(aFabricIndex, subjectID), + // If at least one registration has a persisted entry, do not send Check-In message. + // The resumption of the persisted subscription will serve the same function a check-in would have served. + VerifyOrReturnValue(!app::InteractionModelEngine::GetInstance()->SubjectHasPersistedSubscription(aFabricIndex, subjectID), false); #endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS @@ -577,6 +581,9 @@ void Server::Shutdown() chip::Dnssd::Resolver::Instance().Shutdown(); chip::app::InteractionModelEngine::GetInstance()->Shutdown(); +#if CHIP_CONFIG_ENABLE_ICD_SERVER + chip::app::InteractionModelEngine::GetInstance()->SetICDManager(nullptr); +#endif // CHIP_CONFIG_ENABLE_ICD_SERVER mCommissioningWindowManager.Shutdown(); mMessageCounterManager.Shutdown(); mExchangeMgr.Shutdown(); diff --git a/src/app/tests/TestICDManager.cpp b/src/app/tests/TestICDManager.cpp index ba311f889989f8..10530e36b86617 100644 --- a/src/app/tests/TestICDManager.cpp +++ b/src/app/tests/TestICDManager.cpp @@ -48,10 +48,10 @@ namespace { constexpr uint16_t kMaxTestClients = 2; constexpr FabricIndex kTestFabricIndex1 = 1; constexpr FabricIndex kTestFabricIndex2 = kMaxValidFabricIndex; -constexpr uint64_t kClientNodeId11 = 0x100001; -constexpr uint64_t kClientNodeId12 = 0x100002; -constexpr uint64_t kClientNodeId21 = 0x200001; -constexpr uint64_t kClientNodeId22 = 0x200002; +constexpr NodeId kClientNodeId11 = 0x100001; +constexpr NodeId kClientNodeId12 = 0x100002; +constexpr NodeId kClientNodeId21 = 0x200001; +constexpr NodeId kClientNodeId22 = 0x200002; constexpr uint8_t kKeyBuffer1a[] = { 0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18, 0x19, 0x1a, 0x1b, 0x1c, 0x1d, 0x1e, 0x1f @@ -80,13 +80,15 @@ class TestSubscriptionsInfoProvider : public SubscriptionsInfoProvider TestSubscriptionsInfoProvider() = default; ~TestSubscriptionsInfoProvider(){}; - void SetReturnValue(bool value) { mReturnValue = value; }; + void SetHasActiveSubscription(bool value) { mHasActiveSubscription = value; }; + void SetHasPersistedSubscription(bool value) { mHasPersistedSubscription = value; }; - bool SubjectHasActiveSubscription(FabricIndex aFabricIndex, NodeId subject) { return mReturnValue; }; - bool SubjectHasPersistedSubscription(FabricIndex aFabricIndex, NodeId subject) { return mReturnValue; }; + bool SubjectHasActiveSubscription(FabricIndex aFabricIndex, NodeId subject) { return mHasActiveSubscription; }; + bool SubjectHasPersistedSubscription(FabricIndex aFabricIndex, NodeId subject) { return mHasPersistedSubscription; }; private: - bool mReturnValue = false; + bool mHasActiveSubscription = false; + bool mHasPersistedSubscription = false; }; class TestContext : public chip::Test::AppContext @@ -197,7 +199,8 @@ class TestICDManager ctx->mICDManager.SetTestFeatureMapValue(0x07); // Set that there are no matching subscriptions - ctx->mSubInfoProvider.SetReturnValue(false); + ctx->mSubInfoProvider.SetHasActiveSubscription(false); + ctx->mSubInfoProvider.SetHasPersistedSubscription(false); // Set New durations for test case Milliseconds32 oldActiveModeDuration = icdConfigData.GetActiveModeDuration(); @@ -276,7 +279,8 @@ class TestICDManager ctx->mICDManager.SetTestFeatureMapValue(0x07); // Set that there are not matching subscriptions - ctx->mSubInfoProvider.SetReturnValue(true); + ctx->mSubInfoProvider.SetHasActiveSubscription(true); + ctx->mSubInfoProvider.SetHasPersistedSubscription(true); // Set New durations for test case Milliseconds32 oldActiveModeDuration = icdConfigData.GetActiveModeDuration(); @@ -617,6 +621,77 @@ class TestICDManager // confirm the promised time is 20000 since the device is already planing to stay active longer than the requested time NL_TEST_ASSERT(aSuite, stayActivePromisedMs == 20000); } + +#if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS +#if CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION + static void TestCheckInWouldBeSentAtActiveModeVerifier(nlTestSuite * aSuite, void * aContext) + { + TestContext * ctx = static_cast(aContext); + + // Test 1 - Has no ActiveSubscription & no persisted subscription + ctx->mSubInfoProvider.SetHasActiveSubscription(false); + ctx->mSubInfoProvider.SetHasPersistedSubscription(false); + NL_TEST_ASSERT(aSuite, ctx->mICDManager.CheckInWouldBeSentAtActiveModeVerifier(kTestFabricIndex1, kClientNodeId11)); + + // Test 2 - Has no active subscription & a persisted subscription + ctx->mSubInfoProvider.SetHasActiveSubscription(false); + ctx->mSubInfoProvider.SetHasPersistedSubscription(true); + NL_TEST_ASSERT(aSuite, !(ctx->mICDManager.CheckInWouldBeSentAtActiveModeVerifier(kTestFabricIndex1, kClientNodeId11))); + + // Test 3 - Has an active subscription & a persisted subscription + ctx->mSubInfoProvider.SetHasActiveSubscription(true); + ctx->mSubInfoProvider.SetHasPersistedSubscription(true); + NL_TEST_ASSERT(aSuite, !(ctx->mICDManager.CheckInWouldBeSentAtActiveModeVerifier(kTestFabricIndex1, kClientNodeId11))); + } +#else + static void TestCheckInWouldBeSentAtActiveModeVerifier(nlTestSuite * aSuite, void * aContext) + { + TestContext * ctx = static_cast(aContext); + + // Test 1 - Has no active subscription and no persisted subscription at boot up + ctx->mSubInfoProvider.SetHasActiveSubscription(false); + ctx->mSubInfoProvider.SetHasPersistedSubscription(false); + ctx->mICDManager.mIsBootUpResumeSubscriptionExecuted = false; + NL_TEST_ASSERT(aSuite, ctx->mICDManager.CheckInWouldBeSentAtActiveModeVerifier(kTestFabricIndex1, kClientNodeId11)); + + // Test 2 - Has no active subscription and a persisted subscription at boot up + ctx->mSubInfoProvider.SetHasActiveSubscription(false); + ctx->mSubInfoProvider.SetHasPersistedSubscription(true); + ctx->mICDManager.mIsBootUpResumeSubscriptionExecuted = false; + NL_TEST_ASSERT(aSuite, !(ctx->mICDManager.CheckInWouldBeSentAtActiveModeVerifier(kTestFabricIndex1, kClientNodeId11))); + + // Test 3 - Has an active subscription and a persisted subscription during normal operations + ctx->mSubInfoProvider.SetHasActiveSubscription(true); + ctx->mSubInfoProvider.SetHasPersistedSubscription(true); + ctx->mICDManager.mIsBootUpResumeSubscriptionExecuted = true; + NL_TEST_ASSERT(aSuite, !(ctx->mICDManager.CheckInWouldBeSentAtActiveModeVerifier(kTestFabricIndex1, kClientNodeId11))); + + // Test 4 - Has no active subscription and a persisted subscription during normal operations + ctx->mSubInfoProvider.SetHasActiveSubscription(false); + ctx->mSubInfoProvider.SetHasPersistedSubscription(true); + ctx->mICDManager.mIsBootUpResumeSubscriptionExecuted = true; + NL_TEST_ASSERT(aSuite, ctx->mICDManager.CheckInWouldBeSentAtActiveModeVerifier(kTestFabricIndex1, kClientNodeId11)); + } +#endif // CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION +#else + static void TestCheckInWouldBeSentAtActiveModeVerifier(nlTestSuite * aSuite, void * aContext) + { + TestContext * ctx = static_cast(aContext); + + // Test 1 - Has an active subscription + ctx->mSubInfoProvider.SetHasActiveSubscription(true); + NL_TEST_ASSERT(aSuite, + ctx->mICDManager.CheckInWouldBeSentAtActiveModeVerifier(kTestFabricIndex1, kClientNodeId11) == false); + + // Test 2 - Has no active subscription + ctx->mSubInfoProvider.SetHasActiveSubscription(false); + NL_TEST_ASSERT(aSuite, ctx->mICDManager.CheckInWouldBeSentAtActiveModeVerifier(kTestFabricIndex1, kClientNodeId11)); + + // Test 3 - Make sure that the persisted subscription has no impact + ctx->mSubInfoProvider.SetHasPersistedSubscription(true); + NL_TEST_ASSERT(aSuite, ctx->mICDManager.CheckInWouldBeSentAtActiveModeVerifier(kTestFabricIndex1, kClientNodeId11)); + } +#endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS }; } // namespace app @@ -634,6 +709,7 @@ static const nlTest sTests[] = { NL_TEST_DEF("TestICDMRegisterUnregisterEvents", TestICDManager::TestICDMRegisterUnregisterEvents), NL_TEST_DEF("TestICDCounter", TestICDManager::TestICDCounter), NL_TEST_DEF("TestICDStayActive", TestICDManager::TestICDMStayActive), + NL_TEST_DEF("TestCheckInWouldBeSentAtActiveModeVerifier", TestICDManager::TestCheckInWouldBeSentAtActiveModeVerifier), NL_TEST_SENTINEL(), }; diff --git a/src/app/tests/TestInteractionModelEngine.cpp b/src/app/tests/TestInteractionModelEngine.cpp index 562cafc9879794..370117032c424e 100644 --- a/src/app/tests/TestInteractionModelEngine.cpp +++ b/src/app/tests/TestInteractionModelEngine.cpp @@ -24,6 +24,7 @@ #include #include +#include #include #include #include @@ -34,6 +35,7 @@ #include #include #include +#include #include #include #include @@ -82,6 +84,7 @@ class TestInteractionModelEngine static void TestSubjectHasActiveSubscriptionMultipleSubsSingleEntry(nlTestSuite * apSuite, void * apContext); static void TestSubjectHasActiveSubscriptionMultipleSubsMultipleEntries(nlTestSuite * apSuite, void * apContext); static void TestSubjectHasActiveSubscriptionSubWithCAT(nlTestSuite * apSuite, void * apContext); + static void TestDecrementNumSubscriptionToResume(nlTestSuite * apSuite, void * apContext); }; int TestInteractionModelEngine::GetAttributePathListLength(SingleLinkedListNode * apAttributePathParamsList) @@ -682,6 +685,63 @@ void TestInteractionModelEngine::TestSubscriptionResumptionTimer(nlTestSuite * a #endif // CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION #endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS +void TestInteractionModelEngine::TestDecrementNumSubscriptionToResume(nlTestSuite * apSuite, void * apContext) +{ + TestContext & ctx = *static_cast(apContext); + CHIP_ERROR err = CHIP_NO_ERROR; + InteractionModelEngine * engine = InteractionModelEngine::GetInstance(); + constexpr uint8_t kNumberOfSubsToResume = 5; + uint8_t numberOfSubsRemaining = kNumberOfSubsToResume; + + err = engine->Init(&ctx.GetExchangeManager(), &ctx.GetFabricTable(), app::reporting::GetDefaultReportScheduler()); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + +#if CHIP_CONFIG_ENABLE_ICD_CIP && CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION + ICDManager manager; + engine->SetICDManager(&manager); +#endif // CHIP_CONFIG_ENABLE_ICD_CIP && CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION + + // Set number of subs + engine->mNumOfSubscriptionToResume = kNumberOfSubsToResume; + +#if CHIP_CONFIG_ENABLE_ICD_CIP && CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION + // Verify mIsBootUpResumeSubscriptionExecuted has not been set + NL_TEST_ASSERT(apSuite, !manager.GetIsBootUpResumeSubscriptionExecuted()); +#endif // CHIP_CONFIG_ENABLE_ICD_CIP && CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION + + // Decrease number of subs by 1 + numberOfSubsRemaining--; + engine->DecrementNumSubscriptionToResume(); + NL_TEST_ASSERT_EQUALS(apSuite, numberOfSubsRemaining, engine->mNumOfSubscriptionToResume); + + // Decrease to 0 subs remaining + while (numberOfSubsRemaining > 0) + { +#if CHIP_CONFIG_ENABLE_ICD_CIP && CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION + // Verify mIsBootUpResumeSubscriptionExecuted has not been set + NL_TEST_ASSERT(apSuite, !manager.GetIsBootUpResumeSubscriptionExecuted()); +#endif // CHIP_CONFIG_ENABLE_ICD_CIP && CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION + + numberOfSubsRemaining--; + engine->DecrementNumSubscriptionToResume(); + NL_TEST_ASSERT_EQUALS(apSuite, numberOfSubsRemaining, engine->mNumOfSubscriptionToResume); + } + +#if CHIP_CONFIG_ENABLE_ICD_CIP && CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION + // Verify mIsBootUpResumeSubscriptionExecuted has been set + NL_TEST_ASSERT(apSuite, manager.GetIsBootUpResumeSubscriptionExecuted()); +#endif // CHIP_CONFIG_ENABLE_ICD_CIP && CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION + + // Make sure we don't rollover / go negative + engine->DecrementNumSubscriptionToResume(); + NL_TEST_ASSERT_EQUALS(apSuite, numberOfSubsRemaining, engine->mNumOfSubscriptionToResume); + + // Clean up +#if CHIP_CONFIG_ENABLE_ICD_CIP && CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION + engine->SetICDManager(nullptr); +#endif // CHIP_CONFIG_ENABLE_ICD_CIP && CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION +} + } // namespace app } // namespace chip @@ -703,6 +763,7 @@ const nlTest sTests[] = NL_TEST_DEF("TestSubjectHasActiveSubscriptionMultipleSubsSingleEntry", chip::app::TestInteractionModelEngine::TestSubjectHasActiveSubscriptionMultipleSubsSingleEntry), NL_TEST_DEF("TestSubjectHasActiveSubscriptionMultipleSubsMultipleEntries", chip::app::TestInteractionModelEngine::TestSubjectHasActiveSubscriptionMultipleSubsMultipleEntries), NL_TEST_DEF("TestSubjectHasActiveSubscriptionSubWithCAT", chip::app::TestInteractionModelEngine::TestSubjectHasActiveSubscriptionSubWithCAT), + NL_TEST_DEF("TestDecrementNumSubscriptionToResume", chip::app::TestInteractionModelEngine::TestDecrementNumSubscriptionToResume), NL_TEST_SENTINEL() }; // clang-format on From 7105c8886fad3a6361ddabdaf633d30e6333540c Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Tue, 26 Mar 2024 15:09:48 -0400 Subject: [PATCH 24/29] Rename verifier function --- src/app/icd/server/ICDManager.cpp | 22 +++++++++++----------- src/app/icd/server/ICDManager.h | 20 +++++--------------- src/app/server/Server.cpp | 4 ++-- src/app/tests/TestICDManager.cpp | 28 ++++++++++++++-------------- 4 files changed, 32 insertions(+), 42 deletions(-) diff --git a/src/app/icd/server/ICDManager.cpp b/src/app/icd/server/ICDManager.cpp index cec70780c57737..d77de97ebe053b 100644 --- a/src/app/icd/server/ICDManager.cpp +++ b/src/app/icd/server/ICDManager.cpp @@ -178,7 +178,7 @@ void ICDManager::SendCheckInMsgs() continue; } - if (!CheckInWouldBeSentAtActiveModeVerifier(entry.fabricIndex, entry.monitoredSubject)) + if (!ShouldCheckInMsgsBeSentAtActiveModeFunction(entry.fabricIndex, entry.monitoredSubject)) { continue; } @@ -208,9 +208,9 @@ void ICDManager::SendCheckInMsgs() #endif // CONFIG_BUILD_FOR_HOST_UNIT_TEST } -bool ICDManager::CheckInMessagesWouldBeSent(std::function RegistrationVerifier) +bool ICDManager::CheckInMessagesWouldBeSent(const std::function shouldCheckInMsgsBeSentFunction &) { - VerifyOrReturnValue(RegistrationVerifier, false); + VerifyOrReturnValue(shouldCheckInMsgsBeSentFunction, false); for (const auto & fabricInfo : *mFabricTable) { @@ -240,7 +240,7 @@ bool ICDManager::CheckInMessagesWouldBeSent(std::functionSubjectHasActiveSubscription(aFabricIndex, subjectID) || mSubInfoProvider->SubjectHasPersistedSubscription(aFabricIndex, subjectID)); @@ -291,7 +291,7 @@ bool ICDManager::CheckInWouldBeSentAtActiveModeVerifier(FabricIndex aFabricIndex * If the boot up susbscription has not been completed, * if the fabricIndex and subjectId combination has a persisted subscription. */ -bool ICDManager::CheckInWouldBeSentAtActiveModeVerifier(FabricIndex aFabricIndex, NodeId subjectID) +bool ICDManager::ShouldCheckInMsgsBeSentAtActiveModeFunction(FabricIndex aFabricIndex, NodeId subjectID) { bool wouldSendCheckIn = !(mSubInfoProvider->SubjectHasActiveSubscription(aFabricIndex, subjectID)); @@ -314,13 +314,13 @@ bool ICDManager::CheckInWouldBeSentAtActiveModeVerifier(FabricIndex aFabricIndex * @return true Returns true if the fabricIndex and subjectId combination does not have an active subscription. * @return false Returns false if the fabricIndex and subjectId combination has an active subscription. */ -bool ICDManager::CheckInWouldBeSentAtActiveModeVerifier(FabricIndex aFabricIndex, NodeId subjectID) +bool ICDManager::ShouldCheckInMsgsBeSentAtActiveModeFunction(FabricIndex aFabricIndex, NodeId subjectID) { return !(mSubInfoProvider->SubjectHasActiveSubscription(aFabricIndex, subjectID)); } #endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS -void ICDManager::TriggerCheckInMessages(const std::function & verifier) +void ICDManager::TriggerCheckInMessages(const std::function & verifier) { VerifyOrReturn(SupportsFeature(Feature::kCheckInProtocolSupport)); @@ -390,8 +390,8 @@ void ICDManager::UpdateOperationState(OperationalState state) mOperationalState = OperationalState::IdleMode; #if CHIP_CONFIG_ENABLE_ICD_CIP - std::function verifier = - std::bind(&ICDManager::CheckInWouldBeSentAtActiveModeVerifier, this, std::placeholders::_1, std::placeholders::_2); + std::function verifier = + std::bind(&ICDManager::ShouldCheckInMsgsBeSentAtActiveModeFunction, this, std::placeholders::_1, std::placeholders::_2); #endif // CHIP_CONFIG_ENABLE_ICD_CIP // When the active mode interval is 0, we stay in idleMode until a notification brings the icd into active mode diff --git a/src/app/icd/server/ICDManager.h b/src/app/icd/server/ICDManager.h index b644e2d501eda3..eb33d22a50a778 100644 --- a/src/app/icd/server/ICDManager.h +++ b/src/app/icd/server/ICDManager.h @@ -89,7 +89,7 @@ class ICDManager : public ICDListener * false No Check-In messages would be sent */ - using RegistrationVerificationFunction = bool(FabricIndex aFabricIndex, NodeId subjectID); + using ShouldCheckInMsgsBeSentFunction = bool(FabricIndex aFabricIndex, NodeId subjectID); ICDManager() {} void Init(PersistentStorageDelegate * storage, FabricTable * fabricTable, Crypto::SymmetricKeystore * symmetricKeyStore, @@ -137,7 +137,7 @@ class ICDManager : public ICDListener * * @param[in] algo Verifier function to use to determine if we need to send check-in messages */ - void TriggerCheckInMessages(const std::function & verifier); + void TriggerCheckInMessages(const std::function & verifier); #if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION /** @@ -190,28 +190,18 @@ class ICDManager : public ICDListener private: #if CHIP_CONFIG_ENABLE_ICD_CIP - /** - * @brief Verifier to determine if a Check-In message would be sent on transition to ActiveMode - * - * @param aFabricIndex client fabric index - * @param subjectID client subject ID - * @return true Check-In message would be sent on transition to ActiveMode. - * @return false Device has an active subscription with the subjectID. - * Device is trying to resume inactive subscriptions with the client. See CHIP_CONFIG_PERSIST_SUBSCRIPTIONS and - * CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION - */ - bool CheckInWouldBeSentAtActiveModeVerifier(FabricIndex aFabricIndex, NodeId subjectID); + bool ShouldCheckInMsgsBeSentAtActiveModeFunction(FabricIndex aFabricIndex, NodeId subjectID); /** * @brief Function checks if at least one client registration would require a Check-In message * - * @param[in] RegistrationVerifier function to use to determine if a Check-In message would be sent for a given registration + * @param[in] function function to use to determine if a Check-In message would be sent for a given registration * * @return true At least one registration would require an Check-In message if we were entering ActiveMode. * @return false None of the registration would require a Check-In message either because there are no registration or * because they all have associated subscriptions. */ - bool CheckInMessagesWouldBeSent(std::function RegistrationVerifier); + bool CheckInMessagesWouldBeSent(std::function function); #endif // CHIP_CONFIG_ENABLE_ICD_CIP KeepActiveFlags mKeepActiveFlags{ 0 }; diff --git a/src/app/server/Server.cpp b/src/app/server/Server.cpp index 4a67cf562a94a4..db4c7bccaac92b 100644 --- a/src/app/server/Server.cpp +++ b/src/app/server/Server.cpp @@ -450,7 +450,7 @@ void Server::OnPlatformEvent(const DeviceLayer::ChipDeviceEvent & event) // We trigger Check-In messages before resuming subscriptions to avoid doing both. if (!mFailSafeContext.IsFailSafeArmed()) { - std::function verifier = + std::function verifier = std::bind(&Server::CheckInWouldBeSentAtBootVerifier, this, std::placeholders::_1, std::placeholders::_2); mICDManager.TriggerCheckInMessages(verifier); } @@ -526,7 +526,7 @@ void Server::RejoinExistingMulticastGroups() } #if CHIP_CONFIG_ENABLE_ICD_CIP -bool Server::CheckInWouldBeSentAtBootVerifier(FabricIndex aFabricIndex, NodeId subjectID) +bool Server::ShouldCheckInMsgsBeSentAtBootFunction(FabricIndex aFabricIndex, NodeId subjectID) { #if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS // If at least one registration has a persisted entry, do not send Check-In message. diff --git a/src/app/tests/TestICDManager.cpp b/src/app/tests/TestICDManager.cpp index 10530e36b86617..ac8c220351fd86 100644 --- a/src/app/tests/TestICDManager.cpp +++ b/src/app/tests/TestICDManager.cpp @@ -624,27 +624,27 @@ class TestICDManager #if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS #if CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION - static void TestCheckInWouldBeSentAtActiveModeVerifier(nlTestSuite * aSuite, void * aContext) + static void TestShouldCheckInMsgsBeSentAtActiveModeFunction(nlTestSuite * aSuite, void * aContext) { TestContext * ctx = static_cast(aContext); // Test 1 - Has no ActiveSubscription & no persisted subscription ctx->mSubInfoProvider.SetHasActiveSubscription(false); ctx->mSubInfoProvider.SetHasPersistedSubscription(false); - NL_TEST_ASSERT(aSuite, ctx->mICDManager.CheckInWouldBeSentAtActiveModeVerifier(kTestFabricIndex1, kClientNodeId11)); + NL_TEST_ASSERT(aSuite, ctx->mICDManager.ShouldCheckInMsgsBeSentAtActiveModeFunction(kTestFabricIndex1, kClientNodeId11)); // Test 2 - Has no active subscription & a persisted subscription ctx->mSubInfoProvider.SetHasActiveSubscription(false); ctx->mSubInfoProvider.SetHasPersistedSubscription(true); - NL_TEST_ASSERT(aSuite, !(ctx->mICDManager.CheckInWouldBeSentAtActiveModeVerifier(kTestFabricIndex1, kClientNodeId11))); + NL_TEST_ASSERT(aSuite, !(ctx->mICDManager.ShouldCheckInMsgsBeSentAtActiveModeFunction(kTestFabricIndex1, kClientNodeId11))); // Test 3 - Has an active subscription & a persisted subscription ctx->mSubInfoProvider.SetHasActiveSubscription(true); ctx->mSubInfoProvider.SetHasPersistedSubscription(true); - NL_TEST_ASSERT(aSuite, !(ctx->mICDManager.CheckInWouldBeSentAtActiveModeVerifier(kTestFabricIndex1, kClientNodeId11))); + NL_TEST_ASSERT(aSuite, !(ctx->mICDManager.ShouldCheckInMsgsBeSentAtActiveModeFunction(kTestFabricIndex1, kClientNodeId11))); } #else - static void TestCheckInWouldBeSentAtActiveModeVerifier(nlTestSuite * aSuite, void * aContext) + static void TestShouldCheckInMsgsBeSentAtActiveModeFunction(nlTestSuite * aSuite, void * aContext) { TestContext * ctx = static_cast(aContext); @@ -652,44 +652,44 @@ class TestICDManager ctx->mSubInfoProvider.SetHasActiveSubscription(false); ctx->mSubInfoProvider.SetHasPersistedSubscription(false); ctx->mICDManager.mIsBootUpResumeSubscriptionExecuted = false; - NL_TEST_ASSERT(aSuite, ctx->mICDManager.CheckInWouldBeSentAtActiveModeVerifier(kTestFabricIndex1, kClientNodeId11)); + NL_TEST_ASSERT(aSuite, ctx->mICDManager.ShouldCheckInMsgsBeSentAtActiveModeFunction(kTestFabricIndex1, kClientNodeId11)); // Test 2 - Has no active subscription and a persisted subscription at boot up ctx->mSubInfoProvider.SetHasActiveSubscription(false); ctx->mSubInfoProvider.SetHasPersistedSubscription(true); ctx->mICDManager.mIsBootUpResumeSubscriptionExecuted = false; - NL_TEST_ASSERT(aSuite, !(ctx->mICDManager.CheckInWouldBeSentAtActiveModeVerifier(kTestFabricIndex1, kClientNodeId11))); + NL_TEST_ASSERT(aSuite, !(ctx->mICDManager.ShouldCheckInMsgsBeSentAtActiveModeFunction(kTestFabricIndex1, kClientNodeId11))); // Test 3 - Has an active subscription and a persisted subscription during normal operations ctx->mSubInfoProvider.SetHasActiveSubscription(true); ctx->mSubInfoProvider.SetHasPersistedSubscription(true); ctx->mICDManager.mIsBootUpResumeSubscriptionExecuted = true; - NL_TEST_ASSERT(aSuite, !(ctx->mICDManager.CheckInWouldBeSentAtActiveModeVerifier(kTestFabricIndex1, kClientNodeId11))); + NL_TEST_ASSERT(aSuite, !(ctx->mICDManager.ShouldCheckInMsgsBeSentAtActiveModeFunction(kTestFabricIndex1, kClientNodeId11))); // Test 4 - Has no active subscription and a persisted subscription during normal operations ctx->mSubInfoProvider.SetHasActiveSubscription(false); ctx->mSubInfoProvider.SetHasPersistedSubscription(true); ctx->mICDManager.mIsBootUpResumeSubscriptionExecuted = true; - NL_TEST_ASSERT(aSuite, ctx->mICDManager.CheckInWouldBeSentAtActiveModeVerifier(kTestFabricIndex1, kClientNodeId11)); + NL_TEST_ASSERT(aSuite, ctx->mICDManager.ShouldCheckInMsgsBeSentAtActiveModeFunction(kTestFabricIndex1, kClientNodeId11)); } #endif // CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION #else - static void TestCheckInWouldBeSentAtActiveModeVerifier(nlTestSuite * aSuite, void * aContext) + static void TestShouldCheckInMsgsBeSentAtActiveModeFunction(nlTestSuite * aSuite, void * aContext) { TestContext * ctx = static_cast(aContext); // Test 1 - Has an active subscription ctx->mSubInfoProvider.SetHasActiveSubscription(true); NL_TEST_ASSERT(aSuite, - ctx->mICDManager.CheckInWouldBeSentAtActiveModeVerifier(kTestFabricIndex1, kClientNodeId11) == false); + ctx->mICDManager.ShouldCheckInMsgsBeSentAtActiveModeFunction(kTestFabricIndex1, kClientNodeId11) == false); // Test 2 - Has no active subscription ctx->mSubInfoProvider.SetHasActiveSubscription(false); - NL_TEST_ASSERT(aSuite, ctx->mICDManager.CheckInWouldBeSentAtActiveModeVerifier(kTestFabricIndex1, kClientNodeId11)); + NL_TEST_ASSERT(aSuite, ctx->mICDManager.ShouldCheckInMsgsBeSentAtActiveModeFunction(kTestFabricIndex1, kClientNodeId11)); // Test 3 - Make sure that the persisted subscription has no impact ctx->mSubInfoProvider.SetHasPersistedSubscription(true); - NL_TEST_ASSERT(aSuite, ctx->mICDManager.CheckInWouldBeSentAtActiveModeVerifier(kTestFabricIndex1, kClientNodeId11)); + NL_TEST_ASSERT(aSuite, ctx->mICDManager.ShouldCheckInMsgsBeSentAtActiveModeFunction(kTestFabricIndex1, kClientNodeId11)); } #endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS }; @@ -709,7 +709,7 @@ static const nlTest sTests[] = { NL_TEST_DEF("TestICDMRegisterUnregisterEvents", TestICDManager::TestICDMRegisterUnregisterEvents), NL_TEST_DEF("TestICDCounter", TestICDManager::TestICDCounter), NL_TEST_DEF("TestICDStayActive", TestICDManager::TestICDMStayActive), - NL_TEST_DEF("TestCheckInWouldBeSentAtActiveModeVerifier", TestICDManager::TestCheckInWouldBeSentAtActiveModeVerifier), + NL_TEST_DEF("TestShouldCheckInMsgsBeSentAtActiveModeFunction", TestICDManager::TestShouldCheckInMsgsBeSentAtActiveModeFunction), NL_TEST_SENTINEL(), }; From 50b128b4f6e36cd174883bdf14205b0267dc7184 Mon Sep 17 00:00:00 2001 From: mkardous-silabs <84793247+mkardous-silabs@users.noreply.github.com> Date: Tue, 26 Mar 2024 15:24:13 -0400 Subject: [PATCH 25/29] Clean up comments & code --- src/app/InteractionModelEngine.cpp | 4 +-- src/app/InteractionModelEngine.h | 2 +- ...bscriptionResumptionSessionEstablisher.cpp | 4 +-- src/app/icd/server/BUILD.gn | 2 +- src/app/icd/server/ICDManager.cpp | 33 ++++++++++--------- src/app/icd/server/ICDManager.h | 10 +++--- src/app/server/Server.cpp | 6 ++-- src/app/server/Server.h | 4 +-- src/app/tests/TestInteractionModelEngine.cpp | 24 +++++++------- 9 files changed, 46 insertions(+), 43 deletions(-) diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index c19357b1615514..a308750f967ecd 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -2064,12 +2064,12 @@ bool InteractionModelEngine::HasSubscriptionsToResume() #endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION #if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS -void InteractionModelEngine::DecrementNumSubscriptionToResume() +void InteractionModelEngine::DecrementNumSubscriptionsToResume() { VerifyOrReturn(mNumOfSubscriptionToResume > 0); #if CHIP_CONFIG_ENABLE_ICD_CIP && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION VerifyOrDie(mICDManager); -#endif // CHIP_CONFIG_ENABLE_ICD_CIP && CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION +#endif // CHIP_CONFIG_ENABLE_ICD_CIP && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION mNumOfSubscriptionToResume--; diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h index 86a266fbad73a2..51daf2a0ee818f 100644 --- a/src/app/InteractionModelEngine.h +++ b/src/app/InteractionModelEngine.h @@ -330,7 +330,7 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, bool SubjectHasPersistedSubscription(FabricIndex aFabricIndex, NodeId subjectID) override; #if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS - void DecrementNumSubscriptionToResume(); + void DecrementNumSubscriptionsToResume(); #endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS #if CONFIG_BUILD_FOR_HOST_UNIT_TEST diff --git a/src/app/SubscriptionResumptionSessionEstablisher.cpp b/src/app/SubscriptionResumptionSessionEstablisher.cpp index 393ca3d4c6aae5..8eaef424230725 100644 --- a/src/app/SubscriptionResumptionSessionEstablisher.cpp +++ b/src/app/SubscriptionResumptionSessionEstablisher.cpp @@ -92,7 +92,7 @@ void SubscriptionResumptionSessionEstablisher::HandleDeviceConnected(void * cont InteractionModelEngine * imEngine = InteractionModelEngine::GetInstance(); // Decrement the number of subscriptions to resume - imEngine->DecrementNumSubscriptionToResume(); + imEngine->DecrementNumSubscriptionsToResume(); if (!imEngine->EnsureResourceForSubscription(subscriptionInfo.mFabricIndex, subscriptionInfo.mAttributePaths.AllocatedSize(), subscriptionInfo.mEventPaths.AllocatedSize())) @@ -130,7 +130,7 @@ void SubscriptionResumptionSessionEstablisher::HandleDeviceConnectionFailure(voi error.Format()); // Decrement the number of subscriptions to resume - imEngine->DecrementNumSubscriptionToResume(); + imEngine->DecrementNumSubscriptionsToResume(); auto * subscriptionResumptionStorage = imEngine->GetSubscriptionResumptionStorage(); if (!subscriptionResumptionStorage) diff --git a/src/app/icd/server/BUILD.gn b/src/app/icd/server/BUILD.gn index 45cc4a845c8965..a6c5096dde7d0c 100644 --- a/src/app/icd/server/BUILD.gn +++ b/src/app/icd/server/BUILD.gn @@ -24,7 +24,7 @@ buildconfig_header("icd-server-buildconfig") { if (chip_enable_icd_lit || chip_enable_icd_checkin || chip_enable_icd_user_active_mode_trigger) { - assert(chip_enable_icd_lit) + assert(chip_enable_icd_server) } if (chip_enable_icd_lit) { diff --git a/src/app/icd/server/ICDManager.cpp b/src/app/icd/server/ICDManager.cpp index d77de97ebe053b..4b026bd76a7930 100644 --- a/src/app/icd/server/ICDManager.cpp +++ b/src/app/icd/server/ICDManager.cpp @@ -108,7 +108,7 @@ void ICDManager::Shutdown() #if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION mIsBootUpResumeSubscriptionExecuted = false; -#endif // !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION +#endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION #endif // CHIP_CONFIG_ENABLE_ICD_CIP } @@ -208,7 +208,7 @@ void ICDManager::SendCheckInMsgs() #endif // CONFIG_BUILD_FOR_HOST_UNIT_TEST } -bool ICDManager::CheckInMessagesWouldBeSent(const std::function shouldCheckInMsgsBeSentFunction &) +bool ICDManager::CheckInMessagesWouldBeSent(const std::function & shouldCheckInMsgsBeSentFunction) { VerifyOrReturnValue(shouldCheckInMsgsBeSentFunction, false); @@ -250,18 +250,18 @@ bool ICDManager::CheckInMessagesWouldBeSent(const std::function verifier = + std::function function = std::bind(&ICDManager::ShouldCheckInMsgsBeSentAtActiveModeFunction, this, std::placeholders::_1, std::placeholders::_2); #endif // CHIP_CONFIG_ENABLE_ICD_CIP @@ -398,7 +399,7 @@ void ICDManager::UpdateOperationState(OperationalState state) // unless the device would need to send Check-In messages if (ICDConfigurationData::GetInstance().GetActiveModeDuration() > kZero #if CHIP_CONFIG_ENABLE_ICD_CIP - || CheckInMessagesWouldBeSent(verifier) + || CheckInMessagesWouldBeSent(function) #endif // CHIP_CONFIG_ENABLE_ICD_CIP ) { diff --git a/src/app/icd/server/ICDManager.h b/src/app/icd/server/ICDManager.h index eb33d22a50a778..058285802eb309 100644 --- a/src/app/icd/server/ICDManager.h +++ b/src/app/icd/server/ICDManager.h @@ -135,9 +135,9 @@ class ICDManager : public ICDListener /** * @brief Trigger the ICDManager to send Check-In message if necessary * - * @param[in] algo Verifier function to use to determine if we need to send check-in messages + * @param[in] function to use to determine if we need to send check-in messages */ - void TriggerCheckInMessages(const std::function & verifier); + void TriggerCheckInMessages(const std::function & function); #if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION /** @@ -151,7 +151,9 @@ class ICDManager : public ICDListener #if CONFIG_BUILD_FOR_HOST_UNIT_TEST void SetTestFeatureMapValue(uint32_t featureMap) { mFeatureMap = featureMap; }; +#if !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION && CHIP_CONFIG_PERSIST_SUBSCRIPTIONS bool GetIsBootUpResumeSubscriptionExecuted() { return mIsBootUpResumeSubscriptionExecuted; }; +#endif // !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION && CHIP_CONFIG_PERSIST_SUBSCRIPTIONS #endif // Implementation of ICDListener functions. @@ -201,7 +203,7 @@ class ICDManager : public ICDListener * @return false None of the registration would require a Check-In message either because there are no registration or * because they all have associated subscriptions. */ - bool CheckInMessagesWouldBeSent(std::function function); + bool CheckInMessagesWouldBeSent(const std::function & function); #endif // CHIP_CONFIG_ENABLE_ICD_CIP KeepActiveFlags mKeepActiveFlags{ 0 }; @@ -214,7 +216,7 @@ class ICDManager : public ICDListener #if CHIP_CONFIG_ENABLE_ICD_CIP #if !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION && CHIP_CONFIG_PERSIST_SUBSCRIPTIONS bool mIsBootUpResumeSubscriptionExecuted = false; -#endif // !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION +#endif // !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION && CHIP_CONFIG_PERSIST_SUBSCRIPTIONS PersistentStorageDelegate * mStorage = nullptr; FabricTable * mFabricTable = nullptr; Messaging::ExchangeManager * mExchangeManager = nullptr; diff --git a/src/app/server/Server.cpp b/src/app/server/Server.cpp index db4c7bccaac92b..d45ab7eefacfb6 100644 --- a/src/app/server/Server.cpp +++ b/src/app/server/Server.cpp @@ -450,9 +450,9 @@ void Server::OnPlatformEvent(const DeviceLayer::ChipDeviceEvent & event) // We trigger Check-In messages before resuming subscriptions to avoid doing both. if (!mFailSafeContext.IsFailSafeArmed()) { - std::function verifier = - std::bind(&Server::CheckInWouldBeSentAtBootVerifier, this, std::placeholders::_1, std::placeholders::_2); - mICDManager.TriggerCheckInMessages(verifier); + std::function function = + std::bind(&Server::ShouldCheckInMsgsBeSentAtBootFunction, this, std::placeholders::_1, std::placeholders::_2); + mICDManager.TriggerCheckInMessages(function); } #endif // CHIP_CONFIG_ENABLE_ICD_SERVER && CHIP_CONFIG_ENABLE_ICD_CIP #if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS diff --git a/src/app/server/Server.h b/src/app/server/Server.h index 83b0a65b18daf1..8f6fcd5abecc66 100644 --- a/src/app/server/Server.h +++ b/src/app/server/Server.h @@ -372,14 +372,14 @@ class Server #if CHIP_CONFIG_ENABLE_ICD_CIP /** - * @brief Verifier to determine if a Check-In message would be sent at Boot up + * @brief Function to determine if a Check-In message would be sent at Boot up * * @param aFabricIndex client fabric index * @param subjectID client subject ID * @return true Check-In message would be sent on boot up. * @return false Device has a persisted subscription with the client. See CHIP_CONFIG_PERSIST_SUBSCRIPTIONS. */ - bool CheckInWouldBeSentAtBootVerifier(FabricIndex aFabricIndex, NodeId subjectID); + bool ShouldCheckInMsgsBeSentAtBootFunction(FabricIndex aFabricIndex, NodeId subjectID); #endif // CHIP_CONFIG_ENABLE_ICD_CIP #endif // CHIP_CONFIG_ENABLE_ICD_SERVER diff --git a/src/app/tests/TestInteractionModelEngine.cpp b/src/app/tests/TestInteractionModelEngine.cpp index 370117032c424e..09f713f170c96e 100644 --- a/src/app/tests/TestInteractionModelEngine.cpp +++ b/src/app/tests/TestInteractionModelEngine.cpp @@ -84,7 +84,7 @@ class TestInteractionModelEngine static void TestSubjectHasActiveSubscriptionMultipleSubsSingleEntry(nlTestSuite * apSuite, void * apContext); static void TestSubjectHasActiveSubscriptionMultipleSubsMultipleEntries(nlTestSuite * apSuite, void * apContext); static void TestSubjectHasActiveSubscriptionSubWithCAT(nlTestSuite * apSuite, void * apContext); - static void TestDecrementNumSubscriptionToResume(nlTestSuite * apSuite, void * apContext); + static void TestDecrementNumSubscriptionsToResume(nlTestSuite * apSuite, void * apContext); }; int TestInteractionModelEngine::GetAttributePathListLength(SingleLinkedListNode * apAttributePathParamsList) @@ -683,9 +683,8 @@ void TestInteractionModelEngine::TestSubscriptionResumptionTimer(nlTestSuite * a } #endif // CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION -#endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS -void TestInteractionModelEngine::TestDecrementNumSubscriptionToResume(nlTestSuite * apSuite, void * apContext) +void TestInteractionModelEngine::TestDecrementNumSubscriptionsToResume(nlTestSuite * apSuite, void * apContext) { TestContext & ctx = *static_cast(apContext); CHIP_ERROR err = CHIP_NO_ERROR; @@ -696,7 +695,7 @@ void TestInteractionModelEngine::TestDecrementNumSubscriptionToResume(nlTestSuit err = engine->Init(&ctx.GetExchangeManager(), &ctx.GetFabricTable(), app::reporting::GetDefaultReportScheduler()); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); -#if CHIP_CONFIG_ENABLE_ICD_CIP && CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION +#if CHIP_CONFIG_ENABLE_ICD_CIP && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION ICDManager manager; engine->SetICDManager(&manager); #endif // CHIP_CONFIG_ENABLE_ICD_CIP && CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION @@ -704,43 +703,44 @@ void TestInteractionModelEngine::TestDecrementNumSubscriptionToResume(nlTestSuit // Set number of subs engine->mNumOfSubscriptionToResume = kNumberOfSubsToResume; -#if CHIP_CONFIG_ENABLE_ICD_CIP && CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION +#if CHIP_CONFIG_ENABLE_ICD_CIP && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION // Verify mIsBootUpResumeSubscriptionExecuted has not been set NL_TEST_ASSERT(apSuite, !manager.GetIsBootUpResumeSubscriptionExecuted()); #endif // CHIP_CONFIG_ENABLE_ICD_CIP && CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION // Decrease number of subs by 1 numberOfSubsRemaining--; - engine->DecrementNumSubscriptionToResume(); + engine->DecrementNumSubscriptionsToResume(); NL_TEST_ASSERT_EQUALS(apSuite, numberOfSubsRemaining, engine->mNumOfSubscriptionToResume); // Decrease to 0 subs remaining while (numberOfSubsRemaining > 0) { -#if CHIP_CONFIG_ENABLE_ICD_CIP && CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION +#if CHIP_CONFIG_ENABLE_ICD_CIP && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION // Verify mIsBootUpResumeSubscriptionExecuted has not been set NL_TEST_ASSERT(apSuite, !manager.GetIsBootUpResumeSubscriptionExecuted()); #endif // CHIP_CONFIG_ENABLE_ICD_CIP && CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION numberOfSubsRemaining--; - engine->DecrementNumSubscriptionToResume(); + engine->DecrementNumSubscriptionsToResume(); NL_TEST_ASSERT_EQUALS(apSuite, numberOfSubsRemaining, engine->mNumOfSubscriptionToResume); } -#if CHIP_CONFIG_ENABLE_ICD_CIP && CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION +#if CHIP_CONFIG_ENABLE_ICD_CIP && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION // Verify mIsBootUpResumeSubscriptionExecuted has been set NL_TEST_ASSERT(apSuite, manager.GetIsBootUpResumeSubscriptionExecuted()); #endif // CHIP_CONFIG_ENABLE_ICD_CIP && CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION // Make sure we don't rollover / go negative - engine->DecrementNumSubscriptionToResume(); + engine->DecrementNumSubscriptionsToResume(); NL_TEST_ASSERT_EQUALS(apSuite, numberOfSubsRemaining, engine->mNumOfSubscriptionToResume); // Clean up -#if CHIP_CONFIG_ENABLE_ICD_CIP && CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION +#if CHIP_CONFIG_ENABLE_ICD_CIP && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION engine->SetICDManager(nullptr); #endif // CHIP_CONFIG_ENABLE_ICD_CIP && CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION } +#endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS } // namespace app } // namespace chip @@ -754,6 +754,7 @@ const nlTest sTests[] = NL_TEST_DEF("TestRemoveDuplicateConcreteAttribute", chip::app::TestInteractionModelEngine::TestRemoveDuplicateConcreteAttribute), #if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS NL_TEST_DEF("TestSubjectHasPersistedSubscription", chip::app::TestInteractionModelEngine::TestSubjectHasPersistedSubscription), + NL_TEST_DEF("TestDecrementNumSubscriptionsToResume", chip::app::TestInteractionModelEngine::TestDecrementNumSubscriptionsToResume), #if CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION NL_TEST_DEF("TestSubscriptionResumptionTimer", chip::app::TestInteractionModelEngine::TestSubscriptionResumptionTimer), #endif // CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION @@ -763,7 +764,6 @@ const nlTest sTests[] = NL_TEST_DEF("TestSubjectHasActiveSubscriptionMultipleSubsSingleEntry", chip::app::TestInteractionModelEngine::TestSubjectHasActiveSubscriptionMultipleSubsSingleEntry), NL_TEST_DEF("TestSubjectHasActiveSubscriptionMultipleSubsMultipleEntries", chip::app::TestInteractionModelEngine::TestSubjectHasActiveSubscriptionMultipleSubsMultipleEntries), NL_TEST_DEF("TestSubjectHasActiveSubscriptionSubWithCAT", chip::app::TestInteractionModelEngine::TestSubjectHasActiveSubscriptionSubWithCAT), - NL_TEST_DEF("TestDecrementNumSubscriptionToResume", chip::app::TestInteractionModelEngine::TestDecrementNumSubscriptionToResume), NL_TEST_SENTINEL() }; // clang-format on From d30312bcc66c80340cc34cff9b33f3f148d96732 Mon Sep 17 00:00:00 2001 From: mkardous-silabs <84793247+mkardous-silabs@users.noreply.github.com> Date: Tue, 2 Apr 2024 10:56:06 -0400 Subject: [PATCH 26/29] Apply suggestions from code review Co-authored-by: Boris Zbarsky --- src/app/InteractionModelEngine.cpp | 2 +- src/app/InteractionModelEngine.h | 2 +- src/app/icd/server/ICDManager.cpp | 28 +++++++++++++--------------- src/app/server/Server.cpp | 13 ++++++------- 4 files changed, 21 insertions(+), 24 deletions(-) diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index a308750f967ecd..186f971df74769 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -365,7 +365,7 @@ bool InteractionModelEngine::SubjectHasPersistedSubscription(FabricIndex aFabric #if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS auto * iterator = mpSubscriptionResumptionStorage->IterateSubscriptions(); - // Verify that we were able t0 allocate an iterator. If not, assume we have a persisted subscription. + // Verify that we were able to allocate an iterator. If not, assume we have a persisted subscription. VerifyOrReturnValue(iterator, true); SubscriptionResumptionStorage::SubscriptionInfo subscriptionInfo; diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h index 51daf2a0ee818f..faeddcf3c84640 100644 --- a/src/app/InteractionModelEngine.h +++ b/src/app/InteractionModelEngine.h @@ -681,7 +681,7 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, #endif // CONFIG_BUILD_FOR_HOST_UNIT_TEST #if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS - int8_t mNumOfSubscriptionToResume = 0; + int8_t mNumOfSubscriptionsToResume = 0; #if CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION bool HasSubscriptionsToResume(); uint32_t ComputeTimeSecondsTillNextSubscriptionResumption(); diff --git a/src/app/icd/server/ICDManager.cpp b/src/app/icd/server/ICDManager.cpp index 4b026bd76a7930..61d7e90979067c 100644 --- a/src/app/icd/server/ICDManager.cpp +++ b/src/app/icd/server/ICDManager.cpp @@ -257,8 +257,8 @@ bool ICDManager::CheckInMessagesWouldBeSent(const std::functionSubjectHasActiveSubscription(aFabricIndex, subjectID)); - - if (!mIsBootUpResumeSubscriptionExecuted) - { - wouldSendCheckIn = wouldSendCheckIn && !mSubInfoProvider->SubjectHasPersistedSubscription(aFabricIndex, subjectID); + bool mightHaveSubscription = mSubInfoProvider->SubjectHasActiveSubscription(aFabricIndex, subjectID); + if (!mightHaveSubscription && !mIsBootUpResumeSubscriptionExecuted) { + mightHaveSubscription = mSubInfoProvider->SubjectHasPersistedSubscription(aFabricIndex, subjectID); } - - return wouldSendCheckIn; + + return !mightHaveSubscription; } #endif // CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION #else /** - * @brief Implementation for when neither the persistant subscription and subscription timeout resumption features are present. + * @brief Implementation for when neither the persistent subscription nor the subscription timeout resumption features are present. * Function checks that there no active sbuscriptions for a given fabricIndex and subjectId combination. * * @param aFabricIndex @@ -391,7 +389,7 @@ void ICDManager::UpdateOperationState(OperationalState state) mOperationalState = OperationalState::IdleMode; #if CHIP_CONFIG_ENABLE_ICD_CIP - std::function function = + std::function sendCheckInMessagesOnActiveMode = std::bind(&ICDManager::ShouldCheckInMsgsBeSentAtActiveModeFunction, this, std::placeholders::_1, std::placeholders::_2); #endif // CHIP_CONFIG_ENABLE_ICD_CIP diff --git a/src/app/server/Server.cpp b/src/app/server/Server.cpp index d45ab7eefacfb6..2058a2ad56cb97 100644 --- a/src/app/server/Server.cpp +++ b/src/app/server/Server.cpp @@ -334,7 +334,7 @@ CHIP_ERROR Server::Init(const ServerInitParams & initParams) SuccessOrExit(err); #if CHIP_CONFIG_ENABLE_ICD_SERVER - chip::app::InteractionModelEngine::GetInstance()->SetICDManager(&mICDManager); + app::InteractionModelEngine::GetInstance()->SetICDManager(&mICDManager); #endif // CHIP_CONFIG_ENABLE_ICD_SERVER // ICD Init needs to be after data model init and InteractionModel Init @@ -450,7 +450,7 @@ void Server::OnPlatformEvent(const DeviceLayer::ChipDeviceEvent & event) // We trigger Check-In messages before resuming subscriptions to avoid doing both. if (!mFailSafeContext.IsFailSafeArmed()) { - std::function function = + std::function function = std::bind(&Server::ShouldCheckInMsgsBeSentAtBootFunction, this, std::placeholders::_1, std::placeholders::_2); mICDManager.TriggerCheckInMessages(function); } @@ -531,11 +531,10 @@ bool Server::ShouldCheckInMsgsBeSentAtBootFunction(FabricIndex aFabricIndex, Nod #if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS // If at least one registration has a persisted entry, do not send Check-In message. // The resumption of the persisted subscription will serve the same function a check-in would have served. - VerifyOrReturnValue(!app::InteractionModelEngine::GetInstance()->SubjectHasPersistedSubscription(aFabricIndex, subjectID), - false); -#endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS - + return !app::InteractionModelEngine::GetInstance()->SubjectHasPersistedSubscription(aFabricIndex, subjectID); +#else return true; +#endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS } #endif // CHIP_CONFIG_ENABLE_ICD_CIP @@ -582,7 +581,7 @@ void Server::Shutdown() chip::Dnssd::Resolver::Instance().Shutdown(); chip::app::InteractionModelEngine::GetInstance()->Shutdown(); #if CHIP_CONFIG_ENABLE_ICD_SERVER - chip::app::InteractionModelEngine::GetInstance()->SetICDManager(nullptr); + app::InteractionModelEngine::GetInstance()->SetICDManager(nullptr); #endif // CHIP_CONFIG_ENABLE_ICD_SERVER mCommissioningWindowManager.Shutdown(); mMessageCounterManager.Shutdown(); From c45315ba02c100010fc7497271200ff06cf470d1 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Tue, 2 Apr 2024 11:13:55 -0400 Subject: [PATCH 27/29] Fix comment application --- src/app/InteractionModelEngine.cpp | 18 +++++++++--------- src/app/icd/server/ICDManager.cpp | 7 ++++--- src/app/server/Server.cpp | 4 ++-- src/app/tests/TestInteractionModelEngine.cpp | 8 ++++---- 4 files changed, 19 insertions(+), 18 deletions(-) diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 186f971df74769..4a6e2391b5d224 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -1926,22 +1926,22 @@ CHIP_ERROR InteractionModelEngine::ResumeSubscriptions() // future improvements: https://github.com/project-chip/connectedhomeip/issues/25439 SubscriptionResumptionStorage::SubscriptionInfo subscriptionInfo; - auto * iterator = mpSubscriptionResumptionStorage->IterateSubscriptions(); - mNumOfSubscriptionToResume = 0; - uint16_t minInterval = 0; + auto * iterator = mpSubscriptionResumptionStorage->IterateSubscriptions(); + mNumOfSubscriptionsToResume = 0; + uint16_t minInterval = 0; while (iterator->Next(subscriptionInfo)) { - mNumOfSubscriptionToResume++; + mNumOfSubscriptionsToResume++; minInterval = std::max(minInterval, subscriptionInfo.mMinInterval); } iterator->Release(); - if (mNumOfSubscriptionToResume) + if (mNumOfSubscriptionsToResume) { #if CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION mSubscriptionResumptionScheduled = true; #endif - ChipLogProgress(InteractionModel, "Resuming %d subscriptions in %u seconds", mNumOfSubscriptionToResume, minInterval); + ChipLogProgress(InteractionModel, "Resuming %d subscriptions in %u seconds", mNumOfSubscriptionsToResume, minInterval); ReturnErrorOnFailure(mpExchangeMgr->GetSessionManager()->SystemLayer()->StartTimer(System::Clock::Seconds16(minInterval), ResumeSubscriptionsTimerCallback, this)); } @@ -2066,15 +2066,15 @@ bool InteractionModelEngine::HasSubscriptionsToResume() #if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS void InteractionModelEngine::DecrementNumSubscriptionsToResume() { - VerifyOrReturn(mNumOfSubscriptionToResume > 0); + VerifyOrReturn(mNumOfSubscriptionsToResume > 0); #if CHIP_CONFIG_ENABLE_ICD_CIP && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION VerifyOrDie(mICDManager); #endif // CHIP_CONFIG_ENABLE_ICD_CIP && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION - mNumOfSubscriptionToResume--; + mNumOfSubscriptionsToResume--; #if CHIP_CONFIG_ENABLE_ICD_CIP && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION - if (!mNumOfSubscriptionToResume) + if (!mNumOfSubscriptionsToResume) { mICDManager->SetBootUpResumeSubscriptionExecuted(); } diff --git a/src/app/icd/server/ICDManager.cpp b/src/app/icd/server/ICDManager.cpp index 61d7e90979067c..c198863cb65ad6 100644 --- a/src/app/icd/server/ICDManager.cpp +++ b/src/app/icd/server/ICDManager.cpp @@ -294,10 +294,11 @@ bool ICDManager::ShouldCheckInMsgsBeSentAtActiveModeFunction(FabricIndex aFabric bool ICDManager::ShouldCheckInMsgsBeSentAtActiveModeFunction(FabricIndex aFabricIndex, NodeId subjectID) { bool mightHaveSubscription = mSubInfoProvider->SubjectHasActiveSubscription(aFabricIndex, subjectID); - if (!mightHaveSubscription && !mIsBootUpResumeSubscriptionExecuted) { + if (!mightHaveSubscription && !mIsBootUpResumeSubscriptionExecuted) + { mightHaveSubscription = mSubInfoProvider->SubjectHasPersistedSubscription(aFabricIndex, subjectID); } - + return !mightHaveSubscription; } #endif // CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION @@ -397,7 +398,7 @@ void ICDManager::UpdateOperationState(OperationalState state) // unless the device would need to send Check-In messages if (ICDConfigurationData::GetInstance().GetActiveModeDuration() > kZero #if CHIP_CONFIG_ENABLE_ICD_CIP - || CheckInMessagesWouldBeSent(function) + || CheckInMessagesWouldBeSent(sendCheckInMessagesOnActiveMode) #endif // CHIP_CONFIG_ENABLE_ICD_CIP ) { diff --git a/src/app/server/Server.cpp b/src/app/server/Server.cpp index 2058a2ad56cb97..eff11222bfae7b 100644 --- a/src/app/server/Server.cpp +++ b/src/app/server/Server.cpp @@ -450,9 +450,9 @@ void Server::OnPlatformEvent(const DeviceLayer::ChipDeviceEvent & event) // We trigger Check-In messages before resuming subscriptions to avoid doing both. if (!mFailSafeContext.IsFailSafeArmed()) { - std::function function = + std::function sendCheckInMessagesOnBootUp = std::bind(&Server::ShouldCheckInMsgsBeSentAtBootFunction, this, std::placeholders::_1, std::placeholders::_2); - mICDManager.TriggerCheckInMessages(function); + mICDManager.TriggerCheckInMessages(sendCheckInMessagesOnBootUp); } #endif // CHIP_CONFIG_ENABLE_ICD_SERVER && CHIP_CONFIG_ENABLE_ICD_CIP #if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS diff --git a/src/app/tests/TestInteractionModelEngine.cpp b/src/app/tests/TestInteractionModelEngine.cpp index 09f713f170c96e..4f0b75315355ad 100644 --- a/src/app/tests/TestInteractionModelEngine.cpp +++ b/src/app/tests/TestInteractionModelEngine.cpp @@ -701,7 +701,7 @@ void TestInteractionModelEngine::TestDecrementNumSubscriptionsToResume(nlTestSui #endif // CHIP_CONFIG_ENABLE_ICD_CIP && CHIP_CONFIG_PERSIST_SUBSCRIPTIONS && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION // Set number of subs - engine->mNumOfSubscriptionToResume = kNumberOfSubsToResume; + engine->mNumOfSubscriptionsToResume = kNumberOfSubsToResume; #if CHIP_CONFIG_ENABLE_ICD_CIP && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION // Verify mIsBootUpResumeSubscriptionExecuted has not been set @@ -711,7 +711,7 @@ void TestInteractionModelEngine::TestDecrementNumSubscriptionsToResume(nlTestSui // Decrease number of subs by 1 numberOfSubsRemaining--; engine->DecrementNumSubscriptionsToResume(); - NL_TEST_ASSERT_EQUALS(apSuite, numberOfSubsRemaining, engine->mNumOfSubscriptionToResume); + NL_TEST_ASSERT_EQUALS(apSuite, numberOfSubsRemaining, engine->mNumOfSubscriptionsToResume); // Decrease to 0 subs remaining while (numberOfSubsRemaining > 0) @@ -723,7 +723,7 @@ void TestInteractionModelEngine::TestDecrementNumSubscriptionsToResume(nlTestSui numberOfSubsRemaining--; engine->DecrementNumSubscriptionsToResume(); - NL_TEST_ASSERT_EQUALS(apSuite, numberOfSubsRemaining, engine->mNumOfSubscriptionToResume); + NL_TEST_ASSERT_EQUALS(apSuite, numberOfSubsRemaining, engine->mNumOfSubscriptionsToResume); } #if CHIP_CONFIG_ENABLE_ICD_CIP && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION @@ -733,7 +733,7 @@ void TestInteractionModelEngine::TestDecrementNumSubscriptionsToResume(nlTestSui // Make sure we don't rollover / go negative engine->DecrementNumSubscriptionsToResume(); - NL_TEST_ASSERT_EQUALS(apSuite, numberOfSubsRemaining, engine->mNumOfSubscriptionToResume); + NL_TEST_ASSERT_EQUALS(apSuite, numberOfSubsRemaining, engine->mNumOfSubscriptionsToResume); // Clean up #if CHIP_CONFIG_ENABLE_ICD_CIP && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION From 59a9862b1b2574777eac9d090111d231d22c65a9 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Tue, 2 Apr 2024 12:24:37 -0400 Subject: [PATCH 28/29] Add / update comments --- src/app/InteractionModelEngine.h | 11 +++++++++++ src/app/SubscriptionResumptionSessionEstablisher.cpp | 8 ++++++-- src/app/icd/server/ICDManager.cpp | 9 +++++++++ 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h index faeddcf3c84640..935bca002b0dd1 100644 --- a/src/app/InteractionModelEngine.h +++ b/src/app/InteractionModelEngine.h @@ -330,6 +330,11 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, bool SubjectHasPersistedSubscription(FabricIndex aFabricIndex, NodeId subjectID) override; #if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS + /** + * @brief Function decrements the number of subscriptions to resume counter - mNumOfSubscriptionsToResume. + * This should be called after we have completed a re-subscribe attempt on a persisted subscription wether the attempt + * was succesful or not. + */ void DecrementNumSubscriptionsToResume(); #endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS @@ -681,6 +686,12 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, #endif // CONFIG_BUILD_FOR_HOST_UNIT_TEST #if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS + /** + * mNumOfSubscriptionsToResume tracks the number of subscriptions that the device will try to resume at its next resumption + * attempt. At boot up, the attempt will be at the highest min interval of all the subscriptions to resume. + * When the subscription timeout resumption feature is present, after the boot up attempt, the next attempt will be determined + * by ComputeTimeSecondsTillNextSubscriptionResumption. + */ int8_t mNumOfSubscriptionsToResume = 0; #if CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION bool HasSubscriptionsToResume(); diff --git a/src/app/SubscriptionResumptionSessionEstablisher.cpp b/src/app/SubscriptionResumptionSessionEstablisher.cpp index 8eaef424230725..6e016684410fc8 100644 --- a/src/app/SubscriptionResumptionSessionEstablisher.cpp +++ b/src/app/SubscriptionResumptionSessionEstablisher.cpp @@ -91,7 +91,9 @@ void SubscriptionResumptionSessionEstablisher::HandleDeviceConnected(void * cont SubscriptionResumptionStorage::SubscriptionInfo & subscriptionInfo = establisher->mSubscriptionInfo; InteractionModelEngine * imEngine = InteractionModelEngine::GetInstance(); - // Decrement the number of subscriptions to resume + // Decrement the number of subscriptions to resume since we have completed our retry attempt for a given subscription. + // We do this before the readHandler creation since we do not care if the subscription has successfully been resumed or + // not. Counter only tracks the number of individual subscriptions we will try to resume. imEngine->DecrementNumSubscriptionsToResume(); if (!imEngine->EnsureResourceForSubscription(subscriptionInfo.mFabricIndex, subscriptionInfo.mAttributePaths.AllocatedSize(), @@ -129,7 +131,9 @@ void SubscriptionResumptionSessionEstablisher::HandleDeviceConnectionFailure(voi ChipLogError(DataManagement, "Failed to establish CASE for subscription-resumption with error '%" CHIP_ERROR_FORMAT "'", error.Format()); - // Decrement the number of subscriptions to resume + // Decrement the number of subscriptions to resume since we have completed our retry attempt for a given subscription. + // We do this here since we were not able to connect to the subscriber thus we have completed our resumption attempt. + // Counter only tracks the number of individual subscriptions we will try to resume. imEngine->DecrementNumSubscriptionsToResume(); auto * subscriptionResumptionStorage = imEngine->GetSubscriptionResumptionStorage(); diff --git a/src/app/icd/server/ICDManager.cpp b/src/app/icd/server/ICDManager.cpp index c198863cb65ad6..f4442e85972ee2 100644 --- a/src/app/icd/server/ICDManager.cpp +++ b/src/app/icd/server/ICDManager.cpp @@ -260,6 +260,12 @@ bool ICDManager::CheckInMessagesWouldBeSent(const std::function Date: Tue, 2 Apr 2024 17:24:41 -0400 Subject: [PATCH 29/29] update comment with more details --- src/app/InteractionModelEngine.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 4a6e2391b5d224..d0b020caf96315 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -365,7 +365,10 @@ bool InteractionModelEngine::SubjectHasPersistedSubscription(FabricIndex aFabric #if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS auto * iterator = mpSubscriptionResumptionStorage->IterateSubscriptions(); - // Verify that we were able to allocate an iterator. If not, assume we have a persisted subscription. + // Verify that we were able to allocate an iterator. If not, we are probably currently trying to resubscribe to our persisted + // subscriptions. As such, we assume we have a persisted subscription and return true. + // If we don't have a persisted subscription for the given fabric index and subjectID, we will send a Check-In message next time + // we transition to ActiveMode. VerifyOrReturnValue(iterator, true); SubscriptionResumptionStorage::SubscriptionInfo subscriptionInfo;