Toggling a blinking led ON & OFF

Question

I am trying to implement a toggle switch to turn blinking ON & OFF. However, the code only turns the led ON or OFF and that too unreliably. Can you help me understand why it isn't working?

   const int redLed = 10;
const int buttonPin = 2;
unsigned long newTime;
unsigned long oldTime = 0;
byte newButtonState;
byte oldButtonState = 0;
boolean ledState = false;
boolean blinkState = false;
boolean state = LOW;


void setup() {
  pinMode(redLed, OUTPUT);
  Serial.begin(9600);
  pinMode(buttonPin, INPUT);
}

void loop() {

  newButtonState = digitalRead(buttonPin);
  delay(100);


  // check if button state has changed
  if (newButtonState != oldButtonState) {
    // if button was pushed
    if (newButtonState == HIGH) {
      // check led state
      if (ledState == false) {
        ledState = true;
        blinkState = true;
      } else {
        ledState = false;
        blinkState = false;
      }
      if (ledState == true && blinkState == true) {
        newTime = millis();
        if (newTime - oldTime >= 250) {
          state = !state;
          oldTime = newTime;
        }
        digitalWrite(redLed, state);
      }
    }
  }
  oldButtonState = newButtonState;
}

Answer 1

I don't see why you would set blinkState together with ledState. I assume, that you want the button to toggle the blinking. 1 press --> LED is blinking, another press --> LED stops blinking, ...

I suggest to restructure your code. First separate input and output, meaning button check code and LED blink code. For blinking you already have a state variable named blinkState. So in your loop() function you first write

newTime = millis();
if(blinkState && newTime - oldTime >= 250){
    ledState = !ledState;
    oldTime = newTime;
}

That will toggle ledState, whenever blinkState is set, in intervals. Now we simply digitalWrite() that value out directly after that:

digitalWrite(redLed, ledState);

Now we need to handle the button. Your button if statements seem to be good, so we only need to change, whats inside the statements. There we simply negate the blinkState variable:

if (newButtonState != oldButtonState) {
    // if button was pushed
    if (newButtonState == HIGH) {
        blinkState = !blinkState;
    }
}
oldButtonState = newButtonState;
delay(100);

With this code the LED will stop changing and keep the state, that it had, when you pressed the button. If you want the LED to turn off at that point, you shoud add ledState = false; after negating blinkState.

Note, that this code doesn't need your state variable. 2 variables are enough for this. 1 for the current led state, 1 for the blinking state.

Also note, that using delay() is not the best way to debounce a button. It is an easy way, but it might not behave exactly, like you want to. If you want better debouncing, you can use the Bounce2 library from github. If you don't want to use a library for it, you can still learn about their ways to debounce in the Readme file.

Answer 2

Note, this is not an answer, but I want to show some code examples based on the comment:

You can use as said in the comment:

• Remove '== true' in a boolean condition
• Replace '== false' by '!' in a boolean condition

You will get this:

  // check if button state has changed
  if (newButtonState != oldButtonState) {
    // if button was pushed
    if (newButtonState == HIGH) {
      // check led state
      if (!ledState) {
        ledState = true;
        blinkState = true;
      } else {
        ledState = false;
        blinkState = false;
      }
      if (ledState && blinkState) {
        newTime = millis();
        if (newTime - oldTime >= 250) {
          state = !state;
          oldTime = newTime;
        }
        digitalWrite(redLed, state);
      }
    }
  }
  oldButtonState = newButtonState;
}

About the following fragment:

  if (!ledState) {
    ledState = true;
    blinkState = true;
  } else {
    ledState = false;
    blinkState = false;
  }

Since you write ledState and blinkState in both clauses, and the condition is simple (so you don't need to put it in a temporary variable, you can replace this fragment by:

blinkState = !ledState;
ledState = !ledState;

Note you have to reverse the order as blinkState depends on ledState.

Answer 3

You never set the pinMode of buttonPin.

Normally it defaults to INPUT if you don't manually specify anything, and I assume from your results that you don't have an external pullup or pulldown resistor. Therefore you need to specify pinMode(buttonPin, INPUT_PULLUP); and make sure that your switch or button connects the input pin to ground when activated (which means that "active" will be LOW).

Or, if you do have an external pullup or pulldown resistor then it sounds like it may not be wired corrcetly.

Answer 4

Your blink code is only active

if (newButtonState != oldButtonState) 

so it won't blink (by itself)

To toggle blinking, just change it e.g. like this

void loop() {
  bool newButtonState = digitalRead(buttonPin);
  if (newButtonState != oldButtonState) { 
    // button state has changed
    delay(5);  // debounce
    oldButtonState = newButtonState; // run only once per button push
    if (newButtonState == HIGH) {
      // toggle blink state
      blinkState = ! blinkState;
    } 
  }

  if (blinkState) {
     // BlinkWithoutDelay
     unsigned long newTime = millis();
     if (newTime - oldTime >= 250) {
        state = !state;
        oldTime = newTime;
        digitalWrite(redLed, state);
     }
  } else {
     // no Blinking : just leave redLed as is
  }
}

Now please clean up the global variables: some are not required globally, some are not required at all.