How to shorten this long If else sketch.

Anyone can help advise me how to shorten this sketch because I need to add more library later for radio,...

/*
Written by Ben W. on 21 Nov 2017
benarto_@yahoo.com
*/

// Prevent multiple inputs trigger more than 1 output, only 1 output at 1 time allowed

// these are the pins that Arduino are using
int bp1 = 3; 
int bp2 = 4;
int bp3 = 5;
int bp4 = 6;
int bp5 = 7;
int led1 = 8;
int led2 = 9;
int led3 = 10;
int led4 = 11;
int led5 = 12;
int led6 = A0;

void setup()
 {
 //-----( Initialize Pins so all leds and relays are inactive at reset)-----
  digitalWrite(led1, HIGH);
  digitalWrite(led2, HIGH);
  digitalWrite(led3, HIGH);
  digitalWrite(led4, HIGH);
  digitalWrite(led5, HIGH);
  digitalWrite(led6, HIGH);

  Serial.begin(9600); 
//-----( THEN set pins as outputs )-----
  pinMode(led1, OUTPUT);     // E.STOP relay and red LED on
  pinMode(led2, OUTPUT);     // OFF relay 3,4,5 and green LED on
  pinMode(led3, OUTPUT);     // relay 3 and blue LED1 on
  pinMode(led4, OUTPUT);     // relay 4 and blue LED2 on
  pinMode(led5, OUTPUT);     // relay 5 and blue LED3 on
  pinMode(led6, OUTPUT);     // 4 channel relay module supply-on transistor 6
  pinMode(bp1, INPUT);       // E.STOP signal, IC 7
  pinMode(bp2, INPUT);       // OFF relay 3,4,5 signal, D0 receiver
  pinMode(bp3, INPUT);       // relay 3 signal, D1 receiver
  pinMode(bp4, INPUT);       // relay 4 signal, D2 receiver
  pinMode(bp5, INPUT);       // relay 5 signal, D3 receiver

}

void loop()

{

delay(1000);                 // delay 1 minute for 4 channel relay module supply-on
digitalWrite(led6, LOW);      

if ((bp1 == HIGH) && (bp2 == LOW) && (bp3 == LOW) && (bp4 == LOW) && (bp5 == LOW))
{
  digitalWrite(led1, LOW);
  digitalWrite(led2, HIGH);
  digitalWrite(led3, HIGH);
  digitalWrite(led4, HIGH);
  digitalWrite(led5, HIGH);
  delay(3600000);            // delay 1 hour maintenance while prevent from any reset
 }
else
{
  digitalWrite(led1, HIGH);
  digitalWrite(led2, HIGH);
  digitalWrite(led3, HIGH);
  digitalWrite(led4, HIGH);
  digitalWrite(led5, HIGH);
 }

if ((bp1 == LOW) && (bp2 == HIGH) && (bp3 == LOW) && (bp4 == LOW) && (bp5 == LOW))
{
  digitalWrite(led1, HIGH);
  digitalWrite(led2, LOW);   // immediate off all relays except E.STOP
  digitalWrite(led3, HIGH);
  digitalWrite(led4, HIGH);
  digitalWrite(led5, HIGH);
 }
else
{
  digitalWrite(led1, HIGH);
  digitalWrite(led2, HIGH);
  digitalWrite(led3, HIGH);
  digitalWrite(led4, HIGH);
  digitalWrite(led5, HIGH);
 }

if ((bp1 == LOW) && (bp2 == LOW) && (bp3 == HIGH) && (bp4 == LOW) && (bp5 == LOW))
{
  digitalWrite(led1, HIGH);
  digitalWrite(led2, HIGH);
  digitalWrite(led4, HIGH);
  digitalWrite(led5, HIGH);
  delay(1000UL);             // delay 1 second to start while other button pins can reset
  digitalWrite(led3, LOW);
 }
else
{
  digitalWrite(led1, HIGH);
  digitalWrite(led2, HIGH);
  digitalWrite(led3, HIGH);
  digitalWrite(led4, HIGH);
  digitalWrite(led5, HIGH);
 }

if ((bp1 == LOW) && (bp2 == LOW) && (bp3 == LOW) && (bp4 == HIGH) && (bp5 == LOW))
{
  digitalWrite(led1, HIGH);
  digitalWrite(led2, HIGH);
  digitalWrite(led3, HIGH);
  digitalWrite(led5, HIGH);
  delay(1000UL);             // delay 1 second to start while other button pins can reset
  digitalWrite(led4, LOW);
 }
else
{
  digitalWrite(led1, HIGH);
  digitalWrite(led2, HIGH);
  digitalWrite(led3, HIGH);
  digitalWrite(led4, HIGH);
  digitalWrite(led5, HIGH);
 }

if ((bp1 == LOW) && (bp2 == LOW) && (bp3 == LOW) && (bp4 == LOW) && (bp5 == HIGH))
{
  digitalWrite(led1, HIGH);
  digitalWrite(led2, HIGH);
  digitalWrite(led3, HIGH);
  digitalWrite(led4, HIGH);
  delay(1000UL);             // delay 1 second to start while other button pins can reset
  digitalWrite(led5, LOW);
 }
else
{
  digitalWrite(led1, HIGH);
  digitalWrite(led2, HIGH);
  digitalWrite(led3, HIGH);
  digitalWrite(led4, HIGH);
  digitalWrite(led5, HIGH);
 }


}

