Buttons and Led's - Button switching too quickly

I've got a project where 16 different strips of RGB led's are changing color. I have a button set up to each strip and when that button is hit I want it to turn that strip a solid color until the button is pressed again. When the button is pressed again I want the strip to return to changing colors. (i want to avoid delay so I looked up how to do that).

I'm having an issue where if I don't press the button quickly enough, it somewhat glitches. It will turn the switch on and then off right away.

I've attached my code. Right now I'm just trying to deal with the first strip. I'm very new to programming so bear with me if this code is redundant. If you have any pointers on how to set all this up more simply please let me know.

Thanks.

I'm using an arduino mega 2560.

// output pins
int inMin = 1;
int inMax = 48;

//input pins
int button1 = A0, button2 = A1, button3 = A2, button4 = A3, button5 = A4,
    button6 = A5, button7 = A6, button8 = A7, button9 = A8, button10 = A9,
    button11 = A10, button12 = 11, button13 = A12, button14 = A13, 
    button15 = A14, button16 = A15;
    
int buttonPress1 = 0, buttonPress2 = 0, buttonPress3 = 0, buttonPress4 = 0,
    buttonPress5 = 0, buttonPress6 = 0, buttonPress7 = 0, buttonPress8 = 0,
    buttonPress9 = 0, buttonPress10 = 0, buttonPress11 = 0, buttonPress12 = 0,
    buttonPress13 = 0, buttonPress14 = 0, buttonPress15 = 0, buttonPress16 = 0;
  
long previousMillis1 = 0, previousMillis2 = 0, previousMillis3 = 0, 
     previousMillis4 = 0, previousMillis5 = 0, previousMillis6 = 0,
     previousMillis7 = 0, previousMillis8 = 0, previousMillis9 = 0,
     previousMillis10 = 0, previousMillis11 = 0, previousMillis12 = 0,
     previousMillis13 = 0, previousMillis14 = 0, previousMillis15 = 0,
     previousMillis16 = 0;
     
long interval1 = 500, interval2 = 1000, interval3 = 1000, interval4 = 1000,
     interval5 = 1000, interval6 = 1000, interval7 = 1000, interval8 = 1000;
 
void setup() { 

Serial.begin(9600);  

  for(int i=inMin; i<=inMax; i++)
{
   pinMode(i, OUTPUT);
}   
  pinMode(button1, INPUT); pinMode(button2, INPUT); 
  pinMode(button3, INPUT); pinMode(button4, INPUT); 
  pinMode(button5, INPUT); pinMode(button6, INPUT);
  pinMode(button7, INPUT); pinMode(button8, INPUT); 
  pinMode(button9, INPUT); pinMode(button10, INPUT); 
  pinMode(button11, INPUT); pinMode(button12, INPUT);
  pinMode(button13, INPUT); pinMode(button14, INPUT); 
  pinMode(button15, INPUT);pinMode(button16, INPUT);
  
  }

void loop() 
{
  Serial.println(buttonPress1);
  
  int buttonState1 = digitalRead(button2);   
 
 if (buttonState1 == 1 && buttonPress1 == 0)
   {
     digitalWrite(4, HIGH);
     digitalWrite(5, LOW);
     buttonPress1++;}
     
  else  if (buttonState1 == 1 & buttonPress1 == 1)
  {buttonPress1 = 0;}
     
  else if (buttonState1 == 0 && buttonPress1 == 0)
  {
 unsigned long currentMillis1 = millis();
   
   if (currentMillis1 - previousMillis1 >= interval1) {
     previousMillis1 = currentMillis1;
     
         digitalWrite(4, !digitalRead(4));
       }
       
  unsigned long currentMillis2 = millis();
   
   if (currentMillis2 - previousMillis2 >= interval2) {
     previousMillis2 = currentMillis2;
     
         digitalWrite(5, !digitalRead(5));
       
   }
     
}}
int buttonPress1 = 0, buttonPress2 = 0, buttonPress3 = 0, buttonPress4 = 0,
    buttonPress5 = 0, buttonPress6 = 0, buttonPress7 = 0, buttonPress8 = 0,
    buttonPress9 = 0, buttonPress10 = 0, buttonPress11 = 0, buttonPress12 = 0,
    buttonPress13 = 0, buttonPress14 = 0, buttonPress15 = 0, buttonPress16 = 0;

You are simultaneously over-thinking and under-thinking this.
You need an array.
You need to rely on crt0.

You were missing a "&".. Try this code again and let me know what happens

// output pins
int inMin = 1;
int inMax = 48;

