Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

taskENABLE_INTERRUPTS/taskDISABLE_INTERRUPTS causing panic resets on ESP32 #2342

Open
idobh2 opened this issue Mar 7, 2023 · 3 comments
Open
Labels
ESP32 This is only a problem on ESP32-based devices

Comments

@idobh2
Copy link

idobh2 commented Mar 7, 2023

I'm seeing an issue with interrupts on ESP32, causing panic resets.
My js code is pretty simple:

setWatch(() => {console.log("change")}, D21, { repeat: true, });

After a few log lines, I'm seeing the following error dumped
Guru Meditation Error: Core 0 panic'ed (StoreProhibited). Exception was unhandled.
With some cpu register dump and then a reboot.

After some digging, I'm pretty sure the cause to this error are the calls to taskENABLE_INTERRUPTS() and taskDISABLE_INTERRUPTS(); in jshInterruptOn and jshInterruptOff respectively, under the targets/esp32 implementaion.

When I did the following (probably very irresponsible) change locally to the targets/esp32/jshardware.c, I stopped getting these panics, and interrupts worked well

  1. First I defined a new global boolean
static bool intr_enabled = true;
  1. Then made sure the gpio_intr_handler doesn't push a new event if intr_enabled is false. this is done only after the interrupt is read and cleared
static void IRAM_ATTR gpio_intr_handler(void* arg){
  //GPIO intr process. Mainly copied from esp-idf
  UNUSED(arg);
  IOEventFlags exti;
  Pin gpio_num = 0;
  uint32_t gpio_intr_status = READ_PERI_REG(GPIO_STATUS_REG);   //read status to get interrupt status for GPIO0-31
  uint32_t gpio_intr_status_h = READ_PERI_REG(GPIO_STATUS1_REG);//read status1 to get interrupt status for GPIO32-39
  SET_PERI_REG_MASK(GPIO_STATUS_W1TC_REG, gpio_intr_status);    //Clear intr for gpio0-gpio31
  SET_PERI_REG_MASK(GPIO_STATUS1_W1TC_REG, gpio_intr_status_h); //Clear intr for gpio32-39
  if(!intr_enabled){
    return;
  }
  do {
    g_pinState[gpio_num] = 0;
    // ...
  1. Then I modified jshInterruptOff and jshInterruptOn to change intr_enabled instead of calling taskDISABLE_INTERRUPTS/taskENABLE_INTERRUPTS
void jshInterruptOff() { 
  //taskDISABLE_INTERRUPTS();
  intr_enabled = false;
}

void jshInterruptOn()  {
  //taskENABLE_INTERRUPTS();
  intr_enabled = true;
}

This resulted in consistent operation of interrupts and got the setWatch working properly.
That said, I guess my code is really naïve, as i'm not too familiar with the internals of the ESP32 libs and parallelism in this specific case.
I guess there's a more elegant solution here, but if this is something you think should be introduced as presented above, I'd be happy to create a PR,.

@idobh2 idobh2 changed the title taskENABLE_INTERRUPTS/taskENABLE_INTERRUPTS causing panic resets on ESP32 taskENABLE_INTERRUPTS/taskDISABLE_INTERRUPTS causing panic resets on ESP32 Mar 7, 2023
@MaBecker MaBecker added the ESP32 This is only a problem on ESP32-based devices label Mar 15, 2023
@hagai-shatz
Copy link

Seems like using taskENABLE_INTERRUPTS/taskDISABLE_INTERRUPTS is not a valid method on multi-core, see FreeRTOS documentation:

Warning
Disabling interrupts is a valid method of achieve mutual exclusion in Vanilla FreeRTOS (and single core systems in general). However, in an SMP system, disabling interrupts is NOT a valid method ensuring mutual exclusion. Refer to Critical Sections for more details.

Seems like a change is needed to use taskENTER_CRITICAL(&spinlock) / taskEXIT_CRITICAL(&spinlock) (or taskENTER_CRITICAL_ISR(&spinlock) / taskEXIT_CRITICAL_ISR(&spinlock) from an interrupt context).

@MaBecker
Copy link
Contributor

@idobh2 does your changes work for multiple setwatches too?

@MaBecker
Copy link
Contributor

MaBecker commented Jun 14, 2024

look's like this code is helpfull to fix this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ESP32 This is only a problem on ESP32-based devices
Projects
None yet
Development

No branches or pull requests

3 participants