There must be a cleaner way to do this

Projects Programming
1

I posted about a month back about using the arduino to control 8 12V printers for a theatrical effect. I have wired 8 relays using optoisolators.
This program is written to do the following.

During the show, there are 13 instances where one printer must print 1 page.
Then there needs to be a slow "rain" of pages from all over.
Finally there is a storm of papper.

I am using 1 latching button to control this by counting button presses. The numbers are even so that there is no action during odd presses. The additional buttonstate and button pin was my attempt at adding a button for the final two effects but I had no success with that.

Are ther any ways that I can stream line this code?

int buttonPin2 = 3;
int buttonPin = 2;     
int pinArray[9] = {5, 6, 7, 8, 9, 10, 11, 12};     
int timer = 1400;
int count = 0;
int buttonState = 0;  
int buttonState2 = 0;
int buttonPushCounter = 0; 
int lastButtonState = LOW;
int printMode = 0;
int val;                        
int val2;   
int ledPin = 13;
void setup() {
  
for (count=0;count<8;count++) {
  pinMode(pinArray[count], OUTPUT); 
  pinMode(ledPin, OUTPUT);  
  pinMode(buttonPin, INPUT); 
  pinMode(buttonPin2, INPUT);
Serial.begin(9600);  
 
 
buttonState2 = digitalRead(buttonPin2);
}}
void loop() {
  buttonState = digitalRead(buttonPin);

  if (buttonState != lastButtonState) {
      buttonPushCounter++;
      Serial.println("on");
      Serial.print("number of button pushes:");
      Serial.println(buttonPushCounter, DEC);
    } 
   lastButtonState = buttonState;
  
  if (buttonPushCounter % 2 == 0) {
    digitalWrite(ledPin, HIGH);
  } else {
   digitalWrite(ledPin, LOW);
   
 } if (buttonPushCounter == 2){
   delay(500);
   digitalWrite(pinArray[1], HIGH);
   delay(timer);
   digitalWrite(pinArray[1], LOW);
   delay(3000);
   } 
 if (buttonPushCounter == 4){
   delay(500);
   digitalWrite(pinArray[2], HIGH);
   delay(timer);
   digitalWrite(pinArray[2], LOW);
   delay(3000);
 } 
 if (buttonPushCounter == 6){
   delay(500);
   digitalWrite(pinArray[3], HIGH);
   delay(timer);
   digitalWrite(pinArray[3], LOW);
   delay(3000);
 }
 if (buttonPushCounter == 8){
   delay(500);
   digitalWrite(pinArray[4], HIGH);
   delay(timer);
   digitalWrite(pinArray[4], LOW);
   delay(3000);
 }
 if (buttonPushCounter == 10){
   delay(500);
   digitalWrite(pinArray[5], HIGH);
   delay(timer);
   digitalWrite(pinArray[5], LOW);
   delay(3000);
 }
 if (buttonPushCounter == 12){
   delay(500);
   digitalWrite(pinArray[6], HIGH);
   delay(timer);
   digitalWrite(pinArray[6], LOW);
   delay(3000);
 }
 if (buttonPushCounter == 14){
   delay(500);
   digitalWrite(pinArray[7], HIGH);
   delay(timer);
   digitalWrite(pinArray[7], LOW);
   delay(3000);
 }
 if (buttonPushCounter == 16){
   delay(500);
   digitalWrite(pinArray[8], HIGH);
   delay(timer);
   digitalWrite(pinArray[8], LOW);
   delay(3000);
 }
 if (buttonPushCounter == 18){
   delay(500);
   digitalWrite(pinArray[6], HIGH);
   delay(timer);
   digitalWrite(pinArray[6], LOW);
   delay(3000);
 }
 if (buttonPushCounter == 20){
   delay(500);
   digitalWrite(pinArray[2], HIGH);
   delay(timer);
   digitalWrite(pinArray[2], LOW);
   delay(3000);
 }
 if (buttonPushCounter == 22){
   delay(500);
   digitalWrite(pinArray[3], HIGH);
   delay(timer);
   digitalWrite(pinArray[3], LOW);
   delay(3000);
 }
 if (buttonPushCounter == 24){
   delay(500);
   digitalWrite(pinArray[4], HIGH);
   delay(timer);
   digitalWrite(pinArray[4], LOW);
   delay(3000);
 }
 if (buttonPushCounter == 26){
   delay(500);
   digitalWrite(pinArray[2], HIGH);
   delay(timer);
   digitalWrite(pinArray[2], LOW);
   delay(3000);
 }
 if (buttonPushCounter == 28){
   for (count=0;count<8;count++){
   digitalWrite(pinArray[count], HIGH);
   delay(timer);
   digitalWrite(pinArray[count], LOW);
   delay(2000);
 }
 if (buttonPushCounter == 30){
   for (count=0;count<8;count++){
   digitalWrite(pinArray[count], HIGH);
   delay(timer);
   digitalWrite(pinArray[count], LOW);
   delay(100);
 }
   }}}
2

Instead of using a bunch of if() this or if() that, you can change that section into a switch statement. But if you look at the structure of what you are doing in each if() clause, because it is so similar I'd probably combine all of those into a single if(). Something like:

if (buttonPushCounter % 2 == 0 && buttonPushCounter >= 2 && buttonPushCounter <= 16){
   delay(500);
   digitalWrite(pinArray[buttonPushCounter/2], HIGH);
   delay(timer);
   digitalWrite(pinArray[buttonPushCounter/2], LOW);
   delay(3000);
}

Also for your 28 and 30 case you can combine them but it looks like you have a bug in the code based on what you wrote (the for look looks like it contains too much). For those two cases where you want lots of paper, why aren't you turning them all on at once then turn them all off, instead of doing them in sequence.

Another method to reduce the code is to use a simple lookup table for number of pushes to pin number to activate. That way the only special cases would seem to be the last two.

3

One simple change would be to replace anything like this:

 if (buttonPushCounter == 6)
{
   delay(500);
   digitalWrite(pinArray[3], HIGH);
   delay(timer);
   digitalWrite(pinArray[3], LOW);
   delay(3000);
 }

With this

if (buttonPushCounter == 6)
  PrintPage(3);

Where the PrintPage function looks like this:

void PrintPage(int PinIdx)
{
delay(500);
digitalWrite(pinArray[PinIdx], HIGH);
delay(timer);
digitalWrite(pinArray[PinIdx], LOW);
delay(3000);
}

A more sophisticated change would be to create an array of structs, each of which contains a ButtonPressNumber, affected pinIndex and size of pre and post delays, plus a flag to indicate whether to iterate through the entire pinarray. Then your program is tiny - a little more complex version of PrintPage, and entirely data driven.

4 
for (count=0;count<8;count++) {
  pinMode(pinArray[count], OUTPUT); 
  pinMode(ledPin, OUTPUT);  
  pinMode(buttonPin, INPUT); 
  pinMode(buttonPin2, INPUT);
Serial.begin(9600);  
 
 
buttonState2 = digitalRead(buttonPin2);
}}

Each curly brace on its own line, and proper indentation, might lead you to ask yourself why you need to set the mode of the ledPin, buttonPin, and buttonPin2 pins 8 times, and why you need to call Serial.begin() 8 times.

Naming the second instance of something by appending a number, and not appending a number to the first instance, strikes me as lazy.

5

pinArray has nine elements, but you only set pinMode for eight of them (to be fair, you only assign eight of the nine), but you refer to the ninth element in your sketch - that sort of thing leads to heartache and hair-loss later.