//input pins
int button1 = A0, button2 = A1, button3 = A2, button4 = A3, button5 = A4,
    button6 = A5, button7 = A6, button8 = A7, button9 = A8, button10 = A9,
    button11 = A10, button12 = 11, button13 = A12, button14 = A13, 
    button15 = A14, button16 = A15;
    
int buttonPress1 = 0, buttonPress2 = 0, buttonPress3 = 0, buttonPress4 = 0,
    buttonPress5 = 0, buttonPress6 = 0, buttonPress7 = 0, buttonPress8 = 0,
    buttonPress9 = 0, buttonPress10 = 0, buttonPress11 = 0, buttonPress12 = 0,
    buttonPress13 = 0, buttonPress14 = 0, buttonPress15 = 0, buttonPress16 = 0;
  
long previousMillis1 = 0, previousMillis2 = 0, previousMillis3 = 0, 
     previousMillis4 = 0, previousMillis5 = 0, previousMillis6 = 0,
     previousMillis7 = 0, previousMillis8 = 0, previousMillis9 = 0,
     previousMillis10 = 0, previousMillis11 = 0, previousMillis12 = 0,
     previousMillis13 = 0, previousMillis14 = 0, previousMillis15 = 0,
     previousMillis16 = 0;
     
long interval1 = 500, interval2 = 1000, interval3 = 1000, interval4 = 1000,
     interval5 = 1000, interval6 = 1000, interval7 = 1000, interval8 = 1000;
 
void setup() { 

Serial.begin(9600);  

  for(int i=inMin; i<=inMax; i++)
{
   pinMode(i, OUTPUT);
}   
  pinMode(button1, INPUT); pinMode(button2, INPUT); 
  pinMode(button3, INPUT); pinMode(button4, INPUT); 
  pinMode(button5, INPUT); pinMode(button6, INPUT);
  pinMode(button7, INPUT); pinMode(button8, INPUT); 
  pinMode(button9, INPUT); pinMode(button10, INPUT); 
  pinMode(button11, INPUT); pinMode(button12, INPUT);
  pinMode(button13, INPUT); pinMode(button14, INPUT); 
  pinMode(button15, INPUT);pinMode(button16, INPUT);
  
  }

void loop() 
{
  Serial.println(buttonPress1);
  
  int buttonState1 = digitalRead(button2);   
 
 if (buttonState1 == 1 && buttonPress1 == 0)
   {
     digitalWrite(4, HIGH);
     digitalWrite(5, LOW);
     buttonPress1++;}
     
  else  if (buttonState1 == 1 && buttonPress1 == 1)
  {buttonPress1 = 0;}
     
  else if (buttonState1 == 0 && buttonPress1 == 0)
  {
 unsigned long currentMillis1 = millis();
   
   if (currentMillis1 - previousMillis1 >= interval1) {
     previousMillis1 = currentMillis1;
     
         digitalWrite(4, !digitalRead(4));
       }
       
  unsigned long currentMillis2 = millis();
   
   if (currentMillis2 - previousMillis2 >= interval2) {
     previousMillis2 = currentMillis2;
     
         digitalWrite(5, !digitalRead(5));
       
   }
     
}}

Arrays, ARRAYS... use ARRAYS

You want to have the arduino remember the last button press state, and if the state does NOT change,( high to low or low to high) then don't do anything. Only when the button changes, do something.

You may also need to debounce the buttons too.

I might have something you can use, let me look.

const byte button = 2;
boolean state, lastState = LOW;

void setup()
{
  Serial.begin(115200);
  Serial.println("Start");
  pinMode(button, INPUT);
}

void loop()
{
  state = digitalRead(button);
  
  if(state != lastState) // only do something if the button state changes, ie. not held down
  {
    lastState = state; // update lastState
    Serial.print("The buttons state is: ");
    Serial.println(state);
    
    if(state == HIGH) // do something ONLY when the buttons state is HIGH
      digitalWrite(13, !digitalRead(13) ); 
  }
}

zaxarias:
You were missing a "&".. Try this code again and let me know what happens

// output pins

int inMin = 1;
int inMax = 48;

//input pins
int button1 = A0, button2 = A1, button3 = A2, button4 = A3, button5 = A4,
   button6 = A5, button7 = A6, button8 = A7, button9 = A8, button10 = A9,
   button11 = A10, button12 = 11, button13 = A12, button14 = A13,
   button15 = A14, button16 = A15;
   
