[ICD]Add needed elements to the ICD Manager to handle LIT mode (#27916)

* Add needed elements to the ICD Manager to handle LIT mode * separate IcdMonitorinTable in its own sourceset to fix build issues on examples that have the ICD cluster but not not enable chip_enable_icd_server * address comments. Don't force Slow Polling interval in SIT to 15s to respect the current SHOULD conformance * fix test build
project-chip · Jul 18, 2023 · e6f610b · e6f610b
1 parent 70bb397
commit e6f610b
Show file tree

Hide file tree

Showing 13 changed files with 76 additions and 47 deletions.
diff --git a/examples/all-clusters-app/esp32/main/CMakeLists.txt b/examples/all-clusters-app/esp32/main/CMakeLists.txt
@@ -34,6 +34,7 @@ set(SRC_DIRS_LIST
                       "${CMAKE_SOURCE_DIR}/third_party/connectedhomeip/examples/platform/esp32/lock"
                       "${CMAKE_SOURCE_DIR}/third_party/connectedhomeip/examples/platform/esp32/mode-support"
                       "${CMAKE_SOURCE_DIR}/third_party/connectedhomeip/src/app/server"
+                      "${CMAKE_SOURCE_DIR}/third_party/connectedhomeip/src/app/icd"
                       "${CMAKE_SOURCE_DIR}/third_party/connectedhomeip/src/app/util"
                       "${CMAKE_SOURCE_DIR}/third_party/connectedhomeip/src/app/reporting"
                       "${CMAKE_SOURCE_DIR}/third_party/connectedhomeip/src/app/clusters/administrator-commissioning-server"

diff --git a/examples/all-clusters-minimal-app/esp32/main/CMakeLists.txt b/examples/all-clusters-minimal-app/esp32/main/CMakeLists.txt
@@ -32,6 +32,7 @@ set(SRC_DIRS_LIST
                       "${CMAKE_SOURCE_DIR}/third_party/connectedhomeip/examples/platform/esp32/shell_extension"
                       "${CMAKE_SOURCE_DIR}/third_party/connectedhomeip/examples/providers"
                       "${CMAKE_SOURCE_DIR}/third_party/connectedhomeip/src/app/server"
+                      "${CMAKE_SOURCE_DIR}/third_party/connectedhomeip/src/app/icd"
                       "${CMAKE_SOURCE_DIR}/third_party/connectedhomeip/src/app/util"
                       "${CMAKE_SOURCE_DIR}/third_party/connectedhomeip/src/app/reporting"
                       "${CMAKE_SOURCE_DIR}/third_party/connectedhomeip/src/app/clusters/administrator-commissioning-server"

diff --git a/src/app/chip_data_model.cmake b/src/app/chip_data_model.cmake
@@ -143,7 +143,7 @@ function(chip_configure_data_model APP_TARGET)
         ${CHIP_APP_BASE_DIR}/util/attribute-storage.cpp
         ${CHIP_APP_BASE_DIR}/util/attribute-table.cpp
         ${CHIP_APP_BASE_DIR}/util/binding-table.cpp
-        ${CHIP_APP_BASE_DIR}/util/IcdMonitoringTable.cpp
+        ${CHIP_APP_BASE_DIR}/icd/IcdMonitoringTable.cpp
         ${CHIP_APP_BASE_DIR}/util/DataModelHandler.cpp
         ${CHIP_APP_BASE_DIR}/util/ember-compatibility-functions.cpp
         ${CHIP_APP_BASE_DIR}/util/error-mapping.cpp

diff --git a/src/app/chip_data_model.gni b/src/app/chip_data_model.gni
@@ -165,8 +165,6 @@ template("chip_data_model") {
       "${_app_root}/clusters/scenes-server/SceneTableImpl.h",
       "${_app_root}/clusters/scenes-server/scenes-server.h",
       "${_app_root}/util/DataModelHandler.cpp",
-      "${_app_root}/util/IcdMonitoringTable.cpp",
-      "${_app_root}/util/IcdMonitoringTable.h",
       "${_app_root}/util/attribute-size-util.cpp",
       "${_app_root}/util/attribute-storage.cpp",
       "${_app_root}/util/attribute-table.cpp",
@@ -197,6 +195,10 @@ template("chip_data_model") {
                                      [ invoker.zap_file ])
     }
 
+    if (!defined(deps)) {
+      deps = []
+    }
+
     foreach(cluster, _cluster_sources) {
       if (cluster == "door-lock-server") {
         sources += [
@@ -256,6 +258,10 @@ template("chip_data_model") {
           "${_app_root}/clusters/${cluster}/${cluster}.h",
           "${_app_root}/clusters/${cluster}/operational-state-delegate.h",
         ]
+      } else if (cluster == "icd-management-server") {
+        sources += [ "${_app_root}/clusters/${cluster}/${cluster}.cpp" ]
+
+        deps += [ "${chip_root}/src/app/icd:monitoring-table" ]
       } else {
         sources += [ "${_app_root}/clusters/${cluster}/${cluster}.cpp" ]
       }

diff --git a/src/app/clusters/icd-management-server/icd-management-server.cpp b/src/app/clusters/icd-management-server/icd-management-server.cpp
@@ -26,7 +26,7 @@
 #include <app/AttributeAccessInterface.h>
 #include <app/CommandHandler.h>
 #include <app/ConcreteAttributePath.h>
-#include <app/util/IcdMonitoringTable.h>
+#include <app/icd/IcdMonitoringTable.h>
 #include <app/util/af.h>
 #include <app/util/attribute-storage.h>
 

diff --git a/src/app/icd/BUILD.gn b/src/app/icd/BUILD.gn
@@ -25,6 +25,16 @@ source_set("manager-srcs") {
     "ICDManager.h",
   ]
 
+  deps = [ ":monitoring-table" ]
+  public_deps = [ "${chip_root}/src/credentials:credentials" ]
+}
+
+source_set("monitoring-table") {
+  sources = [
+    "IcdMonitoringTable.cpp",
+    "IcdMonitoringTable.h",
+  ]
+
   public_deps = [
     "${chip_root}/src/lib/core",
     "${chip_root}/src/platform:platform",

diff --git a/src/app/icd/ICDManager.cpp b/src/app/icd/ICDManager.cpp
@@ -19,11 +19,13 @@
 #include <app-common/zap-generated/ids/Attributes.h>
 #include <app-common/zap-generated/ids/Clusters.h>
 #include <app/icd/ICDManager.h>
+#include <app/icd/IcdMonitoringTable.h>
 #include <lib/support/CodeUtils.h>
 #include <lib/support/logging/CHIPLogging.h>
 #include <platform/ConnectivityManager.h>
 #include <platform/LockTracker.h>
 #include <platform/internal/CHIPDeviceLayerInternal.h>
+#include <stdlib.h>
 
 namespace chip {
 namespace app {
@@ -32,8 +34,13 @@ using namespace chip::app;
 using namespace chip::app::Clusters;
 using namespace chip::app::Clusters::IcdManagement;
 
-void ICDManager::ICDManager::Init()
+void ICDManager::ICDManager::Init(PersistentStorageDelegate * storage, FabricTable * fabricTable)
 {
+    VerifyOrDie(storage != nullptr);
+    VerifyOrDie(fabricTable != nullptr);
+    mStorage     = storage;
+    mFabricTable = fabricTable;
+
     uint32_t activeModeInterval;
     if (Attributes::ActiveModeInterval::Get(kRootEndpointId, &activeModeInterval) != EMBER_ZCL_STATUS_SUCCESS)
     {
@@ -49,16 +56,17 @@ void ICDManager::ICDManager::Shutdown()
     // cancel any running timer of the icd
     DeviceLayer::SystemLayer().CancelTimer(OnIdleModeDone, this);
     DeviceLayer::SystemLayer().CancelTimer(OnActiveModeDone, this);
-    mIcdMode          = ICDMode::SIT;
+    mICDMode          = ICDMode::SIT;
     mOperationalState = OperationalState::IdleMode;
+    mStorage          = nullptr;
+    mFabricTable      = nullptr;
 }
 
 bool ICDManager::SupportsCheckInProtocol()
 {
     bool success;
     uint32_t featureMap;
     success = (Attributes::FeatureMap::Get(kRootEndpointId, &featureMap) == EMBER_ZCL_STATUS_SUCCESS);
-
     return success ? ((featureMap & to_underlying(Feature::kCheckInProtocolSupport)) != 0) : false;
 }
 
@@ -68,25 +76,25 @@ void ICDManager::UpdateIcdMode()
 
     ICDMode tempMode = ICDMode::SIT;
 
-    // TODO ICD LIT FIX DEPENDENCY ISSUE with app/util/IcdMonitoringTable.h and app/server:server
     // 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 (kSlowPollingInterval > kICDSitModePollingThreashold && SupportsCheckInProtocol())
-    // {
-    //     // We can only get to LIT Mode, if at least one client is registered to the ICD device
-    //     const auto & fabricTable = Server::GetInstance().GetFabricTable();
-    //     for (const auto & fabricInfo : fabricTable)
-    //     {
-    //         PersistentStorageDelegate & storage = Server::GetInstance().GetPersistentStorage();
-    //         IcdMonitoringTable table(storage, fabricInfo.GetFabricIndex(), 1);
-    //         if (!table.IsEmpty())
-    //         {
-    //             tempMode = ICDMode::LIT;
-    //             break;
-    //         }
-    //     }
-    // }
-    mIcdMode = tempMode;
+    if (GetSlowPollingInterval() > GetSITPollingThreshold() && SupportsCheckInProtocol())
+    {
+        VerifyOrDie(mStorage != nullptr);
+        VerifyOrDie(mFabricTable != nullptr);
+        // We can only get to LIT Mode, if at least one client is registered with the ICD device
+        for (const auto & fabricInfo : *mFabricTable)
+        {
+            // We only need 1 valid entry to ensure LIT compliance
+            IcdMonitoringTable table(*mStorage, fabricInfo.GetFabricIndex(), 1 /*Table entry limit*/);
+            if (!table.IsEmpty())
+            {
+                tempMode = ICDMode::LIT;
+                break;
+            }
+        }
+    }
+    mICDMode = tempMode;
 }
 
 void ICDManager::UpdateOperationState(OperationalState state)
@@ -105,7 +113,17 @@ void ICDManager::UpdateOperationState(OperationalState state)
         }
         DeviceLayer::SystemLayer().StartTimer(System::Clock::Timeout(idleModeInterval), OnIdleModeDone, this);
 
-        CHIP_ERROR err = DeviceLayer::ConnectivityMgr().SetPollingInterval(GetSlowPollingInterval());
+        System::Clock::Milliseconds32 slowPollInterval = GetSlowPollingInterval();
+        // When in SIT mode, the slow poll interval SHOULDN'T be greater than the SIT mode polling threshold, per spec.
+        if (mICDMode == ICDMode::SIT && slowPollInterval > GetSITPollingThreshold())
+        {
+            ChipLogDetail(AppServer, "The Slow Polling Interval of an ICD in SIT mode should be <= %" PRIu32,
+                          (GetSITPollingThreshold().count() / 1000));
+            // TODO Spec to define this conformance as a SHALL
+            // slowPollInterval = GetSITPollingThreshold();
+        }
+
+        CHIP_ERROR err = DeviceLayer::ConnectivityMgr().SetPollingInterval(slowPollInterval);
         if (err != CHIP_NO_ERROR)
         {
             ChipLogError(AppServer, "Failed to set Polling Interval: err %" CHIP_ERROR_FORMAT, err.Format());

diff --git a/src/app/icd/ICDManager.h b/src/app/icd/ICDManager.h
@@ -16,6 +16,7 @@
  */
 #pragma once
 
+#include <credentials/FabricTable.h>
 #include <lib/support/BitFlags.h>
 #include <platform/CHIPDeviceConfig.h>
 #include <platform/internal/CHIPDeviceLayerInternal.h>
@@ -50,15 +51,16 @@ class ICDManager
     };
 
     ICDManager() {}
-    void Init();
+    void Init(PersistentStorageDelegate * storage, FabricTable * fabricTable);
     void Shutdown();
     void UpdateIcdMode();
     void UpdateOperationState(OperationalState state);
     void SetKeepActiveModeRequirements(KeepActiveFlags flag, bool state);
     bool IsKeepActive() { return mKeepActiveFlags.HasAny(); }
-    ICDMode GetIcdMode() { return mIcdMode; }
+    ICDMode GetICDMode() { return mICDMode; }
     OperationalState GetOperationalState() { return mOperationalState; }
 
+    static System::Clock::Milliseconds32 GetSITPollingThreshold() { return kSITPollingThreshold; }
     static System::Clock::Milliseconds32 GetSlowPollingInterval() { return kSlowPollingInterval; }
     static System::Clock::Milliseconds32 GetFastPollingInterval() { return kFastPollingInterval; }
 
@@ -67,9 +69,10 @@ class ICDManager
     static void OnActiveModeDone(System::Layer * aLayer, void * appState);
 
 private:
-    static constexpr System::Clock::Milliseconds32 kICDSitModePollingThreashold = System::Clock::Milliseconds32(15000);
-    static constexpr System::Clock::Milliseconds32 kSlowPollingInterval         = CHIP_DEVICE_CONFIG_ICD_SLOW_POLL_INTERVAL;
-    static constexpr System::Clock::Milliseconds32 kFastPollingInterval         = CHIP_DEVICE_CONFIG_ICD_FAST_POLL_INTERVAL;
+    // 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);
+    static constexpr System::Clock::Milliseconds32 kSlowPollingInterval = CHIP_DEVICE_CONFIG_ICD_SLOW_POLL_INTERVAL;
+    static constexpr System::Clock::Milliseconds32 kFastPollingInterval = CHIP_DEVICE_CONFIG_ICD_FAST_POLL_INTERVAL;
 
     // Minimal constraint value of the the ICD attributes.
     static constexpr uint32_t kMinIdleModeInterval    = 500;
@@ -79,8 +82,10 @@ class ICDManager
     bool SupportsCheckInProtocol();
 
     BitFlags<KeepActiveFlags> mKeepActiveFlags{ 0 };
-    OperationalState mOperationalState = OperationalState::IdleMode;
-    ICDMode mIcdMode                   = ICDMode::SIT;
+    OperationalState mOperationalState   = OperationalState::IdleMode;
+    ICDMode mICDMode                     = ICDMode::SIT;
+    PersistentStorageDelegate * mStorage = nullptr;
+    FabricTable * mFabricTable           = nullptr;
 };
 
 } // namespace app

diff --git a/src/app/util/IcdMonitoringTable.cpp → src/app/icd/IcdMonitoringTable.cpp b/src/app/util/IcdMonitoringTable.cpp → src/app/icd/IcdMonitoringTable.cpp
diff --git a/src/app/util/IcdMonitoringTable.h → src/app/icd/IcdMonitoringTable.h b/src/app/util/IcdMonitoringTable.h → src/app/icd/IcdMonitoringTable.h
diff --git a/src/app/server/Server.cpp b/src/app/server/Server.cpp
@@ -249,7 +249,7 @@ CHIP_ERROR Server::Init(const ServerInitParams & initParams)
 #endif // CHIP_CONFIG_ENABLE_SERVER_IM_EVENT
 
 #if CHIP_CONFIG_ENABLE_ICD_SERVER
-    mICDManager.Init();
+    mICDManager.Init(mDeviceStorage, &GetFabricTable());
     mICDEventManager.Init(&mICDManager);
 #endif // CHIP_CONFIG_ENABLE_ICD_SERVER
 

diff --git a/src/app/tests/BUILD.gn b/src/app/tests/BUILD.gn
@@ -54,18 +54,6 @@ source_set("binding-test-srcs") {
   ]
 }
 
-source_set("icd-management-test-srcs") {
-  sources = [
-    "${chip_root}/src/app/util/IcdMonitoringTable.cpp",
-    "${chip_root}/src/app/util/IcdMonitoringTable.h",
-  ]
-
-  public_deps = [
-    "${chip_root}/src/app/common:cluster-objects",
-    "${chip_root}/src/lib/core",
-  ]
-}
-
 source_set("ota-requestor-test-srcs") {
   sources = [
     "${chip_root}/src/app/clusters/ota-requestor/DefaultOTARequestorStorage.cpp",
@@ -182,14 +170,14 @@ chip_test_suite("tests") {
 
   public_deps = [
     ":binding-test-srcs",
-    ":icd-management-test-srcs",
     ":operational-state-test-srcs",
     ":ota-requestor-test-srcs",
     ":scenes-table-test-srcs",
     ":time-sync-data-provider-test-srcs",
     "${chip_root}/src/app",
     "${chip_root}/src/app/common:cluster-objects",
     "${chip_root}/src/app/icd:manager-srcs",
+    "${chip_root}/src/app/icd:monitoring-table",
     "${chip_root}/src/app/tests:helpers",
     "${chip_root}/src/app/util/mock:mock_ember",
     "${chip_root}/src/lib/core",

diff --git a/src/app/tests/TestIcdMonitoringTable.cpp b/src/app/tests/TestIcdMonitoringTable.cpp
@@ -15,7 +15,7 @@
  *    limitations under the License.
  */
 
-#include <app/util/IcdMonitoringTable.h>
+#include <app/icd/IcdMonitoringTable.h>
 #include <lib/core/CHIPError.h>
 #include <lib/support/DefaultStorageKeyAllocator.h>
 #include <lib/support/TestPersistentStorageDelegate.h>