Update NcpFrameBuffer callbacks, add NCP frame tags, update unit test (#1799)

This commit contains the following changes to the `NcpFrameBuffer`
class:

It updates `NcpFrameBuffer` callbacks by adding `FrameAddedCallback`
and `FrameRemovedCallback` which are respectively invoked when a frame
is added/removed to/from the NCP buffer.

It also adds new concept of NCP frame tag which is a unique (within
the queued NCP frames) value associated with every successfully
queued/added frame in `NcpFrameBufffer`. The tags are passed in the
callbacks to indicate the frame that was added or removed.  New
methods are added to get the tag of the frame at front/top of the
queue and the tag associated with last (successfully) added frame. The
tags are used to track when a NCP frame is removed, which enable us to
detect when the reply to a specific spinel exchange is sent (e.g.,
this is used to track the reply to `SPINEL_HOST_POWER_STATE_ONLINE`
property).

The `NcpBase`, `NcpSpi`, and `NcpUart` implementation are modified to
use the new callbacks and tag related methods.

This commit also updates the `test_ncp_buffer` unit test:

- Additional checks are added to test the new callbacks behavior

- Tests are updated to verify the tag related methods and their
  functionality.

- A new fuzz test is added for NCP buffer where over many iteration, a
  random number of frame with random content and random length are
  added to the NCP frame buffer. It is then verified that frames with
  same content are read back.

src/ncp/ncp_base.cpp

@@ -570,6 +570,7 @@ NcpBase::NcpBase(otInstance *aInstance):
     mShouldSignalEndOfScan(false),
     mHostPowerState(SPINEL_HOST_POWER_STATE_ONLINE),
     mHostPowerStateInProgress(false),
+    mHostPowerReplyFrameTag(NcpFrameBuffer::kInvalidTag),
     mHostPowerStateHeader(0),
 #if OPENTHREAD_ENABLE_JAM_DETECTION
     mShouldSignalJamStateChange(false),
@@ -602,6 +603,8 @@ NcpBase::NcpBase(otInstance *aInstance):
 
     sNcpInstance = this;
 
+    mTxFrameBuffer.SetFrameRemovedCallback(&NcpBase::HandleFrameRemovedFromNcpBuffer, this);
+
     otSetStateChangedCallback(mInstance, &NcpBase::HandleNetifStateChanged, this);
     otIp6SetReceiveCallback(mInstance, &NcpBase::HandleDatagramFromStack, this);
     otIp6SetReceiveFilterEnabled(mInstance, true);
@@ -646,6 +649,11 @@ otError NcpBase::OutboundFrameEnd(void)
     return mTxFrameBuffer.InFrameEnd();
 }
 
