0

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;
  }
}
Lundin
  • 195,001
  • 40
  • 254
  • 396
wimalopaan
  • 4,838
  • 1
  • 21
  • 39
  • 2
    This isn't really a "language lawyer" question. `__SIG_ATOMIC_TYPE__` is some misguided compiler extension and that's about it. Just as the ISR syntax isn't standard C either but compiler extensions. – Lundin Feb 07 '23 at 11:04
  • @Lundin And also __attribute__(()) is GCC and CLANG specific. The other embedded compilers usually just use #pragmas (which are compiler specific too though, but #pragma or _Pragma() are at least mentioned in the standard :D ). – kesselhaus Feb 07 '23 at 22:24
  • Does anything in the standards indicate that `sig_atomic_t` has any special powers at all with implementation-specific things like ISRs, as opposed to signals in particular? – Joseph Sible-Reinstate Monica Feb 08 '23 at 05:08

1 Answers1

5

__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.

Lundin
  • 195,001
  • 40
  • 254
  • 396
  • @emacsdrivesmenuts I've troubleshooted many very hard to find bugs caused by sloppily toggling the global interrupt mask needlessly. Most obviously it screws up all timing-related interrupts, it screws up PWM generation, it screws up ADC readings, it screws up UART reception = overrun errors and so on and so on. Less obviously, you stall all interrupts indeed, so that they will execute in sequence once `i` is released. Which repeatedly enforces a worst-case scenario with all interrupts triggering at once. – Lundin Feb 08 '23 at 08:52
  • And yet another common bug related to this is that toggling cli/sei is distruptive, because if done proper you must restore the `i` bit to the state it was before, meaning you'll have to save it down first. Otherwise you end up with some random crappily written driver might silently enable/disable interrupts internally. – Lundin Feb 08 '23 at 08:54
  • 1
    Summary: _Just don't do this_ when the solution is so obvious and easy: just disable the relevant interrupt only. Don't be a sloppy Arduino code monkey, do things proper. – Lundin Feb 08 '23 at 08:54