[Darwin] Follow-up changes for PR#35475 (#35497)
* [Darwin] Follow-up changes for PR#35475
* restyled
* Additional fixes and unit test fix, and rebased to current master
diff --git a/src/darwin/Framework/CHIP/MTRDefines_Internal.h b/src/darwin/Framework/CHIP/MTRDefines_Internal.h
index 9832267..14683f3 100644
--- a/src/darwin/Framework/CHIP/MTRDefines_Internal.h
+++ b/src/darwin/Framework/CHIP/MTRDefines_Internal.h
@@ -151,6 +151,4 @@
}
#endif
-#ifndef YES_NO
-#define YES_NO(x) ((x) ? @"YES" : @"NO")
-#endif
+#define MTR_YES_NO(x) ((x) ? @"YES" : @"NO")
diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm
index f323c22..a0f985b 100644
--- a/src/darwin/Framework/CHIP/MTRDevice.mm
+++ b/src/darwin/Framework/CHIP/MTRDevice.mm
@@ -1158,7 +1158,7 @@
// 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 {
diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.h b/src/darwin/Framework/CHIP/MTRDeviceController.h
index d731b13..0b553c9 100644
--- a/src/darwin/Framework/CHIP/MTRDeviceController.h
+++ b/src/darwin/Framework/CHIP/MTRDeviceController.h
@@ -198,6 +198,13 @@
/**
* 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
diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm
index dc36aeb..b84505f 100644
--- a/src/darwin/Framework/CHIP/MTRDeviceController.mm
+++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm
@@ -165,7 +165,7 @@
BOOL _shutdownPending;
os_unfair_lock _assertionLock;
- NSMutableSet<MTRDeviceControllerDelegateInfo *> * _delegates;
+ NSMutableArray<MTRDeviceControllerDelegateInfo *> * _delegates;
id<MTRDeviceControllerDelegate> _strongDelegateForSetDelegateAPI;
}
@@ -196,7 +196,7 @@
_nodeIDToDeviceMap = [NSMapTable strongToWeakObjectsMapTable];
- _delegates = [NSMutableSet set];
+ _delegates = [NSMutableArray array];
return self;
}
@@ -1793,7 +1793,7 @@
#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)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 @@
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 @@
}
// 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 @@
}
} 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));
}
diff --git a/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm b/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm
index 6342a46..a70e495 100644
--- a/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm
+++ b/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm
@@ -445,8 +445,8 @@
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 @@
// 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 {
diff --git a/src/darwin/Framework/CHIP/MTRDevice_XPC.mm b/src/darwin/Framework/CHIP/MTRDevice_XPC.mm
index 2f7fb26..794696f 100644
--- a/src/darwin/Framework/CHIP/MTRDevice_XPC.mm
+++ b/src/darwin/Framework/CHIP/MTRDevice_XPC.mm
@@ -111,8 +111,8 @@
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
diff --git a/src/darwin/Framework/CHIPTests/MTRPairingTests.m b/src/darwin/Framework/CHIPTests/MTRPairingTests.m
index 4e59796..d23c7f3 100644
--- a/src/darwin/Framework/CHIPTests/MTRPairingTests.m
+++ b/src/darwin/Framework/CHIPTests/MTRPairingTests.m
@@ -138,21 +138,35 @@
@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 @@
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 @@
// 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 @@
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
