From 99586761ab7d40268fff4377c93617b7ed6b4755 Mon Sep 17 00:00:00 2001 From: Karsten Sperling <113487422+ksperling-apple@users.noreply.github.com> Date: Thu, 21 Dec 2023 13:46:14 +1300 Subject: [PATCH] Darwin: Consistently use ivars in MTRDeviceController and a few other places (#31141) --- .../commands/common/CHIPToolKeypair.mm | 15 ++-- .../pairing/DeviceControllerDelegateBridge.mm | 3 - .../CHIP/MTRAsyncCallbackWorkQueue.mm | 4 + src/darwin/Framework/CHIP/MTRBaseDevice.mm | 4 +- .../Framework/CHIP/MTRDeviceController.mm | 89 +++++++++---------- .../CHIP/MTRThreadOperationalDataset.mm | 13 +-- 6 files changed, 59 insertions(+), 69 deletions(-) diff --git a/examples/darwin-framework-tool/commands/common/CHIPToolKeypair.mm b/examples/darwin-framework-tool/commands/common/CHIPToolKeypair.mm index fccf3d2734407e..4d6f53de9aa08f 100644 --- a/examples/darwin-framework-tool/commands/common/CHIPToolKeypair.mm +++ b/examples/darwin-framework-tool/commands/common/CHIPToolKeypair.mm @@ -12,15 +12,14 @@ static NSString * const kOperationalCredentialsIssuerKeypairStorage = @"ChipToolOpCredsCAKey"; static NSString * const kOperationalCredentialsIPK = @"ChipToolOpCredsIPK"; -@interface CHIPToolKeypair () -@property (nonatomic) chip::Crypto::P256Keypair mKeyPair; -@property (nonatomic) chip::Crypto::P256Keypair mIssuer; -@property (nonatomic) NSData * ipk; -@property (atomic) uint32_t mNow; -@property (nonatomic, readonly) SecKeyRef mPublicKey; -@end +@implementation CHIPToolKeypair { + chip::Crypto::P256Keypair _mKeyPair; + chip::Crypto::P256Keypair _mIssuer; + NSData * _ipk; + uint32_t _mNow; + SecKeyRef _mPublicKey; +} -@implementation CHIPToolKeypair - (instancetype)init { if (self = [super init]) { diff --git a/examples/darwin-framework-tool/commands/pairing/DeviceControllerDelegateBridge.mm b/examples/darwin-framework-tool/commands/pairing/DeviceControllerDelegateBridge.mm index b3476119c224b5..7b9f4369a79c7d 100644 --- a/examples/darwin-framework-tool/commands/pairing/DeviceControllerDelegateBridge.mm +++ b/examples/darwin-framework-tool/commands/pairing/DeviceControllerDelegateBridge.mm @@ -19,9 +19,6 @@ #include "DeviceControllerDelegateBridge.h" #import -@interface CHIPToolDeviceControllerDelegate () -@end - @implementation CHIPToolDeviceControllerDelegate - (void)controller:(MTRDeviceController *)controller statusUpdate:(MTRCommissioningStatus)status { diff --git a/src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue.mm b/src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue.mm index ece651a21238de..00b6d33788f6c0 100644 --- a/src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue.mm +++ b/src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue.mm @@ -15,6 +15,10 @@ * limitations under the License. */ +// NOTE: This class was not intended to be part of the public Matter API; +// internally this class has been replaced by MTRAsyncWorkQueue. This code +// remains here simply to preserve API/ABI compatibility. + #import #import diff --git a/src/darwin/Framework/CHIP/MTRBaseDevice.mm b/src/darwin/Framework/CHIP/MTRBaseDevice.mm index d053cc8f007cf8..a4bcf3742c8d6e 100644 --- a/src/darwin/Framework/CHIP/MTRBaseDevice.mm +++ b/src/darwin/Framework/CHIP/MTRBaseDevice.mm @@ -2744,12 +2744,10 @@ - (id)copyWithZone:(NSZone *)zone @end -@interface MTREventReport () { +@implementation MTREventReport { NSNumber * _timestampValue; } -@end -@implementation MTREventReport + (void)initialize { // One of our init methods ends up doing Platform::MemoryAlloc. diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index 04f92e8e3bdf1a..ba3ec1958a42fd 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -101,31 +101,27 @@ typedef id (^SyncWorkQueueBlockWithReturnValue)(void); typedef BOOL (^SyncWorkQueueBlockWithBoolReturnValue)(void); -@interface MTRDeviceController () { +@implementation MTRDeviceController { // Atomic because it can be touched from multiple threads. std::atomic _storedFabricIndex; -} - -// queue used to serialize all work performed by the MTRDeviceController -@property (atomic, readonly) dispatch_queue_t chipWorkQueue; -@property (readonly) chip::Controller::DeviceCommissioner * cppCommissioner; -@property (readonly) chip::Credentials::PartialDACVerifier * partialDACVerifier; -@property (readonly) chip::Credentials::DefaultDACVerifier * defaultDACVerifier; -@property (readonly) MTRDeviceControllerDelegateBridge * deviceControllerDelegateBridge; -@property (readonly) MTROperationalCredentialsDelegate * operationalCredentialsDelegate; -@property (readonly) MTRP256KeypairBridge signingKeypairBridge; -@property (readonly) MTRP256KeypairBridge operationalKeypairBridge; -@property (readonly) MTRDeviceAttestationDelegateBridge * deviceAttestationDelegateBridge; -@property (readonly) MTRDeviceControllerFactory * factory; -@property (readonly) NSMutableDictionary * nodeIDToDeviceMap; -@property (readonly) os_unfair_lock deviceMapLock; // protects nodeIDToDeviceMap -@property (readonly) MTRCommissionableBrowser * commissionableBrowser; -@property (readonly) MTRAttestationTrustStoreBridge * attestationTrustStoreBridge; + // queue used to serialize all work performed by the MTRDeviceController + dispatch_queue_t _chipWorkQueue; -@end - -@implementation MTRDeviceController + chip::Controller::DeviceCommissioner * _cppCommissioner; + chip::Credentials::PartialDACVerifier * _partialDACVerifier; + chip::Credentials::DefaultDACVerifier * _defaultDACVerifier; + MTRDeviceControllerDelegateBridge * _deviceControllerDelegateBridge; + MTROperationalCredentialsDelegate * _operationalCredentialsDelegate; + MTRP256KeypairBridge _signingKeypairBridge; + MTRP256KeypairBridge _operationalKeypairBridge; + MTRDeviceAttestationDelegateBridge * _deviceAttestationDelegateBridge; + MTRDeviceControllerFactory * _factory; + NSMutableDictionary * _nodeIDToDeviceMap; + os_unfair_lock _deviceMapLock; // protects nodeIDToDeviceMap + MTRCommissionableBrowser * _commissionableBrowser; + MTRAttestationTrustStoreBridge * _attestationTrustStoreBridge; +} - (nullable instancetype)initWithParameters:(MTRDeviceControllerAbstractParameters *)parameters error:(NSError * __autoreleasing *)error { @@ -249,7 +245,7 @@ - (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory - (BOOL)isRunning { - return self.cppCommissioner != nullptr; + return _cppCommissioner != nullptr; } - (void)shutdown @@ -271,8 +267,8 @@ - (void)cleanupAfterStartup // while calling out into arbitrary invalidation code, snapshot the list of // devices before we start invalidating. os_unfair_lock_lock(&_deviceMapLock); - NSArray * devices = [self.nodeIDToDeviceMap allValues]; - [self.nodeIDToDeviceMap removeAllObjects]; + NSArray * devices = [_nodeIDToDeviceMap allValues]; + [_nodeIDToDeviceMap removeAllObjects]; os_unfair_lock_unlock(&_deviceMapLock); for (MTRDevice * device in devices) { @@ -590,7 +586,7 @@ - (BOOL)setupCommissioningSessionWithPayload:(MTRSetupPayload *)payload chip::NodeId nodeId = [newNodeID unsignedLongLongValue]; self->_operationalCredentialsDelegate->SetDeviceID(nodeId); - auto errorCode = self.cppCommissioner->EstablishPASEConnection(nodeId, [pairingCode UTF8String]); + auto errorCode = self->_cppCommissioner->EstablishPASEConnection(nodeId, [pairingCode UTF8String]); return ![MTRDeviceController checkForError:errorCode logMsg:kErrorPairDevice error:error]; }; @@ -612,7 +608,7 @@ - (BOOL)setupCommissioningSessionWithDiscoveredDevice:(MTRCommissionableBrowserR auto pinCode = static_cast(payload.setupPasscode.unsignedLongValue); params.Value().SetSetupPINCode(pinCode); - errorCode = self.cppCommissioner->EstablishPASEConnection(nodeId, params.Value()); + errorCode = self->_cppCommissioner->EstablishPASEConnection(nodeId, params.Value()); } else { // Try to get a QR code if possible (because it has a better // discriminator, etc), then fall back to manual code if that fails. @@ -630,7 +626,7 @@ - (BOOL)setupCommissioningSessionWithDiscoveredDevice:(MTRCommissionableBrowserR continue; } - errorCode = self.cppCommissioner->EstablishPASEConnection( + errorCode = self->_cppCommissioner->EstablishPASEConnection( nodeId, [pairingCode UTF8String], chip::Controller::DiscoveryType::kDiscoveryNetworkOnly, resolutionData); if (CHIP_NO_ERROR != errorCode) { break; @@ -745,7 +741,7 @@ - (BOOL)commissionNodeWithID:(NSNumber *)nodeID chip::NodeId deviceId = [nodeID unsignedLongLongValue]; self->_operationalCredentialsDelegate->SetDeviceID(deviceId); - auto errorCode = self.cppCommissioner->Commission(deviceId, params); + auto errorCode = self->_cppCommissioner->Commission(deviceId, params); return ![MTRDeviceController checkForError:errorCode logMsg:kErrorPairDevice error:error]; }; @@ -762,7 +758,7 @@ - (BOOL)continueCommissioningDevice:(void *)device : chip::Credentials::AttestationVerificationResult::kSuccess; auto deviceProxy = static_cast(device); - auto errorCode = self.cppCommissioner->ContinueCommissioningAfterDeviceAttestation(deviceProxy, + auto errorCode = self->_cppCommissioner->ContinueCommissioningAfterDeviceAttestation(deviceProxy, ignoreAttestationFailure ? chip::Credentials::AttestationVerificationResult::kSuccess : lastAttestationResult); return ![MTRDeviceController checkForError:errorCode logMsg:kErrorPairDevice error:error]; }; @@ -774,7 +770,7 @@ - (BOOL)cancelCommissioningForNodeID:(NSNumber *)nodeID error:(NSError * __autor { auto block = ^BOOL { self->_operationalCredentialsDelegate->ResetDeviceID(); - auto errorCode = self.cppCommissioner->StopPairing([nodeID unsignedLongLongValue]); + auto errorCode = self->_cppCommissioner->StopPairing([nodeID unsignedLongLongValue]); return ![MTRDeviceController checkForError:errorCode logMsg:kErrorStopPairing error:error]; }; @@ -784,7 +780,7 @@ - (BOOL)cancelCommissioningForNodeID:(NSNumber *)nodeID error:(NSError * __autor - (BOOL)startBrowseForCommissionables:(id)delegate queue:(dispatch_queue_t)queue { auto block = ^BOOL { - VerifyOrReturnValue(self.commissionableBrowser == nil, NO); + VerifyOrReturnValue(self->_commissionableBrowser == nil, NO); auto commissionableBrowser = [[MTRCommissionableBrowser alloc] initWithDelegate:delegate controller:self queue:queue]; VerifyOrReturnValue([commissionableBrowser start], NO); @@ -799,9 +795,9 @@ - (BOOL)startBrowseForCommissionables:(id)dele - (BOOL)stopBrowseForCommissionables { auto block = ^BOOL { - VerifyOrReturnValue(self.commissionableBrowser != nil, NO); + VerifyOrReturnValue(self->_commissionableBrowser != nil, NO); - auto commissionableBrowser = self.commissionableBrowser; + auto commissionableBrowser = self->_commissionableBrowser; VerifyOrReturnValue([commissionableBrowser stop], NO); self->_commissionableBrowser = nil; @@ -845,7 +841,7 @@ - (MTRBaseDevice *)baseDeviceForNodeID:(NSNumber *)nodeID - (MTRDevice *)deviceForNodeID:(NSNumber *)nodeID { os_unfair_lock_lock(&_deviceMapLock); - MTRDevice * deviceToReturn = self.nodeIDToDeviceMap[nodeID]; + MTRDevice * deviceToReturn = _nodeIDToDeviceMap[nodeID]; if (!deviceToReturn) { deviceToReturn = [[MTRDevice alloc] initWithNodeID:nodeID controller:self]; // If we're not running, don't add the device to our map. That would @@ -853,7 +849,7 @@ - (MTRDevice *)deviceForNodeID:(NSNumber *)nodeID // 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[nodeID] = deviceToReturn; + _nodeIDToDeviceMap[nodeID] = deviceToReturn; } } os_unfair_lock_unlock(&_deviceMapLock); @@ -864,12 +860,13 @@ - (MTRDevice *)deviceForNodeID:(NSNumber *)nodeID - (void)removeDevice:(MTRDevice *)device { os_unfair_lock_lock(&_deviceMapLock); - MTRDevice * deviceToRemove = self.nodeIDToDeviceMap[device.nodeID]; + auto * nodeID = device.nodeID; + MTRDevice * deviceToRemove = _nodeIDToDeviceMap[nodeID]; if (deviceToRemove == device) { [deviceToRemove invalidate]; - self.nodeIDToDeviceMap[device.nodeID] = nil; + _nodeIDToDeviceMap[nodeID] = nil; } else { - MTR_LOG_ERROR("Error: Cannot remove device %p with nodeID %llu", device, device.nodeID.unsignedLongLongValue); + MTR_LOG_ERROR("Error: Cannot remove device %p with nodeID %llu", device, nodeID.unsignedLongLongValue); } os_unfair_lock_unlock(&_deviceMapLock); } @@ -935,7 +932,7 @@ - (NSData * _Nullable)attestationChallengeForDeviceID:(NSNumber *)deviceID auto block = ^NSData * { chip::CommissioneeDeviceProxy * deviceProxy; - auto errorCode = self.cppCommissioner->GetDeviceBeingCommissioned([deviceID unsignedLongLongValue], &deviceProxy); + auto errorCode = self->_cppCommissioner->GetDeviceBeingCommissioned([deviceID unsignedLongLongValue], &deviceProxy); VerifyOrReturnValue(![MTRDeviceController checkForError:errorCode logMsg:kErrorGetCommissionee error:nil], nil); uint8_t challengeBuffer[chip::Crypto::kAES_CCM128_Key_Length]; @@ -1098,7 +1095,7 @@ - (void)asyncGetCommissionerOnMatterQueue:(void (^)(chip::Controller::DeviceComm return; } - block(self.cppCommissioner); + block(self->_cppCommissioner); }); } @@ -1208,7 +1205,7 @@ - (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 = self.nodeIDToDeviceMap[@(nodeID)]; + MTRDevice * device = _nodeIDToDeviceMap[@(nodeID)]; os_unfair_lock_unlock(&_deviceMapLock); if (device == nil) { @@ -1400,7 +1397,7 @@ - (BOOL)pairDevice:(uint64_t)deviceID VerifyOrReturnValue(![MTRDeviceController checkForError:errorCode logMsg:kErrorSetupCodeGen error:error], NO); self->_operationalCredentialsDelegate->SetDeviceID(deviceID); - errorCode = self.cppCommissioner->EstablishPASEConnection(deviceID, manualPairingCode.c_str()); + errorCode = self->_cppCommissioner->EstablishPASEConnection(deviceID, manualPairingCode.c_str()); return ![MTRDeviceController checkForError:errorCode logMsg:kErrorPairDevice error:error]; }; @@ -1421,7 +1418,7 @@ - (BOOL)pairDevice:(uint64_t)deviceID self->_operationalCredentialsDelegate->SetDeviceID(deviceID); auto params = chip::RendezvousParameters().SetSetupPINCode(setupPINCode).SetPeerAddress(peerAddress); - auto errorCode = self.cppCommissioner->EstablishPASEConnection(deviceID, params); + auto errorCode = self->_cppCommissioner->EstablishPASEConnection(deviceID, params); return ![MTRDeviceController checkForError:errorCode logMsg:kErrorPairDevice error:error]; }; @@ -1432,7 +1429,7 @@ - (BOOL)pairDevice:(uint64_t)deviceID onboardingPayload:(NSString *)onboardingPa { auto block = ^BOOL { self->_operationalCredentialsDelegate->SetDeviceID(deviceID); - auto errorCode = self.cppCommissioner->EstablishPASEConnection(deviceID, [onboardingPayload UTF8String]); + auto errorCode = self->_cppCommissioner->EstablishPASEConnection(deviceID, [onboardingPayload UTF8String]); return ![MTRDeviceController checkForError:errorCode logMsg:kErrorPairDevice error:error]; }; @@ -1468,7 +1465,7 @@ - (BOOL)openPairingWindow:(uint64_t)deviceID duration:(NSUInteger)duration error auto block = ^BOOL { auto errorCode = chip::Controller::AutoCommissioningWindowOpener::OpenBasicCommissioningWindow( - self.cppCommissioner, deviceID, chip::System::Clock::Seconds16(static_cast(duration))); + self->_cppCommissioner, deviceID, chip::System::Clock::Seconds16(static_cast(duration))); return ![MTRDeviceController checkForError:errorCode logMsg:kErrorOpenPairingWindow error:error]; }; @@ -1508,7 +1505,7 @@ - (NSString *)openPairingWindowWithPIN:(uint64_t)deviceID auto block = ^NSString * { chip::SetupPayload setupPayload; - auto errorCode = chip::Controller::AutoCommissioningWindowOpener::OpenCommissioningWindow(self.cppCommissioner, deviceID, + auto errorCode = chip::Controller::AutoCommissioningWindowOpener::OpenCommissioningWindow(self->_cppCommissioner, deviceID, chip::System::Clock::Seconds16(static_cast(duration)), chip::Crypto::kSpake2p_Min_PBKDF_Iterations, static_cast(discriminator), chip::MakeOptional(static_cast(setupPIN)), chip::NullOptional, setupPayload); diff --git a/src/darwin/Framework/CHIP/MTRThreadOperationalDataset.mm b/src/darwin/Framework/CHIP/MTRThreadOperationalDataset.mm index bb5db0af9a977b..a547321d88c2e0 100644 --- a/src/darwin/Framework/CHIP/MTRThreadOperationalDataset.mm +++ b/src/darwin/Framework/CHIP/MTRThreadOperationalDataset.mm @@ -29,14 +29,9 @@ size_t const MTRSizeThreadPSKc = chip::Thread::kSizePSKc; size_t const MTRSizeThreadPANID = 2; // Thread's PAN ID is 2 bytes -@interface MTRThreadOperationalDataset () - -@property (readonly) chip::Thread::OperationalDataset cppThreadOperationalDataset; -@property (nonatomic, copy) NSNumber * channelNumber; - -@end - -@implementation MTRThreadOperationalDataset +@implementation MTRThreadOperationalDataset { + chip::Thread::OperationalDataset _cppThreadOperationalDataset; +} - (instancetype _Nullable)initWithNetworkName:(NSString *)networkName extendedPANID:(NSData *)extendedPANID @@ -157,7 +152,7 @@ @implementation MTRThreadOperationalDataset (Deprecated) - (void)setChannel:(uint16_t)channel { - self.channelNumber = @(channel); + _channelNumber = @(channel); } - (uint16_t)channel