Make MTRBaseDevice be explicit about when it's assuming a concrete co…

…ntroller. (#35957) * Make MTRBaseDevice be explicit about when it's assuming a concrete controller. This allows us to move several selectors from MTRDeviceController_Internal to MTRDeviceController_Concrete, and get rid of the never-actually-set _cppCommissioner ivar on MTRDeviceController. * Address review comment: add logs.
project-chip · Oct 9, 2024 · 50b5dfb · 50b5dfb
1 parent 59f7615
commit 50b5dfb
Show file tree

Hide file tree

Showing 6 changed files with 235 additions and 216 deletions.
diff --git a/src/darwin/Framework/CHIP/MTRBaseDevice.mm b/src/darwin/Framework/CHIP/MTRBaseDevice.mm
diff --git a/src/darwin/Framework/CHIP/MTRCallbackBridgeBase.h b/src/darwin/Framework/CHIP/MTRCallbackBridgeBase.h
@@ -81,6 +81,9 @@ class MTRCallbackBridgeBase {
     {
         LogRequestStart();
 
+        // TODO: Figure out whether we can usefully get an MTRDeviceController_Concrete in here, so
+        // we can move getSessionForCommissioneeDevice off of MTRDeviceController_Internal.  Ideally
+        // without bloating this inline method too much.
         [device.deviceController getSessionForCommissioneeDevice:device.nodeID
                                                       completion:^(chip::Messaging::ExchangeManager * exchangeManager,
                                                           const chip::Optional<chip::SessionHandle> & session, NSError * _Nullable error, NSNumber * _Nullable retryDelay) {
@@ -92,6 +95,8 @@ class MTRCallbackBridgeBase {
     {
         LogRequestStart();
 
+        // TODO: Figure out whether we can usefully get an MTRDeviceController_Concrete in here, so
+        // we can move getSessionForNode off of MTRDeviceController_Internal.
         [controller getSessionForNode:nodeID
                            completion:^(chip::Messaging::ExchangeManager * _Nullable exchangeManager,
                                const chip::Optional<chip::SessionHandle> & session, NSError * _Nullable error, NSNumber * _Nullable retryDelay) {

diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm
@@ -119,7 +119,6 @@ - (instancetype)initWithDelegate:(id<MTRDeviceControllerDelegate>)delegate queue
 @end
 
 @implementation MTRDeviceController {
-    chip::Controller::DeviceCommissioner * _cppCommissioner;
     chip::Credentials::PartialDACVerifier * _partialDACVerifier;
     chip::Credentials::DefaultDACVerifier * _defaultDACVerifier;
     MTRDeviceControllerDelegateBridge * _deviceControllerDelegateBridge;
@@ -206,7 +205,8 @@ - (NSString *)description
 
 - (BOOL)isRunning
 {
-    return _cppCommissioner != nullptr;
+    MTR_ABSTRACT_METHOD();
+    return NO;
 }
 
 #pragma mark - Suspend/resume support
@@ -547,37 +547,9 @@ - (void)getSessionForCommissioneeDevice:(chip::NodeId)deviceID completion:(MTRIn
     completion(nullptr, chip::NullOptional, [MTRError errorForCHIPErrorCode:CHIP_ERROR_INCORRECT_STATE], nil);
 }
 
-- (MTRTransportType)sessionTransportTypeForDevice:(MTRBaseDevice *)device
-{
-    VerifyOrReturnValue([self checkIsRunning], MTRTransportTypeUndefined);
-
-    __block MTRTransportType result = MTRTransportTypeUndefined;
-    dispatch_sync(_chipWorkQueue, ^{
-        VerifyOrReturn([self checkIsRunning]);
-
-        if (device.isPASEDevice) {
-            chip::CommissioneeDeviceProxy * deviceProxy;
-            VerifyOrReturn(CHIP_NO_ERROR == self->_cppCommissioner->GetDeviceBeingCommissioned(device.nodeID, &deviceProxy));
-            result = MTRMakeTransportType(deviceProxy->GetDeviceTransportType());
-        } else {
-            auto scopedNodeID = self->_cppCommissioner->GetPeerScopedId(device.nodeID);
-            auto sessionHandle = self->_cppCommissioner->SessionMgr()->FindSecureSessionForNode(scopedNodeID);
-            VerifyOrReturn(sessionHandle.HasValue());
-            result = MTRMakeTransportType(sessionHandle.Value()->AsSecureSession()->GetPeerAddress().GetTransportType());
-        }
-    });
-    return result;
-}
-
-- (void)asyncGetCommissionerOnMatterQueue:(void (^)(chip::Controller::DeviceCommissioner *))block
-                             errorHandler:(nullable MTRDeviceErrorHandler)errorHandler
-{
-    MTR_ABSTRACT_METHOD();
-    errorHandler([MTRError errorForCHIPErrorCode:CHIP_ERROR_INCORRECT_STATE]);
-}
-
 - (void)asyncDispatchToMatterQueue:(dispatch_block_t)block errorHandler:(nullable MTRDeviceErrorHandler)errorHandler
 {
+    // TODO: Figure out how to get callsites to have an MTRDeviceController_Concrete.
     MTR_ABSTRACT_METHOD();
     errorHandler([MTRError errorForCHIPErrorCode:CHIP_ERROR_INCORRECT_STATE]);
 }
@@ -627,31 +599,6 @@ - (nullable NSNumber *)compressedFabricID
     return storedValue.has_value() ? @(storedValue.value()) : nil;
 }
 
-- (void)invalidateCASESessionForNode:(chip::NodeId)nodeID;
-{
-    auto block = ^{
-        auto sessionMgr = self->_cppCommissioner->SessionMgr();
-        VerifyOrDie(sessionMgr != nullptr);
-
-        sessionMgr->MarkSessionsAsDefunct(
-            self->_cppCommissioner->GetPeerScopedId(nodeID), chip::MakeOptional(chip::Transport::SecureSession::Type::kCASE));
-    };
-
-    [self syncRunOnWorkQueue:block error:nil];
-}
-
-- (void)downloadLogFromNodeWithID:(NSNumber *)nodeID
-                             type:(MTRDiagnosticLogType)type
-                          timeout:(NSTimeInterval)timeout
-                            queue:(dispatch_queue_t)queue
-                       completion:(void (^)(NSURL * _Nullable url, NSError * _Nullable error))completion
-{
-    MTR_ABSTRACT_METHOD();
-    dispatch_async(queue, ^{
-        completion(nil, [MTRError errorForCHIPErrorCode:CHIP_ERROR_INCORRECT_STATE]);
-    });
-}
-
 #ifdef DEBUG
 + (void)forceLocalhostAdvertisingOnly
 {

diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h
@@ -18,6 +18,7 @@
 #import <Foundation/Foundation.h>
 
 #import <Matter/MTRAccessGrant.h>
+#import <Matter/MTRBaseDevice.h>
 #import <Matter/MTRDefines.h>
 #import <Matter/MTRDeviceController.h>
 #import <Matter/MTRDeviceControllerFactory.h>
@@ -149,6 +150,43 @@ NS_ASSUME_NONNULL_BEGIN
  */
 - (nullable NSNumber *)neededReadPrivilegeForClusterID:(NSNumber *)clusterID attributeID:(NSNumber *)attributeID;
 
+/**
+ * Try to asynchronously dispatch the given block on the Matter queue.  If the
+ * controller is not running either at call time or when the block would be
+ * about to run, the provided error handler will be called with an error.  Note
+ * that this means the error handler might be called on an arbitrary queue, and
+ * might be called before this function returns or after it returns.
+ *
+ * The DeviceCommissioner pointer passed to the callback should only be used
+ * synchronously during the callback invocation.
+ *
+ * If the error handler is nil, failure to run the block will be silent.
+ */
+- (void)asyncGetCommissionerOnMatterQueue:(void (^)(chip::Controller::DeviceCommissioner *))block
+                             errorHandler:(nullable MTRDeviceErrorHandler)errorHandler;
+
+/**
+ * Returns the transport used by the current session with the given device,
+ * or `MTRTransportTypeUndefined` if no session is currently active.
+ */
+- (MTRTransportType)sessionTransportTypeForDevice:(MTRBaseDevice *)device;
+
+/**
+ * Invalidate the CASE session for the given node ID.  This is a temporary thing
+ * just to support MTRBaseDevice's invalidateCASESession.  Must not be called on
+ * the Matter event queue.
+ */
+- (void)invalidateCASESessionForNode:(NSNumber *)nodeID;
+
+/**
+ * Download log of the desired type from the device.
+ */
+- (void)downloadLogFromNodeWithID:(NSNumber *)nodeID
+                             type:(MTRDiagnosticLogType)type
+                          timeout:(NSTimeInterval)timeout
+                            queue:(dispatch_queue_t)queue
+                       completion:(void (^)(NSURL * _Nullable url, NSError * _Nullable error))completion;
+
 @end
 
 NS_ASSUME_NONNULL_END
diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm
@@ -1586,14 +1586,14 @@ - (CHIP_ERROR)isRunningOnFabric:(chip::FabricTable *)fabricTable
     return CHIP_NO_ERROR;
 }
 
-- (void)invalidateCASESessionForNode:(chip::NodeId)nodeID;
+- (void)invalidateCASESessionForNode:(NSNumber *)nodeID;
 {
     auto block = ^{
         auto sessionMgr = self->_cppCommissioner->SessionMgr();
         VerifyOrDie(sessionMgr != nullptr);
 
         sessionMgr->MarkSessionsAsDefunct(
-            self->_cppCommissioner->GetPeerScopedId(nodeID), chip::MakeOptional(chip::Transport::SecureSession::Type::kCASE));
+            self->_cppCommissioner->GetPeerScopedId(nodeID.unsignedLongLongValue), chip::MakeOptional(chip::Transport::SecureSession::Type::kCASE));
     };
 
     [self syncRunOnWorkQueue:block error:nil];

diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h b/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h
@@ -154,34 +154,6 @@ NS_ASSUME_NONNULL_BEGIN
  */
 - (void)getSessionForCommissioneeDevice:(chip::NodeId)deviceID completion:(MTRInternalDeviceConnectionCallback)completion;
 
-/**
- * Returns the transport used by the current session with the given device,
- * or `MTRTransportTypeUndefined` if no session is currently active.
- */
-- (MTRTransportType)sessionTransportTypeForDevice:(MTRBaseDevice *)device;
-
-/**
- * Invalidate the CASE session for the given node ID.  This is a temporary thing
- * just to support MTRBaseDevice's invalidateCASESession.  Must not be called on
- * the Matter event queue.
- */
-- (void)invalidateCASESessionForNode:(chip::NodeId)nodeID;
-
-/**
- * Try to asynchronously dispatch the given block on the Matter queue.  If the
- * controller is not running either at call time or when the block would be
- * about to run, the provided error handler will be called with an error.  Note
- * that this means the error handler might be called on an arbitrary queue, and
- * might be called before this function returns or after it returns.
- *
- * The DeviceCommissioner pointer passed to the callback should only be used
- * synchronously during the callback invocation.
- *
- * If the error handler is nil, failure to run the block will be silent.
- */
-- (void)asyncGetCommissionerOnMatterQueue:(void (^)(chip::Controller::DeviceCommissioner *))block
-                             errorHandler:(nullable MTRDeviceErrorHandler)errorHandler;
-
 /**
  * Try to asynchronously dispatch the given block on the Matter queue.  If the
  * controller is not running either at call time or when the block would be
@@ -200,15 +172,6 @@ NS_ASSUME_NONNULL_BEGIN
  */
 - (MTRBaseDevice *)baseDeviceForNodeID:(NSNumber *)nodeID;
 
-/**
- * Download log of the desired type from the device.
- */
-- (void)downloadLogFromNodeWithID:(NSNumber *)nodeID
-                             type:(MTRDiagnosticLogType)type
-                          timeout:(NSTimeInterval)timeout
-                            queue:(dispatch_queue_t)queue
-                       completion:(void (^)(NSURL * _Nullable url, NSError * _Nullable error))completion;
-
 #pragma mark - Device-specific data and SDK access
 // DeviceController will act as a central repository for this opaque dictionary that MTRDevice manages
 - (MTRDevice *)deviceForNodeID:(NSNumber *)nodeID;