[ICD] Fix ordering issue when triggering a report on Active mode (#31524

)

* Add on subscription report callback for the ICD Manager

* Add comment to engine notifier call

* Move event in the sub specific location

* Add ICDManager test

* remove unnecessary checks

* Broadcast* function to Notify*

examples/lit-icd-app/linux/args.gni

@@ -29,3 +29,4 @@ matter_enable_tracing_support = true
 # ICD configurations
 chip_enable_icd_server = true
 chip_subscription_timeout_resumption = false
+chip_icd_report_on_active_mode = true

examples/lit-icd-app/silabs/openthread.gni

@@ -31,6 +31,7 @@ chip_enable_icd_server = true
 chip_subscription_timeout_resumption = false
 sl_use_subscription_synching = true
 icd_enforce_sit_slow_poll_limit = true
+chip_icd_report_on_active_mode = true
 
 # Openthread Configuration flags
 sl_ot_idle_interval_ms = 3600000  # 60mins Idle Polling Interval

examples/platform/silabs/BaseApplication.cpp

@@ -485,7 +485,7 @@ void BaseApplication::ButtonHandler(AppEvent * aEvent)
 #if CHIP_CONFIG_ENABLE_ICD_SERVER
                 // Temporarily claim network activity, until we implement a "user trigger" reason for ICD wakeups.
                 PlatformMgr().LockChipStack();
-                ICDNotifier::GetInstance().BroadcastNetworkActivityNotification();
+                ICDNotifier::GetInstance().NotifyNetworkActivityNotification();
                 PlatformMgr().UnlockChipStack();
 #endif // CHIP_CONFIG_ENABLE_ICD_SERVER
             }

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

@@ -176,7 +176,7 @@ class IcdManagementFabricDelegate : public FabricTable::Delegate
         uint16_t supported_clients = mICDConfigurationData->GetClientsSupportedPerFabric();
         ICDMonitoringTable table(*mStorage, fabricIndex, supported_clients, mSymmetricKeystore);
         table.RemoveAll();
-        ICDNotifier::GetInstance().BroadcastICDManagementEvent(ICDListener::ICDManagementEvents::kTableUpdated);
+        ICDNotifier::GetInstance().NotifyICDManagementEvent(ICDListener::ICDManagementEvents::kTableUpdated);
     }
 
 private:
