Looking for tips if need be

I’m sorry if this is misplaced, seemed like the most appropriate section that I saw. What I’m after is pointers to better coding habits and such. I have here mostly completed code for a paintball marker. My intent was to try keeping the code as basic and simple as possible to get my marker working and cycling properly again.

I’ve been utilizing 123D Circuits to test the code/circuit design after each change. It’s a godsend, no worries about frying anything! So far, ever bit is working as I expected/wanted it to. By next weekend I should have it wired with the marker solenoids and test fire. I currently have it wired up on my UNO but I was awaiting transistors to test with the solenoids instead of leds.

On to the code then! I have eye detection, which I’m unsure currently if it will work properly; multiple fire modes, Safe/Semi/3-Round/Full; and adjustable ROF. I have most everything declared globally, could that be tidied up any? Also, I’m going to probably put a switch between the battery and the Nano I’m installing in it. I’m curious how much current draw I will have with it in mode 5 or “off”

Thanks so much for your time and any help is appreciated! I’ve thoroughly enjoyed this project and can’t wait to play!

const int powerButton	        = 3;	//input for power button
const int trigger		= 4;	//input for trigger
const int ledAuto		= 7;	//output for led full auto mode
const int ledSemi		= 6;	//output for led semi auto mode
const int eyes			= 9;	//input for eye detector
const int solenoid 		= 5;	//output for solenoid to release sear
const int valve			= 10;	//output to open and close front valve
const int ledDelay		= 200;	//delay for led blink

int ballInPlaceDelay	        = 20;	//delay after reading ball in place
int valveOpenTime		= 15;	//how long valve stays open to keep bolt forward
int valveCloseTime		= 8;	//how long to wait for valve to close
int searOpenTime		= 4;	//how long sear solenoid is open
int searCloseTime		= 2;	//delay after valve opens to close sear
int bps			        = 10;	//balls per second limit
int bpsDelay			= 0;	//resulting delay to achieve desired bps
int totalTimingDelay	        = 0;	//result of all timings summed

int lastLedState		= LOW;	//previous state of led
int ledState			= LOW;	//state of led for blinking
int powerState			= 0;	//determines the power/mode of the marker
int buttonState		        = LOW;	//high or low state of power button
int triggerState		= LOW;	//high or low state of trigger
int ballInPlace			= LOW;	//reads input from eye detector

long lastMillis			= 0;	//holds last millisecond count for blinking leds

void setup() {
 Serial.begin(9600);
 pinMode(powerButton,	        INPUT);
 pinMode(trigger,		INPUT);
 pinMode(solenoid,		OUTPUT);
 pinMode(ledSemi,		OUTPUT);
 pinMode(ledAuto,		OUTPUT);
 pinMode(eyes,			INPUT);
 pinMode(valve,			OUTPUT);
 int totalTimingDelay = (ballInPlaceDelay + valveOpenTime + valveCloseTime + searOpenTime + searCloseTime);
 bpsDelay = ((1000/bps)-(totalTimingDelay));
 if(bpsDelay < 0){
   bpsDelay = 0;
 }
 Serial.print("Balls per Second: ");
 Serial.println(1000/(totalTimingDelay + bpsDelay));		//this only returns the calculated ball per second using the above formula
}

void loop() {
  int buttonPress = digitalRead(powerButton);
  int triggerPress = digitalRead(trigger);
  int ballInPlace = digitalRead(eyes);
  
  if(buttonPress == HIGH && buttonPress != buttonState){
    powerState == powerState ++;
   
  }
  
  if(powerState == 1){						//while in mode 1, considered safe mode, all leds lit.
    digitalWrite(ledAuto, HIGH);
    digitalWrite(ledSemi, HIGH);
  }
  
  else if(powerState == 2){							//while in mode 2, operation is semi auto and blue leds are lit.
    digitalWrite(ledAuto, LOW);
    digitalWrite(ledSemi, HIGH);
    
    if(triggerPress == HIGH && ballInPlace == HIGH && triggerState != triggerPress){
      delay(ballInPlaceDelay);
      digitalWrite(solenoid, HIGH);
      delay(searOpenTime);
      digitalWrite(valve, HIGH);
      delay(searCloseTime);
      digitalWrite(solenoid, LOW);
      delay(valveOpenTime);
      digitalWrite(valve, LOW);
      delay(valveCloseTime);
   	  delay(bpsDelay);
    } 
  }
     
  else if(powerState == 3){							//while in mode 3, operation is 3 round burst and blue leds are blinking.
    digitalWrite(ledAuto, LOW);
    if(millis() - lastMillis >= ledDelay){
      lastMillis = millis();
      ledState = !lastLedState;
    }  
    digitalWrite(ledSemi, ledState);
    lastLedState = ledState;
    
    if(triggerPress == HIGH && ballInPlace == HIGH && triggerState != triggerPress){
      for(int i = 3; i > 1; i--){
        if(ballInPlace == HIGH){
          delay(ballInPlaceDelay);
      	  digitalWrite(solenoid, HIGH);
          delay(searOpenTime);
          digitalWrite(valve, HIGH);
          delay(searCloseTime);
          digitalWrite(solenoid, LOW);
          delay(valveOpenTime);
          digitalWrite(valve, LOW);
          delay(valveCloseTime);
    	  delay(bpsDelay);
        }
      }
    } 
  }
  
  else if(powerState == 4){						//while in mode 4, operation is full auto and red leds are lit.
    digitalWrite(ledAuto, HIGH);
    digitalWrite(ledSemi, LOW);
    
    if(triggerPress == HIGH && ballInPlace == HIGH){
      	delay(ballInPlaceDelay);
          digitalWrite(solenoid, HIGH);
          delay(searOpenTime);
          digitalWrite(valve, HIGH);
          delay(searCloseTime);
          digitalWrite(solenoid, LOW);
          delay(valveOpenTime);
          digitalWrite(valve, LOW);
          delay(valveCloseTime);
    	  delay(bpsDelay);
    }
  }   
  else if(powerState == 5){						//mode 5 is reset to "off".
    digitalWrite(ledAuto, LOW);
    digitalWrite(ledSemi, LOW);
    powerState = 0; 
  }
  buttonState = buttonPress;
  triggerState = triggerPress;
}

