please help an idiot out with a loop led question...please

Hi as you will tell I am new to this but have undertaken a minor Halloween project that I though would be simple....and am stuck

The code below is to light 3 LED's one constantly and the other 2 in differing flash sequences. There is a function called BlinkLed in a while loop triggered by a button press.

This all worked perfectly. Press the button, lights sequence as expected ..push it again and they stop. The problem was the I pushed it again they stopped flashing but stayed in whatever state they were on. I thought I would tidy the code with three digitalwrite (led,LOW) lines to turn them all off.

Weirdly this didn't do the trick. Now when the button is pressed, the green LED lights as required but the 2 flashing ones, that called the function, came on at the right time but then straight off...they didn't stay on for the correct length of time.

When the button is pressed again they all go off.

The whole flashing stuff thing is in a while loop but I can't understand why the digitalwrite LOW lines which are outside the loop seems to affect a function inside the loop?

Can anyone help please...would be very grateful

#define redLed 13 //
#define yellowLed 12
#define greenLed 11//pin for each led

unsigned long previousMillis[2]; //[x] = number of leds
int buttonState = 0;

const byte buttonPin = 2;

void setup() {
pinMode(redLed, OUTPUT);
pinMode(yellowLed, OUTPUT);
pinMode(greenLed, OUTPUT);

buttonState = digitalRead(buttonPin);

pinMode(buttonPin, INPUT);

}

void loop() {

buttonState = digitalRead(buttonPin);

while (buttonState == HIGH) {
digitalWrite(greenLed,HIGH);

BlinkLed(redLed, 2000, 0);

BlinkLed(yellowLed, 1000, 1);
buttonState =digitalRead(buttonPin);

}
{
digitalWrite(greenLed,LOW);

digitalWrite(redLed,LOW);
digitalWrite(yellowLed,LOW);
}
}

void BlinkLed (int led, int interval, int array){

//(long) can be omitted if you dont plan to blink led for very long time I think
if (((long)millis() - previousMillis[array]) >= interval)
{

previousMillis[array]= millis(); //stores the millis value in the selected array

digitalWrite(led, !digitalRead(led)); //changes led state
}
}

I think you have too many braces. I cleaned up your code a bit. This is untested, but should work.

const byte redLed = 13;//pin for each led
const byte yellowLed = 12;
const byte greenLed = 11;
const byte buttonPin = 2;

unsigned long previousMillis[2]; // number of blinking leds

void setup() {
  pinMode(redLed, OUTPUT);
  pinMode(yellowLed, OUTPUT);
  pinMode(greenLed, OUTPUT);
  pinMode(buttonPin, INPUT);
}


void loop() {
  while (digitalRead(buttonPin) == HIGH) {
    digitalWrite(greenLed, HIGH);
    blinkLed(redLed, 2000UL, 0);
    blinkLed(yellowLed, 1000UL, 1);
  }
  digitalWrite(greenLed, LOW);
  digitalWrite(redLed, LOW);
  digitalWrite(yellowLed, LOW);
}

void blinkLed (byte led, unsigned long interval, byte arrayIndex) {
  if (millis() - previousMillis[arrayIndex] >= interval)
  {
    previousMillis[arrayIndex] = millis(); //stores the millis value in the selected array
    digitalWrite(led, !digitalRead(led)); //changes led state
  }
}

Here it is with a momentary push-button action.

const byte RedLed = 13;//pin for each led
const byte YellowLed = 12;
const byte GreenLed = 11;
const byte ButtonPin = 2;

unsigned long ledTimestamp[2]; // number of blinking leds
int lastButtonState;
unsigned long buttonDebounceTimestamp;
byte toggleSwitch;

void setup() {
  pinMode(RedLed, OUTPUT);
  pinMode(YellowLed, OUTPUT);
  pinMode(GreenLed, OUTPUT);
  pinMode(ButtonPin, INPUT_PULLUP);// Keeps pin HIGH, make to ground to go LOW, external pullup more reliable
}


void loop() {
  pollButton();
  manageLEDs();
}

void pollButton() {
  int buttonState = digitalRead(ButtonPin);
  if (buttonState != lastButtonState && buttonState == LOW) {// transition is HIGH to lOW
    if (millis() - buttonDebounceTimestamp >= 50UL) {// 50UL = 50ms = time for debounce
      toggleSwitch = !toggleSwitch;
      lastButtonState = buttonState;
      buttonDebounceTimestamp = millis();
    }
  }
  else lastButtonState = buttonState;
}

void manageLEDs() {
  if (toggleSwitch) {
    digitalWrite(GreenLed, HIGH);
    blinkLed(RedLed, 2000UL, 0);
    blinkLed(YellowLed, 1000UL, 1);
  }
  else {
    digitalWrite(GreenLed, LOW);
    digitalWrite(RedLed, LOW);
    digitalWrite(YellowLed, LOW);
  }
}

void blinkLed (byte led, unsigned long interval, byte arrayIndex) {
  if (millis() - ledTimestamp[arrayIndex] >= interval)
  {
    ledTimestamp[arrayIndex] = millis(); //stores the millis value in the selected array
    digitalWrite(led, !digitalRead(led)); //changes led state
  }
}

hi Perehama: thanks for the reply unfortunately that doesn't work...although it behaves differently. one push the LEDS go on but the red and yellow still flicker at the right time rather than stay on. Second push they all go off...but weirdly third push nothing lights and no subsequent pushes do anything until a reset...odd. BTW the button is latching rather than momentary

andigoodman:
BTW the button is latching rather than momentary

Did you try the first code?

yes that's the one :slight_smile:

A few questions about the electrical setup. The button mechanically latches closed on one press, then opens on the seconds press, yes? Then, closed on the third press, open on the 4th...?
Do you have a resistor pulling the input pin HIGH or LOW when the button is open?
Does the resistor pull it HIGH or LOW?
When the button is closed, does it pull HIGH or LOW?

yes the button latches mechanically as you describe. There is a resistor between the LED and the digital pin. The other side of the led is to GND.

I have put some serial logging in which has helped. When he button is pushed in, the LEDS work perfectly and the button state shows as 1 (high) in the serial console...however when the button is pushed to off, the serial console shows it varying between 0 and 1 ...which I guess is why it is breaking out of the while loop...weird

sorry to have wasted your time....the switch was wired wrong...thank you so much for pointing me into the direction of the circuit rather than the code....appreciate it!!! thank you