Add mutiple push and pop for circular buffer #13563

paul-szczepanek-arm · 2020-09-07T12:35:18Z

Summary of changes

Add functions to circular buffer to allow pushing and popping multiple items at a time.

Impact of changes

Migration actions required

Documentation

None

Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[x] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[x] Tests / results supplied as part of this PR

Reviewers

@pan-

pan-

Thanks Paul, that is a useful addition. Would it be possible to preserve the doc. It has been modified for bool pop(T& data).

I'm not convinced we need an array overload AND a span overload, the span should be enough.

@kjbracey-arm What do you think of these additions ? I'm still not sure about the return of the pop(Span<T>&).

ciarmcom · 2020-09-07T13:00:30Z

@paul-szczepanek-arm, thank you for your changes.
@pan- @ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

paul-szczepanek-arm · 2020-09-07T13:05:03Z

Thanks Paul, that is a useful addition. Would it be possible to preserve the doc. It has been modified for bool pop(T& data).

Can you please comment on the line that is wrong and point out the error? The old docs had grammar mistakes and just plain incorrect text presumably based on the original implementation.

I'm not convinced we need an array overload AND a span overload, the span should be enough.

~~Which one do you want me to remove?~~ OK, I assume both pop an push

I'm still not sure about the return of the pop(Span<T>&).

You're the one who asked for it not 10 minutes ago. What do you want it to return instead?

kjbracey · 2020-09-07T13:18:49Z

platform/include/platform/CircularBuffer.h

-     * @return True if the buffer is empty, false if not
+     * @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)


There's usually no reason to pass spans by reference rather than value - I've seen doing so mess with code generation in practice. If you did want to pass by reference, this should be const span & - the lack of const would be getting in the way of the automatic conversion from array to span, I think.

Return type seems reasonable to me, but ARM ABI does cause crummy code generation sometimes because of it, even with inlining. So returning just popped might be more efficient, but a span return is probably more idiomatic. No strong feelings.

It's already const, I don't mind what is returned by pop either so unless either of you has opinions I'm leaving it.

I see, it wasn't const before the span commit, must've been a change that didn't get saved

Thanks for the feedback @kjbracey-arm .

If a span is returned then I believe it can be a const span & in input. The container in input is not altered and we return the content from the function as a result.

For compiler optimizations, I suppose one can use the non span function if it matters.

I initially asked Paul to return a span to allow writing code like this:

uint8_t chunk[50]; auto popped_chunk = queue.pop(chunk); for (auto element : popped_chunk) { // do something with the element } // or directly for (auto element : queue.pop(chunk)) { // do something with the element }

OK, I'm passing it by value, thanks

Certainly want to pass spans in by value normally. Doing it by reference isn't quite as hard to read as int * const &ptr, but it's just as weird, and does generate worse code.

I'm fine with span return.

platform/include/platform/CircularBuffer.h

evedon · 2020-09-07T14:30:12Z

@paul-szczepanek-arm Thanks for the PR. We will need tests and documentation for the new functionality.

donatieng

Thanks @paul-szczepanek-arm, would you be able to amend the unit tests for the circular buffer class?
https://github.com/ARMmbed/mbed-os/blob/master/platform/tests/UNITTESTS/CircularBuffer/test_CircularBuffer.cpp (if we can call these "tests" 😅)

donatieng · 2020-09-08T14:11:51Z

Ah sorry @evedon hadn't seen you were already on the case :)

Pull request has been modified.

paul-szczepanek-arm · 2020-09-08T19:39:57Z

Unit tests added, documentation in doxygen and example expanded to cover multiple push ARMmbed/mbed-os-examples-docs_only#120

kjbracey

I think the % isn't acceptable. Had so many slow device issues with slow division in critical places.

This would be somewhat alleviated by #13289, but it's still something to consider. (Although I note that may not be coming - hadn't checked it for a while - thought it had been merged).

kjbracey · 2020-09-10T09:11:42Z

platform/include/platform/CircularBuffer.h

+
+    CounterType incrementCounter(CounterType val)
+    {
+        return (++val) % BufferSize;


If BufferSize isn't a power of two this could be very inefficient on M0-type devices - using an actual software division routine - which would be bad in an interrupt context. You're better off doing a comparison. You know val can't be greater than BufferSize to start with - the compiler doesn't.

Would it make sense to force a power of two for the size?

Would be a backwards-incompatible API change.

kjbracey · 2020-09-10T09:15:14Z

platform/include/platform/CircularBuffer.h

+            }
+
+            _head += written;
+            _head %= BufferSize;


Same comment as for the other %.

kjbracey · 2020-09-10T09:16:39Z

platform/include/platform/CircularBuffer.h

+            }
+
+            _tail += data_popped;
+            _tail %= BufferSize;


