Skip to content

Commit

Permalink
MTRDevice should remember new DataVersion values even if the attribut…
Browse files Browse the repository at this point in the history 
…e value did not change. (#35756)

We used to ignore "same value but new DataVersion" because some devices would
spam reports that looked like that and we were constantly hitting storage for
the DataVersion updates.  But now that we have backoffs on the storage of our
cluster data, we can go ahead and just note the new DataVersion and rely on that
backoff to prevent hitting storage too much.

The test update verifies that:

1) Reports with same value but new DataVersion do get persisted but are subject
   to the persistence backoff settings.
2) Reports with the same value and same DataVersion do not lead to any storage
   traffic.
  • Loading branch information
@bzbarsky-apple @pull
bzbarsky-apple authored and pull[bot] committed Sep 25, 2024
1 parent 85ac076 commit 4475720
Show file tree
Hide file tree
Showing 2 changed files with 81 additions and 48 deletions.
6 changes: 2 additions & 4 deletions src/darwin/Framework/CHIP/MTRDevice_Concrete.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3533,6 +3533,8 @@ - (NSArray *)_getAttributesToReportWithReportedValues:(NSArray<NSDictionary<NSSt
NSNumber * dataVersion = attributeDataValue[MTRDataVersionKey];
MTRClusterPath * clusterPath = [MTRClusterPath clusterPathWithEndpointID:attributePath.endpoint clusterID:attributePath.cluster];
if (dataVersion) {
[self _noteDataVersion:dataVersion forClusterPath:clusterPath];

// Remove data version from what we cache in memory
attributeDataValue = [self _dataValueWithoutDataVersion:attributeDataValue];
}
Expand All @@ -3545,10 +3547,6 @@ - (NSArray *)_getAttributesToReportWithReportedValues:(NSArray<NSDictionary<NSSt
#endif
// Now that we have grabbed previousValue, update our cache with the attribute value.
if (readCacheValueChanged) {
if (dataVersion) {
[self _noteDataVersion:dataVersion forClusterPath:clusterPath];
}

[self _pruneStoredDataForPath:attributePath missingFrom:attributeDataValue];

if (!_deviceConfigurationChanged) {
Expand Down
123 changes: 79 additions & 44 deletions src/darwin/Framework/CHIPTests/MTRDeviceTests.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3746,11 +3746,16 @@ - (void)test035_TestMTRDeviceSubscriptionNotEstablishedOverXPC
}

- (NSArray<NSDictionary<NSString *, id> *> *)_testAttributeReportWithValue:(unsigned int)testValue
{
return [self _testAttributeReportWithValue:testValue dataVersion:testValue];
}

- (NSArray<NSDictionary<NSString *, id> *> *)_testAttributeReportWithValue:(unsigned int)testValue dataVersion:(unsigned int)dataVersion
{
return @[ @{
MTRAttributePathKey : [MTRAttributePath attributePathWithEndpointID:@(0) clusterID:@(MTRClusterIDTypeLevelControlID) attributeID:@(MTRAttributeIDTypeClusterLevelControlAttributeCurrentLevelID)],
MTRDataKey : @ {
MTRDataVersionKey : @(testValue),
MTRDataVersionKey : @(dataVersion),
MTRTypeKey : MTRUnsignedIntegerValueType,
MTRValueKey : @(testValue),
}
Expand Down Expand Up @@ -3809,7 +3814,7 @@ - (void)test036_TestStorageBehaviorConfiguration
[device setDelegate:delegate queue:queue];

// Use a counter that will be incremented for each report as the value.
unsigned int currentTestValue = 1;
__block unsigned int currentTestValue = 1;

// Initial setup: Inject report and see that the attribute persisted. No delay is
// expected for the first (priming) report.
Expand Down Expand Up @@ -3928,54 +3933,84 @@ - (void)test036_TestStorageBehaviorConfiguration
XCTAssertLessThan(reportToPersistenceDelay, baseTestDelayTime * 2 * 5 * 1.3);

// Test 4: test reporting excessively, and see that persistence does not happen until
// reporting frequency goes back above the threshold
reportEndTime = nil;
dataPersistedTime = nil;
XCTestExpectation * dataPersisted4 = [self expectationWithDescription:@"data persisted 4"];
delegate.onClusterDataPersisted = ^{
os_unfair_lock_lock(&lock);
if (!dataPersistedTime) {
dataPersistedTime = [NSDate now];
}
os_unfair_lock_unlock(&lock);
[dataPersisted4 fulfill];
};

// Set report times with short delay and check that the multiplier is engaged
[device unitTestSetMostRecentReportTimes:[NSMutableArray arrayWithArray:@[
[NSDate dateWithTimeIntervalSinceNow:-(baseTestDelayTime * 0.1 * 4)],
[NSDate dateWithTimeIntervalSinceNow:-(baseTestDelayTime * 0.1 * 3)],
[NSDate dateWithTimeIntervalSinceNow:-(baseTestDelayTime * 0.1 * 2)],
[NSDate dateWithTimeIntervalSinceNow:-(baseTestDelayTime * 0.1)],
]]];

// Inject report that makes MTRDevice detect the device is reporting excessively
[device unitTestInjectAttributeReport:[self _testAttributeReportWithValue:currentTestValue++] fromSubscription:YES];
// reporting frequency goes back below the threshold
__auto_type excessiveReportTest = ^(unsigned int testId, NSArray<NSDictionary<NSString *, id> *> * (^reportGenerator)(void), bool expectPersistence) {
reportEndTime = nil;
dataPersistedTime = nil;
XCTestExpectation * dataPersisted = [self expectationWithDescription:[NSString stringWithFormat:@"data persisted %u", testId]];
dataPersisted.inverted = !expectPersistence;
delegate.onClusterDataPersisted = ^{
os_unfair_lock_lock(&lock);
if (!dataPersistedTime) {
dataPersistedTime = [NSDate now];
}
os_unfair_lock_unlock(&lock);
[dataPersisted fulfill];
};

// Now keep reporting excessively for base delay time max times max multiplier, plus a bit more
NSDate * excessiveStartTime = [NSDate now];
for (;;) {
usleep((useconds_t) (baseTestDelayTime * 0.1 * USEC_PER_SEC));
[device unitTestInjectAttributeReport:[self _testAttributeReportWithValue:currentTestValue++] fromSubscription:YES];
NSTimeInterval elapsed = -[excessiveStartTime timeIntervalSinceNow];
if (elapsed > (baseTestDelayTime * 2 * 5 * 1.2)) {
break;
// Set report times with short delay and check that the multiplier is engaged
[device unitTestSetMostRecentReportTimes:[NSMutableArray arrayWithArray:@[
[NSDate dateWithTimeIntervalSinceNow:-(baseTestDelayTime * 0.1 * 4)],
[NSDate dateWithTimeIntervalSinceNow:-(baseTestDelayTime * 0.1 * 3)],
[NSDate dateWithTimeIntervalSinceNow:-(baseTestDelayTime * 0.1 * 2)],
[NSDate dateWithTimeIntervalSinceNow:-(baseTestDelayTime * 0.1)],
]]];

// Inject report that makes MTRDevice detect the device is reporting excessively
[device unitTestInjectAttributeReport:reportGenerator() fromSubscription:YES];

// Now keep reporting excessively for base delay time max times max multiplier, plus a bit more
NSDate * excessiveStartTime = [NSDate now];
for (;;) {
usleep((useconds_t) (baseTestDelayTime * 0.1 * USEC_PER_SEC));
[device unitTestInjectAttributeReport:reportGenerator() fromSubscription:YES];
NSTimeInterval elapsed = -[excessiveStartTime timeIntervalSinceNow];
if (elapsed > (baseTestDelayTime * 2 * 5 * 1.2)) {
break;
}
}
}

// Check that persistence has not happened because it's now turned off
XCTAssertNil(dataPersistedTime);
// Check that persistence has not happened because it's now turned off
XCTAssertNil(dataPersistedTime);

// Now force report times to large number, to simulate time passage
[device unitTestSetMostRecentReportTimes:[NSMutableArray arrayWithArray:@[
[NSDate dateWithTimeIntervalSinceNow:-(baseTestDelayTime * 10)],
]]];
// Now force report times to large number, to simulate time passage
[device unitTestSetMostRecentReportTimes:[NSMutableArray arrayWithArray:@[
[NSDate dateWithTimeIntervalSinceNow:-(baseTestDelayTime * 10)],
]]];

// And inject a report to trigger MTRDevice to recalculate that this device is no longer
// reporting excessively
[device unitTestInjectAttributeReport:[self _testAttributeReportWithValue:currentTestValue++] fromSubscription:YES];
// And inject a report to trigger MTRDevice to recalculate that this device is no longer
// reporting excessively
[device unitTestInjectAttributeReport:reportGenerator() fromSubscription:YES];

[self waitForExpectations:@[ dataPersisted ] timeout:60];
};

[self waitForExpectations:@[ dataPersisted4 ] timeout:60];
excessiveReportTest(
4, ^{
return [self _testAttributeReportWithValue:currentTestValue++];
}, true);

// Test 5: test reporting excessively with the same value and different data
// versions, and see that persistence does not happen until reporting
// frequency goes back below the threshold.
__block __auto_type dataVersion = currentTestValue;
// We incremented currentTestValue after injecting the last report. Make sure all the new
// reports use that last-reported value.
__auto_type lastReportedValue = currentTestValue - 1;
excessiveReportTest(
5, ^{
return [self _testAttributeReportWithValue:lastReportedValue dataVersion:dataVersion++];
}, true);

// Test 6: test reporting excessively with the same value and same data
// version, and see that persistence does not happen at all.
// We incremented dataVersion after injecting the last report. Make sure all the new
// reports use that last-reported value.
__block __auto_type lastReportedDataVersion = dataVersion - 1;
excessiveReportTest(
6, ^{
return [self _testAttributeReportWithValue:lastReportedValue dataVersion:lastReportedDataVersion];
}, false);

delegate.onReportEnd = nil;
delegate.onClusterDataPersisted = nil;
Expand Down

0 comments on commit 4475720

Please sign in to comment.