Problem with multiple, variable timed blinks for single LED using milis()

Hello good people of Arduino land.

A brief bit of background:

I'm building a multi-valve, multi-drop, water drop controller for high speed water drop photography projects.
(Or more specifically a controller that will operate three independent valves, which will each dispense three independently timed drops.)
I have taken the (brave?) step of moving away from the delay() function, since this is the only solution to running different things at (virtually) the same time - in a non-blocking way.
I'm very new to Arduino, so please accept my apologies for being thick!

Back when I was working with a single valve, life was easy....delay() did the job nicely.
But now I would like more than one valve....and so it's time for milis().

I have been working with the classic 'BlinkWithoutDelay' sketch, with some success.
I have learned how to vary the on and off times of the LED - which is standing in for the valve.

However, I would like to blink the LED three times only, via a switch press, and vary the on and off times for each blink.

This is what I'm having a great deal of trouble with!

Here is the order of events I would like to see:

  • turn on LED
  • wait ¾ second
  • turn off LED
  • wait 2 seconds
  • turn on LED
  • wait ½ second
  • turn off LED
  • wait ¼ second
  • turn on LED
  • wait 3 seconds
  • turn off LED

The timings are arbitrary...suffice to say that I need them all to be different.
These values will eventually be entered via potentiometers....or perhaps rotary encoders if I'm feeling very brave!
(And naturally I realise that I will be working with milliseconds...I'm just illustrating my basic requirements)

Here is the sketch I'm starting with...a variation of the aforementioned 'BlinkWithoutDelay' sketch:

//define pins
const byte startButton = 6;
const byte led = 7;

//variable to keep track of the LED
bool ledState = LOW;

//standard millis timing variables
unsigned long currentMillis;
unsigned long previousMillis = 0;

//my timing intervals
const unsigned long onTime1 = 750;
const unsigned long offTime1 = 2000;

void setup() {

  Serial.begin(9600);
  pinMode(startButton, INPUT_PULLUP);
  pinMode(led, OUTPUT);
  digitalWrite(led, LOW);

}

void loop() {

  currentMillis = millis(); //grab the millis() value and set it to currentMillis

  if ((ledState == HIGH) && (currentMillis - previousMillis >= onTime1)) { //if LED is on (ledState = HIGH) and onTime has passed...
    ledState = LOW; //...set ledState LOW
    previousMillis = currentMillis; //remember previousMillis for next iteration
    digitalWrite(led, ledState); //change actual LED according to ledState - LOW in this case
  }
  
  else if ((ledState == LOW) && (currentMillis - previousMillis >= offTime1)) { //if LED is off (ledState = LOW) and offTime has passed...
    ledState = HIGH; //...set ledState to HIGH
    previousMillis = currentMillis; //remember previousMillis for next iteration
    digitalWrite(led, ledState); //change actual LED according to ledState - HIGH in this case
  }
 
}

Obviously this will just cycle the same blink over and over.
(I'll cross the whole 'startButton' thing when I get this nailed down)

I've tried adding more timing variables and more if else() statements to accomodate the other 'blinks', but it doesn't seem to work.

I tried adding this in my variable declarations:

const unsigned long onTime2 = 500;
const unsigned long offTime2 = 250;

...and this in the main loop after the last if else() statement to add another timed 'blink':

  else if ((ledState == HIGH) && (currentMillis - previousMillis >= onTime2)) {
    ledState = LOW; 
    previousMillis = currentMillis;
    digitalWrite(led, ledState);
  }
  
  else if ((ledState == LOW) && (currentMillis - previousMillis >= offTime2)) {
    ledState = HIGH;
    previousMillis = currentMillis;
    digitalWrite(led, ledState);
  }

What is happening when I added these extras is that I only seem to be getting a steady blink with no variation.
I've tried a variety of 'nesting' the if else() statements, but again no success.

It seems that simply adding more if else() statements is not the same as adding more delay() functions, and I've got myself in all sorts of knots....can't seem to understand why!

I've googled till I'm blue in the face and still have got nowhere...all I seem to get is solutions to problems I don't have - eg. how to blink several LEDs at different rates.

I hope some kind soul would be kind enough to nudge this old thicko in the right direction...I think I'm going blinking mad!! :sob:

One option would be to put the intervals in an array

unsigned long intervals[6] = {0, 750, 2000, 500, 250, 3000};

and then work through the values with code like this

void loop() {
   static byte ndx = 0;
   if (ndx < 6) {
      if (millis() - previousMillis >= intervals[ndx] ) {
          previousMillis = millis();
          digitalWrite(ledPin, ! digitalRead(ledPin)); // toggle the led
          ndx ++;
      }
   }
}

...R

EDIT ... to remove the erroneous ) in digitalWrite(ledPin), Apologies for the confusion

There's an extra ) here

digitalWrite(ledPin[b][color=red])[/color][/b], ! digitalRead(ledPin)); // toggle the led

