[ncp] enhance handling of outbound IP message over spinel (to host) (#2259)

This commit changes how the outbound datagram IPv6 messages (going
from NCP to host over spinel) are handled. If NCP spinel buffer is
full, new outbound messages are saved in a message queue and are
transfered later (as soon as buffer space becomes available). This
ensures that outbound IPv6 messages are never dropped due to spinel
NCP buffer being full.

To realize this, the following changes are made in this commit: (a)
The behavior of methods `NcpFrameBuffer::InFrameFeedMessage()` and
`SpinelEncoder::WriteMessage()` is changed so that the passed-in
message ownership changes to NCP buffer ONLY when the entire spinel
frame is written and the frame is successfully ended/finished. If the
spinel frame gets discarded (e.g. no buffer space) the caller continues
to own the message instance and should either free or save it. (b)
The unit test `test_ncp_buffer` is changed to address and check for
this change. (c) The `NcpBase::HandleDatagramFromStack()` is modified
to implement/use a message queue to save outbound messages and try again
to send the queued messages from `HandleFrameRemovedFromNcpBuffer()`
callback (when spinel buffer becomes available).

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:

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.

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)

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
 

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;

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

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

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