One button working but not both

Projects General Guidance

angusvanderslott April 12, 2018, 12:15pm 1

Hi guys,
I'm currently building a guitar tuner and am currently in the process of building the interface. I am using gamestates to represent the tuning of each string (standard tuning, E, A, D, G, B, e) and I have a strip of 6 LEDs with a button either side and when one LED is on, that will represent the string being tuned. The way you cycle through the gamestates is using the buttons, when you press the left one, the next LED to the left of the current one will turn on with every press and the same for the right button. However, when I upload the code, only the right button works and not the left. Both buttons are showing as high in the serial monitor when pressed. Any suggestions? Thanks!

const int stateLowE = 0;
const int stateA = 1;
const int stateD = 2;
const int stateG = 3;
const int stateB = 4;
const int stateHighE = 5;

int leftButton = 6;
int rightButton = 7;

int stateLeft = 0;
int stateRight = 0;

int lowE = 8;
int A = 9;
int D = 10;
int G = 11;
int B = 12;
int highE = 13;

int gameState;

void setup() {

pinMode(leftButton, INPUT);
pinMode(rightButton, INPUT);

pinMode(lowE, OUTPUT);
pinMode(A, OUTPUT);
pinMode(D, OUTPUT);
pinMode(G, OUTPUT);
pinMode(B, OUTPUT);
pinMode(highE, OUTPUT);

gameState = stateLowE;
Serial.begin(9600);

}

void loop() {

if(gameState == stateLowE) {
tuneLowE();
} else if (gameState == stateA){
tuneA();
} else if (gameState == stateD){
tuneD();
} else if (gameState == stateG){
tuneG();
} else if (gameState == stateB){
tuneB();
} else if (gameState == stateHighE){
tuneHighE();
}

Serial.println(digitalRead(rightButton));
//Serial.println(digitalRead(leftButton));
}

void tuneLowE()
{

stateLeft = digitalRead(leftButton);
stateRight = digitalRead(rightButton);

digitalWrite(lowE, HIGH);
digitalWrite(A, LOW);
digitalWrite(D, LOW);
digitalWrite(G, LOW);
digitalWrite(B, LOW);
digitalWrite(highE, LOW);

if(stateRight == HIGH){
delay(500);
gameState = stateA;
} else if (stateRight == LOW){
gameState = stateLowE;
} else if(stateLeft == HIGH){
delay(500);
gameState = stateLowE;
} else if (stateLeft == LOW){
gameState = stateLowE;
}

}

void tuneA(){

stateLeft = digitalRead(leftButton);
stateRight = digitalRead(rightButton);

digitalWrite(lowE, LOW);
digitalWrite(A, HIGH);
digitalWrite(D, LOW);
digitalWrite(G, LOW);
digitalWrite(B, LOW);
digitalWrite(highE, LOW);

if(stateRight == HIGH){
delay(500);
gameState = stateD;
} else if (stateRight == LOW){
gameState = stateA;
} else if(stateLeft == HIGH){
delay(500);
gameState = stateLowE;
} else if (stateLeft == LOW){
gameState = stateA;
}

}

void tuneD(){

stateLeft = digitalRead(leftButton);
stateRight = digitalRead(rightButton);

digitalWrite(lowE, LOW);
digitalWrite(A, LOW);
digitalWrite(D, HIGH);
digitalWrite(G, LOW);
digitalWrite(B, LOW);
digitalWrite(highE, LOW);

if(stateRight == HIGH){
delay(500);
gameState = stateG;
} else if (stateRight == LOW){
gameState = stateD;
} else if(stateLeft == HIGH){
delay(500);
gameState = stateA;
} else if (stateLeft == LOW){
gameState = stateD;
}

}

void tuneG(){

stateLeft = digitalRead(leftButton);
stateRight = digitalRead(rightButton);

digitalWrite(lowE, LOW);
digitalWrite(A, LOW);
digitalWrite(D, LOW);
digitalWrite(G, HIGH);
digitalWrite(B, LOW);
digitalWrite(highE, LOW);

if(stateRight == HIGH){
delay(500);
gameState = stateB;
} else if (stateRight == LOW){
gameState = stateG;
} else if(stateLeft == HIGH){
delay(500);
gameState = stateD;
} else if (stateLeft == LOW){
gameState = stateG;
}

}