int buttonPress1 = 0, buttonPress2 = 0, buttonPress3 = 0, buttonPress4 = 0,
   buttonPress5 = 0, buttonPress6 = 0, buttonPress7 = 0, buttonPress8 = 0,
   buttonPress9 = 0, buttonPress10 = 0, buttonPress11 = 0, buttonPress12 = 0,
   buttonPress13 = 0, buttonPress14 = 0, buttonPress15 = 0, buttonPress16 = 0;
 
long previousMillis1 = 0, previousMillis2 = 0, previousMillis3 = 0,
    previousMillis4 = 0, previousMillis5 = 0, previousMillis6 = 0,
    previousMillis7 = 0, previousMillis8 = 0, previousMillis9 = 0,
    previousMillis10 = 0, previousMillis11 = 0, previousMillis12 = 0,
    previousMillis13 = 0, previousMillis14 = 0, previousMillis15 = 0,
    previousMillis16 = 0;
   
long interval1 = 500, interval2 = 1000, interval3 = 1000, interval4 = 1000,
    interval5 = 1000, interval6 = 1000, interval7 = 1000, interval8 = 1000;

void setup() {

Serial.begin(9600);

for(int i=inMin; i<=inMax; i++)
{
  pinMode(i, OUTPUT);
}  
 pinMode(button1, INPUT); pinMode(button2, INPUT);
 pinMode(button3, INPUT); pinMode(button4, INPUT);
 pinMode(button5, INPUT); pinMode(button6, INPUT);
 pinMode(button7, INPUT); pinMode(button8, INPUT);
 pinMode(button9, INPUT); pinMode(button10, INPUT);
 pinMode(button11, INPUT); pinMode(button12, INPUT);
 pinMode(button13, INPUT); pinMode(button14, INPUT);
 pinMode(button15, INPUT);pinMode(button16, INPUT);
 
 }

void loop()
{
 Serial.println(buttonPress1);
 
 int buttonState1 = digitalRead(button2);

if (buttonState1 == 1 && buttonPress1 == 0)
  {
    digitalWrite(4, HIGH);
    digitalWrite(5, LOW);
    buttonPress1++;}
   
 else  if (buttonState1 == 1 && buttonPress1 == 1)
 {buttonPress1 = 0;}
   
 else if (buttonState1 == 0 && buttonPress1 == 0)
 {
unsigned long currentMillis1 = millis();
 
  if (currentMillis1 - previousMillis1 >= interval1) {
    previousMillis1 = currentMillis1;
   
        digitalWrite(4, !digitalRead(4));
      }
     
 unsigned long currentMillis2 = millis();
 
  if (currentMillis2 - previousMillis2 >= interval2) {
    previousMillis2 = currentMillis2;
   
        digitalWrite(5, !digitalRead(5));
     
  }
   
}}

Same issue still. Thank you for helping me with that fix though.

HazardsMind:
Arrays, ARRAYS... use ARRAYS

You want to have the arduino remember the last button press state, and if the state does NOT change,( high to low or low to high) then don't do anything. Only when the button changes, do something.

You may also need to debounce the buttons too.

I might have something you can use, let me look.

Okay, yeah I'll definitely start with the array. (My noob skills are very evident haha).

Thank you, I really appreciate your help.

I've posted the code thus far.

Is there anyway to make it more accurate? Even with just two strips changing colors right now, and having a button for each, sometimes there is a big delay or it won't read the button state at all.

I suppose I'm the issue being such a noob.

// output pins
int inMin = 1;
int inMax = 48;

//input pins
const byte button[]= {A0, A1, A2, A3, A4, A5, A6, A7, A8, A9, A10, A11, A12, A13,
               A14, A15};
              
int buttonPress[16] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
  
long previousMillis[48] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
                           0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
                           0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,};
     
long interval[48] = {500, 600, 500, 500, 600, 500, 1000};
 
boolean state1, lastState1 = LOW;
boolean state2, lastState2 = LOW;
boolean state3, lastState3 = LOW;
boolean state4, lastState4 = LOW;
boolean state5, lastState5 = LOW;
boolean state6, lastState6 = LOW;
boolean state7, lastState7 = LOW;
boolean state8, lastState8 = LOW;
boolean state9, lastState9 = LOW;
boolean state10, lastState10 = LOW;
boolean state11, lastState11 = LOW;
boolean state12, lastState12 = LOW;
boolean state13, lastState13 = LOW;
boolean state14, lastState14 = LOW;
boolean state15, lastState15 = LOW;
boolean state16, lastState16 = LOW;



 
void setup() { 

Serial.begin(115200);  

  for(int i=inMin; i<=inMax; i++)
{
   pinMode(i, OUTPUT);
}   
 
 for(int i = 0; i<= 16; i++)
 {
  pinMode(button[i], INPUT);
  } 
}



void loop() 
{

  
  
  
  
  //strip 1
  
 state1 = digitalRead(button[0]);
state2 = digitalRead(button[1]); 
    
 if(state1 != lastState1)
 {
   lastState1 = state1;
 }

 if(state1 == HIGH && buttonPress[0] == 0)
 {
   digitalWrite(1, HIGH);
   digitalWrite(2, LOW);
   digitalWrite(3, LOW);
   buttonPress[0]++;}
     
     
  else if (state1 == 0 && buttonPress[0] == 0)
  {
    
 unsigned long currentMillis1 = millis();
   
   if (currentMillis1 - previousMillis[0] >= interval[0]) {
     previousMillis[0] = currentMillis1;
     
         digitalWrite(1, !digitalRead(1));
       }
       
  unsigned long currentMillis2 = millis();
   
   if (currentMillis2 - previousMillis[1] >= interval[1]) {
     previousMillis[1] = currentMillis2;
     
         digitalWrite(2, !digitalRead(2));}
  
  unsigned long currentMillis3 = millis();
   
   if (currentMillis3 - previousMillis[2] >= interval[2]) {
     previousMillis[2] = currentMillis3;
     
         digitalWrite(3, !digitalRead(3));}
       
   }
   
   else  if (state1 == 1 && buttonPress[0] == 1)
  {buttonPress[0] = 0;}
     
     
     
     
     
     
     
  //strip 2      
    
 if(state2 != lastState2)
 {
   lastState2 = state2;
}

 if(state2 == HIGH && buttonPress[1] == 0)
 {
   digitalWrite(4, HIGH);
   digitalWrite(5, LOW);
   digitalWrite(6, LOW);
   buttonPress[1]++;}
     
     
  else if (state2 == 0 && buttonPress[1] == 0)
  {
    
 unsigned long currentMillis4 = millis();
   
   if (currentMillis4 - previousMillis[3] >= interval[3]) {
     previousMillis[3] = currentMillis4;
     
         digitalWrite(4, !digitalRead(4));
       }
       
  unsigned long currentMillis5 = millis();
   
   if (currentMillis5 - previousMillis[4] >= interval[4]) {
     previousMillis[4] = currentMillis5;
     
         digitalWrite(5, !digitalRead(5));}
  
  unsigned long currentMillis6 = millis();
   
   if (currentMillis6 - previousMillis[2] >= interval[5]) {
     previousMillis[2] = currentMillis6;
     
         digitalWrite(6, !digitalRead(6));}
       
   }
   
   else  if (state2 == 1 && buttonPress[1] == 1)
  {buttonPress[1] = 0;}
}

:c
You were doing so umm... good with your arrays, why did you stop?

boolean state1, lastState1 = LOW;
boolean state2, lastState2 = LOW;
boolean state3, lastState3 = LOW;
boolean state4, lastState4 = LOW;
boolean state5, lastState5 = LOW;
boolean state6, lastState6 = LOW;
boolean state7, lastState7 = LOW;
boolean state8, lastState8 = LOW;
boolean state9, lastState9 = LOW;
boolean state10, lastState10 = LOW;
boolean state11, lastState11 = LOW;
boolean state12, lastState12 = LOW;
boolean state13, lastState13 = LOW;
boolean state14, lastState14 = LOW;
boolean state15, lastState15 = LOW;
boolean state16, lastState16 = LOW;

These here can go into FOR loops,

boolean buttonPress[16]; // No need for zeros here, and should be boolean, unless you are looking for a value between 0 -1023, in which case you would be using type INT and analogRead().

long previousMillis[48]; // again no zeros needed here

In your setup() function, you use the FOR loops and set the arrays to zero, just like you did with your pinModes.

In your loop() function, you could use either a FOR loop or IF/ELSE statements and go through each array and change their indexes at the same time.

HazardsMind:
:c
You were doing so umm... good with your arrays, why did you stop?

boolean state1, lastState1 = LOW;
boolean state2, lastState2 = LOW;
boolean state3, lastState3 = LOW;
boolean state4, lastState4 = LOW;
boolean state5, lastState5 = LOW;
boolean state6, lastState6 = LOW;
boolean state7, lastState7 = LOW;
boolean state8, lastState8 = LOW;
boolean state9, lastState9 = LOW;
boolean state10, lastState10 = LOW;
boolean state11, lastState11 = LOW;
boolean state12, lastState12 = LOW;
boolean state13, lastState13 = LOW;
boolean state14, lastState14 = LOW;
boolean state15, lastState15 = LOW;
boolean state16, lastState16 = LOW;

