From 77602610c93e6aeb26a2a221865d5cbe3ea9f1b4 Mon Sep 17 00:00:00 2001 From: Kiel Oleson Date: Sat, 17 Aug 2024 19:09:24 -0700 Subject: [PATCH] [Darwin] more XPC service tweaks (#35048) * return `MTRDevice_XPC`s from XPC controller * more logging * move shadow property declarations to internal header * declare `_setupDeviceForNodeID` as common internal device controller method * prefetchedClusterData is nullable * fix a few properties that needed raising to base class * you get a log and you get a log EVERYONE GETS A LOG * convert device map lock for use in subclasses * check for optional delegate method impl before calling * ivar no longer necessary with accessor method underlying lock is the only state needed * Restyled by clang-format * remove more obsolete lock bits from `MTRDeviceController_XPC` --------- Co-authored-by: Restyled.io --- .../Framework/CHIP/MTRDeviceController.mm | 29 +++++++----- .../CHIP/MTRDeviceController_Concrete.mm | 44 ++++++++----------- .../CHIP/MTRDeviceController_Internal.h | 6 +++ .../Framework/CHIP/MTRDeviceController_XPC.mm | 21 ++++++++- src/darwin/Framework/CHIP/MTRDevice_XPC.mm | 26 +++++++++-- 5 files changed, 84 insertions(+), 42 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index 3483ff33320c31..bfbb62fa2fa8c8 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -120,7 +120,7 @@ @implementation MTRDeviceController { MTRDeviceAttestationDelegateBridge * _deviceAttestationDelegateBridge; MTRDeviceControllerFactory * _factory; NSMapTable * _nodeIDToDeviceMap; - os_unfair_lock _deviceMapLock; // protects nodeIDToDeviceMap + os_unfair_lock _underlyingDeviceMapLock; MTRCommissionableBrowser * _commissionableBrowser; MTRAttestationTrustStoreBridge * _attestationTrustStoreBridge; @@ -134,12 +134,17 @@ @implementation MTRDeviceController { MTRP256KeypairBridge _operationalKeypairBridge; } +- (os_unfair_lock_t)deviceMapLock +{ + return &_underlyingDeviceMapLock; +} + - (instancetype)initForSubclasses { if (self = [super init]) { // nothing, as superclass of MTRDeviceController is NSObject } - + _underlyingDeviceMapLock = OS_UNFAIR_LOCK_INIT; return self; } @@ -250,7 +255,7 @@ - (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory _chipWorkQueue = queue; _factory = factory; - _deviceMapLock = OS_UNFAIR_LOCK_INIT; + _underlyingDeviceMapLock = OS_UNFAIR_LOCK_INIT; _nodeIDToDeviceMap = [NSMapTable strongToWeakObjectsMapTable]; _serverEndpoints = [[NSMutableArray alloc] init]; _commissionableBrowser = nil; @@ -330,10 +335,10 @@ - (void)cleanupAfterStartup // while calling out into arbitrary invalidation code, snapshot the list of // devices before we start invalidating. MTR_LOG("cleanupAfterStartup MTRDeviceController: %@", self); - os_unfair_lock_lock(&_deviceMapLock); + os_unfair_lock_lock(self.deviceMapLock); NSEnumerator * devices = [_nodeIDToDeviceMap objectEnumerator]; [_nodeIDToDeviceMap removeAllObjects]; - os_unfair_lock_unlock(&_deviceMapLock); + os_unfair_lock_unlock(self.deviceMapLock); for (MTRDevice * device in devices) { [device invalidate]; @@ -634,7 +639,7 @@ - (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams [_controllerDataStore fetchAttributeDataForAllDevices:^(NSDictionary *> * _Nonnull clusterDataByNode) { MTR_LOG("%@ Loaded attribute values for %lu nodes from storage for controller uuid %@", self, static_cast(clusterDataByNode.count), self->_uniqueIdentifier); - std::lock_guard lock(self->_deviceMapLock); + std::lock_guard lock(*self.deviceMapLock); NSMutableArray * deviceList = [NSMutableArray array]; for (NSNumber * nodeID in clusterDataByNode) { NSDictionary * clusterData = clusterDataByNode[nodeID]; @@ -1004,7 +1009,7 @@ - (MTRBaseDevice *)baseDeviceForNodeID:(NSNumber *)nodeID // If prefetchedClusterData is not provided, load attributes individually from controller data store - (MTRDevice *)_setupDeviceForNodeID:(NSNumber *)nodeID prefetchedClusterData:(NSDictionary *)prefetchedClusterData { - os_unfair_lock_assert_owner(&_deviceMapLock); + os_unfair_lock_assert_owner(self.deviceMapLock); MTRDevice * deviceToReturn = [[MTRDevice_Concrete alloc] initWithNodeID:nodeID controller:self]; // If we're not running, don't add the device to our map. That would @@ -1043,7 +1048,7 @@ - (MTRDevice *)_setupDeviceForNodeID:(NSNumber *)nodeID prefetchedClusterData:(N - (MTRDevice *)deviceForNodeID:(NSNumber *)nodeID { - std::lock_guard lock(_deviceMapLock); + std::lock_guard lock(*self.deviceMapLock); MTRDevice * deviceToReturn = [_nodeIDToDeviceMap objectForKey:nodeID]; if (!deviceToReturn) { deviceToReturn = [self _setupDeviceForNodeID:nodeID prefetchedClusterData:nil]; @@ -1054,7 +1059,7 @@ - (MTRDevice *)deviceForNodeID:(NSNumber *)nodeID - (void)removeDevice:(MTRDevice *)device { - std::lock_guard lock(_deviceMapLock); + std::lock_guard lock(*self.deviceMapLock); auto * nodeID = device.nodeID; MTRDevice * deviceToRemove = [_nodeIDToDeviceMap objectForKey:nodeID]; if (deviceToRemove == device) { @@ -1068,7 +1073,7 @@ - (void)removeDevice:(MTRDevice *)device #ifdef DEBUG - (NSDictionary *)unitTestGetDeviceAttributeCounts { - std::lock_guard lock(_deviceMapLock); + std::lock_guard lock(*self.deviceMapLock); NSMutableDictionary * deviceAttributeCounts = [NSMutableDictionary dictionary]; for (NSNumber * nodeID in _nodeIDToDeviceMap) { deviceAttributeCounts[nodeID] = @([[_nodeIDToDeviceMap objectForKey:nodeID] unitTestAttributeCount]); @@ -1508,9 +1513,9 @@ - (void)operationalInstanceAdded:(chip::NodeId)nodeID { // Don't use deviceForNodeID here, because we don't want to create the // device if it does not already exist. - os_unfair_lock_lock(&_deviceMapLock); + os_unfair_lock_lock(self.deviceMapLock); MTRDevice * device = [_nodeIDToDeviceMap objectForKey:@(nodeID)]; - os_unfair_lock_unlock(&_deviceMapLock); + os_unfair_lock_unlock(self.deviceMapLock); if (device == nil) { return; diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm index d53d94b83db946..b3152a9e34c207 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm @@ -82,8 +82,6 @@ #include #include -#import - typedef void (^SyncWorkQueueBlock)(void); typedef id (^SyncWorkQueueBlockWithReturnValue)(void); typedef BOOL (^SyncWorkQueueBlockWithBoolReturnValue)(void); @@ -103,8 +101,6 @@ @interface MTRDeviceController_Concrete () @property (nonatomic, readwrite) NSUUID * uniqueIdentifier; @property (nonatomic, readonly) dispatch_queue_t chipWorkQueue; @property (nonatomic, readonly, nullable) MTRDeviceControllerFactory * factory; -@property (nonatomic, readonly, nullable) NSMapTable * nodeIDToDeviceMap; -@property (nonatomic, readonly) os_unfair_lock deviceMapLock; @property (nonatomic, readonly, nullable) id otaProviderDelegate; @property (nonatomic, readonly, nullable) dispatch_queue_t otaProviderDelegateQueue; @property (nonatomic, readonly, nullable) MTRCommissionableBrowser * commissionableBrowser; @@ -270,11 +266,9 @@ - (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory _otaProviderDelegate = otaProviderDelegate; _otaProviderDelegateQueue = otaProviderDelegateQueue; - _chipWorkQueue = queue; _factory = factory; - _deviceMapLock = OS_UNFAIR_LOCK_INIT; - _nodeIDToDeviceMap = [NSMapTable strongToWeakObjectsMapTable]; + self.nodeIDToDeviceMap = [NSMapTable strongToWeakObjectsMapTable]; _serverEndpoints = [[NSMutableArray alloc] init]; _commissionableBrowser = nil; @@ -352,10 +346,10 @@ - (void)cleanupAfterStartup // while calling out into arbitrary invalidation code, snapshot the list of // devices before we start invalidating. MTR_LOG("cleanupAfterStartup MTRDeviceController: %@", self); - os_unfair_lock_lock(&_deviceMapLock); - NSEnumerator * devices = [_nodeIDToDeviceMap objectEnumerator]; - [_nodeIDToDeviceMap removeAllObjects]; - os_unfair_lock_unlock(&_deviceMapLock); + os_unfair_lock_lock(self.deviceMapLock); + NSEnumerator * devices = [self.nodeIDToDeviceMap objectEnumerator]; + [self.nodeIDToDeviceMap removeAllObjects]; + os_unfair_lock_unlock(self.deviceMapLock); for (MTRDevice * device in devices) { [device invalidate]; @@ -654,7 +648,7 @@ - (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams [_controllerDataStore fetchAttributeDataForAllDevices:^(NSDictionary *> * _Nonnull clusterDataByNode) { MTR_LOG("%@ Loaded attribute values for %lu nodes from storage for controller uuid %@", self, static_cast(clusterDataByNode.count), self->_uniqueIdentifier); - std::lock_guard lock(self->_deviceMapLock); + std::lock_guard lock(*self.deviceMapLock); NSMutableArray * deviceList = [NSMutableArray array]; for (NSNumber * nodeID in clusterDataByNode) { NSDictionary * clusterData = clusterDataByNode[nodeID]; @@ -1024,7 +1018,7 @@ - (MTRBaseDevice *)baseDeviceForNodeID:(NSNumber *)nodeID // If prefetchedClusterData is not provided, load attributes individually from controller data store - (MTRDevice *)_setupDeviceForNodeID:(NSNumber *)nodeID prefetchedClusterData:(NSDictionary *)prefetchedClusterData { - os_unfair_lock_assert_owner(&_deviceMapLock); + os_unfair_lock_assert_owner(self.deviceMapLock); MTRDevice * deviceToReturn = [[MTRDevice alloc] initWithNodeID:nodeID controller:self]; // If we're not running, don't add the device to our map. That would @@ -1032,7 +1026,7 @@ - (MTRDevice *)_setupDeviceForNodeID:(NSNumber *)nodeID prefetchedClusterData:(N // which will be in exactly the state it would be in if it were created // while we were running and then we got shut down. if ([self isRunning]) { - [_nodeIDToDeviceMap setObject:deviceToReturn forKey:nodeID]; + [self.nodeIDToDeviceMap setObject:deviceToReturn forKey:nodeID]; } if (prefetchedClusterData) { @@ -1063,8 +1057,8 @@ - (MTRDevice *)_setupDeviceForNodeID:(NSNumber *)nodeID prefetchedClusterData:(N - (MTRDevice *)deviceForNodeID:(NSNumber *)nodeID { - std::lock_guard lock(_deviceMapLock); - MTRDevice * deviceToReturn = [_nodeIDToDeviceMap objectForKey:nodeID]; + std::lock_guard lock(*self.deviceMapLock); + MTRDevice * deviceToReturn = [self.nodeIDToDeviceMap objectForKey:nodeID]; if (!deviceToReturn) { deviceToReturn = [self _setupDeviceForNodeID:nodeID prefetchedClusterData:nil]; } @@ -1074,12 +1068,12 @@ - (MTRDevice *)deviceForNodeID:(NSNumber *)nodeID - (void)removeDevice:(MTRDevice *)device { - std::lock_guard lock(_deviceMapLock); + std::lock_guard lock(*self.deviceMapLock); auto * nodeID = device.nodeID; - MTRDevice * deviceToRemove = [_nodeIDToDeviceMap objectForKey:nodeID]; + MTRDevice * deviceToRemove = [self.nodeIDToDeviceMap objectForKey:nodeID]; if (deviceToRemove == device) { [deviceToRemove invalidate]; - [_nodeIDToDeviceMap removeObjectForKey:nodeID]; + [self.nodeIDToDeviceMap removeObjectForKey:nodeID]; } else { MTR_LOG_ERROR("%@ Error: Cannot remove device %p with nodeID %llu", self, device, nodeID.unsignedLongLongValue); } @@ -1088,10 +1082,10 @@ - (void)removeDevice:(MTRDevice *)device #ifdef DEBUG - (NSDictionary *)unitTestGetDeviceAttributeCounts { - std::lock_guard lock(_deviceMapLock); + std::lock_guard lock(*self.deviceMapLock); NSMutableDictionary * deviceAttributeCounts = [NSMutableDictionary dictionary]; - for (NSNumber * nodeID in _nodeIDToDeviceMap) { - deviceAttributeCounts[nodeID] = @([[_nodeIDToDeviceMap objectForKey:nodeID] unitTestAttributeCount]); + for (NSNumber * nodeID in self.nodeIDToDeviceMap) { + deviceAttributeCounts[nodeID] = @([[self.nodeIDToDeviceMap objectForKey:nodeID] unitTestAttributeCount]); } return deviceAttributeCounts; } @@ -1540,9 +1534,9 @@ - (void)operationalInstanceAdded:(chip::NodeId)nodeID { // Don't use deviceForNodeID here, because we don't want to create the // device if it does not already exist. - os_unfair_lock_lock(&_deviceMapLock); - MTRDevice * device = [_nodeIDToDeviceMap objectForKey:@(nodeID)]; - os_unfair_lock_unlock(&_deviceMapLock); + os_unfair_lock_lock(self.deviceMapLock); + MTRDevice * device = [self.nodeIDToDeviceMap objectForKey:@(nodeID)]; + os_unfair_lock_unlock(self.deviceMapLock); if (device == nil) { return; diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h b/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h index d943775d43475d..85d2c2e069ee2c 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h +++ b/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h @@ -29,6 +29,8 @@ #include #include +#import + #import "MTRBaseDevice.h" #import "MTRDeviceController.h" #import "MTRDeviceControllerDataStore.h" @@ -63,6 +65,9 @@ NS_ASSUME_NONNULL_BEGIN @interface MTRDeviceController () +@property (nonatomic, readwrite, nullable) NSMapTable * nodeIDToDeviceMap; +@property (readonly, assign) os_unfair_lock_t deviceMapLock; + - (instancetype)initForSubclasses; #pragma mark - MTRDeviceControllerFactory methods @@ -270,6 +275,7 @@ NS_ASSUME_NONNULL_BEGIN #pragma mark - Device-specific data and SDK access // DeviceController will act as a central repository for this opaque dictionary that MTRDevice manages - (MTRDevice *)deviceForNodeID:(NSNumber *)nodeID; +- (MTRDevice *)_setupDeviceForNodeID:(NSNumber *)nodeID prefetchedClusterData:(nullable NSDictionary *)prefetchedClusterData; - (void)removeDevice:(MTRDevice *)device; /** diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_XPC.mm b/src/darwin/Framework/CHIP/MTRDeviceController_XPC.mm index 1d91ae0d5d5f9a..d93a379ae3538f 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_XPC.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController_XPC.mm @@ -15,6 +15,7 @@ */ #import "MTRDeviceController_XPC.h" + #import "MTRDefines_Internal.h" #import "MTRDeviceController_Internal.h" #import "MTRDevice_XPC.h" @@ -45,7 +46,7 @@ @implementation MTRDeviceController_XPC - (id)initWithUniqueIdentifier:(NSUUID *)UUID xpConnectionBlock:(NSXPCConnection * (^)(void) )connectionBlock { if (self = [super initForSubclasses]) { - MTR_LOG("Setting up XPC Controller for UUID: %@ with connection block: %p", UUID, connectionBlock); + MTR_LOG("Setting up XPC Controller for UUID: %@ with connection block: %p", UUID, connectionBlock); if (UUID == nil) { MTR_LOG_ERROR("MTRDeviceController_XPC initWithUniqueIdentifier failed, nil UUID"); @@ -111,6 +112,24 @@ - (nullable instancetype)initWithParameters:(MTRDeviceControllerAbstractParamete return nil; } +// If prefetchedClusterData is not provided, load attributes individually from controller data store +- (MTRDevice *)_setupDeviceForNodeID:(NSNumber *)nodeID prefetchedClusterData:(NSDictionary *)prefetchedClusterData +{ + MTR_LOG("%s", __PRETTY_FUNCTION__); + os_unfair_lock_assert_owner(self.deviceMapLock); + + MTRDevice * deviceToReturn = [[MTRDevice_XPC alloc] initWithNodeID:nodeID controller:self]; + // If we're not running, don't add the device to our map. That would + // create a cycle that nothing would break. Just return the device, + // which will be in exactly the state it would be in if it were created + // while we were running and then we got shut down. + if ([self isRunning]) { + [self.nodeIDToDeviceMap setObject:deviceToReturn forKey:nodeID]; + } + MTR_LOG("%s: returning XPC device for node id %@", __PRETTY_FUNCTION__, nodeID); + return deviceToReturn; +} + #pragma mark - XPC Action Overrides MTR_DEVICECONTROLLER_SIMPLE_REMOTE_XPC_GETTER(isRunning, BOOL, NO, getIsRunningWithReply) diff --git a/src/darwin/Framework/CHIP/MTRDevice_XPC.mm b/src/darwin/Framework/CHIP/MTRDevice_XPC.mm index 95f69cb7d4dc90..69eba5bc6f5ea4 100644 --- a/src/darwin/Framework/CHIP/MTRDevice_XPC.mm +++ b/src/darwin/Framework/CHIP/MTRDevice_XPC.mm @@ -82,41 +82,59 @@ @implementation MTRDevice_XPC -#pragma mark - Client Callbacks +#pragma mark - Client Callbacks (MTRDeviceDelegate) + +// required methods for MTRDeviceDelegates - (oneway void)device:(NSNumber *)nodeID stateChanged:(MTRDeviceState)state { + MTR_LOG("%s", __PRETTY_FUNCTION__); [self _callDelegatesWithBlock:^(id delegate) { [delegate device:self stateChanged:state]; }]; } + - (oneway void)device:(NSNumber *)nodeID receivedAttributeReport:(NSArray *> *)attributeReport { + MTR_LOG("%s", __PRETTY_FUNCTION__); [self _callDelegatesWithBlock:^(id delegate) { [delegate device:self receivedAttributeReport:attributeReport]; }]; } + - (oneway void)device:(NSNumber *)nodeID receivedEventReport:(NSArray *> *)eventReport { + MTR_LOG("%s", __PRETTY_FUNCTION__); [self _callDelegatesWithBlock:^(id delegate) { [delegate device:self receivedEventReport:eventReport]; }]; } + +// optional methods for MTRDeviceDelegates - check for implementation before calling - (oneway void)deviceBecameActive:(NSNumber *)nodeID { + MTR_LOG("%s", __PRETTY_FUNCTION__); [self _callDelegatesWithBlock:^(id delegate) { - [delegate deviceBecameActive:self]; + if ([delegate respondsToSelector:@selector(deviceBecameActive:)]) { + [delegate deviceBecameActive:self]; + } }]; } + - (oneway void)deviceCachePrimed:(NSNumber *)nodeID { [self _callDelegatesWithBlock:^(id delegate) { - [delegate deviceCachePrimed:self]; + if ([delegate respondsToSelector:@selector(deviceCachePrimed:)]) { + [delegate deviceCachePrimed:self]; + } }]; } + - (oneway void)deviceConfigurationChanged:(NSNumber *)nodeID { [self _callDelegatesWithBlock:^(id delegate) { - [delegate deviceConfigurationChanged:self]; + if ([delegate respondsToSelector:@selector(deviceConfigurationChanged:)]) { + [delegate deviceConfigurationChanged:self]; + } }]; }