Address post-landing review comments from PR 22521. (#22843)

The comments came in after that PR merged. * Store NSNumber as the NodeID in MTRDevice. * Have a helper in MTRDevice for creating MTRBaseDevice instances.
project-chip · Oct 27, 2023 · 3502542 · 3502542
1 parent 9ab8026
commit 3502542
Show file tree

Hide file tree

Showing 5 changed files with 203 additions and 384 deletions.
diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm
@@ -146,7 +146,7 @@ - (instancetype)initWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceControlle
 {
     if (self = [super init]) {
         _lock = OS_UNFAIR_LOCK_INIT;
-        _nodeID = [nodeID unsignedLongLongValue];
+        _nodeID = [nodeID copy];
         _deviceController = controller;
         _queue = dispatch_queue_create("com.apple.matter.framework.xpc.workqueue", DISPATCH_QUEUE_SERIAL);
         ;
@@ -286,7 +286,7 @@ - (void)subscribeWithMinInterval:(uint16_t)minInterval maxInterval:(uint16_t)max
 
     _subscriptionActive = YES;
 
-    [_deviceController getSessionForNode:_nodeID
+    [_deviceController getSessionForNode:[_nodeID unsignedLongLongValue]
                               completion:^(chip::Messaging::ExchangeManager * _Nullable exchangeManager,
                                   const chip::Optional<chip::SessionHandle> & session, NSError * _Nullable error) {
                                   if (error != nil) {
@@ -369,7 +369,7 @@ - (void)subscribeWithMinInterval:(uint16_t)minInterval maxInterval:(uint16_t)max
     // Create work item, set ready handler to perform task, then enqueue the work
     MTRAsyncCallbackQueueWorkItem * workItem = [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:_queue];
     MTRAsyncCallbackReadyHandler readyHandler = ^(MTRDevice * device, NSUInteger retryCount) {
-        MTRBaseDevice * baseDevice = [[MTRBaseDevice alloc] initWithNodeID:@(self.nodeID) controller:self.deviceController];
+        MTRBaseDevice * baseDevice = [self newBaseDevice];
 
         [baseDevice readAttributePathWithEndpointID:endpointID
                                           clusterID:clusterID
@@ -412,7 +412,7 @@ - (void)writeAttributeWithEndpointID:(NSNumber *)endpointID
                    timedWriteTimeout:(NSNumber * _Nullable)timeout
 {
     // Start the asynchronous operation
-    MTRBaseDevice * baseDevice = [[MTRBaseDevice alloc] initWithNodeID:@(self.nodeID) controller:self.deviceController];
+    MTRBaseDevice * baseDevice = [self newBaseDevice];
     [baseDevice
         writeAttributeWithEndpointID:endpointID
                            clusterID:clusterID
@@ -446,7 +446,7 @@ - (void)invokeCommandWithEndpointID:(NSNumber *)endpointID
                          completion:(MTRDeviceResponseHandler)completion
 {
     // Perform this operation
-    MTRBaseDevice * baseDevice = [[MTRBaseDevice alloc] initWithNodeID:@(self.nodeID) controller:self.deviceController];
+    MTRBaseDevice * baseDevice = [self newBaseDevice];
     [baseDevice
         invokeCommandWithEndpointID:endpointID
                           clusterID:clusterID
@@ -469,7 +469,7 @@ - (void)openCommissioningWindowWithSetupPasscode:(NSNumber *)setupPasscode
                                            queue:(dispatch_queue_t)queue
                                       completion:(MTRDeviceOpenCommissioningWindowHandler)completion
 {
-    auto * baseDevice = [[MTRBaseDevice alloc] initWithNodeID:@(self.nodeID) controller:self.deviceController];
+    auto * baseDevice = [self newBaseDevice];
     [baseDevice openCommissioningWindowWithSetupPasscode:setupPasscode
                                            discriminator:discriminator
                                                 duration:duration
@@ -689,6 +689,11 @@ - (void)setExpectedValues:(NSArray<NSDictionary<NSString *, id> *> *)values expe
     os_unfair_lock_unlock(&self->_lock);
 }
 
+- (MTRBaseDevice *)newBaseDevice
+{
+    return [[MTRBaseDevice alloc] initWithNodeID:self.nodeID controller:self.deviceController];
+}
+
 @end
 
 #pragma mark - SubscriptionCallback

diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm
@@ -524,11 +524,11 @@ - (MTRDevice *)deviceForNodeID:(NSNumber *)nodeID
 - (void)removeDevice:(MTRDevice *)device
 {
     os_unfair_lock_lock(&_deviceMapLock);
-    MTRDevice * deviceToRemove = self.nodeIDToDeviceMap[@(device.nodeID)];
+    MTRDevice * deviceToRemove = self.nodeIDToDeviceMap[device.nodeID];
     if (deviceToRemove == device) {
-        self.nodeIDToDeviceMap[@(device.nodeID)] = nil;
+        self.nodeIDToDeviceMap[device.nodeID] = nil;
     } else {
-        MTR_LOG_ERROR("Error: Cannot remove device %p with nodeID %llu", device, device.nodeID);
+        MTR_LOG_ERROR("Error: Cannot remove device %p with nodeID %llu", device, [device.nodeID unsignedLongLongValue]);
     }
     os_unfair_lock_unlock(&_deviceMapLock);
 }

diff --git a/src/darwin/Framework/CHIP/MTRDevice_Internal.h b/src/darwin/Framework/CHIP/MTRDevice_Internal.h
@@ -33,8 +33,8 @@ typedef void (^MTRDevicePerformAsyncBlock)(MTRBaseDevice * baseDevice);
 - (void)setExpectedValues:(NSArray<NSDictionary<NSString *, id> *> *)values
     expectedValueInterval:(NSNumber *)expectedValueIntervalMs;
 
-@property (nonatomic, readonly, strong, nonnull) MTRDeviceController * deviceController;
-@property (nonatomic, readonly) uint64_t nodeID;
+@property (nonatomic, readonly) MTRDeviceController * deviceController;
+@property (nonatomic, readonly, copy) NSNumber * nodeID;
 
 @end
 

diff --git a/src/darwin/Framework/CHIP/templates/MTRClusters-src.zapt b/src/darwin/Framework/CHIP/templates/MTRClusters-src.zapt
@@ -55,7 +55,7 @@ using chip::SessionHandle;
 {
     // Make a copy of params before we go async.
     params = [params copy];
-    MTRBaseDevice *baseDevice = [[MTRBaseDevice alloc] initWithNodeID:@(self.device.nodeID) controller:self.device.deviceController];
+    MTRBaseDevice *baseDevice = [[MTRBaseDevice alloc] initWithNodeID:self.device.nodeID controller:self.device.deviceController];
     new MTR{{>callbackName}}CallbackBridge(self.callbackQueue,
       baseDevice,
       {{#if hasSpecificResponse}}