takes two cycles in loop to put ATMega328P-PU to sleep (code attached)

Using Nick Gammon’s Waking From Sleep with a Signal code:

#include <avr/sleep.h>

const byte LED = 9;
  
void wake ()
{
  // cancel sleep as a precaution
  sleep_disable();
  // precautionary while we do other stuff
  detachInterrupt (0);
}  // end of wake

void setup () 
  {
  digitalWrite (2, HIGH);  // enable pull-up
  }  // end of setup

void loop () 
{
 
  pinMode (LED, OUTPUT);
  digitalWrite (LED, HIGH);
  delay (1000);
  digitalWrite (LED, LOW);
  delay (1000);
  pinMode (LED, INPUT);
  
  // disable ADC
  ADCSRA = 0;  
  
  set_sleep_mode (SLEEP_MODE_PWR_DOWN);  
  sleep_enable();

  // Do not interrupt before we go to sleep, or the
  // ISR will detach interrupts and we won't wake.
  noInterrupts ();
  
  // will be called when pin D2 goes low  
  attachInterrupt (0, wake, FALLING);
 
  // turn off brown-out enable in software
  // BODS must be set to one and BODSE must be set to zero within four clock cycles
  MCUCR = bit (BODS) | bit (BODSE);
  // The BODS bit is automatically cleared after three clock cycles
  MCUCR = bit (BODS); 
  
  // We are guaranteed that the sleep_cpu call will be done
  // as the processor executes the next instruction after
  // interrupts are turned on.
  interrupts ();  // one cycle
  sleep_cpu ();   // one cycle

  } // end of loop

*modified the LED portion of the code so that it should flash ON/OFF once per time through the loop.

Anyhow, I’m getting two LED flashes, indicating that the loop is being run through TWICE after each press of the button.

I’ve confirmed with a DMM that I am in fact getting into sleep mode after the two flashes. (0.1 uA during sleep)

Everything else about the code appears to be working.
(disable ADC, BOD, Button triggers ISR)

my setup:

stand-alone ATMega328P-PU w/ arduino bootloader running internal 8 MHz oscillator wired on a breadboard.

Any idears?

ok, I figured it out.

Here’s the modified code that works for me:

#include <avr/sleep.h>

const byte LED = 9;
  
void wake ()
{
// *** moved "sleep_disable()" and 
// *** moved "detachInterrupt(0)" from here ***
}  // end of wake

void setup () 
  {
  digitalWrite (2, HIGH);  // enable pull-up
  }  // end of setup

void loop () 
{
 
  pinMode (LED, OUTPUT);
  digitalWrite (LED, HIGH);
  delay (1000);
  digitalWrite (LED, LOW);
  delay (1000);
  pinMode (LED, INPUT);
  
  // disable ADC
  ADCSRA = 0;  
  
  set_sleep_mode (SLEEP_MODE_PWR_DOWN);  
  sleep_enable();
  
  // will be called when pin D2 goes low  
  attachInterrupt (0, wake, FALLING);

  // Do not interrupt before we go to sleep, or the
  // ISR will detach interrupts and we won't wake.
  noInterrupts ();


// *** moved "attachInterrupt(0, wake, FALLING)" from here ***

  
  // turn off brown-out enable in software
  // BODS must be set to one and BODSE must be set to zero within four clock cycles
  MCUCR = bit (BODS) | bit (BODSE);
  // The BODS bit is automatically cleared after three clock cycles
  MCUCR = bit (BODS); 
  
  // We are guaranteed that the sleep_cpu call will be done
  // as the processor executes the next instruction after
  // interrupts are turned on.
  interrupts ();  // one cycle
  sleep_cpu ();   // one cycle

  // cancel sleep as a precaution
  sleep_disable();
  // precautionary while we do other stuff
  detachInterrupt (0);


  } // end of loop

You can see in the code above: *** what I needed to alter ***.

I figured this out by flashing a modified version of the arduino playground sleep demo code,
which worked. Here’s that code:

#include <avr/sleep.h>

int wakePin = 2;

void wakeUpNow()
{

}

void setup()
{
  pinMode(wakePin, INPUT);
  // turn on internal 20K pull-up:
  digitalWrite(wakePin, HIGH);
  pinMode(9, OUTPUT);

  attachInterrupt(0, wakeUpNow, LOW);
}

void sleepNow()
{
    set_sleep_mode(SLEEP_MODE_PWR_DOWN);
    sleep_enable();         
    attachInterrupt(0,wakeUpNow, LOW); 
    sleep_mode();            
    sleep_disable();
    detachInterrupt(0);      
}

