Toggle button with timed on off help

Having trouble with a timed based project.

With every button press, the code should switch between one or the other led. Once that led is on, it should turn off after one second.

const int SW = 2;     // the number of the pushbutton pin
const int LED1 = 12; 
const int LED2 =  13;  

const unsigned long debounce = 25; // debounce timer
const unsigned long ledofftime = 1000;// turn off after this time has passed.

unsigned long currentTime = 0; 
unsigned long buttonstatechangetime = 0; 
unsigned long ledontime = 0; 

bool ledstate1 = true;
bool ledstate2 = false;

bool buttonwaspressed = false;

void setup() {

  //SET PINS:
  pinMode(SW, INPUT_PULLUP);
  pinMode(LED1, OUTPUT);
  pinMode(LED2, OUTPUT);

}//end setup

void loop() {

  currentTime = millis();

  checkbutton();
  led();

}//End.

void checkbutton() {

  bool buttonispressed = digitalRead(SW) == LOW;  // Active LOW

  // Check for button state change debounce.
  if (buttonispressed != buttonwaspressed &&
      currentTime  -  buttonstatechangetime > debounce) {
    // Button state has changed.
    buttonwaspressed = buttonispressed;
    if (buttonwaspressed) {
      ledstate1 = ! ledstate1;
      ledstate2 = ! ledstate2;
    }
    buttonstatechangetime = currentTime;
  }
}//end

void led() {

  digitalWrite(LED1, ledstate1);
  if (currentTime - ledontime >= ledofftime) {
    digitalWrite(LED1, false);
  }

  digitalWrite(LED2, ledstate2);
  if (currentTime - ledontime >= ledofftime) {
    digitalWrite(LED2, false);
  }
  ledontime = currentTime;

}//end

All it's doing is just toggling between the 2 leds, but not switching off after the second has passed.

Tinkercad to simulate and show the wiring of the project.

Seems simple, but can't get the phrasing right.
Thank you!

No, not at all.
It is also very hard to describe this in a logical way.
This would be an excellent question for school. It seems simple but it puts your brain at work. You have to be able to see the logic.

If both leds are off for a few days and the button is pressed, do you want to start with the first led, or the other led regarding to the led that was last on a few days ago ?

Can you describe the initial state (when resetting or powering up the Arduino board). I would assume that both leds are off. If one led is already on, should that turn off after the timer has finished ?

A millis-timer runs typically on its own in the main level of the loop().
With every button press, start the timer, and let the timer turn the led(s) off.
You can keep the ledstate1 and ledstate2, but you can also make a variable that is 0, 1 or 2 (no leds, first led, second led).
Here is an example that shows how to start a timer and how the timer turns itself off: millis_single_delay.ino

The problem with your code is that you use a digitalWrite() to turn the led off but don't change the ledstate variable. However, that's not good enough. Can you make better code that turns on and off the led by calling digitalWrite() just once instead of hundred times per second.

I've encountered a similar or same issue with one LED. It's blinking, and I want to turn it on or off. But it's not enough to just halt the blink code, because it could be in the on state when it's commanded to do that. So you need some additional logic to ensure that it's set to some defined state when the blinking stops.

Does that sound similar to your problem?

There are many potential solutions to it.

Upon powerup, led1 is on. Since ledstate1 is true i would assume that the code would run and start the timer to run out and turn off .

the flow is want is: Powerup -> led1 on for one second, led2 off -> until button is pressed, both leds are off -> when button is pressed, led2 on for one second and then off -> both leds off until button is pressed -> back to led1... and so on.

so like and empty state so everything can reset after the button press?

There is no check or guard on the first digitalWrite, it's unconditional. Since you also unconditionally set ledontime = currentTime at the end of the function, the conditional statements never become true because the difference between currentTime and ledontime is never greater than a few milliseconds.

Unless I've missed something...

BTW this is not the approach I would take at all.

An outline of one possibility:

Debounce your switch.

When the switch becomes pressed  (IDE/file/examples/State Change Detection) toggle a boolean variable.  Call it buttonWasPressed.

Use SCD again to detect when buttonWasPressed goes false-to-true and also when it goes true-to-false.

Use the t-f edge of buttonWasPressed to trigger a timer for LED1.  Use the f-t edge to trigger a timer for LED2.  Or vicey versy.

There might still be holes in your description.
If a led is on, and the button is pressed (while the led is still on), should it advance to the next led ?

I wanted to see how many variables I would need when there are 4 blinking leds. It turns out that I need a many variables. Maybe too many :woozy_face:
This is not me knowing how to write this sketch. To be honest, I randomly threw in the code that I needed and then I moved around the code lines until it worked :face_with_raised_eyebrow:

#define NUM_LEDS 4
int ledPins[NUM_LEDS] = { 13, 12, 11, 10};

int currentLed = 0;                         // this is also the index to the array with pins
int ledState = LOW;                         // used the blink the active current led
bool ledActive;                             // is the led blinking and the timeout active ?
unsigned long timeStampButton = 0;          // remember moment of last button press
unsigned long previousMillisBlink;          // used to blink the led
const unsigned long timeOut = 2000UL;       // timeout for millis-timer
const unsigned long blinkInterval = 150UL;  // interval for blinking
const unsigned long debounceTime = 20UL;    // timing for debouncing

const int buttonPin = 2;                    // a button is connected to pin 2 and GND
int lastButtonState = HIGH;                 // idle high

void setup() 
{
  for( int i=0; i<NUM_LEDS; i++)
    pinMode( ledPins[i], OUTPUT);           // all the leds are outputs

  pinMode( buttonPin, INPUT_PULLUP);

  // Set initital state, turn first led on, start millis-timer
  currentLed = 0;
  timeStampButton = millis();
  ledActive = true;
}

void loop() 
{
  unsigned long currentMillis = millis();

  int buttonState = digitalRead( buttonPin);

  if( buttonState != lastButtonState && currentMillis - timeStampButton >= debounceTime)
  {
    lastButtonState = buttonState;
    timeStampButton = currentMillis;        // timestamp of last valid button press

    if( buttonState == LOW)                 // low is active
    {
  
      // The current led could be on
      if( ledActive && ledState == HIGH)
        digitalWrite( ledPins[currentLed], LOW);

      // advance to the next led
      currentLed++;
      if( currentLed >= NUM_LEDS)
        currentLed = 0;

      ledActive = true;        
    }
  }

  if( ledActive)
  {
    if( currentMillis - timeStampButton >= timeOut)
    {
      // turn this led off
      digitalWrite( ledPins[currentLed], LOW);
      ledActive = false;           // turn millis-timer off
    }
    else if( currentMillis - previousMillisBlink >= blinkInterval)
    {
      previousMillisBlink = currentMillis;

      ledState = ledState == HIGH ? LOW : HIGH;   // toggle led state
      digitalWrite( ledPins[currentLed], ledState);
    }
  }
}

Try this sketch in Wokwi simulation:

You should find you own way to make your own sketch. I hope that you see that I put the millis-timer in the main level of the loop() and the millis-timer runs on its own.

Hello
Check out this sketch. The sketch is using an array to collect all relevant information like pin addresses, states and times. Inside the loop() a button debouncing, state change detection and led action will take place on this information.

/* BLOCK COMMENT
  ATTENTION: This Sketch contains elements of C++.
  https://www.learncpp.com/cpp-tutorial/
  https://forum.arduino.cc/t/toggle-button-with-timed-on-off-help/927965
*/
#define ProjectName "Toggle button with timed on off help"
// HARDWARE SETTINGS
// YOU MAY NEED TO CHANGE THESE CONSTANTS TO YOUR HARDWARE AND NEEDS
constexpr byte ButtonPin {A0};      // portPin o---|button|---GND
constexpr byte Led1Pin {3};         // portPin o---|220|---|LED|---GND
constexpr byte Led2Pin {5};         // portPin o---|220|---|LED|---GND
#define OutPutTest
// CONSTANT DEFINITION
enum {Button_};
enum {Led1, Led2};
enum {One, Two};
constexpr byte Input_[] {ButtonPin};
constexpr byte Output_[] {Led1Pin, Led2Pin};
constexpr  unsigned long OutPutTestTime {1000};
// VARIABLE DECLARATION AND DEFINITION
unsigned long currentTime;
struct TOOGLEBUTTON {
  byte pin;
  bool statusQuo;
  byte led [sizeof(Output_)];
  unsigned long stamp;
  unsigned long duration;
} toogleButton {Input_[Button_], false, {Output_[Led1], Output_[Led2]}, 0, 20};
// -------------------------------------------------------------------
void setup() {
  Serial.begin(9600);
  Serial.println(F("."));
  Serial.print(F("File   : ")), Serial.println(__FILE__);
  Serial.print(F("Date   : ")), Serial.println(__DATE__);
  Serial.print(F("Project: ")), Serial.println(ProjectName);
  pinMode (LED_BUILTIN, OUTPUT);  // used as heartbeat indicator
  //  https://www.learncpp.com/cpp-tutorial/for-each-loops/
  for (auto Input : Input_) pinMode(Input, INPUT_PULLUP);
  for (auto Output : Output_) pinMode(Output, OUTPUT);
#ifdef OutPutTest
  // check outputs
  for (auto Output : Output_) digitalWrite(Output, HIGH), delay(OutPutTestTime);
  for (auto Output : Output_) digitalWrite(Output, LOW), delay(OutPutTestTime);
#endif
  digitalWrite(toogleButton.led[Led1], HIGH);
}
void loop () {
  currentTime = millis();
  digitalWrite(LED_BUILTIN, (currentTime / 500) % 2);
  // debounce button
  if (currentTime - toogleButton.stamp >= toogleButton.duration) {
    toogleButton.stamp = currentTime;
    // check state change
    bool stateNew = !digitalRead(toogleButton.pin);
    if (toogleButton.statusQuo != stateNew) {
      toogleButton.statusQuo = stateNew;
      // take action on change
      if (stateNew) {
      digitalWrite(toogleButton.led[One], !digitalRead(toogleButton.led[One]));
      digitalWrite(toogleButton.led[Two], !digitalRead(toogleButton.led[Two]));
      }
    }
  }
}

