I made a panel of buttons which run commands when pushed. In order to not have the command run on every loop if the button is held down, I am comparing the button to a previous state and only run if it is not, like so:
buttonStateR1 = digitalRead(redButton1);
if (buttonStateR1 != lastButtonStateR1) {
if (buttonStateR1 == LOW) {
startFlash();
Serial.println('R1');
}
}
lastButtonStateR1 = buttonStateR1;
This works quite well for only a couple of buttons. Is there a more effective method to use when there are 20 or so buttons? Having 20 or so of these statements does work but it get unwieldy and hard to maintain. Luckily each button acts the same (only needs to know that it has been pressed, not that it is held down or held down for a certain period of time, etc).
This is the full code and setup with 4 buttons and a toggle to turn it all on or off:
// LED output mappings
int greenLED = 0;
int redLED = 1;
// Pin input mappings
const int redButton1 = 5;
const int redButton2 = 3;
const int blackButton1 = 4;
const int blackButton2 = 6;
const int safetyToggle = 2;
// Button states
int buttonStateR1 = HIGH;
int lastButtonStateR1 = HIGH;
int buttonStateR2 = HIGH;
int lastButtonStateR2 = HIGH;
int buttonStateB1 = HIGH;
int lastButtonStateB1 = HIGH;
int buttonStateB2 = HIGH;
int lastButtonStateB2 = HIGH;
// Serial reading
String inputString = "";
boolean stringComplete = false;
// Helper functions
void turnOn(int pin) {
digitalWrite(pin, HIGH);
}
void turnOff(int pin) {
digitalWrite(pin, LOW);
}
void cycleLED(int led, int time){
turnOn(led);
delay(time);
turnOff(led);
delay(time);
}
void startFlash() {
cycleLED(greenLED, 50);
cycleLED(redLED, 50);
cycleLED(greenLED, 50);
cycleLED(redLED, 50);
}
void successFlash() {
cycleLED(greenLED, 50);
cycleLED(greenLED, 50);
}
void failureFlash() {
cycleLED(redLED, 50);
cycleLED(redLED, 50);
}
void setup() {
pinMode(greenLED, OUTPUT);
pinMode(redLED, OUTPUT);
digitalWrite(greenLED, LOW);
digitalWrite(redLED, LOW);
// INPUT_PULLUP sets input buttons when unconnected to high
// this means that the 'pressed' state outputs low
pinMode(redButton1, INPUT_PULLUP);
pinMode(redButton2, INPUT_PULLUP);
pinMode(blackButton1, INPUT_PULLUP);
pinMode(blackButton2, INPUT_PULLUP);
pinMode(safetyToggle, INPUT_PULLUP);
Serial.begin(9600);
startFlash();
}
void loop() {
if (digitalRead(safetyToggle) == LOW) { // 'safety' toggle
// Button Red 1
buttonStateR1 = digitalRead(redButton1);
if (buttonStateR1 != lastButtonStateR1) {
if (buttonStateR1 == LOW) {
startFlash();
Serial.println('R1');
}
}
lastButtonStateR1 = buttonStateR1;
// Button Red 2
buttonStateR2 = digitalRead(redButton2);
if (buttonStateR2 != lastButtonStateR2) {
if (buttonStateR2 == LOW) {
startFlash();
Serial.println('R2');
}
}
lastButtonStateR2 = buttonStateR2;
// Button Black 1
buttonStateB1 = digitalRead(blackButton1);
if (buttonStateB1 != lastButtonStateB1) {
if (buttonStateB1 == LOW) {
startFlash();
Serial.println('B1');
}
}
lastButtonStateB1 = buttonStateB1;
//Button Black 2
buttonStateB2 = digitalRead(blackButton2);
if (buttonStateB2 != lastButtonStateB2) {
if (buttonStateB2 == LOW) {
startFlash();
Serial.println('B2');
}
}
lastButtonStateB2 = buttonStateB2;
}
if (stringComplete) {
if (inputString.equals("OK")) {
successFlash();
}
else {
failureFlash();
};
inputString = "";
stringComplete = false;
}
}
void serialEvent() {
while (Serial.available()) {
char inChar = (char)Serial.read();
inputString += inChar;
if (inChar == '\n') {
stringComplete = true;
}
}
}
1 Answer 1
There's a few things about this that could potentially be improved here.
C++ related
You can improve the C++ code here in a few ways. If you have access to the C++11 language features I'd suggest replacing:
// Pin input mappings
const int redButton1 = 5;
const int redButton2 = 3;
const int blackButton1 = 4;
const int blackButton2 = 6;
const int safetyToggle = 2;
With a typesafe enum:
enum class InputPinMappings: uint8_t{
redButton1 = 5,
redButton2 = 3,
blackButton1 = 4,
blackButton2 = 6,
safetyToggle = 2
}
I'd do the same for the LED pin mappings too.
Platform related
Most of the biggest imrpovements you can get here come from knowing the hardware platform better.
Get to know the hardware platform
In the arduino the GPIO pins are mapped to a hardware address where each pin is a bit in a block of pins. The Arduino UNO for example is a platform that uses the Atmega328p to power it. If you look at the datasheet for the Atmega 328p www.atmel.com/Images/doc8161.pdf you'll see how the pins are mapped to the physical addresses.
Specifically you want to look at the register summary on the datasheet.
You'll see that Pins are grouped into blocks PINB, PINC, etc. These blocks are one 8bit byte in the memory with each pin represented by a bit in those bytes.
You can create a helper struct to map to the individal pins if you would like to encapsulate the bit manipulations, I'd highly recommend putting as much of that as possible at compile time though as you don't want to incur the runtime performance hit from doing that if you can at all avoid it. (If you improve your code and would like to look into some compile time hackery for this platform perhaps that would make for a good followup question, I think it's a bit out of the scope of this question though)
Use datatypes that match the underlying system
Currently you are using an int type to store each pin, now that you know how these pins are represented you can see that this might be wasteful in a few ways.
You can represent your buttons state more effeciently by packing them into a single 8bit integers as represented by the actual bit masks of the pins. You probably want to group the buttons into variables based on which Pin grouping they are on.
For example:
uint8_t ButtonsOnPINDmask = PIND7 | PIND3; //button on PIND7 and PIND3
This variable can represent which buttons you have connected to PIND.
We can then use this to our advantage when checking which buttons have changed state. We only need to do 1 integer comparison per block to see if anything has changed as opposed to having to check for each pin individually. Once you have seen that something has change then you can look through to find which buttons have changed state one bit at a time.
uint8_t previousPINDstate;
if(previousPINDstate == *PIND){
//nothing changed
}else{
//check for changes
previousPINDstate = *PIND;
}
(Note that this should be in an atomic/synchronized block or you risk run into some nasty problems with the state of PIND changing in between the initial check for the state change and then using the PIND value)
Now this brings me to an important point, if you are checking the status of the buttons every time you go through the main loop you are likely wasting a lot of cycles. You are going to be doing a lot of redundant checks and wasting a bunch of cycles on this.
Use interrupts
Luckily the chip designers have taken pin change considerations into account and they provide pin change interrupts for you for this exact reason. So what I would suggest is using the pin change interrupt capabilities of the underlying hardware to inform you of when a pin change occurs.
You can hook up an interrupt that gets fired when the state of a pin gets changed which will then call your hander function. Specifically you want to look into the interrupts such as PCINT0_vect. The interrupts available will depend on what Arduino hardware you are running this on, so go check out the documentation.
By using the ISR approach you will save a lot of main loop cycles, so it's worth looking into this.