1
void setup() {
  pinMode(13, OUTPUT);  // Red
  pinMode(14, OUTPUT);  // Yellow
  pinMode(15, OUTPUT);  // Green
}

void loop() {
  digitalWrite(13, HIGH);
  digitalWrite(14, LOW);
  digitalWrite(15, LOW);  // Stop
  delay(20000);           // Wait 20 seconds
  digitalWrite(13, HIGH);
  digitalWrite(14, HIGH);
  digitalWrite(15, LOW);  // Listen
  delay(20000);           // Wait 20 seconds
  digitalWrite(13, LOW);
  digitalWrite(14, LOW);
  digitalWrite(15, HIGH);  // Go
  delay(20000);            // Wait 20 seconds
}

How good is this code and is there any error ?

  • 1
    Other than that it goes from green to red without intermediate yellow it looks alright. – fuenfundachtzig May 27 '15 at 6:21
  • Replace all literal values with named constants. Instead of 13 define "const int RED_LIGHT = 13". It might be difficult to use this code for all traffic lights at a crossing. Define the use case. – Mikael Patel Jan 23 '16 at 9:22
1

There is nothing wrong with your code. But it could use some cleaning to make it efficient.

The code at the moment will be 'deaf' to any user inputs for twenty seconds between light changes.

This can be fixed by using a timer interrupt and changing the lights every 20 seconds and listen for other inputs or do other things. You can use the arduino timer library, this is well documented and you should be able to find examples of its use. Basically you would monitor a variable until it reaches a point and then trigger the lights.

The other option would be to use the millis() and every 20 seconds change a light, but do stuff between those 20s. This can be done like this:

unsigned long previousMillis;  //hold the previous time
unsigned long currentMillis;   // hold the current time

long threshold = 20000;
int count = 0;

int pins[] = {13, 14, 15};

//setup code stuff

//put the following at the top of loop()

currentMillis = millis();

if(currentMillis - previousMillis > threshold){

    previousMillis = currentMillis;

    for(int i = 0; i < 3; i++){
    digitalWrite(pins[i], LOW);  //turn off all

    }

    if(count > 2) count = 0;

    digitalWrite(pins[count], HIGH);

    count++;
}
  • I don't think this works as a traffic light sequence, I might be reading it wrong, but I think this just switches a light on every few seconds. – Code Gorilla Nov 23 '15 at 14:00
  • 1
    @Matt - the code does switch a light on every few seconds, 20s, but that is as it is an example based off the OPs code where they used 20 second delay for each transition. The code does not change the sequence to match traffic lights but that would be each with a few if() statements to change the delay and keep the amber on. – RSM Nov 24 '15 at 6:59
0

I have a similar requirement, this gives you a little more flexibility , My traffic light code is controlled by a PIR outside my door , if the PIR detects an individual the lights turn to stop when the PIR clears the lights turn to green.

the code and hardware are still in the test phase.

#include <EasyTransfer.h>

int D_Status_Word = 0;
int Status_Word = 0; //sets all bits to  zero
int Light_Threshold = 150; //the light level that constitutes an unusual entry
int Active = 0;
int Passive = 100;
int Counter = 100;
int Green_Pin = 7;
int Amber_Pin = 8;
int Red_Pin = 9;
int Photocell_Pin = 1;     // the cell and 10K pulldown are connected to a0
int Photocell_Reading;
int Sensor_Frequency = 0;

struct SEND_DATA_STRUCTURE {
  //put your variable definitions here for the data you want to receive
  //THIS MUST BE EXACTLY THE SAME ON THE OTHER ARDUINO
  int Out_Word;
  int LDR_Level;
};

SEND_DATA_STRUCTURE My_Tx_Data;

//create an object for themessage
EasyTransfer ETout;
int D_Sensor_Pin [] = {
  2, 3, 5, 6
};
// pin hallwaypir, 3 doorpir, 5 door sensor, 6 hallpir

void setup() {
  Serial.begin (9600);
  // put your setup code here, to run once:
  ETout.begin(details(My_Tx_Data), &Serial);
  for (int i = 0; i <= 3; i++)// read the two state sensors
  {
    pinMode(D_Sensor_Pin [i], INPUT);
  }
  pinMode(A1, INPUT);//set the analoge u ldr pin ot input mode
  pinMode (Green_Pin, OUTPUT);
  pinMode (Amber_Pin , OUTPUT);
  pinMode (Red_Pin , OUTPUT);
}

