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?

• Gerben: Please explain what your code is supposed to do. You can't just dump the code on us, an expecting us to decipher what it does. Also, what is a casual?
• linofex: edited the question with more informations
• Gerben: Thank you. Just use unsigned long time = 0; instead. What I also see, is that led 1 remains on, even if the button is pressed for more that one second. Idem for led 2 after two seconds.

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);}

• Gerben: 32768 will actually become -32768 not -1. I also think this will create a new problem after 65 seconds. Better to just use the same type that millis return. I.e. unsigned long.
• slash-dev: Oopsies, edited -1 above. However, he can use any size of unsigned int. The key is to compare a difference with the desired interval: (unsigned int)millis()-start > interval, all unsigned numbers. If he were watching for intervals < 256, he could have used unsigned char.
• linofex: Thank you for the replies,I have solved the problem, but I have a question: I see that int time reaches 32768 and then becomes -32768, but after that it returns positive again. If I wait for 65 seconds (so time is positive) the LED still doesn't work. Why? What does U mean?
• slash-dev: Yes, I forgot that you need to cast the millis() to the smaller type. My example does an implicit cast by assigning to the smaller unsigned int. Edited answer and comment.
• Gerben: This is getting a bit silly. Casting the millis to an int?! Just use the proper type that is returned by millis, and use unsigned long time = 0;. This makes the code a lot more readable.

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.