espruino / Espruino

The Espruino JavaScript interpreter - Official Repo
http://www.espruino.com/
Other
2.78k stars 746 forks source link

Low Power interrupt - WARNING: No free EXTI for watch #1995

Closed jeffmer closed 3 years ago

jeffmer commented 3 years ago

On building the latest version of Espruino for an SMAQ3, I got the no free EXTI warning which comes from the jshPinWatch routine in jshardware.c when no channels are free.

The reason is that for the SMAQ3, jswrap_banglejs_kill() does deallocate all interrupt channels so after a few reset's, all of the 8 channels are used up. I added:

  jshPinWatch(BTN1_PININDEX, false);
  jshSetPinShouldStayWatched(BTN1_PININDEX,false);
  jshPinWatch(TOUCH_PIN_IRQ, false);
  jshSetPinShouldStayWatched(TOUCH_PIN_IRQ,false);

and that got rid of the warning, however the touch interrupt stopped working after the first reset. I fixed this by properly uninitialising/deallocating the interrupt channel in jshPinWatch as in:

  } else {
    for (int i=0;i<EXTI_COUNT;i++)
      if (extiToPin[i] == p)
        extiToPin[i] = PIN_UNDEFINED;
    nrf_drv_gpiote_in_event_disable(p);
    nrf_drv_gpiote_in_uninit(p);  // make sure channel is deallocated
    return EV_NONE;
  }

I have not submitted a pull request as I have not seen this bug on a NRF52832 - it all worked OK on the P8 so I am not sure if this last changed should be ifdeffed or not.

Low power interrupts are definitely worth having as if you use DMA with the previous interrupts using the fast clock you get a sleep overhead of 450uA which can only be avoided by an ugly hack. My P8 build uses DMA for the display.

jeffmer commented 3 years ago

Actually, the above code is unsafe as it can cause an interrupt channel to be disabled by apin that has already been unwatched. I found this caused some odd watchdog timeouts aand that this version fixed it:

  } else {
    for (int i=0;i<EXTI_COUNT;i++)
      if (extiToPin[i] == p) {
        extiToPin[i] = PIN_UNDEFINED;
        nrf_drv_gpiote_in_event_disable(p);
        nrf_drv_gpiote_in_uninit(p);
      }
    return EV_NONE;
  }
jeffmer commented 3 years ago

I built the latest Espruino for the Bangle and sadly ended up with another bug. The system no longer generates an event for BTN5. I did not get the EXTI warning, however, if I swap the order that BTN4 and BNT5 are allocated channels, BTN5 works but BTN4 does not. My current hypothesis is that even though the EXTI count of 8 has not been exceeded all of the 8 gpiote channels must be in use.

UPDATE - It looks like that is what is happening as I get the warning generated from the following which I added to jshPinWatch.

    if (nrf_drv_gpiote_in_init(p, &cls_1_config, jsvPinWatchHandler)!=0)
      jsWarn("No free GPIOTE for watch");

So it looks like only four channels are free rather than 8?

FIX: On further investigation, it turns out that there are only 4 slots for low accuracy interrupts as specified by the macro GPIOTE_CONFIG_NUM_OF_LOW_POWER_EVENTS in sdk_config.h. when I set this to 8, everything works ok.

For reference, my final working version:

