interrupt buttons, code stops running sometimes

i plan to run LED animations from my arduino.
basically i have 2 buttons, one should switch the main mode i am in, the other should change sub-modes like color or speed.
when using buttons like i had in my (last post) this caused problems since it only worked when i pressed them at the right moment. i figure this is because some of the animation are quite long. so now i switched to interrupts

(please tell me if this is wong!)

the problem now is, that this code runs for a few cycle where i can change modes just fine. but at some point it will just get stuck. there is no serial output anymore and i need to restart the whole thing.
i saw this happening after just ~20 cycles, other times it runs fine for minutes

another thing i see sometimes is that when pressing the 2nd button instead of going from mode 1-1 to 1-2 it seems to rest and go back to 1-0.
i tried to make sure that i am not pressing the button too long - this does not seem to be the case

am i doing this completely wrong, or am i at least on the right track?

#include "FastLED.h"
#define NUM_LEDS 25 
CRGB leds[NUM_LEDS];
#define LedPin 5 

#define BUTTON 2
#define BUTTON2 3


// Variables will change:
int buttonPushCounter = 0;   // counters for the number of button presses
int buttonPushCounter2 = 0;



void setup() {


  // initialize serial communication:
  Serial.begin(9600);
  
  //setup LEDs
  FastLED.addLeds<WS2812, LedPin, GRB>(leds, NUM_LEDS).setCorrection( TypicalLEDStrip );

  //attach ínterrupts
  attachInterrupt (digitalPinToInterrupt (BUTTON), changeEffect, CHANGE); // pressed
  attachInterrupt (digitalPinToInterrupt (BUTTON2), changeEffect2, CHANGE); // pressed


}


void loop() {



  switch (buttonPushCounter)
  {
    case 0:
      Serial.println(F("Case 0"));
      //flickeringRainbow()
      //flickeringRainbowSolid()
      case0();
      break;

    case 1:
      Serial.println(F("Case 1"));
      case1();
      break;

    case 2:
      Serial.println(F("Case 2"));
      break;

    case 3:
      Serial.println(F("Case 3"));
      break;

  }


  if (buttonPushCounter == 4)
    buttonPushCounter = 0;

}

// ########## Case 0 ##############

void case0()
{
    //Serial.println(F("buttonPushCounter2= "));Serial.println(buttonPushCounter2);   // debug pushCounter2
  switch (buttonPushCounter2)
  {
    case 0:
      Serial.println(F("Case 0-0"));
      //flickeringRainbow();
      break;

    case 1:
      Serial.println(F("Case 0-1"));
      //flickeringRainbowSolid();
      break;
  }


  if (buttonPushCounter2 == 2)
  {
    buttonPushCounter2 = 0;
  }

}


// ########## Case 1 ##############


void case1()
{

    //Serial.println(F("buttonPushCounter2= "));Serial.println(buttonPushCounter2);   // debug pushCounter2
  switch (buttonPushCounter2)
  {
    case 0:
      Serial.println(F("Case 1-0"));
      //rainbowCycle(10);
      break;

    case 1:
      Serial.println(F("Case 1-1"));
      //rainbowCycle(20);
      break;
      
    case 2:
      Serial.println(F("Case 1-2"));
      //rainbowCycle(50);
      break;     
     
     
  }

  if (buttonPushCounter2 == 2)
  {
    buttonPushCounter2 = 0;
  }

}




void changeEffect() {
  if (digitalRead (BUTTON) == HIGH) {
    buttonPushCounter++;
    buttonPushCounter2 = 0;
  }
}

void changeEffect2() {
  if (digitalRead (BUTTON2) == HIGH) {
    buttonPushCounter2++;
  }
}

It would make more sense to use a RISING or FALLING interrupt because CHANGE will cause two interrupts for every button press.

And change these

if (buttonPushCounter == 4)

to

if (buttonPushCounter >= 4)

so you catch the situation where the count has gone higher than you expect.

...R

thank you. i did not know about the RISING or FALLING options (i am still new to this)
also thanks for the >= idea. that makes a lot of sense

edit:
this seems to solve a lot of problems!

sometimes though i see something that looks like a single button press is interpreted as 2.
i am not familiar with debouncing a button yet - is this something that can help here?

I would not recommend to use interrupts for detecting button presses, but that may just be me. When you press a button, the contact in it will cause som “bouncing” where the signal switches up and down a few times until a stable connection is made in the switch. When you debounce a button you either delay for some milliseconds (bad sollution), or you use timing with millis() to make sure that at least some milliseconds passes between each press. You cannot use delay() in an interrupt handler.

#define BUTTON_DEBOUNCE 50
unsigned long lastPress = 0;
if (millis() - lastPress > BUTTON_DEBOUNCE)
{
  lastPress = millis();
  //Handle button press
}

You could also use a button handler like TButton from TDuino. Then you can use code like this:

#include <TDuino.h>

#define BUTTON_PIN_1 2
#define BUTTON_PIN_2 3
#define BUTTON_DEBOUNCE 50

TButton btn1(BUTTON_PIN_1), btn2(BUTTON_PIN_2);

void buttonPress(byte pin, int state)
{
  if (pin == BUTTON_PIN_1)
  {
    //Handle button 1
  }
  else
  {
    //Handle button 2
  }
}

void setup()
{
  btn1.setup();
  btn1.setDebounce(BUTTON_DEBOUNCE);
  btn1.onPress(buttonPress);

  btn2.setup();
  btn2.setDebounce(BUTTON_DEBOUNCE);
  btn2.onPress(buttonPress);
}

void loop()
{
  btn1.loop();
  btn2.loop();
  //Other non-blocking loop code here..
}

The above code would require that you do not use blocking code elsewhere in the sketch.

m3phisto23:
i am not familiar with debouncing a button yet - is this something that can help here?

The simplest way to do that is to ignore button presses that happen close together - say within 50 millisecs.

Try this

void changeEffect() {
  if (millis() - lastButtonPressMillis >= 50) {
     lastButtonPressMillis = millis();
     buttonPushCounter++;
     buttonPushCounter2 = 0;
  }
}

If you use a RISING interrupt you will know that the button is HIGH

...R

Even if your ISRs detect the button pushes, all they do is update the ‘buttonPushCounter’ and ‘buttonPushCounter2’ state variables (pretty poor names to say the least). So, once you un-comment and start running the LED functions (rainbowCycle, etc) the program is still not going to seem very responsive. Each of those function will still run to completion and the change to the ‘buttonPushCounter’ variables (from the ISRs) will only take effect when it finishes.

That will still be a rather poor user experience. The right way to do it is to unravel the blocking loops in the LED functions and make them state machines.

gfvalvo:
That will still be a rather poor user experience. The right way to do it is to unravel the blocking loops in the LED functions and make them state machines.

+1

...R