From 1e4dc6c1c7a1ffd63592bbfe6cfb21ff6fa5f41a Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 26 Sep 2024 13:08:08 -0400 Subject: [PATCH] Move the operationalInstanceAdded/nodeMayBeAdvertisingOperational bits to concrete device/controller. These are only used in the concrete case. --- src/darwin/Framework/CHIP/MTRDevice.mm | 7 ------- src/darwin/Framework/CHIP/MTRDeviceController.mm | 16 ---------------- .../Framework/CHIP/MTRDeviceControllerFactory.mm | 2 +- .../CHIP/MTRDeviceController_Concrete.h | 6 ++++++ .../CHIP/MTRDeviceController_Concrete.mm | 16 ++++++++++++---- .../CHIP/MTRDeviceController_Internal.h | 6 ------ src/darwin/Framework/CHIP/MTRDevice_Concrete.h | 5 +++++ src/darwin/Framework/CHIP/MTRDevice_Internal.h | 5 ----- 8 files changed, 24 insertions(+), 39 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index 812c389e311115..a006d464f4763f 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -432,13 +432,6 @@ - (void)invalidate [_delegates removeAllObjects]; } -- (void)nodeMayBeAdvertisingOperational -{ - assertChipStackLockedByCurrentThread(); - - MTR_LOG("%@ saw new operational advertisement", self); -} - - (BOOL)_delegateExists { os_unfair_lock_assert_owner(&self->_lock); diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index 8b5dee9f6e8b72..c9f070fbc467b3 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -675,22 +675,6 @@ - (void)invalidateCASESessionForNode:(chip::NodeId)nodeID; [self syncRunOnWorkQueue:block error:nil]; } -- (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(self.deviceMapLock); - MTRDevice * device = [_nodeIDToDeviceMap objectForKey:@(nodeID)]; - os_unfair_lock_unlock(self.deviceMapLock); - - if (device == nil) { - return; - } - - ChipLogProgress(Controller, "Notifying device about node 0x" ChipLogFormatX64 " advertising", ChipLogValueX64(nodeID)); - [device nodeMayBeAdvertisingOperational]; -} - - (void)downloadLogFromNodeWithID:(NSNumber *)nodeID type:(MTRDiagnosticLogType)type timeout:(NSTimeInterval)timeout diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm index 1d9d8a30e6970d..085d0a2c55ca6d 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm @@ -1132,7 +1132,7 @@ - (void)operationalInstanceAdded:(chip::PeerId &)operationalID if (compressedFabricId != nil && compressedFabricId.unsignedLongLongValue == operationalID.GetCompressedFabricId()) { ChipLogProgress(Controller, "Notifying controller at fabric index %u about new operational node 0x" ChipLogFormatX64, controller.fabricIndex, ChipLogValueX64(operationalID.GetNodeId())); - [controller operationalInstanceAdded:operationalID.GetNodeId()]; + [controller operationalInstanceAdded:@(operationalID.GetNodeId())]; } // Keep going: more than one controller might match a given compressed diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h index ae45d852ea8471..fe7b562b0cee3d 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h +++ b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.h @@ -117,6 +117,12 @@ NS_ASSUME_NONNULL_BEGIN */ - (void)directlyGetSessionForNode:(chip::NodeId)nodeID completion:(MTRInternalDeviceConnectionCallback)completion; +/** + * Notify the controller that a new operational instance with the given node id + * and a compressed fabric id that matches this controller has been observed. + */ +- (void)operationalInstanceAdded:(NSNumber *)nodeID; + @end NS_ASSUME_NONNULL_END diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm index d0cab185e40bf6..cb86c5d2db044f 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm @@ -1633,20 +1633,28 @@ - (void)invalidateCASESessionForNode:(chip::NodeId)nodeID; [self syncRunOnWorkQueue:block error:nil]; } -- (void)operationalInstanceAdded:(chip::NodeId)nodeID +- (void)operationalInstanceAdded:(NSNumber *)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(self.deviceMapLock); - MTRDevice * device = [self.nodeIDToDeviceMap objectForKey:@(nodeID)]; + MTRDevice * device = [self.nodeIDToDeviceMap objectForKey:nodeID]; os_unfair_lock_unlock(self.deviceMapLock); if (device == nil) { return; } - ChipLogProgress(Controller, "Notifying device about node 0x" ChipLogFormatX64 " advertising", ChipLogValueX64(nodeID)); - [device nodeMayBeAdvertisingOperational]; + // TODO: Can we not just assume this isKindOfClass test is true? Would be + // really nice if we had compile-time checking for this somehow... + if (![device isKindOfClass:MTRDevice_Concrete.class]) { + MTR_LOG_ERROR("%@ somehow has %@ instead of MTRDevice_Concrete for node ID 0x%016llX (%llu)", self, device, nodeID.unsignedLongLongValue, nodeID.unsignedLongLongValue); + return; + } + + MTR_LOG("%@ Notifying %@ about its node advertising", self, device); + auto * concreteDevice = static_cast(device); + [concreteDevice nodeMayBeAdvertisingOperational]; } - (void)downloadLogFromNodeWithID:(NSNumber *)nodeID diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h b/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h index a35761e6fbf670..958e7914b1dd3a 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h +++ b/src/darwin/Framework/CHIP/MTRDeviceController_Internal.h @@ -201,12 +201,6 @@ NS_ASSUME_NONNULL_BEGIN */ - (MTRBaseDevice *)baseDeviceForNodeID:(NSNumber *)nodeID; -/** - * Notify the controller that a new operational instance with the given node id - * and a compressed fabric id that matches this controller has been observed. - */ -- (void)operationalInstanceAdded:(chip::NodeId)nodeID; - /** * Download log of the desired type from the device. */ diff --git a/src/darwin/Framework/CHIP/MTRDevice_Concrete.h b/src/darwin/Framework/CHIP/MTRDevice_Concrete.h index 74a9a5788c5b8b..70dac9a362b10d 100644 --- a/src/darwin/Framework/CHIP/MTRDevice_Concrete.h +++ b/src/darwin/Framework/CHIP/MTRDevice_Concrete.h @@ -26,6 +26,11 @@ NS_ASSUME_NONNULL_BEGIN - (instancetype)initWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceController_Concrete *)controller; +// Called by controller when a new operational advertisement for what we think +// is this device's identity has been observed. This could have +// false-positives, for example due to compressed fabric id collisions. +- (void)nodeMayBeAdvertisingOperational; + @end NS_ASSUME_NONNULL_END diff --git a/src/darwin/Framework/CHIP/MTRDevice_Internal.h b/src/darwin/Framework/CHIP/MTRDevice_Internal.h index f99814941a4710..2cdcd66976099a 100644 --- a/src/darwin/Framework/CHIP/MTRDevice_Internal.h +++ b/src/darwin/Framework/CHIP/MTRDevice_Internal.h @@ -128,11 +128,6 @@ MTR_DIRECT_MEMBERS // called by controller to clean up and shutdown - (void)invalidate; -// Called by controller when a new operational advertisement for what we think -// is this device's identity has been observed. This could have -// false-positives, for example due to compressed fabric id collisions. -- (void)nodeMayBeAdvertisingOperational; - - (BOOL)_callDelegatesWithBlock:(void (^)(id delegate))block; // Called by MTRDevice_XPC to forward delegate callbacks