If statements debugging

Im really stuck here, this program should blink an led at two intervals. The actual times are not important just the ability to have a different on time then off time. The rough sketch is verbalized in the beginning comments.

As far as I can see the program enters the first “if” statement, sets the led pin HIGH but never enters the 2nd “if” statement.
Ive placed serial print lines in each statement to try to see what is happening, and it never meets the 2nd “if” criteria.

Advice?

//Aug 27 2014
/*Set led pin

Set pin to ouput
set a long time variabe
set a short time variable

Start loop
timestamp to create a point to compare to

If (led off) and the current time - timestamp has exceeded long time
 Then turn on led 
    reset timestamp
if (led on) and current time - timestamp has exceeded the short time 
  Then turn off led
      reset timestamp
	
repeat*/


int ledpin;
int pinstate;  
int on;
int off;
unsigned long timestamp;//this sets a time to compare to current time in the loop
unsigned long currentmillis;
void setup() //define variable statement {}
{
  ledpin = 13;
  pinMode(ledpin, OUTPUT);
  on=500;// time led on
  off=500;// time led off
  currentmillis=millis();
  pinstate=digitalRead(ledpin);
  timestamp=1;
  Serial.begin(9600);

}
void loop() 

{

if(pinstate==LOW && currentmillis-timestamp > off)
{ 
      digitalWrite(ledpin, HIGH);//turns pin 13 on
      timestamp=currentmillis;
Serial.println("ping");

}
  if(pinstate==HIGH && currentmillis-timestamp > on){
//Serial.println("ping 2");  
      digitalWrite(ledpin, LOW);//turn pin 13 off
      timestamp=currentmillis;
  }      
  

}

eureka.ino (1.12 KB)

first of all use CTRL-T before posting code, that reindents the code to make it better readable

second, currentmillis() should get an update at the start of loop()

here a state machine that does what you want.

int ledpin = 13
int pinstate = LOW
int on = 700;
int off = 300;

unsigned long timestamp = 0;
unsigned long currentmillis = 0;

void setup()
{
  pinMode(ledpin, OUTPUT);
  digitalWrite(ledpin, LOW);
}

void loop() 
{
  // get time
  currentmillis = millis();

  // switch state if needed
  switch(pinstate)
  {
  case LOW: 
    if (currentmillis - timestamp >= off)
    {
      pinstate = HIGH;
    }
  case HIGH:
    if (currentmillis  - timestamp >= on)
    {
      pinstate = LOW;
    }
  }

  // reflect pinstate in LED
  digitalWrite(ledpin, pinstate );
  timestamp = currentmillis;
}

Welcome to the Forum. It's nice to see a new person using code tags to properly post their code!

If you look closely at your code, there are a couple of things that will make your code more efficient. When I compiled your version of the program, it compiled to 2732 bytes. Since memory is scarce, always do what you can to conserve it. First, you really don't need the Serial object in your code. I'm sure it was just there as part of your debugging efforts. That will save a considerable amount of memory. Second, you don't need variables on and off. It appears you are using these to pause the LED for a half second. As such, why not use a symbolic constant for it (i.e., PAUSE) to make the code easier to read. I got rid of these two variables because you really don't need them. Third, because pinstate is used to toggle your LED, you need to make sure it changes state somewhere in the code. Fourth, you also need to update currentmillis each time you pass through loop(). Fifth, I used another symbolic constant, DEBUG, to toggle the Serial object into or out of the code. If you comment out it's line, DEBUG is not defined for the program and the compiler will not add in the code for the Serial object. This allows you to easily toggle debugging code into and out of the program without commenting it out.

The result is the following:

#define DEBUG          // Comment this out to get rid of Serial debug statements

#define PAUSE  500
const int LEDPIN = 13;
int pinstate;

unsigned long timestamp;//this sets a time to compare to current time in the loop
unsigned long currentmillis;

void setup() //define variable statement {}
{
  pinMode(LEDPIN, OUTPUT);
  pinstate = digitalRead(LEDPIN);
  timestamp = 1;
#ifdef DEBUG
  Serial.begin(9600);
#endif

}
void loop() {
  currentmillis = millis();
  if (pinstate == LOW && currentmillis - timestamp > PAUSE)
  {
    digitalWrite(LEDPIN, HIGH);//turns pin 13 on
    timestamp = currentmillis;
#ifdef DEBUG
  Serial.println("pinstate to HIGH");
#endif
    pinstate = HIGH;
  }
  if (pinstate == HIGH && currentmillis - timestamp > PAUSE) {
    //Serial.println("ping 2");
    digitalWrite(LEDPIN, LOW);//turn pin 13 off
    timestamp = currentmillis;
#ifdef DEBUG
  Serial.println("pinstate to LOW");
#endif
    pinstate = LOW;
  }
}

The code size has also dropped to 1236 bytes, mainly by doing away with the Serial object.

Do you want to make a led blink (long ‘on’, short ‘off’) continuesly ? without the use of delay() ?

I have to explain about the setup() and loop() function:
The setup() runs just once to initialize things. For example set a pin as output, and check that everything is okay. After that, the loop() function is called over and over again.