@@ -337,13 +337,13 @@ Status ICDManagementServer::StayActiveRequest(FabricIndex fabricIndex)
 {
     // TODO: Implementent stay awake logic for end device
     // https://github.com/project-chip/connectedhomeip/issues/24259
-    ICDNotifier::GetInstance().BroadcastICDManagementEvent(ICDListener::ICDManagementEvents::kStayActiveRequestReceived);
+    ICDNotifier::GetInstance().NotifyICDManagementEvent(ICDListener::ICDManagementEvents::kStayActiveRequestReceived);
     return InteractionModel::Status::UnsupportedCommand;
 }
 
 void ICDManagementServer::TriggerICDMTableUpdatedEvent()
 {
-    ICDNotifier::GetInstance().BroadcastICDManagementEvent(ICDListener::ICDManagementEvents::kTableUpdated);
+    ICDNotifier::GetInstance().NotifyICDManagementEvent(ICDListener::ICDManagementEvents::kTableUpdated);
 }
 
 void ICDManagementServer::Init(PersistentStorageDelegate & storage, Crypto::SymmetricKeystore * symmetricKeystore,

src/app/icd/BUILD.gn

@@ -24,7 +24,7 @@ buildconfig_header("icd_buildconfig") {
 
   defines = [
     "CHIP_CONFIG_ENABLE_ICD_SERVER=${chip_enable_icd_server}",
-    "ICD_REPORT_ON_ENTER_ACTIVE_MODE=${chip_report_on_active_mode}",
+    "ICD_REPORT_ON_ENTER_ACTIVE_MODE=${chip_icd_report_on_active_mode}",
     "ICD_MAX_NOTIFICATION_SUBSCRIBERS=${icd_max_notification_subscribers}",
     "ICD_ENFORCE_SIT_SLOW_POLL_LIMIT=${icd_enforce_sit_slow_poll_limit}",
   ]

src/app/icd/ICDCheckInSender.cpp

@@ -41,12 +41,12 @@ void ICDCheckInSender::OnNodeAddressResolved(const PeerId & peerId, const Addres
         ChipLogError(AppServer, "Failed to send the ICD Check-In message");
     }
 
-    ICDNotifier::GetInstance().BroadcastActiveRequestWithdrawal(ICDListener::KeepActiveFlag::kCheckInInProgress);
+    ICDNotifier::GetInstance().NotifyActiveRequestWithdrawal(ICDListener::KeepActiveFlag::kCheckInInProgress);
 }
 
 void ICDCheckInSender::OnNodeAddressResolutionFailed(const PeerId & peerId, CHIP_ERROR reason)
 {
-    ICDNotifier::GetInstance().BroadcastActiveRequestWithdrawal(ICDListener::KeepActiveFlag::kCheckInInProgress);
+    ICDNotifier::GetInstance().NotifyActiveRequestWithdrawal(ICDListener::KeepActiveFlag::kCheckInInProgress);
     ChipLogProgress(AppServer, "Node Address resolution failed for ICD Check-In with Node ID " ChipLogFormatX64,
                     ChipLogValueX64(peerId.GetNodeId()));
 }
@@ -110,7 +110,7 @@ CHIP_ERROR ICDCheckInSender::RequestResolve(ICDMonitoringEntry & entry, FabricTa
 
     if (err == CHIP_NO_ERROR)
     {
-        ICDNotifier::GetInstance().BroadcastActiveRequestNotification(ICDListener::KeepActiveFlag::kCheckInInProgress);
+        ICDNotifier::GetInstance().NotifyActiveRequestNotification(ICDListener::KeepActiveFlag::kCheckInInProgress);
     }
 
     return err;

src/app/icd/ICDManager.cpp

@@ -488,6 +488,15 @@ void ICDManager::OnICDManagementServerEvent(ICDManagementEvents event)
     }
 }
 
+void ICDManager::OnSubscriptionReport()
+{
+    // If the device is already in ActiveMode, that means that all active subscriptions have already been marked dirty.
+    // Since we only mark them dirty when we enter ActiveMode, it is not necessary to update the operational state a second time.
+    // Doing so will only add an ActiveModeThreshold to the active time which we don't want to do here.
+    VerifyOrReturn(mOperationalState == OperationalState::IdleMode);
+    this->UpdateOperationState(OperationalState::ActiveMode);
+}
+
 ICDManager::ObserverPointer * ICDManager::RegisterObserver(ICDStateObserver * observer)
 {
     return mStateObserverPool.CreateObject(observer);

src/app/icd/ICDManager.h

@@ -105,6 +105,7 @@ class ICDManager : public ICDListener
     void OnKeepActiveRequest(KeepActiveFlags request) override;
     void OnActiveRequestWithdrawal(KeepActiveFlags request) override;
     void OnICDManagementServerEvent(ICDManagementEvents event) override;
+    void OnSubscriptionReport() override;
 
 protected:
     friend class TestICDManager;

src/app/icd/ICDNotifier.cpp

@@ -56,7 +56,7 @@ void ICDNotifier::Unsubscribe(ICDListener * subscriber)
     }
 }
 
-void ICDNotifier::BroadcastNetworkActivityNotification()
+void ICDNotifier::NotifyNetworkActivityNotification()
 {
     for (auto subscriber : mSubscribers)
     {
@@ -67,7 +67,7 @@ void ICDNotifier::BroadcastNetworkActivityNotification()
     }
 }
 
