From 166c4b5eeda39f8d4e57fe18779d676bbe395829 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Tue, 23 Jan 2024 10:22:21 -0500 Subject: [PATCH] Union pointers to save space in `CommandSender` (#31609) * Union pointers to save space in CommandSender * Restyled by clang-format --------- Co-authored-by: Restyled.io --- src/app/CommandSender.cpp | 18 ++++----------- src/app/CommandSender.h | 48 +++++++++++++++++++++++---------------- 2 files changed, 33 insertions(+), 33 deletions(-) diff --git a/src/app/CommandSender.cpp b/src/app/CommandSender.cpp index 6ab19b2ebd2276..1066ca92825891 100644 --- a/src/app/CommandSender.cpp +++ b/src/app/CommandSender.cpp @@ -63,7 +63,7 @@ CHIP_ERROR GetRef(ParserT aParser, Optional & 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(); } @@ -71,8 +71,8 @@ CommandSender::CommandSender(Callback * apCallback, Messaging::ExchangeManager * 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(); } @@ -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); @@ -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 }; @@ -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); diff --git a/src/app/CommandSender.h b/src/app/CommandSender.h index dfcd3e1a62a5f0..41ec5befe3e40a 100644 --- a/src/app/CommandSender.h +++ b/src/app/CommandSender.h @@ -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. @@ -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; @@ -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 @@ -564,15 +571,16 @@ class CommandSender final : public Messaging::ExchangeDelegate Optional 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