Is this a good way of writing a module for traffic lights?

Here im trying to write some code for traffic lights without using delays, there are 3 different sets, both light have equal priority, set 1 has priority and set 2 has priority.

I was just wondering if this was a good way of doing it or is there something much more simple?

#define LEDS_OFF B00000000
#define LR B10000000//PIN7 R
#define LY B01000000//PIN6 Y
#define LG B00100000//PIN5 G
#define RG B00010000//PIN4 GG
#define RY B00001000//PIN3 YY
#define RR B00000100//PIN2 RR

int arrayCounter = 0, arrayCounter1 = 0;

unsigned long LS1, LS2, LS3;
const byte LightsSet1[] = { LY, LR, LR | LY, LG, LY, LR, LR, LR, LR };
const byte LightsSet2[] = { RY, RR, RR, RR, RR, RR, RR | RY, RG, RY };
int T_SET1[] = { 0, 2, 1, 1, 5, 1, 1, 1, 3, 1 };
int T_SET2[] = { 0, 2, 1, 1, 5, 1, 1, 1, 3, 1 };


void TrafficPriority(bool tfLights) {
  if (tfLights) {
    if (((long)(currentTime - LS1) > T_SET1[arrayCounter] * 1000)) {
      PORTD = LightsSet1[arrayCounter] | LightsSet2[arrayCounter1];
      LS1 = millis();
      arrayCounter = (arrayCounter + 1);
      if (arrayCounter >= 8) {
        arrayCounter = 0;
      }
    }

    if (((long)(currentTime - LS2) > T_SET2[arrayCounter1] * 1000)) {
      PORTD = LightsSet2[arrayCounter1] | LightsSet1[arrayCounter];
      LS2 = millis();
      arrayCounter1 = (arrayCounter1 + 1);
      if (arrayCounter1 == 8) {
        arrayCounter1 = 0;
      }
    }

  }
}

Later in the code I can change some of the timings in the T_SET1[] or the T_SET2[] arrays depending on which has priority.

Everything works so far, Im just looking for advice on if its good or stupid or over complicated.

Thank you very much.

Definitely over complicated. Using direct port access is completely unnecessary and makes the code more complex and less portable. Just use digitalWrite().

To save you future challenges, anything dealing with time or millis() should be unsigned long. force constants to be unsigned long too by using the ul suffix ➜ 1000ul

+1 on direct port manipulation, or if you care about performance, then don't do the x 1000 multiplication, store the right value in the array

I don't see any masking with & so you never reset pins ?

Never any | with current port values, so all pins are set/reset every time.

Use comments to tell what's done. That will help even You maintaning the code in the future.

Using direct port manipulation - why do you think that is necessary?

... or ...

      arrayCounter1 = (arrayCounter1 + 1) % 8;

Not sure why you would pass a boolean to a function that controls whether the function does anything or not. Why don't you make that decision in the calling code.

Would be better if you posted all the code so we could better understand what you are actually trying to do.

ah, I did not check the details, just saw | being used like in

but indeed, it's a full write not a |= (possibly messing around with pin 0 and 1?)

Well I can choose to use port manipulation or just digitalWrite(), I used it becuase I think it saves space and is easier for me to call it later in the code since I use those LEDS in lots of different places and for different modules. So instead of turning each LED off with digitalwrite(), I can just doPORD = LEDS_OFF, etc...

As for this arrayCounter1 = (arrayCounter1 + 1) % 8; Im not really sure what that does. Is it the same as what I had and only goes up to 8 then resets to 0?

The full project has lots of different Modules and Im using an Arduino nano, an ESP826 That communicate over a webpage, to control a Accelerometer, an RGB, 6 LEDS and a 7 seg display that is using a serial bus.

Here is the full code for the Arduino Side if youre interested.

//TASK LIST
//1. Resource Manager Finite State Machine (FSM)
//2. A multi-functional traffic lights controller
//3. Barrier crossing controller
//4. Motor racing lights controller
//5. A 3-colour LED controller [DONE]
//6. Two switch-debounce modules [DONE]
//7. A heartbeat module [DONE]
//8. A Tilt-Sensor and Thermometer based on the MPU6050
//9. A simple Command Interpreter
//10. Scheduler(s)

