Model railway multi-tasking - state machine - millis() issues

Hi, I asked a question a few days ago and have been working on my solution.
I have been building a model railway but don’t have the space for a full loop. I have installed LDR sensors under the track at each end and have managed to get the trains to stop, wait and reverse automatically. However I am trying to allow both parallel lines to run at the same time with both trains stopping and reversing when they reach the ends of their respective tracks.
I knew that I couldn’t use the delay function which had worked for the individual lines because the other lines sensors would be inoperable during the delay function of the other line. I was advised to try and incorporate state machine mechanics and to study the ‘blink without delay’ and ‘millis()’ functions.
I have done my best to rewrite my codes in these formats but the system now seems to do nothing that I want it to and the LDRs don’t even stop the trains anymore.
I’m still a novice Arduino programmer so feel feel free to suggest any fixes or simply point out any obvious mistakes I might have made with my code.

const int relayPin01 = 13;
const int relayPin02 = 12;
const int relayPin03 = 11;
const int relayPin04 = 10;
const int relayPin05 = 9;
const int relayPin06 = 8;
const int relayPin07 = 7;
const int relayPin08 = 6;
const int ldr_A_Pin = A0;
const int ldr_B_Pin = A1;
const int ldr_C_Pin = A2;
const int ldr_D_Pin = A3;

unsigned long previousMillis = 0;

const long interval01 = 2000;
const long interval02 = 2000;
const long interval03 = 2000;

void setup() {

    Serial.begin(9600);
  
  pinMode (relayPin01, OUTPUT);
  pinMode (relayPin02, OUTPUT);
  pinMode (relayPin03, OUTPUT);
  pinMode (relayPin04, OUTPUT);
  pinMode (relayPin05, OUTPUT);
  pinMode (relayPin06, OUTPUT);
  pinMode (relayPin07, OUTPUT);
  pinMode (relayPin08, OUTPUT);
  pinMode (ldr_A_Pin, INPUT);
  pinMode (ldr_B_Pin, INPUT);
  pinMode (ldr_C_Pin, INPUT);
  pinMode (ldr_D_Pin, INPUT);
  digitalWrite(relayPin01, HIGH);
  digitalWrite(relayPin02, HIGH);
  digitalWrite(relayPin03, HIGH);
  digitalWrite(relayPin04, HIGH);
  digitalWrite(relayPin05, HIGH);
  digitalWrite(relayPin06, HIGH);
  digitalWrite(relayPin07, HIGH);
  digitalWrite(relayPin08, HIGH);
  
}

void loop(){

unsigned long currentMillis = millis();

int ldrStatus01 = analogRead(ldr_A_Pin);
int ldrStatus02 = analogRead(ldr_B_Pin);
int ldrStatus03 = analogRead(ldr_C_Pin);
int ldrStatus04 = analogRead(ldr_D_Pin);

if (ldrStatus01 >=400){
  previousMillis = currentMillis;
  
  if(currentMillis - previousMillis <= interval01){
    digitalWrite(relayPin03, !digitalRead(relayPin03));
    digitalWrite(relayPin04, !digitalRead(relayPin04));
    previousMillis = currentMillis;

    if(currentMillis - previousMillis <= interval02){
      digitalWrite(relayPin01, !digitalRead(relayPin01));
      digitalWrite(relayPin02, !digitalRead(relayPin02));
      previousMillis = currentMillis;

      if(currentMillis - previousMillis <= interval03){
        digitalWrite(relayPin03, !digitalRead(relayPin03));
        digitalWrite(relayPin04, !digitalRead(relayPin04));
        previousMillis = currentMillis;
      }
    }
  }
}

if (ldrStatus02 >=400){
    previousMillis = currentMillis;
  
  if(currentMillis - previousMillis <= interval01){
    digitalWrite(relayPin03, !digitalRead(relayPin03));
    digitalWrite(relayPin04, !digitalRead(relayPin04));
    previousMillis = currentMillis;

    if(currentMillis - previousMillis <= interval02){
      digitalWrite(relayPin01, !digitalRead(relayPin01));
      digitalWrite(relayPin02, !digitalRead(relayPin02));
      previousMillis = currentMillis;

      if(currentMillis - previousMillis <= interval03){
        digitalWrite(relayPin03, !digitalRead(relayPin03));
        digitalWrite(relayPin04, !digitalRead(relayPin04));
        previousMillis = currentMillis;
      }
    }
  }
}
if (ldrStatus03 >=400){
    previousMillis = currentMillis;
  
  if(currentMillis - previousMillis <= interval01){
    digitalWrite(relayPin07, !digitalRead(relayPin07));
    digitalWrite(relayPin08, !digitalRead(relayPin08));
    previousMillis = currentMillis;

    if(currentMillis - previousMillis <= interval02){
      digitalWrite(relayPin05, !digitalRead(relayPin05));
      digitalWrite(relayPin06, !digitalRead(relayPin06));
      previousMillis = currentMillis;

      if(currentMillis - previousMillis <= interval03){
        digitalWrite(relayPin07, !digitalRead(relayPin07));
        digitalWrite(relayPin08, !digitalRead(relayPin08));
        previousMillis = currentMillis;
      }
    }
  }
}
if (ldrStatus03 >=400){
    previousMillis = currentMillis;
  
  if(currentMillis - previousMillis <= interval01){
    digitalWrite(relayPin07, !digitalRead(relayPin07));
    digitalWrite(relayPin08, !digitalRead(relayPin08));
    previousMillis = currentMillis;

    if(currentMillis - previousMillis <= interval02){
      digitalWrite(relayPin05, !digitalRead(relayPin05));
      digitalWrite(relayPin06, !digitalRead(relayPin06));
      previousMillis = currentMillis;

      if(currentMillis - previousMillis <= interval03){
        digitalWrite(relayPin07, !digitalRead(relayPin07));
        digitalWrite(relayPin08, !digitalRead(relayPin08));
        previousMillis = currentMillis;
      }
    }
  }
}
}
  pinMode (relayPin01, OUTPUT);
  pinMode (relayPin02, OUTPUT);
  pinMode (relayPin03, OUTPUT);
  pinMode (relayPin04, OUTPUT);
  pinMode (relayPin05, OUTPUT);
  pinMode (relayPin06, OUTPUT);
  pinMode (relayPin07, OUTPUT);
  pinMode (relayPin08, OUTPUT);

When variable names start getting numeric suffixes, it’s a good time to start thinking about arrays.

After a very quick skim of your code, I’d say you need a lot more “previousMillis” variables - one per sensor. Again, another use for an array.

Yeah I probably could have named them better and they do pretty much work in pairs. I did toy with the idea of just using one output to trigger both relays. But, I figured at least for the first few attempts/ prototypes I'd try and stick to the drawings I'd made. I'll probably neaten things like that up once I actually get the code working as it should... so would you recommend a 'previousMillis' value for each sensor or a new value between each relay switchover?

