[ICD]Trigger resubscription when receiving check-in and subscription has not yet become abnormal #37831
+170
−11
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bzbarsky-apple
requested changes
Mar 1, 2025
src/app/ReadClient.cpp
Outdated
| @@ -484,6 +484,7 @@ CHIP_ERROR ReadClient::GenerateDataVersionFilterList(DataVersionFilterIBs::Build | |||
|
|
|||
| void ReadClient::OnActiveModeNotification() | |||
| { | |||
| // Note: this API only works when issuing subscription via SendAutoResubscribeRequest. | |||
There was a problem hiding this comment.
So hold on. When not doing auto-resubscribe, do we want to ignore this call, or still close ourselves even though it's fatal?
There was a problem hiding this comment.
I think the whole OnActiveModeNotification is designed for auto-resubscription stuff, if they are not using OnActiveModeNotification, it is really application consumer's responsibility to decide how to manipulate close and retry logic when receiving check-in message, and application consumer need implement their CheckInDelegate and not call InteracitonModelEngine::OnActiveModeNotification.
I think we should ignore this call when not using auto-resubscription.
There was a problem hiding this comment.
just update code to reflect this change.
src/app/ReadClient.cpp
Outdated
Comment on lines
497
to
498
| // If the server sends out check-in message, and there is no reschedule subscription yet in client side at the same time, it means | ||
| // current client does not realize subscription has gone, and we should forcibly timeout current subscription, and schedule a new one. |
There was a problem hiding this comment.
So the thing is: per spec, the check-in target and the thing whose subscription the server checks for might be different nodes. And in practice this is actually going to happen, I can guarantee that.
So it's entirely possible for this subscription to be just fine and yet for a check-in message to arrive because some other subscription from a different node is not fine.
I assume we're already surfacing check-in messages to the SDK API consumer? If so, they would need to retrigger subscriptions as needed. Or we need some way to configure this behavior, at the very least, because the right thing to do here depends on information this code does not have.
There was a problem hiding this comment.
yes, we have already surfacing check-in messages to the SDK API consumer, it is consumer's reponsibility to retrigger subscription as need, they can mimic OnActiveModeNotification behavior to configure their behavior.
There was a problem hiding this comment.
So the thing is: per spec, the check-in target and the thing whose subscription the server checks for might be different nodes. And in practice this is actually going to happen, I can guarantee that.
If subscription from node A is and good, register node for check-in is for nodeB, check-in message would never arrive at nodeA, right? since server send check-in message based upon registered node.
Unless we allow that multiple nodes reside in same client device?
if yes, maybe we need add one more check between ICDClientInfo::check_in_node against the node id extracted from readClient in InteractionModelEngine:: OnActiveModeNotification? it seems we cannot get mLocalNodeId from SecureSession?
There was a problem hiding this comment.
just updated code to reflect the above change.
There was a problem hiding this comment.
If subscription from node A is and good, register node for check-in is for nodeB, check-in message would never arrive at nodeA, right?
Wrong scenario. The correct scenario is that node A is check-in target and has a subscription but that is NOT the subscription check-in is tracking. Node B is the one the check-in is tracking and that one is dead. Node A will receive the check-in, because Node B is not subscribed, and the right action is to prod Node B (or maybe Node C, depending) in the app, not tear down A's perfectly OK subscription.
Point being: ReadClient does not have the information to make this decision; the application does.
|
PR #37831: Size comparison from 649c341 to cfcf399 Full report (74 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
yunhanw-google
force-pushed
the
feature/30902
branch
from
cfcf399 to
0eed745
Compare
|
PR #37831: Size comparison from 36a1bbd to 0eed745 Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
bzbarsky-apple
requested changes
Mar 10, 2025
bzbarsky-apple
requested changes
Mar 10, 2025
|
PR #37831: Size comparison from 36a1bbd to 180d135 Increases above 0.2%:
Full report (29 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, qpg, stm32, telink, tizen)
|
yunhanw-google
force-pushed
the
feature/30902
branch
from
180d135 to
090d9b7
Compare
|
PR #37831: Size comparison from b56134c to 090d9b7 Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
…ot yet become abnoaml in client
--When sit transition to lit, check-in would be sent and activate the subscription. if any client subscribe to operating mode, and detect it becoems sit, just update the operating mode, there is no need to re-activate the subscription.
If caseTag for current subscription does not match with the one set in ICDManagementCluster::RegisterClient for check-in, when receiving check-in message, OnActiveModeNotification would do nothing for current subscription.
…tion matched with the current subscription
yunhanw-google
force-pushed
the
feature/30902
branch
from
090d9b7 to
6a97e2c
Compare
|
PR #37831: Size comparison from b56134c to 6a97e2c Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
| // called, either mEventPathParamsListSize or mAttributePathParamsListSize is not 0. | ||
| VerifyOrReturn(mReadPrepareParams.mEventPathParamsListSize != 0 || mReadPrepareParams.mAttributePathParamsListSize != 0); | ||
|
|
||
| // If mCatsMatchCheckIn is true, it means cats used in icd registration matches with the one in current subscription, |
There was a problem hiding this comment.
Matches which side? Should be "our side of the current subscription" or something, right?
But do we really want to push this off onto the API client? Or just examine the fabric table?
And if we do push this off onto the API client, as this PR does, then should this boolean just be mResetSubscriptionOnCheckin?
There was a problem hiding this comment.
If we are going with fabrictable path, then we can get fabricTable from interaction model engine, is it controller's fabric table? then use get fabricId via FabricIndex readClient::GetFabricIndex() const { return mPeer.GetFabricIndex(); }, but it is peer's fabric index instead of controller's fabric index, how can we get the controller's fabric index? further we can get cats via FetchCATs(mFabricIndex, cats).
| // If the peer is no longer LIT, try to wake up the subscription and do resubscribe when necessary. | ||
| if (!mIsPeerLIT) | ||
| { | ||
| OnActiveModeNotification(); |
There was a problem hiding this comment.
So I am trying to understand why it's OK to remove this. The reason this seems to be here is that it's envisioning us having two subscriptions, one of which (call it A) is watching for peer type change and one of which (call it B) is IsInactiveICDSubscription(). When the transition to SIT is observed, B will no longer get any OnActiveModeNotifications, so how will it get out of the "inactive ICD subscription" state?
Of course if A does not exist and we just have B, and it's in the "inactive ICD subscription" state and then the server changes to A SIT, I think we just get stuck; that seems to be a hole in the whole logic of this stuff, including the spec?
There was a problem hiding this comment.
Yes, we get stuck, it is also spec issue, the whole scenario is as below
Assume we have one inactive wildcard subscription between controller and ICD device. When ICD transitions from LIT to SIT, check-in message is sent to controller, and controller activate the sleepy subscription, right? then the operation mode attribute is changed, OnPeerTypeChange is called so that OnActiveModeNotification is called again and this subscription would be recreated again with the changes on this PR. Here It seems OnActiveModeNotification is invoked by check-in message and subscribed operation mode attribute change.
Of course if A does not exist and we just have B, and it's in the "inactive ICD subscription" state and then the server changes to A SIT, I think we just get stuck; that seems to be a hole in the whole logic of this stuff, including the spec?
There was a problem hiding this comment.
You are right, the current change does not work on the below scenario
So I am trying to understand why it's OK to remove this. The reason this seems to be here is that it's envisioning us having two subscriptions, one of which (call it A) is watching for peer type change and one of which (call it B) is IsInactiveICDSubscription(). When the transition to SIT is observed, B will no longer get any OnActiveModeNotifications, so how will it get out of the "inactive ICD subscription" state?
Comment on lines
+360
to
+362
| * If the server sends out check-in message, and there is a active tracked active subscription in client side at the same time, | ||
| * it means current client does not realize this tracked subscription has gone, and we should forcibly timeout current | ||
| * subscription, and schedule a new one. |
There was a problem hiding this comment.
This comment does not match what the API actually does, really.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If the server sends out check-in message, and there is no reschedule subscription yet in client side at the same time, it means current client does not realize subscription has gone, and we should forcibly timeout current subscription, and schedule a new one.
In addition, a check-in activates the subscription during the 'lit inactive' to 'sit' transition. For clients subscribed with the operating mode attribute, a transition to 'sit' only requires updating the operating mode, we don't need reactivate the subscription, in this PR, we remove this unnecessary logic in OnPeerTypeChange
fixes #28343
Testing
Drafting