#include <Wire.h>

const byte Characters[] = {

  0b01100000, //1 0
  0b11011010, //2 1
  0b00111110, //B 2
  0b10011110, //E 3
  0b10001110, //F 4
  0b00000001, //DP 5
  0b00011110, //t 6
  0b01111100, //U 7
  0b00011100, //L 8
  0b10000100, //I 9
  0b00000110, //R 10
  0b00000000  //OFF 11
};

unsigned long currentTime;
byte SegDSPL;
//==============================RGB FADE LEDS=====================================
#define RED 9
#define BLUE 10
#define GREEN 11

int color = 8;
int i = 0;
bool fade = false;


//================================================================================================//
//====================================SW1 MODULE (MODE SELECT)====================================//
//================================================================================================//

unsigned long SW1_LastDT = 0;
int SW1, SW1_State, SW1_lastState = 1, SW1flickState = 0;

int Module = 0;
int Mode = 0;
int Trains = 0;

#define button1 A0

void switchTwo() {

  SW1 = digitalRead(A0);
  //unsigned long currentTime = millis();
  if (SW1 == LOW) {
    SW1_State = 0; // Replicating digitalRead() function.
  }
  else {
    SW1_State = 1;
  }
  if (SW1_State != SW1flickState) {
    // reset the debouncing timer
    SW1_LastDT = millis();
    // save the the last flickerable state
    SW1flickState = SW1_State;
  }
  // Checks that the apporriate time has passed, basically a delay();
  if (((long)(currentTime - SW1_LastDT) >= 300)) {
    if (SW1_lastState == 1 && SW1_State == 0) {

      Mode = Mode + 1;
      Trains = Trains + 1;
    }

    SW1_lastState = SW1_State;
  }

}

//================================================================================================//
//====================================SW2 MODULE (MODULE SELECT)==================================//
//================================================================================================//

unsigned long SW2_LastDT = 0;
int SW2, SW2_State, SW2_lastState = 1, SW2flickState = 0;


#define button2 A1

void switchOne() {

  SW2 = digitalRead(A1);
  //unsigned long currentTime = millis();
  if (SW2 == LOW) {
    SW2_State = 0; // Replicating digitalRead() function.
  }
  else {
    SW2_State = 1;
  }
  if (SW2_State != SW2flickState) {
    // reset the debouncing timer
    SW2_LastDT = millis();
    // save the the last flickerable state
    SW2flickState = SW2_State;
  }
  // Checks that the apporriate time has passed, basically a delay();
  if (((long)(currentTime - SW2_LastDT) >= 300)) {
    if (SW2_lastState == 1 && SW2_State == 0) {

      Module = Module + 1;
      Trains = Trains - 1;

    }
    SW2_lastState = SW2_State;
  }
}
//================================================================================================//
//===================================FSM MODULE===================================================//
//================================================================================================//

#define GRANTED A2
#define GRANTED_H pinMode(GRANTED,INPUT_PULLUP);
#define GRANTED_L digitalWrite(GRANTED,LOW); pinMode(GRANTED,OUTPUT);

// State definitions
#define NoTriggerNoDemand    0   // idle state
#define TriggerIamMaster     1   // we are master after a successful claim
#define IamSlave             2   // we are slave after other device has made a successful claim

bool ACCEL = false;
bool TRIGGER;
int STATE;
int timingState, Counter;
char data, response;
unsigned long lastTime, lastTime_1 = 0;
//unsigned long currentTime;

//=================================================================================
void leaveHigh(unsigned char pin) {
  pinMode(pin, INPUT_PULLUP);
}
//=================================================================================
void pullLow(unsigned char pin) {
  digitalWrite(pin, LOW);
  pinMode(pin, OUTPUT);
}
//=================================================================================
bool demandREQ() {
  return (!(analogRead(A6) >> 8));
}
//=================================================================================
// function that executes whenever data is requested from master
void requestEvent() {
  Wire.write("ACK"); /*send string on request */
}
//=================================================================================

