Address API review issues in MTRControllerFactory. (#23505)

This is a re-landing of PR #22606 but modified to preserve the old APIs. * Rename to MTRDeviceControllerFactory. * Change the startup params startServer to shouldStartServer. * Change the startup params init signatures to be more aligned. * Change isRunning to running. * Rename startup to startControllerFactory and add NSError outparam. * Rename shutdown to stopControllerFactory * Rename startControllerOnExistingFabric to createControllerOnExistingFabric and add NSError outparam. * Rename startControllerOnNewFabric to createControllerOnNewFabric and add NSError outparam. * Fix a leak when we failed to start a controller because it wanted a fabric that does not exist, or wanted a new fabric and a matching one existed. This used to not show up in LSan before, maybe because we did not have an autoreleasepool in place. The header changes not accompanied by backwards-compat shims are OK for the following reasons: * The change in MTRBaseDevice_Internal.h is comment-only. * The change in MTRDeviceController.h should be source and binary compatible. * MTRDeviceControllerFactory_Internal.h is not a public header. * In MTRDeviceControllerStartupParams.h the changes with no shims are either comments or changes to MTR_NEWLY_AVAILABLE APIs. * MTRDeviceControllerStartupParams_Internal.h is not a public header. * MTRDeviceController_Internal.h is not a public header.
project-chip · Dec 18, 2023 · 3753466 · 3753466
1 parent 001d396
commit 3753466
Show file tree

Hide file tree

Showing 17 changed files with 567 additions and 400 deletions.
diff --git a/examples/darwin-framework-tool/commands/common/CHIPCommandBridge.mm b/examples/darwin-framework-tool/commands/common/CHIPCommandBridge.mm
@@ -111,25 +111,26 @@
 
     mOTADelegate = [[OTAProviderDelegate alloc] init];
 
-    auto factory = [MTRControllerFactory sharedInstance];
+    auto factory = [MTRDeviceControllerFactory sharedInstance];
     if (factory == nil) {
         ChipLogError(chipTool, "Controller factory is nil");
         return CHIP_ERROR_INTERNAL;
     }
 
-    auto params = [[MTRControllerFactoryParams alloc] initWithStorage:storage];
+    auto params = [[MTRDeviceControllerFactoryParams alloc] initWithStorage:storage];
     params.port = @(kListenPort);
-    params.startServer = YES;
+    params.shouldStartServer = YES;
     params.otaProviderDelegate = mOTADelegate;
     NSArray<NSData *> * paaCertResults;
     ReturnLogErrorOnFailure(GetPAACertsFromFolder(&paaCertResults));
     if ([paaCertResults count] > 0) {
         params.paaCerts = paaCertResults;
     }
 
-    if ([factory startup:params] == NO) {
+    NSError * error;
+    if ([factory startControllerFactory:params error:&error] == NO) {
         ChipLogError(chipTool, "Controller factory startup failed");
-        return CHIP_ERROR_INTERNAL;
+        return MTRErrorToCHIPErrorCode(error);
     }
 
     ReturnLogErrorOnFailure([gNocSigner createOrLoadKeys:storage]);
@@ -138,21 +139,19 @@
 
     constexpr const char * identities[] = { kIdentityAlpha, kIdentityBeta, kIdentityGamma };
     for (size_t i = 0; i < ArraySize(identities); ++i) {
-        auto controllerParams = [[MTRDeviceControllerStartupParams alloc] initWithSigningKeypair:gNocSigner
-                                                                                        fabricID:@(i + 1)
-                                                                                             ipk:ipk];
+        auto controllerParams = [[MTRDeviceControllerStartupParams alloc] initWithIPK:ipk fabricID:@(i + 1) nocSigner:gNocSigner];
 
         // We're not sure whether we're creating a new fabric or using an
         // existing one, so just try both.
-        auto controller = [factory startControllerOnExistingFabric:controllerParams];
+        auto controller = [factory createControllerOnExistingFabric:controllerParams error:&error];
         if (controller == nil) {
             // Maybe we didn't have this fabric yet.
             controllerParams.vendorID = @(chip::VendorId::TestVendor1);
-            controller = [factory startControllerOnNewFabric:controllerParams];
+            controller = [factory createControllerOnNewFabric:controllerParams error:&error];
         }
         if (controller == nil) {
             ChipLogError(chipTool, "Controller startup failure.");
-            return CHIP_ERROR_INTERNAL;
+            return MTRErrorToCHIPErrorCode(error);
         }
 
         mControllers[identities[i]] = controller;
@@ -195,16 +194,14 @@
 {
     StopCommissioners();
 
-    auto factory = [MTRControllerFactory sharedInstance];
+    auto factory = [MTRDeviceControllerFactory sharedInstance];
     NSData * ipk = [gNocSigner getIPK];
 
     constexpr const char * identities[] = { kIdentityAlpha, kIdentityBeta, kIdentityGamma };
     for (size_t i = 0; i < ArraySize(identities); ++i) {
-        auto controllerParams = [[MTRDeviceControllerStartupParams alloc] initWithSigningKeypair:gNocSigner
-                                                                                        fabricID:@(i + 1)
-                                                                                             ipk:ipk];
+        auto controllerParams = [[MTRDeviceControllerStartupParams alloc] initWithIPK:ipk fabricID:@(i + 1) nocSigner:gNocSigner];
 
-        auto controller = [factory startControllerOnExistingFabric:controllerParams];
+        auto controller = [factory createControllerOnExistingFabric:controllerParams error:nil];
         mControllers[identities[i]] = controller;
     }
 }
@@ -216,7 +213,7 @@
     mControllers.clear();
     mCurrentController = nil;
 
-    [[MTRControllerFactory sharedInstance] shutdown];
+    [[MTRDeviceControllerFactory sharedInstance] stopControllerFactory];
 }
 
 CHIP_ERROR CHIPCommandBridge::StartWaiting(chip::System::Clock::Timeout duration)

diff --git a/src/darwin/Framework/CHIP/MTRBaseDevice_Internal.h b/src/darwin/Framework/CHIP/MTRBaseDevice_Internal.h
@@ -55,9 +55,6 @@ NS_ASSUME_NONNULL_BEGIN
  */
 @property (nonatomic, assign, readonly) chip::NodeId nodeID;
 
-/**
- * Controllers are created via the MTRControllerFactory object.
- */
 - (instancetype)init NS_UNAVAILABLE;
 + (instancetype)new NS_UNAVAILABLE;
 

diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.h b/src/darwin/Framework/CHIP/MTRDeviceController.h
@@ -31,7 +31,7 @@ typedef void (^MTRDeviceConnectionCallback)(MTRBaseDevice * _Nullable device, NS
 
 @interface MTRDeviceController : NSObject
 
-@property (readonly, nonatomic) BOOL isRunning;
+@property (readonly, nonatomic, getter=isRunning) BOOL running;
 
 /**
  * Return the Node ID assigned to the controller.  Will return nil if the
@@ -139,7 +139,7 @@ typedef void (^MTRDeviceConnectionCallback)(MTRBaseDevice * _Nullable device, NS
                                           error:(NSError * __autoreleasing *)error;
 
 /**
- * Controllers are created via the MTRControllerFactory object.
+ * Controllers are created via the MTRDeviceControllerFactory object.
  */
 - (instancetype)init NS_UNAVAILABLE;
 + (instancetype)new NS_UNAVAILABLE;

diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm
@@ -20,7 +20,7 @@
 
 #import "MTRBaseDevice_Internal.h"
 #import "MTRCommissioningParameters.h"
-#import "MTRControllerFactory_Internal.h"
+#import "MTRDeviceControllerFactory_Internal.h"
 #import "MTRDeviceControllerStartupParams.h"
 #import "MTRDeviceControllerStartupParams_Internal.h"
 #import "MTRDevicePairingDelegateBridge.h"
@@ -87,14 +87,14 @@ @interface MTRDeviceController ()
 @property (readonly) MTRP256KeypairBridge signingKeypairBridge;
 @property (readonly) MTRP256KeypairBridge operationalKeypairBridge;
 @property (readonly) MTRDeviceAttestationDelegateBridge * deviceAttestationDelegateBridge;
-@property (readonly) MTRControllerFactory * factory;
+@property (readonly) MTRDeviceControllerFactory * factory;
 @property (readonly) NSMutableDictionary * nodeIDToDeviceMap;
 @property (readonly) os_unfair_lock deviceMapLock; // protects nodeIDToDeviceMap
 @end
 
 @implementation MTRDeviceController
 
-- (instancetype)initWithFactory:(MTRControllerFactory *)factory queue:(dispatch_queue_t)queue
+- (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory queue:(dispatch_queue_t)queue
 {
     if (self = [super init]) {
         _chipWorkQueue = queue;
@@ -143,7 +143,7 @@ - (void)cleanupAfterStartup
 }
 
 // Part of cleanupAfterStartup that has to interact with the Matter work queue
-// in a very specific way that only MTRControllerFactory knows about.
+// in a very specific way that only MTRDeviceControllerFactory knows about.
 - (void)shutDownCppController
 {
     if (_cppCommissioner) {

diff --git a/...win/Framework/CHIP/MTRControllerFactory.h → ...amework/CHIP/MTRDeviceControllerFactory.h b/...win/Framework/CHIP/MTRControllerFactory.h → ...amework/CHIP/MTRDeviceControllerFactory.h
@@ -32,7 +32,8 @@ NS_ASSUME_NONNULL_BEGIN
 @class MTRDeviceController;
 @class MTRDeviceControllerStartupParams;
 
-@interface MTRControllerFactoryParams : NSObject
+MTR_NEWLY_AVAILABLE
+@interface MTRDeviceControllerFactoryParams : NSObject
 /*
  * Storage delegate must be provided for correct functioning of Matter
  * controllers.  It is used to store persistent information for the fabrics the
@@ -67,44 +68,47 @@ NS_ASSUME_NONNULL_BEGIN
  * Whether to run a server capable of accepting incoming CASE
  * connections.  Defaults to NO.
  */
-@property (nonatomic, assign) BOOL startServer;
+@property (nonatomic, assign) BOOL shouldStartServer MTR_NEWLY_AVAILABLE;
 
 - (instancetype)init NS_UNAVAILABLE;
 - (instancetype)initWithStorage:(id<MTRStorage>)storage;
 @end
 
-@interface MTRControllerFactory : NSObject
-
-@property (readonly, nonatomic) BOOL isRunning;
+@interface MTRDeviceControllerFactory : NSObject
 
-- (instancetype)init NS_UNAVAILABLE;
-+ (instancetype)new NS_UNAVAILABLE;
+/**
+ * If true, the factory is in a state where it can create controllers:
+ * startControllerFactory has been called, but stopControllerFactory has not been called
+ * since then.
+ */
+@property (readonly, nonatomic, getter=isRunning) BOOL running;
 
 /**
- * Return the single MTRControllerFactory we support existing.  It starts off
+ * Return the single MTRDeviceControllerFactory we support existing.  It starts off
  * in a "not started" state.
  */
 + (instancetype)sharedInstance;
 
 /**
- * Start the controller factory. Repeated calls to startup without calls to
- * shutdown in between are NO-OPs. Use the isRunning property to check whether
- * the controller factory needs to be started up.
+ * Start the controller factory. Repeated calls to startControllerFactory
+ * without calls to stopControllerFactory in between are NO-OPs. Use the
+ * isRunning property to check whether the controller factory needs to be
+ * started up.
  *
  * @param[in] startupParams data needed to start up the controller factory.
  *
  * @return Whether startup succeded.
  */
-- (BOOL)startup:(MTRControllerFactoryParams *)startupParams;
+- (BOOL)startControllerFactory:(MTRDeviceControllerFactoryParams *)startupParams error:(NSError * __autoreleasing *)error;
 
 /**
- * Shut down the controller factory. This will shut down any outstanding
- * controllers as part of the factory shutdown.
+ * Stop the controller factory. This will shut down any outstanding
+ * controllers as part of the factory stopping.
  *
- * Repeated calls to shutdown without calls to startup in between are
- * NO-OPs.
+ * Repeated calls to stopControllerFactory without calls to
+ * startControllerFactory in between are NO-OPs.
  */
-- (void)shutdown;
+- (void)stopControllerFactory;
 
 /**
  * Create a MTRDeviceController on an existing fabric.  Returns nil on failure.
@@ -115,7 +119,8 @@ NS_ASSUME_NONNULL_BEGIN
  * The fabric is identified by the root public key and fabric id in
  * the startupParams.
  */
-- (MTRDeviceController * _Nullable)startControllerOnExistingFabric:(MTRDeviceControllerStartupParams *)startupParams;
+- (MTRDeviceController * _Nullable)createControllerOnExistingFabric:(MTRDeviceControllerStartupParams *)startupParams
+                                                              error:(NSError * __autoreleasing *)error;
 
 /**
  * Create a MTRDeviceController on a new fabric.  Returns nil on failure.
@@ -125,13 +130,31 @@ NS_ASSUME_NONNULL_BEGIN
  * The fabric is identified by the root public key and fabric id in
  * the startupParams.
  */
-- (MTRDeviceController * _Nullable)startControllerOnNewFabric:(MTRDeviceControllerStartupParams *)startupParams;
+- (MTRDeviceController * _Nullable)createControllerOnNewFabric:(MTRDeviceControllerStartupParams *)startupParams
+                                                         error:(NSError * __autoreleasing *)error;
+
+- (instancetype)init NS_UNAVAILABLE;
++ (instancetype)new NS_UNAVAILABLE;
 
 @end
 
-@interface MTRControllerFactoryParams (Deprecated)
+MTR_NEWLY_DEPRECATED("Please use MTRDeviceControllerFactoryParams")
+@interface MTRControllerFactoryParams : MTRDeviceControllerFactoryParams
 @property (nonatomic, strong, readonly) id<MTRPersistentStorageDelegate> storageDelegate MTR_NEWLY_DEPRECATED(
     "Please use the storage property");
+@property (nonatomic, assign) BOOL startServer;
+@end
+
+MTR_NEWLY_DEPRECATED("Please use MTRDeviceControllerFactory")
+@interface MTRControllerFactory : NSObject
+@property (readonly, nonatomic) BOOL isRunning;
++ (instancetype)sharedInstance;
+- (BOOL)startup:(MTRControllerFactoryParams *)startupParams;
+- (void)shutdown;
+- (MTRDeviceController * _Nullable)startControllerOnExistingFabric:(MTRDeviceControllerStartupParams *)startupParams;
+- (MTRDeviceController * _Nullable)startControllerOnNewFabric:(MTRDeviceControllerStartupParams *)startupParams;
+- (instancetype)init NS_UNAVAILABLE;
++ (instancetype)new NS_UNAVAILABLE;
 @end
 
 NS_ASSUME_NONNULL_END