[Darwin] Follow-up changes for PR#35475 (project-chip#35497)

* [Darwin] Follow-up changes for PR#35475

* restyled

* Additional fixes and unit test fix, and rebased to current master

src/darwin/Framework/CHIP/MTRDefines_Internal.h

@@ -151,6 +151,4 @@ typedef struct {} variable_hidden_by_mtr_hide;
     }
 #endif
 
-#ifndef YES_NO
-#define YES_NO(x) ((x) ? @"YES" : @"NO")
-#endif
+#define MTR_YES_NO(x) ((x) ? @"YES" : @"NO")

src/darwin/Framework/CHIP/MTRDevice.mm

@@ -1158,7 +1158,7 @@ - (nullable MTRDeviceClusterData *)_clusterDataForPath:(MTRClusterPath *)cluster
 
     // Page in the stored value for the data.
     MTRDeviceClusterData * data = [_deviceController.controllerDataStore getStoredClusterDataForNodeID:_nodeID endpointID:clusterPath.endpoint clusterID:clusterPath.cluster];
-    MTR_LOG("%@ cluster path %@ cache miss - load from storage success %@", self, clusterPath, YES_NO(data));
+    MTR_LOG("%@ cluster path %@ cache miss - load from storage success %@", self, clusterPath, MTR_YES_NO(data));
     if (data != nil) {
         [_persistedClusterData setObject:data forKey:clusterPath];
     } else {

src/darwin/Framework/CHIP/MTRDeviceController.h

@@ -198,6 +198,13 @@ MTR_AVAILABLE(ios(16.1), macos(13.0), watchos(9.1), tvos(16.1))
 /**
  * Adds a Delegate to the device controller as well as the Queue on which the Delegate callbacks will be triggered
  *
+ * Multiple delegates can be added to monitor MTRDeviceController state changes. Note that there should only
+ * be one delegate that responds to pairing related callbacks.
+ *
+ * If a delegate is added a second time, the call would be ignored.
+ *
+ * All delegates are held by weak references, and so if a delegate object goes away, it will be automatically removed.
+ *
  * @param[in] delegate The delegate the commissioning process should use
  *
  * @param[in] queue The queue on which the callbacks will be delivered

src/darwin/Framework/CHIP/MTRDeviceController.mm

@@ -165,7 +165,7 @@ @implementation MTRDeviceController {
     BOOL _shutdownPending;
     os_unfair_lock _assertionLock;
 
-    NSMutableSet<MTRDeviceControllerDelegateInfo *> * _delegates;
+    NSMutableArray<MTRDeviceControllerDelegateInfo *> * _delegates;
     id<MTRDeviceControllerDelegate> _strongDelegateForSetDelegateAPI;
 }
 
@@ -196,7 +196,7 @@ - (instancetype)initForSubclasses:(BOOL)startSuspended
 
     _nodeIDToDeviceMap = [NSMapTable strongToWeakObjectsMapTable];
 
-    _delegates = [NSMutableSet set];
+    _delegates = [NSMutableArray array];
 
     return self;
 }
@@ -1793,7 +1793,7 @@ + (void)forceLocalhostAdvertisingOnly
 
 #pragma mark - MTRDeviceControllerDelegate management
 
-// Note these are implemented in the base class so that XPC subclass can use it as well when it
+// Note these are implemented in the base class so that XPC subclass can use it as well
 - (void)setDeviceControllerDelegate:(id<MTRDeviceControllerDelegate>)delegate queue:(dispatch_queue_t)queue
 {
     @synchronized(self) {
@@ -1814,6 +1814,17 @@ - (void)setDeviceControllerDelegate:(id<MTRDeviceControllerDelegate>)delegate qu
 - (void)addDeviceControllerDelegate:(id<MTRDeviceControllerDelegate>)delegate queue:(dispatch_queue_t)queue
 {
     @synchronized(self) {
+        __block BOOL delegateAlreadyAdded = NO;
+        [self _iterateDelegateInfoWithBlock:^(MTRDeviceControllerDelegateInfo * delegateInfo) {
+            if (delegateInfo.delegate == delegate) {
+                delegateAlreadyAdded = YES;
+            }
+        }];
+        if (delegateAlreadyAdded) {
+            MTR_LOG("%@ addDeviceControllerDelegate: delegate already added", self);
+            return;
+        }
+
         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));
@@ -1836,9 +1847,6 @@ - (void)removeDeviceControllerDelegate:(id<MTRDeviceControllerDelegate>)delegate
 
         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));
@@ -1857,7 +1865,7 @@ - (NSUInteger)_iterateDelegateInfoWithBlock:(void (^_Nullable)(MTRDeviceControll
         }
 
         // Opportunistically remove defunct delegate references on every iteration
-        NSMutableSet * delegatesToRemove = nil;
+        NSMutableArray * delegatesToRemove = nil;
         for (MTRDeviceControllerDelegateInfo * delegateInfo in _delegates) {
             id<MTRDeviceControllerDelegate> strongDelegate = delegateInfo.delegate;
             if (strongDelegate) {
@@ -1866,14 +1874,14 @@ - (NSUInteger)_iterateDelegateInfoWithBlock:(void (^_Nullable)(MTRDeviceControll
                 }
             } else {
                 if (!delegatesToRemove) {
-                    delegatesToRemove = [NSMutableSet set];
+                    delegatesToRemove = [NSMutableArray array];
                 }
                 [delegatesToRemove addObject:delegateInfo];
             }
         }
 
         if (delegatesToRemove.count) {
-            [_delegates minusSet:delegatesToRemove];
+            [_delegates removeObjectsInArray:delegatesToRemove];
             MTR_LOG("%@ _iterateDelegatesWithBlock: removed %lu remaining %lu", self, static_cast<unsigned long>(delegatesToRemove.count), static_cast<unsigned long>(_delegates.count));
         }
 

src/darwin/Framework/CHIP/MTRDevice_Concrete.mm

@@ -445,8 +445,8 @@ - (NSString *)description
         wifi = @"NO";
         thread = @"NO";
     } else {
-        wifi = YES_NO(networkFeatures.unsignedLongLongValue & MTRNetworkCommissioningFeatureWiFiNetworkInterface);
-        thread = YES_NO(networkFeatures.unsignedLongLongValue & MTRNetworkCommissioningFeatureThreadNetworkInterface);
+        wifi = MTR_YES_NO(networkFeatures.unsignedLongLongValue & MTRNetworkCommissioningFeatureWiFiNetworkInterface);
+        thread = MTR_YES_NO(networkFeatures.unsignedLongLongValue & MTRNetworkCommissioningFeatureThreadNetworkInterface);
     }
 
     NSString * reportAge;
@@ -2103,7 +2103,7 @@ - (nullable MTRDeviceClusterData *)_clusterDataForPath:(MTRClusterPath *)cluster
 
     // Page in the stored value for the data.
     MTRDeviceClusterData * data = [_deviceController.controllerDataStore getStoredClusterDataForNodeID:_nodeID endpointID:clusterPath.endpoint clusterID:clusterPath.cluster];
-    MTR_LOG("%@ cluster path %@ cache miss - load from storage success %@", self, clusterPath, YES_NO(data));
+    MTR_LOG("%@ cluster path %@ cache miss - load from storage success %@", self, clusterPath, MTR_YES_NO(data));
     if (data != nil) {
         [_persistedClusterData setObject:data forKey:clusterPath];
     } else {

src/darwin/Framework/CHIP/MTRDevice_XPC.mm

@@ -111,8 +111,8 @@ - (NSString *)description
         wifi = @"NO";
         thread = @"NO";
     } else {
-        wifi = YES_NO(networkFeatures.unsignedLongLongValue & MTRNetworkCommissioningFeatureWiFiNetworkInterface);
-        thread = YES_NO(networkFeatures.unsignedLongLongValue & MTRNetworkCommissioningFeatureThreadNetworkInterface);
+        wifi = MTR_YES_NO(networkFeatures.unsignedLongLongValue & MTRNetworkCommissioningFeatureWiFiNetworkInterface);
+        thread = MTR_YES_NO(networkFeatures.unsignedLongLongValue & MTRNetworkCommissioningFeatureThreadNetworkInterface);
     }
 
     // TODO: Add these to the description

src/darwin/Framework/CHIPTests/MTRPairingTests.m

@@ -138,21 +138,35 @@ @interface MTRPairingTestMonitoringControllerDelegate : NSObject <MTRDeviceContr
 @property (atomic, readwrite) BOOL commissioningSessionEstablishmentDoneCalled;
 @property (atomic, readwrite) BOOL commissioningCompleteCalled;
 @property (atomic, readwrite) BOOL readCommissioningInfoCalled;
+@property (atomic, readwrite, strong) XCTestExpectation * allCallbacksCalledExpectation;
 @end
 
 @implementation MTRPairingTestMonitoringControllerDelegate
 - (NSString *)description
 {
-    return [NSString stringWithFormat:@"<MTRPairingTestMonitoringControllerDelegate: %p statusUpdateCalled %@ commissioningSessionEstablishmentDoneCalled %@ commissioningCompleteCalled %@ readCommissioningInfoCalled %@>", self, YES_NO(_statusUpdateCalled), YES_NO(_commissioningSessionEstablishmentDoneCalled), YES_NO(_commissioningCompleteCalled), YES_NO(_readCommissioningInfoCalled)];
+    return [NSString stringWithFormat:@"<MTRPairingTestMonitoringControllerDelegate: %p statusUpdateCalled %@ commissioningSessionEstablishmentDoneCalled %@ commissioningCompleteCalled %@ readCommissioningInfoCalled %@>", self, MTR_YES_NO(_statusUpdateCalled), MTR_YES_NO(_commissioningSessionEstablishmentDoneCalled), MTR_YES_NO(_commissioningCompleteCalled), MTR_YES_NO(_readCommissioningInfoCalled)];
 }
+
+- (void)_checkIfAllCallbacksCalled
+{
+    if (self.allCallbacksCalledExpectation) {
+        if (self.statusUpdateCalled && self.commissioningSessionEstablishmentDoneCalled && self.commissioningCompleteCalled && self.readCommissioningInfoCalled) {
+            [self.allCallbacksCalledExpectation fulfill];
+            self.allCallbacksCalledExpectation = nil;
+        }
+    }
+}
+
 - (void)controller:(MTRDeviceController *)controller statusUpdate:(MTRCommissioningStatus)status
 {
     self.statusUpdateCalled = YES;
+    [self _checkIfAllCallbacksCalled];
 }
 
 - (void)controller:(MTRDeviceController *)controller commissioningSessionEstablishmentDone:(NSError * _Nullable)error
 {
     self.commissioningSessionEstablishmentDoneCalled = YES;
+    [self _checkIfAllCallbacksCalled];
 }
 
 - (void)controller:(MTRDeviceController *)controller
@@ -161,11 +175,13 @@ - (void)controller:(MTRDeviceController *)controller
                   metrics:(MTRMetrics *)metrics
 {
     self.commissioningCompleteCalled = YES;
+    [self _checkIfAllCallbacksCalled];
 }
 
 - (void)controller:(MTRDeviceController *)controller readCommissioningInfo:(MTRProductIdentity *)info
 {
     self.readCommissioningInfoCalled = YES;
+    [self _checkIfAllCallbacksCalled];
 }
 @end
 
@@ -259,6 +275,8 @@ - (void)doPairingTestWithAttestationDelegate:(id<MTRDeviceAttestationDelegate>)a
 
     // Test that a monitoring delegate works
     __auto_type * monitoringControllerDelegate = [[MTRPairingTestMonitoringControllerDelegate alloc] init];
+    XCTestExpectation * allCallbacksCalledExpectation = [self expectationWithDescription:@"All callbacks called on monitoring delegate"];
+    monitoringControllerDelegate.allCallbacksCalledExpectation = allCallbacksCalledExpectation;
     [sController addDeviceControllerDelegate:monitoringControllerDelegate queue:callbackQueue];
     XCTAssertEqual([sController unitTestDelegateCount], 2);
 
@@ -278,7 +296,7 @@ - (void)doPairingTestWithAttestationDelegate:(id<MTRDeviceAttestationDelegate>)a
     XCTAssertTrue([sController setupCommissioningSessionWithPayload:payload newNodeID:@(sDeviceId) error:&error]);
     XCTAssertNil(error);
 
-    [self waitForExpectations:@[ expectation ] timeout:kPairingTimeoutInSeconds];
+    [self waitForExpectations:@[ expectation, allCallbacksCalledExpectation ] timeout:kPairingTimeoutInSeconds];
     XCTAssertNil(controllerDelegate.commissioningCompleteError);
 
     // Test that the monitoring delegate got all the callbacks