Am I overflowing a variable on State 2?

I have hit the wall and cannot see what I am doing wrong.

It will not exit state 2 after turning on a valve.

I will be using times up to 60min in each zone, FYI

any help is appreciated.

p.s. it is not getting hung on any of the radio commands.

/*
Sprinkler Valves
*/
#include <Relay.h>
#include <SPI.h>
#include <EEPROM.h> 
#include <RF24.h>
//
#define NUMBER_OF_VALVES 8 // Change this to set you valve count.
#define RESET_TIME 5000    // Change this (in milliseconds) for the time you need your valves to change state
int valveTime [9];
int valveSoloTime [9];
byte valveByte [8] = {0x01, 0x02, 0x04, 0x08, 0x10, 0x20, 0x40, 0x80};
int valveNumber;
byte state = 0;
unsigned long startMillis;
//
int latchPin = 8;
int clockPin = 4;
int dataPin  = 7;
// 
Sensor gw;
//
void setup() 
{ 
  Serial.begin(115200);
  gw.begin();

  pinMode(latchPin, OUTPUT);
  pinMode(clockPin, OUTPUT);
  pinMode(dataPin, OUTPUT);
  //
  gw.sendSketchInfo("Sprinkler", "0.9");
  //
  for (int i=0; i<NUMBER_OF_VALVES + 1; i++) // Register all valves to gw (they will be created as child devices)
  {
    gw.sendSensorPresentation(i, S_LIGHT);
  }
  Serial.println("Sensor Presentation Complete");
  //
  allValvesOff();
  Serial.println("All Valves OFF");
  //  
  for (int i = 0; i < NUMBER_OF_VALVES + 1; i++)
  {
    gw.sendVariable( i, V_LIGHT, 0); //Display each Valve OFF
    valveTime [i] = atoi(gw.getStatus( i, V_VAR1));// Get each Valve Cycle time
    valveSoloTime [i] = atoi(gw.getStatus( i, V_VAR2));// Get each Valve Solo time
    Serial.print("Watering times collected from device: "); 
    Serial.println(i);
  }
  for (int i = 0; i < NUMBER_OF_VALVES; i++)
  {
    Serial.print("Valve "); 
    Serial.print(i); 
    Serial.print(" cycle time="); 
    Serial.println(valveTime[i]);
    Serial.print("Valve "); 
    Serial.print(i); 
    Serial.print(" solo  time="); 
    Serial.println(valveSoloTime[i]);
  }
  Serial.println("READY");
}
//
void loop()
{
  if (gw.messageAvailable()) {
    message_s message = gw.getMessage();
    setValveStatus(message);
  }
  if (state == 0) 
  {
    allValvesOff();
  }
  if (state == 1) //Run all Valves
  {
    unsigned long nowMillis = millis();
    if (nowMillis - startMillis < RESET_TIME)
    {
      allValvesOff();
    }
    else if (nowMillis - startMillis < valveTime[valveNumber] * 60 * 1000)
    {
      digitalWrite(latchPin, LOW);
      shiftOut(dataPin, clockPin, LSBFIRST, valveByte [valveNumber]); 
      digitalWrite(latchPin, HIGH);
    }
    else if (nowMillis - startMillis > valveTime [valveNumber]  * 60 * 1000)
    {
      allValvesOff();
      startMillis = millis();
      gw.sendVariable( valveNumber, V_LIGHT, 0);
      valveNumber++;
      gw.sendVariable( valveNumber, V_LIGHT, 1);
      if (valveNumber > NUMBER_OF_VALVES) 
      {
        state = 0;
        Serial.print("State = "); Serial.println(state);
        gw.sendVariable( NUMBER_OF_VALVES + 1, V_LIGHT, 0);
      }
    }
  }
  if (state == 2)// Run single valve
  {
    unsigned long nowMillis = millis();
    if (nowMillis - startMillis < RESET_TIME)
    {
      allValvesOff();
    }
    else if (nowMillis - startMillis < valveSoloTime [valveNumber] * 60 * 1000)
    {
      digitalWrite(latchPin, LOW);
      shiftOut(dataPin, clockPin, LSBFIRST, valveByte [valveNumber]); 
      digitalWrite(latchPin, HIGH);
    }
    else if (nowMillis - startMillis > valveSoloTime [valveNumber] * 60 * 1000)
    {
      allValvesOff();
      state = 0;
      Serial.print("State = "); Serial.println(state);
      gw.sendVariable( valveNumber, V_LIGHT, 0);
    }
  }
}
//
void setValveStatus(message_s message) {
  if (message.header.messageType==M_SET_VARIABLE && message.header.type==V_LIGHT) 
  {
    valveNumber = message.header.childId;
    int incomingRelayStatus = atoi(message.data);
    if (incomingRelayStatus != 1)
    {
      state = 0;
      Serial.println("All Valves Off");
      Serial.print("state = "); 
      Serial.println(state);
    }
    else
    { 
      if (valveNumber != NUMBER_OF_VALVES)
      {
        state = 2;
        startMillis = millis();
        Serial.print("Cycling Valve #: "); 
        Serial.println(valveNumber);
        Serial.print("state = "); 
        Serial.println(state);
      }
      else 
      {
        state = 1;
        valveNumber = 0;
        gw.sendVariable( valveNumber, V_LIGHT, 1);
        startMillis = millis();
        Serial.println("Cycling through ALL Valves");
        Serial.print("state = "); 
        Serial.println(state);
      }
    }
  }
}
//
void allValvesOff()
{
  digitalWrite(latchPin, LOW);
  shiftOut(dataPin, clockPin, LSBFIRST, 0x00);
  digitalWrite(latchPin, HIGH); 
}
else if (nowMillis - startMillis < valveTime[valveNumber] * 60 * 1000)

