Counter in seconds when pin (led) is high

Hello, i am trying to make a counter in seconds. When led is HIGH the counter needs to start. It need to remember the total time in seconds of the led being HIGH. Right now it is just counting and not only counting the time the led was high. Does anyone know the problem? Right i have coded the led to blink, this to test if it only counts up when the pin is high (led is on).

int led = 7; //Define the LED pin
int seconds; // needed for setting the milliseconds to seconds
unsigned long startTime; //Start Time
unsigned long currentTime; //End Time
unsigned long stopTime; //Difference of Times
unsigned long elapsedTime; //Difference of Times

void setup()
{
  pinMode(led,OUTPUT);
  Serial.begin(9600);
}

void loop()
{
  digitalWrite(led, HIGH);
  delay(1000);
  digitalWrite(led, LOW);
  delay(1000);
   
  
  if(led==HIGH) // if led is high, start timer
  {
    startTime = millis();
  }
  
  if(led==LOW) // if led is off, stop timer
  {
    stopTime = millis();
  }
 
  {
    currentTime - millis();
  }
  
  elapsedTime = currentTime - startTime + seconds; // after the pin is low, the new added time needs to add up to seconds it has already been on in total.
  seconds = millis()/1000; // for setting the milliseconds to seconds
  Serial.println(seconds);
  delay(100);
}

if(digitalRead(led)==HIGH) // if led is high, start timer


When a Flag is reset and your LED is turned ON, set the Flag and record the time.

When the Flag is set and your LED goes OFF, reset the Flag and calculate the time it was ON

This does nothing. Did you want something to happen here?

You are turning the LED on for one second. Why not just say "seconds = seconds + 1;" here?

What do you mean by?:

if(led==HIGH)

led is a pin number, see usage in:

pinMode(led,OUTPUT);
pinMode(led,OUTPUT);

led has a fix value, here 0x7 (the pin number of led). It will never match with ==HIGH or ==LOW
(assuming HIGH is 1 and LOW is 0).

You had to remember the state how you have set the led. The led itself is not an object which you can query for its state (just a pin number).

Therefore. neither of your if(led ==) is matching. And startTime remains 0 all the time.
And seconds is not set for the first loop(), used as 0.

seconds becomes always and just millis()/1000; - the current time divided by 1000.,
There is nothing what waits really, except the delay(1000); but this code is never executed during your if(led==...) is always false.

And:

currentTime - millis();

does not do anything! It is a subtraction of two values where you never assign the result to any variable (operation result is lost) - No compiler warning???

You can do this (toggling the LED for 1 sec):

int led = 7; //Define the LED pin

void loop()
{
  digitalWrite(led, HIGH);
  delay(1000);
  digitalWrite(led, LOW);
  delay(1000);
}

it should toggle the led with 1 sec on and off (0.5 Hz period).

You can do also a bit more complicated:

void loop()
{
    unsigned long currentMS;                    //remember when we have changed the led state
    unsigned long ms;                           //helper variable
    unsigned long elapsed;                       //helper (temporary) variable
    int ledState = 0;                          //our state of led

    if ( ! ledState) {                          //ledState toggles between 0 and 0xFFFFFFFF (not 0)
        digitalWrite(led, LOW);
        currentMS = millis();
    }
    else {
        digitalWrite(led, HIGH);
        currentMS = millis();
    }
    //one or the other is executed - remember the time when we have changed

    ms = millis();                    //get the milli-sec. time stamp
    if (ms > currentMS)                //handle an overflow on millis() - realize when it has wrapped!
          elapsed = ms - currentMS;
    else
         elapsed = (0xFFFFFFFF - currentMS) + ms;    //it has wrapped
    //think about if needed: how to handle situation that millis() wraps around in between?

    if (elapsed >= 1000)              //1000 ms is 1 sec - change state every 1 sec
         ledState ~= ledState;       //invert the state and keep going (0 or 0xFFFFFFFF)
}

BTW: never do here in this example:

if (elapsed == 1000)

What, if you are 1 millisec behind? This "perfect match" will not hit anymore.
So, think about what happens with "noise", with "time slips" ... Can a comparison for an exact match work all the time? (NO!)

Upps - my own example wrong!:
We do all the time one of the ledState again. And we udpate all the time currentMS - cannot work.

So, correct it into something like this:

void loop()
{
    unsigned long currentMS = millis();   //remember when we have changed the led state
    unsigned long ms;                           //helper variable
    unsigned long elapsed;                       //helper (temporary) variable
    int ledState = 0;                          //our state of led

    if ( ! ledState) {                          //ledState toggles between 0 and 0xFFFFFFFF (not 0)
        digitalWrite(led, LOW);
    }
    else {
        digitalWrite(led, HIGH);
    }
    //one or the other is executed - remember the time when we have changed

    ms = millis();                    //get the milli-sec. time stamp
    if (ms > currentMS)                //handle an overflow on millis() - realize when it has wrapped!
          elapsed = ms - currentMS;
    else
         elapsed = (0xFFFFFFFF - currentMS) + ms;    //it has wrapped
    //think about if needed: how to handle situation that millis() wraps around in between?

    if (elapsed >= 1000) {             //1000 ms is 1 sec - change state every 1 sec
         ledState ~= ledState;       //invert the state and keep going (0 or 0xFFFFFFFF)
         currentMS = millis();
    }
}