void receiveEvent(int howMany) {
  while (0 < Wire.available()) {
    data = Wire.read(); /* receive byte as a character */
    //Serial.print(data); /* print the character */
  }
  //Serial.println(); /* to newline */
}
//=================================================================================

void FSM() {

  switch (STATE) {
    case NoTriggerNoDemand: //No trigger, No demand
      GRANTED_H;
      if (TRIGGER == true) {
        STATE = 1;
      }
      else if (!TRIGGER && demandREQ()) {
        STATE = 2;
      }
      break;
    case TriggerIamMaster: // I am master
      GRANTED_H;
      if (TRIGGER == false) {
        STATE = 0;
      }

      break;
    case IamSlave: // I am Slave
      GRANTED_L;
      if (!demandREQ()) {
        STATE = 0;
      }
      break;
  }
  if (((long)(currentTime - lastTime_1)) >= 1) {
    TimerModule();
    lastTime_1 = currentTime;
  }
}

////////////////////////////////////////////////////////////////////////////////////////////////////
////////////////////////////////////////////////////////////////////////////////////////////////////

void TimerModule() {
  switch (timingState) {
    case 0:

      timingState = 1;
      lastTime = millis();

      break;
    case 1:

      {
        currentTime = millis();
        if (((long)(currentTime - lastTime)) > 17) {
          TRIGGER = true;
          lastTime = millis();
          timingState = 2;
        }
        else {
          timingState = 1;
        }
      }

      break;
    case 2:

      if (STATE == TriggerIamMaster) { //State 1 is MASTER state
        timingState = 3;
      }
      else {
        timingState = 2;
      }

      break;
    case 3:

      receiveEvent(1);
      TRIGGER = false;
      timingState = 0;

      break;
  }
}



//================================================================================================//
//===================================HEARTBEAT MODULE=============================================//
//================================================================================================//

unsigned long HB_LastTime = 0;
byte DP_ON_OFF;
void HeartBeatMod() {

  if (((long)(currentTime - HB_LastTime)) >= 500) {
    DP_ON_OFF = Characters[5];

  }
  if (((long)(currentTime - HB_LastTime)) >= 1000) {
    DP_ON_OFF = Characters[11];

    HB_LastTime = millis();
  }
}
//================================================================================================//
//===================================BIT BANGING MODULE===========================================//
//================================================================================================//

#define LATCH 13
#define CLOCK 12
#define DATA 8

void bitBang(unsigned char b) {
  for (int a = 0; a < 8; a++) {
    if ((b >> a) & (0x01)) {
      digitalWrite(DATA, HIGH);
    }
    else {
      digitalWrite(DATA, LOW);
    }
    digitalWrite(CLOCK, HIGH);
    digitalWrite(CLOCK, LOW);
  }
  digitalWrite(LATCH, HIGH);
  digitalWrite(LATCH, LOW);
}



//================================================================================================//
//==================================RGB LIGHTS====================================================//
//================================================================================================//

int RGBLastTime;

void RGB(int R, int G, int B) {
  analogWrite(RED, R);
  analogWrite(GREEN, G);
  analogWrite(BLUE, B);
}

void ColorFade() {
  Serial.println(i);
  if (!fade) {
    if (((long)(currentTime - RGBLastTime)) > 4) {
      i++;
      if (i == 255) {
        fade = true;
      }
      RGBLastTime = millis();
    }
  }

  if (fade) {
    if (((long)(currentTime - RGBLastTime)) > 4) {
      i--;
      if (i == 0) {
        fade = false;
      }
      RGBLastTime = millis();
    }
  }
}



//================================================================================================//
//===================================ACCELERMOTERE MODULE=========================================//
//================================================================================================//



//================================================================================================//
//===================================ON OFF MODULE SORTING=========================================//
//================================================================================================//

