[ICD] Refactor ICDCounter logic (project-chip#31957)

* Refactor ICD Check-In counter
Fix initial Check-In counter value

* Add basic integration tests to validate behavior

* Restyled by whitespace

* Restyled by prettier-yaml

* rename source_set

* Rename constant name

* Apply suggestions from code review

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

* addres review comments

* Add init/shutdown unit-test for the ICDManager

* Fix Build.gn - multiple inclusion of CheckInMessage.cpp

---------

Co-authored-by: Restyled.io <commits@restyled.io>
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

Author: 3 people

src/app/clusters/icd-management-server/icd-management-server.cpp

@@ -149,7 +149,7 @@ CHIP_ERROR IcdManagementAttributeAccess::ReadRegisteredClients(EndpointId endpoi
 
 CHIP_ERROR IcdManagementAttributeAccess::ReadICDCounter(EndpointId endpoint, AttributeValueEncoder & encoder)
 {
-    return encoder.Encode(mICDConfigurationData->GetICDCounter());
+    return encoder.Encode(mICDConfigurationData->GetICDCounter().GetValue());
 }
 
 CHIP_ERROR IcdManagementAttributeAccess::ReadClientsSupportedPerFabric(EndpointId endpoint, AttributeValueEncoder & encoder)
@@ -292,7 +292,7 @@ Status ICDManagementServer::RegisterClient(CommandHandler * commandObj, const Co
         TriggerICDMTableUpdatedEvent();
     }
 
-    icdCounter = mICDConfigurationData->GetICDCounter();
+    icdCounter = mICDConfigurationData->GetICDCounter().GetValue();
     return InteractionModel::Status::Success;
 }
 

src/app/icd/server/BUILD.gn

@@ -115,5 +115,6 @@ source_set("configuration-data") {
     ":icd-server-config",
     "${chip_root}/src/lib/core",
     "${chip_root}/src/lib/support",
+    "${chip_root}/src/protocols/secure_channel:check-in-counter",
   ]
 }

src/app/icd/server/ICDConfigurationData.h

@@ -20,6 +20,7 @@
 #include <lib/core/Optional.h>
 #include <lib/support/TimeUtils.h>
 #include <platform/CHIPDeviceConfig.h>
+#include <protocols/secure_channel/CheckInCounter.h>
 #include <system/SystemClock.h>
 
 namespace chip {
@@ -44,7 +45,7 @@ class TestICDManager;
 class ICDConfigurationData
 {
 public:
-    static constexpr uint32_t ICD_CHECK_IN_COUNTER_MIN_INCREMENT = 100;
+    static constexpr uint32_t kICDCounterPersistenceIncrement = 100;
 
     enum class ICDMode : uint8_t
     {
@@ -60,7 +61,7 @@ class ICDConfigurationData
 
     System::Clock::Milliseconds16 GetActiveModeThreshold() { return mActiveThreshold; }
 
-    uint32_t GetICDCounter() { return mICDCounter; }
+    Protocols::SecureChannel::CheckInCounter & GetICDCounter() { return mICDCounter; }
 
     uint16_t GetClientsSupportedPerFabric() { return mFabricClientsSupported; }
 
@@ -99,7 +100,6 @@ class ICDConfigurationData
     friend class chip::app::TestICDManager;
 
     void SetICDMode(ICDMode mode) { mICDMode = mode; };
-    void SetICDCounter(uint32_t count) { mICDCounter = count; }
     void SetSlowPollingInterval(System::Clock::Milliseconds32 slowPollInterval) { mSlowPollingInterval = slowPollInterval; };
     void SetFastPollingInterval(System::Clock::Milliseconds32 fastPollInterval) { mFastPollingInterval = fastPollInterval; };
 
@@ -137,7 +137,7 @@ class ICDConfigurationData
 
     System::Clock::Milliseconds16 mActiveThreshold = System::Clock::Milliseconds16(CHIP_CONFIG_ICD_ACTIVE_MODE_THRESHOLD_MS);
 
-    uint32_t mICDCounter = 0;
+    Protocols::SecureChannel::CheckInCounter mICDCounter;
 
     static_assert((CHIP_CONFIG_ICD_CLIENTS_SUPPORTED_PER_FABRIC) >= 1,
                   "Spec requires the minimum of supported clients per fabric be equal or greater to 1.");

src/app/icd/server/ICDManager.cpp

@@ -72,7 +72,9 @@ void ICDManager::Init(PersistentStorageDelegate * storage, FabricTable * fabricT
     mExchangeManager   = exchangeManager;
     mSubManager        = manager;
 
-    VerifyOrDie(InitCounter() == CHIP_NO_ERROR);
+    VerifyOrDie(ICDConfigurationData::GetInstance().GetICDCounter().Init(mStorage, DefaultStorageKeyAllocator::ICDCheckInCounter(),
+                                                                         ICDConfigurationData::kICDCounterPersistenceIncrement) ==
+                CHIP_NO_ERROR);
 
     UpdateICDMode();
     UpdateOperationState(OperationalState::IdleMode);
@@ -115,7 +117,8 @@ void ICDManager::SendCheckInMsgs()
 #if !CONFIG_BUILD_FOR_HOST_UNIT_TEST
     VerifyOrDie(mStorage != nullptr);
     VerifyOrDie(mFabricTable != nullptr);
-    uint32_t counter        = ICDConfigurationData::GetInstance().GetICDCounter();
+
+    uint32_t counterValue   = ICDConfigurationData::GetInstance().GetICDCounter().GetNextCheckInCounterValue();
     bool counterIncremented = false;
 
     for (const auto & fabricInfo : *mFabricTable)
@@ -156,7 +159,7 @@ void ICDManager::SendCheckInMsgs()
             {
                 counterIncremented = true;
 
-                if (CHIP_NO_ERROR != IncrementCounter())
+                if (CHIP_NO_ERROR != ICDConfigurationData::GetInstance().GetICDCounter().Advance())
                 {
                     ChipLogError(AppServer, "Incremented ICDCounter but failed to access/save to Persistent storage");
                 }
@@ -167,7 +170,7 @@ void ICDManager::SendCheckInMsgs()
             ICDCheckInSender * sender = mICDSenderPool.CreateObject(mExchangeManager);
             VerifyOrReturn(sender != nullptr, ChipLogError(AppServer, "Failed to allocate ICDCheckinSender"));
 
-            if (CHIP_NO_ERROR != sender->RequestResolve(entry, mFabricTable, counter))
+            if (CHIP_NO_ERROR != sender->RequestResolve(entry, mFabricTable, counterValue))
             {
                 ChipLogError(AppServer, "Failed to send ICD Check-In");
             }
@@ -176,54 +179,6 @@ void ICDManager::SendCheckInMsgs()
 #endif // CONFIG_BUILD_FOR_HOST_UNIT_TEST
 }
 
-CHIP_ERROR ICDManager::InitCounter()
-{
-    CHIP_ERROR err;
-    uint32_t temp;
-    uint16_t size = static_cast<uint16_t>(sizeof(uint32_t));
-
-    err = mStorage->SyncGetKeyValue(DefaultStorageKeyAllocator::ICDCheckInCounter().KeyName(), &temp, size);
-    if (err == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND)
-    {
-        // First time retrieving the counter
-        temp = chip::Crypto::GetRandU32();
-    }
-    else if (err != CHIP_NO_ERROR)
-    {
-        return err;
-    }
-
-    ICDConfigurationData::GetInstance().SetICDCounter(temp);
-    temp += ICDConfigurationData::ICD_CHECK_IN_COUNTER_MIN_INCREMENT;
-
-    // Increment the count directly to minimize flash write.
-    return mStorage->SyncSetKeyValue(DefaultStorageKeyAllocator::ICDCheckInCounter().KeyName(), &temp, size);
-}
-
-CHIP_ERROR ICDManager::IncrementCounter()
-{
-    uint32_t temp      = 0;
-    StorageKeyName key = DefaultStorageKeyAllocator::ICDCheckInCounter();
-    uint16_t size      = static_cast<uint16_t>(sizeof(uint32_t));
-
-    ICDConfigurationData::GetInstance().mICDCounter++;
-
-    if (mStorage == nullptr)
-    {
-        return CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND;
-    }
-
-    ReturnErrorOnFailure(mStorage->SyncGetKeyValue(key.KeyName(), &temp, size));
-
-    if (temp == ICDConfigurationData::GetInstance().mICDCounter)
-    {
-        temp = ICDConfigurationData::GetInstance().mICDCounter + ICDConfigurationData::ICD_CHECK_IN_COUNTER_MIN_INCREMENT;
-        return mStorage->SyncSetKeyValue(key.KeyName(), &temp, size);
-    }
-
-    return CHIP_NO_ERROR;
-}
-
 void ICDManager::UpdateICDMode()
 {
     assertChipStackLockedByCurrentThread();

src/app/icd/server/ICDManager.h

@@ -122,10 +122,6 @@ class ICDManager : public ICDListener
      */
     static void OnTransitionToIdle(System::Layer * aLayer, void * appState);
 
-    // ICD Counter
-    CHIP_ERROR IncrementCounter();
-    CHIP_ERROR InitCounter();
-
     uint8_t mOpenExchangeContextCount = 0;
     uint8_t mCheckInRequestCount      = 0;
 

src/app/tests/TestICDManager.cpp

@@ -27,6 +27,7 @@
 #include <lib/support/TestPersistentStorageDelegate.h>
 #include <lib/support/TimeUtils.h>
 #include <lib/support/UnitTestContext.h>
+#include <lib/support/UnitTestExtendedAssertions.h>
 #include <lib/support/UnitTestRegistration.h>
 #include <nlunit-test.h>
 #include <system/SystemLayerImpl.h>
@@ -113,7 +114,7 @@ class TestContext : public chip::Test::AppContext
     CHIP_ERROR SetUp() override
     {
         ReturnErrorOnFailure(chip::Test::AppContext::SetUp());
-        mICDManager.Init(&testStorage, &GetFabricTable(), &mKeystore, &GetExchangeManager(), &mSubManager);
+        mICDManager.Init(&testStorage, &GetFabricTable(), &mKeystore, &GetExchangeManager(), &mSubInfoProvider);
         mICDManager.RegisterObserver(&mICDStateObserver);
         return CHIP_NO_ERROR;
     }
@@ -128,12 +129,12 @@ class TestContext : public chip::Test::AppContext
     System::Clock::Internal::MockClock mMockClock;
     TestSessionKeystoreImpl mKeystore;
     app::ICDManager mICDManager;
-    TestSubscriptionsInfoProvider mSubManager;
+    TestSubscriptionsInfoProvider mSubInfoProvider;
     TestPersistentStorageDelegate testStorage;
+    TestICDStateObserver mICDStateObserver;
 
 private:
     System::Clock::ClockBase * mRealClock;
-    TestICDStateObserver mICDStateObserver;
 };
 
 } // namespace
@@ -196,7 +197,7 @@ class TestICDManager
         ctx->mICDManager.SetTestFeatureMapValue(0x07);
 
         // Set that there are no matching subscriptions
-        ctx->mSubManager.SetReturnValue(false);
+        ctx->mSubInfoProvider.SetReturnValue(false);
 
         // Set New durations for test case
         Milliseconds32 oldActiveModeDuration = icdConfigData.GetActiveModeDuration();
@@ -275,7 +276,7 @@ class TestICDManager
         ctx->mICDManager.SetTestFeatureMapValue(0x07);
 
         // Set that there are not matching subscriptions
-        ctx->mSubManager.SetReturnValue(true);
+        ctx->mSubInfoProvider.SetReturnValue(true);
 
         // Set New durations for test case
         Milliseconds32 oldActiveModeDuration = icdConfigData.GetActiveModeDuration();
@@ -506,10 +507,16 @@ class TestICDManager
     static void TestICDCounter(nlTestSuite * aSuite, void * aContext)
     {
         TestContext * ctx = static_cast<TestContext *>(aContext);
-        uint32_t counter  = ICDConfigurationData::GetInstance().GetICDCounter();
-        ctx->mICDManager.IncrementCounter();
-        uint32_t counter2 = ICDConfigurationData::GetInstance().GetICDCounter();
-        NL_TEST_ASSERT(aSuite, (counter + 1) == counter2);
+        uint32_t counter  = ICDConfigurationData::GetInstance().GetICDCounter().GetValue();
+
+        // Shut down and reinit ICDManager to increment counter
+        ctx->mICDManager.Shutdown();
+        ctx->mICDManager.Init(&(ctx->testStorage), &(ctx->GetFabricTable()), &(ctx->mKeystore), &(ctx->GetExchangeManager()),
+                              &(ctx->mSubInfoProvider));
+        ctx->mICDManager.RegisterObserver(&(ctx->mICDStateObserver));
+
+        NL_TEST_ASSERT_EQUALS(aSuite, counter + ICDConfigurationData::kICDCounterPersistenceIncrement,
+                              ICDConfigurationData::GetInstance().GetICDCounter().GetValue());
     }
 
     static void TestOnSubscriptionReport(nlTestSuite * aSuite, void * aContext)

src/app/tests/suites/TestIcdManagementCluster.yaml

@@ -18,6 +18,9 @@ config:
     nodeId: 0x12344321
     cluster: "ICD Management"
     endpoint: 0
+    beforeRebootICDCounter:
+        type: int32u
+        defaultValue: 0
 
 tests:
     - label: "Read the commissioner node ID"
@@ -36,6 +39,26 @@ tests:
               - name: "nodeId"
                 value: nodeId
 
+    # chip-tool will register itself with the ICD during the tests.
+    - label: "Read RegisteredClients For Registration During Commissioning"
+      command: "readAttribute"
+      attribute: "RegisteredClients"
+      response:
+          value:
+              [
+                  {
+                      CheckInNodeID: commissionerNodeId,
+                      MonitoredSubject: commissionerNodeId,
+                  },
+              ]
+
+    - label: "Unregister Client Registered During Commissioning"
+      command: "UnregisterClient"
+      arguments:
+          values:
+              - name: "CheckInNodeID"
+                value: commissionerNodeId
+
     - label: "Read Feature Map"
       command: "readAttribute"
       attribute: "FeatureMap"
@@ -68,6 +91,25 @@ tests:
               type: int32u
               minValue: 0x0
               maxValue: 0xFFFFFFFF
+          saveAs: beforeRebootICDCounter
+
+    - label: "Reboot target device"
+      cluster: "SystemCommands"
+      command: "Reboot"
+
+    - label: "Connect to the device again"
+      cluster: "DelayCommands"
+      command: "WaitForCommissionee"
+      arguments:
+          values:
+              - name: "nodeId"
+                value: nodeId
+
+    - label: "Read ICDCounter after reboot"
+      command: "readAttribute"
+      attribute: "ICDCounter"
+      response:
+          value: beforeRebootICDCounter + 100
 
     - label: "Read UserActiveModeTriggerHint"
       command: "readAttribute"
@@ -119,26 +161,6 @@ tests:
       response:
           error: NOT_FOUND
 
-    # chip-tool will register itself with the ICD during the tests.
-    - label: "Read RegisteredClients For Registration During Commissioning"
-      command: "readAttribute"
-      attribute: "RegisteredClients"
-      response:
-          value:
-              [
-                  {
-                      CheckInNodeID: commissionerNodeId,
-                      MonitoredSubject: commissionerNodeId,
-                  },
-              ]
-
-    - label: "Unregister Client Registered During Commissioning"
-      command: "UnregisterClient"
-      arguments:
-          values:
-              - name: "CheckInNodeID"
-                value: commissionerNodeId
-
     - label: "Register 1.0 (key too short)"
       command: "RegisterClient"
       arguments:

src/lib/support/PersistedCounter.h

@@ -92,13 +92,12 @@ class PersistedCounter : public MonotonicallyIncreasingCounter<T>
         ReturnErrorOnFailure(ReadStartValue(startValue));
 
 #if CHIP_CONFIG_PERSISTED_COUNTER_DEBUG_LOGGING
-        // Compiler should optimize these branches.
-        if (is_same_v<decltype(T), uint64_t>)
+        if constexpr (std::is_same_v<decltype(startValue), uint64_t>)
         {
             ChipLogDetail(EventLogging, "PersistedCounter::Init() aEpoch 0x" ChipLogFormatX64 " startValue 0x" ChipLogFormatX64,
                           ChipLogValueX64(aEpoch), ChipLogValueX64(startValue));
         }
-        else if (is_same_v<decltype(T), uint32_t>)
+        else if (std::is_same_v<decltype(startValue), uint32_t>)
         {
             ChipLogDetail(EventLogging, "PersistedCounter::Init() aEpoch 0x%" PRIx32 " startValue 0x%" PRIx32,
                           static_cast<uint32_t>(aEpoch), static_cast<uint32_t>(startValue));
@@ -151,8 +150,7 @@ class PersistedCounter : public MonotonicallyIncreasingCounter<T>
     {
         mNextEpoch = aStartValue;
 #if CHIP_CONFIG_PERSISTED_COUNTER_DEBUG_LOGGING
-        // Compiler should optimize these branches.
-        if (is_same_v<decltype(T), uint64_t>)
+        if constexpr (std::is_same_v<decltype(aStartValue), uint64_t>)
         {
             ChipLogDetail(EventLogging, "PersistedCounter::WriteStartValue() aStartValue 0x" ChipLogFormatX64,
                           ChipLogValueX64(aStartValue));
@@ -178,7 +176,7 @@ class PersistedCounter : public MonotonicallyIncreasingCounter<T>
      */
     CHIP_ERROR ReadStartValue(T & aStartValue)
     {
-        T valueLE     = 0;
+        T valueLE     = GetInitialCounterValue();
         uint16_t size = sizeof(valueLE);
 
         VerifyOrReturnError(mKey.IsInitialized(), CHIP_ERROR_INCORRECT_STATE);
@@ -206,12 +204,28 @@ class PersistedCounter : public MonotonicallyIncreasingCounter<T>
         aStartValue = Encoding::LittleEndian::HostSwap<T>(valueLE);
 
 #if CHIP_CONFIG_PERSISTED_COUNTER_DEBUG_LOGGING
-        ChipLogDetail(EventLogging, "PersistedCounter::ReadStartValue() aStartValue 0x%x", aStartValue);
+        if constexpr (std::is_same_v<decltype(aStartValue), uint64_t>)
+        {
+            ChipLogDetail(EventLogging, "PersistedCounter::ReadStartValue() aStartValue 0x" ChipLogFormatX64,
+                          ChipLogValueX64(aStartValue));
+        }
+        else
+        {
+            ChipLogDetail(EventLogging, "PersistedCounter::ReadStartValue() aStartValue 0x%" PRIx32,
+                          static_cast<uint32_t>(aStartValue));
+        }
 #endif
 
         return CHIP_NO_ERROR;
     }
 
+    /**
+     * @brief Get the Initial Counter Value
+     *
+     * By default, persisted counters start off at 0.
+     */
+    virtual inline T GetInitialCounterValue() { return 0; }
+
     PersistentStorageDelegate * mStorage = nullptr; // start value is stored here
     StorageKeyName mKey;
     T mEpoch     = 0; // epoch modulus value