Add Stream Batching Buffer

cperkulator commented 8 months ago

This will cause a task to sleep if the buffer it is waiting on does not have enough data in it yet. The purpose of this for example, would be to queue up samples in an interrupt but only operate on those samples after a certain amount of samples have been queued.

This is in reference to issue #643.

Still need to go through testing but putting this out here for people to comment on

Description

Test Steps

Checklist:

[ ] I have tested my changes. No regression in existing tests.
[ ] I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

sonarcloud[bot] commented 8 months ago

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

kstribrnAmzn commented 8 months ago

@cperkulator sorry for the delay in looking at this. I'm taking a look now and will post back either questions or my plan of getting this merged in a few hours. Given some checks are failing I may attempt to fix these myself by appending onto your PR.

cperkulator commented 8 months ago

@cperkulator sorry for the delay in looking at this. I'm taking a look now and will post back either questions or my plan of getting this merged in a few hours. Given some checks are failing I may attempt to fix these myself by appending onto your PR.

Great! I just wanted to get the idea out there. A lot of what is missing is just boilerplate stuff

kstribrnAmzn commented 8 months ago

Overall I think this is well done. I'm going to post a couple thoughts below, but I think they are easily changeable (if we agree on it) and don't affect your design in any way. Your feedback would be great. My next step will be to chat with @aggarg and @RichardBarry to see what their thought are. We can worry about formatting, unit tests, and all the other checks once we have a finalized design.

Thoughts:

Not a huge fan of 'BlockingBuffer' since the term 'block'/'blocking' is used already in the FreeRTOS ecosystem. Maybe 'BatchingBuffer' could work? I believe this better communicates the all-or-nothing nature of the batching buffer - with the user either getting the batch of bytes or being blocked until a batch is ready.
This one is more radical - and will further break backwards compatibility - but heck it's already broken a little in this PR by adding new parameters. Maybe we should consider taking in a stream buffer 'behavior type' instead of multiple behavior flags like BaseType_t xIsBlockingBuffer and BaseType_t xIsMessageBuffer. This would make the growing number of behaviors simpler to specify.

aggarg commented 7 months ago

Thank you for the proposal @cperkulator. We can avoid expanding the API surface so much if we consider blocking buffer a special type of stream buffer:

#define xStreamBufferBlockingCreate( xBufferSizeBytes, xTriggerLevelBytes ) \
    xStreamBufferGenericCreate( ( xBufferSizeBytes ), ( xTriggerLevelBytes ), pdTRUE, pdFALSE, NULL, NULL )

#if ( configUSE_SB_COMPLETED_CALLBACK == 1 )
    #define xStreamBufferBlockingCreateWithCallback( xBufferSizeBytes, xTriggerLevelBytes, pxSendCompletedCallback, pxReceiveCompletedCallback ) \
    xStreamBufferGenericCreate( ( xBufferSizeBytes ), ( xTriggerLevelBytes ), pdTRUE, ( pxSendCompletedCallback ), ( pxReceiveCompletedCallback ) )
#endif

#define xStreamBufferBlockingCreateStatic( xBufferSizeBytes, xTriggerLevelBytes, pucStreamBufferStorageArea, pxStaticStreamBuffer ) \
    xStreamBufferGenericCreateStatic( ( xBufferSizeBytes ), ( xTriggerLevelBytes ), pdTRUE, ( pucStreamBufferStorageArea ), ( pxStaticStreamBuffer ), NULL, NULL )

#if ( configUSE_SB_COMPLETED_CALLBACK == 1 )
    #define xStreamBufferBlockingCreateStaticWithCallback( xBufferSizeBytes, xTriggerLevelBytes, pucStreamBufferStorageArea, pxStaticStreamBuffer, pxSendCompletedCallback, pxReceiveCompletedCallback ) \
    xStreamBufferGenericCreateStatic( ( xBufferSizeBytes ), ( xTriggerLevelBytes ), pdFALpdTRUESE, ( pucStreamBufferStorageArea ), ( pxStaticStreamBuffer ), ( pxSendCompletedCallback ), ( pxReceiveCompletedCallback ) )
#endif

What do you think about it?

Skptak commented 7 months ago

/bot run formatting

cperkulator commented 7 months ago

Thank you for the proposal @cperkulator. We can avoid expanding the API surface so much if we consider blocking buffer a special type of stream buffer:

#define xStreamBufferBlockingCreate( xBufferSizeBytes, xTriggerLevelBytes ) \
    xStreamBufferGenericCreate( ( xBufferSizeBytes ), ( xTriggerLevelBytes ), pdTRUE, pdFALSE, NULL, NULL )

#if ( configUSE_SB_COMPLETED_CALLBACK == 1 )
    #define xStreamBufferBlockingCreateWithCallback( xBufferSizeBytes, xTriggerLevelBytes, pxSendCompletedCallback, pxReceiveCompletedCallback ) \
    xStreamBufferGenericCreate( ( xBufferSizeBytes ), ( xTriggerLevelBytes ), pdTRUE, ( pxSendCompletedCallback ), ( pxReceiveCompletedCallback ) )
#endif

#define xStreamBufferBlockingCreateStatic( xBufferSizeBytes, xTriggerLevelBytes, pucStreamBufferStorageArea, pxStaticStreamBuffer ) \
    xStreamBufferGenericCreateStatic( ( xBufferSizeBytes ), ( xTriggerLevelBytes ), pdTRUE, ( pucStreamBufferStorageArea ), ( pxStaticStreamBuffer ), NULL, NULL )

