From 20c307a50f00622e68d55ae4c5036a587f18c82e Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 5 Sep 2023 12:06:43 -0400 Subject: [PATCH] Address review comment. --- .../CHIP/MTRDeviceControllerFactory.mm | 86 ++++++------------- 1 file changed, 28 insertions(+), 58 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm index 65bfe22126b99a..21660828e25847 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm @@ -553,11 +553,10 @@ - (void)stopControllerFactory * return nil if pre-startup fabric table checks fail, and set fabricError to * the right error value in that situation. * - * existingController can be provided if the controller object has already been - * allocated but not yet initialized. If not provided, a new controller object - * will be created. + * The provided controller is expected to have just been allocated and to not be + * initialized yet. */ -- (MTRDeviceController * _Nullable)_startDeviceController:(nullable MTRDeviceController *)existingController +- (MTRDeviceController * _Nullable)_startDeviceController:(MTRDeviceController *)controller startupParams:(id)startupParams fabricChecker:(MTRDeviceControllerStartupParamsInternal * (^)(FabricTable * fabricTable, MTRDeviceController * controller, @@ -618,21 +617,35 @@ - (MTRDeviceController * _Nullable)_startDeviceController:(nullable MTRDeviceCon otaProviderDelegateQueue = self.otaProviderDelegateQueue; } - // Create the controller, so we start the event loop, since we plan to do - // our fabric table operations there. - auto * controller = [self _createController:existingController - storageDelegate:storageDelegate - storageDelegateQueue:storageDelegateQueue - otaProviderDelegate:otaProviderDelegate - otaProviderDelegateQueue:otaProviderDelegateQueue - uniqueIdentifier:uniqueIdentifier]; + controller = [controller initWithFactory:self + queue:_chipWorkQueue + storageDelegate:storageDelegate + storageDelegateQueue:storageDelegateQueue + otaProviderDelegate:otaProviderDelegate + otaProviderDelegateQueue:otaProviderDelegateQueue + uniqueIdentifier:uniqueIdentifier]; if (controller == nil) { if (error != nil) { - *error = [MTRError errorForCHIPErrorCode:CHIP_ERROR_NO_MEMORY]; + *error = [MTRError errorForCHIPErrorCode:CHIP_ERROR_INVALID_ARGUMENT]; } return nil; } + if ([_controllers count] == 0) { + // Bringing up the first controller. Start the event loop now. If we + // fail to bring it up, its cleanup will stop the event loop again. + chip::DeviceLayer::PlatformMgrImpl().StartEventLoopTask(); + dispatch_sync(_chipWorkQueue, ^{ + self->_operationalBrowser = new MTROperationalBrowser(self, self->_chipWorkQueue); + }); + } + + // Add the controller to _controllers now, so if we fail partway through its + // startup we will still do the right cleanups. + os_unfair_lock_lock(&_controllersLock); + [_controllers addObject:controller]; + os_unfair_lock_unlock(&_controllersLock); + __block MTRDeviceControllerStartupParamsInternal * params = nil; __block CHIP_ERROR fabricError = CHIP_NO_ERROR; @@ -727,7 +740,7 @@ - (MTRDeviceController * _Nullable)createControllerOnExistingFabric:(MTRDeviceCo return nil; } - return [self _startDeviceController:nil + return [self _startDeviceController:[MTRDeviceController alloc] startupParams:startupParams fabricChecker:^MTRDeviceControllerStartupParamsInternal *( FabricTable * fabricTable, MTRDeviceController * controller, CHIP_ERROR & fabricError) { @@ -804,7 +817,7 @@ - (MTRDeviceController * _Nullable)createControllerOnNewFabric:(MTRDeviceControl return nil; } - return [self _startDeviceController:nil + return [self _startDeviceController:[MTRDeviceController alloc] startupParams:startupParams fabricChecker:^MTRDeviceControllerStartupParamsInternal *( FabricTable * fabricTable, MTRDeviceController * controller, CHIP_ERROR & fabricError) { @@ -838,49 +851,6 @@ - (MTRDeviceController * _Nullable)createControllerOnNewFabric:(MTRDeviceControl error:error]; } -- (MTRDeviceController * _Nullable)_createController:(nullable MTRDeviceController *)existingController - storageDelegate:(id _Nullable)storageDelegate - storageDelegateQueue:(dispatch_queue_t _Nullable)storageDelegateQueue - otaProviderDelegate:(id _Nullable)otaProviderDelegate - otaProviderDelegateQueue:(dispatch_queue_t _Nullable)otaProviderDelegateQueue - uniqueIdentifier:(NSUUID *)uniqueIdentifier -{ - [self _assertCurrentQueueIsNotMatterQueue]; - - MTRDeviceController * controller = existingController; - if (controller == nil) { - controller = [MTRDeviceController alloc]; - } - controller = [controller initWithFactory:self - queue:_chipWorkQueue - storageDelegate:storageDelegate - storageDelegateQueue:storageDelegateQueue - otaProviderDelegate:otaProviderDelegate - otaProviderDelegateQueue:otaProviderDelegateQueue - uniqueIdentifier:uniqueIdentifier]; - if (controller == nil) { - MTR_LOG_ERROR("Failed to init controller"); - return nil; - } - - if ([_controllers count] == 0) { - // Bringing up the first controller. Start the event loop now. If we - // fail to bring it up, its cleanup will stop the event loop again. - chip::DeviceLayer::PlatformMgrImpl().StartEventLoopTask(); - dispatch_sync(_chipWorkQueue, ^{ - self->_operationalBrowser = new MTROperationalBrowser(self, self->_chipWorkQueue); - }); - } - - // Add the controller to _controllers now, so if we fail partway through its - // startup we will still do the right cleanups. - os_unfair_lock_lock(&_controllersLock); - [_controllers addObject:controller]; - os_unfair_lock_unlock(&_controllersLock); - - return controller; -} - // Finds a fabric that matches the given params, if one exists. // // Returns NO on failure, YES on success. If YES is returned, the