Making a tester/controller for addressable LEDs - problem with if else statement

I am trying to make a tester (using an Arduino Nano) for addressable LED products that I sometimes buy. I would like the tester to be able to put out four different animations; a rainbow one and a solid colour one both with two variations; GRB and RGBW (for LEDs which have a fourth, white LED). I'm happy with the animations (I got the rainbow ones from the examples given by Adafruit and modified them slightly). The problem is using switches to change between them. For some reason, regardless what the switches are set to, I get two patterns when I only want one.

The part of the code I think is relevant is the else if statements in the loop function. Does it look right to you?

void loop() {
  NUM_RAINBOW_LEDS = map(analogRead(potentiometerPin), 0, 1023, 1, 60);

  patternSwitchState = digitalRead(patternSwitchPin);

  if (patternSwitchState == 1) { // for debugging LED - remove later
    digitalWrite(5, HIGH);
  }
  else {
    digitalWrite(5, LOW);
  }

  if ((patternSwitchState == 0) && (whiteSwitchState == 0)) {
    solidColoursRGB();
  } else if ((patternSwitchState == 1) && (whiteSwitchState == 0)) {
    rainbowCycle(2);
  } else if ((patternSwitchState == 0) && (whiteSwitchState == 1)) {
    solidColoursRGBW();
  } else if ((patternSwitchState == 1) && (whiteSwitchState == 1)); {
    whiteOverRainbow(20, 75, (NUM_RAINBOW_LEDS / 8 + 1));
  }
}

According to my understanding, only one animation should be possible and is selected by the setting of the patternSwitch and the whiteSwitch but like I said, it's alternating between two animations, whiteOverRainbow always being the second one. The entirity of the code is here: #include <Adafruit_NeoPixel.h>#ifdef __AVR__#include <avr/power.h>#endif - Pastebin.com

Thanks for any help.

Please post a complete sketch that illustrates the problem

How are the switches wired and what pinMode() did you use for them ?

The code in its entirety is as follows:

#include <Adafruit_NeoPixel.h>
#ifdef __AVR__
#include <avr/power.h>
#endif

#define DATA_PIN 19
#define NUM_LEDS 300
int NUM_RAINBOW_LEDS = 300;
const int potentiometerPin = 15;
const int potentiometerPinPlus1 = potentiometerPin + 1;
const int potentiometerPinMinus1 = potentiometerPin - 1;
const int patternSwitchPin = 4;
const int whiteSwitchPin = 3;
const int protocolSwitchPin = 2;

// Parameter 1 = number of pixels in strip
// Parameter 2 = Arduino pin number (most are valid)
// Parameter 3 = pixel type flags, add together as needed:
//   NEO_KHZ800  800 KHz bitstream (most NeoPixel products w/WS2812 LEDs)
//   NEO_KHZ400  400 KHz (classic 'v1' (not v2) FLORA pixels, WS2811 drivers)
//   NEO_GRB     Pixels are wired for GRB bitstream (most NeoPixel products)
//   NEO_RGB     Pixels are wired for RGB bitstream (v1 FLORA pixels, not v2)
//   NEO_RGBW    Pixels are wired for RGBW bitstream (NeoPixel RGBW products)
Adafruit_NeoPixel *strip;

int patternSwitchState = 1;
int whiteSwitchState = 1;
int protocolSwitchState = 1;

void setup() {

  pinMode(patternSwitchPin, INPUT_PULLUP);
  pinMode(potentiometerPin, INPUT_PULLUP);
  pinMode(potentiometerPinPlus1, OUTPUT);
  pinMode(potentiometerPinMinus1, OUTPUT);
  pinMode(whiteSwitchPin, INPUT_PULLUP);
  pinMode(5, OUTPUT); // debugging LED - remove later

  digitalWrite(potentiometerPinPlus1, LOW); // leave this low to give a ground pin conveniently next to the potentiometer input pin
  digitalWrite(potentiometerPinMinus1, HIGH); // leave this high to give a 5 V pin conveniently next to the potentiometer input pin

  if (digitalRead(whiteSwitchPin) == HIGH) {
    whiteSwitchState = 1;
  } else {
    whiteSwitchState = 0;
  }

  strip = new Adafruit_NeoPixel(NUM_LEDS, DATA_PIN, (whiteSwitchState ? NEO_GRB : NEO_RGBW) + NEO_KHZ800);

  strip->begin();
  strip->show(); // Initialize all pixels to 'off'
  strip->setBrightness(2); // Set BRIGHTNESS to about 1/5 (max = 255)
}

