From 1b4fa25e56a1e9ff9dae10f955ab09e2d47c82a2 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Fri, 27 Sep 2024 03:49:21 -0400 Subject: [PATCH] Remove some more unused bits in MTRDeviceController. (#35801) Controller's downloadLogFromNodeWithID is only used from MTRBaseDevice, which is not expected to be used with a non-concrete controller. Unfortunately, nothing actually prevents an MTRBaseDevice beign created against a non-concrete controller, so we can't just move this API to MTRDeviceController_Concrete. With the implementation of downloadLogFromNodeWithID removed, _factory is unused and can be removed. Also, with this implementation removed the factory's downloadLogFromNodeWithID can take MTRDeviceController_Concrete, as can the diagnostic log downloader. Similarly, getSessionFromNode is only used from MTRBaseDevice and MTRCallbackBridgeBase's ActionWithNodeID. And ActionWithNodeID is only used from DispatchAction, which is only called from MTRBaseClusters and MTRBaseDevice, none of which is expected to work with a non-concrete controller. So this implementation can also be removed. At that point directlyGetSessionForNode is only used from MTRDevice_Concrete, so we can just move it to MTRDeviceController_Concrete. getSessionForCommissioneeDevice is only used from ActionWithPASEDevice, which is only used from DispatchAction, like getSessionFromNode. So this implementation can also be removed. At this point asyncDispatchToMatterQueue is only used from: * MTRBaseDevice * MTRClusterStateCacheContainer, where it's used on a controller coming from MTRBaseDevice. * MTRDevice_Concrete, where it's being used on a concrete controller. * MTRDiagnosticLogsDownloader, where it's being used on a concrete controller. * MTRServerAttribute/MTRServerCluster/MTRServerEndpoint, which are not usable with non-concrete controllers as things stand. * MTROTAProviderDelegateBridge, where its being used on a concrete controller. So the MTRDeviceController implementation of asyncDispatchToMatterQueue can also be removed. --- .../Framework/CHIP/MTRDeviceController.mm | 81 +++---------------- .../CHIP/MTRDeviceControllerFactory.mm | 2 +- .../MTRDeviceControllerFactory_Internal.h | 2 +- .../CHIP/MTRDeviceController_Concrete.h | 8 ++ .../CHIP/MTRDeviceController_Internal.h | 7 -- .../Framework/CHIP/MTRDevice_Concrete.mm | 10 ++- .../Framework/CHIP/MTRDevice_Internal.h | 3 +- .../CHIP/MTRDiagnosticLogsDownloader.h | 6 +- .../CHIP/MTRDiagnosticLogsDownloader.mm | 8 +- 9 files changed, 39 insertions(+), 88 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index d1759e3008eee8..8b5dee9f6e8b72 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -126,7 +126,6 @@ @implementation MTRDeviceController { MTRDeviceControllerDelegateBridge * _deviceControllerDelegateBridge; MTROperationalCredentialsDelegate * _operationalCredentialsDelegate; MTRDeviceAttestationDelegateBridge * _deviceAttestationDelegateBridge; - MTRDeviceControllerFactory * _factory; os_unfair_lock _underlyingDeviceMapLock; MTRCommissionableBrowser * _commissionableBrowser; MTRAttestationTrustStoreBridge * _attestationTrustStoreBridge; @@ -554,65 +553,14 @@ - (BOOL)checkIsRunning:(NSError * __autoreleasing *)error - (void)getSessionForNode:(chip::NodeId)nodeID completion:(MTRInternalDeviceConnectionCallback)completion { - // Get the corresponding MTRDevice object to determine if the case/subscription pool is to be used - MTRDevice * device = [self deviceForNodeID:@(nodeID)]; - - // In the case that this device is known to use thread, queue this with subscription attempts as well, to - // help with throttling Thread traffic. - if ([device deviceUsesThread]) { - MTRAsyncWorkItem * workItem = [[MTRAsyncWorkItem alloc] initWithQueue:dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0)]; - [workItem setReadyHandler:^(id _Nonnull context, NSInteger retryCount, MTRAsyncWorkCompletionBlock _Nonnull workItemCompletion) { - MTRInternalDeviceConnectionCallback completionWrapper = ^(chip::Messaging::ExchangeManager * _Nullable exchangeManager, - const chip::Optional & session, NSError * _Nullable error, NSNumber * _Nullable retryDelay) { - completion(exchangeManager, session, error, retryDelay); - workItemCompletion(MTRAsyncWorkComplete); - }; - [self directlyGetSessionForNode:nodeID completion:completionWrapper]; - }]; - - [_concurrentSubscriptionPool enqueueWorkItem:workItem descriptionWithFormat:@"device controller getSessionForNode nodeID: 0x%016llX", nodeID]; - } else { - [self directlyGetSessionForNode:nodeID completion:completion]; - } -} - -- (void)directlyGetSessionForNode:(chip::NodeId)nodeID completion:(MTRInternalDeviceConnectionCallback)completion -{ - [self - asyncGetCommissionerOnMatterQueue:^(chip::Controller::DeviceCommissioner * commissioner) { - auto connectionBridge = new MTRDeviceConnectionBridge(completion); - - // MTRDeviceConnectionBridge always delivers errors async via - // completion. - connectionBridge->connect(commissioner, nodeID); - } - errorHandler:^(NSError * error) { - completion(nullptr, chip::NullOptional, error, nil); - }]; + MTR_ABSTRACT_METHOD(); + completion(nullptr, chip::NullOptional, [MTRError errorForCHIPErrorCode:CHIP_ERROR_INCORRECT_STATE], nil); } - (void)getSessionForCommissioneeDevice:(chip::NodeId)deviceID completion:(MTRInternalDeviceConnectionCallback)completion { - [self - asyncGetCommissionerOnMatterQueue:^(chip::Controller::DeviceCommissioner * commissioner) { - chip::CommissioneeDeviceProxy * deviceProxy; - CHIP_ERROR err = commissioner->GetDeviceBeingCommissioned(deviceID, &deviceProxy); - if (err != CHIP_NO_ERROR) { - completion(nullptr, chip::NullOptional, [MTRError errorForCHIPErrorCode:err], nil); - return; - } - - chip::Optional session = deviceProxy->GetSecureSession(); - if (!session.HasValue() || !session.Value()->AsSecureSession()->IsPASESession()) { - completion(nullptr, chip::NullOptional, [MTRError errorForCHIPErrorCode:CHIP_ERROR_INCORRECT_STATE], nil); - return; - } - - completion(deviceProxy->GetExchangeManager(), session, nil, nil); - } - errorHandler:^(NSError * error) { - completion(nullptr, chip::NullOptional, error, nil); - }]; + MTR_ABSTRACT_METHOD(); + completion(nullptr, chip::NullOptional, [MTRError errorForCHIPErrorCode:CHIP_ERROR_INCORRECT_STATE], nil); } - (MTRTransportType)sessionTransportTypeForDevice:(MTRBaseDevice *)device @@ -665,10 +613,8 @@ - (void)asyncGetCommissionerOnMatterQueue:(void (^)(chip::Controller::DeviceComm - (void)asyncDispatchToMatterQueue:(dispatch_block_t)block errorHandler:(nullable MTRDeviceErrorHandler)errorHandler { - auto adapter = ^(chip::Controller::DeviceCommissioner *) { - block(); - }; - [self asyncGetCommissionerOnMatterQueue:adapter errorHandler:errorHandler]; + MTR_ABSTRACT_METHOD(); + errorHandler([MTRError errorForCHIPErrorCode:CHIP_ERROR_INCORRECT_STATE]); } - (void)syncRunOnWorkQueue:(SyncWorkQueueBlock)block error:(NSError * __autoreleasing *)error @@ -751,17 +697,10 @@ - (void)downloadLogFromNodeWithID:(NSNumber *)nodeID queue:(dispatch_queue_t)queue completion:(void (^)(NSURL * _Nullable url, NSError * _Nullable error))completion { - [self asyncDispatchToMatterQueue:^() { - [self->_factory downloadLogFromNodeWithID:nodeID - controller:self - type:type - timeout:timeout - queue:queue - completion:completion]; - } - errorHandler:^(NSError * error) { - completion(nil, error); - }]; + MTR_ABSTRACT_METHOD(); + dispatch_async(queue, ^{ + completion(nil, [MTRError errorForCHIPErrorCode:CHIP_ERROR_INCORRECT_STATE]); + }); } - (NSArray *)accessGrantsForClusterPath:(MTRClusterPath *)clusterPath diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm index 4cff8a633105b2..1d9d8a30e6970d 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm @@ -1099,7 +1099,7 @@ - (nullable NSNumber *)neededReadPrivilegeForClusterID:(NSNumber *)clusterID att } - (void)downloadLogFromNodeWithID:(NSNumber *)nodeID - controller:(MTRDeviceController *)controller + controller:(MTRDeviceController_Concrete *)controller type:(MTRDiagnosticLogType)type timeout:(NSTimeInterval)timeout queue:(dispatch_queue_t)queue diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory_Internal.h b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory_Internal.h index 1642641717cbe8..3ff9d0268703fa 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory_Internal.h +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory_Internal.h @@ -83,7 +83,7 @@ MTR_DIRECT_MEMBERS * Download log of the desired type from the device. */ - (void)downloadLogFromNodeWithID:(NSNumber *)nodeID - controller:(MTRDeviceController *)controller + controller:(MTRDeviceController_Concrete *)controller type:(MTRDiagnosticLogType)type timeout:(NSTimeInterval)timeout queue:(dispatch_queue_t)queue diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h index ea71f351c72377..ae45d852ea8471 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h +++ b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h @@ -19,6 +19,7 @@ #import +#import "MTRDeviceConnectionBridge.h" #import "MTRDeviceControllerStartupParams_Internal.h" NS_ASSUME_NONNULL_BEGIN @@ -109,6 +110,13 @@ NS_ASSUME_NONNULL_BEGIN */ - (void)clearPendingShutdown; +/** + * Since getSessionForNode now enqueues by the subscription pool for Thread + * devices, MTRDevice_Concrete needs a direct non-queued access because it already + * makes use of the subscription pool. + */ +- (void)directlyGetSessionForNode:(chip::NodeId)nodeID completion:(MTRInternalDeviceConnectionCallback)completion; + @end NS_ASSUME_NONNULL_END diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h b/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h index a052d527b89b86..a35761e6fbf670 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h +++ b/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h @@ -243,13 +243,6 @@ NS_ASSUME_NONNULL_BEGIN - (MTRDevice *)_setupDeviceForNodeID:(NSNumber *)nodeID prefetchedClusterData:(nullable NSDictionary *)prefetchedClusterData; - (void)removeDevice:(MTRDevice *)device; -/** - * Since getSessionForNode now enqueues by the subscription pool for Thread - * devices, MTRDevice needs a direct non-queued access because it already - * makes use of the subscription pool. - */ -- (void)directlyGetSessionForNode:(chip::NodeId)nodeID completion:(MTRInternalDeviceConnectionCallback)completion; - @end /** diff --git a/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm b/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm index a2dbb2dcbaf35d..e69417b1fb23ee 100644 --- a/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm +++ b/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm @@ -2388,7 +2388,7 @@ - (void)_setupSubscriptionWithReason:(NSString *)reason MATTER_LOG_METRIC_BEGIN(kMetricMTRDeviceInitialSubscriptionSetup); // Call directlyGetSessionForNode because the subscription setup already goes through the subscription pool queue - [_deviceController + [[self _concreteController] directlyGetSessionForNode:_nodeID.unsignedLongLongValue completion:^(chip::Messaging::ExchangeManager * _Nullable exchangeManager, const chip::Optional & session, NSError * _Nullable error, @@ -4138,6 +4138,14 @@ - (void)controllerResumed [self _ensureSubscriptionForExistingDelegates:@"Controller resumed"]; } +// nullable because technically _deviceController is nullable. +- (nullable MTRDeviceController_Concrete *)_concreteController +{ + // We know our _deviceController is actually an MTRDeviceController_Concrete, since that's what + // gets passed to initWithNodeID. + return static_cast(_deviceController); +} + @end /* BEGIN DRAGONS: Note methods here cannot be renamed, and are used by private callers, do not rename, remove or modify behavior here */ diff --git a/src/darwin/Framework/CHIP/MTRDevice_Internal.h b/src/darwin/Framework/CHIP/MTRDevice_Internal.h index 6aea99eeda1ffb..f99814941a4710 100644 --- a/src/darwin/Framework/CHIP/MTRDevice_Internal.h +++ b/src/darwin/Framework/CHIP/MTRDevice_Internal.h @@ -118,7 +118,8 @@ MTR_DIRECT_MEMBERS NSNumber * _nodeID; // Our controller. Declared nullable because our property is, though in - // practice it does not look like we ever set it to nil. + // practice it does not look like we ever set it to nil. If this changes, + // fix _concreteController on MTRDevice_Concrete accordingly. MTRDeviceController * _Nullable _deviceController; } diff --git a/src/darwin/Framework/CHIP/MTRDiagnosticLogsDownloader.h b/src/darwin/Framework/CHIP/MTRDiagnosticLogsDownloader.h index da7a780c1237f0..8acc6357729d74 100644 --- a/src/darwin/Framework/CHIP/MTRDiagnosticLogsDownloader.h +++ b/src/darwin/Framework/CHIP/MTRDiagnosticLogsDownloader.h @@ -20,6 +20,8 @@ #import #import +#import "MTRDeviceController_Concrete.h" + namespace chip { namespace bdx { class BDXTransferServerDelegate; @@ -33,14 +35,14 @@ NS_ASSUME_NONNULL_BEGIN // Must be called on Matter queue - (void)downloadLogFromNodeWithID:(NSNumber *)nodeID - controller:(MTRDeviceController *)controller + controller:(MTRDeviceController_Concrete *)controller type:(MTRDiagnosticLogType)type timeout:(NSTimeInterval)timeout queue:(dispatch_queue_t)queue completion:(void (^)(NSURL * _Nullable url, NSError * _Nullable error))completion; // Must be called on Matter queue -- (void)abortDownloadsForController:(MTRDeviceController *)controller; +- (void)abortDownloadsForController:(MTRDeviceController_Concrete *)controller; @end diff --git a/src/darwin/Framework/CHIP/MTRDiagnosticLogsDownloader.mm b/src/darwin/Framework/CHIP/MTRDiagnosticLogsDownloader.mm index fdd8a389f71309..606bd2dbfbc9b5 100644 --- a/src/darwin/Framework/CHIP/MTRDiagnosticLogsDownloader.mm +++ b/src/darwin/Framework/CHIP/MTRDiagnosticLogsDownloader.mm @@ -86,7 +86,7 @@ - (MTRDownload * _Nullable)add:(MTRDiagnosticLogType)type completion:(void (^)(NSURL * _Nullable url, NSError * _Nullable error))completion done:(void (^)(MTRDownload * finishedDownload))done; -- (void)abortDownloadsForController:(MTRDeviceController *)controller; +- (void)abortDownloadsForController:(MTRDeviceController_Concrete *)controller; @end @@ -354,7 +354,7 @@ - (MTRDownload * _Nullable)add:(MTRDiagnosticLogType)type return download; } -- (void)abortDownloadsForController:(MTRDeviceController *)controller +- (void)abortDownloadsForController:(MTRDeviceController_Concrete *)controller { assertChipStackLockedByCurrentThread(); @@ -408,7 +408,7 @@ - (void)dealloc } - (void)downloadLogFromNodeWithID:(NSNumber *)nodeID - controller:(MTRDeviceController *)controller + controller:(MTRDeviceController_Concrete *)controller type:(MTRDiagnosticLogType)type timeout:(NSTimeInterval)timeout queue:(dispatch_queue_t)queue @@ -462,7 +462,7 @@ - (void)downloadLogFromNodeWithID:(NSNumber *)nodeID } } -- (void)abortDownloadsForController:(MTRDeviceController *)controller; +- (void)abortDownloadsForController:(MTRDeviceController_Concrete *)controller; { assertChipStackLockedByCurrentThread();