Fix serial XON/XOFF handshaking

Co-Authored-By: ejtagle <ejtagle@hotmail.com>

Marlin/MarlinSerial.cpp

@@ -55,16 +55,15 @@
     ring_buffer_r rx_buffer = { { 0 }, 0, 0 };
     #if TX_BUFFER_SIZE > 0
       ring_buffer_t tx_buffer = { { 0 }, 0, 0 };
-      static bool _written;
     #endif
+    static bool _written;
   #endif
 
   #if ENABLED(SERIAL_XON_XOFF)
-    constexpr uint8_t XON_XOFF_CHAR_SENT = 0x80;  // XON / XOFF Character was sent
+    constexpr uint8_t XON_XOFF_CHAR_SENT = 0x80,  // XON / XOFF Character was sent
-    constexpr uint8_t XON_XOFF_CHAR_MASK = 0x1F;  // XON / XOFF character to send
+                      XON_XOFF_CHAR_MASK = 0x1F;  // XON / XOFF character to send
     // XON / XOFF character definitions
-    constexpr uint8_t XON_CHAR  = 17;
+    constexpr uint8_t XON_CHAR  = 17, XOFF_CHAR = 19;
-    constexpr uint8_t XOFF_CHAR = 19;
     uint8_t xon_xoff_state = XON_XOFF_CHAR_SENT | XON_CHAR;
   #endif
 