#if ( configUSE_SB_COMPLETED_CALLBACK == 1 )
    #define xStreamBufferBlockingCreateStaticWithCallback( xBufferSizeBytes, xTriggerLevelBytes, pucStreamBufferStorageArea, pxStaticStreamBuffer, pxSendCompletedCallback, pxReceiveCompletedCallback ) \
    xStreamBufferGenericCreateStatic( ( xBufferSizeBytes ), ( xTriggerLevelBytes ), pdFALpdTRUESE, ( pucStreamBufferStorageArea ), ( pxStaticStreamBuffer ), ( pxSendCompletedCallback ), ( pxReceiveCompletedCallback ) )
#endif

What do you think about it?

@aggarg yeah this was my exact thinking, though it probably doesn't look like it from the few changes I made.

however, @kstribrnAmzn makes a good point about trying to merge the flags into a behaviour type rather than multiple flags. I'm indifferent though.

and @kstribrnAmzn, I like batching buffer definitely makes it more clear what is happening

sonarcloud[bot] commented 6 months ago

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

chinglee-iot commented 4 months ago

I like @kstribrnAmzn and @aggarg 's idea.

I prefer "batch" than "blocking". A task is also blocked by stream buffers or message buffers. "batch" provides more associations than "blocking".
The major difference between "StreamBufferBatch" and "StreamBuffer" when receiving from stream buffer is that: StreamBuffer - The task is blocked when no data in stream buffer StreamBufferBatch - The task is blocked when the data in stream buffer is not of a batch ( less than trigger level ) Other operations are the same as stream buffer. Since the difference is not much, it makes sense to me to consider it still a type of stream buffer rather than a new kind of buffer.

#define xStreamBufferBatchCreate( xBufferSizeBytes, xTriggerLevelBytes ) \
    xStreamBufferGenericCreate( ( xBufferSizeBytes ), ( xTriggerLevelBytes ), pdFALSE, pdTRUE, NULL, NULL )

#if ( configUSE_SB_COMPLETED_CALLBACK == 1 )
    #define xStreamBufferBatchCreateWithCallback( xBufferSizeBytes, xTriggerLevelBytes, pxSendCompletedCallback, pxReceiveCompletedCallback ) \
    xStreamBufferGenericCreate( ( xBufferSizeBytes ), ( xTriggerLevelBytes ), pdFALSE, pdTRUE, ( pxSendCompletedCallback ), ( pxReceiveCompletedCallback ) )
#endif

#define xStreamBufferBatchCreateStatic( xBufferSizeBytes, xTriggerLevelBytes, pucStreamBufferStorageArea, pxStaticStreamBuffer ) \
    xStreamBufferGenericCreateStatic( ( xBufferSizeBytes ), ( xTriggerLevelBytes ), pdFALSE, pdTRUE, ( pucStreamBufferStorageArea ), ( pxStaticStreamBuffer ), NULL, NULL )

#if ( configUSE_SB_COMPLETED_CALLBACK == 1 )
    #define xStreamBufferBatchCreateStaticWithCallback( xBufferSizeBytes, xTriggerLevelBytes, pucStreamBufferStorageArea, pxStaticStreamBuffer, pxSendCompletedCallback, pxReceiveCompletedCallback ) \
    xStreamBufferGenericCreateStatic( ( xBufferSizeBytes ), ( xTriggerLevelBytes ), pdFALSE, pdTRUE, ( pucStreamBufferStorageArea ), ( pxStaticStreamBuffer ), ( pxSendCompletedCallback ), ( pxReceiveCompletedCallback ) )
#endif

@cperkulator @kstribrnAmzn @aggarg If this looks good to you, I would like to continue the work in this PR and have it merged.

kstribrnAmzn commented 4 months ago

Related to this change - we should add unit tests here.

chinglee-iot commented 4 months ago

I would like to suggest to adapt @kstribrnAmzn another suggestion in the implementation.

This one is more radical - and will further break backwards compatibility - but heck it's already broken a little in this PR by adding new parameters. Maybe we should consider taking in a stream buffer 'behavior type' instead of multiple behavior flags like BaseType_t xIsBlockingBuffer and BaseType_t xIsMessageBuffer. This would make the growing number of behaviors simpler to specify.

The function prototype is not changed in the following example, and it also simplies the growing number of behaviors.

#define sbTYPE_IS_STREAM_BUFFER     ( ( BaseType_t ) 0 )   /* pdFALSE is defined as 0. */
#define sbTYPE_IS_MESSAGE_BUFFER    ( ( BaseType_t ) 1 )  /* pdTRUE is defined as 1. */
#define sbTYPE_IS_STREAM_BATCHING_BUFFER      ( ( BaseType_t ) 2 )

StreamBufferHandle_t xStreamBufferGenericCreate( size_t xBufferSizeBytes,
                                                 size_t xTriggerLevelBytes,
                                                 BaseType_t xStreamBufferType,
                                                 StreamBufferCallbackFunction_t pxSendCompletedCallback,
                                                 StreamBufferCallbackFunction_t pxReceiveCompletedCallback ) PRIVILEGED_FUNCTION;

#if ( configSUPPORT_STATIC_ALLOCATION == 1 )
    StreamBufferHandle_t xStreamBufferGenericCreateStatic( size_t xBufferSizeBytes,
                                                           size_t xTriggerLevelBytes,
                                                           BaseType_t xStreamBufferType,
                                                           uint8_t * const pucStreamBufferStorageArea,
                                                           StaticStreamBuffer_t * const pxStaticStreamBuffer,
                                                           StreamBufferCallbackFunction_t pxSendCompletedCallback,
                                                           StreamBufferCallbackFunction_t pxReceiveCompletedCallback ) PRIVILEGED_FUNCTION;
#endif

sonarcloud[bot] commented 4 months ago

Quality Gate passed

Issues
16 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

FreeRTOS / FreeRTOS-Kernel