Simple LED pushbutton sketch won't behave as it should.

Projects Programming
1

I'm following a tutorial in a book to turn off and on an LED using a pushbutton. Theoretically the LED should remain off until I turn it on using the pushbutton switch. What actually happens is that the LED is constantly on and not reacting to the pushbutton switch at all. Can someone please look this over and explain what is wrong? Here is the code and fritzing diagram of my wiring.

#define LED 13    // Pin for the LED
 #define BUTTON 2  // The input pin where the
                   // pushbutton is connected
 int val = 0;      // variable stores value of input 
                   // pin
 int old_val = 0;  // this variable stores previous 
                   // value of val
 int state = 0;    // 0 = LED off and 1 = LED on
 
 void setup() {
   pinMode(LED, OUTPUT);
   pinMode(BUTTON, INPUT);
 }
 
 void loop() {
   val = digitalRead(BUTTON); 
   
   // check if there was a transition
   if ((val == HIGH) && (old_val == LOW)) {
     state = 1 - state;
     delay(10);
   }
   
   old_val = val;
   
   if (state = 1) {
     digitalWrite(LED, HIGH); // turn LED on
   } else {
     digitalWrite(LED, LOW); // turn LED off
   }
 }

pushitrealgood.png
pushitrealgood.png1755×1419 297 KB

2

Does this look correct to you?

if (state = 1) {

One other thing, why use an external LED, when the board has a built-in LED?

3

Sorry for the lack of a response you're probably looking for but ...

While comments can be interesting they can also be wrong, misleading or out of sync with the actual code.

I'm going to make a suggestion early on that you come up with some naming convention of symbols to allows you to reduce the need for comments as much as possible. This will make your cod easier to read and debug ...

// CONSTANTS and SYNONYMS

const uint8_t   pinBUTTON           =  2;
const uint8_t   pinLED              = 13;

const  uint8_t  stateBUTTON_UP      = LOW;
const  uint8_t  stateBUTTON_DOWN    = HIGH;

const  uint8_t  stateLED_OFF        = LOW;
const  uint8_t  stateLED_ON         = HIGH;


// GLOBAL VARIABLES

uint8_t         stateButtonThen;
uint8_t         stateLED;

void loop()
{
    // check if there was a transition of button state

    uint8_t stateButtonNow = digitalRead(pinBUTTON); 

    if ( (stateButtonNow == stateBUTTON_DOWN) && (stateButtonThen == stateBUTTON_UP) )
    {
        stateLED = ! stateLED;
        delay(10UL);
    }

    stateButtonThen = stateButtonNow;

    digitalWrite(pinLED, stateLED);
}

void setup()
{
    pinMode(pinBUTTON, INPUT);
    pinMode(pinLED, OUTPUT);

    stateButtonThen     = stateBUTTON_UP;
    stateLED            = stateLED_OFF;
}
4

Thank you HazardsMind! It works great now and I also just learned it's possible to assign variables within a conditional expression. I need to pay way more attention to my tokens. @Lloyddean, I'm obviously still in arduino kindergarten, but I will heed that advice. Thanks again.

5

Eric, not your code, I know, but the state variable is already set to the value needed to turn the LED on or off so

   if (state = 1) {
     digitalWrite(LED, HIGH); // turn LED on
   } else {
     digitalWrite(LED, LOW); // turn LED off
   }

can be replaced with

 digitalWrite(LED, state);
6

Using an LED without a current limiting resistor is a good way to burn out the Arduino. Better order a spare.

7

Despite the errors in the code I think debounce would be an issue as well. When you push a button, it takes around 150ms to get it down. But in those 150ms the button sometimes doesn't just change the state at once but instead it could have on/off flicks of as fast as 5ms in one single push. This happens because of the hardware, but then the Arduino reads those debounces and thinks that that they are state changes. Consider adding some debounce code to you sketch.

Read more on the topic for a better understanding: