Blink LED at intervals defined by array - no delay

Can anyone point out the mistakes I’m making in this sketch please?

const byte grnLed = 5;
const byte button = 9;
unsigned long greenTime_now = 0;
unsigned long intervalTime_now = 0;
const int flashLength = 50;
int flashInterval [] = {500, 1800, 3900, 5200, 5600, 9600};
int counter = 0;
int trigger = 0;

void setup()
{
  Serial.begin (9600);
  pinMode (grnLed, OUTPUT);
  pinMode (button, INPUT_PULLUP);
  digitalWrite (grnLed, LOW);


}
void loop()
{
  trigger = digitalRead (button);
  if (!trigger)
  {
    intervalTime_now = millis();
    {
      for (counter = 0; counter < 6; counter++)
      {
//       Serial.println (counter);
//       Serial.println (flashInterval[counter]);
//       Serial.println (intervalTime_now);
        if (millis() - intervalTime_now > flashInterval[counter])
        {
          digitalWrite (grnLed, HIGH);
          greenTime_now = millis();
        }
        if (millis() - greenTime_now > flashLength)
        {
          analogWrite (grnLed, 10);
          greenTime_now = millis();
        }
      }
    }
  }
  intervalTime_now = millis();
}

Basically, I’m attempting to turn an led on fully for 50ms and then dim to an analog value of 10 for the intervals defined in the array
flashInterval, i.e. off for 500 ms, then fully on for 50ms then reduce to analog 10 for another 1300ms (giving a second array entry of 1300 + 500 = 1800ms) then fully on for 50ms then reduce to analog 10 for 2100 (1800 + 2100 = 3900) etc.

The code above simply turns the led on immediately and it remains on what seems to be analog 10 after the switch is pressed. This sketch is simply a demonstration sketch to illustrate my problem and the switch has only been added to trigger this piece of code, which will eventually be in a larger sketch and be triggered by another event.

I think that that its down to where I have the " intervalTime_now = millis(); " but I get the same result wherever I place it.

You can't use a for loop in this way - you could try a state machine

Ah, ok, in that case I'll have another go using a state machine!

Thanks AWOL.

You only want to increase the counter variable when an interval is passed.

  if (!trigger)
  {
    intervalTime_now = millis();
    {
      for (counter = 0; counter < 6; )
      {
        //       Serial.println (counter);
        //       Serial.println (flashInterval[counter]);
        //       Serial.println (intervalTime_now);
        if (millis() - intervalTime_now > flashInterval[counter])
        {
          counter++;    // move to next interval
          digitalWrite (grnLed, HIGH);
          greenTime_now = millis();
        }
        if (millis() - greenTime_now > flashLength)
        {
          analogWrite (grnLed, 10);
          Serial.println("10");
          greenTime_now = millis();
        }
      }
    }
  }

But I’m not convinced the LED will be HIGH long enough to see a difference in the brightness.

But I'm not convinced the LED will be HIGH long enough to see a difference in the brightness.

@Blue Eyes - yes, the brightness change is long enough to see, its just an attention grabbing flash.

Thanks for the code modification, that almost works. The only problem with it is that the sequence ends with an unexplained switching on of the led at full brightness as if the final flash timing was ignored. I can deal with that, but was wondering why it happened that way.

@AWOL - still looking at state machines. Am I on the right lines if I double the size of the array by making every other element include the led on interval? I.e. the array I'm using would become {500, 550, 1800, 1850, 3900, 3950, 5200, 5250, 5600, 5650, 9600, 9650} and I keep checking the time passed to change the led state at each element?

There are a couple big problems in this sketch.

AWOL already mentioned the for loop. The reason this is bad is because it is a blocking construct. There is literally no point to going through all the trouble of using nonblocking timing just to wrap the whole thing in a for loop. Program execution is trapped in the for loop until the entire sequence is finished, blocking you from executing other code while the sequence is going on. If it's just a construct for the test sketch then it's fine, but you should mention that if it is because it will get attention.

Storing accumulated values in your array. You should never have to perform any arithmetic to define your constants; that's the kind of thing computers are good at. A good programmer is a lazy programmer that makes the computer do as much work for them as possible. The problem with storing accumulated values in the array like that is that if you change one interval, you have to also alter every value after that to take into account the new sum. There's a very real potential for you to either forget that or make a mistake if you have to do it manually. Even if it technically works, it's bad form because it makes things much harder for you than necessary.

Have your array contain just the length of each interval, and refresh the value of intervalTime_now after the timer has expired. Then you don't have to add anything up.

Here’s a way you could do it using a state machine:

#define BTN_READ_INTERVAL       50      //mS between button reads
#define DEFAULT_LED_ONTIME      50      //mS LED on-time
#define NUM_OFF_TIMES           6       //table size
#define MIN_LED_PWM             10      //10/255 PWM for "off"
#define MAX_LED_PWM             255     //value for LED on when PWMing    

