Disallow read-through on internally-created MTRDevice instances.

We want to be able to instantiate an MTRDevice and readAttibute to get its
cached state without ever triggering read-throughs.

The MTRDiagnosticLogsDownloader.mm changes were because it was using internal
APIs to create an MTRDevice when it does not need an MTRDevice at all.

Author: bzbarsky-apple

src/darwin/Framework/CHIP/MTRDevice.h

@@ -37,13 +37,11 @@ MTR_AVAILABLE(ios(16.1), macos(13.0), watchos(9.1), tvos(16.1))
 + (instancetype)new NS_UNAVAILABLE;
 
 /**
- * TODO: Document usage better
+ * Get an MTRDevice object representing a device with a specific node ID
+ * associated with a specific controller.
  *
- * Directly instantiate a MTRDevice with a MTRDeviceController as a shim.
- *
- * All device-specific information would be stored on the device controller, and
- * retrieved when performing actions using a combination of MTRBaseDevice
- * and MTRAsyncCallbackQueue.
+ * MTRDevice objects are stateful, and callers should hold on to the MTRDevice
+ * while they are using it.
  */
 + (MTRDevice *)deviceWithNodeID:(NSNumber *)nodeID
                      controller:(MTRDeviceController *)controller MTR_AVAILABLE(ios(16.4), macos(13.3), watchos(9.4), tvos(16.4));

src/darwin/Framework/CHIP/MTRDevice.mm

@@ -306,6 +306,7 @@ - (instancetype)initForSubclassesWithNodeID:(NSNumber *)nodeID controller:(MTRDe
         _delegates = [NSMutableSet set];
         _deviceController = controller;
         _nodeID = nodeID;
+        _accessedViaPublicAPI = NO;
         _state = MTRDeviceStateUnknown;
     }
 
@@ -321,6 +322,7 @@ - (instancetype)initWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceControlle
         _nodeID = [nodeID copy];
         _fabricIndex = controller.fabricIndex;
         _deviceController = controller;
+        _accessedViaPublicAPI = NO;
         _queue
             = dispatch_queue_create("org.csa-iot.matter.framework.device.workqueue", DISPATCH_QUEUE_SERIAL_WITH_AUTORELEASE_POOL);
         _asyncWorkQueue = [[MTRAsyncWorkQueue alloc] initWithContext:self];
@@ -360,11 +362,21 @@ - (void)dealloc
     MTR_LOG("MTRDevice dealloc: %p", self);
 }
 
-+ (MTRDevice *)deviceWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceController *)controller
++ (MTRDevice *)_deviceWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceController *)controller
 {
     return [controller deviceForNodeID:nodeID];
 }
 
++ (MTRDevice *)deviceWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceController *)controller
+{
+    auto * device = [self _deviceWithNodeID:nodeID controller:controller];
+    if (device) {
+        std::lock_guard lock(device->_lock);
+        device->_accessedViaPublicAPI = YES;
+    }
+    return device;
+}
+
 #pragma mark - Time Synchronization
 
 - (void)_setTimeOnDevice

src/darwin/Framework/CHIP/MTRDevice_Concrete.mm

@@ -464,11 +464,6 @@ - (NSString *)description
         stringWithFormat:@"<MTRDevice: %p, XPC: NO, node: %016llX-%016llX (%llu), VID: %@, PID: %@, WiFi: %@, Thread: %@, state: %@, last subscription attempt wait: %lus, queued work: %lu, last report: %@%@, last subscription failure: %@%@, controller: %@>", self, _deviceController.compressedFabricID.unsignedLongLongValue, _nodeID.unsignedLongLongValue, _nodeID.unsignedLongLongValue, vid, pid, wifi, thread, InternalDeviceStateString(internalDeviceState), static_cast<unsigned long>(lastSubscriptionAttemptWait), static_cast<unsigned long>(_asyncWorkQueue.itemCount), mostRecentReportTime, reportAge, lastSubscriptionFailureTime, subscriptionFailureAge, _deviceController.uniqueIdentifier];
 }
 