void tuneB(){

stateLeft = digitalRead(leftButton);
stateRight = digitalRead(rightButton);

digitalWrite(lowE, LOW);
digitalWrite(A, LOW);
digitalWrite(D, LOW);
digitalWrite(G, LOW);
digitalWrite(B, HIGH);
digitalWrite(highE, LOW);

if(stateRight == HIGH){
delay(500);
gameState = stateHighE;
} else if (stateRight == LOW){
gameState = stateB;
} else if(stateLeft == HIGH){
delay(500);
gameState = stateG;
} else if (stateLeft == LOW){
gameState = stateB;
}

}

void tuneHighE(){

stateLeft = digitalRead(leftButton);
stateRight = digitalRead(rightButton);

digitalWrite(lowE, LOW);
digitalWrite(A, LOW);
digitalWrite(D, LOW);
digitalWrite(G, LOW);
digitalWrite(B, LOW);
digitalWrite(highE, HIGH);

if(stateRight == HIGH){
delay(500);
gameState = stateHighE;
} else if (stateRight == LOW){
gameState = stateHighE;
} else if(stateLeft == HIGH){
delay(500);
gameState = stateB;
} else if (stateLeft == LOW){
gameState = stateHighE;
}

}

6v6gt April 12, 2018, 1:13pm 2

Have you got pull down resistors on these buttons ?

pinMode(leftButton, INPUT);
pinMode(rightButton, INPUT);

. . .
. . .
stateLeft = digitalRead(leftButton);
stateRight = digitalRead(rightButton);
. . .
. . .

