problem using millis as timer

I my code I use
previousMillis = currentMillis;
in order to reset the timer to 0
for some reason this works only part of the time and when it not work my sw does not do what its has to do even thou that the code seem to be OK.
does anyone know the reason?

here is my full code:

#include <JeeLib.h> // Low power functions library
#include <avr/sleep.h> //Power Management and Sleep Modes

unsigned long t0=1;//8000; // t0 = if the manual valve is closed at this time, program will stop
unsigned long t1=4;//2000; // t1 = additional period of time that the latch valve will be open.
unsigned long t12=9000; // t12 = if the manual valve is closed for up to t12 mSec during t1 than don't do nothing. if over t12 than t2=0
unsigned long t2=5000;//00; // t2 = time that the latch valve is closed.
int OpenValve = 4; // Digital Arduino Pin used to open the latch valve
int CloseValve = 5; // Digital Arduino Pin used to close the latch valve
boolean flow=1; //state of the flow sensor
unsigned long currentMillis;
unsigned long previousMillis;
unsigned long tManualValveOpen; //time that the manual valve has been reopened
unsigned long tManualValveClose; //time that the manual valve has been closed
ISR(WDT_vect) { Sleepy::watchdogEvent(); }

void setup(){
Serial.begin(9600); //start serial communication at 9600 baud
pinMode(13,OUTPUT); // set pin 13 as an output so we can use LED to monitor
digitalWrite(13,LOW); // turn pin 13 LED off
pinMode(OpenValve, OUTPUT); // declare pin 4 to be an output:
pinMode(CloseValve, OUTPUT); // declare pin 5 to be an output:
pinMode(2, INPUT_PULLUP); //declare p1n 2 as input:
Sleepy::loseSomeTime(200);
digitalWrite(4,1); //open the valve
Sleepy::loseSomeTime(200);
digitalWrite(4,0); //close the valve
tManualValveOpen = millis();
tManualValveClose=tManualValveOpen;
}
void loop() {
endofrules:
flow=inputValue(2) ;// flow=digitalRead(2);
tManualValveOpen = millis();
tManualValveClose=tManualValveOpen; //reset the close valve timer
while (flow==1){ //while there is no flow
tManualValveOpen = millis(); //get the time that the manual valve has been opened
flow=inputValue(2) ;// flow=digitalRead(2);
// sleepSetup1();
}
currentMillis = millis(); // Get snapshot of time How much time has passed, accounting for rollover with subtraction!
// sleepSetup1();
Sleepy::loseSomeTime(100);
flow=inputValue(2) ;// flow=digitalRead(2);

if ((currentMillis - previousMillis) < t0 && flow==1){ //If the valve is closed during t0
** previousMillis = currentMillis; //reset the timer**
goto endofrules;}

if ((currentMillis - previousMillis) >= t0 && (tManualValveOpen-tManualValveClose) > t12){ //if t0 and manual valve is closed more than t12
** previousMillis = currentMillis; //reset the timer**
goto endofrules;}

// flow=digitalRead(2);

if ((currentMillis - previousMillis) >= (t0+t1) && (flow==0)) { //after t0+t1 close the valve
digitalWrite(5,1); //close the valve
Sleepy::loseSomeTime(200);
digitalWrite(5,0);
Sleepy::loseSomeTime(t2); //wait for delay time at close state
digitalWrite(4,1); //reopen the valve
Sleepy::loseSomeTime(200);
digitalWrite(4,0);;
sleepSetup(); //sleep till next operation
** previousMillis = currentMillis; //reset the timer**
}
}
void sleepSetup()
{
sleep_enable();
attachInterrupt(0, pinInterrupt, CHANGE);//Set pin 2 as interrupt and attach Interrupt Service Routine (ISR)
set_sleep_mode(SLEEP_MODE_PWR_DOWN);//define power down sleep mode
digitalWrite(13,LOW); // turn LED off to indicate sleep
sleep_cpu();//Set sleep enable (SE) bit, this puts the ATmega to sleep
Serial.println("just woke up!");//When it wakes up due to the interrupt the program continues with the instruction following sleep_cpu()
digitalWrite(13,HIGH); // turn LED on to indicate wake up
}

void sleepSetup1()
{
sleep_enable();
attachInterrupt(0, pinInterrupt, CHANGE);//Set pin 2 as interrupt and attach Interrupt Service Routine (ISR)
set_sleep_mode(SLEEP_MODE_IDLE);//define power down sleep mode
// digitalWrite(13,1); // turn LED off to indicate sleep
sleep_cpu();//Set sleep enable (SE) bit, this puts the ATmega to sleep
// Serial.println("just woke up!");//When it wakes up due to the interrupt the program continues with the instruction following sleep_cpu()
// digitalWrite(13,0); // turn LED on to indicate wake up
}
void pinInterrupt()//ISR
{
sleep_disable();//this is important. It is possible that the interrupt is called between executing "attachInterrupt(...)" and sleep_CPU() in the main loop
//if that happens without the sleep_disable() in the ISR, the ISR would be called, the interrupt detached and the device put to sleep.
//since the interrupt would be disabled at that point, there would be no way to wake the device up anymore.
//by putting sleep_disable() in the ISR, sleep_cpu() would not be effective during that loop, i.e. the main loop would run one more time
//and then properly attach the interrupt before hitting the sleep_cpu() a second time. At that point the device would go to sleep, but
//the interrupt would now be activated, i.e. wake-up can be induced.
detachInterrupt(0);//disable the interrupt. This effectively debounces the interrupt mechanism to prevent multiple interrupt calls.

}
boolean inputValue(int pin){
boolean val=0;
int vallow=0;
int valhigh=0;
for(int i=0;i<10;i++){
val=digitalRead(pin);
if (val=0){vallow++;} else{valhigh++;};
}
if (valhigh>vallow){val=1;}else{val=0;};
}

goto endofrules;

Uh-oh.

First, loose the stupid gotos. A simple return will cause loop() to be called again.

  if (valhigh>vallow){val=1;}else{val=0;};

Carriage returns cost nothing. Use them!

  int vallow=0;
   int valhigh=0;

camelCase is much preferred over this crap.

#include <avr/sleep.h>  //Power Management and Sleep Modes

If you expect millis() to keep working, STAY AWAKE!

Code: [Select]

#include <avr/sleep.h> //Power Management and Sleep Modes

If you expect millis() to keep working, STAY AWAKE!

I need to work my Arduino on batteries
The sleep mode use the "SLEEP_MODE_IDLE"
which keep the millis timer awake.
anyhow my problem is that the program sometimes does not run the command "previousMillis = currentMillis;"

Thanks for your help

     if (val=0)

Oops.

 digitalWrite(4,1); //reopen the valve

You've given the pins nice names - why not use them, then you could lose the (now) obvious comments?

Thanks for the code writing remarks

anyhow my problem is that the program sometimes does not run the command "previousMillis = currentMillis;"

Nonsense. There is something else wrong with your improperly posted code.

The paragraphs of comments in your code don't help. Stripping away all the crap, the pinInterrupt() function looks like:

void pinInterrupt()
{
  sleep_disable();
  detachInterrupt(0);
}

So much simpler to read!

If the comments ARE necessary, they should be placed in column 0, BEFORE the function with spacing between the // and the text, and broiken up so as to NOT require horizontal spacing to read:

// this is important. It is possible that the interrupt is called between executing
// "attachInterrupt(...)" and sleep_CPU() in the main loop
// if that happens without the sleep_disable() in the ISR, the ISR would be called,
// the interrupt detached and the device put to sleep.
// since the interrupt would be disabled at that point, there would
// be no way to wake the device up anymore.
// by putting sleep_disable() in the ISR, sleep_cpu() would not be effective
// during that loop, i.e. the main loop would run one more time
// and then properly attach the interrupt before hitting the sleep_cpu()
// a second time. At that point the device would go to sleep, but
// the interrupt would now be activated, i.e. wake-up can be induced.

void pinInterrupt()
{
  sleep_disable();
  detachInterrupt(0);
}

if ((currentMillis - previousMillis) >= (t0+t1) && (flow==0)) { //after t0+t1 close the valve
digitalWrite(5,1); //close the valve
Sleepy::loseSomeTime(200);
digitalWrite(5,0);
Sleepy::loseSomeTime(t2); //wait for delay time at close state
digitalWrite(4,1); //reopen the valve
Sleepy::loseSomeTime(200);
digitalWrite(4,0);;
sleepSetup(); //sleep till next operation
previousMillis = currentMillis; //reset the timer
}

look at this code

when the loop first run the digitalWrite to both ports occur once (giving 200ms pulse) as written on the code
then its sleep and when its wake, the

previousMillis = currentMillis;

should change the value in the "if" to (currentMillis - previousMillis) =0
and if so the "if" is false and its has to go to the end of the loop.
but instead, its repeat the "if" instruction again

yachin:
should change the value in the "if" to (currentMillis - previousMillis) =0
and if so the "if" is false and its has to go to the end of the loop.
but instead, its repeat the "if" instruction again

If the IF statement runs again it is because the test is true.

What happens to the millis() counter while the Arduino is asleep ?
The value of currentMillis won't have moved on but the value of millis() may have. Maybe in this case your code should say prevMillis = millis();

...R

There is a REALLY simple way to test your assertion that the statement is not being executed. After the statement, put this:

   Serial.print("Current value: ");
    Serial.println(currentMillis);
    Serial.print("Previous value: ");
    Serial.println(previousMillis);

If the two values printed are the same, you are wrong. If not, I'm wrong. I know which one I and betting on.

Be sure to put those 4 lines in all three places where the assignment happens/does not happen.

Robin2:
If the IF statement runs again it is because the test is true.

What happens to the millis() counter while the Arduino is asleep ?
The value of currentMillis won't have moved on but the value of millis() may have. Maybe in this case your code should say prevMillis = millis();

...R

If I eliminate the sleep, then its getting to an endless loop

@yachin: Please edit your post, select the code, and put it between [code] ... [/code] tags.

You can do that by hitting the "Code" icon above the posting area. It is the first icon, with the symbol: </>

Apart from the atrocious formatting, and what everyone else has said, what about this?

boolean inputValue(int pin){
  boolean val=0;
  int vallow=0;
  int valhigh=0;
  for(int i=0;i<10;i++){
    val=digitalRead(pin);
    if (val=0){
      vallow++;
    } 
    else{
      valhigh++;
    };
  }
  if (valhigh>vallow){
    val=1;
  }
  else{
    val=0;
  };
}

In particular all the effort you go to to set val to 0 or 1. What is the point? val goes out of scope immediately, so who cares what its value is?

Not to mention that that function SAYS that it returns a boolean, but doesn't actually return anything.

PaulS:
There is a REALLY simple way to test your assertion that the statement is not being executed. After the statement, put this:

   Serial.print("Current value: ");

Serial.println(currentMillis);
   Serial.print("Previous value: ");
   Serial.println(previousMillis);




If the two values printed are the same, you are wrong. If not, I'm wrong. I know which one I and betting on.

Be sure to put those 4 lines in all three places where the assignment happens/does not happen.

well i did so and I got those values:
currentMillis - previousMil =0
t0+t1 = 4000
flow = 0

the "if" statement was:

if ((currentMillis - previousMillis) >= (t0+t1) && (flow==0))

since currentMillis - previousMil =0
and
t0+t1 = 4000

the "if" is false
but its still with endless loop doing the "if" instructions

this code I wrote to try and see if there are any interference causing the digitalRead to get the wrong value.
I read few times and return the value of "flow" as the most times value

Did you fix the errors pointed-out?

AWOL:
Did you fix the errors pointed-out?

YES

(that was hint to post your code, sorry)

AWOL:
(that was hint to post your code, sorry)

#include <JeeLib.h> // Low power functions library
#include <avr/sleep.h>  //Power Management and Sleep Modes
unsigned long t0=1000;//8000;  // t0 = if the manual valve is closed at this time, program will stop
unsigned long t1=4000;//2000;  // t1 = additional period of time that the latch valve will be open.
unsigned long t12=9000;  // t12 = if the manual valve is closed for up to t12 mSec during t1 than don't do nothing. if over t12 than t2=0
unsigned long t2=5000;//00;  // t2 = time that the latch valve is closed.
int OpenValve = 4;    // Digital Arduino Pin used to open the latch valve
int CloseValve = 5;    // Digital Arduino Pin used to close the latch valve
boolean flow=1;  //state of the flow sensor
unsigned long currentMillis;
unsigned long previousMillis;
unsigned long tManualValveOpen;  //time that the manual valve has been reopened
unsigned long tManualValveClose;  //time that the manual valve has been closed
ISR(WDT_vect) { Sleepy::watchdogEvent(); }

