Interrupts / Delay

Hi, I have a simple sketch that cycles through some modes using switch case.

It works great other than when I try to add a delay.

I wrote a “myDelay” function that uses the millis() function thinking that would work but my code freezes when it gets to that point.

So far I have only started coding mode0 but this should give you the idea.

Would love any advice.

Cheers.

Phil

const int buttonPin = 2;    // the number of the pushbutton pin
const int numberModes = 4;

const int minFlash = 100;
const int maxFlash = 2000;


int mode = 0;

void setup() {
  pinMode(2, INPUT_PULLUP);
  Serial.begin(9600);  //turn on serial communication
  attachInterrupt(0, modeChange, CHANGE);


}

void loop() {



}


void mode0 () {
  Serial.println("Sequence Mode");

  while (1) { // loop forever
    for (int pin = 3; pin < 11; pin++) {

      int seqSpeed =  map(analogRead(0), 0, 1024, minFlash, maxFlash);

      Serial.println(pin);
      myDelay(2000);


    } // pins



  } // while

}

void mode1 () {
  Serial.println("Mode1");

}



void mode2 () {
  Serial.println("Mode2");

}

void mode3 () {
  Serial.println("Mode3");

}





void modeChange()
{


  static unsigned long last_interrupt_time = 0;
  unsigned long interrupt_time = millis();

  if (interrupt_time - last_interrupt_time > 300) // debounce
  {


    mode++;
    mode = mode % numberModes;
    switch (mode) {
      case 0:
        mode0();
        break;
      case 1:
        mode1();
        break;
      case 2:
        mode2();
        break;
      case 3:
        mode3();
        break;
    }

  }

  last_interrupt_time = interrupt_time;
}




void myDelay(int delayTime) {

  unsigned long currentTime =  millis();

  while ((millis() - currentTime) < delayTime) {


  }


}

You are calling serial print from ISR also delay is there. It is not good idea because of the interrupts are disabled inside of ISR and function can wait for another interrupt => frozen Arduino.
Move the switch/case out of ISR. Inside of ISR just set the mode variable and let to know somehow to the main program - loop() that the mode was modified via some variable or flag. Inside of loop: if(mode modified) then switch/case + clear flag to prevent cycling.
BTW: ISR must be as short as possible. Keep on mind that another interrupts can wait.

You are debouncing for 300 milliseconds, so this is a long running routine. Is there a compelling reason to use an interrupt at all here?

Why are you using an interrupt to read a pin that could be more easily polled in the loop() function ? Why introduce complication where none are required ?

Incidentally the myDelay() function behaves the same way as the delay() function and blocks code operation until the delay is finished.

Have a look at the code in Several Things at a Time which reads a button using polling and does not need interrupts.

It also illustrates how millis() is used to manage timing without blocking.

...R

Thanks for all the advice.

I thought using interrupts would be best for me as I need to be able to switch modes easily and the modes will all be loops looping through pins with a delay of some sort.

I'll read through the advice later and see what I can implement.

Cheers.

Phil

the modes will all be loops looping through pins with a delay of some sort.

Only because you wrote it that way.

Use millis() for timing then each time through loop(), when the program runs the code for the current state have it check whether it is time to do something, such as turn on the next LED. If so, then do it and save the time it happened ready for the next check. If not then go round loop() again reading inputs as you do and reacting to them by perhaps changing the state so that a different section of code is activated.

This way you get responsive code with no need for the complications of interrupts.

Hi, I tried the suggestion of moving the switch / case to the main loop and removing any delays.

The interrupt works but as soon as I get inside the mode0 function my interrupt stops working.

I have pasted my most resent code below.

Cheers.

Phil

const int buttonPin = 2;    // the number of the pushbutton pin
const int numberModes = 4;

const int minFlash = 100;
const int maxFlash = 2000;


int mode = 0;
boolean active = false;


void setup() {
  pinMode(2, INPUT_PULLUP);
  Serial.begin(9600);  //turn on serial communication
  attachInterrupt(0, modeChange, CHANGE);


}

void loop() {

if (active == true){

switch (mode) {
      case 0:
        mode0();
        break;
      case 1:
        mode1();
        break;
      case 2:
        mode2();
        break;
      case 3:
        mode3();
        break;
    }

  }

}



void mode0 () {
  active = false;
  Serial.println("Sequence Mode");

 
    for (int pin = 3; pin < 10001; pin++) {

      int seqSpeed =  map(analogRead(0), 0, 1024, minFlash, maxFlash);

      Serial.println(pin);
    


    } // pins



}

void mode1 () {
  active = false;
  Serial.println("Mode1");

}



void mode2 () {
  active = false;
  Serial.println("Mode2");

}

void mode3 () {
  active = false;
  Serial.println("Mode3");

}





void modeChange()
{


  static unsigned long last_interrupt_time = 0;
  unsigned long interrupt_time = millis();

  if (interrupt_time - last_interrupt_time > 300) // debounce
  {

    active = true;
    mode++;
    mode = mode % numberModes;    

  last_interrupt_time = interrupt_time;
}

}