-void ICDNotifier::BroadcastActiveRequestNotification(ICDListener::KeepActiveFlags request)
+void ICDNotifier::NotifyActiveRequestNotification(ICDListener::KeepActiveFlags request)
 {
     for (auto subscriber : mSubscribers)
     {
@@ -78,7 +78,7 @@ void ICDNotifier::BroadcastActiveRequestNotification(ICDListener::KeepActiveFlag
     }
 }
 
-void ICDNotifier::BroadcastActiveRequestWithdrawal(ICDListener::KeepActiveFlags request)
+void ICDNotifier::NotifyActiveRequestWithdrawal(ICDListener::KeepActiveFlags request)
 {
     for (auto subscriber : mSubscribers)
     {
@@ -89,7 +89,7 @@ void ICDNotifier::BroadcastActiveRequestWithdrawal(ICDListener::KeepActiveFlags
     }
 }
 
-void ICDNotifier::BroadcastICDManagementEvent(ICDListener::ICDManagementEvents event)
+void ICDNotifier::NotifyICDManagementEvent(ICDListener::ICDManagementEvents event)
 {
     for (auto subscriber : mSubscribers)
     {
@@ -100,5 +100,16 @@ void ICDNotifier::BroadcastICDManagementEvent(ICDListener::ICDManagementEvents e
     }
 }
 
+void ICDNotifier::NotifySubscriptionReport()
+{
+    for (auto subscriber : mSubscribers)
+    {
+        if (subscriber != nullptr)
+        {
+            subscriber->OnSubscriptionReport();
+        }
+    }
+}
+
 } // namespace app
 } // namespace chip

src/app/icd/ICDNotifier.h

@@ -58,34 +58,40 @@ class ICDListener
     virtual ~ICDListener() {}
 
     /**
-     * @brief This function is called for all subscribers of the ICDNotifier when it calls BroadcastNetworkActivityNotification
+     * @brief This function is called for all subscribers of the ICDNotifier when it calls NotifyNetworkActivityNotification.
      * It notifies the subscriber that a NetworkActivity occurred. For example, a message sent or received.
      */
     virtual void OnNetworkActivity() = 0;
 
     /**
-     * @brief This function is called for all subscribers of the ICDNotifier when it calls BroadcastActiveRequestNotification
+     * @brief This function is called for all subscribers of the ICDNotifier when it calls NotifyActiveRequestNotification.
      * It informs the subscriber that there is a need to place and keep the ICD in its Active Mode.
      *
      * @param request : Identity the request source
      */
     virtual void OnKeepActiveRequest(KeepActiveFlags request) = 0;
 
     /**
-     * @brief This function is called for all subscribers of the ICDNotifier when it calls BroadcastActiveRequestWithdrawal
+     * @brief This function is called for all subscribers of the ICDNotifier when it calls NotifyActiveRequestWithdrawal.
      * It informs the subscriber that a previous request no longer needs ICD to maintain its Active Mode.
      *
      * @param request : The request source
      */
     virtual void OnActiveRequestWithdrawal(KeepActiveFlags request) = 0;
 
     /**
-     * @brief This function is called for all subscribers of the ICDNotifier when it calls BroadcastICDManagementEvent
+     * @brief This function is called for all subscribers of the ICDNotifier when it calls NotifyICDManagementEvent.
      * It informs the subscriber that an ICD Management action has happened and needs to be processed
      *
      * @param event : The event type
      */
-    virtual void OnICDManagementServerEvent(ICDManagementEvents event){};
+    virtual void OnICDManagementServerEvent(ICDManagementEvents event) = 0;
+
+    /**
+     * @brief This function is called for all subscribers of the ICDNoitifier when it calls NotifySubscriptionReport.
+     * It informs the subscriber that a subscription report data is being sent.
+     */
+    virtual void OnSubscriptionReport() = 0;
 };
 
 class ICDNotifier
@@ -100,14 +106,15 @@ class ICDNotifier
      * For thread-safety reason (mostly of the ICDManager, which is a full time subscriber),
      * Those functions require to be called from the Chip Task Context, or by holding the chip stack lock.
      */
