[ICD]Update the DNS-SD advertissement (SII and ICD keys) on icd opera…

…tion mode changes (#30422)

* WIP restart DNS advertissement on ICD mode changes

* dont advertise the SII key when in ICD is in LIT

* Create a pool of ICDStateObservers pointers. Add an API to register and release ICDStateObservers. DNSSD server is now an ICDStateObserver so it can update the DNS-SD entries on ICD Mode changes

* add AppConfig.h for missing define on linux

* Fix TestAdvertiser, Add a test step to validate when ICD Key is set to 1, the SII key is not included in the Adv

* Apply suggestions

* Restyled by clang-format

* Fix unit test when chip_enable_icd_server=false, add comment to config

* Restyled by whitespace

* Fix build error after last minute change :(

---------

Co-authored-by: Restyled.io <commits@restyled.io>

src/app/icd/ICDManager.cpp

@@ -44,12 +44,10 @@ uint8_t ICDManager::OpenExchangeContextCount = 0;
 static_assert(UINT8_MAX >= CHIP_CONFIG_MAX_EXCHANGE_CONTEXTS,
               "ICDManager::OpenExchangeContextCount cannot hold count for the max exchange count");
 
-void ICDManager::Init(PersistentStorageDelegate * storage, FabricTable * fabricTable, ICDStateObserver * stateObserver,
-                      Crypto::SymmetricKeystore * symmetricKeystore)
+void ICDManager::Init(PersistentStorageDelegate * storage, FabricTable * fabricTable, Crypto::SymmetricKeystore * symmetricKeystore)
 {
     VerifyOrDie(storage != nullptr);
     VerifyOrDie(fabricTable != nullptr);
-    VerifyOrDie(stateObserver != nullptr);
     VerifyOrDie(symmetricKeystore != nullptr);
 
     bool supportLIT = SupportsFeature(Feature::kLongIdleTimeSupport);
@@ -62,9 +60,8 @@ void ICDManager::Init(PersistentStorageDelegate * storage, FabricTable * fabricT
     // VerifyOrDieWithMsg((supportLIT == false) && (GetSlowPollingInterval() <= GetSITPollingThreshold()) , AppServer,
     //                    "LIT support is required for slow polling intervals superior to 15 seconds");
 
-    mStorage       = storage;
-    mFabricTable   = fabricTable;
-    mStateObserver = stateObserver;
+    mStorage     = storage;
+    mFabricTable = fabricTable;
     VerifyOrDie(ICDNotifier::GetInstance().Subscribe(this) == CHIP_NO_ERROR);
     mSymmetricKeystore = symmetricKeystore;
 
@@ -90,20 +87,20 @@ void ICDManager::Shutdown()
     mOperationalState = OperationalState::IdleMode;
     mStorage          = nullptr;
     mFabricTable      = nullptr;
+    mStateObserverPool.ReleaseAll();
 }
 
 bool ICDManager::SupportsFeature(Feature feature)
 {
     // Can't use attribute accessors/Attributes::FeatureMap::Get in unit tests
-#ifndef CONFIG_BUILD_FOR_HOST_UNIT_TEST
-    bool success        = false;
+#if !CONFIG_BUILD_FOR_HOST_UNIT_TEST
     uint32_t featureMap = 0;
-    success             = (Attributes::FeatureMap::Get(kRootEndpointId, &featureMap) == EMBER_ZCL_STATUS_SUCCESS);
-
+    bool success        = (Attributes::FeatureMap::Get(kRootEndpointId, &featureMap) == EMBER_ZCL_STATUS_SUCCESS);
     return success ? ((featureMap & to_underlying(feature)) != 0) : false;
 #else
+
     return ((mFeatureMap & to_underlying(feature)) != 0);
-#endif // CONFIG_BUILD_FOR_HOST_UNIT_TEST
+#endif // !CONFIG_BUILD_FOR_HOST_UNIT_TEST
 }
 
 void ICDManager::UpdateICDMode()
@@ -129,7 +126,12 @@ void ICDManager::UpdateICDMode()
             }
         }
     }
-    mICDMode = tempMode;
+
+    if (mICDMode != tempMode)
+    {
+        mICDMode = tempMode;
+        postObserverEvent(ObserverEventType::ICDModeChange);
+    }
 
     // When in SIT mode, the slow poll interval SHOULDN'T be greater than the SIT mode polling threshold, per spec.
     if (mICDMode == ICDMode::SIT && GetSlowPollingInterval() > GetSITPollingThreshold())
@@ -202,7 +204,7 @@ void ICDManager::UpdateOperationState(OperationalState state)
                 ChipLogError(AppServer, "Failed to set Polling Interval: err %" CHIP_ERROR_FORMAT, err.Format());
             }
 
