Track minutes per day a valve is on

I would like to track the number of minutes a valve is on each day. It can go on and off several times during the day and I want to accumulate all that time, then reset at midnight. I have a Real Time Clock, so it's easy to reset at midnight. The solution I came up with to track the number of minutes seems cludgy to me and I'm wondering if there is a simpler solution. My current approach is to use millis() to count in 1 second intervals when pin 2 is high. Every second that passes I add 1/60 (one second) to my daily timer. I know this is not super accurate, but that's fine for my purposes. I'd like to know if anyone has a simpler way.

uint32_t oneSecondTimer;
bool oneSecondTimerRunning;   
float valveOnMinutes;   // # minutes each day valve is open (on)

void setup() {
  pinMode(2, OUTPUT);
}

void loop() {
  if (digitalRead(2) == HIGH)  {
    if (oneSecondTimerRunning == false) {
      // start one second timer
      oneSecondTimer = millis() + 1000;
      oneSecondTimerRunning = true; 
    }
    
    // if one second has passed, increment valveOnMintues and reset one second timer
    if ((long)(millis() - oneSecondTimer >= 0)) {
      valveOnMinutes += 1/60.0;  // add one second to minutes valve was on
      oneSecondTimerRunning = false;
    }
  }
  // reset valveOnMinutes at midnight
}

You know when the valve goes on. You know when it goes off. Determining hos long it has been on, when it goes off, in milliseconds, is easy. Converting that to seconds is easy. Adding that to a running total is easy.

Resetting the running total, and, if necessary, pretending that the valve was shut off and turned back on at reset time is easy. I guess I'm failing to see the problem.

Maybe a little cludgy, but if it's working, it is fine by me.

My approach would be different.
I would set the sample rate at 100ms. Every 100ms I would check the pin and increment an unsigned long if the pin was high. At the end of the day the total is the time in 0.1seconds. Divide that by 600 for the minutes.
A sample rate of 100ms can be created with millis(), and the rollover problem can be avoided.
http://playground.arduino.cc/Code/TimingRollover#arithmetic

An other way is to use interrupt. Generate an interrupt when the signal changes to high and to low, and use millis() to determine the time.

bug!

void setup(){
  pinMode(2, OUTPUT);  // should be INPUT
}

