CircularBuffer class modification and test #5578

mprse · 2017-11-24T11:35:57Z

Description

This PR provides:

proposal of updating the CircularBuffer class (assertion for CounterType).
CircularBuffer test.

Status

READY

Migrations

NO

pan- · 2017-11-24T13:39:08Z

platform/CircularBuffer.h

@@ -30,21 +30,25 @@ namespace mbed {
 *
 *  @note Synchronization level: Interrupt safe
 */
-template<typename T, uint32_t BufferSize, typename CounterType = uint32_t>
+template<typename T, uint32_t BufferSize>


That's a breaking change; why not use a static assertion rather than removing the template parameter:

MBED_STATIC_ASSERT( (sizeof(CounterType) >= sizeof(uint32_t)) || (BufferSize < (((uint64_t) 1) << (sizeof(CounterType) * 8))), "Invalid BufferSize for the CounterType" );

Assertion is an alternative to this solution. I decided to make a proposal with removed template parameter since I can't see any advantage of having it. Why do we need it?

Provided solution is not complete. Also signed counter types should be forbidden. The following declaration is also invalid and will not be caught by the static assertion:
CircularBuffer<int, 255, char> cb;

CounterType allows user of the class to choose the most appropriate variable type for inner counters of the object; it might save few bytes by CircularBuffer. It would have been better if the type selection was made automatically based on the buffer size but it is here now and we have to live with it.

Provided solution is not complete. Also signed counter types should be forbidden. The following declaration is also invalid and will not be caught by the static assertion:
CircularBuffer<int, 255, char> cb;

The static assertion can handle the sign of the CounterType if you define a type traits that determine if an integral type is signed or unsigned.

We are committed to preserving backward compatibility for our user facing APIs for 5.x versions of Mbed OS.

bulislaw · 2017-11-27T11:28:32Z

platform/CircularBuffer.h

@@ -30,21 +30,25 @@ namespace mbed {
 *
 *  @note Synchronization level: Interrupt safe
 */
-template<typename T, uint32_t BufferSize, typename CounterType = uint32_t>
+template<typename T, uint32_t BufferSize>


We are committed to preserving backward compatibility for our user facing APIs for 5.x versions of Mbed OS.

bulislaw · 2017-11-27T11:36:28Z

platform/CircularBuffer.h

@@ -110,9 +118,10 @@ class CircularBuffer {
    }

    /** Get the number of elements currently stored in the circular_buffer */
-    CounterType size() const {
+    uint32_t size() const


We can't change APIs.

mprse · 2017-11-27T13:28:42Z

Modified this patch. In current version CounterType template parameter stays, but it must be unsigned type consistent with the BufferSize. Test has been also adapted.

deepikabhavnani · 2017-11-28T15:22:35Z

Linking the issue: #5384

pan- · 2017-11-28T16:05:55Z

platform/CircularBuffer.h

@@ -18,6 +18,20 @@

 #include "platform/mbed_critical.h"

+/* Detect if CounterType of the Circular buffer is of unsigned type. */


Could you put the traits in a namespace named internal within the mbed namespace.

Fixed as suggested.

CounterType is used to define type of _head and _tail counters. This may cause the following problems: - counters are used as array indexes so only unsigned types should be used for counters, - CounterType must be consistent with BufferSize and BufferSize is of uint32_t type (i.e. Circular Buffer with the following parameters: BufferSize = 1000, CounterType = unsigned char will not work properly).

0xc0170 · 2017-11-30T17:34:20Z

@mprse What was updated, please state to let reviewers aware this is ready for another round

mprse · 2017-11-30T18:16:08Z

I provided responses to review comments. All change requests have been processed.

bulislaw

@SenRamakri ping

0xc0170 · 2017-12-07T12:12:35Z

/morph build

mbed-ci · 2017-12-07T12:41:10Z

Build : SUCCESS

Build number : 663
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5578/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

mbed-ci · 2017-12-07T13:45:20Z

Test : SUCCESS

Build number : 489
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/5578/489

mbed-ci · 2017-12-07T13:46:13Z

Exporter Build : SUCCESS

Build number : 305
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/5578/

SeppoTakalo · 2017-12-21T10:24:35Z

This PR just broke all our Ci builds.

MBED_STATIC_ASSERT is a macro that is defined in platform/mbed_assert.h
If you include the header before any other header that is including the assert macros, then the build fails.

Provided fix in #5748

mprse · 2017-12-21T10:40:13Z

This PR just broke all our Ci builds.

Sorry for missing platform/mbed_assert.h include and thanks for provided fix.

0xc0170 · 2017-12-21T11:29:09Z

Thanks @SeppoTakalo . How come our all CI did not catch this one?

SeppoTakalo · 2017-12-21T11:33:12Z

@0xc0170 The cliapp that is build in the PR contains copy&paste from CircularBuffer.

The build that broke, is my test branch (soon to be merged to master), and there I have removed the CircularBuffer and I use the one from Mbed OS.

0xc0170 added the needs: review label Nov 24, 2017

pan- reviewed Nov 24, 2017

View reviewed changes

bulislaw reviewed Nov 27, 2017

View reviewed changes

mprse force-pushed the CircularBuffer_tests branch from 3100589 to 9e0d6b1 Compare November 27, 2017 13:26

pan- reviewed Nov 28, 2017

View reviewed changes

mprse added 2 commits November 29, 2017 11:26

Add CircularBuffer test.

a488424

mprse force-pushed the CircularBuffer_tests branch from 9e0d6b1 to a488424 Compare November 29, 2017 10:30

0xc0170 requested a review from SenRamakri December 1, 2017 12:43

bulislaw approved these changes Dec 5, 2017

View reviewed changes

SenRamakri approved these changes Dec 11, 2017

View reviewed changes

0xc0170 added release-version: 5.8.0-rc1 ready for merge and removed needs: review labels Dec 18, 2017

0xc0170 merged commit b89cf3f into ARMmbed:master Dec 20, 2017

0xc0170 removed the ready for merge label Dec 20, 2017

0xc0170 mentioned this pull request Dec 20, 2017

CircularBuffer API - Error checking #5384

Closed

SeppoTakalo mentioned this pull request Dec 21, 2017

Fix PR5578 #5748

Merged

sg- assigned SeppoTakalo Dec 21, 2017

		@@ -18,6 +18,20 @@

		#include "platform/mbed_critical.h"

		/* Detect if CounterType of the Circular buffer is of unsigned type. */

CircularBuffer class modification and test #5578

CircularBuffer class modification and test #5578

Uh oh!

Conversation

mprse commented Nov 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Status

Migrations

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mprse Nov 27, 2017 • 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!

pan- Nov 27, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

mprse commented Nov 27, 2017

Uh oh!

deepikabhavnani commented Nov 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Nov 30, 2017

Uh oh!

mprse commented Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bulislaw left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Dec 7, 2017

Uh oh!

mbed-ci commented Dec 7, 2017

Triggering tests

Uh oh!

mbed-ci commented Dec 7, 2017

Uh oh!

mbed-ci commented Dec 7, 2017

Uh oh!

SeppoTakalo commented Dec 21, 2017

Uh oh!

mprse commented Dec 21, 2017

Uh oh!

0xc0170 commented Dec 21, 2017

Uh oh!

SeppoTakalo commented Dec 21, 2017

Uh oh!

Uh oh!

mprse commented Nov 24, 2017 •

edited

Loading

mprse Nov 27, 2017 •

edited

Loading

pan- Nov 27, 2017 •

edited

Loading

mprse commented Nov 30, 2017 •

edited

Loading