millis() not working properly

Question

I'm doing a little project that, on a press of a button, will light up three LEDs and then will turn off the LEDs in different times. I'm using the millis() function to control the time that each LED will be on.

On the first time that I press the button, everything works fine. But the problem starts when I press the button for the second time.

If I press the button soon after the loop ends, something goes wrong and the behavior of the LED become a mess (just one or two LEDs light up in weird times). But, if I wait about 2 seconds before pressing the button for the second time, everything goes right.

Here's my code:

const int ledPin1 =  11;
  const int ledPin2 =  12;
  const int ledPin3 =  13;
  const int buttonPin = 2; 

  int buttonState = 0;
  long previousMillis1 = 0;
  long previousMillis2 = 0;
  long previousMillis3 = 0;

  int state = 0;

  void setup() 
  {
    pinMode(ledPin1, OUTPUT);
    pinMode(ledPin2, OUTPUT);
    pinMode(ledPin3, OUTPUT);   
    pinMode(buttonPin, INPUT);

    Serial.begin(9600);
  }

  void loop()
  {

       buttonState = digitalRead(buttonPin);

       if (buttonState == HIGH) {
        state = 1;
       }

       if (state == 1)
       {
        taskExecute(3000, 2000, 4000);
       }
  }

  void taskExecute(int tMotor1, int tMotor2, int tMotor3)
  {

    int ledState1 = HIGH; 
    int ledState2 = HIGH;
    int ledState3 = HIGH; 

    digitalWrite(ledPin1, ledState1);
    digitalWrite(ledPin2, ledState2);
    digitalWrite(ledPin3, ledState3);

    do
    {
      unsigned long currentMillis = millis();

      if(currentMillis - previousMillis1 > tMotor1) 
      {
        previousMillis1 = currentMillis;   

        if (ledState1 == HIGH)
          ledState1 = LOW;

        digitalWrite(ledPin1, ledState1);
      }

      if(currentMillis - previousMillis2 > tMotor2) 
      {
        previousMillis2 = currentMillis;   

        if (ledState2 == HIGH)
          ledState2 = LOW;

        digitalWrite(ledPin2, ledState2);
      }

      if(currentMillis - previousMillis3 > tMotor3) 
      {
        previousMillis3 = currentMillis;   

        if (ledState3 == HIGH)
          ledState3 = LOW;

        digitalWrite(ledPin3, ledState3);
      }
    } while (ledState1 == HIGH || ledState2 == HIGH || ledState3 == HIGH);
    state = 0;
  }

Other strange thing that happens is that sometimes the time that i've been set in millis for the led get on become a little different. Example: when I press the button, the LED 1 needs to light up for 3 seconds, but he lights up just 2.5 seconds.

What i'm doing wrong? Thanks!!

Answer 1

The variables previousMillis (1-3) are not properly initialised. So if you wait a while before pressing the button, that delay time becomes part of the time that the LED is illuminated for.

If your taskExecute() function sets them before the loop, this will fix that:

void taskExecute(int tMotor1, int tMotor2, int tMotor3)
{
    int ledState1 = HIGH;
    int ledState2 = HIGH;
    int ledState3 = HIGH;

    digitalWrite(ledPin1, ledState1);
    digitalWrite(ledPin2, ledState2);
    digitalWrite(ledPin3, ledState3);

    unsigned long time_now = millis();
    previousMillis1 = time_now;
    previousMillis2 = time_now;
    previousMillis3 = time_now;

    do 
    {
        ...
    } while (ledState1 == HIGH || ledState2 == HIGH || ledState3 == HIGH);
    state = 0;
}

On a coding-style note - I did not initially see the resetting of the global variable state=0 at the end of the taskExecute() function. It might be better to move this up to underneath the initial call to taskExecute() so all the state handling is in one place.