Inconsistent documentation for xStreamBufferCreateStatic()

[paul-igor]

In stream_buffer.h, the documentation for xStreamBufferCreateStatic() describes the first and third parameters as follows:

 * @param xBufferSizeBytes The size, in bytes, of the buffer pointed to by the
 * pucStreamBufferStorageArea parameter.

 * @param pucStreamBufferStorageArea Must point to a uint8_t array that is at
 * least xBufferSizeBytes + 1 big.  This is the array to which streams are
 * copied when they are written to the stream buffer.

However, in the example that follows in the header file, the crucial +1 seems to have been omitted. This can result in an off-by-one issue that overflows the buffer. When following the example, my team's project experienced a crash that was difficult to narrow down.

 *  xStreamBuffer = xStreamBufferCreateStatic( sizeof( ucBufferStorage ),
 *                                             xTriggerLevel,
 *                                             ucBufferStorage,
 *                                             &xStreamBufferStruct );

A similar documentation pattern appears in message_buffer.h for xMessageBufferCreateStatic(). Maybe there are others?

[paul-igor]

My hunch that the parameter documentation is correct, rather than the example, is based on this type of logic found in xStreamBufferSpacesAvailable():

    xSpace = pxStreamBuffer->xLength + pxStreamBuffer->xTail;
    xSpace -= pxStreamBuffer->xHead;
    xSpace -= ( size_t ) 1;

And this of course is part of the classic "dummy entry" solution to distinguish fifo empty versus fifo full conditions.

[paul-igor]

I've created a PR with the simple update: #380

[jefftenney]

The line xSpace -= ( size_t ) 1; shows that FreeRTOS makes room for the unused byte (dummy entry). No need for the caller of xStreamBufferCreateStatic() to subtract one from the buffer size too.

Maybe there's another explanation for the crash your team was debugging?

[paul-igor]

@jefftenney, thanks. I see what you mean. Our crash is likely related to something else. I think I'll close the PR.

However, the documentation still looks inconsistent. Specifically, the + 1 in the parameter:

 * @param pucStreamBufferStorageArea Must point to a uint8_t array that is at
 * least xBufferSizeBytes + 1 big.  This is the array to which streams are
 * copied when they are written to the stream buffer.

And, here:

 * @param pucMessageBufferStorageArea Must point to a uint8_t array that is at
 * least xBufferSizeBytes + 1 big.  This is the array to which messages are
 * copied when they are written to the message buffer.

The correct description would state that the StorageArea fields must be at least xBufferSizeBytes big.

Do I have that right this time?

[jefftenney]

Yes I think so. Both statements you quoted should be fixed to remove the "+1". You could include those changes in your PR or in a new PR.

[paul-igor]

PR updated. Thanks.