-+ (MTRDevice *)deviceWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceController *)controller
-{
-    return [controller deviceForNodeID:nodeID];
-}
-
 #pragma mark - Time Synchronization
 
 - (void)_setTimeOnDevice
@@ -2593,7 +2588,15 @@ static BOOL AttributeHasChangesOmittedQuality(MTRAttributePath * attributePath)
     // 2. The attribute has the Changes Omitted quality, so we won't get reports for it.
     // 3. The attribute is not in the spec, and the read params asks to assume
     //    an unknown attribute has the Changes Omitted quality.
-    if (![self _subscriptionAbleToReport] || hasChangesOmittedQuality) {
+    //
+    // But all this only happens if this device has been accessed via the public
+    // API.  If it's a device we just created internally, don't do read-throughs.
+    BOOL readThroughsAllowed;
+    {
+        std::lock_guard lock(_lock);
+        readThroughsAllowed = _accessedViaPublicAPI;
+    }
+    if (readThroughsAllowed && (![self _subscriptionAbleToReport] || hasChangesOmittedQuality)) {
         // Read requests container will be a mutable array of items, each being an array containing:
         //   [attribute request path, params]
         // Batching handler should only coalesce when params are equal.

src/darwin/Framework/CHIP/MTRDevice_Internal.h

@@ -120,8 +120,19 @@ MTR_DIRECT_MEMBERS
     // Our controller.  Declared nullable because our property is, though in
     // practice it does not look like we ever set it to nil.
     MTRDeviceController * _Nullable _deviceController;
+
+    // Whether this device has been accessed via the public deviceWithNodeID API
+    // (as opposed to just via the internal _deviceWithNodeID).
+    BOOL _accessedViaPublicAPI;
 }
 
+/**
+ * Internal way of creating an MTRDevice that does not flag the device as being
+ * visible to external API consumers.
+ */
++ (MTRDevice *)_deviceWithNodeID:(NSNumber *)nodeID
+                      controller:(MTRDeviceController *)controller;
+
 - (instancetype)initForSubclassesWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceController *)controller;
 - (instancetype)initWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceController *)controller;
 

src/darwin/Framework/CHIP/MTRDiagnosticLogsDownloader.mm

@@ -16,10 +16,13 @@
  */
 
 #import "MTRDiagnosticLogsDownloader.h"
+#import <Matter/Matter.h>
 
+#include <platform/CHIPDeviceLayer.h>
+#include <platform/LockTracker.h>
 #include <protocols/bdx/BdxTransferServerDelegate.h>
+#include <protocols/bdx/DiagnosticLogs.h>
 
-#import "MTRDeviceControllerFactory_Internal.h"
 #import "MTRDeviceController_Internal.h"
 #import "MTRError_Internal.h"
 #import "MTRLogging_Internal.h"
@@ -425,15 +428,15 @@ - (void)downloadLogFromNodeWithID:(NSNumber *)nodeID
         [download checkInteractionModelResponse:response error:error];
     };
 
-    auto * device = [controller deviceForNodeID:nodeID];
-    auto * cluster = [[MTRClusterDiagnosticLogs alloc] initWithDevice:device endpointID:@(kDiagnosticLogsEndPoint) queue:queue];
+    auto * device = [MTRBaseDevice deviceWithNodeID:nodeID controller:controller];
+    auto * cluster = [[MTRBaseClusterDiagnosticLogs alloc] initWithDevice:device endpointID:@(kDiagnosticLogsEndPoint) queue:queue];
 
     auto * params = [[MTRDiagnosticLogsClusterRetrieveLogsRequestParams alloc] init];
     params.intent = @(type);
     params.requestedProtocol = @(MTRDiagnosticLogsTransferProtocolBDX);
     params.transferFileDesignator = download.fileDesignator;
 
-    [cluster retrieveLogsRequestWithParams:params expectedValues:nil expectedValueInterval:nil completion:interactionModelDone];
+    [cluster retrieveLogsRequestWithParams:params completion:interactionModelDone];
 
     if (timeoutInSeconds > 0) {
         auto err = _bridge->StartBDXTransferTimeout(download, timeoutInSeconds);