-            mStateObserver->OnEnterActiveMode();
+            postObserverEvent(ObserverEventType::EnterActiveMode);
         }
         else
         {
@@ -264,7 +266,7 @@ void ICDManager::OnTransitionToIdle(System::Layer * aLayer, void * appState)
     // OnTransitionToIdle will trigger a report message if reporting is needed, which should extend the active mode until the
     // ack for the report is received.
     pICDManager->mTransitionToIdleCalled = true;
-    pICDManager->mStateObserver->OnTransitionToIdle();
+    pICDManager->postObserverEvent(ObserverEventType::TransitionToIdle);
 }
 
 /* ICDListener functions. */
@@ -339,5 +341,60 @@ void ICDManager::OnICDManagementServerEvent(ICDManagementEvents event)
     }
 }
 
+System::Clock::Milliseconds32 ICDManager::GetSlowPollingInterval()
+{
+#if ICD_ENFORCE_SIT_SLOW_POLL_LIMIT
+    // When in SIT mode, the slow poll interval SHOULDN'T be greater than the SIT mode polling threshold, per spec.
+    // This is important for ICD device configured for LIT operation but currently operating as a SIT
+    // due to a lack of client registration
+    if (mICDMode == ICDMode::SIT && GetSlowPollingInterval() > GetSITPollingThreshold())
+    {
+        return GetSITPollingThreshold();
+    }
+#endif
+    return kSlowPollingInterval;
+}
+
+ICDManager::ObserverPointer * ICDManager::RegisterObserver(ICDStateObserver * observer)
+{
+    return mStateObserverPool.CreateObject(observer);
+}
+
+void ICDManager::ReleaseObserver(ICDStateObserver * observer)
+{
+    mStateObserverPool.ForEachActiveObject([this, observer](ObserverPointer * obs) {
+        if (obs->mObserver == observer)
+        {
+            mStateObserverPool.ReleaseObject(obs);
+            return Loop::Break;
+        }
+        return Loop::Continue;
+    });
+}
+
+void ICDManager::postObserverEvent(ObserverEventType event)
+{
+    mStateObserverPool.ForEachActiveObject([event](ObserverPointer * obs) {
+        switch (event)
+        {
+        case ObserverEventType::EnterActiveMode: {
+            obs->mObserver->OnEnterActiveMode();
+            return Loop::Continue;
+        }
+        case ObserverEventType::TransitionToIdle: {
+            obs->mObserver->OnTransitionToIdle();
+            return Loop::Continue;
+        }
+        case ObserverEventType::ICDModeChange: {
+            obs->mObserver->OnICDModeChange();
+            return Loop::Continue;
+        }
+        default: {
+            ChipLogError(DeviceLayer, "Invalid ICD Observer event type");
+            return Loop::Break;
+        }
+        }
+    });
+}
 } // namespace app
 } // namespace chip

src/app/icd/ICDManager.h

