Add Stream Batching Buffer #916

cperkulator · 2023-12-05T04:23:50Z

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.

sonarqubecloud · 2023-12-05T04:24:17Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

kstribrnAmzn · 2023-12-21T17:57:27Z

@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 · 2023-12-21T18:14:05Z

@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 · 2023-12-21T21:53:37Z

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 · 2024-01-25T07:18:18Z

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 · 2024-01-25T20:50:07Z

/bot run formatting

cperkulator · 2024-01-25T22:24:21Z

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

sonarqubecloud · 2024-02-01T18:21:22Z

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 · 2024-04-15T06:51:59Z

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.

Signed-off-by: Gaurav Aggarwal <[email protected]>

kstribrnAmzn · 2024-04-16T16:20:41Z

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

chinglee-iot · 2024-04-17T02:09:09Z

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

chinglee-iot · 2024-04-17T02:12:33Z

include/FreeRTOS.h

@@ -2402,15 +2402,15 @@
 #endif

 #ifndef traceENTER_xStreamBufferGenericCreate
-    #define traceENTER_xStreamBufferGenericCreate( xBufferSizeBytes, xTriggerLevelBytes, xIsMessageBuffer, pxSendCompletedCallback, pxReceiveCompletedCallback )
+    #define traceENTER_xStreamBufferGenericCreate( xBufferSizeBytes, xTriggerLevelBytes, xIsMessageBuffer, xIsBatchingBuffer, pxSendCompletedCallback, pxReceiveCompletedCallback )


The following macros also need to be udpated:

traceSTREAM_BUFFER_CREATE_FAILED

traceSTREAM_BUFFER_CREATE_STATIC_FAILED

traceSTREAM_BUFFER_CREATE

Signed-off-by: Gaurav Aggarwal <[email protected]>

sonarqubecloud · 2024-04-17T08:59:23Z

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

Add ability to block when buffer is not at least trigger level

7beae28

cperkulator requested a review from a team as a code owner December 5, 2023 04:23

Merge branch 'main' into blocking_buffer

2869514

Uncrustify: triggered by comment.

e4784d1

Skptak and others added 4 commits January 28, 2024 13:25

Merge branch 'main' into blocking_buffer

001d924

Merge branch 'main' into blocking_buffer

a122210

Merge branch 'main' into blocking_buffer

49d9b84

Merge branch 'main' into blocking_buffer

fbc1b1e

Merge branch 'main' into blocking_buffer

ab31cf4

chinglee-iot and others added 5 commits April 15, 2024 14:52

Merge branch 'main' into blocking_buffer

8663dd7

Code review suggestions

2fe9508

Signed-off-by: Gaurav Aggarwal <[email protected]>

Merge branch 'main' into blocking_buffer

630cad3

Remove not needed file

64cafa4

Signed-off-by: Gaurav Aggarwal <[email protected]>

Update MPU wrappers

9d80b0b

Signed-off-by: Gaurav Aggarwal <[email protected]>

aggarg previously approved these changes Apr 16, 2024

View reviewed changes

kstribrnAmzn previously approved these changes Apr 16, 2024

View reviewed changes

Merge branch 'main' into blocking_buffer

a934ce9

chinglee-iot reviewed Apr 17, 2024

View reviewed changes

Incorporate code review feedback

4d45445

Signed-off-by: Gaurav Aggarwal <[email protected]>

aggarg dismissed stale reviews from kstribrnAmzn and themself via 4d45445 April 17, 2024 06:22

Fix formatting

e7de930

Signed-off-by: Gaurav Aggarwal <[email protected]>

aggarg previously approved these changes Apr 17, 2024

View reviewed changes

Add stream buffer and stream batching buffer difference

cd10aea

Signed-off-by: Gaurav Aggarwal <[email protected]>

aggarg dismissed their stale review via cd10aea April 17, 2024 08:40

aggarg approved these changes Apr 17, 2024

View reviewed changes

chinglee-iot approved these changes Apr 17, 2024

View reviewed changes

Merge branch 'main' into blocking_buffer

e278f3c

aggarg changed the title ~~Add ability to block when buffer is not at least trigger level~~ Add Stream Batching Buffer Apr 17, 2024

aggarg merged commit f69b1db into FreeRTOS:main Apr 17, 2024
16 checks passed

cperkulator deleted the blocking_buffer branch April 27, 2024 02:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Stream Batching Buffer #916

Add Stream Batching Buffer #916

cperkulator commented Dec 5, 2023

sonarqubecloud bot commented Dec 5, 2023

kstribrnAmzn commented Dec 21, 2023

cperkulator commented Dec 21, 2023

kstribrnAmzn commented Dec 21, 2023

aggarg commented Jan 25, 2024

Skptak commented Jan 25, 2024

cperkulator commented Jan 25, 2024

sonarqubecloud bot commented Feb 1, 2024

chinglee-iot commented Apr 15, 2024 •

edited

Loading

kstribrnAmzn commented Apr 16, 2024

chinglee-iot commented Apr 17, 2024 •

edited

Loading

chinglee-iot Apr 17, 2024

sonarqubecloud bot commented Apr 17, 2024

Add Stream Batching Buffer #916

Add Stream Batching Buffer #916

Conversation

cperkulator commented Dec 5, 2023

Description

Test Steps

Checklist:

Related Issue

sonarqubecloud bot commented Dec 5, 2023

kstribrnAmzn commented Dec 21, 2023

cperkulator commented Dec 21, 2023

kstribrnAmzn commented Dec 21, 2023

aggarg commented Jan 25, 2024

Skptak commented Jan 25, 2024

cperkulator commented Jan 25, 2024

sonarqubecloud bot commented Feb 1, 2024

Quality Gate passed

chinglee-iot commented Apr 15, 2024 • edited Loading

kstribrnAmzn commented Apr 16, 2024

chinglee-iot commented Apr 17, 2024 • edited Loading

chinglee-iot Apr 17, 2024

Choose a reason for hiding this comment

sonarqubecloud bot commented Apr 17, 2024

Quality Gate passed

chinglee-iot commented Apr 15, 2024 •

edited

Loading

chinglee-iot commented Apr 17, 2024 •

edited

Loading