2
\$\begingroup\$

Assignment description

I'd like to receive feedback on an assignment which I'm currently working on. I have to make two traffic lights which allow traffic to run smoothly. I'm using a master Arduino which handles the main serial communication and one slave Arduino which handles the light sequence. Both lights turn red if the master Arduino is disconnected. I must use the SoftwareSerial library. The situation is shown below: 1] Wiring enter image description here Code master Arduino

#include <Arduino.h>
#include <SoftwareSerial.h>

SoftwareSerial serial(2, 3); // RX, TX

// Timer
unsigned long currentTime;
unsigned long previousTime = 0;
const unsigned long event = 50;

String readString = ""; // String containing serial data
bool sendACK = false;  // Boolean regulating handshaking

void setup()
{
  Serial.begin(9600); // Native USB connection (baudrate 9600)
  Serial.setTimeout(10);
  while (!Serial)
  {
    ; // Wait for serial port to connect
  }
  serial.begin(9600);    // SoftwareSerial connection (baudrate 9600)
  serial.setTimeout(10); // Maximum wait time
}

void loop()
{
  currentTime = millis(); // Set current time

  /* Set string to incoming data. Once request to 'three-way handshake' has been acknowledged,
  set boolean sendACK to true, allowing "ACK" to be sent. */
  if (serial.available() > 0)
  {
    while (serial.available() > 0)
    {
      readString = serial.readString();
      Serial.print(readString);
    }
    if (readString == "SYN-ACK/")
    {
      sendACK = true;
    }
  }

  /* Send "SYN" to initiate 'three-way handshake'. Once connection established, send "ACK" */
  if ((currentTime - previousTime) > event)
  {
    if (sendACK == false)
    {
      serial.print("SYN/");
      Serial.print("SYN/");
    }
    else if (sendACK == true)
    {
      serial.print("ACK/");
      Serial.print("ACK/");
      sendACK = false;
    }
    previousTime = currentTime;
  }
}

Code slave Arduino

#include <Arduino.h>
#include <SoftwareSerial.h>

SoftwareSerial serial(8, 9); // RX, TX

// First traffic light LED's
const int redLED1 = 2;
const int yellowLED1 = 3;
const int greenLED1 = 4;

// Second traffic light LED's
const int redLED2 = 5;
const int yellowLED2 = 6;
const int greenLED2 = 7;

long counter = 0; // Counter

bool setLight = false;

// Timer
unsigned long currentTime;
unsigned long previousTime = 0;
const unsigned long event = 200;

String readString = ""; // String containing serial data

void setup()
{
  // Multiple LED outputs
  pinMode(redLED1, OUTPUT);
  pinMode(yellowLED1, OUTPUT);
  pinMode(greenLED1, OUTPUT);
  pinMode(redLED2, OUTPUT);
  pinMode(yellowLED2, OUTPUT);
  pinMode(greenLED2, OUTPUT);

  serial.begin(9600); // SoftwareSerial connection (baudrate 9600)
  serial.setTimeout(10); // Maximum wait time
}

void loop()
{
  currentTime = millis(); // Set current time

  /* Set string to incoming data. Establish 'three-way handshake' and count the amount
  of times handshaking performed. Set lights according to the handshakes counted. */
  if (serial.available() > 0)
  {
    while (serial.available() > 0)
    {
      readString = serial.readString();
    }
    if (readString == "SYN/")
    {
      serial.print("SYN-ACK/");
    }
    else if (readString == "ACK/")
    {
      switch (counter)
      {
      case 0:
        digitalWrite(redLED1, HIGH);
        digitalWrite(redLED2, HIGH);
        break;

      case 10:
        digitalWrite(greenLED1, HIGH);
        digitalWrite(redLED1, LOW);
        break;

      case 40:
        digitalWrite(yellowLED1, HIGH);
        digitalWrite(greenLED1, LOW);
        break;

      case 50:
        digitalWrite(redLED1, HIGH);
        digitalWrite(yellowLED1, LOW);
        break;

      case 60:
        digitalWrite(greenLED2, HIGH);
        digitalWrite(redLED2, LOW);
        break;

      case 90:
        digitalWrite(yellowLED2, HIGH);
        digitalWrite(greenLED2, LOW);
        break;

      case 100:
        digitalWrite(redLED2, HIGH);
        digitalWrite(yellowLED2, LOW);
        break;
      }
      counter++;
      if (counter > 100)
      {
        counter = 0;
      }
    }
    setLight = false;
    previousTime = currentTime;
  }
  // If handshake did not work, wait for 200 milliseconds and set traffic lights to red.
  else if ((currentTime - previousTime) > event && !setLight)
  {
    digitalWrite(redLED1, HIGH);
    digitalWrite(redLED2, HIGH);
    digitalWrite(yellowLED1, LOW);
    digitalWrite(yellowLED2, LOW);
    digitalWrite(greenLED1, LOW);
    digitalWrite(greenLED2, LOW);
    counter = 0;
    setLight = true;
  }
}

Please give me feedback on my code and which parts I could do more efficiently. Thanks in advance!

\$\endgroup\$
2
  • 2
    \$\begingroup\$ Welcome to CodeReview@SE. How is this c++? \$\endgroup\$
    – greybeard
    Mar 14, 2020 at 7:36
  • 1
    \$\begingroup\$ I have rolled this back to the original code. Please do not update it as a result of feedback; open a new question if you want another round of feedback. \$\endgroup\$
    – Reinderien
    Mar 14, 2020 at 15:31

1 Answer 1

3
\$\begingroup\$

Use a common protocol file

Any common constants, such as

  • 9600
  • SYN, ACK, etc. strings

should be #defined in one place, in this case a .h file accessible to both Arduino projects.

Indentation

One-space indentation is essentially never used. Typically four is used instead.

Globals

In the master code, every single one of those globals should be moved to loop() and declared static. That way they will retain their value through multiple calls but will also have their scope reduced.

Efficiency

I realize a lot of these may fall outside of the constraints of your assignment; nevertheless:

  • Don't use software-serial; use a hardware serial port
  • Don't use Arduino; use smaller microcontrollers that are better-suited to your application
  • Don't use ASCII string signalling; use single-byte constants. If fidelity is a concern, add a checksum.
\$\endgroup\$
4
  • \$\begingroup\$ Thanks for your feedback! I've made the following changes (and edited the code in the main question): 1. Added a header file. I don't know if I should make 3 header files (master Arduino constants, slave Arduino constants and common protocol) or one header like I made. 2. Changed constants and moved most to loop() function. 3. Changed indentation to four. Could you check if I did it correctly? Thanks again :) \$\endgroup\$
    – Stefan2000
    Mar 14, 2020 at 15:10
  • 1
    \$\begingroup\$ Please do not edit the code in the main question as a result of feedback - CR policy is that you open a new question. \$\endgroup\$
    – Reinderien
    Mar 14, 2020 at 15:29
  • 1
    \$\begingroup\$ I don't know if I should make 3 header files (master Arduino constants, slave Arduino constants and common protocol) or one header like I made. 3, in my opinion. \$\endgroup\$
    – Reinderien
    Mar 14, 2020 at 15:32
  • \$\begingroup\$ Alright I will open a new question and split the header file into 3 \$\endgroup\$
    – Stefan2000
    Mar 14, 2020 at 15:53

This site is temporarily in read-only mode and not accepting new answers.

Not the answer you're looking for? Browse other questions tagged .