void loop() {
  Scan_Sensors();
  if (D_Status_Word != Status_Word)
  {
    Sensor_Frequency = Active; // high frequency sampling because a sensor has activated
    //send a data packet to the mega with mem card via serial.
    Send_Data_Packet();
  }
  if (bitRead (Status_Word, 1) != bitRead (D_Status_Word, 1))
  {
    if ((D_Status_Word, 1) == 1)
    {
      Traffic_Light_Stop();
    }
    else
    {
      Traffic_Light_Go();
    }
  }
  Status_Word = D_Status_Word;

  delay(Sensor_Frequency);// 1000= waits for a second
}

void Scan_Sensors()
{
  byte readings = 0;
  Photocell_Reading  = 0;
  for (int i = 0; i <= 3; i++)// read the two state sensors
  {
    bitWrite(readings, i, digitalRead(D_Sensor_Pin [i] ));
  }
  //test to see if any sensors have trivggered (readings>0)
  if (readings > 0)
  { // a 2 state sensor has triggered, reset the Counter to 100
    Counter = 100;
  }

  //read threshold sensors
  Photocell_Reading  = analogRead(Photocell_Pin);
  {
    bitSet(readings, 4); // 4 becaus there are 4 2 state sensors (0 to 3)
    //we write 1 because thelight is below the threshold
    Serial.print ("photocell Reading = ");
    Serial.println(Photocell_Reading);
  }
  D_Status_Word = readings;
  Counter -= 1; //decrement the Counter
  if (Counter <= 0)
  { //the Counter has counted 100 scans since a 2state has triggered
    //rest freqenvcy to Passive
    Sensor_Frequency = Passive;

  }
}
void Send_Data_Packet()
{
  My_Tx_Data.Out_Word = D_Status_Word;
  My_Tx_Data.LDR_Level = Photocell_Reading;
  Serial.print ("photocell Reading = ");
  Serial.println(Photocell_Reading);
  Serial.print ("status word = ");
  Serial.println(D_Status_Word);
  ETout.sendData();
}

void Traffic_Light_Stop()
{
  //lights turn from green to yellow to red
  digitalWrite (Green_Pin, LOW);
  digitalWrite (Amber_Pin, HIGH);
  digitalWrite (Red_Pin, LOW);
  delay(750);
  digitalWrite (Green_Pin, LOW);
  digitalWrite (Amber_Pin, LOW);
  digitalWrite (Red_Pin, HIGH);
}

void Traffic_Light_Go()
{
  //lights turn red to red and yellow to green
  digitalWrite (Green_Pin, LOW);
  digitalWrite (Amber_Pin, HIGH);
  digitalWrite (Red_Pin, HIGH);
  delay(750);
  digitalWrite (Green_Pin, HIGH);
  digitalWrite (Amber_Pin, LOW);
  digitalWrite (Red_Pin, LOW);

}

0

There is nothing wrong with the code that I can see although you do have some redundant statements: When you go to "Listen" you set the red and green pins high and low respectively, but they are already high and low from the previous state. Not a big issue, being verbose adds clarity so there is some argument for leaving it as is.

I would consider using #define macros to number the ports and set the delay times. This adds readability and also means you can make changes easily without having to search through the code and match up appropriate instances of the port number that you want to change, for example:

#define RED_PIN 13
#define DELAY_BETWEEN_CHANGES 20000
...
digitalWrite(RED_PIN, HIGH);
delay(DELAY_BETWEEN_CHANGES);

This also means that you can get rid of comments in your code as it becomes pretty self explanatory. I personally believe that comments are bad. If you write code and put a comment explaining what it does, when you have to modify the code you also have to modify the comment. Make the code understandable in the first place, no need for comments.

Final point (not related to code): What happens after "Go"? I'm from the UK so we have another phase after Go. It reminds me of an old driving test joke: Instructor: What is the sequence of traffic lights? Student: Red, red and amber, green. Instructor: What comes after green? Student: How would I know? I've always gone by then!

  • also in this case not so important but in large projects where you reuse the information, the variable can save some space, specially if you use the ATTiny85 that only have 8 kb flash memory – Magic-Mouse Oct 24 '15 at 13:44

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.