Skip to content

Commit

Permalink
Add a way for Darwin invokes to specify a server-side processing time…
Browse files Browse the repository at this point in the history
…out.

The default 2s timeout is way too low for some commands (like ScanNetworks).

Fixes project-chip#24333
  • Loading branch information
bzbarsky-apple committed Jan 20, 2023
1 parent f683392 commit 305f9c5
Show file tree
Hide file tree
Showing 10 changed files with 6,615 additions and 1,126 deletions.
16 changes: 14 additions & 2 deletions src/darwin/Framework/CHIP/MTRBaseClusterUtils.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include <lib/core/DataModelTypes.h>
#include <lib/core/TLV.h>
#include <lib/support/CHIPMem.h>
#include <system/SystemClock.h>

NS_ASSUME_NONNULL_BEGIN

Expand Down Expand Up @@ -378,11 +379,18 @@ template <typename InvokeBridgeType, typename ResponseType> class MTRInvokeCallb
bool mCalledCallback = false;
};

/**
* timedInvokeTimeoutMs, if provided, is how long the server will wait for us to
* send the invoke after we sent the Timed Request message.
*
* invokeTimeout, if provided, will have possible MRP latency added to it and
* the result is how long we will wait for the server to respond.
*/
template <typename BridgeType, typename RequestDataType>
CHIP_ERROR MTRStartInvokeInteraction(BridgeType * _Nonnull bridge, const RequestDataType & requestData,
chip::Messaging::ExchangeManager & exchangeManager, const chip::SessionHandle & session,
typename BridgeType::SuccessCallbackType successCb, MTRErrorCallback failureCb, chip::EndpointId endpoint,
chip::Optional<uint16_t> timedInvokeTimeoutMs)
chip::Optional<uint16_t> timedInvokeTimeoutMs, chip::Optional<chip::System::Clock::Timeout> invokeTimeout)
{
auto callback = chip::Platform::MakeUnique<MTRInvokeCallback<BridgeType, typename RequestDataType::ResponseType>>(
bridge, successCb, failureCb);
Expand All @@ -395,7 +403,11 @@ CHIP_ERROR MTRStartInvokeInteraction(BridgeType * _Nonnull bridge, const Request
chip::app::CommandPathParams commandPath(endpoint, 0, RequestDataType::GetClusterId(), RequestDataType::GetCommandId(),
chip::app::CommandPathFlags::kEndpointIdValid);
ReturnErrorOnFailure(commandSender->AddRequestData(commandPath, requestData, timedInvokeTimeoutMs));
ReturnErrorOnFailure(commandSender->SendCommandRequest(session));

if (invokeTimeout.HasValue()) {
invokeTimeout.SetValue(session->ComputeRoundTripTimeout(invokeTimeout.Value()));
}
ReturnErrorOnFailure(commandSender->SendCommandRequest(session, invokeTimeout));

callback->AdoptCommandSender(std::move(commandSender));
callback.release();
Expand Down
8 changes: 7 additions & 1 deletion src/darwin/Framework/CHIP/MTRBaseDevice.mm
Original file line number Diff line number Diff line change
Expand Up @@ -1132,7 +1132,13 @@ - (void)invokeCommandWithEndpointID:(NSNumber *)endpointID

ReturnErrorOnFailure(commandSender->AddRequestData(commandPath, MTRDataValueDictionaryDecodableType(commandFields),
(timeoutMs == nil) ? NullOptional : Optional<uint16_t>([timeoutMs unsignedShortValue])));
ReturnErrorOnFailure(commandSender->SendCommandRequest(session));

Optional<System::Clock::Timeout> invokeTimeout;
NSNumber * serverSideProcessingTimeoutMs = [commandFields serverSideProcessingTimeoutMs];
if (serverSideProcessingTimeoutMs != nil) {
invokeTimeout.SetValue(System::Clock::Timeout(serverSideProcessingTimeoutMs.unsignedLongValue));
}
ReturnErrorOnFailure(commandSender->SendCommandRequest(session, invokeTimeout));

decoder.release();
commandSender.release();
Expand Down
14 changes: 12 additions & 2 deletions src/darwin/Framework/CHIP/templates/MTRBaseClusters-src.zapt
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#import "MTRCluster_Internal.h"
#import "MTRClusterStateCacheContainer_Internal.h"
#import "MTRCommandPayloadsObjc.h"
#import "MTRDevice_Internal.h"
#import "MTRStructsObjc.h"

#include <lib/support/CHIPListUtils.h>
Expand All @@ -20,6 +21,8 @@ using chip::Callback::Cancelable;
using namespace chip::app::Clusters;
using chip::Messaging::ExchangeManager;
using chip::SessionHandle;
using chip::Optional;
using chip::System::Clock::Timeout;

// NOLINTBEGIN(clang-analyzer-cplusplus.NewDeleteLeaks): Linter is unable to locate the delete on these objects.
{{#chip_client_clusters includeAll=true}}
Expand Down Expand Up @@ -80,13 +83,20 @@ MTR{{cluster}}Cluster{{command}}Params
{{/if}}
^(ExchangeManager & exchangeManager, const SessionHandle & session, {{>callbackName}}CallbackType successCb, MTRErrorCallback failureCb, MTRCallbackBridgeBase * bridge) {
auto * typedBridge = static_cast<MTR{{>callbackName}}CallbackBridge *>(bridge);
chip::Optional<uint16_t> timedInvokeTimeoutMs;
Optional<uint16_t> timedInvokeTimeoutMs;
Optional<Timeout> invokeTimeout;
ListFreer listFreer;
{{asUpperCamelCase parent.name}}::Commands::{{asUpperCamelCase name}}::Type request;
if (params != nil) {
if (params.timedInvokeTimeoutMs != nil) {
params.timedInvokeTimeoutMs = MTRClampedNumber(params.timedInvokeTimeoutMs, @(1), @(UINT16_MAX));
timedInvokeTimeoutMs.SetValue(params.timedInvokeTimeoutMs.unsignedShortValue);
}
if (params.serverSideProcessingTimeoutMs != nil) {
// Clamp to UINT32_MAX/2 to leave room to adjust for MRP later without overflowing.
params.serverSideProcessingTimeoutMs = MTRClampedNumber(params.serverSideProcessingTimeoutMs, @(0), @(UINT32_MAX/2));
invokeTimeout.SetValue(Timeout(params.serverSideProcessingTimeoutMs.unsignedLongValue));
}
}
{{#if mustUseTimedInvoke}}
if (!timedInvokeTimeoutMs.HasValue()) {
Expand All @@ -107,7 +117,7 @@ MTR{{cluster}}Cluster{{command}}Params
{{/last}}
{{/chip_cluster_command_arguments}}

return MTRStartInvokeInteraction(typedBridge, request, exchangeManager, session, successCb, failureCb, self->_endpoint, timedInvokeTimeoutMs);
return MTRStartInvokeInteraction(typedBridge, request, exchangeManager, session, successCb, failureCb, self->_endpoint, timedInvokeTimeoutMs, invokeTimeout);
});
std::move(*bridge).DispatchAction(self.device);
}
Expand Down
17 changes: 14 additions & 3 deletions src/darwin/Framework/CHIP/templates/MTRClusters-src.zapt
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#import <Foundation/Foundation.h>

#import "MTRAsyncCallbackWorkQueue.h"
#import "MTRBaseClusterUtils.h"
#import "MTRBaseDevice_Internal.h"
#import "MTRClusterConstants.h"
#import "MTRClusters_Internal.h"
Expand All @@ -22,6 +23,8 @@ using chip::Callback::Cancelable;
using namespace chip::app::Clusters;
using chip::Messaging::ExchangeManager;
using chip::SessionHandle;
using chip::Optional;
using chip::System::Clock::Timeout;

static void MTRClustersLogEnqueue(NSString *logPrefix, MTRAsyncCallbackWorkQueue *workQueue) {
MTR_LOG_INFO("%@ enqueueWorkItem %@", logPrefix, workQueue);
Expand Down Expand Up @@ -120,12 +123,21 @@ MTRCommandIDTypeCluster{{cluster}}Command{{command}}ID
[workItem endWork];
},
^(ExchangeManager & exchangeManager, const SessionHandle & session, {{>callbackName}}CallbackType successCb, MTRErrorCallback failureCb, MTRCallbackBridgeBase * bridge) {
chip::Optional<uint16_t> timedInvokeTimeoutMs;
auto * typedBridge = static_cast<MTR{{>callbackName}}CallbackBridge *>(bridge);
Optional<uint16_t> timedInvokeTimeoutMs;
Optional<Timeout> invokeTimeout;
ListFreer listFreer;
{{asUpperCamelCase parent.name}}::Commands::{{asUpperCamelCase name}}::Type request;
if (timedInvokeTimeoutMsParam != nil) {
timedInvokeTimeoutMs.SetValue(timedInvokeTimeoutMsParam.unsignedShortValue);
}
if (params != nil) {
if (params.serverSideProcessingTimeoutMs != nil) {
// Clamp to UINT32_MAX/2 to leave room to adjust for MRP later without overflowing.
params.serverSideProcessingTimeoutMs = MTRClampedNumber(params.serverSideProcessingTimeoutMs, @(0), @(UINT32_MAX/2));
invokeTimeout.SetValue(Timeout(params.serverSideProcessingTimeoutMs.unsignedLongValue));
}
}
{{#if mustUseTimedInvoke}}
if (!timedInvokeTimeoutMs.HasValue()) {
timedInvokeTimeoutMs.SetValue(10000);
Expand All @@ -145,8 +157,7 @@ MTRCommandIDTypeCluster{{cluster}}Command{{command}}ID
{{/last}}
{{/chip_cluster_command_arguments}}

chip::Controller::{{asUpperCamelCase parent.name}}Cluster cppCluster(exchangeManager, session, self->_endpoint);
return cppCluster.InvokeCommand(request, bridge, successCb, failureCb, timedInvokeTimeoutMs);
return MTRStartInvokeInteraction(typedBridge, request, exchangeManager, session, successCb, failureCb, self->_endpoint, timedInvokeTimeoutMs, invokeTimeout);
});
std::move(*bridge).DispatchAction(baseDevice);
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ NS_ASSUME_NONNULL_BEGIN
{{#if (or (isStrEqual source "client")
(wasIntroducedBeforeRelease "First major API revamp" (compatClusterNameRemapping parent.name) command=(compatCommandNameRemapping parent.name name)))}}
_timedInvokeTimeoutMs = nil;
{{/if}}
{{#if (isStrEqual source "client")}}
_serverSideProcessingTimeoutMs = nil;
{{/if}}
}
return self;
Expand All @@ -35,6 +38,9 @@ NS_ASSUME_NONNULL_BEGIN
(wasIntroducedBeforeRelease "First major API revamp" (compatClusterNameRemapping parent.name) command=(compatCommandNameRemapping parent.name name)))}}
other.timedInvokeTimeoutMs = self.timedInvokeTimeoutMs;
{{/if}}
{{#if (isStrEqual source "client")}}
other.serverSideProcessingTimeoutMs = self.serverSideProcessingTimeoutMs;
{{/if}}

return other;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,15 @@ NS_ASSUME_NONNULL_BEGIN
*
*/
@property (nonatomic, copy, nullable) NSNumber * timedInvokeTimeoutMs;

/**
* Controls how much time, in milliseconds, we will allow for the server to process the command.
*
* The command will then time out if that much time, plus an allowance for retransmits due to network failures, passes.
*
* If nil, the framework will try to select an appropriate timeout value itself.
*/
@property (nonatomic, copy, nullable) NSNumber * serverSideProcessingTimeoutMs;
{{! This is using the pre-renaming names for the isAvailableBefore test, because the pre-rename things inherit
from the post-rename ones and need to have this selector.}}
{{else if (wasIntroducedBeforeRelease "First major API revamp" (compatClusterNameRemapping parent.name) command=(compatCommandNameRemapping parent.name name))}}
Expand Down
Loading

0 comments on commit 305f9c5

Please sign in to comment.