next topic »

[on:]

I was looking at some of the code in the Arduino runtime library today, and I saw something that looked a bit fishy to me.  In the following code, it occurred to me that there could be a bug when rx_buffer_head wraps around from 127 back to 0:

Code:

int serialAvailable()
{
    return (rx_buffer_head - rx_buffer_tail) % RX_BUFFER_SIZE;
}

In that case, it occurred to me that you could have rx_buffer_head==0, and rx_buffer_tail==127.  Then serialAvailable() would return (0 - 127) % 128 == -127.  Or at least, that's the mod operator works on most computers I am used to.  

So I decided to test this out with a program before making a fool of myself!    I wrote the following code and ran it.

Code:

extern volatile int rx_buffer_head, rx_buffer_tail;

void setup()
{
    Serial.begin (9600);
}

void loop()
{
    cli();      // disable interrupts, to make absolutely sure we get consistent numbers
    int avail = Serial.available();
    int head  = rx_buffer_head;
    int tail  = rx_buffer_tail;
    sei();      // enable interrupts again

    if (avail) {
        Serial.print (" h=");
        Serial.print (head);
        Serial.print (" t=");
        Serial.print (tail);
        Serial.print (" a=");
        Serial.print (avail);
        Serial.println();

        Serial.read();  // read and discard one byte
    }
}

While running this code, I kept sending the Arduino board one character at a time over the serial port.  Here is the output, with a lot of unnecessary lines removed:

Code:

 h=1 t=0 a=1            
 h=2 t=1 a=1            
 h=3 t=2 a=1            
 h=4 t=3 a=1            
 h=5 t=4 a=1            
 h=6 t=5 a=1            
 h=7 t=6 a=1            
 h=8 t=7 a=1            
 h=9 t=8 a=1            
 h=10 t=9 a=1            
 h=11 t=10 a=1              
 h=12 t=11 a=1              
// ... over 100 lines deleted for brevity's sake ...
 h=122 t=121 a=1
 h=123 t=122 a=1
 h=124 t=123 a=1
 h=125 t=124 a=1
 h=126 t=125 a=1
 h=127 t=126 a=1
 h=0 t=127 a=129      // <=== *** RED ALERT ***  should have said a=1
 h=1 t=0 a=1
 h=2 t=1 a=1
 h=3 t=2 a=1
 h=4 t=3 a=1
 h=5 t=4 a=1
 h=6 t=5 a=1
 h=7 t=6 a=1
 h=8 t=7 a=1

As predicted, you get the wrong value returned by serialAvailable() whenever rx_buffer_head < rx_buffer_tail.  The reason it wasn't negative is because I am calling Serial.available(), which calls serialAvailable(), then typecasts the negative value to an unsigned byte.

To fix this problem, the function serialAvailable needs to be modified as follows:

Code:

int serialAvailable()
{
    return (RX_BUFFER_SIZE + rx_buffer_head - rx_buffer_tail) % RX_BUFFER_SIZE;
}

After applying this fix and running again, now I see correct behavior:

Code:

// ... blah blah blah
 h=124 t=123 a=1
 h=125 t=124 a=1
 h=126 t=125 a=1
 h=127 t=126 a=1
 h=0 t=127 a=1      // <===  Yes!  It works!
 h=1 t=0 a=1
 h=2 t=1 a=1
 h=3 t=2 a=1

I just thought I would mention this so that

• It could be fixed for Arduino 0008.
• People could edit their own copy of Arduino 0007 and fix it themselves, or
• They could be very careful calling Serial.available(), knowing it can return incorrect values.

[Reply #1 on:]

Whoops!

Good catch, I'll fix this for Arduino 0008.

Thanks much.

[Reply #2 on:]

Hey Mellis,

Thanks!

By the way, just wondering if you have seen the latest version of the library patch I did in the playground?  I was able to get rid of the link_order file that neither of us liked... didn't hear back from you after that change, so I just figured you have been pretty busy.  I'm excited about the possibility for my first time of seeing my small contribution show up in an open-source project, so please forgive me for pestering!    Also just want to make sure there are no roadblocks to prevent it from being included.  Looks like I might have someone help me figure out the problem with makefile build:

http://www.arduino.cc/cgi-bin/yabb2/YaBB.pl?num=1168127321

[Reply #3 on:]

Sorry, I have been busy.

The patch should certainly make it into Arduino 0008.  When I get a chance to look at it more closely, I'll give you more specific feedback.  If you need it soon (e.g. because you won't have time to work on it later), let me know and I'll make time for it.

Thanks again.

[Reply #4 on:]

To fix this problem, the function serialAvailable needs to be modified as follows:

Code:

int serialAvailable()
{
    return (RX_BUFFER_SIZE + rx_buffer_head - rx_buffer_tail) % RX_BUFFER_SIZE;
}

Nice catch on that bug.  I was just checking out circular buffers myself recently, and I have been doing a lot of bitwise stuff for Firmata, so I thought this recommendation from the circular buffer wikipedia page might be useful:

where
    * w represents the position of the write pointer;
    * r represents the position of the read pointer;
    * n is the size of the buffer.

(If n is a power of 2, the calculation can be simplified to a bit-wise AND of (w [ch8722] r) with (n [ch8722] 1).)

e.g.

Code:

int serialAvailable()
{
    return (rx_buffer_tail - rx_buffer_head) & (RX_BUFFER_SIZE - 1)
}

[Reply #5 on:]

I haven't parsed out most of the discussion above but can testify that Serial.available indeed has a bug.

In my program when a single ASCII 13 character gets left in the Serial buffer, Serial.available reports 131 or 135.

Paul Badger

[Reply #6 on:]

Changed the function and program works fine. For those who want to patch their Arduino .007 apps the function referenced above that you need to change is in arduino-007/lib/targets/arduino/wiring.c

Or at least that was my experience on a Mac.


Paul Badger

[Reply #7 on:]

Note that I have installed the "make your arduino sketch size smaller" patch on 0007 and it seems this fix is included.

[Reply #8 on:]

To fix this problem, the function serialAvailable needs to be modified as follows:

Code:

int serialAvailable()
{
    return (RX_BUFFER_SIZE + rx_buffer_head - rx_buffer_tail) % RX_BUFFER_SIZE;
}

Nice catch on that bug.  I was just checking out circular buffers myself recently, and I have been doing a lot of bitwise stuff for Firmata, so I thought this recommendation from the circular buffer wikipedia page might be useful:

where
    * w represents the position of the write pointer;
    * r represents the position of the read pointer;
    * n is the size of the buffer.

(If n is a power of 2, the calculation can be simplified to a bit-wise AND of (w [ch8722] r) with (n [ch8722] 1).)

e.g.

Code:

int serialAvailable()
{
    return (rx_buffer_tail - rx_buffer_head) & (RX_BUFFER_SIZE - 1)
}

Isn't rx_buffer_head the write pointer? In that case it should be (rx_buffer_head - rx_buffer_tail), not the other way around...