void loop() {
  NUM_RAINBOW_LEDS = map(analogRead(potentiometerPin), 0, 1023, 1, 60);

  patternSwitchState = digitalRead(patternSwitchPin);

  if (patternSwitchState == 1) { // for debugging LED - remove later
    digitalWrite(5, HIGH);
  }
  else {
    digitalWrite(5, LOW);
  }

  if ((patternSwitchState == 0) && (whiteSwitchState == 0)) {
    solidColoursRGB();
  } else if ((patternSwitchState == 1) && (whiteSwitchState == 0)) {
    rainbowCycle(2);
  } else if ((patternSwitchState == 0) && (whiteSwitchState == 1)) {
    solidColoursRGBW();
  } else if ((patternSwitchState == 1) && (whiteSwitchState == 1)); {
    whiteOverRainbow(20, 75, (NUM_RAINBOW_LEDS / 8 + 1));
  }
}

void solidColoursRGB() {
  // Clear the existing led values, then set all to green according to potentimeter reading.
  strip->clear();
  for (int led = 0; led < NUM_LEDS; led++) {
    strip->setPixelColor(led, 255, 0, 0);
  }
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);

  // Clear the existing led values, then set all to red according to potentimeter reading.
  strip->clear();
  for (int led = 0; led < NUM_LEDS; led++) {
    strip->setPixelColor(led, 0, 255, 0);
  }
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);

  // Clear the existing led values, then set all to blue according to potentimeter reading.
  strip->clear();
  for (int led = 0; led < NUM_LEDS; led++) {
    strip->setPixelColor(led, 0, 0, 255);
  }
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
}



void solidColoursRGBW() {
  // Clear the existing led values, then set all to green according to potentimeter reading.
  strip->clear();
  for (int led = 0; led < NUM_LEDS; led++) {
    strip->setPixelColor(led, 255, 0, 0, 0);
  }
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);


  // Clear the existing led values, then set all to red according to potentimeter reading.
  strip->clear();
  for (int led = 0; led < NUM_LEDS; led++) {
    strip->setPixelColor(led, 0, 255, 0, 0);
  }
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);


  // Clear the existing led values, then set all to blue according to potentimeter reading.
  strip->clear();
  for (int led = 0; led < NUM_LEDS; led++) {
    strip->setPixelColor(led, 0, 0, 255, 0);
  }
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);


  // Clear the existing led values, then set all to white according to potentimeter reading.
  strip->clear();
  for (int led = 0; led < NUM_LEDS; led++) {
    strip->setPixelColor(led, 0, 0, 0, 255);
  }
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);

}


// Slightly different, this makes the rainbow equally distributed throughout
void rainbowCycle(uint8_t wait) {
  uint16_t i, j;

  for (j = 0; j < 256 * 5; j++) { // 5 cycles of all colors on wheel
    for (i = 0; i < NUM_RAINBOW_LEDS; i++) {
      strip->setPixelColor(i, Wheel(((i * 256 / NUM_RAINBOW_LEDS) + j) & 255));
      //      strip->setPixelColor(1, 255, 255, 255);
    }
    strip->show();
    delay(wait);
  }
}

// Input a value 0 to 255 to get a color value.
// The colours are a transition r - g - b - back to r.
uint32_t Wheel(byte WheelPos) {
  WheelPos = 255 - WheelPos;
  if (WheelPos < 85) {
    return strip->Color(255 - WheelPos * 3, 0, WheelPos * 3);
  }
  if (WheelPos < 170) {
    WheelPos -= 85;
    return strip->Color(0, WheelPos * 3, 255 - WheelPos * 3);
  }
  WheelPos -= 170;
  return strip->Color(WheelPos * 3, 255 - WheelPos * 3, 0);
}

void whiteOverRainbow(uint8_t wait, uint8_t whiteSpeed, uint8_t whiteLength ) {

  if (whiteLength >= NUM_RAINBOW_LEDS) whiteLength = NUM_RAINBOW_LEDS - 1;

  int head = whiteLength - 1;
  int tail = 0;

  int loops = 3;
  int loopNum = 0;

  static unsigned long lastTime = 0;


  while (true) {
    for (int j = 0; j < 256; j++) {
      for (uint16_t i = 0; i < NUM_RAINBOW_LEDS; i++) {
        if ((i >= tail && i <= head) || (tail > head && i >= tail) || (tail > head && i <= head) ) {
          strip->setPixelColor(i, strip->Color(0, 0, 0, 255 ) );
        }
        else {
          strip->setPixelColor(i, Wheel(((i * 256 / NUM_RAINBOW_LEDS) + j) & 255));
        }

      }

      if (millis() - lastTime > whiteSpeed) {
        head++;
        tail++;
        if (head == NUM_RAINBOW_LEDS) {
          loopNum++;
        }
        lastTime = millis();
      }

      if (loopNum == loops) return;

      head %= NUM_RAINBOW_LEDS;
      tail %= NUM_RAINBOW_LEDS;
      strip->show();
      delay(wait);
    }
  }

}

The switches are connected to GND and the pinmode is INPUT_PULLUP.

What do you see if you print the values of patternSwitchState and whiteSwitchState immediately before testing their values ?

HINT : print text labels before the values so that you know which value you are seeing and a linefeed after each pair of values to make interpretation easier

Not part of the problem but

  if (digitalRead(whiteSwitchPin) == HIGH)
  {
    whiteSwitchState = 1;
  }
  else
  {
    whiteSwitchState = 0;
  }

can be simplified to

  whiteSwitchState = digitalRead(whiteSwitchPin);
  digitalWrite(potentiometerPinPlus1, LOW); // leave this low to give a ground pin conveniently next to the potentiometer input pin
  digitalWrite(potentiometerPinMinus1, HIGH); // leave this high to give a 5 V pin conveniently next to the potentiometer input pin

Why not simply wire the pot to 5V and GND ?

This needs to be in loop(), otherwise it is only read once in setup(), then never again

  if (digitalRead(whiteSwitchPin) == HIGH) {
    whiteSwitchState = 1;
  } else {
    whiteSwitchState = 0;
  }

what is the purpose of this ?

  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);

I solved the problem with my else if statement (it was an extraneous semi-colon) but left the descriptions of the problem, having typed them out already, on the off chance that it helps someone solve some problem in the future. Thanks for your help (will use serial monitor for debugging in the future instead of LEDs!)

UKHeliBob:
What do you see if you print the values of patternSwitchState and whiteSwitchState immediately before testing their values ?

HINT : print text labels before the values so that you know which value you are seeing and a linefeed after each pair of values to make interpretation easier

Not part of the problem but

  if (digitalRead(whiteSwitchPin) == HIGH)

{
    whiteSwitchState = 1;
  }
  else
  {
    whiteSwitchState = 0;
  }



can be simplified to


whiteSwitchState = digitalRead(whiteSwitchPin);






digitalWrite(potentiometerPinPlus1, LOW); // leave this low to give a ground pin conveniently next to the potentiometer input pin
  digitalWrite(potentiometerPinMinus1, HIGH); // leave this high to give a 5 V pin conveniently next to the potentiometer input pin



Why not simply wire the pot to 5V and GND ?

It's nice to be able to just shove a potentiometer into my breadboard at the right location without using jumpers and I don't need all the pins on the Arduino right now. When I have got everything working right, I will use soldered wires.

I have modified the code to print the states of patternSwitchState and whiteSwitchState to the serial monitor and the switch states printed to the serial monitor match the phyisical reality of the switches on my breadboard. However...

With patternSwitchState at 0 and whiteSwitchState at 1, I expect to get solidColoursRGBW (in the NEO_RGBW format) and nothing else. Instead I get solidColoursRGBW (good) followed by whiteOverRainbow (bad! naughty!), both in the NEO_RGBW format (good)!

With patternSwitchState at 1 and whiteSwitchState at 1, I expect to get whiteOverRain in the NEO_RGBW format and nothing else and that's exactly what happens!

With patternSwitchState at 0 and whiteSwitchState at 0, I expect to get solidColoursRGB (in the NEO_GRB format) and nothing else. Instead I get solidColoursRGB (good) followed by what I think is an attempt at whiteOverRainbow (bad; in the NEO_GRB format).

With patternSwitchState at 1 and whiteSwitchState at 0, I expect to get rainbowCycle (in the NEO_GRB format) and nothing else. Instead I get rainbowCycle (good) followed by what I think is an attempt at whiteOverRainbow (bad; in the NEO_GRB format).

Setup and loop are now:

void setup() {

  pinMode(patternSwitchPin, INPUT_PULLUP);
  pinMode(potentiometerPin, INPUT_PULLUP);
  pinMode(potentiometerPinPlus1, OUTPUT);
  pinMode(potentiometerPinMinus1, OUTPUT);
  pinMode(whiteSwitchPin, INPUT_PULLUP);

  digitalWrite(potentiometerPinPlus1, LOW); // leave this low to give a ground pin conveniently next to the potentiometer input pin
  digitalWrite(potentiometerPinMinus1, HIGH); // leave this high to give a 5 V pin conveniently next to the potentiometer input pin

  Serial.begin(9600);

  whiteSwitchState = digitalRead(whiteSwitchPin);

  Serial.print("Setup whiteSwitchState: ");
  Serial.println(whiteSwitchState);

  strip = new Adafruit_NeoPixel(NUM_LEDS, DATA_PIN, (whiteSwitchState ? NEO_RGBW : NEO_GRB) + NEO_KHZ800);

  strip->begin();
  strip->show(); // Initialize all pixels to 'off'
  strip->setBrightness(2); // Set BRIGHTNESS to about 1/5 (max = 255)
}

void loop() {
  NUM_RAINBOW_LEDS = map(analogRead(potentiometerPin), 0, 1023, 1, 60);

  patternSwitchState = digitalRead(patternSwitchPin);

  Serial.print("whiteSwitchState: ");
  Serial.println(whiteSwitchState);
  Serial.print("patternSwitchState: ");
  Serial.println(patternSwitchState);

  if ((patternSwitchState == 0) && (whiteSwitchState == 0)) {
    solidColoursRGB();
  } else if ((patternSwitchState == 1) && (whiteSwitchState == 0)) {
    rainbowCycle(2);
  } else if ((patternSwitchState == 0) && (whiteSwitchState == 1)) {
    solidColoursRGBW();
  } else if ((patternSwitchState == 1) && (whiteSwitchState == 1))[color=red][b];[/b][/color] {
    whiteOverRainbow(20, 75, (NUM_RAINBOW_LEDS / 8 + 1));
  }
}

The problem was the semi-colon that I've highlighted in red, at the end of the last else if line. Edit: lol. I didn't highlight it in red but you can see where I tried!

CrossRoads:
This needs to be in loop(), otherwise it is only read once in setup(), then never again

  if (digitalRead(whiteSwitchPin) == HIGH) {

whiteSwitchState = 1;
  } else {
    whiteSwitchState = 0;
  }

I'm not sure I can do that because

 strip = new Adafruit_NeoPixel(NUM_LEDS, DATA_PIN, (whiteSwitchState ? NEO_GRB : NEO_RGBW) + NEO_KHZ800);

needs to be set up in the setup section of the code after which I think it's not possible to change without resetting the Arduino. Do you think it should be possible to change the setting from NEO_GRB to NEO_RGBW without resetting the Arduino?

Deva_Rishi:
what is the purpose of this ?

  strip->show();

delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);
  strip->show();
  delay(250);

The reason for this is that I want to use pogo pins to test boards without soldering them first. I want the board to change colours every two seconds so I can check that each LED on the board lights up in each colour. If I do strip->show; delay(2000); then I have to wait up to two seconds after pressing the board on the pogo pins to find out if I have aligned the board correctly on the pogo pins. By repeating strip->show(); every quarter second, I don't have to wait very long to find out whether I'm holding the board properly against the pogo pins. I realise I could make it tidier using a for loop though.

seanspotatobusiness:

 strip = new Adafruit_NeoPixel(NUM_LEDS, DATA_PIN, (whiteSwitchState ? NEO_GRB : NEO_RGBW) + NEO_KHZ800);

needs to be set up in the setup section of the code after which I think it's not possible to change without resetting the Arduino. Do you think it should be possible to change the setting from NEO_GRB to NEO_RGBW without resetting the Arduino?

Can you not assign different output pins and pogo pins for each one, run both, and just connect to the one you need?

aarg:
Can you not assign different output pins and pogo pins for each one, run both, and just connect to the one you need?

Maybe but I don't know how to make the different programs run simultaneously. I'm just a script kiddy, trying to fudge my way through!

I think it's not possible to change without resetting the Arduino

It is possible but not recommended. The memory will be freed and re-allocated on the heap.

Can you not assign different output pins and pogo pins for each one, run both, and just connect to the one you need?

In fact you can assign the same output pin to different objects, and select the one you want to .show()

rgbstrip = new Adafruit_NeoPixel(NUM_LEDS, DATA_PIN, NEO_GRB + NEO_KHZ800);
whitestrip = new Adafruit_NeoPixel(NUM_LEDS, DATA_PIN, NEO_RGBW + NEO_KHZ800);

and perform actions on the object of your choosing. Disadvantage is that you will double up on buffer space, and with 300 leds that is going to be an issue (3 bytes * 300 + 4 bytes * 300 = 2100 bytes oops out of memory). All in all the first option is probably safer, since you are not using the heap for anything else, but what you have is functional, so i would leave it at that.