Skip to content

Commit 68ebd09

Browse files
jtung-applejtung-apple
authored
[Darwin] MTRDevice attribute storage should store values by cluster (#32765)
* [Darwin] MTRDevice attribute storage should store values by cluster * Address review comments * Only persist cluster data when there is something to persist
1 parent 3559bdc commit 68ebd09

8 files changed

+262
-29
lines changed

‎src/darwin/Framework/CHIP/MTRBaseDevice.h

+4-1
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,9 @@
2626

2727
NS_ASSUME_NONNULL_BEGIN
2828

29+
typedef NSDictionary<NSString *, id> * MTRDeviceResponseValueDictionary;
30+
typedef NSDictionary<NSString *, id> * MTRDeviceDataValueDictionary;
31+
2932
/**
3033
* Handler for read attribute response, write attribute response, invoke command response and reports.
3134
*
@@ -96,7 +99,7 @@ NS_ASSUME_NONNULL_BEGIN
9699
*
97100
* MTRDataKey : Data-value NSDictionary object.
98101
*/
99-
typedef void (^MTRDeviceResponseHandler)(NSArray<NSDictionary<NSString *, id> *> * _Nullable values, NSError * _Nullable error);
102+
typedef void (^MTRDeviceResponseHandler)(NSArray<MTRDeviceResponseValueDictionary> * _Nullable values, NSError * _Nullable error);
100103

101104
/**
102105
* Handler for -subscribeWithQueue: attribute and event reports

‎src/darwin/Framework/CHIP/MTRDevice.mm

+148-18
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,7 @@ typedef NS_ENUM(NSUInteger, MTRDeviceWorkItemDuplicateTypeID) {
176176
@implementation MTRDeviceClusterData
177177

178178
static NSString * const sDataVersionKey = @"dataVersion";
179+
static NSString * const sAttributesKey = @"attributes";
179180

180181
+ (BOOL)supportsSecureCoding
181182
{
@@ -184,7 +185,20 @@ + (BOOL)supportsSecureCoding
184185

185186
- (NSString *)description
186187
{
187-
return [NSString stringWithFormat:@"<MTRDeviceClusterData: dataVersion %@>", _dataVersion];
188+
return [NSString stringWithFormat:@"<MTRDeviceClusterData: dataVersion %@ attributes count %lu>", _dataVersion, static_cast<unsigned long>(_attributes.count)];
189+
}
190+
191+
- (nullable instancetype)initWithDataVersion:(NSNumber * _Nullable)dataVersion attributes:(NSDictionary<NSNumber *, MTRDeviceDataValueDictionary> * _Nullable)attributes
192+
{
193+
self = [super init];
194+
if (self == nil) {
195+
return nil;
196+
}
197+
198+
_dataVersion = [dataVersion copy];
199+
_attributes = [attributes copy];
200+
201+
return self;
188202
}
189203

190204
- (nullable instancetype)initWithCoder:(NSCoder *)decoder
@@ -200,12 +214,25 @@ - (nullable instancetype)initWithCoder:(NSCoder *)decoder
200214
return nil;
201215
}
202216

217+
static NSSet * const sAttributeValueClasses = [NSSet setWithObjects:[NSDictionary class], [NSArray class], [NSData class], [NSString class], [NSNumber class], nil];
218+
_attributes = [decoder decodeObjectOfClasses:sAttributeValueClasses forKey:sAttributesKey];
219+
if (_attributes != nil && ![_attributes isKindOfClass:[NSDictionary class]]) {
220+
MTR_LOG_ERROR("MTRDeviceClusterData got %@ for attributes, not NSDictionary.", _attributes);
221+
return nil;
222+
}
223+
203224
return self;
204225
}
205226

206227
- (void)encodeWithCoder:(NSCoder *)coder
207228
{
208229
[coder encodeObject:self.dataVersion forKey:sDataVersionKey];
230+
[coder encodeObject:self.attributes forKey:sAttributesKey];
231+
}
232+
233+
- (id)copyWithZone:(NSZone *)zone
234+
{
235+
return [[MTRDeviceClusterData alloc] initWithDataVersion:_dataVersion attributes:_attributes];
209236
}
210237

211238
@end
@@ -285,8 +312,11 @@ @implementation MTRDevice {
285312
NSUInteger _unitTestAttributesReportedSinceLastCheck;
286313
#endif
287314
BOOL _delegateDeviceCachePrimedCalled;
315+
316+
// With MTRDeviceClusterData now able to hold attribute data, the plan is to move to using it
317+
// as the read cache, should testing prove attribute storage by cluster is the better solution.
288318
NSMutableDictionary<MTRClusterPath *, MTRDeviceClusterData *> * _clusterData;
289-
NSMutableDictionary<MTRClusterPath *, MTRDeviceClusterData *> * _clusterDataToPersist;
319+
NSMutableSet<MTRClusterPath *> * _clustersToPersist;
290320
}
291321

292322
- (instancetype)initWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceController *)controller
@@ -836,6 +866,34 @@ - (void)_handleReportBegin
836866
}
837867
}
838868

869+
- (NSDictionary<NSNumber *, MTRDeviceDataValueDictionary> *)_attributesForCluster:(MTRClusterPath *)clusterPath
870+
{
871+
os_unfair_lock_assert_owner(&self->_lock);
872+
NSMutableDictionary * attributesToReturn = [NSMutableDictionary dictionary];
873+
for (MTRAttributePath * attributePath in _readCache) {
874+
if ([attributePath.endpoint isEqualToNumber:clusterPath.endpoint] && [attributePath.cluster isEqualToNumber:clusterPath.cluster]) {
875+
attributesToReturn[attributePath.attribute] = _readCache[attributePath];
876+
}
877+
}
878+
return attributesToReturn;
879+
}
880+
881+
- (NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> *)_clusterDataForPaths:(NSSet<MTRClusterPath *> *)clusterPaths
882+
{
883+
os_unfair_lock_assert_owner(&self->_lock);
884+
NSMutableDictionary * clusterDataToReturn = [NSMutableDictionary dictionary];
885+
for (MTRClusterPath * clusterPath in clusterPaths) {
886+
NSNumber * dataVersion = _clusterData[clusterPath].dataVersion;
887+
NSDictionary<NSNumber *, MTRDeviceDataValueDictionary> * attributes = [self _attributesForCluster:clusterPath];
888+
if (dataVersion || attributes) {
889+
MTRDeviceClusterData * clusterData = [[MTRDeviceClusterData alloc] initWithDataVersion:dataVersion attributes:attributes];
890+
clusterDataToReturn[clusterPath] = clusterData;
891+
}
892+
}
893+
894+
return clusterDataToReturn;
895+
}
896+
839897
- (void)_handleReportEnd
840898
{
841899
std::lock_guard lock(_lock);
@@ -844,10 +902,11 @@ - (void)_handleReportEnd
844902
_estimatedStartTimeFromGeneralDiagnosticsUpTime = nil;
845903

846904
BOOL dataStoreExists = _deviceController.controllerDataStore != nil;
847-
if (dataStoreExists && _clusterDataToPersist.count) {
848-
MTR_LOG_DEFAULT("%@ Storing cluster information (data version) count: %lu", self, static_cast<unsigned long>(_clusterDataToPersist.count));
849-
[_deviceController.controllerDataStore storeClusterData:_clusterDataToPersist forNodeID:_nodeID];
850-
_clusterDataToPersist = nil;
905+
if (dataStoreExists && _clustersToPersist.count) {
906+
MTR_LOG_DEFAULT("%@ Storing cluster information (data version) count: %lu", self, static_cast<unsigned long>(_clustersToPersist.count));
907+
NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> * clusterData = [self _clusterDataForPaths:_clustersToPersist];
908+
[_deviceController.controllerDataStore storeClusterData:clusterData forNodeID:_nodeID];
909+
_clustersToPersist = nil;
851910
}
852911

853912
// For unit testing only
@@ -1939,13 +1998,28 @@ - (void)_performScheduledExpirationCheck
19391998

19401999
- (BOOL)_attributeDataValue:(NSDictionary *)one isEqualToDataValue:(NSDictionary *)theOther
19412000
{
1942-
// Attribute data-value dictionaries are equal if type and value are equal
2001+
// Sanity check for nil cases
2002+
if (!one && !theOther) {
2003+
MTR_LOG_ERROR("%@ attribute data-value comparison does not expect comparing two nil dictionaries", self);
2004+
return YES;
2005+
}
2006+
if (!one || !theOther) {
2007+
MTR_LOG_ERROR("%@ attribute data-value comparison does not expect a dictionary to be nil: %@ %@", self, one, theOther);
2008+
return NO;
2009+
}
2010+
2011+
// Attribute data-value dictionaries are equal if type and value are equal, and specifically, this should return true if values are both nil
19432012
return [one[MTRTypeKey] isEqual:theOther[MTRTypeKey]] && ((one[MTRValueKey] == theOther[MTRValueKey]) || [one[MTRValueKey] isEqual:theOther[MTRValueKey]]);
19442013
}
19452014

19462015
// Utility to return data value dictionary without data version
19472016
- (NSDictionary *)_dataValueWithoutDataVersion:(NSDictionary *)attributeValue;
19482017
{
2018+
// Sanity check for nil - return the same input to fail gracefully
2019+
if (!attributeValue || !attributeValue[MTRTypeKey]) {
2020+
return attributeValue;
2021+
}
2022+
19492023
if (attributeValue[MTRValueKey]) {
19502024
return @{ MTRTypeKey : attributeValue[MTRTypeKey], MTRValueKey : attributeValue[MTRValueKey] };
19512025
} else {
@@ -1954,9 +2028,12 @@ - (NSDictionary *)_dataValueWithoutDataVersion:(NSDictionary *)attributeValue;
19542028
}
19552029

19562030
// Update cluster data version and also note the change, so at onReportEnd it can be persisted
1957-
- (void)_updateDataVersion:(NSNumber *)dataVersion forClusterPath:(MTRClusterPath *)clusterPath
2031+
- (void)_noteDataVersion:(NSNumber *)dataVersion forClusterPath:(MTRClusterPath *)clusterPath
19582032
{
2033+
os_unfair_lock_assert_owner(&self->_lock);
2034+
19592035
BOOL dataVersionChanged = NO;
2036+
// Update data version used for subscription filtering
19602037
MTRDeviceClusterData * clusterData = _clusterData[clusterPath];
19612038
if (!clusterData) {
19622039
clusterData = [[MTRDeviceClusterData alloc] init];
@@ -1969,17 +2046,25 @@ - (void)_updateDataVersion:(NSNumber *)dataVersion forClusterPath:(MTRClusterPat
19692046
if (dataVersionChanged) {
19702047
clusterData.dataVersion = dataVersion;
19712048

1972-
// Set up for persisting if there is data store
2049+
// Mark cluster path as needing persistence if needed
19732050
BOOL dataStoreExists = _deviceController.controllerDataStore != nil;
19742051
if (dataStoreExists) {
1975-
if (!_clusterDataToPersist) {
1976-
_clusterDataToPersist = [NSMutableDictionary dictionary];
1977-
}
1978-
_clusterDataToPersist[clusterPath] = clusterData;
2052+
[self _noteChangeForClusterPath:clusterPath];
19792053
}
19802054
}
19812055
}
19822056

2057+
// Assuming data store exists, note that the cluster should be persisted at onReportEnd
2058+
- (void)_noteChangeForClusterPath:(MTRClusterPath *)clusterPath
2059+
{
2060+
os_unfair_lock_assert_owner(&self->_lock);
2061+
2062+
if (!_clustersToPersist) {
2063+
_clustersToPersist = [NSMutableSet set];
2064+
}
2065+
[_clustersToPersist addObject:clusterPath];
2066+
}
2067+
19832068
// assume lock is held
19842069
- (NSArray *)_getAttributesToReportWithReportedValues:(NSArray<NSDictionary<NSString *, id> *> *)reportedAttributeValues
19852070
{
@@ -1988,10 +2073,12 @@ - (NSArray *)_getAttributesToReportWithReportedValues:(NSArray<NSDictionary<NSSt
19882073
NSMutableArray * attributesToReport = [NSMutableArray array];
19892074
NSMutableArray * attributePathsToReport = [NSMutableArray array];
19902075
BOOL dataStoreExists = _deviceController.controllerDataStore != nil;
2076+
#if !MTRDEVICE_ATTRIBUTE_CACHE_STORE_ATTRIBUTES_BY_CLUSTER
19912077
NSMutableArray * attributesToPersist;
19922078
if (dataStoreExists) {
19932079
attributesToPersist = [NSMutableArray array];
19942080
}
2081+
#endif
19952082
for (NSDictionary<NSString *, id> * attributeResponseValue in reportedAttributeValues) {
19962083
MTRAttributePath * attributePath = attributeResponseValue[MTRAttributePathKey];
19972084
NSDictionary * attributeDataValue = attributeResponseValue[MTRDataKey];
@@ -2023,9 +2110,9 @@ - (NSArray *)_getAttributesToReportWithReportedValues:(NSArray<NSDictionary<NSSt
20232110
} else {
20242111
// First separate data version and restore data value to a form without data version
20252112
NSNumber * dataVersion = attributeDataValue[MTRDataVersionKey];
2113+
MTRClusterPath * clusterPath = [MTRClusterPath clusterPathWithEndpointID:attributePath.endpoint clusterID:attributePath.cluster];
20262114
if (dataVersion) {
2027-
MTRClusterPath * clusterPath = [MTRClusterPath clusterPathWithEndpointID:attributePath.endpoint clusterID:attributePath.cluster];
2028-
[self _updateDataVersion:dataVersion forClusterPath:clusterPath];
2115+
[self _noteDataVersion:dataVersion forClusterPath:clusterPath];
20292116

20302117
// Remove data version from what we cache in memory
20312118
attributeDataValue = [self _dataValueWithoutDataVersion:attributeDataValue];
@@ -2034,6 +2121,9 @@ - (NSArray *)_getAttributesToReportWithReportedValues:(NSArray<NSDictionary<NSSt
20342121
BOOL readCacheValueChanged = ![self _attributeDataValue:attributeDataValue isEqualToDataValue:_readCache[attributePath]];
20352122
// Check if attribute needs to be persisted - compare only to read cache and disregard expected values
20362123
if (dataStoreExists && readCacheValueChanged) {
2124+
#if MTRDEVICE_ATTRIBUTE_CACHE_STORE_ATTRIBUTES_BY_CLUSTER
2125+
[self _noteChangeForClusterPath:clusterPath];
2126+
#else
20372127
NSDictionary * attributeResponseValueToPersist;
20382128
if (dataVersion) {
20392129
// Remove data version from what we cache in memory and storage
@@ -2044,6 +2134,7 @@ - (NSArray *)_getAttributesToReportWithReportedValues:(NSArray<NSDictionary<NSSt
20442134
attributeResponseValueToPersist = attributeResponseValue;
20452135
}
20462136
[attributesToPersist addObject:attributeResponseValueToPersist];
2137+
#endif
20472138
}
20482139

20492140
// if expected values exists, purge and update read cache
@@ -2106,17 +2197,22 @@ - (NSArray *)_getAttributesToReportWithReportedValues:(NSArray<NSDictionary<NSSt
21062197

21072198
MTR_LOG_INFO("%@ report from reported values %@", self, attributePathsToReport);
21082199

2200+
#if !MTRDEVICE_ATTRIBUTE_CACHE_STORE_ATTRIBUTES_BY_CLUSTER
21092201
if (dataStoreExists && attributesToPersist.count) {
21102202
[_deviceController.controllerDataStore storeAttributeValues:attributesToPersist forNodeID:_nodeID];
21112203
}
2204+
#endif
21122205

21132206
return attributesToReport;
21142207
}
21152208

2116-
- (void)setAttributeValues:(NSArray<NSDictionary *> *)attributeValues reportChanges:(BOOL)reportChanges
2209+
- (void)_setAttributeValues:(NSArray<NSDictionary *> *)attributeValues reportChanges:(BOOL)reportChanges
21172210
{
2118-
MTR_LOG_INFO("%@ setAttributeValues count: %lu reportChanges: %d", self, static_cast<unsigned long>(attributeValues.count), reportChanges);
2119-
std::lock_guard lock(_lock);
2211+
os_unfair_lock_assert_owner(&self->_lock);
2212+
2213+
if (!attributeValues.count) {
2214+
return;
2215+
}
21202216

21212217
if (reportChanges) {
21222218
[self _reportAttributes:[self _getAttributesToReportWithReportedValues:attributeValues]];
@@ -2134,8 +2230,42 @@ - (void)setAttributeValues:(NSArray<NSDictionary *> *)attributeValues reportChan
21342230
}
21352231
}
21362232

2233+
- (void)setAttributeValues:(NSArray<NSDictionary *> *)attributeValues reportChanges:(BOOL)reportChanges
2234+
{
2235+
MTR_LOG_INFO("%@ setAttributeValues count: %lu reportChanges: %d", self, static_cast<unsigned long>(attributeValues.count), reportChanges);
2236+
std::lock_guard lock(_lock);
2237+
[self _setAttributeValues:attributeValues reportChanges:reportChanges];
2238+
}
2239+
21372240
- (void)setClusterData:(NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> *)clusterData
21382241
{
2242+
MTR_LOG_INFO("%@ setClusterData count: %lu", self, static_cast<unsigned long>(clusterData.count));
2243+
if (!clusterData.count) {
2244+
return;
2245+
}
2246+
2247+
std::lock_guard lock(_lock);
2248+
2249+
#if MTRDEVICE_ATTRIBUTE_CACHE_STORE_ATTRIBUTES_BY_CLUSTER
2250+
// For each cluster, extract and create the attribute response-value for the read cache
2251+
// TODO: consider some optimization in how the read cache is structured so there's fewer conversions from this format to what's in the cache
2252+
for (MTRClusterPath * clusterPath in clusterData) {
2253+
MTRDeviceClusterData * data = clusterData[clusterPath];
2254+
// Build and set attributes one cluster at a time to avoid creating a ton of temporary objects at a time
2255+
@autoreleasepool {
2256+
NSMutableArray * attributeValues = [NSMutableArray array];
2257+
for (NSNumber * attributeID in data.attributes) {
2258+
MTRAttributePath * attributePath = [MTRAttributePath attributePathWithEndpointID:clusterPath.endpoint clusterID:clusterPath.cluster attributeID:attributeID];
2259+
NSDictionary * responseValue = @{ MTRAttributePathKey : attributePath, MTRDataKey : data.attributes[attributeID] };
2260+
[attributeValues addObject:responseValue];
2261+
}
2262+
if (attributeValues.count) {
2263+
[self _setAttributeValues:attributeValues reportChanges:NO];
2264+
}
2265+
}
2266+
}
2267+
#endif
2268+
21392269
[_clusterData addEntriesFromDictionary:clusterData];
21402270
}
21412271

‎src/darwin/Framework/CHIP/MTRDeviceController.mm

+3
Original file line numberDiff line numberDiff line change
@@ -926,12 +926,15 @@ - (MTRDevice *)deviceForNodeID:(NSNumber *)nodeID
926926
_nodeIDToDeviceMap[nodeID] = deviceToReturn;
927927
}
928928

929+
#if !MTRDEVICE_ATTRIBUTE_CACHE_STORE_ATTRIBUTES_BY_CLUSTER
929930
// Load persisted attributes if they exist.
930931
NSArray * attributesFromCache = [_controllerDataStore getStoredAttributesForNodeID:nodeID];
931932
MTR_LOG_INFO("Loaded %lu attributes from storage for %@", static_cast<unsigned long>(attributesFromCache.count), deviceToReturn);
932933
if (attributesFromCache.count) {
933934
[deviceToReturn setAttributeValues:attributesFromCache reportChanges:NO];
934935
}
936+
#endif
937+
// Load persisted cluster data if they exist.
935938
NSDictionary * clusterData = [_controllerDataStore getStoredClusterDataForNodeID:nodeID];
936939
MTR_LOG_INFO("Loaded %lu cluster data from storage for %@", static_cast<unsigned long>(clusterData.count), deviceToReturn);
937940
if (clusterData.count) {

‎src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -68,8 +68,8 @@ NS_ASSUME_NONNULL_BEGIN
6868
* Storage for MTRDevice attribute read cache. This is local-only storage as an optimization. New controller devices using MTRDevice API can prime their own local cache from devices directly.
6969
*/
7070
- (nullable NSArray<NSDictionary *> *)getStoredAttributesForNodeID:(NSNumber *)nodeID;
71-
- (nullable NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> *)getStoredClusterDataForNodeID:(NSNumber *)nodeID;
7271
- (void)storeAttributeValues:(NSArray<NSDictionary *> *)dataValues forNodeID:(NSNumber *)nodeID;
72+
- (nullable NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> *)getStoredClusterDataForNodeID:(NSNumber *)nodeID;
7373
- (void)storeClusterData:(NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> *)clusterData forNodeID:(NSNumber *)nodeID;
7474
- (void)clearStoredAttributesForNodeID:(NSNumber *)nodeID;
7575
- (void)clearAllStoredAttributes;

0 commit comments

Comments
 (0)