You use the variables ‘pinstate’ and ‘timestamp’ and ‘currentmillis’ in the loop() function. But they are variables, the don’t change unless you assign another value to them. When you want to know the time, you have to call millis().

It is possible to read a digital output, but it is better to use a variable to remember if the led is on or off.

The demo several things at a time shows how to make LEDs blink using the BWoD technique.

...R

Thanks to all for help, To answer a few questions, I need to get this to work within some basic functions. Specifically millis(), and "if". It must blink at a different "off" interval then "on" interval. Example on for 300ms off for 600ms.

Im not at all worried about memory right now, just result.

I really really want to understand this , so please understand my own frustration with not "getting" it. I have spent upwards of 10 hours combined banging away on the keys trying to sort this out for myself and I realize it should not be this difficult.

So, looking at the way experts code it is great, but right now is just adding to my confusion. I like Robs approach with the Case structure. I have a little Matlab experience and it make sense to me. When I paste it into a sketch and compile, it doesn't work. I played with it for a few minutes but cannot debug it..

Can anyone explain why my code doesn't enter the second "If" statement ?

Thanks. 9

You use the variables ‘pinstate’ and ‘timestamp’ and ‘currentmillis’ in the loop() function. But they are variables, the don’t change unless you assign another value to them. When you want to know the time, you have to call millis().

It must blink at a different "off" interval then "on" interval. Example on for 300ms off for 600ms.

If that's the case, then why would you give them the same value in setup()?

Can anyone explain why my code doesn't enter the second "If" statement ?

Several of us have answered that question already. In your code, the statement:

if(pinstate==LOW && currentmillis-timestamp > off)

can only be true if either pinstate or currentmillis changes as the program runs. Show us in your code how that happens.

Ok, in my variable statements I defined currentmillis=millis() and timestamp=1ms just as a starting value

on the 4th line of the first "if" statement timestamp=currentmillis

What I think this does. if the conditions of if are met, led is turned on, and the timestamp variable is reset to current time which is the millis()

(And this what I don't know, Does the loop start again at that point ? or will it look and wait till the next "if" condition will be met.)

Before that line is executed the time stamp would be = to the last stored value, and then reset to the current time each time through the loop. In essence catching up to the current time each successive pass. Because what I need to know is how long the led has been on, or off, for the condition in either "if" to be met.

Econjack,

Good point on the time on, time off. Oops. Ive changed them 20 times.

you set the pin state for 13, so why do you need to check it?

unsigned long timeStamp;
unsigned long onTime = 300UL; // on time
unsigned long totalTime = 900UL; // off time plus on time
int ledPin = 13;

void setup() 
{
  Serial.begin(9600);
}

void loop() 
{
  if (millis() - timeStamp <= onTime)
  { 
    digitalWrite(ledPin, HIGH);
  }
  else if (millis() - timeStamp < totalTime)
  {
    digitalWrite(ledPin, LOW);
  }
  else timeStamp = millis();
}

Take the code that appears at the bottom of your original loop() code and add the new lines below:

  if(pinstate==HIGH && currentmillis-timestamp > on){
//Serial.println("ping 2");  
      digitalWrite(ledpin, LOW);//turn pin 13 off
      timestamp=currentmillis;
  }      
  // ============= Add these new lines ==========
  Serial.print("currentmillis = ");
  Serial.print(currentmillis);
  Serial.print("   timestamp = ");
  Serial.print(timestamp);
  Serial.print("   pinstate = ");
  Serial.println(pinstate);

}

and then evaluate your if statements as the program runs. You might want to uncheck the Autoscroll feature of the Serial monitor.

I think the error in my logic is I was expecting the program to wait until both conditions in the "If" statement were met. So,it sees the pin is HIGH waits for the time interval, executes. Which really means I should start over. I need to take a break..

Thanks all for help

9

I hope you all like my alternative sketch. Normally the state of the led should be remembered, but when using a 'tick' of 100ms (10Hz) it is possible to do actions at a certain 'tick'. The 10Hz can be used for everything, for example another led at a different blink rate (with its own static counter).

// Blink a led with 300ms on and 600ms off
//
// With the use of a 'tick' of 100ms (10 Hz)

unsigned long previousMillis;
const int milliscount = 100; // 100 ms for 10 Hz 'tick'
const int pinLed = 13;

void setup() {
  pinMode(pinLed, OUTPUT);
  Serial.begin(9600);
  
  // set previousMillis at the end of setup()
  previousMillis = millis();
}

void loop() {
  // Do here what needs to be done in the loop().
  
  
  // Create 10Hz 'tick'
  if( millis() - previousMillis > milliscount) {
    // advance to next 'tick' moment
    previousMillis += milliscount;
    
    // 10Hz part
    // ---------
    // This part of the sketch runs at 10Hz
    // Now the fun with the led begins.
    // Make variable static, it should keep its value.
    static int ledcounter = 0;

    // ledcounter counts : 0,1,2,3,4,5,6,7,8
    // led turned on at '0' and off at '3'
    
    if (ledcounter == 0) {
      digitalWrite(pinLed, HIGH);
    } else if (ledcounter == 3) {
      digitalWrite(pinLed, LOW);
    }
    
    ledcounter++;
    if (ledcounter == 9)
      ledcounter = 0;
  }
}