@@ -85,123 +84,197 @@
 
   // (called with RX interrupts disabled)
   FORCE_INLINE void store_rxd_char() {
-    const ring_buffer_pos_t h = rx_buffer.head,
+
-                            i = (ring_buffer_pos_t)(h + 1) & (ring_buffer_pos_t)(RX_BUFFER_SIZE - 1);
+    // Get the tail - Nothing can alter its value while we are at this ISR
+    const ring_buffer_pos_t t = rx_buffer.tail;
+
+    // Get the head pointer
+    ring_buffer_pos_t h = rx_buffer.head;
+
+    // Get the next element
+    ring_buffer_pos_t i = (ring_buffer_pos_t)(h + 1) & (ring_buffer_pos_t)(RX_BUFFER_SIZE - 1);
+
+    // Read the character from the USART
+    uint8_t c = M_UDRx;
+
+    #if ENABLED(EMERGENCY_PARSER)
+      emergency_parser.update(c);
+    #endif
 
     // If the character is to be stored at the index just before the tail
-    // (such that the head would advance to the current tail), the buffer is
+    // (such that the head would advance to the current tail), the RX FIFO is
-    // critical, so don't write the character or advance the head.
+    // full, so don't write the character or advance the head.
-    const char c = M_UDRx;
+    if (i != t) {
-    if (i != rx_buffer.tail) {
       rx_buffer.buffer[h] = c;
-      rx_buffer.head = i;
+      h = i;
     }
-    else {
     #if ENABLED(SERIAL_STATS_DROPPED_RX)
-        if (!++rx_dropped_bytes) ++rx_dropped_bytes;
+      else if (!++rx_dropped_bytes) --rx_dropped_bytes;
     #endif
-    }
 
     #if ENABLED(SERIAL_STATS_MAX_RX_QUEUED)
-      // calculate count of bytes stored into the RX buffer
+      // Calculate count of bytes stored into the RX buffer
-      ring_buffer_pos_t rx_count = (ring_buffer_pos_t)(rx_buffer.head - rx_buffer.tail) & (ring_buffer_pos_t)(RX_BUFFER_SIZE - 1);
+      const ring_buffer_pos_t rx_count = (ring_buffer_pos_t)(h - t) & (ring_buffer_pos_t)(RX_BUFFER_SIZE - 1);
+
       // Keep track of the maximum count of enqueued bytes
       NOLESS(rx_max_enqueued, rx_count);
     #endif
 
     #if ENABLED(SERIAL_XON_XOFF)
-
+      // If the last char that was sent was an XON
-      // for high speed transfers, we can use XON/XOFF protocol to do
-      // software handshake and avoid overruns.
       if ((xon_xoff_state & XON_XOFF_CHAR_MASK) == XON_CHAR) {
 
-        // calculate count of bytes stored into the RX buffer
+        // Bytes stored into the RX buffer
-        ring_buffer_pos_t rx_count = (ring_buffer_pos_t)(rx_buffer.head - rx_buffer.tail) & (ring_buffer_pos_t)(RX_BUFFER_SIZE - 1);
+        const ring_buffer_pos_t rx_count = (ring_buffer_pos_t)(h - t) & (ring_buffer_pos_t)(RX_BUFFER_SIZE - 1);
 
-        // if we are above 12.5% of RX buffer capacity, send XOFF before
+        // If over 12.5% of RX buffer capacity, send XOFF before running out of
-        // we run out of RX buffer space .. We need 325 bytes @ 250kbits/s to
+        // RX buffer space .. 325 bytes @ 250kbits/s needed to let the host react
-        // let the host react and stop sending bytes. This translates to 13mS
+        // and stop sending bytes. This translates to 13mS propagation time.
-        // propagation time.
         if (rx_count >= (RX_BUFFER_SIZE) / 8) {
 
-          // If TX interrupts are disabled and data register is empty,
+          // At this point, definitely no TX interrupt was executing, since the TX isr can't be preempted.
-          // just write the byte to the data register and be done. This
+          // Don't enable the TX interrupt here as a means to trigger the XOFF char, because if it happens
-          // shortcut helps significantly improve the effective datarate
+          // to be in the middle of trying to disable the RX interrupt in the main program, eventually the
-          // at high (>500kbit/s) bitrates, where interrupt overhead
+          // enabling of the TX interrupt could be undone. The ONLY reliable thing this can do to ensure
-          // becomes a slowdown.
+          // the sending of the XOFF char is to send it HERE AND NOW.
-          if (!TEST(M_UCSRxB, M_UDRIEx) && TEST(M_UCSRxA, M_UDREx)) {
 
-            // Send an XOFF character
+          // About to send the XOFF char
-            M_UDRx = XOFF_CHAR;
-
-            // 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(M_UCSRxA, M_TXCx);
-
-            // And remember it was sent
           xon_xoff_state = XOFF_CHAR | XON_XOFF_CHAR_SENT;
-          }
-          else {
-            // TX interrupts disabled, but buffer still not empty ... or
-            // TX interrupts enabled. Reenable TX ints and schedule XOFF
-            // character to be sent
-            #if TX_BUFFER_SIZE > 0
-              SBI(M_UCSRxB, M_UDRIEx);
-              xon_xoff_state = XOFF_CHAR;
-            #else
-              // We are not using TX interrupts, we will have to send this manually
-              while (!TEST(M_UCSRxA, M_UDREx)) sw_barrier();
-              M_UDRx = XOFF_CHAR;
 
-              // clear the TXC bit -- "can be cleared by writing a one to its bit
+          // Wait until the TX register becomes empty and send it - Here there could be a problem
-              // location". This makes sure flush() won't return until the bytes
+          // - While waiting for the TX register to empty, the RX register could receive a new
-              // actually got written
+          //   character. This must also handle that situation!
-              SBI(M_UCSRxA, M_TXCx);
+          while (!TEST(M_UCSRxA, M_UDREx)) {
 
-              // And remember we already sent it
+            if (TEST(M_UCSRxA,M_RXCx)) {
-              xon_xoff_state = XOFF_CHAR | XON_XOFF_CHAR_SENT;
+              // A char arrived while waiting for the TX buffer to be empty - Receive and process it!
-            #endif
+
-          }
+              i = (ring_buffer_pos_t)(h + 1) & (ring_buffer_pos_t)(RX_BUFFER_SIZE - 1);
-        }
+
-      }
+              // Read the character from the USART
-    #endif // SERIAL_XON_XOFF
+              c = M_UDRx;
 
               #if ENABLED(EMERGENCY_PARSER)
                 emergency_parser.update(c);
               #endif
+
+              // If the character is to be stored at the index just before the tail
+              // (such that the head would advance to the current tail), the FIFO is
+              // full, so don't write the character or advance the head.
+              if (i != t) {
+                rx_buffer.buffer[h] = c;
+                h = i;
+              }
+              #if ENABLED(SERIAL_STATS_DROPPED_RX)
+                else if (!++rx_dropped_bytes) --rx_dropped_bytes;
+              #endif
+            }
+            sw_barrier();
+          }
+
+          M_UDRx = XOFF_CHAR;
+
+          // 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(M_UCSRxA, M_TXCx);
+
+          // At this point there could be a race condition between the write() function
+          // and this sending of the XOFF char. This interrupt could happen between the
+          // wait to be empty TX buffer loop and the actual write of the character. Since
+          // the TX buffer is full because it's sending the XOFF char, the only way to be
+          // sure the write() function will succeed is to wait for the XOFF char to be
+          // completely sent. Since an extra character could be received during the wait
+          // it must also be handled!
+          while (!TEST(M_UCSRxA, M_UDREx)) {
+
+            if (TEST(M_UCSRxA,M_RXCx)) {
+              // A char arrived while waiting for the TX buffer to be empty - Receive and process it!
+
+              i = (ring_buffer_pos_t)(h + 1) & (ring_buffer_pos_t)(RX_BUFFER_SIZE - 1);
+
+              // Read the character from the USART
+              c = M_UDRx;
+
+              #if ENABLED(EMERGENCY_PARSER)
+                emergency_parser.update(c);
+              #endif
+
+              // If the character is to be stored at the index just before the tail
+              // (such that the head would advance to the current tail), the FIFO is
+              // full, so don't write the character or advance the head.
+              if (i != t) {
+                rx_buffer.buffer[h] = c;
+                h = i;
+              }
+              #if ENABLED(SERIAL_STATS_DROPPED_RX)
+                else if (!++rx_dropped_bytes) --rx_dropped_bytes;
+              #endif
+            }
+            sw_barrier();
+          }
+
+          // At this point everything is ready. The write() function won't
+          // have any issues writing to the UART TX register if it needs to!
+        }
+      }
+    #endif // SERIAL_XON_XOFF
+
+    // Store the new head value
+    rx_buffer.head = h;
   }
 
   #if TX_BUFFER_SIZE > 0
 
     // (called with TX irqs disabled)
     FORCE_INLINE void _tx_udr_empty_irq(void) {
-      // If interrupts are enabled, there must be more data in the output
+
-      // buffer.
+      // Read positions
+      uint8_t t = tx_buffer.tail;
+      const uint8_t h = tx_buffer.head;
 
       #if ENABLED(SERIAL_XON_XOFF)
-        // Do a priority insertion of an XON/XOFF char, if needed.
+        // If an XON char is pending to be sent, do it now
-        const uint8_t state = xon_xoff_state;
+        if (xon_xoff_state == XON_CHAR) {
-        if (!(state & XON_XOFF_CHAR_SENT)) {
+
-          M_UDRx = state & XON_XOFF_CHAR_MASK;
+          // Send the character
-          xon_xoff_state = state | XON_XOFF_CHAR_SENT;
+          M_UDRx = XON_CHAR;
-        }
-        else
-      #endif
-      { // Send the next byte
-        const uint8_t t = tx_buffer.tail, c = tx_buffer.buffer[t];
-        tx_buffer.tail = (t + 1) & (TX_BUFFER_SIZE - 1);
-        M_UDRx = 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(M_UCSRxA, M_TXCx);
 
-      // Disable interrupts if the buffer is empty
+          // Remember we sent it.
-      if (tx_buffer.head == tx_buffer.tail)
+          xon_xoff_state = XON_CHAR | XON_XOFF_CHAR_SENT;
-        CBI(M_UCSRxB, M_UDRIEx);
+
+          // If nothing else to transmit, just disable TX interrupts.
+          if (h == t) CBI(M_UCSRxB, M_UDRIEx); // (Non-atomic, could be reenabled by the main program, but eventually this will succeed)
+
+          return;
+        }
+      #endif
+
+      // If nothing to transmit, just disable TX interrupts. This could
+      // happen as the result of the non atomicity of the disabling of RX
+      // interrupts that could end reenabling TX interrupts as a side effect.
+      if (h == t) {
+        CBI(M_UCSRxB, M_UDRIEx); // (Non-atomic, could be reenabled by the main program, but eventually this will succeed)
+        return;
+      }
+
+      // There is something to TX, Send the next byte
+      const uint8_t c = tx_buffer.buffer[t];
+      t = (t + 1) & (TX_BUFFER_SIZE - 1);
+      M_UDRx = c;
+      tx_buffer.tail = t;
+
+      // Clear the TXC bit (by writing a one to its bit location).
+      // Ensures flush() won't return until the bytes are actually written/
+      SBI(M_UCSRxA, M_TXCx);
+
+      // Disable interrupts if there is nothing to transmit following this byte
+      if (h == t) CBI(M_UCSRxB, M_UDRIEx); // (Non-atomic, could be reenabled by the main program, but eventually this will succeed)
     }
 
     #ifdef M_USARTx_UDRE_vect
@@ -245,8 +318,8 @@
     SBI(M_UCSRxB, M_RXCIEx);
     #if TX_BUFFER_SIZE > 0
       CBI(M_UCSRxB, M_UDRIEx);
-      _written = false;
     #endif
+    _written = false;
   }
 
   void MarlinSerial::end() {
@@ -273,11 +346,11 @@
   }
 
   int MarlinSerial::read(void) {
-    int v;
 
     #if RX_BUFFER_SIZE > 256
-      // Disable RX interrupts to ensure atomic reads
+      // Disable RX interrupts to ensure atomic reads - This could reenable TX interrupts,
-      const bool isr_enabled = TEST(M_UCSRxB, M_RXCIEx);
+      //  but this situation is explicitly handled at the TX isr, so no problems there
+      bool isr_enabled = TEST(M_UCSRxB, M_RXCIEx);
       CBI(M_UCSRxB, M_RXCIEx);
     #endif
 
@@ -290,16 +363,19 @@
 
     ring_buffer_pos_t t = rx_buffer.tail;
 
-    if (h == t)
+    // If nothing to read, return now
-      v = -1;
+    if (h == t) return -1;
-    else {
+
-      v = rx_buffer.buffer[t];
+    // Get the next char
+    const int v = rx_buffer.buffer[t];
     t = (ring_buffer_pos_t)(t + 1) & (RX_BUFFER_SIZE - 1);
 
     #if RX_BUFFER_SIZE > 256
       // Disable RX interrupts to ensure atomic write to tail, so
-        // the RX isr can't read partially updated values
+      // the RX isr can't read partially updated values - This could
-        const bool isr_enabled = TEST(M_UCSRxB, M_RXCIEx);
+      // reenable TX interrupts, but this situation is explicitly
+      // handled at the TX isr, so no problems there
+      isr_enabled = TEST(M_UCSRxB, M_RXCIEx);
       CBI(M_UCSRxB, M_RXCIEx);
     #endif
 
@@ -312,21 +388,25 @@
     #endif
 
     #if ENABLED(SERIAL_XON_XOFF)
+      // If the XOFF char was sent, or about to be sent...
       if ((xon_xoff_state & XON_XOFF_CHAR_MASK) == XOFF_CHAR) {
-
         // Get count of bytes in the RX buffer
-          ring_buffer_pos_t rx_count = (ring_buffer_pos_t)(h - t) & (ring_buffer_pos_t)(RX_BUFFER_SIZE - 1);
+        const ring_buffer_pos_t rx_count = (ring_buffer_pos_t)(h - t) & (ring_buffer_pos_t)(RX_BUFFER_SIZE - 1);
-
-          // When below 10% of RX buffer capacity, send XON before
-          // running out of RX buffer bytes
         if (rx_count < (RX_BUFFER_SIZE) / 10) {
+          #if TX_BUFFER_SIZE > 0
+            // Signal we want an XON character to be sent.
+            xon_xoff_state = XON_CHAR;
+            // Enable TX isr. Non atomic, but it will eventually enable them
+            SBI(M_UCSRxB, M_UDRIEx);
+          #else
+            // If not using TX interrupts, we must send the XON char now
             xon_xoff_state = XON_CHAR | XON_XOFF_CHAR_SENT;
-            write(XON_CHAR);
+            while (!TEST(M_UCSRxA, M_UDREx)) sw_barrier();
-            return v;
+            M_UDRx = XON_CHAR;
+          #endif
         }
       }
     #endif
-    }
 
     return v;
   }
