[ICD] Subscription resumption should respect min interval on startup (#…

…25416) * [ICD] Subscription resumption should respect min interval on startup * Added documentation for sub resumption wait logic and referenced improvement issue #25439 * Cancel timer on Shutdown
project-chip · Mar 7, 2023 · 4f53be4 · 4f53be4
1 parent c14b011
commit 4f53be4
Show file tree

Hide file tree

Showing 2 changed files with 52 additions and 8 deletions.
diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp
@@ -74,6 +74,8 @@ CHIP_ERROR InteractionModelEngine::Init(Messaging::ExchangeManager * apExchangeM
 
 void InteractionModelEngine::Shutdown()
 {
+    mpExchangeMgr->GetSessionManager()->SystemLayer()->CancelTimer(ResumeSubscriptionsTimerCallback, this);
+
     CommandHandlerInterface * handlerIter = mCommandHandlerList;
 
     //
@@ -1591,33 +1593,73 @@ CHIP_ERROR InteractionModelEngine::ResumeSubscriptions()
 #if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS
     ReturnErrorCodeIf(!mpSubscriptionResumptionStorage, CHIP_NO_ERROR);
 
+    // To avoid the case of a reboot loop causing rapid traffic generation / power consumption, subscription resumption should make
+    // use of the persisted min-interval values, and wait before resumption. Ideally, each persisted subscription should wait their
+    // own min-interval value before resumption, but that both A) potentially runs into a timer resource issue, and B) having a
+    // low-powered device wake many times also has energy use implications. The logic below waits the largest of the persisted
+    // min-interval values before resuming subscriptions.
+
+    // Even though this causes subscription-to-subscription interaction by linking the min-interval values, this is the right thing
+    // to do for now because it's both simple and avoids the timer resource and multiple-wake problems. This issue is to track
+    // future improvements: https://github.com/project-chip/connectedhomeip/issues/25439
+
     SubscriptionResumptionStorage::SubscriptionInfo subscriptionInfo;
-    auto * iterator = mpSubscriptionResumptionStorage->IterateSubscriptions();
+    auto * iterator           = mpSubscriptionResumptionStorage->IterateSubscriptions();
+    int subscriptionsToResume = 0;
+    uint16_t minInterval      = 0;
+    while (iterator->Next(subscriptionInfo))
+    {
+        subscriptionsToResume++;
+        minInterval = std::max(minInterval, subscriptionInfo.mMinInterval);
+    }
+    iterator->Release();
+
+    if (subscriptionsToResume)
+    {
+        ChipLogProgress(InteractionModel, "Resuming %d subscriptions in %u seconds", subscriptionsToResume, minInterval);
+        ReturnErrorOnFailure(mpExchangeMgr->GetSessionManager()->SystemLayer()->StartTimer(System::Clock::Seconds16(minInterval),
+                                                                                           ResumeSubscriptionsTimerCallback, this));
+    }
+    else
+    {
+        ChipLogProgress(InteractionModel, "No subscriptions to resume");
+    }
+#endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS
+
+    return CHIP_NO_ERROR;
+}
+
+void InteractionModelEngine::ResumeSubscriptionsTimerCallback(System::Layer * apSystemLayer, void * apAppState)
+{
+#if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS
+    VerifyOrReturn(apAppState != nullptr);
+    InteractionModelEngine * imEngine = static_cast<InteractionModelEngine *>(apAppState);
+    SubscriptionResumptionStorage::SubscriptionInfo subscriptionInfo;
+    auto * iterator = imEngine->mpSubscriptionResumptionStorage->IterateSubscriptions();
     while (iterator->Next(subscriptionInfo))
     {
         auto requestedAttributePathCount = subscriptionInfo.mAttributePaths.AllocatedSize();
         auto requestedEventPathCount     = subscriptionInfo.mEventPaths.AllocatedSize();
-        if (!EnsureResourceForSubscription(subscriptionInfo.mFabricIndex, requestedAttributePathCount, requestedEventPathCount))
+        if (!imEngine->EnsureResourceForSubscription(subscriptionInfo.mFabricIndex, requestedAttributePathCount,
+                                                     requestedEventPathCount))
         {
             ChipLogProgress(InteractionModel, "no resource for Subscription resumption");
             iterator->Release();
-            return CHIP_ERROR_NO_MEMORY;
+            return;
         }
 
-        ReadHandler * handler = mReadHandlers.CreateObject(*this);
+        ReadHandler * handler = imEngine->mReadHandlers.CreateObject(*imEngine);
         if (handler == nullptr)
         {
             ChipLogProgress(InteractionModel, "no resource for ReadHandler creation");
             iterator->Release();
-            return CHIP_ERROR_NO_MEMORY;
+            return;
         }
 
-        handler->ResumeSubscription(*mpCASESessionMgr, subscriptionInfo);
+        handler->ResumeSubscription(*imEngine->mpCASESessionMgr, subscriptionInfo);
     }
     iterator->Release();
 #endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS
-
-    return CHIP_NO_ERROR;
 }
 
 } // namespace app

diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h
@@ -536,6 +536,8 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
     void ShutdownMatchingSubscriptions(const Optional<FabricIndex> & aFabricIndex = NullOptional,
                                        const Optional<NodeId> & aPeerNodeId       = NullOptional);
 
+    static void ResumeSubscriptionsTimerCallback(System::Layer * apSystemLayer, void * apAppState);
+
     template <typename T, size_t N>
     void ReleasePool(ObjectList<T> *& aObjectList, ObjectPool<ObjectList<T>, N> & aObjectPool);
     template <typename T, size_t N>