kjbracey · 2020-09-10T09:18:59Z

platform/include/platform/CircularBuffer.h

+    mbed::Span<T> pop(mbed::Span<T> dest)
+    {
+        CounterType popped = pop(dest.data(), dest.size());
+        return mbed::make_Span(dest.data(), popped);


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.

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.

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

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

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.)

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?

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.

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).

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

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

kjbracey · 2020-09-10T09:21:30Z

platform/include/platform/CircularBuffer.h

+     *
+     * @return The number of elements popped.
+     */
+    CounterType pop(T* dest, CounterType len)


Astyle should be nagging you to make this T *dest. Isn't it?

paul-szczepanek-arm · 2020-09-10T11:43:59Z

Thank you, @kjbracey-arm, I've changed the counter calculation to avoid %

paul-szczepanek-arm · 2020-09-10T11:44:34Z

platform/include/platform/CircularBuffer.h

@@ -129,23 +131,21 @@ class CircularBuffer {
            CounterType written = len;

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


this is the same, just changed for readability

Pull request has been modified.

paul-szczepanek-arm · 2020-09-10T11:46:47Z

platform/include/platform/CircularBuffer.h

-        return (++val) % BufferSize;
+        val += increment;
+
+        MBED_ASSERT(val <= BufferSize);


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

Pull request has been modified.

0xc0170 · 2020-09-10T13:00:21Z

CI started

0xc0170 · 2020-09-10T13:01:11Z

allow poppiong and pushing of multiplke items in circular buf

Typo in the commit msg for the first commit. Lets fix it after CI run.

0xc0170 · 2020-09-10T13:01:43Z

Or wait, we got something in the queue, faster is if I abort now. aborting the CI

paul-szczepanek-arm · 2020-09-10T14:03:11Z

fixed the typos in the commit message (I had two!)

Pull request has been modified.

mbed-ci · 2020-09-10T15:57:13Z

Jenkins CI Test : ❌ FAILED

Build Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	❌
jenkins-ci/mbed-os-ci_build-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-ARM	✔️

0xc0170 · 2020-09-15T07:42:14Z

I restarted tests

kjbracey · 2020-09-15T08:02:14Z

One general thought on the class - this extension preserves the existing "silently overwrite existing data when pushing" API, which I've yet to see a use for. Over on a customer branch I added try_push as an optimisation to refuse when full, which is what BufferedSerial really needs.

When that gets merged, we'd want try_push forms of these as well, returning number of items successfully pushed.

mbed-ci · 2020-09-15T08:29:54Z

Jenkins CI Test : ❌ FAILED

Build Number: 3 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	❌
jenkins-ci/mbed-os-ci_build-ARM	✔️
jenkins-ci/mbed-os-ci_build-GCC_ARM	✔️

Pull request has been modified.

0xc0170 · 2020-09-16T10:06:38Z

CI restarted

mbed-ci · 2020-09-16T12:04:30Z

Jenkins CI Test : ❌ FAILED

Build Number: 4 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_build-ARM	✔️
jenkins-ci/mbed-os-ci_build-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_dynamic-memory-usage	❌
jenkins-ci/mbed-os-ci_cloud-client-pytest	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️

0xc0170 · 2020-09-17T07:27:26Z

I had to restart entire pipeline

mbed-ci · 2020-09-17T08:25:51Z

Jenkins CI Test : ✔️ SUCCESS

Build Number: 5 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_build-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-ARM	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️
jenkins-ci/mbed-os-ci_dynamic-memory-usage	✔️
jenkins-ci/mbed-os-ci_cloud-client-pytest	✔️

pan- reviewed Sep 7, 2020

View reviewed changes

ciarmcom added the release-type: feature label Sep 7, 2020

ciarmcom requested review from pan- and a team September 7, 2020 13:00

ciarmcom added the needs: review label Sep 7, 2020

kjbracey reviewed Sep 7, 2020

View reviewed changes

donatieng previously requested changes Sep 8, 2020

View reviewed changes

mergify bot added needs: work and removed needs: review labels Sep 8, 2020

paul-szczepanek-arm force-pushed the circ-buf branch from af0e32b to f5d2d7f Compare September 8, 2020 19:08

paul-szczepanek-arm mentioned this pull request Sep 8, 2020

And mutliple push to circular buffer example ARMmbed/mbed-os-examples-docs_only#120

Open

paul-szczepanek-arm requested review from donatieng and kjbracey September 9, 2020 15:49

kjbracey previously requested changes Sep 10, 2020

View reviewed changes

kjbracey mentioned this pull request Sep 10, 2020

Replace microlib arithmetic #13289

Closed

paul-szczepanek-arm requested a review from kjbracey September 10, 2020 11:44

paul-szczepanek-arm commented Sep 10, 2020

View reviewed changes

kjbracey previously approved these changes Sep 10, 2020

View reviewed changes

paul-szczepanek-arm force-pushed the circ-buf branch from 864952f to 4ca8f99 Compare September 10, 2020 12:32

0xc0170 added needs: CI and removed needs: review labels Sep 10, 2020

paul-szczepanek-arm added 2 commits September 10, 2020 15:00

allow popping and pushing of multiple items in circular buffer

d4eaa48

unit tests for circular buffer

3d827e8

paul-szczepanek-arm force-pushed the circ-buf branch from 4ca8f99 to 3d827e8 Compare September 10, 2020 14:02

kjbracey previously approved these changes Sep 15, 2020

View reviewed changes

missing dependencies

b335675

0xc0170 approved these changes Sep 17, 2020

View reviewed changes

0xc0170 added ready for merge and removed needs: CI labels Sep 17, 2020

0xc0170 merged commit 2ab7b44 into ARMmbed:master Sep 21, 2020

mergify bot removed the ready for merge label Sep 21, 2020

mbedmain added release-version: 6.4.0 Release-pending and removed release-type: feature Release-pending labels Oct 20, 2020

Add mutiple push and pop for circular buffer #13563

Add mutiple push and pop for circular buffer #13563

Uh oh!

Conversation

paul-szczepanek-arm commented Sep 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

pan- left a comment

Choose a reason for hiding this comment

Uh oh!

ciarmcom commented Sep 7, 2020

Uh oh!

paul-szczepanek-arm commented Sep 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

evedon commented Sep 7, 2020

Uh oh!

donatieng left a comment

Choose a reason for hiding this comment

Uh oh!

donatieng commented Sep 8, 2020

Uh oh!

paul-szczepanek-arm commented Sep 8, 2020

Uh oh!

kjbracey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjbracey Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paul-szczepanek-arm commented Sep 10, 2020

paul-szczepanek-arm commented Sep 7, 2020 •

edited

Loading

paul-szczepanek-arm commented Sep 7, 2020 •

edited

Loading

kjbracey Sep 15, 2020 •

edited

Loading