[ICD] Add Check-In message check when switching to idle mode (#31563)

* Add Check-In message check when switching to idle mode * Rename function to something shorter Co-authored-by: Boris Zbarsky <[email protected]> * Add unit test to validate fix * Rename last functions * add lib/support to the ICDConfigurationData dependencies * Apply suggestions from code review Co-authored-by: Boris Zbarsky <[email protected]> * Fix comment * Refactor ICDManager dependency with InteractionModelEngine to introduce mediator * Clean up refactor * Address comments * Fix comment typos Co-authored-by: Junior Martinez <[email protected]> * Use a variable instead of GetInstance * Rename SetDurations to SetModeDurations * Fix typo --------- Co-authored-by: Boris Zbarsky <[email protected]> Co-authored-by: Junior Martinez <[email protected]>
project-chip · Jan 30, 2024 · d61bfd6 · d61bfd6
1 parent 6fd32ba
commit d61bfd6
Show file tree

Hide file tree

Showing 12 changed files with 422 additions and 50 deletions.
diff --git a/src/app/BUILD.gn b/src/app/BUILD.gn
@@ -114,6 +114,12 @@ source_set("pre-encoded-value") {
   ]
 }
 
+source_set("subscription-manager") {
+  sources = [ "SubscriptionManager.h" ]
+
+  public_deps = [ "${chip_root}/src/lib/core" ]
+}
+
 source_set("message-def") {
   sources = [
     "MessageDef/ArrayBuilder.cpp",
@@ -247,6 +253,7 @@ static_library("interaction-model") {
     ":app_config",
     ":message-def",
     ":paths",
+    ":subscription-manager",
     "${chip_root}/src/app/icd:icd_config",
     "${chip_root}/src/app/icd:observer",
     "${chip_root}/src/lib/address_resolve",

diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp
@@ -330,7 +330,7 @@ void InteractionModelEngine::ShutdownMatchingSubscriptions(const Optional<Fabric
 }
 #endif // CHIP_CONFIG_ENABLE_READ_CLIENT
 
-bool InteractionModelEngine::SubjectHasActiveSubscription(const FabricIndex aFabricIndex, const NodeId & subjectID)
+bool InteractionModelEngine::SubjectHasActiveSubscription(const FabricIndex & aFabricIndex, const NodeId & subjectID)
 {
     bool isActive = false;
     mReadHandlers.ForEachActiveObject([aFabricIndex, subjectID, &isActive](ReadHandler * handler) {
@@ -367,6 +367,12 @@ bool InteractionModelEngine::SubjectHasActiveSubscription(const FabricIndex aFab
     return isActive;
 }
 
+bool InteractionModelEngine::SubjectHasPersistedSubscription(const FabricIndex & aFabricIndex, const NodeId & subject)
+{
+    // TODO(#30281) : Implement persisted sub check and verify how persistent subscriptions affects this at ICDManager::Init
+    return false;
+}
+
 void InteractionModelEngine::OnDone(CommandHandler & apCommandObj)
 {
     mCommandHandlerObjs.ReleaseObject(&apCommandObj);

diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h
@@ -27,21 +27,6 @@
 
 #include <access/AccessControl.h>
 #include <app/AppConfig.h>
-#include <app/MessageDef/AttributeReportIBs.h>
-#include <app/MessageDef/ReportDataMessage.h>
-#include <app/SubscriptionResumptionSessionEstablisher.h>
-#include <lib/core/CHIPCore.h>
-#include <lib/support/CodeUtils.h>
-#include <lib/support/DLLUtil.h>
-#include <lib/support/Pool.h>
-#include <lib/support/logging/CHIPLogging.h>
-#include <messaging/ExchangeContext.h>
-#include <messaging/ExchangeMgr.h>
-#include <messaging/Flags.h>
-#include <protocols/Protocols.h>
-#include <protocols/interaction_model/Constants.h>
-#include <system/SystemPacketBuffer.h>
-
 #include <app/AttributePathParams.h>
 #include <app/CommandHandler.h>
 #include <app/CommandHandlerInterface.h>
@@ -51,17 +36,32 @@
 #include <app/ConcreteEventPath.h>
 #include <app/DataVersionFilter.h>
 #include <app/EventPathParams.h>
+#include <app/MessageDef/AttributeReportIBs.h>
+#include <app/MessageDef/ReportDataMessage.h>
 #include <app/ObjectList.h>
 #include <app/ReadClient.h>
 #include <app/ReadHandler.h>
 #include <app/StatusResponse.h>
+#include <app/SubscriptionManager.h>
+#include <app/SubscriptionResumptionSessionEstablisher.h>
 #include <app/TimedHandler.h>
 #include <app/WriteClient.h>
 #include <app/WriteHandler.h>
 #include <app/reporting/Engine.h>
 #include <app/reporting/ReportScheduler.h>
 #include <app/util/attribute-metadata.h>
 #include <app/util/basic-types.h>
+#include <lib/core/CHIPCore.h>
+#include <lib/support/CodeUtils.h>
+#include <lib/support/DLLUtil.h>
+#include <lib/support/Pool.h>
+#include <lib/support/logging/CHIPLogging.h>
+#include <messaging/ExchangeContext.h>
+#include <messaging/ExchangeMgr.h>
+#include <messaging/Flags.h>
+#include <protocols/Protocols.h>
+#include <protocols/interaction_model/Constants.h>
+#include <system/SystemPacketBuffer.h>
 
 #include <app/CASESessionManager.h>
 
@@ -79,7 +79,8 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
                                public Messaging::ExchangeDelegate,
                                public CommandHandler::Callback,
                                public ReadHandler::ManagementCallback,
-                               public FabricTable::Delegate
+                               public FabricTable::Delegate,
+                               public SubscriptionManager
 {
 public:
     /**
@@ -323,8 +324,9 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
 
     CHIP_ERROR ResumeSubscriptions();
 
-    // Check if a given subject (CAT or NodeId) has at least 1 active subscription
-    bool SubjectHasActiveSubscription(const FabricIndex aFabricIndex, const NodeId & subject);
+    bool SubjectHasActiveSubscription(const FabricIndex & aFabricIndex, const NodeId & subject) override;
+
+    bool SubjectHasPersistedSubscription(const FabricIndex & aFabricIndex, const NodeId & subject) override;
 
 #if CONFIG_BUILD_FOR_HOST_UNIT_TEST
     //

diff --git a/src/app/SubscriptionManager.h b/src/app/SubscriptionManager.h
@@ -0,0 +1,67 @@
+/*
+ *
+ *    Copyright (c) 2024 Project CHIP Authors
+ *    All rights reserved.
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
+/**
+ *    @file
+ *      This file defines an interface that exposes all the public subscription management APIs.
+ *      The interface is implemented by the InteractionModelEngine to avoid creating unnecessary dependencies
+ *      Since the IMEgine has more dependency than its consummers need.
+ *      By leveraging the SubscriptionManager APIs, a consummer avoids depending on the global data model functions.
+ */
+
+#pragma once
+
+#include <lib/core/DataModelTypes.h>
+#include <lib/core/NodeId.h>
+
+namespace chip {
+namespace app {
+
+class SubscriptionManager
+{
+public:
+    virtual ~SubscriptionManager(){};
+
+    /**
+     * @brief Check if a given subject (CAT or operational NodeId) has at least 1 active subscription.
+     *
+     * @param[in] aFabricIndex fabric index of the subject
+     * @param[in] subject NodeId of the subect
+     *
+     * @return true subject has at least one active subscription with the device
+     *         false subject doesn't have any acitve subscription with the device
+     */
+    virtual bool SubjectHasActiveSubscription(const FabricIndex & aFabricIndex, const NodeId & subject) = 0;
+
+    /**
+     * @brief Check if a given subject (CAT or operational NodeId) has at least 1 persisted subscription.
+     *        If CHIP_CONFIG_PERSIST_SUBSCRIPTIONS is not enable, function alweays returns false.
+     *        See the CHIP_CONFIG_PERSIST_SUBSCRIPTIONS for more information on persisted subscriptions.
+     *
+     * @param[in] aFabricIndex fabric index of the subject
+     * @param[in] subject NodeId of the subect
+     *
+     * @return true subject has at least one persisted subscription with the device
+     *         false subject doesn't have any acitve subscription with the device
+     *         false If CHIP_CONFIG_PERSIST_SUBSCRIPTIONS is not enabled
+     */
+    virtual bool SubjectHasPersistedSubscription(const FabricIndex & aFabricIndex, const NodeId & subject) = 0;
+};
+
+} // namespace app
+} // namespace chip
diff --git a/src/app/icd/BUILD.gn b/src/app/icd/BUILD.gn
@@ -70,7 +70,7 @@ source_set("manager") {
     ":notifier",
     ":observer",
     ":sender",
-    "${chip_root}/src/app:interaction-model",
+    "${chip_root}/src/app:subscription-manager",
     "${chip_root}/src/credentials:credentials",
   ]
 }
@@ -114,5 +114,6 @@ source_set("configuration-data") {
   deps = [
     ":icd_config",
     "${chip_root}/src/lib/core",
+    "${chip_root}/src/lib/support",
   ]
 }
diff --git a/src/app/icd/ICDConfigurationData.cpp b/src/app/icd/ICDConfigurationData.cpp
@@ -17,6 +17,7 @@
 
 #include "ICDConfigurationData.h"
 #include <app/icd/ICDConfig.h>
+#include <lib/support/CodeUtils.h>
 
 namespace chip {
 
@@ -36,4 +37,16 @@ System::Clock::Milliseconds32 ICDConfigurationData::GetSlowPollingInterval()
     return mSlowPollingInterval;
 }
 
+CHIP_ERROR ICDConfigurationData::SetModeDurations(uint32_t activeModeDuration_ms, uint32_t idleModeInterval_s)
+{
+    VerifyOrReturnError(activeModeDuration_ms <= (idleModeInterval_s * 1000), CHIP_ERROR_INVALID_ARGUMENT);
+    VerifyOrReturnError(idleModeInterval_s <= kMaxIdleModeDuration_s, CHIP_ERROR_INVALID_ARGUMENT);
+    VerifyOrReturnError(idleModeInterval_s >= kMinIdleModeDuration_s, CHIP_ERROR_INVALID_ARGUMENT);
+
+    mIdleModeDuration_s    = idleModeInterval_s;
+    mActiveModeDuration_ms = activeModeDuration_ms;
+
+    return CHIP_NO_ERROR;
+}
+
 } // namespace chip
diff --git a/src/app/icd/ICDConfigurationData.h b/src/app/icd/ICDConfigurationData.h
@@ -25,6 +25,11 @@ namespace chip {
 namespace app {
 // Forward declaration of ICDManager to allow it to be friend with ICDConfigurationData
 class ICDManager;
+
+// Forward declaration of TestICDManager to allow it to be friend with the ICDConfigurationData
+// Used in unit tests
+class TestICDManager;
+
 } // namespace app
 
 /**
@@ -47,9 +52,9 @@ class ICDConfigurationData
 
     static ICDConfigurationData & GetInstance() { return instance; };
 
-    uint32_t GetIdleModeDurationSec() { return mIdleInterval_s; }
+    uint32_t GetIdleModeDurationSec() { return mIdleModeDuration_s; }
 
-    uint32_t GetActiveModeDurationMs() { return mActiveInterval_ms; }
+    uint32_t GetActiveModeDurationMs() { return mActiveModeDuration_ms; }
 
     uint16_t GetActiveModeThresholdMs() { return mActiveThreshold_ms; }
 
@@ -89,6 +94,7 @@ class ICDConfigurationData
     // the ICDManager, the ICDManager is a friend that can access the private setters. If a consummer needs to be notified when a
     // value is changed, they can leverage the Observer events the ICDManager generates. See src/app/icd/ICDStateObserver.h
     friend class chip::app::ICDManager;
+    friend class chip::app::TestICDManager;
 
     void SetICDMode(ICDMode mode) { mICDMode = mode; };
     void SetICDCounter(uint32_t count) { mICDCounter = count; }
@@ -97,15 +103,31 @@ class ICDConfigurationData
 
     static constexpr uint32_t kMinLitActiveModeThreshold_ms = 5000;
 
-    static_assert((CHIP_CONFIG_ICD_IDLE_MODE_DURATION_SEC) <= 64800,
+    /**
+     * @brief Change the ActiveModeDuration and IdleModeDuration value
+     *        To only change one value, pass the old value for the other one
+     *
+     * @param[in] activeModeDuration_ms new ActiveModeDuration value
+     * @param[in] idleModeDuration_s new IdleModeDuration value
+     * @return CHIP_ERROR CHIP_ERROR_INVALID_ARGUMENT is returned if idleModeDuration_s is smaller than activeModeDuration_ms
+     *                                                is returned if idleModeDuration_s is greater than 64800 seconds
+     *                                                is returned if idleModeDuration_s is smaller than 1 seconds
+     *                    CHIP_NO_ERROR is returned if the new intervals were set
+     */
+    CHIP_ERROR SetModeDurations(uint32_t activeModeDuration_ms, uint32_t idleModeDuration_s);
+
+    static constexpr uint32_t kMaxIdleModeDuration_s = 64800;
+    static constexpr uint32_t kMinIdleModeDuration_s = 1;
+
+    static_assert((CHIP_CONFIG_ICD_IDLE_MODE_DURATION_SEC) <= kMaxIdleModeDuration_s,
                   "Spec requires the IdleModeDuration to be equal or inferior to 64800s.");
-    static_assert((CHIP_CONFIG_ICD_IDLE_MODE_DURATION_SEC) >= 1,
+    static_assert((CHIP_CONFIG_ICD_IDLE_MODE_DURATION_SEC) >= kMinIdleModeDuration_s,
                   "Spec requires the IdleModeDuration to be equal or greater to 1s.");
-    uint32_t mIdleInterval_s = CHIP_CONFIG_ICD_IDLE_MODE_DURATION_SEC;
+    uint32_t mIdleModeDuration_s = CHIP_CONFIG_ICD_IDLE_MODE_DURATION_SEC;
 
     static_assert((CHIP_CONFIG_ICD_ACTIVE_MODE_DURATION_MS) <= (CHIP_CONFIG_ICD_IDLE_MODE_DURATION_SEC * kMillisecondsPerSecond),
                   "Spec requires the IdleModeDuration be equal or greater to the ActiveModeDuration.");
-    uint32_t mActiveInterval_ms = CHIP_CONFIG_ICD_ACTIVE_MODE_DURATION_MS;
+    uint32_t mActiveModeDuration_ms = CHIP_CONFIG_ICD_ACTIVE_MODE_DURATION_MS;
 
     uint16_t mActiveThreshold_ms = CHIP_CONFIG_ICD_ACTIVE_MODE_THRESHOLD_MS;