May I ask what it does?

Incidentally, your if-else approach has not gone nearly far enough, if that is what you are aiming for, since a test of bp1,bp2... is not followed by an else if another test of the same bp1,bp2...instead you redundantly test bp1,bp2 again even though it has already been tested. That's only useful if you intend more than one combination to be simultaneously true.

Do you think maybe you might need to do some digitalReads bp1-bp5? :wink:

bp1State = digitalRead(bp1);
etc.

.

When attempting to shorten any code, a nice place to start is to identify code which is repetitive. In your case, you have five lines to set led1 through led5 all to high. That code repeats itself 5 times. Why not creae a function setAllLEDHigh, put those five lines in the function and call it instead of repeating the code over and over?

Secondly, you have LOTS of code which does almost the same Thing. It again constantly nearly repeats itself Setting a different pin low. Why not make one function that accepts five Parameters of high/low and set the Pins according to the Parameters?

Employing those two suggestions would reduce the amount of code by more than half.

I suspect that it still wouldn't work as you expect because you Need to do what Larryd said. You can't set an integer variable to 3 and then test if that variable is high or low. It will be high or low, but will never Change so what's the Point?

in every one of you else you are setting all of your led’s to high. Instead set them all high in the benignning of your loop. Don’t set them high again in you conditional. only set the led you want to set low in you conditional.

Something like this… It should give you the same results.

void loop()

{

delay(1000);                 // delay 1 minute for 4 channel relay module supply-on
digitalWrite(led6, LOW);      
  digitalWrite(led1, HIGH);
  digitalWrite(led2, HIGH);
  digitalWrite(led3, HIGH);
  digitalWrite(led4, HIGH);
  digitalWrite(led5, HIGH);
if ((bp1 == HIGH) && (bp2 == LOW) && (bp3 == LOW) && (bp4 == LOW) && (bp5 == LOW))
{
  digitalWrite(led1, LOW);
  delay(3600000);            // delay 1 hour maintenance while prevent from any reset
 }

if ((bp1 == LOW) && (bp2 == HIGH) && (bp3 == LOW) && (bp4 == LOW) && (bp5 == LOW))
{
  digitalWrite(led2, LOW);   // immediate off all relays except E.STOP

 }

if ((bp1 == LOW) && (bp2 == LOW) && (bp3 == HIGH) && (bp4 == LOW) && (bp5 == LOW))
{

  delay(1000UL);             // delay 1 second to start while other button pins can reset
  digitalWrite(led3, LOW);
 }


if ((bp1 == LOW) && (bp2 == LOW) && (bp3 == LOW) && (bp4 == HIGH) && (bp5 == LOW))
{
  delay(1000UL);             // delay 1 second to start while other button pins can reset
  digitalWrite(led4, LOW);
 }

if ((bp1 == LOW) && (bp2 == LOW) && (bp3 == LOW) && (bp4 == LOW) && (bp5 == HIGH))
{

  delay(1000UL);             // delay 1 second to start while other button pins can reset
  digitalWrite(led5, LOW);
 }

}

Also do you need to check the condition of all the input on every statement. Or just the input you are looking to be high.

if ((bp1 == HIGH) && (bp2 == LOW) && (bp3 == LOW) && (bp4 == LOW) && (bp5 == LOW))