//pin definitions
const byte pinGreenLED = 5;
const byte pinButton = 9;

//LED off time table
unsigned long flashInterval[NUM_OFF_TIMES] = {500, 1800, 3900, 5200, 5600, 9600};    //values in mS

//leeps track of the last button state
byte
    lastButton;
    
void setup()
{
    Serial.begin (9600);
    pinMode (pinButton, INPUT_PULLUP);
    lastButton = digitalRead( pinButton );  //establish the "last" state of the button input
    
    pinMode (pinGreenLED, OUTPUT);          
    digitalWrite( pinGreenLED, LOW );       //turn off the LED

}//setup

//ReadButton
// - returns false if button reading has not changed or if not time to read yet
bool ReadButton( byte *button )
{
    static unsigned long
        timeButton = 0;
    unsigned long
        timeNow;
    byte
        currButton;

    //read the button every 50mS
    timeNow = millis();
    if( timeNow - timeButton < BTN_READ_INTERVAL )
        return false;   //if not time yet, just return false

    timeButton = timeNow;

    //read the button input
    currButton = digitalRead( pinButton );
    //if the same as last, return false
    if( currButton == lastButton )
        return false;
        
    //record this as the last
    lastButton = currButton;
    //and store the value read
    *button = currButton;

    //return true to indicate a valid button press
    return true;
        
}//ReadButton

//names of LED states
#define ST_LED_HOLDINACTIVE         0       //when button shows released (not pressed)
#define ST_LED_ON                   1       //when button pressed; on state
#define ST_LED_OFF                  2       //when button pressed; off state
//
void LEDStateMachine( void )
{
    byte
        buttonState;
    static byte
        OffTimeTableIndex = 0;
    static byte
        stateLED = ST_LED_HOLDINACTIVE;
    static unsigned long
        timeLEDDelay,
        timeLED = 0;
    unsigned long
        timeNow;

    //record the current millis() count every time in
    timeNow = millis();

    //if ReadButton returns true, the state of the button changed...
    if( ReadButton( &buttonState ) )
    {
        //if it was released, turn off the LED and enter "hold" state where nothing happens
        if( buttonState == HIGH )
        {
            //button has been released; LED off and enter "holding state" where nothing happens with it
            digitalWrite( pinGreenLED, LOW );
            stateLED = ST_LED_HOLDINACTIVE;
            
        }//if
        else
        {
            //button was pressed; reset the LED variables and begin cycling it in the state machine
            timeLED = timeNow;
            timeLEDDelay = 0;
            OffTimeTableIndex = 0;
            stateLED = ST_LED_OFF;
            
        }//else
        
    }//if

    //start of LED state machine
    switch( stateLED )
    {
        case    ST_LED_HOLDINACTIVE:
            //do nothing in this state (button is released, LED is off...)
            
        break;
        
        case    ST_LED_ON:
            //when 50mS on-time has elapsed...
            if( (timeNow - timeLED) <= DEFAULT_LED_ONTIME )
                return;

            //get the off time from the table
            timeLEDDelay = flashInterval[OffTimeTableIndex];
            analogWrite( pinGreenLED, MIN_LED_PWM );            //set PWM to low value
            timeLED = timeNow;                                  //set the LED time to "now"
            stateLED = ST_LED_OFF;                              //next state is off-timing state

            OffTimeTableIndex++;                                //bump the off-table index
            if( OffTimeTableIndex == NUM_OFF_TIMES )            //if we run off the end of the table, cycle back to the start
                OffTimeTableIndex = 0;
                             
        break;

        case    ST_LED_OFF:
            //if LED off time has elapsed...
            if( (timeNow - timeLED) <= timeLEDDelay )
                return;

            //turn it back on
            timeLED = timeNow;                                  //set LED time to "now"
            analogWrite( pinGreenLED, MAX_LED_PWM );            //brighten the LED
            stateLED = ST_LED_ON;                               //and move to time the on-state
            
        break;                    
        
    }//switch
    
}//LEDStateMchine

void loop()
{
    //run the state machine every pass though loop
    LEDStateMachine();

    //can do other stuff here...
    
}//loop

Whoa, that certainly works Blackfin, many thanks :slight_smile: . Now I just need to get my head around why and how! State machines is a subject I've always meant to understand, now I have the opportunity to study one!

I'll also need to work out how to convert this complete sketch into a function that can be called to perform just once through the array whenever demanded by a button press using the existing button library JC_Button I'm using in my final sketch, but I suspect I'm going to need some time to digest the state machine methodology first.

