[ICD] Address post-merge comments for project-chip#31563 (project-chip#31811)

* address post review comments

* Simplify empty optional call

Author: mkardous-silabs

src/app/BUILD.gn

@@ -115,7 +115,7 @@ source_set("pre-encoded-value") {
 }
 
 source_set("subscription-manager") {
-  sources = [ "SubscriptionManager.h" ]
+  sources = [ "SubscriptionsInfoProvider.h" ]
 
   public_deps = [ "${chip_root}/src/lib/core" ]
 }

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(FabricIndex aFabricIndex, NodeId subjectID)
 {
     bool isActive = false;
     mReadHandlers.ForEachActiveObject([aFabricIndex, subjectID, &isActive](ReadHandler * handler) {
@@ -367,7 +367,7 @@ bool InteractionModelEngine::SubjectHasActiveSubscription(const FabricIndex & aF
     return isActive;
 }
 
-bool InteractionModelEngine::SubjectHasPersistedSubscription(const FabricIndex & aFabricIndex, const NodeId & subject)
+bool InteractionModelEngine::SubjectHasPersistedSubscription(FabricIndex aFabricIndex, NodeId subjectID)
 {
     // TODO(#30281) : Implement persisted sub check and verify how persistent subscriptions affects this at ICDManager::Init
     return false;

src/app/InteractionModelEngine.h

@@ -42,8 +42,8 @@
 #include <app/ReadClient.h>
 #include <app/ReadHandler.h>
 #include <app/StatusResponse.h>
-#include <app/SubscriptionManager.h>
 #include <app/SubscriptionResumptionSessionEstablisher.h>
+#include <app/SubscriptionsInfoProvider.h>
 #include <app/TimedHandler.h>
 #include <app/WriteClient.h>
 #include <app/WriteHandler.h>
@@ -80,7 +80,7 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
                                public CommandHandler::Callback,
                                public ReadHandler::ManagementCallback,
                                public FabricTable::Delegate,
-                               public SubscriptionManager
+                               public SubscriptionsInfoProvider
 {
 public:
     /**
@@ -324,9 +324,9 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
 
     CHIP_ERROR ResumeSubscriptions();
 
-    bool SubjectHasActiveSubscription(const FabricIndex & aFabricIndex, const NodeId & subject) override;
+    bool SubjectHasActiveSubscription(FabricIndex aFabricIndex, NodeId subjectID) override;
 
-    bool SubjectHasPersistedSubscription(const FabricIndex & aFabricIndex, const NodeId & subject) override;
+    bool SubjectHasPersistedSubscription(FabricIndex aFabricIndex, NodeId subjectID) override;
 
 #if CONFIG_BUILD_FOR_HOST_UNIT_TEST
     //

src/app/SubscriptionManager.h src/app/SubscriptionsInfoProvider.h

@@ -18,10 +18,10 @@
 
 /**
  *    @file
- *      This file defines an interface that exposes all the public subscription management APIs.
+ *      This file defines an interface that exposes all the public subscription information 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.
+ *      since the IMEngine has more dependency than its consummers need.
+ *      By leveraging the SubscriptionInfoProvider APIs, a consumer avoids depending on the global data model functions.
  */
 
 #pragma once
@@ -32,35 +32,33 @@
 namespace chip {
 namespace app {
 
-class SubscriptionManager
+class SubscriptionsInfoProvider
 {
 public:
-    virtual ~SubscriptionManager(){};
+    virtual ~SubscriptionsInfoProvider(){};
 
     /**
      * @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
+     * @param[in] subjectID NodeId of the subject
      *
      * @return true subject has at least one active subscription with the device
-     *         false subject doesn't have any acitve subscription with the device
+     *         false subject doesn't have any active subscription with the device
      */
-    virtual bool SubjectHasActiveSubscription(const FabricIndex & aFabricIndex, const NodeId & subject) = 0;
+    virtual bool SubjectHasActiveSubscription(FabricIndex aFabricIndex, NodeId subjectID) = 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
+     * @param[in] subjectID NodeId of the subject
      *
      * @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
+     *         false subject doesn't have any persisted subscription with the device
      */
-    virtual bool SubjectHasPersistedSubscription(const FabricIndex & aFabricIndex, const NodeId & subject) = 0;
+    virtual bool SubjectHasPersistedSubscription(FabricIndex aFabricIndex, NodeId subjectID) = 0;
 };
 
 } // namespace app

src/app/icd/server/ICDConfigurationData.cpp

@@ -37,14 +37,20 @@ System::Clock::Milliseconds32 ICDConfigurationData::GetSlowPollingInterval()
     return mSlowPollingInterval;
 }
 
-CHIP_ERROR ICDConfigurationData::SetModeDurations(uint32_t activeModeDuration_ms, uint32_t idleModeInterval_s)
+CHIP_ERROR ICDConfigurationData::SetModeDurations(Optional<uint32_t> activeModeDuration_ms, Optional<uint32_t> idleModeDuration_ms)
 {
-    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);
+    VerifyOrReturnError(activeModeDuration_ms.HasValue() || idleModeDuration_ms.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
 
-    mIdleModeDuration_s    = idleModeInterval_s;
-    mActiveModeDuration_ms = activeModeDuration_ms;
+    uint32_t tmpIdleModeDuration_s =
+        idleModeDuration_ms.HasValue() ? (idleModeDuration_ms.Value() / kMillisecondsPerSecond) : mIdleModeDuration_s;
+    uint32_t tmpActiveModeDuration_ms = activeModeDuration_ms.HasValue() ? activeModeDuration_ms.Value() : mActiveModeDuration_ms;
+
+    VerifyOrReturnError(tmpActiveModeDuration_ms <= (tmpIdleModeDuration_s * kMillisecondsPerSecond), CHIP_ERROR_INVALID_ARGUMENT);
+    VerifyOrReturnError(tmpIdleModeDuration_s <= kMaxIdleModeDuration_s, CHIP_ERROR_INVALID_ARGUMENT);
+    VerifyOrReturnError(tmpIdleModeDuration_s >= kMinIdleModeDuration_s, CHIP_ERROR_INVALID_ARGUMENT);
+
+    mIdleModeDuration_s    = tmpIdleModeDuration_s;
+    mActiveModeDuration_ms = tmpActiveModeDuration_ms;
 
     return CHIP_NO_ERROR;
 }

src/app/icd/server/ICDConfigurationData.h

@@ -17,16 +17,18 @@
 
 #pragma once
 
+#include <lib/core/Optional.h>
+#include <lib/support/TimeUtils.h>
 #include <platform/CHIPDeviceConfig.h>
 #include <system/SystemClock.h>
 
 namespace chip {
 
 namespace app {
-// Forward declaration of ICDManager to allow it to be friend with ICDConfigurationData
+// 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
+// Forward declaration of TestICDManager to allow it to be friend with the ICDConfigurationData.
 // Used in unit tests
 class TestICDManager;
 
@@ -104,19 +106,21 @@ class ICDConfigurationData
     static constexpr uint32_t kMinLitActiveModeThreshold_ms = 5000;
 
     /**
-     * @brief Change the ActiveModeDuration and IdleModeDuration value
-     *        To only change one value, pass the old value for the other one
+     * @brief Change the ActiveModeDuration or the IdleModeDuration value
+     *        If only one value is provided, check will be done agaisn't the other already set value.
      *
      * @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
+     * @param[in] idleModeDuration_ms new IdleModeDuration value
+     *                                The precision of the IdleModeDuration must be seconds.
+     * @return CHIP_ERROR CHIP_ERROR_INVALID_ARGUMENT is returned if idleModeDuration_ms is smaller than activeModeDuration_ms
+     *                                                is returned if idleModeDuration_ms is greater than 64800000 ms
+     *                                                is returned if idleModeDuration_ms is smaller than 1000 ms
+     *                                                is returned if no valid values are provided
      *                    CHIP_NO_ERROR is returned if the new intervals were set
      */
-    CHIP_ERROR SetModeDurations(uint32_t activeModeDuration_ms, uint32_t idleModeDuration_s);
+    CHIP_ERROR SetModeDurations(Optional<uint32_t> activeModeDuration_ms, Optional<uint32_t> idleModeDuration_ms);
 
-    static constexpr uint32_t kMaxIdleModeDuration_s = 64800;
+    static constexpr uint32_t kMaxIdleModeDuration_s = (18 * kSecondsPerHour);
     static constexpr uint32_t kMinIdleModeDuration_s = 1;
 
     static_assert((CHIP_CONFIG_ICD_IDLE_MODE_DURATION_SEC) <= kMaxIdleModeDuration_s,

src/app/icd/server/ICDManager.cpp

@@ -40,7 +40,7 @@ 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, SubscriptionManager * manager)
+                      Messaging::ExchangeManager * exchangeManager, SubscriptionsInfoProvider * manager)
 {
     VerifyOrDie(storage != nullptr);
     VerifyOrDie(fabricTable != nullptr);

src/app/icd/server/ICDManager.h

@@ -17,7 +17,7 @@
 #pragma once
 
 #include <app-common/zap-generated/cluster-enums.h>
-#include <app/SubscriptionManager.h>
+#include <app/SubscriptionsInfoProvider.h>
 #include <app/icd/server/ICDCheckInSender.h>
 #include <app/icd/server/ICDConfigurationData.h>
 #include <app/icd/server/ICDMonitoringTable.h>
@@ -67,7 +67,7 @@ class ICDManager : public ICDListener
 
     ICDManager() {}
     void Init(PersistentStorageDelegate * storage, FabricTable * fabricTable, Crypto::SymmetricKeystore * symmetricKeyStore,
-              Messaging::ExchangeManager * exchangeManager, SubscriptionManager * manager);
+              Messaging::ExchangeManager * exchangeManager, SubscriptionsInfoProvider * manager);
     void Shutdown();
     void UpdateICDMode();
     void UpdateOperationState(OperationalState state);
@@ -148,7 +148,7 @@ class ICDManager : public ICDListener
     FabricTable * mFabricTable                     = nullptr;
     Messaging::ExchangeManager * mExchangeManager  = nullptr;
     Crypto::SymmetricKeystore * mSymmetricKeystore = nullptr;
-    SubscriptionManager * mSubManager              = nullptr;
+    SubscriptionsInfoProvider * mSubManager        = nullptr;
 
     bool mTransitionToIdleCalled = false;
 

src/app/tests/TestICDManager.cpp

@@ -16,7 +16,7 @@
  *    limitations under the License.
  */
 #include <app/EventManagement.h>
-#include <app/SubscriptionManager.h>
+#include <app/SubscriptionsInfoProvider.h>
 #include <app/icd/server/ICDConfigurationData.h>
 #include <app/icd/server/ICDManager.h>
 #include <app/icd/server/ICDNotifier.h>
@@ -71,16 +71,16 @@ class TestICDStateObserver : public app::ICDStateObserver
     void OnICDModeChange() {}
 };
 
-class TestSubscriptionManager : public SubscriptionManager
+class TestSubscriptionsInfoProvider : public SubscriptionsInfoProvider
 {
 public:
-    TestSubscriptionManager() = default;
-    ~TestSubscriptionManager(){};
+    TestSubscriptionsInfoProvider() = default;
+    ~TestSubscriptionsInfoProvider(){};
 
     void SetReturnValue(bool value) { mReturnValue = value; };
 
-    bool SubjectHasActiveSubscription(const FabricIndex & aFabricIndex, const NodeId & subject) { return mReturnValue; };
-    bool SubjectHasPersistedSubscription(const FabricIndex & aFabricIndex, const NodeId & subject) { return mReturnValue; };
+    bool SubjectHasActiveSubscription(FabricIndex aFabricIndex, NodeId subject) { return mReturnValue; };
+    bool SubjectHasPersistedSubscription(FabricIndex aFabricIndex, NodeId subject) { return mReturnValue; };
 
 private:
     bool mReturnValue = false;
@@ -126,7 +126,7 @@ class TestContext : public chip::Test::AppContext
     System::Clock::Internal::MockClock mMockClock;
     TestSessionKeystoreImpl mKeystore;
     app::ICDManager mICDManager;
-    TestSubscriptionManager mSubManager;
+    TestSubscriptionsInfoProvider mSubManager;
     TestPersistentStorageDelegate testStorage;
 
 private:
@@ -198,7 +198,7 @@ class TestICDManager
 
         // Set New durations for test case
         uint32_t oldActiveModeDuration_ms = icdConfigData.GetActiveModeDurationMs();
-        icdConfigData.SetModeDurations(0, icdConfigData.GetIdleModeDurationSec());
+        icdConfigData.SetModeDurations(MakeOptional(static_cast<uint32_t>(0)), NullOptional);
 
         // Verify That ICDManager starts in Idle
         NL_TEST_ASSERT(aSuite, ctx->mICDManager.mOperationalState == ICDManager::OperationalState::IdleMode);
@@ -256,7 +256,7 @@ class TestICDManager
         NL_TEST_ASSERT(aSuite, ctx->mICDManager.mOperationalState == ICDManager::OperationalState::IdleMode);
 
         // Reset Old durations
-        icdConfigData.SetModeDurations(oldActiveModeDuration_ms, icdConfigData.GetIdleModeDurationSec());
+        icdConfigData.SetModeDurations(MakeOptional(oldActiveModeDuration_ms), NullOptional);
     }
 
     /**
@@ -277,7 +277,7 @@ class TestICDManager
 
         // Set New durations for test case
         uint32_t oldActiveModeDuration_ms = icdConfigData.GetActiveModeDurationMs();
-        icdConfigData.SetModeDurations(0, icdConfigData.GetIdleModeDurationSec());
+        icdConfigData.SetModeDurations(MakeOptional(static_cast<uint32_t>(0)), NullOptional);
 
         // Verify That ICDManager starts in Idle
         NL_TEST_ASSERT(aSuite, ctx->mICDManager.mOperationalState == ICDManager::OperationalState::IdleMode);
@@ -346,7 +346,7 @@ class TestICDManager
         NL_TEST_ASSERT(aSuite, ctx->mICDManager.mOperationalState == ICDManager::OperationalState::IdleMode);
 
         // Reset Old durations
-        icdConfigData.SetModeDurations(oldActiveModeDuration_ms, icdConfigData.GetIdleModeDurationSec());
+        icdConfigData.SetModeDurations(MakeOptional(oldActiveModeDuration_ms), NullOptional);
     }
 
     static void TestKeepActivemodeRequests(nlTestSuite * aSuite, void * aContext)