From d6489326e460330255fcda8d0337e15cd73c7648 Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Sun, 8 Sep 2024 12:31:03 -0700 Subject: [PATCH 1/7] [Darwin] Implement delegate add/remove for device controller --- .../Framework/CHIP/MTRDeviceController.h | 19 +- .../Framework/CHIP/MTRDeviceController.mm | 171 +++++++++++++++++- .../CHIP/MTRDeviceController_Concrete.h | 10 +- .../CHIP/MTRDeviceController_Concrete.mm | 13 +- 4 files changed, 185 insertions(+), 28 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.h b/src/darwin/Framework/CHIP/MTRDeviceController.h index ad025c87d64043..d731b13052798c 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.h +++ b/src/darwin/Framework/CHIP/MTRDeviceController.h @@ -186,7 +186,7 @@ MTR_AVAILABLE(ios(16.1), macos(13.0), watchos(9.1), tvos(16.1)) - (void)preWarmCommissioningSession MTR_DEPRECATED("-[MTRDeviceControllerFactory preWarmCommissioningSession]", ios(16.4, 17.6), macos(13.3, 14.6), watchos(9.4, 10.6), tvos(16.4, 17.6)); /** - * Set the Delegate for the device controller as well as the Queue on which the Delegate callbacks will be triggered + * Set the Delegate for the device controller as well as the Queue on which the Delegate callbacks will be triggered * * @param[in] delegate The delegate the commissioning process should use * @@ -195,6 +195,23 @@ MTR_AVAILABLE(ios(16.1), macos(13.0), watchos(9.1), tvos(16.1)) - (void)setDeviceControllerDelegate:(id)delegate queue:(dispatch_queue_t)queue MTR_AVAILABLE(ios(16.4), macos(13.3), watchos(9.4), tvos(16.4)); +/** + * Adds a Delegate to the device controller as well as the Queue on which the Delegate callbacks will be triggered + * + * @param[in] delegate The delegate the commissioning process should use + * + * @param[in] queue The queue on which the callbacks will be delivered + */ +- (void)addDeviceControllerDelegate:(id)delegate + queue:(dispatch_queue_t)queue MTR_NEWLY_AVAILABLE; + +/** + * Removes a Delegate from the device controller + * + * @param[in] delegate The delegate to be removed + */ +- (void)removeDeviceControllerDelegate:(id)delegate MTR_NEWLY_AVAILABLE; + /** * Start scanning for commissionable devices. * diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index 021ada1852cf9c..8ff31a1918e012 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -112,6 +112,27 @@ using namespace chip::Tracing::DarwinFramework; +@interface MTRDeviceControllerDelegateInfo : NSObject +- (instancetype)initWithDelegate:(id)delegate queue:(dispatch_queue_t)queue; +@property(nonatomic, weak, readonly) id delegate; +@property(nonatomic, readonly) dispatch_queue_t queue; +@end + +@implementation MTRDeviceControllerDelegateInfo +@synthesize delegate = _delegate, queue = _queue; +- (instancetype)initWithDelegate:(id)delegate queue:(dispatch_queue_t)queue +{ + if (!(self = [super init])) { + return nil; + } + + _delegate = delegate; + _queue = queue; + + return self; +} +@end + @implementation MTRDeviceController { chip::Controller::DeviceCommissioner * _cppCommissioner; chip::Credentials::PartialDACVerifier * _partialDACVerifier; @@ -139,6 +160,9 @@ @implementation MTRDeviceController { NSUInteger _keepRunningAssertionCounter; BOOL _shutdownPending; os_unfair_lock _assertionLock; + + NSMutableSet *_delegates; + id _strongDelegateForSetDelegateAPI; } @synthesize uniqueIdentifier = _uniqueIdentifier; @@ -168,6 +192,8 @@ - (instancetype)initForSubclasses:(BOOL)startSuspended _nodeIDToDeviceMap = [NSMapTable strongToWeakObjectsMapTable]; + _delegates = [NSMutableSet set]; + return self; } @@ -567,6 +593,10 @@ - (void)cleanup if (_deviceControllerDelegateBridge) { delete _deviceControllerDelegateBridge; _deviceControllerDelegateBridge = nullptr; + @synchronized (self) { + _strongDelegateForSetDelegateAPI = nil; + [_delegates removeAllObjects]; + } } } @@ -1243,15 +1273,6 @@ - (void)removeDevice:(MTRDevice *)device } #endif -- (void)setDeviceControllerDelegate:(id)delegate queue:(dispatch_queue_t)queue -{ - [self - asyncDispatchToMatterQueue:^() { - self->_deviceControllerDelegateBridge->setDelegate(self, delegate, queue); - } - errorHandler:nil]; -} - - (BOOL)setOperationalCertificateIssuer:(nullable id)operationalCertificateIssuer queue:(nullable dispatch_queue_t)queue { @@ -1751,6 +1772,138 @@ + (void)forceLocalhostAdvertisingOnly } #endif // DEBUG +#pragma mark - MTRDeviceControllerDelegate management + +// Note these are implemented in the base class so that XPC subclass can use it as well when it +- (void)setDeviceControllerDelegate:(id)delegate queue:(dispatch_queue_t)queue +{ + @synchronized (self) { + _strongDelegateForSetDelegateAPI = delegate; + [self addDeviceControllerDelegate:delegate queue:queue]; + } +} + +- (void)addDeviceControllerDelegate:(id)delegate queue:(dispatch_queue_t)queue +{ + @synchronized (self) { + MTRDeviceControllerDelegateInfo *newDelegateInfo = [[MTRDeviceControllerDelegateInfo alloc] initWithDelegate:delegate queue:queue]; + [_delegates addObject:newDelegateInfo]; + MTR_LOG("%@ addDeviceControllerDelegate: added %p total %lu", self, delegate, static_cast(_delegates.count)); + } +} + +- (void)removeDeviceControllerDelegate:(id)delegate +{ + @synchronized (self) { + __block MTRDeviceControllerDelegateInfo *delegateInfoToRemove = nil; + [self _iterateDelegateInfoWithBlock:^(MTRDeviceControllerDelegateInfo *delegateInfo) { + if (delegateInfo.delegate == delegate) { + delegateInfoToRemove = delegateInfo; + } + }]; + + if (delegateInfoToRemove) { + [_delegates removeObject:delegateInfoToRemove]; + if (_strongDelegateForSetDelegateAPI == delegate) { + _strongDelegateForSetDelegateAPI = nil; + } + MTR_LOG("%@ removeDeviceControllerDelegate: removed %p remaining %lu", self, delegate, static_cast(_delegates.count)); + } else { + MTR_LOG("%@ removeDeviceControllerDelegate: delegate %p not found in %lu", self, delegate, static_cast(_delegates.count)); + } + } +} + +// Iterates the delegates, and remove delegate info objects if the delegate object has dealloc'ed +// Returns number of delegates called +- (NSUInteger)_iterateDelegateInfoWithBlock:(void(^ _Nullable)(MTRDeviceControllerDelegateInfo * delegateInfo))block +{ + @synchronized (self) { + if (!_delegates.count) { + MTR_LOG("%@ No delegates to iterate", self); + return 0; + } + + // Opportunistically remove defunct delegate references on every iteration + NSMutableSet *delegatesToRemove = nil; + for (MTRDeviceControllerDelegateInfo *delegateInfo in _delegates) { + id strongDelegate = delegateInfo.delegate; + if (strongDelegate) { + if (block) { + block(delegateInfo); + } + } else { + if (!delegatesToRemove) { + delegatesToRemove = [NSMutableSet set]; + } + [delegatesToRemove addObject:delegateInfo]; + } + } + + if (delegatesToRemove.count) { + [_delegates minusSet:delegatesToRemove]; + MTR_LOG("%@ _iterateDelegatesWithBlock: removed %lu remaining %lu", self, static_cast(delegatesToRemove.count), static_cast(_delegates.count)); + } + + return _delegates.count; + } +} + +- (void)_callDelegatesWithBlock:(void(^ _Nullable)(id delegate))block logString:(const char *)logString; +{ + NSUInteger delegatesCalled = [self _iterateDelegateInfoWithBlock:^(MTRDeviceControllerDelegateInfo *delegateInfo) { + id strongDelegate = delegateInfo.delegate; + dispatch_async(delegateInfo.queue, ^{ + block(strongDelegate); + }); + }]; + + MTR_LOG("%@ %lu delegates called for %s", self, static_cast(delegatesCalled), logString); +} + +- (void)controller:(MTRDeviceController *)controller statusUpdate:(MTRCommissioningStatus)status +{ + [self _callDelegatesWithBlock:^(id delegate) { + if ([delegate respondsToSelector:@selector(controller:statusUpdate:)]) { + [delegate controller:controller statusUpdate:status]; + } + } logString:__PRETTY_FUNCTION__]; +} + +- (void)controller:(MTRDeviceController *)controller commissioningSessionEstablishmentDone:(NSError * _Nullable)error +{ + [self _callDelegatesWithBlock:^(id delegate) { + if ([delegate respondsToSelector:@selector(controller:commissioningSessionEstablishmentDone:)]) { + [delegate controller:controller commissioningSessionEstablishmentDone:error]; + } + } logString:__PRETTY_FUNCTION__]; +} + +- (void)controller:(MTRDeviceController *)controller +commissioningComplete:(NSError * _Nullable)error + nodeID:(NSNumber * _Nullable)nodeID + metrics:(MTRMetrics *)metrics +{ + [self _callDelegatesWithBlock:^(id delegate) { + if ([delegate respondsToSelector:@selector(controller:commissioningComplete:nodeID:metrics:)]) { + [delegate controller:controller commissioningComplete:error nodeID:nodeID metrics:metrics]; + } else if ([delegate respondsToSelector:@selector(controller:commissioningComplete:nodeID:)]) { + [delegate controller:controller commissioningComplete:error nodeID:nodeID]; + } else if ([delegate respondsToSelector:@selector(controller:commissioningComplete:)]) { + [delegate controller:controller commissioningComplete:error]; + } + } logString:__PRETTY_FUNCTION__]; +} + +- (void)controller:(MTRDeviceController *)controller readCommissioningInfo:(MTRProductIdentity *)info +{ + [self _callDelegatesWithBlock:^(id delegate) { + if ([delegate respondsToSelector:@selector(controller:readCommissioningInfo:)]) { + [delegate controller:controller readCommissioningInfo:info]; + } + } logString:__PRETTY_FUNCTION__]; +} + @end // TODO: This should not be in the superclass: either move to diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h index 34c5ca8efad1da..3e83317d8fe780 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h +++ b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h @@ -168,15 +168,7 @@ typedef void (^MTRDeviceConnectionCallback)(MTRBaseDevice * _Nullable device, NS - (void)preWarmCommissioningSession MTR_DEPRECATED("-[MTRDeviceControllerFactory preWarmCommissioningSession]", ios(16.4, 17.6), macos(13.3, 14.6), watchos(9.4, 10.6), tvos(16.4, 17.6)); -/** - * Set the Delegate for the device controller as well as the Queue on which the Delegate callbacks will be triggered - * - * @param[in] delegate The delegate the commissioning process should use - * - * @param[in] queue The queue on which the callbacks will be delivered - */ -- (void)setDeviceControllerDelegate:(id)delegate - queue:(dispatch_queue_t)queue MTR_AVAILABLE(ios(16.4), macos(13.3), watchos(9.4), tvos(16.4)); +// Use super class implementation for -setDeviceControllerDelegate:queue: /** * Start scanning for commissionable devices. diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm index 1edcb4928943a1..78aa16969f3342 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm @@ -717,6 +717,10 @@ - (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams commissionerInitialized = YES; + // Set self as delegate, which fans out delegate callbacks to all added delegates + id selfDelegate = static_cast>(self); + self->_deviceControllerDelegateBridge->setDelegate(self, selfDelegate, dispatch_get_main_queue()); + MTR_LOG("%@ startup succeeded for nodeID 0x%016llX", self, self->_cppCommissioner->GetNodeId()); }); @@ -1185,15 +1189,6 @@ - (void)removeDevice:(MTRDevice *)device } #endif -- (void)setDeviceControllerDelegate:(id)delegate queue:(dispatch_queue_t)queue -{ - [self - asyncDispatchToMatterQueue:^() { - self->_deviceControllerDelegateBridge->setDelegate(self, delegate, queue); - } - errorHandler:nil]; -} - - (BOOL)setOperationalCertificateIssuer:(nullable id)operationalCertificateIssuer queue:(nullable dispatch_queue_t)queue { From 1acb8c8694f89c59e3f9d3d4c741b67ade650499 Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Sun, 8 Sep 2024 12:36:56 -0700 Subject: [PATCH 2/7] Restyled changes --- .../Framework/CHIP/MTRDeviceController.mm | 38 +++++++++---------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index 8ff31a1918e012..e622b5aff57aae 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -114,8 +114,8 @@ @interface MTRDeviceControllerDelegateInfo : NSObject - (instancetype)initWithDelegate:(id)delegate queue:(dispatch_queue_t)queue; -@property(nonatomic, weak, readonly) id delegate; -@property(nonatomic, readonly) dispatch_queue_t queue; +@property (nonatomic, weak, readonly) id delegate; +@property (nonatomic, readonly) dispatch_queue_t queue; @end @implementation MTRDeviceControllerDelegateInfo @@ -161,7 +161,7 @@ @implementation MTRDeviceController { BOOL _shutdownPending; os_unfair_lock _assertionLock; - NSMutableSet *_delegates; + NSMutableSet * _delegates; id _strongDelegateForSetDelegateAPI; } @@ -593,7 +593,7 @@ - (void)cleanup if (_deviceControllerDelegateBridge) { delete _deviceControllerDelegateBridge; _deviceControllerDelegateBridge = nullptr; - @synchronized (self) { + @synchronized(self) { _strongDelegateForSetDelegateAPI = nil; [_delegates removeAllObjects]; } @@ -1777,7 +1777,7 @@ + (void)forceLocalhostAdvertisingOnly // Note these are implemented in the base class so that XPC subclass can use it as well when it - (void)setDeviceControllerDelegate:(id)delegate queue:(dispatch_queue_t)queue { - @synchronized (self) { + @synchronized(self) { _strongDelegateForSetDelegateAPI = delegate; [self addDeviceControllerDelegate:delegate queue:queue]; } @@ -1785,8 +1785,8 @@ - (void)setDeviceControllerDelegate:(id)delegate qu - (void)addDeviceControllerDelegate:(id)delegate queue:(dispatch_queue_t)queue { - @synchronized (self) { - MTRDeviceControllerDelegateInfo *newDelegateInfo = [[MTRDeviceControllerDelegateInfo alloc] initWithDelegate:delegate queue:queue]; + @synchronized(self) { + MTRDeviceControllerDelegateInfo * newDelegateInfo = [[MTRDeviceControllerDelegateInfo alloc] initWithDelegate:delegate queue:queue]; [_delegates addObject:newDelegateInfo]; MTR_LOG("%@ addDeviceControllerDelegate: added %p total %lu", self, delegate, static_cast(_delegates.count)); } @@ -1794,9 +1794,9 @@ - (void)addDeviceControllerDelegate:(id)delegate qu - (void)removeDeviceControllerDelegate:(id)delegate { - @synchronized (self) { - __block MTRDeviceControllerDelegateInfo *delegateInfoToRemove = nil; - [self _iterateDelegateInfoWithBlock:^(MTRDeviceControllerDelegateInfo *delegateInfo) { + @synchronized(self) { + __block MTRDeviceControllerDelegateInfo * delegateInfoToRemove = nil; + [self _iterateDelegateInfoWithBlock:^(MTRDeviceControllerDelegateInfo * delegateInfo) { if (delegateInfo.delegate == delegate) { delegateInfoToRemove = delegateInfo; } @@ -1816,17 +1816,17 @@ - (void)removeDeviceControllerDelegate:(id)delegate // Iterates the delegates, and remove delegate info objects if the delegate object has dealloc'ed // Returns number of delegates called -- (NSUInteger)_iterateDelegateInfoWithBlock:(void(^ _Nullable)(MTRDeviceControllerDelegateInfo * delegateInfo))block +- (NSUInteger)_iterateDelegateInfoWithBlock:(void (^_Nullable)(MTRDeviceControllerDelegateInfo * delegateInfo))block { - @synchronized (self) { + @synchronized(self) { if (!_delegates.count) { MTR_LOG("%@ No delegates to iterate", self); return 0; } // Opportunistically remove defunct delegate references on every iteration - NSMutableSet *delegatesToRemove = nil; - for (MTRDeviceControllerDelegateInfo *delegateInfo in _delegates) { + NSMutableSet * delegatesToRemove = nil; + for (MTRDeviceControllerDelegateInfo * delegateInfo in _delegates) { id strongDelegate = delegateInfo.delegate; if (strongDelegate) { if (block) { @@ -1849,9 +1849,9 @@ - (NSUInteger)_iterateDelegateInfoWithBlock:(void(^ _Nullable)(MTRDeviceControll } } -- (void)_callDelegatesWithBlock:(void(^ _Nullable)(id delegate))block logString:(const char *)logString; +- (void)_callDelegatesWithBlock:(void (^_Nullable)(id delegate))block logString:(const char *)logString; { - NSUInteger delegatesCalled = [self _iterateDelegateInfoWithBlock:^(MTRDeviceControllerDelegateInfo *delegateInfo) { + NSUInteger delegatesCalled = [self _iterateDelegateInfoWithBlock:^(MTRDeviceControllerDelegateInfo * delegateInfo) { id strongDelegate = delegateInfo.delegate; dispatch_async(delegateInfo.queue, ^{ block(strongDelegate); @@ -1880,9 +1880,9 @@ - (void)controller:(MTRDeviceController *)controller commissioningSessionEstabli } - (void)controller:(MTRDeviceController *)controller -commissioningComplete:(NSError * _Nullable)error - nodeID:(NSNumber * _Nullable)nodeID - metrics:(MTRMetrics *)metrics + commissioningComplete:(NSError * _Nullable)error + nodeID:(NSNumber * _Nullable)nodeID + metrics:(MTRMetrics *)metrics { [self _callDelegatesWithBlock:^(id delegate) { if ([delegate respondsToSelector:@selector(controller:commissioningComplete:nodeID:metrics:)]) { From 2f54ea0f6d2dc9e477e6c74bb16317dfd12435fb Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Sun, 8 Sep 2024 12:52:44 -0700 Subject: [PATCH 3/7] Fixed setDeviceControllerDelegate --- src/darwin/Framework/CHIP/MTRDeviceController.mm | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index e622b5aff57aae..ac9ab10c697ccb 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -1778,6 +1778,10 @@ + (void)forceLocalhostAdvertisingOnly - (void)setDeviceControllerDelegate:(id)delegate queue:(dispatch_queue_t)queue { @synchronized(self) { + if (_strongDelegateForSetDelegateAPI) { + MTR_LOG("%@ setDeviceControllerDelegate: replacing %p with %p", self, _strongDelegateForSetDelegateAPI, delegate); + [self removeDeviceControllerDelegate:_strongDelegateForSetDelegateAPI]; + } _strongDelegateForSetDelegateAPI = delegate; [self addDeviceControllerDelegate:delegate queue:queue]; } From 3e07df2dc9cfdb3a8b524b070c430371828c4d87 Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Sun, 8 Sep 2024 14:03:56 -0700 Subject: [PATCH 4/7] Added unit test --- .../Framework/CHIP/MTRDefines_Internal.h | 4 ++ .../Framework/CHIP/MTRDeviceController.mm | 16 +++++ src/darwin/Framework/CHIP/MTRError_Internal.h | 4 -- .../Framework/CHIPTests/MTRPairingTests.m | 58 +++++++++++++++++++ .../TestHelpers/MTRTestDeclarations.h | 4 ++ 5 files changed, 82 insertions(+), 4 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDefines_Internal.h b/src/darwin/Framework/CHIP/MTRDefines_Internal.h index e22f00dd4e6a0b..94a1cbb3da61f4 100644 --- a/src/darwin/Framework/CHIP/MTRDefines_Internal.h +++ b/src/darwin/Framework/CHIP/MTRDefines_Internal.h @@ -150,3 +150,7 @@ typedef struct {} variable_hidden_by_mtr_hide; } \ } #endif + +#ifndef YES_NO +#define YES_NO(x) ((x) ? @"YES" : @"NO") +#endif diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index ac9ab10c697ccb..9dc9f5a147db81 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -1779,6 +1779,11 @@ - (void)setDeviceControllerDelegate:(id)delegate qu { @synchronized(self) { if (_strongDelegateForSetDelegateAPI) { + if (_strongDelegateForSetDelegateAPI == delegate) { + MTR_LOG("%@ setDeviceControllerDelegate: delegate %p is already set", self, delegate); + return; + } + MTR_LOG("%@ setDeviceControllerDelegate: replacing %p with %p", self, _strongDelegateForSetDelegateAPI, delegate); [self removeDeviceControllerDelegate:_strongDelegateForSetDelegateAPI]; } @@ -1799,6 +1804,10 @@ - (void)addDeviceControllerDelegate:(id)delegate qu - (void)removeDeviceControllerDelegate:(id)delegate { @synchronized(self) { + if (_strongDelegateForSetDelegateAPI == delegate) { + _strongDelegateForSetDelegateAPI = nil; + } + __block MTRDeviceControllerDelegateInfo * delegateInfoToRemove = nil; [self _iterateDelegateInfoWithBlock:^(MTRDeviceControllerDelegateInfo * delegateInfo) { if (delegateInfo.delegate == delegate) { @@ -1865,6 +1874,13 @@ - (void)_callDelegatesWithBlock:(void (^_Nullable)(id(delegatesCalled), logString); } +#if DEBUG +- (NSUInteger)unitTestDelegateCount +{ + return [self _iterateDelegateInfoWithBlock:nil]; +} +#endif + - (void)controller:(MTRDeviceController *)controller statusUpdate:(MTRCommissioningStatus)status { [self _callDelegatesWithBlock:^(id delegate) { diff --git a/src/darwin/Framework/CHIP/MTRError_Internal.h b/src/darwin/Framework/CHIP/MTRError_Internal.h index af19f4888ff050..c79cc17d95f198 100644 --- a/src/darwin/Framework/CHIP/MTRError_Internal.h +++ b/src/darwin/Framework/CHIP/MTRError_Internal.h @@ -26,10 +26,6 @@ NS_ASSUME_NONNULL_BEGIN -#ifndef YES_NO -#define YES_NO(x) ((x) ? @"YES" : @"NO") -#endif - MTR_DIRECT_MEMBERS @interface MTRError : NSObject + (NSError *)errorWithCode:(MTRErrorCode)code; diff --git a/src/darwin/Framework/CHIPTests/MTRPairingTests.m b/src/darwin/Framework/CHIPTests/MTRPairingTests.m index cd1802dff3dd47..807e9b046e2fdc 100644 --- a/src/darwin/Framework/CHIPTests/MTRPairingTests.m +++ b/src/darwin/Framework/CHIPTests/MTRPairingTests.m @@ -18,9 +18,11 @@ // module headers #import +#import "MTRDefines_Internal.h" #import "MTRErrorTestUtils.h" #import "MTRTestCase+ServerAppRunner.h" #import "MTRTestCase.h" +#import "MTRTestDeclarations.h" #import "MTRTestKeys.h" #import "MTRTestStorage.h" @@ -131,6 +133,42 @@ - (void)controller:(MTRDeviceController *)controller commissioningComplete:(NSEr @end +@interface MTRPairingTestMonitoringControllerDelegate : NSObject +@property(nonatomic, readonly) BOOL statusUpdateCalled; +@property(nonatomic, readonly) BOOL commissioningSessionEstablishmentDoneCalled; +@property(nonatomic, readonly) BOOL commissioningCompleteCalled; +@property(nonatomic, readonly) BOOL readCommissioningInfoCalled; +@end + +@implementation MTRPairingTestMonitoringControllerDelegate +- (NSString *)description +{ + return [NSString stringWithFormat:@"", self, YES_NO(_statusUpdateCalled), YES_NO(_commissioningSessionEstablishmentDoneCalled), YES_NO(_commissioningCompleteCalled), YES_NO(_readCommissioningInfoCalled)]; +} +- (void)controller:(MTRDeviceController *)controller statusUpdate:(MTRCommissioningStatus)status +{ + _statusUpdateCalled = YES; +} + +- (void)controller:(MTRDeviceController *)controller commissioningSessionEstablishmentDone:(NSError * _Nullable)error +{ + _commissioningSessionEstablishmentDoneCalled = YES; +} + +- (void)controller:(MTRDeviceController *)controller + commissioningComplete:(NSError * _Nullable)error + nodeID:(NSNumber * _Nullable)nodeID + metrics:(MTRMetrics *)metrics +{ + _commissioningCompleteCalled = YES; +} + +- (void)controller:(MTRDeviceController *)controller readCommissioningInfo:(MTRProductIdentity *)info +{ + _readCommissioningInfoCalled = YES; +} +@end + @interface MTRPairingTests : MTRTestCase @property (nullable) MTRPairingTestControllerDelegate * controllerDelegate; @end @@ -219,6 +257,19 @@ - (void)doPairingTestWithAttestationDelegate:(id)a [sController setDeviceControllerDelegate:controllerDelegate queue:callbackQueue]; self.controllerDelegate = controllerDelegate; + // Test that a monitoring delegate works + __auto_type * monitoringControllerDelegate = [[MTRPairingTestMonitoringControllerDelegate alloc] init]; + [sController addDeviceControllerDelegate:monitoringControllerDelegate queue:callbackQueue]; + XCTAssertEqual([sController unitTestDelegateCount], 2); + + // Test that the addDeviceControllerDelegate delegate is held weakly by the controller + @autoreleasepool { + __auto_type * monitoringControllerDelegate = [[MTRPairingTestMonitoringControllerDelegate alloc] init]; + [sController addDeviceControllerDelegate:monitoringControllerDelegate queue:callbackQueue]; + XCTAssertEqual([sController unitTestDelegateCount], 3); + } + XCTAssertEqual([sController unitTestDelegateCount], 2); + NSError * error; __auto_type * payload = [MTRSetupPayload setupPayloadWithOnboardingPayload:kOnboardingPayload error:&error]; XCTAssertNotNil(payload); @@ -229,6 +280,13 @@ - (void)doPairingTestWithAttestationDelegate:(id)a [self waitForExpectations:@[ expectation ] timeout:kPairingTimeoutInSeconds]; XCTAssertNil(controllerDelegate.commissioningCompleteError); + + // Test that the monitoring delegate got all the callbacks + XCTAssertTrue(monitoringControllerDelegate.statusUpdateCalled); + XCTAssertTrue(monitoringControllerDelegate.commissioningSessionEstablishmentDoneCalled); + XCTAssertTrue(monitoringControllerDelegate.commissioningCompleteCalled); + XCTAssertTrue(monitoringControllerDelegate.readCommissioningInfoCalled); + [sController removeDeviceControllerDelegate:monitoringControllerDelegate]; } - (void)test001_PairWithoutAttestationDelegate diff --git a/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestDeclarations.h b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestDeclarations.h index 46d6c61e950f2d..22998eb4496216 100644 --- a/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestDeclarations.h +++ b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestDeclarations.h @@ -18,6 +18,9 @@ #import #import +// For MTRDeviceDataValueDictionary: +#import "MTRDevice_Internal.h" + NS_ASSUME_NONNULL_BEGIN #pragma mark - Declarations for internal methods @@ -55,6 +58,7 @@ NS_ASSUME_NONNULL_BEGIN #ifdef DEBUG @interface MTRDeviceController (TestDebug) - (NSDictionary *)unitTestGetDeviceAttributeCounts; +- (NSUInteger)unitTestDelegateCount; @end @interface MTRBaseDevice (TestDebug) From 0f6d069bc1e53ee57a970d435a0639641e4334df Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Sun, 8 Sep 2024 14:04:34 -0700 Subject: [PATCH 5/7] Restyled --- src/darwin/Framework/CHIPTests/MTRPairingTests.m | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/darwin/Framework/CHIPTests/MTRPairingTests.m b/src/darwin/Framework/CHIPTests/MTRPairingTests.m index 807e9b046e2fdc..b2470c4bd7dde9 100644 --- a/src/darwin/Framework/CHIPTests/MTRPairingTests.m +++ b/src/darwin/Framework/CHIPTests/MTRPairingTests.m @@ -134,10 +134,10 @@ - (void)controller:(MTRDeviceController *)controller commissioningComplete:(NSEr @end @interface MTRPairingTestMonitoringControllerDelegate : NSObject -@property(nonatomic, readonly) BOOL statusUpdateCalled; -@property(nonatomic, readonly) BOOL commissioningSessionEstablishmentDoneCalled; -@property(nonatomic, readonly) BOOL commissioningCompleteCalled; -@property(nonatomic, readonly) BOOL readCommissioningInfoCalled; +@property (nonatomic, readonly) BOOL statusUpdateCalled; +@property (nonatomic, readonly) BOOL commissioningSessionEstablishmentDoneCalled; +@property (nonatomic, readonly) BOOL commissioningCompleteCalled; +@property (nonatomic, readonly) BOOL readCommissioningInfoCalled; @end @implementation MTRPairingTestMonitoringControllerDelegate From df36535a69cbe05e0950e54c052f36e2c07d2318 Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Sun, 8 Sep 2024 15:19:07 -0700 Subject: [PATCH 6/7] Unit test fix for TSAN --- src/darwin/Framework/CHIPTests/MTRPairingTests.m | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/darwin/Framework/CHIPTests/MTRPairingTests.m b/src/darwin/Framework/CHIPTests/MTRPairingTests.m index b2470c4bd7dde9..4e597961603ee9 100644 --- a/src/darwin/Framework/CHIPTests/MTRPairingTests.m +++ b/src/darwin/Framework/CHIPTests/MTRPairingTests.m @@ -134,10 +134,10 @@ - (void)controller:(MTRDeviceController *)controller commissioningComplete:(NSEr @end @interface MTRPairingTestMonitoringControllerDelegate : NSObject -@property (nonatomic, readonly) BOOL statusUpdateCalled; -@property (nonatomic, readonly) BOOL commissioningSessionEstablishmentDoneCalled; -@property (nonatomic, readonly) BOOL commissioningCompleteCalled; -@property (nonatomic, readonly) BOOL readCommissioningInfoCalled; +@property (atomic, readwrite) BOOL statusUpdateCalled; +@property (atomic, readwrite) BOOL commissioningSessionEstablishmentDoneCalled; +@property (atomic, readwrite) BOOL commissioningCompleteCalled; +@property (atomic, readwrite) BOOL readCommissioningInfoCalled; @end @implementation MTRPairingTestMonitoringControllerDelegate @@ -147,12 +147,12 @@ - (NSString *)description } - (void)controller:(MTRDeviceController *)controller statusUpdate:(MTRCommissioningStatus)status { - _statusUpdateCalled = YES; + self.statusUpdateCalled = YES; } - (void)controller:(MTRDeviceController *)controller commissioningSessionEstablishmentDone:(NSError * _Nullable)error { - _commissioningSessionEstablishmentDoneCalled = YES; + self.commissioningSessionEstablishmentDoneCalled = YES; } - (void)controller:(MTRDeviceController *)controller @@ -160,12 +160,12 @@ - (void)controller:(MTRDeviceController *)controller nodeID:(NSNumber * _Nullable)nodeID metrics:(MTRMetrics *)metrics { - _commissioningCompleteCalled = YES; + self.commissioningCompleteCalled = YES; } - (void)controller:(MTRDeviceController *)controller readCommissioningInfo:(MTRProductIdentity *)info { - _readCommissioningInfoCalled = YES; + self.readCommissioningInfoCalled = YES; } @end From 98be8ea7ec3aab7ad33948346de3883636f0521e Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Mon, 9 Sep 2024 16:22:16 -0700 Subject: [PATCH 7/7] Use matter queue for device controller callback queue to avoid commandline tool main queue blockage --- src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm index 78aa16969f3342..31de6111a328e3 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm @@ -719,7 +719,7 @@ - (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams // Set self as delegate, which fans out delegate callbacks to all added delegates id selfDelegate = static_cast>(self); - self->_deviceControllerDelegateBridge->setDelegate(self, selfDelegate, dispatch_get_main_queue()); + self->_deviceControllerDelegateBridge->setDelegate(self, selfDelegate, _chipWorkQueue); MTR_LOG("%@ startup succeeded for nodeID 0x%016llX", self, self->_cppCommissioner->GetNodeId()); });