Skip to content

Commit

Permalink
Stop assuming all unknown attributes have the C quality in MTRDevice. (
Browse files Browse the repository at this point in the history
…#33009)

* Stop assuming all unknown attributes have the C quality in MTRDevice.

Assume things we don't know about don't have the C quality unless told to assume
otherwise.

Also fixes XPC detection check; non-XPC controllers also respond to the
"sharedControllerWithID:xpcConnectBlock:" selector.

* Address review comments and fix tests to work in the new setup.
  • Loading branch information
bzbarsky-apple authored and pull[bot] committed Apr 24, 2024
1 parent a530e30 commit 7975908
Show file tree
Hide file tree
Showing 8 changed files with 199 additions and 87 deletions.
11 changes: 11 additions & 0 deletions src/darwin/Framework/CHIP/MTRCluster.h
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,17 @@ MTR_AVAILABLE(ios(16.1), macos(13.0), watchos(9.1), tvos(16.1))
*/
@property (nonatomic, copy, nullable) NSNumber * minEventNumber MTR_AVAILABLE(ios(16.4), macos(13.3), watchos(9.4), tvos(16.4));

/**
* Controls whether attributes without known schema (e.g. vendor-specific
* attributes) should be assumed to be reportable normally via subscriptions.
* The default is YES.
*
* This setting is only relevant to some consumers of MTRReadParams. One of
* those consumers is readAttributeWithEndpointID:clusterID:attributeID:params:
* on MTRDevice.
*/
@property (nonatomic, assign, getter=shouldAssumeUnknownAttributesReportable) BOOL assumeUnknownAttributesReportable MTR_NEWLY_AVAILABLE;

@end

/**
Expand Down
3 changes: 3 additions & 0 deletions src/darwin/Framework/CHIP/MTRCluster.mm
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ - (instancetype)init
{
if (self = [super init]) {
_filterByFabric = YES;
_assumeUnknownAttributesReportable = YES;
}
return self;
}
Expand All @@ -93,6 +94,7 @@ - (id)copyWithZone:(NSZone * _Nullable)zone
auto other = [[MTRReadParams alloc] init];
other.filterByFabric = self.filterByFabric;
other.minEventNumber = self.minEventNumber;
other.assumeUnknownAttributesReportable = self.assumeUnknownAttributesReportable;
return other;
}

Expand Down Expand Up @@ -124,6 +126,7 @@ - (id)copyWithZone:(NSZone * _Nullable)zone
auto other = [[MTRSubscribeParams alloc] initWithMinInterval:self.minInterval maxInterval:self.maxInterval];
other.filterByFabric = self.filterByFabric;
other.minEventNumber = self.minEventNumber;
other.assumeUnknownAttributesReportable = self.assumeUnknownAttributesReportable;
other.replaceExistingSubscriptions = self.replaceExistingSubscriptions;
other.reportEventsUrgently = self.reportEventsUrgently;
other.resubscribeAutomatically = self.resubscribeAutomatically;
Expand Down
38 changes: 30 additions & 8 deletions src/darwin/Framework/CHIP/MTRDevice.mm
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,7 @@ - (void)unitTestReportEndForDevice:(MTRDevice *)device;
- (BOOL)unitTestShouldSetUpSubscriptionForDevice:(MTRDevice *)device;
- (BOOL)unitTestShouldSkipExpectedValuesForWrite:(MTRDevice *)device;
- (NSNumber *)unitTestMaxIntervalOverrideForSubscription:(MTRDevice *)device;
- (BOOL)unitTestForceAttributeReportsIfMatchingCache:(MTRDevice *)device;
@end
#endif

Expand Down Expand Up @@ -745,7 +746,7 @@ - (BOOL)_subscriptionAbleToReport

// Unfortunately, we currently have no subscriptions over our hacked-up XPC
// setup. Try to detect that situation.
if ([_deviceController.class respondsToSelector:@selector(sharedControllerWithID:xpcConnectBlock:)]) {
if ([_deviceController isKindOfClass:MTRDeviceControllerOverXPC.class]) {
return NO;
}

Expand Down Expand Up @@ -1603,24 +1604,33 @@ static BOOL AttributeHasChangesOmittedQuality(MTRAttributePath * attributePath)
- (NSDictionary<NSString *, id> * _Nullable)readAttributeWithEndpointID:(NSNumber *)endpointID
clusterID:(NSNumber *)clusterID
attributeID:(NSNumber *)attributeID
params:(MTRReadParams *)params
params:(MTRReadParams * _Nullable)params
{
MTRAttributePath * attributePath = [MTRAttributePath attributePathWithEndpointID:endpointID
clusterID:clusterID
attributeID:attributeID];

BOOL attributeIsSpecified = MTRAttributeIsSpecified(clusterID.unsignedIntValue, attributeID.unsignedIntValue);
BOOL hasChangesOmittedQuality = AttributeHasChangesOmittedQuality(attributePath);
BOOL hasChangesOmittedQuality;
if (attributeIsSpecified) {
hasChangesOmittedQuality = AttributeHasChangesOmittedQuality(attributePath);
} else {
if (params == nil) {
hasChangesOmittedQuality = NO;
} else {
hasChangesOmittedQuality = !params.assumeUnknownAttributesReportable;
}
}

// 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
// TODO: add option for BaseSubscriptionCallback to report during priming, to reduce when case 4 is hit
if (!attributeIsSpecified || ![self _subscriptionAbleToReport] || hasChangesOmittedQuality) {
// 1. Subscription not in a state we can expect reports
// 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) {
// 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.
Expand Down Expand Up @@ -2344,6 +2354,18 @@ - (NSArray *)_getAttributesToReportWithReportedValues:(NSArray<NSDictionary<NSSt
}
NSArray * expectedValue = _expectedValueCache[attributePath];

// Unit test only code.
#ifdef DEBUG
if (!readCacheValueChanged) {
id delegate = _weakDelegate.strongObject;
if (delegate) {
if ([delegate respondsToSelector:@selector(unitTestForceAttributeReportsIfMatchingCache:)]) {
readCacheValueChanged = [delegate unitTestForceAttributeReportsIfMatchingCache:self];
}
}
}
#endif // DEBUG

// Report the attribute if a read would get a changed value. This happens
// when our cached value changes and no expected value exists.
if (readCacheValueChanged && !expectedValue) {
Expand Down
7 changes: 6 additions & 1 deletion src/darwin/Framework/CHIPTests/MTRDeviceTests.m
Original file line number Diff line number Diff line change
Expand Up @@ -1518,6 +1518,10 @@ - (void)test017_TestMTRDeviceBasics
[self waitForExpectations:@[ onTimeWriteSuccess, onTimePreviousValue ] timeout:10];

// Test if errors are properly received
// TODO: We might stop reporting these altogether from MTRDevice, and then
// this test will need updating.
__auto_type * readThroughForUnknownAttributesParams = [[MTRReadParams alloc] init];
readThroughForUnknownAttributesParams.assumeUnknownAttributesReportable = NO;
XCTestExpectation * attributeReportErrorExpectation = [self expectationWithDescription:@"Attribute read error"];
delegate.onAttributeDataReceived = ^(NSArray<NSDictionary<NSString *, id> *> * data) {
for (NSDictionary<NSString *, id> * attributeReponseValue in data) {
Expand All @@ -1526,8 +1530,9 @@ - (void)test017_TestMTRDeviceBasics
}
}
};

// use the nonexistent attribute and expect read error
[device readAttributeWithEndpointID:testEndpointID clusterID:testClusterID attributeID:testAttributeID params:nil];
[device readAttributeWithEndpointID:testEndpointID clusterID:testClusterID attributeID:testAttributeID params:readThroughForUnknownAttributesParams];
[self waitForExpectations:@[ attributeReportErrorExpectation ] timeout:10];

// Resubscription test setup
Expand Down
Loading

0 comments on commit 7975908

Please sign in to comment.