Else command ends a nestled loop, but not how I expected...

Hi all,

I am new to arduino, coming over from parallax and BASIC language, so please forgive the basic nature of this topic, but...

I am attempting to nestle a blinking LED without delay loop inside an If/Else digital button press command.

I hoped letting go of the button would stop the blinking loop with the output pin LOW.
Instead, letting go of the button stops the loop, but continues with whatever state the blinking is in --
e.g. letting go of the button while output is HIGH, output continues to be HIGH.

While I am sure there are more efficient ways to code this, I am just trying to familiarize myself with the new language.
Thanks in advance:

Any advice? See code below:

const int buttonPin = 2;
const int ledPin = 12;


int buttonState = 0;
int ledState = LOW;

unsigned long previousMillis = 0;

const long interval = 1000;

void setup() {
  pinMode(ledPin, OUTPUT);
  pinMode(buttonPin, INPUT);
}

void loop() {
  buttonState = digitalRead(buttonPin);
  unsigned long currentMillis = millis();
  
  if (buttonState == HIGH) {
    
    if (currentMillis - previousMillis >= interval) {
      previousMillis = currentMillis;

      if (ledState == LOW) {
        ledState = HIGH;
      } else {
        ledState = LOW;
      }

      digitalWrite(ledPin, ledState);
     } else {
      digitalWrite(ledState, LOW);
     }

   } else {
     ledState = LOW;
   }
 }

Always show us your wiring.
How is your switch wired?

BTW
Start getting into the habit of adding comments to your code.

Use "Ctrl T" to format your code so you can see the relation of the { }.

Good habit also to have { and } on separate lines.

.

Thanks LarryD,

I will start to implement those suggestions! The ctrl+t is already engaged, but I will keep the brackets on their own line and comment in line. In BASIC I was in the habit of commenting before and after the code in a block and using line numbers to indicate what I was referencing.

I am not at the same computer, but here is the image I wired from off the Button tutorial:

with the addition of a wire from pin 12 to a 220Ω resistor and LED in series to ground. Like I said, it follows the script to blink the LED and when I stop pressing the button while the LED is off it stays off.

The unexpected part is if I stop pressing the button while the LED is on, it stays on indefinitely.

In that else at the end, you set the variable ledState to LOW, but you never write that to the pin with any digitalWrite.

const unsigned long interval = 1000UL; //Good Habit to stay with unsigned long unless you need the RAM

As Deta_G mention update the LED

Maybe this is what you wanted?

const int buttonPin = 2;
const int ledPin = 12;


int buttonState = 0;
int ledState = LOW;

unsigned long previousMillis = 0;

const unsigned long interval = 1000UL; 

void setup()
{
  pinMode(ledPin, OUTPUT);
  pinMode(buttonPin, INPUT_PULLUP);
}

void loop()
{
  buttonState = digitalRead(buttonPin);
  unsigned long currentMillis = millis();

  if (buttonState == HIGH)
  {

    if (currentMillis - previousMillis >= interval)
    {
      previousMillis = currentMillis;

      if (ledState == LOW)
      {
        ledState = HIGH;
      }
      else
      {
        ledState = LOW;
      }

      digitalWrite(ledPin, ledState);
    }
  }

  //    else
  //    {
  //      digitalWrite(ledState, LOW);
  //    }

  else
  {
    ledState = LOW;
    digitalWrite(ledPin, ledState);
  }
}

Why waste the bytes on interval? I'd make it an int and save the two bytes. There's no reason why it would need to be 32 bit.

Why waste the bytes on interval? I'd make it an int and save the two bytes. There's no reason why it would need to be 32 bit.

For 1000 your are right.

Only argument is consistency.
As I mentioned, "unless you need the RAM"

vogtm2:
I hoped letting go of the button would stop the blinking loop with the output pin LOW.
Instead, letting go of the button stops the loop, but continues with whatever state the blinking is in --
e.g. letting go of the button while output is HIGH, output continues to be HIGH.

Generally, we don't "hope" that code will chose to do what we intended. Instead, we write code that does do what we intend.

Seeing as your code always puts the required state in ledState, move the digitalWrite to outside the if()

  if (buttonState == HIGH)
  {
    // stuff to make ledState blink , etc
  }
  else
  {
    ledState = LOW;
  }

  digitalWrite(ledPin, ledState);

Hi All,

Thank you for the tips!

Delta_G:
In that else at the end, you set the variable ledState to LOW, but you never write that to the pin with any digitalWrite.

Delta_G, you were right, that was exactly what I left out. Thanks for spotting it!

LarryD:
const unsigned long interval = 1000UL; //Good Habit to stay with unsigned long unless you need the RAM

...

Maybe this is what you wanted?...

LarryD, your code does exactly what I wanted...
In my quest to familiarize myself with the syntax, why did you use
pinMode(buttonPin, INPUT_PULLUP);
in the setup instead of
pinMode(buttonPin, INPUT);

PaulMurrayCbr:
Generally, we don't "hope" that code will chose to do what we intended. Instead, we write code that does do what we intend.

My background is not programming... I'm coming from an EE and Physics background. My experience is that our intentions are rarely translated into reality on the first draft/trial. Hope and optimism are important in the design & learning process!

Thanks again for all the advice!

To modify AutoFormat tool to your (BASIC ) liking check out formatter.conf file.
Jim

LarryD, your code does exactly what I wanted...
In my quest to familiarize myself with the syntax, why did you use
pinMode(buttonPin, INPUT_PULLUP);
in the setup instead of
pinMode(buttonPin, INPUT);

In your diagram, you have a pullup resistor the switch to +5 V so you 'do not' need to add INPUT_PULLUP to that line.

Note if you ever need a pull up resistor, you can turn on the internal one (~30K) with that syntax.

.