if(stateRight == HIGH){
delay(500);
gameState = stateA;
. . .
. . .

INTP April 12, 2018, 1:27pm 3

Your state right might be staying high if you have no pulldown so it doesn't get to the else ifs. Just a cursory glance.

angusvanderslott April 12, 2018, 1:33pm 4

yeah mate, each connected to ground through 220Ω resistors

6v6gt:
Have you got pull down resistors on these buttons ?

pinMode(leftButton, INPUT);
pinMode(rightButton, INPUT);

. . .
. . .
stateLeft = digitalRead(leftButton);
stateRight = digitalRead(rightButton);
. . .
. . .

if(stateRight == HIGH){
delay(500);
gameState = stateA;
. . .
. . .

angusvanderslott April 12, 2018, 1:49pm 5

INTP:
Your state right might be staying high if you have no pulldown so it doesn't get to the else ifs. Just a cursory glance.

hey, yeah they're both connected to pull down resistors and when i release them they go back to 0 in the serial monitor

wvmarle April 12, 2018, 2:15pm 6

I see a few things in this code: lots of repetition, and delays. Neither are good. Cleaning up your code will go quite a way towards tracking down the issue with the button.

You can toggle much easier through your states:

if (digitalRead(leftButton)) {
  gameState--;
  if (gameState < 0) gameState = 5;
}
if (digitalRead(rightButton)) {
  gameState++;
  if (gameState > 5) gameState = 0;
}

OK this may lead to very sensitive buttons as I don't do state tracking or debouncing. All other digitalRead() calls on the buttons can go.

For the LEDs: create a function that sets it based on the state.

const byte LEDs = [8, 9, 10, 11, 12, 13];

void setLEDs() {
  for (byte i = 0; i < 5; i++) {
     digitalWrite(LEDs[i], i == gameState);
  }
}

Just call this function every time you have that whole bunch of digitalWrite() calls.

Going through your code I found your problem:

  if (stateRight == HIGH) {
    delay(500);
    gameState = stateA;
  } else if (stateRight == LOW) {
    gameState = stateLowE;
  } else if (stateLeft == HIGH) {
    delay(500);
    gameState = stateLowE;
  } else if (stateLeft == LOW) {
    gameState = stateLowE;
  }

stateRight is always LOW or HIGH so it never gets past the first if/else set!

Anyway, after all these optimisations this is what's left of your sketch (afaict functionally equivalent, except that both buttons should actually work):

const byte stateLowE = 0;
const byte stateA = 1;
const byte stateD = 2;
const byte stateG = 3;
const byte stateB = 4;
const byte stateHighE = 5;

const byte leftButton = 6;
const byte rightButton = 7;

bool leftPressed = 0;
bool rightPressed = 0;

const byte LEDs[] = {8, 9, 10, 11, 12, 13};

byte gameState;

void setup() {

  pinMode(leftButton, INPUT);
  pinMode(rightButton, INPUT);
  for (byte i = 0; i < 5; i++) {
    pinMode(LEDs[i], OUTPUT);
  }
  gameState = stateLowE;
  setLEDs();
  Serial.begin(9600);

}

void loop() {
  switch (gameState) {
    case stateLowE:
      tuneLowE();
      break;
    case stateA:
      tuneA();
      break;
    case stateD:
      tuneD();
      break;
    case stateG:
      tuneG();
      break;
    case stateB:
      tuneB();
      break;
    case stateHighE:
      tuneHighE();
      break;
  }

  if (digitalRead(leftButton) && leftPressed == false) {
    leftPressed = true; // state checking
    delay(25); // debounce
    gameState--;
    if (gameState < 0) gameState = 5;
  }
  else {
    leftPressed = false;
  }
  if (digitalRead(rightButton) && rightPressed == false) {
    rightPressed = true; // state checking
    delay(25); // debounce
    gameState++;
    if (gameState > 5) gameState = 0;
  }
  else {
    rightPressed = false;
  }

  setLEDs();
}

void setLEDs() {
  for (byte i = 0; i < 5; i++) {
    digitalWrite(LEDs[i], i == gameState);
  }
}

void tuneLowE() {
}

void tuneA() {
}

void tuneD() {
}

void tuneG() {
}

void tuneB() {
}

void tuneHighE() {
}

Which is of course the functional equivalent of this, after stripping the now unused functions:

const byte leftButton = 6;
const byte rightButton = 7;

bool leftPressed = 0;
bool rightPressed = 0;

const byte LEDs[] = {8, 9, 10, 11, 12, 13};

byte gameState;

void setup() {

  pinMode(leftButton, INPUT);
  pinMode(rightButton, INPUT);
  for (byte i = 0; i < 5; i++) {
    pinMode(LEDs[i], OUTPUT);
  }
  gameState = 0;
  setLEDs();
  Serial.begin(9600);

}

void loop() {
  if (digitalRead(leftButton) && leftPressed == false) {
    leftPressed = true; // state checking
    delay(25); // debounce
    gameState--;
    if (gameState < 0) gameState = 5;
  }
  else {
    leftPressed = false;
  }
  if (digitalRead(rightButton) && rightPressed == false) {
    rightPressed = true; // state checking
    delay(25); // debounce
    gameState++;
    if (gameState > 5) gameState = 0;
  }
  else {
    rightPressed = false;
  }

  setLEDs();
}

void setLEDs() {
  for (byte i = 0; i < 5; i++) {
    digitalWrite(LEDs[i], i == gameState);
  }
}

angusvanderslott April 16, 2018, 10:28am 7

wvmarle:
I see a few things in this code: lots of repetition, and delays. Neither are good. Cleaning up your code will go quite a way towards tracking down the issue with the button.

You can toggle much easier through your states:
if (digitalRead(leftButton)) {

gameState--;
if (gameState < 0) gameState = 5;
}
if (digitalRead(rightButton)) {
gameState++;
if (gameState > 5) gameState = 0;
}




OK this may lead to very sensitive buttons as I don't do state tracking or debouncing. All other digitalRead() calls on the buttons can go.

For the LEDs: create a function that sets it based on the state.

const byte LEDs = [8, 9, 10, 11, 12, 13];

void setLEDs() {
for (byte i = 0; i < 5; i++) {
digitalWrite(LEDs[i], i == gameState);
}
}

Just call this function every time you have that whole bunch of digitalWrite() calls.

Going through your code I found your problem:
if (stateRight == HIGH) {
delay(500);
gameState = stateA;
} else if (stateRight == LOW) {
gameState = stateLowE;
} else if (stateLeft == HIGH) {
delay(500);
gameState = stateLowE;
} else if (stateLeft == LOW) {
gameState = stateLowE;
}
stateRight is always LOW or HIGH so it never gets past the first if/else set!

Anyway, after all these optimisations this is what's left of your sketch (afaict functionally equivalent, except that both buttons should actually work):
const byte stateLowE = 0;
const byte stateA = 1;
const byte stateD = 2;
const byte stateG = 3;
const byte stateB = 4;
const byte stateHighE = 5;

const byte leftButton = 6;
const byte rightButton = 7;

bool leftPressed = 0;
bool rightPressed = 0;

const byte LEDs[] = {8, 9, 10, 11, 12, 13};

byte gameState;

void setup() {

pinMode(leftButton, INPUT);
pinMode(rightButton, INPUT);
for (byte i = 0; i < 5; i++) {
pinMode(LEDs[i], OUTPUT);
}
gameState = stateLowE;
setLEDs();
Serial.begin(9600);

}

void loop() {
switch (gameState) {
case stateLowE:
tuneLowE();
break;
case stateA:
tuneA();
break;
case stateD:
tuneD();
break;
case stateG:
tuneG();
break;
case stateB:
tuneB();
break;
case stateHighE:
tuneHighE();
break;
}

if (digitalRead(leftButton) && leftPressed == false) {
leftPressed = true; // state checking
delay(25); // debounce
gameState--;
if (gameState < 0) gameState = 5;
}
else {
leftPressed = false;
}
if (digitalRead(rightButton) && rightPressed == false) {
rightPressed = true; // state checking
delay(25); // debounce
gameState++;
if (gameState > 5) gameState = 0;
}
else {
rightPressed = false;
}

setLEDs();
}

void setLEDs() {
for (byte i = 0; i < 5; i++) {
digitalWrite(LEDs[i], i == gameState);
}
}

void tuneLowE() {
}

void tuneA() {
}

void tuneD() {
}

void tuneG() {
}

void tuneB() {
}

void tuneHighE() {
}

Which is of course the functional equivalent of this, after stripping the now unused functions:
const byte leftButton = 6;
const byte rightButton = 7;

bool leftPressed = 0;
bool rightPressed = 0;

const byte LEDs[] = {8, 9, 10, 11, 12, 13};

byte gameState;

void setup() {

pinMode(leftButton, INPUT);
pinMode(rightButton, INPUT);
for (byte i = 0; i < 5; i++) {
pinMode(LEDs[i], OUTPUT);
}
gameState = 0;
setLEDs();
Serial.begin(9600);

}

void loop() {
if (digitalRead(leftButton) && leftPressed == false) {
leftPressed = true; // state checking
delay(25); // debounce
gameState--;
if (gameState < 0) gameState = 5;
}
else {
leftPressed = false;
}
if (digitalRead(rightButton) && rightPressed == false) {
rightPressed = true; // state checking
delay(25); // debounce
gameState++;
if (gameState > 5) gameState = 0;
}
else {
rightPressed = false;
}

setLEDs();
}

void setLEDs() {
for (byte i = 0; i < 5; i++) {
digitalWrite(LEDs[i], i == gameState);
}
}

Incredible! Thanks for your help

Topic		Replies	Views	Activity
Two buttons to control two different set of leds Programming	6	882	October 9, 2022
Single button works, two buttons fail Programming	11	2354	May 5, 2021
Coding Issues with Buttons Programming	6	1716	May 5, 2021
Trying to play 2 differents melodies with 2 buttons but only 1 works... General Guidance	3	440	May 5, 2021
Using 2 buttons simultaneously to make a 3rd action Programming	8	8677	May 5, 2021