Fix for Serial::flush() deadlock with high baud rates

[allesblinkt]

I am currently developing a couple of Arduino Pro Mini (Atmega 328 @16mhz) based motor controllers that get their commands via RS-485. For that reason I have to switch the bus transceiver to transmit whenever one of my nodes sends something back.

Everything works nicely, however HardwareSerial::flush() occasionally locks up when I am using high baudrates (1.000.000 Bit/s).

I am not evening using interrupts or interrupt timers, but have found the problem to be in HardwareSerial::send(). When sending bytes, the interrupt for UDRE is enabled before the TXC0 is cleared (set to one). If the interrupt fires fast enough, consumes and shifts out the byte, the TXC0 bit is cleared after it is set when the last byte has been shifted out. Calling Serial.flush() in a case like this causes the Arduino to lock up, because it is stuck in its while loop indefinitely.

Here is a small sketch that will crash an Atmega 328 @16mhz after a couple of seconds:

#define DIR_PIN 10
#define LED_PIN 13

#define MAX_DATA_LENGTH 6

byte data[] = {
  0xFF, 0xFF, 0xAB, 0xCD, 0xDE, 0x66};

void setup() { 
  Serial.begin(1000000);

  pinMode(DIR_PIN, OUTPUT);
  pinMode(LED_PIN, OUTPUT);
  digitalWrite(DIR_PIN, LOW); // receive
}


void loop() {
  digitalWrite(LED_PIN, HIGH);

  digitalWrite(DIR_PIN, HIGH); // send
  Serial.write(data, MAX_DATA_LENGTH);
  Serial.flush();
  digitalWrite(DIR_PIN, LOW); // receive

  delay(100);
  digitalWrite(LED_PIN, LOW);
  delay(100);
}

Clearing the TXC0 bit before enabling the interrupt in HardwareSerial::send makes the problem go away (see commit).

[matthijskooijman]

Looking at the code, your observation and fix look correct to me.

However, I recently created a series of patches to HardwareSerial, which also change the code related to these registers. Would you care to test that code and report if the problem still occurs there (I think the race condition still exists, would be good to have it confirmed). The code is in #1369.

If the problem still exists, I think I know how to fix it in my changed code as well, I'll try to incorporate a fix there, then.

[allesblinkt]

Hello Matthijs,

thank you for the prompt answer. I could not get the above sketch to cause a deadlock (also with different data sizes), however looking at your code, the race condition might still be there... Humm...

By the way, your current branch did not compile, because it could not find avr/cpufunc.h. For testing I fixed this by including compat/ina90.h which also defines the _NOP macro.

[matthijskooijman]

Thinking about this a bit more, I'm not sure if first clearing TXC and then writing to UDR is a sufficient fix. I think the following can happen:

• The hardware is writing out a byte (which is no longer in UDR)

• TXC is cleared

• The hardware finishes writing and sets TXC

• UDR is written

  Now, the hardware is still writing a byte, but TXC indicates transmission is complete.

I haven't looked closely if this problem can actually appear in the current HardwareSerial code, but I'm pretty sure it can happen with my HardwareSerial changes. This needs a bit more thinking to get it right, I just wanted to note it down for now, lest I forget.

[matthijskooijman]

Thinking on this a bit more, the fix is simple: Simply clear TXC directly after writing to UDR. This technically still leaves a race condition: If the data byte written to UDR is sent out (causing TXC to be set) before TXC is cleared and then the hardware is done, but TXC is cleared. However, we can assume that sending out a data byte will take longer than the one or two cycles needed to clear the TXC bit, so this race can never occur.

[matthijskooijman]

Ah, and that is already what the patch in my pullrequest does, so that explains why you couldn't reproduce the race condition using my branch.

Do you agree with my analysis, or did I perhaps miss something?

[matthijskooijman]

A bit of testing supports the analysis: If I delay for 10μs (startbit, 8 databits, stopbit) between writing UDR and clearing TXC, the Arduino deadlocks. A delay of 9μs works like a charm.

[cmaglie]

Fixed using Matthijs' patch. Release scheduled for 1.5.6.