**EDIT....**Something odd Blackfin. I've just realised that, using your sketch, I'm getting an additional led flash immediately the button is pressed, prior to the first delay in the array, i.e. there are seven flashes for an array size of six entries. Any ideas as to why this could be?

STDummy:
Whoa, that certainly works Blackfin, many thanks :slight_smile: . Now I just need to get my head around why and how! State machines is a subject I've always meant to understand, now I have the opportunity to study one!

I'll also need to work out how to convert this complete sketch into a function that can be called to perform just once through the array whenever demanded by a button press using the existing button library JC_Button I'm using in my final sketch, but I suspect I'm going to need some time to digest the state machine methodology first.

**EDIT....**Something odd Blackfin. I've just realised that, using your sketch, I'm getting an additional led flash immediately the button is pressed, prior to the first delay in the array, i.e. there are seven flashes for an array size of six entries. Any ideas as to why this could be?

The way the code is written right now is supposed to be when the button is released the delay index is reset to '0' and the statemachine enters a "do nothing" state. When the button is detected as being pressed the LED is immediately turned on for 50mS, then dimmed and the first delay applied. Is that what you're seeing?

The way the code is written right now is supposed to be when the button is released the delay index is reset to '0' and the statemachine enters a "do nothing" state. When the button is detected as being pressed the LED is immediately turned on for 50mS, then dimmed and the first delay applied. Is that what you're seeing?

@ Blackfin
Yes that is exactly what I'm seeing, so the code is in fact actually working perfectly as you wrote it. The only thing is, this sketch will actually become a function within a larger sketch and at the point where it is called by the button press, the led is already on at the analogWrite 10 level. The intention is, when the button is pressed, the led stays at that level for the first array interval (500ms), then flashes at full brightness for 50ms then back to analogWrite 10 level for the second array interval and so forth. This is to actually synchronize the led flashes with an audio soundtrack which is initiated by the same button press.

However, the sound track starts with 500ms of silence, so rather than changing your code, as it works so well, what I can do is to adjust the start of the sound track so that the initial 500ms of silence on it is removed (its not essential) so then the first sound pulse will be immediate (to coincide with the first coded flash) and then I can remove that first 500ms entry from the array.

I need to study your code for a while longer to get my head around the code logic. I'm going to have to understand it fully in order to integrate it as a function within my complete sketch in a way that it can be called to cycle through the array just once for each button press. I'll also need to decide whether to use your pushbutton method ( in which case I'll adopt it for all the other different pushbuttons I use in the main sketch) or to convert the button press method to the method I currently use (JC_Button)

Many thanks again for your help with this Blackfin :slight_smile:

OK, so I decided I really needed to work this one through myself, so I've been looking at state machines as well as revisiting BWD and I've finally come up with something that actually works :slight_smile:

Its probably not the most elegant bit of code you'll see today, but its a working framework for designing the function I want to add to an existing sketch.

For anyone interested, here's what I've done:

const byte grnLed =  5;                          // LED pin
const byte button = 9;                           // pushbutton switch pin
int ledState = 10;                               // ledState used to set the LED
long flashLength = 50;                           // LED on-time (ms)
long flashInterval[] = {50,50, 50, 1000, 1300, 600}; // LED off-time (ms) (dummy intervals now for testing)
byte counter = 0;                                // variable to step through the array
bool trigger = true;                             // boolean to store switch status
unsigned long previousMillis = 0;                // will remember last time LED was updated

void setup()
{
  pinMode(grnLed, OUTPUT);                       // set the LED pin as output
  pinMode (button, INPUT_PULLUP);                // set the button pin as input
  analogWrite (grnLed, 10);                      // turn the LED on at low level
}

void loop()
{
  trigger = digitalRead (button);                 // get switch status
  if (!trigger)                                   // if switch pressed, flash the led as per flashInterval array:
  {
    unsigned long currentMillis = millis();         // look at the time
    if ((ledState == 255) && (currentMillis - previousMillis >= flashLength)) // if it's time to turn the led off:
    {
      ledState = 10;                               // Turn it off
      previousMillis = currentMillis;              // Remember the time
      analogWrite(grnLed, ledState);               // Update the LED
      counter++;                                   // increment the variable to get the next offTime value
      if (counter >= 6)                            // if the end of the flashInterval array is reached: 
        counter = 0;                               // set the counter to the array start again
    }
    else if ((ledState == 10) && (currentMillis - previousMillis >= flashInterval[counter])) // if it's time to turn the led on,
    {
      ledState = 255;                              // turn it on
      analogWrite(grnLed, ledState);               // update the LED
      previousMillis = currentMillis;              // remember the time
    }
  }
  else if (trigger)                               // if switch released:
  {
  analogWrite(grnLed, 10);                        // default the LED to low level and
  counter = 0;                                    // set the counter to the array ready to start again
  }
}