Pages: [1]   Go Down
Author Topic: found bug in Serial.available()  (Read 952 times)
0 Members and 1 Guest are viewing this topic.
Florida, USA
Offline Offline
Full Member
***
Karma: 0
Posts: 146
meow!
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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!  smiley  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.
Logged

Forum Administrator
Cambridge, MA
Offline Offline
Faraday Member
*****
Karma: 9
Posts: 3538
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

Whoops!

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

Thanks much.
Logged

Florida, USA
Offline Offline
Full Member
***
Karma: 0
Posts: 146
meow!
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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!  smiley  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
Logged

Forum Administrator
Cambridge, MA
Offline Offline
Faraday Member
*****
Karma: 9
Posts: 3538
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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.
Logged

Brooklyn, NY, USA
Offline Offline
Full Member
***
Karma: 0
Posts: 115
arduino for all
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

Quote

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:

Quote
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)
}
Logged

0
Offline Offline
Sr. Member
****
Karma: 0
Posts: 267
dinosaur cork
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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
Logged

0
Offline Offline
Sr. Member
****
Karma: 0
Posts: 267
dinosaur cork
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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
Logged

0
Offline Offline
Jr. Member
**
Karma: 0
Posts: 81
everywhen
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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

0
Offline Offline
Newbie
*
Karma: 0
Posts: 15
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Quote
Quote

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:

Quote
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...
Logged

Pages: [1]   Go Up
Jump to: