Woohoo!
I just got back from work and noticed the problem is diagnosed, fixed, tested and committed in git-hub!
Thanks guys.
Nevertheless I still feel we should think about the approach taken to avoid entering accept() from the isr. This is what happens without the guards:
- bytes arrive for the 1st time from the host
- the isr fills up the serial rx buffer, it can't free the fifo because it can only write 511 bytes and it has 512 bytes available.
- the sketch performs SerialUSB.read() so it reads 1 byte.
- so we end up in accept() which calls USBD_Recv().
This function automatically frees the fifo if we read the last byte from it! I think this is where the problem lies. This causes a new irq to arrive while we are still in our loop and did not yet update the head ptr of the rx buffer.
Therefore I tried with the function
UDD_Recv8(): it does not automatically free the fifo, so we can finish our loop and call udd_ack_fifocon(CDC_RX) when we are done accessing the state in rx buffer.
I could not see the problem yesterday, but now, after seeing your fix, I can. Following code works without guards:
void Serial_::accept(void)
{
ring_buffer *buffer = &cdc_rx_buffer;
uint32_t i = (uint32_t)(buffer->head+1) % CDC_SERIAL_BUFFER_SIZE;
if (!USBD_Available(CDC_RX))
return;
// 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 (i != buffer->tail) {
// UDD_Recv8() does not automatically free fifo
uint32_t c = UDD_Recv8(CDC_RX & 0xF);
buffer->buffer[buffer->head] = c;
buffer->head = i;
if (!USBD_Available(CDC_RX)) {
udd_ack_fifocon(CDC_RX);
break;
}
i = (i + 1) % CDC_SERIAL_BUFFER_SIZE;
}
}
Safer would be to disable the rx interrupt during accept(). The various low level api's like USBD_Recv() already do something like that via the LockEP class. (though I don't like the idea to use a class for this)
Let us further think about this...
It's not amazingly fast (I am getting a 7MByte file through in about 50s) but it is reliable
Indeed, consuming these bytes one by one is really inefficient. But now we have something reliable, we can think about optimization strategies.
B.t.w if we define CDC_SERIAL_BUFFER_SIZE as 513, would this be noticable? (then we have room to accept an entire fifo bank).