diff --git a/src/ncp/ncp_base.cpp b/src/ncp/ncp_base.cpp index 768dc82c8..482dff3bf 100644 --- a/src/ncp/ncp_base.cpp +++ b/src/ncp/ncp_base.cpp @@ -554,6 +554,7 @@ NcpBase::NcpBase(otInstance *aInstance): mTxFrameBuffer.SetFrameRemovedCallback(&NcpBase::HandleFrameRemovedFromNcpBuffer, this); #if OPENTHREAD_MTD || OPENTHREAD_FTD + otMessageQueueInit(&mMessageQueue); otSetStateChangedCallback(mInstance, &NcpBase::HandleNetifStateChanged, this); otIp6SetReceiveCallback(mInstance, &NcpBase::HandleDatagramFromStack, this); otIp6SetReceiveFilterEnabled(mInstance, true); @@ -711,6 +712,10 @@ void NcpBase::HandleFrameRemovedFromNcpBuffer(NcpFrameBuffer::FrameTag aFrameTag } } +#if OPENTHREAD_MTD || OPENTHREAD_FTD + SuccessOrExit(SendQueuedDatagramMessages()); +#endif + UpdateChangedProps(); exit: diff --git a/src/ncp/ncp_base.hpp b/src/ncp/ncp_base.hpp index 4c55b7935..7ac5948c9 100644 --- a/src/ncp/ncp_base.hpp +++ b/src/ncp/ncp_base.hpp @@ -219,6 +219,9 @@ private: static void HandleDatagramFromStack(otMessage *aMessage, void *aContext); void HandleDatagramFromStack(otMessage *aMessage); + otError SendQueuedDatagramMessages(void); + otError SendDatagramMessage(otMessage *aMessage); + static void HandleActiveScanResult_Jump(otActiveScanResult *aResult, void *aContext); void HandleActiveScanResult(otActiveScanResult *aResult); @@ -649,6 +652,8 @@ private: #endif // OPENTHREAD_ENABLE_RAW_LINK_API #if OPENTHREAD_MTD || OPENTHREAD_FTD + otMessageQueue mMessageQueue; + uint32_t mInboundSecureIpFrameCounter; // Number of secure inbound data/IP frames. uint32_t mInboundInsecureIpFrameCounter; // Number of insecure inbound data/IP frames. uint32_t mOutboundSecureIpFrameCounter; // Number of secure outbound data/IP frames. diff --git a/src/ncp/ncp_base_ftd.cpp b/src/ncp/ncp_base_ftd.cpp index 83c7efe0a..eb0a6f748 100644 --- a/src/ncp/ncp_base_ftd.cpp +++ b/src/ncp/ncp_base_ftd.cpp @@ -470,17 +470,16 @@ void NcpBase::HandleTmfProxyStream(otMessage *aMessage, uint16_t aLocator, uint1 SuccessOrExit(error = mEncoder.WriteUint16(length)); SuccessOrExit(error = mEncoder.WriteMessage(aMessage)); - // Set the `aMessage` pointer to NULL to indicate that it does - // not need to be freed at the exit. The `aMessage` is now owned - // by the outbound frame and will be freed when the frame is either - // successfully sent and then removed, or if the frame gets - // discarded. - aMessage = NULL; - SuccessOrExit(error = mEncoder.WriteUint16(aLocator)); SuccessOrExit(error = mEncoder.WriteUint16(aPort)); SuccessOrExit(error = mEncoder.EndFrame()); + // The `aMessage` is owned by the outbound frame and NCP buffer + // after frame was finished/ended successfully. It will be freed + // when the frame is successfully sent and removed. + + aMessage = NULL; + exit: if (aMessage != NULL) diff --git a/src/ncp/ncp_base_mtd.cpp b/src/ncp/ncp_base_mtd.cpp index 1fef0240b..2188441c5 100644 --- a/src/ncp/ncp_base_mtd.cpp +++ b/src/ncp/ncp_base_mtd.cpp @@ -2549,58 +2549,76 @@ void NcpBase::HandleDatagramFromStack(otMessage *aMessage, void *aContext) } void NcpBase::HandleDatagramFromStack(otMessage *aMessage) +{ + VerifyOrExit(aMessage != NULL); + + SuccessOrExit(otMessageQueueEnqueue(&mMessageQueue, aMessage)); + SuccessOrExit(SendQueuedDatagramMessages()); + +exit: + // If the queued message can not be sent now (out of buffer), + // it will be sent once spinel buffer becomes available from + // `HandleFrameRemovedFromNcpBuffer()` callback. + + return; +} + +otError NcpBase::SendDatagramMessage(otMessage *aMessage) { otError error = OT_ERROR_NONE; uint8_t header = SPINEL_HEADER_FLAG | SPINEL_HEADER_IID_0; bool isSecure = otMessageIsLinkSecurityEnabled(aMessage); - uint16_t length = otMessageGetLength(aMessage); + spinel_prop_key_t propKey = isSecure ? SPINEL_PROP_STREAM_NET : SPINEL_PROP_STREAM_NET_INSECURE; - SuccessOrExit(error = mEncoder.BeginFrame( - header, - SPINEL_CMD_PROP_VALUE_IS, - isSecure ? SPINEL_PROP_STREAM_NET : SPINEL_PROP_STREAM_NET_INSECURE - )); - - SuccessOrExit(error = mEncoder.WriteUint16(length)); + SuccessOrExit(error = mEncoder.BeginFrame(header, SPINEL_CMD_PROP_VALUE_IS, propKey)); + SuccessOrExit(error = mEncoder.WriteUint16(otMessageGetLength(aMessage))); SuccessOrExit(error = mEncoder.WriteMessage(aMessage)); - // Set the `aMessage` pointer to NULL to indicate that it does - // not need to be freed at the exit. The `aMessage` is now owned - // by the OutboundFrame and will be freed when the frame is either - // successfully sent and then removed, or if the frame gets - // discarded. - aMessage = NULL; - // Append any metadata (rssi, lqi, channel, etc) here! SuccessOrExit(error = mEncoder.EndFrame()); -exit: - - if (aMessage != NULL) + if (isSecure) { - otMessageFree(aMessage); - } - - if (error != OT_ERROR_NONE) - { - mChangedPropsSet.AddLastStatus(SPINEL_STATUS_DROPPED); - mUpdateChangedPropsTask.Post(); - mDroppedOutboundIpFrameCounter++; + mOutboundSecureIpFrameCounter++; } else { - if (isSecure) - { - mOutboundSecureIpFrameCounter++; - } - else - { - mOutboundInsecureIpFrameCounter++; - } + mOutboundInsecureIpFrameCounter++; } + +exit: + return error; } +otError NcpBase::SendQueuedDatagramMessages(void) +{ + otError error = OT_ERROR_NONE; + otMessage *message; + + while ((message = otMessageQueueGetHead(&mMessageQueue)) != NULL) + { + // Since an `otMessage` instance can be in one queue at a time, + // it is first dequeued from `mMessageQueue` before attempting + // to include it in a spinel frame by calling `SendDatagramMessage()` + // If forming of the spinel frame fails, the message is enqueued + // back at the front of `mMessageQueue`. + + otMessageQueueDequeue(&mMessageQueue, message); + + error = SendDatagramMessage(message); + + if (error != OT_ERROR_NONE) + { + otMessageQueueEnqueueAtHead(&mMessageQueue, message); + } + + SuccessOrExit(error); + } + +exit: + return error; +} // ---------------------------------------------------------------------------- // MARK: Property/Status Changed @@ -2702,7 +2720,6 @@ exit: return; } - } // namespace Ncp } // namespace ot diff --git a/src/ncp/ncp_buffer.cpp b/src/ncp/ncp_buffer.cpp index 49ef17578..69d6d2340 100644 --- a/src/ncp/ncp_buffer.cpp +++ b/src/ncp/ncp_buffer.cpp @@ -90,7 +90,10 @@ void NcpFrameBuffer::Clear(void) while ((message = otMessageQueueGetHead(&mWriteFrameMessageQueue)) != NULL) { otMessageQueueDequeue(&mWriteFrameMessageQueue, message); - otMessageFree(message); + + // Note that messages associated with current (unfinished) input frame + // are not yet owned by the `NcpFrameBuffer` and therefore should not + // be freed. } for (uint8_t priority = 0; priority < kNumPrios; priority++) @@ -304,11 +307,13 @@ void NcpFrameBuffer::InFrameDiscard(void) // Move the write segment head and tail pointers back to frame start. mWriteSegmentHead = mWriteSegmentTail = mWriteFrameStart[mWriteDirection]; - // Free any messages associated with current frame. while ((message = otMessageQueueGetHead(&mWriteFrameMessageQueue)) != NULL) { otMessageQueueDequeue(&mWriteFrameMessageQueue, message); - otMessageFree(message); + + // Note that messages associated with current (unfinished) input frame + // being discarded, are not yet owned by the `NcpFrameBuffer` and + // therefore should not be freed. } mWriteDirection = kUnknown; diff --git a/src/ncp/ncp_buffer.hpp b/src/ncp/ncp_buffer.hpp index 2fdacf4bb..78a42e1a6 100644 --- a/src/ncp/ncp_buffer.hpp +++ b/src/ncp/ncp_buffer.hpp @@ -209,8 +209,11 @@ public: * If no buffer space is available, this method will discard and clear the frame and return error status * `OT_ERROR_NO_BUFS`. * - * In case of success, the passed-in message @p aMessage will be owned by the frame buffer instance and will be - * freed when either the frame is removed or discarded. In case of failure @p aMessage remains unchanged. + * The ownership of the passed-in message @p aMessage changes to `NcpFrameBuffer` ONLY when the entire frame is + * successfully finished (i.e., with a successful call to `InFrameEnd()` for the current input frame), and in this + * case the `otMessage` instance will be freed once the frame is removed (using `OutFrameRemove()`) from NCP buffer. + * However, if the input frame gets discarded before it is finished (e.g., running out of buffer space), the + * `otMessage` instance remains unchanged. * * @param[in] aMessage A message to be added to current frame. * diff --git a/src/ncp/spinel_encoder.hpp b/src/ncp/spinel_encoder.hpp index fe40f80e1..cf1def941 100644 --- a/src/ncp/spinel_encoder.hpp +++ b/src/ncp/spinel_encoder.hpp @@ -508,8 +508,11 @@ public: * If no buffer space is available, this method will discard and clear the frame and return error status * `OT_ERROR_NO_BUFS`. * - * In case of success, the passed-in message @p aMessage will be owned by the frame buffer instance and will be - * freed when either the the frame is removed or discarded. In case of failure @p aMessage remains unchanged. + * The ownership of the passed-in message @p aMessage changes to underlying `NcpFrameBuffer` ONLY when the entire + * frame is successfully finished (i.e., with a successful call to `EndFrame()` for the current frame being written), + * and in this case the `otMessage` instance will be freed once the frame is removed from the `NcpFrameBuffer`. + * However, if the frame gets discarded before it is finished (e.g., running out of buffer space), the `otMessage` + * instance remains unchanged. * * @param[in] aMessage A message to be added to current frame. * diff --git a/tests/unit/test_ncp_buffer.cpp b/tests/unit/test_ncp_buffer.cpp index 092f5be8d..b3ab4eb95 100644 --- a/tests/unit/test_ncp_buffer.cpp +++ b/tests/unit/test_ncp_buffer.cpp @@ -574,6 +574,10 @@ void TestNcpFrameBuffer(void) WriteTestFrame1(ncpBuffer, frame1IsHighPriority ? NcpFrameBuffer::kPriorityHigh : NcpFrameBuffer::kPriorityLow); + // Note that message will not be freed by the NCP buffer since the frame associated with it was discarded and + // not yet finished/ended. + otMessageFree(message); + VerifyAndRemoveFrame3(ncpBuffer); // Start reading few bytes from the frame