This part is wrong:

int totalTimingDelay	        = 0;	//result of all timings summed
...
 int totalTimingDelay = (ballInPlaceDelay + valveOpenTime + valveCloseTime + searOpenTime + searCloseTime);

You have two variables totalTimingDelay - not one, which you then calculate the value of. Lose the second "int".

You are doing the same thing with ballInPlace.


  if(powerState == 1){						//while in mode 1, considered safe mode, all leds lit.
...
  else if(powerState == 2){							//while in mode 2, operation is semi auto and blue leds are lit.
...
  else if(powerState == 3){

I would be using switch/case here. Also move some of that stuff into functions so that the main loop is not as cluttered. Particularly the parts that look like a block of code copied and pasted. Namely:

      delay(ballInPlaceDelay);
      digitalWrite(solenoid, HIGH);
      delay(searOpenTime);
      digitalWrite(valve, HIGH);
      delay(searCloseTime);
      digitalWrite(solenoid, LOW);
      delay(valveOpenTime);
      digitalWrite(valve, LOW);
      delay(valveCloseTime);
      delay(bpsDelay);

Whenever you see blocks of code doing the same thing: make them a function!

Thank you for pointing out the double variables! Quite easy to overlook that, especially since it hasn’t caused me a problem so far.

Not having used switch/case before, i reverted to what i do know and how to make it work. I want to branch out and explore other options such as switch. Are there any specific advantages in this example using switch over if?

I initially started to write out a firing sequence function, i will get back to that after i know this will operate the marker.

Thanks for the tips!

accusedmonk:
I’m sorry if this is misplaced, seemed like the most appropriate section that I saw. What I’m after is pointers to better coding habits and such. I have here mostly completed code for a paintball marker. My intent was to try keeping the code as basic and simple as possible to get my marker working and cycling properly again.

I’ve been utilizing 123D Circuits to test the code/circuit design after each change. It’s a godsend, no worries about frying anything! So far, ever bit is working as I expected/wanted it to. By next weekend I should have it wired with the marker solenoids and test fire. I currently have it wired up on my UNO but I was awaiting transistors to test with the solenoids instead of leds.

On to the code then! I have eye detection, which I’m unsure currently if it will work properly; multiple fire modes, Safe/Semi/3-Round/Full; and adjustable ROF. I have most everything declared globally, could that be tidied up any? Also, I’m going to probably put a switch between the battery and the Nano I’m installing in it. I’m curious how much current draw I will have with it in mode 5 or “off”

Thanks so much for your time and any help is appreciated! I’ve thoroughly enjoyed this project and can’t wait to play!

const int powerButton	        = 3;	//input for power button

const int trigger = 4; //input for trigger
const int ledAuto = 7; //output for led full auto mode
const int ledSemi = 6; //output for led semi auto mode
const int eyes = 9; //input for eye detector
const int solenoid = 5; //output for solenoid to release sear
const int valve = 10; //output to open and close front valve
const int ledDelay = 200; //delay for led blink

int ballInPlaceDelay         = 20; //delay after reading ball in place
int valveOpenTime = 15; //how long valve stays open to keep bolt forward
int valveCloseTime = 8; //how long to wait for valve to close
int searOpenTime = 4; //how long sear solenoid is open
int searCloseTime = 2; //delay after valve opens to close sear
int bps         = 10; //balls per second limit
int bpsDelay = 0; //resulting delay to achieve desired bps
int totalTimingDelay         = 0; //result of all timings summed

int lastLedState = LOW; //previous state of led
int ledState = LOW; //state of led for blinking
int powerState = 0; //determines the power/mode of the marker
int buttonState         = LOW; //high or low state of power button
int triggerState = LOW; //high or low state of trigger
int ballInPlace = LOW; //reads input from eye detector

long lastMillis = 0; //holds last millisecond count for blinking leds

void setup() {
Serial.begin(9600);
pinMode(powerButton,         INPUT);
pinMode(trigger, INPUT);
pinMode(solenoid, OUTPUT);
pinMode(ledSemi, OUTPUT);
pinMode(ledAuto, OUTPUT);
pinMode(eyes, INPUT);
pinMode(valve, OUTPUT);
int totalTimingDelay = (ballInPlaceDelay + valveOpenTime + valveCloseTime + searOpenTime + searCloseTime);
bpsDelay = ((1000/bps)-(totalTimingDelay));
if(bpsDelay < 0){
  bpsDelay = 0;
}
Serial.print("Balls per Second: ");
Serial.println(1000/(totalTimingDelay + bpsDelay)); //this only returns the calculated ball per second using the above formula
}

void loop() {
  int buttonPress = digitalRead(powerButton);
  int triggerPress = digitalRead(trigger);
  int ballInPlace = digitalRead(eyes);
 
  if(buttonPress == HIGH && buttonPress != buttonState){
    powerState == powerState ++;
 
  }
 
  if(powerState == 1){ //while in mode 1, considered safe mode, all leds lit.
    digitalWrite(ledAuto, HIGH);
    digitalWrite(ledSemi, HIGH);
  }
 
  else if(powerState == 2){ //while in mode 2, operation is semi auto and blue leds are lit.
    digitalWrite(ledAuto, LOW);
    digitalWrite(ledSemi, HIGH);
   
    if(triggerPress == HIGH && ballInPlace == HIGH && triggerState != triggerPress){
      delay(ballInPlaceDelay);
      digitalWrite(solenoid, HIGH);
      delay(searOpenTime);
      digitalWrite(valve, HIGH);
      delay(searCloseTime);
      digitalWrite(solenoid, LOW);
      delay(valveOpenTime);
      digitalWrite(valve, LOW);
      delay(valveCloseTime);
    delay(bpsDelay);
    }
  }
   
  else if(powerState == 3){ //while in mode 3, operation is 3 round burst and blue leds are blinking.
    digitalWrite(ledAuto, LOW);
    if(millis() - lastMillis >= ledDelay){
      lastMillis = millis();
      ledState = !lastLedState;
    } 
    digitalWrite(ledSemi, ledState);
    lastLedState = ledState;
   
    if(triggerPress == HIGH && ballInPlace == HIGH && triggerState != triggerPress){
      for(int i = 3; i > 1; i–){
        if(ballInPlace == HIGH){
          delay(ballInPlaceDelay);
        digitalWrite(solenoid, HIGH);
          delay(searOpenTime);
          digitalWrite(valve, HIGH);
          delay(searCloseTime);
          digitalWrite(solenoid, LOW);
          delay(valveOpenTime);
          digitalWrite(valve, LOW);
          delay(valveCloseTime);
      delay(bpsDelay);
        }
      }
    }
  }
 
  else if(powerState == 4){ //while in mode 4, operation is full auto and red leds are lit.
    digitalWrite(ledAuto, HIGH);
    digitalWrite(ledSemi, LOW);
   
    if(triggerPress == HIGH && ballInPlace == HIGH){
      delay(ballInPlaceDelay);
          digitalWrite(solenoid, HIGH);
          delay(searOpenTime);
          digitalWrite(valve, HIGH);
          delay(searCloseTime);
          digitalWrite(solenoid, LOW);
          delay(valveOpenTime);
          digitalWrite(valve, LOW);
          delay(valveCloseTime);
      delay(bpsDelay);
    }
  } 
  else if(powerState == 5){ //mode 5 is reset to “off”.
    digitalWrite(ledAuto, LOW);
    digitalWrite(ledSemi, LOW);
    powerState = 0;
  }
  buttonState = buttonPress;
  triggerState = triggerPress;
}

" I have most everything declared globally, could that be tidied up any? "

No tiding needed , but you need to pay attention to “scope” of variable.
Unless it is intentional , you have couple of global variables redefined as locals.

accusedmonk:
Are there any specific advantages in this example using switch over if?

Just readability. It is clearer that you are testing a single variable for multiple possible values. Using if/else could be for independent variables. eg.

if (foo == 24)
  {
  // whatever
  }
else if (bar < 13)
  {
  // whatever
  }

Whereas:

  switch (powerState)
    {
    case 1:
      // whatever
      break;

    case 2:
      // whatever
      break;

    case 3:
      // whatever
      break;
    }  // end of switch

It’s just clearer that the whole intent is to test powerState against various values. You are making that obvious up front.

What I'm after is pointers to better coding habits

Use variables of the smallest appropriate size.

const int powerButton        = 3; //input for power button
const int trigger = 4; //input for trigger
const int ledAuto = 7; //output for led full auto mode
const int ledSemi = 6; //output for led semi auto mode
const int eyes = 9; //input for eye detector
const int solenoid = 5; //output for solenoid to release sear
const int valve = 10; //output to open and close front valve
const int ledDelay = 200; //delay for led blink

int ballInPlaceDelay        = 20; //delay after reading ball in place
int valveOpenTime = 15; //how long valve stays open to keep bolt forward
int valveCloseTime = 8; //how long to wait for valve to close
int searOpenTime = 4; //how long sear solenoid is open
int searCloseTime = 2; //delay after valve opens to close sear
int bps        = 10; //balls per second limit
int bpsDelay = 0; //resulting delay to achieve desired bps
int totalTimingDelay        = 0; //result of all timings summed

int lastLedState = LOW; //previous state of led
int ledState = LOW; //state of led for blinking
int powerState = 0; //determines the power/mode of the marker
int buttonState        = LOW; //high or low state of power button
int triggerState = LOW; //high or low state of trigger
int ballInPlace = LOW; //reads input from eye detector

How many of these variables are ever going to have a value greater than 255 ? Any that could be declared as byte should be.

Many thanks! I've been following threads here for awhile and am grateful of how wonderful and helpful the community is.

The local declarations i made duplicates of globals are fixed, i probably wouldn't have noticed that.

I have swapped all variables to byte now. I honestly thought that was just best tied to serial information. For example, sending a byte of information to a shift register. I'm just not fully aware of how different things are handled, but the more i read and ask question the clearer everything becomes.

The blocks of code for the fire sequences are now in one function. Also removed the ifs and it's now switch case instead. It looks neater, and simpler.

Again, thanks to everyone for the constructive tips! Hopefully I'll have a working marker next weekend to link a video in the projects section!

Well, if we are talking style, for state I like to use an enum so that there are expressive names.

enum PowerState {
  START = 0,
  ARM=1,
  TRIGGER_THINGY = 2,
  RELEASE_BALL = 3,
  RETRACT = 5
} powerState;


switch(powerState) {
case START:
  ...;
  break;

case ARM:
  ...;
  break;

... etc etc ...
}

You asked about the use of the switch, and Nick pointed out that it is easier to read, which is true. However, it can also be more efficient. For example, suppose you need to do a specific thing on each day of the month. You could use:

if (day == 1)    // The first of the month, pay the rent
{
    PayRent(800);
}
else if (day == 2)  // Second of the month, pay insurance
{
      PayInsurance(50);
}
// And so on for 31 days

Now, if today is the 31st, you have to fall through 30 false if tests before you get to the one you need to process. Now consider the alternative:

   switch (day) {
      case 1:                  // The first of the month, pay the rent
         PayRent(800);
         break;

      case 2:                  // The second of the month, pay insurance
         PayInsurance(50);
         break;

// And so on for 31 days
      default:                  // Always add one of these
         Serial.print("I shouldn't be here. day = ");
         Serial.println(day);
         break;
   }

First, this alternative is easier to read and, hence, debug. Second, the compiler generates a jump table creating a target for each of the case statements. Therefore, when the switch is read, the code jumps to the proper memory location to execute the statements associated with that case. For the 31st of the month, this means you "jump over" 30 unnecessary if tests and immediately start executing the proper code. So you get a slight speed improvement plus code that's easier to read and debug. Finally, I always add a default statement block when I use a non-trivial switch. It can help in debugging.

Finally, I always add a default statement block when I use a non-trivial switch. It can help in debugging.
Very good point.

And same goes for if( true ){...} else {...}.
Especially when the code is govern by loop(). Unfortunately it is seldom seen since its takes more coding effort to control the loop() from going into "else" most of the time.

I’ll have to look into the enum, i haven’t seen that before. Basically let’s you name the switch instead of using numbered cases?

That’s quite a difference between switch and ifs, speaking strickly efficiency. It really helps to understand what’s going on in the background! Thank you for the information!

enum lets you name related things. It can also auto-assign values, so you don't need to worry about them (if it doesn't matter).

So, for example:

enum PowerState {
  START,
  ARM,
  TRIGGER_THINGY,
  RELEASE_BALL,
  RETRACT
} powerState;

They get given consecutive numbers. Who cares what the numbers are? As long as you use the names you are fine.

However if the number are defined externally (like, in a datasheet for a chip) then you have to go with the numbers that you are given.