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

CircularBuffer class modification and test #5578

Merged
merged 2 commits into from Dec 20, 2017

Conversation

Projects
None yet
8 participants
@mprse
Member

mprse commented Nov 24, 2017

Description

This PR provides:

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

Status

READY

Migrations

NO

platform/CircularBuffer.h Outdated
@@ -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>

This comment has been minimized.

@pan-

pan- Nov 24, 2017

Member

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"
    );

This comment has been minimized.

@mprse

mprse Nov 27, 2017

Member

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;

This comment has been minimized.

@pan-

pan- Nov 27, 2017

Member

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.

This comment has been minimized.

@bulislaw

bulislaw Nov 27, 2017

Member

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

platform/CircularBuffer.h Outdated
@@ -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>

This comment has been minimized.

@bulislaw

bulislaw Nov 27, 2017

Member

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

platform/CircularBuffer.h Outdated
@@ -110,9 +118,10 @@ class CircularBuffer {
}
/** Get the number of elements currently stored in the circular_buffer */
CounterType size() const {
uint32_t size() const

This comment has been minimized.

@bulislaw

bulislaw Nov 27, 2017

Member

We can't change APIs.

@mprse mprse force-pushed the mprse:CircularBuffer_tests branch Nov 27, 2017

@mprse

This comment has been minimized.

Member

mprse commented Nov 27, 2017

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

This comment has been minimized.

Contributor

deepikabhavnani commented Nov 28, 2017

Linking the issue: #5384

platform/CircularBuffer.h Outdated
@@ -18,6 +18,20 @@
#include "platform/mbed_critical.h"
/* Detect if CounterType of the Circular buffer is of unsigned type. */

This comment has been minimized.

@pan-

pan- Nov 28, 2017

Member

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

This comment has been minimized.

@mprse

mprse Nov 29, 2017

Member

Fixed as suggested.

mprse added some commits Nov 7, 2017

Add assertion to make CounterType consistent with BufferSize.
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).

@mprse mprse force-pushed the mprse:CircularBuffer_tests branch to a488424 Nov 29, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 30, 2017

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

@mprse

This comment has been minimized.

Member

mprse commented Nov 30, 2017

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

@0xc0170 0xc0170 requested a review from SenRamakri Dec 1, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Dec 7, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Dec 7, 2017

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

This comment has been minimized.

@mbed-ci

This comment has been minimized.

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

6 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@SeppoTakalo SeppoTakalo referenced this pull request Dec 21, 2017

Merged

Fix PR5578 #5748

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Dec 21, 2017

This PR just broke all our Ci builds.

screen shot 2017-12-21 at 12 19 17

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

This comment has been minimized.

Member

mprse commented Dec 21, 2017

This PR just broke all our Ci builds.

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Dec 21, 2017

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

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Dec 21, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment