Union pointers to save space in CommandSender (#31609)

* Union pointers to save space in CommandSender

* Restyled by clang-format

---------

Co-authored-by: Restyled.io <commits@restyled.io>

src/app/CommandSender.cpp

@@ -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();
 }
@@ -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);
 

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<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