Skip to content

Commit abb9cde

Browse files
Fix up guarantees around the "device cache primed" notification.
We should be dispatching this only if we did not previously have a primed device cache. testAttributeReportWithValue was renamed, because methods starting with "test" are run as actual tests, and that's not what it's supposed to be doing.
1 parent 3e93ba6 commit abb9cde

File tree

3 files changed

+81
-82
lines changed

3 files changed

+81
-82
lines changed

src/darwin/Framework/CHIP/MTRDevice.mm

+30-59
Original file line numberDiff line numberDiff line change
@@ -376,7 +376,11 @@ @implementation MTRDevice {
376376
#ifdef DEBUG
377377
NSUInteger _unitTestAttributesReportedSinceLastCheck;
378378
#endif
379-
BOOL _delegateDeviceCachePrimedCalled;
379+
380+
// _deviceCachePrimed is true if we have the data that comes from an initial
381+
// subscription priming report (whether it came from storage or from our
382+
// subscription).
383+
BOOL _deviceCachePrimed;
380384

381385
// _persistedClusterData stores data that we have already persisted (when we have
382386
// cluster data persistence enabled). Nil when we have no persistence enabled.
@@ -736,11 +740,6 @@ - (void)setDelegate:(id<MTRDeviceDelegate>)delegate queue:(dispatch_queue_t)queu
736740
_weakDelegate = [MTRWeakReference weakReferenceWithObject:delegate];
737741
_delegateQueue = queue;
738742

739-
// If Check if cache is already primed and client hasn't been informed yet, call the -deviceCachePrimed: callback
740-
if (!_delegateDeviceCachePrimedCalled && [self _isCachePrimedWithInitialConfigurationData]) {
741-
[self _callDelegateDeviceCachePrimed];
742-
}
743-
744743
if (setUpSubscription) {
745744
_initialSubscribeStart = [NSDate now];
746745
if ([self _deviceUsesThread]) {
@@ -915,7 +914,7 @@ - (BOOL)_callDelegateWithBlock:(void (^)(id<MTRDeviceDelegate>))block
915914
- (void)_callDelegateDeviceCachePrimed
916915
{
917916
os_unfair_lock_assert_owner(&self->_lock);
918-
_delegateDeviceCachePrimedCalled = [self _callDelegateWithBlock:^(id<MTRDeviceDelegate> delegate) {
917+
[self _callDelegateWithBlock:^(id<MTRDeviceDelegate> delegate) {
919918
if ([delegate respondsToSelector:@selector(deviceCachePrimed:)]) {
920919
[delegate deviceCachePrimed:self];
921920
}
@@ -994,11 +993,6 @@ - (void)_handleSubscriptionEstablished
994993
[self _changeInternalState:MTRInternalDeviceStateInitialSubscriptionEstablished];
995994
}
996995

997-
// As subscription is established, check if the delegate needs to be informed
998-
if (!_delegateDeviceCachePrimedCalled) {
999-
[self _callDelegateDeviceCachePrimed];
1000-
}
1001-
1002996
[self _changeState:MTRDeviceStateReachable];
1003997

1004998
// No need to monitor connectivity after subscription establishment
@@ -1480,6 +1474,12 @@ - (void)_scheduleClusterDataPersistence
14801474
return;
14811475
}
14821476

1477+
// If we have nothing stored at all yet, store directly, so we move into a
1478+
// primed state.
1479+
if (!_deviceCachePrimed) {
1480+
[self _persistClusterData];
1481+
}
1482+
14831483
// Ensure there is an array to keep the most recent report times
14841484
if (!_mostRecentReportTimes) {
14851485
_mostRecentReportTimes = [NSMutableArray array];
@@ -1525,7 +1525,7 @@ - (void)_scheduleClusterDataPersistence
15251525

15261526
// Set current multiplier to [1, MaxMultiplier]
15271527
_reportToPersistenceDelayCurrentMultiplier = 1 + (proportionTowardMinThreshold * (_storageBehaviorConfiguration.reportToPersistenceDelayMaxMultiplier - 1));
1528-
MTR_LOG("%@ storage behavior: device reporting frequently - setting delay multiplied to %lf", self, _reportToPersistenceDelayCurrentMultiplier);
1528+
MTR_LOG("%@ storage behavior: device reporting frequently - setting delay multiplier to %lf", self, _reportToPersistenceDelayCurrentMultiplier);
15291529
} else {
15301530
_reportToPersistenceDelayCurrentMultiplier = 1;
15311531
}
@@ -1601,6 +1601,19 @@ - (void)_handleReportEnd
16011601
_deviceConfigurationChanged = NO;
16021602
}
16031603

1604+
// Do this after the _deviceConfigurationChanged check, so that we don't
1605+
// call deviceConfigurationChanged: immediately after telling our delegate
1606+
// we are now primed.
1607+
//
1608+
// TODO: Maybe we shouldn't dispatch deviceConfigurationChanged: for the
1609+
// initial priming bits?
1610+
if (!_deviceCachePrimed) {
1611+
// This is the end of the priming sequence of data reports, so we have
1612+
// all the data for the device now.
1613+
_deviceCachePrimed = YES;
1614+
[self _callDelegateDeviceCachePrimed];
1615+
}
1616+
16041617
// For unit testing only
16051618
#ifdef DEBUG
16061619
id delegate = _weakDelegate.strongObject;
@@ -3165,10 +3178,9 @@ - (void)setPersistedClusterData:(NSDictionary<MTRClusterPath *, MTRDeviceCluster
31653178
[_persistedClusterData setObject:clusterData[clusterPath] forKey:clusterPath];
31663179
}
31673180

3168-
// If cache is set from storage and is primed with initial configuration data, then assume the client had beeen informed in the past, and mark that the callback has been called
3169-
if ([self _isCachePrimedWithInitialConfigurationData]) {
3170-
_delegateDeviceCachePrimedCalled = YES;
3171-
}
3181+
// We have some stored data. Since we don't store data until the end of the
3182+
// initial priming report, our device cache must be primed.
3183+
_deviceCachePrimed = YES;
31723184
}
31733185

31743186
- (void)_setLastInitialSubscribeLatency:(id)latency
@@ -3218,7 +3230,7 @@ - (void)_storePersistedDeviceData
32183230
- (BOOL)deviceCachePrimed
32193231
{
32203232
std::lock_guard lock(_lock);
3221-
return [self _isCachePrimedWithInitialConfigurationData];
3233+
return _deviceCachePrimed;
32223234
}
32233235

32243236
// If value is non-nil, associate with expectedValueID
@@ -3395,47 +3407,6 @@ - (void)_removeExpectedValueForAttributePath:(MTRAttributePath *)attributePath e
33953407
}
33963408
}
33973409

3398-
// This method checks if there is a need to inform delegate that the attribute cache has been "primed"
3399-
- (BOOL)_isCachePrimedWithInitialConfigurationData
3400-
{
3401-
os_unfair_lock_assert_owner(&self->_lock);
3402-
3403-
// Check if root node descriptor exists
3404-
MTRDeviceDataValueDictionary rootDescriptorPartsListDataValue = [self _cachedAttributeValueForPath:[MTRAttributePath attributePathWithEndpointID:@(kRootEndpointId) clusterID:@(MTRClusterIDTypeDescriptorID) attributeID:@(MTRAttributeIDTypeClusterDescriptorAttributePartsListID)]];
3405-
if (!rootDescriptorPartsListDataValue || ![MTRArrayValueType isEqualToString:rootDescriptorPartsListDataValue[MTRTypeKey]]) {
3406-
return NO;
3407-
}
3408-
NSArray * partsList = rootDescriptorPartsListDataValue[MTRValueKey];
3409-
if (![partsList isKindOfClass:[NSArray class]] || !partsList.count) {
3410-
MTR_LOG_ERROR("%@ unexpected type %@ for parts list %@", self, [partsList class], partsList);
3411-
return NO;
3412-
}
3413-
3414-
// Check if we have cached descriptor clusters for each listed endpoint
3415-
for (NSDictionary * endpointDictionary in partsList) {
3416-
NSDictionary * endpointDataValue = endpointDictionary[MTRDataKey];
3417-
if (![endpointDataValue isKindOfClass:[NSDictionary class]]) {
3418-
MTR_LOG_ERROR("%@ unexpected parts list dictionary %@ data value class %@", self, endpointDictionary, [endpointDataValue class]);
3419-
continue;
3420-
}
3421-
if (![MTRUnsignedIntegerValueType isEqual:endpointDataValue[MTRTypeKey]]) {
3422-
MTR_LOG_ERROR("%@ unexpected parts list data value %@ item type %@", self, endpointDataValue, endpointDataValue[MTRTypeKey]);
3423-
continue;
3424-
}
3425-
NSNumber * endpoint = endpointDataValue[MTRValueKey];
3426-
if (![endpoint isKindOfClass:[NSNumber class]]) {
3427-
MTR_LOG_ERROR("%@ unexpected parts list item value class %@", self, [endpoint class]);
3428-
continue;
3429-
}
3430-
MTRDeviceDataValueDictionary descriptorDeviceTypeListDataValue = [self _cachedAttributeValueForPath:[MTRAttributePath attributePathWithEndpointID:endpoint clusterID:@(MTRClusterIDTypeDescriptorID) attributeID:@(MTRAttributeIDTypeClusterDescriptorAttributeDeviceTypeListID)]];
3431-
if (![MTRArrayValueType isEqualToString:descriptorDeviceTypeListDataValue[MTRTypeKey]] || !descriptorDeviceTypeListDataValue[MTRValueKey]) {
3432-
return NO;
3433-
}
3434-
}
3435-
3436-
return YES;
3437-
}
3438-
34393410
- (MTRBaseDevice *)newBaseDevice
34403411
{
34413412
return [MTRBaseDevice deviceWithNodeID:self.nodeID controller:self.deviceController];

src/darwin/Framework/CHIPTests/MTRDeviceTests.m

+32-14
Original file line numberDiff line numberDiff line change
@@ -3669,7 +3669,7 @@ - (void)test035_TestMTRDeviceSubscriptionNotEstablishedOverXPC
36693669
XCTAssertEqual([device _getInternalState], MTRInternalDeviceStateUnsubscribed);
36703670
}
36713671

3672-
- (NSArray<NSDictionary<NSString *, id> *> *)testAttributeReportWithValue:(unsigned int)testValue
3672+
- (NSArray<NSDictionary<NSString *, id> *> *)_testAttributeReportWithValue:(unsigned int)testValue
36733673
{
36743674
return @[ @{
36753675
MTRAttributePathKey : [MTRAttributePath attributePathWithEndpointID:@(0) clusterID:@(MTRClusterIDTypeLevelControlID) attributeID:@(MTRAttributeIDTypeClusterLevelControlAttributeCurrentLevelID)],
@@ -3696,7 +3696,7 @@ - (void)test036_TestStorageBehaviorConfiguration
36963696
__block NSDate * reportEndTime = nil;
36973697
__block NSDate * dataPersistedTime = nil;
36983698

3699-
XCTestExpectation * dataPersisted1 = [self expectationWithDescription:@"data persisted 1"];
3699+
XCTestExpectation * dataPersistedInitial = [self expectationWithDescription:@"data persisted initial"];
37003700
delegate.onReportEnd = ^() {
37013701
os_unfair_lock_lock(&lock);
37023702
if (!reportEndTime) {
@@ -3711,7 +3711,7 @@ - (void)test036_TestStorageBehaviorConfiguration
37113711
dataPersistedTime = [NSDate now];
37123712
}
37133713
os_unfair_lock_unlock(&lock);
3714-
[dataPersisted1 fulfill];
3714+
[dataPersistedInitial fulfill];
37153715
};
37163716

37173717
// Do not subscribe - only inject sequence of reports to control the timing
@@ -3732,11 +3732,29 @@ - (void)test036_TestStorageBehaviorConfiguration
37323732

37333733
[device setDelegate:delegate queue:queue];
37343734

3735-
// Use a mutable dictionary so the data value can be easily changed between reports
3735+
// Use a counter that will be incremented for each report as the value.
37363736
unsigned int currentTestValue = 1;
37373737

3738+
// Initial setup: Inject report and see that the attribute persisted. No delay is
3739+
// expected for the first (priming) report.
3740+
[device unitTestInjectAttributeReport:[self _testAttributeReportWithValue:currentTestValue++] fromSubscription:YES];
3741+
3742+
[self waitForExpectations:@[ dataPersistedInitial ] timeout:60];
3743+
3744+
XCTestExpectation * dataPersisted1 = [self expectationWithDescription:@"data persisted 1"];
3745+
delegate.onClusterDataPersisted = ^{
3746+
os_unfair_lock_lock(&lock);
3747+
if (!dataPersistedTime) {
3748+
dataPersistedTime = [NSDate now];
3749+
}
3750+
os_unfair_lock_unlock(&lock);
3751+
[dataPersisted1 fulfill];
3752+
};
3753+
37383754
// Test 1: Inject report and see that the attribute persisted, with a delay
3739-
[device unitTestInjectAttributeReport:[self testAttributeReportWithValue:currentTestValue++] fromSubscription:YES];
3755+
reportEndTime = nil;
3756+
dataPersistedTime = nil;
3757+
[device unitTestInjectAttributeReport:[self _testAttributeReportWithValue:currentTestValue++] fromSubscription:YES];
37403758

37413759
[self waitForExpectations:@[ dataPersisted1 ] timeout:60];
37423760

@@ -3762,20 +3780,20 @@ - (void)test036_TestStorageBehaviorConfiguration
37623780
// Test 2: Inject multiple reports with delay and see that the attribute persisted eventually
37633781
reportEndTime = nil;
37643782
dataPersistedTime = nil;
3765-
[device unitTestInjectAttributeReport:[self testAttributeReportWithValue:currentTestValue++] fromSubscription:YES];
3783+
[device unitTestInjectAttributeReport:[self _testAttributeReportWithValue:currentTestValue++] fromSubscription:YES];
37663784

37673785
double frequentReportMultiplier = 0.5;
37683786
usleep((useconds_t) (baseTestDelayTime * frequentReportMultiplier * USEC_PER_SEC));
3769-
[device unitTestInjectAttributeReport:[self testAttributeReportWithValue:currentTestValue++] fromSubscription:YES];
3787+
[device unitTestInjectAttributeReport:[self _testAttributeReportWithValue:currentTestValue++] fromSubscription:YES];
37703788

37713789
usleep((useconds_t) (baseTestDelayTime * frequentReportMultiplier * USEC_PER_SEC));
3772-
[device unitTestInjectAttributeReport:[self testAttributeReportWithValue:currentTestValue++] fromSubscription:YES];
3790+
[device unitTestInjectAttributeReport:[self _testAttributeReportWithValue:currentTestValue++] fromSubscription:YES];
37733791

37743792
usleep((useconds_t) (baseTestDelayTime * frequentReportMultiplier * USEC_PER_SEC));
3775-
[device unitTestInjectAttributeReport:[self testAttributeReportWithValue:currentTestValue++] fromSubscription:YES];
3793+
[device unitTestInjectAttributeReport:[self _testAttributeReportWithValue:currentTestValue++] fromSubscription:YES];
37763794

37773795
usleep((useconds_t) (baseTestDelayTime * frequentReportMultiplier * USEC_PER_SEC));
3778-
[device unitTestInjectAttributeReport:[self testAttributeReportWithValue:currentTestValue++] fromSubscription:YES];
3796+
[device unitTestInjectAttributeReport:[self _testAttributeReportWithValue:currentTestValue++] fromSubscription:YES];
37793797

37803798
// At this point, the threshold for reportToPersistenceDelayTimeMax should have hit, and persistence
37813799
// should have happened with timer running down to persist again with the 5th report above. Need to
@@ -3817,7 +3835,7 @@ - (void)test036_TestStorageBehaviorConfiguration
38173835
]]];
38183836

38193837
// Inject final report that makes MTRDevice recalculate delay with multiplier
3820-
[device unitTestInjectAttributeReport:[self testAttributeReportWithValue:currentTestValue++] fromSubscription:YES];
3838+
[device unitTestInjectAttributeReport:[self _testAttributeReportWithValue:currentTestValue++] fromSubscription:YES];
38213839

38223840
[self waitForExpectations:@[ dataPersisted3 ] timeout:60];
38233841

@@ -3856,13 +3874,13 @@ - (void)test036_TestStorageBehaviorConfiguration
38563874
]]];
38573875