void setup(){ 
    Serial.begin(9600);     //start serial communication at 9600 baud
    pinMode(13,OUTPUT);     // set pin 13 as an output so we can use LED to monitor
    digitalWrite(13,LOW);   // turn pin 13 LED off
//  ADCSRA = 0;
  pinMode(OpenValve, OUTPUT); // declare pin 4 to be an output:
  pinMode(CloseValve, OUTPUT); // declare pin 5 to be an output:
  pinMode(2, INPUT_PULLUP); //declare p1n 2 as input:
  Sleepy::loseSomeTime(200);
  digitalWrite(OpenValve,1); //open the valve
  Sleepy::loseSomeTime(200);
  digitalWrite(OpenValve,0); //close the valve
  tManualValveOpen = millis();
  tManualValveClose=tManualValveOpen;
}
void loop()   {
flow=inputValue(2) ;//   flow=digitalRead(2);   
   tManualValveOpen = millis();
   tManualValveClose=tManualValveOpen; //reset the close valve timer
       while (flow==1){   //while there is no flow
 tManualValveOpen = millis();  //get the time that the manual valve has been opened
flow=inputValue(2) ;// flow=digitalRead(2);
//        sleepSetup1();
      }
      currentMillis = millis();  // Get snapshot of time How much time has passed, accounting for rollover with subtraction!
//      sleepSetup1();
  Sleepy::loseSomeTime(100);
flow=inputValue(2) ;//  flow=digitalRead(2);
  
      if ((currentMillis - previousMillis) < t0 && flow==1){ //If the valve is closed during t0
          previousMillis = currentMillis; //reset the timer
          return;}

      if ((currentMillis - previousMillis) >= t0 && (tManualValveOpen-tManualValveClose) > t12){ //if t0 and manual valve is closed more than t12 
          previousMillis = currentMillis; //reset the timer
          return;}

//  flow=digitalRead(2);   

      if ((currentMillis - previousMillis >= (t0+t1)) && (flow==0)) {  //after t0+t1 close the valve
          digitalWrite(CloseValve,1); //close the valve
          Sleepy::loseSomeTime(200);
          digitalWrite(CloseValve,0);
          Sleepy::loseSomeTime(t2);  //wait for delay time at close state
          digitalWrite(OpenValve,1); //reopen the valve
          Sleepy::loseSomeTime(200);
          digitalWrite(OpenValve,0);; 
  previousMillis = currentMillis; //reset the timer  
//  sleepSetup(); //sleep till next operation
  previousMillis = currentMillis; //reset the timer 
     Serial.print("t0+t1: ");
     Serial.println(t0+t1);
     Serial.print("flow: ");
     Serial.println(flow);
         Serial.print("currentMillis - previousMillis: ");
         Serial.println(currentMillis - previousMillis);
      }  
   }
void sleepSetup() 
{
    sleep_enable();
    attachInterrupt(0, pinInterrupt, CHANGE);//Set pin 2 as interrupt and attach Interrupt Service Routine (ISR)
    set_sleep_mode(SLEEP_MODE_PWR_DOWN);//define power down sleep mode
    digitalWrite(13,LOW);   // turn LED off to indicate sleep
    sleep_cpu();//Set sleep enable (SE) bit, this puts the ATmega to sleep
    Serial.println("just woke up!");//When it wakes up due to the interrupt the program continues with the instruction following sleep_cpu()
    digitalWrite(13,HIGH);   // turn LED on to indicate wake up
}

void sleepSetup1()
{
 sleep_enable();
 attachInterrupt(0, pinInterrupt, CHANGE);//Set pin 2 as interrupt and attach Interrupt Service Routine (ISR)
 set_sleep_mode(SLEEP_MODE_IDLE);//define power down sleep mode
// digitalWrite(13,1);   // turn LED off to indicate sleep
 sleep_cpu();//Set sleep enable (SE) bit, this puts the ATmega to sleep
// Serial.println("just woke up!");//When it wakes up due to the interrupt the program continues with the instruction following sleep_cpu()
// digitalWrite(13,0);   // turn LED on to indicate wake up
}
void pinInterrupt()//ISR
{
  sleep_disable();//this is important. It is possible that the interrupt is called between executing "attachInterrupt(...)" and sleep_CPU() in the main loop
                  //if that happens without the sleep_disable() in the ISR, the ISR would be called, the interrupt detached and the device put to sleep.
                  //since the interrupt would be disabled at that point, there would be no way to wake the device up anymore.
                  //by putting sleep_disable() in the ISR, sleep_cpu() would not be effective during that loop, i.e. the main loop would run one more time
                  //and then properly attach the interrupt before hitting the sleep_cpu() a second time. At that point the device would go to sleep, but 
                  //the interrupt would now be activated, i.e. wake-up can be induced.
  detachInterrupt(0);//disable the interrupt. This effectively debounces the interrupt mechanism to prevent multiple interrupt calls.
}
boolean inputValue(int pin){
 boolean val=0;
 int vallow=0;
 int valhigh=0;
 for(int i=0;i<10;i++){
 val=digitalRead(pin);
 if (val=0){vallow++;} else{valhigh++;};
 }
 if (valhigh>vallow){val=1;}else{val=0;};
}

Moderator edit: Tags corrected.