[Darwin] MTRDevice should not always send reads to device directly

src/darwin/Framework/CHIP/MTRAttributeSpecifiedCheck.h

@@ -0,0 +1,25 @@
+/*
+ *
+ *    Copyright (c) 2023 Project CHIP Authors
+ *    All rights reserved.
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
+#pragma once
+
+#import <Foundation/Foundation.h>
+
+#include <lib/core/DataModelTypes.h>
+
+BOOL MTRAttributeIsSpecified(chip::ClusterId aClusterId, chip::AttributeId aAttributeId);

src/darwin/Framework/CHIP/MTRDevice.mm

@@ -18,6 +18,7 @@
 #import <os/lock.h>
 
 #import "MTRAsyncCallbackWorkQueue.h"
+#import "MTRAttributeSpecifiedCheck.h"
 #import "MTRBaseDevice_Internal.h"
 #import "MTRBaseSubscriptionCallback.h"
 #import "MTRCluster.h"
@@ -254,6 +255,13 @@ - (void)nodeMayBeAdvertisingOperational
     }
 }
 
+// Return YES if there's a valid delegate AND subscription is expected to report value
+- (BOOL)_subscriptionAbleToReport
+{
+    // TODO: include period from when first report comes in until establish callback
+    return (_weakDelegate.strongObject) && (_state == MTRDeviceStateReachable);
+}
+
 // assume lock is held
 - (void)_changeState:(MTRDeviceState)state
 {
@@ -617,52 +625,191 @@ - (void)_setupSubscription
 }
 
 #pragma mark Device Interactions
+
+// Helper function to determine whether an attribute has "Changes Omitted" quality, which indicates that past the priming report in
+// a subscription, this attribute is not expected to be reported when its value changes
+//   * TODO: xml+codegen version to replace this hardcoded list.
+static BOOL AttributeHasChangesOmittedQuality(MTRAttributePath * attributePath)
+{
+    switch (attributePath.cluster.unsignedLongValue) {
+    case MTRClusterEthernetNetworkDiagnosticsID:
+        switch (attributePath.attribute.unsignedLongValue) {
+        case MTRClusterEthernetNetworkDiagnosticsAttributePacketRxCountID:
+        case MTRClusterEthernetNetworkDiagnosticsAttributePacketTxCountID:
+        case MTRClusterEthernetNetworkDiagnosticsAttributeTxErrCountID:
+        case MTRClusterEthernetNetworkDiagnosticsAttributeCollisionCountID:
+        case MTRClusterEthernetNetworkDiagnosticsAttributeOverrunCountID:
+        case MTRClusterEthernetNetworkDiagnosticsAttributeCarrierDetectID:
+        case MTRClusterEthernetNetworkDiagnosticsAttributeTimeSinceResetID:
+            return YES;
+        default:
+            return NO;
+        }
+    case MTRClusterGeneralDiagnosticsID:
+        switch (attributePath.attribute.unsignedLongValue) {
+        case MTRClusterGeneralDiagnosticsAttributeUpTimeID:
+        case MTRClusterGeneralDiagnosticsAttributeTotalOperationalHoursID:
+            return YES;
+        default:
+            return NO;
+        }
+    case MTRClusterThreadNetworkDiagnosticsID:
+        switch (attributePath.attribute.unsignedLongValue) {
+        case MTRClusterThreadNetworkDiagnosticsAttributeOverrunCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeDetachedRoleCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeChildRoleCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeRouterRoleCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeLeaderRoleCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeAttachAttemptCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributePartitionIdChangeCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeBetterPartitionAttachAttemptCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeParentChangeCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeTxTotalCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeTxUnicastCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeTxBroadcastCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeTxAckRequestedCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeTxAckedCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeTxNoAckRequestedCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeTxDataCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeTxDataPollCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeTxBeaconCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeTxBeaconRequestCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeTxOtherCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeTxRetryCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeTxDirectMaxRetryExpiryCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeTxIndirectMaxRetryExpiryCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeTxErrCcaCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeTxErrAbortCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeTxErrBusyChannelCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeRxTotalCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeRxUnicastCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeRxBroadcastCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeRxDataCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeRxDataPollCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeRxBeaconCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeRxBeaconRequestCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeRxOtherCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeRxAddressFilteredCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeRxDestAddrFilteredCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeRxDuplicatedCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeRxErrNoFrameCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeRxErrUnknownNeighborCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeRxErrInvalidSrcAddrCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeRxErrSecCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeRxErrFcsCountID:
+        case MTRClusterThreadNetworkDiagnosticsAttributeRxErrOtherCountID:
+            return YES;
+        default:
+            return NO;
+        }
+    case MTRClusterWiFiNetworkDiagnosticsID:
+        switch (attributePath.attribute.unsignedLongValue) {
+        case MTRClusterWiFiNetworkDiagnosticsAttributeRssiID:
+        case MTRClusterWiFiNetworkDiagnosticsAttributeBeaconLostCountID:
+        case MTRClusterWiFiNetworkDiagnosticsAttributeBeaconRxCountID:
+        case MTRClusterWiFiNetworkDiagnosticsAttributePacketMulticastRxCountID:
+        case MTRClusterWiFiNetworkDiagnosticsAttributePacketMulticastTxCountID:
+        case MTRClusterWiFiNetworkDiagnosticsAttributePacketUnicastRxCountID:
+        case MTRClusterWiFiNetworkDiagnosticsAttributePacketUnicastTxCountID:
+        case MTRClusterWiFiNetworkDiagnosticsAttributeCurrentMaxRateID:
+        case MTRClusterWiFiNetworkDiagnosticsAttributeOverrunCountID:
+            return YES;
+        default:
+            return NO;
+        }
+    case MTRClusterOperationalCredentialsID:
+        switch (attributePath.attribute.unsignedLongValue) {
+        case MTRClusterOperationalCredentialsAttributeNOCsID:
+        case MTRClusterOperationalCredentialsAttributeTrustedRootCertificatesID:
+            return YES;
+        default:
+            return NO;
+        }
+    case MTRClusterPowerSourceID:
+        switch (attributePath.attribute.unsignedLongValue) {
+        case MTRClusterPowerSourceAttributeWiredAssessedInputVoltageID:
+        case MTRClusterPowerSourceAttributeWiredAssessedInputFrequencyID:
+        case MTRClusterPowerSourceAttributeWiredAssessedCurrentID:
+        case MTRClusterPowerSourceAttributeBatVoltageID:
+        case MTRClusterPowerSourceAttributeBatPercentRemainingID:
+        case MTRClusterPowerSourceAttributeBatTimeRemainingID:
+        case MTRClusterPowerSourceAttributeBatTimeToFullChargeID:
+        case MTRClusterPowerSourceAttributeBatChargingCurrentID:
+            return YES;
+        default:
+            return NO;
+        }
+    case MTRClusterTimeSynchronizationID:
+        switch (attributePath.attribute.unsignedLongValue) {
+        case MTRClusterTimeSynchronizationAttributeUTCTimeID:
+        case MTRClusterTimeSynchronizationAttributeLocalTimeID:
+            return YES;
+        default:
+            return NO;
+        }
+    default:
+        return NO;
+    }
+}
+
 - (NSDictionary<NSString *, id> *)readAttributeWithEndpointID:(NSNumber *)endpointID
                                                     clusterID:(NSNumber *)clusterID
                                                   attributeID:(NSNumber *)attributeID
                                                        params:(MTRReadParams *)params
 {
     NSString * logPrefix = [NSString stringWithFormat:@"%@ read %@ %@ %@", self, endpointID, clusterID, attributeID];
-    // Create work item, set ready handler to perform task, then enqueue the work
-    MTRAsyncCallbackQueueWorkItem * workItem = [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:self.queue];
-    MTRAsyncCallbackReadyHandler readyHandler = ^(MTRDevice * device, NSUInteger retryCount) {
-        MTR_LOG_DEFAULT("%@ dequeueWorkItem %@", logPrefix, self->_asyncCallbackWorkQueue);
-        MTRBaseDevice * baseDevice = [self newBaseDevice];
-        [baseDevice
-            readAttributesWithEndpointID:endpointID
-                               clusterID:clusterID
-                             attributeID:attributeID
-                                  params:params
-                                   queue:self.queue
-                              completion:^(NSArray<NSDictionary<NSString *, id> *> * _Nullable values, NSError * _Nullable error) {
-                                  if (values) {
-                                      // Since the format is the same data-value dictionary, this looks like an attribute
-                                      // report
-                                      MTR_LOG_INFO("%@ completion values %@", logPrefix, values);
-                                      [self _handleAttributeReport:values];
-                                  }
-
-                                  // TODO: better retry logic
-                                  if (error && (retryCount < 2)) {
-                                      MTR_LOG_ERROR(
-                                          "%@ completion error %@ retryWork %lu", logPrefix, error, (unsigned long) retryCount);
-                                      [workItem retryWork];
-                                  } else {
-                                      MTR_LOG_DEFAULT("%@ completion error %@ endWork", logPrefix, error);
-                                      [workItem endWork];
-                                  }
-                              }];
-    };
-    workItem.readyHandler = readyHandler;
-    MTR_LOG_DEFAULT("%@ enqueueWorkItem %@", logPrefix, _asyncCallbackWorkQueue);
-    [_asyncCallbackWorkQueue enqueueWorkItem:workItem];
-
-    // Return current known / expected value right away
     MTRAttributePath * attributePath = [MTRAttributePath attributePathWithEndpointID:endpointID
                                                                            clusterID:clusterID
                                                                          attributeID:attributeID];
+
+    BOOL attributeIsSpecified = MTRAttributeIsSpecified(clusterID.unsignedIntValue, attributeID.unsignedIntValue);
+    BOOL hasChangesOmittedQuality = AttributeHasChangesOmittedQuality(attributePath);
+
+    // Return current known / expected value right away
     NSDictionary<NSString *, id> * attributeValueToReturn = [self _attributeValueDictionaryForAttributePath:attributePath];
 
+    // Send read request to device if any of the following are true:
+    // 1. The attribute is not in the specification (so we don't know whether hasChangesOmittedQuality can be trusted).
+    // 2. Subscription not in a state we can expect reports
+    // 3. There is subscription but attribute has Changes Omitted quality
+    // 4. Cache has no entry
+    // TODO: add option for BaseSubscriptionCallback to report during priming, to reduce when case 4 is hit
+    if (!attributeIsSpecified || ![self _subscriptionAbleToReport] || hasChangesOmittedQuality || !attributeValueToReturn) {
+        // Create work item, set ready handler to perform task, then enqueue the work
+        MTRAsyncCallbackQueueWorkItem * workItem = [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:self.queue];
+        MTRAsyncCallbackReadyHandler readyHandler = ^(MTRDevice * device, NSUInteger retryCount) {
+            MTR_LOG_DEFAULT("%@ dequeueWorkItem %@", logPrefix, self->_asyncCallbackWorkQueue);
+            MTRBaseDevice * baseDevice = [self newBaseDevice];
+            [baseDevice readAttributesWithEndpointID:endpointID
+                                           clusterID:clusterID
+                                         attributeID:attributeID
+                                              params:params
+                                               queue:self.queue
+                                          completion:^(NSArray<NSDictionary<NSString *, id> *> * _Nullable values,
+                                              NSError * _Nullable error) {
+                                              if (values) {
+                                                  // Since the format is the same data-value dictionary, this looks like an
+                                                  // attribute report
+                                                  MTR_LOG_INFO("%@ completion values %@", logPrefix, values);
+                                                  [self _handleAttributeReport:values];
+                                              }
+
+                                              // TODO: better retry logic
+                                              if (error && (retryCount < 2)) {
+                                                  MTR_LOG_ERROR("%@ completion error %@ retryWork %lu", logPrefix, error,
+                                                      (unsigned long) retryCount);
+                                                  [workItem retryWork];
+                                              } else {
+                                                  MTR_LOG_DEFAULT("%@ completion error %@ endWork", logPrefix, error);
+                                                  [workItem endWork];
+                                              }
+                                          }];
+        };
+        workItem.readyHandler = readyHandler;
+        MTR_LOG_DEFAULT("%@ enqueueWorkItem %@", logPrefix, _asyncCallbackWorkQueue);
+        [_asyncCallbackWorkQueue enqueueWorkItem:workItem];
+    }
+
     return attributeValueToReturn;
 }
 

src/darwin/Framework/CHIP/templates/MTRAttributeSpecifiedCheck-src.zapt

@@ -0,0 +1,47 @@
+{{> header excludeZapComment=true}}
+
+#import "MTRAttributeSpecifiedCheck.h"
+
+#include <app-common/zap-generated/ids/Attributes.h>
+#include <app-common/zap-generated/ids/Clusters.h>
+
+using namespace chip;
+using namespace chip::app;
+
+{{#zcl_clusters}}
+{{#if (isSupported (asUpperCamelCase name preserveAcronyms=true))}}
+static BOOL AttributeIsSpecifiedIn{{asUpperCamelCase name preserveAcronyms=true}}Cluster(AttributeId aAttributeId)
+{
+    using namespace Clusters::{{asUpperCamelCase name}};
+    switch (aAttributeId) {
+        {{#zcl_attributes_server}}
+        {{#if (isSupported (asUpperCamelCase ../name preserveAcronyms=true) attribute=(asUpperCamelCase name preserveAcronyms=true))}}
+        case Attributes::{{asUpperCamelCase name}}::Id: {
+            return YES;
+        }
+        {{/if}}
+        {{/zcl_attributes_server}}
+        default: {
+            return NO;
+        }
+    }
+}
+{{/if}}
+{{/zcl_clusters}}
+
+BOOL MTRAttributeIsSpecified(ClusterId aClusterId, AttributeId aAttributeId)
+{
+    switch (aClusterId)
+    {
+        {{#zcl_clusters}}
+        {{#if (isSupported (asUpperCamelCase name preserveAcronyms=true))}}
+        case Clusters::{{asUpperCamelCase name}}::Id: {
+            return AttributeIsSpecifiedIn{{asUpperCamelCase name preserveAcronyms=true}}Cluster(aAttributeId);
+        }
+        {{/if}}
+        {{/zcl_clusters}}
+        default: {
+            return NO;
+        }
+    }
+}

src/darwin/Framework/CHIP/templates/templates.json

@@ -110,7 +110,7 @@
         },
         {
             "path": "MTRCommandPayloadsObjc-src.zapt",
-            "name": "Objc reflections of MTR command payloads header",
+            "name": "Objc reflections of MTR command payloads",
             "output": "src/darwin/Framework/CHIP/zap-generated/MTRCommandPayloadsObjc.mm"
         },
         {
@@ -132,6 +132,11 @@
             "path": "MTRClusterConstants.zapt",
             "name": "Constants for cluster IDs",
             "output": "src/darwin/Framework/CHIP/zap-generated/MTRClusterConstants.h"
+        },
+        {
+            "path": "MTRAttributeSpecifiedCheck-src.zapt",
+            "name": "Function to check if attribute is specified",
+            "output": "src/darwin/Framework/CHIP/zap-generated/MTRAttributeSpecifiedCheck.mm"
         }
     ]
 }