Improve HardwareSerial::flush()

The flush() method blocks until all characters in the serial buffer have
been written to the uart _and_ transmitted. This is checked by waiting
until the "TXC" (TX Complete) bit is set by the UART, signalling
completion. This bit is cleared by write() when adding a new byte to the
buffer and set by the hardware after tranmission ends, so it is always
guaranteed to be zero from the moment the first byte in a sequence is
queued until the moment the last byte is transmitted, and it is one from
the moment the last byte in the buffer is transmitted until the first
byte in the next sequence is queued.

However, the TXC bit is also zero from initialization to the moment the
first byte ever is queued (and then continues to be zero until the first
sequence of bytes completes transmission). Unfortunately we cannot
manually set the TXC bit during initialization, we can only clear it. To
make sure that flush() would not (indefinitely) block when it is called
_before_ anything was written to the serial device, the "transmitting"
variable was introduced.

This variable suggests that it is only true when something is
transmitting, which isn't currently the case (it remains true after
transmission is complete until flush() is called, for example).
Furthermore, there is no need to keep the status of transmission, the
only thing needed is to remember if anything has ever been written, so
the corner case described above can be detected.

This commit improves the code by:
 - Renaming the "transmitting" variable to _written (making it more
   clear and following the leading underscore naming convention).
 - Not resetting the value of _written at the end of flush(), there is
   no point to this.
 - Only checking the "_written" value once in flush(), since it can
   never be toggled off anyway.
 - Initializing the value of _written in both versions of _begin (though
   it probably gets initialized to 0 by default anyway, better to be
   explicit).

Author: matthijskooijman

hardware/arduino/avr/cores/arduino/HardwareSerial.cpp

@@ -281,6 +281,8 @@ void HardwareSerial::begin(unsigned long baud, byte config)
   *_ubrrh = baud_setting >> 8;
   *_ubrrl = baud_setting;
 
+  _written = false;
+
   //set the data bits, parity, and stop bits
 #if defined(__AVR_ATmega8__)
   config |= 0x80; // select UCSRC register (shared with UBRRH)
@@ -336,9 +338,15 @@ int HardwareSerial::read(void)
 
 void HardwareSerial::flush()
 {
-  // UDR is kept full while the buffer is not empty, so TXC triggers when EMPTY && SENT
-  while (transmitting && bit_is_clear(*_ucsra, TXC0));
-  transmitting = false;
+  // If we have never written a byte, no need to flush. This special
+  // case is needed since there is no way to force the TXC (transmit
+  // complete) bit to 1 during initialization
+  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));
 }
 
 size_t HardwareSerial::write(uint8_t c)
@@ -355,8 +363,10 @@ size_t HardwareSerial::write(uint8_t c)
   _tx_buffer_head = i;
 
   sbi(*_ucsrb, UDRIE0);
-  // clear the TXC bit -- "can be cleared by writing a one to its bit location"
-  transmitting = true;
+  _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;

hardware/arduino/avr/cores/arduino/HardwareSerial.h

@@ -46,7 +46,8 @@ class HardwareSerial : public Stream
     volatile uint8_t *_ucsrb;
     volatile uint8_t *_ucsrc;
     volatile uint8_t *_udr;
-    bool transmitting;
+    // Has any byte been written to the UART since begin()
+    bool _written;
 
     volatile uint8_t _rx_buffer_head;
     volatile uint8_t _rx_buffer_tail;