@@ -359,9 +439,19 @@
     #endif
 
     #if ENABLED(SERIAL_XON_XOFF)
+      // If the XOFF char was sent, or about to be sent...
       if ((xon_xoff_state & XON_XOFF_CHAR_MASK) == XOFF_CHAR) {
+        #if TX_BUFFER_SIZE > 0
+          // Signal we want an XON character to be sent.
+          xon_xoff_state = XON_CHAR;
+          // Enable TX isr. Non atomic, but it will eventually enable it.
+          SBI(M_UCSRxB, M_UDRIEx);
+        #else
+          // If not using TX interrupts, we must send the XON char now
           xon_xoff_state = XON_CHAR | XON_XOFF_CHAR_SENT;
-        write(XON_CHAR);
+          while (!TEST(M_UCSRxA, M_UDREx)) sw_barrier();
+          M_UDRx = XON_CHAR;
+        #endif
       }
     #endif
   }
@@ -375,6 +465,8 @@
       // be done. This shortcut helps significantly improve the
       // effective datarate at high (>500kbit/s) bitrates, where
       // interrupt overhead becomes a slowdown.
+      // Yes, there is a race condition between the sending of the
+      // XOFF char at the RX isr, but it is properly handled there
       if (!TEST(M_UCSRxB, M_UDRIEx) && TEST(M_UCSRxA, M_UDREx)) {
         M_UDRx = c;
 
@@ -387,61 +479,79 @@
 
       const uint8_t i = (tx_buffer.head + 1) & (TX_BUFFER_SIZE - 1);
 
-      // If the output buffer is full, there's nothing for it other than to
+      // If global interrupts are disabled (as the result of being called from an ISR)...
-      // wait for the interrupt handler to empty it a bit
-      while (i == tx_buffer.tail) {
       if (!ISRS_ENABLED()) {
-          // Interrupts are disabled, so we'll have to poll the data
+
-          // register empty flag ourselves. If it is set, pretend an
+        // Make room by polling if it is possible to transmit, and do so!
-          // interrupt has happened and call the handler to free up
+        while (i == tx_buffer.tail) {
-          // space for us.
+
-          if (TEST(M_UCSRxA, M_UDREx))
+          // If we can transmit another byte, do it.
-            _tx_udr_empty_irq();
+          if (TEST(M_UCSRxA, M_UDREx)) _tx_udr_empty_irq();
-        }
-        // (else , the interrupt handler will free up space for us)
 
           // Make sure compiler rereads tx_buffer.tail
           sw_barrier();
         }
+      }
+      else {
+        // Interrupts are enabled, just wait until there is space
+        while (i == tx_buffer.tail) { sw_barrier(); }
+      }
 
       // Store new char. head is always safe to move
       tx_buffer.buffer[tx_buffer.head] = c;
       tx_buffer.head = i;
 
-      // Enable TX isr
+      // Enable TX isr - Non atomic, but it will eventually enable TX isr
       SBI(M_UCSRxB, M_UDRIEx);
-      return;
     }
 
     void MarlinSerial::flushTX(void) {
-      // TX
+      // No bytes written, no need to flush. This special case is needed since there's
-      // If we have never written a byte, no need to flush. This special
+      // no way to force the TXC (transmit complete) bit to 1 during initialization.
-      // case is needed since there is no way to force the TXC (transmit
+      if (!_written) return;
-      // complete) bit to 1 during initialization
-      if (!_written)
-        return;
 
-      while (TEST(M_UCSRxB, M_UDRIEx) || !TEST(M_UCSRxA, M_TXCx)) {
+      // If global interrupts are disabled (as the result of being called from an ISR)...
       if (!ISRS_ENABLED()) {
-          // Interrupts are globally disabled, but the DR empty
+
-          // interrupt should be enabled, so poll the DR empty flag to
+        // Wait until everything was transmitted - We must do polling, as interrupts are disabled
-          // prevent deadlock
+        while (tx_buffer.head != tx_buffer.tail || !TEST(M_UCSRxA, M_TXCx)) {
+
+          // If there is more space, send an extra character
           if (TEST(M_UCSRxA, M_UDREx))
             _tx_udr_empty_irq();
-        }
+
           sw_barrier();
         }
-      // If we get here, nothing is queued anymore (DRIE is disabled) and
+
+      }
+      else {
+        // Wait until everything was transmitted
+        while (tx_buffer.head != tx_buffer.tail || !TEST(M_UCSRxA, M_TXCx)) sw_barrier();
+      }
+
+      // At this point nothing is queued anymore (DRIE is disabled) and
       // the hardware finished transmission (TXC is set).
     }
 
   #else // TX_BUFFER_SIZE == 0
 
     void MarlinSerial::write(const uint8_t c) {
+      _written = true;
       while (!TEST(M_UCSRxA, M_UDREx)) sw_barrier();
       M_UDRx = c;
     }
 
