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

Introduce CommandSender::ExtendedCallback #31324

Merged
merged 27 commits into from
Jan 11, 2024
Merged

Introduce CommandSender::ExtendedCallback #31324

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
4b07754
Introduce CommandSender::Callback::PathSpecificErrorGoesToOnResponseC…
tehampson Jan 9, 2024
5287ed8
Restyled by whitespace
restyled-commits Jan 9, 2024
334b035
Restyled by clang-format
restyled-commits Jan 9, 2024
092f9b2
Address PR comments
tehampson Jan 9, 2024
814071c
Address PR comments
tehampson Jan 9, 2024
5c77a13
Add unit test
tehampson Jan 9, 2024
97111e7
Restyled by clang-format
restyled-commits Jan 9, 2024
d07f57d
Address PR comments
tehampson Jan 9, 2024
6f2d8f8
Rework PR based on offline discussions
tehampson Jan 10, 2024
ee8adec
Restyled by whitespace
restyled-commits Jan 10, 2024
b1910f7
Restyled by clang-format
restyled-commits Jan 10, 2024
d5bdaa9
Address TODO comment
tehampson Jan 10, 2024
2d8dd35
Quick cleanup before initial santity check reviews
tehampson Jan 10, 2024
d02f838
Quick cleanup before initial santity check reviews
tehampson Jan 10, 2024
c61976c
Polish up PR for reviews
tehampson Jan 10, 2024
ec3c501
Restyled by whitespace
restyled-commits Jan 10, 2024
5391bb8
Restyled by clang-format
restyled-commits Jan 10, 2024
272cf33
Update API comments
tehampson Jan 10, 2024
9fedf93
Restyled by whitespace
restyled-commits Jan 10, 2024
d026238
Address PR comments
tehampson Jan 10, 2024
7d2d077
Restyled by clang-format
restyled-commits Jan 10, 2024
825e11a
Apply suggestions from code review
tehampson Jan 11, 2024
cb4c3ab
Address PR comments
tehampson Jan 11, 2024
295afb1
Address PR comments
tehampson Jan 11, 2024
05e4f28
Restyled by clang-format
restyled-commits Jan 11, 2024
cd8e4c8
Address PR comments
tehampson Jan 11, 2024
78ec6ff
Restyled by clang-format
restyled-commits Jan 11, 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
31 changes: 17 additions & 14 deletions src/app/CommandSender.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,10 @@ CommandSender::CommandSender(Callback * apCallback, Messaging::ExchangeManager *
assertChipStackLockedByCurrentThread();
}

