From 5869c1e5399f06af74cf2caf7cf71424980c013e Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Mon, 3 May 2021 18:42:30 -0400 Subject: [PATCH] Use the incoming exchange context to send reply messages instead of creating a new one. --- src/app/util/af-main-common.cpp | 3 +-- src/app/util/chip-message-send.cpp | 33 +++++++++++++++++------- src/app/util/chip-message-send.h | 18 +++++++++++++ src/controller/CHIPDevice.cpp | 34 +++++++++---------------- src/controller/CHIPDevice.h | 23 ++++++++++++++--- src/controller/CHIPDeviceController.cpp | 7 ++++- 6 files changed, 81 insertions(+), 37 deletions(-) diff --git a/src/app/util/af-main-common.cpp b/src/app/util/af-main-common.cpp index 00488903befc4b..6ffd8cd5983822 100644 --- a/src/app/util/af-main-common.cpp +++ b/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; diff --git a/src/app/util/chip-message-send.cpp b/src/app/util/chip-message-send.cpp index 8fd9ba88a20bd3..5ab6dfd5d74cb4 100644 --- a/src/app/util/chip-message-send.cpp +++ b/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; } diff --git a/src/app/util/chip-message-send.h b/src/app/util/chip-message-send.h index cfbc5987264f45..ca429785eb360b 100644 --- a/src/app/util/chip-message-send.h +++ b/src/app/util/chip-message-send.h @@ -25,6 +25,24 @@ #pragma once #include // For EmberApsFrame, EmberStatus, node ids. +#include +#include + +/** + * @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 diff --git a/src/controller/CHIPDevice.cpp b/src/controller/CHIPDevice.cpp index f92931a29b20fe..ffbb7b73b222bf 100644 --- a/src/controller/CHIPDevice.cpp +++ b/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) diff --git a/src/controller/CHIPDevice.h b/src/controller/CHIPDevice.h index dd30724ef50fdf..195c5c13217932 100644 --- a/src/controller/CHIPDevice.h +++ b/src/controller/CHIPDevice.h @@ -33,6 +33,7 @@ #include #include #include +#include #include #include #include @@ -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 diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index dbce37a31c1027..2705e35377ef1d 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/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)