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(): get available transactions #5058

Merged
merged 4 commits into from Nov 9, 2017

Conversation

Projects
None yet
10 participants
@0x6d61726b
Contributor

0x6d61726b commented Sep 8, 2017

Description

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

Status

READY

Todos

  • Documentation
CircularBuffer(): get available transactions
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.

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

This comment has been minimized.

@0xc0170

0xc0170 Sep 19, 2017

Member

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 0xc0170 added needs: work and removed needs: review labels Sep 19, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 19, 2017

@pan-

This comment has been minimized.

Member

pan- commented Sep 19, 2017

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

This comment has been minimized.

Member

0xc0170 commented Sep 26, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Oct 5, 2017

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

@0x6d61726b

This comment has been minimized.

Contributor

0x6d61726b commented Oct 5, 2017

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-

This comment has been minimized.

Member

pan- commented Oct 6, 2017

Thanks for the changes.

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

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

This comment has been minimized.

@0xc0170
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 9, 2017

Thanks for the changes.

@pan- Happy with the new method?

@pan-

This comment has been minimized.

Member

pan- commented Oct 9, 2017

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 13, 2017

retest uvisor

@adbridge

This comment has been minimized.

Contributor

adbridge commented Oct 16, 2017

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

@0x6d61726b

This comment has been minimized.

Contributor

0x6d61726b commented Oct 16, 2017

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-

This comment has been minimized.

Member

pan- commented Oct 17, 2017

Thank for the clarification.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 17, 2017

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

This comment has been minimized.

Member

0xc0170 commented Oct 19, 2017

@0x6d61726b Any update?

@0x6d61726b

This comment has been minimized.

Contributor

0x6d61726b commented Oct 20, 2017

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

This comment has been minimized.

Member

mprse commented Oct 23, 2017

(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

This comment has been minimized.

Member

bulislaw commented Oct 23, 2017

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

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Oct 23, 2017

@0xc0170 Could you review?

@0xc0170

Other than this one last change request, LGTM

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

This comment has been minimized.

@0xc0170

0xc0170 Oct 23, 2017

Member

} else on one line

@adbridge

This comment has been minimized.

Contributor

adbridge commented Oct 31, 2017

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

@0x6d61726b

This comment has been minimized.

Contributor

0x6d61726b commented Oct 31, 2017

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

@adbridge

This comment has been minimized.

Contributor

adbridge commented Oct 31, 2017

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

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Nov 6, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Nov 6, 2017

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

This comment has been minimized.

@0xc0170

0xc0170 approved these changes Nov 7, 2017

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

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Nov 8, 2017

/morph test

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 8, 2017

/morph uvisor-test

2 similar comments
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 9, 2017

/morph uvisor-test

@alzix

This comment has been minimized.

Contributor

alzix commented Nov 9, 2017

/morph uvisor-test

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

5 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build 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

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

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