void loop()
{
  digitalWrite(9, HIGH);
  delay(1000);
  digitalWrite(9,LOW);
  delay(1000);
  
  sleepNow();     // sleep function called here
}

Then a did a compare and contrast w/ Gammon’s code.
Altered one thing at a time until I figured out both modifications that were necessary to get Gammon’s code working for me.

OK, thanks very much for posting this. :slight_smile:

As I write these things subtle improvements keep popping up.

The real problem is that there is a pending interrupt. You need to clear that.

Without changing anything else, add one line:

  // Do not interrupt before we go to sleep, or the
  // ISR will detach interrupts and we won't wake.
  noInterrupts ();
 
  // will be called when pin D2 goes low 
  attachInterrupt (0, wake, FALLING);
  EIFR = bit (INTF0);  // clear flag for interrupt 0    <---------------- ADD THIS LINE
 
  // turn off brown-out enable in software
  // BODS must be set to one and BODSE must be set to zero within four clock cycles
  MCUCR = bit (BODS) | bit (BODSE);
  // The BODS bit is automatically cleared after three clock cycles
  MCUCR = bit (BODS);
 
  // We are guaranteed that the sleep_cpu call will be done
  // as the processor executes the next instruction after
  // interrupts are turned on.
  interrupts ();  // one cycle
  sleep_cpu ();   // one cycle

That clears (while interrupts are inactive) any pending interrupt. Now when we go to sleep it is the actual interrupt (and not the pending one) that is taken.

What has almost certainly happened here is that you had switch bounce. So you get one falling interrupt, and a second one a millisecond later. The initial one wakes the processor. The second one is now queued up. When you go to sleep (without clearing it) it wakes the processor again. Now, there are no pending interrupts and it sleeps properly.

I’ll amend my interrupts page.

Thanks Nick,
That does indeed solve the problem.

I didn’t realize that interrupts could be Q’ed.
EIFR = External Interrupt Flag Register
INTF0 = external "INT"erupt Flag 0.
INTF1 = external "INT"erupt Flag 1.

So I imagine the moment an interrupt occurs, INTF0 becomes a “1”… and because of button bounce, I’m calling a second interrupt before the first can completely execute… therefore INTF1 also becomes a “1”.

I guess that makes sense then that I would need to clear INTF1.

from the data-sheet:
“the flag can be cleared by writing a logical one to it” ← that seems strange to me, considering a “1” means not-clear, and a “0” means clear.

EDIT 1: I just tested… and indeed EFIR = 1; works.

I was un-familiar with “bit()”, but according to arduino info, it returns the value of the bit in question. So it seems instead of just setting:
EFIR = 0;
you are telling EFIR to be equal to the INTF0 bit-value (which should be “0”) since the first Q’d ISR already happened.

EDIT 2: Ok, now I’m really confused… shouldn’t INTF0 = “0” since the first ISR already happened, in which case I don’t see how setting EFIR to INTF0’s value is helping.

EDIT 3: (added question)

  • Can you explain my confusing w/ 0’s and 1’s above?
  • Any reason why you would want do that instead of just coding EFIR = 0; ?
  • Also: Is there a fool-proof way to write the code such that it’s not possible to get Q’d ISR’s in the first place? (like in my solution above… but not sure if my solution is “fool proof”)

Thanks Nick.

So I imagine the moment an interrupt occurs, INTF0 becomes a “1”… and because of button bounce, I’m calling a second interrupt before the first can completely execute… therefore INTF1 also becomes a “1”.

No, that is for the second external interrupt. Pin D3 on the Uno.

The interrupt is called and then a second one occurs so INTF0 becomes 1 again.

“the flag can be cleared by writing a logical one to it” ← that seems strange to me, considering a “1” means not-clear, and a “0” means clear.

Still, that is how the hardware works. Writing a 0 is “no operation” and writing a 1 clears the interrupt.

So it seems instead of just setting:
EFIR = 0;
you are telling EFIR to be equal to the INTF0 bit-value (which should be “0”) since the first Q’d ISR already happened.

Writing 1 to EIFR clears that bit. Writing zero to it does nothing.

I was un-familiar with “bit()” …

Equivalent to:

EIFR = (1 << 0);

That is:

EIFR = 1;
  • Any reason why you would want do that instead of just coding EFIR = 0; ?

Because writing zero does nothing. Writing 1 does something.

Is there a fool-proof way to write the code such that it’s not possible to get Q’d ISR’s in the first place?

Yes, you do what I posted. You clear the (pending) interrupt if you don’t want a previously-occurred event to trigger the interrupt.