Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Address API review issues in MTRControllerFactory. (#23505)
Browse files Browse the repository at this point in the history
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.
bzbarsky-apple authored and pull[bot] committed Nov 13, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
1 parent 7af4a03 commit d75af30
Showing 17 changed files with 567 additions and 400 deletions.
31 changes: 14 additions & 17 deletions examples/darwin-framework-tool/commands/common/CHIPCommandBridge.mm
Original file line number Diff line number Diff line change
@@ -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)
3 changes: 0 additions & 3 deletions src/darwin/Framework/CHIP/MTRBaseDevice_Internal.h
Original file line number Diff line number Diff line change
@@ -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;

4 changes: 2 additions & 2 deletions src/darwin/Framework/CHIP/MTRDeviceController.h
Original file line number Diff line number Diff line change
@@ -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;
8 changes: 4 additions & 4 deletions src/darwin/Framework/CHIP/MTRDeviceController.mm
Original file line number Diff line number Diff line change
@@ -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) {
Original file line number Diff line number Diff line change
@@ -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
Loading

0 comments on commit d75af30

Please sign in to comment.