Skip to content

Commit

Permalink
Refactor CommandSender newly added APIs to be more extendable (projec…
Browse files Browse the repository at this point in the history 
…t-chip#31837)
  • Loading branch information
@tehampson @erwinpan1
tehampson authored and erwinpan1 committed Feb 12, 2024
1 parent eaa16be commit d308a50
Show file tree
Hide file tree
Showing 5 changed files with 198 additions and 114 deletions.
45 changes: 27 additions & 18 deletions src/app/CommandSender.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,8 @@ CHIP_ERROR CommandSender::SetCommandSenderConfig(CommandSender::ConfigParameters
#endif
}

CHIP_ERROR CommandSender::PrepareCommand(const CommandPathParams & aCommandPathParams, AdditionalCommandParameters & aOptionalArgs)
CHIP_ERROR CommandSender::PrepareCommand(const CommandPathParams & aCommandPathParams,
PrepareCommandParameters & aPrepareCommandParams)
{
ReturnErrorOnFailure(AllocateBuffer());

Expand All @@ -454,10 +455,10 @@ CHIP_ERROR CommandSender::PrepareCommand(const CommandPathParams & aCommandPathP
VerifyOrReturnError(mState == State::Idle || canAddAnotherCommand, CHIP_ERROR_INCORRECT_STATE);
VerifyOrReturnError(mFinishedCommandCount < mRemoteMaxPathsPerInvoke, CHIP_ERROR_MAXIMUM_PATHS_PER_INVOKE_EXCEEDED);

VerifyOrReturnError(!aOptionalArgs.commandRef.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
if (mBatchCommandsEnabled)
{
aOptionalArgs.commandRef.SetValue(mFinishedCommandCount);
VerifyOrReturnError(aPrepareCommandParams.commandRef.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(aPrepareCommandParams.commandRef.Value() == mFinishedCommandCount, CHIP_ERROR_INVALID_ARGUMENT);
}

InvokeRequests::Builder & invokeRequests = mInvokeRequestBuilder.GetInvokeRequests();
Expand All @@ -467,7 +468,7 @@ CHIP_ERROR CommandSender::PrepareCommand(const CommandPathParams & aCommandPathP
ReturnErrorOnFailure(invokeRequest.GetError());
ReturnErrorOnFailure(path.Encode(aCommandPathParams));

if (aOptionalArgs.startOrEndDataStruct)
if (aPrepareCommandParams.startDataStruct)
{
ReturnErrorOnFailure(invokeRequest.GetWriter()->StartContainer(TLV::ContextTag(CommandDataIB::Tag::kFields),
TLV::kTLVType_Structure, mDataElementContainerType));
Expand All @@ -477,35 +478,46 @@ CHIP_ERROR CommandSender::PrepareCommand(const CommandPathParams & aCommandPathP
return CHIP_NO_ERROR;
}

CHIP_ERROR CommandSender::FinishCommand(const AdditionalCommandParameters & aOptionalArgs)
CHIP_ERROR CommandSender::FinishCommand(FinishCommandParameters & aFinishCommandParams)
{
if (mBatchCommandsEnabled)
{
VerifyOrReturnError(aFinishCommandParams.commandRef.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(aFinishCommandParams.commandRef.Value() == mFinishedCommandCount, CHIP_ERROR_INVALID_ARGUMENT);
}

return FinishCommandInternal(aFinishCommandParams);
}

CHIP_ERROR CommandSender::FinishCommandInternal(FinishCommandParameters & aFinishCommandParams)
{
CHIP_ERROR err = CHIP_NO_ERROR;

VerifyOrReturnError(mState == State::AddingCommand, err = CHIP_ERROR_INCORRECT_STATE);

CommandDataIB::Builder & commandData = mInvokeRequestBuilder.GetInvokeRequests().GetCommandData();

if (aOptionalArgs.startOrEndDataStruct)
if (aFinishCommandParams.endDataStruct)
{
ReturnErrorOnFailure(commandData.GetWriter()->EndContainer(mDataElementContainerType));
}

if (mBatchCommandsEnabled)
{
// If error below occurs, aOptionalArgs.commandRef was modified since PerpareCommand was called.
VerifyOrReturnError(aOptionalArgs.commandRef.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
}

if (aOptionalArgs.commandRef.HasValue())
if (aFinishCommandParams.commandRef.HasValue())
{
ReturnErrorOnFailure(commandData.Ref(aOptionalArgs.commandRef.Value()));
ReturnErrorOnFailure(commandData.Ref(aFinishCommandParams.commandRef.Value()));
}

ReturnErrorOnFailure(commandData.EndOfCommandDataIB());

MoveToState(State::AddedCommand);

mFinishedCommandCount++;

if (aFinishCommandParams.timedInvokeTimeoutMs.HasValue())
{
SetTimedInvokeTimeoutMs(aFinishCommandParams.timedInvokeTimeoutMs);
}

return CHIP_NO_ERROR;
}

Expand All @@ -519,10 +531,8 @@ TLV::TLVWriter * CommandSender::GetCommandDataIBTLVWriter()
return mInvokeRequestBuilder.GetInvokeRequests().GetCommandData().GetWriter();
}

CHIP_ERROR CommandSender::FinishCommand(const Optional<uint16_t> & aTimedInvokeTimeoutMs,
const AdditionalCommandParameters & aOptionalArgs)
void CommandSender::SetTimedInvokeTimeoutMs(const Optional<uint16_t> & aTimedInvokeTimeoutMs)
{
ReturnErrorOnFailure(FinishCommand(aOptionalArgs));
if (!mTimedInvokeTimeoutMs.HasValue())
{
mTimedInvokeTimeoutMs = aTimedInvokeTimeoutMs;
Expand All @@ -532,7 +542,6 @@ CHIP_ERROR CommandSender::FinishCommand(const Optional<uint16_t> & aTimedInvokeT
uint16_t newValue = std::min(mTimedInvokeTimeoutMs.Value(), aTimedInvokeTimeoutMs.Value());
mTimedInvokeTimeoutMs.SetValue(newValue);
}
return CHIP_NO_ERROR;
}

size_t CommandSender::GetInvokeResponseMessageCount()
Expand Down
192 changes: 128 additions & 64 deletions src/app/CommandSender.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,29 +181,101 @@ class CommandSender final : public Messaging::ExchangeDelegate
uint16_t remoteMaxPathsPerInvoke = 1;
};

// PrepareCommand and FinishCommand are public SDK APIs, so we cannot break source
// compatibility for them. By having parameters to those APIs use this struct instead
// of individual function arguments, we centralize required changes to one file
// when adding new functionality.
struct AdditionalCommandParameters
// AddRequestData is a public SDK API, so we must maintain source compatibility.
// Using this struct for API parameters instead of individual parameters allows us
// to make necessary changes for new functionality in a single location.
struct AddRequestDataParameters
{
// gcc bug requires us to have the constructor below
// https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96645
AdditionalCommandParameters() {}
AddRequestDataParameters() {}

AdditionalCommandParameters & SetStartOrEndDataStruct(bool aStartOrEndDataStruct)
AddRequestDataParameters(const Optional<uint16_t> & aTimedInvokeTimeoutMs) : timedInvokeTimeoutMs(aTimedInvokeTimeoutMs) {}

// When a value is provided for timedInvokeTimeoutMs, this invoke becomes a timed
// invoke. CommandSender will use the minimum of all provided timeouts for execution.
const Optional<uint16_t> timedInvokeTimeoutMs;
// The command reference is required when sending multiple commands. It allows the caller
// to associate this request with its corresponding response.
Optional<uint16_t> commandRef;
};

// PrepareCommand is a public SDK API, so we must maintain source compatibility.
// Using this struct for API parameters instead of individual parameters allows us
// to make necessary changes for new functionality in a single location.
struct PrepareCommandParameters
{
// gcc bug requires us to have the constructor below
// https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96645
PrepareCommandParameters() {}

PrepareCommandParameters(const AddRequestDataParameters & aAddRequestDataParam) :
commandRef(aAddRequestDataParam.commandRef)
{}

PrepareCommandParameters & SetStartDataStruct(bool aStartDataStruct)
{
startOrEndDataStruct = aStartOrEndDataStruct;
startDataStruct = aStartDataStruct;
return *this;
}

// From the perspective of `PrepareCommand`, this is an out parameter. This value will be
// set by `PrepareCommand` and is expected to be unchanged by caller until it is provided
// to `FinishCommand`.
PrepareCommandParameters & SetCommandRef(uint16_t aCommandRef)
{
commandRef.SetValue(aCommandRef);
return *this;
}
// The command reference is required when sending multiple commands. It allows the caller
// to associate this request with its corresponding response. We validate the reference
// early in PrepareCommand, even though it's not used until FinishCommand. This proactive
// validation helps prevent unnecessary writing an InvokeRequest into the packet that later
// needs to be undone.
//
// Currently, provided commandRefs for the first request must start at 0 and increment by one
// for each subsequent request. This requirement can be relaxed in the future if a compelling
// need arises.
// TODO(#30453): After introducing Request/Response tracking, remove statement above about
// this currently enforced requirement on commandRefs.
Optional<uint16_t> commandRef;
// For both `PrepareCommand` and `FinishCommand` this is an in parameter. It must have
// the same value when calling `PrepareCommand` and `FinishCommand` for a given command.
bool startOrEndDataStruct = false;
// If the InvokeRequest needs to be in a state with a started data TLV struct container
bool startDataStruct = false;
};

// FinishCommand is a public SDK API, so we must maintain source compatibility.
// Using this struct for API parameters instead of individual parameters allows us
// to make necessary changes for new functionality in a single location.
struct FinishCommandParameters
{
// gcc bug requires us to have the constructor below
// https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96645
FinishCommandParameters() {}

FinishCommandParameters(const Optional<uint16_t> & aTimedInvokeTimeoutMs) : timedInvokeTimeoutMs(aTimedInvokeTimeoutMs) {}
FinishCommandParameters(const AddRequestDataParameters & aAddRequestDataParam) :
timedInvokeTimeoutMs(aAddRequestDataParam.timedInvokeTimeoutMs), commandRef(aAddRequestDataParam.commandRef)
{}

FinishCommandParameters & SetEndDataStruct(bool aEndDataStruct)
{
endDataStruct = aEndDataStruct;
return *this;
}

FinishCommandParameters & SetCommandRef(uint16_t aCommandRef)
{
commandRef.SetValue(aCommandRef);
return *this;
}

// When a value is provided for timedInvokeTimeoutMs, this invoke becomes a timed
// invoke. CommandSender will use the minimum of all provided timeouts for execution.
const Optional<uint16_t> timedInvokeTimeoutMs;
// The command reference is required when sending multiple commands. It allows the caller
// to associate this request with its corresponding response. This value must be
// the same as the one provided in PrepareCommandParameters when calling PrepareCommand.
Optional<uint16_t> commandRef;
// If InvokeRequest is in a state where the data TLV struct container is currently open
// and FinishCommand should close it.
bool endDataStruct = false;
};

/*
Expand Down Expand Up @@ -243,24 +315,30 @@ class CommandSender final : public Messaging::ExchangeDelegate
*/
CHIP_ERROR SetCommandSenderConfig(ConfigParameters & aConfigParams);

CHIP_ERROR PrepareCommand(const CommandPathParams & aCommandPathParams, AdditionalCommandParameters & aOptionalArgs);
CHIP_ERROR PrepareCommand(const CommandPathParams & aCommandPathParams, PrepareCommandParameters & aPrepareCommandParams);

[[deprecated("PrepareCommand should migrate to calling PrepareCommand with AdditionalCommandParameters")]] CHIP_ERROR
[[deprecated("PrepareCommand should migrate to calling PrepareCommand with PrepareCommandParameters")]] CHIP_ERROR
PrepareCommand(const CommandPathParams & aCommandPathParams, bool aStartDataStruct = true)
{
AdditionalCommandParameters optionalArgs;
optionalArgs.SetStartOrEndDataStruct(aStartDataStruct);
return PrepareCommand(aCommandPathParams, optionalArgs);
PrepareCommandParameters prepareCommandParams;
prepareCommandParams.SetStartDataStruct(aStartDataStruct);
return PrepareCommand(aCommandPathParams, prepareCommandParams);
}

CHIP_ERROR FinishCommand(const AdditionalCommandParameters & aOptionalArgs);
CHIP_ERROR FinishCommand(FinishCommandParameters & aFinishCommandParams);

[[deprecated("FinishCommand should migrate to calling FinishCommand with AdditionalCommandParameters")]] CHIP_ERROR
[[deprecated("FinishCommand should migrate to calling FinishCommand with FinishCommandParameters")]] CHIP_ERROR
FinishCommand(bool aEndDataStruct = true)
{
AdditionalCommandParameters optionalArgs;
optionalArgs.SetStartOrEndDataStruct(aEndDataStruct);
return FinishCommand(optionalArgs);
FinishCommandParameters finishCommandParams;
finishCommandParams.SetEndDataStruct(aEndDataStruct);
return FinishCommand(finishCommandParams);
}
[[deprecated("FinishCommand should migrate to calling FinishCommand with FinishCommandParameters")]] CHIP_ERROR
FinishCommand(const Optional<uint16_t> & aTimedInvokeTimeoutMs)
{
FinishCommandParameters finishCommandParams(aTimedInvokeTimeoutMs);
return FinishCommand(finishCommandParams);
}

TLV::TLVWriter * GetCommandDataIBTLVWriter();
Expand All @@ -275,47 +353,27 @@ class CommandSender final : public Messaging::ExchangeDelegate
* @param [in] aData The data for the request.
*/
template <typename CommandDataT, typename std::enable_if_t<!CommandDataT::MustUseTimedInvoke(), int> = 0>
CHIP_ERROR AddRequestData(const CommandPathParams & aCommandPath, const CommandDataT & aData,
AdditionalCommandParameters & aOptionalArgs)
{
return AddRequestData(aCommandPath, aData, NullOptional, aOptionalArgs);
}
template <typename CommandDataT, typename std::enable_if_t<!CommandDataT::MustUseTimedInvoke(), int> = 0>
CHIP_ERROR AddRequestData(const CommandPathParams & aCommandPath, const CommandDataT & aData)
{
AdditionalCommandParameters optionalArgs;
return AddRequestData(aCommandPath, aData, optionalArgs);
AddRequestDataParameters addRequestDataParams;
return AddRequestData(aCommandPath, aData, addRequestDataParams);
}

/**
* API for adding a data request that allows caller to provide a timed
* invoke timeout. If provided, this invoke will be a timed invoke, using
* the minimum of the provided timeouts.
*/
template <typename CommandDataT>
CHIP_ERROR AddRequestData(const CommandPathParams & aCommandPath, const CommandDataT & aData,
const Optional<uint16_t> & aTimedInvokeTimeoutMs, AdditionalCommandParameters & aOptionalArgs)
AddRequestDataParameters & aAddRequestDataParams)
{
VerifyOrReturnError(!CommandDataT::MustUseTimedInvoke() || aTimedInvokeTimeoutMs.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
// AddRequestDataInternal encodes the data and requires startOrEndDataStruct to be false.
VerifyOrReturnError(aOptionalArgs.startOrEndDataStruct == false, CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(!CommandDataT::MustUseTimedInvoke() || aAddRequestDataParams.timedInvokeTimeoutMs.HasValue(),
CHIP_ERROR_INVALID_ARGUMENT);

return AddRequestDataInternal(aCommandPath, aData, aTimedInvokeTimeoutMs, aOptionalArgs);
return AddRequestDataInternal(aCommandPath, aData, aAddRequestDataParams);
}
template <typename CommandDataT>
CHIP_ERROR AddRequestData(const CommandPathParams & aCommandPath, const CommandDataT & aData,
const Optional<uint16_t> & aTimedInvokeTimeoutMs)
{
AdditionalCommandParameters optionalArgs;
return AddRequestData(aCommandPath, aData, aTimedInvokeTimeoutMs, optionalArgs);
}

CHIP_ERROR FinishCommand(const Optional<uint16_t> & aTimedInvokeTimeoutMs, const AdditionalCommandParameters & aOptionalArgs);

CHIP_ERROR FinishCommand(const Optional<uint16_t> & aTimedInvokeTimeoutMs)
{
AdditionalCommandParameters optionalArgs;
return FinishCommand(aTimedInvokeTimeoutMs, optionalArgs);
AddRequestDataParameters addRequestDataParams(aTimedInvokeTimeoutMs);
return AddRequestData(aCommandPath, aData, addRequestDataParams);
}

/**
Expand All @@ -333,19 +391,19 @@ class CommandSender final : public Messaging::ExchangeDelegate
* timeout parameter. For use in tests only.
*/
template <typename CommandDataT>
CHIP_ERROR AddRequestDataNoTimedCheck(const CommandPathParams & aCommandPath, const CommandDataT & aData,
const Optional<uint16_t> & aTimedInvokeTimeoutMs,
AdditionalCommandParameters & aOptionalArgs)
CHIP_ERROR TestOnlyAddRequestDataNoTimedCheck(const CommandPathParams & aCommandPath, const CommandDataT & aData,
AddRequestDataParameters & aAddRequestDataParams)
{
return AddRequestDataInternal(aCommandPath, aData, aTimedInvokeTimeoutMs, aOptionalArgs);
return AddRequestDataInternal(aCommandPath, aData, aAddRequestDataParams);
}

template <typename CommandDataT>
CHIP_ERROR AddRequestDataNoTimedCheck(const CommandPathParams & aCommandPath, const CommandDataT & aData,
const Optional<uint16_t> & aTimedInvokeTimeoutMs)
CHIP_ERROR TestOnlyFinishCommand(FinishCommandParameters & aFinishCommandParams)
{
AdditionalCommandParameters optionalArgs;
return AddRequestDataNoTimedCheck(aCommandPath, aData, aTimedInvokeTimeoutMs, optionalArgs);
if (mBatchCommandsEnabled)
{
VerifyOrReturnError(aFinishCommandParams.commandRef.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
}
return FinishCommandInternal(aFinishCommandParams);
}

/**
Expand All @@ -360,15 +418,19 @@ class CommandSender final : public Messaging::ExchangeDelegate
private:
template <typename CommandDataT>
CHIP_ERROR AddRequestDataInternal(const CommandPathParams & aCommandPath, const CommandDataT & aData,
const Optional<uint16_t> & aTimedInvokeTimeoutMs, AdditionalCommandParameters & aOptionalArgs)
AddRequestDataParameters & aAddRequestDataParams)
{
ReturnErrorOnFailure(PrepareCommand(aCommandPath, aOptionalArgs));
PrepareCommandParameters prepareCommandParams(aAddRequestDataParams);
ReturnErrorOnFailure(PrepareCommand(aCommandPath, prepareCommandParams));
TLV::TLVWriter * writer = GetCommandDataIBTLVWriter();
VerifyOrReturnError(writer != nullptr, CHIP_ERROR_INCORRECT_STATE);
ReturnErrorOnFailure(DataModel::Encode(*writer, TLV::ContextTag(CommandDataIB::Tag::kFields), aData));
return FinishCommand(aTimedInvokeTimeoutMs, aOptionalArgs);
FinishCommandParameters finishCommandParams(aAddRequestDataParams);
return FinishCommand(finishCommandParams);
}

CHIP_ERROR FinishCommandInternal(FinishCommandParameters & aFinishCommandParams);

public:
// Sends a queued up command request to the target encapsulated by the secureSession handle.
//
Expand Down Expand Up @@ -453,6 +515,8 @@ class CommandSender final : public Messaging::ExchangeDelegate
CHIP_ERROR ProcessInvokeResponse(System::PacketBufferHandle && payload, bool & moreChunkedMessages);
CHIP_ERROR ProcessInvokeResponseIB(InvokeResponseIB::Parser & aInvokeResponse);

void SetTimedInvokeTimeoutMs(const Optional<uint16_t> & aTimedInvokeTimeoutMs);

// Send our queued-up Invoke Request message. Assumes the exchange is ready
// and mPendingInvokeData is populated.
CHIP_ERROR SendInvokeRequest();
Expand Down
Loading

0 comments on commit d308a50

Please sign in to comment.