From d854fe863b8d0ae86cc5cd44fc7cee9d7818ba4f Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Thu, 18 Apr 2013 18:50:10 +0200 Subject: [PATCH 01/15] Use uint8_t for HardwareSerial ringbuffer pointers Since the buffers aren't bigger than 64 bytes, these values can be smaller. This saves a few bytes of ram, but also saves around 50 bytes of program space, since the values can now be loaded using a single instruction. To prevent problems when people manually increase the buffer size, a compile-time check is added. Closes: #1078 --- hardware/arduino/cores/arduino/HardwareSerial.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/hardware/arduino/cores/arduino/HardwareSerial.cpp b/hardware/arduino/cores/arduino/HardwareSerial.cpp index eb2365f3337..7b056e98937 100644 --- a/hardware/arduino/cores/arduino/HardwareSerial.cpp +++ b/hardware/arduino/cores/arduino/HardwareSerial.cpp @@ -62,10 +62,14 @@ struct ring_buffer { unsigned char buffer[SERIAL_BUFFER_SIZE]; - volatile unsigned int head; - volatile unsigned int tail; + volatile uint8_t head; + volatile uint8_t tail; }; +#if SERIAL_BUFFER_SIZE > 256 +#error Serial buffer size too big for head and tail pointers +#endif + #if defined(USBCON) ring_buffer rx_buffer = { { 0 }, 0, 0}; ring_buffer tx_buffer = { { 0 }, 0, 0}; From a2a64259e7d14019b306b4c0f60bdfa388e9cdd3 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Thu, 18 Apr 2013 18:52:48 +0200 Subject: [PATCH 02/15] Move buffers into HardwareSerial This removes the need for doing an extra pointer dereference on every access to the buffers, shrinking the code by around 100 bytes. The members for these buffers must be public for now, since the interrupt handlers also need to access them. These can later be made private again. Furthermore, the struct ring_buffer was removed. This allows the all head and tail pointers to be put into the HardwareSerial struct before the actual buffers, so the pointers all end up in the first 32 bytes of the struct that can be accessed using a single instruction (ldd). References: #947 --- .../arduino/cores/arduino/HardwareSerial.cpp | 125 ++++++------------ .../arduino/cores/arduino/HardwareSerial.h | 26 +++- 2 files changed, 63 insertions(+), 88 deletions(-) diff --git a/hardware/arduino/cores/arduino/HardwareSerial.cpp b/hardware/arduino/cores/arduino/HardwareSerial.cpp index 7b056e98937..e40bfbc0fd9 100644 --- a/hardware/arduino/cores/arduino/HardwareSerial.cpp +++ b/hardware/arduino/cores/arduino/HardwareSerial.cpp @@ -49,59 +49,17 @@ #endif #endif -// Define constants and variables for buffering incoming serial data. We're -// using a ring buffer (I think), in which head is the index of the location -// to which to write the next incoming character and tail is the index of the -// location from which to read. -#if (RAMEND < 1000) - #define SERIAL_BUFFER_SIZE 16 -#else - #define SERIAL_BUFFER_SIZE 64 -#endif - -struct ring_buffer -{ - unsigned char buffer[SERIAL_BUFFER_SIZE]; - volatile uint8_t head; - volatile uint8_t tail; -}; - -#if SERIAL_BUFFER_SIZE > 256 -#error Serial buffer size too big for head and tail pointers -#endif - -#if defined(USBCON) - ring_buffer rx_buffer = { { 0 }, 0, 0}; - ring_buffer tx_buffer = { { 0 }, 0, 0}; -#endif -#if defined(UBRRH) || defined(UBRR0H) - ring_buffer rx_buffer = { { 0 }, 0, 0 }; - ring_buffer tx_buffer = { { 0 }, 0, 0 }; -#endif -#if defined(UBRR1H) - ring_buffer rx_buffer1 = { { 0 }, 0, 0 }; - ring_buffer tx_buffer1 = { { 0 }, 0, 0 }; -#endif -#if defined(UBRR2H) - ring_buffer rx_buffer2 = { { 0 }, 0, 0 }; - ring_buffer tx_buffer2 = { { 0 }, 0, 0 }; -#endif -#if defined(UBRR3H) - ring_buffer rx_buffer3 = { { 0 }, 0, 0 }; - ring_buffer tx_buffer3 = { { 0 }, 0, 0 }; -#endif - -inline void store_char(unsigned char c, ring_buffer *buffer) +inline void store_char(unsigned char c, HardwareSerial *s) { - int i = (unsigned int)(buffer->head + 1) % SERIAL_BUFFER_SIZE; + int i = (unsigned int)(s->_rx_buffer_head + 1) % SERIAL_BUFFER_SIZE; // if we should be storing the received character into the location // just before the tail (meaning that the head would advance to the // current location of the tail), we're about to overflow the buffer // and so we don't write the character or advance the head. - if (i != buffer->tail) { - buffer->buffer[buffer->head] = c; - buffer->head = i; + if (i != s->_rx_buffer_tail) { + s->_rx_buffer[s->_rx_buffer_head] = c; + s->_rx_buffer_head = i; } } @@ -126,7 +84,7 @@ inline void store_char(unsigned char c, ring_buffer *buffer) #if defined(UDR0) if (bit_is_clear(UCSR0A, UPE0)) { unsigned char c = UDR0; - store_char(c, &rx_buffer); + store_char(c, &Serial); } else { unsigned char c = UDR0; }; @@ -152,7 +110,7 @@ inline void store_char(unsigned char c, ring_buffer *buffer) { if (bit_is_clear(UCSR1A, UPE1)) { unsigned char c = UDR1; - store_char(c, &rx_buffer1); + store_char(c, &Serial1); } else { unsigned char c = UDR1; }; @@ -167,7 +125,7 @@ inline void store_char(unsigned char c, ring_buffer *buffer) { if (bit_is_clear(UCSR2A, UPE2)) { unsigned char c = UDR2; - store_char(c, &rx_buffer2); + store_char(c, &Serial2); } else { unsigned char c = UDR2; }; @@ -182,7 +140,7 @@ inline void store_char(unsigned char c, ring_buffer *buffer) { if (bit_is_clear(UCSR3A, UPE3)) { unsigned char c = UDR3; - store_char(c, &rx_buffer3); + store_char(c, &Serial3); } else { unsigned char c = UDR3; }; @@ -222,7 +180,7 @@ ISR(USART0_UDRE_vect) ISR(USART_UDRE_vect) #endif { - if (tx_buffer.head == tx_buffer.tail) { + if (Serial._tx_buffer_head == Serial._tx_buffer_tail) { // Buffer empty, so disable interrupts #if defined(UCSR0B) cbi(UCSR0B, UDRIE0); @@ -232,8 +190,8 @@ ISR(USART_UDRE_vect) } else { // There is more data in the output buffer. Send the next byte - unsigned char c = tx_buffer.buffer[tx_buffer.tail]; - tx_buffer.tail = (tx_buffer.tail + 1) % SERIAL_BUFFER_SIZE; + unsigned char c = Serial._tx_buffer[Serial._tx_buffer_tail]; + Serial._tx_buffer_tail = (Serial._tx_buffer_tail + 1) % SERIAL_BUFFER_SIZE; #if defined(UDR0) UDR0 = c; @@ -250,14 +208,14 @@ ISR(USART_UDRE_vect) #ifdef USART1_UDRE_vect ISR(USART1_UDRE_vect) { - if (tx_buffer1.head == tx_buffer1.tail) { + if (Serial1._tx_buffer_head == Serial1._tx_buffer_tail) { // Buffer empty, so disable interrupts cbi(UCSR1B, UDRIE1); } else { // There is more data in the output buffer. Send the next byte - unsigned char c = tx_buffer1.buffer[tx_buffer1.tail]; - tx_buffer1.tail = (tx_buffer1.tail + 1) % SERIAL_BUFFER_SIZE; + unsigned char c = Serial1._tx_buffer[Serial1._tx_buffer_tail]; + Serial1._tx_buffer_tail = (Serial1._tx_buffer_tail + 1) % SERIAL_BUFFER_SIZE; UDR1 = c; } @@ -267,14 +225,14 @@ ISR(USART1_UDRE_vect) #ifdef USART2_UDRE_vect ISR(USART2_UDRE_vect) { - if (tx_buffer2.head == tx_buffer2.tail) { + if (Serial2._tx_buffer_head == Serial2._tx_buffer_tail) { // Buffer empty, so disable interrupts cbi(UCSR2B, UDRIE2); } else { // There is more data in the output buffer. Send the next byte - unsigned char c = tx_buffer2.buffer[tx_buffer2.tail]; - tx_buffer2.tail = (tx_buffer2.tail + 1) % SERIAL_BUFFER_SIZE; + unsigned char c = Serial2._tx_buffer[Serial2._tx_buffer_tail]; + Serial2._tx_buffer_tail = (Serial2._tx_buffer_tail + 1) % SERIAL_BUFFER_SIZE; UDR2 = c; } @@ -284,31 +242,30 @@ ISR(USART2_UDRE_vect) #ifdef USART3_UDRE_vect ISR(USART3_UDRE_vect) { - if (tx_buffer3.head == tx_buffer3.tail) { + if (Serial3._tx_buffer_head == Serial3._tx_buffer_tail) { // Buffer empty, so disable interrupts cbi(UCSR3B, UDRIE3); } else { // There is more data in the output buffer. Send the next byte - unsigned char c = tx_buffer3.buffer[tx_buffer3.tail]; - tx_buffer3.tail = (tx_buffer3.tail + 1) % SERIAL_BUFFER_SIZE; + unsigned char c = Serial3._tx_buffer[Serial3._tx_buffer_tail]; + Serial3._tx_buffer_tail = (Serial3._tx_buffer_tail + 1) % SERIAL_BUFFER_SIZE; UDR3 = c; } } #endif - // Constructors //////////////////////////////////////////////////////////////// -HardwareSerial::HardwareSerial(ring_buffer *rx_buffer, ring_buffer *tx_buffer, +HardwareSerial::HardwareSerial( volatile uint8_t *ubrrh, volatile uint8_t *ubrrl, volatile uint8_t *ucsra, volatile uint8_t *ucsrb, volatile uint8_t *ucsrc, volatile uint8_t *udr, uint8_t rxen, uint8_t txen, uint8_t rxcie, uint8_t udrie, uint8_t u2x) { - _rx_buffer = rx_buffer; - _tx_buffer = tx_buffer; + _tx_buffer_head = _tx_buffer_tail = 0; + _rx_buffer_head = _rx_buffer_tail = 0; _ubrrh = ubrrh; _ubrrl = ubrrl; _ucsra = ucsra; @@ -416,7 +373,7 @@ void HardwareSerial::begin(unsigned long baud, byte config) void HardwareSerial::end() { // wait for transmission of outgoing data - while (_tx_buffer->head != _tx_buffer->tail) + while (_tx_buffer_head != _tx_buffer_tail) ; cbi(*_ucsrb, _rxen); @@ -425,31 +382,31 @@ void HardwareSerial::end() cbi(*_ucsrb, _udrie); // clear any received data - _rx_buffer->head = _rx_buffer->tail; + _rx_buffer_head = _rx_buffer_tail; } int HardwareSerial::available(void) { - return (unsigned int)(SERIAL_BUFFER_SIZE + _rx_buffer->head - _rx_buffer->tail) % SERIAL_BUFFER_SIZE; + return (unsigned int)(SERIAL_BUFFER_SIZE + _rx_buffer_head - _rx_buffer_tail) % SERIAL_BUFFER_SIZE; } int HardwareSerial::peek(void) { - if (_rx_buffer->head == _rx_buffer->tail) { + if (_rx_buffer_head == _rx_buffer_tail) { return -1; } else { - return _rx_buffer->buffer[_rx_buffer->tail]; + return _rx_buffer[_rx_buffer_tail]; } } int HardwareSerial::read(void) { // if the head isn't ahead of the tail, we don't have any characters - if (_rx_buffer->head == _rx_buffer->tail) { + if (_rx_buffer_head == _rx_buffer_tail) { return -1; } else { - unsigned char c = _rx_buffer->buffer[_rx_buffer->tail]; - _rx_buffer->tail = (unsigned int)(_rx_buffer->tail + 1) % SERIAL_BUFFER_SIZE; + unsigned char c = _rx_buffer[_rx_buffer_tail]; + _rx_buffer_tail = (unsigned int)(_rx_buffer_tail + 1) % SERIAL_BUFFER_SIZE; return c; } } @@ -463,16 +420,16 @@ void HardwareSerial::flush() size_t HardwareSerial::write(uint8_t c) { - int i = (_tx_buffer->head + 1) % SERIAL_BUFFER_SIZE; + int i = (_tx_buffer_head + 1) % SERIAL_BUFFER_SIZE; // If the output buffer is full, there's nothing for it other than to // wait for the interrupt handler to empty it a bit // ???: return 0 here instead? - while (i == _tx_buffer->tail) + while (i == _tx_buffer_tail) ; - _tx_buffer->buffer[_tx_buffer->head] = c; - _tx_buffer->head = i; + _tx_buffer[_tx_buffer_head] = c; + _tx_buffer_head = i; sbi(*_ucsrb, _udrie); // clear the TXC bit -- "can be cleared by writing a one to its bit location" @@ -489,9 +446,9 @@ HardwareSerial::operator bool() { // Preinstantiate Objects ////////////////////////////////////////////////////// #if defined(UBRRH) && defined(UBRRL) - HardwareSerial Serial(&rx_buffer, &tx_buffer, &UBRRH, &UBRRL, &UCSRA, &UCSRB, &UCSRC, &UDR, RXEN, TXEN, RXCIE, UDRIE, U2X); + HardwareSerial Serial(&UBRRH, &UBRRL, &UCSRA, &UCSRB, &UCSRC, &UDR, RXEN, TXEN, RXCIE, UDRIE, U2X); #elif defined(UBRR0H) && defined(UBRR0L) - HardwareSerial Serial(&rx_buffer, &tx_buffer, &UBRR0H, &UBRR0L, &UCSR0A, &UCSR0B, &UCSR0C, &UDR0, RXEN0, TXEN0, RXCIE0, UDRIE0, U2X0); + HardwareSerial Serial(&UBRR0H, &UBRR0L, &UCSR0A, &UCSR0B, &UCSR0C, &UDR0, RXEN0, TXEN0, RXCIE0, UDRIE0, U2X0); #elif defined(USBCON) // do nothing - Serial object and buffers are initialized in CDC code #else @@ -499,13 +456,13 @@ HardwareSerial::operator bool() { #endif #if defined(UBRR1H) - HardwareSerial Serial1(&rx_buffer1, &tx_buffer1, &UBRR1H, &UBRR1L, &UCSR1A, &UCSR1B, &UCSR1C, &UDR1, RXEN1, TXEN1, RXCIE1, UDRIE1, U2X1); + HardwareSerial Serial1(&UBRR1H, &UBRR1L, &UCSR1A, &UCSR1B, &UCSR1C, &UDR1, RXEN1, TXEN1, RXCIE1, UDRIE1, U2X1); #endif #if defined(UBRR2H) - HardwareSerial Serial2(&rx_buffer2, &tx_buffer2, &UBRR2H, &UBRR2L, &UCSR2A, &UCSR2B, &UCSR2C, &UDR2, RXEN2, TXEN2, RXCIE2, UDRIE2, U2X2); + HardwareSerial Serial2(&UBRR2H, &UBRR2L, &UCSR2A, &UCSR2B, &UCSR2C, &UDR2, RXEN2, TXEN2, RXCIE2, UDRIE2, U2X2); #endif #if defined(UBRR3H) - HardwareSerial Serial3(&rx_buffer3, &tx_buffer3, &UBRR3H, &UBRR3L, &UCSR3A, &UCSR3B, &UCSR3C, &UDR3, RXEN3, TXEN3, RXCIE3, UDRIE3, U2X3); + HardwareSerial Serial3(&UBRR3H, &UBRR3L, &UCSR3A, &UCSR3B, &UCSR3C, &UDR3, RXEN3, TXEN3, RXCIE3, UDRIE3, U2X3); #endif #endif // whole file diff --git a/hardware/arduino/cores/arduino/HardwareSerial.h b/hardware/arduino/cores/arduino/HardwareSerial.h index a73117f5681..a091e0374a9 100644 --- a/hardware/arduino/cores/arduino/HardwareSerial.h +++ b/hardware/arduino/cores/arduino/HardwareSerial.h @@ -27,13 +27,19 @@ #include "Stream.h" -struct ring_buffer; +// Define constants and variables for buffering incoming serial data. We're +// using a ring buffer (I think), in which head is the index of the location +// to which to write the next incoming character and tail is the index of the +// location from which to read. +#if (RAMEND < 1000) + #define SERIAL_BUFFER_SIZE 16 +#else + #define SERIAL_BUFFER_SIZE 64 +#endif class HardwareSerial : public Stream { private: - ring_buffer *_rx_buffer; - ring_buffer *_tx_buffer; volatile uint8_t *_ubrrh; volatile uint8_t *_ubrrl; volatile uint8_t *_ucsra; @@ -46,8 +52,20 @@ class HardwareSerial : public Stream uint8_t _udrie; uint8_t _u2x; bool transmitting; + public: - HardwareSerial(ring_buffer *rx_buffer, ring_buffer *tx_buffer, + volatile uint8_t _rx_buffer_head; + volatile uint8_t _rx_buffer_tail; + volatile uint8_t _tx_buffer_head; + volatile uint8_t _tx_buffer_tail; + + // Don't put any members after these buffers, since only the first + // 32 bytes of this struct can be accessed quickly using the ldd + // instruction. + unsigned char _rx_buffer[SERIAL_BUFFER_SIZE]; + unsigned char _tx_buffer[SERIAL_BUFFER_SIZE]; + + HardwareSerial( volatile uint8_t *ubrrh, volatile uint8_t *ubrrl, volatile uint8_t *ucsra, volatile uint8_t *ucsrb, volatile uint8_t *ucsrc, volatile uint8_t *udr, From b48f112163b8e8511c97fa1dc254290205efcf24 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Thu, 18 Apr 2013 14:17:47 +0200 Subject: [PATCH 03/15] Use constants for register bit positions in HardwareSerial Previously, the constants to use for the bit positions of the various UARTs were passed to the HardwareSerial constructor. However, this meant that whenever these values were used, the had to be indirectly loaded, resulting in extra code overhead. Additionally, since there is no instruction to shift a value by a variable amount, the 1 << x expressions (inside _BV and sbi() / cbi()) would be compiled as a loop instead of being evaluated at compiletime. Now, the HardwareSerial class always uses the constants for the bit positions of UART 0 (and some code is present to make sure these constants exist, even for targets that only have a single unnumbered UART or start at UART1). This was already done for the TXC0 constant, for some reason. For the actual register addresses, this approach does not work, since these are of course different between the different UARTs on a single chip. Of course, always using the UART 0 constants is only correct when the constants are actually identical for the different UARTs. It has been verified that this is currently the case for all targets supported by avr-gcc 4.7.2, and the code contains compile-time checks to verify this for the current target, in case a new target is added for which this does not hold. This verification was done using: for i in TXC RXEN TXEN RXCIE UDRIE U2X UPE; do echo $i; grep --no-filename -r "#define $i[0-9]\? " /usr/lib/avr/include/avr/io* | sed "s/#define $i[0-9]\?\s*\(\S\)\+\s*\(\/\*.*\*\/\)\?$/\1/" | sort | uniq ; done This command shows that the above constants are identical for all uarts on all platforms, except for TXC, which is sometimes 6 and sometimes 0. Further investigation shows that it is always 6, except in io90scr100.h, but that file defines TXC0 with value 6 for the UART and uses TXC with value 0 for some USB-related register. This commit reduces program size on the uno by around 120 bytes. --- .../arduino/cores/arduino/HardwareSerial.cpp | 86 ++++++++++++------- .../arduino/cores/arduino/HardwareSerial.h | 8 +- 2 files changed, 55 insertions(+), 39 deletions(-) diff --git a/hardware/arduino/cores/arduino/HardwareSerial.cpp b/hardware/arduino/cores/arduino/HardwareSerial.cpp index e40bfbc0fd9..50edafe9749 100644 --- a/hardware/arduino/cores/arduino/HardwareSerial.cpp +++ b/hardware/arduino/cores/arduino/HardwareSerial.cpp @@ -34,19 +34,47 @@ #include "HardwareSerial.h" -/* - * on ATmega8, the uart and its bits are not numbered, so there is no "TXC0" - * definition. - */ +// Ensure that the various bit positions we use are available with a 0 +// postfix, so we can always use the values for UART0 for all UARTs. The +// alternative, passing the various values for each UART to the +// HardwareSerial constructor also works, but makes the code bigger and +// slower. #if !defined(TXC0) #if defined(TXC) +// On ATmega8, the uart and its bits are not numbered, so there is no TXC0 etc. #define TXC0 TXC +#define RXEN0 RXEN +#define TXEN0 TXEN +#define RXCIE0 RXCIE +#define UDRIE0 UDRIE +#define U2X0 U2X #elif defined(TXC1) // Some devices have uart1 but no uart0 #define TXC0 TXC1 +#define RXEN0 RXEN1 +#define TXEN0 TXEN1 +#define RXCIE0 RXCIE1 +#define UDRIE0 UDRIE1 +#define U2X0 U2X1 #else -#error TXC0 not definable in HardwareSerial.h +#error No UART found in HardwareSerial.cpp +#endif +#endif // !defined TXC0 + +// Check at compiletime that it is really ok to use the bit positions of +// UART0 for the other UARTs as well, in case these values ever get +// changed for future hardware. +#if defined(TXC1) && (TXC1 != TXC0 || RXEN1 != RXEN0 || RXCIE1 != RXCIE0 || \ + UDRIE1 != UDRIE0 || U2X1 != U2X0) +#error "Not all bit positions for UART1 are the same as for UART0" +#endif +#if defined(TXC2) && (TXC2 != TXC0 || RXEN2 != RXEN0 || RXCIE2 != RXCIE0 || \ + UDRIE2 != UDRIE0 || U2X2 != U2X0) +#error "Not all bit positions for UART2 are the same as for UART0" #endif +#if defined(TXC3) && (TXC3 != TXC0 || RXEN3 != RXEN0 || RXCIE3 != RXCIE0 || \ + UDRIE3 != UDRIE0 || U3X3 != U3X0) +#error "Not all bit positions for UART3 are the same as for UART0" #endif inline void store_char(unsigned char c, HardwareSerial *s) @@ -261,8 +289,7 @@ ISR(USART3_UDRE_vect) HardwareSerial::HardwareSerial( volatile uint8_t *ubrrh, volatile uint8_t *ubrrl, volatile uint8_t *ucsra, volatile uint8_t *ucsrb, - volatile uint8_t *ucsrc, volatile uint8_t *udr, - uint8_t rxen, uint8_t txen, uint8_t rxcie, uint8_t udrie, uint8_t u2x) + volatile uint8_t *ucsrc, volatile uint8_t *udr) { _tx_buffer_head = _tx_buffer_tail = 0; _rx_buffer_head = _rx_buffer_tail = 0; @@ -272,11 +299,6 @@ HardwareSerial::HardwareSerial( _ucsrb = ucsrb; _ucsrc = ucsrc; _udr = udr; - _rxen = rxen; - _txen = txen; - _rxcie = rxcie; - _udrie = udrie; - _u2x = u2x; } // Public Methods ////////////////////////////////////////////////////////////// @@ -298,7 +320,7 @@ void HardwareSerial::begin(unsigned long baud) try_again: if (use_u2x) { - *_ucsra = 1 << _u2x; + *_ucsra = 1 << U2X0; baud_setting = (F_CPU / 4 / baud - 1) / 2; } else { *_ucsra = 0; @@ -317,10 +339,10 @@ void HardwareSerial::begin(unsigned long baud) transmitting = false; - sbi(*_ucsrb, _rxen); - sbi(*_ucsrb, _txen); - sbi(*_ucsrb, _rxcie); - cbi(*_ucsrb, _udrie); + sbi(*_ucsrb, RXEN0); + sbi(*_ucsrb, TXEN0); + sbi(*_ucsrb, RXCIE0); + cbi(*_ucsrb, UDRIE0); } void HardwareSerial::begin(unsigned long baud, byte config) @@ -341,7 +363,7 @@ void HardwareSerial::begin(unsigned long baud, byte config) try_again: if (use_u2x) { - *_ucsra = 1 << _u2x; + *_ucsra = 1 << U2X0; baud_setting = (F_CPU / 4 / baud - 1) / 2; } else { *_ucsra = 0; @@ -364,10 +386,10 @@ void HardwareSerial::begin(unsigned long baud, byte config) #endif *_ucsrc = config; - sbi(*_ucsrb, _rxen); - sbi(*_ucsrb, _txen); - sbi(*_ucsrb, _rxcie); - cbi(*_ucsrb, _udrie); + sbi(*_ucsrb, RXEN0); + sbi(*_ucsrb, TXEN0); + sbi(*_ucsrb, RXCIE0); + cbi(*_ucsrb, UDRIE0); } void HardwareSerial::end() @@ -376,10 +398,10 @@ void HardwareSerial::end() while (_tx_buffer_head != _tx_buffer_tail) ; - cbi(*_ucsrb, _rxen); - cbi(*_ucsrb, _txen); - cbi(*_ucsrb, _rxcie); - cbi(*_ucsrb, _udrie); + cbi(*_ucsrb, RXEN0); + cbi(*_ucsrb, TXEN0); + cbi(*_ucsrb, RXCIE0); + cbi(*_ucsrb, UDRIE0); // clear any received data _rx_buffer_head = _rx_buffer_tail; @@ -431,7 +453,7 @@ size_t HardwareSerial::write(uint8_t c) _tx_buffer[_tx_buffer_head] = c; _tx_buffer_head = i; - sbi(*_ucsrb, _udrie); + sbi(*_ucsrb, UDRIE0); // clear the TXC bit -- "can be cleared by writing a one to its bit location" transmitting = true; sbi(*_ucsra, TXC0); @@ -446,9 +468,9 @@ HardwareSerial::operator bool() { // Preinstantiate Objects ////////////////////////////////////////////////////// #if defined(UBRRH) && defined(UBRRL) - HardwareSerial Serial(&UBRRH, &UBRRL, &UCSRA, &UCSRB, &UCSRC, &UDR, RXEN, TXEN, RXCIE, UDRIE, U2X); + HardwareSerial Serial(&UBRRH, &UBRRL, &UCSRA, &UCSRB, &UCSRC, &UDR); #elif defined(UBRR0H) && defined(UBRR0L) - HardwareSerial Serial(&UBRR0H, &UBRR0L, &UCSR0A, &UCSR0B, &UCSR0C, &UDR0, RXEN0, TXEN0, RXCIE0, UDRIE0, U2X0); + HardwareSerial Serial(&UBRR0H, &UBRR0L, &UCSR0A, &UCSR0B, &UCSR0C, &UDR0); #elif defined(USBCON) // do nothing - Serial object and buffers are initialized in CDC code #else @@ -456,13 +478,13 @@ HardwareSerial::operator bool() { #endif #if defined(UBRR1H) - HardwareSerial Serial1(&UBRR1H, &UBRR1L, &UCSR1A, &UCSR1B, &UCSR1C, &UDR1, RXEN1, TXEN1, RXCIE1, UDRIE1, U2X1); + HardwareSerial Serial1(&UBRR1H, &UBRR1L, &UCSR1A, &UCSR1B, &UCSR1C, &UDR1); #endif #if defined(UBRR2H) - HardwareSerial Serial2(&UBRR2H, &UBRR2L, &UCSR2A, &UCSR2B, &UCSR2C, &UDR2, RXEN2, TXEN2, RXCIE2, UDRIE2, U2X2); + HardwareSerial Serial2(&UBRR2H, &UBRR2L, &UCSR2A, &UCSR2B, &UCSR2C, &UDR2); #endif #if defined(UBRR3H) - HardwareSerial Serial3(&UBRR3H, &UBRR3L, &UCSR3A, &UCSR3B, &UCSR3C, &UDR3, RXEN3, TXEN3, RXCIE3, UDRIE3, U2X3); + HardwareSerial Serial3(&UBRR3H, &UBRR3L, &UCSR3A, &UCSR3B, &UCSR3C, &UDR3); #endif #endif // whole file diff --git a/hardware/arduino/cores/arduino/HardwareSerial.h b/hardware/arduino/cores/arduino/HardwareSerial.h index a091e0374a9..cdff65531d8 100644 --- a/hardware/arduino/cores/arduino/HardwareSerial.h +++ b/hardware/arduino/cores/arduino/HardwareSerial.h @@ -46,11 +46,6 @@ class HardwareSerial : public Stream volatile uint8_t *_ucsrb; volatile uint8_t *_ucsrc; volatile uint8_t *_udr; - uint8_t _rxen; - uint8_t _txen; - uint8_t _rxcie; - uint8_t _udrie; - uint8_t _u2x; bool transmitting; public: @@ -68,8 +63,7 @@ class HardwareSerial : public Stream HardwareSerial( volatile uint8_t *ubrrh, volatile uint8_t *ubrrl, volatile uint8_t *ucsra, volatile uint8_t *ucsrb, - volatile uint8_t *ucsrc, volatile uint8_t *udr, - uint8_t rxen, uint8_t txen, uint8_t rxcie, uint8_t udrie, uint8_t u2x); + volatile uint8_t *ucsrc, volatile uint8_t *udr); void begin(unsigned long); void begin(unsigned long, uint8_t); void end(); From 980405543249ee5cdf822a396914bda00a108d52 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Thu, 18 Apr 2013 11:38:13 +0200 Subject: [PATCH 04/15] Move interrupt handlers into HardwareSerial class The actual interrupt vectors are of course defined as before, but they let new methods in the HardwareSerial class do the actual work. This greatly reduces code duplication and prepares for one of my next commits which requires the tx interrupt handler to be called from another context as well. The actual content of the interrupts handlers was pretty much identical, so that remains unchanged (except that store_char was now only needed once, so it was inlined). Now all access to the buffers are inside the HardwareSerial class, the buffer variables can be made private. One would expect a program size reduction from this change (at least with multiple UARTs), but due to the fact that the interrupt handlers now only have indirect access to a few registers (which previously were just hardcoded in the handlers) and because there is some extra function call overhead, the code size on the uno actually increases by around 70 bytes. On the mega, which has four UARTs, the code size decreases by around 70 bytes. --- .../arduino/cores/arduino/HardwareSerial.cpp | 153 ++++++------------ .../arduino/cores/arduino/HardwareSerial.h | 6 +- 2 files changed, 55 insertions(+), 104 deletions(-) diff --git a/hardware/arduino/cores/arduino/HardwareSerial.cpp b/hardware/arduino/cores/arduino/HardwareSerial.cpp index 50edafe9749..5c9b3a6d144 100644 --- a/hardware/arduino/cores/arduino/HardwareSerial.cpp +++ b/hardware/arduino/cores/arduino/HardwareSerial.cpp @@ -48,6 +48,7 @@ #define RXCIE0 RXCIE #define UDRIE0 UDRIE #define U2X0 U2X +#define UPE0 UPE #elif defined(TXC1) // Some devices have uart1 but no uart0 #define TXC0 TXC1 @@ -56,6 +57,7 @@ #define RXCIE0 RXCIE1 #define UDRIE0 UDRIE1 #define U2X0 U2X1 +#define UPE0 UPE1 #else #error No UART found in HardwareSerial.cpp #endif @@ -65,32 +67,18 @@ // UART0 for the other UARTs as well, in case these values ever get // changed for future hardware. #if defined(TXC1) && (TXC1 != TXC0 || RXEN1 != RXEN0 || RXCIE1 != RXCIE0 || \ - UDRIE1 != UDRIE0 || U2X1 != U2X0) + UDRIE1 != UDRIE0 || U2X1 != U2X0 || UPE1 != UPE0) #error "Not all bit positions for UART1 are the same as for UART0" #endif #if defined(TXC2) && (TXC2 != TXC0 || RXEN2 != RXEN0 || RXCIE2 != RXCIE0 || \ - UDRIE2 != UDRIE0 || U2X2 != U2X0) + UDRIE2 != UDRIE0 || U2X2 != U2X0 || UPE2 != UPE0) #error "Not all bit positions for UART2 are the same as for UART0" #endif #if defined(TXC3) && (TXC3 != TXC0 || RXEN3 != RXEN0 || RXCIE3 != RXCIE0 || \ - UDRIE3 != UDRIE0 || U3X3 != U3X0) + UDRIE3 != UDRIE0 || U3X3 != U3X0 || UPE3 != UPE0) #error "Not all bit positions for UART3 are the same as for UART0" #endif -inline void store_char(unsigned char c, HardwareSerial *s) -{ - int i = (unsigned int)(s->_rx_buffer_head + 1) % SERIAL_BUFFER_SIZE; - - // if we should be storing the received character into the location - // just before the tail (meaning that the head would advance to the - // current location of the tail), we're about to overflow the buffer - // and so we don't write the character or advance the head. - if (i != s->_rx_buffer_tail) { - s->_rx_buffer[s->_rx_buffer_head] = c; - s->_rx_buffer_head = i; - } -} - #if !defined(USART0_RX_vect) && defined(USART1_RX_vect) // do nothing - on the 32u4 the first USART is USART1 #else @@ -109,23 +97,7 @@ inline void store_char(unsigned char c, HardwareSerial *s) ISR(USART_RXC_vect) // ATmega8 #endif { - #if defined(UDR0) - if (bit_is_clear(UCSR0A, UPE0)) { - unsigned char c = UDR0; - store_char(c, &Serial); - } else { - unsigned char c = UDR0; - }; - #elif defined(UDR) - if (bit_is_clear(UCSRA, PE)) { - unsigned char c = UDR; - store_char(c, &rx_buffer); - } else { - unsigned char c = UDR; - }; - #else - #error UDR not defined - #endif + Serial._rx_complete_irq(); } #endif #endif @@ -136,12 +108,7 @@ inline void store_char(unsigned char c, HardwareSerial *s) #define serialEvent1_implemented ISR(USART1_RX_vect) { - if (bit_is_clear(UCSR1A, UPE1)) { - unsigned char c = UDR1; - store_char(c, &Serial1); - } else { - unsigned char c = UDR1; - }; + Serial1._rx_complete_irq(); } #endif @@ -151,12 +118,7 @@ inline void store_char(unsigned char c, HardwareSerial *s) #define serialEvent2_implemented ISR(USART2_RX_vect) { - if (bit_is_clear(UCSR2A, UPE2)) { - unsigned char c = UDR2; - store_char(c, &Serial2); - } else { - unsigned char c = UDR2; - }; + Serial2._rx_complete_irq(); } #endif @@ -166,12 +128,7 @@ inline void store_char(unsigned char c, HardwareSerial *s) #define serialEvent3_implemented ISR(USART3_RX_vect) { - if (bit_is_clear(UCSR3A, UPE3)) { - unsigned char c = UDR3; - store_char(c, &Serial3); - } else { - unsigned char c = UDR3; - }; + Serial3._rx_complete_irq(); } #endif @@ -208,27 +165,7 @@ ISR(USART0_UDRE_vect) ISR(USART_UDRE_vect) #endif { - if (Serial._tx_buffer_head == Serial._tx_buffer_tail) { - // Buffer empty, so disable interrupts -#if defined(UCSR0B) - cbi(UCSR0B, UDRIE0); -#else - cbi(UCSRB, UDRIE); -#endif - } - else { - // There is more data in the output buffer. Send the next byte - unsigned char c = Serial._tx_buffer[Serial._tx_buffer_tail]; - Serial._tx_buffer_tail = (Serial._tx_buffer_tail + 1) % SERIAL_BUFFER_SIZE; - - #if defined(UDR0) - UDR0 = c; - #elif defined(UDR) - UDR = c; - #else - #error UDR not defined - #endif - } + Serial._tx_udr_empty_irq(); } #endif #endif @@ -236,53 +173,63 @@ ISR(USART_UDRE_vect) #ifdef USART1_UDRE_vect ISR(USART1_UDRE_vect) { - if (Serial1._tx_buffer_head == Serial1._tx_buffer_tail) { - // Buffer empty, so disable interrupts - cbi(UCSR1B, UDRIE1); - } - else { - // There is more data in the output buffer. Send the next byte - unsigned char c = Serial1._tx_buffer[Serial1._tx_buffer_tail]; - Serial1._tx_buffer_tail = (Serial1._tx_buffer_tail + 1) % SERIAL_BUFFER_SIZE; - - UDR1 = c; - } + Serial1._tx_udr_empty_irq(); } #endif #ifdef USART2_UDRE_vect ISR(USART2_UDRE_vect) { - if (Serial2._tx_buffer_head == Serial2._tx_buffer_tail) { - // Buffer empty, so disable interrupts - cbi(UCSR2B, UDRIE2); - } - else { - // There is more data in the output buffer. Send the next byte - unsigned char c = Serial2._tx_buffer[Serial2._tx_buffer_tail]; - Serial2._tx_buffer_tail = (Serial2._tx_buffer_tail + 1) % SERIAL_BUFFER_SIZE; - - UDR2 = c; - } + Serial2._tx_udr_empty_irq(); } #endif #ifdef USART3_UDRE_vect ISR(USART3_UDRE_vect) { - if (Serial3._tx_buffer_head == Serial3._tx_buffer_tail) { - // Buffer empty, so disable interrupts - cbi(UCSR3B, UDRIE3); + Serial3._tx_udr_empty_irq(); +} +#endif + + +// Actual interrupt handlers ////////////////////////////////////////////////////////////// + +void HardwareSerial::_rx_complete_irq(void) +{ + if (bit_is_clear(*_ucsra, UPE0)) { + // No Parity error, read byte and store it in the buffer if there is + // room + unsigned char c = *_udr; + int i = (unsigned int)(_rx_buffer_head + 1) % SERIAL_BUFFER_SIZE; + + // if we should be storing the received character into the location + // just before the tail (meaning that the head would advance to the + // current location of the tail), we're about to overflow the buffer + // and so we don't write the character or advance the head. + if (i != _rx_buffer_tail) { + _rx_buffer[_rx_buffer_head] = c; + _rx_buffer_head = i; + } + } else { + // Parity error, read byte but discard it + unsigned char c = *_udr; + }; +} + +void HardwareSerial::_tx_udr_empty_irq(void) +{ + if (_tx_buffer_head == _tx_buffer_tail) { + // Buffer empty, so disable interrupts + cbi(*_ucsrb, UDRIE0); } else { // There is more data in the output buffer. Send the next byte - unsigned char c = Serial3._tx_buffer[Serial3._tx_buffer_tail]; - Serial3._tx_buffer_tail = (Serial3._tx_buffer_tail + 1) % SERIAL_BUFFER_SIZE; - - UDR3 = c; + unsigned char c = _tx_buffer[_tx_buffer_tail]; + _tx_buffer_tail = (_tx_buffer_tail + 1) % SERIAL_BUFFER_SIZE; + + *_udr = c; } } -#endif // Constructors //////////////////////////////////////////////////////////////// diff --git a/hardware/arduino/cores/arduino/HardwareSerial.h b/hardware/arduino/cores/arduino/HardwareSerial.h index cdff65531d8..8f810250924 100644 --- a/hardware/arduino/cores/arduino/HardwareSerial.h +++ b/hardware/arduino/cores/arduino/HardwareSerial.h @@ -48,7 +48,6 @@ class HardwareSerial : public Stream volatile uint8_t *_udr; bool transmitting; - public: volatile uint8_t _rx_buffer_head; volatile uint8_t _rx_buffer_tail; volatile uint8_t _tx_buffer_head; @@ -60,6 +59,7 @@ class HardwareSerial : public Stream unsigned char _rx_buffer[SERIAL_BUFFER_SIZE]; unsigned char _tx_buffer[SERIAL_BUFFER_SIZE]; + public: HardwareSerial( volatile uint8_t *ubrrh, volatile uint8_t *ubrrl, volatile uint8_t *ucsra, volatile uint8_t *ucsrb, @@ -78,6 +78,10 @@ class HardwareSerial : public Stream inline size_t write(int n) { return write((uint8_t)n); } using Print::write; // pull in write(str) and write(buf, size) from Print operator bool(); + + // Interrupt handlers - Not intended to be called externally + void _rx_complete_irq(void); + void _tx_udr_empty_irq(void); }; // Define config for Serial.begin(baud, config); From 3b28084b1886f23bbc68b631ee05f4c4a3e0f52e Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Thu, 18 Apr 2013 20:20:03 +0200 Subject: [PATCH 05/15] Use bit_is_clear in HardwareSerial::flush() This is slightly more clear than the previous explicit comparison. --- hardware/arduino/cores/arduino/HardwareSerial.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hardware/arduino/cores/arduino/HardwareSerial.cpp b/hardware/arduino/cores/arduino/HardwareSerial.cpp index 5c9b3a6d144..f852efd189c 100644 --- a/hardware/arduino/cores/arduino/HardwareSerial.cpp +++ b/hardware/arduino/cores/arduino/HardwareSerial.cpp @@ -383,7 +383,7 @@ 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 && ! (*_ucsra & _BV(TXC0))); + while (transmitting && bit_is_clear(*_ucsra, TXC0)); transmitting = false; } From b008ef6924de8ab870a6d5722501bdd654bdd5d7 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Thu, 18 Apr 2013 21:12:01 +0200 Subject: [PATCH 06/15] 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). --- .../arduino/cores/arduino/HardwareSerial.cpp | 22 ++++++++++++++----- .../arduino/cores/arduino/HardwareSerial.h | 3 ++- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/hardware/arduino/cores/arduino/HardwareSerial.cpp b/hardware/arduino/cores/arduino/HardwareSerial.cpp index f852efd189c..cd97e99014f 100644 --- a/hardware/arduino/cores/arduino/HardwareSerial.cpp +++ b/hardware/arduino/cores/arduino/HardwareSerial.cpp @@ -284,7 +284,7 @@ void HardwareSerial::begin(unsigned long baud) *_ubrrh = baud_setting >> 8; *_ubrrl = baud_setting; - transmitting = false; + _written = false; sbi(*_ucsrb, RXEN0); sbi(*_ucsrb, TXEN0); @@ -327,6 +327,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) @@ -382,9 +384,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) @@ -401,8 +409,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; diff --git a/hardware/arduino/cores/arduino/HardwareSerial.h b/hardware/arduino/cores/arduino/HardwareSerial.h index 8f810250924..ce6899cac1b 100644 --- a/hardware/arduino/cores/arduino/HardwareSerial.h +++ b/hardware/arduino/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; From bf41c7f0ab28994be9332bb73d34e4f7c89b9b2a Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Fri, 19 Apr 2013 12:56:54 +0200 Subject: [PATCH 07/15] 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. --- hardware/arduino/cores/arduino/HardwareSerial.cpp | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/hardware/arduino/cores/arduino/HardwareSerial.cpp b/hardware/arduino/cores/arduino/HardwareSerial.cpp index cd97e99014f..9c64b5f1500 100644 --- a/hardware/arduino/cores/arduino/HardwareSerial.cpp +++ b/hardware/arduino/cores/arduino/HardwareSerial.cpp @@ -228,6 +228,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); } } @@ -390,9 +395,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) @@ -410,10 +415,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; } From d7e814310ed7061be7f5428844758162de13efd2 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Thu, 18 Apr 2013 21:34:00 +0200 Subject: [PATCH 08/15] Fix lockup when writing to HardwareSerial with interrupts disabled When interrupts are disabled, writing to HardwareSerial could cause a lockup. When the tx buffer is full, a busy-wait loop is used to wait for the interrupt handler to free up a byte in the buffer. However, when interrupts are disabled, this will of course never happen and the Arduino will lock up. This often caused lockups when doing (big) debug printing from an interrupt handler. Additionally, calling flush() with interrupts disabled while transmission was in progress would also cause a lockup. When interrupts are disabled, the code now actively checks the UDRE (UART Data Register Empty) and calls the interrupt handler to free up room if the bit is set. This can lead to delays in interrupt handlers when the serial buffer is full, but a delay is of course always preferred to a lockup. Closes: #672 References: #1147 --- .../arduino/cores/arduino/HardwareSerial.cpp | 26 +++++++++++++++---- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/hardware/arduino/cores/arduino/HardwareSerial.cpp b/hardware/arduino/cores/arduino/HardwareSerial.cpp index 9c64b5f1500..6715635b4e7 100644 --- a/hardware/arduino/cores/arduino/HardwareSerial.cpp +++ b/hardware/arduino/cores/arduino/HardwareSerial.cpp @@ -395,7 +395,14 @@ void HardwareSerial::flush() if (!_written) return; - while (bit_is_set(*_ucsrb, UDRIE0) || bit_is_clear(*_ucsra, TXC0)); + while (bit_is_set(*_ucsrb, UDRIE0) || bit_is_clear(*_ucsra, TXC0)) { + if (bit_is_clear(SREG, SREG_I) && bit_is_set(*_ucsrb, UDRIE0)) + // Interrupts are globally disabled, but the DR empty + // interrupt should be enabled, so poll the DR empty flag to + // prevent deadlock + if (bit_is_set(*_ucsra, UDRE0)) + _tx_udr_empty_irq(); + } // If we get here, nothing is queued anymore (DRIE is disabled) and // the hardware finished tranmission (TXC is set). } @@ -406,10 +413,19 @@ size_t HardwareSerial::write(uint8_t c) // If the output buffer is full, there's nothing for it other than to // wait for the interrupt handler to empty it a bit - // ???: return 0 here instead? - while (i == _tx_buffer_tail) - ; - + while (i == _tx_buffer_tail) { + if (bit_is_clear(SREG, SREG_I)) { + // Interrupts are disabled, so we'll have to poll the data + // register empty flag ourselves. If it is set, pretend an + // interrupt has happened and call the handler to free up + // space for us. + if(bit_is_set(*_ucsra, UDRE0)) + _tx_udr_empty_irq(); + } else { + // nop, the interrupt handler will free up space for us + } + } + _tx_buffer[_tx_buffer_head] = c; _tx_buffer_head = i; From 282f0f02e73b7a2a3949d68d6a357b107910dcd2 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Thu, 18 Apr 2013 16:55:06 +0200 Subject: [PATCH 09/15] Make all access to the HardwareSerial ringbuffers atomic This prevents a potential race condition where for example write() would detect that there is room in the tx buffer, an interrupt would trigger where the interrupt handler writes to Serial filling up that room, and write() would then reset the head pointer, effectively throwing away the bytes written by the interrupt handler. References: #1147 --- .../arduino/cores/arduino/HardwareSerial.cpp | 90 ++++++++++++------- 1 file changed, 56 insertions(+), 34 deletions(-) diff --git a/hardware/arduino/cores/arduino/HardwareSerial.cpp b/hardware/arduino/cores/arduino/HardwareSerial.cpp index 6715635b4e7..ebae3bd6567 100644 --- a/hardware/arduino/cores/arduino/HardwareSerial.cpp +++ b/hardware/arduino/cores/arduino/HardwareSerial.cpp @@ -25,6 +25,8 @@ #include #include #include +#include +#include #include "Arduino.h" #include "wiring_private.h" @@ -363,27 +365,33 @@ void HardwareSerial::end() int HardwareSerial::available(void) { - return (unsigned int)(SERIAL_BUFFER_SIZE + _rx_buffer_head - _rx_buffer_tail) % SERIAL_BUFFER_SIZE; + ATOMIC_BLOCK(ATOMIC_RESTORESTATE) { + return (unsigned int)(SERIAL_BUFFER_SIZE + _rx_buffer_head - _rx_buffer_tail) % SERIAL_BUFFER_SIZE; + } } int HardwareSerial::peek(void) { - if (_rx_buffer_head == _rx_buffer_tail) { - return -1; - } else { - return _rx_buffer[_rx_buffer_tail]; + ATOMIC_BLOCK(ATOMIC_RESTORESTATE) { + if (_rx_buffer_head == _rx_buffer_tail) { + return -1; + } else { + return _rx_buffer[_rx_buffer_tail]; + } } } int HardwareSerial::read(void) { - // if the head isn't ahead of the tail, we don't have any characters - if (_rx_buffer_head == _rx_buffer_tail) { - return -1; - } else { - unsigned char c = _rx_buffer[_rx_buffer_tail]; - _rx_buffer_tail = (unsigned int)(_rx_buffer_tail + 1) % SERIAL_BUFFER_SIZE; - return c; + ATOMIC_BLOCK(ATOMIC_RESTORESTATE) { + // if the head isn't ahead of the tail, we don't have any characters + if (_rx_buffer_head == _rx_buffer_tail) { + return -1; + } else { + unsigned char c = _rx_buffer[_rx_buffer_tail]; + _rx_buffer_tail = (unsigned int)(_rx_buffer_tail + 1) % SERIAL_BUFFER_SIZE; + return c; + } } } @@ -409,30 +417,44 @@ void HardwareSerial::flush() size_t HardwareSerial::write(uint8_t c) { - int i = (_tx_buffer_head + 1) % SERIAL_BUFFER_SIZE; - - // If the output buffer is full, there's nothing for it other than to - // wait for the interrupt handler to empty it a bit - while (i == _tx_buffer_tail) { - if (bit_is_clear(SREG, SREG_I)) { - // Interrupts are disabled, so we'll have to poll the data - // register empty flag ourselves. If it is set, pretend an - // interrupt has happened and call the handler to free up - // space for us. - if(bit_is_set(*_ucsra, UDRE0)) - _tx_udr_empty_irq(); - } else { - // nop, the interrupt handler will free up space for us + bool interrupts_enabled = bit_is_set(SREG, SREG_I); + ATOMIC_BLOCK(ATOMIC_RESTORESTATE) { + int i = (_tx_buffer_head + 1) % SERIAL_BUFFER_SIZE; + + // If the output buffer is full, there's nothing for it other than to + // wait for the interrupt handler to empty it a bit + while (i == _tx_buffer_tail) { + if (!interrupts_enabled) { + // Interrupts were disabled at the start of this function, so we + // can't just enable them (that would allow other interrupts to + // trigger as well!). We'll have to poll the data register empty + // flag ourselves. If it is set, pretend an interrupt has happened + // and call the handler to free up space for us. + if(bit_is_set(*_ucsra, UDRE0)) + _tx_udr_empty_irq(); + } else { + // Interrupts were enabled, so we can temporarily enable them so + // our tx interrupt can fire and free up some space. Note that we + // can't just use the UDRE polling approach here instead of + // enabling interrupts, since that would prevent other + // interrupts from triggering while we wait for room. + NONATOMIC_BLOCK(ATOMIC_RESTORESTATE) { + _NOP(); // Any pending interrupts will trigger here + } + // Recalculate i, since other interrupt handlers might have + // written to our buffer + i = (_tx_buffer_head + 1) % SERIAL_BUFFER_SIZE; + } } - } - _tx_buffer[_tx_buffer_head] = c; - _tx_buffer_head = i; - - sbi(*_ucsrb, UDRIE0); - _written = true; - - return 1; + _tx_buffer[_tx_buffer_head] = c; + _tx_buffer_head = i; + + sbi(*_ucsrb, UDRIE0); + _written = true; + + return 1; + } } HardwareSerial::operator bool() { From c598b8e2a025a10b509bf8b663432aa8b6ecc833 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Thu, 18 Apr 2013 19:06:00 +0200 Subject: [PATCH 10/15] Disable the UDRE interrupt sooner in HardwareSerial Before, the interrupt was disabled when it was triggered and it turned out there was no data to send. However, the interrupt can be disabled already when the last byte is written to the UART, since write() will always re-enable the interrupt when it adds new data to the buffer. Closes: #1008 --- .../arduino/cores/arduino/HardwareSerial.cpp | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/hardware/arduino/cores/arduino/HardwareSerial.cpp b/hardware/arduino/cores/arduino/HardwareSerial.cpp index ebae3bd6567..7b5fb16ba0b 100644 --- a/hardware/arduino/cores/arduino/HardwareSerial.cpp +++ b/hardware/arduino/cores/arduino/HardwareSerial.cpp @@ -220,22 +220,22 @@ void HardwareSerial::_rx_complete_irq(void) void HardwareSerial::_tx_udr_empty_irq(void) { + // If interrupts are enabled, there must be more data in the output + // buffer. Send the next byte + unsigned char c = _tx_buffer[_tx_buffer_tail]; + _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); + if (_tx_buffer_head == _tx_buffer_tail) { // Buffer empty, so disable interrupts cbi(*_ucsrb, UDRIE0); } - else { - // There is more data in the output buffer. Send the next byte - unsigned char c = _tx_buffer[_tx_buffer_tail]; - _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); - } } // Constructors //////////////////////////////////////////////////////////////// From c3b4ff3faa142614e75a4cd007431b8268388535 Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Thu, 18 Apr 2013 18:46:14 +0200 Subject: [PATCH 11/15] Make private members of HardwareSerial protected This allows users to create subclasses. Closes: #947 --- hardware/arduino/cores/arduino/HardwareSerial.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hardware/arduino/cores/arduino/HardwareSerial.h b/hardware/arduino/cores/arduino/HardwareSerial.h index ce6899cac1b..c6649df83a6 100644 --- a/hardware/arduino/cores/arduino/HardwareSerial.h +++ b/hardware/arduino/cores/arduino/HardwareSerial.h @@ -39,7 +39,7 @@ class HardwareSerial : public Stream { - private: + protected: volatile uint8_t *_ubrrh; volatile uint8_t *_ubrrl; volatile uint8_t *_ucsra; From 1b17ed8663f596932e31827cbd0159cc6064a90c Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Fri, 19 Apr 2013 15:29:43 +0200 Subject: [PATCH 12/15] Remove duplicate code from HardwareSerial::begin() methods. There are two begin methods, one which accepts just a baud rate and uses the default bit settings and one which accepts both a baudrate and a bit config. Previously, both of these contained a complete implementation, but now the former just calls the latter, explicitely passing the default 8N1 configuration. Technically, this causes a small change: Before the UCSRC register was untouched when calling begin(baud), now it is explicitely initialized with 8N1. However, since this is the default configuration for at least the Uno and the Mega (didn't check any others), probably for all avrs, this shouldn't effectively change anything. Given that the Arduino documentation also documents this as the default when none is passed, explicitly setting it is probably a good idea in any case. --- .../arduino/cores/arduino/HardwareSerial.cpp | 39 +------------------ 1 file changed, 1 insertion(+), 38 deletions(-) diff --git a/hardware/arduino/cores/arduino/HardwareSerial.cpp b/hardware/arduino/cores/arduino/HardwareSerial.cpp index 7b5fb16ba0b..498033e88ad 100644 --- a/hardware/arduino/cores/arduino/HardwareSerial.cpp +++ b/hardware/arduino/cores/arduino/HardwareSerial.cpp @@ -259,44 +259,7 @@ HardwareSerial::HardwareSerial( void HardwareSerial::begin(unsigned long baud) { - uint16_t baud_setting; - bool use_u2x = true; - -#if F_CPU == 16000000UL - // hardcoded exception for compatibility with the bootloader shipped - // with the Duemilanove and previous boards and the firmware on the 8U2 - // on the Uno and Mega 2560. - if (baud == 57600) { - use_u2x = false; - } -#endif - -try_again: - - if (use_u2x) { - *_ucsra = 1 << U2X0; - baud_setting = (F_CPU / 4 / baud - 1) / 2; - } else { - *_ucsra = 0; - baud_setting = (F_CPU / 8 / baud - 1) / 2; - } - - if ((baud_setting > 4095) && use_u2x) - { - use_u2x = false; - goto try_again; - } - - // assign the baud_setting, a.k.a. ubbr (USART Baud Rate Register) - *_ubrrh = baud_setting >> 8; - *_ubrrl = baud_setting; - - _written = false; - - sbi(*_ucsrb, RXEN0); - sbi(*_ucsrb, TXEN0); - sbi(*_ucsrb, RXCIE0); - cbi(*_ucsrb, UDRIE0); + begin(baud, SERIAL_8N1); } void HardwareSerial::begin(unsigned long baud, byte config) From f60d5ccf20ee8b5abe0aa3c90fc4c88347ae74cb Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Fri, 19 Apr 2013 16:12:31 +0200 Subject: [PATCH 13/15] Remove unused variable --- hardware/arduino/cores/arduino/HardwareSerial.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/hardware/arduino/cores/arduino/HardwareSerial.cpp b/hardware/arduino/cores/arduino/HardwareSerial.cpp index 498033e88ad..81c21f75c91 100644 --- a/hardware/arduino/cores/arduino/HardwareSerial.cpp +++ b/hardware/arduino/cores/arduino/HardwareSerial.cpp @@ -265,7 +265,6 @@ void HardwareSerial::begin(unsigned long baud) void HardwareSerial::begin(unsigned long baud, byte config) { uint16_t baud_setting; - uint8_t current_config; bool use_u2x = true; #if F_CPU == 16000000UL From 28fe88c8be6b37cab33437d44c46c760ebf4c27a Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Fri, 19 Apr 2013 16:32:33 +0200 Subject: [PATCH 14/15] Simplify HardwareSerial::begin() This simplifies the baud rate calculation, removing the need for a goto and shortening the code a bit. Other than that, this code should not use any different settings than before. Code was suggested by Rob Tillaart on github. Closes: #1262 --- .../arduino/cores/arduino/HardwareSerial.cpp | 35 ++++++------------- 1 file changed, 11 insertions(+), 24 deletions(-) diff --git a/hardware/arduino/cores/arduino/HardwareSerial.cpp b/hardware/arduino/cores/arduino/HardwareSerial.cpp index 81c21f75c91..09f8e313e03 100644 --- a/hardware/arduino/cores/arduino/HardwareSerial.cpp +++ b/hardware/arduino/cores/arduino/HardwareSerial.cpp @@ -264,33 +264,20 @@ void HardwareSerial::begin(unsigned long baud) void HardwareSerial::begin(unsigned long baud, byte config) { - uint16_t baud_setting; - bool use_u2x = true; - -#if F_CPU == 16000000UL - // hardcoded exception for compatibility with the bootloader shipped - // with the Duemilanove and previous boards and the firmware on the 8U2 - // on the Uno and Mega 2560. - if (baud == 57600) { - use_u2x = false; - } -#endif - -try_again: - - if (use_u2x) { - *_ucsra = 1 << U2X0; - baud_setting = (F_CPU / 4 / baud - 1) / 2; - } else { + // Try u2x mode first + uint16_t baud_setting = (F_CPU / 4 / baud - 1) / 2; + *_ucsra = 1 << U2X0; + + // hardcoded exception for 57600 for compatibility with the bootloader + // shipped with the Duemilanove and previous boards and the firmware + // on the 8U2 on the Uno and Mega 2560. Also, The baud_setting cannot + // be > 4095, so switch back to non-u2x mode if the baud rate is too + // low. + if (((F_CPU == 16000000UL) && (baud == 57600)) || (baud_setting >4095)) + { *_ucsra = 0; baud_setting = (F_CPU / 8 / baud - 1) / 2; } - - if ((baud_setting > 4095) && use_u2x) - { - use_u2x = false; - goto try_again; - } // assign the baud_setting, a.k.a. ubbr (USART Baud Rate Register) *_ubrrh = baud_setting >> 8; From ccefff5995b64c67a060f6a781813c5137102bfe Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Thu, 18 Apr 2013 17:53:40 +0200 Subject: [PATCH 15/15] Make blocking behaviour of HardwareSerial::write optional This introduces two new methods, Serial.setBlocking() and Serial.getBlocking(), which can be used to enable or disable blocking. References: #672 --- TODO: This commit is probably not ready to merge, I'm not sure what the proper API for this feature would be (the current enum approach requires qualifying the constants with HardwareSerial:: for example). I'm also not sure where and how to add the new function to a keywords.txt file. --- .../arduino/cores/arduino/HardwareSerial.cpp | 15 +++++++++++++++ hardware/arduino/cores/arduino/HardwareSerial.h | 16 ++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/hardware/arduino/cores/arduino/HardwareSerial.cpp b/hardware/arduino/cores/arduino/HardwareSerial.cpp index 09f8e313e03..9b97e7ffe29 100644 --- a/hardware/arduino/cores/arduino/HardwareSerial.cpp +++ b/hardware/arduino/cores/arduino/HardwareSerial.cpp @@ -253,6 +253,7 @@ HardwareSerial::HardwareSerial( _ucsrb = ucsrb; _ucsrc = ucsrc; _udr = udr; + _blocking = SERIAL_BLOCK_ALWAYS; } // Public Methods ////////////////////////////////////////////////////////////// @@ -312,6 +313,16 @@ void HardwareSerial::end() _rx_buffer_head = _rx_buffer_tail; } +void HardwareSerial::setBlocking(blocking_behaviour when) +{ + _blocking = when; +} + +HardwareSerial::blocking_behaviour HardwareSerial::getBlocking() +{ + return _blocking; +} + int HardwareSerial::available(void) { ATOMIC_BLOCK(ATOMIC_RESTORESTATE) { @@ -374,6 +385,8 @@ size_t HardwareSerial::write(uint8_t c) // wait for the interrupt handler to empty it a bit while (i == _tx_buffer_tail) { if (!interrupts_enabled) { + if (!(_blocking & SERIAL_BLOCK_NON_INTERRUPTIBLE)) + return 0; // Interrupts were disabled at the start of this function, so we // can't just enable them (that would allow other interrupts to // trigger as well!). We'll have to poll the data register empty @@ -382,6 +395,8 @@ size_t HardwareSerial::write(uint8_t c) if(bit_is_set(*_ucsra, UDRE0)) _tx_udr_empty_irq(); } else { + if (!(_blocking & SERIAL_BLOCK_INTERRUPTIBLE)) + return 0; // Interrupts were enabled, so we can temporarily enable them so // our tx interrupt can fire and free up some space. Note that we // can't just use the UDRE polling approach here instead of diff --git a/hardware/arduino/cores/arduino/HardwareSerial.h b/hardware/arduino/cores/arduino/HardwareSerial.h index c6649df83a6..b6e2d0881f0 100644 --- a/hardware/arduino/cores/arduino/HardwareSerial.h +++ b/hardware/arduino/cores/arduino/HardwareSerial.h @@ -39,6 +39,19 @@ class HardwareSerial : public Stream { + public: + // Describes the blocking behaviour for writing (note that flushing + // will _always_ block until the transmission is complete). + enum blocking_behaviour { + // Never block, discard data when buffer is full + SERIAL_BLOCK_NEVER = 0, + // Block only when interrupts are enabled + SERIAL_BLOCK_INTERRUPTIBLE = 1 << 0, + // Block only when interrupts are disabled + SERIAL_BLOCK_NON_INTERRUPTIBLE = 1 << 1, + // Always block, never discard data + SERIAL_BLOCK_ALWAYS = SERIAL_BLOCK_INTERRUPTIBLE | SERIAL_BLOCK_NON_INTERRUPTIBLE, + }; protected: volatile uint8_t *_ubrrh; volatile uint8_t *_ubrrl; @@ -48,6 +61,7 @@ class HardwareSerial : public Stream volatile uint8_t *_udr; // Has any byte been written to the UART since begin() bool _written; + blocking_behaviour _blocking; volatile uint8_t _rx_buffer_head; volatile uint8_t _rx_buffer_tail; @@ -68,6 +82,8 @@ class HardwareSerial : public Stream void begin(unsigned long); void begin(unsigned long, uint8_t); void end(); + void setBlocking(blocking_behaviour when); + blocking_behaviour getBlocking(); virtual int available(void); virtual int peek(void); virtual int read(void);