bool Equal = false, Set1 = false, Set2 = false, Maintenance = false, Crossing = false, Racing = false, 
ledOFF = false, Amber = false, Blue = false, Green = false, Red = false;


void sortingModule() {

  //----------------------------------- Traffic Lights ------------------------

  if (data == 0x61) {
    TrafficPriority(true);
    Equal = true;
    Set1 = Set2 = Maintenance = Crossing = Racing = false;
  }
  if (data == 0x62) {
    TrafficPriority(true);
    Set1 = true;
    Equal = Set2 = Maintenance = Crossing = Racing = false;
  }
  if (data == 0x63) {
    TrafficPriority(true);
    Set2 = true;
    Set1 = Equal = Maintenance = Crossing = Racing = false;
  }
  if (data == 0x64) {
    TrafficPriority(false);
    Crossing = true;
    Set1 = Set2 = Maintenance = Equal = Racing = false;
  }
  if (data == 0x65) {
    TrafficPriority(false);
    Maintenance = true;
    Set1 = Set2 = Equal = Crossing = Racing = false;
  }

  if (data == 0x66) {
    TrafficPriority(false);
    Racing = true;
    Set1 = Set2 = Maintenance = Crossing = Equal = false;
  }

  //----------------------------------- RGB Lights ------------------------

  if (data == 0x67) {
    RGB(0, 0, 0);
    Blue = Green = Red = Amber = false;
  }

  if (data == 0x68) {
    Amber = true;
    Blue = Green = Red = false;
  }

  if (data == 0x69) {
    Blue = true;
    Amber = Green = Red = false;
  }

  if (data == 0x6A) {
    Green = true;
    Blue = Amber = Red = false;
  }

  if (data == 0x6B) {
    Red = true;
    Blue = Green = Amber = false;
  }
}

//================================================================================================//
//====================================MULTIFUNCTIONAL TRAFFIC LIGHTS MODULES======================//
//================================================================================================//

#define LEDS_OFF B00000000
#define LR B10000000//PIN7 R
#define LY B01000000//PIN6 Y
#define LG B00100000//PIN5 G
#define RG B00010000//PIN4 GG
#define RY B00001000//PIN3 YY
#define RR B00000100//PIN2 RR

//===============================================================================================//
int arrayCounter = 0, arrayCounter1 = 0;
unsigned long LS1, LS2, LS3;
const byte LightsSet1[] = { LY, LR, LR | LY, LG, LY, LR, LR, LR, LR };
const byte LightsSet2[] = { RY, RR, RR, RR, RR, RR, RR | RY, RG, RY };
int T_SET1[] = { 0, 2, 1, 1, 5, 1, 1, 1, 3, 1 };
int T_SET2[] = { 0, 2, 1, 1, 5, 1, 1, 1, 3, 1 };


void TrafficPriority(bool tfLights) {
  if (tfLights) {
    if (((long)(currentTime - LS1) > T_SET1[arrayCounter] * 1000)) {
      PORTD = LightsSet1[arrayCounter] | LightsSet2[arrayCounter1];
      LS1 = millis();
      arrayCounter = (arrayCounter + 1);
      if (arrayCounter >= 8) {
        arrayCounter = 0;
      }
    }

    if (((long)(currentTime - LS2) > T_SET2[arrayCounter1] * 1000)) {
      PORTD = LightsSet2[arrayCounter1] | LightsSet1[arrayCounter];
      LS2 = millis();
      arrayCounter1 = (arrayCounter1 + 1);
      if (arrayCounter1 == 8) {
        arrayCounter1 = 0;
      }
    }

  }
}

//--------------------------------------------------Racing Lights-------------------------------//

int racing_aCounter;
unsigned long r_lastTime;
int r_Timing[] = { 1, 1, 1, 1, r_Random , 2 , 0.5 , 0.5 };


void racingLights(){
  if(Mode <= 0){
    PORD = LEDS_OFF;
  }

  if(Mode == 1){
    
  }
}