@Robin2

Thank you so much Robin....that indeed works!
Well, I am now seeing the correct sequence of timed blinks.

(And by the way, I did read your excellent post titled: Demonstration code for several things at the same time)

It only runs once and then will not cycle again however.

I have added a switch condition, so I can cycle it when I want...like so:

void loop() {


  if (digitalRead(startButton) == LOW) {
    ledReady = true;
  }
  if (ledReady) {

    static byte ndx = 0;
    if (ndx < 6) {
      if (millis() - previousMillis >= intervals[ndx] ) {
        previousMillis = millis();
        digitalWrite((led), ! digitalRead(led)); // toggle the led
        ndx ++;
      }
    }
  }
}

But again, it seems to only run through once, and will not repeat, no matter how many times I press the button.

I know there should be a....

ledReady = false;

....in there somewhere, but I've moved it around, and cannot find where it should go in order that I can get it to cycle every time I press the button.

I think I need another nudge!

@hamish_mackinlay

I spotted that too...I think it should be this:

digitalWrite((led), ! digitalRead(led));

Edit

Oops...sorry @hamish_mackinlay, you were right, your way makes sense!

I spotted that too...I think it should be this:

digitalWrite((led), ! digitalRead(led));

Without the () also works

digitalWrite(led, ! digitalRead(led));

I would have lost the extra one rather than having mated it with another extra one :wink:

digitalWrite(led, ! digitalRead(led));

But as long as they balance, that's the main thing I suppose.

@sterretje & @hamish_mackinlay

Thanks chaps, either way works the same.

I still can't figure out why it's only cycling once though...then requires a reset to run again.

void loop() {
   static byte ndx = 0;
   if (ndx < 6) {
      if (millis() - previousMillis >= intervals[ndx] ) {
          previousMillis = millis();
          digitalWrite(ledPin), ! digitalRead(ledPin)); // toggle the led
         }
      ndx ++;
      }
      else
      {
      ndx = 0;
      ledReady = false;
      }
   }
}

The problem is ndx. It's static, and once it gets to six, your digitalWrite code never runs again. After you increment it, check to see if it is six. If so, zero it out and set ledready false.

And of course your next challenge bongobreak, is to make the whole thing 2-dimensional so it operates over your many valves. You will want it to not only step through the intervals for a valve, but also step through lines of intervals where each line will be a valve.

@aarg & @wildbill

Thanks, that's just the hint I was looking for...however, I nested the 'else' statement one layer further down....if that makes any sense!

Like this:

void loop() {

  static byte ndx = 0;

  if (ndx < 6) {
    if (millis() - previousMillis >= intervals[ndx] ) {
      previousMillis = millis();
      digitalWrite(led, ! digitalRead(led)); // toggle the led
      ndx ++;
    }
  }
  else {
    ndx = 0;
  }
}

Now it cycles perfectly, with the correct timings....just have to put my startButton back in again!

@hamish_mackinlay

Indeed!
I realise that I have a monumental challenge ahead of me...I have a feeling I will become quite the nuisance over the next weeks/months!! :smiley:

bongobreak:
I realise that I have a monumental challenge ahead of me...I have a feeling I will become quite the nuisance over the next weeks/months!! :smiley:

People who follow the forum guidelines as you did, are (almost) never a nuisance...

aarg:
People who follow the forum guidelines are (almost) never a nuisance...

Thanks @aarg, I appreciate that...however....
...you say that now, but just wait! :smiley:

Anyway, I've had a very successful outcome!

This now works exactly how I envisioned...

//Thanks to Robin2, hamish_mackinlay, sterretje, wildbill and aarg
//on the Arduino forums

//define pins
const byte startButton = 6;
const byte led = 7;

//variable to keep track of the LED
bool ledReady = false;

unsigned long previousMillis = 0;
unsigned long intervals[6] = {0, 750, 2000, 500, 250, 3000};