Wouldn't it be nice to be able to simply name a new track and all the code created for track 1, migrates to track 2? Although it may be ambitious for you, class objects might be ideal for what you want to do.

  if(currentMillis - previousMillis <= interval01){
    digitalWrite(relayPin03, !digitalRead(relayPin03));
    digitalWrite(relayPin04, !digitalRead(relayPin04));
    previousMillis = currentMillis;

    if(currentMillis - previousMillis <= interval02){

It’s difficult to say, but normally, I’d expect to see a ‘}’ before that second “if”.

And pictures. By official ruling of ME, no one is permitted to discuss their model train layout without also posting pictures of said layout.

Haha I did not even notice I’d missed half the brackets of the code off… like I said still a rookie and still making rookie errors. I’ll chuck a load of brackets in and see of I get anything any different.

And I’m not sure you’d be at all impressed yet…I wanted to get the electronics set up before I started painting or making it look anything like a proper model

No, the process of getting there is more important. We have guys in our club who completed their layout, and then tore it down to start the next one.

Pictures. Any mechanical working should be well detailed. :-D

JoeEason: so would you recommend a 'previousMillis' value for each sensor or a new value between each relay switchover?

Maybe a 'previousMillis' for each interval. Even with curly braces added the current order of if()s looks like a recipe for buzzing relays.

I just checked and i dont think I missed the brackets I just layered them at the end so I had

If(x){
Y;
If(x){
Y;
If(x){
Y;
}
}
}
I thought it would work better that way because I was ‘stacking’ the time delays. My original code which worked for a single line worked something like this:

Relay01, HIGH
Delay(2000)
Relay02, HIGH
delay(2000)
Relay03, HIGH
Delay(2000)

So the first relays stopped the train. It waited. The second relays switched the direction. It waited. The third relay restarted the train and then it waited just so it would clear the LDR

The previous millis might be part of the issue but it's almost like none of the sensors are even working with this code... I know it's all correctly wired, I can upload my code with delay()s and it works fine just with the previously mentioned issues... does anybody know of any similar builds or a project which may use a similar programme?

I am building an automated model railway using LDRs to detect the train positions.

I think you will find it easier to follow the logic if you use the State Machine approach. State Machine is just a fancy name for a system in which one or more variables is used to keep track of the state of the system.

For one of your trains the states might be waitingLeft, movingRight, stoppingRight, waitingRight, movingLeft an stoppingLeft

Suppose a train is movingRight. Eventually it will trigger the LDR and then the state will change to stoppingRight. In the stoppingRight state the train will be halted and the timer will be reset and the state will change to waitingRight. In the waitingRight state the timer will be checked regularly and when the time has expired the state will change to movingLeft. In the movongLeft state the train will be set moving towards the left. etc etc etc

The nice thing about this style of program is that it is easy to introduce other states - for example if you want one train to wait at an intermediate station until the other train arrives in the other platform.

...R

“previousMillis” is a horrible confusing name for what this variable does.
I think you might better understand what’s causing some of your problems if we just give it a better name.

if (ldrStatus03 >=400){
    startTime = currentTime; 
 
  if(currentTime - startTime <= interval01){
    digitalWrite(relayPin07, !digitalRead(relayPin07));
    digitalWrite(relayPin08, !digitalRead(relayPin08));
    startTime = currentTime;

    if(currentTime - startTime <= interval02){
      digitalWrite(relayPin05, !digitalRead(relayPin05));
      digitalWrite(relayPin06, !digitalRead(relayPin06));
      startTime = currentTime;

      if(currentTime - startTime <= interval03){
        digitalWrite(relayPin07, !digitalRead(relayPin07));
        digitalWrite(relayPin08, !digitalRead(relayPin08));
        startTime = currentTime;
      }
    }
  }
}

Thank you for that, I think that does help :) I've done my best to rewrite it in a way i hope will work :). But I'm working away from home for a couple of days and I can't test it properly... I know its a stupid thing to ask seeing as I don't even know if it works or not but could anyone skim through the new code I've written and just see if you think it has a better chance of working. I've changed the intervals, so rather than trying to reset the interval after every relay change they can just count through. I've rectified a couple of other issues I found but without being able to plug into my layout I can't tell if I've done enough.

const int relayPin01 = 13;
const int relayPin02 = 12;
const int relayPin03 = 11;
const int relayPin04 = 10;
const int relayPin05 = 9;
const int relayPin06 = 8;
const int relayPin07 = 7;
const int relayPin08 = 6;
const int ldr_A_Pin = A0;
const int ldr_B_Pin = A1;
const int ldr_C_Pin = A2;
const int ldr_D_Pin = A3;

unsigned long previousMillis01 = 0;
unsigned long previousMillis02 = 0;

const long interval01 = 2000;
const long interval02 = 4000;
const long interval03 = 6000;

