Skip to content

ringbuffer: versatility and optimizations#1101

Merged
Kijewski merged 8 commits intoRIOT-OS:masterfrom
Kijewski:issue-979-2
Jul 28, 2014
Merged

ringbuffer: versatility and optimizations#1101
Kijewski merged 8 commits intoRIOT-OS:masterfrom
Kijewski:issue-979-2

Conversation

@Kijewski
Copy link
Copy Markdown
Contributor

@Kijewski Kijewski commented May 3, 2014

This is my second take on #933, to make the ringbuffer more versatile.

  • This PR is rebased on sys: rename ringbuffer functions #1099.
  • It removes clutter from the code (but might need unit tests).
  • It adds peek functionality, and helper functions to test if the ringbuffer is full/empty.
  • It optimizes the parameter usage (unsigned types for lengths, restrict).
  • It makes the implementation easier to understand by dropping the explicit pointer to the end of the buffer. This also strips sizeof (void *) bytes per ringbuffer.
  • ringbuffer_add() won't overwrite existing content. ringbuffer_get_one() will tell what was overwritten.
  • It adds the missing documentation for the library.

Please see the individual commits.

@mehlis
Copy link
Copy Markdown
Contributor

mehlis commented May 4, 2014

@kaspar030 can you review?

@Kijewski
Copy link
Copy Markdown
Contributor Author

Kijewski commented May 6, 2014

Don't be afraid to review this change, please. ;)
The functions add_tail() and get_head() of 9007bb7 should make the implementation easy to understand.

@miri64
Copy link
Copy Markdown
Member

miri64 commented May 6, 2014

Looks good. If you are at it: could you write some unittests?

@Kijewski
Copy link
Copy Markdown
Contributor Author

Kijewski commented May 6, 2014

I added the test for ringbuffer_(add|get)_one().

@miri64
Copy link
Copy Markdown
Member

miri64 commented May 6, 2014

Is there a reason for you not using embunit?

@Kijewski
Copy link
Copy Markdown
Contributor Author

Kijewski commented May 6, 2014

Is lazyness a valid reason? :-/

@Kijewski Kijewski mentioned this pull request May 8, 2014
@miri64
Copy link
Copy Markdown
Member

miri64 commented May 9, 2014

Since the framework takes most of the work away from you: no.

@miri64
Copy link
Copy Markdown
Member

miri64 commented May 9, 2014

But I had something laying around anyways: I'll send you a PR as soon as I fitted it to your PR

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

s/rb_add_element()/ringbuffer_add_one()/

@miri64 miri64 mentioned this pull request May 9, 2014
@Kijewski
Copy link
Copy Markdown
Contributor Author

@authmillenon is 418e24e useful for your use case?

@miri64
Copy link
Copy Markdown
Member

miri64 commented May 11, 2014

I find it pretty complicated oO
why not

char *ringbuffer_allocate(ringbuffer_t *restrict rb, unsigned num, unsigned size);

@Kijewski
Copy link
Copy Markdown
Contributor Author

Because the buffer can wrap around.

@OlegHahm OlegHahm assigned miri64 and unassigned mehlis May 16, 2014
@OlegHahm OlegHahm removed this from the Release NEXT MAJOR milestone Jun 3, 2014
@Kijewski
Copy link
Copy Markdown
Contributor Author

Kijewski commented Jun 3, 2014

@authmillenon are you OK with my explanation, or do you think https://github.com/Kijewski/RIOT/commit/195061ad5c88e6bf4b76ef51bfaf16a0e47f394c isn't worth it?

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 3, 2014

I do not really need it anymore and it seems very complicated for an "lightweight ringbuffer implementation", so I say: kick it out ;-)

@Kijewski
Copy link
Copy Markdown
Contributor Author

Kijewski commented Jun 3, 2014

@Kijewski
Copy link
Copy Markdown
Contributor Author

Ping.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

overwring?

@Kijewski Kijewski added this to the FIX ME FIRST milestone Jun 18, 2014
@Kijewski
Copy link
Copy Markdown
Contributor Author

Marked as "fix me first", because #935 depends on it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Doesn't rb->avail give the number of available slots in the ringbuffer? Does this addition make sense here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, an element is added to the tail, so the number of elements will be increased by one.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, I misinterpreted the meaning of available.

@Kijewski
Copy link
Copy Markdown
Contributor Author

Kijewski commented Jul 9, 2014

I made the test a unittest.

@haukepetersen
Copy link
Copy Markdown
Contributor

As you added the unittests, I gues you can remove the old test?!

Kijewski added 8 commits July 28, 2014 20:44
This patch add `peek` functionality, and empty and full helpers.
It is a bad idea to use signed types for lengths.
Mark pointers a `restrict`, since the ringbuffer is not thread safe
anyway.
There is no need for an explicit pointer to the end of the buffer.
@Kijewski
Copy link
Copy Markdown
Contributor Author

Rebased, old tests removed, typo fixed.

@haukepetersen
Copy link
Copy Markdown
Contributor

looks good to me: ACK

Kijewski added a commit that referenced this pull request Jul 28, 2014
ringbuffer: versatility and optimizations
@Kijewski Kijewski merged commit 2c885ba into RIOT-OS:master Jul 28, 2014
@Kijewski Kijewski deleted the issue-979-2 branch July 28, 2014 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants