Skip to content

Commit

Permalink
Merge 2d8dd35 into c36f4a7
Browse files Browse the repository at this point in the history
  • Loading branch information
@tehampson
tehampson committed Jan 10, 2024
2 parents c36f4a7 + 2d8dd35 commit 4330621
Show file tree
Hide file tree
Showing 4 changed files with 224 additions and 69 deletions.
44 changes: 24 additions & 20 deletions src/app/CommandSender.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,16 @@ CommandSender::CommandSender(Callback * apCallback, Messaging::ExchangeManager *
assertChipStackLockedByCurrentThread();
}

CommandSender::CommandSender(ExtendedCallback * apExtendedCallback, Messaging::ExchangeManager * apExchangeMgr,
bool aIsTimedRequest, bool aSuppressResponse) :
mExchangeCtx(*this),
mpExtendedCallback(apExtendedCallback), mpExchangeMgr(apExchangeMgr), mSuppressResponse(aSuppressResponse),
mTimedRequest(aIsTimedRequest)
{
assertChipStackLockedByCurrentThread();
mUsingExtendedCallbacks = true;
}

CommandSender::~CommandSender()
{
assertChipStackLockedByCurrentThread();
Expand Down Expand Up @@ -229,12 +239,9 @@ CHIP_ERROR CommandSender::OnMessageReceived(Messaging::ExchangeContext * apExcha
}

exit:
if (mpCallback != nullptr)
if (err != CHIP_NO_ERROR)
{
if (err != CHIP_NO_ERROR)
{
mpCallback->OnError(this, err);
}
OnErrorCallback(err);
}

if (sendStatusResponse)
Expand Down Expand Up @@ -295,7 +302,7 @@ void CommandSender::OnResponseTimeout(Messaging::ExchangeContext * apExchangeCon

if (mpCallback != nullptr)
{
mpCallback->OnError(this, CHIP_ERROR_TIMEOUT);
OnErrorCallback(CHIP_ERROR_TIMEOUT);
}

Close();
Expand All @@ -307,10 +314,7 @@ void CommandSender::Close()
mTimedRequest = false;
MoveToState(State::AwaitingDestruction);

if (mpCallback)
{
mpCallback->OnDone(this);
}
OnDoneCallback();
}

CHIP_ERROR CommandSender::ProcessInvokeResponseIB(InvokeResponseIB::Parser & aInvokeResponse)
Expand Down Expand Up @@ -382,17 +386,17 @@ CHIP_ERROR CommandSender::ProcessInvokeResponseIB(InvokeResponseIB::Parser & aIn
}
ReturnErrorOnFailure(err);

if (mpCallback != nullptr)
// When using ExtendedCallbacks, we are adhearing to a different API contract where path
// specific error are send to the OnResponse callback. For more information on the history
// of this issue please see https://github.com/project-chip/connectedhomeip/issues/30991
if (statusIB.IsSuccess() || mUsingExtendedCallbacks)
{
if (statusIB.IsSuccess())
{
mpCallback->OnResponseWithAdditionalData(this, ConcreteCommandPath(endpointId, clusterId, commandId), statusIB,
hasDataResponse ? &commandDataReader : nullptr, additionalResponseData);
}
else
{
mpCallback->OnError(this, statusIB.ToChipError());
}
OnResponseCallback(ConcreteCommandPath(endpointId, clusterId, commandId), statusIB,
hasDataResponse ? &commandDataReader : nullptr, additionalResponseData);
}
else
{
OnErrorCallback(statusIB.ToChipError());
}
}
return CHIP_NO_ERROR;
Expand Down
205 changes: 170 additions & 35 deletions src/app/CommandSender.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,76 +53,146 @@ namespace app {
class CommandSender final : public Messaging::ExchangeDelegate
{
public:
// CommandSender::Callback::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
{
Optional<uint16_t> mCommandRef;
};

class Callback
{
public:
virtual ~Callback() = default;

/**
* OnResponseWithAdditionalData will be called when a successful response from the server has been received and processed.
* OnResponse will be called when a successful response from server has been received and processed.
* Specifically:
* - When a status code is received and it is IM::Success, aData will be nullptr.
* - When a data response is received, aData will point to a valid TLVReader initialized to point at the struct container
* that contains the data payload (callee will still need to open and process the container).
*
* This OnResponseWithAdditionalData is similar to OnResponse mentioned below, except it contains an additional parameter
* `AdditionalResponseData`. This was added in Matter 1.3 to not break backward compatibility, but is extendable in the
* future to provide additional response data by only making changes to `AdditionalResponseData`, and not all the potential
* callees.
*
* The CommandSender object MUST continue to exist after this call is completed. The application shall wait until it
* receives an OnDone call to destroy the object.
*
* It is advised that subclass should only override this or `OnResponse`. But, it shouldn't actually matter if both are
* overridden, just that `OnResponse` will never be called by CommandSender directly.
* It is advised that subclasses should only override this or `OnResponseWithAdditionalData`. But, it shouldn't actually
* matter if both are overridden, just that `OnResponse` will never be called by CommandSender directly.
*
* @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.
*/
virtual void OnResponseWithAdditionalData(CommandSender * apCommandSender, const ConcreteCommandPath & aPath,
const StatusIB & aStatusIB, TLV::TLVReader * apData,
const AdditionalResponseData & aAdditionalResponseData)
{
OnResponse(apCommandSender, aPath, aStatusIB, apData);
}
virtual void OnResponse(CommandSender * apCommandSender, const ConcreteCommandPath & aPath, const StatusIB & aStatusIB,
TLV::TLVReader * apData)
{}

/**
* OnError will be called when an error occur *after* a successful call to SendCommandRequest(). The following
* errors will be delivered through this call in the aError field:
*
* - 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.
* - CHIP_ERROR encapsulating a StatusIB: If we got a non-path-specific
* status response from the server. In that case,
* StatusIB::InitFromChipError can be used to extract the status.
* - CHIP_ERROR encapsulating a StatusIB: If we got a path-specific
* status response from the server. In that case,
* StatusIB::InitFromChipError can be used to extract the status.
* - Note path specific error only come here happens if UsingExtendedPathCallbacks()
* returns false.
* - There isn't a guaranteeded way to differentiate between a non-path-specific error and a
* path-specific error from 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
* receives an OnDone call to destroy and free the object.
*
* @param[in] apCommandSender The command sender object that initiated the command transaction.
* @param[in] aError A system error code that conveys the overall error code.
*/
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
* allocated CommandSender object.
*
* This function will:
* - Always be called exactly *once* for a given CommandSender instance.
* - Be called even in error circumstances.
* - Only be called after a successful call to SendCommandRequest returns, if SendCommandRequest is used.
* - Always be called before a successful return from SendGroupCommandRequest, if SendGroupCommandRequest is used.
*
* This function must be implemented to destroy the CommandSender object.
*
* @param[in] apCommandSender The command sender object of the terminated invoke command transaction.
*/
virtual void OnDone(CommandSender * apCommandSender) = 0;
};

// CommandSender::ExtendedCallback::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
{
Optional<uint16_t> mCommandRef;
};

// CommandSender::ExtendedCallback::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
{
CHIP_ERROR mChipError;
};

/**
* @brief asdf
*
* The two major differences between ExtendedCallback 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
*/
class ExtendedCallback
{
public:
virtual ~ExtendedCallback() = default;

/**
* OnResponse will be called when a successful response from server has been received and processed. Specifically:
* OnResponseWithAdditionalData will be called for all path specific responses from the server has 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.
* - Note this only happens if UsingExtendedPathCallbacks() returns true.
* - When a data response is received, aData will point to a valid TLVReader initialized to point at the struct container
* that contains the data payload (callee will still need to open and process the container).
*
* This OnResponseWithAdditionalData is similar to OnResponse mentioned below, except it contains an additional parameter
* `AdditionalResponseData`. This was added in Matter 1.3 to not break backward compatibility, but is extendable in the
* future to provide additional response data by only making changes to `AdditionalResponseData`, and not all the potential
* callees.
*
* The CommandSender object MUST continue to exist after this call is completed. The application shall wait until it
* receives an OnDone call to destroy the object.
*
* It is advised that subclasses should only override this or `OnResponseWithAdditionalData`. But, it shouldn't actually
* matter if both are overridden, just that `OnResponse` will never be called by CommandSender directly.
* It is advised that subclass should only override this or `OnResponse`. But, it shouldn't actually matter if both are
* overridden, just that `OnResponse` will never be called by CommandSender directly.
*
* @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.
*/
virtual void OnResponse(CommandSender * apCommandSender, const ConcreteCommandPath & aPath, const StatusIB & aStatusIB,
TLV::TLVReader * apData)
TLV::TLVReader * apData, const AdditionalResponseData & aAdditionalResponseData)
{}

/**
Expand All @@ -134,6 +204,13 @@ class CommandSender final : public Messaging::ExchangeDelegate
* - CHIP_ERROR encapsulating a StatusIB: If we got a non-path-specific
* status response from the server. In that case,
* StatusIB::InitFromChipError can be used to extract the status.
* - CHIP_ERROR encapsulating a StatusIB: If we got a path-specific
* status response from the server. In that case,
* StatusIB::InitFromChipError can be used to extract the status.
* - Note path specific error only come here happens if UsingExtendedPathCallbacks()
* returns false.
* - There isn't a guaranteeded way to differentiate between a non-path-specific error and a
* path-specific error from 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 @@ -142,7 +219,7 @@ class CommandSender final : public Messaging::ExchangeDelegate
* @param[in] apCommandSender The command sender object that initiated the command transaction.
* @param[in] aError A system error code that conveys the overall error code.
*/
virtual void OnError(const CommandSender * apCommandSender, CHIP_ERROR aError) {}
virtual void OnError(const CommandSender * apCommandSender, ErrorData aErrorData) {}

/**
* OnDone will be called when CommandSender has finished all work and is safe to destroy and free the
Expand Down Expand Up @@ -212,6 +289,12 @@ 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,
bool aSuppressResponse = false);
CommandSender(std::nullptr_t, Messaging::ExchangeManager * apExchangeMgr, bool aIsTimedRequest = false,
bool aSuppressResponse = false) :
CommandSender(static_cast<Callback *>(nullptr), apExchangeMgr, aIsTimedRequest, aSuppressResponse)
{}
~CommandSender();

/**
Expand Down Expand Up @@ -436,8 +519,59 @@ 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)
{
if (mUsingExtendedCallbacks)
{
if (mpExtendedCallback)
{
mpExtendedCallback->OnResponse(this, aPath, aStatusIB, apData, aAdditionalResponseData);
}
return;
}
if (mpCallback)
{
mpCallback->OnResponse(this, aPath, aStatusIB, apData);
}
}

void OnErrorCallback(CHIP_ERROR aError)
{
if (mUsingExtendedCallbacks)
{
if (mpExtendedCallback)
{
ErrorData errorData = { aError };
mpExtendedCallback->OnError(this, errorData);
}
return;
}
if (mpCallback)
{
mpCallback->OnError(this, aError);
}
}

void OnDoneCallback()
{
if (mUsingExtendedCallbacks)
{
if (mpExtendedCallback)
{
mpExtendedCallback->OnDone(this);
}
return;
}
if (mpCallback)
{
mpCallback->OnDone(this);
}
}

Messaging::ExchangeHolder mExchangeCtx;
Callback * mpCallback = nullptr;
ExtendedCallback * mpExtendedCallback = nullptr;
Messaging::ExchangeManager * mpExchangeMgr = nullptr;
InvokeRequestMessage::Builder mInvokeRequestBuilder;
// TODO Maybe we should change PacketBufferTLVWriter so we can finalize it
Expand All @@ -454,10 +588,11 @@ class CommandSender final : public Messaging::ExchangeDelegate
uint16_t mFinishedCommandCount = 0;
uint16_t mRemoteMaxPathsPerInvoke = 1;

bool mSuppressResponse = false;
bool mTimedRequest = false;
bool mBufferAllocated = false;
bool mBatchCommandsEnabled = false;
bool mSuppressResponse = false;
bool mTimedRequest = false;
bool mBufferAllocated = false;
bool mBatchCommandsEnabled = false;
bool mUsingExtendedCallbacks = false;
};

} // namespace app
Expand Down

0 comments on commit 4330621

Please sign in to comment.