38583876
// Inject report that makes MTRDevice detect the device is reporting excessively
3859-
[device unitTestInjectAttributeReport:[self testAttributeReportWithValue:currentTestValue++] fromSubscription:YES];
3877+
[device unitTestInjectAttributeReport:[self _testAttributeReportWithValue:currentTestValue++] fromSubscription:YES];
38603878

38613879
// Now keep reporting excessively for base delay time max times max multiplier, plus a bit more
38623880
NSDate * excessiveStartTime = [NSDate now];
38633881
for (;;) {
38643882
usleep((useconds_t) (baseTestDelayTime * 0.1 * USEC_PER_SEC));
3865-
[device unitTestInjectAttributeReport:[self testAttributeReportWithValue:currentTestValue++] fromSubscription:YES];
3883+
[device unitTestInjectAttributeReport:[self _testAttributeReportWithValue:currentTestValue++] fromSubscription:YES];
38663884
NSTimeInterval elapsed = -[excessiveStartTime timeIntervalSinceNow];
38673885
if (elapsed > (baseTestDelayTime * 2 * 5 * 1.2)) {
38683886
break;
@@ -3879,7 +3897,7 @@ - (void)test036_TestStorageBehaviorConfiguration
38793897

38803898
// And inject a report to trigger MTRDevice to recalculate that this device is no longer
38813899
// reporting excessively
3882-
[device unitTestInjectAttributeReport:[self testAttributeReportWithValue:currentTestValue++] fromSubscription:YES];
3900+
[device unitTestInjectAttributeReport:[self _testAttributeReportWithValue:currentTestValue++] fromSubscription:YES];
38833901

38843902
[self waitForExpectations:@[ dataPersisted4 ] timeout:60];
38853903

src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m

+19-9
Original file line numberDiff line numberDiff line change
@@ -1436,29 +1436,29 @@ - (void)doDataStoreMTRDeviceTestWithStorageDelegate:(id<MTRDeviceControllerStora
14361436
__auto_type * delegate = [[MTRDeviceTestDelegate alloc] init];
14371437

14381438
XCTestExpectation * subscriptionExpectation = [self expectationWithDescription:@"Subscription has been set up"];
1439-
XCTestExpectation * gotClusterDataPersisted = nil;
1440-
if (!disableStorageBehaviorOptimization) {
1441-
gotClusterDataPersisted = [self expectationWithDescription:@"Cluster data persisted"];
1442-
}
14431439

14441440
delegate.onReportEnd = ^{
14451441
[subscriptionExpectation fulfill];
14461442
};
1447-
delegate.onClusterDataPersisted = ^{
1448-
[gotClusterDataPersisted fulfill];
1443+
1444+
__block BOOL onDeviceCachePrimedCalled = NO;
1445+
delegate.onDeviceCachePrimed = ^{
1446+
onDeviceCachePrimedCalled = YES;
14491447
};
14501448

14511449
// Verify that initially (before we have ever subscribed while using this
14521450
// datastore) the device has no estimate for subscription latency.
14531451
XCTAssertNil(device.estimatedSubscriptionLatency);
14541452

1453+
// And that the device cache is not primed.
1454+
XCTAssertFalse(device.deviceCachePrimed);
1455+
14551456
[device setDelegate:delegate queue:queue];
14561457

14571458
[self waitForExpectations:@[ subscriptionExpectation ] timeout:60];
14581459

1459-
if (!disableStorageBehaviorOptimization) {
1460-
[self waitForExpectations:@[ gotClusterDataPersisted ] timeout:60];
1461-
}
1460+
XCTAssertTrue(device.deviceCachePrimed);
1461+
XCTAssertTrue(onDeviceCachePrimedCalled);
14621462

14631463
NSUInteger dataStoreValuesCount = 0;
14641464
NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> * dataStoreClusterData = [controller.controllerDataStore getStoredClusterDataForNodeID:deviceID];
@@ -1497,6 +1497,9 @@ - (void)doDataStoreMTRDeviceTestWithStorageDelegate:(id<MTRDeviceControllerStora
14971497
// Check that the new device has an estimated subscription latency.
14981498
XCTAssertNotNil(device.estimatedSubscriptionLatency);
14991499

1500+
// And that it's already primed.
1501+
XCTAssertTrue(device.deviceCachePrimed);
1502+
15001503
// Check that this estimate is positive, since subscribing must have taken
15011504
// some time.
15021505
XCTAssertGreaterThan(device.estimatedSubscriptionLatency.doubleValue, 0);
@@ -1517,6 +1520,11 @@ - (void)doDataStoreMTRDeviceTestWithStorageDelegate:(id<MTRDeviceControllerStora
15171520
[newDeviceGotClusterDataPersisted fulfill];
15181521
};
15191522

1523+
__block BOOL newOnDeviceCachePrimedCalled = NO;
1524+
newDelegate.onDeviceCachePrimed = ^{
1525+
newOnDeviceCachePrimedCalled = YES;
1526+
};
1527+
15201528
[newDevice setDelegate:newDelegate queue:queue];
15211529

15221530
[self waitForExpectations:@[ newDeviceSubscriptionExpectation ] timeout:60];
@@ -1525,6 +1533,8 @@ - (void)doDataStoreMTRDeviceTestWithStorageDelegate:(id<MTRDeviceControllerStora
15251533
}
15261534
newDelegate.onReportEnd = nil;
15271535

1536+
XCTAssertFalse(newOnDeviceCachePrimedCalled);
1537+
15281538
// 1) MTRDevice actually gets some attributes reported more than once
15291539
// 2) Some attributes do change on resubscribe
15301540
// * With all-clusts-app as of 2024-02-10, out of 1287 persisted attributes, still 450 attributes were reported with filter

0 commit comments

Comments
 (0)