void myDelay(int delayTime) {

  unsigned long currentTime =  millis();

  while ((millis() - currentTime) < delayTime) {


  }


}

any variable you manipulate in an ISR should be declared volatile.

const int buttonPin = 2;    // the number of the pushbutton pin
const int numberModes = 4;

const int minFlash = 100;
const int maxFlash = 2000;

volatile int mode = 0;
volatile boolean active = false;

void setup() 
{
  pinMode(2, INPUT_PULLUP);
  Serial.begin(9600);  //turn on serial communication
  attachInterrupt(0, modeChange, CHANGE);
}

void loop() 
{
  if (active == true) 
  {
    switch (mode) 
    {
      case 0:
        mode0();
        break;
      case 1:
        mode1();
        break;
      case 2:
        mode2();
        break;
      case 3:
        mode3();
        break;
    }
  }
}



void mode0 () 
{
  active = false;
  Serial.println("Sequence Mode");
  for (int pin = 3; pin < 10001; pin++) 
  {
    int seqSpeed =  map(analogRead(0), 0, 1024, minFlash, maxFlash);
    Serial.println(pin);
  } // pins
}

void mode1 () {
  active = false;
  Serial.println("Mode1");

}



void mode2 () {
  active = false;
  Serial.println("Mode2");

}

void mode3 () {
  active = false;
  Serial.println("Mode3");

}

void modeChange()
{
  static volatile unsigned long last_interrupt_time = 0;
  unsigned long interrupt_time = millis();
  if (interrupt_time - last_interrupt_time > 300) // debounce
  {
    active = true;
    mode++;
    mode = mode % numberModes;
    last_interrupt_time = interrupt_time;
  }
}


//void myDelay(int delayTime) 
//{
//  unsigned long currentTime =  millis();
//  while ((millis() - currentTime) < delayTime) 
//  {
//  }
//}

try to declare your active Boolean as volatile to prevent the compiler from doing an unwanted optimization (reading the value from a register instead of memory and having the value changed underneath by the interrupt but not the register and thus being stuck )

volatile boolean active = false;

Static within the interrupt should be fine as by definition you use it only there but An other issue you have is that when you launch your program, your run loop wait doing nothing until the first interrupt. When that first interrupts kicks in, your interrupt_time - last_interrupt_time > 300 will evaluate most likely to true if you waited more than 300 milliseconds before pressing your button, thus you are not debouncing anything that time and setting your Boolean to true and going to mode 1. At that point you exit the interrupt and the loop kicks in, you enter the loop, get ready to execute mode1 but probably your button bounces and you are interrupted again. This time the debouncing works and after stabilization you get to mode 2 and set again your Boolean to true. Depending on race conditions your program might not have executed in the mode1 the first action and directly sets the Boolean to false.

I would recommend initializing with millis() your last_interrupt_time if it is equal to zero, so that the first time you enter your interrupt, you do wait for the debouncing to work and I would disable the interrupts when setting the bool to true. Then in the main loop execute your mode functions and reactivate the interrupts once done, this way you are sure not to run in race conditions (assuming you don’t want to interrupt your mode functions given the way you have written your code)

Last notes, 300 millisecond is pretty long for debouncing, 15 to 20 ms should be enough.

Also it is not recommended to use millis() in an interrupt as it is not updated - micros() will keep ticking if your interrupt is short enough.

(And indeed the mode should also be volatile)

Hi, I tried the suggestion of moving the switch / case to the main loop and removing any delays.

The interrupt works but as soon as I get inside the mode0 function my interrupt stops working.

Did you miss the point about using millis() for timing polling the mode change switch in the loop() function ?

The idea was to get rid of the troublesome interrupts.

I realized I actually need pin2 for one of my many relays I’m using in this project so I will be better off not using interrupts (as others mentioned in this thread).

I reworked the code and feel like I’m close but cannot figure out without interrupts, how do I quit out of my functions.

Here is my reworked code…

Much appreciated.

const int buttonPin = 0;    // the number of the pushbutton pin
const int numberModes = 4;
int buttonState = 0;

const int minFlash = 100;
const int maxFlash = 2000;


int mode = 0;
boolean modeChanged = false;


void setup() {
  pinMode(buttonPin, INPUT_PULLUP);
  Serial.begin(9600);  //turn on serial communication


}

void loop() {

  buttonState = digitalRead(buttonPin);
  if (buttonState == 0) {

    static unsigned long last_interrupt_time = 0;
    unsigned long interrupt_time = millis();

    if (interrupt_time - last_interrupt_time > 50) // debounce
    {

      modeChanged = true;
      mode++;
      mode = mode % numberModes;
      last_interrupt_time = interrupt_time;

    }



  }
  if (modeChanged) {
    switch (mode) {
      case 0:
        mode0();
        break;
      case 1:
        mode1();
        break;
      case 2:
        mode2();
        break;
      case 3:
        mode3();
        break;
    }

  } // modeChanged


} // loop






