Turn LED on and off with switch

Question

I'm trying to write code to get an LED to turn on when it is off and to turn off when it is on using a tactile push button switch. I've written what I believe to be is the right code with the wiringPi library, but I can only get it to turn on when it is off and can't get it to turn off after that. On very rare instances and after many repeated presses will the LED turn off when it is on and I press the button, but I'm sure that's not how it's supposed to work.

#include <wiringPi.h>
int main (void)
{
    wiringPiSetup ();
    pinMode (0, OUTPUT);
    pinMode (1, INPUT);
    digitalWrite (0, LOW);
    for(;;)
    {
        if(digitalRead (1) == LOW)
        {
            if(digitalRead (0) == HIGH)
                digitalWrite (0, LOW);
            else if(digitalRead (0) == LOW)
                digitalWrite (0, HIGH);
         }
     }
     return 0;
}

I have attached an image of how the circuit is wired.

Answer 1

The wiring looks correct for the code.

The problem is that the code is in a very tight loop. In theory, when the button is pressed, the loop body repeatedly turns the LED on and off. In theory, there would be a 50/50 chance the LED is left on (or off) when the button is released. Do you notice a change in brightness when the button is pressed. There might not be enough to be noticed.

In practice, the reason for the tendency to leave the LED on is the way you test to see if it already is on. Writing pin 0 HIGH applies 3.3 V to the output. But that wire is connected to the LED and the pin configured to be an output. The LED may be dropping the voltage low enough to not register as HIGH when it is read, but sometimes it does because it is near the cutoff.

In practice, code to turn the LED off and on with each button press would use a falling-edge-triggered interrupt. As pointed out in the comments, you would want to debounce the interrupt in that case. You could also do the same thing without interrupts by recording the previous state of the button and only changing the LED when the button state had changed. Debouncing as the code is written now makes no sense.

#include <wiringPi.h>
int main (void)
{
    wiringPiSetup ();
    pinMode (0, OUTPUT);
    pinMode (1, INPUT);
    digitalWrite (0, LOW);

    int prevButton = HIGH, LED = 0;

    for(;;)
    {
        if(prevButton == HIGH && digitalRead(1) == LOW)  // a falling edge
        {
            prevButton = LOW;

            if(LED)
            {
                LED = 0;
                digitalWrite(0, LOW);
            }
            else
            {
                LED = 1;
                digitalWrite(0, HIGH);
            }
        }
        else if(prevButton == LOW && digitalRead(1) == HIGH)  // a rising edge, do nothing
        {
            prevButton = HIGH;
        )

        // Add a delay here to debounce the button 

    }
    return 0;
}

Answer 2

It is probably simpler to maintain "state" in normal variables rather than trying to infer it from the current GPIO state.

Also the "busy-loop" will consume every CPU cycle the OS will allow the process; for such a simple process, you will see that your CPU load will rise to 100%! You should allow the process to relinquish the CPU to other tasks with a usleep() call for example. The delay will also serve to de-bounce the switch.

#include <wiringPi.h>
#include <unistd.h>

int main (void)
{
  wiringPiSetup ();
  pinMode (0, OUTPUT);
  pinMode (1, INPUT);
  digitalWrite (0, LOW);

  // Initial state
  int led = LOW ;
  bool button_down = (digitalRead(1) == LOW) ;

  for(;;)
  {
    // If button-down event (Hi-lo transition)...
    if( !button_down && digitalRead(1) == LOW )
    { 
      // Keep button state
      button_down = true ;

      // Toggle LED state
      led = (led == LOW) ? HIGH : LOW ;
      digitalWrite( 0, led ) ;
    }
    // Button up event...
    else if( button_down && digitalRead(1) == HIGH ) 
    {
      // Keep button state
      button_down = false ;
    }

    usleep( 10000 ) ;
  }

  return 0;
}