BEGIN_CRITICAL/END_CRITICAL resets the stm8 when used in interrupt

[publee]

When I setup the interrupt:

GPIO_Init(GPIOA,GPIO_PIN_3,GPIO_MODE_IN_FL_IT);
EXTI_SetExtIntSensitivity(EXTI_PORT_GPIOA, EXTI_SENSITIVITY_FALL_ONLY);
attachInterrupt(INT_PORTA, irq_button, 0); 

And use digitalWrite inside interrupt handler

void irq_button(void)
{
    out = out == HIGH ? LOW: HIGH;
    digitalWrite(LED_BUILTIN, out);
}

The STM will reboot. I tried to comment out in Arduino.h

#define BEGIN_CRITICAL      __critical {
#define END_CRITICAL        }

then it works, but I am afraid it might have unwanted side effects... Is it a bug?

[publee]

In servo.c I can see digitalWrite is used inside interrupt handler, so it should be possible!

[tenbaht]

This depends. __critical{} simply guards the code with sim and rim commands and does not preserve the state of the interrupt masking flag. I tried preserving the flag by using

push cc
sim
....
pop cc

in Arduino.h instead, but this does not work. I never investigated the details.

It sounds strange that your simple code causes a crash. digitalWrite (in wiring_digital.c-6.c) ends with END_CRITICAL, so the very last command before the return command is rim. That shouldn't be a problem for your code, even when the button is bouncing. How much stack space is available in your application? (how much of the 1kB RAM is used?). But so far I haven't seen any buttons bouncing more often than a few times and all relatively slow, much slower than any interrupt routine would fill up the stack.

What is connected to the other pins of port A? Maybe there is a floating pin causing many falling edges? Or is there an external oscillator?

[tenbaht]

I just committed an implementation for attachInterrupt() https://github.com/tenbaht/sduino/commit/0ce7e4c381028f6472eb29e8459210cee90b937d It includes a simple example in test/attachInterrupts and works fine for me. Here is a modified version using digitalWrite() inside the ISR which seems to work as well. I often get multiple triggers on button presses, but that is to be expected. I don't see a reset.

#define BUTTON  PA2

// volatile is important, because this variable is modified in IRQ routine
volatile uint8_t flag = 0;
uint8_t out;

/*
 * IRQ routine
 *
 * no debouncing is done here. Expect multiple trigger for every button press.
 */
void on_button_pressed(void)
{
    flag = 1;
    out  = !out;
    digitalWrite(LED_BUILTIN, out);
}

void setup()
{
    Serial_begin(115200);
    Serial_println_s("start");

    pinMode(LED_BUILTIN, OUTPUT);
    digitalWrite(LED_BUILTIN, 1);   // turn off the LED

    pinMode(BUTTON, INPUT_PULLUP);

    attachInterrupt(digitalPinToInterrupt(BUTTON), on_button_pressed, FALLING);
}

void loop()
{
    if (flag) {
        Serial_println_s("trigger");
        flag = 0;
    }
}

[stefaandesmet2003]

I can't explain why publee's code crashed, but I can confirm there is an issue with BEGIN_CRITICAL and END_CRITICAL. This simple code copies the value of pin PD2 onto the BUILTIN_LED, using pin change interrupt (falling & rising edge). With PD2 connected to ground the led is on, with PD2 floating (pulled high), the led should be off. Well try, it's easy to have the led on while PD2 is high. Remove the BEGIN_CRITICAL and END_CRITICAL and problem solved.

// test pin change interrupt on pin PD2

static void portd_irq (void) {

  uint8_t d2 = digitalRead(PD2);
  for (int i=0;i<10;i++) {
    digitalWrite(LED_BUILTIN,d2);
  }
}

void setup() {
  pinMode(LED_BUILTIN,OUTPUT);
  digitalWrite(LED_BUILTIN,HIGH);

  pinMode (PD2, INPUT_PULLUP);
  attachInterrupt(3,portd_irq, 0); // mode is not implemented in sduino release 0.5, but we do the config manually:

  disableInterrupts(); // EXTI->CR1 only writable under disabled interrupts (CCR=3)
  EXTI->CR1 = 0xC0; // set falling+rising interrupt for all pins on port D
  GPIOD->CR2 = 0x04; // enable ext interrupt on pin PD2
  enableInterrupts();

}
void loop() {
}

The reason is that digitalWrite in ISR starts under software priority 3 (uninterruptable), and because of the END_CRITICAL terminates under software priority 0 (main). The rest of the ISR code becomes interruptable (nested interrupt), and that's what happens during the debouncing of PD2. pin PD2 is read as 0, is preempted by the next pin change interrupt where pin PD2 is read as 1, digitalWrite sets LED_BUILTIN = 1 (led off), execution returns to the previous interrupt, digitalWrite sets LED_BUILTIN = 0 (led on). The for-loop is only there to make the bug easily reproducible.

The solution that worked for me is to replace BEGIN_CRITICAL by :

uint8_t cc = ITC_GetSoftIntStatus();
disableInterrupts();

and END_CRITICAL by :

if (cc == 20) // we are not called from within an ISR
  enableInterrupts();

Am not using other software priorities (1 & 2), so haven't tested these. Like you documented in the code 'push cc' and 'pop cc' doesn't seem to work. But this works. Btw, I'm glad to help out on other open issues in this repo, but am only starting to explore stm8.