1

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;
}
  • Also it is not needed to add '== true' for comparing booleans, you can remove '== true' and instead of '== false' you put ! in front of the expression. – Michel Keijzers Apr 8 at 19:28
  • 1
    Thanks I didn't know that. I have seen a few sketches where they have used booleans without using "==" and I wondered if it's a different notation of if statements. But what if you are evaluating two booleans at once e.g. "if (ledState == true && blinkState == true)" , can you also write them without the == signs? – Zaffresky Apr 8 at 20:02
  • 1
    Yes than you can write if (ledState && blinkState), if you want to write if (ledState == true && blinkState == false) you can write if (ledState && !blinkState) – Michel Keijzers Apr 8 at 20:05
  • 1
    Cool. Learnt something new. Thanks – Zaffresky Apr 8 at 20:09
  • I added an answer to show another improvement. – Michel Keijzers Apr 8 at 20:12
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.

| improve this answer | |
  • Thanks for your very clear explanation. Also the suggestion to break the code makes it easier to follow. – Zaffresky Apr 9 at 20:05
  • I will try to follow this approach but all these state variables that are interdependent are new to me and at time confusing. I have also read about the concept of state machine and I wonder if it would be easier to code in such contexts – Zaffresky Apr 9 at 20:12
  • The state variables are not interdependent. You could see both variables to be of a different kind. While ledState directly describes the current output value of the LED pin, blinkState describes an action, which is a bit more abstract. Think of setting one foot before the other, which you could call one step. By changing a fictional state variable like footstate we could change, which foot is the front one, and thus do steps. Then the action "walking" is more abstract, since it is a succession of steps. walking describes the action of successively changing the front foot. – chrisl Apr 9 at 20:27
  • In this case a state variable will not be particular easier to implement. To be exactly: With this code you already have a state machine (as you handle state variables and act upon them). Just not one, that can easily be extended. You can change that to the more extendible version (on the web and on this side are many tutorials about FSMs), though that would be more to make it easier to extend the functionalities. With the current scope of the code you are fine with this structure. – chrisl Apr 9 at 20:30
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.

| improve this answer | |
0

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.

| improve this answer | |
  • There is a pull down resistor in the circuit. I have added the pinMode but it still doesn't work. I suspect it has to do with the conditional statements. I tried changing the "if ledState == true && blinkState == true" to a while and it does start blinking but then it doesn't turn off. I really want to understand what's wrong with my logic here. – Zaffresky Apr 8 at 19:55
0

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.

| improve this answer | |

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.