void setup() {

    Serial.begin(9600);
  
  pinMode (relayPin01, OUTPUT);
  pinMode (relayPin02, OUTPUT);
  pinMode (relayPin03, OUTPUT);
  pinMode (relayPin04, OUTPUT);
  pinMode (ldr_A_Pin, INPUT);
  pinMode (ldr_B_Pin, INPUT);
  pinMode (ldr_C_Pin, INPUT);
  pinMode (ldr_D_Pin, INPUT);
  digitalWrite(relayPin01, HIGH);
  digitalWrite(relayPin02, HIGH);
  digitalWrite(relayPin03, HIGH);
  digitalWrite(relayPin04, HIGH);
  digitalWrite(relayPin05, HIGH);
  digitalWrite(relayPin06, HIGH);
  digitalWrite(relayPin07, HIGH);
  digitalWrite(relayPin08, HIGH);
  
}

void loop(){

unsigned long currentMillis01 = millis();
unsigned long currentMillis02 = millis();

int ldrStatus01 = analogRead(ldr_A_Pin);
int ldrStatus02 = analogRead(ldr_B_Pin);
int ldrStatus03 = analogRead(ldr_C_Pin);
int ldrStatus04 = analogRead(ldr_D_Pin);

if (ldrStatus01 >=400){
  previousMillis01 = currentMillis01;
  
  if(currentMillis01 - previousMillis01 >= interval01){
    digitalWrite(relayPin03, !digitalRead(relayPin03));
    digitalWrite(relayPin04, !digitalRead(relayPin04));

    if(currentMillis01 - previousMillis01 >= interval02){
      digitalWrite(relayPin01, !digitalRead(relayPin01));
      digitalWrite(relayPin02, !digitalRead(relayPin02));

      if(currentMillis01 - previousMillis01 >= interval03){
        digitalWrite(relayPin03, !digitalRead(relayPin03));
        digitalWrite(relayPin04, !digitalRead(relayPin04));
      }
    }
  }
}

if (ldrStatus02 >=400){
    previousMillis01 = currentMillis01;
  
  if(currentMillis01 - previousMillis01 >= interval01){
    digitalWrite(relayPin03, !digitalRead(relayPin03));
    digitalWrite(relayPin04, !digitalRead(relayPin04));

    if(currentMillis01 - previousMillis01 >= interval02){
      digitalWrite(relayPin01, !digitalRead(relayPin01));
      digitalWrite(relayPin02, !digitalRead(relayPin02));

      if(currentMillis01 - previousMillis01 >= interval03){
        digitalWrite(relayPin03, !digitalRead(relayPin03));
        digitalWrite(relayPin04, !digitalRead(relayPin04));
      }
    }
  }
}

if (ldrStatus03 >=400){
    previousMillis02 = currentMillis02;
  
  if(currentMillis02 - previousMillis02 >= interval01){
    digitalWrite(relayPin05, !digitalRead(relayPin05));
    digitalWrite(relayPin06, !digitalRead(relayPin06));

    if(currentMillis02 - previousMillis02 >= interval02){
      digitalWrite(relayPin07, !digitalRead(relayPin07));
      digitalWrite(relayPin08, !digitalRead(relayPin08));

      if(currentMillis02 - previousMillis02 >= interval03){
        digitalWrite(relayPin05, !digitalRead(relayPin05));
        digitalWrite(relayPin06, !digitalRead(relayPin06));
      }
    }
  }
}

if (ldrStatus04 >=400){
    previousMillis02 = currentMillis02;
  
  if(currentMillis02 - previousMillis02 >= interval01){
    digitalWrite(relayPin05, !digitalRead(relayPin05));
    digitalWrite(relayPin06, !digitalRead(relayPin06));

    if(currentMillis02 - previousMillis02 >= interval02){
      digitalWrite(relayPin07, !digitalRead(relayPin07));
      digitalWrite(relayPin08, !digitalRead(relayPin08));

      if(currentMillis02 - previousMillis02 >= interval03){
        digitalWrite(relayPin05, !digitalRead(relayPin05));
        digitalWrite(relayPin06, !digitalRead(relayPin06));
      }
    }
  }
}
}

