1
\$\begingroup\$

Assignment description

This is the improved code for a question I've posted earlier. 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: Traffic flow

Wiring

Wiring

Common protocol header file

#define SYNCHRONIZE "SYN"
#define SYNACK "SYN-ACK"
#define ACKNOWLEDGED "ACK"

#define BAUDRATE 9600
#define TIMEOUT 10

Master header file

#define MASTERPINS 2, 3
#define MASTEREVENT 50

Code master Arduino

#include <Arduino.h>
#include <SoftwareSerial.h>
#include "Protocol.h"
#include "Master.h"
SoftwareSerial serial(MASTERPINS); // RX, TX

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

void loop()
{
    static String readString;    // String containing serial data
    static bool sendACK = false; // Boolean regulating handshaking

    /* 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 == SYNACK)
        {
            sendACK = true;
        }
    }

    unsigned long currentTime = millis();  // Set current time
    static unsigned long previousTime = 0; // Set previous time

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

Slave header file

#define SLAVEPINS  8, 9

#define SLAVEEVENT 200

#define REDLED1 2
#define YELLOWLED1 3
#define GREENLED1 4

#define REDLED2 5
#define YELLOWLED2 6
#define GREENLED2 7

Code slave Arduino

#include <Arduino.h>
#include <SoftwareSerial.h>
#include "Protocol.h"
#include "Slave.h"
SoftwareSerial serial(SLAVEPINS); // RX, TX

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(BAUDRATE);     // SoftwareSerial connection (baudrate 9600)
    serial.setTimeout(TIMEOUT); // Maximum wait time
}

void loop()
{
    static String readString;     // String containing serial data
    static long counter = 0;      // Counter
    static bool setLight = false; // Turn lights on once

    unsigned long currentTime = millis();  // Set current time
    static unsigned long previousTime = 0; // Set previous 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 == SYNCHRONIZE)
        {
            serial.print(SYNACK);
        }
        else if (readString == ACKNOWLEDGED)
        {
            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) > SLAVEEVENT && !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 better and/or more efficiently. Thanks!

\$\endgroup\$
3
\$\begingroup\$

Separate pin definitions

Clearer and easier to maintain.

//#define SLAVEPINS  8, 9
//SoftwareSerial serial(SLAVEPINS); // RX, TX

#define SLAVEPINS_RX  8
#define SLAVEPINS_TX  9
SoftwareSerial serial(SLAVEPINS_RX, SLAVEPINS_TX);

Code guards

#ifndef _SLAVE_H
#define _SLAVE_H 1

#define SLAVEPINS  8, 9
#define SLAVEEVENT 200
....

#endif

Also see #pragma once

Suggest prefix for name space control

Consider file name

//#define SLAVEEVENT 200
//#define REDLED1 2
//#define REDLED2 5
#define SLAVE_EVENT 200
#define SLAVE_REDLED1 2
#define SLAVE_REDLED2 5

Use standard header files

Code uses bool in static bool sendACK yet lacks needed include. Do no rely on other includes to provide that definition.

#include <stdbool.h>

Use units

50 what? micro, milli, seconds?

// #define MASTEREVENT 50
#define MASTEREVENT 50 /* ms */

Hardware idea

Rather than both legs of LED hot, place one on ground and the resistor on the hot side. Safer and more common for serviceable parts (lights) to have one side ground.

///

Perhaps more later.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ #pragma once isn't portable C, although it's present in Arduino and widely implemented in other compilers. \$\endgroup\$ – S.S. Anne Mar 15 at 17:16

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.