Interrupts only working sometimes

I am making a box that, after being opened and shut, locks itself and won’t open again for another 6 hours (or so). I’m using an Arduino Pro Mini, and to save power, it spends most of its time in SLEEP_MODE_PWR_DOWN, and I’m using interrupts to wake it.

The circuit consists of a hall-effect magnet sensor and a momentary button, wired to pins 2 and 3 respectively, and set to INPUT_PULLUP mode. The solenoids are connected to pin 12, and are extended (locked) when unpowered.

Here’s how it’s supposed to work:

  1. The program starts out assuming the box is opened, and the solenoids are extended (locked) (Since the box will have to be open to replace the batteries, which is when the arduino will start up)
  2. You hit a button on the outside of the box, which triggers an interrupt and retracts the solenoids.
  3. You close the box, which triggers an interrupt via a hall-effect sensor in the lid and a magnet in the box, and extends the solenoids.
  4. The lid is now closed and locked
  5. You hit the button again, which triggers an interrupt. Either
    a- Not enough time has passed, so nothing happens, except the button interrupt is reattached
    b- Enough time has passed, so the solenoids are retracted and the lid is unlocked
  6. You open the lid, which triggers an interrupt via that aforementioned hall-effect sensor, and extends the solenoids
  7. Return to step 1

Here’s the code:

#include <avr/sleep.h>

#define SOLENOID_TIME 1000 //Time solenoids are to remain activated
#define WAIT_TIME 2 //Time box is to remain locked, in 8-second intervals (Is only this small for testing. Would be more like 2700 in the final thing)

#define MAGNET_PIN 2
#define BUTTON_PIN 3
#define SOLENOID_PIN 12

enum state{
  OPEN,         //Lid is open, solenoids unlocked
  CLOSED,       //Lid is closed, but solenoids are unlocked
  LOCKED_CLOSED,       //Lid is closed and locked
  LOCKED_OPEN,   //Lid is open, solenoids locked
  WAITING_FOR_LID_OPEN,
  WAITING_FOR_LID_CLOSE
};

enum event{
  BUTTON_PRESSED,
  BUTTON_RELEASED,
  LID_OPENED,
  LID_CLOSED,
  TIMER_TRIGGERED,
  NO_EVENT
};

long lastTime; //Time last event happened. Not currently used for anything
long lastLocked; //Time box was last locked
long currentTime; //Increment this every time timer is triggered
volatile event lastEvent;
volatile state currentState;

void setup() {
  lastTime = 0;
  lastLocked = 0;
  currentTime = 0;
  currentState = LOCKED_OPEN;
  lastEvent = LID_OPENED;

  pinMode(MAGNET_PIN, INPUT_PULLUP);
  pinMode(BUTTON_PIN, INPUT_PULLUP);
  pinMode(SOLENOID_PIN, OUTPUT);

  /* Clear the reset flag. */
  MCUSR &= ~(1<<WDRF);
  
  /* In order to change WDE or the prescaler, we need to
   * set WDCE (This will allow updates for 4 clock cycles).
   */
  WDTCSR |= (1<<WDCE) | (1<<WDE);

  /* set new watchdog timeout prescaler value */
  WDTCSR = 1<<WDP0 | 1<<WDP3; /* 8.0 seconds */
  /* Enable the WD interrupt (note no reset). */
  WDTCSR |= _BV(WDIE);
  Serial.begin(9600);
}

void loop() {
  sleep_enable();
  set_sleep_mode(SLEEP_MODE_PWR_DOWN);

  if(lastEvent == BUTTON_PRESSED){
    Serial.println("Button pressed");
    attachInterrupt(digitalPinToInterrupt(BUTTON_PIN), buttonReleased, HIGH);
    detachInterrupt(digitalPinToInterrupt(MAGNET_PIN));
    lastTime = currentTime;
    lastEvent = NO_EVENT;
  }else if(lastEvent == BUTTON_RELEASED){
    Serial.println("Button released");
    if(currentState == LOCKED_OPEN){
      currentState = WAITING_FOR_LID_CLOSE;
      attachInterrupt(digitalPinToInterrupt(MAGNET_PIN), lidClosed, LOW);
      detachInterrupt(digitalPinToInterrupt(BUTTON_PIN));
      digitalWrite(SOLENOID_PIN, HIGH);
    }else if(currentState == LOCKED_CLOSED){
      long timeElapsed = currentTime - lastLocked;
      if(timeElapsed >= WAIT_TIME){
        Serial.println("Opened!");
        digitalWrite(SOLENOID_PIN, HIGH);
        detachInterrupt(digitalPinToInterrupt(BUTTON_PIN));
        Serial.println("Attaching interrupt to magnet pin...");
        attachInterrupt(digitalPinToInterrupt(MAGNET_PIN), lidOpened, HIGH);
        currentState = WAITING_FOR_LID_OPEN;
      }else{
        Serial.println("Not yet");
        detachInterrupt(digitalPinToInterrupt(MAGNET_PIN));
        attachInterrupt(digitalPinToInterrupt(BUTTON_PIN), buttonPressed, LOW);
        //BLINK RED LED MAYBE???
      }
    }
    lastEvent = NO_EVENT;
  }else if(lastEvent == LID_OPENED){
    Serial.println("Lid opened");
    delay(SOLENOID_TIME);
    digitalWrite(SOLENOID_PIN, LOW);
    currentState = LOCKED_OPEN;
    detachInterrupt(digitalPinToInterrupt(MAGNET_PIN));
    attachInterrupt(digitalPinToInterrupt(BUTTON_PIN), buttonPressed, LOW);
    lastEvent = NO_EVENT;
  }else if(lastEvent == LID_CLOSED){
    Serial.println("Lid closed");
    delay(SOLENOID_TIME);
    digitalWrite(SOLENOID_PIN, LOW);
    currentState = LOCKED_CLOSED;
    lastLocked = currentTime;
    detachInterrupt(digitalPinToInterrupt(MAGNET_PIN));
    attachInterrupt(digitalPinToInterrupt(BUTTON_PIN), buttonPressed, LOW);
    lastEvent = NO_EVENT;
  }else if(lastEvent == TIMER_TRIGGERED){
    Serial.println("Timer triggered");
    currentTime++;
    lastEvent = NO_EVENT;
  }

  Serial.println("\n");
  delay(100); //GET RID OF THIS LATER
  cli();
  sleep_bod_disable();
  sei();
  sleep_cpu();
  /* wake up here */
  sleep_disable();
}

void buttonPressed(){
  detachInterrupt(digitalPinToInterrupt(BUTTON_PIN));
  lastEvent = BUTTON_PRESSED;
}

void buttonReleased(){
  detachInterrupt(digitalPinToInterrupt(BUTTON_PIN));
  lastEvent = BUTTON_RELEASED;
}

void lidOpened(){
  detachInterrupt(digitalPinToInterrupt(MAGNET_PIN));
  lastEvent = LID_OPENED;
}

void lidClosed(){
  detachInterrupt(digitalPinToInterrupt(MAGNET_PIN));
  lastEvent = LID_CLOSED;
}

ISR(WDT_vect)
{
  lastEvent = TIMER_TRIGGERED;
}

Here’s the issue: Everything works fine through step 5, so I know that the interrupts are working properly for both the button and the hall-effect sensor. However, in step 6, when the box is opened, the interrupt for the hall-effect sensor is not triggered, so the solenoids do not extend once the box is opened, and the currentState is not changed to LOCKED_OPEN. It seems that all interrupts are working except for a HIGH level interrupt on the hall-effect sensor.
And using debug prints to Serial, I determined that, when pressing the button and unlocking the box, when the solenoids are retracted, the currentState does successfully change to WAITING_FOR_LID_OPEN.

I could fix this by just keeping the solenoids retracted the whole time the lid is opened, but the solenoids use a lot of power in a little bit of time, so I want to avoid this.

What am I doing wrong in the code? Is there any way to fix it without changing the functionality of the device?

It kinda makes my head hurt trying to figure out how that's even supposed to work....

First, don't keep attaching and detaching interrupts. Attach them once, in setup(), and leave them there. Have one handler for each input, and caapture both edges of each interrupt. Have each interrupt handler simply set a variable indicating what event occurred.

You're effectively playing with two sets of states: lastEvent and currentState. That really confuses things. Create a single, global state variable where each value of that state variable directs you to a specific block of code, or better, function. If you create a state diagram for the operation, it should be trivial to map out which events need to be handled within each state, and what transitions are allowed. Within each state function, look at any events that have occurred since the last time you looked, clear them, and take action as needed by setting/clearing outputs, and/or changing to a different state.

Handling it as described above should make for far simpler, more modular, easier-to-follow code that should work on almost the first try. You will easily be able to look at the code for a single state and know whether it's right or not, without having to follow the rather tortured logic in all the nested ifs you have now.

Regards,
Ray L.

OT
You have crated what my boss used to describe as "spaghetti code".
If you are comfortable with it, no problem.

Same as Ray, I find your code hard to follow especially when there are no comments what each "if" suppose to accomplish.

When you feel like it, please answer the following - just for my future reference.

  1. Did you just write the code or made a "flow chart" to follow before you started typing?
  2. Using just your code - how did you determine "step numbers"? ( "..it fails in step 6...")

Actually just using "step numbers" is also not too descriptive - step "initialize" says much more that "step number 1".

Again, matter of personal preference and if it works does not matter anyway.

RayLivingston:
First, don't keep attaching and detaching interrupts. Attach them once, in setup(), and leave them there. Have one handler for each input, and caapture both edges of each interrupt. Have each interrupt handler simply set a variable indicating what event occurred.

Correct me if I'm wrong, but I thought that the only interrupts that can wake an arduino from SLEEP_MODE_PWR_DOWN are level interrupts, which need to be detached each time they're triggered to avoid constantly re-triggering them while the button is pressed (or whatever the case may be).

Vaclav:
2. Using just your code - how did you determine "step numbers"? ( "..it fails in step 6...")

Actually just using "step numbers" is also not too descriptive - step "initialize" says much more that "step number 1".

ItsTimaiFool:
Here's how it's supposed to work:

  1. The program starts out assuming the box is opened, and the solenoids are extended (locked) (Since the box will have to be open to replace the batteries, which is when the arduino will start up)
  2. You hit a button on the outside of the box, which triggers an interrupt and retracts the solenoids.
  3. You close the box, which triggers an interrupt via a hall-effect sensor in the lid and a magnet in the box, and extends the solenoids.
  4. The lid is now closed and locked
  5. You hit the button again, which triggers an interrupt. Either
    a- Not enough time has passed, so nothing happens, except the button interrupt is reattached
    b- Enough time has passed, so the solenoids are retracted and the lid is unlocked
  6. You open the lid, which triggers an interrupt via that aforementioned hall-effect sensor, and extends the solenoids
  7. Return to step 1

That being said, yes, this code is horrid, I wrote it between like midnight and 2 am last night. I'll work on a neater version and post it here. But in the meantime, are there any mistakes that are commonly made with interrupts that I might be making?

You can also wake from sleep with pin change (pcint) interrupts.

DrAzzy:
You can also wake from sleep with pin change (pcint) interrupts.

Thanks! I tested this, and it worked.

I’ve solved the problem. I’m not exactly sure how, I just completely rewrote the code. One thing I discovered is that my arduino doesn’t support HIGH level interrupts: When I try to attach one, it just behaves exactly like a CHANGE interrupt.

Here’s the code, in case anyone is wondering. I’m not sure if it’s any better, but it at least works.

#include <avr/sleep.h>

#define SOLENOID_TIME 1000 //Time solenoids are to remain activated
#define WAIT_TIME 10 //Time box is to remain locked, in seconds

#define MAGNET_PIN 2
#define BUTTON_PIN 3
#define SOLENOID_PIN 12

enum Event {
  LID_CHANGE, //Lid has changed state
  BUTTON_CHANGE, //Button changed state
  TIMER, //Watchdog timer was triggered
  NONE //No event
};

volatile int currentTime; //Time program has been running, in seconds
volatile int timeLastLocked; //Time the lid was last locked

volatile Event event;

