andyvand / arduino

Automatically exported from code.google.com/p/arduino
Other
0 stars 0 forks source link

Leonardo serial buffer overrun when sending more than 64B (from pc to leonardo). #998

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
What steps will reproduce the problem?
(For more details see thread http://arduino.cc/forum/index.php/topic,108270)
1.Upload the ArduinoISP to the leonardo
2.Open the serial monitor and change the baud rate to take away magic baud rate 
(this is a work around for defect 995: unintentional autoreset).
3.In the IDE change the board type to another arduino into which you want to 
burn a bootloader.
4.Use the leonardo to burn a bootloader into the other arduino.

What is the expected output? 
burning the bootloader should succeed.

What do you see instead?
When the ide makes the second call to avrdude (to actually write the bootloader 
into flash), avrdude outputs following message:

...
avrdude: Send: d [64] . [00] . [80] F [46] . [0c] . [94] 4 [34] . [1c] . 
...
avrdude: ser_recv(): programmer is not responding
avrdude: stk500_recv(): programmer is not responding

The [00] [80] indicates avrdude sends 128 bytes to the leonardo. This makes the 
leonardo's serial buffer of 64B overrun.

What version of the Arduino software are you using?
arduino-1.0.1

On what operating system?
Linux Dell4550 2.6.38-8-generic #42-Ubuntu SMP Mon Apr 11 03:31:50 UTC 2011 
i686 i686 i386 GNU/Linux

Which Arduino board are you using?
leonardo

Please provide any additional information below.

More detailed explanation is in message:
http://arduino.cc/forum/index.php/topic,108270.msg838797.html#msg838797

Also there is a possible solution in my fork at
https://github.com/PeterVH/Arduino.git

Original issue reported on code.google.com by peter.va...@belgacom.net on 30 Jul 2012 at 9:51

GoogleCodeExporter commented 9 years ago
From "Noriaki Mitsunaga" on the developers list:

I have found that Arduino Leonard sometimes throws away received
characters through USB CDC. This will happen if you send many bytes
from PC at once or the main routine on Arduino does not read serial
buffer in time.

The issue is caused as follows. When Leonard receives data, it calls
Serial.accept from ISR(USB_GEN_vect) in USBCore.cpp;

                while (USB_Available(CDC_RX))   // Handle received bytes (if any)
                        Serial.accept();

Serial.accept() is implemented in CDC.cpp;

void Serial_::accept(void)
{
        ring_buffer *buffer = &cdc_rx_buffer;
        int c = USB_Recv(CDC_RX);
        int i = (unsigned int)(buffer->head+1) % SERIAL_BUFFER_SIZE;

        if (i != buffer->tail) {
                buffer->buffer[buffer->head] = c;
                buffer->head = i;
        }
}

So it throws away received data if the buffer overflows before main
routine handles them. It can be avoided if we change as follows;

o Modify return value of accept() (in USBAPI.h)
virtual bool accept(void);
           ^^^^

o Modify above part of USBCore.cpp as;
                while (USB_Available(CDC_RX) && Serial.accept())        // Handle received
bytes (if any)
                        ;

o Modify accept() in CDC.cpp as;
bool Serial_::accept(void)
{
        ring_buffer *buffer = &cdc_rx_buffer;
        int i = (unsigned int)(buffer->head+1) % SERIAL_BUFFER_SIZE;

        // if we should be storing the received character into the location
        // just before the tail (meaning that the head would advance to the
        // current location of the tail), we're about to overflow the buffer
        // and so we don't write the character or advance the head.
        if (i != buffer->tail) {
                int c = USB_Recv(CDC_RX);
                if (c>=0) {
                  buffer->buffer[buffer->head] = c;
                  buffer->head = i;
                }
                return true;
        }
        return false;
}

It would not harm USB communication since PC (USB host) will postpone
sending data during USB device returns NAK. It might make delay
handling received data if interrupt routine is not called for a long
time after buffer becomes full. To handle that case we should modify
available, peek, and read in CDC.cpp so that they checks
USB_Available(CDC_RX).

How do you think?

// Noriaki Mitsunaga

Original comment by dmel...@gmail.com on 9 Aug 2012 at 12:46

GoogleCodeExporter commented 9 years ago
[deleted comment]
GoogleCodeExporter commented 9 years ago
Hi Noriaki,

I agree with your reasoning. 

Actually in my fork at https://github.com/PeterVH/Arduino.git, posted before, I 
proposed a fix that functionally does the same, it is just organized a bit 
differently:

1. in USBCore.cpp:
        ...
        // Start of Frame - happens every millisecond so we use it 
        // for TX and RX LED one-shot timing, too
        if (udint & (1<<SOFI))
        {
#ifdef CDC_ENABLED
                USB_Flush(CDC_TX);           // Send a tx frame if found
                if (USB_Available(CDC_RX))   // Handle received bytes (if any)
                        Serial.accept();
    ...

2. In CDC.cpp:

void Serial_::accept(void)
{
        ring_buffer *buffer = &cdc_rx_buffer;
        int i = (unsigned int)(buffer->head+1) % SERIAL_BUFFER_SIZE;

        // if we should be storing the received character into the location
        // just before the tail (meaning that the head would advance to the
        // current location of the tail), we're about to overflow the buffer
        // and so we don't write the character or advance the head.

        // while we have room to store a byte
        while (i != buffer->tail) {
                int c = USB_Recv(CDC_RX);
                if (c == -1)
                        break;  // no more data
                buffer->buffer[buffer->head] = c;
                buffer->head = i;

                i = (unsigned int)(buffer->head+1) % SERIAL_BUFFER_SIZE;
        }
}

Basically I migrated the while loop into _accept() so that the logic:
    while (room_in_sw buffer) {
        read_byte_from_hw
        store_byte_in_sw_buffer
    }
is concentrated in one place.

> ... we should modify available, peek, and read in CDC.cpp so that they checks
> USB_Available(CDC_RX).

Is this necessary? The irq will happen every msec anyway. If we want to see the 
data faster we should use see whether the hw has another irq source for this. 

Original comment by peter.va...@belgacom.net on 16 Aug 2012 at 11:15

GoogleCodeExporter commented 9 years ago
[deleted comment]
GoogleCodeExporter commented 9 years ago
I agree with the modification 1 and 2 in comment 3.

I have suggested to add USB_Available() to other functions since I did not know 
that interrupt routine is called so often. Experimentally, it seemed the 
Leonard works correctly without modifying available, peek, and others. Then I 
don't think it is necessary. 

Original comment by mnori...@gmail.com on 16 Aug 2012 at 11:41

GoogleCodeExporter commented 9 years ago
Ok, so what is next?

Original comment by peter.va...@belgacom.net on 16 Aug 2012 at 8:06

GoogleCodeExporter commented 9 years ago

Original comment by dmel...@gmail.com on 5 Sep 2012 at 1:05

GoogleCodeExporter commented 9 years ago
https://github.com/arduino/Arduino/commit/6ab2a9f95ee31e03dbebdf65cc16712e743c8c
ec

Original comment by dmel...@gmail.com on 13 Sep 2012 at 12:57