Issue with simple project

Question

I'm new to Arduino and today I have a problem with this code:

int led_1 = 10;
int led_2 = 11;
int led_3 = 12;
int button = 3;
int time = 0;
byte val = 0;

void setup() {
  // put your setup code here, to run once:
  pinMode(led_1, OUTPUT);
  pinMode(led_2, OUTPUT);
  pinMode(led_3, OUTPUT);
  pinMode(button, INPUT);
}

void loop() {
  // put your main code here, to run repeatedly:
  val = digitalRead(button);

  if (val == HIGH){
    time = millis();
    while (digitalRead(button) == HIGH) {
      if (millis() - time < 1000) {digitalWrite(led_1, HIGH);}
      else if (millis() -time > 1000 && millis()-time <2000) {digitalWrite(led_2, HIGH);}
      else if (millis()-time >2000) {digitalWrite(led_3, HIGH);}
    }
  } else { 
    time =0;
    digitalWrite(led_1, LOW);
    digitalWrite(led_2, LOW);
    digitalWrite(led_3, LOW);
    }
}

The code should work like this: If I press the button less than one second, led_1 will turn on. If I press it between one and two seconds, led_2 will turn on; and if I press the button more than 2 seconds, led_3 will turn on. If I don't press a button, the LEDs remain off. The time variable is used to take the time when I start to press the button and then the control millis()- time should give how long I press the button.

It works for a while, then after a random time (I think) if I push the button, only the led_3 turns on. Why?

Answer 1

time should be an unsigned int. As soon as millis reaches 32768, it will be stored as -32768 in time, and your comparisons will not work. This happens after 32.768 seconds. Here's a short sketch to illustrate:

int time = 0;

void setup() {
  Serial.begin( 9600 );
}

void loop() {
    time = millis();
    if (time % 1000 == 0) {
      Serial.println( time );
      delay( 2 ); // let millis() increment
    }
}

To see the difference, just change the first line to

unsigned int time = 0;

Note how time rolls over to 0 after 65535 (well, only 65000 is printed).

I would recommend using unsigned constants by adding a "U" after the digits: 1000U. In your if statements, you must cast the millis to the same (smaller) type: (unsigned int)millis(). Also, parentheses make the expression more clear:

      else if (((unsigned int)millis() - time > 1000U) && ((unsigned int)millis() - tempo < 2000U)) {digitalWrite(led_2, HIGH);}

Answer 2

Your code seems inefficient and there is a tempo variable that doesn't appear elsewhere.

  if (val == HIGH) {
    time = millis();
    while (digitalRead(button) == HIGH) {
      int time0=millis();
      if (time0 - time < 1000) {
        digitalWrite(led_1, HIGH);
      } else if (time0 - time < 2000) {
        digitalWrite(led_2, HIGH);
      } else if (time0 - time > 2000) {
        digitalWrite(led_3, HIGH);
      }
    }
  }

Note that this code will turn on all LEDs, one by one. If you want to turn off the other, you'll have to had code for that in the tests.