-    void BroadcastNetworkActivityNotification();
-    void BroadcastActiveRequestNotification(ICDListener::KeepActiveFlags request);
-    void BroadcastActiveRequestWithdrawal(ICDListener::KeepActiveFlags request);
-    void BroadcastICDManagementEvent(ICDListener::ICDManagementEvents event);
+    void NotifyNetworkActivityNotification();
+    void NotifyActiveRequestNotification(ICDListener::KeepActiveFlags request);
+    void NotifyActiveRequestWithdrawal(ICDListener::KeepActiveFlags request);
+    void NotifyICDManagementEvent(ICDListener::ICDManagementEvents event);
+    void NotifySubscriptionReport();
 
     inline void BroadcastActiveRequest(ICDListener::KeepActiveFlags request, bool notify)
     {
-        (notify) ? BroadcastActiveRequestNotification(request) : BroadcastActiveRequestWithdrawal(request);
+        (notify) ? NotifyActiveRequestNotification(request) : NotifyActiveRequestWithdrawal(request);
     }
 
     static ICDNotifier & GetInstance() { return sICDNotifier; }

src/app/icd/icd.gni

@@ -22,7 +22,7 @@ declare_args() {
   chip_enable_icd_client = false
 
   # Matter SDK Configuration flag to make the ICD manager emit a report on entering active mode
-  chip_report_on_active_mode = false
+  chip_icd_report_on_active_mode = false
 
   icd_max_notification_subscribers = 1
 

src/app/reporting/Engine.cpp

@@ -23,6 +23,10 @@
  *
  */
 
+#include <app/icd/ICDConfig.h>
+#if CHIP_CONFIG_ENABLE_ICD_SERVER
+#include <app/icd/ICDNotifier.h> // nogncheck
+#endif
 #include <app/AppConfig.h>
 #include <app/InteractionModelEngine.h>
 #include <app/RequiredPrivilege.h>
@@ -509,6 +513,12 @@ CHIP_ERROR Engine::BuildAndSendSingleReportData(ReadHandler * apReadHandler)
 
     if (apReadHandler->IsType(ReadHandler::InteractionType::Subscribe))
     {
+#if CHIP_CONFIG_ENABLE_ICD_SERVER
+        // Notify the ICDManager that we are about to send a subscription report before we prepare the Report payload.
+        // This allows the ICDManager to trigger any necessary updates and have the information in the report about to be sent.
+        app::ICDNotifier::GetInstance().NotifySubscriptionReport();
+#endif // CHIP_CONFIG_ENABLE_ICD_SERVER
+
         SubscriptionId subscriptionId = 0;
         apReadHandler->GetSubscriptionId(subscriptionId);
         reportDataBuilder.SubscriptionId(subscriptionId);
@@ -642,6 +652,7 @@ void Engine::Run()
 
         if (readHandler->ShouldReportUnscheduled() || imEngine->GetReportScheduler()->IsReportableNow(readHandler))
         {
+
             mRunningReadHandler = readHandler;
             CHIP_ERROR err      = BuildAndSendSingleReportData(readHandler);
             mRunningReadHandler = nullptr;

src/app/reporting/ReportSchedulerImpl.cpp

@@ -44,7 +44,7 @@ void ReportSchedulerImpl::OnEnterActiveMode()
 {
 #if ICD_REPORT_ON_ENTER_ACTIVE_MODE
     Timestamp now = mTimerDelegate->GetCurrentMonotonicTimestamp();
-    mNodesPool.ForEachActiveObject([now](ReadHandlerNode * node) {
+    mNodesPool.ForEachActiveObject([this, now](ReadHandlerNode * node) {
         if (now >= node->GetMinTimestamp())
         {
             this->HandlerForceDirtyState(node->GetReadHandler());

src/app/server/CommissioningWindowManager.cpp

@@ -558,11 +558,11 @@ void CommissioningWindowManager::UpdateWindowStatus(CommissioningWindowStatusEnu
         app::ICDListener::KeepActiveFlags request = app::ICDListener::KeepActiveFlag::kCommissioningWindowOpen;
         if (mWindowStatus != CommissioningWindowStatusEnum::kWindowNotOpen)
         {
-            app::ICDNotifier::GetInstance().BroadcastActiveRequestNotification(request);
+            app::ICDNotifier::GetInstance().NotifyActiveRequestNotification(request);
         }
         else
         {
-            app::ICDNotifier::GetInstance().BroadcastActiveRequestWithdrawal(request);
+            app::ICDNotifier::GetInstance().NotifyActiveRequestWithdrawal(request);
         }
 #endif // CHIP_CONFIG_ENABLE_ICD_SERVER
     }