function of code that interrupt calls is never run

Hello, I have some code that i will post below that runs fine except for the buttonCancel(); function which is set in the interrupt function subroutine is never called. Hopefully it is just a simple fix I have overlooked.

What happens in my program, is a button is pressed, and some music and motors go. What I want to happen is if the button is pressed again, that the motors and music will stop and be ready for the next time the button is pressed.

Please have a look. Suggestions are greatly appreciated:

/*
 *  Oliver McIrwin 
 *  Dancing Plant, debounced
 *  Interaction with radioshack 276-1323 Digital Recording module
 *  And two or three motors across mosfets 
 */
//Adapted from Jeremy Blums lesson 10
//www.jeremyblum.com
//Using hardware Debounced Switch to Control Interrupt
//http://www.jeremyblum.com/2011/03/07/arduino-tutorial-10-interrupts-and-hardware-debouncing/


//Setup the Interrupt Button
int buttonInt = 0; //AKA digital pin 2

int StatusLedPin = 8;
int MusicPin = 9;
int motor1Pin = 3;
int motor2Pin = 4;
int motor3Pin = 5;

int buttonGo = 20;
//int buttonCancel = 21;
int buttonCallCancel = 21;
int buttonSTBY = 22;
int buttonRUN = 23;

//anything modified by interrrupt fcn has to me initialized as a volitile 
volatile int selectedPGM = buttonSTBY;

//Setup the song is finished
boolean isFinished = true;

void setup()
{
  //set pin modes
  pinMode(StatusLedPin, OUTPUT);
  pinMode(MusicPin, OUTPUT);
  pinMode(motor1Pin, OUTPUT);
  pinMode(motor2Pin, OUTPUT);
  pinMode(motor3Pin, OUTPUT);
  //set interrupt (pin, function to run, and when to run interrupt)
  attachInterrupt(buttonInt, swap, FALLING);
}

void buttonCancel()
{
    digitalWrite(motor1Pin, LOW);
    digitalWrite(motor2Pin, LOW);
    digitalWrite(motor3Pin, LOW);
    digitalWrite(MusicPin, HIGH);
    delay(200); 
    digitalWrite(MusicPin, LOW);
    delay (200);
    digitalWrite(StatusLedPin,LOW);
    delay (2000);
    isFinished = true;
    selectedPGM = buttonSTBY;
      digitalWrite(StatusLedPin,HIGH);
      delay (300);
      digitalWrite(StatusLedPin,LOW);
      delay (300);
      digitalWrite(StatusLedPin,HIGH);
      delay (300);
      digitalWrite(StatusLedPin,LOW);
      delay (300);
      digitalWrite(StatusLedPin,HIGH);
      delay (300);
      digitalWrite(StatusLedPin,LOW);
      delay (300);
      
}

//the interrupt function
void swap()
{
  if (selectedPGM == buttonSTBY && isFinished == true)
  {
    selectedPGM = buttonGo;
  }
    
  else if (selectedPGM == buttonRUN && isFinished == false)
  {
    selectedPGM = buttonCallCancel;
    buttonCancel(); 
  }
}

void loop()
{
  if(selectedPGM == buttonGo && isFinished == true)
  {
    selectedPGM = buttonRUN;
    isFinished == false;
      digitalWrite(StatusLedPin, HIGH);
      digitalWrite(MusicPin, HIGH); //latch music relay for 200ms
      delay(200); 
      digitalWrite(MusicPin, LOW);
      delay (400);///offset for motor to match with music
      digitalWrite(motor1Pin, HIGH); //Motor 1 On
      delay(2200);
      digitalWrite(motor2Pin, HIGH);  //Motor 2 On
      delay (75);
      digitalWrite(motor1Pin, LOW); //Stop Motor1
      delay(2300);
      digitalWrite(motor2Pin, LOW); //Stop Motor2
      delay (0);
      digitalWrite(motor1Pin, HIGH); //Motor 1 On Again
      delay(2250);
      digitalWrite(motor2Pin, HIGH); //Motor 2 On (all on)
      digitalWrite(motor3Pin, HIGH); //Motor 3 On (all on)
      delay(2200);
      digitalWrite(motor1Pin, LOW); //Stop Motor1 Again (all off)
      digitalWrite(motor2Pin, LOW); //Stop Motor1 Again (all off)
      digitalWrite(motor3Pin, LOW); //Stop Motor3 (all off)
      delay (75);
      digitalWrite(StatusLedPin,LOW);
      delay (200);
      digitalWrite(StatusLedPin,HIGH);
      delay (200);
      
      digitalWrite(StatusLedPin,LOW);
      delay (200);
      
      
      //
      isFinished = true;
      selectedPGM = buttonSTBY;
  }
  

 /* else if(selectedPGM == buttonCancel && isFinished == false)
  {
    digitalWrite(motor1Pin, LOW);
    //digitalWrite(motor2Pin, LOW);
    digitalWrite(MusicPin, HIGH);
    delay(200); 
    digitalWrite(MusicPin, LOW);
    delay (200);
    digitalWrite(StatusLedPin,LOW);
    isFinished = true;
    selectedPGM = buttonSTBY;
  }  

*/
}

Does your program “stop” and do nothing more?

That is because in your interrupt you are calling a routine that uses delay.

Whilst in your interrupt routine the other other interrupts are blocked, and this millis() does not advance… delay() does not work.

When using interrupts you must keep the interrupt execution SHORT, and certainly not use delay().

The commented out code is better as a principle. Set the button state and test for it in the loop. To avoid the delays blocking the check for the new state,(which is te reason I guess that you decided you neeed interrupts) you must avoid using delay().

Althouh you’d be better of using a proper timer and state logic, to keep things simple in THIS program you replace the delay calls in your loop (when turing things on or off) with something like

 // digitalWrite(xx,HIGH); // whatever needed doing
millisstart = millis() ; note the time now
while ( millis() - millistart < 1000  && selectedPGM != buttonCallCancel ) // whilst the time is not done nor the button pressed again
    /* do nothing */ ;

You (more or less) correctly did this:

//anything modified by interrrupt fcn has to me initialized as a volitile 
volatile int selectedPGM = buttonSTBY;

but then you failed to do the same for other variables that are shared between the ISR and the non-ISR code (isFinished is one such example).

Also, as pointed out elsewhere, using delay(....) in an ISR is a big no-no. delay(...) requires interrupts and interrupts are disabled within an ISR. Ditto for any serial I/O.

Using interrupts feels like an awkward way to solve this problem. The problem you're trying to solve is that you have a long sequence of blocking actions and you want to be able to detect and handle button presses while they are in progress. Using interrupts and flags to control the execution is one way to do it. Your interrupt-based solution would work better if you make all your shared data volatile, and minimise the size of the shared data. As far as I can see, all your shared data could be held in byte variables, which would avoid one source of problems. You also need to refactor it so that you don't need to do any delays inside the interrupt handler.

IMO you end up with a better solution by changing your sketch to use a non-blocking architecture. In this scheme, each of those sequences of timed actions would be implemented by a finite state machine with a separate state for each action. You would end up with a little more code but only a little more, and none of it would be complicated. The advantage of the non-blocking approach is that you can read and debounce you switches and make any changes you like to the state of sketch, so you can easily abort any of these sequences at any point you want. You are also free to handle serial I/O events and so on concurrently if you want - it's just a much more powerful and flexible approach.

Most importantly, it avoids any need for you deal with the complexities and dangers of using interrupts. Sometimes interrupts are needed, but where they aren't needed they're best avoided. They aren't needed here, as far as I can see.

The ISR, swap(), calls buttonCancel(), which calls delay(). You can NOT use delay() in an ISR.

Now, what problem are you trying to solve misusing interrupts?

Thank you all. I will do done testing and see if I can remove the delay in the isr. At this point the code works fine, but any button press during the loop routine is ignored. My problem is that I wanted a second button press that might occur during the loop routine to cancel whatever was happening and wait for another button press to start over.

The suggestion to a non-blocking architecture sounds very interesting. I'm not aware of what makes my code blocking and what changes I could do to make it a non-blocking architecture. I will do done searching, but I'd appreciate if you could make done suggestions our examples of a non-blocking architecture. It sounds like it would be clearer and if I don't have to use interrupts, ask the better.

Is a switch/case scenario an example of a non-blocking architecture? I will Google blocking non-blocking architecture arduino and see what I can lean that way too. Thanks!

I will do done testing and see if I can remove the delay in the isr.

You don't have a choice.

The moment that you call delay(...), you have a blocking architecture. If you want non-blocking, NEVER call delay(...).

delay(...) is not the only function that can cause blocking, but it is perhaps the most common one.

Your program cannot be responsive during a call to delay(,,,) (except perhaps with the use of interrupts, which is another subject entirely).

A non-blocking program is responsive, for some definition of responsive.

switch/case might or might not be non-blocking. It depends upon what each of your cases do.

Thank you for your explanation that delays are blocking types of code.

I had a look at the blink without delay example http://arduino.cc/en/Tutorial/BlinkWithoutDelay and see that they are comparing a current time vs last known time as a non-blocking method. This makes sense to me. I need to research more about millis(), but what happens when it needs to start over? Is there an overflow register that needs to be looked at as well?

Are there other ways of polling a button press besides comparing a last known time to the millis() function?

Thanks again.

I need to research more about millis(), but what happens when it needs to start over?

The same thing that happens when your clock rolls over at midnight. Nothing.

Is there an overflow register that needs to be looked at as well?

No.

Are there other ways of polling a button press besides comparing a last known time to the millis() function?

Polling a pin state has little to do with millis(). Almost nothing, in fact, unless you are debouncing the switch.

In the Blink Without Delay example, If there is a button press as millis() is turning over, we would get a huge negative number, correct? Since the code takes the difference of the current millis() value to last value when pressed and looks to see if it is greater than 1000, that may be ok because we are still less than 1000, correct? Unless negatives are handled differently...

I understand that the millis() function does not poll a button press, but what I was asking was if there were other non-blocking methods of polling different states of button presses that may be different than the Blink Without Delay example provides. Im asking for ideas that could inspire an other way of polling a button press. Thanks.

See:

http://gammon.com.au/millis

AND

http://gammon.com.au/blink

for more information. Nick Gammon has a wealth of easily-read information on this subject and on others.

In the Blink Without Delay example, If there is a button press as millis() is turning over, we would get a huge negative number, correct?

In an unsigned long? How?

redhotdaddy:
If there is a button press as millis() is turning over, …

It’s a valid concern, but in practice there is a programming technique which handles the rollover correctly without needing any special code. As long as you follow this technique, it just works. The critical points are that time values are always held as unsigned values (unsigned long is the correct type) and that times are always compared using subtraction.

The simplest fix is to have a checkForCancel() function that checks for a cancel
button press and handles it and returns a boolean to say if it cancelled.

You then call it in any while loop and replace delays by for loops that check:

  while (<cond>)
  {
     <body>
  }
// is replaced by
  while (<cond> && !checkForCancel())
  {
    <body>
  }

// and
  delay (300) ;
// is replaced by 
  my_delay (300) ;

// where:
boolean my_delay (int n)
{
  for (int i = 0 ; i < n ; i++)
  {
    delay(1) ;
    if (checkForCancel ())
      return true ;
  }
  return false ;
}

So you have to keep testing for a cancel when you call these functions but
its a rough and ready way to provide simple exception handling without having
to totally re-write.