Skip to content

Commit

Permalink
[Darwin] MTRDevice should not always send reads to device directly (#…
Browse files Browse the repository at this point in the history 
…26776)

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

* Update src/darwin/Framework/CHIP/MTRDevice.mm

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

* Update src/darwin/Framework/CHIP/MTRDevice.mm

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

* Update src/darwin/Framework/CHIP/MTRDevice.mm

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

* Update src/darwin/Framework/CHIP/templates/MTRAttributeUtils-src.zapt

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

* Address PR review comments

---------

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
  • Loading branch information
@jtung-apple @bzbarsky-apple @pull
2 people authored and pull[bot] committed Aug 29, 2023
1 parent 04469c2 commit 2490272
Show file tree
Hide file tree
Showing 6 changed files with 5,236 additions and 36 deletions.
25 changes: 25 additions & 0 deletions src/darwin/Framework/CHIP/MTRAttributeSpecifiedCheck.h
View file
Original file line number Diff line number Diff line change
@@ -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);
217 changes: 182 additions & 35 deletions src/darwin/Framework/CHIP/MTRDevice.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#import <os/lock.h>

#import "MTRAsyncCallbackWorkQueue.h"
#import "MTRAttributeSpecifiedCheck.h"
#import "MTRBaseDevice_Internal.h"
#import "MTRBaseSubscriptionCallback.h"
#import "MTRCluster.h"
Expand Down Expand Up @@ -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
{
Expand Down Expand Up @@ -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;
}

Expand Down
47 changes: 47 additions & 0 deletions src/darwin/Framework/CHIP/templates/MTRAttributeSpecifiedCheck-src.zapt
View file
Original file line number Diff line number Diff line change
@@ -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;
}
}
}
7 changes: 6 additions & 1 deletion src/darwin/Framework/CHIP/templates/templates.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
},
{
Expand All @@ -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"
}
]
}
Loading

0 comments on commit 2490272

Please sign in to comment.