Darwin: MTRDevice should not persist non-priming attribute values for…

… attributes with Changes Omitted Quality (#33408) * do not cache attribute values for attributes with Changes Omitted Quality * Restyled by whitespace * Restyled by clang-format * manually address feedback that was previously auto-committed * Restyled by clang-format --------- Co-authored-by: Restyled.io <[email protected]>
project-chip · May 16, 2024 · 4c6e8bc · 4c6e8bc
1 parent f980f2c
commit 4c6e8bc
Showing 1 changed file with 23 additions and 8 deletions.
diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm
@@ -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
+{
+    [self _handleAttributeReport:attributeReport fromSubscription:NO];
 }
 
 #ifdef DEBUG
@@ -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];
@@ -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];
     }
@@ -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
@@ -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);
 
@@ -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];
@@ -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) {