libmaple/ring_buffer.h « LeafLabs Garden

LeafLabs Garden » Support » Maple Hardware Support

libmaple/ring_buffer.h

(8 posts) (3 voices)

Started 4 years ago by geoffreymbrown
Latest reply from mbolivar

geoffreymbrown
Member

This looks pretty broken:

static inline uint32 rb_full_count(ring_buffer *rb) {
return rb->tail - rb->head;
}

Suppose with have a 4 place ring buffer where tail = 0, head = 1 (i.e. there are
three items in the buffer). Type conversion isn't going to magically
return 3. I think what you want is

(rb->size + rb->tail - rb->head) & (rb->size - 1)

Also, your insert and remove routines are definitely unsafe. Notice that
in libmaple/uart.c, you don't check to see if the buffer is full. The right
thing to do here is probably throw the character in the trash since you don't
have flow control. Overflowing your buffer seems pretty crass.

I noticed that you call rb_remove in your usart_getc routine but also don't
check if the buffer is empty. At the least this should fail in benign way
rather than manufacturing a buffer full of trash.

Posted 4 years ago #
mbolivar
LeafLabs

Thanks for the report. This has been fixed in master for over a month:
https://github.com/leaflabs/libmaple/blob/master/libmaple/ring_buffer.h
https://github.com/leaflabs/libmaple/blob/master/libmaple/usart.c#L83

A snapshot was released with the new code (choose the 2011-02-13 snapshot release for your platform):
http://static.leaflabs.com/pub/leaflabs/maple-ide/snapshots/

Posted 4 years ago #
geoffreymbrown
Member

Ok, I missed that. You might still want to make remove do the right thing
when the buffer is empty.

Posted 4 years ago #
mbolivar
LeafLabs

I'd rather not force everyone to pay for checking if the buffer is empty in rb_remove(), so I've added an rb_safe_remove() (in the refactor branch) instead:

https://github.com/leaflabs/libmaple/commit/8097736f3be70ed3a5550334b31e8cfeddc0bfcf

Posted 4 years ago #
geoffreymbrown
Member

Isn't it easier to check "not empty" head != tail rather than full count != 0 ?

Posted 4 years ago #
mbolivar
LeafLabs

yes. sorry for the hasty implementation.

https://github.com/leaflabs/libmaple/commit/12aca5b72d62081c50fafc41e448841a4328a500

Posted 4 years ago #
x893
Member

Logic for count build only on head/tail calculations not correct.
Simple check any logic with next situation
i send 64 bytes (64 = size of buffer)
and rb_full_count(..) return zero !
For correct calculation need add counter to calculate insert/remove elements.
Correctly name ring_buffer - queue with maximum capacity and count of queued elements.

I implement this logic (counter/tail/head) in my CAN library and no lost elements in any case.

Posted 4 years ago #
mbolivar
LeafLabs

x893,

Actually, the size of the buffer is 63, not 64. One byte is always left free to distinguish empty from full. This is what makes the logic correct.

I was unable to reproduce your report, although it's possible that I misunderstood you. Can you please submit some sample code along with your description of the problem? For example, here is a commit I have just pushed into master which tests basic ring_buffer functionality:

https://github.com/leaflabs/libmaple/commit/987fa5f433e0cd41b1621dad8b4e331cf82d49f3

The relevant new file is examples/test-ring-buffer-insertion.cpp:

https://github.com/leaflabs/libmaple/blob/987fa5f433e0cd41b1621dad8b4e331cf82d49f3/examples/test-ring-buffer-insertion.cpp

Here is the output it gives on my Maple Rev 5; everything looks fine to me:

Beginning test.

resetting ring buffer.
ring buffer contents:
rb_push_insert()-ing 63 bytes.
rb_full_count(rb) = 63
ring buffer contents: 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63
------------------------------
resetting ring buffer.
ring buffer contents:
rb_push_insert()-ing 64 bytes.
rb_full_count(rb) = 63
ring buffer contents: 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64
------------------------------
resetting ring buffer.
ring buffer contents:
rb_safe_insert()-ing 63 bytes.
rb_full_count(rb) = 63
ring buffer contents: 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63
------------------------------
resetting ring buffer.
ring buffer contents:
rb_safe_insert()-ing 64 bytes.
rb_full_count(rb) = 63
ring buffer contents: 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63
------------------------------

Test finished.

In the future, when discussing problems, can you please report the version of libmaple or the Maple IDE that you are using? If you are using 0.0.9, the ring_buffer implementation had many bugs which have been fixed in subsequent snapshot releases.

Posted 4 years ago #

RSS feed for this topic

Reply

You must log in to post.