How to make my code run in a circular fashion?

Hello

I’ve built a simple system for two LED’s flashing on and off independently from one another, here’s the code:

// Which pins are connected to which LED
const byte greenLED = 9;
const byte redLED = 8;

// Time periods of blinks in milliseconds.
const unsigned long greenLEDinterval = 1002;
const unsigned long redLEDinterval = 1000;

// Variable holding the timer value so far. One for each "Timer"
unsigned long greenLEDtimer;
unsigned long redLEDtimer;

void setup () 
  {
  pinMode (greenLED, OUTPUT);
  pinMode (redLED, OUTPUT);
  greenLEDtimer = millis ();
  redLEDtimer = millis ();
  }  // end of setup

void toggleGreenLED ()
  {
   if (digitalRead (greenLED) == LOW)
      digitalWrite (greenLED, HIGH);
   else
      digitalWrite (greenLED, LOW);

  // remember when we toggled it
  greenLEDtimer = millis ();  
  }  // end of toggleGreenLED

void toggleRedLED ()
  {
   if (digitalRead (redLED) == LOW)
      digitalWrite (redLED, HIGH);
   else
      digitalWrite (redLED, LOW);

  // remember when we toggled it
  redLEDtimer = millis ();  
  }  // end of toggleRedLED

void loop ()
  {
  // Handling the blink of one LED.
  if ( (millis () - greenLEDtimer) >= greenLEDinterval)
     toggleGreenLED ();
  // The other LED. 
  if ( (millis () - redLEDtimer) >= redLEDinterval) 
    toggleRedLED ();

}  // end of loop

And this works – the LEDs start off switching on and off in unison and slowly the 2 millisecond difference adds up and becomes noticeable until the LEDs are completely desynchronised and flashing on and off one after the another. Then at this point the loop abruptly begins at the beginning again with the LEDs flashing together in unison.

What I want is for the loop not to abruptly begin again - I want the 2 milliseconds to continue to add up so that the LEDs slowly resynchronise with one another again. Basically I want the code to be circular and the code I have at the minute is only half a circle.

Are there any suggestions about how I can make the code more circular?

Thanks in advance, Josy

Are there any suggestions about how I can make the code more circular?

Round file it? 8)

millis() is not a timer. It is a clock. greenLEDtimer isn't a timer. It is the time (no R) that an event occurred.

void toggleGreenLED ()
  {
   if (digitalRead (greenLED) == LOW)
      digitalWrite (greenLED, HIGH);
   else
      digitalWrite (greenLED, LOW);

  // remember when we toggled it
  greenLEDtimer = millis ();  
  }  // end of toggleGreenLED

Could be a lot shorter:

void toggleGreenLED ()
  {
     digitalWrite (greenLED, !digitalRead (greenLED));

  // remember when we toggled it
  greenLEDtimer = millis ();  
  }  // end of toggleGreenLED

Psst: Your indenting sucks. Tools + Auto Format is just dying to fix it. Let it.

Then at this point the loop abruptly begins at the beginning again with the LEDs flashing together in unison.

Loop() executes many millions of times before this happens. You need to carefully observe just what is happening. Start with adding a Serial.print() statement to setup(). Make sure that setup() is not getting called again.

Hi,

thank you PaulS - after auto formating and shortening the code it now works. Now it's cyclically running!

I don't know why it works but it does so - thank you very much!

Josy

That's not a very convincing explanation . Exactly how does re-formatting the code or optimising an expression solve the "problem" here ?

   if (digitalRead (greenLED) == LOW)  // NO, don't do this!
      digitalWrite (greenLED, HIGH);
   else
      digitalWrite (greenLED, LOW);

Your LEDs inverted unexpectedly because you called digitalRead() to find the last state you wrote to the pin, and some glitch returned the wrong value (you do have resistors in series with those LEDs? Since its an output it should be hard to force the voltage to read wrong, but somehow that's what happened)

You should always keep state in variables rather than trust to the pin to remember it, so that you aren't vulnerable to noise on the pin from outside influences.

The way you try to keep time is flawed. You perhaps don't realise that millis() sometimes increases by 2 rather than 1, and that it could roll over between your toggle-test and the line where you call millis() again to set the LEDtimer variable.

Again keep your time state in variables and don't read it from millis(), so your code simply increments the LEDtimer variables by the delay, and the whole code becomes:

// Which pins are connected to which LED
#define greenLED 9
#define redLED   8

// Time periods of blinks in milliseconds.
#define greenLEDinterval 1002L
#define redLEDinterval   1000L

// Variable holding the timer value so far. One for each "Timer"
unsigned long greenLEDtimer;
unsigned long redLEDtimer;

void setup () 
{
  pinMode (greenLED, OUTPUT);
  pinMode (redLED, OUTPUT);
  greenLEDtimer = redLEDtimer = 0 ;
}

boolean green_on = false ;
boolean red_on = false ;

void toggleGreenLED ()
{
  green_on = !green_on ;
  digitalWrite (greenLED, green_on) ;
}

void toggleRedLED ()
{
  red_on = !red_on ;
  digitalWrite (redLED, red_on) ;
}


void loop ()
{
  unsigned long now = millis () ;
  if (now - greenLEDtimer >= greenLEDinterval)
  {
    toggleGreenLED ();
    greenLEDtimer += greenLEDinterval ;
  }
  if (now - redLEDtimer >= redLEDinterval)
  {
    toggleRedLED ();
    redLEDtimer += redLEDinterval ;
  }
}

Hint, if you change greenLEDinterval from 1002 to 1050 you'll see that the code's working in a few tens of seconds rather than 10 minutes of waiting!!

[ also thinking about it a little more, I think your previous run simply reset halfway through (you are not running on Windows are you? There are some windows system where the USB serial lines get probed regularly by background tasks, causing an Arduino reset). ]