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

Union pointers to save space in CommandSender #31609

Merged
merged 2 commits into from
Jan 23, 2024
+33 −33
Merged

Union pointers to save space in CommandSender #31609

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f354f5c
Union pointers to save space in CommandSender
tehampson Jan 22, 2024
6c68463
Restyled by clang-format
restyled-commits Jan 22, 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
18 changes: 5 additions & 13 deletions src/app/CommandSender.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,16 +63,16 @@ CHIP_ERROR GetRef(ParserT aParser, Optional<uint16_t> & aRef, bool commandRefExp
CommandSender::CommandSender(Callback * apCallback, Messaging::ExchangeManager * apExchangeMgr, bool aIsTimedRequest,
bool aSuppressResponse) :
mExchangeCtx(*this),
mpCallback(apCallback), mpExchangeMgr(apExchangeMgr), mSuppressResponse(aSuppressResponse), mTimedRequest(aIsTimedRequest)
mCallbackHandle(apCallback), mpExchangeMgr(apExchangeMgr), mSuppressResponse(aSuppressResponse), mTimedRequest(aIsTimedRequest)
{
assertChipStackLockedByCurrentThread();
}

CommandSender::CommandSender(ExtendableCallback * apExtendableCallback, Messaging::ExchangeManager * apExchangeMgr,
bool aIsTimedRequest, bool aSuppressResponse) :
mExchangeCtx(*this),
mpExtendableCallback(apExtendableCallback), mpExchangeMgr(apExchangeMgr), mSuppressResponse(aSuppressResponse),
mTimedRequest(aIsTimedRequest)
mCallbackHandle(apExtendableCallback), mpExchangeMgr(apExchangeMgr), mSuppressResponse(aSuppressResponse),
mTimedRequest(aIsTimedRequest), mUseExtendableCallback(true)
{
assertChipStackLockedByCurrentThread();
}
Expand All @@ -86,12 +86,6 @@ CHIP_ERROR CommandSender::AllocateBuffer()
{
if (!mBufferAllocated)
{
// 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 = mpExtendableCallback != nullptr && mpCallback != nullptr;
VerifyOrDie(!bothCallbacksAreSet);

mCommandMessageWriter.Reset();

System::PacketBufferHandle commandPacket = System::PacketBufferHandle::New(chip::app::kMaxSecureSduLengthBytes);
Expand Down Expand Up @@ -417,8 +411,7 @@ CHIP_ERROR CommandSender::ProcessInvokeResponseIB(InvokeResponseIB::Parser & aIn
// 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 usingExtendableCallbacks = mpExtendableCallback != nullptr;
if (statusIB.IsSuccess() || usingExtendableCallbacks)
if (statusIB.IsSuccess() || mUseExtendableCallback)
{
const ConcreteCommandPath concretePath = ConcreteCommandPath(endpointId, clusterId, commandId);
ResponseData responseData = { concretePath, statusIB };
Expand Down Expand Up @@ -456,8 +449,7 @@ 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 usingExtendableCallbacks = mpExtendableCallback != nullptr;
bool canAddAnotherCommand = (mState == State::AddedCommand && mBatchCommandsEnabled && usingExtendableCallbacks);
bool canAddAnotherCommand = (mState == State::AddedCommand && mBatchCommandsEnabled && mUseExtendableCallback);
VerifyOrReturnError(mState == State::Idle || canAddAnotherCommand, CHIP_ERROR_INCORRECT_STATE);
VerifyOrReturnError(mFinishedCommandCount < mRemoteMaxPathsPerInvoke, CHIP_ERROR_MAXIMUM_PATHS_PER_INVOKE_EXCEEDED);

Expand Down
48 changes: 28 additions & 20 deletions src/app/CommandSender.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,7 @@ class CommandSender final : public Messaging::ExchangeDelegate
private:
friend class TestCommandInteraction;

enum class State
enum class State : uint8_t
{
Idle, ///< Default state that the object starts out in, where no work has commenced
AddingCommand, ///< In the process of adding a command.
Expand All @@ -466,6 +466,14 @@ class CommandSender final : public Messaging::ExchangeDelegate
AwaitingDestruction, ///< The object has completed its work and is awaiting destruction by the application.
};

union CallbackHandle
{
CallbackHandle(Callback * apCallback) : legacyCallback(apCallback) {}
CallbackHandle(ExtendableCallback * apExtendableCallback) : extendableCallback(apExtendableCallback) {}
Callback * legacyCallback;
ExtendableCallback * extendableCallback;
};

void MoveToState(const State aTargetState);
const char * GetStateStr() const;

Expand Down Expand Up @@ -513,46 +521,45 @@ class CommandSender final : public Messaging::ExchangeDelegate
void OnResponseCallback(const ResponseData & aResponseData)
{
// mpExtendableCallback and mpCallback are mutually exclusive.
if (mpExtendableCallback)
if (mUseExtendableCallback && mCallbackHandle.extendableCallback)
{
mpExtendableCallback->OnResponse(this, aResponseData);
mCallbackHandle.extendableCallback->OnResponse(this, aResponseData);
}
else if (mpCallback)
else if (mCallbackHandle.legacyCallback)
{
mpCallback->OnResponse(this, aResponseData.path, aResponseData.statusIB, aResponseData.data);
mCallbackHandle.legacyCallback->OnResponse(this, aResponseData.path, aResponseData.statusIB, aResponseData.data);
}
}

void OnErrorCallback(CHIP_ERROR aError)
{
// mpExtendableCallback and mpCallback are mutually exclusive.
if (mpExtendableCallback)
if (mUseExtendableCallback && mCallbackHandle.extendableCallback)
{
ErrorData errorData = { aError };
mpExtendableCallback->OnError(this, errorData);
mCallbackHandle.extendableCallback->OnError(this, errorData);
}
else if (mpCallback)
else if (mCallbackHandle.legacyCallback)
{
mpCallback->OnError(this, aError);
mCallbackHandle.legacyCallback->OnError(this, aError);
}
}

void OnDoneCallback()
{
// mpExtendableCallback and mpCallback are mutually exclusive.
if (mpExtendableCallback)
if (mUseExtendableCallback && mCallbackHandle.extendableCallback)
{
mpExtendableCallback->OnDone(this);
mCallbackHandle.extendableCallback->OnDone(this);
}
else if (mpCallback)
else if (mCallbackHandle.legacyCallback)
{
mpCallback->OnDone(this);
mCallbackHandle.legacyCallback->OnDone(this);
}
}

Messaging::ExchangeHolder mExchangeCtx;
Callback * mpCallback = nullptr;
ExtendableCallback * mpExtendableCallback = nullptr;
CallbackHandle mCallbackHandle;
Messaging::ExchangeManager * mpExchangeMgr = nullptr;
InvokeRequestMessage::Builder mInvokeRequestBuilder;
// TODO Maybe we should change PacketBufferTLVWriter so we can finalize it
Expand All @@ -564,15 +571,16 @@ class CommandSender final : public Messaging::ExchangeDelegate
Optional<uint16_t> mTimedInvokeTimeoutMs;
TLV::TLVType mDataElementContainerType = TLV::kTLVType_NotSpecified;

State mState = State::Idle;
chip::System::PacketBufferTLVWriter mCommandMessageWriter;
uint16_t mFinishedCommandCount = 0;
uint16_t mRemoteMaxPathsPerInvoke = 1;

bool mSuppressResponse = false;
bool mTimedRequest = false;
bool mBufferAllocated = false;
bool mBatchCommandsEnabled = false;
State mState = State::Idle;
bool mSuppressResponse = false;
bool mTimedRequest = false;
bool mBufferAllocated = false;
bool mBatchCommandsEnabled = false;
bool mUseExtendableCallback = false;
};

} // namespace app
Expand Down
Loading