+    void MarlinSerial::flushTX(void) {
+      // No bytes written, no need to flush. This special case is needed since there's
+      // no way to force the TXC (transmit complete) bit to 1 during initialization.
+      if (!_written) return;
+
+      // Wait until everything was transmitted
+      while (!TEST(M_UCSRxA, M_TXCx)) sw_barrier();
+
+      // At this point nothing is queued anymore (DRIE is disabled) and
+      // the hardware finished transmission (TXC is set).
+    }
   #endif // TX_BUFFER_SIZE == 0
 
   /**
@@ -465,13 +575,9 @@
   }
 
   void MarlinSerial::print(long n, int base) {
-    if (base == 0)
+    if (base == 0) write(n);
-      write(n);
     else if (base == 10) {
-      if (n < 0) {
+      if (n < 0) { print('-'); n = -n; }
-        print('-');
-        n = -n;
-      }
       printNumber(n, 10);
     }
     else

Marlin/MarlinSerial.h

@@ -26,10 +26,11 @@
  *
  * Modified 28 September 2010 by Mark Sproul
  * Modified 14 February 2016 by Andreas Hardtung (added tx buffer)
+ * Modified 01 October 2017 by Eduardo José Tagle (added XON/XOFF)
  */
 
-#ifndef MARLINSERIAL_H
+#ifndef _MARLINSERIAL_H_
-#define MARLINSERIAL_H
+#define _MARLINSERIAL_H_
 
 #include "MarlinConfig.h"
 