@@ -33,12 +33,21 @@ namespace app {
 // Used in unit tests
 class TestICDManager;
 
+// This structure is used for the creation an ObjectPool of ICDStateObserver pointers
+
 /**
  * @brief ICD Manager is responsible of processing the events and triggering the correct action for an ICD
  */
 class ICDManager : public ICDListener
 {
 public:
+    struct ObserverPointer
+    {
+        ObserverPointer(ICDStateObserver * obs) : mObserver(obs) {}
+        ~ObserverPointer() { mObserver = nullptr; }
+        ICDStateObserver * mObserver;
+    };
+
     enum class OperationalState : uint8_t
     {
         IdleMode,
@@ -51,21 +60,48 @@ class ICDManager : public ICDListener
         LIT, // Long Interval Time ICD
     };
 
+    // This enum class represents to all ICDStateObserver callbacks available from the
+    // mStateObserverPool for the ICDManager.
+    enum class ObserverEventType : uint8_t
+    {
+        EnterActiveMode,
+        TransitionToIdle,
+        ICDModeChange,
+    };
+
     ICDManager() {}
-    void Init(PersistentStorageDelegate * storage, FabricTable * fabricTable, ICDStateObserver * stateObserver,
-              Crypto::SymmetricKeystore * symmetricKeyStore);
+    void Init(PersistentStorageDelegate * storage, FabricTable * fabricTable, Crypto::SymmetricKeystore * symmetricKeyStore);
     void Shutdown();
     void UpdateICDMode();
     void UpdateOperationState(OperationalState state);
     void SetKeepActiveModeRequirements(KeepActiveFlags flag, bool state);
     bool IsKeepActive() { return mKeepActiveFlags.HasAny(); }
     bool SupportsFeature(Clusters::IcdManagement::Feature feature);
+
+    /**
+     * @brief Adds the referenced observer in parameters to the mStateObserverPool
+     * A maximum of CHIP_CONFIG_ICD_OBSERVERS_POOL_SIZE observers can be concurrently registered
+     *
+     * @return The pointer to the pool object, or null if it could not be added.
+     */
+    ObserverPointer * RegisterObserver(ICDStateObserver * observer);
+
+    /**
+     * @brief Remove the referenced observer in parameters from the mStateObserverPool
+     */
+    void ReleaseObserver(ICDStateObserver * observer);
+
+    /**
+     * @brief Associates the ObserverEventType parameters to the correct
+     *  ICDStateObservers function and calls it for all observers in the mStateObserverPool
+     */
+    void postObserverEvent(ObserverEventType event);
     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; }
+    static System::Clock::Milliseconds32 GetSlowPollingInterval();
 
 #ifdef CONFIG_BUILD_FOR_HOST_UNIT_TEST
     void SetTestFeatureMapValue(uint32_t featureMap) { mFeatureMap = featureMap; };
@@ -110,9 +146,9 @@ class ICDManager : public ICDListener
     ICDMode mICDMode                               = ICDMode::SIT;
     PersistentStorageDelegate * mStorage           = nullptr;
     FabricTable * mFabricTable                     = nullptr;
-    ICDStateObserver * mStateObserver              = nullptr;
     bool mTransitionToIdleCalled                   = false;
     Crypto::SymmetricKeystore * mSymmetricKeystore = nullptr;
+    ObjectPool<ObserverPointer, CHIP_CONFIG_ICD_OBSERVERS_POOL_SIZE> mStateObserverPool;
 
 #ifdef CONFIG_BUILD_FOR_HOST_UNIT_TEST
     // feature map that can be changed at runtime for testing purposes

src/app/icd/ICDStateObserver.h

@@ -33,6 +33,7 @@ class ICDStateObserver
     virtual ~ICDStateObserver() {}
     virtual void OnEnterActiveMode()  = 0;
     virtual void OnTransitionToIdle() = 0;
+    virtual void OnICDModeChange()    = 0;
 };
 
 } // namespace app

src/app/reporting/ReportSchedulerImpl.h

@@ -35,6 +35,8 @@ class ReportSchedulerImpl : public ReportScheduler
     // ICDStateObserver
     void OnEnterActiveMode() override;
     void OnTransitionToIdle() override;
+    // No action is needed by the ReportScheduler on ICD operation Mode changes
+    void OnICDModeChange() override{};
 
     // ReadHandlerObserver
     void OnSubscriptionEstablished(ReadHandler * aReadHandler) final;

src/app/server/BUILD.gn

