From 8edbf8097caddac9fc5ec75222be9067c0a2af7b Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Thu, 28 Mar 2024 11:08:49 -0700 Subject: [PATCH] Address review comments --- src/darwin/Framework/CHIP/MTRBaseDevice.h | 5 ++- src/darwin/Framework/CHIP/MTRDevice.mm | 31 +++++++++++-------- .../Framework/CHIP/MTRDevice_Internal.h | 4 +-- .../CHIPTests/MTRPerControllerStorageTests.m | 4 ++- 4 files changed, 26 insertions(+), 18 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRBaseDevice.h b/src/darwin/Framework/CHIP/MTRBaseDevice.h index 5eb00475dfdd8d..fe1592a45fc7b2 100644 --- 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 index 4706a316e4aa25..4659f02edeabeb 100644 --- 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 index 8195a20b8eef08..5d82847f4cbd00 100644 --- 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 index 360d1bbccecab6..fce1ff18b03cf1 100644 --- 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