Fix race condition in USB CDC transmit

[pbrook]

This patch fixes a race that can cause data corruption/loss when sending data over the USB CDC serial port. Exact details in commit message.

The window for the race condition is quite small, and the CDC code very slow (byte writes), so the hardware double buffering means this occurs extremely rarely for most sketches.

I have however seen it occur in real life.

[embmicro]

This bug was causing a lot of problems for me. I was using the Arduino as a high speed USB->Serial converter and it was dropping a lot of data. It now seems to be working great.

[jantje]

I cant get my yun leonardo to talk reliable over USB to my yun linino on serial speeds above 9500.
Big bulks of data fail nearly 100% for sure. Could that be related?

[pbrook]

The Yun problems could be related, though IIUC that uses the UARTs (not USB) to connect the two chips, without any flow control. If so that's your most likely cause of data loss.

It's worth noting that even if overall throughput is sufficient, USB busses tend to experience high latency and intermittent stalls. This and high interrupt latency in the CDC RX path can induce UART buffer overflows much sooner than you might expect.

[jantje]

Thanks for the answer.
I'm using both the bridge (UART if I understood correctly) and serial (USB) to communicate between leonardo and lininio. So I guess your saying I'm likely to suffer this problem.

[cmaglie]

Thanks for the patch!
May you provide a test sketch that trigger the bug? Is it reproducible?
C

[pbrook]

Unfortunately it's quite hard to reproduce with the current code. The host PC usually receives and processes the data much faster than we can send it (and we cannot fill a 64-byte packet in a single frame), so the FIFO never gets more than half full. Triggering the bug requires careful control of (or very bad luck with!) the host USB stack.

I can reproduce consistently with one of the following changes:
either (a) Replace EP_DOUBLE_64 with EP_SINGLE_64 in USBCore.cpp:InitEndpoints to disable double buffering
or (b) Apply the block write patch: #1941

The following code then exhibits somewhere between 0.1% and 0.3% data loss. i.e. a missing character every few lines.

void setup() {
  Serial.begin(9600); // CDC virtual port, baud rate ignored
  while(1) {
    char i;
    // Block write a 64-byte USB packet of data
    for (i = 0; i < 8; i++) Serial.print("12345678");
    // Partially fill the next packet a byte at a time
    for (i = 0; i < 26; i++) Serial.write('a'+i);
    Serial.write('\n');
    // Wait a frame and a bit. Should be sufficient for the USB FIFO
    // to drain, and SOF to occur at a slightly different point
    delayMicroseconds(1000+random(100));
  }
}

[cmaglie]

Thanks, I've checked all the patches #1941 #1945 #1953 and together they works great!
After applying #1941 I was able to consistently reproduce the race-condition with the attached sketch, and I can confirm that is fixed by #1945.

I'll keep the three pull request still open for a short while for some more testing, but I'm confident that they are ready to be merged.

void burstSend() {
  // Send a burst of 1.000.000 bytes of data
  for (uint16_t i=0; i<20000; i++) {
    // 25 bytes
    Serial.print("0123456789abcdeABCDE=)(/&");
    // 25 bytes
    Serial.print("qwertyuiopasdfghjklzQWERT");
  }
}

C

[cmaglie]

Manually merged here:
13c0db5

Thank you!