2
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ Indeed, you may implement the ring buffer in a way that you don't need to disable the interrupts at all: Use (only) two variables: One pointer pointing to the next element to be written and another one pointing to the next element to be read. Make sure to increment the pointers after accessing the buffer elements (i.e. don't use buffer[pos++]=... but buffer[pos]=...; pos++;) and calculate the number of elements in the buffer from both pointers... \$\endgroup\$ – Martin Rosenau Nov 11 at 21:37
7
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks! I haven't thought about the case when other ISRs mixing with the counter. I've seen pretty similar pieces of codes written by senior firmware engineers and when I first saw this code I thought that I might lose something I'm really not aware of. \$\endgroup\$ – MrBit Nov 11 at 9:23
2
\$\begingroup\$

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ writing the critical parts in assembler won't help at all when an ISR fires while you're handling the critical bookkeeping; other than that: fully agree. I presume the data access doesn't have to be protected, because it's read-only by contract and a RX ring buffer, so addresses only increase, and that's kind of non-atomic-safe. \$\endgroup\$ – Marcus Müller Nov 11 at 9:11
  • \$\begingroup\$ @MarcusMüller That you don't have to protect read-only from race conditions is a myth: suppose that you always need to use the most recent data. Without protection you get "store data in x", "interrupt" and now "x" contains outdated data - we've caused 1 packet lag whenever data read occurs at the same time as the interrupt. This would cause intermittent, mysterious packet lag on rare occasions. \$\endgroup\$ – Lundin Nov 11 at 9:22
  • \$\begingroup\$ "suppose you always need the latest data": no, you can't suppose that. Contract of using a ring buffer is that you get the data that was there when you began using it. That's not a myth – as long as you have atomic integer operations on the write pointer, you can construct a safe protocol for reading RX ring buffers. \$\endgroup\$ – Marcus Müller Nov 11 at 9:25
  • 3
    \$\begingroup\$ @Lundin that's exactly the point! to protect from race conditions, you need to increment the write pointer last, and in an atomic fashion. You don't have to protect the data access. \$\endgroup\$ – Marcus Müller Nov 11 at 10:35
  • 3
    \$\begingroup\$ (the reader can't read beyond the old write pointer, so it can't access the data the receive task is currently writing. Only after that data has been written the writer increases the write pointer. Since that increase is atomic, there's no chance that the reader might use a "half-increased" write pointer) \$\endgroup\$ – Marcus Müller Nov 11 at 10:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.