[Darwin] Implement delegate add/remove for device controller (project…

…-chip#35475) * [Darwin] Implement delegate add/remove for device controller * Restyled changes * Fixed setDeviceControllerDelegate * Added unit test * Restyled * Unit test fix for TSAN * Use matter queue for device controller callback queue to avoid commandline tool main queue blockage
jtung-apple · Sep 10, 2024 · 01632c4 · 01632c4
1 parent 2d0135d
commit 01632c4
Show file tree

Hide file tree

Showing 8 changed files with 271 additions and 32 deletions.
diff --git 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.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<MTRDeviceControllerDelegate>)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<MTRDeviceControllerDelegate>)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<MTRDeviceControllerDelegate>)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
@@ -112,6 +112,27 @@
 
 using namespace chip::Tracing::DarwinFramework;
 
+@interface MTRDeviceControllerDelegateInfo : NSObject
+- (instancetype)initWithDelegate:(id<MTRDeviceControllerDelegate>)delegate queue:(dispatch_queue_t)queue;
+@property (nonatomic, weak, readonly) id<MTRDeviceControllerDelegate> delegate;
+@property (nonatomic, readonly) dispatch_queue_t queue;
+@end
+
+@implementation MTRDeviceControllerDelegateInfo
+@synthesize delegate = _delegate, queue = _queue;
+- (instancetype)initWithDelegate:(id<MTRDeviceControllerDelegate>)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<MTRDeviceControllerDelegateInfo *> * _delegates;
+    id<MTRDeviceControllerDelegate> _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<MTRDeviceControllerDelegate>)delegate queue:(dispatch_queue_t)queue
-{
-    [self
-        asyncDispatchToMatterQueue:^() {
-            self->_deviceControllerDelegateBridge->setDelegate(self, delegate, queue);
-        }
-                      errorHandler:nil];
-}
-
 - (BOOL)setOperationalCertificateIssuer:(nullable id<MTROperationalCertificateIssuer>)operationalCertificateIssuer
                                   queue:(nullable dispatch_queue_t)queue
 {
@@ -1751,6 +1772,158 @@ + (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<MTRDeviceControllerDelegate>)delegate queue:(dispatch_queue_t)queue
+{
+    @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];
+        }
+        _strongDelegateForSetDelegateAPI = delegate;
+        [self addDeviceControllerDelegate:delegate queue:queue];
+    }
+}
+
+- (void)addDeviceControllerDelegate:(id<MTRDeviceControllerDelegate>)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<unsigned long>(_delegates.count));
+    }
+}
+
+- (void)removeDeviceControllerDelegate:(id<MTRDeviceControllerDelegate>)delegate
+{
+    @synchronized(self) {
+        if (_strongDelegateForSetDelegateAPI == delegate) {
+            _strongDelegateForSetDelegateAPI = nil;
+        }
+
+        __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<unsigned long>(_delegates.count));
+        } else {
+            MTR_LOG("%@ removeDeviceControllerDelegate: delegate %p not found in %lu", self, delegate, static_cast<unsigned long>(_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<MTRDeviceControllerDelegate> 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<unsigned long>(delegatesToRemove.count), static_cast<unsigned long>(_delegates.count));
+        }
+
+        return _delegates.count;
+    }
+}
+
+- (void)_callDelegatesWithBlock:(void (^_Nullable)(id<MTRDeviceControllerDelegate> delegate))block logString:(const char *)logString;
+{
+    NSUInteger delegatesCalled = [self _iterateDelegateInfoWithBlock:^(MTRDeviceControllerDelegateInfo * delegateInfo) {
+        id<MTRDeviceControllerDelegate> strongDelegate = delegateInfo.delegate;
+        dispatch_async(delegateInfo.queue, ^{
+            block(strongDelegate);
+        });
+    }];
+
+    MTR_LOG("%@ %lu delegates called for %s", self, static_cast<unsigned long>(delegatesCalled), logString);
+}
+
+#if DEBUG
+- (NSUInteger)unitTestDelegateCount
+{
+    return [self _iterateDelegateInfoWithBlock:nil];
+}
+#endif
+
+- (void)controller:(MTRDeviceController *)controller statusUpdate:(MTRCommissioningStatus)status
+{
+    [self _callDelegatesWithBlock:^(id<MTRDeviceControllerDelegate> 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<MTRDeviceControllerDelegate> 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<MTRDeviceControllerDelegate> 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<MTRDeviceControllerDelegate> 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
@@ -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<MTRDeviceControllerDelegate>)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
@@ -717,6 +717,10 @@ - (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams
 
         commissionerInitialized = YES;
 
+        // Set self as delegate, which fans out delegate callbacks to all added delegates
+        id<MTRDeviceControllerDelegate> selfDelegate = static_cast<id<MTRDeviceControllerDelegate>>(self);
+        self->_deviceControllerDelegateBridge->setDelegate(self, selfDelegate, _chipWorkQueue);
+
         MTR_LOG("%@ startup succeeded for nodeID 0x%016llX", self, self->_cppCommissioner->GetNodeId());
     });
 
@@ -1185,15 +1189,6 @@ - (void)removeDevice:(MTRDevice *)device
 }
 #endif
 
-- (void)setDeviceControllerDelegate:(id<MTRDeviceControllerDelegate>)delegate queue:(dispatch_queue_t)queue
-{
-    [self
-        asyncDispatchToMatterQueue:^() {
-            self->_deviceControllerDelegateBridge->setDelegate(self, delegate, queue);
-        }
-                      errorHandler:nil];
-}
-
 - (BOOL)setOperationalCertificateIssuer:(nullable id<MTROperationalCertificateIssuer>)operationalCertificateIssuer
                                   queue:(nullable dispatch_queue_t)queue
 {

diff --git 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;
-Original file line number
+Diff line change
@@ Expand Up / @@ -150,3 +150,7 @@ typedef struct {} variable_hidden_by_mtr_hide; @@
             }                                                                                                                  \
         }
     #endif
+    #ifndef YES_NO
+    #define YES_NO(x) ((x) ? @"YES" : @"NO")
+    #endif