IOEventFlags jshPinWatch(Pin pin, bool shouldWatch) {
  if (!jshIsPinValid(pin)) return EV_NONE;
#if JSH_PORTV_COUNT>0
  // handle virtual ports (eg. pins on an IO Expander)
  if ((pinInfo[pin].port & JSH_PORT_MASK)==JSH_PORTV)
    return EV_NONE;
#endif
  uint32_t p = (uint32_t)pinInfo[pin].pin;
  if (shouldWatch) {
    // allocate an 'EXTI'
    for (int i=0;i<EXTI_COUNT;i++) {
      if (extiToPin[i] == p) return EV_EXTI0+i; //already allocated
      if (extiToPin[i] == PIN_UNDEFINED) {
        // use low accuracy for GPIOTE as we can shut down the high speed oscillator then
       nrf_drv_gpiote_in_config_t cls_1_config = GPIOTE_CONFIG_IN_SENSE_TOGGLE(false /* hi/low accuracy */);
       cls_1_config.is_watcher = true; // stop this resetting the input state
       if (nrf_drv_gpiote_in_init(p, &cls_1_config, jsvPinWatchHandler)!=0)
          jsWarn("No free GPIOTE for watch");
        nrf_drv_gpiote_in_event_enable(p, true);
        extiToPin[i] = p;
        return EV_EXTI0+i;
      }
    }
    jsWarn("No free EXTI for watch");
    return EV_NONE;
  } else {
    for (int i=0;i<EXTI_COUNT;i++)
      if (extiToPin[i] == p) {
        extiToPin[i] = PIN_UNDEFINED;
        nrf_drv_gpiote_in_event_disable(p);
        nrf_drv_gpiote_in_uninit(p);
      }
    return EV_NONE;
  }
} // start watching pin - return the EXTI associated with it
jeffmer commented 3 years ago

Another issue/ feature of low power interrupts is that the following will respond to only one interrupt:

setWatch(()=>{console.log(D8.read()),D8,{edge:"rising",repeat:true});

unless you have previously set the pin mode explicitly to manual:-pinMode(D8,"input",false).

I think this is because if the pin is in automatic mode, the pin read after the interrupt sets the state of the pin which seems to invalidate its low power watched state.

Given this last point, low power interrupts now seem to work reliably on the Bangle, SMAQ3 and P8 systems that I tested:-) Battery life is much better on the P8 and I suspect also on the SMAQ3.

Apologies, just remembered, Bangle.off() does not seem to work with the low power interrupt mods - it simply causes the Bangle to restart. Bangel.softOff() works fine.

gfwilliams commented 3 years ago

Wow, thanks for all your working looking into this!

I'll try and get that all pulled in - and thanks for the note about Bangle.off as well - it shouldn't be too hard to just use the Nordic lib calls directly with non-low-power IRQs

gfwilliams commented 3 years ago

Another issue/ feature of low power interrupts is that the following will respond to only one interrupt:

I just tried that here and I see it for general purpose IO, but not for buttons (I guess Espruino avoids setting state on those). Did you ever figure out why this happens? If not I'll look into it.

And I see what you mean about Bangle.off - will look at that too

edit: I pushed some changes, but leaving open as there's still the Bangle.off and pinMode issue

jeffmer commented 3 years ago

Yes, it does not happen for buttons as these are set up in jswrap_bangle.c and the mode is fixed I guess.

I think the problem is in void jshPinSetState(Pin pin, JshPinState state) in jshHardware.c which I think disables SENSE here:


case JSHPINSTATE_GPIO_IN :
    case JSHPINSTATE_USART_IN :
      reg->PIN_CNF[ipin] = (GPIO_PIN_CNF_SENSE_Disabled << GPIO_PIN_CNF_SENSE_Pos)
                              | (GPIO_PIN_CNF_DRIVE_S0S1 << GPIO_PIN_CNF_DRIVE_Pos)
                              | (GPIO_PIN_CNF_PULL_Disabled << GPIO_PIN_CNF_PULL_Pos)
                              | (GPIO_PIN_CNF_INPUT_Connect << GPIO_PIN_CNF_INPUT_Pos)
                              | (GPIO_PIN_CNF_DIR_Input << GPIO_PIN_CNF_DIR_Pos);
      break;

Low accuracy interrupts use SENSE and I understand the high accuracy interrupts use a different mechanism which does not involve polling the set of pins for which sense is set. A possible fix would be to not reset the pin state if it is already in the required state or simply document it as a feature:-)

gfwilliams commented 3 years ago

Thanks! Just sorted - so now, if we're configuring as an input we keep the sense state. It's only output/adc/undefined where we clear the sense state. So I reckon that should fix it pretty well.

Just .off now...