Have a nice day and enjoy coding in C++.

yes sorry, exactly that. i shouldve specified.

this sketch is very nice actually! im trying to edit out the blinking motion it has to just one quick on to off. otherwise the way this toggles is great

im unfamiliar with this line and when i try to edit it to just high to low, the leds dont turn on at all.

ledState = ledState == HIGH ? LOW : HIGH;   // toggle led state
      digitalWrite( ledPins[currentLed], ledState);

vs what i edited:

if ( ledActive)
  {
    if ( currentMillis - timeStampButton >= timeOut)
    {
      // turn this led off
      digitalWrite( ledPins[currentLed], LOW);
      ledActive = false;           // turn millis-timer off
    }

    previousMillisBlink = currentMillis;

    ledState = ledState == HIGH, LOW;   // toggle led state
    digitalWrite( ledPins[currentLed], ledState);
  }
}

im sure that last part is wrong

thank you for the example!

i tried doing:

void checkbutton() {

  bool buttonispressed = digitalRead(SW) == LOW;  // Active LOW

  // Check for button state change debounce.
  if (buttonispressed != buttonwaspressed &&
      currentTime  -  buttonstatechangetime > debounce) {
    // Button state has changed.
    buttonwaspressed = buttonispressed;
    if (buttonwaspressed) {
      ledstate = ! ledstate;
    }
    buttonstatechangetime = currentTime;
  }
}//end

void led() {
  if (ledstate == true) {
    digitalWrite(LED1, HIGH)
    ledontime = currentTime;
  } if (currentTime - ledontime > ledofftime) {
    digitalWrite(LED1, LOW);
  }
}//end

it would even shut off that one :sweat_smile:

This is a if-else-construction:

ledState = ledState == HIGH ? LOW : HIGH;

In words: is ledState HIGH ? then the result is LOW else the result is HIGH.
In this case the result goes into the ledState variable.

It is the same as this:

if( ledState == HIGH)
  ledState = LOW;
else
  ledstate = HIGH;

But you need the ? and the :

Use a normal if-else if it looks weird.

Hi guys,
I really like keyboards and bit processing, maybe my example can help ? voila:

  #define KeyOne          B00000100                          // PortD (pin 2) to the Ground (0V) via button
  #define KeysAll         B00000100                          // All Keys (on an input Port, 8 keys possible)
  byte    PPImuSta;                                          // (Keys state)
  byte    PPImuMem;                                          // 
  byte    PPImuFro;                                          // (Keys state)
  int     LedDurationOne;                                    // timer Led 1
  int     LedDurationTwo = 100;                              // timer Led 2  
  boolean toggleLeds;                                        // select Led One or Two 
  unsigned long previousmillis;                              // debounce and led duration timer


void setup(void) {                                           //
  PORTD |= KeysAll;                                          // internal pullups on pins "KeysAll"
  DDRB  |= B00110000;                                        // pins 12 and 13 (PB4 PB5) as output
}                                                            //  
  
void loop(void) {                                            //
  KeysLeds();                                                //
}                                                            //
  
void KeysLeds(void) {                                        // 1..8 Keys (input Port)
  if (millis() - previousmillis > 10) {                      // 10 ms for debounce and Leds duration
    previousmillis = millis();                               //
                                                             //
    byte iii = ~PIND & KeysAll;                              // reading (inverted) PORTD (pin 2, pin...)  
    byte jjj = PPImuSta;                                     // 
    PPImuSta = iii & PPImuMem;                               // Keys state : pressed (maintained)
    PPImuMem = iii;                                          // 
    PPImuFro = (jjj ^ PPImuSta) & PPImuSta;                  // Keys state : first press (just changed)

    
    // here you can play and adjust the code : ***********************************//// when a key is pressed :
    if (PPImuFro & KeyOne) {                                                      //// first press (front) on KeyOne 
      toggleLeds = !toggleLeds;                                                   //// swap Led1 <-> Led2
      if (toggleLeds) LedDurationOne=100; else LedDurationTwo=100;                //// 100 * 10 ms = 1 s 
    }                                                                             ////
    if (LedDurationOne) {LedDurationOne--; digitalWrite(12, (LedDurationOne>0));} //// Leds management : ON / OFF
    if (LedDurationTwo) {LedDurationTwo--; digitalWrite(13, (LedDurationTwo>0));} //// Leds management : ON / OFF
  }                                                                               ////
}                                                                                 ////
                                      ////