Skip to content
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.

Sign up for GitHub

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 should not persist non-priming attribute values for attributes with Changes Omitted Quality #33408

Merged
merged 5 commits into from
May 16, 2024
Merged

Darwin: MTRDevice should not persist non-priming attribute values for attributes with Changes Omitted Quality #33408

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4a49625
do not cache attribute values for attributes with Changes Omitted Qua…
kiel-apple May 10, 2024
4a49e27
Restyled by whitespace
restyled-commits May 10, 2024
45e4de3
Restyled by clang-format
restyled-commits May 10, 2024
7a1c140
manually address feedback that was previously auto-committed
kiel-apple May 16, 2024
1f3d9bf
Restyled by clang-format
restyled-commits May 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 23 additions & 8 deletions src/darwin/Framework/CHIP/MTRDevice.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1163,12 +1163,17 @@ - (void)_reportAttributes:(NSArray<NSDictionary<NSString *, id> *> *)attributes
}
}

- (void)_handleAttributeReport:(NSArray<NSDictionary<NSString *, id> *> *)attributeReport
- (void)_handleAttributeReport:(NSArray<NSDictionary<NSString *, id> *> *)attributeReport fromSubscription:(BOOL)isFromSubscription
{
std::lock_guard lock(_lock);

// _getAttributesToReportWithReportedValues will log attribute paths reported
[self _reportAttributes:[self _getAttributesToReportWithReportedValues:attributeReport]];
[self _reportAttributes:[self _getAttributesToReportWithReportedValues:attributeReport fromSubscription:isFromSubscription]];
}

- (void)_handleAttributeReport:(NSArray<NSDictionary<NSString *, id> *> *)attributeReport
kiel-apple marked this conversation as resolved.
Show resolved Hide resolved
{
[self _handleAttributeReport:attributeReport fromSubscription:NO];
}

#ifdef DEBUG
Expand Down Expand Up @@ -1378,11 +1383,11 @@ - (MTRDeviceDataValueDictionary _Nullable)_cachedAttributeValueForPath:(MTRAttri
return clusterData.attributes[path.attribute];
}

- (void)_setCachedAttributeValue:(MTRDeviceDataValueDictionary _Nullable)value forPath:(MTRAttributePath *)path
- (void)_setCachedAttributeValue:(MTRDeviceDataValueDictionary _Nullable)value forPath:(MTRAttributePath *)path fromSubscription:(BOOL)isFromSubscription
{
os_unfair_lock_assert_owner(&self->_lock);

// We need an actual MTRClusterPath, not a subsclass, to do _clusterDataForPath.
// We need an actual MTRClusterPath, not a subclass, to do _clusterDataForPath.
auto * clusterPath = [MTRClusterPath clusterPathWithEndpointID:path.endpoint clusterID:path.cluster];

MTRDeviceClusterData * clusterData = [self _clusterDataForPath:clusterPath];
Expand All @@ -1397,6 +1402,16 @@ - (void)_setCachedAttributeValue:(MTRDeviceDataValueDictionary _Nullable)value f

[clusterData storeValue:value forAttribute:path.attribute];

if (value != nil
&& isFromSubscription
&& !_receivingPrimingReport
&& AttributeHasChangesOmittedQuality(path)) {
// Do not persist new values for Changes Omitted Quality attributes unless
// they're part of a Priming Report or from a read response.
// (removals are OK)
return;
}

if (_clusterDataToPersist == nil) {
_clusterDataToPersist = [NSMutableDictionary dictionary];
}
Expand Down Expand Up @@ -1521,7 +1536,7 @@ - (void)_setupSubscription
MTR_LOG_INFO("%@ got attribute report %@", self, value);
dispatch_async(self.queue, ^{
// OnAttributeData
[self _handleAttributeReport:value];
[self _handleAttributeReport:value fromSubscription:YES];
#ifdef DEBUG
self->_unitTestAttributesReportedSinceLastCheck += value.count;
#endif
Expand Down Expand Up @@ -2523,7 +2538,7 @@ - (BOOL)_attributeAffectsDeviceConfiguration:(MTRAttributePath *)attributePath
}

// assume lock is held
- (NSArray *)_getAttributesToReportWithReportedValues:(NSArray<NSDictionary<NSString *, id> *> *)reportedAttributeValues
- (NSArray *)_getAttributesToReportWithReportedValues:(NSArray<NSDictionary<NSString *, id> *> *)reportedAttributeValues fromSubscription:(BOOL)isFromSubscription
{
os_unfair_lock_assert_owner(&self->_lock);

Expand Down Expand Up @@ -2557,7 +2572,7 @@ - (NSArray *)_getAttributesToReportWithReportedValues:(NSArray<NSDictionary<NSSt
_expectedValueCache[attributePath], previousValue);
_expectedValueCache[attributePath] = nil;
// TODO: Is this clearing business really what we want?
[self _setCachedAttributeValue:nil forPath:attributePath];
[self _setCachedAttributeValue:nil forPath:attributePath fromSubscription:isFromSubscription];
} else {
// First separate data version and restore data value to a form without data version
NSNumber * dataVersion = attributeDataValue[MTRDataVersionKey];
Expand All @@ -2575,7 +2590,7 @@ - (NSArray *)_getAttributesToReportWithReportedValues:(NSArray<NSDictionary<NSSt
[self _noteDataVersion:dataVersion forClusterPath:clusterPath];
}

[self _setCachedAttributeValue:attributeDataValue forPath:attributePath];
[self _setCachedAttributeValue:attributeDataValue forPath:attributePath fromSubscription:isFromSubscription];
if (!_deviceConfigurationChanged) {
_deviceConfigurationChanged = [self _attributeAffectsDeviceConfiguration:attributePath];
if (_deviceConfigurationChanged) {
Expand Down
Loading