Newbie question regarding millis()

I am new to this so please bear with me. I am trying to run several different delays in my code to give time for machinery to move into place etc... I am concerned that my use of multiple delay variables will not work as the code keeps resetting the current time based on example code I found. Thoughts? Sorry about the length - I'm not even sure if it acceptable to post this much code... like I said, new to this.

This is for a machine that de-stacks buckets at one station and then adds water at the next. Homemade in my garage.

// Define pins
const int sensewaterstn = 3;  // Sensor pin (IR sensor, water station)
const int sensedrop = 4;      // Sensor pin (IR sensor, bucket drop)
const int alarm = 6;          // Alarm condition (light stalk) red wire / relay 7
const int drive = 7;          // Motor pin (for conveyor belt stop/start) orange wire / relay 6
const int hold = 8;           // Stop plates for bucket pile / relay 5
const int pawl = 9;           // Air valve pin (bucket stack pawls) / relay 4
const int gate = 10;          // Air valve pin (Bucket exit gate) / relay 3
const int waterfill = 11;     // Water valve pin (water fill station) / relay 1
const int jets = 12;          // Air valve pin (air jets) / relay 2
// delay code
unsigned long DELAY_TIME5 = 5000;    // 5 second delay
unsigned long DELAY_TIME35 = 3500;   // 3.5 second delay
unsigned long DELAY_TIME3 = 3000;    // 3 second delay
unsigned long DELAY_TIME1 = 1000;    // 1 second delay
unsigned long previousWaterstn = 0;  // stores last time bucket present at water station
unsigned long previousDrop = 0;      // stores last time bucket detected at drop location

void setup() {
  // Initialize serial communication
  Serial.begin(9600);

  // Initialize sensor pins as input
  pinMode(sensedrop, INPUT);      // set pins to input
  digitalWrite(sensedrop, HIGH);  // turn on pullup resistors (means grounding pin completes circuit, connect to + side of sensor)
  pinMode(sensewaterstn, INPUT);
  digitalWrite(sensewaterstn, HIGH);

  // Initialize output pins as outputs
  pinMode(drive, OUTPUT);     // set pins to output
  digitalWrite(drive, HIGH);  // set initial voltage to LOW or OFF  NOTE: connect terminals 1 to 4 on Leeson ctrl to START drive - break conncection to STOP
  pinMode(gate, OUTPUT);
  digitalWrite(gate, HIGH);
  pinMode(hold, OUTPUT);
  digitalWrite(hold, HIGH);
  pinMode(pawl, OUTPUT);
  digitalWrite(pawl, LOW);
  pinMode(jets, OUTPUT);
  digitalWrite(jets, LOW);
  pinMode(waterfill, OUTPUT);
  digitalWrite(waterfill, LOW);
}

void loop() {
  unsigned long currentWaterstn = millis();  //save current time for water station
  unsigned long currentDrop = millis();      //save current time for bucket drop
  //Water station
  if (digitalRead(sensewaterstn) == LOW) {  //bucket present at water station and waiting for fill
    digitalWrite(drive, LOW);               //turn off conveyor
    digitalWrite(waterfill, HIGH);          //open fill valve - trigger off-delay relay (analog manual adjust OFF delay simplifies operation)
    // check if delay has timed out for initiating water valve
    if (currentWaterstn - previousWaterstn >= DELAY_TIME1) {  // 1 second delay timer
      previousWaterstn = currentWaterstn;                     // this saves when the water valve last initiated
      digitalWrite(waterfill, LOW);                           // turn OFF water - analog OFF delay relay will continue to dose water till done
    } else {
      digitalWrite(waterfill, HIGH);  //leave water valve ON
    }
    // check if delay has timed out water valve to finish filling
    if (currentWaterstn - previousWaterstn >= DELAY_TIME5) {  // 5 second delay timer (maximum time alotted for analog delay timer to fill bucket)
      previousWaterstn = currentWaterstn;                     //copy current time to memory
      digitalWrite(waterfill, LOW);                           // JUST IN CASE - turn OFF water - 5 second overall alotment of time for fill
      digitalWrite(drive, HIGH);                              //turn conveyor back on
    }
  } else {
    digitalWrite(waterfill, LOW);  //turn off water
    digitalWrite(drive, LOW);      //turn off conveyor
  }

  // Bucket drop detection
  if (digitalRead(sensedrop) == HIGH) {  //no bucket present
    digitalWrite(gate, HIGH);            //close exit gate
    digitalWrite(pawl, HIGH);            //engage pawls to hold stack of buckets
    //delay
    if (currentDrop - previousDrop >= DELAY_TIME1) {  // 1 second delay timer togive time for pawls to engage before opening bottom holding tabs
      previousDrop = currentDrop;                     // this saves when bucket drop last initiated
      digitalWrite(hold, LOW);                        // retract holding tabs - freeing last bucket for drop
    } else {
      digitalWrite(hold, HIGH);  //leave holding tabs engaged - this is to give pawls time to deploy and prevent entire bucket pile from falling
    }
    //delay
    if (currentDrop - previousDrop >= DELAY_TIME3) {  // 3 second delay timer
      previousDrop = currentDrop;                     // this saves when bucket drop last initiated
      digitalWrite(jets, LOW);                        // stop air jets
      digitalWrite(hold, HIGH);                       //re-engage holding tabs under bucket pile
    } else {
      digitalWrite(jets, HIGH);  //leave air jets on to give time for bucket dislodging
      digitalWrite(hold, HIGH);  //re-engage holding tabs - again for insurance
    }
    //delay
    if (currentDrop - previousDrop >= DELAY_TIME35) {  // 3.5 second delay timer
      previousDrop = currentDrop;                      // this saves when bucket drop last initiated
      digitalWrite(jets, LOW);                         // stop air jets
      digitalWrite(pawl, LOW);                         //dis-engage pawls from bucket pile
    } else {
      digitalWrite(pawl, HIGH);  //leave pawls engaged to give time for holding tabs to engage first
    }
  }
}

