rocketscream / Low-Power

Low Power Library for Arduino
www.rocketscream.com
1.26k stars 345 forks source link

No sleep_forever on Atmega328p #84

Closed mrguen closed 4 years ago

mrguen commented 5 years ago

I am first calling lowPower(SLEEP_1S) and then lowPower(SLEEP_FOREVER). WDT stays enabled after the call of sleep_1S, when entering sleep_forever, and so the MCU wakes up after one second.

I tried to fix it by adding wdt_disable() before calling LowPower.powerDown(SLEEP_FOREVER, ADC_OFF, BOD_ON); and indeed it does sleep forever now which was the objective.

I can see that wdt_disable() is called only in the ISR (WDT_vect). So apparently it would need to be initialized in any case otherwise it behaves depending on the last configuration.

rocketscream commented 5 years ago

Hi mrguen, This is odd because prior to your report, I just tested 10 boards of ATMega328P, they worked accordingly. May I know what library revision and avr-gcc revision you are using?

mrguen commented 5 years ago

I use my fork https://github.com/mrguen/Low-Power based on your version 1.8 Arduino IDE 1.8.9

If I was not explicit enough: SLEEP _FOREVER works when called first. It does not work when called after calling SLEEP_xxS because SLEEP_xxS activates wdt and apparently it is never deactivated.

rocketscream commented 5 years ago

I just tried my own branch and it works just fine here:

// **** INCLUDES *****
#include "LowPower.h"

void setup()
{
   pinMode(13, OUTPUT);
   digitalWrite(13, HIGH);
   LowPower.powerDown(SLEEP_1S, ADC_OFF, BOD_OFF);  
   digitalWrite(13, LOW);
}

void loop() 
{
    // Enter power down state for 8 s with ADC and BOD module disabled
    LowPower.powerDown(SLEEP_FOREVER, ADC_OFF, BOD_OFF);  

    // MCU should not wake up here
}

With the above code, you should see the LED turn on for 1 s and then off.

If you look at the code in the WDT ISR, the watchdog is disabled:

ISR (WDT_vect)
{
    // WDIE & WDIF is cleared in hardware upon entering this ISR
    wdt_disable();
}
mrguen commented 5 years ago

Maybe I don't get it but of course the led remains low since it is positionned to low... Maybe you should add a digitalWrite(13, HIGH ); after callling SLEEP_FOREVER....

manosv commented 5 years ago

of course it stays low because in the loop function he has to put digitalWrite(13, HIGH ) after LowPower.powerDown(SLEEP_FOREVER, ADC_OFF, BOD_OFF);

rocketscream commented 5 years ago

Sorry guys, forgot to mention that I also used a meter for the setup (to double check current measurement) so even with the digitalWrite(13, HIGH); after LowPower.powerDown(SLEEP_FOREVER, ADC_OFF, BOD_OFF); it doesn't wake up. Measurement is also spot on. What AVR board package version you are using on the Arduino IDE? Mine is the latest of 1.6.23.

mrguen commented 5 years ago

I have seen a couple of posts complaining about wdt staying enabled like: https://github.com/mysensors/MySensors/issues/577#issuecomment-245063018

As well as https://www.nongnu.org/avr-libc/user-manual/group__avr__watchdog.html saying " clearing the watchdog reset flag before disabling the watchdog is required, according to the datasheet"

Apparently the code should be

MCUSR = 0; wdt_disable( );

From my code I can see that using wdt_disable( ); does the job when called much later on, but not (alone) within the ISR.

But I really don't get the details.

mrguen commented 5 years ago

I tried with MCUSR = 0; but wdt is still activated.

At the moment the code that works is

if (deviceOFF) {
  INA226_USB.configure(INA226_AVERAGES_16, INA226_BUS_CONV_TIME_1100US, 
  INA226_SHUNT_CONV_TIME_1100US, INA226_MODE_POWER_DOWN);
  INA226_VIN.configure(INA226_AVERAGES_16, INA226_BUS_CONV_TIME_1100US, 
      INA226_SHUNT_CONV_TIME_1100US, INA226_MODE_POWER_DOWN);
  oled.ssd1306WriteCmd(SSD1306_DISPLAYOFF);

  wdt_disable(); 
  LowPower.powerDown(SLEEP_FOREVER, ADC_OFF, BOD_ON);

      oled.ssd1306WriteCmd(SSD1306_DISPLAYON);
  deviceOFF = false;
}
else {
  LowPower.powerDown((period_t)period, ADC_OFF, BOD_ON);
}

Configuration:

rocketscream commented 5 years ago

I would advise to use a more standard setup: bootloader and no other libraries. That way we can eliminate other possible factors.

I have check the assembly listing of the generated code I posted earlier and it is very clear that the wdt_disable() is called within the ISR_WDT.

mrguen commented 5 years ago

I can't use a bootloader since this board does not have USB implemented and the libraries are needed. For the moment I will keep wdt_disable(); before calling powerDown.