Notifications
Clear all

#newb Could I simplify my code?  


Pete Bevan
(@pete-bevan)
New Member
Joined: 10 months ago
Posts: 3
Topic starter  

I received my first Arduino Nano through the post in January and this is the first full project I have developed with the Arduino, previously having no experience with microcontrollers or code, so please bare with me if I have missed some basic things out here!

I am building a model railway layout that will be taken to exhibitions and run at home. To control the points in the goods yard I have designed two control panels that mimic each other and sit on ether side of the layout. At an exhibition I will use the top panel which will be situated on the rear of the layout and at home will use the bottom panel, which will be situated on the front of the layout.

The idea is that I can control the goods yard from either side of the layout and have feedback of the position of the points from LEDs situated on the panel.

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

The push buttons 1a-5b switch  DPDT relay modules which in turn control point motors 1-5. The push buttons also control two pairs of LEDs (1a,1b,1c,1d etc) to indicate which road the points are set to.

The code starts by setting the relays LOW and all LEDs of xa and xc HIGH as an initial state, then the code toggles the switches to control the LEDs and relays.

I have setup and successfully tested the code and circuit for points 1 and 2  using an Arduino Nano. Off  the back of the successful test, I have written the fill code but have not yet assigned pins for the Arduino Mega. I have ordered an Arduino Mega which is on its way, with is many more IO pins, but I have a couple of questions...

This is my first ever post on a forum, so please forgive me if it is too long winded and if the code is incorrectly posted.

Regards

Pete

 

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?

3. Could I use a shift register or something similar to enable me to use a Nano rather than a Mega?

Here are the two control panels, one for the front, and the other for the back of the layout:

image

Here is my code:

/*

*/
int LED1aPin=2;
int LED1bPin=3;
int LED1cPin=4;
int LED1dPin=5;

int LED2aPin=14;
int LED2bPin=15;
int LED2cPin=16;
int LED2dPin=17;

int LED3aPin=14;
int LED3bPin=15;
int LED3cPin=16;
int LED3dPin=17;

int LED4aPin=14;
int LED4bPin=15;
int LED4cPin=16;
int LED4dPin=17;

int LED5aPin=14;
int LED5bPin=15;
int LED5cPin=16;
int LED5dPin=17;

int LED1aState=1;
int LED2aState=1;
int LED3aState=1;
int LED4aState=1;
int LED5aState=1;

int button1aPin=6;
int button1bPin=7;
int button2aPin=10;
int button2bPin=11;
int button3aPin=6;
int button3bPin=7;
int button4aPin=8;
int button4bPin=9;
int button5aPin=10;
int button5bPin=11;

int relay1Pin=8;
int relay2Pin=9;
int relay3Pin=9;
int relay4Pin=9;
int relay5Pin=9;

int relay1State=0;
int relay2State=0;
int relay3State=0;
int relay4State=0;
int relay5State=0;

int button1New;
int button1Old=1;
int button2New;
int button2Old=1;
int button3New;
int button3Old=1;
int button4New;
int button4Old=1;
int button5New;
int button5Old=1;

int dt=100;

void setup() {
  
  pinMode(LED1aPin, OUTPUT);
  pinMode(LED1bPin, OUTPUT);
  pinMode(LED1cPin, OUTPUT);
  pinMode(LED1dPin, OUTPUT);
  
  pinMode(LED2aPin, OUTPUT);
  pinMode(LED2bPin, OUTPUT);
  pinMode(LED2cPin, OUTPUT);
  pinMode(LED2dPin, OUTPUT);
  
  pinMode(LED3aPin, OUTPUT);
  pinMode(LED3bPin, OUTPUT);
  pinMode(LED3cPin, OUTPUT);
  pinMode(LED3dPin, OUTPUT);
  
  pinMode(LED4aPin, OUTPUT);
  pinMode(LED4bPin, OUTPUT);
  pinMode(LED4cPin, OUTPUT);
  pinMode(LED4dPin, OUTPUT);
  
  pinMode(LED5aPin, OUTPUT);
  pinMode(LED5bPin, OUTPUT);
  pinMode(LED5cPin, OUTPUT);
  pinMode(LED5dPin, OUTPUT);
  
  
  pinMode(button1aPin, INPUT_PULLUP);
  pinMode(button1bPin, INPUT_PULLUP);
  pinMode(button2aPin, INPUT_PULLUP);
  pinMode(button2bPin, INPUT_PULLUP);
  pinMode(button3aPin, INPUT_PULLUP);
  pinMode(button3bPin, INPUT_PULLUP);
  pinMode(button4aPin, INPUT_PULLUP);
  pinMode(button4bPin, INPUT_PULLUP);
  pinMode(button5aPin, INPUT_PULLUP);
  pinMode(button5bPin, INPUT_PULLUP); 


  pinMode(relay1Pin, OUTPUT);
  pinMode(relay2Pin, OUTPUT);
  pinMode(relay3Pin, OUTPUT);
  pinMode(relay4Pin, OUTPUT);
  pinMode(relay5Pin, OUTPUT);

  digitalWrite(LED1aPin,HIGH);
  digitalWrite(LED1cPin,HIGH);
  digitalWrite(LED2aPin,HIGH);
  digitalWrite(LED2cPin,HIGH);
  digitalWrite(LED3aPin,HIGH);
  digitalWrite(LED3cPin,HIGH);
  digitalWrite(LED4aPin,HIGH);
  digitalWrite(LED4cPin,HIGH);
  digitalWrite(LED5aPin,HIGH);
  digitalWrite(LED5cPin,HIGH);
  digitalWrite(relay1Pin,LOW);
  digitalWrite(relay2Pin,LOW);
  digitalWrite(relay3Pin,LOW);
  digitalWrite(relay4Pin,LOW);
  digitalWrite(relay5Pin,LOW);
}


