I have a routine that looks for input from one of 5 buttons. It was originally written such that on detecting a button press it waited for the button to be released before returning a flag indicating that a button had been pressed and released and the number of the button. This was as follows :
void lookForButton() //return btnPressed=true and btnNum if a button is pressed and released
{
btnPressed = false;
for (int btn=8;btn<=12;btn++)
{
if (digitalRead(btn) == HIGH) // check for a button being pressed
{
btnPressed = true;
btnNum = btn; //save number of button pressed
}
}
if (btnPressed == true)
{
while (digitalRead(btnNum) == HIGH) //wait for the button to be released
{
delay(100); //debounce
}
}
}
Being aware that I might not want the main program to stall at some time in the future I have re-written the routine as follows :
void lookForButton() //return btnReleased=true and btnNum if a button is pressed and released
{
boolean static btnPressed = false; //note STATIC to preserve value for next call
for (int btn=8;btn<=12;btn++)
{
if (digitalRead(btn) == HIGH) // check for a button being pressed now
{
btnPressed = true; //flag that we have found a button pressed and need to check for release
btnNum = btn; //save number of button pressed (could exit the loop at this point)
}
}
if (btnPressed == false) //no button pressed
{
btnReleased = false; //not pressed so it can't have been released so flag it and return
return;
}
currentBtnState = digitalRead(btnNum); //get current state of the button
if (currentBtnState != prevBtnState) //button state has changed. Did it bounce or was it released ?
{
if ((millis() - prevBtnTime > 100) && (digitalRead(btnNum) == LOW)) //button released and debounce time has elapsed
{
Serial.println("got a button"); //***********************DEBUGGING CODE*********************
btnPressed = false; //reset the flag for next time that it is needed
btnReleased = true; //flag release of button for action
}
else
{
Serial.println("no valid button yet"); //***********************DEBUGGING CODE*********************
prevBtnTime = millis(); //button is still pressed or debounce time has not elapsed
prevBtnState = currentBtnState; //remember the current button state and current time
}
}
}
prevBtnTime is defined as a global long initially set to 0
currentBtnState and prevButtonState are defined as global ints initially set to LOW
None of these variables are changed anywhere else in the sketch
The routine appears to work but the debug Serial.print only outputs "no valid button yet" the first time that the routine is used after uploading a sketch or resetting the Arduino so something is wrong and I would be grateful for advice as I suspect my logic is wrong somewhere.
void lookForButton() //return btnReleased=true and btnNum if a button is pressed and released
This function does not return anything. The comment is WRONG. It SETS some global values. It SHOULD return btnNum, which should NOT be a global.
for (int btn=8;btn<=12;btn++)
{
if (digitalRead(btn) == HIGH) // check for a button being pressed now
{
btnPressed = true; //flag that we have found a button pressed and need to check for release
btnNum = btn; //save number of button pressed (could exit the loop at this point)
}
}
There should be a break in this loop. Once you found A switch pressed, there is no reason to check the others.
prevBtnState = currentBtnState; //remember the current button state and current time
This should happen EVERY time currentBtnState is set, not inside nested ifs.
Point taken about the misleading comment and I had already spotted the possibility of breaking out of the loop as noted in my comment.
I have now fixed the logic of where prevBtnState = currentBtnState; occurs
Fixed code as follows :
void lookForButton() //sets btnReleased=true and btnNum if a button is pressed and released
{
boolean static btnPressed = false; //note STATIC to preserve value for next call
for (int btn=8;btn<=12;btn++)
{
if (digitalRead(btn) == HIGH) // check for a button being pressed now
{
btnPressed = true; //flag that we have found a button pressed and need to check for release
btnNum = btn; //save number of button pressed and exit the loop
break;
}
}
if (btnPressed == false) //no button pressed
{
btnReleased = false; //not pressed so it can't have been released so flag it and return
return;
}
prevBtnState = currentBtnState; //remember the current button state and current time
currentBtnState = digitalRead(btnNum); //get current state of the button
if (currentBtnState != prevBtnState) //button state has changed. Did it bounce or was it released ?
{
if ((millis() - prevBtnTime > 100) && (digitalRead(btnNum) == LOW)) //button released and debounce time has elapsed
{
Serial.println("got a button"); //***********************DEBUGGING CODE*********************
btnPressed = false; //reset the flag for next time that it is needed
btnReleased = true; //flag release of button for action
}
else
{
Serial.println("no valid button yet"); //***********************DEBUGGING CODE*********************
prevBtnTime = millis(); //button is still pressed or debounce time has not elapsed
}
}
}
Some sample output :
no valid button yet
got a button
no valid button yet
got a button
no valid button yet
no valid button yet
no valid button yet
no valid button yet
no valid button yet
got a button
The point that I was trying to make, perhaps not clearly enough, is that the problem is fixed. The debug output and the working of the routine with the routines(s) calling it are exactly as expected.
btnPressed is set back to false once a button is released and de-bounce time has elapsed in this portion of the routine.
if ((millis() - prevBtnTime > 100) && (digitalRead(btnNum) == LOW)) //button released and debounce time has elapsed
{
Serial.println("got a button"); //***********************DEBUGGING CODE*********************
btnPressed = false; //reset the flag for next time that it is needed
btnReleased = true; //flag release of button for action
}
else
{
Serial.println("no valid button yet"); //***********************DEBUGGING CODE*********************
prevBtnTime = millis(); //button is still pressed or debounce time has not elapsed
}
This was in my original millis() version of the routine posted here and has not needed to be changed.
It's amazing how easy it is to miss the obvious things as I know only too well.
An earlier version of the routine mistakenly had for (int btn=8;btn<=13;btn++) in the portion that reads the buttons when I really meant for (int btn=8;btn<13;btn++). This routine is part of an alarm clock program that I have written to stretch my understanding of the Arduino.
All was OK until I added the code to set off the alarm and turned on the LED on pin 13 when the alarm was triggered. It took me ages (and several Serial.prints scattered throughout the program to find out what was wrong. That reminds me. I meant to change the code to
for (int btn = 8; btn <= 12; btn++)
to make it easier to read. I prefer to use <= rather than < because to me the limits of the loop are more obvious. What do others think ?