-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Darwin] MTRDevice to persist data versions and use for subscription #32153
[Darwin] MTRDevice to persist data versions and use for subscription #32153
Conversation
PR #32153: Size comparison from 8d30071 to b1da9b0 Decreases (1 build for efr32)
Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, stm32, telink)
|
Found an issue myself - when setDelegate is called, all existing cached values should be reported. |
PR #32153: Size comparison from 8d30071 to bb6fd28 Increases (4 builds for esp32, k32w)
Decreases (1 build for efr32)
Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, stm32, telink)
|
// Convert TLV data into data-value dictionary as described in MTRDeviceResponseHandler | ||
NSDictionary<NSString *, id> * _Nullable MTRDecodeDataValueDictionaryFromCHIPTLV(chip::TLV::TLVReader * data) | ||
NSDictionary<NSString *, id> * _Nullable MTRDecodeDataValueDictionaryFromCHIPTLV(chip::TLV::TLVReader * data, NSNumber * dataVersion) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last arg should be annotated nullable, no?
* | ||
* The data-value dictionary also contains this key: | ||
* | ||
* MTRDataVersionKey : NSNumber-wrapped uin32_t. Monotonically increaseing data version for the cluster. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"increasing".
Also, "monotonically increasing" is somewhere between "misleading" and "wrong". We should not claim it's increasing in our API documentation. It can easily decrease.
- (NSDictionary<MTRClusterPath *, NSNumber *> *)_getCachedDataVersions | ||
{ | ||
NSMutableDictionary<MTRClusterPath *, NSNumber *> * dataVersions = [NSMutableDictionary dictionary]; | ||
os_unfair_lock_lock(&self->_lock); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider using std::lock_guard
. See MTRUnfairLock.h
.
// Use the highest data version | ||
if (currentDataVersion.unsignedLongValue < dataVersionNumber.unsignedLongValue) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we clear out the data versions in _readCache when we resubscribe or something? If not, this is broken.
More to the point, we should be tracking data versions for clusters, not individual attributes, precisely because there is no way to tell after the fact which data version value is "newer".
This PR includes: