potential problem in the loop

In the code the LEDS should flicker in different delay at 10,15 and 20 second, but it doesn’t and it keeps flickering at delay(123) in the loop. I have been trying to figure it out, but I can’t find a problem.

#include <Adafruit_NeoPixel.h>

#define PIN 2
#define LED_COUNT  14

const int NUMPIXELS = 14;         
const int BRIGHTNESS = 255; 
unsigned long startMillis;
unsigned long currentMillis;
const unsigned long period = 5000; 
int white;
int white1;
int white2;

Adafruit_NeoPixel strip = Adafruit_NeoPixel(LED_COUNT, PIN, NEO_GRB + NEO_KHZ800);

void setup(){
  strip.begin();
  startMillis = millis();
}

void setStrip(int g, int r, int b) {     // Set the strip to one color intensity (red)
                                        // Green is set to zero (for non-red colors, change this)      
   for (int x=0; x < NUMPIXELS; x++) {
      strip.setPixelColor(x, strip.Color(r, g, b));
   }
   strip.show();
}


void loop()
{
  currentMillis = millis();
 
        setStrip(white, white1, white2);
  if (white == 0) {
    white = 127;
  } else {
    white = 0;
  }
  if (white1 == 0) {
    white1 = 127;
  } else {
    white1= 0;
  }
  if (white2 == 0) {
    white2 = 127;
  } else {
    white2 = 0;
  }
  delay(123);
     
   if (currentMillis - startMillis >= period+period+period+period)
  {
       
        setStrip(white, white1, white2);
  if (white == 0) {
    white = 127;
  } else {
    white = 0;
  }
  if (white1 == 0) {
    white1 = 127;
  } else {
    white1= 0;
  }
  if (white2 == 0) {
    white2 = 127;
  } else {
    white2 = 0;
  }
  delay(62.5);
  }

   else if (currentMillis - startMillis >= period+period+period)
  {   
        setStrip(white, white1, white2);
  if (white == 0) {
    white = 127;
  } else {
    white = 0;
  }
  if (white1 == 0) {
    white1 = 127;
  } else {
    white1= 0;
  }
  if (white2 == 0) {
    white2 = 127;
  } else {
    white2 = 0;
  }
  delay(71);
  }

   else  (currentMillis - startMillis >= period+period);
  {   
        setStrip(white, white1, white2);
  if (white == 0) {
    white = 127;
  } else {
    white = 0;
  }
  if (white1 == 0) {
    white1 = 127;
  } else {
    white1= 0;
  }
  if (white2 == 0) {
    white2 = 127;
  } else {
    white2 = 0;
  }
  delay(83);
  }
 
 
   if (currentMillis - startMillis >= period+period+period+period+period)
   {
    startMillis = millis();
   } 

}

This:

  delay(123);

Runs unconditionally on every iteration of loop. All your other logic is broken by this.

  if (currentMillis - startMillis >= period + period + period + period + period)
  {
    startMillis = millis();
  }

This is the only time that startMillis is updated, ie every 25 seconds at least. Is that what you want ?

Why the mixture of timing using millis() and delay() ?

delay() takes an unsigned long integer as its parameter.

   delay(62.5);

What is this all about ?

wildbill:
This:

  delay(123);

Runs unconditionally on every iteration of loop. All your other logic is broken by this.

I added the bracket, and now there is an error :

sketch_jul06a:54:4: error: expected unqualified-id before ‘if’

if (currentMillis - startMillis >= period+period+period+period)

^~

sketch_jul06a:76:4: error: expected unqualified-id before ‘else’

else if (currentMillis - startMillis >= period+period+period)

^~~~

sketch_jul06a:97:4: error: expected unqualified-id before ‘else’

else if (currentMillis - startMillis >= period+period)

^~~~

sketch_jul06a:118:3: error: expected unqualified-id before ‘else’

else (currentMillis - startMillis >= period);

^~~~

sketch_jul06a:119:3: error: expected unqualified-id before ‘{’ token

{

^

sketch_jul06a:139:4: error: expected unqualified-id before ‘if’

if (currentMillis - startMillis >= period+period+period+period+period)

^~

sketch_jul06a:144:1: error: expected declaration before ‘}’ token

}

^

exit status 1
expected unqualified-id before ‘if’

I added the bracket

We can't see what you did. Please post the revised program

  1. What this semicolon doing at the end of the else?
 delay(71);
  }

   else  (currentMillis - startMillis >= period+period);
  {   
        setStrip(white, white1, white2);
period+period+period

...more commonly known as...

period * 3

etc.

  1. Why do all the timeouts contain the exact same code?
  if (white == 0) {
   white = 127;
 } else {
   white = 0;
 }
 if (white1 == 0) {
   white1 = 127;
 } else {
   white1= 0;
 }
 if (white2 == 0) {
   white2 = 127;
 } else {
   white2 = 0;
 }
  1. Why is one example of the above code in loop() without any if condition? That won't play well with the following if's. It will just override them.

  2. Why do you seem to be trying to combine millis() timing with delay()?
    Those won't play well together.
    I suspect most probably because of point 4.

Please ignore my previous message, but it still blinks at delay(123)

#include <Adafruit_NeoPixel.h>

#define PIN 2
#define LED_COUNT  14

const int NUMPIXELS = 14;         
const int BRIGHTNESS = 255; 
unsigned long startMillis;
unsigned long currentMillis;
const unsigned long period = 5000; 
int white;
int white1;
int white2;

Adafruit_NeoPixel strip = Adafruit_NeoPixel(LED_COUNT, PIN, NEO_GRB + NEO_KHZ800);

void setup(){
  strip.begin();
  startMillis = millis();
}

void setStrip(int g, int r, int b) {     // Set the strip to one color intensity (red)
                                        // Green is set to zero (for non-red colors, change this)      
   for (int x=0; x < NUMPIXELS; x++) {
      strip.setPixelColor(x, strip.Color(r, g, b));
   }
   strip.show();
}


void loop()
{
{
  currentMillis = millis();
 
        setStrip(white, white1, white2);
  if (white == 0) {
    white = 127;
  } else {
    white = 0;
  }
  if (white1 == 0) {
    white1 = 127;
  } else {
    white1= 0;
  }
  if (white2 == 0) {
    white2 = 127;
  } else {
    white2 = 0;
  }
  delay(123);
  }
     
   if (currentMillis - startMillis >= period+period+period+period)
  {
       
        setStrip(white, white1, white2);
  if (white == 0) {
    white = 127;
  } else {
    white = 0;
  }
  if (white1 == 0) {
    white1 = 127;
  } else {
    white1= 0;
  }
  if (white2 == 0) {
    white2 = 127;
  } else {
    white2 = 0;
  }
  delay(62.5);
  }

   else if (currentMillis - startMillis >= period+period+period)
  {   
        setStrip(white, white1, white2);
  if (white == 0) {
    white = 127;
  } else {
    white = 0;
  }
  if (white1 == 0) {
    white1 = 127;
  } else {
    white1= 0;
  }
  if (white2 == 0) {
    white2 = 127;
  } else {
    white2 = 0;
  }
  delay(71);
  }

   else if (currentMillis - startMillis >= period+period)
  {   
        setStrip(white, white1, white2);
  if (white == 0) {
    white = 127;
  } else {
    white = 0;
  }
  if (white1 == 0) {
    white1 = 127;
  } else {
    white1= 0;
  }
  if (white2 == 0) {
    white2 = 127;
  } else {
    white2 = 0;
  }
  delay(83);
  }
 
  else  (currentMillis - startMillis >= period);
  {   
        setStrip(white, white1, white2);
  if (white == 0) {
    white = 127;
  } else {
    white = 0;
  }
  if (white1 == 0) {
    white1 = 127;
  } else {
    white1= 0;
  }
  if (white2 == 0) {
    white2 = 127;
  } else {
    white2 = 0;
  }
  delay(100);
  }
 
   if (currentMillis - startMillis >= period+period+period+period+period)
   {
    startMillis = millis();
   } 
}

The braces you added do nothing. The delay(123) still runs every time, poisoning the rest of your logic.

wildbill:
The braces you added do nothing. The delay(123) still runs every time, poisoning the rest of your logic.

How to change that?

consider

#if 0
#include <Adafruit_NeoPixel.h>
Adafruit_NeoPixel strip = Adafruit_NeoPixel(LED_COUNT, PIN, NEO_GRB + NEO_KHZ800);
#else
enum { LED_0 = 10, LED_1, LED_2 };
byte leds [] = { LED_0, LED_1, LED_2 };
#define N_LEDS  sizeof(leds)

#define ON  LOW
#define OFF HIGH
#endif

#define PIN 2
#define LED_COUNT  14
const int NUMPIXELS = 14;
const int BRIGHTNESS = 255;
unsigned long startMillis;
unsigned long currentMillis;
const unsigned long period = 5000;
int white;
int white1;
int white2;

// -----------------------------------------------------------------------------
void setup(){
#if 0
    strip.begin();

#else
    for (unsigned n = 0; n < N_LEDS; n++)  {
        digitalWrite (leds [n], OFF);
        pinMode      (leds [n], OUTPUT);
    }
#endif

    startMillis = millis();

    Serial.begin (115200);
}

// -----------------------------------------------------------------------------
void setStrip(int g, int r, int b) {     // Set the strip to one color intensity (red)
    // Green is set to zero (for non-red colors, change this)
#if 0
    for (int x=0; x < NUMPIXELS; x++) {
        strip.setPixelColor(x, strip.Color(r, g, b));
    }

    strip.show();
#else
    digitalWrite (LED_0, g);
    digitalWrite (LED_1, r);
    digitalWrite (LED_2, b);
#endif
}

// -----------------------------------------------------------------------------
unsigned long delays [] = { 123, 62, 71, 83 };
#define N_DELAYS    (sizeof(delays)/sizeof(unsigned long))
int  idx = 0;

void loop()
{
    currentMillis = millis();
    if (currentMillis - startMillis >= period)  {
        startMillis = currentMillis;

        if (N_DELAYS <= ++idx)
            idx = 0;
        Serial.println (delays [idx]);
    }

    if (white == 0) {
        white = 127;
    } else {
        white = 0;
    }

    if (white1 == 0) {
        white1 = 127;
    } else {
        white1= 0;
    }

    if (white2 == 0) {
        white2 = 127;
    } else {
        white2 = 0;
    }

    setStrip(white, white1, white2);
    delay (delays [idx]);
}

wildbill:
The braces you added do nothing. The delay(123) still runs every time, poisoning the rest of your logic.

Not to mention all the other delays, which seem to only have been added so that the loop doesn't immediately restart and reset white/1/2 to their original values.

If you're using millis() for timing, do all your timing with millis().
If you do not want to do that, please list the intervals you are attempting to flash at and we will show you how.

pcbbc:
Not to mention all the other delays, which seem to only have been added so that the loop doesn't immediately restart and reset white/1/2 to their original values.

If you're using millis() for timing, do all your timing with millis().
If you do not want to do that, please list the intervals you are attempting to flash at and we will show you how.

I want it to flicker at delay(123) for the first 10 seconds, then at delay(83) for the next 5seconds, then delay(71) next 5 seconds, and then delay(62) next 5 seconds, and again.

At your level of knowledge, I suggest removing all millis() and use only delay().

#if 0
#include <Adafruit_NeoPixel.h>
Adafruit_NeoPixel strip = Adafruit_NeoPixel(LED_COUNT, PIN, NEO_GRB + NEO_KHZ800);
#else
enum { LED_0 = 10, LED_1, LED_2 };
byte leds [] = { LED_0, LED_1, LED_2 };
#define N_LEDS  sizeof(leds)

#define ON  LOW
#define OFF HIGH
#endif

#define PIN 2
#define LED_COUNT  14
const int NUMPIXELS = 14;
const int BRIGHTNESS = 255;
unsigned long startMillis;
unsigned long currentMillis;
unsigned long flickerMillis;
unsigned long flickerPeriod;

const unsigned long period = 5000;
int white;
int white1;
int white2;

// -----------------------------------------------------------------------------
void setup(){
#if 0
    strip.begin();

#else
    for (unsigned n = 0; n < N_LEDS; n++)  {
        digitalWrite (leds [n], OFF);
        pinMode      (leds [n], OUTPUT);
    }
#endif

    flickerMillis = startMillis = millis();

    Serial.begin (115200);
}

// -----------------------------------------------------------------------------
void setStrip(int g, int r, int b) {     // Set the strip to one color intensity (red)
    // Green is set to zero (for non-red colors, change this)
#if 0
    for (int x=0; x < NUMPIXELS; x++) {
        strip.setPixelColor(x, strip.Color(r, g, b));
    }

    strip.show();
#else
    digitalWrite (LED_0, g);
    digitalWrite (LED_1, r);
    digitalWrite (LED_2, b);
#endif
}

// -----------------------------------------------------------------------------
unsigned long delays [] = { 123, 123, 83, 71, 62 };
#define N_DELAYS    (sizeof(delays)/sizeof(unsigned long))
int  idx = 0;

void loop()
{
    currentMillis = millis();
    if (currentMillis - startMillis >= period)  {
        startMillis = currentMillis;

        if (N_DELAYS <= ++idx) idx = 0;
    }

    if (currentMillis - flickerMillis >= flickerPeriod)  {
        setStrip(white, white1, white2);
        white ^= 127;
        white1 ^= 127;
        white2 ^= 127;
        flickerMillis  = currentMillis;
        flickerPeriod = delays [idx];
    }
}

aarg:
At your level of knowledge, I suggest removing all millis() and use only delay().

but I use delay only to determine the frequency of the LEDS, and this frequency changes 10 seconds and then every 5 seconds, and then the loop repeats.

tobiwan123:
but I use delay only to determine the frequency of the LEDS, and this frequency changes 10 seconds and then every 5 seconds, and then the loop repeats.

Well, you can delay by a variable, e.g. 'delay(intervalValue);' if you need to change it while running.

pcbbc:

#if 0

#include <Adafruit_NeoPixel.h>
Adafruit_NeoPixel strip = Adafruit_NeoPixel(LED_COUNT, PIN, NEO_GRB + NEO_KHZ800);
#else
enum { LED_0 = 10, LED_1, LED_2 };
byte leds = { LED_0, LED_1, LED_2 };
#define N_LEDS  sizeof(leds)

#define ON  LOW
#define OFF HIGH
#endif

#define PIN 2
#define LED_COUNT  14
const int NUMPIXELS = 14;
const int BRIGHTNESS = 255;
unsigned long startMillis;
unsigned long currentMillis;
unsigned long flickerMillis;
unsigned long flickerPeriod;

const unsigned long period = 5000;
int white;
int white1;
int white2;

// -----------------------------------------------------------------------------
void setup(){
#if 0
    strip.begin();

#else
    for (unsigned n = 0; n < N_LEDS; n++)  {
        digitalWrite (leds [n], OFF);
        pinMode      (leds [n], OUTPUT);
    }
#endif

flickerMillis = startMillis = millis();

Serial.begin (115200);
}

// -----------------------------------------------------------------------------
void setStrip(int g, int r, int b) {    // Set the strip to one color intensity (red)
    // Green is set to zero (for non-red colors, change this)
#if 0
    for (int x=0; x < NUMPIXELS; x++) {
        strip.setPixelColor(x, strip.Color(r, g, b));
    }

strip.show();
#else
    digitalWrite (LED_0, g);
    digitalWrite (LED_1, r);
    digitalWrite (LED_2, b);
#endif
}

// -----------------------------------------------------------------------------
unsigned long delays = { 123, 123, 83, 71, 62 };
#define N_DELAYS    (sizeof(delays)/sizeof(unsigned long))
int  idx = 0;

void loop()
{
    currentMillis = millis();
    if (currentMillis - startMillis >= period)  {
        startMillis = currentMillis;

if (N_DELAYS <= ++idx) idx = 0;
    }

if (currentMillis - flickerMillis >= flickerPeriod)  {
        setStrip(white, white1, white2);
        white ^= 127;
        white1 ^= 127;
        white2 ^= 127;
        flickerMillis  = currentMillis;
        flickerPeriod = delays [idx];
    }
}

exit status 1
‘Serial’ was not declared in this scope

tobiwan123:
exit status 1
'Serial' was not declared in this scope

I was just adjusting gcjr code.

We've no idea what Arduino board you are developing for because you don't tell us. I suppose one without a serial implementation?

Remove the Serial lines, you don't need them. I would have thought that was obvious.