What is the logic behind Arduino inlining `HardwareSerial::_rx_complete_irq()` for receiving serial data (but NOT `_tx_udr_empty_irq()`)?

Question

Q: What is the logic behind Arduino inlining HardwareSerial::_rx_complete_irq() for receiving serial data (but NOT _tx_udr_empty_irq()), and when is it advisable?

From the bottom of the HardwareSerial class in HardwareSerial.h:

// Interrupt handlers - Not intended to be called externally
inline void _rx_complete_irq(void);  // <======= inline!
void _tx_udr_empty_irq(void);        // <======= NOT inline! Why?

Furthermore, what is the design logic behind putting one of the serial ISR function definitions in a header file vs in a source file? Also, when is this good design and what are the tradeoffs, and when is it illegal or not permitted by the language, compiler or anything else?

Here is the exact scenario that made me think of this question:

---

See here for the HardwareSerial implementation files: https://github.com/arduino/ArduinoCore-avr/tree/master/cores/arduino

Here is the main header file. https://github.com/arduino/ArduinoCore-avr/blob/master/cores/arduino/HardwareSerial.h

The inline ISR _rx_complete_irq():

• Line 138 of "HardwareSerial.h" declares the inline ISR for when serial data is received:

   inline void _rx_complete_irq(void);
  

  • This ISR is called whenever "there are unread data present in the receive buffer." (ATmega328 Datasheet 20.7.3 p190)

  • Line 40 and 48-50 of "HardwareSerial0.cpp" is where the ISR is set up:

       ISR(USART_RX_vect)  // line 40
       {                   // line 48
         Serial._rx_complete_irq();  // line 49
       }                   // line 50
    

  • Line 101-121 of "HardwareSerial_private.h" implements the inline _rx_complete_irq() function:

      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;
          rx_buffer_index_t i = (unsigned int)(_rx_buffer_head + 1) % SERIAL_RX_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
          *_udr;
        };
      }
    

The NON-inline ISR _tx_udr_empty_irq():

• Line 139 of "HardwareSerial.h" declares the ISR for transmitting serial data: void _tx_udr_empty_irq(void);

  • This ISR is triggered by the "USART Data Register Empty" flag and is called whenever the transmit buffer has passed its value to the shift register and now is "ready to receive new data" (ATmega328 Datasheet 20.11.2 p200)

  • Its implementation is on lines 89-113 of "HardwareSerial.cpp"

       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_TX_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. Other r/w bits are preserved, and zeroes
         // written to the rest.
    
       #ifdef MPCM0
         *_ucsra = ((*_ucsra) & ((1 << U2X0) | (1 << MPCM0))) | (1 << TXC0);
       #else
         *_ucsra = ((*_ucsra) & ((1 << U2X0) | (1 << TXC0)));
       #endif
    
         if (_tx_buffer_head == _tx_buffer_tail) {
           // Buffer empty, so disable interrupts
           cbi(*_ucsrb, UDRIE0);
         }
       }
    

Why the difference? Why inline one ISR and not the other?

Again, from the bottom of the HardwareSerial class in HardwareSerial.h:

// Interrupt handlers - Not intended to be called externally
inline void _rx_complete_irq(void);  // <======= inline!
void _tx_udr_empty_irq(void);        // <======= NOT inline! Why?

Why the somewhat complicated set of 3+ files? Mainly:

• HardwareSerial.h
• HardwareSerial_private.h
• HardwareSerial.cpp

_{I first documented this to myself and wondered about it on 31 Jan. 2018, while studying the Arduino source code. I'd like to hear some more insight on the topic from other developers.}

Answer 1

What is the logic behind Arduino inlining HardwareSerial::_rx_complete_irq() for receiving serial data (but NOT _tx_udr_empty_irq()), and when is it advisable?

There's a couple of reasons which end up with this arrangement:

1. The RX interrupt has to be fast as we're reacting to an external stimulus. The TX interrupt doesn't have to be fast as it's only used to move data from the buffer to the UART.

We need to be able to respond rapidly to incoming data so we can store it away in the RX ring buffer in time to handle the next byte that arrives. Not really a problem at low baud rates, but when it comes to higher ones every cycle that can be shaved off that interrupt time is beneficial. The TX interrupt, though, really doesn't matter. All it does is load the next byte into the outgoing UART buffer, and it doesn't matter if that takes a little longer, so there's no need to shave off the extra instruction cycles consumed by making a function call.

2. The TX interrupt is called from multiple places but the RX isn't.

The RX interrupt code is only ever called when the interrupt fires. That means it's only ever going to be called through one execution path. However the TX interrupt code is not only called by the interrupt itself, but by other areas of the code. For example, if interrupts are disabled then the write function will manually poll the UART for the buffer state and manually call the interrupt function to send the data (see line 262 of HardwareSerial.cpp).

If the TX interrupt code were inlined then there would be lots of places throughout the code where that block would be duplicated causing code bloat. That's code bloat with no reason (See point 1) so it's something to be avoided.

Furthermore, what is the design logic behind putting one of the serial ISR function definitions in a header file vs in a source file?

Inlining can only be done within a single translation unit. If you want a function to be inlined in multiple translation units (HardwareSerial0.cpp, HardwareSerial1.cpp, etc) then you would have to have it within those translation units. And that means that you need it in the header¹. Since the TX interrupt is not wanted to be inlined it can be put in a CPP file instead.

Why the somewhat complicated set of 3+ files?

Simple: management. It's easier to manage the (rather complex UART) code if it's broken down into different areas. You have a common base object in HardwareSerial.cpp which is then used by the code for each individual UART in the HardwareSerial[0-3].cpp files. You have a single header file that defines the class and all its methods and data (HardwareSerial.h). And then you have the "private" header with any inline functions in it that is included where it's needed.

True you could merge the HardwareSerial.h and the HardwareSerial_private.h into one, but this way the inline functions are excluded from translation units where they aren't needed. This way you don't get those chunks of code included in your sketch. True the linker will discard them as they're not used, but it's just tidier this way - the functions are only included in those translation units where those function are actually used.

---

Notes:

1. This code was written before the compiler had support for Link Time Optimization which allows inlining between translation units. The compiler may have moved on, but there is no point in changing code that works perfectly fine.