If and Else not working as expected

Hi,

I haven’t programmed C for a while, but I’m confused by this simple bit of code not working as expected. It is the end of the day and I’m tired, so maybe fresh eyes can see the no doubt obvious mistake! Something seems to be going wrong with the if and else if bit, can anyone point out my mistakes?

#define BATTERYCHECKINTERVAL 10000
//interval in ms
#define BATTERYTHRESHOLD 980
#define BATTERYLOWPIN 13

int batteryTimerPrevious = 0;

void setup() {

  int i;
  pinMode (1, OUTPUT);
  pinMode (2, INPUT);
  pinMode (BATTERYLOWPIN,OUTPUT);
  analogReference (INTERNAL);
  delay (250);
  for (i=0; i<3; i++) {
    digitalWrite (BATTERYLOWPIN, HIGH);
    delay (500);
    digitalWrite (BATTERYLOWPIN,LOW);
    delay (1000);
  }

  //get current time since execution so we can make a comparison in loop()
  delay (3500);
  batteryTimerPrevious = millis ();
}

void loop() {
  static int batteryTimerCurrent=0;
  static boolean batteryLow = false, batteryLowPinState = false;
  // voltage checking timer 
  batteryTimerCurrent = millis ();
//code removed for clarity here
    batteryLowPinState = lowBatteryFlash (BATTERYLOWPIN, batteryLowPinState, batteryTimerCurrent);
}

boolean lowBatteryFlash (int pin, boolean pinState, int currentTime)
//flashes pin at one second on, five seconds off
//returns pin state
{
  static int previousTime=0;
  int difference;
  difference = currentTime-previousTime;
  if (difference < 0) { previousTime = currentTime;}
  //if pin state is off and five seconds have elapsed, turn it on, else if pinstate is on and one second has elasped, turn it off
  if (!pinState && difference > 5000) {
    //turn it on
    digitalWrite (pin, HIGH);
    previousTime = currentTime;
    return true;
  }
  else if (pinState && difference > 1000) {
    //turn it off
    digitalWrite (pin, LOW);
    previousTime = currentTime;
    return false;
  }
}

Cheers,
Mark

It'd help if you let us know what your expectation of this code is!

millis() returns unsigned long. All variables used for math with milliseconds should be unsigned long too. furthermore always use subtraction as this handles the wrapping of the unsigned long after 49++days

With these declared in loop(), both will always be false every iteration of the loop(). Maybe those should be global?

  static boolean batteryLow = false, batteryLowPinState = false;

What do you expect it to return if it doesn't match either condition?

  if (!pinState && difference > 5000) {
    //turn it on
    digitalWrite (pin, HIGH);
    previousTime = currentTime;
    return true;
  }
  else if (pinState && difference > 1000) {
    //turn it off
    digitalWrite (pin, LOW);
    previousTime = currentTime;
    return false;
  }
  else return ?;

Maybe those should be global?

they're qualified "static"

Oops! My bad on that one! But the second is still a concern.

lloyddean: It'd help if you let us know what your expectation of this code is!

Sorry, as I said it's the end of a long day! It's just supposed to flash the pin on and off at the intervals in the comments.

robtillaart: millis() returns unsigned long. All variables used for math with milliseconds should be unsigned long too. furthermore always use subtraction as this handles the wrapping of the unsigned long after 49++days

OK, I hadn't noticed the unsigned long thing. I should have realised that given how large the number is likely to get. Thank you.

SurferTim: Oops! My bad on that one! But the second is still a concern.

Yes, another good point, I don't know what happens if you don't specify a return value explicitly, so I'll correct that.

Thank you all for the comments. I'll update with progress after incorporating them.

The promised update - the key problem was not specifying a return value if neither of the conditions was true. I added 'return pinState' to the end of the function and all was well. Thanks for the help.