Coding tips needed

Hey guys,

I am writing code for my 2 lane finish line sensor.

The project uses 2 laser pointers aimed at 2 photo-transistors, the Arduino checks for a voltage drop on the photo-transistors and switches one of two relays on to indicate what lane won.

I am slightly new to coding and would like to share and hopefully improve on my code if possible.

Any ideas or suggests are welcome.
Thank you.

const int sensor1 = 3;  //phototransistor connected to analog pin 3
const int sensor2 = 4;  //phototransistor connected to analog pin 4
const int relay1 = 13;  //relay connected to pin 13
const int relay2 = 14;  //relay connected to pin 14

int sensorVoltage1 = 0;
int sensorVoltage2 = 0;

const int delayTime = 3000;         //before relay resets
const int sensorSensitivity = 250;  // range between 0-1024

bool status1;
bool status2;

void setup()
{
  pinMode (relay1,OUTPUT);
  pinMode (relay2,OUTPUT);
  Serial.begin (9600);
}

void loop()
{
  sensorVoltage1 = analogRead (sensor1);     // read the voltage of phototransistor 1
  sensorVoltage2 = analogRead (sensor2);     // read the voltage of phototransistor 2
  //Serial.println (sensorVoltage1); 
  //Serial.println (sensorVoltage2);  
  
  if (sensorVoltage1 < sensorSensitivity)    // if there is no laser light to phototransistor 
  {
    status1 = false;                         // laser beam has been broken
  }
  else
  {
    status1 = true;                          // laser beam has not been broken
  }

  if (sensorVoltage2 < sensorSensitivity)   // if there is no laser light to phototransistor 
  {
    status2 = false;                        // laser beam has been broken
  }
  else
  {
    status2 = true;                         // laser beam has not been broken
  }

  if (status1 == false || status2 == false)   
  {
    if (status1 == false)            // if laser beam has been broken
    {
      digitalWrite(relay1, HIGH);    // turn relay1 on to indicate break in laser beam
    }

    if (status2 == false)           // if laser beam has been broken
    {
      digitalWrite(relay2, HIGH);   // turn relay2 on to indicate break in laser beam
    }
      delay(delayTime);             // Wait for timeout
      digitalWrite(relay1, LOW);    // turn relay1 off
      digitalWrite(relay2, LOW);    // turn relay2 off
      status1 = false;              // reset bool to false
      status2  = false;             // reset bool to false
  }
}
const int sensor1 = 3;  //phototransistor connected to analog pin 3
const int sensor2 = 4;  //phototransistor connected to analog pin 4
const int relay1 = 13;  //relay connected to pin 13
const int relay2 = 14;  //relay connected to pin 14

Unless you have an Arduino with 30,000 pins, or you think that using negative pin numbers makes sense, byte is a more appropriate type for variables holding pin numbers.

Using the smallest possible type for any variable is a good habit to develop.

    status1 = false;                         // laser beam has been broken

I really must be missing something. I can so no relationship between status and true or false, or between status and the state of a pin.

    }
      delay(delayTime);             // Wait for timeout
      digitalWrite(relay1, LOW);    // turn relay1 off
      digitalWrite(relay2, LOW);    // turn relay2 off
      status1 = false;              // reset bool to false
      status2  = false;             // reset bool to false
  }
}

I am a big
fan of proper
indenting. I hate
code that jumps
all over
the
place.

You have a lot of global variables that are used only in loop(). Limiting the scope of variables is one sign of a good programmer. Another is using names that make sense.

  if (sensorVoltage1 < sensorSensitivity)    // if there is no laser light to phototransistor 
  {
    status1 = false;                         // laser beam has been broken
  }
  else
  {
    status1 = true;                          // laser beam has not been broken
  }

This is unnecessarily complex. There is never a reason to use an if statement to assign true/false values like this, because the comparison you're using in the if condition already gives a true false value. For this specific case, where the true case is assigning false, you can either:

  1. Reverse the comparison: status1 = sensorVoltage1 >= sensorSensitivity

  2. Invert the result of your current comparison with the negation operator: status1 = !(sensorVoltage1 < sensorSensitivity)

Both will give the same result.

Most importantly, if you really want to improve your coding skills take this opportunity to learn how to use functions and classes. You have several duplicate parts of your code that are doing the same thing to different variables. As you scale up the complexity of your programs this kind of style of duplicating tons of code and dumping it all into loop() creates a Lovecraftian horror of a program that drives you to madness just looking at it. I should know. One of the programs I maintain at work is a set of Excel macros made by someone who coded like you, except his function is 8,000 lines long. :fearful:

Moving these duplications out to their own function also lets you isolate functionality to make it easier to test individual subfunctions to make sure all of your program is working as intended.

Thank you for your time Jiggy i really appreciate your helpful insight. Luckily i use excel sparsely otherwise i may have ended up on somebody's radar.

So after taking your advice i have cleaned up my code but have run into a problem that i hope you can help me with.

The relay triggers on when the laser is sensed not when the laser has been broke. How do make it so the relay turns on when the laser beam has been broke. (Broke wasn't my best choice of words).

const byte sensor1 = 3;  //phototransistor connected to analog pin 3
const byte sensor2 = 4;  //phototransistor connected to analog pin 4
const byte relay1 = 13;  //relay connected to pin 13
const byte relay2 = 12;  //relay connected to pin 14

const int delayTime = 3000;         //before relay resets
const int sensorSensitivity = 600;  // range between 0-1024

void setup()
{
  pinMode (relay1,OUTPUT);
  pinMode (relay2,OUTPUT);
  Serial.begin (9600);
}

void loop()
{
  int sensorVoltage1 = analogRead (sensor1);     // read the voltage of phototransistor 1
  int sensorVoltage2 = analogRead (sensor2);     // read the voltage of phototransistor 2
  Serial.println (sensorVoltage1); 
  Serial.println (sensorVoltage2);  

  bool status1 = sensorVoltage1 >= sensorSensitivity;
  bool status2 = sensorVoltage2 >= sensorSensitivity;

  digitalWrite(relay1, status1);
  digitalWrite(relay2, status2);
  
  if (!status1 || !status2 )
  {
    delay(delayTime);
  } 
}
  bool status1 = sensorVoltage1 >= sensorSensitivity;
  bool status2 = sensorVoltage2 >= sensorSensitivity;

  digitalWrite(relay1, status1);
  digitalWrite(relay2, status2);

Did you miss the part about possibly needing to use the ! operator?

  bool status1 = !(sensorVoltage1 >= sensorSensitivity);
  bool status2 = !(sensorVoltage2 >= sensorSensitivity);

Or, you could just reverse the logic.

  bool status1 = sensorVoltage1 < sensorSensitivity;
  bool status2 = sensorVoltage2 < sensorSensitivity;

Or, you could just reverse the meaning of statusN (still a stupid name):

  digitalWrite(relay1, !status1);
  digitalWrite(relay2, !status2);

Lots of ways to skin that cat.