Sketch works with one button but not 2

Hi All,

I've been trying to write a sketch that controls a motor. One button for up, one for down. I am using the Ardunio Motor Shield to control the motor.
I have everything connected to do with the hardware. If I run the sketch with just one button coded it works as intended, and if I code for just the other that works too, but when without changing any hardware I code for both it does not! The indicating lights come on on the shield but nothing comes from the motor.
The motor is powered by a wall power supply
To add a second button I have re-named, but basically just duplicated the code that makes one button work

#define LED 13
#define BUTTON 5
#define DOWN 6
#define PWA 3
#define DIRA 12

int val = 0;

int old_val = 0;

int va = 0;

int old_va = 0;

int state = 0;



void setup()
{
  pinMode(LED, OUTPUT);
  pinMode(BUTTON, INPUT);
  pinMode(DOWN, INPUT);
  pinMode(PWA, OUTPUT);
  pinMode(DIRA, OUTPUT);
}

void loop()
{
  val = digitalRead(BUTTON);
  va = digitalRead(DOWN);
  
  if((val == HIGH) && (old_val))
  {
    state = 1 - state;
    delay(10);
  }
  
  if((va == HIGH) && (old_va))
  {
    state = 2 - state;
    delay(10);
  }
  
  old_va = va;
  old_val = val;
  
  if(state == 1)
  {
    digitalWrite(LED, HIGH);
    digitalWrite(DIRA, LOW);
 analogWrite(PWA, 255);
  }else{
    digitalWrite(LED, LOW);
    analogWrite(PWA, 0);
  }
  
  if(state == 2)
  {
    digitalWrite(LED, LOW);
    digitalWrite(DIRA, HIGH);
 analogWrite(PWA, 255);
  }else{
    digitalWrite(LED, LOW);
    analogWrite(PWA, 0);
  }
  
}

Couple of things that you can try:

  1. Make your variable names more descriptive. For example if the button is UP, then variables associated should have up in the name. It makes it easier to read for everybody.
  2. You should use Serial.Print() statements to look at what your state variable is, because if it is not 1 or 2 the code will do nothing. Printing stuff out is a good way to understand the way that your program is working and can all be removed at the end.
  3. You have delays in the code that don't appear to be needed. This won't make it stop working but is just unnecessary.

Your code is atrocious to try and read. Even stuff like your if statements, why the explicit "== HIGH" on only one branch?

I'm pretty sure your problem is that you're setting your state variable wrong. I don't think

state = 1 - state;
state = 2 - state;

is doing what you think it is. But because of how poorly written your code is, it's hard to say.

Your if's and else's make no sense. If state = 1, doesn't that guarantee the last thing you do is execute the else for state = 2 so you'll never even see the result of your state = 1 portion?

Thank you for your suggestions, I did them and it now works! I appreciate your points on making the code clearer and less messy. :slight_smile: