Skip to content

Add mutiple push and pop for circular buffer #13563

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Sep 21, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
248 changes: 202 additions & 46 deletions platform/include/platform/CircularBuffer.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
#include <stdint.h>
#include "platform/mbed_critical.h"
#include "platform/mbed_assert.h"
#include "platform/Span.h"
#include "platform/mbed_atomic.h"

namespace mbed {

Expand Down Expand Up @@ -60,8 +62,8 @@ struct is_unsigned<unsigned long long> {

/** Templated Circular buffer class
*
* @note Synchronization level: Interrupt safe
* @note CounterType must be unsigned and consistent with BufferSize
* @note Synchronization level: Interrupt safe.
* @note CounterType must be unsigned and consistent with BufferSize.
*/
template<typename T, uint32_t BufferSize, typename CounterType = uint32_t>
class CircularBuffer {
Expand All @@ -84,77 +86,197 @@ class CircularBuffer {
{
}

/** Push the transaction to the buffer. This overwrites the buffer if it's
* full
/** Push the transaction to the buffer. This overwrites the buffer if it's full.
*
* @param data Data to be pushed to the buffer
* @param data Data to be pushed to the buffer.
*/
void push(const T &data)
{
core_util_critical_section_enter();
if (full()) {
_tail++;
if (_tail == BufferSize) {
_tail = 0;
}

_buffer[_head] = data;

_head = incrementCounter(_head);

if (_full) {
_tail = _head;
} else if (_head == _tail) {
_full = true;
}
_pool[_head++] = data;
if (_head == BufferSize) {

core_util_critical_section_exit();
}

/** Push the transaction to the buffer. This overwrites the buffer if it's full.
*
* @param src Data to be pushed to the buffer.
* @param len Number of items to be pushed to the buffer.
*/
void push(const T *src, CounterType len)
{
MBED_ASSERT(len > 0);

core_util_critical_section_enter();

/* if we try to write more bytes than the buffer can hold we only bother writing the last bytes */
if (len > BufferSize) {
_tail = 0;
_head = 0;
}
if (_head == _tail) {
_full = true;
std::copy(src + len - BufferSize, src + len, _buffer);
} else {
/* we need to adjust the tail at the end if we're filling the buffer of overflowing */
bool adjust_tail = ((BufferSize - non_critical_size()) <= len);

CounterType written = len;

/* on first pass we write as much as we can to the right of head */
if ((_head + written) > BufferSize) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the same, just changed for readability

written = BufferSize - _head;
}

std::copy(src, src + written, _buffer + _head);
_head = incrementCounter(_head, written);

CounterType left_to_write = len - written;

/* we might need to continue to write from the start of the buffer */
if (left_to_write) {
std::copy(src + written, src + written + left_to_write, _buffer);
_head = left_to_write;
}

if (adjust_tail) {
_tail = _head;
_full = true;
}
}

core_util_critical_section_exit();
}

/** Pop the transaction from the buffer
/** Push the transaction to the buffer. This overwrites the buffer if it's full.
*
* @param src Data to be pushed to the buffer.
*/
void push(mbed::Span<const T> src)
{
push(src.data(), src.size());
}

/** Pop from the buffer.
*
* @param data Data to be popped from the buffer
* @return True if the buffer is not empty and data contains a transaction, false otherwise
* @param data Container to store the data to be popped from the buffer.
* @return True if data popped.
*/
bool pop(T &data)
{
bool data_popped = false;

core_util_critical_section_enter();
if (!empty()) {
data = _pool[_tail++];
if (_tail == BufferSize) {
_tail = 0;

if (!non_critical_empty()) {
data_popped = true;

data = _buffer[_tail];
_tail = incrementCounter(_tail);
_full = false;
}

core_util_critical_section_exit();

return data_popped;
}

/**
* Pop multiple elements from the buffer.
*
* @param dest The array which will receive the elements.
* @param len The number of elements to pop.
*
* @return The number of elements popped.
*/
CounterType pop(T *dest, CounterType len)
{
MBED_ASSERT(len > 0);

if (len == 0) {
return 0;
}

CounterType data_popped = 0;

core_util_critical_section_enter();

if (!non_critical_empty()) {
/* make sure we only try to read as much as we have items present */
if (len > non_critical_size()) {
len = non_critical_size();
}
data_popped = len;

/* items may be split by overlap, take only the number we have to the right of tail */
if ((_tail + data_popped) > BufferSize) {
data_popped = BufferSize - _tail;
}

std::copy(_buffer + _tail, _buffer + _tail + data_popped, dest);
_tail = incrementCounter(_tail, data_popped);

/* if we looped over the end we may need to pop again */
CounterType left_to_pop = len - data_popped;

if (left_to_pop) {
std::copy(_buffer, _buffer + left_to_pop, dest + data_popped);
_tail = left_to_pop;

data_popped += left_to_pop;
}

_full = false;
data_popped = true;
}

core_util_critical_section_exit();

return data_popped;
}

/** Check if the buffer is empty
/**
* Pop multiple elements from the buffer.
*
* @param dest The span that contains the buffer that will be used to store the elements.
*
* @return The span with the size set to number of elements popped using the buffer passed in as the parameter.
*/
mbed::Span<T> pop(mbed::Span<T> dest)
{
CounterType popped = pop(dest.data(), dest.size());
return mbed::make_Span(dest.data(), popped);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a random note that as of current C++ version you can do return {dest.data(), popped};.

Don't know what our style view on that is. I'm conflicted.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's great to push good C++ coding standard.

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md uses return {dest.data(), popped}; so I think it's good to stick with that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a more specific reference? That's quite a big page, and can't see anything specifically discussing this case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They don't discuss, they use the construct in code examples for std::pair here -- they don't use std::make_pair

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then it's not really a specific style judgment on how explicit or not to be, which is what I'd like to see.

And there's something of a difference - a pair is a pair, so it's "just" a tuple-type container. You're effectively filling in an aggregate from a list, which is the oldest-school use of {}.

Here, you're invoking a constructor. I always feel a bit icky when using {} on its own to invoke a constructor. Maybe I'm too old-fashioned. At least it's the most "direct" constructor. Doing return { start_ptr, end_ptr }; to use the iterator constructor would feel weirder.

Anyway, I'm broadly in favour of the more concise style; I don't like excess clutter. I think the case for it is even stronger in function parameter context, where the span could just be one of several arguments. Whereas on return you do usually have space to spare for that little type reminder.

(C++17 will change the situation again, because you no longer have to use make_Span, you can have the deduction guides for the constructor letting you do just Span(...), so the clutter of explicitness is reduced further.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed explanations :) (your comments are always great learning material for me)

One side question (also asked here, what's the difference between mbed::Span and std::span? Which one should I use in my application code? and why the S for Span?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mbed::Span is based on an old revision of the std::span proposal, the only difference - API wise - with the one in the (next!) standard is the type used to return the size of the span: it is signed in mbed, not signed on std. I believe we should make the switch to unsigned at some point. Another difference is the debug capability of the iterator, it is very limited in mbed whereas libc++ returns fat iterators in debug.

Copy link
Contributor

@kjbracey kjbracey Sep 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just seen Vincent's answer - this partly repeats it...

std::span is C++20, so not supported by all toolchains we support, so can't use it directly. We're also in C++14 mode, and it may not be accessible in that mode.

mbed::Span was written and added to mbed while C++20 was in draft, so has some minor differences compared to the final revision of std::span - in particular its use of ptrdiff_t instead of size_t. The capital S was to fit mbed style guidelines.

Now that C++20 has been finalised, and following other work done in this style pending full std availability, I would suggest implementing and migrating to a new mstd::span that directly matches std::span alongside the other mstd stuff in the cxxsupport directory.

The mstd types like mstd::mutex and mstd::atomic are close enough matches to std that they're drop-in replacements, possibly just needing to change 1 using directive and 1 include. (And a mstd:: identifier can be made a straight using map to a std:: one if available now or later in a toolchain).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you both (again) for the detailed answers :)

I was not aware of the mstd! This is great stuff!

}

/** Check if the buffer is empty.
*
* @return True if the buffer is empty, false if not
* @return True if the buffer is empty, false if not.
*/
bool empty() const
{
core_util_critical_section_enter();
bool is_empty = (_head == _tail) && !_full;
bool is_empty = non_critical_empty();
core_util_critical_section_exit();
return is_empty;
}

/** Check if the buffer is full
/** Check if the buffer is full.
*
* @return True if the buffer is full, false if not
*/
bool full() const
{
core_util_critical_section_enter();
bool full = _full;
core_util_critical_section_exit();
return full;
return core_util_atomic_load_bool(&_full);
}

/** Reset the buffer
*
/**
* Reset the buffer.
*/
void reset()
{
Expand All @@ -165,10 +287,43 @@ class CircularBuffer {
core_util_critical_section_exit();
}

/** Get the number of elements currently stored in the circular_buffer */
/**
* Get the number of elements currently stored in the circular_buffer.
*/
CounterType size() const
{
core_util_critical_section_enter();
CounterType elements = non_critical_size();
core_util_critical_section_exit();
return elements;
}

/** Peek into circular buffer without popping.
*
* @param data Data to be peeked from the buffer.
* @return True if the buffer is not empty and data contains a transaction, false otherwise.
*/
bool peek(T &data) const
{
bool data_updated = false;
core_util_critical_section_enter();
if (!empty()) {
data = _buffer[_tail];
data_updated = true;
}
core_util_critical_section_exit();
return data_updated;
}

private:
bool non_critical_empty() const
{
bool is_empty = (_head == _tail) && !_full;
return is_empty;
}

CounterType non_critical_size() const
{
CounterType elements;
if (!_full) {
if (_head < _tail) {
Expand All @@ -179,29 +334,30 @@ class CircularBuffer {
} else {
elements = BufferSize;
}
core_util_critical_section_exit();
return elements;
}

/** Peek into circular buffer without popping
/** Used to increment _tail or _head by a given value.
*
* @param data Data to be peeked from the buffer
* @return True if the buffer is not empty and data contains a transaction, false otherwise
* @param val The value of the counter to be incremented.
* @param increment The amount to be added, the value after this incremented must not exceed BufferSize.
* @return The new value of the counter.
*/
bool peek(T &data) const
CounterType incrementCounter(CounterType val, CounterType increment = 1)
{
bool data_updated = false;
core_util_critical_section_enter();
if (!empty()) {
data = _pool[_tail];
data_updated = true;
val += increment;

MBED_ASSERT(val <= BufferSize);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a private function so we can guarantee we will never exceed buffersize even after increment so we can simplify calculation


if (val == BufferSize) {
val = 0;
}
core_util_critical_section_exit();
return data_updated;

return val;
}

private:
T _pool[BufferSize];
T _buffer[BufferSize];
CounterType _head;
CounterType _tail;
bool _full;
Expand Down
Loading