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.
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.
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
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
#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]));
}
}
}
}
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);
}
}