or would this give you the expected result?

if (bp1 == HIGH)

I don’t see the variable that is getting set to 3. You are setting the Variable to the pin number 3. That should work.

kilgorq:
Also do you need to check the condition of all the input on every statement. Or just the input you are looking to be high.

if ((bp1 == HIGH) && (bp2 == LOW) && (bp3 == LOW) && (bp4 == LOW) && (bp5 == LOW))

or would this give you the expected result?

if (bp1 == HIGH)

That's not the same condition; I did not look at the full original, so it might work depending on the rest of the code.

Personally I would store the button states in a single byte and test the byte.

And it was already mentioned that op seems to be missing digitalRead statements to read the pins.

Why not creae a function setAllLEDHigh

Well, for one thing, OP does not have any LEDs connected. He has relays connected.

But, the idea is sound, even though that is NOT a good name for the function.

OP: When you start numbering variables, it is beyond time to think about arrays, instead.

Thanks everyone who replied to help. I am not a programmer and only started to learn this.

It is actually for a fan motor with 3 coils inside. The relays 3,4,5 controls each of their individual coil and only 1 relay can switch on at a time. If more than 1 relay is switched on, the 3 coils will be burnt. led1 is the emergency stop which also prevent looping in case anything wrong had happened. led2 is the stop button for all relays 3,4,5 to stop the motor. The delays there are for safety reason for motor coil to switch on only after the other relays is off.
D0,D1,D2,D3 or bp1,bp2,bp3,bp4 are only input 5V. The relays are normal relays.

aarg:
May I ask what it does?

Incidentally, your if-else approach has not gone nearly far enough, if that is what you are aiming for, since a test of bp1,bp2... is not followed by an else if another test of the same bp1,bp2...instead you redundantly test bp1,bp2 again even though it has already been tested. That's only useful if you intend more than one combination to be simultaneously true.

I repeated the sketch after the "else" because I worried after the new loop, 1 of the relays are not switch off and another is switched on. Example what if bp4 is switch on and then 2 minutes later bp5 is switch on, then will relays 4 and 5 both are on? If yes, then the coils will be burnt.

larryd:
Do you think maybe you might need to do some digitalReads bp1-bp5? :wink:

bp1State = digitalRead(bp1);
etc.

.

Sound likes good idea. Can you please kindly show me how to amend on the sketch for these digitalRead?

JaBa:
When attempting to shorten any code, a nice place to start is to identify code which is repetitive. In your case, you have five lines to set led1 through led5 all to high. That code repeats itself 5 times. Why not creae a function setAllLEDHigh, put those five lines in the function and call it instead of repeating the code over and over?

Secondly, you have LOTS of code which does almost the same Thing. It again constantly nearly repeats itself Setting a different pin low. Why not make one function that accepts five Parameters of high/low and set the Pins according to the Parameters?

Employing those two suggestions would reduce the amount of code by more than half.

I suspect that it still wouldn't work as you expect because you Need to do what Larryd said. You can't set an integer variable to 3 and then test if that variable is high or low. It will be high or low, but will never Change so what's the Point?

Sorry but can you please kindly advise me with sketch examples how to make a function to all these parameters? I also do not understand what you mean "You can't set an integer variable to 3 and then test if that variable is high or low. It will be high or low, but will never Change so what's the Point?". Please explain more.

[/quote]

kilgorq:
in every one of you else you are setting all of your led's to high. Instead set them all high in the benignning of your loop. Don't set them high again in you conditional. only set the led you want to set low in you conditional.

Something like this... It should give you the same results.

void loop()