Not sure if this is it, but try changing the 60 to 60L and see if that helps.

Delta_G:

else if (nowMillis - startMillis < valveTime[valveNumber] * 60 * 1000)

Not sure if this is it, but try changing the 60 to 60L and see if that helps.

Yup, that helped

AWOL:

byte valveByte [8] = {0x01, 0x02, 0x04, 0x08, 0x10, 0x20, 0x40, 0x80};
for (int i = 0; i < NUMBER_OF_VALVES + 1; i++)

{
    gw.sendVariable( i, V_LIGHT, 0); //Display each Valve OFF
    valveTime [i]



Hmmm.

Yup, I declared the times as int, S/B long.

I’m happy now.

Thanks guys!

I find state machines implemented with switch/case much easier to write and maintain than using if/else. Use the state number as the switch variable and have a case for each state. Using an enum or #defines to give states names helps even more.

UKHeliBob: I find state machines implemented with switch/case much easier to write and maintain than using if/else. Use the state number as the switch variable and have a case for each state. Using an enum or #defines to give states names helps even more.

Thanks.

When I set out to do this there were only two states (OFF and CYCLING), but I thought, why not give myself the ability (through the Vera Controller with which this speaks) to lay on a single zone (e.g. if I notice that that patch of grass was dry, or just put in new plantings). That made it three, as these things happen, it ended up here.

I just finished another with a serial connection from a master that I used the switch/case. Works well, thanks to all the help I got here!

unsigned long xyz = 60000UL; // where 60000L is a long constant even if it is 32-bits

but wth, it only makes a difference when time gets to about 25 days and then you post a new thread.

I don’t know why you don’t use unsigned long for all the time variables.

int valveTime [9];
int valveSoloTime [9];

This works out to be: ( UL - UL < int * int * int ) where the 2 multiplies overflow the int for sure every time.

    else if (nowMillis - startMillis < valveTime[valveNumber] * 60 * 1000)

It would be smarter and faster to make valveTime a UL array that already has the full value pre-calculated.
Or for something slower, cast valveTime to unsigned long and multiply by a UL to compare to… a UL.

    else if ( nowMillis - startMillis < ((unsigned long) valveTime[ valveNumber ] * 60000UL ))

What is with the elapsed time (now - start) being LESS THAN the wait time?
That is going to be true an amazing amount of the time.

You might want to use a byte to keep track of the state of the 8 valves as 1 bit each; 1 for open, 0 for shut.
If -any- valve is open, that byte will be non-zero, if all are shut it will be 0.
Then in allValvesOff() you can test the byte and only shiftOut() when the valves aren’t already all shut.

I see repeated groups of lines like:

      digitalWrite(latchPin, LOW);
      shiftOut(dataPin, clockPin, LSBFIRST, valveByte [valveNumber]); 
      digitalWrite(latchPin, HIGH);

that could be put in a function to replace even places where similar lines are used

void writeTheValves( byte valves )
{
      digitalWrite(latchPin, LOW);
      shiftOut(dataPin, clockPin, LSBFIRST, valves ); 
      digitalWrite(latchPin, HIGH);
}

that will handle ALL of your shiftOut’s, even allValvesShut().

byte valveByte [8] = {0x01, 0x02, 0x04, 0x08, 0x10, 0x20, 0x40, 0x80};

could do, without an array but about as fast as an array lookup

//      shiftOut(dataPin, clockPin, LSBFIRST, valveByte [valveNumber]); 
         writeTheValves( 1 << valveNumber );

http://arduino.cc/en/Reference/Bitshift

or

//      shiftOut(dataPin, clockPin, LSBFIRST, valveByte [valveNumber]); 
         writeTheValves( bit( valveNumber ));

http://arduino.cc/en/Reference/Bit

@GoForSmoke,

Thanks for the suggestions, I fixed all the ULs.

I don't understand this:

What is with the elapsed time (now - start) being LESS THAN the wait time?

I am turning off all of the relays for 5 seconds to let the valves get to hydraulic equilibrium, then I hold for the time period of:

((unsigned long) valveTime[ valveNumber ] * 60000UL )

then I de-energize all of the relays.

I don't early care about keeping track of their on/off state, they are only allowed to be on one at a time.

UKHeliBob: I find state machines implemented with switch/case much easier to write and maintain than using if/else. Use the state number as the switch variable and have a case for each state. Using an enum or #defines to give states names helps even more.

What I like even more is to have an array of function pointers to what would be the cases in that model and an enum with the names of the states to index it. Using that method, if one needs to make runtime changes to the behavior of one state or another then one can simply point to a different function.

My loops often look like:

void loop(){

(*callback[STATE])();
}

BulldogLowell: @GoForSmoke,

Thanks for the suggestions, I fixed all the ULs.

I don't understand this:

What is with the elapsed time (now - start) being LESS THAN the wait time?

I am turning off all of the relays for 5 seconds to let the valves get to hydraulic equilibrium, then I hold for the time period of:

((unsigned long) valveTime[ valveNumber ] * 60000UL )

then I de-energize all of the relays.

I don't early care about keeping track of their on/off state, they are only allowed to be on one at a time.

I'm sure that it hurts nothing much. Just be aware that you're probably running the all off more than 10,000 times when once would do. A simple check would take care of that and shorten your loop() for something else.

Everything you do now to shorten/optimize your code will be for your own education. Try things out from time to time since even what doesn't work will have value to you in years to come. Later on you will have answers and ideas for approaches you would not have on your own otherwise.

GoForSmoke: I'm sure that it hurts nothing much. Just be aware that you're probably running the all off more than 10,000 times when once would do. A simple check would take care of that and shorten your loop() for something else.

Everything you do now to shorten/optimize your code will be for your own education. Try things out from time to time since even what doesn't work will have value to you in years to come. Later on you will have answers and ideas for approaches you would not have on your own otherwise.

I recall telling you that my internal library of working examples is still small. Each time I post there it gets a few bytes bigger. XD

I really appreciate everyone's help.

Delta_G: What I like even more is to have an array of function pointers to what would be the cases in that model and an enum with the names of the states to index it. Using that method, if one needs to make runtime changes to the behavior of one state or another then one can simply point to a different function.

So, I may even make that change, too.

Thanks, again.

Delta_G:

UKHeliBob: I find state machines implemented with switch/case much easier to write and maintain than using if/else. Use the state number as the switch variable and have a case for each state. Using an enum or #defines to give states names helps even more.

What I like even more is to have an array of function pointers to what would be the cases in that model and an enum with the names of the states to index it. Using that method, if one needs to make runtime changes to the behavior of one state or another then one can simply point to a different function.

My loops often look like:

void loop(){

(*callback[STATE])(); }

And you can use data to feed what state to run, change the program without changing the code.

Program = code + data.

Most uses of function pointers I've seen are little better than switch-case except for savings in code lines. On AVR's you need to keep the function pointers in PROGMEM if you have more than a few due to limited RAM.

Bulldog! I forgot to mention, you don't use the F( ) macro for constant text prints. When will you be ready to move your constants to flash instead of RAM?

  Serial.println("Sensor Presentation Complete"); // stores in flash then moves to RAM at startup, RAM wasted
  Serial.println( F( "Sensor Presentation Complete" )); // stores in flash and prints from flash, no RAM wasted

GoForSmoke: Bulldog! I forgot to mention, you don't use the F( ) macro for constant text prints. When will you be ready to move your constants to flash instead of RAM?

OK, I am ready.

:blush:

I saw posts on that but didn't really get it. Now I got it.

you see... byte by byte

Here ya go, how and a bit of why to table pre-calculated values in PROGMEM.

http://forum.arduino.cc/index.php?topic=244455.0

Integers (8, 16, 32 or 64 bit) are the way to go when your processor has no FPU.

If you want meters to 3 places then work in millimeters or smaller (smaller to keep accuracy in divides, or divide last) and you will have your precision and accuracy. With floats you get guaranteed 6 place precision but not always the same accuracy. Google "floating point pile of sand" for explanations why.