From 1fd04ac8080e90fff6fc8f0e5234530a71bcc844 Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Thu, 21 Sep 2023 19:51:09 -0700 Subject: [PATCH] [Darwin] Issue 26012 - MTRDevice should stream subscription reports (#29358) * [Darwin] Issue 26012 - MTRDevice should stream subscription reports * Changed implementation to do per-packet batching * Remove test/redundant code * Added unit test protocol comment for readability * Address review comment --- .../CHIP/MTRBaseSubscriptionCallback.h | 10 +++++++ .../CHIP/MTRBaseSubscriptionCallback.mm | 22 +++++++++++++++ src/darwin/Framework/CHIP/MTRDevice.mm | 28 +++++++++++++++++-- .../Framework/CHIPTests/MTRDeviceTests.m | 17 +++++++++-- 4 files changed, 72 insertions(+), 5 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRBaseSubscriptionCallback.h b/src/darwin/Framework/CHIP/MTRBaseSubscriptionCallback.h index 12488e6817b970..42350a51cadbae 100644 --- a/src/darwin/Framework/CHIP/MTRBaseSubscriptionCallback.h +++ b/src/darwin/Framework/CHIP/MTRBaseSubscriptionCallback.h @@ -84,6 +84,11 @@ class MTRBaseSubscriptionCallback : public chip::app::ClusterStateCache::Callbac // Ensure we release the ReadClient before we tear down anything else, // so it can call our OnDeallocatePaths properly. mReadClient = nullptr; + + // Make sure the block isn't run after object destruction + if (mInterimReportBlock) { + dispatch_block_cancel(mInterimReportBlock); + } } chip::app::BufferedReadCallback & GetBufferedCallback() { return mBufferedReadAdapter; } @@ -103,6 +108,10 @@ class MTRBaseSubscriptionCallback : public chip::app::ClusterStateCache::Callbac // be immediately followed by OnDone and we want to do the deletion there. void ReportError(CHIP_ERROR aError, bool aCancelSubscription = true); + // Called at attribute/event report time to queue a block to report on the Matter queue so that for multi-packet reports, this + // block is run and reports in batch. No-op if the block is already queued. + void QueueInterimReport(); + private: void OnReportBegin() override; @@ -166,6 +175,7 @@ class MTRBaseSubscriptionCallback : public chip::app::ClusterStateCache::Callbac std::unique_ptr mClusterStateCache; bool mHaveQueuedDeletion = false; OnDoneHandler _Nullable mOnDoneHandler = nil; + dispatch_block_t mInterimReportBlock = nil; }; NS_ASSUME_NONNULL_END diff --git a/src/darwin/Framework/CHIP/MTRBaseSubscriptionCallback.mm b/src/darwin/Framework/CHIP/MTRBaseSubscriptionCallback.mm index 24301ce2a28062..4b313d75c9e6b6 100644 --- a/src/darwin/Framework/CHIP/MTRBaseSubscriptionCallback.mm +++ b/src/darwin/Framework/CHIP/MTRBaseSubscriptionCallback.mm @@ -46,13 +46,35 @@ if (attributeCallback != nil && attributeReports.count) { attributeCallback(attributeReports); } + if (eventCallback != nil && eventReports.count) { eventCallback(eventReports); } } +void MTRBaseSubscriptionCallback::QueueInterimReport() +{ + if (mInterimReportBlock) { + return; + } + + mInterimReportBlock = dispatch_block_create(DISPATCH_BLOCK_INHERIT_QOS_CLASS, ^{ + mInterimReportBlock = nil; + ReportData(); + // Allocate reports arrays to continue accumulation + mAttributeReports = [NSMutableArray new]; + mEventReports = [NSMutableArray new]; + }); + + dispatch_async(DeviceLayer::PlatformMgrImpl().GetWorkQueue(), mInterimReportBlock); +} + void MTRBaseSubscriptionCallback::OnReportEnd() { + if (mInterimReportBlock) { + dispatch_block_cancel(mInterimReportBlock); + mInterimReportBlock = nil; + } ReportData(); if (mReportEndHandler) { mReportEndHandler(); diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index a2f5f795389776..ea5d9e800de8e5 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -187,6 +187,13 @@ @interface MTRDevice () @end +// Declaring selector so compiler won't complain about testing and calling it in _handleReportEnd +#ifdef DEBUG +@protocol MTRDeviceUnitTestDelegate +- (void)unitTestReportEndForDevice:(MTRDevice *)device; +@end +#endif + @implementation MTRDevice - (instancetype)initWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceController *)controller @@ -402,9 +409,11 @@ - (void)_handleUnsolicitedMessageFromPublisher [self _changeState:MTRDeviceStateReachable]; id delegate = _weakDelegate.strongObject; - if (delegate && [delegate respondsToSelector:@selector(deviceBecameActive:)]) { + if (delegate) { dispatch_async(_delegateQueue, ^{ - [delegate deviceBecameActive:self]; + if ([delegate respondsToSelector:@selector(deviceBecameActive:)]) { + [delegate deviceBecameActive:self]; + } }); } @@ -429,6 +438,17 @@ - (void)_handleReportEnd { os_unfair_lock_lock(&self->_lock); _estimatedStartTimeFromGeneralDiagnosticsUpTime = nil; +// For unit testing only +#ifdef DEBUG + id delegate = _weakDelegate.strongObject; + if (delegate) { + dispatch_async(_delegateQueue, ^{ + if ([delegate respondsToSelector:@selector(unitTestReportEndForDevice:)]) { + [delegate unitTestReportEndForDevice:self]; + } + }); + } +#endif os_unfair_lock_unlock(&self->_lock); } @@ -1546,6 +1566,8 @@ - (void)invokeCommandWithEndpointID:(NSNumber *)endpointID [mEventReports addObject:[MTRBaseDevice eventReportForHeader:aEventHeader andData:value]]; } } + + QueueInterimReport(); } void SubscriptionCallback::OnAttributeData( @@ -1582,5 +1604,7 @@ - (void)invokeCommandWithEndpointID:(NSNumber *)endpointID [mAttributeReports addObject:@ { MTRAttributePathKey : attributePath, MTRDataKey : value }]; } } + + QueueInterimReport(); } } // anonymous namespace diff --git a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m index 95f4aab4d5d30c..2fa2800e0dcf8c 100644 --- a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m +++ b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m @@ -122,6 +122,7 @@ @interface MTRDeviceTestDelegate : NSObject @property (nonatomic, nullable) dispatch_block_t onNotReachable; @property (nonatomic, nullable) MTRDeviceTestDelegateDataHandler onAttributeDataReceived; @property (nonatomic, nullable) MTRDeviceTestDelegateDataHandler onEventDataReceived; +@property (nonatomic, nullable) dispatch_block_t onReportEnd; @end @implementation MTRDeviceTestDelegate @@ -148,6 +149,13 @@ - (void)device:(MTRDevice *)device receivedEventReport:(NSArray *> * data) { attributeReportsReceived += data.count; };