From 173585e6b499d33dc0ee6e6cbc209cbfac48d0cc Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Thu, 1 Jun 2023 07:55:54 -0700 Subject: [PATCH] Properly hook up the duplicate check, and add logging --- src/darwin/Framework/CHIP/MTRDevice.mm | 18 ++++++++++--- .../Framework/CHIPTests/MTRDeviceTests.m | 26 +++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index e014f0b3d21b3e..243bc97d73f4aa 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -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 * 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 * readRequests = [NSMutableArray arrayWithObject:readRequestData]; // Create work item, set ready handler to perform task, then enqueue the work MTRAsyncCallbackQueueWorkItem * workItem = [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:self.queue]; @@ -808,6 +814,7 @@ 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; } @@ -815,15 +822,19 @@ static BOOL AttributeHasChangesOmittedQuality(MTRAttributePath * attributePath) // 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; } } @@ -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; } diff --git a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m index 1c2c1520f586d5..07999e06cafeef 100644 --- a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m +++ b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m @@ -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);