@@ -112,9 +113,7 @@
       static void flush(void);
       static ring_buffer_pos_t available(void);
       static void write(const uint8_t c);
-      #if TX_BUFFER_SIZE > 0
       static void flushTX(void);
-      #endif
 
       #if ENABLED(SERIAL_STATS_DROPPED_RX)
         FORCE_INLINE static uint32_t dropped() { return rx_dropped_bytes; }
@@ -124,11 +123,6 @@
         FORCE_INLINE static ring_buffer_pos_t rxMaxEnqueued() { return rx_max_enqueued; }
       #endif
 
-    private:
-      static void printNumber(unsigned long, const uint8_t);
-      static void printFloat(double, uint8_t);
-
-    public:
       FORCE_INLINE static void write(const char* str) { while (*str) write(*str++); }
       FORCE_INLINE static void write(const uint8_t* buffer, size_t size) { while (size--) write(*buffer++); }
       FORCE_INLINE static void print(const String& s) { for (int i = 0; i < (int)s.length(); i++) write(s[i]); }
@@ -152,6 +146,11 @@
       static void println(unsigned long, int = DEC);
       static void println(double, int = 2);
       static void println(void);
+      operator bool() { return true; }
+
+    private:
+      static void printNumber(unsigned long, const uint8_t);
+      static void printFloat(double, uint8_t);
   };
 
   extern MarlinSerial customizedSerial;
@@ -163,4 +162,4 @@
   extern HardwareSerial bluetoothSerial;
 #endif
 
-#endif // MARLINSERIAL_H
+#endif // _MARLINSERIAL_H_