void setup() {

  Serial.begin(9600);
  pinMode(startButton, INPUT_PULLUP);
  pinMode(led, OUTPUT);
  digitalWrite(led, LOW);

}

void loop() {

  if (digitalRead(startButton) == LOW) {
    ledReady = true;
  }
  if (ledReady) {

    static byte ndx = 0;

    if (ndx < 6) {
      if (millis() - previousMillis >= intervals[ndx] ) {
        previousMillis = millis();
        digitalWrite(led, ! digitalRead(led)); // toggle the led
        ndx ++;
      }
    }
    else {
      ndx = 0;
      ledReady = false;
    }
  }
}

I believe I have earned myself a sit-down and a chocolate biscuit!! :smiley:

A massive thank you to everyone who has contributed to this thread, it's been immensley helpful!

Hope you're all staying safe in these difficult, lock-down times, warmest regards to each and every one of you here on the boards!

:slight_smile:

Edit

Added the complete sketch, and not just the loop()

hamish_mackinlay:
There's an extra ) here

digitalWrite(ledPin[b][color=red])[/color][/b], ! digitalRead(ledPin)); // toggle the led

Thanks for spotting that. I have corrected it

...R

Hello again folks.

Well unfortunately I'm still having issues with this code.

I've added six potentiometers to vary the timing values, and also a 1.8 inch 128x160 TFT screen to display the data.

I have two problems though:

1 - The first entered value - into the array field intervals[0] is supposed to be an initial delay period.

2 - When there is a zero entered into any (or every) field, it still yields a blink.

1 - detail

This one is on me, I should have been more specific in my first post, sorry!
However, I'm having a lot of trouble trying to figure out how to implement it.

I think the problem lies here:

digitalWrite(led, ! digitalRead(led)); // toggle the led

If my understanding is correct, due to the led being set to LOW in the void setup(), naturally the first pass through will toggle this to HIGH, whereas I would like it to remain LOW for the entered value, until the next pass.
Of course I cannot change the initial state to HIGH, because the valve has to remain closed.
I'm sorry to say that I don't have the first clue as to where to begin to correct this!

2 - detail

This will definitely be a problem if I'm getting led blinking (valve openings) where there shouldn't be any.
It registers approximately a ~70ms pulse when there should be none.
I have no idea why this is happening!

If anyone can shed any light on these issues, I would be eternally grateful!

Many thanks.

(Full sketch in next post)

Full sketch:

#include <digitalIOPerformance.h>
#include <Wire.h>
#include <TFT.h>
#include <SPI.h>

#define cs   10
#define dc   9
#define rst  8

//define pins
const byte startButton = 2;
const byte led = 5;
const byte pot1 = A1;
const byte pot2 = A2;
const byte pot3 = A3;
const byte pot4 = A4;
const byte pot5 = A5;
const byte pot6 = A6;

//variable to keep track of the LED
bool ledReady = false;

unsigned long previousMillis = 0;
unsigned long intervals[6];

TFT TFTscreen = TFT(cs, dc, rst);

void setup() {

  Serial.begin(9600);
  pinMode(startButton, INPUT_PULLUP);
  pinMode(led, OUTPUT);
  digitalWrite(led, LOW);

  TFTscreen.begin();
  TFTscreen.background(0, 0, 0);
  TFTscreen.setTextSize(1);
  TFTscreen.stroke(255, 255, 255);
  TFTscreen.text("Del Dr1 Del Dr2 Del Dr3", 6, 6);

}

