Skip to content

Commit

Permalink
Remove some more unused bits in MTRDeviceController. (#35801)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
bzbarsky-apple authored Sep 27, 2024
1 parent 975eaa6 commit 1b4fa25
Show file tree
Hide file tree
Showing 9 changed files with 39 additions and 88 deletions.
81 changes: 10 additions & 71 deletions src/darwin/Framework/CHIP/MTRDeviceController.mm
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,6 @@ @implementation MTRDeviceController {
MTRDeviceControllerDelegateBridge * _deviceControllerDelegateBridge;
MTROperationalCredentialsDelegate * _operationalCredentialsDelegate;
MTRDeviceAttestationDelegateBridge * _deviceAttestationDelegateBridge;
MTRDeviceControllerFactory * _factory;
os_unfair_lock _underlyingDeviceMapLock;
MTRCommissionableBrowser * _commissionableBrowser;
MTRAttestationTrustStoreBridge * _attestationTrustStoreBridge;
Expand Down Expand Up @@ -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<chip::SessionHandle> & 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<chip::SessionHandle> 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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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<MTRAccessGrant *> *)accessGrantsForClusterPath:(MTRClusterPath *)clusterPath
Expand Down
2 changes: 1 addition & 1 deletion src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 8 additions & 0 deletions src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

#import <Matter/Matter.h>

#import "MTRDeviceConnectionBridge.h"
#import "MTRDeviceControllerStartupParams_Internal.h"

NS_ASSUME_NONNULL_BEGIN
Expand Down Expand Up @@ -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
7 changes: 0 additions & 7 deletions src/darwin/Framework/CHIP/MTRDeviceController_Internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -243,13 +243,6 @@ NS_ASSUME_NONNULL_BEGIN
- (MTRDevice *)_setupDeviceForNodeID:(NSNumber *)nodeID prefetchedClusterData:(nullable NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> *)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

/**
Expand Down
10 changes: 9 additions & 1 deletion src/darwin/Framework/CHIP/MTRDevice_Concrete.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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<chip::SessionHandle> & session, NSError * _Nullable error,
Expand Down Expand Up @@ -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<MTRDeviceController_Concrete *>(_deviceController);
}

@end

/* BEGIN DRAGONS: Note methods here cannot be renamed, and are used by private callers, do not rename, remove or modify behavior here */
Expand Down
3 changes: 2 additions & 1 deletion src/darwin/Framework/CHIP/MTRDevice_Internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
6 changes: 4 additions & 2 deletions src/darwin/Framework/CHIP/MTRDiagnosticLogsDownloader.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
#import <Matter/MTRDeviceController.h>
#import <Matter/MTRDiagnosticLogsType.h>

#import "MTRDeviceController_Concrete.h"

namespace chip {
namespace bdx {
class BDXTransferServerDelegate;
Expand All @@ -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

Expand Down
8 changes: 4 additions & 4 deletions src/darwin/Framework/CHIP/MTRDiagnosticLogsDownloader.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -354,7 +354,7 @@ - (MTRDownload * _Nullable)add:(MTRDiagnosticLogType)type
return download;
}

- (void)abortDownloadsForController:(MTRDeviceController *)controller
- (void)abortDownloadsForController:(MTRDeviceController_Concrete *)controller
{
assertChipStackLockedByCurrentThread();

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -462,7 +462,7 @@ - (void)downloadLogFromNodeWithID:(NSNumber *)nodeID
}
}

- (void)abortDownloadsForController:(MTRDeviceController *)controller;
- (void)abortDownloadsForController:(MTRDeviceController_Concrete *)controller;
{
assertChipStackLockedByCurrentThread();

Expand Down

0 comments on commit 1b4fa25

Please sign in to comment.