Skip to content

Commit

Permalink
Apply API review fixes for MTRReadParams and MTRSubscribeParams. (#23491
Browse files Browse the repository at this point in the history 
)

This is a re-landing of PR #22592 but modified to preserve the old APIs.

* Move minInterval and maxInterval into MTRSubscribeParams.

* Make the booleans in MTRSubsribeParams and MTRReadParams just BOOL (inited to
  the default) instead of "NSNumber with nil meaning default".

The header changes not accompanied by backwards-compat shims are OK for the
following reasons:

* In MTRBaseDevice, the changes are to MTR_NEWLY_AVAILABLE APIs.
* The changes to MTRDeviceController+XPC.h are source+binary compatible.
* MTRDeviceOverXPC.h is not a public header.
* In MTRBaseClusters.h, the changes are to MTR_NEWLY_AVAILABLE APIs.
  • Loading branch information
@bzbarsky-apple @pull
bzbarsky-apple authored and pull[bot] committed Jul 11, 2023
1 parent 4c60b2c commit 8dd95b6
Show file tree
Hide file tree
Showing 21 changed files with 34,183 additions and 36,732 deletions.
33 changes: 20 additions & 13 deletions examples/darwin-framework-tool/commands/clusters/ReportCommandBridge.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,9 @@ class ReadAttribute : public ModelCommand {
{
dispatch_queue_t callbackQueue = dispatch_queue_create("com.chip.command", DISPATCH_QUEUE_SERIAL);
MTRReadParams * params = [[MTRReadParams alloc] init];
params.fabricFiltered = mFabricFiltered.HasValue() ? [NSNumber numberWithBool:mFabricFiltered.Value()] : nil;
if (mFabricFiltered.HasValue()) {
params.filterByFabric = mFabricFiltered.Value();
}
[device
readAttributesWithEndpointID:[NSNumber numberWithUnsignedShort:endpointId]
clusterID:[NSNumber numberWithUnsignedInteger:mClusterId]
Expand Down Expand Up @@ -124,16 +126,20 @@ class SubscribeAttribute : public ModelCommand {
{
dispatch_queue_t callbackQueue = dispatch_queue_create("com.chip.command", DISPATCH_QUEUE_SERIAL);

MTRSubscribeParams * params = [[MTRSubscribeParams alloc] init];
params.keepPreviousSubscriptions
= mKeepSubscriptions.HasValue() ? [NSNumber numberWithBool:mKeepSubscriptions.Value()] : nil;
params.autoResubscribe = mAutoResubscribe.HasValue() ? [NSNumber numberWithBool:mAutoResubscribe.Value()] : nil;
MTRSubscribeParams * params = [[MTRSubscribeParams alloc] initWithMinInterval:@(mMinInterval) maxInterval:@(mMaxInterval)];
if (mFabricFiltered.HasValue()) {
params.filterByFabric = mFabricFiltered.Value();
}
if (mKeepSubscriptions.HasValue()) {
params.replaceExistingSubscriptions = !mKeepSubscriptions.Value();
}
if (mAutoResubscribe.HasValue()) {
params.resubscribeIfLost = mAutoResubscribe.Value();
}

[device subscribeToAttributesWithEndpointID:[NSNumber numberWithUnsignedShort:endpointId]
clusterID:[NSNumber numberWithUnsignedInteger:mClusterId]
attributeID:[NSNumber numberWithUnsignedInteger:mAttributeId]
minInterval:[NSNumber numberWithUnsignedInteger:mMinInterval]
maxInterval:[NSNumber numberWithUnsignedInteger:mMaxInterval]
params:params
queue:callbackQueue
reportHandler:^(NSArray<NSDictionary<NSString *, id> *> * _Nullable values, NSError * _Nullable error) {
Expand Down Expand Up @@ -192,14 +198,15 @@ class SubscribeEvent : public ModelCommand {
{
dispatch_queue_t callbackQueue = dispatch_queue_create("com.chip.command", DISPATCH_QUEUE_SERIAL);

MTRSubscribeParams * params = [[MTRSubscribeParams alloc] init];
params.keepPreviousSubscriptions
= mKeepSubscriptions.HasValue() ? [NSNumber numberWithBool:mKeepSubscriptions.Value()] : nil;
params.autoResubscribe = mAutoResubscribe.HasValue() ? [NSNumber numberWithBool:mAutoResubscribe.Value()] : nil;
MTRSubscribeParams * params = [[MTRSubscribeParams alloc] initWithMinInterval:@(mMinInterval) maxInterval:@(mMaxInterval)];
if (mKeepSubscriptions.HasValue()) {
params.replaceExistingSubscriptions = !mKeepSubscriptions.Value();
}
if (mAutoResubscribe.HasValue()) {
params.resubscribeIfLost = mAutoResubscribe.Value();
}

[device subscribeWithQueue:callbackQueue
minInterval:@(mMinInterval)
maxInterval:@(mMaxInterval)
params:params
attributeCacheContainer:nil
attributeReportHandler:^(NSArray * value) {
Expand Down
20 changes: 12 additions & 8 deletions examples/darwin-framework-tool/templates/commands.zapt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ private:
{{/chip_cluster_commands}}

{{#chip_server_cluster_attributes}}
{{#*inline "attribute"}}Attribute{{#if (isStrEqual (asUpperCamelCase parent.name) "Descriptor")}}{{#if (isStrEqual (asUpperCamelCase name) "DeviceTypeList")}}DeviceList{{else}}{{asUpperCamelCase name}}{{/if}}{{else}}{{asUpperCamelCase name}}{{/if}}{{/inline}}
{{#*inline "attribute"}}Attribute{{asUpperCamelCase name}}{{/inline}}

/*
* Attribute {{asUpperCamelCase name}}
Expand All @@ -114,7 +114,9 @@ public:
MTRBaseCluster{{asUpperCamelCase parent.name}} * cluster = [[MTRBaseCluster{{asUpperCamelCase parent.name}} alloc] initWithDevice:device endpointID:@(endpointId) queue:callbackQueue];
{{#if_is_fabric_scoped_struct type}}
MTRReadParams * params = [[MTRReadParams alloc] init];
params.fabricFiltered = mFabricFiltered.HasValue() ? [NSNumber numberWithBool:mFabricFiltered.Value()] : nil;
if (mFabricFiltered.HasValue()) {
params.filterByFabric = mFabricFiltered.Value();
}
{{/if_is_fabric_scoped_struct}}
[cluster readAttribute{{asUpperCamelCase name}}With
{{~#if_is_fabric_scoped_struct type~}}
Expand Down Expand Up @@ -216,12 +218,14 @@ public:
ChipLogProgress(chipTool, "Sending cluster ({{asHex parent.code 8}}) ReportAttribute ({{asHex code 8}}) on endpoint %u", endpointId);
dispatch_queue_t callbackQueue = dispatch_queue_create("com.chip.command", DISPATCH_QUEUE_SERIAL);
MTRBaseCluster{{asUpperCamelCase parent.name}} * cluster = [[MTRBaseCluster{{asUpperCamelCase parent.name}} alloc] initWithDevice:device endpointID:@(endpointId) queue:callbackQueue];
MTRSubscribeParams * params = [[MTRSubscribeParams alloc] init];
params.keepPreviousSubscriptions = mKeepSubscriptions.HasValue() ? [NSNumber numberWithBool:mKeepSubscriptions.Value()] : nil;
params.fabricFiltered = mFabricFiltered.HasValue() ? [NSNumber numberWithBool:mFabricFiltered.Value()] : nil;
[cluster subscribe{{>attribute}}WithMinInterval:[NSNumber numberWithUnsignedInt:mMinInterval]
maxInterval:[NSNumber numberWithUnsignedInt:mMaxInterval]
params:params
MTRSubscribeParams * params = [[MTRSubscribeParams alloc] initWithMinInterval:@(mMinInterval) maxInterval:@(mMaxInterval)];
if (mKeepSubscriptions.HasValue()) {
params.replaceExistingSubscriptions = !mKeepSubscriptions.Value();
}
if (mFabricFiltered.HasValue()) {
params.filterByFabric = mFabricFiltered.Value();
}
[cluster subscribe{{>attribute}}WithParams:params
subscriptionEstablished:^(){ mSubscriptionEstablished=YES; }
reportHandler:^({{asObjectiveCClass type parent.name}} * _Nullable value, NSError * _Nullable error) {
NSLog(@"{{asUpperCamelCase parent.name}}.{{asUpperCamelCase name}} response %@", [value description]);
Expand Down
8 changes: 3 additions & 5 deletions examples/darwin-framework-tool/templates/tests/partials/test_cluster.zapt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,10 +154,8 @@ class {{filename}}: public TestCommandBridge
{{#chip_tests_item_parameters}}
{{asObjectiveCBasicType type}} {{asLowerCamelCase name}}Argument = {{asTypedLiteral definedValue type}};
{{/chip_tests_item_parameters}}
MTRSubscribeParams * params = [[MTRSubscribeParams alloc] init];
[cluster subscribeAttribute{{asUpperCamelCase attribute}}WithMinInterval:[NSNumber numberWithUnsignedInt:minIntervalArgument]
maxInterval:[NSNumber numberWithUnsignedInt:maxIntervalArgument]
params:params
MTRSubscribeParams * params = [[MTRSubscribeParams alloc] initWithMinInterval:@(minIntervalArgument) maxInterval:@(maxIntervalArgument)];
[cluster subscribeAttribute{{asUpperCamelCase attribute}}WithParams:params
subscriptionEstablished:^{
VerifyOrReturn(testSendCluster{{parent.filename}}_{{waitForReport.index}}_{{asUpperCamelCase waitForReport.command}}_Fulfilled, SetCommandExitStatus(CHIP_ERROR_INCORRECT_STATE));
NextTest();
Expand All @@ -168,7 +166,7 @@ class {{filename}}: public TestCommandBridge
{{else if isReadAttribute}}
{{#if_is_fabric_scoped_struct attributeObject.type}}
MTRReadParams * params = [[MTRReadParams alloc] init];
params.fabricFiltered = [NSNumber numberWithBool:{{fabricFiltered}}];
params.filterByFabric = {{fabricFiltered}};
{{/if_is_fabric_scoped_struct}}
[cluster readAttribute{{asUpperCamelCase attribute}}With
{{~#if_is_fabric_scoped_struct attributeObject.type~}}
Expand Down
3 changes: 0 additions & 3 deletions scripts/tests/chiptest/lsan-mac-suppressions.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,6 @@ leak:drbg_bytes
# TODO: OpenSSL ERR_get_state seems to leak.
leak:ERR_get_state

# TODO: https://github.com/project-chip/connectedhomeip/issues/22333
leak:[MTRBaseCluster* subscribeAttribute*WithMinInterval:maxInterval:params:subscriptionEstablished:reportHandler:]

#TODO: Figure out why nw_path_monitor_create leaks. The leak can be reproduced using:
# -- testFile.cpp
#
Expand Down
8 changes: 2 additions & 6 deletions src/darwin/Framework/CHIP/MTRBaseDevice.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,9 +165,7 @@ extern NSString * const MTRArrayValueType;
* attempts fail.
*/
- (void)subscribeWithQueue:(dispatch_queue_t)queue
minInterval:(NSNumber *)minInterval
maxInterval:(NSNumber *)maxInterval
params:(MTRSubscribeParams * _Nullable)params
params:(MTRSubscribeParams *)params
attributeCacheContainer:(MTRAttributeCacheContainer * _Nullable)attributeCacheContainer
attributeReportHandler:(MTRDeviceReportHandler _Nullable)attributeReportHandler
eventReportHandler:(MTRDeviceReportHandler _Nullable)eventReportHandler
Expand Down Expand Up @@ -258,8 +256,6 @@ extern NSString * const MTRArrayValueType;
- (void)subscribeToAttributesWithEndpointID:(NSNumber * _Nullable)endpointID
clusterID:(NSNumber * _Nullable)clusterID
attributeID:(NSNumber * _Nullable)attributeID
minInterval:(NSNumber *)minInterval
maxInterval:(NSNumber *)maxInterval
params:(MTRSubscribeParams * _Nullable)params
queue:(dispatch_queue_t)queue
reportHandler:(MTRDeviceResponseHandler)reportHandler
Expand Down Expand Up @@ -439,7 +435,7 @@ MTR_NEWLY_AVAILABLE
reportHandler:(MTRDeviceResponseHandler)reportHandler
subscriptionEstablished:(dispatch_block_t _Nullable)subscriptionEstablishedHandler
MTR_NEWLY_DEPRECATED("Please use "
"subscribeToAttributesWithEndpointID:clusterID:attributeID:params:minInterval:maxInterval:queue:"
"subscribeToAttributesWithEndpointID:clusterID:attributeID:params:queue:"
"reportHandler:subscriptionEstablished:");

- (void)deregisterReportHandlersWithClientQueue:(dispatch_queue_t)queue
Expand Down
53 changes: 28 additions & 25 deletions src/darwin/Framework/CHIP/MTRBaseDevice.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -260,9 +260,7 @@ - (void)invalidateCASESession
} // anonymous namespace

- (void)subscribeWithQueue:(dispatch_queue_t)queue
minInterval:(NSNumber *)minInterval
maxInterval:(NSNumber *)maxInterval
params:(MTRSubscribeParams * _Nullable)params
params:(MTRSubscribeParams *)params
attributeCacheContainer:(MTRAttributeCacheContainer * _Nullable)attributeCacheContainer
attributeReportHandler:(MTRDeviceReportHandler _Nullable)attributeReportHandler
eventReportHandler:(MTRDeviceReportHandler _Nullable)eventReportHandler
Expand Down Expand Up @@ -297,13 +295,14 @@ - (void)subscribeWithQueue:(dispatch_queue_t)queue
// We want to get event reports at the minInterval, not the maxInterval.
eventPath->mIsUrgentEvent = true;
ReadPrepareParams readParams(session.Value());
readParams.mMinIntervalFloorSeconds = [minInterval unsignedShortValue];
readParams.mMaxIntervalCeilingSeconds = [maxInterval unsignedShortValue];
readParams.mMinIntervalFloorSeconds = [params.minInterval unsignedShortValue];
readParams.mMaxIntervalCeilingSeconds = [params.maxInterval unsignedShortValue];
readParams.mpAttributePathParamsList = attributePath.get();
readParams.mAttributePathParamsListSize = 1;
readParams.mpEventPathParamsList = eventPath.get();
readParams.mEventPathParamsListSize = 1;
readParams.mKeepSubscriptions = [params.keepPreviousSubscriptions boolValue];
readParams.mIsFabricFiltered = params.filterByFabric;
readParams.mKeepSubscriptions = !params.replaceExistingSubscriptions;

std::unique_ptr<ClusterStateCache> attributeCache;
ReadClient::Callback * callbackForReadClient = nullptr;
Expand Down Expand Up @@ -368,7 +367,7 @@ - (void)subscribeWithQueue:(dispatch_queue_t)queue
exchangeManager, *callbackForReadClient, ReadClient::InteractionType::Subscribe);

CHIP_ERROR err;
if (params != nil && params.autoResubscribe != nil && ![params.autoResubscribe boolValue]) {
if (!params.resubscribeIfLost) {
err = readClient->SendRequest(readParams);
} else {
// SendAutoResubscribeRequest cleans up the params, even on failure.
Expand Down Expand Up @@ -834,7 +833,7 @@ new MTRDataValueDictionaryCallbackBridge(queue, self, completion,
chip::app::ReadPrepareParams readParams(session);
readParams.mpAttributePathParamsList = &attributePath;
readParams.mAttributePathParamsListSize = 1;
readParams.mIsFabricFiltered = params == nil || params.fabricFiltered == nil || [params.fabricFiltered boolValue];
readParams.mIsFabricFiltered = params.filterByFabric;

auto onDone = [resultArray, resultSuccess, resultFailure, context, successCb, failureCb](
BufferedReadAttributeCallback<MTRDataValueDictionaryDecodableType> * callback) {
Expand Down Expand Up @@ -1120,8 +1119,6 @@ new MTRDataValueDictionaryCallbackBridge(queue, self, completion,
- (void)subscribeToAttributesWithEndpointID:(NSNumber * _Nullable)endpointID
clusterID:(NSNumber * _Nullable)clusterID
attributeID:(NSNumber * _Nullable)attributeID
minInterval:(NSNumber *)minInterval
maxInterval:(NSNumber *)maxInterval
params:(MTRSubscribeParams * _Nullable)params
queue:(dispatch_queue_t)queue
reportHandler:(MTRDeviceResponseHandler)reportHandler
Expand All @@ -1139,8 +1136,6 @@ - (void)subscribeToAttributesWithEndpointID:(NSNumber * _Nullable)endpointID
endpointID = (endpointID == nil) ? nil : [endpointID copy];
clusterID = (clusterID == nil) ? nil : [clusterID copy];
attributeID = (attributeID == nil) ? nil : [attributeID copy];
minInterval = (minInterval == nil) ? nil : [minInterval copy];
maxInterval = (maxInterval == nil) ? nil : [maxInterval copy];
params = (params == nil) ? nil : [params copy];

[self.deviceController
Expand Down Expand Up @@ -1214,12 +1209,10 @@ - (void)subscribeToAttributesWithEndpointID:(NSNumber * _Nullable)endpointID
chip::app::ReadPrepareParams readParams(session.Value());
readParams.mpAttributePathParamsList = container.pathParams;
readParams.mAttributePathParamsListSize = 1;
readParams.mMinIntervalFloorSeconds = static_cast<uint16_t>([minInterval unsignedShortValue]);
readParams.mMaxIntervalCeilingSeconds = static_cast<uint16_t>([maxInterval unsignedShortValue]);
readParams.mIsFabricFiltered
= (params == nil || params.fabricFiltered == nil || [params.fabricFiltered boolValue]);
readParams.mKeepSubscriptions
= (params != nil && params.keepPreviousSubscriptions != nil && [params.keepPreviousSubscriptions boolValue]);
readParams.mMinIntervalFloorSeconds = static_cast<uint16_t>([params.minInterval unsignedShortValue]);
readParams.mMaxIntervalCeilingSeconds = static_cast<uint16_t>([params.maxInterval unsignedShortValue]);
readParams.mIsFabricFiltered = params.filterByFabric;
readParams.mKeepSubscriptions = !params.replaceExistingSubscriptions;

auto onDone = [container](BufferedReadAttributeCallback<MTRDataValueDictionaryDecodableType> * callback) {
[container onDone];
Expand All @@ -1235,7 +1228,7 @@ - (void)subscribeToAttributesWithEndpointID:(NSNumber * _Nullable)endpointID
auto readClient = Platform::New<app::ReadClient>(
engine, exchangeManager, callback->GetBufferedCallback(), chip::app::ReadClient::InteractionType::Subscribe);

if (params != nil && params.autoResubscribe != nil && ![params.autoResubscribe boolValue]) {
if (params.replaceExistingSubscriptions) {
err = readClient->SendRequest(readParams);
} else {
err = readClient->SendAutoResubscribeRequest(std::move(readParams));
Expand Down Expand Up @@ -1469,10 +1462,15 @@ - (void)subscribeWithQueue:(dispatch_queue_t)queue
subscriptionEstablished:(dispatch_block_t _Nullable)subscriptionEstablishedHandler
resubscriptionScheduled:(MTRDeviceResubscriptionScheduledHandler _Nullable)resubscriptionScheduledHandler
{
MTRSubscribeParams * _Nullable subscribeParams = [params copy];
if (subscribeParams == nil) {
subscribeParams = [[MTRSubscribeParams alloc] initWithMinInterval:@(minInterval) maxInterval:@(maxInterval)];
} else {
subscribeParams.minInterval = @(minInterval);
subscribeParams.maxInterval = @(maxInterval);
}
[self subscribeWithQueue:queue
minInterval:@(minInterval)
maxInterval:@(maxInterval)
params:params
params:subscribeParams
attributeCacheContainer:attributeCacheContainer
attributeReportHandler:attributeReportHandler
eventReportHandler:eventReportHandler
Expand Down Expand Up @@ -1540,12 +1538,17 @@ - (void)subscribeAttributeWithEndpointId:(NSNumber * _Nullable)endpointId
reportHandler:(MTRDeviceResponseHandler)reportHandler
subscriptionEstablished:(dispatch_block_t _Nullable)subscriptionEstablishedHandler
{
MTRSubscribeParams * _Nullable subscribeParams = [params copy];
if (subscribeParams == nil) {
subscribeParams = [[MTRSubscribeParams alloc] initWithMinInterval:minInterval maxInterval:maxInterval];
} else {
subscribeParams.minInterval = minInterval;
subscribeParams.maxInterval = maxInterval;
}
[self subscribeToAttributesWithEndpointID:endpointId
clusterID:clusterId
attributeID:attributeId
minInterval:minInterval
maxInterval:maxInterval
params:params
params:subscribeParams
queue:clientQueue
reportHandler:reportHandler
subscriptionEstablished:subscriptionEstablishedHandler];
Expand Down
Loading

0 comments on commit 8dd95b6

Please sign in to comment.