+NcpFrameBuffer::FrameTag NcpBase::GetLastOutboundFrameTag(void)
+{
+    return mTxFrameBuffer.InFrameGetLastTag();
+}
+
 #if OPENTHREAD_ENABLE_BORDER_AGENT_PROXY && OPENTHREAD_FTD
 void NcpBase::HandleBorderAgentProxyStream(otMessage *aMessage, uint16_t aLocator, uint16_t aPort, void *aContext)
 {
@@ -1245,19 +1253,6 @@ exit:
 // MARK: Serial Traffic Glue
 // ----------------------------------------------------------------------------
 
-void NcpBase::HandleFrameTransmitDone(void *aContext, otError aError)
-{
-    NcpBase *obj = static_cast<NcpBase *>(aContext);
-    obj->HandleFrameTransmitDone(aError);
-}
-
-void NcpBase::HandleFrameTransmitDone(otError aError)
-{
-    (void) aError;
-
-    mHostPowerStateInProgress = false;
-}
-
 otError NcpBase::OutboundFrameSend(void)
 {
     otError errorCode;
@@ -1286,7 +1281,6 @@ void NcpBase::HandleReceive(const uint8_t *buf, uint16_t bufLength)
     // Receiving any message from the host has the side effect of transitioning the host power state to online.
     mHostPowerState = SPINEL_HOST_POWER_STATE_ONLINE;
     mHostPowerStateInProgress = false;
-    mTxFrameBuffer.SetFrameTransmitCallback(NULL, NULL);
 
     if (parsedLength == bufLength)
     {
@@ -1333,8 +1327,24 @@ void NcpBase::HandleReceive(const uint8_t *buf, uint16_t bufLength)
     mRxSpinelFrameCounter++;
 }
 
-void NcpBase::HandleSpaceAvailableInTxBuffer(void)
+void NcpBase::HandleFrameRemovedFromNcpBuffer(void *aContext, NcpFrameBuffer::FrameTag aFrameTag, NcpFrameBuffer *aNcpBuffer)
 {
+    (void)aNcpBuffer;
+    static_cast<NcpBase *>(aContext)->HandleFrameRemovedFromNcpBuffer(aFrameTag);
+}
+
+void NcpBase::HandleFrameRemovedFromNcpBuffer(NcpFrameBuffer::FrameTag aFrameTag)
+{
+    if (mHostPowerStateInProgress == true)
+    {
+        if (aFrameTag == mHostPowerReplyFrameTag)
+        {
+            mHostPowerStateInProgress = false;
+        }
+    }
+
+    // Space is now available in ncp tx frame buffer.
+
     while (mDroppedReplyTid != 0)
     {
         SuccessOrExit(
@@ -1402,7 +1412,7 @@ void NcpBase::HandleSpaceAvailableInTxBuffer(void)
 
         if (mHostPowerState != SPINEL_HOST_POWER_STATE_ONLINE)
         {
-            mTxFrameBuffer.SetFrameTransmitCallback(&NcpBase::HandleFrameTransmitDone, this);
+            mHostPowerReplyFrameTag = GetLastOutboundFrameTag();
             mHostPowerStateInProgress = true;
         }
     }
@@ -3793,7 +3803,11 @@ otError NcpBase::SetPropertyHandler_HOST_POWER_STATE(uint8_t header, spinel_prop
         {
             if (errorCode == OT_ERROR_NONE)
             {
-                mTxFrameBuffer.SetFrameTransmitCallback(&NcpBase::HandleFrameTransmitDone, this);
+                mHostPowerReplyFrameTag = GetLastOutboundFrameTag();
+            }
+            else
+            {
+                mHostPowerReplyFrameTag = NcpFrameBuffer::kInvalidTag;
             }
 
             mHostPowerStateInProgress = true;
@@ -3802,6 +3816,12 @@ otError NcpBase::SetPropertyHandler_HOST_POWER_STATE(uint8_t header, spinel_prop
         if (errorCode != OT_ERROR_NONE)
         {
             mHostPowerStateHeader = header;
+
+            // The reply will be queued when buffer space becomes available
+            // in the NCP tx buffer so we return `success` to avoid sending a
+            // NOMEM status for the same tid through `mDroppedReplyTid` list.
+
+            errorCode = OT_ERROR_NONE;
         }
     }
     else

src/ncp/ncp_base.hpp

@@ -135,11 +135,6 @@ protected:
      */
     void HandleReceive(const uint8_t *buf, uint16_t bufLength);
 
-    /**
-     * Called by the subclass to indicate when a frame was removed and some space in tx buffer is available.
-     */
-    void HandleSpaceAvailableInTxBuffer(void);
-
     /**
      * Called by the subclass to learn when the host wake operation must be issued.
      */
@@ -154,6 +149,8 @@ private:
 
     otError OutboundFrameSend(void);
 
+    NcpFrameBuffer::FrameTag GetLastOutboundFrameTag(void);
+
 #if OPENTHREAD_ENABLE_BORDER_AGENT_PROXY && OPENTHREAD_FTD
     /**
      * Trampoline for HandleBorderAgentProxyStream().
@@ -163,6 +160,14 @@ private:
     void HandleBorderAgentProxyStream(otMessage *aMessage, uint16_t aLocator, uint16_t aPort);
 #endif // OPENTHREAD_ENABLE_BORDER_AGENT_PROXY && OPENTHREAD_FTD
 
+    /**
+     * Trampoline for NcpFrameBuffer FrameRemoved Callback.
+     */
+    static void HandleFrameRemovedFromNcpBuffer(void *aContext, NcpFrameBuffer::FrameTag aFrameTag,
+                                                NcpFrameBuffer *aNcpBuffer);
+
+    void HandleFrameRemovedFromNcpBuffer(NcpFrameBuffer::FrameTag aFrameTag);
+
     /**
      * Trampoline for HandleDatagramFromStack().
      */
@@ -240,10 +245,6 @@ private:
 
     static void HandleNetifStateChanged(uint32_t flags, void *context);
 
-    static void HandleFrameTransmitDone(void *aContext, otError aError);
-
-    void HandleFrameTransmitDone(otError aError);
-
 private:
 
     otError OutboundFrameFeedPacked(const char *pack_format, ...);
@@ -679,6 +680,7 @@ private:
     bool mShouldSignalEndOfScan;
     spinel_host_power_state_t mHostPowerState;
     bool mHostPowerStateInProgress;
+    NcpFrameBuffer::FrameTag mHostPowerReplyFrameTag;
     uint8_t mHostPowerStateHeader;
 
 #if OPENTHREAD_ENABLE_JAM_DETECTION

src/ncp/ncp_buffer.cpp

@@ -37,13 +37,13 @@
 #endif
 
 #include "ncp_buffer.hpp"
-
 #include "utils/wrap_string.h"
-
 #include "common/code_utils.hpp"
 
 namespace ot {
 
+const NcpFrameBuffer::FrameTag NcpFrameBuffer::kInvalidTag = NULL;
+
 NcpFrameBuffer::NcpFrameBuffer(uint8_t *aBuffer, uint16_t aBufferLen) :
     mBuffer(aBuffer),
     mBufferEnd(aBuffer + aBufferLen),
@@ -51,26 +51,20 @@ NcpFrameBuffer::NcpFrameBuffer(uint8_t *aBuffer, uint16_t aBufferLen) :
 {
     otMessageQueueInit(&mMessageQueue);
     otMessageQueueInit(&mWriteFrameMessageQueue);
-    SetCallbacks(NULL, NULL, NULL);
+    SetFrameAddedCallback(NULL, NULL);
-    SetFrameTransmitCallback(NULL, NULL);
+    SetFrameRemovedCallback(NULL, NULL);
-    Clear();
-}
-
-NcpFrameBuffer::~NcpFrameBuffer()
-{
-    SetCallbacks(NULL, NULL, NULL);
     Clear();
 }
 
 void NcpFrameBuffer::Clear(void)
 {
     otMessage *message;
-    bool wasEmpty = IsEmpty();
 
     // Write (InFrame) related variables
     mWriteFrameStart = mBuffer;
     mWriteSegmentHead = mBuffer;
     mWriteSegmentTail = mBuffer;
+    mWriteFrameTag = kInvalidTag;
 
     // Read (OutFrame) related variables
     mReadState = kReadStateDone;
@@ -98,57 +92,21 @@ void NcpFrameBuffer::Clear(void)
         otMessageQueueDequeue(&mMessageQueue, message);
         otMessageFree(message);
     }
-
-    if (!wasEmpty)
-    {
-        if (mEmptyBufferCallback != NULL)
-        {
-            mEmptyBufferCallback(mEmptyBufferCallbackContext, this);
-        }
-    }
-
-    if (mFrameTransmitMark == mReadFrameStart)
-    {
-        if (mFrameTransmitCallback != NULL)
-        {
-            mFrameTransmitCallback(mFrameTransmitContext, OT_ERROR_ABORT);
-            mFrameTransmitCallback = NULL;
-            mFrameTransmitMark = NULL;
-        }
-    }
 }
 
-void NcpFrameBuffer::SetCallbacks(BufferCallback aEmptyBufferCallback, BufferCallback aNonEmptyBufferCallback,
+void NcpFrameBuffer::SetFrameAddedCallback(BufferCallback aFrameAddedCallback, void *aFrameAddedContext)
-                                  void *aContext)
 {
-    mEmptyBufferCallback = aEmptyBufferCallback;
+    mFrameAddedCallback = aFrameAddedCallback;
-    mNonEmptyBufferCallback = aNonEmptyBufferCallback;
+    mFrameAddedContext = aFrameAddedContext;
-    mEmptyBufferCallbackContext = aContext;
 }
 
-otError NcpFrameBuffer::SetFrameTransmitCallback(FrameTransmitCallback aFrameTransmitCallback, void *aContext)
+void NcpFrameBuffer::SetFrameRemovedCallback(BufferCallback aFrameRemovedCallback, void *aFrameRemovedContext)
 {
-    otError error = OT_ERROR_NONE;
+    mFrameRemovedCallback = aFrameRemovedCallback;
-
+    mFrameRemovedContext = aFrameRemovedContext;
-    VerifyOrExit(mFrameTransmitCallback == NULL || aFrameTransmitCallback == NULL, error = OT_ERROR_BUSY);
-
-    mFrameTransmitCallback = aFrameTransmitCallback;
-    mFrameTransmitContext = aContext;
-
-    if (mFrameTransmitCallback != NULL)
-    {
-        mFrameTransmitMark = mWriteFrameStart;
-    }
-    else
-    {
-        mFrameTransmitMark = NULL;
-    }
-    
-exit:
-    return error;
 }
 
-// Returns the next buffer pointer addressing the wrap-around at the end of buffer.
+// Increments the buffer pointer by one byte while handling the the wrap-around at the end of buffer.
 uint8_t *NcpFrameBuffer::Next(uint8_t *aBufPtr) const
 {
     aBufPtr++;
@@ -168,7 +126,7 @@ uint8_t *NcpFrameBuffer::Advance(uint8_t *aBufPtr, uint16_t aOffset) const
     return aBufPtr;
 }
 
-// Get the distance between two buffer pointers (adjusts for the wrap-around).
+// Gets the distance between two buffer pointers (adjusts for the wrap-around).
 uint16_t NcpFrameBuffer::GetDistance(uint8_t *aStartPtr, uint8_t *aEndPtr) const
 {
     size_t distance;
@@ -186,14 +144,14 @@ uint16_t NcpFrameBuffer::GetDistance(uint8_t *aStartPtr, uint8_t *aEndPtr) const
     return static_cast<uint16_t>(distance);
 }
 
-// Write a uint16 value at the given buffer pointer (big-endian style).
+// Writes a uint16 value at the given buffer pointer (big-endian style).
 void NcpFrameBuffer::WriteUint16At(uint8_t *aBufPtr, uint16_t aValue)
 {
     *aBufPtr = (aValue >> 8);
     *Next(aBufPtr) = (aValue & 0xff);
 }
 
-// Read a uint16 value at the given buffer pointer (big-endian style).
+// Reads a uint16 value at the given buffer pointer (big-endian style).
 uint16_t NcpFrameBuffer::ReadUint16At(uint8_t *aBufPtr)
 {
     uint16_t value;
@@ -342,11 +300,13 @@ exit:
 otError NcpFrameBuffer::InFrameEnd(void)
 {
     otMessage *message;
-    bool wasEmpty = IsEmpty();
 
     // End/Close the current segment (if any).
     InFrameEndSegment(kSegmentHeaderNoFlag);
 
+    // Save and use the frame start pointer as the tag associated with the frame.
+    mWriteFrameTag = mWriteFrameStart;
+
     // Update the frame start pointer to current segment head to be ready for next frame.
     mWriteFrameStart = mWriteSegmentHead;
 
@@ -357,18 +317,19 @@ otError NcpFrameBuffer::InFrameEnd(void)
         otMessageQueueEnqueue(&mMessageQueue, message);
     }
 
-    // If buffer was empty before, invoke the callback to signal that buffer is now non-empty.
+    if (mFrameAddedCallback != NULL)
-    if (wasEmpty)
     {
-        if (mNonEmptyBufferCallback != NULL)
+        mFrameAddedCallback(mFrameAddedContext, mWriteFrameTag, this);
-        {
-            mNonEmptyBufferCallback(mEmptyBufferCallbackContext, this);
-        }
     }
 
     return OT_ERROR_NONE;
 }
 
+NcpFrameBuffer::FrameTag NcpFrameBuffer::InFrameGetLastTag(void) const
+{
+    return mWriteFrameTag;
+}
+
 bool NcpFrameBuffer::IsEmpty(void) const
 {
     return (mReadFrameStart == mWriteFrameStart);
@@ -590,9 +551,13 @@ otError NcpFrameBuffer::OutFrameRemove(void)
     uint8_t *bufPtr;
     otMessage *message;
     uint16_t header;
+    FrameTag tag;
 
     VerifyOrExit(!IsEmpty(), error = OT_ERROR_NOT_FOUND);
 
+    // Save the frame start pointer as the tag associated with the frame being removed.
+    tag = mReadFrameStart;
+
     // Begin at the start of current frame and move through all segments.
 
     bufPtr = mReadFrameStart;
@@ -631,23 +596,9 @@ otError NcpFrameBuffer::OutFrameRemove(void)
     mReadState = kReadStateDone;
     mReadFrameLength = kUnknownFrameLength;
 
-    // If the remove causes the buffer to become empty, invoke the callback to signal this.
+    if (mFrameRemovedCallback != NULL)
-    if (IsEmpty())
     {
-        if (mEmptyBufferCallback != NULL)
+        mFrameRemovedCallback(mFrameRemovedContext, tag, this);
-        {
-            mEmptyBufferCallback(mEmptyBufferCallbackContext, this);
-        }
-    }
-
-    if (mFrameTransmitMark == mReadFrameStart)
-    {
-        if (mFrameTransmitCallback != NULL)
-        {
-            mFrameTransmitCallback(mFrameTransmitContext, OT_ERROR_NONE);
-            mFrameTransmitCallback = NULL;
-            mFrameTransmitMark = NULL;
-        }
     }
 
 exit:
@@ -713,4 +664,12 @@ exit:
     return frameLength;
 }
 
+NcpFrameBuffer::FrameTag NcpFrameBuffer::OutFrameGetTag(void) const
+{
+    // If buffer is empty use `kInvalidTag`, otherwise use the frame start pointer as the tag associated with
+    // current out frame being read
+
+    return IsEmpty() ? kInvalidTag : mReadFrameStart;
+}
+
 }  // namespace ot

src/ncp/ncp_buffer.hpp

@@ -42,23 +42,27 @@ class NcpFrameBuffer
 {
 public:
     /**
-     * Defines a function pointer callback which is invoked to inform state transition of buffer, going from empty
+     * Defines the frame tag type. Frame tags can be compared with one another using operator `==`.
-     * to non-empty or becoming empty.
-     *
-     * @param[in]  aContext         A pointer to arbitrary context information.
-     * @param[in]  aNcpFrameBuffer  A pointer to the NcpFrameBuffer.
      *
      */
-    typedef void (*BufferCallback)(void *aContext, NcpFrameBuffer *aNcpFrameBuffer);
+    typedef const void *FrameTag;
 
     /**
-     * Defines a function pointer callback which is invoked to inform that a pre-tagged frame has transmitted.
+     * Defines the tag to indicate an invalid tag (e.g., when there is no frame).
-     *
-     * @param[in]  aContext         A pointer to arbitrary context information.
-     * @param[in]  aError           An error value representing the success or failure of the frame transmit attempt.
      *
      */
-    typedef void (*FrameTransmitCallback)(void *aContext, otError aError);
+    static const FrameTag kInvalidTag;
+
+    /**
+     * Defines a function pointer callback which is invoked to inform a change in `NcpFrameBuffer` either when a new
+     * frame is added/written to `NcpFrameBuffer` or when a frame is removed from `NcpFrameBuffer`.
+     *
+     * @param[in]  aContext         A pointer to arbitrary context information.
+     * @param[in]  aTag             The tag associated with the frame which is added or removed.
+     * @param[in]  aNcpFrameBuffer  A pointer to the `NcpFrameBuffer`.
+     *
+     */
+    typedef void (*BufferCallback)(void *aContext, FrameTag aTag, NcpFrameBuffer *aNcpFrameBuffer);
 
     /**
      * This constructor creates an NCP frame buffer.
@@ -69,31 +73,33 @@ public:
      */
     NcpFrameBuffer(uint8_t *aBuffer, uint16_t aBufferLength);
 
-    /**
-     * This destructor clears the NCP frame buffer and clears all frames..
-     *
-     */
-    ~NcpFrameBuffer();
-
     /**
      * This method clears the NCP frame buffer. All the frames are cleared/removed.
      *
-     * @returns Nothing (void).
      */
     void Clear(void);
 
     /**
-     * This method sets the callbacks and context. Subsequent calls to this method will overwrite the previous
+     * This method sets the `FrameAdded` callback and its context.
-     * callbacks and context.
      *
-     * @param[in]  aEmptyBufferCallback     Callback invoked when buffer become empty.
+     * Subsequent calls to this method will overwrite the previous callback and its context.
-     * @param[in]  aNonEmptyBufferCallback  Callback invoked when buffer transition from empty to non-empty.
-     * @param[in]  aContex                  A pointer to arbitrary context information.
      *
-     * @returns    Nothing (void).
+     * @param[in]  aFrameAddedCallback      Callback invoked when a new frame is successfully added to buffer.
+     * @param[in]  aFrameAddedContext       A pointer to arbitrary context used with frame added callback.
      *
      */
-    void SetCallbacks(BufferCallback aEmptyBufferCallback, BufferCallback aNonEmptyBufferCallback, void *aContext);
+    void SetFrameAddedCallback(BufferCallback aFrameAddedCallback, void *aFrameAddedContext);
+
+    /**
+     * This method sets the `FrameRemoved` callback and its context.
+     *
+     * Subsequent calls to this method will overwrite the previous callback and its context.
+     *
+     * @param[in]  aFrameRemovedCallback    Callback invoked when a frame is removed from buffer.
+     * @param[in]  aFrameRemovedContext     A pointer to arbitrary context used with frame removed callback.
+     *
+     */
+    void SetFrameRemovedCallback(BufferCallback aFrameRemovedCallback, void *aFrameRemovedContext);
 
     /**
      * This method begins a new input frame to be added/written to the frame buffer.
@@ -147,6 +153,16 @@ public:
      */
     otError InFrameEnd(void);
 
+    /**
+     * This method returns the tag assigned to last successfully written/added frame to NcpBuffer (i.e., last input
+     * frame for which `InFrameEnd()` was called and returned success status). The tag is a unique value (within
+     * currently queued frames) associated with each frame in the `NcpFrameBuffer`. The tag can be used to identify the
+     * same frame when it is read and removed from the NcpBuffer. Tags can be compared using operator `==`.
+     *
+     * @returns The tag of last successfully written frame, or `kInvalidTag` if no frame is written so far.
+     */
+    FrameTag InFrameGetLastTag(void) const;
+
     /**
      * This method checks if the buffer is empty. An non-empty buffer contains at least one full frame for reading.
      *
@@ -160,7 +176,7 @@ public:
      * This method begins/prepares a new output frame to be read from the frame buffer.
      *
      * The NCP buffer maintains a read offset for the current frame being read. Before reading any bytes from the frame
-     * this method should be called to prepare the  frame and set the read offset.
+     * this method should be called to prepare the frame and set the read offset.
      *
      * If part of current frame has already been read, a sub-sequent call to this method will reset the read offset
      * back to beginning of current output frame.
@@ -221,7 +237,7 @@ public:
      *
      * When a frame is removed all its associated messages will be freed.
      *
-     * If the remove operation causes the buffer to become empty this method will invoke the `EmptyBufferCallback`.
+     * If the remove operation is successful, this method will invoke the `FrameRemovedCallback` (if provided).
      *
      * @retval OT_ERROR_NONE       Successfully removed the front frame.
      * @retval OT_ERROR_NOT_FOUND  No frame available in NCP frame buffer to remove.
@@ -244,17 +260,18 @@ public:
     uint16_t OutFrameGetLength(void);
 
     /**
-     * This method provides a callback to NcpBuffer that it will use when the last successfully written 
+     * This method returns the tag value associated to current/front frame in the NCP frame buffer.
-     * frame is removed from the buffer.
      *
-     * @param[in]  aFrameTransmitCallback   Callback invoked when NcpBuffer transmits the current last frame.
+     * The NCP buffer stores the frames in FIFO order. This method returns the tag of the front frame (which may
-     * @param[in]  aContex                  A pointer to arbitrary context information.
+     * be the current output frame being read) from the buffer. There is no need to prepare/begin reading the current
+     * frame before calling this method so this method can be used without a previous call to `OutFrameBegin()`.
      *
-     * @retval OT_ERROR_NONE      Successfully accepted the callback.
+     * If there is no frame in buffer, this method returns `kInvalidTag`.
-     * @retval OT_ERROR_BUSY      The feature is already in use and busy.
+     *
+     * @returns    The tag assigned to the current/from output frame, or `kInvalidTag` if no frame in buffer.
      *
      */
-    otError SetFrameTransmitCallback(FrameTransmitCallback aFrameTransmitCallback, void *aContext);
+    FrameTag OutFrameGetTag(void) const;
 
 private:
 
@@ -346,38 +363,36 @@ private:
 
     // Instance variables
 
-    uint8_t * const         mBuffer;                    // Pointer to the buffer used to store the data.
+    uint8_t * const  mBuffer;                    // Pointer to the buffer used to store the data.
-    uint8_t * const         mBufferEnd;                 // Points to after the end of buffer.
+    uint8_t * const  mBufferEnd;                 // Points to after the end of buffer.
-    const uint16_t          mBufferLength;              // Length of the the buffer.
+    const uint16_t   mBufferLength;              // Length of the the buffer.
 
-    BufferCallback          mEmptyBufferCallback;       // Callback to signal when buffer becomes empty.
+    BufferCallback   mFrameAddedCallback;        // Callback to signal when a new frame is added
-    BufferCallback          mNonEmptyBufferCallback;    // Callback to signal when buffer becomes non-empty.
+    void *           mFrameAddedContext;         // Context passed to `mFrameAddedCallback`.
-    void *                  mEmptyBufferCallbackContext;// Context passed to callbacks.
+    BufferCallback   mFrameRemovedCallback;      // Callback to signal when a frame is removed.
+    void *           mFrameRemovedContext;       // Context passed to `mFrameRemovedCallback`.
 
-    FrameTransmitCallback   mFrameTransmitCallback;     // Callback to signal when a particular frame has been transmitted.
+    otMessageQueue   mMessageQueue;              // Main message queue.
-    void *                  mFrameTransmitContext;      // Context passed to mFrameTransmitCallback;
-    uint8_t *               mFrameTransmitMark;         // The end position of the desired frame in the frame buffer.
 
-    otMessageQueue          mMessageQueue;              // Main message queue.
+    otMessageQueue   mWriteFrameMessageQueue;    // Message queue for the current frame being written.
+    uint8_t *        mWriteFrameStart;           // Pointer to start of current frame being written.
+    uint8_t *        mWriteSegmentHead;          // Pointer to start of current segment in the frame being written.
+    uint8_t *        mWriteSegmentTail;          // Pointer to end of current segment in the frame being written.
+    FrameTag         mWriteFrameTag;             // Tag associated with last successfully written frame.
 
-    otMessageQueue          mWriteFrameMessageQueue;    // Message queue for the current frame being written.
+    ReadState        mReadState;                 // Read state.
-    uint8_t *               mWriteFrameStart;           // Pointer to start of current frame being written.
+    uint16_t         mReadFrameLength;           // Length of current frame being read.
-    uint8_t *               mWriteSegmentHead;          // Pointer to start of current segment in the frame being written.
-    uint8_t *               mWriteSegmentTail;          // Pointer to end of current segment in the frame being written.
 
-    ReadState               mReadState;                 // Read state.
+    uint8_t *        mReadFrameStart;            // Pointer to start of current frame being read.
-    uint16_t                mReadFrameLength;           // Length of current frame being read.
+    uint8_t *        mReadSegmentHead;           // Pointer to start of current segment in the frame being read.
+    uint8_t *        mReadSegmentTail;           // Pointer to end of current segment in the frame being read.
+    uint8_t *        mReadPointer;               // Pointer to next byte to read (either in segment or in msg buffer).
 
-    uint8_t *               mReadFrameStart;            // Pointer to start of current frame being read.
+    otMessage *      mReadMessage;               // Current Message in the frame being read.
-    uint8_t *               mReadSegmentHead;           // Pointer to start of current segment in the frame being read.
+    uint16_t         mReadMessageOffset;         // Offset within current message being read.
-    uint8_t *               mReadSegmentTail;           // Pointer to end of current segment in the frame being read.
-    uint8_t *               mReadPointer;               // Pointer to next byte to read (either in segment or in msg buffer).
 
-    otMessage *             mReadMessage;               // Current Message in the frame being read.
+    uint8_t          mMessageBuffer[kMessageReadBufferSize];   // Buffer to hold part of current message being read.
-    uint16_t                mReadMessageOffset;         // Offset within current message being read.
+    uint8_t *        mReadMessageTail;           // Pointer to end of current part in mMessageBuffer.
-
-    uint8_t                 mMessageBuffer[kMessageReadBufferSize];   // Buffer to hold part of current message being read.
-    uint8_t *               mReadMessageTail;           // Pointer to end of current part in mMessageBuffer.
 };
 
 }  // namespace ot

src/ncp/ncp_spi.cpp

@@ -116,7 +116,7 @@ NcpSpi::NcpSpi(otInstance *aInstance) :
     memset(mEmptySendFrameZeroAccept, 0, kSpiHeaderLength);
     memset(mEmptySendFrameFullAccept, 0, kSpiHeaderLength);
 
-    mTxFrameBuffer.SetCallbacks(NULL, TxFrameBufferHasData, this);
+    mTxFrameBuffer.SetFrameAddedCallback(HandleFrameAddedToTxBuffer, this);
 
     spi_header_set_flag_byte(mSendFrame, SPI_RESET_FLAG | SPI_PATTERN_VALUE);
     spi_header_set_flag_byte(mEmptySendFrameZeroAccept, SPI_RESET_FLAG | SPI_PATTERN_VALUE);
@@ -250,9 +250,10 @@ void NcpSpi::SpiTransactionProcess(void)
     }
 }
 
-void NcpSpi::TxFrameBufferHasData(void *aContext, NcpFrameBuffer *aNcpFrameBuffer)
+void NcpSpi::HandleFrameAddedToTxBuffer(void *aContext, NcpFrameBuffer::FrameTag aTag, NcpFrameBuffer *aNcpFrameBuffer)
 {
     (void)aNcpFrameBuffer;
+    (void)aTag;
 
     static_cast<NcpSpi *>(aContext)->mPrepareTxFrameTask.Post();
 }
@@ -308,11 +309,7 @@ otError NcpSpi::PrepareNextSpiSendFrame(void)
         ExitNow();
     }
 
-    // Remove the frame from tx buffer and inform the base
-    // class that space is now available for a new frame.
-
     mTxFrameBuffer.OutFrameRemove();
-    HandleSpaceAvailableInTxBuffer();
 
 exit:
     return errorCode;

src/ncp/ncp_spi.hpp

@@ -85,12 +85,13 @@ private:
     static void SpiTransactionProcess(void *aContext);
     void SpiTransactionProcess(void);
 
+    static void HandleFrameAddedToTxBuffer(void *aContext, NcpFrameBuffer::FrameTag aFrameTag,
+                                           NcpFrameBuffer *aNcpFrameBuffer);
+
     static void PrepareTxFrame(void *context);
     void PrepareTxFrame(void);
     void HandleRxFrame(void);
 
-    static void TxFrameBufferHasData(void *aContext, NcpFrameBuffer *aNcpFrameBuffer);
-
     otError PrepareNextSpiSendFrame(void);
 
     volatile TxState mTxState;

src/ncp/ncp_uart.cpp

@@ -104,19 +104,21 @@ NcpUart::NcpUart(otInstance *aInstance):
     mByte(0),
     mUartSendTask(aInstance->mIp6.mTaskletScheduler, EncodeAndSendToUart, this)
 {
-    mTxFrameBuffer.SetCallbacks(NULL, TxFrameBufferHasData, this);
+    mTxFrameBuffer.SetFrameAddedCallback(HandleFrameAddedToNcpBuffer, this);
 
     otPlatUartEnable();
 }
 
-void NcpUart::TxFrameBufferHasData(void *aContext, NcpFrameBuffer *aNcpFrameBuffer)
+void NcpUart::HandleFrameAddedToNcpBuffer(void *aContext, NcpFrameBuffer::FrameTag aTag,
+                                          NcpFrameBuffer *aNcpFrameBuffer)
 {
     (void)aNcpFrameBuffer;
+    (void)aTag;
 
-    static_cast<NcpUart *>(aContext)->TxFrameBufferHasData();
+    static_cast<NcpUart *>(aContext)->HandleFrameAddedToNcpBuffer();
 }
 
-void NcpUart::TxFrameBufferHasData(void)
+void NcpUart::HandleFrameAddedToNcpBuffer(void)
 {
     if (mUartBuffer.IsEmpty())
     {
@@ -167,9 +169,6 @@ void NcpUart::EncodeAndSendToUart(void)
 
             mTxFrameBuffer.OutFrameRemove();
 
-            // Notify the super/base class that there is space available in tx frame buffer for a new frame.
-            super_t::HandleSpaceAvailableInTxBuffer();
-
             mState = kFinalizingFrame;
 
             // fall through

src/ncp/ncp_uart.hpp

@@ -103,11 +103,13 @@ private:
     void            HandleFrame(uint8_t *aBuf, uint16_t aBufLength);
     void            HandleError(otError aError, uint8_t *aBuf, uint16_t aBufLength);
     void            TxFrameBufferHasData(void);
+    void            HandleFrameAddedToNcpBuffer(void);
 
     static void     EncodeAndSendToUart(void *aContext);
     static void     HandleFrame(void *context, uint8_t *aBuf, uint16_t aBufLength);
     static void     HandleError(void *context, otError aError, uint8_t *aBuf, uint16_t aBufLength);
-    static void     TxFrameBufferHasData(void *aContext, NcpFrameBuffer *aNcpFrameBuffer);
+    static void     HandleFrameAddedToNcpBuffer(void *aContext, NcpFrameBuffer::FrameTag aTag,
+                                                NcpFrameBuffer *aNcpFrameBuffer);
 
     Hdlc::Encoder   mFrameEncoder;
     Hdlc::Decoder   mFrameDecoder;

tests/unit/test_ncp_buffer.cpp

@@ -43,8 +43,9 @@ namespace ot {
 
 enum
 {
-    kTestBufferSize = 101,         // Size of backed buffer for NcpFrameBuffer.
+    kTestBufferSize = 2500,
-    kTestIterationAttemps = 120,
+    kTestIterationAttemps = 10000,
+    kTagArraySize = 1000,
 };
 
 //  Messages used for building frames...
@@ -58,31 +59,76 @@ static MessagePool sMessagePool(&sInstance);
 
 struct CallbackContext
 {
-    uint16_t mEmptyCount;           // Number of times BufferEmptyCallback is invoked.
+    uint32_t mFrameAddedCount;           // Number of times FrameAddedCallback is invoked.
-    uint16_t mNonEmptyCount;        // Number of times BufferNonEmptyCallback is invoked.
+    uint32_t mFrameRemovedCount;         // Number of times FrameRemovedCallback is invoked.
 };
 
-void BufferDidGetEmptyCallback(void *aContext, NcpFrameBuffer *aNcpBuffer)
+CallbackContext sContext;
+
+NcpFrameBuffer::FrameTag sTagHistoryArray[kTagArraySize];
+uint32_t sTagHistoryHead = 0;
+uint32_t sTagHistoryTail = 0;
+NcpFrameBuffer::FrameTag sExpectedRemovedTag = NcpFrameBuffer::kInvalidTag;
+
+void ClearTagHistory(void)
+{
+    sTagHistoryHead = sTagHistoryTail;
+}
+
+void AddTagToHistory(NcpFrameBuffer::FrameTag aTag)
+{
+    sTagHistoryArray[sTagHistoryTail] = aTag;
+
+    if (++sTagHistoryTail == kTagArraySize)
+    {
+        sTagHistoryTail = 0;
+    }
+
+    VerifyOrQuit(sTagHistoryTail != sTagHistoryHead, "Ran out of space in `TagHistoryArray`, increase its size.");
+}
+
+void VerifyAndRemoveTagFromHistory(NcpFrameBuffer::FrameTag aTag)
+{
+    VerifyOrQuit(sTagHistoryHead != sTagHistoryTail, "Tag history is empty,");
+    VerifyOrQuit(aTag == sTagHistoryArray[sTagHistoryHead], "Removed tag does not match the added one");
+
+    if (++sTagHistoryHead == kTagArraySize)
+    {
+        sTagHistoryHead = 0;
+    }
+
+    if (sExpectedRemovedTag != NcpFrameBuffer::kInvalidTag)
+    {
+        VerifyOrQuit(sExpectedRemovedTag == aTag, "Removed tag does match the previous OutFrameGetTag()");
+        sExpectedRemovedTag = NcpFrameBuffer::kInvalidTag;
+    }
+}
+
+void FrameAddedCallback(void *aContext, NcpFrameBuffer::FrameTag aTag, NcpFrameBuffer *aNcpBuffer)
 {
     CallbackContext *callbackContext = reinterpret_cast<CallbackContext *>(aContext);
 
     VerifyOrQuit(aNcpBuffer != NULL, "Null NcpFrameBuffer in the callback");
     VerifyOrQuit(callbackContext != NULL, "Null context in the callback");
+    VerifyOrQuit(aTag != NcpFrameBuffer::kInvalidTag, "Invalid tag in the callback");
+    VerifyOrQuit(aTag == aNcpBuffer->InFrameGetLastTag(), "InFrameGetLastTag() does not match the tag from callback");
+    AddTagToHistory(aTag);
 
-    callbackContext->mEmptyCount++;
+    callbackContext->mFrameAddedCount++;
 }
 
-void BufferDidGetNonEmptyCallback(void *aContext, NcpFrameBuffer *aNcpBuffer)
+void FrameRemovedCallback(void *aContext, NcpFrameBuffer::FrameTag aTag, NcpFrameBuffer *aNcpBuffer)
 {
     CallbackContext *callbackContext = reinterpret_cast<CallbackContext *>(aContext);
 
     VerifyOrQuit(aNcpBuffer != NULL, "Null NcpFrameBuffer in the callback");
     VerifyOrQuit(callbackContext != NULL, "Null context in the callback");
+    VerifyOrQuit(aTag != NcpFrameBuffer::kInvalidTag, "Invalid tag in the callback");
+    VerifyAndRemoveTagFromHistory(aTag);
 
-    callbackContext->mNonEmptyCount++;
+    callbackContext->mFrameRemovedCount++;
 }
 
-
 // Dump the buffer content to screen.
 void DumpBuffer(const char *aTextMessage, uint8_t *aBuffer, uint16_t aBufferLength)
 {
@@ -139,23 +185,32 @@ void ReadAndVerifyContent(NcpFrameBuffer &aNcpBuffer, const uint8_t *aContentBuf
 void WriteTestFrame1(NcpFrameBuffer &aNcpBuffer)
 {
     Message *message;
+    CallbackContext oldContext;
 
     message = sMessagePool.New(Message::kTypeIp6, 0);
     VerifyOrQuit(message != NULL, "Null Message");
     SuccessOrQuit(message->SetLength(sizeof(sMottoText)), "Could not set the length of message.");
     message->Write(0, sizeof(sMottoText), sMottoText);
 
+    oldContext = sContext;
     SuccessOrQuit(aNcpBuffer.InFrameBegin(), "InFrameBegin() failed.");
     SuccessOrQuit(aNcpBuffer.InFrameFeedData(sMottoText, sizeof(sMottoText)), "InFrameFeedData() failed.");
     SuccessOrQuit(aNcpBuffer.InFrameFeedData(sMysteryText, sizeof(sMysteryText)), "InFrameFeedData() failed.");
     SuccessOrQuit(aNcpBuffer.InFrameFeedMessage(message), "InFrameFeedMessage() failed.");
     SuccessOrQuit(aNcpBuffer.InFrameFeedData(sHelloText, sizeof(sHelloText)), "InFrameFeedData() failed.");
     SuccessOrQuit(aNcpBuffer.InFrameEnd(), "InFrameEnd() failed.");
+
+    VerifyOrQuit(oldContext.mFrameAddedCount + 1 == sContext.mFrameAddedCount, "FrameAddedCallback failed.");
+    VerifyOrQuit(oldContext.mFrameRemovedCount == sContext.mFrameRemovedCount, "FrameRemovedCallback failed.");
 }
 
 void VerifyAndRemoveFrame1(NcpFrameBuffer &aNcpBuffer)
 {
+    CallbackContext oldContext = sContext;
+
+    sExpectedRemovedTag = aNcpBuffer.OutFrameGetTag();
     SuccessOrQuit(aNcpBuffer.OutFrameBegin(), "OutFrameBegin() failed unexpectedly.");
+    VerifyOrQuit(sExpectedRemovedTag == aNcpBuffer.OutFrameGetTag(), "OutFrameGetTag() value changed unexpectedly.");
 
     VerifyOrQuit(aNcpBuffer.OutFrameGetLength() == sizeof(sMottoText) + sizeof(sMysteryText) + sizeof(sMottoText)
                  + sizeof(sHelloText), "GetLength() is incorrect.");
@@ -167,13 +222,18 @@ void VerifyAndRemoveFrame1(NcpFrameBuffer &aNcpBuffer)
 
     VerifyOrQuit(aNcpBuffer.OutFrameHasEnded() == true, "Frame longer than expected.");
     VerifyOrQuit(aNcpBuffer.OutFrameReadByte() == 0, "ReadByte() returned non-zero after end of frame.");
+    VerifyOrQuit(sExpectedRemovedTag == aNcpBuffer.OutFrameGetTag(), "OutFrameGetTag() value changed unexpectedly.");
     SuccessOrQuit(aNcpBuffer.OutFrameRemove(), "Remove() failed.");
+
+    VerifyOrQuit(oldContext.mFrameAddedCount == sContext.mFrameAddedCount, "FrameAddedCallback failed.");
+    VerifyOrQuit(oldContext.mFrameRemovedCount + 1 == sContext.mFrameRemovedCount, "FrameRemovedCallback failed.");
 }
 
 void WriteTestFrame2(NcpFrameBuffer &aNcpBuffer)
 {
     Message *message1;
     Message *message2;
+    CallbackContext oldContext = sContext;
 
     message1 = sMessagePool.New(Message::kTypeIp6, 0);
     VerifyOrQuit(message1 != NULL, "Null Message");
@@ -190,10 +250,15 @@ void WriteTestFrame2(NcpFrameBuffer &aNcpBuffer)
     SuccessOrQuit(aNcpBuffer.InFrameFeedData(sOpenThreadText, sizeof(sOpenThreadText)), "InFrameFeedData() failed.");
     SuccessOrQuit(aNcpBuffer.InFrameFeedMessage(message2), "InFrameFeedMessage() failed.");
     SuccessOrQuit(aNcpBuffer.InFrameEnd(), "InFrameEnd() failed.");
+
+    VerifyOrQuit(oldContext.mFrameAddedCount + 1 == sContext.mFrameAddedCount, "FrameAddedCallback failed.");
+    VerifyOrQuit(oldContext.mFrameRemovedCount == sContext.mFrameRemovedCount, "FrameRemovedCallback failed.");
 }
 
 void VerifyAndRemoveFrame2(NcpFrameBuffer &aNcpBuffer)
 {
+    CallbackContext oldContext = sContext;
+
     SuccessOrQuit(aNcpBuffer.OutFrameBegin(), "OutFrameBegin() failed unexpectedly.");
 
     VerifyOrQuit(aNcpBuffer.OutFrameGetLength() == sizeof(sMysteryText) + sizeof(sHelloText) + sizeof(sOpenThreadText),
@@ -205,12 +270,17 @@ void VerifyAndRemoveFrame2(NcpFrameBuffer &aNcpBuffer)
 
     VerifyOrQuit(aNcpBuffer.OutFrameHasEnded() == true, "Frame longer than expected.");
     VerifyOrQuit(aNcpBuffer.OutFrameReadByte() == 0, "ReadByte() returned non-zero after end of frame.");
+    sExpectedRemovedTag = aNcpBuffer.OutFrameGetTag();
     SuccessOrQuit(aNcpBuffer.OutFrameRemove(), "Remove() failed.");
+
+    VerifyOrQuit(oldContext.mFrameAddedCount == sContext.mFrameAddedCount, "FrameAddedCallback failed.");
+    VerifyOrQuit(oldContext.mFrameRemovedCount + 1 == sContext.mFrameRemovedCount, "FrameRemovedCallback failed.");
 }
 
 void WriteTestFrame3(NcpFrameBuffer &aNcpBuffer)
 {
     Message *message1;
+    CallbackContext oldContext = sContext;
 
     message1 = sMessagePool.New(Message::kTypeIp6, 0);
     VerifyOrQuit(message1 != NULL, "Null Message");
@@ -222,10 +292,15 @@ void WriteTestFrame3(NcpFrameBuffer &aNcpBuffer)
     SuccessOrQuit(aNcpBuffer.InFrameFeedMessage(message1), "InFrameFeedMessage() failed.");
     SuccessOrQuit(aNcpBuffer.InFrameFeedData(sMysteryText, sizeof(sMysteryText)), "InFrameFeedData() failed.");
     SuccessOrQuit(aNcpBuffer.InFrameEnd(), "InFrameEnd() failed.");
+
+    VerifyOrQuit(oldContext.mFrameAddedCount + 1 == sContext.mFrameAddedCount, "FrameAddedCallback failed.");
+    VerifyOrQuit(oldContext.mFrameRemovedCount == sContext.mFrameRemovedCount, "FrameRemovedCallback failed.");
 }
 
 void VerifyAndRemoveFrame3(NcpFrameBuffer &aNcpBuffer)
 {
+    CallbackContext oldContext = sContext;
+
     SuccessOrQuit(aNcpBuffer.OutFrameBegin(), "OutFrameBegin() failed unexpectedly.");
 
     VerifyOrQuit(aNcpBuffer.OutFrameGetLength() == sizeof(sMysteryText), "GetLength() is incorrect.");
@@ -234,7 +309,11 @@ void VerifyAndRemoveFrame3(NcpFrameBuffer &aNcpBuffer)
 
     VerifyOrQuit(aNcpBuffer.OutFrameHasEnded() == true, "Frame longer than expected.");
     VerifyOrQuit(aNcpBuffer.OutFrameReadByte() == 0, "ReadByte() returned non-zero after end of frame.");
+    sExpectedRemovedTag = aNcpBuffer.OutFrameGetTag();
     SuccessOrQuit(aNcpBuffer.OutFrameRemove(), "Remove() failed.");
+
+    VerifyOrQuit(oldContext.mFrameAddedCount == sContext.mFrameAddedCount, "FrameAddedCallback failed.");
+    VerifyOrQuit(oldContext.mFrameRemovedCount + 1 == sContext.mFrameRemovedCount, "FrameRemovedCallback failed.");
 }
 
 // This function implements the NcpFrameBuffer tests
@@ -245,8 +324,6 @@ void TestNcpFrameBuffer(void)
     NcpFrameBuffer ncpBuffer(buffer, kTestBufferSize);
 
     Message *message;
-    CallbackContext context;
-    CallbackContext oldContext;
     uint8_t readBuffer[16];
     uint16_t readLen, readOffset;
 
@@ -255,14 +332,23 @@ void TestNcpFrameBuffer(void)
         buffer[i] = 0;
     }
 
-    context.mEmptyCount = 0;
+    sContext.mFrameAddedCount = 0;
-    context.mNonEmptyCount = 0;
+    sContext.mFrameRemovedCount = 0;
+    ClearTagHistory();
 
     // Set the callbacks.
-    ncpBuffer.SetCallbacks(BufferDidGetEmptyCallback, BufferDidGetNonEmptyCallback, &context);
+    ncpBuffer.SetFrameAddedCallback(FrameAddedCallback, &sContext);
+    ncpBuffer.SetFrameRemovedCallback(FrameRemovedCallback, &sContext);
 
     printf("\n- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -");
-    printf("\nTest 1: Write a frame 1 ");
+    printf("\nTest 1: Check initial buffer state");
+
+    VerifyOrQuit(ncpBuffer.IsEmpty() == true, "Not empty after init.");
+    VerifyOrQuit(ncpBuffer.InFrameGetLastTag() == NcpFrameBuffer::kInvalidTag, "Incorrect tag after init.");
+    VerifyOrQuit(ncpBuffer.OutFrameGetTag() == NcpFrameBuffer::kInvalidTag, "Incorrect OutFrameTag after init.");
+
+    printf("\n- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -");
+    printf("\nTest 2: Write a frame 1 ");
 
     WriteTestFrame1(ncpBuffer);
     DumpBuffer("\nBuffer after frame1", buffer, kTestBufferSize);
@@ -286,7 +372,7 @@ void TestNcpFrameBuffer(void)
     printf(" -- PASS\n");
 
     printf("\n- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -");
-    printf("\nTest 2: Multiple frames write and read ");
+    printf("\nTest 3: Multiple frames write and read ");
 
     WriteTestFrame2(ncpBuffer);
     WriteTestFrame3(ncpBuffer);
@@ -327,7 +413,7 @@ void TestNcpFrameBuffer(void)
     printf(" -- PASS\n");
 
     printf("\n- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -");
-    printf("\nTest 3: Frame discard when buffer full and partial read restart");
+    printf("\nTest 4: Frame discard when buffer full and partial read restart");
 
     for (j = 0; j < kTestIterationAttemps; j++)
     {
@@ -374,60 +460,23 @@ void TestNcpFrameBuffer(void)
 
     printf(" -- PASS\n");
 
-    printf("\n- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -");
-    printf("\nTest 4: Callbacks ");
-
-    printf("\nIterations: ");
-
-    // Repeat this multiple times.
-    for (j = 0; j < kTestIterationAttemps; j++)
-    {
-        printf("*");
-        oldContext = context;
-        WriteTestFrame2(ncpBuffer);
-        VerifyOrQuit(ncpBuffer.IsEmpty() == false, "IsEmpty() is incorrect when buffer is non-empty");
-        VerifyOrQuit(oldContext.mEmptyCount == context.mEmptyCount, "Empty callback called incorrectly");
-        VerifyOrQuit(oldContext.mNonEmptyCount + 1 == context.mNonEmptyCount, "NonEmpty callback was not invoked.");
-
-        oldContext = context;
-        WriteTestFrame3(ncpBuffer);
-        VerifyOrQuit(oldContext.mEmptyCount == context.mEmptyCount, "Empty callback called incorrectly");
-        VerifyOrQuit(oldContext.mNonEmptyCount == context.mNonEmptyCount, "NonEmpty callback called incorrectly.");
-
-        oldContext = context;
-        ncpBuffer.OutFrameRemove();
-        VerifyOrQuit(ncpBuffer.IsEmpty() == false, "IsEmpty() is incorrect when buffer is non empty.");
-        VerifyOrQuit(oldContext.mEmptyCount == context.mEmptyCount, "Empty callback called incorrectly");
-        VerifyOrQuit(oldContext.mNonEmptyCount == context.mNonEmptyCount, "NonEmpty callback called incorrectly.");
-
-        oldContext = context;
-        ncpBuffer.OutFrameRemove();
-        VerifyOrQuit(ncpBuffer.IsEmpty() == true, "IsEmpty() is incorrect when buffer is empty.");
-        VerifyOrQuit(oldContext.mEmptyCount + 1 == context.mEmptyCount, "Empty callback was not invoked.");
-        VerifyOrQuit(oldContext.mNonEmptyCount == context.mNonEmptyCount, "NonEmpty callback called incorrectly.");
-    }
-
-    printf(" -- PASS\n");
-
     printf("\n- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -");
     printf("\nTest 5: Clear() and empty buffer method tests");
 
     WriteTestFrame1(ncpBuffer);
 
-    oldContext = context;
     ncpBuffer.Clear();
+    ClearTagHistory();
 
+    VerifyOrQuit(ncpBuffer.InFrameGetLastTag() == NcpFrameBuffer::kInvalidTag, "Incorrect last tag after Clear().");
+    VerifyOrQuit(ncpBuffer.OutFrameGetTag() == NcpFrameBuffer::kInvalidTag, "Incorrect OutFrameTag after Clear().");
     VerifyOrQuit(ncpBuffer.IsEmpty() == true, "IsEmpty() is incorrect when buffer is empty.");
     VerifyOrQuit(ncpBuffer.OutFrameHasEnded() == true, "OutFrameHasEnded() is incorrect when no data in buffer.");
     VerifyOrQuit(ncpBuffer.OutFrameRemove() == OT_ERROR_NOT_FOUND,
                  "Remove() returned incorrect error status when buffer is empty.");
     VerifyOrQuit(ncpBuffer.OutFrameGetLength() == 0, "OutFrameGetLength() returned non-zero length when buffer is empty.");
-    VerifyOrQuit(oldContext.mEmptyCount + 1 == context.mEmptyCount, "Empty callback was not invoked.");
-    VerifyOrQuit(oldContext.mNonEmptyCount == context.mNonEmptyCount, "NonEmpty callback called incorrectly.");
 
     WriteTestFrame1(ncpBuffer);
-
-    oldContext = context;
     VerifyAndRemoveFrame1(ncpBuffer);
 
     VerifyOrQuit(ncpBuffer.IsEmpty() == true, "IsEmpty() is incorrect when buffer is empty.");
@@ -435,8 +484,6 @@ void TestNcpFrameBuffer(void)
     VerifyOrQuit(ncpBuffer.OutFrameRemove() == OT_ERROR_NOT_FOUND,
                  "Remove() returned incorrect error status when buffer is empty.");
     VerifyOrQuit(ncpBuffer.OutFrameGetLength() == 0, "OutFrameGetLength() returned non-zero length when buffer is empty.");
-    VerifyOrQuit(oldContext.mEmptyCount + 1 == context.mEmptyCount, "Empty callback was not invoked.");
-    VerifyOrQuit(oldContext.mNonEmptyCount == context.mNonEmptyCount, "NonEmpty callback called incorrectly.");
 
     printf(" -- PASS\n");
 
@@ -464,12 +511,186 @@ void TestNcpFrameBuffer(void)
     printf("\n -- PASS\n");
 }
 
+/**
+ * NCP Buffer Fuzz testing
+ *
+ * Randomly decide if to read or write a frame to the NCP buffer (use `kReadProbability` in percent to control the
+ * behavior).
+ *
+ * When writing a frame, use a random length (1 up to `kMaxFrameLen`) and generate random byte sequences.
+ * When reading a frame ensure the length and the content matches what was written earlier.
+ * Handle the cases where buffer gets full or empty.
+ *
+ */
+
+enum
+{
+    kFuzTestBufferSize = 2000,             // Size of the buffer used during fuzz testing
+    kFuzTestIterationAttempts = 500000,    // Number of iterations  to run
+    kLensArraySize = 500,                  // Size of "Lengths" array.
+    kMaxFrameLen = 400,                    // Maximum frame length
+    kReadProbability = 50,                 // Probability (in percent) to randomly choose to read vs write frame
+    kUseTrueRandomNumberGenerator = 1,     // To use true random number generator or not.
+};
+
+uint8_t sFrameBuffer[kFuzTestBufferSize];
+uint32_t sFrameBufferTailIndex = 0;
+
+uint32_t GetRandom(uint32_t max)
+{
+    uint32_t value;
+
+    if (kUseTrueRandomNumberGenerator)
+    {
+        otPlatRandomGetTrue(reinterpret_cast<uint8_t *>(&value), sizeof(value));
+    }
+    else
+    {
+        value = otPlatRandomGet();
+    }
+
+    return value % max;
+}
+
+otError WriteRandomFrame(uint32_t aLength, NcpFrameBuffer &aNcpBuffer)
+{
+    otError error;
+    uint8_t byte;
+    CallbackContext oldContext = sContext;
+    uint32_t tail = sFrameBufferTailIndex;
+
+    SuccessOrExit(error = aNcpBuffer.InFrameBegin());
+
+    while (aLength--)
+    {
+        byte = static_cast<uint8_t>(GetRandom(256));
+        SuccessOrExit(error = aNcpBuffer.InFrameFeedData(&byte, sizeof(byte)));
+        sFrameBuffer[tail++] = byte;
+    }
+
+    SuccessOrExit(error = aNcpBuffer.InFrameEnd());
+
+    sFrameBufferTailIndex = tail;
+
+    // check the callbacks
+    VerifyOrQuit(oldContext.mFrameAddedCount + 1 == sContext.mFrameAddedCount, "FrameAddedCallback failed.");
+    VerifyOrQuit(oldContext.mFrameRemovedCount == sContext.mFrameRemovedCount, "FrameRemovedCallback failed.");
+
+exit:
+    return error;
+}
+
+otError ReadRandomFrame(uint32_t aLength, NcpFrameBuffer &aNcpBuffer)
+{
+    CallbackContext oldContext = sContext;
+
+    SuccessOrQuit(aNcpBuffer.OutFrameBegin(), "OutFrameBegin failed");
+    VerifyOrQuit(aNcpBuffer.OutFrameGetLength() == aLength, "OutFrameGetLength() does not match");
+
+    // Read and verify that the content is same as sFrameBuffer values...
+    ReadAndVerifyContent(aNcpBuffer, sFrameBuffer, static_cast<uint16_t>(aLength));
+    sExpectedRemovedTag = aNcpBuffer.OutFrameGetTag();
+
+    SuccessOrQuit(aNcpBuffer.OutFrameRemove(), "OutFrameRemove failed");
+
+    sFrameBufferTailIndex -= aLength;
+    memmove(sFrameBuffer, sFrameBuffer + aLength, sFrameBufferTailIndex);
+
+    // If successful check the callbacks
+    VerifyOrQuit(oldContext.mFrameAddedCount == sContext.mFrameAddedCount, "FrameAddedCallback failed.");
+    VerifyOrQuit(oldContext.mFrameRemovedCount + 1 == sContext.mFrameRemovedCount, "FrameRemovedCallback failed.");
+
+    return OT_ERROR_NONE;
+}
+
+
+// This runs a fuzz test of NCP buffer
+void TestFuzzNcpFrameBuffer(void)
+{
+    uint8_t buffer[kFuzTestBufferSize];
+    NcpFrameBuffer ncpBuffer(buffer, kFuzTestBufferSize);
+
+    uint32_t lensArray[kLensArraySize];          // Keeps track of length of written frames so far
+    uint32_t lensArrayStart;
+    uint32_t lensArrayCount;
+
+    memset(buffer, 0, sizeof(buffer));
+
+    memset(lensArray, 0, sizeof(lensArray));
+    lensArrayStart = 0;
+    lensArrayCount = 0;
+
+    sContext.mFrameAddedCount = 0;
+    sContext.mFrameRemovedCount = 0;
+    ClearTagHistory();
+
+    ncpBuffer.SetFrameAddedCallback(FrameAddedCallback, &sContext);
+    ncpBuffer.SetFrameRemovedCallback(FrameRemovedCallback, &sContext);
+
+    for (uint32_t iter = 0; iter < kFuzTestIterationAttempts; iter++)
+    {
+        bool shouldRead;
+
+        if (lensArrayCount == 0)
+        {
+            shouldRead = false;
+        }
+        else if (lensArrayCount == kLensArraySize - 1)
+        {
+            shouldRead = true;
+        }
+        else
+        {
+            // Randomly decide to read or write.
+            shouldRead = (GetRandom(100) < kReadProbability);
+        }
+
+        if (shouldRead)
+        {
+            uint32_t len = lensArray[lensArrayStart];
+
+            lensArrayStart = (lensArrayStart + 1) % kLensArraySize;
+            lensArrayCount--;
+
+            printf("R%d ", len);
+
+            SuccessOrQuit(ReadRandomFrame(len, ncpBuffer), "Failed to read random frame.");
+        }
+        else
+        {
+            uint32_t len = GetRandom(kMaxFrameLen) + 1;
+
+            if (WriteRandomFrame(len, ncpBuffer) == OT_ERROR_NONE)
+            {
+                lensArray[(lensArrayStart + lensArrayCount) % kLensArraySize] = len;
+                lensArrayCount++;
+
+                printf("W%d ", len);
+            }
+            else
+            {
+                printf("FULL ");
+            }
+        }
+
+        if (lensArrayCount == 0)
+        {
+            VerifyOrQuit(ncpBuffer.IsEmpty() == true, "IsEmpty failed.");
+            printf("EMPTY ");
+        }
+
+    }
+
+    printf("\n -- PASS\n");
+}
+
 }  // namespace ot
 
 #ifdef ENABLE_TEST_MAIN
 int main(void)
 {
     ot::TestNcpFrameBuffer();
+    ot::TestFuzzNcpFrameBuffer();
     printf("\nAll tests passed.\n");
     return 0;
 }