From 60968d9ad9a04d3e5ceb641f0c68455e814ad6b9 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Tue, 23 Jul 2024 14:39:37 -0400 Subject: [PATCH 01/10] Add MaximunCheckInBackOff attribute to ICDM cluster --- .../lit-icd-common/lit-icd-server-app.matter | 1 + .../lit-icd-common/lit-icd-server-app.zap | 30 ++++++++++++++----- .../icd-management-server.cpp | 10 +++++++ src/app/icd/server/ICDConfigurationData.h | 8 +++++ src/app/zap-templates/zcl/zcl.json | 3 +- src/lib/core/CHIPConfig.h | 9 ++++++ 6 files changed, 53 insertions(+), 8 deletions(-) diff --git a/examples/lit-icd-app/lit-icd-common/lit-icd-server-app.matter b/examples/lit-icd-app/lit-icd-common/lit-icd-server-app.matter index 4f8b1b55b51f42..ff09daa38da215 100644 --- a/examples/lit-icd-app/lit-icd-common/lit-icd-server-app.matter +++ b/examples/lit-icd-app/lit-icd-common/lit-icd-server-app.matter @@ -1796,6 +1796,7 @@ endpoint 0 { ram attribute userActiveModeTriggerHint default = 0x111D; ram attribute userActiveModeTriggerInstruction default = "Restart the application"; ram attribute operatingMode default = 0; + callback attribute maximumCheckInBackOff; callback attribute generatedCommandList; callback attribute acceptedCommandList; callback attribute eventList; diff --git a/examples/lit-icd-app/lit-icd-common/lit-icd-server-app.zap b/examples/lit-icd-app/lit-icd-common/lit-icd-server-app.zap index 2a75af9025f0d5..bc32dbce677fb2 100644 --- a/examples/lit-icd-app/lit-icd-common/lit-icd-server-app.zap +++ b/examples/lit-icd-app/lit-icd-common/lit-icd-server-app.zap @@ -17,13 +17,6 @@ } ], "package": [ - { - "pathRelativity": "relativeToZap", - "path": "../../../src/app/zap-templates/app-templates.json", - "type": "gen-templates-json", - "category": "matter", - "version": "chip-v1" - }, { "pathRelativity": "relativeToZap", "path": "../../../src/app/zap-templates/zcl/zcl.json", @@ -31,6 +24,13 @@ "category": "matter", "version": 1, "description": "Matter SDK ZCL data" + }, + { + "pathRelativity": "relativeToZap", + "path": "../../../src/app/zap-templates/app-templates.json", + "type": "gen-templates-json", + "category": "matter", + "version": "chip-v1" } ], "endpointTypes": [ @@ -3552,6 +3552,22 @@ "maxInterval": 65534, "reportableChange": 0 }, + { + "name": "MaximumCheckInBackOff", + "code": 9, + "mfgCode": null, + "side": "server", + "type": "int32u", + "included": 1, + "storageOption": "External", + "singleton": 0, + "bounded": 0, + "defaultValue": "", + "reportable": 1, + "minInterval": 1, + "maxInterval": 65534, + "reportableChange": 0 + }, { "name": "GeneratedCommandList", "code": 65528, 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 55f6b5129c0a41..bfc5dfb70c2feb 100644 --- a/src/app/clusters/icd-management-server/icd-management-server.cpp +++ b/src/app/clusters/icd-management-server/icd-management-server.cpp @@ -69,6 +69,7 @@ class IcdManagementAttributeAccess : public AttributeAccessInterface CHIP_ERROR ReadRegisteredClients(EndpointId endpoint, AttributeValueEncoder & encoder); CHIP_ERROR ReadICDCounter(EndpointId endpoint, AttributeValueEncoder & encoder); CHIP_ERROR ReadClientsSupportedPerFabric(EndpointId endpoint, AttributeValueEncoder & encoder); + CHIP_ERROR ReadMaximumCheckInBackOff(EndpointId endpoint, AttributeValueEncoder & encoder); PersistentStorageDelegate * mStorage = nullptr; Crypto::SymmetricKeystore * mSymmetricKeystore = nullptr; @@ -102,6 +103,10 @@ CHIP_ERROR IcdManagementAttributeAccess::Read(const ConcreteReadAttributePath & case IcdManagement::Attributes::ClientsSupportedPerFabric::Id: return ReadClientsSupportedPerFabric(aPath.mEndpointId, aEncoder); + + case IcdManagement::Attributes::MaximumCheckInBackOff::Id: + return ReadMaximumCheckInBackOff(aPath.mEndpointId, aEncoder); + #endif // CHIP_CONFIG_ENABLE_ICD_CIP } @@ -123,6 +128,11 @@ CHIP_ERROR IcdManagementAttributeAccess::ReadActiveModeThreshold(EndpointId endp return encoder.Encode(mICDConfigurationData->GetActiveModeThreshold().count()); } +CHIP_ERROR IcdManagementAttributeAccess::ReadMaximumCheckInBackOff(EndpointId endpoint, AttributeValueEncoder & encoder) +{ + return encoder.Encode(mICDConfigurationData->GetMaximumCheckInBackoff().count()); +} + #if CHIP_CONFIG_ENABLE_ICD_CIP /** * @brief Implementation of Fabric Delegate for ICD Management cluster diff --git a/src/app/icd/server/ICDConfigurationData.h b/src/app/icd/server/ICDConfigurationData.h index 9358f37fd9ecc6..937b08b99e0e45 100644 --- a/src/app/icd/server/ICDConfigurationData.h +++ b/src/app/icd/server/ICDConfigurationData.h @@ -74,6 +74,8 @@ class ICDConfigurationData System::Clock::Milliseconds16 GetMinLitActiveModeThreshold() { return kMinLitActiveModeThreshold; } + System::Clock::Seconds32 GetMaximumCheckInBackoff() { return mMaximumCheckInBackOff; } + /** * If ICD_ENFORCE_SIT_SLOW_POLL_LIMIT is set to 0, function will always return the configured Slow Polling interval * (CHIP_DEVICE_CONFIG_ICD_SLOW_POLL_INTERVAL). @@ -150,6 +152,12 @@ class ICDConfigurationData "Spec requires the minimum of supported clients per fabric be equal or greater to 1."); uint16_t mFabricClientsSupported = CHIP_CONFIG_ICD_CLIENTS_SUPPORTED_PER_FABRIC; + static_assert((CHIP_CONFIG_ICD_MAXIMUM_CHECK_IN_BACKOFF_SEC) <= kMaxIdleModeDuration.count(), + "Spec requires the MaximumCheckInBackOff to be equal or inferior to 64800s"); + static_assert((CHIP_CONFIG_ICD_IDLE_MODE_DURATION_SEC) <= (CHIP_CONFIG_ICD_MAXIMUM_CHECK_IN_BACKOFF_SEC), + "Spec requires the MaximumCheckInBackOff to be equal or superior to the IdleModeDuration"); + System::Clock::Seconds32 mMaximumCheckInBackOff = System::Clock::Seconds32(CHIP_CONFIG_ICD_MAXIMUM_CHECK_IN_BACKOFF_SEC); + // SIT ICDs should have a SlowPollingThreshold shorter than or equal to 15s (spec 9.16.1.5) static constexpr System::Clock::Milliseconds32 kSITPollingThreshold = System::Clock::Milliseconds32(15000); System::Clock::Milliseconds32 mSlowPollingInterval = CHIP_DEVICE_CONFIG_ICD_SLOW_POLL_INTERVAL; diff --git a/src/app/zap-templates/zcl/zcl.json b/src/app/zap-templates/zcl/zcl.json index 5388a12156601a..13524b99d9b2da 100644 --- a/src/app/zap-templates/zcl/zcl.json +++ b/src/app/zap-templates/zcl/zcl.json @@ -276,7 +276,8 @@ "ActiveModeThreshold", "RegisteredClients", "ICDCounter", - "ClientsSupportedPerFabric" + "ClientsSupportedPerFabric", + "MaximumCheckInBackOff" ], "Occupancy Sensing": ["HoldTimeLimits"], "Operational Credentials": [ diff --git a/src/lib/core/CHIPConfig.h b/src/lib/core/CHIPConfig.h index 32e49ab2e3d45f..e9c317dfc79d5c 100644 --- a/src/lib/core/CHIPConfig.h +++ b/src/lib/core/CHIPConfig.h @@ -1624,6 +1624,15 @@ extern const char CHIP_NON_PRODUCTION_MARKER[]; #define CHIP_CONFIG_ICD_CLIENTS_SUPPORTED_PER_FABRIC 2 #endif +/** + * @def CHIP_CONFIG_ICD_MAXIMUM_CHECK_IN_BACKOFF + * + * @brief Default value for the ICD Management cluster MaximumCheckInBackoff attribute, in seconds + */ +#ifndef CHIP_CONFIG_ICD_MAXIMUM_CHECK_IN_BACKOFF_SEC +#define CHIP_CONFIG_ICD_MAXIMUM_CHECK_IN_BACKOFF_SEC CHIP_CONFIG_ICD_IDLE_MODE_DURATION_SEC +#endif + /** * @name Configuation for resuming subscriptions that timed out * From 239b3544bce34ef9c42aaccd79b5318bdbd98cf7 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Wed, 24 Jul 2024 10:34:49 -0400 Subject: [PATCH 02/10] Add injectable ICD Check-In BackOff strategy --- src/app/icd/server/BUILD.gn | 17 +++++ .../server/DefaultICDCheckInBackOffStrategy.h | 63 +++++++++++++++++++ .../icd/server/ICDCheckInBackOffStrategy.h | 62 ++++++++++++++++++ src/app/icd/server/ICDManager.cpp | 34 +++++----- src/app/icd/server/ICDManager.h | 21 ++++--- src/app/icd/server/tests/BUILD.gn | 2 + .../TestDefaultICDCheckInBackOffStrategy.cpp | 57 +++++++++++++++++ src/app/icd/server/tests/TestICDManager.cpp | 8 ++- src/app/server/BUILD.gn | 7 +++ src/app/server/Server.cpp | 5 +- src/app/server/Server.h | 18 ++++++ 11 files changed, 267 insertions(+), 27 deletions(-) create mode 100644 src/app/icd/server/DefaultICDCheckInBackOffStrategy.h create mode 100644 src/app/icd/server/ICDCheckInBackOffStrategy.h create mode 100644 src/app/icd/server/tests/TestDefaultICDCheckInBackOffStrategy.cpp diff --git a/src/app/icd/server/BUILD.gn b/src/app/icd/server/BUILD.gn index 89c39c203a7c16..b30b79d3d4a1d0 100644 --- a/src/app/icd/server/BUILD.gn +++ b/src/app/icd/server/BUILD.gn @@ -66,6 +66,22 @@ source_set("notifier") { ] } +source_set("check-in-back-off") { + sources = [ "ICDCheckInBackOffStrategy.h" ] + + public_deps = [ + ":monitoring-table", + "${chip_root}/src/lib/core", + "${chip_root}/src/lib/support", + ] +} + +source_set("default-check-in-back-off") { + sources = [ "DefaultICDCheckInBackOffStrategy.h" ] + + public_deps = [ ":check-in-back-off" ] +} + # ICD Manager source-set is broken out of the main source-set to enable unit tests # All sources and configurations used by the ICDManager need to go in this source-set source_set("manager") { @@ -89,6 +105,7 @@ source_set("manager") { if (chip_enable_icd_checkin) { public_deps += [ + ":check-in-back-off", ":monitoring-table", ":sender", "${chip_root}/src/app:app_config", diff --git a/src/app/icd/server/DefaultICDCheckInBackOffStrategy.h b/src/app/icd/server/DefaultICDCheckInBackOffStrategy.h new file mode 100644 index 00000000000000..5043d15185f81d --- /dev/null +++ b/src/app/icd/server/DefaultICDCheckInBackOffStrategy.h @@ -0,0 +1,63 @@ +/* + * + * Copyright (c) 2024 Project CHIP Authors + * + * 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. + */ + +#pragma once + +#include +#include + +namespace chip { +namespace app { + +/** + * @brief Default ICD Check-In BackOff Strategy. + * The default strategy is based on the two types of controllers + * - kPermanent : Always send a Check-In message + * - kEphemeral : Never send a Check-In message + * + * This implementation represents a no back off strategy. + */ +class DefaultICDCheckInBackOffStrategy : public ICDCheckInBackOffStrategy +{ +public: + DefaultICDCheckInBackOffStrategy() = default; + ~DefaultICDCheckInBackOffStrategy() = default; + + /** + * @brief Function checks if the entry is a permanent or ephemeral client. + * If the client is permanent, we should send a Check-In message. + * If the cliet is ephemerakl, we should not send a Check-In message. + * + * @param entry Entry for which we are deciding if we need to send a Check-In message or not. + * @return true If the client is permanent, return true. + * @return false If the client is not permanent, ephemeral or invalid, return false. + */ + bool ShouldSendCheckInMessage(const ICDMonitoringEntry & entry) + { + return (entry.clientType == Clusters::IcdManagement::ClientTypeEnum::kPermanent); + } + + /** + * @brief The default Check-In BackOff fundamentally implements a no back off strategy. + * As such, we don't need to execute anything to force the maximum Check-In BackOff. + * + */ + CHIP_ERROR ForceMaximumCheckInBackoff() { return CHIP_NO_ERROR; } +}; + +} // namespace app +} // namespace chip diff --git a/src/app/icd/server/ICDCheckInBackOffStrategy.h b/src/app/icd/server/ICDCheckInBackOffStrategy.h new file mode 100644 index 00000000000000..97ca7c9a4d3418 --- /dev/null +++ b/src/app/icd/server/ICDCheckInBackOffStrategy.h @@ -0,0 +1,62 @@ +/* + * + * Copyright (c) 2024 Project CHIP Authors + * + * 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. + */ +#pragma once + +#include +#include + +namespace chip { +namespace app { + +/** + * @brief This class defines the necessary interface a ICD Check-In BackOff strategy needs to implment to be consummed by the + * ICDManager class. The strategy is injected with the init server params when initializing the device Server class. + */ +class ICDCheckInBackOffStrategy +{ +public: + virtual ~ICDCheckInBackOffStrategy() = default; + + /** + * @brief Function is used by the ICDManager to determine if a Check-In message should be sent to the given entry based on the + * Check-In BackOff strategy. + * + * There are no requirements on how the Check-In BackOff strategy should behave. + * The only specified requirement is the maximum time between to Check-In message, MaximumCheckInBackOff. + * All strategies must respect this requirement. + * + * @param entry ICDMonitoringEntry for which we are about to send a Check-In message to. + * + * @return true ICDCheckInBackOffStrategy determines that we SHOULD send a Check-In message to the given entry + * @return falseI CDCheckInBackOffStrategy determines that we SHOULD NOT send a Check-In message to the given entry + */ + virtual bool ShouldSendCheckInMessage(const ICDMonitoringEntry & entry) = 0; + + /** + * @brief Function is used within the test event trigger to force the maximum BackOff state of the ICD Check-In BackOff + * strategy. This enables to validate the strategy and to certify it respects the MaximumCheckInBackOff interval during + * certification. + * + * Function sets the maxmimum BackOff state for all clients registered with the ICD + * + * @return CHIP_ERROR Any error returned during the forcing of the maximum BackOff state + */ + virtual CHIP_ERROR ForceMaximumCheckInBackoff() = 0; +}; + +} // namespace app +} // namespace chip diff --git a/src/app/icd/server/ICDManager.cpp b/src/app/icd/server/ICDManager.cpp index ee55b0b0f9b23b..07632a059b2f1c 100644 --- a/src/app/icd/server/ICDManager.cpp +++ b/src/app/icd/server/ICDManager.cpp @@ -31,10 +31,11 @@ namespace { enum class ICDTestEventTriggerEvent : uint64_t { - kAddActiveModeReq = 0x0046'0000'00000001, - kRemoveActiveModeReq = 0x0046'0000'00000002, - kInvalidateHalfCounterValues = 0x0046'0000'00000003, - kInvalidateAllCounterValues = 0x0046'0000'00000004, + kAddActiveModeReq = 0x0046'0000'00000001, + kRemoveActiveModeReq = 0x0046'0000'00000002, + kInvalidateHalfCounterValues = 0x0046'0000'00000003, + kInvalidateAllCounterValues = 0x0046'0000'00000004, + kForceMaximumCheckInBackOffState = 0x0046'0000'00000005, }; } // namespace @@ -52,7 +53,8 @@ static_assert(UINT8_MAX >= CHIP_CONFIG_MAX_EXCHANGE_CONTEXTS, "ICDManager::mOpenExchangeContextCount cannot hold count for the max exchange count"); void ICDManager::Init(PersistentStorageDelegate * storage, FabricTable * fabricTable, Crypto::SymmetricKeystore * symmetricKeystore, - Messaging::ExchangeManager * exchangeManager, SubscriptionsInfoProvider * subInfoProvider) + Messaging::ExchangeManager * exchangeManager, SubscriptionsInfoProvider * subInfoProvider, + ICDCheckInBackOffStrategy * strategy) { #if CHIP_CONFIG_ENABLE_ICD_CIP VerifyOrDie(storage != nullptr); @@ -60,6 +62,7 @@ void ICDManager::Init(PersistentStorageDelegate * storage, FabricTable * fabricT VerifyOrDie(symmetricKeystore != nullptr); VerifyOrDie(exchangeManager != nullptr); VerifyOrDie(subInfoProvider != nullptr); + VerifyOrDie(strategy != nullptr); #endif // CHIP_CONFIG_ENABLE_ICD_CIP #if CHIP_CONFIG_ENABLE_ICD_LIT @@ -82,11 +85,12 @@ void ICDManager::Init(PersistentStorageDelegate * storage, FabricTable * fabricT VerifyOrDie(ICDNotifier::GetInstance().Subscribe(this) == CHIP_NO_ERROR); #if CHIP_CONFIG_ENABLE_ICD_CIP - mStorage = storage; - mFabricTable = fabricTable; - mSymmetricKeystore = symmetricKeystore; - mExchangeManager = exchangeManager; - mSubInfoProvider = subInfoProvider; + mStorage = storage; + mFabricTable = fabricTable; + mSymmetricKeystore = symmetricKeystore; + mExchangeManager = exchangeManager; + mSubInfoProvider = subInfoProvider; + mICDCheckInBackOffStrategy = strategy; VerifyOrDie(ICDConfigurationData::GetInstance().GetICDCounter().Init(mStorage, DefaultStorageKeyAllocator::ICDCheckInCounter(), ICDConfigurationData::kICDCounterPersistenceIncrement) == @@ -188,15 +192,15 @@ void ICDManager::SendCheckInMsgs() continue; } - if (entry.clientType == ClientTypeEnum::kEphemeral) + if (!ShouldCheckInMsgsBeSentAtActiveModeFunction(entry.fabricIndex, entry.monitoredSubject)) { - // If the registered client is ephemeral, do not send a Check-In message - // continue to next entry continue; } - if (!ShouldCheckInMsgsBeSentAtActiveModeFunction(entry.fabricIndex, entry.monitoredSubject)) + // Validate Check-In BackOff strategy if we should send a Check-In message. + if (!mICDCheckInBackOffStrategy->ShouldSendCheckInMessage(entry)) { + // continue to next entry continue; } @@ -689,6 +693,8 @@ CHIP_ERROR ICDManager::HandleEventTrigger(uint64_t eventTrigger) case ICDTestEventTriggerEvent::kInvalidateAllCounterValues: err = ICDConfigurationData::GetInstance().GetICDCounter().InvalidateAllCheckInCounterValues(); break; + case ICDTestEventTriggerEvent::kForceMaximumCheckInBackOffState: + err = mICDCheckInBackOffStrategy->ForceMaximumCheckInBackoff(); #endif // CHIP_CONFIG_ENABLE_ICD_CIP default: err = CHIP_ERROR_INVALID_ARGUMENT; diff --git a/src/app/icd/server/ICDManager.h b/src/app/icd/server/ICDManager.h index 4b996e6dba5258..57aa5069a5904f 100644 --- a/src/app/icd/server/ICDManager.h +++ b/src/app/icd/server/ICDManager.h @@ -33,9 +33,10 @@ #include #if CHIP_CONFIG_ENABLE_ICD_CIP -#include // nogncheck -#include // nogncheck -#endif // CHIP_CONFIG_ENABLE_ICD_CIP +#include // nogncheck +#include // nogncheck +#include // nogncheck +#endif // CHIP_CONFIG_ENABLE_ICD_CIP namespace chip { namespace Crypto { @@ -115,7 +116,8 @@ class ICDManager : public ICDListener, public TestEventTriggerHandler ~ICDManager() = default; void Init(PersistentStorageDelegate * storage, FabricTable * fabricTable, Crypto::SymmetricKeystore * symmetricKeyStore, - Messaging::ExchangeManager * exchangeManager, SubscriptionsInfoProvider * subInfoProvider); + Messaging::ExchangeManager * exchangeManager, SubscriptionsInfoProvider * subInfoProvider, + ICDCheckInBackOffStrategy * strategy); void Shutdown(); /** @@ -318,11 +320,12 @@ class ICDManager : public ICDListener, public TestEventTriggerHandler bool mIsBootUpResumeSubscriptionExecuted = false; #endif // !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION && CHIP_CONFIG_PERSIST_SUBSCRIPTIONS - PersistentStorageDelegate * mStorage = nullptr; - FabricTable * mFabricTable = nullptr; - Messaging::ExchangeManager * mExchangeManager = nullptr; - Crypto::SymmetricKeystore * mSymmetricKeystore = nullptr; - SubscriptionsInfoProvider * mSubInfoProvider = nullptr; + PersistentStorageDelegate * mStorage = nullptr; + FabricTable * mFabricTable = nullptr; + Messaging::ExchangeManager * mExchangeManager = nullptr; + Crypto::SymmetricKeystore * mSymmetricKeystore = nullptr; + SubscriptionsInfoProvider * mSubInfoProvider = nullptr; + ICDCheckInBackOffStrategy * mICDCheckInBackOffStrategy = nullptr; ObjectPool mICDSenderPool; #endif // CHIP_CONFIG_ENABLE_ICD_CIP diff --git a/src/app/icd/server/tests/BUILD.gn b/src/app/icd/server/tests/BUILD.gn index 95ece1d28ae4df..9b08c9e17893ca 100644 --- a/src/app/icd/server/tests/BUILD.gn +++ b/src/app/icd/server/tests/BUILD.gn @@ -22,6 +22,7 @@ chip_test_suite("tests") { output_name = "libICDServerTests" test_sources = [ + "TestDefaultICDCheckInBackOffStrategy.cpp", "TestICDManager.cpp", "TestICDMonitoringTable.cpp", ] @@ -29,6 +30,7 @@ chip_test_suite("tests") { sources = [ "ICDConfigurationDataTestAccess.h" ] public_deps = [ + "${chip_root}/src/app/icd/server:default-check-in-back-off", "${chip_root}/src/app/icd/server:manager", "${chip_root}/src/app/icd/server:monitoring-table", "${chip_root}/src/lib/core:string-builder-adapters", diff --git a/src/app/icd/server/tests/TestDefaultICDCheckInBackOffStrategy.cpp b/src/app/icd/server/tests/TestDefaultICDCheckInBackOffStrategy.cpp new file mode 100644 index 00000000000000..b873379826d468 --- /dev/null +++ b/src/app/icd/server/tests/TestDefaultICDCheckInBackOffStrategy.cpp @@ -0,0 +1,57 @@ +/* + * + * Copyright (c) 2024 Project CHIP Authors + * + * 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. + */ + +#include + +#include +#include +#include +#include +#include +#include + +using namespace chip; +using namespace chip::app; +using namespace chip::app::Clusters::IcdManagement; + +using TestSessionKeystoreImpl = Crypto::DefaultSessionKeystore; + +namespace { + +TEST(TestDefaultICDCheckInBackOffStrategy, TestShouldSendCheckInMessagePermanentClient) +{ + TestSessionKeystoreImpl keystore; + ICDMonitoringEntry entry(&keystore); + + entry.clientType = ClientTypeEnum::kPermanent; + + DefaultICDCheckInBackOffStrategy strategy; + EXPECT_TRUE(strategy.ShouldSendCheckInMessage(entry)); +} + +TEST(TestDefaultICDCheckInBackOffStrategy, TestShouldSendCheckInMessageEphemeralClient) +{ + TestSessionKeystoreImpl keystore; + ICDMonitoringEntry entry(&keystore); + + entry.clientType = ClientTypeEnum::kEphemeral; + + DefaultICDCheckInBackOffStrategy strategy; + EXPECT_FALSE(strategy.ShouldSendCheckInMessage(entry)); +} + +} // namespace diff --git a/src/app/icd/server/tests/TestICDManager.cpp b/src/app/icd/server/tests/TestICDManager.cpp index 3672955bdf4b26..f4b5db2eb42c2a 100644 --- a/src/app/icd/server/tests/TestICDManager.cpp +++ b/src/app/icd/server/tests/TestICDManager.cpp @@ -20,6 +20,7 @@ #include #include +#include #include #include #include @@ -197,7 +198,7 @@ class TestICDManager : public Test::LoopbackMessagingContext mICDStateObserver.ResetAll(); mICDManager.RegisterObserver(&mICDStateObserver); - mICDManager.Init(&testStorage, &GetFabricTable(), &mKeystore, &GetExchangeManager(), &mSubInfoProvider); + mICDManager.Init(&testStorage, &GetFabricTable(), &mKeystore, &GetExchangeManager(), &mSubInfoProvider, &mStrategy); } // Performs teardown for each individual test in the test suite @@ -212,6 +213,7 @@ class TestICDManager : public Test::LoopbackMessagingContext TestSubscriptionsInfoProvider mSubInfoProvider; TestPersistentStorageDelegate testStorage; TestICDStateObserver mICDStateObserver; + DefaultICDCheckInBackOffStrategy mStrategy; }; TEST_F(TestICDManager, TestICDModeDurations) @@ -568,7 +570,7 @@ TEST_F(TestICDManager, TestICDCounter) // Shut down and reinit ICDManager to increment counter mICDManager.Shutdown(); - mICDManager.Init(&(testStorage), &GetFabricTable(), &(mKeystore), &GetExchangeManager(), &(mSubInfoProvider)); + mICDManager.Init(&(testStorage), &GetFabricTable(), &(mKeystore), &GetExchangeManager(), &(mSubInfoProvider), &mStrategy); mICDManager.RegisterObserver(&(mICDStateObserver)); EXPECT_EQ(counter + ICDConfigurationData::kICDCounterPersistenceIncrement, @@ -973,7 +975,7 @@ TEST_F(TestICDManager, TestICDStateObserverOnICDModeChangeOnInit) // Shut down and reinit ICDManager - We should go to LIT mode since we have a registration mICDManager.Shutdown(); mICDManager.RegisterObserver(&(mICDStateObserver)); - mICDManager.Init(&testStorage, &GetFabricTable(), &mKeystore, &GetExchangeManager(), &mSubInfoProvider); + mICDManager.Init(&testStorage, &GetFabricTable(), &mKeystore, &GetExchangeManager(), &mSubInfoProvider, &mStrategy); // We have a registration, transition to LIT mode EXPECT_TRUE(mICDStateObserver.mOnICDModeChangeCalled); diff --git a/src/app/server/BUILD.gn b/src/app/server/BUILD.gn index 51a259c86d2552..264e7e2e6d9daf 100644 --- a/src/app/server/BUILD.gn +++ b/src/app/server/BUILD.gn @@ -72,5 +72,12 @@ static_library("server") { if (chip_enable_icd_server) { public_deps += [ "${chip_root}/src/app/icd/server:notifier" ] + + if (chip_enable_icd_checkin) { + public_deps += [ + "${chip_root}/src/app/icd/server:check-in-back-off", + "${chip_root}/src/app/icd/server:default-check-in-back-off", + ] + } } } diff --git a/src/app/server/Server.cpp b/src/app/server/Server.cpp index 5be815f0864c95..23d3054179edaa 100644 --- a/src/app/server/Server.cpp +++ b/src/app/server/Server.cpp @@ -360,7 +360,7 @@ CHIP_ERROR Server::Init(const ServerInitParams & initParams) mICDManager.RegisterObserver(&app::DnssdServer::Instance()); mICDManager.Init(mDeviceStorage, &GetFabricTable(), mSessionKeystore, &mExchangeMgr, - chip::app::InteractionModelEngine::GetInstance()); + chip::app::InteractionModelEngine::GetInstance(), initParams.icdCheckInBackOffStrategy); // Register Test Event Trigger Handler if (mTestEventTriggerDelegate != nullptr) @@ -769,5 +769,8 @@ app::SimpleSubscriptionResumptionStorage CommonCaseDeviceServerInitParams::sSubs #endif app::DefaultAclStorage CommonCaseDeviceServerInitParams::sAclStorage; Crypto::DefaultSessionKeystore CommonCaseDeviceServerInitParams::sSessionKeystore; +#if CHIP_CONFIG_ENABLE_ICD_CIP +app::DefaultICDCheckInBackOffStrategy CommonCaseDeviceServerInitParams::sDefaultICDCheckInBackOffStrategy; +#endif } // namespace chip diff --git a/src/app/server/Server.h b/src/app/server/Server.h index d649e0fc923896..933b3438d3488a 100644 --- a/src/app/server/Server.h +++ b/src/app/server/Server.h @@ -69,8 +69,13 @@ #include #include +#include #if CHIP_CONFIG_ENABLE_ICD_SERVER #include // nogncheck + +#if CHIP_CONFIG_ENABLE_ICD_CIP +#include // nogncheck +#endif #endif namespace chip { @@ -164,6 +169,9 @@ struct ServerInitParams Credentials::OperationalCertificateStore * opCertStore = nullptr; // Required, if not provided, the Server::Init() WILL fail. app::reporting::ReportScheduler * reportScheduler = nullptr; + // Optionnal. Support for the ICD Check-In BackOff strategy. Must be initialized before being provided. + // If the ICD Check-In protocol use-case is supported and no strategy is prprovided, server will use the default strategy. + app::ICDCheckInBackOffStrategy * icdCheckInBackOffStrategy = nullptr; }; /** @@ -278,6 +286,13 @@ struct CommonCaseDeviceServerInitParams : public ServerInitParams ChipLogProgress(AppServer, "Subscription persistence not supported"); #endif +#if CHIP_CONFIG_ENABLE_ICD_CIP + if (this->icdCheckInBackOffStrategy == nullptr) + { + this->icdCheckInBackOffStrategy = &sDefaultICDCheckInBackOffStrategy; + } +#endif + return CHIP_NO_ERROR; } @@ -297,6 +312,9 @@ struct CommonCaseDeviceServerInitParams : public ServerInitParams #endif static app::DefaultAclStorage sAclStorage; static Crypto::DefaultSessionKeystore sSessionKeystore; +#if CHIP_CONFIG_ENABLE_ICD_CIP + static app::DefaultICDCheckInBackOffStrategy sDefaultICDCheckInBackOffStrategy; +#endif }; /** From b10d0bcd3955590986daebc7d907877d22532ff4 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Wed, 24 Jul 2024 15:47:30 -0400 Subject: [PATCH 03/10] Fix gn dependency --- src/app/server/BUILD.gn | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/app/server/BUILD.gn b/src/app/server/BUILD.gn index 264e7e2e6d9daf..401356d7b753a4 100644 --- a/src/app/server/BUILD.gn +++ b/src/app/server/BUILD.gn @@ -53,6 +53,7 @@ static_library("server") { public_deps = [ "${chip_root}/src/app", "${chip_root}/src/app:test-event-trigger", + "${chip_root}/src/app/icd/server:check-in-back-off", "${chip_root}/src/app/icd/server:icd-server-config", "${chip_root}/src/app/icd/server:observer", "${chip_root}/src/lib/address_resolve", @@ -74,10 +75,8 @@ static_library("server") { public_deps += [ "${chip_root}/src/app/icd/server:notifier" ] if (chip_enable_icd_checkin) { - public_deps += [ - "${chip_root}/src/app/icd/server:check-in-back-off", - "${chip_root}/src/app/icd/server:default-check-in-back-off", - ] + public_deps += + [ "${chip_root}/src/app/icd/server:default-check-in-back-off" ] } } } From c8b04e73b51292914fb3f3f7d25ae57806c8d13d Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Wed, 24 Jul 2024 17:00:07 -0400 Subject: [PATCH 04/10] Fix non CIP builds --- .../icd-management-server/icd-management-server.cpp | 11 +++++------ src/app/icd/server/BUILD.gn | 2 +- src/app/icd/server/ICDManager.h | 8 ++++---- 3 files changed, 10 insertions(+), 11 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 bfc5dfb70c2feb..e98cc5e31718b2 100644 --- a/src/app/clusters/icd-management-server/icd-management-server.cpp +++ b/src/app/clusters/icd-management-server/icd-management-server.cpp @@ -106,7 +106,6 @@ CHIP_ERROR IcdManagementAttributeAccess::Read(const ConcreteReadAttributePath & case IcdManagement::Attributes::MaximumCheckInBackOff::Id: return ReadMaximumCheckInBackOff(aPath.mEndpointId, aEncoder); - #endif // CHIP_CONFIG_ENABLE_ICD_CIP } @@ -128,11 +127,6 @@ CHIP_ERROR IcdManagementAttributeAccess::ReadActiveModeThreshold(EndpointId endp return encoder.Encode(mICDConfigurationData->GetActiveModeThreshold().count()); } -CHIP_ERROR IcdManagementAttributeAccess::ReadMaximumCheckInBackOff(EndpointId endpoint, AttributeValueEncoder & encoder) -{ - return encoder.Encode(mICDConfigurationData->GetMaximumCheckInBackoff().count()); -} - #if CHIP_CONFIG_ENABLE_ICD_CIP /** * @brief Implementation of Fabric Delegate for ICD Management cluster @@ -231,6 +225,11 @@ CHIP_ERROR IcdManagementAttributeAccess::ReadClientsSupportedPerFabric(EndpointI return encoder.Encode(mICDConfigurationData->GetClientsSupportedPerFabric()); } +CHIP_ERROR IcdManagementAttributeAccess::ReadMaximumCheckInBackOff(EndpointId endpoint, AttributeValueEncoder & encoder) +{ + return encoder.Encode(mICDConfigurationData->GetMaximumCheckInBackoff().count()); +} + /** * @brief Function checks if the client has admin permissions to the cluster in the commandPath * diff --git a/src/app/icd/server/BUILD.gn b/src/app/icd/server/BUILD.gn index b30b79d3d4a1d0..f69c25015592af 100644 --- a/src/app/icd/server/BUILD.gn +++ b/src/app/icd/server/BUILD.gn @@ -93,6 +93,7 @@ source_set("manager") { deps = [ ":icd-server-config" ] public_deps = [ + ":check-in-back-off", ":configuration-data", ":notifier", ":observer", @@ -105,7 +106,6 @@ source_set("manager") { if (chip_enable_icd_checkin) { public_deps += [ - ":check-in-back-off", ":monitoring-table", ":sender", "${chip_root}/src/app:app_config", diff --git a/src/app/icd/server/ICDManager.h b/src/app/icd/server/ICDManager.h index 57aa5069a5904f..a697071bd98dd1 100644 --- a/src/app/icd/server/ICDManager.h +++ b/src/app/icd/server/ICDManager.h @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -33,10 +34,9 @@ #include #if CHIP_CONFIG_ENABLE_ICD_CIP -#include // nogncheck -#include // nogncheck -#include // nogncheck -#endif // CHIP_CONFIG_ENABLE_ICD_CIP +#include // nogncheck +#include // nogncheck +#endif // CHIP_CONFIG_ENABLE_ICD_CIP namespace chip { namespace Crypto { From 83b41e54c2f966f29c6da600ac86928af5047177 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Thu, 25 Jul 2024 14:07:12 -0400 Subject: [PATCH 05/10] Fix switch case and add test --- src/app/icd/server/ICDManager.cpp | 1 + src/python_testing/TC_ICDManagementCluster.py | 10 ++++++++++ 2 files changed, 11 insertions(+) diff --git a/src/app/icd/server/ICDManager.cpp b/src/app/icd/server/ICDManager.cpp index 07632a059b2f1c..278ea34c87783a 100644 --- a/src/app/icd/server/ICDManager.cpp +++ b/src/app/icd/server/ICDManager.cpp @@ -695,6 +695,7 @@ CHIP_ERROR ICDManager::HandleEventTrigger(uint64_t eventTrigger) break; case ICDTestEventTriggerEvent::kForceMaximumCheckInBackOffState: err = mICDCheckInBackOffStrategy->ForceMaximumCheckInBackoff(); + break; #endif // CHIP_CONFIG_ENABLE_ICD_CIP default: err = CHIP_ERROR_INVALID_ARGUMENT; diff --git a/src/python_testing/TC_ICDManagementCluster.py b/src/python_testing/TC_ICDManagementCluster.py index 6030830cacded2..9f54e9b7dc227a 100644 --- a/src/python_testing/TC_ICDManagementCluster.py +++ b/src/python_testing/TC_ICDManagementCluster.py @@ -46,6 +46,7 @@ class ICDTestEventTriggerOperations(IntEnum): kRemoveActiveModeReq = 0x0046000000000002 kInvalidateHalfCounterValues = 0x0046000000000003 kInvalidateAllCounterValues = 0x0046000000000004 + kForceMaximumCheckInBackOffState = 0x0046000000000005 class TestICDManagementCluster(MatterBaseTest): @@ -113,6 +114,15 @@ async def test_active_mode_test_event_trigger(self): ) ) + asserts.assert_is_none( + await dev_ctrl.SendCommand( + self.dut_node_id, + endpoint=kRootEndpointId, + payload=Clusters.GeneralDiagnostics.Commands.TestEventTrigger(enableKey=kTestEventTriggerKey, + eventTrigger=ICDTestEventTriggerOperations.kForceMaximumCheckInBackOffState) + ) + ) + if __name__ == "__main__": default_matter_test_main() From ff9e5bbf6c983fbee6c6d7e768fa464a07b5ccba Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Thu, 25 Jul 2024 15:24:24 -0400 Subject: [PATCH 06/10] Update zcl with extensions --- src/app/zap-templates/zcl/zcl-with-test-extensions.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/app/zap-templates/zcl/zcl-with-test-extensions.json b/src/app/zap-templates/zcl/zcl-with-test-extensions.json index 1a76205be885f9..d829ebaf9d5b54 100644 --- a/src/app/zap-templates/zcl/zcl-with-test-extensions.json +++ b/src/app/zap-templates/zcl/zcl-with-test-extensions.json @@ -278,7 +278,8 @@ "ActiveModeThreshold", "RegisteredClients", "ICDCounter", - "ClientsSupportedPerFabric" + "ClientsSupportedPerFabric", + "MaximumCheckInBackOff" ], "Occupancy Sensing": ["HoldTimeLimits"], "Operational Credentials": [ From ebca97f9f8c0e096ddff42bca6b6ae8ce21d9bf1 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Thu, 25 Jul 2024 15:25:53 -0400 Subject: [PATCH 07/10] Fix zap generation --- .../zap-generated/attributes/Accessors.cpp | 46 ------------------- .../zap-generated/attributes/Accessors.h | 6 --- 2 files changed, 52 deletions(-) diff --git a/zzz_generated/app-common/app-common/zap-generated/attributes/Accessors.cpp b/zzz_generated/app-common/app-common/zap-generated/attributes/Accessors.cpp index ca35f4d8464e3c..0e519739bea690 100644 --- a/zzz_generated/app-common/app-common/zap-generated/attributes/Accessors.cpp +++ b/zzz_generated/app-common/app-common/zap-generated/attributes/Accessors.cpp @@ -9397,52 +9397,6 @@ Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, chip::app::Cl } // namespace OperatingMode -namespace MaximumCheckInBackOff { - -Protocols::InteractionModel::Status Get(chip::EndpointId endpoint, uint32_t * value) -{ - using Traits = NumericAttributeTraits; - Traits::StorageType temp; - uint8_t * readable = Traits::ToAttributeStoreRepresentation(temp); - Protocols::InteractionModel::Status status = - emberAfReadAttribute(endpoint, Clusters::IcdManagement::Id, Id, readable, sizeof(temp)); - VerifyOrReturnError(Protocols::InteractionModel::Status::Success == status, status); - if (!Traits::CanRepresentValue(/* isNullable = */ false, temp)) - { - return Protocols::InteractionModel::Status::ConstraintError; - } - *value = Traits::StorageToWorking(temp); - return status; -} - -Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, uint32_t value, MarkAttributeDirty markDirty) -{ - using Traits = NumericAttributeTraits; - if (!Traits::CanRepresentValue(/* isNullable = */ false, value)) - { - return Protocols::InteractionModel::Status::ConstraintError; - } - Traits::StorageType storageValue; - Traits::WorkingToStorage(value, storageValue); - uint8_t * writable = Traits::ToAttributeStoreRepresentation(storageValue); - return emberAfWriteAttribute(endpoint, Clusters::IcdManagement::Id, Id, writable, ZCL_INT32U_ATTRIBUTE_TYPE, markDirty); -} - -Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, uint32_t value) -{ - using Traits = NumericAttributeTraits; - if (!Traits::CanRepresentValue(/* isNullable = */ false, value)) - { - return Protocols::InteractionModel::Status::ConstraintError; - } - Traits::StorageType storageValue; - Traits::WorkingToStorage(value, storageValue); - uint8_t * writable = Traits::ToAttributeStoreRepresentation(storageValue); - return emberAfWriteAttribute(endpoint, Clusters::IcdManagement::Id, Id, writable, ZCL_INT32U_ATTRIBUTE_TYPE); -} - -} // namespace MaximumCheckInBackOff - namespace FeatureMap { Protocols::InteractionModel::Status Get(chip::EndpointId endpoint, uint32_t * value) diff --git a/zzz_generated/app-common/app-common/zap-generated/attributes/Accessors.h b/zzz_generated/app-common/app-common/zap-generated/attributes/Accessors.h index 78334996c0e75e..3fed44373526df 100644 --- a/zzz_generated/app-common/app-common/zap-generated/attributes/Accessors.h +++ b/zzz_generated/app-common/app-common/zap-generated/attributes/Accessors.h @@ -1506,12 +1506,6 @@ Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, chip::app::Cl MarkAttributeDirty markDirty); } // namespace OperatingMode -namespace MaximumCheckInBackOff { -Protocols::InteractionModel::Status Get(chip::EndpointId endpoint, uint32_t * value); // int32u -Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, uint32_t value); -Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, uint32_t value, MarkAttributeDirty markDirty); -} // namespace MaximumCheckInBackOff - namespace FeatureMap { Protocols::InteractionModel::Status Get(chip::EndpointId endpoint, uint32_t * value); // bitmap32 Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, uint32_t value); From f1484c02b79e27580dcee08b4fab50c795d60b1b Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Fri, 26 Jul 2024 13:45:57 -0400 Subject: [PATCH 08/10] Refactor ICDManager init to leverage a builder pattern --- src/app/icd/server/ICDManager.cpp | 34 +++++--------- src/app/icd/server/ICDManager.h | 49 +++++++++++++++++++-- src/app/icd/server/tests/TestICDManager.cpp | 25 +++++++++-- src/app/server/Server.cpp | 12 ++++- 4 files changed, 89 insertions(+), 31 deletions(-) diff --git a/src/app/icd/server/ICDManager.cpp b/src/app/icd/server/ICDManager.cpp index 278ea34c87783a..2ba08990aef4b6 100644 --- a/src/app/icd/server/ICDManager.cpp +++ b/src/app/icd/server/ICDManager.cpp @@ -52,17 +52,19 @@ using chip::Protocols::InteractionModel::Status; static_assert(UINT8_MAX >= CHIP_CONFIG_MAX_EXCHANGE_CONTEXTS, "ICDManager::mOpenExchangeContextCount cannot hold count for the max exchange count"); -void ICDManager::Init(PersistentStorageDelegate * storage, FabricTable * fabricTable, Crypto::SymmetricKeystore * symmetricKeystore, - Messaging::ExchangeManager * exchangeManager, SubscriptionsInfoProvider * subInfoProvider, - ICDCheckInBackOffStrategy * strategy) +void ICDManager::Init() { #if CHIP_CONFIG_ENABLE_ICD_CIP - VerifyOrDie(storage != nullptr); - VerifyOrDie(fabricTable != nullptr); - VerifyOrDie(symmetricKeystore != nullptr); - VerifyOrDie(exchangeManager != nullptr); - VerifyOrDie(subInfoProvider != nullptr); - VerifyOrDie(strategy != nullptr); + VerifyOrDie(mStorage != nullptr); + VerifyOrDie(mFabricTable != nullptr); + VerifyOrDie(mSymmetricKeystore != nullptr); + VerifyOrDie(mExchangeManager != nullptr); + VerifyOrDie(mSubInfoProvider != nullptr); + VerifyOrDie(mICDCheckInBackOffStrategy != nullptr); + + VerifyOrDie(ICDConfigurationData::GetInstance().GetICDCounter().Init(mStorage, DefaultStorageKeyAllocator::ICDCheckInCounter(), + ICDConfigurationData::kICDCounterPersistenceIncrement) == + CHIP_NO_ERROR); #endif // CHIP_CONFIG_ENABLE_ICD_CIP #if CHIP_CONFIG_ENABLE_ICD_LIT @@ -84,19 +86,6 @@ void ICDManager::Init(PersistentStorageDelegate * storage, FabricTable * fabricT VerifyOrDie(ICDNotifier::GetInstance().Subscribe(this) == CHIP_NO_ERROR); -#if CHIP_CONFIG_ENABLE_ICD_CIP - mStorage = storage; - mFabricTable = fabricTable; - mSymmetricKeystore = symmetricKeystore; - mExchangeManager = exchangeManager; - mSubInfoProvider = subInfoProvider; - mICDCheckInBackOffStrategy = strategy; - - VerifyOrDie(ICDConfigurationData::GetInstance().GetICDCounter().Init(mStorage, DefaultStorageKeyAllocator::ICDCheckInCounter(), - ICDConfigurationData::kICDCounterPersistenceIncrement) == - CHIP_NO_ERROR); -#endif // CHIP_CONFIG_ENABLE_ICD_CIP - UpdateICDMode(); UpdateOperationState(OperationalState::IdleMode); } @@ -197,7 +186,6 @@ void ICDManager::SendCheckInMsgs() continue; } - // Validate Check-In BackOff strategy if we should send a Check-In message. if (!mICDCheckInBackOffStrategy->ShouldSendCheckInMessage(entry)) { // continue to next entry diff --git a/src/app/icd/server/ICDManager.h b/src/app/icd/server/ICDManager.h index a697071bd98dd1..4ec1dfe65231d3 100644 --- a/src/app/icd/server/ICDManager.h +++ b/src/app/icd/server/ICDManager.h @@ -115,9 +115,52 @@ class ICDManager : public ICDListener, public TestEventTriggerHandler ICDManager() = default; ~ICDManager() = default; - void Init(PersistentStorageDelegate * storage, FabricTable * fabricTable, Crypto::SymmetricKeystore * symmetricKeyStore, - Messaging::ExchangeManager * exchangeManager, SubscriptionsInfoProvider * subInfoProvider, - ICDCheckInBackOffStrategy * strategy); + /* + Builder function to set all necessary members for the ICDManager class + */ + +#if CHIP_CONFIG_ENABLE_ICD_CIP + ICDManager & SetPersistentStorageDelegate(PersistentStorageDelegate * storage) + { + mStorage = storage; + return *this; + }; + + ICDManager & SetFabricTable(FabricTable * fabricTable) + { + mFabricTable = fabricTable; + return *this; + }; + + ICDManager & SetSymmetricKeyStore(Crypto::SymmetricKeystore * symmetricKeystore) + { + mSymmetricKeystore = symmetricKeystore; + return *this; + }; + + ICDManager & SetExchangeManager(Messaging::ExchangeManager * exchangeManager) + { + mExchangeManager = exchangeManager; + return *this; + }; + + ICDManager & SetSubscriptionsInfoProvider(SubscriptionsInfoProvider * subInfoProvider) + { + mSubInfoProvider = subInfoProvider; + return *this; + }; + + ICDManager & SetICDCheckInBackOffStrategy(ICDCheckInBackOffStrategy * strategy) + { + mICDCheckInBackOffStrategy = strategy; + return *this; + }; +#endif // CHIP_CONFIG_ENABLE_ICD_CIP + + /** + * @brief Validates that the ICDManager has all the necessary members to function and initializes the class + */ + void Init(); void Shutdown(); /** diff --git a/src/app/icd/server/tests/TestICDManager.cpp b/src/app/icd/server/tests/TestICDManager.cpp index f4b5db2eb42c2a..9b5cb532e1b69c 100644 --- a/src/app/icd/server/tests/TestICDManager.cpp +++ b/src/app/icd/server/tests/TestICDManager.cpp @@ -198,7 +198,14 @@ class TestICDManager : public Test::LoopbackMessagingContext mICDStateObserver.ResetAll(); mICDManager.RegisterObserver(&mICDStateObserver); - mICDManager.Init(&testStorage, &GetFabricTable(), &mKeystore, &GetExchangeManager(), &mSubInfoProvider, &mStrategy); + + mICDManager.SetPersistentStorageDelegate(&testStorage) + .SetFabricTable(&GetFabricTable()) + .SetSymmetricKeyStore(&mKeystore) + .SetExchangeManager(&GetExchangeManager()) + .SetSubscriptionsInfoProvider(&mSubInfoProvider) + .SetICDCheckInBackOffStrategy(&mStrategy) + .Init(); } // Performs teardown for each individual test in the test suite @@ -570,7 +577,13 @@ TEST_F(TestICDManager, TestICDCounter) // Shut down and reinit ICDManager to increment counter mICDManager.Shutdown(); - mICDManager.Init(&(testStorage), &GetFabricTable(), &(mKeystore), &GetExchangeManager(), &(mSubInfoProvider), &mStrategy); + mICDManager.SetPersistentStorageDelegate(&testStorage) + .SetFabricTable(&GetFabricTable()) + .SetSymmetricKeyStore(&mKeystore) + .SetExchangeManager(&GetExchangeManager()) + .SetSubscriptionsInfoProvider(&mSubInfoProvider) + .SetICDCheckInBackOffStrategy(&mStrategy) + .Init(); mICDManager.RegisterObserver(&(mICDStateObserver)); EXPECT_EQ(counter + ICDConfigurationData::kICDCounterPersistenceIncrement, @@ -975,7 +988,13 @@ TEST_F(TestICDManager, TestICDStateObserverOnICDModeChangeOnInit) // Shut down and reinit ICDManager - We should go to LIT mode since we have a registration mICDManager.Shutdown(); mICDManager.RegisterObserver(&(mICDStateObserver)); - mICDManager.Init(&testStorage, &GetFabricTable(), &mKeystore, &GetExchangeManager(), &mSubInfoProvider, &mStrategy); + mICDManager.SetPersistentStorageDelegate(&testStorage) + .SetFabricTable(&GetFabricTable()) + .SetSymmetricKeyStore(&mKeystore) + .SetExchangeManager(&GetExchangeManager()) + .SetSubscriptionsInfoProvider(&mSubInfoProvider) + .SetICDCheckInBackOffStrategy(&mStrategy) + .Init(); // We have a registration, transition to LIT mode EXPECT_TRUE(mICDStateObserver.mOnICDModeChangeCalled); diff --git a/src/app/server/Server.cpp b/src/app/server/Server.cpp index 23d3054179edaa..33f657f7ec1030 100644 --- a/src/app/server/Server.cpp +++ b/src/app/server/Server.cpp @@ -359,8 +359,16 @@ CHIP_ERROR Server::Init(const ServerInitParams & initParams) mICDManager.RegisterObserver(mReportScheduler); mICDManager.RegisterObserver(&app::DnssdServer::Instance()); - mICDManager.Init(mDeviceStorage, &GetFabricTable(), mSessionKeystore, &mExchangeMgr, - chip::app::InteractionModelEngine::GetInstance(), initParams.icdCheckInBackOffStrategy); +#if CHIP_CONFIG_ENABLE_ICD_CIP + mICDManager.SetPersistentStorageDelegate(mDeviceStorage) + .SetFabricTable(&GetFabricTable()) + .SetSymmetricKeyStore(mSessionKeystore) + .SetExchangeManager(&mExchangeMgr) + .SetSubscriptionsInfoProvider(chip::app::InteractionModelEngine::GetInstance()) + .SetICDCheckInBackOffStrategy(initParams.icdCheckInBackOffStrategy); + +#endif // CHIP_CONFIG_ENABLE_ICD_CIP + mICDManager.Init(); // Register Test Event Trigger Handler if (mTestEventTriggerDelegate != nullptr) From ca9e27ff1db5e099b463a74691bff27e9fdd3ca0 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Fri, 26 Jul 2024 14:41:41 -0400 Subject: [PATCH 09/10] Fix non LIT test builds --- src/app/icd/server/tests/TestICDManager.cpp | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/src/app/icd/server/tests/TestICDManager.cpp b/src/app/icd/server/tests/TestICDManager.cpp index 9b5cb532e1b69c..df5c2e4970c579 100644 --- a/src/app/icd/server/tests/TestICDManager.cpp +++ b/src/app/icd/server/tests/TestICDManager.cpp @@ -199,13 +199,15 @@ class TestICDManager : public Test::LoopbackMessagingContext mICDStateObserver.ResetAll(); mICDManager.RegisterObserver(&mICDStateObserver); +#if CHIP_CONFIG_ENABLE_ICD_CIP mICDManager.SetPersistentStorageDelegate(&testStorage) .SetFabricTable(&GetFabricTable()) .SetSymmetricKeyStore(&mKeystore) .SetExchangeManager(&GetExchangeManager()) .SetSubscriptionsInfoProvider(&mSubInfoProvider) - .SetICDCheckInBackOffStrategy(&mStrategy) - .Init(); + .SetICDCheckInBackOffStrategy(&mStrategy); +#endif // CHIP_CONFIG_ENABLE_ICD_CIP + mICDManager.Init(); } // Performs teardown for each individual test in the test suite @@ -577,13 +579,16 @@ TEST_F(TestICDManager, TestICDCounter) // Shut down and reinit ICDManager to increment counter mICDManager.Shutdown(); +#if CHIP_CONFIG_ENABLE_ICD_CIP mICDManager.SetPersistentStorageDelegate(&testStorage) .SetFabricTable(&GetFabricTable()) .SetSymmetricKeyStore(&mKeystore) .SetExchangeManager(&GetExchangeManager()) .SetSubscriptionsInfoProvider(&mSubInfoProvider) - .SetICDCheckInBackOffStrategy(&mStrategy) - .Init(); + .SetICDCheckInBackOffStrategy(&mStrategy); +#endif // CHIP_CONFIG_ENABLE_ICD_CIP + mICDManager.Init(); + mICDManager.RegisterObserver(&(mICDStateObserver)); EXPECT_EQ(counter + ICDConfigurationData::kICDCounterPersistenceIncrement, @@ -988,13 +993,15 @@ TEST_F(TestICDManager, TestICDStateObserverOnICDModeChangeOnInit) // Shut down and reinit ICDManager - We should go to LIT mode since we have a registration mICDManager.Shutdown(); mICDManager.RegisterObserver(&(mICDStateObserver)); +#if CHIP_CONFIG_ENABLE_ICD_CIP mICDManager.SetPersistentStorageDelegate(&testStorage) .SetFabricTable(&GetFabricTable()) .SetSymmetricKeyStore(&mKeystore) .SetExchangeManager(&GetExchangeManager()) .SetSubscriptionsInfoProvider(&mSubInfoProvider) - .SetICDCheckInBackOffStrategy(&mStrategy) - .Init(); + .SetICDCheckInBackOffStrategy(&mStrategy); +#endif // CHIP_CONFIG_ENABLE_ICD_CIP + mICDManager.Init(); // We have a registration, transition to LIT mode EXPECT_TRUE(mICDStateObserver.mOnICDModeChangeCalled); From 4755d13cb7c6dbbbc45d2ed8e05c0771845f58ec Mon Sep 17 00:00:00 2001 From: mkardous-silabs <84793247+mkardous-silabs@users.noreply.github.com> Date: Fri, 26 Jul 2024 17:00:24 -0400 Subject: [PATCH 10/10] Apply suggestions from code review Co-authored-by: Boris Zbarsky --- src/app/icd/server/DefaultICDCheckInBackOffStrategy.h | 8 ++++---- src/app/icd/server/ICDCheckInBackOffStrategy.h | 4 ++-- src/app/server/Server.h | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/app/icd/server/DefaultICDCheckInBackOffStrategy.h b/src/app/icd/server/DefaultICDCheckInBackOffStrategy.h index 5043d15185f81d..cdf29f0f2d9f9e 100644 --- a/src/app/icd/server/DefaultICDCheckInBackOffStrategy.h +++ b/src/app/icd/server/DefaultICDCheckInBackOffStrategy.h @@ -40,13 +40,13 @@ class DefaultICDCheckInBackOffStrategy : public ICDCheckInBackOffStrategy /** * @brief Function checks if the entry is a permanent or ephemeral client. * If the client is permanent, we should send a Check-In message. - * If the cliet is ephemerakl, we should not send a Check-In message. + * If the client is ephemeral, we should not send a Check-In message. * - * @param entry Entry for which we are deciding if we need to send a Check-In message or not. + * @param entry Entry for which we are deciding whether we need to send a Check-In message or not. * @return true If the client is permanent, return true. * @return false If the client is not permanent, ephemeral or invalid, return false. */ - bool ShouldSendCheckInMessage(const ICDMonitoringEntry & entry) + bool ShouldSendCheckInMessage(const ICDMonitoringEntry & entry) override { return (entry.clientType == Clusters::IcdManagement::ClientTypeEnum::kPermanent); } @@ -56,7 +56,7 @@ class DefaultICDCheckInBackOffStrategy : public ICDCheckInBackOffStrategy * As such, we don't need to execute anything to force the maximum Check-In BackOff. * */ - CHIP_ERROR ForceMaximumCheckInBackoff() { return CHIP_NO_ERROR; } + CHIP_ERROR ForceMaximumCheckInBackoff() override { return CHIP_NO_ERROR; } }; } // namespace app diff --git a/src/app/icd/server/ICDCheckInBackOffStrategy.h b/src/app/icd/server/ICDCheckInBackOffStrategy.h index 97ca7c9a4d3418..e0a5a317cadf5d 100644 --- a/src/app/icd/server/ICDCheckInBackOffStrategy.h +++ b/src/app/icd/server/ICDCheckInBackOffStrategy.h @@ -35,14 +35,14 @@ class ICDCheckInBackOffStrategy * @brief Function is used by the ICDManager to determine if a Check-In message should be sent to the given entry based on the * Check-In BackOff strategy. * - * There are no requirements on how the Check-In BackOff strategy should behave. + * There are no requirements on how the Check-In BackOff strategy should behave. * The only specified requirement is the maximum time between to Check-In message, MaximumCheckInBackOff. * All strategies must respect this requirement. * * @param entry ICDMonitoringEntry for which we are about to send a Check-In message to. * * @return true ICDCheckInBackOffStrategy determines that we SHOULD send a Check-In message to the given entry - * @return falseI CDCheckInBackOffStrategy determines that we SHOULD NOT send a Check-In message to the given entry + * @return false ICDCheckInBackOffStrategy determines that we SHOULD NOT send a Check-In message to the given entry */ virtual bool ShouldSendCheckInMessage(const ICDMonitoringEntry & entry) = 0; diff --git a/src/app/server/Server.h b/src/app/server/Server.h index 933b3438d3488a..9e0216877fea61 100644 --- a/src/app/server/Server.h +++ b/src/app/server/Server.h @@ -169,8 +169,8 @@ struct ServerInitParams Credentials::OperationalCertificateStore * opCertStore = nullptr; // Required, if not provided, the Server::Init() WILL fail. app::reporting::ReportScheduler * reportScheduler = nullptr; - // Optionnal. Support for the ICD Check-In BackOff strategy. Must be initialized before being provided. - // If the ICD Check-In protocol use-case is supported and no strategy is prprovided, server will use the default strategy. + // Optional. Support for the ICD Check-In BackOff strategy. Must be initialized before being provided. + // If the ICD Check-In protocol use-case is supported and no strategy is provided, server will use the default strategy. app::ICDCheckInBackOffStrategy * icdCheckInBackOffStrategy = nullptr; };