{

delay(1000);                // delay 1 minute for 4 channel relay module supply-on
digitalWrite(led6, LOW);     
  digitalWrite(led1, HIGH);
  digitalWrite(led2, HIGH);
  digitalWrite(led3, HIGH);
  digitalWrite(led4, HIGH);
  digitalWrite(led5, HIGH);
if ((bp1 == HIGH) && (bp2 == LOW) && (bp3 == LOW) && (bp4 == LOW) && (bp5 == LOW))
{
  digitalWrite(led1, LOW);
  delay(3600000);            // delay 1 hour maintenance while prevent from any reset
}

if ((bp1 == LOW) && (bp2 == HIGH) && (bp3 == LOW) && (bp4 == LOW) && (bp5 == LOW))
{
  digitalWrite(led2, LOW);  // immediate off all relays except E.STOP

}

if ((bp1 == LOW) && (bp2 == LOW) && (bp3 == HIGH) && (bp4 == LOW) && (bp5 == LOW))
{

delay(1000UL);            // delay 1 second to start while other button pins can reset
  digitalWrite(led3, LOW);
}

if ((bp1 == LOW) && (bp2 == LOW) && (bp3 == LOW) && (bp4 == HIGH) && (bp5 == LOW))
{
  delay(1000UL);            // delay 1 second to start while other button pins can reset
  digitalWrite(led4, LOW);
}

if ((bp1 == LOW) && (bp2 == LOW) && (bp3 == LOW) && (bp4 == LOW) && (bp5 == HIGH))
{

delay(1000UL);            // delay 1 second to start while other button pins can reset
  digitalWrite(led5, LOW);
}

}




Also do you need to check the condition of all the input on every statement. Or just the input you are looking to be high. 




if ((bp1 == HIGH) && (bp2 == LOW) && (bp3 == LOW) && (bp4 == LOW) && (bp5 == LOW))




or would this give you the expected result?



if (bp1 == HIGH)




I don't see the variable that is getting set to 3. You are setting the Variable to the pin number 3. That should work.

What do you mean variable set to pin 3?
If bp4 is switch on and then 2 minutes later bp5 is switch on, then will relays 4 and 5 both are on? If yes, then the coils will be burnt and cannot use.

[/quote]

sterretje:
Personally I would store the button states in a single byte and test the byte.

And it was already mentioned that op seems to be missing digitalRead statements to read the pins.

What do you mean store in a single byte and test the byte? Can you please kindly show me the sketch?

Hi;
Can you please post a copy of your circuit, in CAD or a picture of a hand drawn circuit in jpg, png?

How fast are you switching these relays?
What is connected to bp1 to bp5?
What is a D1 receiver?

A fan with 3 coils inside?
Can you post a picture of the motor and its model/specs please?

Thanks.. Tom... :o :o :o :o

led1 is the emergency stop which also prevent looping in case anything wrong had happened. led2 is the stop button

It make no sense to talk about switches in led1 and led2.

benxx:
Sound likes good idea. Can you please kindly show me how to amend on the sketch for these digitalRead?

Maybe you should slow down a little and go back to the basics. LarryD gave you an example in reply #2.

benxx:
What do you mean store in a single byte and test the byte? Can you please kindly show me the sketch?

A byte contains 8 bits. Once you have managed to properly read your buttons, you can use one bit in the byte for the status of each of the buttons.

  7   6   5   4   3   2   1   0
+---+---+---+---+---+---+---+---+
|   |   |   |   |   |   |   |   |
+---+---+---+---+---+---+---+---+
              |   |   |   |   |
              |   |   |   |   +-- bp1 status
              |   |   |   +------ bp2 status
              |   |   +---------- bp3 status
              |   +-------------- bp4 status
              +------------------ bp5 status

Hi,
This;

int bp1 = 3; 
int bp2 = 4;
int bp3 = 5;
int bp4 = 6;
int bp5 = 7;
int led1 = 8;
int led2 = 9;
int led3 = 10;
int led4 = 11;
int led5 = 12;
int led6 = A0;

Should be this;

int bp1Pin = 3; 
int bp2Pin = 4;
int bp3Pin = 5;
int bp4Pin = 6;
int bp5Pin = 7;
int led1Pin = 8;
int led2Pin = 9;
int led3Pin = 10;
int led4Pin = 11;
int led5Pin = 12;
int led6Pin = A0;

And this added;

bool pb1Status;
bool pb2Status;
bool pb3Status;
bool pb4Status;
bool pb5Status;
bool led1Status;
bool led2Status;
bool led3Status;
bool led4Status;
bool led5Status;

Then after reading all the inputs, just once, use this equation.

int val= pb1Status * 1 + pb2Status *2 + pb3Status *4 + pb4Status *8 + pb5Status *16

The value of val will be unique for any combination of pb1Status to pb5Status.

Now you use "switch .. case " function to output the desired relay combinations.
https://www.arduino.cc/reference/en/language/structure/control-structure/switchcase/

You don't need all that if.. elseif.. else daisy chain, and its easier to follow.

Just an idea, Tom... :slight_smile: