0

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?

  • 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? – Gerben Dec 6 '15 at 15:55
  • edited the question with more informations – linofex Dec 6 '15 at 16:02
  • 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. – Gerben Dec 6 '15 at 16:37
2

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);}
  • 1
    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. – Gerben Dec 6 '15 at 16:35
  • 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. – slash-dev Dec 6 '15 at 16:48
  • 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? – linofex Dec 6 '15 at 16:48
  • 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. – slash-dev Dec 6 '15 at 16:54
  • 1
    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. – Gerben Dec 6 '15 at 19:52
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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.