Upps - this happens when I do try to compile my provided code samples:

The operator '~' is a uni operator : it has to be:

ledState = ~ledState;

it should invert the ledState (from 0 to 0xFFFFFFFF). But this "bi-directional" was stupid.
An operation as:

ledState = ledState ~ ledState;
//which is the same as: ledState ~= ledState;

does not exist! Sorry.

You should code this: if(digitalRead(led)==HIGH) // if led is high, start timer

Suspect the OP is AWOL. :grin:

Seriously needs to reply and explain properly what is the intention of this project.

Yes, correct and reasonable:
It would read back what you have written to OUTPUT buffer (for led), in order to set led high or low. Makes sense and should work.

Just to mention: my example CANNOT work, still!:
the variables currentMS and ledState would not "survive".
This function loop() is called from outside, in a loop, but the call of loop() is a regular function call: it would initialize all local variables again. But we need these two variables stored even the function returns and is called again (next call has to reuse the left values there).

So, you need statement "static" in front of these two variables:

static unsigned long currentMS = millis();
static int ledState = 0;

But as albert_wu1234 has mentioned: instead of using ledState - you can read back how led was set (as high or low). You can use the GPIO output register instead. No need to remember as long you can read back what the led state is.

This is all speculation and frankly, getting nowhere.

Still awaiting some sense from the OP. :roll_eyes:

I think this is a school assignment and the deadline has passed.

Distinct possibility, given the usual approach to assignments. :rofl:

Do you suppose the answer was found? :thinking:

Hello, sorry i was on vacation! What i am trying to make a simple counter that starts when the led is HIGH. If the led is LOW the counter pauzes, but when the led gets HIGH again the counter continues counting (so it needs to add up the total time of the LED being high). Thats why i also added the blinking of the led, so i can check if the time only adds up when the led is HIGH.
I made a new code but it still won't work properly.
Anyone who can help? I used to work with Arduino in school, and i am trying to expand my knowledge.

  int led = 7;
  unsigned long previousTime = 0;

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

  }

  void loop() {

  digitalWrite(led, HIGH);
  delay(1000);
  digitalWrite(led, LOW);
  delay(1000);
   
  
 if (digitalRead(led)==HIGH)
 {
 previousTime = millis();

 Serial.println(previousTime);
 
 }  
 else if (digitalRead(led)==LOW)
 {
 Serial.println("led pin is low"); 
 }

 }

That will never happen, since just a second before that ask you set it low.

So it should work if i manually set the pin LOW or HIGH without the delays?

What does that mean?.... "manually set the pin"

edit: It makes me think you might be thinking of a button on the led pin as well as the led, to take the pin to 5V? But not sure what you mean, hence my asking.

digitalWrite(led, VALUE) is a write to a MCU register which sets the GPIO output to VALUE. This VALUE comes out on the pin.
You can read back what you have written to this GPIO output register: digitalRead(led) will do.
led is just the GPIO pin you want to handle.

"manually set the pin" means: do this digitalWrite(led, VALUE) which is a write to GPIO output register and you set the VALUE coming out of the related pin.

No, no button there: you can read back what was written into GPIO output register. It tells you what the GPIO tries to drive (a low or high). But it does not mean you read the GPIO as an input (it is an output here). If you have a short outside, the pin is grounded - you still read the GPIO output register, not the value from the pin input (just if it would be configured as input you would do so).

Here, how it works:

  • GPIO pin configured as output: write to output register and the value should come out on pin;
    read this output GPIO and you get the Output Register value back (not the real signal level on pin, e.g. it is shortened - Output register has still what it should be, not what it is really)

  • GPIO pin is configured as input: you can still write to GPIO output register - but it does not come out (it is an input, the output register is disconnected from pin); when you read this GPIO pin - you get now the real level seen on pin (the input from pin is connected, instead of reading the output register back)

The GPIO pin configuration has a switch inside what reading should provide: the register value (when output) or the real level on pin (when input).

For an output - always possible to read back how the pin should be driven, but not reading the pin level like an input (exception: when it is an Open Drain output).

If an output - you write AND read the Output register. If input - you read the level of the pin but you can still write to Output register (without any visible effects: disconnected from pin when input but MCU can still write to this register).

OP's already doing a digitalWrite(), was doing that in the opening post a month ago, so they know how to do a digitalWrite(). Yet, they asked only a day ago about "manually setting the pin" which is why I asked what that meant, since it seemed to me that they meant two different things. You @tjaekel are answering my question on the OP's behalf; now, you may be right that they just mean doing a digitalWrite() when they say "manually setting the pin" but it's up to them to answer....

(I'm not sure if the whole previous post #18 was aimed at me or at the OP? I know all that, thanks.)

This topic was automatically closed 180 days after the last reply. New replies are no longer allowed.