FreeRTOS-Kernel
FreeRTOS-Kernel copied to clipboard
FreeRTOS
Add ability to block when buffer is not at least trigger level
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.
Kudos, SonarCloud Quality Gate passed! 
0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell
No Coverage information
0.0% Duplication
![sonarqubecloud[bot] avatar](https://avatars.githubusercontent.com/in/12526?v=4 "Published by a pub.dev verified publisher"){kind=small}
@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 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
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 xIsBlockingBufferandBaseType_t xIsMessageBuffer. This would make the growing number of behaviors simpler to specify.
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?
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 ) ) #endifWhat 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
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
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.
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
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