#newb Could I simplify my code?

Posted by: @pete-bevan

The switches are momentary action SPST push button types and use the internal pull up resistor on the Arduino

Hello Pete, before i check your code, a first question : why not wire the "symetrical" push buttons in "parallel pairs" if you don't need to know from which side a button has been pushed : 1a and 1b coud share a pin, etc...

Posted by: @pete-bevan

1. Is there a better way to debounce without adding a lot of extra code?

2. Can I simplify this code, there seems to be a lot of repeated statements and similar code throughout the program?

Sometimes for a short program with limited goals its actually can be better in terms of readability to simply repeat similar code, but I have written a small program to mimic the setting and toggling of relays and LED's pins on button pressing for amusement and comparison.

First thing I should point out is that when using a lot of pins for lots of LED's then you should be looking at multiplexing.  Here is a link to an article on this 

https://www.instructables.com/id/Multiplexing-with-Arduino-and-the-74HC595/

You will find others with some googling.

I don't go into multiplexing and my program uses lots of pins and assumes the following.

As I understand it, you have five relays that can be be switched on or off.  The on/off state of each relay is indicated by 2 LED's on 2 panels. (so 4 LED's for each relay).  Each pair of LED's on a  panel represents one for the on state and one for the off state and the appropriate LED will be turned on or off to indicate the state of the relay.   Each relay has 2 momentary push button switches, one on each panel and the press of any one of the pair will toggle the relay and toggle the LED's on both panels.

As I though it would be handy to group together the pin assignments for all LED's related to each relay I created a structure that can hold the pin assignments and their current status (hight or low).

A button push relating to a relay then tiggers the relay and associated LED's to go high or low.

The trigger of a button push is of course achieved by making a pin go high or low.  But, as @ZeFerby indicates the two buttons associate with a relay can both share the same pin as we just need to know that the pin is triggered and do not care which button (or which panel) triggered it. 

The program needs to be alerted to the fact of a button push and which pin changed its state from high to low (or vice versa) There are several ways to achieve this, but the use of an Interrupt Service Routine is one good way, and is the way I chose for my program.

The wiring of the buttons has to be from its button pin to a shared interrupt pin.  So all buttons are wired to this shared pin and another pin.

In my test program I only allowed for 2 relays.  You will not doubt see how it could easily be expanded to cover more relays without much extra coding.  However you should not really go down this path and should consider multiplexing where just 3 pins can control a raft of LED's.  But as I'm locked down for the corona I enjoyed a couple of hours doing your program my way.  And there will be other ways of course.   At the end of the day you may have to come back to your program and its far better to have one you can read and understand, which may well not be what my code does for you.   However I think my code uses functions for better use of repeating routines.  The debouncing is all done in the interrupt routine.  There is a bunch of Serial.print statements that are not necessary, but used to check the program works without wiring up some real LED's

So here it is.

You have a lot of repeated statements that don't change anything.  For example pins 14 thru 17 only need to be defined as outputs once with your code from pinMode(LED2aPin, OUTPUT); thru  pinMode(LED2dPin, OUTPUT);

The next 12 lines after that can be deleted because they are just restating the same thing over and over.  Same thing with the pinModes: button1aPin and button3aPin are the same integer (6) so it only needs to be defined as INPUT_PULLUP once. And you write a led pin high and then a few lines later write it high again. This is all in the setup. So yes many duplicate lines of code can be eliminated.

The code I gave a short while ago was in an attached file that had to be downloaded.  Then I got to thinking thats not much good if browsing on a tablet and just how do I load code in those nice blue boxed that most use so I post the code again to see if I can succeed.  It does not contain anything that was not in the previously attached attached file.  I've just preview it, blue box and all, so thats how its done 😀 

 // a structure to goup all the LED pins, button pins, and relay pins associated with a relay
struct relays
  {
    int buttonPin;
    int P1_ledOn;
    int P1_ledOff;
    int P2_ledOn;
    int P2_ledOff;
    int relay;
  };

// 2 relay structures are created - values are set up in setup()
relays relay1[2];
relays relay2[2];
  
const int SharedInteruptPin = 2;  // all buttons attached to this pin and their respective button pins

// create an array to hold the button pins associated with each relay (increse number for more than 2 button pins)
int buttonPins[2];

unsigned long lastPress = 0;      // variabe to hold last time a button was pressed for debouncing

void setup() 
{
  // put your setup code here, to run once:
  // a serial monitor is set up for checking / debugging purposes - can be removed later
  Serial.begin(9600);

 // relay 1 pin assignments
  relay1[0].buttonPin = 6; // 2 buttons attached to this pin
  relay1[0].P1_ledOn = 10;
  relay1[0].P1_ledOff = 11;
  relay1[0].P2_ledOn = 12;
  relay1[0].P2_ledOff = 13;
  relay1[0].relay = 8;
// relay 1 initial status 
  relay1[1].P1_ledOn = LOW;
  relay1[1].P1_ledOff = HIGH;
  relay1[1].P2_ledOn = LOW;
  relay1[1].P2_ledOff = HIGH;
  relay1[1].relay = LOW;
 // relay 2 pin assignments  
  relay2[0].buttonPin = 7; // 2 buttons attached to this pin
  relay2[0].P1_ledOn = A0;
  relay2[0].P1_ledOff = A1;
  relay2[0].P2_ledOn = A2;
  relay2[0].P2_ledOff = A3;
  relay2[0].relay = 9;
// relay 2 initial status 
  relay2[1].P1_ledOn = LOW;
  relay2[1].P1_ledOff = HIGH;
  relay2[1].P2_ledOn = LOW;
  relay2[1].P2_ledOff = HIGH;
  relay2[1].relay = LOW;

// populate an array for easy iteration of button pins
  buttonPins[0] = relay1[0].buttonPin;
  buttonPins[1] = relay2[0].buttonPin;

// set all button pins to low and interupt pin to high to trigger interupt on button press  
  setButtonPinsToInterupt();   

// attach a function to an interupt and trigger on FALLING
  attachInterrupt(digitalPinToInterrupt(SharedInteruptPin), buttonInterrupt, FALLING);

// set pins to status found in the relays structures (2 relays in this case)
setPins(relay1[0],relay1[1]);
setPins(relay2[0],relay2[1]);

// print the relay pins status - not necessary of couse but I dont have anything wired up so this is a check.
printMePinsStatus("Relay1",relay1[0],relay1[1]);
printMePinsStatus("Relay2",relay2[0],relay2[1]);
} 