CommandSender::CommandSender(ExtendedCallback * apExtendedCallback, Messaging::ExchangeManager * apExchangeMgr,
CommandSender::CommandSender(ExtendableCallback * apExtendableCallback, Messaging::ExchangeManager * apExchangeMgr,
bool aIsTimedRequest, bool aSuppressResponse) :
mExchangeCtx(*this),
mpExtendedCallback(apExtendedCallback), mpExchangeMgr(apExchangeMgr), mSuppressResponse(aSuppressResponse),
mpExtendableCallback(apExtendableCallback), mpExchangeMgr(apExchangeMgr), mSuppressResponse(aSuppressResponse),
mTimedRequest(aIsTimedRequest)
{
assertChipStackLockedByCurrentThread();
tcarmelveilleux marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -89,7 +89,7 @@ CHIP_ERROR CommandSender::AllocateBuffer()
// We are making sure that both callbacks are not set. This should never happen as the constructors
// are strongly typed and only one should ever be set, but explicit check is here to ensure that is
// always the case.
bool bothCallbacksAreSet = mpExtendedCallback != nullptr && mpCallback != nullptr;
bool bothCallbacksAreSet = mpExtendableCallback != nullptr && mpCallback != nullptr;
VerifyOrDie(!bothCallbacksAreSet);

mCommandMessageWriter.Reset();
Expand Down Expand Up @@ -334,7 +334,7 @@ CHIP_ERROR CommandSender::ProcessInvokeResponseIB(InvokeResponseIB::Parser & aIn
bool commandRefExpected = (mFinishedCommandCount > 1);
bool hasDataResponse = false;
TLV::TLVReader commandDataReader;
AdditionalResponseData additionalResponseData;
ResponseData responseData;

CommandStatusIB::Parser commandStatus;
err = aInvokeResponse.GetStatus(&commandStatus);
Expand All @@ -349,7 +349,7 @@ CHIP_ERROR CommandSender::ProcessInvokeResponseIB(InvokeResponseIB::Parser & aIn
StatusIB::Parser status;
commandStatus.GetErrorStatus(&status);
ReturnErrorOnFailure(status.DecodeStatusIB(statusIB));
ReturnErrorOnFailure(GetRef(commandStatus, additionalResponseData.commandRef, commandRefExpected));
ReturnErrorOnFailure(GetRef(commandStatus, responseData.commandRef, commandRefExpected));
}
else if (CHIP_END_OF_TLV == err)
{
Expand All @@ -361,7 +361,7 @@ CHIP_ERROR CommandSender::ProcessInvokeResponseIB(InvokeResponseIB::Parser & aIn
ReturnErrorOnFailure(commandPath.GetClusterId(&clusterId));
ReturnErrorOnFailure(commandPath.GetCommandId(&commandId));
commandData.GetFields(&commandDataReader);
ReturnErrorOnFailure(GetRef(commandData, additionalResponseData.commandRef, commandRefExpected));
ReturnErrorOnFailure(GetRef(commandData, responseData.commandRef, commandRefExpected));
err = CHIP_NO_ERROR;
hasDataResponse = true;
}
Expand Down Expand Up @@ -390,14 +390,17 @@ CHIP_ERROR CommandSender::ProcessInvokeResponseIB(InvokeResponseIB::Parser & aIn
}
ReturnErrorOnFailure(err);

// When using ExtendedCallbacks, we are adhearing to a different API contract where path
// specific error are sent to the OnResponse callback. For more information on the history
// When using ExtendableCallbacks, we are adhering to a different API contract where path
// specific errors are sent to the OnResponse callback. For more information on the history
// of this issue please see https://github.com/project-chip/connectedhomeip/issues/30991
bool usingExtendedCallbacks = mpExtendedCallback != nullptr;
if (statusIB.IsSuccess() || usingExtendedCallbacks)
bool usingExtendableCallbacks = mpExtendableCallback != nullptr;
if (statusIB.IsSuccess() || usingExtendableCallbacks)
{
OnResponseCallback(ConcreteCommandPath(endpointId, clusterId, commandId), statusIB,
hasDataResponse ? &commandDataReader : nullptr, additionalResponseData);
const ConcreteCommandPath concretePath = ConcreteCommandPath(endpointId, clusterId, commandId);
responseData.path = &concretePath;
responseData.statusIB = &statusIB;
responseData.data = hasDataResponse ? &commandDataReader : nullptr;
OnResponseCallback(responseData);
}
else
{
Expand Down Expand Up @@ -429,8 +432,8 @@ CHIP_ERROR CommandSender::PrepareCommand(const CommandPathParams & aCommandPathP
//
// We must not be in the middle of preparing a command, and must not have already sent InvokeRequestMessage.
//
bool usingExtendedCallbacks = mpExtendedCallback != nullptr;
bool canAddAnotherCommand = (mState == State::AddedCommand && mBatchCommandsEnabled && usingExtendedCallbacks);
bool usingExtendableCallbacks = mpExtendableCallback != nullptr;
bool canAddAnotherCommand = (mState == State::AddedCommand && mBatchCommandsEnabled && usingExtendableCallbacks);
VerifyOrReturnError(mState == State::Idle || canAddAnotherCommand, CHIP_ERROR_INCORRECT_STATE);
VerifyOrReturnError(mFinishedCommandCount < mRemoteMaxPathsPerInvoke, CHIP_ERROR_MAXIMUM_PATHS_PER_INVOKE_EXCEEDED);

Expand Down
89 changes: 47 additions & 42 deletions src/app/CommandSender.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ namespace app {
class CommandSender final : public Messaging::ExchangeDelegate
{
public:
// TODO(#30453) Reorder CommandSender::Callback and CommandSender::ExtendableCallback. To keep follow up
// review simple, this was not done initially, and is expected to be completed within day of PR
// introducing CommandSender::ExtendableCallback getting merged.
/**
* @brief Legacy callbacks for CommandSender
tehampson marked this conversation as resolved.
Show resolved Hide resolved
*/
Expand Down Expand Up @@ -91,9 +94,9 @@ class CommandSender final : public Messaging::ExchangeDelegate
* - CHIP_ERROR encapsulating a StatusIB: If we got a non-path-specific or path-specific
* status response from the server. In that case,
* StatusIB::InitFromChipError can be used to extract the status.
* - Note CommandSender's using `CommandSender::Callback` only supports sending
* single InvokeRequest. As a result, only one path-specific error is expected
* to ever be sent to OnError callback.
* - Note: a CommandSender using `CommandSender::Callback` only supports sending
* a single InvokeRequest. As a result, only one path-specific error is expected
* to ever be sent to the OnError callback.
* - CHIP_ERROR*: All other cases.
*
* The CommandSender object MUST continue to exist after this call is completed. The application shall wait until it
Expand All @@ -105,7 +108,7 @@ class CommandSender final : public Messaging::ExchangeDelegate
virtual void OnError(const CommandSender * apCommandSender, CHIP_ERROR aError) {}

/**
* OnDone will be called when CommandSender has finished all work and is safe to destroy and free the
* OnDone will be called when CommandSender has finished all work and it is safe to destroy and free the
* allocated CommandSender object.
*
* This function will:
Expand All @@ -121,25 +124,35 @@ class CommandSender final : public Messaging::ExchangeDelegate
virtual void OnDone(CommandSender * apCommandSender) = 0;
};

// CommandSender::ExtendedCallback::OnResponse is public SDK API, so we cannot break
// CommandSender::ExtendableCallback::OnResponse is public SDK API, so we cannot break
// source compatibility for it. To allow for additional values to be added at a future
// time without constantly changing the function's declaration parameter list, we are
// defining the struct AdditionalResponseData and adding that to the parameter list
// to allow for future extendability.
struct AdditionalResponseData
// defining the struct ResponseData and adding that to the parameter list to allow for
// future extendability.
struct ResponseData
{
// The command path field in invoke command response.
const ConcreteCommandPath * path;
tehampson marked this conversation as resolved.
Show resolved Hide resolved
// The status of the command. It can be any success status, including possibly a cluster-specific one.
// If `data` is not null, statusIB will always be a generic SUCCESS status with no-cluster specific
// information.
const StatusIB * statusIB;
// The command data, will be nullptr if the server returns a StatusIB.
TLV::TLVReader * data;
// Reference for the command. This should be associated with the reference value sent out in the initial
// invoke request.
Optional<uint16_t> commandRef;
tehampson marked this conversation as resolved.
Show resolved Hide resolved
};

// CommandSender::ExtendedCallback::OnError is public SDK API, so we cannot break source
// CommandSender::ExtendableCallback::OnError is public SDK API, so we cannot break source
// compatibility for it. To allow for additional values to be added at a future time
// without constantly changing the function's declaration parameter list, we are
// defining the struct ErrorData and adding that to the parameter list
// to allow for future extendability.
struct ErrorData
{
/**
* The following errors will be delivered through chipError
* The following errors will be delivered through `error`
*
* - CHIP_ERROR_TIMEOUT: A response was not received within the expected response timeout.
* - CHIP_ERROR_*TLV*: A malformed, non-compliant response was received from the server.
Expand All @@ -148,28 +161,28 @@ class CommandSender final : public Messaging::ExchangeDelegate
* StatusIB::InitFromChipError can be used to extract the status.
* - CHIP_ERROR*: All other cases.
*/
CHIP_ERROR chipError;
CHIP_ERROR error;
};

/**
* @brief Callback that is extendable for future features, starting with batch commands
*
* The two major differences between ExtendedCallback and Callback are:
* The two major differences between ExtendableCallback and Callback are:
* 1. Path-specific errors go to OnResponse instead of OnError
* - Note: Non-path-specific errors still go to OnError.
* 2. Instead of having new parameters at the end of the arguments list, with defaults,
* as functionality expands, a parameter whose type is defined in this header is used
* as the argument to the callbacks
*
* To support batch commands client needs to be using ExtendedCallback
* To support batch commands client must use ExtendableCallback.
*/
class ExtendedCallback
class ExtendableCallback
{
public:
virtual ~ExtendedCallback() = default;
virtual ~ExtendableCallback() = default;

/**
* OnResponse will be called for all path specific responses from the server that has been received
* OnResponse will be called for all path specific responses from the server that have been received
* and processed. Specifically:
* - When a status code is received and it is IM::Success, aData will be nullptr.
* - When a status code is received and it is IM and/or cluster error, aData will be nullptr.
Expand All @@ -181,25 +194,18 @@ class CommandSender final : public Messaging::ExchangeDelegate
* receives an OnDone call to destroy the object.
*
* @param[in] apCommandSender The command sender object that initiated the command transaction.
* @param[in] aPath The command path field in invoke command response.
* @param[in] aStatusIB It will always have a success status. If apData is null, it can be any success status,
* including possibly a cluster-specific one. If apData is not null it aStatusIB will always
* be a generic SUCCESS status with no-cluster specific information.
* @param[in] apData The command data, will be nullptr if the server returns a StatusIB.
* @param[in] aAdditionalResponseData
* Additional response data that comes within the InvokeResponseMessage.
* @param[in] aResponseData Information pertaining to the response.
*/
virtual void OnResponse(CommandSender * apCommandSender, const ConcreteCommandPath & aPath, const StatusIB & aStatusIB,
TLV::TLVReader * apData, const AdditionalResponseData & aAdditionalResponseData)
{}
;
virtual void OnResponse(CommandSender * commandSender, const ResponseData & aResponseData) {}

/**
* OnError will be called when an error occurs *after* a successful call to SendCommandRequest().
* OnError will be called when a non-path-specific error occurs *after* a successful call to SendCommandRequest().
*
* The CommandSender object MUST continue to exist after this call is completed. The application shall wait until it
* receives an OnDone call to destroy and free the object.
*
* NOTE: Path specific error do NOT come to OnError, but instead go to OnResponse.
* NOTE: Path specific errors do NOT come to OnError, but instead go to OnResponse.
*
* @param[in] apCommandSender The command sender object that initiated the command transaction.
* @param[in] aErrorData A error data regarding error that occurred.
Expand Down Expand Up @@ -274,7 +280,7 @@ class CommandSender final : public Messaging::ExchangeDelegate
*/
CommandSender(Callback * apCallback, Messaging::ExchangeManager * apExchangeMgr, bool aIsTimedRequest = false,
bool aSuppressResponse = false);
CommandSender(ExtendedCallback * apCallback, Messaging::ExchangeManager * apExchangeMgr, bool aIsTimedRequest = false,
CommandSender(ExtendableCallback * apCallback, Messaging::ExchangeManager * apExchangeMgr, bool aIsTimedRequest = false,
bool aSuppressResponse = false);
CommandSender(std::nullptr_t, Messaging::ExchangeManager * apExchangeMgr, bool aIsTimedRequest = false,
bool aSuppressResponse = false) :
Expand Down Expand Up @@ -504,27 +510,26 @@ class CommandSender final : public Messaging::ExchangeDelegate

CHIP_ERROR SendCommandRequestInternal(const SessionHandle & session, Optional<System::Clock::Timeout> timeout);

void OnResponseCallback(const ConcreteCommandPath & aPath, const StatusIB & aStatusIB, TLV::TLVReader * apData,
const AdditionalResponseData & aAdditionalResponseData)
void OnResponseCallback(const ResponseData & aResponseData)
{
// mpExtendedCallback and mpCallback are mutually exclusive.
if (mpExtendedCallback)
// mpExtendableCallback and mpCallback are mutually exclusive.
if (mpExtendableCallback)
{
mpExtendedCallback->OnResponse(this, aPath, aStatusIB, apData, aAdditionalResponseData);
mpExtendableCallback->OnResponse(this, aResponseData);
}
else if (mpCallback)
{
mpCallback->OnResponse(this, aPath, aStatusIB, apData);
mpCallback->OnResponse(this, *aResponseData.path, *aResponseData.statusIB, aResponseData.data);
}
}

void OnErrorCallback(CHIP_ERROR aError)
{
// mpExtendedCallback and mpCallback are mutually exclusive.
if (mpExtendedCallback)
// mpExtendableCallback and mpCallback are mutually exclusive.
if (mpExtendableCallback)
{
ErrorData errorData = { aError };
mpExtendedCallback->OnError(this, errorData);
mpExtendableCallback->OnError(this, errorData);
}
else if (mpCallback)
{
Expand All @@ -534,10 +539,10 @@ class CommandSender final : public Messaging::ExchangeDelegate

void OnDoneCallback()
{
// mpExtendedCallback and mpCallback are mutually exclusive.
if (mpExtendedCallback)
// mpExtendableCallback and mpCallback are mutually exclusive.
if (mpExtendableCallback)
{
mpExtendedCallback->OnDone(this);
mpExtendableCallback->OnDone(this);
}
else if (mpCallback)
{
Expand All @@ -547,7 +552,7 @@ class CommandSender final : public Messaging::ExchangeDelegate

Messaging::ExchangeHolder mExchangeCtx;
Callback * mpCallback = nullptr;
ExtendedCallback * mpExtendedCallback = nullptr;
ExtendableCallback * mpExtendableCallback = nullptr;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider putting this together with mpCallback in a union to save RAM. An outer bitfield:1 could be used to differentiate which field is active.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#31609

Messaging::ExchangeManager * mpExchangeMgr = nullptr;
InvokeRequestMessage::Builder mInvokeRequestBuilder;
// TODO Maybe we should change PacketBufferTLVWriter so we can finalize it
Expand Down
Loading
Loading