void loop() {
  if (digitalRead(2) == HIGH)  {
  ...

using additions with time that can will overflow is not recommended, you should rewrite using subtractions.
oneSecondTimer = millis() + 1000;

also casting millis() to long will cause trouble. unsigned long or uint32_t is the right type

PaulS:
You know when the valve goes on. You know when it goes off. Determining hos long it has been on, when it goes off, in milliseconds, is easy. Converting that to seconds is easy. Adding that to a running total is easy.

Resetting the running total, and, if necessary, pretending that the valve was shut off and turned back on at reset time is easy. I guess I'm failing to see the problem.

Actually that's the way I'm doing it now, I should have mentioned that. I'm sending the data real time to Xively.com which is charting the data. Currently it will not show any change until the valve has turned off and then I calculate the time on. But I would prefer to show minutes on in real time, not at the end.

Currently it will not show any change until the valve has turned off and then I calculate the time on.

Keep track of how long the valve has been on, as of the last time it was shut off. Then, add the amount of time the valve has been on, since it was turned on, if its on. Again, this seems so simple. I suspect that you are simply overthinking the problem.

Imagine how YOU would track the time. Only then, can you figure out how to make the Arduino do it.

ScottG:
I'd like to know if anyone has a simpler way.

Poll the state of the valve as frequently as you like. Remember the previous state, and compare with the new state to see whether the state has changed. There is an example sketch demonstrating this 'edge detection' logic. I recommend including some debounce logic so you don't get multiple triggers when the valve is moving (there are example sketches demonstrating this, too).

If the valve state changes from OFF to ON then record the current time in a variable.
If the valve state changes from ON to OFF then subtract the 'on' time you recorded earlier from the current time to get the elapsed time it was on for. Add this to an accumulator.

Use the logic PaulS mentioned to reset the accumulator and restart timing at midnight.

robtillaart:
bug!

void setup(){

pinMode(2, OUTPUT);  // should be INPUT
}

The arduino controls the valve, so pin 2 is an output.

void loop() {
if (digitalRead(2) == HIGH) {

The, why are you reading it? Can't you remember something simple like having turned the valve on? You really should, so that you can record when you did it, so that, when you turn it off, you can remember when you did that.

ScottG:
The arduino controls the valve, so pin 2 is an output.

That's incorrect - the sketch contains no code that controls the valve. Pin 2 is used as an input in this sketch. If that's not what's happening in your real project then you need to step back and ask your question again, this time using code that demonstrates the actual problem.

I did not post my actual sketch. My current sketch doesn't even do it the way I showed in the original post, I'm already doing it the way PaulS suggested. I just whipped up the code to show an example of another way I was thinking about. I wanted to see if anyone had suggestions for a simpler way to do this. I was hoping their might be some elegant way of doing this that I hadn't though of.

Here's the real sketch:

It might be me, but I think my idea in reply #2 is the most elegant.
Using millis() to get a timing of 100ms, and all that is needed is : if pin is high: count++.

Did you check that link to solve the rollover problem ?
You cast millis() to a signed long, that introduces the rollover problem.
But you have to check every 'if' to check how millis() and the timer variable is used, to be sure that the value does not get below zero. See also that link in reply #2 how the led is blinking.

Erdin:
It might be me, but I think my idea in reply #2 is the most elegant.
Using millis() to get a timing of 100ms, and all that is needed is : if pin is high: count++.

Did you check that link to solve the rollover problem ?
You cast millis() to a signed long, that introduces the rollover problem.
But you have to check every 'if' to check how millis() and the timer variable is used, to be sure that the value does not get below zero. See also that link in reply #2 how the led is blinking.

I did look at the rolleover link. The solution shown in the first example is how I generally deal with timers.

I think your suggestion would look something like this:

// note code is not complete, just trying to get across an idea, not working code
uint32_t checkValveStatus;
uint32_t dailyValveOnTime = 0;  // divide by 600 to get minutes

void setup() {
  checkValveStatus = millis() + 100UL;
}

void loop() {
  if ((long)(millis() - checkValveStatus) >= 0 )
  {
    checkValveStatus = millis() + 100UL;

   // 100mS has passed , time to update counter if the valve is on
    if (digitalRead(2) == HIGH )
    { dailyValveOnTime++; }
  }

  // reset counter at midnight
  if(IsMidnight) { 
    dailyValveOnTime = 0; }
}

I do like your way better. It's a bit simpler.

Sorry, but no.
You still cast millis() to long, introducing the rollover problem.
And you call millis() twice for the same event, that could cause an out of pace glitch.

Erdin:
Sorry, but no.
You still cast millis() to long, introducing the rollover problem.
And you call millis() twice for the same event, that could cause an out of pace glitch.

I don't think I have a rollover problem. I'm not really casting millis to long, I'm casting (millis() - checkValveStatus).

I did a little test. I created a couple uint32_t variables to take the place of millis() and my timer. I forced it to rollover to see the result. Here's my sketch

uint32_t mlls =  4294967195;  // millis() substitute. Rolls over at 4294967295
uint32_t tmr = 4294967250;    // timer

void setup()
{
  // tmr = tmr + 60; 
  Serial.begin(9600);
  while (!Serial) ; // Needed for Leonardo only
  Serial.println("Rollover test");
  Serial.println("i\tmlls        \ttmr             mlls-tmr   long(mlls-tmr)");
  for (int i=0; i < 20; i++){
    Serial.print(i);
    Serial.print("\t");
    Serial.print(mlls);
    if (mlls < 1000) Serial.print("\t");  // helps line up columns
    Serial.print("\t");
    Serial.print(tmr);
    Serial.print("\t");
    if (tmr < 1000) Serial.print("\t");  // helps line up columns
    Serial.print(mlls - tmr);
    Serial.print("\t");
    if (mlls - tmr < 1000) Serial.print("\t");  // helps line up columns
    Serial.print(long(mlls - tmr));
    Serial.println();
    mlls = mlls + 10;    
  }
}

void loop(){}

Here's the output

Rollover test
i	mlls        	tmr             mlls-tmr   long(mlls-tmr)
0	4294967195	4294967250	4294967241	-55
1	4294967205	4294967250	4294967251	-45
2	4294967215	4294967250	4294967261	-35
3	4294967225	4294967250	4294967271	-25
4	4294967235	4294967250	4294967281	-15
5	4294967245	4294967250	4294967291	-5
6	4294967255	4294967250	5		5
7	4294967265	4294967250	15		15
8	4294967275	4294967250	25		25
9	4294967285	4294967250	35		35
10	4294967295	4294967250	45		45
11	9		4294967250	55		55
12	19		4294967250	65		65
13	29		4294967250	75		75
14	39		4294967250	85		85
15	49		4294967250	95		95
16	59		4294967250	105		105
17	69		4294967250	115		115
18	79		4294967250	125		125
19	89		4294967250	135		135

If I was doing something like if (long(millis() - tmr) >=0) {}, then when i=6 the if() statement would be true, which is what I want. If millis keep going and rolls over at i=11, everything is still okay.

If if tmr was close to rolling over and I added time to it, it still works. If you remove comment in front of tmr = tmr + 60. Then tmr starts out rolled over and is 14. But long(mlls-tmr) still stays negative until mlls rolls over and is > 14. Here's the output of that scenario.

Rollover test
i	mlls        	tmr             mlls-tmr   long(mlls-tmr)
0	4294967195	14		4294967181	-115
1	4294967205	14		4294967191	-105
2	4294967215	14		4294967201	-95
3	4294967225	14		4294967211	-85
4	4294967235	14		4294967221	-75
5	4294967245	14		4294967231	-65
6	4294967255	14		4294967241	-55
7	4294967265	14		4294967251	-45
8	4294967275	14		4294967261	-35
9	4294967285	14		4294967271	-25
10	4294967295	14		4294967281	-15
11	9		14		4294967291	-5
12	19		14		5		5
13	29		14		15		15
14	39		14		25		25
15	49		14		35		35
16	59		14		45		45
17	69		14		55		55
18	79		14		65		65
19	89		14		75		75

Erdin:
An other way is to use interrupt. Generate an interrupt when the signal changes to high and to low, and use millis() to determine the time.

If there is a long time between cycles of the valve, I think this is the way to go. You could even put the system to sleep in the mean time, to conserve power. Do an interrupt on pin change, then when the interrupt is called, read whether the pin is high or low. If the pin is high, the valve turned on, and vice versa. Mark time when valve turns on. Mark time when valve turns off and add to counter.

ScottG:
I did a little test.

Thank for that test !
You have convinced me.

When you call millis() just once, and use option 2, the timing stays in pace.
http://playground.arduino.cc/Code/TimingRollover#arithmetic
Option two is not to add the interval to millis(), but start with a variable and increment that variable. If the sketch delays the timing, it will catch up.
That variable can be set in the setup() function to millis().

I had set that variable at the beginning of setup(), but that caused a lot timing ticks shortly after another when the loop() function starts running. I had a Ethernet DHCP in setup(), so that took some time. Now I set that variable to millis() at the end of setup() and all is well.

joshuabardwell:

Erdin:
An other way is to use interrupt. Generate an interrupt when the signal changes to high and to low, and use millis() to determine the time.

If there is a long time between cycles of the valve, I think this is the way to go. You could even put the system to sleep in the mean time, to conserve power. Do an interrupt on pin change, then when the interrupt is called, read whether the pin is high or low. If the pin is high, the valve turned on, and vice versa. Mark time when valve turns on. Mark time when valve turns off and add to counter.

The valve is on for about 15 minutes each time. My sketch is doing a bunch of other things so there is no incentive to put it to sleep. It's not battery operated, so I don't care about power anyway. The sketch controls the valve, so there is no need to use interrupts or anything. I always know when the valve is on or off.