void loop() 
{
  // nothing to put in here!! its all governed by the interupts the push button generate.
}

//set LED / relay pins to desired state
void setPins(relays &myStructPins, relays &myStructStatus)
{
   
    digitalWrite(myStructPins.relay, myStructStatus.relay); 
    digitalWrite(myStructPins.P1_ledOn, myStructStatus.P1_ledOn);
    digitalWrite(myStructPins.P1_ledOff, myStructStatus.P1_ledOff);
    digitalWrite(myStructPins.P1_ledOn, myStructStatus.P1_ledOn);
    digitalWrite(myStructPins.P1_ledOff, myStructStatus.P1_ledOff);
    
}    

void printMePinsStatus(String RelayName, relays &myStructPins, relays &myStructStatus)
{  
    Serial.println(RelayName);
    
    Serial.print("Pin :");
    Serial.print( myStructPins.relay);
    Serial.print(" Status: ");
    Serial.println(myStructStatus.relay);

    Serial.print("Pin :");
    Serial.print( myStructPins.P1_ledOn);
    Serial.print(" Status: ");
    Serial.println(myStructStatus.P1_ledOn);

    Serial.print("Pin :");
    Serial.print( myStructPins.P1_ledOff);
    Serial.print(" Status: ");
    Serial.println(myStructStatus.P1_ledOff);

    Serial.print("Pin :");
    Serial.print( myStructPins.P2_ledOn);
    Serial.print(" Status: ");
    Serial.println(myStructStatus.P2_ledOn);

    Serial.print("Pin :");
    Serial.print( myStructPins.P2_ledOff);
    Serial.print(" Status: ");
    Serial.println(myStructStatus.P2_ledOff);
}


// set the button pins to generate an external interupt on pin 2 (this can only be pins 2 or 3 on a Uno)
void setButtonPinsToInterupt()
{
  pinMode(SharedInteruptPin, INPUT_PULLUP);
  for (int x = 0; x< sizeof(buttonPins) / sizeof(int); x++)
    {
      pinMode(buttonPins[x], OUTPUT);
      digitalWrite(buttonPins[x], LOW);
    }
}

//set the button pins to find out which pin generated the interupt.
void setButtonPinsWhichButton()
{
  pinMode(SharedInteruptPin, OUTPUT);
  digitalWrite(SharedInteruptPin, LOW);
  for (int x = 0; x< sizeof(buttonPins) / sizeof(int); x++)
    {
      pinMode(buttonPins[x], INPUT_PULLUP);
    }
}

// The Interupt Service Routine called when an interupt (button push) is generated.  Pin debouncing is done here.
void buttonInterrupt() 
{ 
  //to debounce
  if (millis() - lastPress < 400) 
  {
    return;
  }
  lastPress = millis();

// set button pins to find out which pin was activated.
  setButtonPinsWhichButton(); 
// find out which button it was
  for (int x = 0; x < sizeof(buttonPins) / sizeof(int); x++) 
  { 
    if (!digitalRead(buttonPins[x])) 
    {
      pressed(buttonPins[x]);  // send pin number to function to handle the presses.
    }
  }
 setButtonPinsToInterupt(); // Return pins to original state to await the next interupt.
}

// what to do when a button is pressed - more 'case's for each additional button/pin
void pressed(int buttonPinPressed)
{
  Serial.println(buttonPinPressed);
  switch (buttonPinPressed)
    {
      case 6:
        relayToggle(relay1[1]);  // pass the appropriate relay struction to toggle to the toggle function
        break;
      case 7:
        relayToggle(relay2[1]);
        break;
    }
}

void relayToggle(relays &myrelay)
  {
   if(myrelay.relay == 0)
      { myrelay.relay = 1;}
   else
      { myrelay.relay = 0;}
   
   if(myrelay.P1_ledOn == 0)
      {myrelay.P1_ledOn = 1;}
   else
      {myrelay.P1_ledOn = 0;} 
      
   if(myrelay.P1_ledOff == 0)
      {myrelay.P1_ledOff = 1;}
   else
      {myrelay.P1_ledOff = 0;}   

   if(myrelay.P2_ledOn == 0)
      {myrelay.P2_ledOn = 1;}
   else
      {myrelay.P2_ledOn = 0;} 
      
   if(myrelay.P2_ledOff == 0)
      {myrelay.P2_ledOff = 1;}
   else
      {myrelay.P2_ledOff = 0;}    
   
   // having toggled the status of the pins I print out the current status to the serial port for checking.
   
  printMePinsStatus("Relay1",relay1[0],relay1[1]);
  printMePinsStatus("Relay2",relay2[0],relay2[1]);
 
  }