Address review comments

project-chip · Mar 28, 2024 · 8edbf80 · 8edbf80
1 parent cc80b77
commit 8edbf80
Show file tree

Hide file tree

Showing 4 changed files with 26 additions and 18 deletions.
diff --git a/src/darwin/Framework/CHIP/MTRBaseDevice.h b/src/darwin/Framework/CHIP/MTRBaseDevice.h
@@ -26,6 +26,9 @@
 
 NS_ASSUME_NONNULL_BEGIN
 
+typedef NSDictionary<NSString *, id> * MTRDeviceResponseValueDictionary;
+typedef NSDictionary<NSString *, id> * MTRDeviceDataValueDictionary;
+
 /**
  * Handler for read attribute response, write attribute response, invoke command response and reports.
  *
@@ -96,7 +99,7 @@ NS_ASSUME_NONNULL_BEGIN
  *
  *                MTRDataKey : Data-value NSDictionary object.
  */
-typedef void (^MTRDeviceResponseHandler)(NSArray<NSDictionary<NSString *, id> *> * _Nullable values, NSError * _Nullable error);
+typedef void (^MTRDeviceResponseHandler)(NSArray<MTRDeviceResponseValueDictionary> * _Nullable values, NSError * _Nullable error);
 
 /**
  * Handler for -subscribeWithQueue: attribute and event reports

diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm
@@ -165,10 +165,10 @@ + (BOOL)supportsSecureCoding
 
 - (NSString *)description
 {
-    return [NSString stringWithFormat:@"<MTRDeviceClusterData: dataVersion %@>", _dataVersion];
+    return [NSString stringWithFormat:@"<MTRDeviceClusterData: dataVersion %@ attributes count %lu>", _dataVersion, static_cast<unsigned long>(_attributes.count)];
 }
 
-- (nullable instancetype)initWithDataVersion:(NSNumber * _Nullable)dataVersion attributes:(NSDictionary * _Nullable)attributes
+- (nullable instancetype)initWithDataVersion:(NSNumber * _Nullable)dataVersion attributes:(NSDictionary<NSNumber *, MTRDeviceDataValueDictionary> * _Nullable)attributes
 {
     self = [super init];
     if (self == nil) {
@@ -840,9 +840,9 @@ - (void)_handleReportBegin
     }
 }
 
-// assume lock is held
-- (NSDictionary<NSNumber *, NSDictionary *> *)_attributesForCluster:(MTRClusterPath *)clusterPath
+- (NSDictionary<NSNumber *, MTRDeviceDataValueDictionary> *)_attributesForCluster:(MTRClusterPath *)clusterPath
 {
+    os_unfair_lock_assert_owner(&self->_lock);
     NSMutableDictionary * attributesToReturn = [NSMutableDictionary dictionary];
     for (MTRAttributePath * attributePath in _readCache) {
         if ([attributePath.endpoint isEqualToNumber:clusterPath.endpoint] && [attributePath.cluster isEqualToNumber:clusterPath.cluster]) {
@@ -852,13 +852,13 @@ - (void)_handleReportBegin
     return attributesToReturn;
 }
 
-// assume lock is held
 - (NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> *)_clusterDataForPaths:(NSSet<MTRClusterPath *> *)clusterPaths
 {
+    os_unfair_lock_assert_owner(&self->_lock);
     NSMutableDictionary * clusterDataToReturn = [NSMutableDictionary dictionary];
     for (MTRClusterPath * clusterPath in clusterPaths) {
         NSNumber * dataVersion = _clusterData[clusterPath].dataVersion;
-        NSDictionary<NSNumber *, NSDictionary *> * attributes = [self _attributesForCluster:clusterPath];
+        NSDictionary<NSNumber *, MTRDeviceDataValueDictionary> * attributes = [self _attributesForCluster:clusterPath];
         MTRDeviceClusterData * clusterData = [[MTRDeviceClusterData alloc] initWithDataVersion:dataVersion attributes:attributes];
         clusterDataToReturn[clusterPath] = clusterData;
     }
@@ -1973,9 +1973,11 @@ - (BOOL)_attributeDataValue:(NSDictionary *)one isEqualToDataValue:(NSDictionary
 {
     // Sanity check for nil cases
     if (!one && !theOther) {
+        MTR_LOG_ERROR("%@ attribute data-value comparison does not expect comparing two nil dictionaries", self);
         return YES;
     }
-    if ((!one && theOther) || (one && !theOther)) {
+    if (!one || !theOther) {
+        MTR_LOG_ERROR("%@ attribute data-value comparison does not expect a dictionary to be nil: %@ %@", self, one, theOther);
         return NO;
     }
 
@@ -2001,6 +2003,8 @@ - (NSDictionary *)_dataValueWithoutDataVersion:(NSDictionary *)attributeValue;
 // Update cluster data version and also note the change, so at onReportEnd it can be persisted
 - (void)_noteDataVersion:(NSNumber *)dataVersion forClusterPath:(MTRClusterPath *)clusterPath
 {
+    os_unfair_lock_assert_owner(&self->_lock);
+
     BOOL dataVersionChanged = NO;
     // Update data version used for subscription filtering
     MTRDeviceClusterData * clusterData = _clusterData[clusterPath];
@@ -2018,17 +2022,16 @@ - (void)_noteDataVersion:(NSNumber *)dataVersion forClusterPath:(MTRClusterPath
         // Mark cluster path as needing persistence if needed
         BOOL dataStoreExists = _deviceController.controllerDataStore != nil;
         if (dataStoreExists) {
-            if (!_clustersToPersist) {
-                _clustersToPersist = [NSMutableSet set];
-            }
-            [_clustersToPersist addObject:clusterPath];
+            [self _noteChangeForClusterPath:clusterPath];
         }
     }
 }
 
 // Assuming data store exists, note that the cluster should be persisted at onReportEnd
-- (void)_noteAttributeChangedForClusterPath:(MTRClusterPath *)clusterPath
+- (void)_noteChangeForClusterPath:(MTRClusterPath *)clusterPath
 {
+    os_unfair_lock_assert_owner(&self->_lock);
+
     if (!_clustersToPersist) {
         _clustersToPersist = [NSMutableSet set];
     }
@@ -2092,7 +2095,7 @@ - (NSArray *)_getAttributesToReportWithReportedValues:(NSArray<NSDictionary<NSSt
             // Check if attribute needs to be persisted - compare only to read cache and disregard expected values
             if (dataStoreExists && readCacheValueChanged) {
 #if MTRDEVICE_ATTRIBUTE_CACHE_STORE_ATTRIBUTES_BY_CLUSTER
-                [self _noteAttributeChangedForClusterPath:clusterPath];
+                [self _noteChangeForClusterPath:clusterPath];
 #else
                 NSDictionary * attributeResponseValueToPersist;
                 if (dataVersion) {
@@ -2178,6 +2181,8 @@ - (NSArray *)_getAttributesToReportWithReportedValues:(NSArray<NSDictionary<NSSt
 
 - (void)_setAttributeValues:(NSArray<NSDictionary *> *)attributeValues reportChanges:(BOOL)reportChanges
 {
+    os_unfair_lock_assert_owner(&self->_lock);
+
     if (!attributeValues.count) {
         return;
     }

diff --git a/src/darwin/Framework/CHIP/MTRDevice_Internal.h b/src/darwin/Framework/CHIP/MTRDevice_Internal.h
@@ -37,9 +37,7 @@ typedef void (^MTRDevicePerformAsyncBlock)(MTRBaseDevice * baseDevice);
 MTR_TESTABLE
 @interface MTRDeviceClusterData : NSObject <NSSecureCoding, NSCopying>
 @property (nonatomic) NSNumber * dataVersion;
-#if MTRDEVICE_ATTRIBUTE_CACHE_STORE_ATTRIBUTES_BY_CLUSTER
-@property (nonatomic) NSDictionary<NSNumber *, NSDictionary *> * attributes; // attributeID => data-value dictionary
-#endif
+@property (nonatomic) NSDictionary<NSNumber *, MTRDeviceDataValueDictionary> * attributes; // attributeID => data-value dictionary
 @end
 
 @interface MTRDevice ()

diff --git a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m
@@ -1289,7 +1289,9 @@ - (void)test009_TestDataStoreMTRDevice
         for (NSNumber * attributeID in data.attributes) {
             NSDictionary * dataValue = data.attributes[attributeID];
             NSDictionary * dataValueFromMTRDevice = [newDevice readAttributeWithEndpointID:path.endpoint clusterID:path.cluster attributeID:attributeID params:nil];
-            XCTAssertTrue([newDevice _attributeDataValue:dataValue isEqualToDataValue:dataValueFromMTRDevice]);
+            if (![newDevice _attributeDataValue:dataValue isEqualToDataValue:dataValueFromMTRDevice]) {
+                storedAttributeDifferFromMTRDeviceCount++;
+            }
         }
     }
 #else