void setup() {
  pinMode(MAGNET_PIN, INPUT_PULLUP);
  pinMode(BUTTON_PIN, INPUT_PULLUP);
  pinMode(SOLENOID_PIN, OUTPUT);
  //lockSolenoids();
  
  attachInterrupt(digitalPinToInterrupt(MAGNET_PIN), lidChangeInterrupt, CHANGE);
  attachInterrupt(digitalPinToInterrupt(BUTTON_PIN), buttonPressInterrupt, CHANGE);

  currentTime = 0;
  timeLastLocked = 0;
  event = NONE;

  /*The following code sets up the Watchdog timer. */

  /* Clear the reset flag. */
  MCUSR &= ~(1 << WDRF);

  /* In order to change WDE or the prescaler, we need to
     set WDCE (This will allow updates for 4 clock cycles).
  */
  WDTCSR |= (1 << WDCE) | (1 << WDE);

  /* set new watchdog timeout prescaler value */
  WDTCSR = 1 << WDP0 | 1 << WDP3; /* 8.0 seconds */
  /* Enable the WD interrupt (note no reset). */
  WDTCSR |= _BV(WDIE);

  Serial.begin(9600);
}



void loop() {

  switch (event) {
    case LID_CHANGE:
      Serial.println("Lid change");
      lockSolenoids();
      if (lidIsClosed()) { //Lid has just now been closed. Reset the unlock timer
        timeLastLocked = currentTime;
      }
      break;
    case BUTTON_CHANGE:
      Serial.println("Button change");
      if (!buttonIsPressed()) { //Do nothing when button is pressed; only when released
        buttonPressed();
      }
      break;
    case TIMER:
      Serial.println("Timer");
      currentTime += 8; //Timer is triggered every 8 seconds. Add 8 seconds to current time.
      break;
    case NONE:
      Serial.println("Loop was executed but no event. Wtf?");
      lockSolenoids();
      break;
  }

  delay(100); //To let the Serial.println()'s go through
  event = NONE;

  sleep_enable();
  set_sleep_mode(SLEEP_MODE_PWR_DOWN);
  cli();
  sleep_bod_disable();
  sei();
  sleep_cpu();
  /* wake up here */
  sleep_disable();
}

void attemptToUnlockBox() {
  if ((currentTime - timeLastLocked) >= WAIT_TIME) {
    //It's been long enough. Unlock the box.
    unlockSolenoids();
    Serial.println("Opened!");
    //TODO: Flash a green LED?
  } else {
    //It has not been long enough. Do not unlock the box.
    //TODO: Flash red LED?
    Serial.println("Not yet...");
  }
}

void buttonPressed() {
  Serial.println("Button pressed!");
  if (lidIsClosed() && solenoidsLocked()) {
    //Lid is closed and locked, so try to unlock it
    Serial.println("Attempting to unlock box...");
    attemptToUnlockBox();
  } else if (lidIsClosed() && !solenoidsLocked()) {
    //Successfully unlocked box, but changed mind and hit button again
    //(This probably will never happen, but just in case...)
    Serial.println("Changed mind, locking box");
    lockSolenoids();
  } else if (!lidIsClosed() && solenoidsLocked()) {
    //Lid is open. User is attempting to close lid.
    Serial.println("Close the lid!");
    unlockSolenoids();
  } else if (!lidIsClosed() && !solenoidsLocked()) {
    //User changed their mind on closing the box and hit the button again.
    Serial.println("Changed mind, not closing lid");
    lockSolenoids();
  }
}

void lidChangeInterrupt() {
  event = LID_CHANGE;
}

void buttonPressInterrupt() {
  event = BUTTON_CHANGE;
}

/**
   This interrupt is triggered to run every ~8 seconds by
   the watchdog timer
*/
ISR(WDT_vect)
{
  event = TIMER;
}

bool lidIsClosed() {
  return digitalRead(MAGNET_PIN) == LOW;
}

bool buttonIsPressed() {
  return digitalRead(BUTTON_PIN) == LOW;
}

bool solenoidsLocked() {
  return digitalRead(SOLENOID_PIN) == LOW;
}

void lockSolenoids(){
  Serial.println("Locking solenoids...");
  digitalWrite(SOLENOID_PIN, LOW);
}

void unlockSolenoids(){
  Serial.println("Unlocking solenoids...");
  digitalWrite(SOLENOID_PIN, HIGH);
}

One thing I discovered is that my arduino doesn't support HIGH level interrupts

No surprise there unless it is a Due
See attachInterrupt()

UKHeliBob:
No surprise there unless it is a Due
See attachInterrupt()

Yeah, I guess I phrased that poorly. I didn't so much "discover" it as "re-read that page and realize how little attention I must have paid to it the first time"