Noob question regarding variable positioning in the sketch

Something very strange happened today. This code:

unsigned long previousfanTime = 0;
unsigned long lightTime = 1000;
unsigned long nolightTime = 500;
unsigned long fanTime1 = 2000;
unsigned long nofanTime1 = 1000;
//unsigned long fanTime2 = 1000;
//unsigned long nofanTime2 = 2000;
const int ledPin = 13;
const int light1 = 2;
const int fan = 3;
const int light2= 4;
const int valve = 5;
long previousTime = 0;
long totalTime = (lightTime + nolightTime);

// the setup routine runs once when you press reset:
void setup() {
Serial.begin(9600);
pinMode(light1, OUTPUT);
pinMode(ledPin, OUTPUT);
pinMode(light2, OUTPUT);
pinMode(valve, OUTPUT);
pinMode(fan, OUTPUT);
digitalWrite(ledPin, HIGH);
}

// the loop routine runs over and over again forever:
void loop() {

unsigned long currentTime = millis();

if (currentTime - previousTime < lightTime)//this is what happens while the light is on
{
digitalWrite(ledPin, HIGH);

if (currentTime - previousfanTime <= fanTime1) //si el tiempo actual es menor que el tiempo de encendido de ventilador
//entonces encendido
{ digitalWrite(fan, HIGH);
}
else {
digitalWrite(fan, LOW); //si no apagado
}
//end
if (currentTime - previousfanTime > (fanTime1 + nofanTime1)) //if time is over the total fan cycle, the fan cycle restarts
{
previousfanTime = currentTime;
}
}
else // now we stablish the loop for the time interval between lightTime and lightTime+nolightTime
//that is to say the complete cycle
{
digitalWrite(ledPin, LOW);
}

if (currentTime - previousTime >= totalTime) {
previousTime = currentTime;
}

Serial.print("previousTime: ");
Serial.println(previousTime);
Serial.print("currentTime: ");
Serial.println(currentTime);
Serial.print("difference: ");
Serial.println(currentTime - previousTime);

}

When I had the long previousTime in the void loop() after one cycle, that is reaching totalTime = 1500ms, the int variable previousTime would be continuously updated to be equal to currentTime = millis().

However after 2 hours trying to find the problem, and comparing with the BlinkWithoutDelay example I realized that long previousMillis = 0 is placed in the header and not inside the loop. I thought that positioning in one place or another only affects the scope of the variable in this fashion: Header = universal scope, void setup() only for the setup, and void loop() only for the loop. But somehow placing the variable inside the loop made it become updated with the currentTime continuously after one cycle yielding the sketch useless.

Can somebody explain me why?

You've perhaps noticed that there is a code icon next to the quote icon. You should have used that icon, instead.

You've perhaps noticed, in the IDE that there are several items in the menu bar. One of them is Tools. That has an item, Auto Format, that would have corralled your code, and stopped it wandering all over the page.

When I had the long previousTime in the void loop() after one cycle, that is reaching totalTime = 1500ms, the int variable previousTime would be continuously updated to be equal to currentTime = millis().

If you had looked at the documentation for the functions you are using, you'd have seen that millis() returns an UNSIGNED LONG, not a long, not an int.

I thought that positioning in one place or another only affects the scope of the variable

That's not all it affects. Global variables are initialized. Local variables are not.

I'm confused here (I know, it's not the first time :) )

@Javi_hache says that when he placed " long previousTime in the void loop()" and I presume he means "long previousTime = millis()" that it updated every pass through the loop. That's exactly what I would expect it to do.

However @PaulS says "Global variables are initialized. Local variables are not." and if this was true I don't see how @Javi_hache had a problem.

...R

long previousTime = 0;
long totalTime = (lightTime + nolightTime);

These should be unsigned. It probably isn’t the problem but it will cause issues in the long run.

Here is your code with the if/else reformatted so they make sense to most people:

unsigned long previousfanTime = 0;
unsigned long lightTime = 1000;
unsigned long nolightTime = 500;
unsigned long fanTime1 = 2000;
unsigned long nofanTime1 = 1000;
//unsigned long fanTime2 = 1000;
//unsigned long nofanTime2 = 2000;
const int ledPin = 13;
const int light1 = 2;
const int fan = 3;
const int light2= 4;
const int valve = 5;
long previousTime = 0;
long totalTime = (lightTime + nolightTime);


// the setup routine runs once when you press reset:
void setup() 
{                
  Serial.begin(9600);
  pinMode(light1, OUTPUT);
  pinMode(ledPin, OUTPUT);
  pinMode(light2, OUTPUT);
  pinMode(valve, OUTPUT); 
  pinMode(fan, OUTPUT); 
  digitalWrite(ledPin, HIGH);
}

// the loop routine runs over and over again forever:
void loop() 
{
  unsigned long currentTime = millis();

  if (currentTime - previousTime < lightTime)//this is what happens while the light is on 
  {
    digitalWrite(ledPin, HIGH);

    if (currentTime - previousfanTime <= fanTime1)                                                  //si el tiempo actual es menor que el tiempo de encendido de ventilador 
      //entonces encendido
    { 
      digitalWrite(fan, HIGH);
    }
    else 
    {
      digitalWrite(fan, LOW);                                                                         //si no apagado
    }
    //end 
    if (currentTime - previousfanTime > (fanTime1 + nofanTime1))                                    //if time is over the total fan cycle, the fan cycle restarts
    {
      previousfanTime = currentTime; 
    }
  }
  else // now we stablish the loop for the time interval between lightTime and lightTime+nolightTime 
  //that is to say the complete cycle
  {
    digitalWrite(ledPin, LOW);
  }

  if (currentTime - previousTime >= totalTime) 
  {
    previousTime = currentTime;
  }

  Serial.print("previousTime: ");
  Serial.println(previousTime);
  Serial.print("currentTime: ");
  Serial.println(currentTime);
  Serial.print("difference: ");
  Serial.println(currentTime - previousTime);
}

Check that the logic you have is correct. Also, is you could explain what this is supposed to do (ie, why you wrote the program) we can look at it with some context for this logic and make comments.

Well I used the option in the IDE "copy for forum"... that's why I didn't use the code button this time (just trying things out :) ).

I used long previousTime analogue to long previousMillis in the BlinkWithoutDelay tutorial. True that previousTime should be "unsigned long" because the timer is gonna run for more than 500hours and will be continuously updated with currentTime=millis().

@Robin2 no, previousTime must be an unsigned long because it gets updated with every cycle by the value from currentTime = millis().

@marco_c Autoformat didn't work for me... it says "too many left parentheses."

The code is for a timer for several devices... in which I can select the ON and OFF times not depending on the time of the day.

The problem was that when placing long previousMillis or unsigned long previousMillis (makes no difference) in the void loop() instead of in the header, after one complete cycle (so when totalTime hit the limit and previousTime became the currentTime, instead of being updated one off... previousMillis kept gettng the value of currentTime continuously therefore rendering the code useless.

However when placing the previousTime = 0 in the header it updates only once after completing one cycle and the code works.

You can try on your arduino... the serial monitor helps see what happens. Put the line unsigned long previousTime = 0 in the void loop() instead of in the header, load the program and check what happens at 1500ms... difference becomes 0 and currentTime and previousTime get the same value. However put that string in the header and see how it perfectly works.

Thank you all for your replies!

However when placing the previousTime = 0 in the header it updates only once after completing one cycle and the code works.

A header is a .h file, not a place in the sketch. When you say "in the header", are you meaning "at global scope"? It's confusing to guess what you mean.

@marco_c Autoformat didn't work for me... it says "too many left parentheses."

Then, you need(ed) to fix that issue, first.

The problem was that when placing long previousMillis or unsigned long previousMillis (makes no difference) in the void loop() instead of in the header, after one complete cycle (so when totalTime hit the limit and previousTime became the currentTime, instead of being updated one off... previousMillis kept gettng the value of currentTime continuously therefore rendering the code useless.

The issue, when the variable is at local scope, is that the variable does not retain it's value when a function, like loop(), ends. (Note, it is not "the void loop()". It is "the loop() function".) It gets a new value the next time the function is called, unless you use the static keyword.

Yes I was meaning at the global scope.

and yes I meant the loop() function.

How should I have written it at the local scope of the loop() function in order to make it work in a similar fashion as when placing it at the global scope?

How should I have written it at the local scope of the loop() function in order to make it work in a similar fashion as when placing it at the global scope?

Use the static keyword.

void loop()
{
   static unsigned long previousMillis = 0;

   // The rest of your code...
}

I don't understand your comment. I was commenting about where the where the variable was defined, not how it is defined.

...R

Javi_Hache: @Robin2 no, previousTime must be an unsigned long because it gets updated with every cycle by the value from currentTime = millis().

@Robin2

@Javi_hache says that when he placed " long previousTime in the void loop()" and I presume he means "long previousTime = millis()" that it updated every pass through the loop. That's exactly what I would expect it to do.

So I just said that long previousTime needs to be equal 0 (previousTime = 0) and after one cycle gets updated with currentTime, that is the value of millis() at that very moment through the function:

 if (currentTime - previousTime > totalTime) 
previousTime = currentTime;

I don't know if it's clear now :cold_sweat:

PaulS's reply shows how to do that.

void loop()
{
    static unsigned long previousMillis = 0; // this is a static variable - it is initialised once at startup, and retains its value after that;
                                                        // it behaves like a global variable except that it is only visible within this function

    unsigned long currentMillis = millis(); // this is an automatic variable - it is declared and reinitialised each time the function is called
}

Now it all makes sense. Thank you Paul, Peter and Robin for your help. Now it's clear why the variable kept changing after the first cycle.

It's clear that this forum is the place to learn thanks to the uninterested help of all you! Hopefully soon I will be helping others...