Is sleepmgr_sleep() wrong implemented in ASF?

Go To Last Post
3 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hello Forum,

 

I assume the implementation of sleepmgr_sleep() of ASF is wrong. The problem is that before __WFI() the mcu's interrupts are enabled. This allows a race condition in case an interrupts occurs right before _WFI is reached. The interrupts should be disabled instead, the __WFI() will still work as intended.

Have a look at the Linux kernel block, where the problem is explained in more detail.

http://comments.gmane.org/gmane....

        > In default_idle() we disable interrupts and then go idle.  Where do
        > those interrupts get enabled again so that we wake up when interrupts
        > come in?

        We don't - most non-brain damaged CPUs will wake up with the interrupt masked at the CPU.  There's a good reason why this is so.

        If you're intending to idle until the next interrupt, you have to do some preparation.  During that preparation, an interrupt may become
        active.  Such an interrupt may be a wake up event that you're looking for.

        No matter how good your code is, if you don't disable interrupts, you will always have a race between preparing to go to sleep and actually
        going to sleep, which results in lost wake up events.

        This is why all ARM CPUs I'm aware of will wake up even if they are masked at the core CPU (CPSR I bit.)
        Anything else and you should forget using idle mode.

 

static inline void system_sleep(void)

{

  __DSB();

  __WFI();

}

static inline void sleepmgr_sleep(const enum sleepmgr_mode sleep_mode)

{

  Assert(sleep_mode != SLEEPMGR_ACTIVE);

#ifdef CONFIG_SLEEPMGR_ENABLE

  cpu_irq_disable();

 

  /* Enter the sleep mode. */

  system_set_sleepmode((enum system_sleepmode)(sleep_mode - 1));

  cpu_irq_enable();  // <<<<< race condition starts here!

  system_sleep();

#else

  UNUSED(sleep_mode);

  cpu_irq_enable();

#endif /* CONFIG_SLEEPMGR_ENABLE */

}

 

IMHO correct would be:

 

static inline void sleepmgr_sleep(const enum sleepmgr_mode sleep_mode)

{

  Assert(sleep_mode != SLEEPMGR_ACTIVE);

#ifdef CONFIG_SLEEPMGR_ENABLE

  cpu_irq_disable();

 

  /* Enter the sleep mode. */

  system_set_sleepmode((enum system_sleepmode)(sleep_mode - 1));

  system_sleep();

  cpu_irq_enable();

#else

  UNUSED(sleep_mode);

  cpu_irq_enable();

#endif /* CONFIG_SLEEPMGR_ENABLE */

}

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hello,

 

After you call:

system_sleep();

the MCU is "gone". You suggest to enable interrupts after the MCU was awoken (by the interrupts that are potentially disabled?). So, first interrupts should be enabled and the then - sleep activated.

 

I think the idea behind __WFI() instruction is that it can not be interrupted (atomic instruction it is called, if I am not mistaken).

 

I hope I understand it right.

 

Regards

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Absolutely not, the CPU is not gone, the WFI instruction will wake up the CPU even if interrupts are masked. The contrary makes it impossible to write a race free support for idle mode.

 

Anyway, how bad not disabling interrupts before sleeping affects you depends on your application behavior.

 

If you really have nothing to do before sleeping, like in the sleepmgr_sleep() function above, disabling interrupt before sleeping is not necessary. But only very basic systems have nothing to do before sleeping.

 

If you need to check if the system meet all conditions to enter sleep mode, as simple as checking if a user button was not pressed, how it affects you depend on how long you are going to sleep:

 

If you are waking up every 1 ms, e.g. to service a tick handler, the worse that can happen is a 1 ms delay, this is probably acceptable. But might become critical if you need a latency less than the periodic interrupt somewhere in your system.

 

If you are waking up every 60s, e.g. on a low power product running on an alkaline battery reading a sensor value every minute and a user button for user interface, missing the button event before sleeping is a critical issue.

 

A correctly written sleep routine should look like the sample code below, for a simple example of a user button press. It is written to be easy to understand first, not performance wise.

 

static volatile bool button_pressed;

 

static void button_press_irq_handler(void) {

  button_pressed = true;

}

 

int main(void) {

  ...

  while (1) {

    if (button_pressed) {

      button_pressed = false;

      process_button_press();

    }

    cpu_irq_disable();

    /* ISR are disabled, button_pressed variable won't change */

    if (!button_pressed) {

      /* Will wake up the CPU if an interrupt need to be serviced, will even not enter sleep mode at all if an interrupt is already pending to be serviced */

      system_sleep();

    }

    cpu_irq_enable();

  }

}

 

Sylvain

 

Last Edited: Wed. May 27, 2020 - 08:30 PM