[ICD]Retrigger subscription immediately when OnActiveModeNotification is invoked

yunhanw-google · yunhanw-google · commit c77099794f11 · 2025-02-12T18:15:09.000-08:00
diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp
@@ -1051,17 +1051,17 @@ void InteractionModelEngine::OnActiveModeNotification(ScopedNodeId aPeer)
     }
 }
 
-void InteractionModelEngine::OnPeerTypeChange(ScopedNodeId aPeer, ReadClient::PeerType aType)
+void InteractionModelEngine::OnPeerOperatingModeChange(ScopedNodeId aPeer, ReadClient::PeerOperatingMode aMode)
 {
     // TODO: Follow up to use a iterator function to avoid copy/paste here.
     for (ReadClient * pListItem = mpActiveReadClientList; pListItem != nullptr;)
     {
-        // It is possible that pListItem is destroyed by the app in OnPeerTypeChange.
-        // Get the next item before invoking `OnPeerTypeChange`.
+        // It is possible that pListItem is destroyed by the app in OnPeerOperatingModeChange.
+        // Get the next item before invoking `OnPeerOperatingModeChange`.
         auto pNextItem = pListItem->GetNextClient();
         if (ScopedNodeId(pListItem->GetPeerNodeId(), pListItem->GetFabricIndex()) == aPeer)
         {
-            pListItem->OnPeerTypeChange(aType);
+            pListItem->OnPeerOperatingModeChange(aMode);
         }
         pListItem = pNextItem;
     }
diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h
@@ -245,12 +245,12 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
     void OnActiveModeNotification(ScopedNodeId aPeer);
 
     /**
-     *  Used to notify when a peer becomes LIT ICD or vice versa.
+     *  Used to notify when a peer operating mode becomes LIT ICD or vice versa.
      *
      *  ReadClient will call this function when it finds any updates of the OperatingMode attribute from ICD management
      * cluster. The application doesn't need to call this function, usually.
      */
-    void OnPeerTypeChange(ScopedNodeId aPeer, ReadClient::PeerType aType);
+    void OnPeerOperatingModeChange(ScopedNodeId aPeer, ReadClient::PeerOperatingMode aMode);
 
     /**
      * Add a read client to the internally tracked list of weak references. This list is used to
diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp
@@ -208,13 +208,11 @@ void ReadClient::Close(CHIP_ERROR aError, bool allowResubscription)
                 aError = mpCallback.OnResubscriptionNeeded(this, aError);
                 if (aError == CHIP_NO_ERROR)
                 {
-                    return;
-                }
-                if (aError == CHIP_ERROR_LIT_SUBSCRIBE_INACTIVE_TIMEOUT)
-                {
-                    VerifyOrDie(originalReason == CHIP_ERROR_LIT_SUBSCRIBE_INACTIVE_TIMEOUT);
-                    ChipLogProgress(DataManagement, "ICD device is inactive mark subscription as InactiveICDSubscription");
-                    MoveToState(ClientState::InactiveICDSubscription);
+                    if (originalReason == CHIP_ERROR_LIT_SUBSCRIBE_INACTIVE_TIMEOUT)
+                    {
+                        ChipLogProgress(DataManagement, "ICD device is unreachable, mark subscription as InactiveICDSubscription, retrying is scheduled");
+                        MoveToState(ClientState::InactiveICDSubscription);
+                    }
                     return;
                 }
             }
@@ -484,26 +482,25 @@ CHIP_ERROR ReadClient::GenerateDataVersionFilterList(DataVersionFilterIBs::Build
 
 void ReadClient::OnActiveModeNotification()
 {
-    // This function just tries to complete the deferred resubscription logic in `OnLivenessTimeoutCallback`.
     VerifyOrDie(mpImEngine->InActiveReadClientList(this));
-    // If we are not in InactiveICDSubscription state, that means the liveness timeout has not been reached. Simply do nothing.
+    // If we are in InactiveICDSubscription state, it means the LIT ICD device was unreachable, and ClearActiveSubscriptionState 
+    // should have been cleared in previous close call, now it transitions to the reachable status, client needs to cancel
+    // scheduled resubscription and trigger a new one immediately. Case session should have been defunct when going 
+    // into InactiveICDSubscription state during previous close call, 
     VerifyOrReturn(IsInactiveICDSubscription());
-
-    // When we reach here, the subscription definitely exceeded the liveness timeout. Just continue the unfinished resubscription
-    // logic in `OnLivenessTimeoutCallback`.
-    TriggerResubscriptionForLivenessTimeout(CHIP_ERROR_TIMEOUT);
+    CloseSession();
+    CancelResubscribeTimer();
+    MoveToState(ClientState::Idle);
+    OnResubscribeTimerCallback(nullptr, this);
 }
 
-void ReadClient::OnPeerTypeChange(PeerType aType)
+void ReadClient::OnPeerOperatingModeChange(PeerOperatingMode aMode)
 {
     VerifyOrDie(mpImEngine->InActiveReadClientList(this));
+    ChipLogProgress(DataManagement, "Peer operation mode is now %s LIT ICD.", mPeerOperatingMode == PeerOperatingMode::kLITICD ? "a" : "not a");
 
-    mIsPeerLIT = (aType == PeerType::kLITICD);
-
-    ChipLogProgress(DataManagement, "Peer is now %s LIT ICD.", mIsPeerLIT ? "a" : "not a");
-
-    // If the peer is no longer LIT, try to wake up the subscription and do resubscribe when necessary.
-    if (!mIsPeerLIT)
+    // If the peer operating mode is no longer LIT, try to wake up the subscription and do resubscribe when necessary.
+    if (mPeerOperatingMode == PeerOperatingMode::kNormal)
     {
         OnActiveModeNotification();
     }
@@ -720,10 +717,18 @@ void ReadClient::OnResponseTimeout(Messaging::ExchangeContext * apExchangeContex
     ChipLogError(DataManagement, "Time out! failed to receive report data from Exchange: " ChipLogFormatExchange,
                  ChipLogValueExchange(apExchangeContext));
 
-    Close(CHIP_ERROR_TIMEOUT);
+    if (IsPeerLIT())
+    {
+        Close(CHIP_ERROR_LIT_SUBSCRIBE_INACTIVE_TIMEOUT);
+    }
+    else
+    {
+        Close(CHIP_ERROR_TIMEOUT);
+    }
+    
 }
 
-CHIP_ERROR ReadClient::ReadICDOperatingModeFromAttributeDataIB(TLV::TLVReader && aReader, PeerType & aType)
+CHIP_ERROR ReadClient::ReadICDOperatingModeFromAttributeDataIB(TLV::TLVReader && aReader, PeerOperatingMode & aMode)
 {
     Clusters::IcdManagement::Attributes::OperatingMode::TypeInfo::DecodableType operatingMode;
 
@@ -733,10 +738,12 @@ CHIP_ERROR ReadClient::ReadICDOperatingModeFromAttributeDataIB(TLV::TLVReader &&
     switch (operatingMode)
     {
     case Clusters::IcdManagement::OperatingModeEnum::kSit:
-        aType = PeerType::kNormal;
+        aMode = PeerOperatingMode::kNormal;
         break;
     case Clusters::IcdManagement::OperatingModeEnum::kLit:
-        aType = PeerType::kLITICD;
+        aMode = PeerOperatingMode::kLITICD;
+        // If operating mode is LIT, this device should be LIT ICD.
+        mIsPeerLIT = true;
         break;
     default:
         err = CHIP_ERROR_INVALID_ARGUMENT;
@@ -843,14 +850,14 @@ CHIP_ERROR ReadClient::ProcessAttributeReportIBs(TLV::TLVReader & aAttributeRepo
             if (attributePath.MatchesConcreteAttributePath(ConcreteAttributePath(
                     kRootEndpointId, Clusters::IcdManagement::Id, Clusters::IcdManagement::Attributes::OperatingMode::Id)))
             {
-                PeerType peerType;
+                PeerOperatingMode peerMode;
                 TLV::TLVReader operatingModeTlvReader;
                 operatingModeTlvReader.Init(dataReader);
-                if (CHIP_NO_ERROR == ReadICDOperatingModeFromAttributeDataIB(std::move(operatingModeTlvReader), peerType))
+                if (CHIP_NO_ERROR == ReadICDOperatingModeFromAttributeDataIB(std::move(operatingModeTlvReader), peerMode))
                 {
-                    // It is safe to call `OnPeerTypeChange` since we are in the middle of parsing the attribute data, And
+                    // It is safe to call `OnPeerOperatingModeChange` since we are in the middle of parsing the attribute data, And
                     // the subscription should be active so `OnActiveModeNotification` is a no-op in this case.
-                    InteractionModelEngine::GetInstance()->OnPeerTypeChange(mPeer, peerType);
+                    InteractionModelEngine::GetInstance()->OnPeerOperatingModeChange(mPeer, peerMode);
                 }
                 else
                 {
@@ -1029,15 +1036,16 @@ void ReadClient::OnLivenessTimeoutCallback(System::Layer * apSystemLayer, void *
                  "Subscription Liveness timeout with SubscriptionID = 0x%08" PRIx32 ", Peer = %02x:" ChipLogFormatX64,
                  _this->mSubscriptionId, _this->GetFabricIndex(), ChipLogValueX64(_this->GetPeerNodeId()));
 
-    if (_this->mIsPeerLIT)
+    if (_this->IsPeerLIT())
     {
         subscriptionTerminationCause = CHIP_ERROR_LIT_SUBSCRIBE_INACTIVE_TIMEOUT;
     }
 
-    _this->TriggerResubscriptionForLivenessTimeout(subscriptionTerminationCause);
+    _this->CloseSession();
+    _this->Close(subscriptionTerminationCause);
 }
 
-void ReadClient::TriggerResubscriptionForLivenessTimeout(CHIP_ERROR aReason)
+void ReadClient::CloseSession()
 {
     // We didn't get a message from the server on time; it's possible that it no
     // longer has a useful CASE session to us.  Mark defunct all sessions that
@@ -1060,8 +1068,6 @@ void ReadClient::TriggerResubscriptionForLivenessTimeout(CHIP_ERROR aReason)
             session->MarkAsDefunct();
         });
     }
-
-    Close(aReason);
 }
 
 CHIP_ERROR ReadClient::ProcessSubscribeResponse(System::PacketBufferHandle && aPayload)
@@ -1265,12 +1271,6 @@ CHIP_ERROR ReadClient::SendSubscribeRequestImpl(const ReadPrepareParams & aReadP
 
 CHIP_ERROR ReadClient::DefaultResubscribePolicy(CHIP_ERROR aTerminationCause)
 {
-    if (aTerminationCause == CHIP_ERROR_LIT_SUBSCRIBE_INACTIVE_TIMEOUT)
-    {
-        ChipLogProgress(DataManagement, "ICD device is inactive, skipping scheduling resubscribe within DefaultResubscribePolicy");
-        return CHIP_ERROR_LIT_SUBSCRIBE_INACTIVE_TIMEOUT;
-    }
-
     VerifyOrReturnError(IsIdle(), CHIP_ERROR_INCORRECT_STATE);
 
     auto timeTillNextResubscription = ComputeTimeTillNextSubscription();
@@ -1305,9 +1305,9 @@ void ReadClient::HandleDeviceConnectionFailure(void * context, const Operational
 {
     ReadClient * const _this = static_cast<ReadClient *>(context);
     VerifyOrDie(_this != nullptr);
-
+    CHIP_ERROR err = failureInfo.error;
     ChipLogError(DataManagement, "Failed to establish CASE for re-subscription with error '%" CHIP_ERROR_FORMAT "'",
-                 failureInfo.error.Format());
+                 err.Format());
 
 #if CHIP_CONFIG_ENABLE_BUSY_HANDLING_FOR_OPERATIONAL_SESSION_SETUP
 #if CHIP_DETAIL_LOGGING
@@ -1322,7 +1322,11 @@ void ReadClient::HandleDeviceConnectionFailure(void * context, const Operational
     _this->mMinimalResubscribeDelay = System::Clock::kZero;
 #endif // CHIP_CONFIG_ENABLE_BUSY_HANDLING_FOR_OPERATIONAL_SESSION_SETUP
 
-    _this->Close(failureInfo.error);
+    if (_this->IsPeerLIT() && err == CHIP_ERROR_TIMEOUT)
+    {
+       err = CHIP_ERROR_LIT_SUBSCRIBE_INACTIVE_TIMEOUT;
+    }
+    _this->Close(err);
 }
 
 void ReadClient::OnResubscribeTimerCallback(System::Layer * /* If this starts being used, fix callers that pass nullptr */,
@@ -1334,7 +1338,6 @@ void ReadClient::OnResubscribeTimerCallback(System::Layer * /* If this starts be
     _this->mIsResubscriptionScheduled = false;
 
     CHIP_ERROR err;
-
     ChipLogProgress(DataManagement, "OnResubscribeTimerCallback: ForceCASE = %d", _this->mForceCaseOnNextResub);
     _this->mNumRetries++;
 
diff --git a/src/app/ReadClient.h b/src/app/ReadClient.h
@@ -165,12 +165,6 @@ class ReadClient : public Messaging::ExchangeDelegate
          * ReadClient::DefaultResubscribePolicy is broken down into its constituent methods that are publicly available for
          * applications to call and sequence.
          *
-         * If the peer is LIT ICD, and the timeout is reached, `aTerminationCause` will be
-         * `CHIP_ERROR_LIT_SUBSCRIBE_INACTIVE_TIMEOUT`. In this case, returning `CHIP_NO_ERROR` will still trigger a resubscribe
-         * attempt, while returning `CHIP_ERROR_LIT_SUBSCRIBE_INACTIVE_TIMEOUT` will put the subscription in the
-         * `InactiveICDSubscription` state.  In the latter case, OnResubscriptionNeeded will be called again when
-         * `OnActiveModeNotification` is called.
-         *
          * If the method is over-ridden, it's the application's responsibility to take the appropriate steps needed to eventually
          * call-back into the ReadClient object to schedule a re-subscription (by invoking ReadClient::ScheduleResubscription).
          *
@@ -292,7 +286,7 @@ class ReadClient : public Messaging::ExchangeDelegate
         Subscribe,
     };
 
-    enum class PeerType : uint8_t
+    enum class PeerOperatingMode : uint8_t
     {
         kNormal,
         kLITICD,
@@ -350,12 +344,12 @@ class ReadClient : public Messaging::ExchangeDelegate
     /**
      *  Re-activate an inactive subscription.
      *
-     *  When subscribing to LIT-ICD and liveness timeout reached and OnResubscriptionNeeded returns
-     * CHIP_ERROR_LIT_SUBSCRIBE_INACTIVE_TIMEOUT, the read client will move to the InactiveICDSubscription state and
-     * resubscription can be triggered via OnActiveModeNotification().
+     *  When subscribing to LIT-ICD and timeout reached, the read client will move to the InactiveICDSubscription state
+     *  and resubscription should have been scheduled by previous close call, resubscription can be triggered immediately via 
+     *  OnActiveModeNotification() when receiving check-in message.
      *
      *  If the subscription is not in the `InactiveICDSubscription` state, this function will do nothing. So it is always safe to
-     * call this function when a check-in message is received.
+     *  call this function when a check-in message is received.
      */
     void OnActiveModeNotification();
 
@@ -503,6 +497,10 @@ class ReadClient : public Messaging::ExchangeDelegate
      */
     Optional<System::Clock::Timeout> GetSubscriptionTimeout();
 
+
+    bool IsPeerLIT() { return mIsPeerLIT; }
+    PeerOperatingMode GetLITOperatingMode() { return mPeerOperatingMode; }
+
 private:
     friend class TestReadInteraction;
     friend class InteractionModelEngine;
@@ -534,13 +532,13 @@ class ReadClient : public Messaging::ExchangeDelegate
     void OnResponseTimeout(Messaging::ExchangeContext * apExchangeContext) override;
 
     /**
-     *  Updates the type (LIT ICD or not) of the peer.
+     *  Updates the operating mode (LIT ICD or not) of the peer.
      *
      *  When the subscription is active, this function will just set the flag. When the subscription is an InactiveICDSubscription,
      * setting the peer type to SIT or normal devices will also trigger a resubscription attempt.
      *
      */
-    void OnPeerTypeChange(PeerType aType);
+    void OnPeerOperatingModeChange(PeerOperatingMode aMode);
 
     /**
      *  Check if current read client is being used
@@ -562,7 +560,7 @@ class ReadClient : public Messaging::ExchangeDelegate
     CHIP_ERROR BuildDataVersionFilterList(DataVersionFilterIBs::Builder & aDataVersionFilterIBsBuilder,
                                           const Span<AttributePathParams> & aAttributePaths,
                                           const Span<DataVersionFilter> & aDataVersionFilters, bool & aEncodedDataVersionList);
-    CHIP_ERROR ReadICDOperatingModeFromAttributeDataIB(TLV::TLVReader && aReader, PeerType & aType);
+    CHIP_ERROR ReadICDOperatingModeFromAttributeDataIB(TLV::TLVReader && aReader, PeerOperatingMode & aMode);
     CHIP_ERROR ProcessAttributeReportIBs(TLV::TLVReader & aAttributeDataIBsReader);
     CHIP_ERROR ProcessEventReportIBs(TLV::TLVReader & aEventReportIBsReader);
 
@@ -572,7 +570,7 @@ class ReadClient : public Messaging::ExchangeDelegate
     CHIP_ERROR ComputeLivenessCheckTimerTimeout(System::Clock::Timeout * aTimeout);
     void CancelLivenessCheckTimer();
     void CancelResubscribeTimer();
-    void TriggerResubscriptionForLivenessTimeout(CHIP_ERROR aReason);
+    void CloseSession();
     void MoveToState(const ClientState aTargetState);
     CHIP_ERROR ProcessAttributePath(AttributePathIB::Parser & aAttributePath, ConcreteDataAttributePath & aClusterInfo);
     CHIP_ERROR ProcessReportData(System::PacketBufferHandle && aPayload, ReportType aReportType);
@@ -667,6 +665,8 @@ class ReadClient : public Messaging::ExchangeDelegate
 
     bool mIsPeerLIT = false;
 
+    PeerOperatingMode mPeerOperatingMode = PeerOperatingMode::kNormal ;
+
     // End Of Container (0x18) uses one byte.
     static constexpr uint16_t kReservedSizeForEndOfContainer = 1;
     // Reserved size for the uint8_t InteractionModelRevision flag, which takes up 1 byte for the control tag and 1 byte for the

Original file line number	Diff line number	Diff line change
`@@ -1051,17 +1051,17 @@ void InteractionModelEngine::OnActiveModeNotification(ScopedNodeId aPeer)`
`1051`	`1051`	`}`
`1052`	`1052`	`}`
`1053`	`1053`
`1054`		`-void InteractionModelEngine::OnPeerTypeChange(ScopedNodeId aPeer, ReadClient::PeerType aType)`
	`1054`	`+void InteractionModelEngine::OnPeerOperatingModeChange(ScopedNodeId aPeer, ReadClient::PeerOperatingMode aMode)`
`1055`	`1055`	`{`
`1056`	`1056`	`// TODO: Follow up to use a iterator function to avoid copy/paste here.`
`1057`	`1057`	`for (ReadClient * pListItem = mpActiveReadClientList; pListItem != nullptr;)`
`1058`	`1058`	`{`
`1059`		`- // It is possible that pListItem is destroyed by the app in OnPeerTypeChange.`
`1060`		- // Get the next item before invoking `OnPeerTypeChange`.
	`1059`	`+ // It is possible that pListItem is destroyed by the app in OnPeerOperatingModeChange.`
	`1060`	+ // Get the next item before invoking `OnPeerOperatingModeChange`.
`1061`	`1061`	`auto pNextItem = pListItem->GetNextClient();`
`1062`	`1062`	`if (ScopedNodeId(pListItem->GetPeerNodeId(), pListItem->GetFabricIndex()) == aPeer)`
`1063`	`1063`	`{`
`1064`		`- pListItem->OnPeerTypeChange(aType);`
	`1064`	`+ pListItem->OnPeerOperatingModeChange(aMode);`
`1065`	`1065`	`}`
`1066`	`1066`	`pListItem = pNextItem;`
`1067`	`1067`	`}`