void loop() {

  intervals[0] = map(analogRead(pot1), 4, 1019, 0, 200);
  intervals[1] = map(analogRead(pot2), 4, 1019, 0, 200);
  intervals[2] = map(analogRead(pot3), 4, 1019, 0, 200);
  intervals[3] = map(analogRead(pot4), 4, 1019, 0, 200);
  intervals[4] = map(analogRead(pot5), 4, 1019, 0, 200);
  intervals[5] = map(analogRead(pot6), 4, 1019, 0, 200);

  if (digitalRead(startButton) == LOW) {
    ledReady = true;
  }
  if (ledReady) {

    static byte ndx = 0;

    if (ndx < 6) {
      if (millis() - previousMillis >= intervals[ndx] ) {
        previousMillis = millis();
        digitalWrite(led, ! digitalRead(led)); // toggle the led
        ndx ++;
      }
    }
    else {
      ndx = 0;
      ledReady = false;
    }
  }
  updateTFT();
}

  void updateTFT() {

  TFTscreen.setTextSize(1);
  TFTscreen.stroke(255, 0, 0);
  TFTscreen.setCursor(5, 40);
  TFTscreen.print(intervals[0]);
  TFTscreen.setCursor(30, 40);
  TFTscreen.print(intervals[1]);
  TFTscreen.setCursor(55, 40);
  TFTscreen.print(intervals[2]);
  TFTscreen.setCursor(80, 40);
  TFTscreen.print(intervals[3]);
  TFTscreen.setCursor(105, 40);
  TFTscreen.print(intervals[4]);
  TFTscreen.setCursor(130, 40);
  TFTscreen.print(intervals[5]);

  delay(50);

  TFTscreen.setTextSize(1);
  TFTscreen.stroke(0, 0, 0);
  TFTscreen.setCursor(5, 40);
  TFTscreen.print(intervals[0]);
  TFTscreen.setCursor(30, 40);
  TFTscreen.print(intervals[1]);
  TFTscreen.setCursor(55, 40);
  TFTscreen.print(intervals[2]);
  TFTscreen.setCursor(80, 40);
  TFTscreen.print(intervals[3]);
  TFTscreen.setCursor(105, 40);
  TFTscreen.print(intervals[4]);
  TFTscreen.setCursor(130, 40);
  TFTscreen.print(intervals[5]);

  }

Are you saying that you observe the blink without pushing the startButton?

Is your led wired between Vcc and pin or between pin and GND? Not sure if it was mentioned in the posts. If the former, driving the led pin low will switch it on.

Thanks for responding @sterretje.

I apologise for not replying til now, and for not being clearer with my question.

sterretje:
Are you saying that you observe the blink without pushing the startButton?

No, the startButton works well - I press it and the sequence runs as expected (in a manner of speaking!) and then stops til the startButton is pressed again.

The problem is that I would like the first value of the array (unsigned long intervals[6] - in the position intervals[0]) to be a delay.

At the moment, I am seeing no effect of this value.

I could enter a zero, or the max allowed (200 in this case) and it always seems to ignore it.
It appears to skip it completely, continue with the next value, which corresponds to the led going HIGH, and therefore a blink.

What I would like to see happen is this:

  • initial led state is LOW (off)

  • press startButton

  • pause for value intervals[0] - initial delay

  • led goes HIGH

  • pause for value intervals[1] - 1st drop size

  • led goes LOW

  • pause for value intervals[2] - delay between 1st and 2nd drops

  • led goes HIGH

  • pause for value intervals[3] - 2nd drop size

  • led goes LOW

  • pause for value intervals[4] - delay between 2nd and 3rd drops

  • led goes HIGH

  • pause for value intervals[5] - 3rd drop size

  • led goes LOW

  • end of sequence

Is your led wired between Vcc and pin or between pin and GND? Not sure if it was mentioned in the posts. If the former, driving the led pin low will switch it on.

Again, sorry for not being clearer - my led is wired from digital pin 5 through a 330Ω resistor to GND, so a LOW would be off.

The second part of my latest questions is frightfully vexing too!
I can't seem to figure out why there are still led blinks when all of the values in the array are set to zero.

I hope you can point me in the right direction, I'm really trying to learn, but it's tough going....I'm worried that the old adage 'You can't teach an old dog new tricks.' may sadly be true in my case! :smiley:

The problem is that previousMillis is zero, so by the time you get around to pressing the start button, the initial interval period has already passed. When you set ledReady to true, you should also set previousMillis to millis().

wildbill:
The problem is that previousMillis is zero, so by the time you get around to pressing the start button, the initial interval period has already passed. When you set ledReady to true, you should also set previousMillis to millis().

Thank you so much @wildbill...

...I followed your advice, like so...

  if (digitalRead(startButton) == LOW) {
    ledReady = true;
    previousMillis = millis();
  }

...and I am now seeing the initial pause correctly!

Any ideas why the 'zero' value issue still exists?

If I set a 'drop size' to zero the led should stay off, but it doesn't....very puzzling!

If the 'blink' were a few milliseconds, I could live with it since the response times for my valves are ~20 milliseconds, but I've measured the time on my (laughable but only just capable DSO shell) scope and I'm seeing a whopping ~70 milliseconds!