Blink function

hey guys. trying to make a function for blinking an LED. running into some issues though

void blink(int ledpin){
unsigned long previousblinktime = 0;
unsigned long currentblinktime = millis();
if (currentblinktime - previousblinktime > 2000)
{
previousblinktime = currentblinktime;
if( ledstate == 0)
ledstate = 255;
else
ledstate = 0;
analogWrite(ledpin, ledstate);

}
}

as per usual i'm sure its some simple mistake but its late and i'm really rackin' my brain on this one. "ledstate" was setup (int ledstate = 0;) earlier in the script. this is just an excerpt of just the function i'm trying to define to use in a switch/case situation. thus far tho the LED stays on... but its not quite 255 on.. its got a very little bit of wobble to it. hope that can help point out tthe isue
thanks
Abraxasa

Just curious, why are you using analogWrite for this?

using digitalWrite &
pinMode(ledpin, OUTPUT);
& just write it HIGH or LOW
would seem safer, than any pin from D0 to D19 (on a '328 for example) could be used,
vs the 6 analog pins only, since you're not PWMing the pin

As is, I think you need
int ledpin = A0; or equivalent someplace it would seem.
Either way, some { }s would probably help:

    if( ledstate == 0){
      ledstate = 255;}
    else{
      ledstate = 0;}
// then do the write
      analogWrite(ledpin, ledstate);
[code]

[/code]

i'm using analogwrite because its a RGB led and i'm using analogWrite through-out the entire script for various patterns. the brackets dont seem to make any difference. ive rearranged things every which way and it still does the same thing. very frustrating

I think your logic may be a little off then.

You call the function:
unsigned long previousblinktime = 0; << this gets set to 0 every time
unsigned long currentblinktime = millis();
if (currentblinktime - previousblinktime > 2000) << this will then always be true after 2 seconds of operation
{
previousblinktime = currentblinktime;
if( ledstate == 0) << so it will change state every time thru - is that what is intended?
ledstate = 255;
else
ledstate = 0; << so it will change state every time thru - is that what is intended?
analogWrite(ledpin, ledstate);

Crossroads is right. You can't "remember" stuff with a non-static variable inside a function. Move it outside or make it static. Eg. either:

unsigned long previousblinktime = 0;

void blink(int ledpin){
  unsigned long currentblinktime = millis();
  if (currentblinktime - previousblinktime > 2000)

or:

void blink(int ledpin){
  static unsigned long previousblinktime = 0;
  unsigned long currentblinktime = millis();
  if (currentblinktime - previousblinktime > 2000)

so here is my latest code

//this is defined at the beginning of the script
int ledblinkstate = 0;

//this is down at the bottom
void blink(int ledpin){
static unsigned long previousblinktime = 0;
unsigned long currentblinktime = millis();
if (currentblinktime - previousblinktime > 2000)
{
previousblinktime = currentblinktime;
if( ledblinkstate == 0){
ledblinkstate = 255;
Serial.println("on");}
else{
ledblinkstate = 0;
analogWrite(ledpin, ledblinkstate);
Serial.println("off");}

}
}

i am now getting a toggle of "on" and "off" every two seconds on the monitor but the LED stays off.... HELP!

oooops. just realized i was missing the analogWrite for the On setting.... added that and its running great! thanks for all your help guys. on to the next pattern!

CrossRoads:
Just curious, why are you using analogWrite for this?

using digitalWrite &
pinMode(ledpin, OUTPUT);
& just write it HIGH or LOW
would seem safer, than any pin from D0 to D19 (on a '328 for example) could be used,
vs the 6 analog pins only, since you're not PWMing the pin

This is incorrect. analogWrite() does not use analog at all.
While the code may physically live inside the wiring_analog.c module it has nothing to
with analog functions or with the arduino analog pin numbering space.
It uses the digital pin numbers.
i.e. analogRead(0) and analogWrite(0, val) are not the same pin.

You can do everything with analogWrite() than you can do with digitalWrite()
plus PWM on the pins that support PWM.

0 is constant low and 255 is constant high. (no PWM)
So
analogWrite(pin, 0);
is the same as
digitalWrite(pin, LOW);

and
analogWrite(pin, 255);
is the same as
digitalWrite(pin, HIGH);

And when doing "analogWrite()" on non PWM pins,
if the value is < 128 it uses LOW as a value to digitalWrite()
otherwise it uses HIGH.

This was an unfortunate API choice in arduino.
A better name would have be to call the function pwmWrite()
or even better redefine HIGH as 255 instead of 1, totally eliminate analogWrite()
and then use digitalWrite() for both HIGH/LOW settings as well as
PWM.

--- bill