void setup() { //-----------------------------------SETUP----------------------------------------//

  ////////////////////////////////////////////////////////////////////
  leaveHigh(GRANTED);
  TRIGGER = false;
  STATE = 0;
  timingState = 0;
  Wire.begin(8); /* join i2c bus with address 8 */
  Wire.onReceive(receiveEvent); /* register receive event */
  Wire.onRequest(requestEvent); /* register request event */
  /////////////////////////////////////////////////////////////////////


  pinMode(DATA, OUTPUT);
  pinMode(LATCH, OUTPUT);
  pinMode(CLOCK, OUTPUT);
  DDRD |= B11111100;
  pinMode(button1, INPUT_PULLUP);
  pinMode(button2, INPUT_PULLUP);
  Serial.begin(9600);
  pinMode(RED, OUTPUT);
  pinMode(BLUE, OUTPUT);
  pinMode(GREEN, OUTPUT);

}

int T_barrierCrossing[] = { 5000, 500, 500, 500 };
const byte L_barrierCrossing[] = { LY | RY, LR | RR, LR, RR };
bool Exit = false;
unsigned long mlastTime, clastTime, cArrayCounter;


void loop() { //-----------------------------------LOOP----------------------------------------//
  currentTime = millis(); // Main track of time used in all modules
  FSM();

  switchOne();
  switchTwo();
  HeartBeatMod();
  bitBang(DP_ON_OFF);

  sortingModule();

  if (Equal) { // Equal Priority

    bitBang(Characters[3] | DP_ON_OFF);
    T_SET1[4] = 5;
    T_SET1[8] = 3;
    T_SET2[4] = 5;
    T_SET2[8] = 3;
  }

  if (Set1) { // Set 1 Priority

    bitBang(Characters[0] | DP_ON_OFF);
    T_SET1[4] = 7;
    T_SET1[8] = 7;
    T_SET2[4] = 5;
    T_SET2[8] = 3;
  }

  if (Set2) {  // Set 2 Priority

    bitBang(Characters[1] | DP_ON_OFF);
    T_SET1[4] = 5;
    T_SET1[8] = 3;
    T_SET2[4] = 3;
    T_SET2[8] = 5;
  }

  if (Maintenance) {  // Maintenance Mode
    bitBang(Characters[6] | DP_ON_OFF); // If maintenance mode 7SEG = t.
    if (((long)(currentTime - mlastTime) >= 500)) {
      PORTD = LY | RY;
    }
    if (((long)(currentTime - mlastTime) >= 1000)) {
      PORTD = LEDS_OFF;
      mlastTime = millis();
    }
  }

  if (Crossing) {  // Barrier Crossing
    bitBang(Characters[2] | DP_ON_OFF);
    if (Trains <= 0) {
      PORTD = LEDS_OFF;
      cArrayCounter = -1;
    }

    else if (Trains >= 1) {
      PORTD = L_barrierCrossing[cArrayCounter];
      if ((long)(currentTime - clastTime) >= T_barrierCrossing[cArrayCounter]) {
        cArrayCounter = cArrayCounter + 1;
        clastTime = millis();
        if (cArrayCounter == 4) {
          cArrayCounter = 2;
        }
      }
    }
  }

  //----------------------------------- RGB Lights ------------------------
  if (Amber) {
    ColorFade();
    RGB(i, i / 10, 0);
  }
  if (Blue) {
    ColorFade();
    RGB(0, 0, i);
  }
  if (Green) {
    ColorFade();
    RGB(0, i, 0);
  }
  if (Red) {
    ColorFade();
    RGB(i, 0, 0);
  }











}

Im sorry but im not sure what the difference between | and |= is, im just trying to run 2 sets of 3 LEDS at the same time without affecting each other.

When you assign a byte to PORTD you modify all the pins associated with that port. On a UNO portD covers pins fr 0 to 7 so you would be modifying the Serial pin state possibly

2 posts were split to a new topic: Schematic for some random thing

This topic was automatically closed 180 days after the last reply. New replies are no longer allowed.