Use the incoming exchange context to send reply messages instead of c…

…reating a new one.

src/app/util/af-main-common.cpp

@@ -671,8 +671,7 @@ EmberStatus emAfSend(EmberOutgoingMessageType type, MessageSendDestination desti
         status = EMBER_ERR_FATAL;
         break;
     case EMBER_OUTGOING_VIA_EXCHANGE:
-        status =
-            chipSendUnicast(destination.mExchangeContext->GetSecureSession().GetPeerNodeId(), apsFrame, messageLength, message);
+        status = chipSendUnicast(destination.mExchangeContext, apsFrame, messageLength, message);
         break;
     default:
         status = EMBER_BAD_ARGUMENT;

src/app/util/chip-message-send.cpp

@@ -51,7 +51,8 @@ class DeviceExchangeDelegate : public Messaging::ExchangeDelegate
 extern Messaging::ExchangeManager * ExchangeManager();
 } // namespace chip
 
-EmberStatus chipSendUnicast(NodeId destination, EmberApsFrame * apsFrame, uint16_t messageLength, uint8_t * message)
+EmberStatus chipSendUnicast(Messaging::ExchangeContext * exchange, EmberApsFrame * apsFrame, uint16_t messageLength,
+                            uint8_t * message, Messaging::SendFlags sendFlags)
 {
     uint16_t frameSize           = encodeApsFrame(nullptr, 0, apsFrame);
     uint32_t dataLengthUnchecked = uint32_t(frameSize) + uint32_t(messageLength);
@@ -86,6 +87,23 @@ EmberStatus chipSendUnicast(NodeId destination, EmberApsFrame * apsFrame, uint16
     memcpy(buffer->Start() + frameSize, message, messageLength);
     buffer->SetDataLength(dataLength);
 
+    // TODO: For some reason if we don't set kNoAutoRequestAck here CRMP gets
+    // very confused and ends up not delivering other messages targeting us as
+    // it should until some sort of timeout happens.  Need to sort out why.
+    sendFlags.Set(Messaging::SendMessageFlags::kNoAutoRequestAck);
+    CHIP_ERROR err = exchange->SendMessage(Protocols::TempZCL::Id, 0, std::move(buffer), sendFlags);
+
+    if (err != CHIP_NO_ERROR)
+    {
+        // FIXME: Figure out better translations between our error types?
+        return EMBER_DELIVERY_FAILED;
+    }
+
+    return EMBER_SUCCESS;
+}
+
+EmberStatus chipSendUnicast(NodeId destination, EmberApsFrame * apsFrame, uint16_t messageLength, uint8_t * message)
+{
     // TODO: temporary create a handle from node id, will be fix in PR 3602
     Messaging::ExchangeManager * exchangeMgr = ExchangeManager();
     if (exchangeMgr == nullptr)
@@ -106,18 +124,15 @@ EmberStatus chipSendUnicast(NodeId destination, EmberApsFrame * apsFrame, uint16
     // receive the ack message. This logic needs to be deleted after we convert all legacy ZCL messages to IM messages.
     DeviceExchangeDelegate delegate;
     exchange->SetDelegate(&delegate);
+
     Messaging::SendFlags sendFlags;
 
     sendFlags.Set(Messaging::SendMessageFlags::kFromInitiator).Set(Messaging::SendMessageFlags::kNoAutoRequestAck);
-    CHIP_ERROR err = exchange->SendMessage(Protocols::TempZCL::Id, 0, std::move(buffer), sendFlags);
 
-    exchange->Close();
+    EmberStatus err = chipSendUnicast(exchange, apsFrame, messageLength, message, sendFlags);
 
-    if (err != CHIP_NO_ERROR)
-    {
-        // FIXME: Figure out better translations between our error types?
-        return EMBER_DELIVERY_FAILED;
-    }
+    // Make sure we always close the temporary exchange we just created.
+    exchange->Close();
 
-    return EMBER_SUCCESS;
+    return err;
 }

src/app/util/chip-message-send.h

@@ -25,6 +25,24 @@
 #pragma once
 
 #include <app/util/types_stub.h> // For EmberApsFrame, EmberStatus, node ids.
+#include <messaging/ExchangeContext.h>
+#include <messaging/Flags.h>
+
+/**
+ * @brief
+ *    Called to send a unicast message on the given exchange.  The message is
+ *    constructed by serializing the given APS frame followed by the actual
+ *    message buffer passed in.
+ *
+ * @param[in] exchange The exchange to send the message on.
+ * @param[in] apsFrame The APS frame to use for the message.
+ * @param[in] messageLength The length of the message to send after the APS
+ *                          frame.
+ * @param[in] message The message to send after the APS frame.
+ * @param[in] sendFlags The SendFlags needed, if any.
+ */
+EmberStatus chipSendUnicast(chip::Messaging::ExchangeContext * exchange, EmberApsFrame * apsFrame, uint16_t messageLength,
+                            uint8_t * message, chip::Messaging::SendFlags sendFlags = chip::Messaging::SendFlags());
 
 /**
  * @brief

src/controller/CHIPDevice.cpp

@@ -55,16 +55,6 @@ using namespace chip::Callback;
 
 namespace chip {
 namespace Controller {
-// TODO: This is a placeholder delegate for exchange context created in Device::SendMessage()
-//       Delete this class when Device::SendMessage() is obsoleted.
-class DeviceExchangeDelegate : public Messaging::ExchangeDelegate
-{
-    void OnMessageReceived(Messaging::ExchangeContext * ec, const PacketHeader & packetHeader, const PayloadHeader & payloadHeader,
-                           System::PacketBufferHandle payload) override
-    {}
-    void OnResponseTimeout(Messaging::ExchangeContext * ec) override {}
-};
-
 CHIP_ERROR Device::SendMessage(Protocols::Id protocolId, uint8_t msgType, System::PacketBufferHandle buffer)
 {
     System::PacketBufferHandle resend;
@@ -88,15 +78,10 @@ CHIP_ERROR Device::SendMessage(Protocols::Id protocolId, uint8_t msgType, System
         resend = buffer.CloneData();
     }
 
-    // TODO(#5675): This code is temporary, and must be updated to use the IM API. Currenlty, we use a temporary Protocol
-    // TempZCL to carry over legacy ZCL messages, use an ephemeral exchange to send message and use its unsolicited message
-    // handler to receive messages. We need to set flag kFromInitiator to allow receiver to deliver message to corresponding
-    // unsolicited message handler, and we also need to set flag kNoAutoRequestAck since there is no persistent exchange to
-    // receive the ack message. This logic need to be deleted after we converting all legacy ZCL messages to IM messages.
+    // TODO(#5675): This code is temporary, and must be updated to use the IM API. Currently, we use a temporary Protocol
+    // TempZCL to carry over legacy ZCL messages.
     sendFlags.Set(Messaging::SendMessageFlags::kFromInitiator).Set(Messaging::SendMessageFlags::kNoAutoRequestAck);
-
-    DeviceExchangeDelegate delegate;
-    exchange->SetDelegate(&delegate);
+    exchange->SetDelegate(this);
 
     CHIP_ERROR err = exchange->SendMessage(protocolId, msgType, std::move(buffer), sendFlags);
 
@@ -112,16 +97,15 @@ CHIP_ERROR Device::SendMessage(Protocols::Id protocolId, uint8_t msgType, System
         ReturnErrorOnFailure(LoadSecureSessionParameters(ResetTransport::kYes));
 
         err = exchange->SendMessage(protocolId, msgType, std::move(resend), sendFlags);
-        ChipLogDetail(Controller, "Re-SendMessage returned %d", err);
-        ReturnErrorOnFailure(err);
+        ChipLogDetail(Controller, "Re-SendMessage returned %s", ErrorStr(err));
     }
 
-    if (exchange != nullptr)
+    if (err != CHIP_NO_ERROR)
     {
         exchange->Close();
     }
 
-    return CHIP_NO_ERROR;
+    return err;
 }
 
 CHIP_ERROR Device::LoadSecureSessionParametersIfNeeded(bool & didLoad)
@@ -290,6 +274,12 @@ void Device::OnMessageReceived(Messaging::ExchangeContext * exchange, const Pack
             HandleDataModelMessage(exchange, std::move(msgBuf));
         }
     }
+    exchange->Close();
+}
+
+void Device::OnResponseTimeout(Messaging::ExchangeContext * ec)
+{
+    ec->Close();
 }
 
 CHIP_ERROR Device::OpenPairingWindow(uint32_t timeout, PairingWindowOption option, SetupPayload & setupPayload)

src/controller/CHIPDevice.h

@@ -33,6 +33,7 @@
 #include <core/CHIPCallback.h>
 #include <core/CHIPCore.h>
 #include <messaging/ExchangeContext.h>
+#include <messaging/ExchangeDelegate.h>
 #include <messaging/ExchangeMgr.h>
 #include <protocols/secure_channel/PASESession.h>
 #include <setup_payload/SetupPayload.h>
@@ -79,11 +80,19 @@ struct ControllerDeviceInitParams
 #endif
 };
 
-class DLL_EXPORT Device
+class DLL_EXPORT Device : public Messaging::ExchangeDelegate
 {
 public:
     ~Device()
     {
+        if (mExchangeMgr)
+        {
+            // Ensure that any exchange contexts we have open get closed now,
+            // because we don't want them to call back in to us after this
+            // point.
+            mExchangeMgr->CloseAllContextsForDelegate(this);
+        }
+
         if (mCommandSender != nullptr)
         {
             mCommandSender->Shutdown();
@@ -237,13 +246,21 @@ class DLL_EXPORT Device
      *   device. The message ownership is transferred to the function, and it is expected
      *   to release the message buffer before returning.
      *
-     * @param[in] exchange      The exchange context the message was received on.
+     * @param[in] exchange      The exchange context the message was received
+     *                          on.  The Device guarantees that it will call
+     *                          Close() on exchange when it's done processing
+     *                          the message.
      * @param[in] header        Reference to common packet header of the received message
      * @param[in] payloadHeader Reference to payload header in the message
      * @param[in] msgBuf        The message buffer
      */
     void OnMessageReceived(Messaging::ExchangeContext * exchange, const PacketHeader & header, const PayloadHeader & payloadHeader,
-                           System::PacketBufferHandle msgBuf);
+                           System::PacketBufferHandle msgBuf) override;
+
+    /**
+     * @brief ExchangeDelegate implementation of OnResponseTimeout.
+     */
+    void OnResponseTimeout(Messaging::ExchangeContext * exchange) override;
 
     /**
      * @brief

src/controller/CHIPDeviceController.cpp

@@ -413,6 +413,7 @@ void DeviceController::OnMessageReceived(Messaging::ExchangeContext * ec, const
                                          const PayloadHeader & payloadHeader, System::PacketBufferHandle msgBuf)
 {
     uint16_t index;
+    bool needClose = true;
 
     VerifyOrExit(mState == State::Initialized, ChipLogError(Controller, "OnMessageReceived was called in incorrect state"));
 
@@ -422,10 +423,14 @@ void DeviceController::OnMessageReceived(Messaging::ExchangeContext * ec, const
     index = FindDeviceIndex(packetHeader.GetSourceNodeId().Value());
     VerifyOrExit(index < kNumMaxActiveDevices, ChipLogError(Controller, "OnMessageReceived was called for unknown device object"));
 
+    needClose = false; // Device will handle it
     mActiveDevices[index].OnMessageReceived(ec, packetHeader, payloadHeader, std::move(msgBuf));
 
 exit:
-    ec->Close();
+    if (needClose)
+    {
+        ec->Close();
+    }
 }
 
 void DeviceController::OnResponseTimeout(Messaging::ExchangeContext * ec)