So, each line has two direction relays and two power relays, each with it's own pin (why? cant you run both pwer relays offthe same pin? You aren't hooking relay coils directly to an arduino, are you?)

When the ldr senses the train, we want the thing to go through a sequence - engine off, wait two seconds, change direction, wait two seconds, engine on, wiat two seconds. I suppose that final wait is to give the train enough time to clear the LDR, but that's really not necessary. You always know which way the train is travelling, so there's no need to check the end stop that the train is moving away from.

I'm kinda not getting the direction thing. To change direction, you flip both relays. But in the setup() in your previous posts, both direction relays are initialized to HIGH - wouldn't you have one high and one low? I'd be far more comfortable with "set the relays to X" raher than "flip the relays away from what they currently are". I mean - what happens when a moth lands on your LDR?

Say, if I did up a web page on how to code this, would you read it? It's just going to take more space to explain things that I'd be comfortable typing into a web forum.

I know I would!

I suppose I could trigger both relays from 1 output and it would make the code slightly more compact but it wouldn’t really change the effectiveness of anything.
And yes I had a simpler code for a single line which simply used the ‘delay()’ function but that obviously can’t be used when you’re still trying to detect moving trains on the other line.
And the first relays simply cut the power. The second two switch the polarity so that the trains will reverse. The delays were primarily to clear the LDR sensor and also to make sure the direction only switches when the power is cut. If a train is running and the direction is immediately changed it can cause the internal short circuit protection on the controller to trip.
I may be trying to link it a bit too heavily with my original code by not adding any directional coordination but I thought it was that much of a simple idea that it shouldn’t need too much changing.
As for flipping the outputs, that seemed like a good idea as I can leave one line running while I put another train on the opposite line. Leaving both directions high doesn’t matter too much because i can just connect the outputs to the tracks in opposite polarity so that they run in opposite directions

And yes if you've got an idea for it that would be amazing :)

Your getting closer but lets take an imaginary step thru of your code for one ldr (you should always get one thing working before creating more).

We will start 1 sec. before ldrStatus goes above 400

First loop: ldrStatus is below 400 so the first if statement is false so no other code is executed.

next one hundred(or more) loops: properly written code should execute the loop() function very quickly, so again nothing happens lots of times. this is a very important thing to remember.

loop 101: ldrStatus is now above 400 the first if statement is true so previousMillis(ugh!) is set equal to currentMillis. currentMillis - previousMillis is not >= interval so the if statement is false. no futher code is executed and loop() ends.

loop 102: ldrStatus is now above 400 the first if statement is true so previousMillis(ugh!) is set equal to currentMillis AGAIN! currentMillis - previousMillis is not >= interval so the if statement is false. no futher code is executed and loop() ends.

loop 103: ldrStatus is now above 400 the first if statement is true so previousMillis(ugh!) is set equal to currentMillis AGAIN! currentMillis - previousMillis is not >= interval so the if statement is false. no futher code is executed and loop() ends.

Basicly your timers will not work yet because the timer keeps getting restarted before it has a chance to do anything.

Unless you want the timing sequence to stop immediately when ldrStatus goes below 400 you should also separate the trigger code from the timing code(action code) and add a variable that tracks whether the action code should be acted on or not.

This way the sequence will run to completion once triggered

bool ldr01Trigger = false; // global variable



if(ldrStatus01>=400)
{
  previousMillis = currentMillis;
  ldr01Trigger = true;  
}


if (ldr01Trigger = true)
{
  // timing code here
  ldr01Trigger = false;// Put this in the last stage of the timing sequence
}

These both will always be the same. why do you think you need more than one.
previousMillis is the only one that must to be unique for each timing sequence.

unsigned long currentMillis01 = millis();
unsigned long currentMillis02 = millis();

The primary purpose of this post was to show how you can think through how the code works.