void mode0 () {
  modeChanged = false;
  delay(300);
  Serial.println("Sequence Mode");

  while (1) {
    for (int pin = 3; pin < 11; pin++) {

      int seqSpeed =  map(analogRead(0), 0, 1024, minFlash, maxFlash);

      Serial.println(pin);
      myDelay(seqSpeed);
      buttonState = digitalRead(buttonPin);

      if (buttonState == 0) {
        break;
      }

    } // pins



  } //while(1)

}// mode0

void mode1 () {
  modeChanged = false;
  Serial.println("Mode1");

}



void mode2 () {
  modeChanged = false;
  Serial.println("Mode2");

}

void mode3 () {
  modeChanged = false;
  Serial.println("Mode3");

}



void myDelay(int delayTime) {

  unsigned long currentTime =  millis();

  while ((millis() - currentTime) < delayTime) {


  }


}

I suggest a state machine, if you are getting hung up on exiting a function at some arbitrary time.

http://www.gammon.com.au/statemachine

cannot figure out without interrupts, how do I quit out of my functions.

Write them so that all they do is to check whether it is time to do the next thing, do it if necessary then exit.

An example

const byte ledPin = 13;
unsigned long interval = 1000;

void setup()
{
  pinMode(ledPin, OUTPUT);
  digitalWrite(ledPin, HIGH);
}

void loop()
{
  //read the input here and act on it
  aFunction();
}


void aFunction()
{
  static unsigned long startTime = millis();        //save the start time
  if (millis() - startTime >= interval)        //is it time to change the state of the LED ?
  {
    digitalWrite(ledPin, !digitalRead(ledPin));    //if yes then change the state
    startTime = millis();                         //save the time of change
  }
}

Thanks, that seems to make sense.

The issue is that in each of my 3 mode functions I am looping through pins turning them on and off, it's not just a single LED. I don't want to wait until the end of the loop to check if the mode has changed.

Cheers

Phil

The issue is that in each of my 3 mode functions I am looping through pins turning them on and off, it’s not just a single LED.

I understand that, but now you have seen the principle you need to make each function non blocking, perhaps by using arrays like this.

const byte ledPin = 13;
const byte ledPins[] = {10, 11, 12, 13};
const byte numberOfLeds = sizeof(ledPins) / sizeof(ledPins[0]);
unsigned long interval = 1000;

void setup()
{
  for (int led = 0; led < numberOfLeds ; led++)
  {
    pinMode(ledPins[led], OUTPUT);
    digitalWrite(ledPins[led], HIGH);    //turn off the LEDs
  }
}

void loop()
{
  //read the input here and act on it
  aFunction();
}

void aFunction()
{
  static unsigned long startTime = millis();        //save the start time
  static byte ledIndex = 0;                         //start with the first LED

  if (millis() - startTime >= interval)        //is it time to change the state of the LED ?
  {
    digitalWrite(ledPins[ledIndex], HIGH);    //turn off the current LED
    ledIndex++;
    if (ledIndex == numberOfLeds)              //? reached the last LED ?
    {
      ledIndex = 0;                            //start LED sequence again
    }
    digitalWrite(ledPins[ledIndex], LOW);      //turn the next LED on
    startTime = millis();                         //save the time of change
  }
}

You will need to make changes to suit your system, timing etc.

Note that you still have the problem with first press that is not debounced

when you write

static unsigned long last_interrupt_time = 0;

you set the time of the last button press at zero
when you first press the button in your program, you will most likely be passed 50 milliseconds and thus the test

if (interrupt_time - last_interrupt_time > 50) // debounce

will not debounce anything because it will be true and you'll skip right to the next mode

I would suggest to move last_interrupt_time as a global variable (this way you have also access to the variable into your mode functions and use the approach suggested above) - no need to make it static there (may be change the name because you don't use interrupts anymore) and initialise it with zero (well arduino does it for you, but just to be sure :slight_smile: ) and to don't miss the debounce section for the first press do something like this

void loop() {
  unsigned long interrupt_time;

  buttonState = digitalRead(buttonPin);
  
if (buttonState == LOW) {
	interrupt_time = millis();
	if (last_interrupt_time == 0) last_interrupt_time = interrupt_time; // for the first time
	    if (interrupt_time - last_interrupt_time > 50) // debounced
	    {
	      modeChanged = true;
	      mode++;
	      mode = mode % numberModes;
	      last_interrupt_time = interrupt_time;
	    }
	  }
...

UKHeliBob:

cannot figure out without interrupts, how do I quit out of my functions.

Write them so that all they do is to check whether it is time to do the next thing, do it if necessary then exit.

As they do in the demo I linked to in Reply #4

...R
PS .. this is addressed to the OP.

J-M-L:
Note that you still have the problem with first press that is not debounced

Maybe it's me that doesn't understand your point here...

The first button press needs no debouncing, it would be all of the subsequent bounciness that needed to be filtered.