currentMillis not working

Hello, me again,

Can someone please help. I’m trying to not use delays.
In this sketch, I can’t see what I’m doing wrong. The millis parts do not work.

int ldrPin = 0;
int LED1 = 2;
int LED2 = 3;
//int pirPin = 5;
//int relayPin = 8;
//int pirState = LOW;
//int leds[2] = {2, 3};
unsigned long interval = 2000;
unsigned long previousMillis = 0;
unsigned long currentMillis = 0;

void setup() {


  pinMode(0, INPUT);
  pinMode(2, OUTPUT);
  pinMode(3, OUTPUT);
  pinMode(5, INPUT);
  pinMode(8, OUTPUT);
}

void loop () {

  unsigned long currentMillis = millis();
  
  int ldrState = analogRead(ldrPin);
  if (ldrState <= 500)  {


    digitalWrite(LED1, HIGH);
    digitalWrite(LED2, LOW);
    delay(100);

    digitalWrite(LED1, LOW);
    digitalWrite(LED2, HIGH);
    delay(100);
  }
  if ((unsigned long)(currentMillis - previousMillis) >= interval) {
    
      digitalWrite  (LED1, LOW);
      digitalWrite  (LED2, LOW);
      previousMillis = millis();
    
  }
}

Not having any wiring information is not helpful. Being told that something is wrong is not helpful. Being told that something is not working is not helpful. Using millis() and also using delay(...) is not helpful. Using an unnecessary cast is not helpful.

I checked the wiring. The program works in that when the ldr is covered the leds blink like I expect. However, the never stop. I thought the millis part would stop the blinking after 2 seconds. I replaced only one delay to see if it worked. What is an unnecessary cast???

Polliwog: I thought the millis part would stop the blinking after 2 seconds.

But even when the 2000ms is up after 10 blinks, and the millis()-based if() turns both LED1&2 low, loop() takes you back to the top, and there's nothing to stop the blink happening again.

If you put a Serial.print inside the millis()-based if() you would see that the if() is being actioned.

That is a great help. Thank you.

vaj4088:
Not having any wiring information is not helpful.
Being told that something is wrong is not helpful.
Being told that something is not working is not helpful.
Using millis() and also using delay(…) is not helpful.
Using an unnecessary cast is not helpful.

I’m sorry I upset you. I’m just looking for help. If you don’t wish to help, that’s OK, don’t let it bother you, just don’t answer.

I wonder if what you actually want is 2000ms' worth of blinking from when the ldr goes under 500 from when it wasn't?

In that case you would need to keep comparing ldrState_now to ldrState_old, capture the millis() start time when it goes under 500, and then only blink until millis() goes ahead 2000 from then.

pinMode(0, INPUT);Get rid of that too.

@Polliwog, stay closer to the BlinkWithoutDelay example: https://www.arduino.cc/en/tutorial/BlinkWithoutDelay. Read the reference of the functions. For example analogRead(): https://www.arduino.cc/reference/en/language/functions/analog-io/analogread/.

If something is not working, remove everything that is not needed. Or make a small test-sketch to test a part of the sketch.

Compare both examples from the links above with your sketch.

Ready? Here we go:

int ldrPin = 0;

It has the word 'pin' in its name, I like that. Pin number '0' is confusing. Please use 'A0' for analog pins. You can add 'const' in front of it to tell the compiler that the value is a constant and does not change.

The result would be:

const int ldrPin = A0;
int LED1 = 2;
int LED2 = 3;

Please add the word 'pin' to the names. For example LED1pin and LED2pin. If you have added 'const' in front of 'ldrPin', then you can do that as well with these variables.

unsigned long interval = 2000;
unsigned long previousMillis = 0;
unsigned long currentMillis = 0;

O no, a bug. You declare a global variable 'currentMillis', and in the loop() you declare another variable also called 'currentMillis'. Which one should the compiler use ? Please remove this global variable 'currentMillis'.

pinMode(0, INPUT);
  pinMode(2, OUTPUT);
  pinMode(3, OUTPUT);
  pinMode(5, INPUT);
  pinMode(8, OUTPUT);

Stay away from the digital pin 0 as TheMemberFormerlyKnownAsAWOL wrote. Remove that line. Pin 0 and 1 for an Arduino Uno are used for the serial monitor and to upload a sketch. You don't have to initialize pin 'A0', as you can see in the reference for analogRead(). Please use the names 'LED1pin' and 'LED2pin'. instead of numbers. When not using pin 5 and 8 for now, please add comments '//' in front of them.

You don't have to do this:

if ((unsigned long)(currentMillis - previousMillis) >= interval)

You can do this:

if (currentMillis - previousMillis >= interval)

That is nicer and cleaner. The '(unsigned long)' converts it once more to unsigned long. That is called a 'cast' to unsigned long. It is not needed here, and when you use it, you make it look as if you don't understand that they are already two unsigned long variables. That is what vaj4088 wrote.

You forgot to explain what the sketch should to. Therefor the first answer by vaj4088 is exactly pinpointing the problem. When you say that something is wrong and not working, then we have no clue what the sketch should do. To be honest, I still have no clue.

No worries, it is a beginners mistake on a forum. You are too focussed on this sketch and looking to hard at your screen. The best thing to do is to forget this sketch and lean backwards. Then try to explain to us what you want to do with a LDR and two LEDs.

Do you want the leds to blink continuously or just once ? Do you want to use millis() to continously blink the leds, or do you want to use it as a single shot timer to turn off the leds ? The BlinkWithoutDelay does not show how to use millis() as a single shot timer. A single shot timer uses millis() in a different way.

fishboneDiagram is looking into the future and forseeing that you need an extra variable for the state of the led. I have the same feeling. You probably need an extra variable for the state of the led or a boolean variable to turn on and off the blinking.

It has the word 'pin' 'Pin' in its name, I like that.

Koepel: fishboneDiagram is .... forseeing

No, I was simply wondering... ;)

fishboneDiagram: I wonder forsee ...

... that the //'d out pins for a relay and a pir indicate there's more to the sketch than meets the eye. If so it would behoove you, Polliwog, to say what's required in the long term. It would be especially useful to indicate the interaction (if any) between the ldr and the pir. Maybe they're independent and cause outputs of their own, or perhaps the pir only needs to be read if it's dark, or the ldr only needs reading if there's motion....