CircularBuffer(): get available transactions #5058

0x6d61726b · 2017-09-08T13:35:07Z

Description

This implementation returns the number of available (stored) transactions in the buffer. It was introduced to improve efficiency.

Status

READY

Todos

Documentation

This implementation returns the number of available (stored) transactions in the buffer

0xc0170

Please align the code style.

The name available or size() ? From boost circular buffer:

size_type size() const - Get the number of elements currently stored in the circular_buffer.

0xc0170 · 2017-09-19T06:55:14Z

platform/CircularBuffer.h

-
+
+    /** Returns the number of available transactions the buffer contains */
+    CounterType available() {


Can you please fix the code style : https://docs.mbed.com/docs/mbed-os-handbook/en/latest/cont/code_style/ (the file uses it as well, the only exception here is the body of the function :-) )

0xc0170 · 2017-09-19T07:06:27Z

cc @pan- @geky

pan- · 2017-09-19T10:08:16Z

It was introduced to improve efficiency.

How is it supposed to improve efficiency ?

This implementation returns the number of available (stored) transactions in the buffer.

The buffer store elements not transactions and it would be better better to stick with the familiar C++ idiom size and capacity. The size being equal to the number of elements present in the container and the capacity being equal to the number of elements that can be stored in the container.

0xc0170 · 2017-09-26T13:56:15Z

@0x6d61726b bump

theotherjimmy · 2017-10-05T15:51:52Z

@0x6d61726b bump. Is there any intention to progress this further? If not, can we close this?

0x6d61726b · 2017-10-05T18:49:54Z

How is it supposed to improve efficiency ?

It allows to determine the number of used elements before processing them.

I hope I made all the changes you intended me to do?

pan- · 2017-10-06T09:27:29Z

Thanks for the changes.

Could you provide an example (in code) showing how should be used to improve efficiency ?

0xc0170 · 2017-10-09T13:29:58Z

platform/CircularBuffer.h

+    {
+        core_util_critical_section_enter();
+        CounterType elements;
+        if (!_full)


Please use https://docs.mbed.com/docs/mbed-os-handbook/en/latest/cont/code_style/ (as it is in this file)

0xc0170 · 2017-10-09T13:30:16Z

Thanks for the changes.

@pan- Happy with the new method?

pan- · 2017-10-09T13:40:13Z

Happy with the name changes, however I would still like a code example demonstrating how it should be used to improve efficiency.

0xc0170 · 2017-10-13T10:42:53Z

retest uvisor

adbridge · 2017-10-16T15:45:14Z

@0x6d61726b could you please address @pan- comments?

0x6d61726b · 2017-10-16T21:47:38Z

The example is about knowing the size of the buffer, so a following routine will not be triggered until the buffer contains a certain amount of expected data. The current implementation does not allow any knowledge about the stored amount of data in the buffer.
Providing an example requires some time because I am on vacation right now.

pan- · 2017-10-17T08:19:38Z

Thank for the clarification.

0xc0170 · 2017-10-17T12:40:38Z

The last oustanding item for this patch is to align the our code style, plus would be good if the new method is tested (we are lacking the test for Circularbuffer, that shall be added, @fkjagodzinski @mprse for visiblity).

0xc0170 · 2017-10-19T15:20:22Z

@0x6d61726b Any update?

0x6d61726b · 2017-10-20T20:07:16Z

I updated the code style. Hopefully it complies now, if not let me know whats wrong.
Here is an excerpt of the CircularBuffer size operation I use:

CircularBuffer<uint8_t, 256> circbuf;

// check if buffer contains at least the header
if (circbuf.size() >= 16)
{
    // parse header
}

mprse · 2017-10-23T08:20:19Z

(we are lacking the test for Circularbuffer, that shall be added, @fkjagodzinski @mprse for visiblity).
@0xc0170 @bulislaw
I can create tests for Circular buffer. Do we have task in Jira for this?

bulislaw · 2017-10-23T14:16:57Z

@mprse not yet, I'll create some more tasks soon.

theotherjimmy · 2017-10-23T15:13:59Z

@0xc0170 Could you review?

0xc0170

Other than this one last change request, LGTM

0xc0170 · 2017-10-23T21:32:45Z

platform/CircularBuffer.h

+            if (_head < _tail) {
+                elements = BufferSize + _head - _tail;
+            }
+            else {


} else on one line

adbridge · 2017-10-31T14:49:53Z

@0x6d61726b Could you please address @0xc0170 review comment above ?

0x6d61726b · 2017-10-31T15:09:28Z

@adbridge thanks for the reminder, I just did it.

adbridge · 2017-10-31T15:24:02Z

@0x6d61726b Thanks, LGTM, @0xc0170 can you please confirm you are happy with this now?

studavekar · 2017-11-06T05:04:38Z

/morph build

mbed-ci · 2017-11-06T05:27:10Z

Build : SUCCESS

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

Triggering tests

/morph test
/morph uvisor-test

mbed-ci · 2017-11-06T07:15:15Z

Test : FAILURE

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

studavekar · 2017-11-08T05:08:08Z

/morph test

mbed-ci · 2017-11-08T06:48:26Z

Test : SUCCESS

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

0xc0170 · 2017-11-08T14:32:36Z

/morph uvisor-test

0xc0170 · 2017-11-09T10:38:35Z

/morph uvisor-test

alzix · 2017-11-09T12:46:13Z

/morph uvisor-test

CircularBuffer(): get available transactions

839cd7e

This implementation returns the number of available (stored) transactions in the buffer

theotherjimmy added the needs: review label Sep 14, 2017

0xc0170 requested changes Sep 19, 2017

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Sep 19, 2017

updates according to comments in pull request

69e6f11

0xc0170 reviewed Oct 9, 2017

View reviewed changes

Code style updated

c1ab43c

theotherjimmy added needs: review and removed needs: work labels Oct 23, 2017

0xc0170 requested changes Oct 23, 2017

View reviewed changes

platform/CircularBuffer.h Outdated

if (_head < _tail) {

elements = BufferSize + _head - _tail;

}

else {

Copy link

Contributor

0xc0170 Oct 23, 2017

Choose a reason for hiding this comment

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

} else on one line

code style adopted

830db6a

0xc0170 approved these changes Nov 7, 2017

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels Nov 7, 2017

0xc0170 added ready for merge release-version: 5.7.0-rc1 and removed needs: CI labels Nov 9, 2017

0xc0170 merged commit 589d76e into ARMmbed:master Nov 9, 2017

0xc0170 removed the ready for merge label Nov 9, 2017



		/** Returns the number of available transactions the buffer contains */
		CounterType available() {

CircularBuffer(): get available transactions #5058

CircularBuffer(): get available transactions #5058

Uh oh!

Conversation

0x6d61726b commented Sep 8, 2017 • edited by theotherjimmy Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Status

Todos

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 Sep 19, 2017

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Sep 19, 2017

Uh oh!

pan- commented Sep 19, 2017

Uh oh!

0xc0170 commented Sep 26, 2017

Uh oh!

theotherjimmy commented Oct 5, 2017

Uh oh!

0x6d61726b commented Oct 5, 2017

Uh oh!

pan- commented Oct 6, 2017

Uh oh!

0xc0170 Oct 9, 2017

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Oct 9, 2017

Uh oh!

pan- commented Oct 9, 2017

Uh oh!

0xc0170 commented Oct 13, 2017

Uh oh!

adbridge commented Oct 16, 2017

Uh oh!

0x6d61726b commented Oct 16, 2017

Uh oh!

pan- commented Oct 17, 2017

Uh oh!

0xc0170 commented Oct 17, 2017

Uh oh!

0xc0170 commented Oct 19, 2017

Uh oh!

0x6d61726b commented Oct 20, 2017

Uh oh!

mprse commented Oct 23, 2017

Uh oh!

bulislaw commented Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theotherjimmy commented Oct 23, 2017

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 Oct 23, 2017

Choose a reason for hiding this comment

Uh oh!

adbridge commented Oct 31, 2017

Uh oh!

0x6d61726b commented Oct 31, 2017

Uh oh!

adbridge commented Oct 31, 2017

Uh oh!

studavekar commented Nov 6, 2017

Uh oh!

mbed-ci commented Nov 6, 2017

Build : SUCCESS

Triggering tests

Uh oh!

mbed-ci commented Nov 6, 2017

Test : FAILURE

Uh oh!

studavekar commented Nov 8, 2017

Uh oh!

mbed-ci commented Nov 8, 2017

0x6d61726b commented Sep 8, 2017 •

edited by theotherjimmy

Loading

bulislaw commented Oct 23, 2017 •

edited

Loading