These here can go into FOR loops,

boolean buttonPress[16]; // No need for zeros here, and should be boolean, unless you are looking for a value between 0 -1023, in which case you would be using type INT and analogRead().

long previousMillis[48]; // again no zeros needed here

In your setup() function, you use the FOR loops and set the arrays to zero, just like you did with your pinModes.

In your loop() function, you could use either a FOR loop or IF/ELSE statements and go through each array and change their indexes at the same time.

Thanks HazardsMind! haha I guess I got a little confused. Thanks for helping me along the way. Do you know if changing this up will address my button issue or not?

How big of a delay? I can tell you your serial monitor is not going to work correctly because your using digital pin 1. Start at pin 2 instead.

Added:
Should be, for(int i = 0; i < 16; i++) and not, for(int i = 0; i<= 16; i++)

It's not a delay as much as it sometimes works and sometimes doesn't. It usually works to change to solid color. But sometimes to change it back to color-changing I have to hit the button 3-4 times before it works.

int buttonPress[16] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
  
long previousMillis[48] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
                           0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
                           0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,};

Isn't necessary.

Let the computer do the work -

int buttonPress [16];
unsigned long previousMillis[48];

is sufficient.

HazardsMind:
How big of a delay? I can tell you your serial monitor is not going to work correctly because your using digital pin 1. Start at pin 2 instead.

Added:
Should be, for(int i = 0; i < 16; i++) and not, for(int i = 0; i<= 16; i++)

Just so I know for future use, is there a reason starting at Pin 1 is a bad thing?

Just so I know for future use, is there a reason starting at Pin 1 is a bad thing?

It is if you want to use the Serial functions in your program because hardware Serial uses pins 0 and 1. Even if your program does not intrinsically use Serial it is a good idea to allow for it to be used for printing debugging information whilst testing.

UKHeliBob:

Just so I know for future use, is there a reason starting at Pin 1 is a bad thing?

It is if you want to use the Serial functions in your program because hardware Serial uses pins 0 and 1. Even if your program does not intrinsically use Serial it is a good idea to allow for it to be used for printing debugging information whilst testing.

Ok cool. I had no idea I'd be learning so much from this project. Thanks for the info!

HazardsMind:
Arrays, ARRAYS... use ARRAYS

You want to have the arduino remember the last button press state, and if the state does NOT change,( high to low or low to high) then don't do anything. Only when the button changes, do something.

You may also need to debounce the buttons too.

I might have something you can use, let me look.

const byte button = 2;

boolean state, lastState = LOW;

void setup()
{
  Serial.begin(115200);
  Serial.println("Start");
  pinMode(button, INPUT);
}

void loop()
{
  state = digitalRead(button);
 
  if(state != lastState) // only do something if the button state changes, ie. not held down
  {
    lastState = state; // update lastState
    Serial.print("The buttons state is: ");
    Serial.println(state);
   
    if(state == HIGH) // do something ONLY when the buttons state is HIGH
      digitalWrite(13, !digitalRead(13) );
  }
}

That doesn't address bounce at all.

Bounce is when physical contacts make and break the connection many times in a short period.
Bounce happens over time. If all you do is look for an instant, you can't tell what is happening.

Debounce is when you watch or wait for the bouncing to stop for longer than the bouncing happens.
If my code watches the switch change state back and forth, and it's easy to test-sketch that, then it's bouncing.

Once the state stays the same for some period of time, once it is stable, THEN it's time to set a state that the rest of the code can use as button state.

Unstable button state has some uses but generally the desired logic is based on the ideal people think of rather than the reality we need to code for.

Throw in an array of buttons needing to work at the same time together with an array of leds (strips) and you have something that I gave working examples of how to do 3 days ago, but not all in 1 package.
http://forum.arduino.cc/index.php?topic=256847.msg1816262#msg1816262

I could have posted the 4x4 multiplexed buttons sketch but that's a whole nother step more complicated.
Maybe the thing to do is get it done for 1 button first.

I know what bouncing a button is, but that code doesn't need it.

Because the serial print is your delay and you have buttons that finish bouncing by the time it's done.

Do you use a delay after serial available to allow the rest of the message to arrive?

GoForSmoke:
Because the serial print is your delay and you have buttons that finish bouncing by the time it's done.

Do you use a delay after serial available to allow the rest of the message to arrive?

I guess I'm a little lost with the bouncing thing. I'm not using a delay no.