[Darwin] MTRDevice should persist data version by cluster #32674
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
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
harsha-rajendran,
hawk248,
hicklin,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy,
joonhaengHeo,
kkasperczyk-no,
kpschoedel,
ksperling-apple,
lazarkov,
lpbeliveau-silabs,
LuDuda,
mhazley,
mkardous-silabs and
mspang
pullapprove
bot
requested review from
nivi-apple,
p0fi,
pidarped,
plauric,
rcasallas-silabs,
robszewczyk,
saurabhst,
selissia,
sharadb-amazon,
tcarmelveilleux,
tecimovic,
tehampson,
tima-q,
tobiasgraf,
turon,
vivien-apple,
wiba-nordic,
woody-apple,
younghak-hwang,
yufengwangca and
yunhanw-google
woody-apple
approved these changes
Mar 21, 2024
| // Attribute data-value dictionary should be all standard containers which | ||
| // means isEqual: comparisons all the way down, making a deep comparison. | ||
| return [one isEqualToDictionary:theOther]; | ||
| // Attribute data-value dictionaries are equal if type and value are equal |
There was a problem hiding this comment.
I know they weren’t here before, but the lack of nil checks, are anxiety inducing
| // Utility to return data value dictionary without data version | ||
| - (NSDictionary *)_dataValueWithoutDataVersion:(NSDictionary *)attributeValue; | ||
| { | ||
| if (attributeValue[MTRValueKey]) { |
There was a problem hiding this comment.
Same as above, nil checks…
| @@ -2013,6 +2107,11 @@ - (void)setAttributeValues:(NSArray<NSDictionary *> *)attributeValues reportChan | |||
| } | |||
| } | |||
|
|
|||
| - (void)setClusterData:(NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> *)clusterData | |||
| { | |||
| [_clusterData addEntriesFromDictionary:clusterData]; | |||
|
|
||
| - (NSString *)_clusterDataKeyForNodeID:(NSNumber *)nodeID endpointID:(NSNumber *)endpointID clusterID:(NSNumber *)clusterID | ||
| { | ||
| return [sAttributeCacheClusterDataKeyPrefix stringByAppendingFormat:@":0x%016llX:%0x04X:0x%08lX", nodeID.unsignedLongLongValue, endpointID.unsignedShortValue, clusterID.unsignedLongValue]; |
|
|
||
| - (nullable MTRDeviceClusterData *)_fetchClusterDataForNodeID:(NSNumber *)nodeID endpointID:(NSNumber *)endpointID clusterID:(NSNumber *)clusterID | ||
| { | ||
| return [self _fetchAttributeCacheValueForKey:[self _clusterDataKeyForNodeID:nodeID endpointID:endpointID clusterID:clusterID] expectedClass:[MTRDeviceClusterData class]]; |
There was a problem hiding this comment.
Same for all these new accessors
| @@ -809,6 +847,140 @@ - (void)clearAllStoredAttributes | |||
| }); | |||
| } | |||
|
|
|||
| - (nullable NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> *)getStoredClusterDataForNodeID:(NSNumber *)nodeID | |||
| { | |||
| __block NSMutableDictionary<MTRClusterPath *, MTRDeviceClusterData *> * clusterDataToReturn = nil; | |||
|
|
||
| - (void)storeClusterData:(NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> *)clusterData forNodeID:(NSNumber *)nodeID | ||
| { | ||
| dispatch_async(_storageDelegateQueue, ^{ |
|
PR #32674: Size comparison from 1f2aff4 to c71a826 Full report (41 builds for cc13x4_26x4, cc32xx, cyw30739, k32w, nrfconnect, psoc6, qpg, stm32, telink)
|
| // means isEqual: comparisons all the way down, making a deep comparison. | ||
| return [one isEqualToDictionary:theOther]; | ||
| // Attribute data-value dictionaries are equal if type and value are equal | ||
| return [one[MTRTypeKey] isEqual:theOther[MTRTypeKey]] && ((one[MTRValueKey] == theOther[MTRValueKey]) || [one[MTRValueKey] isEqual:theOther[MTRValueKey]]); |
There was a problem hiding this comment.
This boolean condition could at least use some comments. Is one[MTRValueKey] == theOther[MTRValueKey] supposed to catch the "they are both nil" case? If so that should definitely be documented, because it's super non-obvious.
There was a problem hiding this comment.
Will document in the next PR.
| return [one[MTRTypeKey] isEqual:theOther[MTRTypeKey]] && ((one[MTRValueKey] == theOther[MTRValueKey]) || [one[MTRValueKey] isEqual:theOther[MTRValueKey]]); | ||
| } | ||
|
|
||
| // Utility to return data value dictionary without data version |
There was a problem hiding this comment.
So this assumes that the incoming dictionary only has type, value (maybe), and data version keys, right? Wouldn't it be safer to go over all keys and add them all to the return dictionary, while skipping the data version key?
| #endif | ||
|
|
||
| for (NSNumber * endpointID in endpointIndex) { | ||
| // Fetch endpoint index |
There was a problem hiding this comment.
This is not fetching the endpoint index...
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.
The initial implementation for the attribute cache storage made a mistaken assumption about how data versions work. This change moves the data version stored from by-attribute to by-cluster.