Reading interrupt safe ring buffer

Question

I have a piece of code here that was generated by Atmel START Wizard. It setups a UART and it also creates a ring buffer for reading data.

This is the code that reads data from the buffer:

uint8_t USART_2_read(void)
{
    uint8_t tmptail;

    /* Wait for incoming data */
    while (USART_2_rx_elements == 0)
        ;
    /* Calculate buffer index */
    tmptail = (USART_2_rx_tail + 1) & USART_2_RX_BUFFER_MASK;
    /* Store new index */
    USART_2_rx_tail = tmptail;
    ENTER_CRITICAL(R);
    USART_2_rx_elements--;
    EXIT_CRITICAL(R);

    /* Return data */
    return USART_2_rxbuf[tmptail];
}

When the main program access the buffer it calls an ENTER_CRITICAL(R) macro before decreases the elements counter by one.

Here is the ENTER_CRITICAL macro:

#define ENTER_CRITICAL(UNUSED) __asm__ __volatile__ (   \
   "in __tmp_reg__, __SREG__"                    "\n\t" \
   "cli"                                         "\n\t" \
   "push __tmp_reg__"                            "\n\t" \
   ::: "memory"                                         \
   )

and the EXIT_CRITICAL macro:

#define EXIT_CRITICAL(UNUSED)  __asm__ __volatile__ (   \
   "pop __tmp_reg__"                             "\n\t" \
   "out __SREG__, __tmp_reg__"                   "\n\t" \
   ::: "memory"                                         \
   )

I noticed that it disables the global interrupt, not only the Receive Complete Interrupt of the UART. Usually what I'd do is to disable only the "Receive Complete Interrupt" letting any other interrupt enabled. This all make me wonder if there's any particular reason that Atmel or Microchip developers make it that way.

Does Global Interrupt need to be disabled? Would the other interrupt affect this operation if they were enabled? Is there any difference by disabling only peripheral's respective interrupt?

This code is running on an AVR8

Answer 1

Such macros are usually put where there is code that must not be interrupted by any interrupt; so yes, disabling all interrupts is what you'd typically do here.

Note that on more complex cores, this is often undesirable, and a sign of either complex synchronization challenges (you find these critical regions in kernels for multi-core capable OSes, too), or bad design (because interrupts can, on many platforms, have priorities, thus allowing well-written systems to function without disabling all interrupts).

In this case, yes, you know which interrupt to disable, so just disable that one instead of having a critical section.

Atmel's software tools can't know that, because nobody guarantees that other ISRs than the one you're considering aren't messing with that counter, so they have to play it safe and disable all interrupts.

Answer 2

The macro is likely meant as an universal one, macros like these toggling/storing the global interrupt mask are common. It is sloppy style to disable the global interrupt in this case.

Indeed it is much better practice to only disable the UART-specific interrupt. You should do that from inside your UART driver.

The code also looks fishy since it doesn't protect the data access, just the size counter. Both of these ought to be protected unless atomic reads/writes can be guaranteed. Note that the size of data vs size of MCU doesn't matter at all, because a data read in C isn't guaranteed to be single instruction (unless C11 _Atomic is used). As an alternative do disabling the interrupt, you could also consider writing the critical parts in inline assembler.