Fix HardwareSerial::flush() when interrupts are kept disabled for a while

It turns out there is an additional corner case. The analysis in the
previous commit wrt to flush() assumes that the data register is always
kept filled by the interrupt handler, so the TXC bit won't get set until
all the queued bytes have been transmitted. But, when interrupts are
disabled for a longer period (for example when an interrupt handler for
another device is running for longer than 1-2 byte times), it could
happen that the UART stops transmitting while there are still more bytes
queued (but these are in the buffer, not in the UDR register, so the
UART can't know about them).

In this case, the TXC bit would get set, but the transmission is not
complete yet. We can easily detect this case by looking at the head and
tail pointers, but it seems easier to instead look at the UDRIE bit
(the TX interrupt is enabled if and only if there are bytes in the
queue). To fix this corner case, this commit:
 - Checks the UDRIE bit and only if it is unset, looks at the TXC bit.
 - Moves the clearing of TXC from write() to the tx interrupt handler.
   This (still) causes the TXC bit to be cleared whenever a byte is
   queued when the buffer is empty (in this case the tx interrupt will
   trigger directly after write() is called). It also causes the TXC bit
   to be cleared whenever transmission is resumed after it halted
   because interrupts have been disabled for too long.

As a side effect, another race condition is prevented. This could occur
at very high bitrates, where the transmission would be completed before
the code got time to clear the TXC0 register, making the clear happen
_after_ the transmission was already complete. With the new code, the
clearing of TXC happens directly after writing to the UDR register,
while interrupts are disabled, and we can be certain the data
transmission needs more time than one instruction to complete. This
fixes #1463 and replaces #1456.

1 files changed, 8 insertions, 7 deletions

diff --git a/cores/arduino/HardwareSerial.cpp b/cores/arduino/HardwareSerial.cpp
index d78c205..1bd3bff 100644
--- a/cores/arduino/HardwareSerial.cpp
+++ b/cores/arduino/HardwareSerial.cpp
@@ -233,6 +233,11 @@ void HardwareSerial::_tx_udr_empty_irq(void)
     _tx_buffer_tail = (_tx_buffer_tail + 1) % SERIAL_BUFFER_SIZE;
 
     *_udr = c;
+
+    // clear the TXC bit -- "can be cleared by writing a one to its bit
+    // location". This makes sure flush() won't return until the bytes
+    // actually got written
+    sbi(*_ucsra, TXC0);
   }
 }
 
@@ -339,9 +344,9 @@ void HardwareSerial::flush()
   if (!_written)
     return;
 
-  // UDR is kept full while the buffer is not empty, so TXC triggers
-  // when EMPTY && SENT
-  while (bit_is_clear(*_ucsra, TXC0));
+  while (bit_is_set(*_ucsrb, UDRIE0) || bit_is_clear(*_ucsra, TXC0));
+  // If we get here, nothing is queued anymore (DRIE is disabled) and
+  // the hardware finished tranmission (TXC is set).
 }
 
 size_t HardwareSerial::write(uint8_t c)
@@ -359,10 +364,6 @@ size_t HardwareSerial::write(uint8_t c)
 	
   sbi(*_ucsrb, UDRIE0);
   _written = true;
-  // clear the TXC bit -- "can be cleared by writing a one to its bit
-  // location". This makes sure flush() won't return until the bytes
-  // actually got written
-  sbi(*_ucsra, TXC0);
   
   return 1;
 }