From 112030006288f0f437df9391465e7322f9c2db90 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Fri, 3 Nov 2023 13:36:10 -0400 Subject: [PATCH] add observer logic between ICDManager and ICDManagementServer --- .../icd-management-server.cpp | 1 + src/app/icd/BUILD.gn | 1 + src/app/icd/ICDManagementServer.cpp | 23 +++ src/app/icd/ICDManagementServer.h | 4 + src/app/icd/ICDManager.cpp | 38 ++++- src/app/icd/ICDManager.h | 10 ++ src/app/icd/ICDNotifier.cpp | 11 ++ src/app/icd/ICDNotifier.h | 15 ++ src/app/tests/TestICDManager.cpp | 132 +++++++++++++++++- src/app/tests/TestICDMonitoringTable.cpp | 2 - 10 files changed, 225 insertions(+), 12 deletions(-) diff --git a/src/app/clusters/icd-management-server/icd-management-server.cpp b/src/app/clusters/icd-management-server/icd-management-server.cpp index 059d0e915b44b4..25df0270750ccd 100644 --- a/src/app/clusters/icd-management-server/icd-management-server.cpp +++ b/src/app/clusters/icd-management-server/icd-management-server.cpp @@ -169,6 +169,7 @@ class IcdManagementFabricDelegate : public chip::FabricTable::Delegate ICDMonitoringTable table(Server::GetInstance().GetPersistentStorage(), fabricIndex, supported_clients, Server::GetInstance().GetSessionKeystore()); table.RemoveAll(); + ICDManagementServer::GetInstance().TableIsEmptyForFabric(); } }; diff --git a/src/app/icd/BUILD.gn b/src/app/icd/BUILD.gn index 0879e296865a04..db40cfff85b3ff 100644 --- a/src/app/icd/BUILD.gn +++ b/src/app/icd/BUILD.gn @@ -57,6 +57,7 @@ source_set("cluster") { ] public_deps = [ + ":notifier", "${chip_root}/src/lib/core", "${chip_root}/src/platform:platform", "${chip_root}/src/protocols:im_status", diff --git a/src/app/icd/ICDManagementServer.cpp b/src/app/icd/ICDManagementServer.cpp index fe41617cfb3a71..2e0fe0a00b326d 100644 --- a/src/app/icd/ICDManagementServer.cpp +++ b/src/app/icd/ICDManagementServer.cpp @@ -1,4 +1,5 @@ #include "ICDManagementServer.h" +#include using namespace chip; using namespace chip::Protocols; @@ -11,6 +12,7 @@ Status ICDManagementServer::RegisterClient(PersistentStorageDelegate & storage, uint64_t monitored_subject, chip::ByteSpan key, Optional verification_key, bool is_admin) { + bool isFirstEntryForFabric = false; ICDMonitoringTable table(storage, fabric_index, GetClientsSupportedPerFabric(), mSymmetricKeystore); // Get current entry, if exists @@ -29,6 +31,9 @@ Status ICDManagementServer::RegisterClient(PersistentStorageDelegate & storage, { // New entry VerifyOrReturnError(entry.index < table.Limit(), InteractionModel::Status::ResourceExhausted); + + // Check if it's going to be the first entry for fabric + isFirstEntryForFabric = table.IsEmpty(); } else { @@ -58,6 +63,12 @@ Status ICDManagementServer::RegisterClient(PersistentStorageDelegate & storage, VerifyOrReturnError(CHIP_ERROR_INVALID_ARGUMENT != err, InteractionModel::Status::ConstraintError); VerifyOrReturnError(CHIP_NO_ERROR == err, InteractionModel::Status::Failure); + if (isFirstEntryForFabric) + { + // Notify subscribers that the first entry for the fabric was successfully added + app::ICDNotifier::GetInstance().BroadcastICDManagementEvent(app::ICDListener::ICDManagementEvents::kTableUpdated); + } + return InteractionModel::Status::Success; } @@ -82,13 +93,25 @@ Status ICDManagementServer::UnregisterClient(PersistentStorageDelegate & storage err = table.Remove(entry.index); VerifyOrReturnError(CHIP_NO_ERROR == err, InteractionModel::Status::Failure); + if (table.IsEmpty()) + { + TableIsEmptyForFabric(); + } + return InteractionModel::Status::Success; } Status ICDManagementServer::StayActiveRequest(FabricIndex fabric_index) { // TODO: Implementent stay awake logic for end device + // https://github.com/project-chip/connectedhomeip/issues/24259 + app::ICDNotifier::GetInstance().BroadcastICDManagementEvent(app::ICDListener::ICDManagementEvents::kStayActiveRequestReceived); return InteractionModel::Status::UnsupportedCommand; } +void ICDManagementServer::TableIsEmptyForFabric() +{ + app::ICDNotifier::GetInstance().BroadcastICDManagementEvent(app::ICDListener::ICDManagementEvents::kTableUpdated); +} + } // namespace chip diff --git a/src/app/icd/ICDManagementServer.h b/src/app/icd/ICDManagementServer.h index 7bb97e39a33dfd..25a4029ebd573f 100644 --- a/src/app/icd/ICDManagementServer.h +++ b/src/app/icd/ICDManagementServer.h @@ -53,6 +53,8 @@ class ICDManagementServer Status UnregisterClient(PersistentStorageDelegate & storage, FabricIndex fabric_index, chip::NodeId node_id, Optional verificationKey, bool is_admin); + void TableIsEmptyForFabric(); + Status StayActiveRequest(FabricIndex fabric_index); static ICDManagementServer & GetInstance() { return mInstance; } @@ -75,6 +77,8 @@ class ICDManagementServer uint16_t mActiveThreshold_ms = CHIP_CONFIG_ICD_ACTIVE_MODE_THRESHOLD_MS; + // TODO : Implement ICD counter + // https://github.com/project-chip/connectedhomeip/issues/29184 uint32_t mICDCounter = 0; static_assert((CHIP_CONFIG_ICD_CLIENTS_SUPPORTED_PER_FABRIC) >= 1, diff --git a/src/app/icd/ICDManager.cpp b/src/app/icd/ICDManager.cpp index 027b7ca6be53d3..c53a0b66cb8d7a 100644 --- a/src/app/icd/ICDManager.cpp +++ b/src/app/icd/ICDManager.cpp @@ -58,6 +58,10 @@ void ICDManager::Init(PersistentStorageDelegate * storage, FabricTable * fabricT VerifyOrDieWithMsg((supportLIT == false) || SupportsFeature(Feature::kUserActiveModeTrigger), AppServer, "The user ActiveMode trigger feature is required for LIT support"); + // Disabling check until LIT support is compelte + // VerifyOrDieWithMsg((supportLIT == false) && (GetSlowPollingInterval() <= GetSITPollingThreshold()) , AppServer, + // "LIT support is required for slow polling intervals superior to 15 seconds"); + mStorage = storage; mFabricTable = fabricTable; mStateObserver = stateObserver; @@ -90,13 +94,16 @@ void ICDManager::Shutdown() bool ICDManager::SupportsFeature(Feature feature) { - bool success = false; - uint32_t featureMap = 0; // Can't use attribute accessors/Attributes::FeatureMap::Get in unit tests #ifndef CONFIG_BUILD_FOR_HOST_UNIT_TEST - success = (Attributes::FeatureMap::Get(kRootEndpointId, &featureMap) == EMBER_ZCL_STATUS_SUCCESS); -#endif + bool success = false; + uint32_t featureMap = 0; + success = (Attributes::FeatureMap::Get(kRootEndpointId, &featureMap) == EMBER_ZCL_STATUS_SUCCESS); + return success ? ((featureMap & to_underlying(feature)) != 0) : false; +#else + return ((mFeatureMap & to_underlying(feature)) != 0); +#endif // CONFIG_BUILD_FOR_HOST_UNIT_TEST } void ICDManager::UpdateICDMode() @@ -105,9 +112,8 @@ void ICDManager::UpdateICDMode() ICDMode tempMode = ICDMode::SIT; - // The Check In Protocol Feature is required and the slow polling interval shall also be greater than 15 seconds - // to run an ICD in LIT mode. - if (GetSlowPollingInterval() > GetSITPollingThreshold() && SupportsFeature(Feature::kLongIdleTimeSupport)) + // Device can only switch to the LIT operating mode if LIT support is present + if (SupportsFeature(Feature::kLongIdleTimeSupport)) { VerifyOrDie(mStorage != nullptr); VerifyOrDie(mFabricTable != nullptr); @@ -315,5 +321,23 @@ void ICDManager::OnNetworkActivity() { this->UpdateOperationState(OperationalState::ActiveMode); } + +void ICDManager::OnICDManagementServerEvent(ICDManagementEvents event) +{ + switch (event) + { + case ICDManagementEvents::kTableUpdated: + this->UpdateICDMode(); + break; + + case ICDManagementEvents::kStayActiveRequestReceived: + // TODO : Implement the StayActiveRequest + // https://github.com/project-chip/connectedhomeip/issues/24259 + break; + default: + break; + } +} + } // namespace app } // namespace chip diff --git a/src/app/icd/ICDManager.h b/src/app/icd/ICDManager.h index 61910370b9a40b..c12f194798ccf6 100644 --- a/src/app/icd/ICDManager.h +++ b/src/app/icd/ICDManager.h @@ -67,11 +67,16 @@ class ICDManager : public ICDListener static System::Clock::Milliseconds32 GetSlowPollingInterval() { return kSlowPollingInterval; } static System::Clock::Milliseconds32 GetFastPollingInterval() { return kFastPollingInterval; } +#ifdef CONFIG_BUILD_FOR_HOST_UNIT_TEST + void SetTestFeatureMapValue(uint32_t featureMap) { mFeatureMap = featureMap; }; +#endif + // Implementation of ICDListener functions. // Callers must origin from the chip task context or be holding the ChipStack lock. void OnNetworkActivity() override; void OnKeepActiveRequest(KeepActiveFlags request) override; void OnActiveRequestWithdrawal(KeepActiveFlags request) override; + void OnICDManagementServerEvent(ICDManagementEvents event) override; protected: friend class TestICDManager; @@ -108,6 +113,11 @@ class ICDManager : public ICDListener ICDStateObserver * mStateObserver = nullptr; bool mTransitionToIdleCalled = false; Crypto::SymmetricKeystore * mSymmetricKeystore = nullptr; + +#ifdef CONFIG_BUILD_FOR_HOST_UNIT_TEST + // feature map that can be changed at runtime for testing purposes + uint32_t mFeatureMap = 0; +#endif }; } // namespace app diff --git a/src/app/icd/ICDNotifier.cpp b/src/app/icd/ICDNotifier.cpp index a3b4588004809a..6f2e537b0bd5c7 100644 --- a/src/app/icd/ICDNotifier.cpp +++ b/src/app/icd/ICDNotifier.cpp @@ -89,5 +89,16 @@ void ICDNotifier::BroadcastActiveRequestWithdrawal(ICDListener::KeepActiveFlags } } +void ICDNotifier::BroadcastICDManagementEvent(ICDListener::ICDManagementEvents event) +{ + for (auto subscriber : mSubscribers) + { + if (subscriber != nullptr) + { + subscriber->OnICDManagementServerEvent(event); + } + } +} + } // namespace app } // namespace chip diff --git a/src/app/icd/ICDNotifier.h b/src/app/icd/ICDNotifier.h index fc2b8b744c8302..9f6032c8616903 100644 --- a/src/app/icd/ICDNotifier.h +++ b/src/app/icd/ICDNotifier.h @@ -45,6 +45,12 @@ class ICDListener kExchangeContextOpen = 0x03, }; + enum class ICDManagementEvents : uint8_t + { + kTableUpdated = 0x01, + kStayActiveRequestReceived = 0x02, + }; + virtual ~ICDListener() {} /** @@ -68,6 +74,14 @@ class ICDListener * @param request : The request source */ virtual void OnActiveRequestWithdrawal(KeepActiveFlags request) = 0; + + /** + * @brief This function is called for all subscribers of the ICDNotifier when it calls BroadcastICDManagementEvent + * It informs the subscriber that an ICD Management action has happened and needs to be processed + * + * @param event : The event type + */ + virtual void OnICDManagementServerEvent(ICDManagementEvents event){}; }; class ICDNotifier @@ -85,6 +99,7 @@ class ICDNotifier void BroadcastNetworkActivityNotification(); void BroadcastActiveRequestNotification(ICDListener::KeepActiveFlags request); void BroadcastActiveRequestWithdrawal(ICDListener::KeepActiveFlags request); + void BroadcastICDManagementEvent(ICDListener::ICDManagementEvents event); static ICDNotifier & GetInstance() { return sICDNotifier; } diff --git a/src/app/tests/TestICDManager.cpp b/src/app/tests/TestICDManager.cpp index 3b204df306c06d..0972f4388db42f 100644 --- a/src/app/tests/TestICDManager.cpp +++ b/src/app/tests/TestICDManager.cpp @@ -38,6 +38,28 @@ using TestSessionKeystoreImpl = Crypto::DefaultSessionKeystore; namespace { +// Test Values +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 uint8_t kKeyBuffer1a[] = { + 0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18, 0x19, 0x1a, 0x1b, 0x1c, 0x1d, 0x1e, 0x1f +}; +constexpr uint8_t kKeyBuffer1b[] = { + 0xf1, 0xe1, 0xd1, 0xc1, 0xb1, 0xa1, 0x91, 0x81, 0x71, 0x61, 0x51, 0x14, 0x31, 0x21, 0x11, 0x01 +}; +constexpr uint8_t kKeyBuffer2a[] = { + 0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27, 0x28, 0x29, 0x2a, 0x2b, 0x2c, 0x2d, 0x2e, 0x2f +}; +constexpr uint8_t kKeyBuffer2b[] = { + 0xf2, 0xe2, 0xd2, 0xc2, 0xb2, 0xa2, 0x92, 0x82, 0x72, 0x62, 0x52, 0x42, 0x32, 0x22, 0x12, 0x02 +}; + class TestICDStateObserver : public app::ICDStateObserver { public: @@ -67,8 +89,7 @@ class TestContext : public Test::AppContext { return FAILURE; } - TestSessionKeystoreImpl keystore; - ctx->mICDManager.Init(&ctx->testStorage, &ctx->GetFabricTable(), &mICDStateObserver, &keystore); + ctx->mICDManager.Init(&ctx->testStorage, &ctx->GetFabricTable(), &mICDStateObserver, &(ctx->mKeystore)); return SUCCESS; } @@ -86,10 +107,11 @@ class TestContext : public Test::AppContext return SUCCESS; } + TestSessionKeystoreImpl mKeystore; app::ICDManager mICDManager; + TestPersistentStorageDelegate testStorage; private: - TestPersistentStorageDelegate testStorage; MonotonicallyIncreasingCounter mEventCounter; }; @@ -184,6 +206,109 @@ class TestICDManager notifier.BroadcastActiveRequestWithdrawal(ActiveFlag::kExchangeContextOpen); NL_TEST_ASSERT(aSuite, ctx->mICDManager.mOperationalState == ICDManager::OperationalState::IdleMode); } + + /* + * Test that verifies that the ICDManager is the correct operating mode based on entries + * in the ICDMonitoringTable + */ + static void TestICDMRegisterUnregisterEvents(nlTestSuite * aSuite, void * aContext) + { + TestContext * ctx = static_cast(aContext); + typedef ICDListener::ICDManagementEvents ICDMEvent; + ICDNotifier notifier = ICDNotifier::GetInstance(); + + // Set FeatureMap + // Configures CIP, UAT and LITS to 1 + ctx->mICDManager.SetTestFeatureMapValue(0x07); + + // Check ICDManager starts in SIT mode if no entries are present + NL_TEST_ASSERT(aSuite, ctx->mICDManager.GetICDMode() == ICDManager::ICDMode::SIT); + + // Trigger a "fake" register, ICDManager shoudl remain in SIT mode + notifier.BroadcastICDManagementEvent(ICDMEvent::kTableUpdated); + + // Check ICDManager stayed in SIT mode + NL_TEST_ASSERT(aSuite, ctx->mICDManager.GetICDMode() == ICDManager::ICDMode::SIT); + + // Create tables with different fabrics + ICDMonitoringTable table1(ctx->testStorage, kTestFabricIndex1, kMaxTestClients, &(ctx->mKeystore)); + ICDMonitoringTable table2(ctx->testStorage, kTestFabricIndex2, kMaxTestClients, &(ctx->mKeystore)); + + // Add first entry to the first fabric + ICDMonitoringEntry entry1(&(ctx->mKeystore)); + entry1.checkInNodeID = kClientNodeId11; + entry1.monitoredSubject = kClientNodeId12; + NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == entry1.SetKey(ByteSpan(kKeyBuffer1a))); + NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == table1.Set(0, entry1)); + + // Trigger register event after first entry was added + notifier.BroadcastICDManagementEvent(ICDMEvent::kTableUpdated); + + // Check ICDManager is now in the LIT operating mode + NL_TEST_ASSERT(aSuite, ctx->mICDManager.GetICDMode() == ICDManager::ICDMode::LIT); + + // Add second entry to the first fabric + ICDMonitoringEntry entry2(&(ctx->mKeystore)); + entry2.checkInNodeID = kClientNodeId12; + entry2.monitoredSubject = kClientNodeId11; + NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == entry2.SetKey(ByteSpan(kKeyBuffer1b))); + NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == table1.Set(1, entry2)); + + // Trigger register event after first entry was added + notifier.BroadcastICDManagementEvent(ICDMEvent::kTableUpdated); + + // Check ICDManager is now in the LIT operating mode + NL_TEST_ASSERT(aSuite, ctx->mICDManager.GetICDMode() == ICDManager::ICDMode::LIT); + + // Add first entry to the first fabric + ICDMonitoringEntry entry3(&(ctx->mKeystore)); + entry3.checkInNodeID = kClientNodeId21; + entry3.monitoredSubject = kClientNodeId22; + NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == entry3.SetKey(ByteSpan(kKeyBuffer2a))); + NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == table2.Set(0, entry3)); + + // Trigger register event after first entry was added + notifier.BroadcastICDManagementEvent(ICDMEvent::kTableUpdated); + + // Check ICDManager is now in the LIT operating mode + NL_TEST_ASSERT(aSuite, ctx->mICDManager.GetICDMode() == ICDManager::ICDMode::LIT); + + // Add second entry to the first fabric + ICDMonitoringEntry entry4(&(ctx->mKeystore)); + entry4.checkInNodeID = kClientNodeId22; + entry4.monitoredSubject = kClientNodeId21; + NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == entry4.SetKey(ByteSpan(kKeyBuffer2b))); + NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == table2.Set(1, entry4)); + + // Clear a fabric + NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == table2.RemoveAll()); + + // Trigger register event after fabric was cleared + notifier.BroadcastICDManagementEvent(ICDMEvent::kTableUpdated); + + // Check ICDManager is still in the LIT operating mode + NL_TEST_ASSERT(aSuite, ctx->mICDManager.GetICDMode() == ICDManager::ICDMode::LIT); + + // Remove single entry from remaining fabric + NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == table1.Remove(1)); + + // Trigger register event after fabric was cleared + notifier.BroadcastICDManagementEvent(ICDMEvent::kTableUpdated); + + // Check ICDManager is still in the LIT operating mode + NL_TEST_ASSERT(aSuite, ctx->mICDManager.GetICDMode() == ICDManager::ICDMode::LIT); + + // Remove last entry from remaining fabric + NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == table1.Remove(0)); + NL_TEST_ASSERT(aSuite, table1.IsEmpty()); + NL_TEST_ASSERT(aSuite, table2.IsEmpty()); + + // Trigger register event after fabric was cleared + notifier.BroadcastICDManagementEvent(ICDMEvent::kTableUpdated); + + // Check ICDManager is still in the LIT operating mode + NL_TEST_ASSERT(aSuite, ctx->mICDManager.GetICDMode() == ICDManager::ICDMode::SIT); + } }; } // namespace app @@ -198,6 +323,7 @@ static const nlTest sTests[] = { NL_TEST_DEF("TestICDModeDurations", TestICDManager::TestICDModeDurations), NL_TEST_DEF("TestKeepActivemodeRequests", TestICDManager::TestKeepActivemodeRequests), + NL_TEST_DEF("TestICDMRegisterUnregisterEvents", TestICDManager::TestICDMRegisterUnregisterEvents), NL_TEST_SENTINEL() }; // clang-format on diff --git a/src/app/tests/TestICDMonitoringTable.cpp b/src/app/tests/TestICDMonitoringTable.cpp index 91430e404188e6..a228e3368696fc 100644 --- a/src/app/tests/TestICDMonitoringTable.cpp +++ b/src/app/tests/TestICDMonitoringTable.cpp @@ -60,8 +60,6 @@ constexpr uint8_t kKeyBuffer2b[] = { constexpr uint8_t kKeyBuffer3a[] = { 0x30, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, 0x38, 0x39, 0x3a, 0x3b, 0x3c, 0x3d, 0x3e, 0x3f }; -// constexpr uint8_t kKeyBuffer3b[] = { 0xf3, 0xe3, 0xd3, 0xc3, 0xb3, 0xa3, 0x93, 0x83, 0x73, 0x63, 0x53, 0x14, 0x33, 0x23, 0x13, -// 0x03 }; void TestEntryKeyFunctions(nlTestSuite * aSuite, void * aContext) {