2

In my project, I am trying to create a timer display with LEDs base on how long a button was pressed and hold.

There are 8 LED lights in total, when the toggle button was pressed, 1st LED lights up; keep holding/ pressing the button, the 2nd LED lights up after 1sec, the 3rd LED lights up after 2sec and so on. So 7s after the button was pressed and hold, all 8 LEDs shall light up. When the button was released at anytime, all LEDs would be off.

Something is wrong with my code which the timer seems not working (saving the time when the button was pressed) and calculate the time difference ("current time" - "time when button pressed") in order to lights up the LED.

My codes:

 int current;
 long millis_held;      
 long prev_secs_held;
 unsigned long firstTime;     
 byte previous = LOW;    

 const int buttonPin = 13;

 void setup() {
 pinMode(buttonPin, INPUT);
 pinMode(2,OUTPUT) ;
 pinMode(3,OUTPUT) ;
 pinMode(4,OUTPUT) ;
 }

 void loop() {
 current = digitalRead(buttonPin);

 if (current == HIGH && previous == LOW) { 
 firstTime = millis();
 digitalWrite(2, HIGH);

 millis_held = (millis() - firstTime);

 if (millis_held >= 1000 && firstTime <2000){
 digitalWrite(3, HIGH);
 }

  if (millis_held >= 2000){
  digitalWrite(4, HIGH);
 }
 }

 else {
 digitalWrite(2, LOW);
 digitalWrite(3, LOW);
 digitalWrite(4, LOW);
 }
 }

Hope you guys can help, thanks a lot.

  • 1
    We can’t help you without seeing your code. – StarCat Sep 9 at 5:25
  • if I waited too long before initiating the 1st button press, same thing happens with all LEDs light up together ... that indicates that you are not waiting for the button to be pressed before starting the timer – jsotola Sep 9 at 6:13
  • Thanks for your reply, I have added the codes I am currently working on for your reference. See id you can help. Thanks! – Kelvin Ng Sep 9 at 7:02
  • 3
    your code is improperly indented ... that makes the code difficult to read and leads to errors during code development – jsotola Sep 9 at 9:25
3

There are multiple errors in your code.

In the loop, the first if might not have the use you intended it to have. You are checking for previous == low, but you never set previous to high. Therefore this check can be removed entirely, or you could restructure your code a bit and add a previous = high.

Secondly, this if is also flawed:

if (millis_held >= 1000 && firstTime <2000){
    digitalWrite(3, HIGH);
}

You are checking for firstTime < 2000 which means you need to press the button within 2 seconds of the power-up of the Arduino or else this code will not run.

Lastly, for this to run you probably only want to set firstTime = millis() once when you press the button, and not every time the loop() runs thereafter while you keep the button pressed. You would need to move the bracket on line 32 to line 22, that is from just above the else to the line above millis_held = (millis() - firstTime); and then add previous = HIGH; in the first if.

I am leaving it to you to fix your code, since that would be good practice to get some feeling for it. Happy coding!

Lastly you still need to add the last 5 leds, but once the first few work you should be fine.

| improve this answer | |
  • Thanks a lot for your advise, I think I kinda get it, allow me some time to digest and work on it. I will get back to you soon. Appreciate your help!!! – Kelvin Ng Sep 9 at 7:46
3

Great thanks to Ananas_hoi, I was finally able to get my code work accordingly!! The following code only show the 4 LED to be lights up one by one every second when the button was pressed and hold:

int current;
long millis_held;      
unsigned long firstTime; 
byte previous = LOW;         

const int buttonPin = 13;

void setup() {
  pinMode(buttonPin, INPUT);
  pinMode(2,OUTPUT) ;  
  pinMode(3,OUTPUT) ;
  pinMode(4,OUTPUT) ;
  pinMode(5,OUTPUT) ; 
}

void loop() {
  current = digitalRead(buttonPin); 

  if (current == HIGH && previous == LOW) { 
     firstTime = millis();
     digitalWrite(2, HIGH);
     previous = HIGH;
  }
 
  millis_held = (millis() - firstTime);

  if (millis_held >= 1000 && previous == HIGH) {
      digitalWrite(3, HIGH);
     }
 
  if (millis_held >= 2000 && previous == HIGH) {
      digitalWrite(4, HIGH);
     }
 
  if (millis_held >= 3000 && previous == HIGH) {
      digitalWrite(5, HIGH);
     }


  if (current == LOW) {
  digitalWrite(2, LOW);
  digitalWrite(3, LOW);
  digitalWrite(4, LOW);
  digitalWrite(5, LOW);
  previous = LOW;
 }
}
| improve this answer | |
  • 1
    Thanks for posting the solution. (voted.) Lots of new users don't give back like that. This way others can learn from your project. The indentation in your answer is much better than in your original question, but still needs some cleanup. Could you indent the code inside the braces from the if (current == low) statement? And it would be better for new readers if you cleaned up the indentation of your original question. It's pretty hard to read as is. – Duncan C Sep 9 at 21:54

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.