@@ -51,6 +51,7 @@ static_library("server") {
   public_deps = [
     "${chip_root}/src/app",
     "${chip_root}/src/app/icd:notifier",
+    "${chip_root}/src/app/icd:observer",
     "${chip_root}/src/lib/address_resolve",
     "${chip_root}/src/lib/dnssd",
     "${chip_root}/src/messaging",

src/app/server/Dnssd.cpp

@@ -489,5 +489,12 @@ CHIP_ERROR DnssdServer::GenerateRotatingDeviceId(char rotatingDeviceIdHexBuffer[
 }
 #endif
 
+void DnssdServer::OnICDModeChange()
+{
+    // ICDMode changed, restart DNS-SD advertising, because SII and ICD key are affected by this change.
+    // StartServer will take care of setting the operational and commissionable advertissements
+    StartServer();
+}
+
 } // namespace app
 } // namespace chip

src/app/server/Dnssd.h

@@ -17,6 +17,7 @@
 
 #pragma once
 
+#include <app/icd/ICDStateObserver.h>
 #include <app/server/CommissioningModeProvider.h>
 #include <credentials/FabricTable.h>
 #include <lib/core/CHIPError.h>
@@ -29,7 +30,7 @@
 namespace chip {
 namespace app {
 
-class DLL_EXPORT DnssdServer
+class DLL_EXPORT DnssdServer : public ICDStateObserver
 {
 public:
     static constexpr System::Clock::Timestamp kTimeoutCleared = System::Clock::kZero;
@@ -119,6 +120,12 @@ class DLL_EXPORT DnssdServer
      */
     CHIP_ERROR SetEphemeralDiscriminator(Optional<uint16_t> discriminator);
 
+    // ICDStateObserver
+    // No action is needed by the DnssdServer on active or idle state entries
+    void OnEnterActiveMode() override{};
+    void OnTransitionToIdle() override{};
+    void OnICDModeChange() override;
+
 private:
     /// Overloaded utility method for commissioner and commissionable advertisement
     /// This method is used for both commissioner discovery and commissionable node discovery since

src/app/server/Server.cpp

@@ -257,7 +257,11 @@ CHIP_ERROR Server::Init(const ServerInitParams & initParams)
 
 // ICD Init needs to be after data model init
 #if CHIP_CONFIG_ENABLE_ICD_SERVER
-    mICDManager.Init(mDeviceStorage, &GetFabricTable(), mReportScheduler, mSessionKeystore);
+    mICDManager.Init(mDeviceStorage, &GetFabricTable(), mSessionKeystore);
+    // Register the ICDStateObservers. All observers are released at mICDManager.Shutdown()
+    // They can be released individually with ReleaseObserver
+    mICDManager.RegisterObserver(mReportScheduler);
+    mICDManager.RegisterObserver(&app::DnssdServer::Instance());
 #endif // CHIP_CONFIG_ENABLE_ICD_SERVER
 
 #if defined(CHIP_APP_USE_ECHO)

src/app/tests/TestICDManager.cpp

@@ -65,6 +65,7 @@ class TestICDStateObserver : public app::ICDStateObserver
 public:
     void OnEnterActiveMode() {}
     void OnTransitionToIdle() {}
+    void OnICDModeChange() {}
 };
 
 TestICDStateObserver mICDStateObserver;
@@ -89,7 +90,8 @@ class TestContext : public Test::AppContext
         {
             return FAILURE;
         }
-        ctx->mICDManager.Init(&ctx->testStorage, &ctx->GetFabricTable(), &mICDStateObserver, &(ctx->mKeystore));
+        ctx->mICDManager.Init(&ctx->testStorage, &ctx->GetFabricTable(), &(ctx->mKeystore));
+        ctx->mICDManager.RegisterObserver(&mICDStateObserver);
         return SUCCESS;
     }
 

src/lib/core/CHIPConfig.h

@@ -1600,6 +1600,16 @@ extern const char CHIP_NON_PRODUCTION_MARKER[];
 #define CHIP_CONFIG_SYNCHRONOUS_REPORTS_ENABLED 0
 #endif
 
+/**
+ * @def CHIP_CONFIG_ICD_OBSERVERS_POOL_SIZE
+ *
+ * @brief Defines the entry iterator delegate pool size of the ICDObserver object pool in ICDManager.h.
+ *        Two are used in the default implementation. Users can increase it to register more observers.
+ */
+#ifndef CHIP_CONFIG_ICD_OBSERVERS_POOL_SIZE
+#define CHIP_CONFIG_ICD_OBSERVERS_POOL_SIZE 2
+#endif
+
 /**
  * @}
  */

src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp

@@ -23,6 +23,7 @@
 #include "MinimalMdnsServer.h"
 #include "ServiceNaming.h"
 
+#include <app/AppConfig.h>
 #include <crypto/RandUtils.h>
 #include <lib/dnssd/Advertiser_ImplMinimalMdnsAllocator.h>
 #include <lib/dnssd/minimal_mdns/AddressPolicy.h>
@@ -230,6 +231,14 @@ class AdvertiserMinMdns : public ServiceAdvertiser,
         if (optionalMrp.HasValue())
         {
             auto mrp = optionalMrp.Value();
+
+#if CHIP_CONFIG_ENABLE_ICD_SERVER
+            // An ICD operating as a LIT should not advertise its slow polling interval.
+            // When the ICD doesn't support the LIT feature, it doesn't set nor advertise the GetICDOperatingAsLIT entry.
+            // Therefore when GetICDOperatingAsLIT has no value or a value of 0, we advertise the slow polling interval
+            // otherwise we don't include the SII key in the advertisement.
+            if (!params.GetICDOperatingAsLIT().ValueOr(false))
+#endif
             {
                 if (mrp.mIdleRetransTimeout > kMaxRetryInterval)
                 {

src/lib/dnssd/Discovery_ImplPlatform.cpp

@@ -224,6 +224,16 @@ CHIP_ERROR CopyTxtRecord(TxtFieldKey key, char * buffer, size_t bufferLen, const
     case TxtFieldKey::kTcpSupported:
         return CopyTextRecordValue(buffer, bufferLen, params.GetTcpSupported());
     case TxtFieldKey::kSessionIdleInterval:
+#if CHIP_CONFIG_ENABLE_ICD_SERVER
+        // A ICD operating as a LIT should not advertise its slow polling interval
+        if (params.GetICDOperatingAsLIT().HasValue() && params.GetICDOperatingAsLIT().Value())
+        {
+            // returning WELL_UNINITIALIZED ensures that the SII string isn't added by the AddTxtRecord
+            // without erroring out the action.
+            return CHIP_ERROR_WELL_UNINITIALIZED;
+        }
+        FALLTHROUGH;
+#endif
     case TxtFieldKey::kSessionActiveInterval:
     case TxtFieldKey::kSessionActiveThreshold:
         return CopyTextRecordValue(buffer, bufferLen, params.GetLocalMRPConfig(), key);