Is it legal to use `volatile uint8_t` instead of `volatile sig_atomic_t`?

Question

The following code uses a volatile uint8_t instead of a volatile sig_atomic_t as the C-standard mandates, because on avr platform the type sig_atomic_t isn't available.

Is this still legal code? Is it legal using __SIG_ATOMIC_TYPE__ ?

Is it neccesary to include cli() / sei() Macros?

#include <stdint.h>
#include <signal.h>
#include <avr/interrupt.h>

volatile uint8_t flag;  
//volatile sig_atomic_t flag; // not available in avr-gcc
//volatile __SIG_ATOMIC_TYPE__ flag; // ok?

void isr() __asm__("__vector_5") __attribute__ ((__signal__, __used__, __externally_visible__)); 
void isr() {
    flag = 1;
}

void func(void) {
  for (uint8_t i=0; i<20; i++) {
      flag = !flag;
  }
}

Answer 1

__SIG_ATOMIC_TYPE__ appears to be noise, it doesn't achieve jack from what I can tell, since it's just a definition boiling down to a plain unsigned char, which you already had in the form of uint8_t. Which is not an atomic type as far as the C language is concerned.

With or without the __SIG_ATOMIC_TYPE__ spam, the code is not interrupt safe. C does not guarantee atomicity for uint8_t/unsigned char and volatile in this context only protects against incorrect optimizations, not against race conditions.

Disassemble to tell (AVR gcc 12.2 -O3):

__zero_reg__ = 1
__vector_5:
        __gcc_isr 1
        ldi r24,lo8(1)
        sts flag,r24
        __gcc_isr 2
        reti
        __gcc_isr 0,r24
func:
        ldi r24,lo8(20)
.L5:
        lds r18,flag
        ldi r25,lo8(1)
        cpse r18,__zero_reg__
        ldi r25,0
        sts flag,r25
        subi r24,lo8(-(-1))
        cpse r24,__zero_reg__
        rjmp .L5
        ret
flag:
        .zero   1
sig_atomic_t:
        .zero   1

This means that the generated code is "thread safe" as far as not doing partial read/writes go. The flag memory location is read/written to in a single instruction. But that's not helpful. Because there is a latency in the func() code from the point where flag is loaded into r18 until flag is updated. During this time, flag may be updated by the ISR and then the updated value will be destroyed by sts flag, r25. This is bad, value updates from the ISR will get arbitrary lost. So yeah this needs some manner of re-entrancy protection.

---

Is it neccesary to include cli() / sei() Macros?

That would toggle the global interrupt mask and therefore is never the correct way to provide re-entrancy. Any code doing such (like some of the Arduino libs) is just sheer quackery and there's no excuse for it. Disabling interrupts is one way to protect against race conditions, but in that case you should disable the specific interrupt and not EVERY interrupt on the MCU! Or you'll screw up every other unrelated interrupt in the whole program. This is an incredibly common beginner bug in embedded systems that utilize maskable, non-interruptable interrupts (like AVR).

If you don't want information loss when the ISR tries to set the flag, then your code should be:

for (uint8_t i=0; i<20; i++) {
  disable_the_specific_interrupt();
  flag = !flag;
  enable_the_specific_interrupt();
}

Given that this doesn't touch/clear any flags that the interrupt triggers from, it will only delay the interrupt from firing until we are done writing to it. Which is exactly what we want, no data loss, just a slight delay of some 5 or so instructions (maybe 15-20 CPU ticks?) when the interrupt fires during execution of this loop.