void loop() {
//Button1
    button1New=digitalRead(button1aPin);

    if(button1Old==0 && button1New==1) {
     if(LED1aState==0) {
       digitalWrite(LED1aPin,HIGH);
       digitalWrite(LED1bPin,LOW);
       digitalWrite(LED1cPin,HIGH);
       digitalWrite(LED1dPin,LOW);
       digitalWrite(relay1Pin,LOW);
       LED1aState=1;
     }
    else{
       digitalWrite(LED1aPin,LOW);
       digitalWrite(LED1bPin,HIGH);
       digitalWrite(LED1cPin,LOW);
       digitalWrite(LED1dPin,HIGH);
       digitalWrite(relay1Pin,HIGH);
       LED1aState=0;      
    }
}
button1Old=button1New;

delay(dt);

button1New=digitalRead(button1bPin);

    if(button1Old==0 && button1New==1) {
     if(LED1aState==0) {
       digitalWrite(LED1aPin,HIGH);
       digitalWrite(LED1bPin,LOW);
       digitalWrite(LED1cPin,HIGH);
       digitalWrite(LED1dPin,LOW);
       digitalWrite(relay1Pin,LOW);
       LED1aState=1;
     }
    else{
       digitalWrite(LED1aPin,LOW);
       digitalWrite(LED1bPin,HIGH);
       digitalWrite(LED1cPin,LOW);
       digitalWrite(LED1dPin,HIGH);
       digitalWrite(relay1Pin,HIGH);
       LED1aState=0;      
    }
}
button1Old=button1New;

delay(dt);
//Button1
//Button2
    button2New=digitalRead(button2aPin);

    if(button2Old==0 && button2New==1) {
     if(LED2aState==0) {
       digitalWrite(LED2aPin,HIGH);
       digitalWrite(LED2bPin,LOW);
       digitalWrite(LED2cPin,HIGH);
       digitalWrite(LED2dPin,LOW);
       digitalWrite(relay2Pin,LOW);
       LED2aState=1;
     }
    else{
       digitalWrite(LED2aPin,LOW);
       digitalWrite(LED2bPin,HIGH);
       digitalWrite(LED2cPin,LOW);
       digitalWrite(LED2dPin,HIGH);
       digitalWrite(relay2Pin,HIGH);
       LED2aState=0;      
    }
}
button2Old=button2New;

delay(dt);

button2New=digitalRead(button2bPin);

    if(button2Old==0 && button2New==1) {
     if(LED2aState==0) {
       digitalWrite(LED2aPin,HIGH);
       digitalWrite(LED2bPin,LOW);
       digitalWrite(LED2cPin,HIGH);
       digitalWrite(LED2dPin,LOW);
       digitalWrite(relay2Pin,LOW);
       LED2aState=1;
     }
    else{
       digitalWrite(LED2aPin,LOW);
       digitalWrite(LED2bPin,HIGH);
       digitalWrite(LED2cPin,LOW);
       digitalWrite(LED2dPin,HIGH);
       digitalWrite(relay2Pin,HIGH);
       LED2aState=0;      
    }
}
button2Old=button2New;

delay(dt);

//Button2
//Button3
    button3New=digitalRead(button3aPin);

    if(button3Old==0 && button3New==1) {
     if(LED3aState==0) {
       digitalWrite(LED3aPin,HIGH);
       digitalWrite(LED3bPin,LOW);
       digitalWrite(LED3cPin,HIGH);
       digitalWrite(LED3dPin,LOW);
       digitalWrite(relay3Pin,LOW);
       LED3aState=1;
     }
    else{
       digitalWrite(LED3aPin,LOW);
       digitalWrite(LED3bPin,HIGH);
       digitalWrite(LED3cPin,LOW);
       digitalWrite(LED3dPin,HIGH);
       digitalWrite(relay3Pin,HIGH);
       LED3aState=0;      
    }
}
button3Old=button3New;

delay(dt);

button3New=digitalRead(button3bPin);

    if(button3Old==0 && button3New==1) {
     if(LED3aState==0) {
       digitalWrite(LED3aPin,HIGH);
       digitalWrite(LED3bPin,LOW);
       digitalWrite(LED3cPin,HIGH);
       digitalWrite(LED3dPin,LOW);
       digitalWrite(relay3Pin,LOW);
       LED3aState=1;
     }
    else{
       digitalWrite(LED3aPin,LOW);
       digitalWrite(LED3bPin,HIGH);
       digitalWrite(LED3cPin,LOW);
       digitalWrite(LED3dPin,HIGH);
       digitalWrite(relay3Pin,HIGH);
       LED3aState=0;      
    }
}
button3Old=button3New;

delay(dt);
//Button3
//Button4
    button4New=digitalRead(button4aPin);

    if(button4Old==0 && button4New==1) {
     if(LED4aState==0) {
       digitalWrite(LED4aPin,HIGH);
       digitalWrite(LED4bPin,LOW);
       digitalWrite(LED4cPin,HIGH);
       digitalWrite(LED4dPin,LOW);
       digitalWrite(relay4Pin,LOW);
       LED4aState=1;
     }
    else{
       digitalWrite(LED4aPin,LOW);
       digitalWrite(LED4bPin,HIGH);
       digitalWrite(LED4cPin,LOW);
       digitalWrite(LED4dPin,HIGH);
       digitalWrite(relay4Pin,HIGH);
       LED4aState=0;      
    }
}
button4Old=button4New;

delay(dt);

button4New=digitalRead(button4bPin);

    if(button4Old==0 && button4New==1) {
     if(LED4aState==0) {
       digitalWrite(LED4aPin,HIGH);
       digitalWrite(LED4bPin,LOW);
       digitalWrite(LED4cPin,HIGH);
       digitalWrite(LED4dPin,LOW);
       digitalWrite(relay4Pin,LOW);
       LED4aState=1;
     }
    else{
       digitalWrite(LED4aPin,LOW);
       digitalWrite(LED4bPin,HIGH);
       digitalWrite(LED4cPin,LOW);
       digitalWrite(LED4dPin,HIGH);
       digitalWrite(relay4Pin,HIGH);
       LED4aState=0;      
    }
}
button4Old=button4New;

delay(dt);
//Button4
//Button5
    button5New=digitalRead(button5aPin);

    if(button5Old==0 && button5New==1) {
     if(LED5aState==0) {
       digitalWrite(LED5aPin,HIGH);
       digitalWrite(LED5bPin,LOW);
       digitalWrite(LED5cPin,HIGH);
       digitalWrite(LED5dPin,LOW);
       digitalWrite(relay5Pin,LOW);
       LED5aState=1;
     }
    else{
       digitalWrite(LED5aPin,LOW);
       digitalWrite(LED5bPin,HIGH);
       digitalWrite(LED5cPin,LOW);
       digitalWrite(LED5dPin,HIGH);
       digitalWrite(relay5Pin,HIGH);
       LED5aState=0;      
    }
}
button5Old=button5New;

delay(dt);

button5New=digitalRead(button5bPin);

    if(button5Old==0 && button5New==1) {
     if(LED5aState==0) {
       digitalWrite(LED5aPin,HIGH);
       digitalWrite(LED5bPin,LOW);
       digitalWrite(LED5cPin,HIGH);
       digitalWrite(LED5dPin,LOW);
       digitalWrite(relay5Pin,LOW);
       LED5aState=1;
     }
    else{
       digitalWrite(LED5aPin,LOW);
       digitalWrite(LED5bPin,HIGH);
       digitalWrite(LED5cPin,LOW);
       digitalWrite(LED5dPin,HIGH);
       digitalWrite(relay5Pin,HIGH);
       LED5aState=0;      
    }
}
button5Old=button5New;

delay(dt);
//Button5
}


Quote
ZeFerby
(@zeferby)
Reputable Member
Joined: 1 year ago
Posts: 380
 
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...

Eric


ReplyQuote
byron
(@byron)
Reputable Member
Joined: 2 years ago
Posts: 389
 
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.


ReplyQuote
JBeazy
(@jbeazy)
Eminent Member
Joined: 1 year ago
Posts: 23
 

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.


ReplyQuote
byron
(@byron)
Reputable Member
Joined: 2 years ago
Posts: 389
 

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

 


ReplyQuote