Skip to content

On McuSleep

Jump to bottom
jmattsson edited this page Dec 11, 2011 · 1 revision

Musings on McuSleep and the implementation thereof.

  1. All of the current implementations of McuSleep follow the same pattern: enableInterrupts(); sleep(); disableInterrupts(); This is even outlined in TEP131 example. While it is indeed highly important that interrupts are enabled before sleep mode is entered, it seems wrong to unconditionally disable interrupts when exiting from sleep. While this works fine for the scheduler, which very explicitly (mentioned in TEP131) calls McuSleep.sleep() from within an atomic section, it would have very bad consequences if anyone else ever calls it outside an atomic. Doing so would end up with interrupts disabled always, except while the MCU sleeps! It is only by virtue of the exit-from-atomic that interrupts get re-enabled. Everything in the architecture relies on the proper restoration of SREG, so having a component which does not follow that idiom is a bit of a ticking bomb in my opinion.

    As far as I can see, there is no reason why McuSleep.sleep() should not simply restore the interrupt(able) state on exit like everything else. The scheduler would not be affected, and it would prevent Bad Things from happening should anyone call McuSleep.sleep() from outside an atomic. At the very least the interface description should carry a big fat warning "DO NOT CALL EXCEPT FROM INSIDE AN ATOMIC SECTION", but why warn when you can fix it properly instead?

  2. It might save someone else a headache of two if TEP131 was updated to explicitly mention that McuSleep.sleep() MUST have an _asm volatile("" ::: "memory");_ to stop the compiler from optimising away SchedulerBasicP's m_head variable loads. This bit me (the AVR-libc's sleep_mode() call didn't provide this!), and the side effects are really perplexing until you chase it down. All initial tasks are happily run, but once the scheduler enters the main task loop, only async events get processed. I might suggest marking the scheduler queue variables as volatile, but it would be safer to make it the responsibility of McuSleep.sleep() to enforce it.
Clone this wiki locally