Skip to content

Commit

Permalink
Properly hook up the duplicate check, and add logging
Browse files Browse the repository at this point in the history
  • Loading branch information
@jtung-apple
jtung-apple committed Jun 1, 2023
1 parent 6d7b16f commit 173585e
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 3 deletions.
18 changes: 15 additions & 3 deletions src/darwin/Framework/CHIP/MTRDevice.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -790,13 +790,19 @@ static BOOL AttributeHasChangesOmittedQuality(MTRAttributePath * attributePath)
// 4. Cache has no entry
// TODO: add option for BaseSubscriptionCallback to report during priming, to reduce when case 4 is hit
if (!attributeIsSpecified || ![self _subscriptionAbleToReport] || hasChangesOmittedQuality || !attributeValueToReturn) {
// Read requests container will be an array of items, each being an array containing:
// Read requests container will be a mutable array of items, each being an array containing:
// [endpoint ID, cluster ID, attribute ID, params]
// Batching handler should only coalesce when params are equal.

// For this single read API there's only 1 array item. Use NSNull to stand in for nil params for easy comparison.
NSMutableArray<NSArray *> * readRequests =
[NSMutableArray arrayWithObject:@[ endpointID, clusterID, attributeID, params ?: [NSNull null] ]];
NSArray * readRequestData = @[ endpointID, clusterID, attributeID, params ?: [NSNull null] ];

// But first, check if a duplicate read request is already queued and return
if ([_asyncCallbackWorkQueue isDuplicateForTypeID:MTRDeviceWorkItemDuplicateReadTypeID workItemData:readRequestData]) {
return attributeValueToReturn;
}

NSMutableArray<NSArray *> * readRequests = [NSMutableArray arrayWithObject:readRequestData];

// Create work item, set ready handler to perform task, then enqueue the work
MTRAsyncCallbackQueueWorkItem * workItem = [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:self.queue];
Expand All @@ -808,22 +814,27 @@ static BOOL AttributeHasChangesOmittedQuality(MTRAttributePath * attributePath)

// Can only read up to 9 paths at a time, per spec
if (readRequestsCurrent.count >= 9) {
MTR_LOG_DEFAULT("%@ batching cannot add more", logPrefix);
return;
}

while (readRequestsNext.count) {
// if params don't match then they cannot be merged
if (![readRequestsNext[0][MTRDeviceReadRequestFieldParamsIndex]
isEqual:readRequestsCurrent[0][MTRDeviceReadRequestFieldParamsIndex]]) {
MTR_LOG_DEFAULT("%@ batching merged all possible items", logPrefix);
return;
}

// merge the next item's first request into the current item's list
[readRequestsCurrent addObject:readRequestsNext[0]];
MTR_LOG_INFO("%@ batching merging %@ => %lu total", logPrefix, readRequestsNext[0],
(unsigned long) readRequestsCurrent.count);
[readRequestsNext removeObjectAtIndex:0];

// Can only read up to 9 paths at a time, per spec
if (readRequestsCurrent.count == 9) {
MTR_LOG_DEFAULT("%@ batching to max paths allowed", logPrefix);
break;
}
}
Expand All @@ -836,6 +847,7 @@ static BOOL AttributeHasChangesOmittedQuality(MTRAttributePath * attributePath)
*isDuplicate = NO;
for (NSArray * readItem in readRequests) {
if ([readItem isEqual:opaqueItemData]) {
MTR_LOG_DEFAULT("%@ duplicate check found %@", logPrefix, readItem);
*isDuplicate = YES;
return;
}
Expand Down
26 changes: 26 additions & 0 deletions src/darwin/Framework/CHIPTests/MTRDeviceTests.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1454,6 +1454,32 @@ - (void)test017_TestMTRDeviceBasics

[device setDelegate:delegate queue:queue];

// Test batching and duplicate check
// - Read 13 different attributes in a row, expect that the 1st to go out by itself, the next 9 batch, and then the 3 after
// are correctly queued in one batch
// - Then read 3 duplicates and expect them to be filtered
// - Note that these tests can only be verified via logs
[device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeScenesID) attributeID:@(0) params:nil];

[device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeScenesID) attributeID:@(1) params:nil];
[device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeScenesID) attributeID:@(2) params:nil];
[device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeScenesID) attributeID:@(3) params:nil];
[device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeScenesID) attributeID:@(4) params:nil];
[device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeScenesID) attributeID:@(5) params:nil];

[device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeScenesID) attributeID:@(6) params:nil];
[device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeScenesID) attributeID:@(7) params:nil];
[device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeLevelControlID) attributeID:@(0) params:nil];
[device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeLevelControlID) attributeID:@(1) params:nil];

[device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeLevelControlID) attributeID:@(2) params:nil];
[device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeLevelControlID) attributeID:@(3) params:nil];
[device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeLevelControlID) attributeID:@(4) params:nil];

[device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeLevelControlID) attributeID:@(4) params:nil];
[device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeLevelControlID) attributeID:@(4) params:nil];
[device readAttributeWithEndpointID:@(1) clusterID:@(MTRClusterIDTypeLevelControlID) attributeID:@(4) params:nil];

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

XCTAssertNotEqual(attributeReportsReceived, 0);
Expand Down

0 comments on commit 173585e

Please sign in to comment.