[ICD] Server side subscription persistence and resumption #24361
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
|
PR #24361: Size comparison from 18d3671 to 0e003ba Increases above 0.2%:
Increases (13 builds for linux)
Full report (13 builds for linux)
|
|
PR #24361: Size comparison from 18d3671 to a3b8add Increases above 0.2%:
Increases (2 builds for linux)
Full report (2 builds for linux)
|
|
PR #24361: Size comparison from 18d3671 to c6fb866 Increases above 0.2%:
Increases (15 builds for bl602, bl702, k32w, linux, mbed, nrfconnect, qpg)
Full report (15 builds for bl602, bl702, k32w, linux, mbed, nrfconnect, qpg)
|
|
PR #24361: Size comparison from 18d3671 to a82a9d0 Increases above 0.2%:
Increases (15 builds for bl602, bl702, k32w, linux, mbed, nrfconnect, qpg)
Full report (15 builds for bl602, bl702, k32w, linux, mbed, nrfconnect, qpg)
|
|
PR #24361: Size comparison from 18d3671 to 05006c3 Increases above 0.2%:
Increases (38 builds for bl602, bl702, efr32, k32w, linux, mbed, nrfconnect, qpg, telink)
Full report (38 builds for bl602, bl702, efr32, k32w, linux, mbed, nrfconnect, qpg, telink)
|
pullapprove
bot
requested review from
andy31415,
anush-apple,
arkq,
Byungjoo-Lee,
carol-apple,
chrisdecenzo,
chshu and
chulspro
msandstedt
reviewed
Jan 20, 2023
msandstedt
reviewed
Jan 20, 2023
Added setters for SubscriptionInfo attribute and event paths Fixed wrong constant Enabled server interactions for chiptool
msandstedt
approved these changes
Jan 20, 2023
andy31415
dismissed
tcarmelveilleux’s stale review
Dismissing CR from Tennessee - comments addressed, will start clean for another review pass.
andy31415
reviewed
Jan 20, 2023
andy31415
reviewed
Jan 20, 2023
andy31415
reviewed
Jan 20, 2023
andy31415
reviewed
Jan 20, 2023
andy31415
reviewed
Jan 20, 2023
andy31415
reviewed
Jan 20, 2023
andy31415
reviewed
Jan 20, 2023
andy31415
approved these changes
Jan 20, 2023
There was a problem hiding this comment.
Approving based on offline conversation with @jtung-apple that extra comments (mainly GN bits) will be addressed later.
The GN changes would not change logic, just maintainability, hence the checkmark.
Delete() error return clarification Comment doc cleanup Fix loop variable build warning Revert chip-tool server interactions enablement
anush-apple
approved these changes
Jan 21, 2023
|
|
||
| CHIP_ERROR InteractionModelEngine::ResumeSubscriptions() | ||
| { | ||
| #if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS |
There was a problem hiding this comment.
This should probably error out if we have some read handlers allocated already, because that would mean we might create read handlers to represent stored subscriptions that are already represented by an allocated read handler.
And the declaration in the header should document that this is expected to be called during startup, after initing the IM engine?
| } | ||
| } | ||
|
|
||
| // Ask IM engine to start CASE session with subscriber |
There was a problem hiding this comment.
Suggested change
| // Ask IM engine to start CASE session with subscriber | |
| // Start a CASE session with subscriber so we can keep delivering reports. |
We're not using the IM engine for this here, right?
| { | ||
| #if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS | ||
| if (options == CloseOptions::kDropPersistedSubscription) |
There was a problem hiding this comment.
This should probably check that mInteractionType == InteractionType::Subscribe before using the otherwise-probably-not-meaningful mSubscriptionId.
| { | ||
| kDropPersistedSubscription, | ||
| kKeepPersistedSubscription | ||
| }; | ||
| /** | ||
| * Called internally to signal the completion of all work on this objecta and signal to a registered callback that it's |
There was a problem hiding this comment.
Suggested change
| * Called internally to signal the completion of all work on this objecta and signal to a registered callback that it's | |
| * Called internally to signal the completion of all work on this object and signal to a registered callback that it's |
Pre-existing, I know.
| /** | ||
| * Called internally to signal the completion of all work on this objecta and signal to a registered callback that it's | ||
| * safe to release this object. | ||
| * | ||
| * @param options This specifies whether to drop or keep the subscription |
There was a problem hiding this comment.
Should document that this param is ignored for non-subscription ReadHandlers.
| @@ -479,6 +504,19 @@ class Server | |||
| } | |||
| } | |||
|
|
|||
| void ClearSubscriptionResumptionStateOnFabricChange(chip::FabricIndex fabricIndex) | |||
There was a problem hiding this comment.
This is really fabric removal, not fabric change... For CASE it's fabric change because UpdateNOC needs to do the CASE things too, but this bit is removal-specific.
| * Number of iterator instances that can be allocated at any one time | ||
| */ | ||
| #ifndef CHIP_CONFIG_MAX_SUBSCRIPTION_RESUMPTION_STORAGE_CONCURRENT_ITERATORS | ||
| #define CHIP_CONFIG_MAX_SUBSCRIPTION_RESUMPTION_STORAGE_CONCURRENT_ITERATORS 2 |
There was a problem hiding this comment.
Why would we ever have more than one iterator in practice?
| if (pathCount) | ||
| { | ||
| subscriptionInfo.mAttributePaths.Calloc(pathCount); | ||
| ReturnErrorCodeIf(subscriptionInfo.mAttributePaths.Get() == nullptr, CHIP_ERROR_NO_MEMORY); |
There was a problem hiding this comment.
Again, could test either the return value of Calloc, or just `ReturnErrorCodeIf(!subscriptionInfo.mAttributePaths, CHIP_ERROR_NO_MEMORY);
| } | ||
| } | ||
|
|
||
| // if there are no persisted subscriptions, the MaxCount can also be deleted |
There was a problem hiding this comment.
I think it's worth documenting why this is OK to do. We always store it on init. We don't store it on Save(). Why is it ok to clear it on delete?
In particular, if we start, then store a subscription, then delete it, then store another on we will not have a stored max count, afaict. That seems wrong.
| if (fabricIndex == subscriptionInfo.mFabricIndex) | ||
| { | ||
| err = Delete(subscriptionIndex); | ||
| if ((err != CHIP_NO_ERROR) && (err != CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND)) |
There was a problem hiding this comment.
How could we have CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND here, if we just loaded that index above?
| @@ -64,6 +64,9 @@ using CHIP_CONFIG_PERSISTED_STORAGE_KEY_TYPE = const char *; | |||
| #define CHIP_CONFIG_BDX_MAX_NUM_TRANSFERS 1 | |||
| #endif // CHIP_CONFIG_BDX_MAX_NUM_TRANSFERS | |||
|
|
|||
| // Enable subscription persistence and resumption for CI | |||
| #define CHIP_CONFIG_PERSIST_SUBSCRIPTIONS 1 | |||
There was a problem hiding this comment.
This was supposed to be DISABLED by default (except on 1 platform, and Darwin is 1 platform) until spec work made this behavior well-defined at the client-level
lecndav
pushed a commit
to lecndav/connectedhomeip
that referenced
this pull request
Mar 22, 2023
…ip#24361) * [ICD] Server side subscription persistence and resumption * restyled change * Correct SimpleSubscriptionResumptionStorage TLV format documentation * Fix ReadHandler resumption * Correct state size estimate * Replace %zu in log format * Move TLV buffer off stack * restyled * Replaced vector with ScopedMemoryBufferWithSize and shim structs * Fix struct member order * Fix one more struct member order * Fixed more stack buffer * Fix copy/paste bug * Moved SubscriptionList array to unique_ptr and dynamically allocated / off stack * Moved SubscriptionIndex array to unique_ptr and dynamically allocated / off stack * Fixed error condition checks * Fixed array size check * Addressed CI issues, and disabled subscription persistence and resumption for cc13x2_26x2 and CYW30739 * Addressed PR review comments, including: - ReadHandler constructor side effect moved to separate function - SubscriptionList and SubscriptionIndex member initialization moved to runtome - Improved error handling - remove stored info on error - Changed for loop indices to more descriptive names - Disabled feature by default except Linux and Darwin for CI testing - Added operator[] getter with index to access SubscriptionList and SubscriptionIndex elements * Restyled and ReadHandler include * ReadHandler Callback fix * Fix ReadHandler Callback const argument * Explicitly disable subscription persistence and address review comments * Fixed priming reports on resumption * Revamp subscription storage into flat structure and add unit test * Fix unit test build warning and minor PR comment change * Update src/app/SubscriptionResumptionStorage.h Co-authored-by: Michael Sandstedt <[email protected]> * Minor changes to address PR comments * Address PR review comments: Unit test structs constructed explicitly in place for clarity IM engine ResumeSubscriptions nullptr check and exit conditions fix * Address PR comments: Nullptr checks Minor refactor Unit test fix * Changed storage MaxCount mechanics to Init time clean up * Clean up comments and unused commented-out old code * Addressed PR comments: Removed AllocatedCount, and made AllocatedSize return count of elements * Update src/app/SimpleSubscriptionResumptionStorage.cpp Co-authored-by: Michael Sandstedt <[email protected]> * Update src/app/InteractionModelEngine.cpp Co-authored-by: Michael Sandstedt <[email protected]> * Remove reference to previously removed variable for config that turns the feature off * Addressed PR comments and enabled chip-tool for testing Added setters for SubscriptionInfo attribute and event paths Fixed wrong constant Enabled server interactions for chiptool * Changed storage of attribute/event paths to proper List/Structure TLV * Fixed attribute load * Make Unit Test names more unique and tighten CHIP_CONFIG_PERSIST_SUBSCRIPTIONS usage * Addressed PR comments and CI issues: Delete() error return clarification Comment doc cleanup Fix loop variable build warning Revert chip-tool server interactions enablement Co-authored-by: Michael Sandstedt <[email protected]>
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.
Initial implementation for server side subscription persistence and resumption