Introduce a type-safe API for single-attribute subscriptions #12758
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 review from
mrjerryjohns,
yunhanw-google and
vivien-apple
pullapprove
bot
requested review from
andy31415,
anush-apple,
balducci-apple,
carol-apple,
chrisdecenzo,
chulspro,
Damian-Nordic,
electrocucaracha,
franck-apple,
harimau-qirex,
hawk248,
holbrookt,
jelderton,
jepenven-silabs,
jmartinez-silabs,
LuDuda,
lzgrablic02,
msandstedt,
mspang,
pan-apple,
sagar-apple,
saurabhst,
selissia and
tcarmelveilleux
pullapprove
bot
requested review from
tecimovic,
turon,
vijs,
wbschiller,
woody-apple and
yufengwangca
bzbarsky-apple
force-pushed
the
better-single-attr-subscription
branch
from
1160743 to
9c496a2
Compare
yunhanw-google
approved these changes
Dec 8, 2021
|
PR #12758: Size comparison from 4584707 to 9c496a2 Increases above 0.2%:
Increases (3 builds for esp32, linux, mbed)
Decreases (1 build for linux)
Full report (32 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
woody-apple
approved these changes
Dec 9, 2021
bzbarsky-apple
force-pushed
the
better-single-attr-subscription
branch
from
9c496a2 to
8b5ee95
Compare
|
PR #12758: Size comparison from bba082b to 8b5ee95 Increases above 0.2%:
Increases (2 builds for esp32, linux)
Decreases (1 build for linux)
Full report (32 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
|
/rebase |
woody-apple
force-pushed
the
better-single-attr-subscription
branch
from
8b5ee95 to
c480263
Compare
|
PR #12758: Size comparison from 6ea2c90 to c480263 Increases above 0.2%:
Increases (2 builds for esp32, linux)
Decreases (1 build for linux)
Full report (32 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
|
@msandstedt @saurabhst @jmartinez-silabs @jepenven-silabs @LuDuda @Damian-Nordic @chrisdecenzo Please take a look? |
|
fast track: change created and approved by domain owner, up sufficient time for all timezones to have a chance to review. |
mrjerryjohns
suggested changes
Dec 9, 2021
| * The ReadClient object MUST continue to exist after this call is completed. The application shall wait until it | ||
| * receives an OnDone call before it shuts down the object. | ||
| * The ReadClient object MUST continue to exist after this call is completed. The application shall not shut down the | ||
| * object until after this call has returned, and shall not delete the object until OnDone is called. |
There was a problem hiding this comment.
Is this actually true? Why can't I shut down this object at any time during the lifetime of this object, including before the subscription is established? I'd think that's actually possible, since that would terminate the open exchange and release this object back into the pool.
There was a problem hiding this comment.
More-over, ReadClient has still yet to be converted to being application-allocated, so deletion as a concept does not yet apply here.
There was a problem hiding this comment.
That's fair; I think I had lost track of these things being pool allocated (which we want to change) when I was changing the comments here...
I think calling Shutdown "not from inside one of the handlers" is always fine. Doing it from inside the handlers can get us into various corrupted states, though.....
| CHIP_ERROR SubscribeAttribute(void * context, ClusterId clusterId, AttributeId attributeId, | ||
| ReadResponseSuccessCallback<DecodableArgType> reportCb, ReadResponseFailureCallback failureCb, | ||
| uint16_t minIntervalFloorSeconds, uint16_t maxIntervalCeilingSeconds, | ||
| SubscriptionEstablishedCallback subscriptionEstablishedCb = nullptr) |
There was a problem hiding this comment.
How do you terminate a subscription after it's been established with such an API? If you were to repeatedly call this with new subscription requests, would that not leak ReadClient's to the point that subscriptions could not be further established?
There was a problem hiding this comment.
How do you terminate a subscription after it's been established with such an API?
You don't, just like the API it's a replacement for.
The best you can do is create a new subscription to the same server, which kills the first one on the server and then eventually that gets notified and the ReadClient shut down.
That said, we might in fact want to change this API to hand out the ReadClient so you can shut down the subscription. We want to get our ReadClient lifetime/handle story straight first, right?
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.
Problem
Our subscription APIs are not very type-safe and can't represent all Matter types.
Change overview
Introduce an API that subscribes to a single attribute that can do both.
Testing
Modified chip-tool command line to use the new API and tested using that.