After only a second's glance, I've got to say that some of the overall logic looks iffy to me. I suspect you need distinct previous... variables for each of your delays at a minimum.

But as far as the delays themselves go, I'd get rid of currentDrop and currentWaterstn and just use millis() in their place.

Also, you'll likely need to either reset the previous... variables to zero after the delay expires and add a test for a non zero value like so if( previous... && millis() - previous... >= DELAY_TIMExxx) or add a flag that's set when the delay is running. Setting the previous... value to zero when the delay expires does it w/o an extra variable.

Thank you. I'll see what I can figure out from what you are saying. Instinctively, I knew there was something wrong with the sequencing of delays.

A process like your code attempts to model is best represented as a finite state machine. You best start with a diagram where each state is identified such as WAITING_FOR_BUCKET or FILLING etc. as are the rules or triggers for the transitions between those states such as "water level detection switch triggered" or "10 secon timeout reached".

1 Like

I suspect you're also missing a state machine to handle sequencing all those delays but it's been a long day here and I likely need to sleep on it.

Edit: ah, I see @6v6gt came to the same conclusion. I imagine they're more awake than I am right now! :slight_smile:

Yes. Or my habit, learned from someone on these fora, is to grab millis() once at the top of your (free running) loop. I call it now, fancier ppl use currentMillis or something official. Use an unsigned long global variable, then all your functions get to use the same moment in time.

This puts everything on the same time, so if your loop takes significant milliseconds, all calculations are done on one clock.

I run loops to go at a frame rate, like every 15 milliseconds and often take a good fraction of that during one loop's processing.

a7

Just a bit of insomnia relief with a tablet PC :slight_smile:

Are you referring to a "Case statement" kind of thing? I had considered that initially.

1 Like

See here for an example of a state diagram which can more less be coded from directly: Make an output pin behave like a monostable multivibrator - #12 by 6v6gt or here: With millis() i have to wait after loop finish - #52 by 6v6gt

I like this approach as there are some sizeable delays which could cause errors (phasing?? - where actions are out of synch). I believe I am grabbing millis() early and storing it in currentWaterstn & currentDrop.

  • Suggest you avoid using HIGH and LOW

Instead use a name that documents what is happening.


#define LEDon       HIGH
#define LEDoff      LOW

#define MOTORon     HIGH
#define MOTORoff    LOW

. . .
//example
digitalWrite(rewindMotor, MOTORoff);
2 Likes

Hello biffidum

I looked at the sketch and I think the project needs a timer() function with the following controls:

  1. init() -> pass the timer variable
  2. start(currentMillis) -> start the timer at the specified time
  3. stop() -> stop the timer
  4. expired(currentMillis) -> check if the timer has expired to start another function.

Have a nice day and enjoy coding in C++.

p.s. enums, arrays and structs are your friends.

Yes, that's exactly.

You don't need two unsigned long integers that will almost always have the same value, which value has more to do with time than anything being timed.

So is where the idea of currentMillis springs from. Named for time, leave previousWaterstn to carry the meaning when you are programming literally. When timing is done with some kind of array, array elements are compared to now. Or currentTime or whatever.

a7

1 Like

after "timing out" what prevents a sensor from immediately turning things back on?

also looks like multiple timers may conflict with one another, don't each of their else conditions override what happens?

how would the DELAY_TIME3 timer ever expire if the DELAY_TIME1 timer resets previousDrop when it expires and similarly for DELAY_TIME35

it seems like the sensors input trigger a sequence of "timed" operations

this might be what you're trying to do
Capture

Sure you have well done to post your code as a code-section in your very first posting.
The code-section limits the height. So no problem to post many lines of code.

Yes exactly

here is a tutorial that explains non-blocking timing based on millis()

And here is a tutorial explaining how state-machines work

New to this - so thanks for the link to Graphviz. It helps to visualize things. My challenge here is that I have two separate loops that are interconnected with one loop reigning supreme (water fill loop). Now that I look at it, it might make more sense to link the timing of bucket drop directly to the water fill station - so they always happen simultaneously. I can use the sensorDrop as alarm exit point instead.

https://is.gd/FVCIPr

my primary point is that i don't believe your timers are working the way you think because of

  • the else cases
  • they are all active at the same time
  • they use the same timestamp variable
1 Like

Agreed. I am going to implement the changes as I understand. Back to my day-job for now :frowning: I'll report back on my progress. Many thanks to all.

@biffidum

you should use self-explaining names for everything.
Your diagram uses names like "time5" "time1" etc.

these names are not self-explaining.

sensedrop what does it mean sensing the dropping = bucket starts to fall down?
or
bucket has finished falling down? So what is the place called where the bucket is when the bucket has finished dropping down?

let's assume after the bucket has fallen down the place is the fillstation

So I would name the sensor "bucketInFillStation"

this name describes what the real situation is.

And as you use INPUT_PULLUP-resistors I wiuld define constants that express the logic state

const byte isPresent = LOW;
const byte notPresent = HIGH;

then your if-condition would be

 if (digitalRead(bucketInFillStation) == notPresent 

which describes it even more precise.

Gotcha. Fill station is further along the conveyor. I'll work on my syntax. Thanks for the feedback.