Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor CommandSender newly added APIs to be more extendable #31837

Merged
merged 4 commits into from
Feb 8, 2024
Merged

Refactor CommandSender newly added APIs to be more extendable #31837

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
57bd203
Working refactor
tehampson Feb 1, 2024
880522e
Restyled by clang-format
restyled-commits Feb 1, 2024
df35368
Address PR comments
tehampson Feb 7, 2024
bf7510b
Merge branch 'master' into modify-command-sender-apis
tehampson Feb 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 27 additions & 18 deletions src/app/CommandSender.cpp
View file
Open in desktop
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
Open in desktop
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;
tehampson marked this conversation as resolved.
Show resolved Hide resolved
// 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.
bzbarsky-apple marked this conversation as resolved.
Show resolved Hide resolved
tehampson marked this conversation as resolved.
Show resolved Hide resolved
// 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;
tehampson marked this conversation as resolved.
Show resolved Hide resolved
// 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
Loading