From 5590c8ffd0157d12fcca77f7a89d5d41d54945ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eduardo=20Jos=C3=A9=20Tagle?= Date: Sun, 10 Jun 2018 22:32:20 -0300 Subject: [PATCH] Fix MarlinSerial (AVR) (#10991) An undocumented hw bug makes the UART lose chars when RX ISR is disabled, even for a very small amount of time. This happens when RX_BUFFER > 256, and the result is corrupted received commands. Solved by implementing pseudo-atomic operations on 16bit indices. --- Marlin/src/HAL/HAL_AVR/MarlinSerial.cpp | 162 +++++++++++++----------- 1 file changed, 90 insertions(+), 72 deletions(-) diff --git a/Marlin/src/HAL/HAL_AVR/MarlinSerial.cpp b/Marlin/src/HAL/HAL_AVR/MarlinSerial.cpp index 6c165a4af5..ece2497766 100644 --- a/Marlin/src/HAL/HAL_AVR/MarlinSerial.cpp +++ b/Marlin/src/HAL/HAL_AVR/MarlinSerial.cpp @@ -28,7 +28,9 @@ * 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) + * Modified 10 June 2018 by Eduardo José Tagle (See #10991) */ + #ifdef __AVR__ // Disable HardwareSerial.cpp to support chips without a UART (Attiny, etc.) @@ -91,6 +93,70 @@ #include "../../feature/emergency_parser.h" #endif + // "Atomically" read the RX head index value without disabling interrupts: + // This MUST be called with RX interrupts enabled, and CAN'T be called + // from the RX ISR itself! + FORCE_INLINE ring_buffer_pos_t atomic_read_rx_head() { + #if RX_BUFFER_SIZE > 256 + // Keep reading until 2 consecutive reads return the same value, + // meaning there was no update in-between caused by an interrupt. + // This works because serial RX interrupts happen at a slower rate + // than successive reads of a variable, so 2 consecutive reads with + // the same value means no interrupt updated it. + ring_buffer_pos_t vold, vnew = rx_buffer.head; + sw_barrier(); + do { + vold = vnew; + vnew = rx_buffer.head; + sw_barrier(); + } while (vold != vnew); + return vnew; + #else + // With an 8bit index, reads are always atomic. No need for special handling + return rx_buffer.head; + #endif + } + + #if RX_BUFFER_SIZE > 256 + static volatile bool rx_tail_value_not_stable = false; + static volatile uint16_t rx_tail_value_backup = 0; + #endif + + // Set RX tail index, taking into account the RX ISR could interrupt + // the write to this variable in the middle - So a backup strategy + // is used to ensure reads of the correct values. + // -Must NOT be called from the RX ISR - + FORCE_INLINE void atomic_set_rx_tail(ring_buffer_pos_t value) { + #if RX_BUFFER_SIZE > 256 + // Store the new value in the backup + rx_tail_value_backup = value; + sw_barrier(); + // Flag we are about to change the true value + rx_tail_value_not_stable = true; + sw_barrier(); + // Store the new value + rx_buffer.tail = value; + sw_barrier(); + // Signal the new value is completely stored into the value + rx_tail_value_not_stable = false; + sw_barrier(); + #else + rx_buffer.tail = value; + #endif + } + + // Get the RX tail index, taking into account the read could be + // interrupting in the middle of the update of that index value + // -Called from the RX ISR - + FORCE_INLINE ring_buffer_pos_t atomic_read_rx_tail() { + #if RX_BUFFER_SIZE > 256 + // If the true index is being modified, return the backup value + if (rx_tail_value_not_stable) return rx_tail_value_backup; + #endif + // The true index is stable, return it + return rx_buffer.tail; + } + // (called with RX interrupts disabled) FORCE_INLINE void store_rxd_char() { @@ -98,10 +164,12 @@ static EmergencyParser::State emergency_state; // = EP_RESET #endif - // 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 tail - Nothing can alter its value while this ISR is executing, but there's + // a chance that this ISR interrupted the main process while it was updating the index. + // The backup mechanism ensures the correct value is always returned. + const ring_buffer_pos_t t = atomic_read_rx_tail(); - // Get the head pointer + // Get the head pointer - This ISR is the only one that modifies its value, so it's safe to read here ring_buffer_pos_t h = rx_buffer.head; // Get the next element @@ -158,7 +226,7 @@ // and stop sending bytes. This translates to 13mS propagation time. if (rx_count >= (RX_BUFFER_SIZE) / 8) { - // At this point, definitely no TX interrupt was executing, since the TX isr can't be preempted. + // At this point, definitely no TX interrupt was executing, since the TX ISR can't be preempted. // Don't enable the TX interrupt here as a means to trigger the XOFF char, because if it happens // to be in the middle of trying to disable the RX interrupt in the main program, eventually the // enabling of the TX interrupt could be undone. The ONLY reliable thing this can do to ensure @@ -246,7 +314,7 @@ } #endif // SERIAL_XON_XOFF - // Store the new head value + // Store the new head value - The main loop will retry until the value is stable rx_buffer.head = h; } @@ -356,37 +424,14 @@ } int MarlinSerial::peek(void) { - #if RX_BUFFER_SIZE > 256 - // Disable RX interrupts, but only if non atomic reads - const bool isr_enabled = TEST(M_UCSRxB, M_RXCIEx); - CBI(M_UCSRxB, M_RXCIEx); - #endif - - const int v = rx_buffer.head == rx_buffer.tail ? -1 : rx_buffer.buffer[rx_buffer.tail]; - - #if RX_BUFFER_SIZE > 256 - // Reenable RX interrupts if they were enabled - if (isr_enabled) SBI(M_UCSRxB, M_RXCIEx); - #endif - return v; + const ring_buffer_pos_t h = atomic_read_rx_head(), t = rx_buffer.tail; + return h == t ? -1 : rx_buffer.buffer[t]; } int MarlinSerial::read(void) { + const ring_buffer_pos_t h = atomic_read_rx_head(); - #if RX_BUFFER_SIZE > 256 - // Disable RX interrupts to ensure atomic reads - This could reenable TX interrupts, - // 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 - - const ring_buffer_pos_t h = rx_buffer.head; - - #if RX_BUFFER_SIZE > 256 - // End critical section - if (isr_enabled) SBI(M_UCSRxB, M_RXCIEx); - #endif - + // Read the tail. Main thread owns it, so it is safe to directly read it ring_buffer_pos_t t = rx_buffer.tail; // If nothing to read, return now @@ -396,22 +441,9 @@ 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 - This could - // 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 - - // Advance tail - rx_buffer.tail = t; - - #if RX_BUFFER_SIZE > 256 - // End critical section - if (isr_enabled) SBI(M_UCSRxB, M_RXCIEx); - #endif + // Advance tail - Making sure the RX ISR will always get an stable value, even + // if it interrupts the writing of the value of that variable in the middle. + atomic_set_rx_tail(t); #if ENABLED(SERIAL_XON_XOFF) // If the XOFF char was sent, or about to be sent... @@ -422,7 +454,7 @@ #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 + // 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 @@ -438,31 +470,17 @@ } ring_buffer_pos_t MarlinSerial::available(void) { - #if RX_BUFFER_SIZE > 256 - const bool isr_enabled = TEST(M_UCSRxB, M_RXCIEx); - CBI(M_UCSRxB, M_RXCIEx); - #endif - - const ring_buffer_pos_t h = rx_buffer.head, t = rx_buffer.tail; - - #if RX_BUFFER_SIZE > 256 - if (isr_enabled) SBI(M_UCSRxB, M_RXCIEx); - #endif - + const ring_buffer_pos_t h = atomic_read_rx_head(), t = rx_buffer.tail; return (ring_buffer_pos_t)(RX_BUFFER_SIZE + h - t) & (RX_BUFFER_SIZE - 1); } void MarlinSerial::flush(void) { - #if RX_BUFFER_SIZE > 256 - const bool isr_enabled = TEST(M_UCSRxB, M_RXCIEx); - CBI(M_UCSRxB, M_RXCIEx); - #endif - rx_buffer.tail = rx_buffer.head; - - #if RX_BUFFER_SIZE > 256 - if (isr_enabled) SBI(M_UCSRxB, M_RXCIEx); - #endif + // Set the tail to the head: + // - Read the RX head index in a safe way. (See atomic_read_rx_head.) + // - Set the tail, making sure the RX ISR will always get a stable value, even + // if it interrupts the writing of the value of that variable in the middle. + atomic_set_rx_tail(atomic_read_rx_head()); #if ENABLED(SERIAL_XON_XOFF) // If the XOFF char was sent, or about to be sent... @@ -470,7 +488,7 @@ #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. + // 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 @@ -492,7 +510,7 @@ // 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 + // 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; @@ -527,7 +545,7 @@ tx_buffer.buffer[tx_buffer.head] = c; tx_buffer.head = i; - // Enable TX isr - Non atomic, but it will eventually enable TX isr + // Enable TX ISR - Non atomic, but it will eventually enable TX ISR SBI(M_UCSRxB, M_UDRIEx); }