Using the debounce program from https://www.arduino.cc/en/Tutorial/Debounce I want to introduce a delayed action only when the output from the debounce results in a HIGH.
The debounce in question works as a toggle switch: press switch once and output (LED1) goes HIGH, press again and output goes LOW.
So if after debounce the output has been HIGH longer than a set delay, a second output (LED2) goes HIGH. When the switch is toggled again then both outputs need to go LOW.
In my case the delay for LED2 is not being reset when the switch is toggled, and LED1 and LED2 go both HIGH, and LOW, when the switch is toggled.
What is wrong in my code?
const int buttonPin1 = 12;
const int buttonPin2 = 13;
const int LED1 = 14;
const int LED2 = 16;
int ledState = HIGH;
int buttonState1;
int buttonState2;
int lastButtonState1 = LOW;
int lastButtonState2 = LOW;
unsigned long lastDebounceTime = 0;
unsigned long debounceDelay = 50;
unsigned long elapsedTime;
unsigned long startTime;
unsigned long currentTime;
void setup() {
pinMode(buttonPin1, INPUT);
pinMode(buttonPin2, INPUT);
pinMode(LED1, OUTPUT);
pinMode(LED2, OUTPUT);
Serial.begin(115200); // Setup Serial Communication.
digitalWrite(LED1, ledState);
}
void loop() {
int reading = digitalRead(buttonPin1);
unsigned long currentTime = millis();
unsigned long elapsedTime = currentTime - startTime;
if (elapsedTime > 10000 && ledState == HIGH) {
digitalWrite(LED2, HIGH);
}
else {
digitalWrite(LED2, LOW);
}
if (reading != lastButtonState1) {
lastDebounceTime = millis();
}
if ((millis() - lastDebounceTime) > debounceDelay) {
if (reading != buttonState1) {
buttonState1 = reading;
if (buttonState1 == HIGH) {
ledState = !ledState;
if (ledState == HIGH) {
unsigned long startTime = millis();
}
}
}
}
digitalWrite(LED1, ledState);
lastButtonState1 = reading;
}
The code in the Original Post looks very convoluted to me. I think the following does the same thing but IMHO in a more logical way. I deal with the debounce and the state-change check first. Then deal with the led timing. Note that I have changed the name of the variable reading for what I believe is greater clarity.
void loop() {
currentTime = millis();
// first debounce
if (currentTime - lastDebounceTime > debounceDelay) {
lastDebounceTime += debounceDelay;
buttonState1 = digitalRead(buttonPin1);
// then check the change of state
if (buttonState1 != prevButtonState1) {
if (buttonState1 == HIGH) {
ledState = !ledState;
if (ledState == HIGH) {
startTime = currentTime;
}
}
prevButtonState1 = buttonState1;
}
}
// then manage the LED
if (currentTime - startTime > 10000 and ledState == HIGH) {
digitalWrite(LED2, HIGH);
}
else {
digitalWrite(LED2, LOW);
}
}
EDIT to add ...
The final IF statement (or more particularly its ELSE clause) may not do what you expect because the ELSE will be triggered if either part of the IF is false - which is probably most of the time.
Robin2:
The code in the Original Post looks very convoluted to me. I think the following does the same thing but IMHO in a more logical way. I deal with the debounce and the state-change check first. Then deal with the led timing. Note that I have changed the name of the variable reading for what I believe is greater clarity.
void loop() {
currentTime = millis();
// first debounce
if (currentTime - lastDebounceTime > debounceDelay) {
lastDebounceTime += debounceDelay;
buttonState1 = digitalRead(buttonPin1);
// then check the change of state
if (buttonState1 != prevButtonState1) {
if (buttonState1 == HIGH) {
ledState = !ledState;
if (ledState == HIGH) {
startTime = currentTime;
}
}
prevButtonState1 = buttonState1;
}
}
// then manage the LED
if (currentTime - startTime > 10000 and ledState == HIGH) {
digitalWrite(LED2, HIGH);
}
else {
digitalWrite(LED2, LOW);
}
}
EDIT to add ...
The final IF statement (or more particularly its ELSE clause) may not do what you expect because the ELSE will be triggered if either part of the IF is false - which is probably most of the time.
...R
The final IF statement (or more particularly its ELSE clause) may not do what you expect because the ELSE will be triggered if either part of the IF is false - which is probably most of the time.
...R
How do I execute the else only once, ie once the LED2 is LOW, no further digitalWrite; and same for LED2 set to HIGH.
Because now this gets executed time and again.
Introduce ledState2 maybe, and get digitalWrite to execute dependent on ledState2?
How do I execute the else only once, ie once the LED2 is LOW, no further digitalWrite; and same for LED2 set to HIGH.
Do you really care if the digitalWrite()s happen over and over ? The Arduino certainly won't care
If you do, then introduce a boolean variable with a suitable name, perhaps ledHasBeenWrittenTo and set it to true when you write to the LED and make the writing to the LED dependant on the variable being false. Change the variable to false when you change the state of the LED
UKHeliBob:
Do you really care if the digitalWrite()s happen over and over ? The Arduino certainly won't care
If you do, then introduce a boolean variable with a suitable name, perhaps ledHasBeenWrittenTo and set it to true when you write to the LED and make the writing to the LED dependant on the variable being false. Change the variable to false when you change the state of the LED
I am not sure indeed; but I am writing this code for ESP8266, and maybe indeed there too it does not matter?
Would it make cleaner code? Execute faster? Use less memory?.. I am not yet sure enough where to draw the line when or when not to prevent tasks repeating themselves endlessly.
UKHeliBob:
No, it would be messier
Possibly, but there is no point trying to make it faster if it is already fast enough
Unlikely as there will be more instructions to execute
Fast enough is good enough
One particular use for the execution limitation is when data is to be sent to Thingspeak on a time-based schedule; to avoid flooding it is necessary to strictly send data only once per time-based condition.
brice3010:
One particular use for the execution limitation is when data is to be sent to Thingspeak on a time-based schedule; to avoid flooding it is necessary to strictly send data only once per time-based condition.
Agreed, but the context of this thread is controlling LEDs
Other circumstances will require different solutions.
brice3010:
One particular use for the execution limitation is when data is to be sent to Thingspeak on a time-based schedule; to avoid flooding it is necessary to strictly send data only once per time-based condition.
That sounds like slower rather than faster?
If you monitor the change of state of something it is easy to send messages only when the state changes.