Streamlining and optimizing LED display code

Projects Programming
1

My sketch is working, if not potentially sloppy and redundant, so I thought I'd throw it up here and see about recommendations to improve it.

The Project: To upgrade my holiday display by adding addressable LEDs to my christmas tree and village, and controlling it via Serial input (eventually by way of Bluetooth).

The Code:

#include <Adafruit_NeoPixel.h>
#include <SoftwareSerial.h>
SoftwareSerial BT(0, 1);

#define villagePIN 6
#define treePIN 5
#define villageStripLength 9
#define treeStripLength 88
String mode = "rainbowCycle";
Adafruit_NeoPixel treeStrip = Adafruit_NeoPixel(treeStripLength, treePIN, NEO_GRB + NEO_KHZ800);
Adafruit_NeoPixel villageStrip = Adafruit_NeoPixel(villageStripLength, villagePIN, NEO_GRB + NEO_KHZ800);


void setup() {
  randomSeed(analogRead(0));
  Serial.begin(9600);
  BT.begin(9600);
  villageStrip.begin();
  villageStrip.show(); // Initiallizes all pixels to 'off'
  treeStrip.begin();
  treeStrip.show(); // Initialize all pixels to 'off'
  flicker(255, 50, 0);
  rainbowCycle(20);
}

void loop() {
  serialEvent();
}

void rainbowCycle(uint8_t wait) {
  uint16_t i, j;
  while (mode == "rainbowCycle") { //loops forever
    for (j = 0; j < -1; j++) { // 5 cycles of all colors on wheel
      for (i = 0; i < treeStrip.numPixels(); i++) {
        treeStrip.setPixelColor(i, Wheel(((i * 256 / treeStrip.numPixels()) + j) & 255));
      }
      treeStrip.setBrightness(128);
      treeStrip.show();
      serialEvent();
      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 treeStrip.Color(255 - WheelPos * 3, 0, WheelPos * 3);
  }
  if (WheelPos < 170) {
    WheelPos -= 85;
    return treeStrip.Color(0, WheelPos * 3, 255 - WheelPos * 3);
  }
  WheelPos -= 170;
  return treeStrip.Color(WheelPos * 3, 255 - WheelPos * 3, 0);
}

void flicker(uint8_t r, uint8_t g, uint8_t b) {
  for (uint8_t i = 0; i < villageStripLength; i++) {
    uint8_t brightness = (random(2, 4) * 2) * 25.5;
    uint8_t rr = (r * brightness) / 255;
    uint8_t gg = (g * brightness) / 255;
    uint8_t bb = (b * brightness) / 255;
    villageStrip.setPixelColor(i, rr, gg, bb);
    delay(0);
  }
  villageStrip.show();
}

void colorStripes(uint32_t c1, uint32_t c2, uint8_t s, uint8_t w, uint8_t d) {
  int L;
  int numP = treeStrip.numPixels();
  while (mode == "stripes") {
    for (int j = 0; j < (w * treeStripLength); j++) {
      for (int i = 0; i < treeStrip.numPixels(); i++) {
        int mod = ((i + j) % (w * 2));
        L = treeStrip.numPixels() - i - 1;
        if ( mod < w) {
          if (mod == 4 || mod == 0) {
            treeStrip.setPixelColor(i, splitColor(c1, 'r')*.25, splitColor(c1, 'g')*.25, splitColor(c1, 'b')*.25);
          } else {
            treeStrip.setPixelColor(i, splitColor(c1, 'r')*.5, splitColor(c1, 'g')*.5, splitColor(c1, 'b')*.5);
          }
        } else {
          if (mod == 5 || mod == 9) {
            treeStrip.setPixelColor(i, splitColor(c1, 'r')*.25, splitColor(c1, 'g')*.25, splitColor(c1, 'b')*.25);

          } else {
            treeStrip.setPixelColor(i, splitColor(c2, 'r')*.5, splitColor(c2, 'g')*.5, splitColor(c2, 'b')*.5);
          }
        }
        treeStrip.show();
        delay(d);
        serialEvent();
      }
    }
  }
}

/*** splitColor() - Receive a uint32_t value, and spread into bits. */
uint8_t splitColor ( uint32_t c, char value )
{
  switch ( value ) {
    case 'r': return (uint8_t)(c >> 16);
    case 'g': return (uint8_t)(c >>  8);
    case 'b': return (uint8_t)(c >>  0);
    default:  return 0;
  }
}

void solid(uint32_t c1) {
  while (mode == "solid") {
    for (int i = 0; i < treeStripLength; i++) {
      treeStrip.setPixelColor(i, c1);
    }
    treeStrip.setBrightness(100);
    treeStrip.show();
    serialEvent();
  }
}


void starSparkle(uint32_t c1, uint32_t c2) {
  while (mode == "sparkles") {
    treeStrip.setBrightness(0);
    int rp = random((1, treeStripLength) - 1);
    int rc = random(1, 3);
    for (int i = 255; i > -10; i = i - 5) {
      if (rc == 1) {
        treeStrip.setPixelColor(rp, c1);
      } else if (rc == 2) {
        treeStrip.setPixelColor(rp, c2);
      }
      treeStrip.setBrightness(i);
      treeStrip.show();
      serialEvent();
    }
  }

}

void serialEvent() {
  if (Serial.available() > 0) {
    treeStrip.begin();
    String newMode = Serial.readString();
    newMode.trim();
    if (newMode == "default") {

    } else if (newMode == "candyCane") {
      mode = "stripes";
      colorStripes(treeStrip.Color(255, 0, 0), treeStrip.Color(255, 255, 255), 2, 5, 0);


    } else if (newMode == "hanukkah") {
      mode = "stripes";
      colorStripes(treeStrip.Color(0, 0, 255), treeStrip.Color(255, 255, 255), 2, 5, 0);


    } else if (newMode == "redAndGreen") {
      mode = "stripes";
      colorStripes(treeStrip.Color(255, 0, 0), treeStrip.Color(0, 255, 0), 2, 5, 0);


    } else if (newMode == "silverAndGold") {
      mode = "stripes";
      colorStripes(treeStrip.Color(192, 192, 192), treeStrip.Color(255, 215, 0), 2, 5, 0);


    } else if (newMode == "rainbowCycle") {
      mode = "rainbowCycle";
      rainbowCycle(20);


    } else if (newMode == "sparkles") {
      mode = "sparkles";
      starSparkle(treeStrip.Color(255, 255, 255), treeStrip.Color(0, 191, 255));

    } else if (newMode == "red") {
      mode = "solid";
      solid(treeStrip.Color(255, 0, 0));
    } else if (newMode == "green") {
      mode = "solid";
      solid(treeStrip.Color(0, 255, 0));
    } else if (newMode == "blue") {
      mode = "solid";
      solid(treeStrip.Color(0, 0, 255));
    } else if (newMode == "orange") {
      mode = "solid";
      solid(treeStrip.Color(255, 50, 0));
    } else if (newMode == "yellow") {
      mode = "solid";
      solid(treeStrip.Color(255, 100, 0));
    } else if (newMode == "purple") {
      mode = "solid";
      solid(treeStrip.Color(201, 0, 158));
    }
  }
}

One little annoyance I see is that I need to put in loops where they don't seem necessary. For example, if I don't loop through the solid function instead of just setting all the pixels and leaving them, the program defaults back to the rainbowCycle function, as if it never stopped running. It's not a huge deal, but I'd like to keep things as svelte as possible.

Of course, I'm completely open to suggestions or ideas to make this better. I'm coupling it with an Android app that sends the serial commands, but until that's finished, testing is just being done via the Serial console.

2 
SoftwareSerial BT(0, 1);

It is silly to use SoftwareSerial on the hardware serial pins.

  Serial.begin(9600);

It is impossible to use them for software serial AND hardware serial at the same time.

For example, if I don't loop through the solid function instead of just setting all the pixels and leaving them, the program defaults back to the rainbowCycle function, as if it never stopped running

What does "loop through the solid function" mean? There is a while loop in the solid function. That keeps the solid function executing until a new mode is selected. But, if loop() did all the serial IO, and changed the mode only when new data arrived, it would keep calling solid() over and over.

3

You should never call serialEvent() directly, it is called when data arrives at the serial port. You can remove all your calls to serialEvent() and the check for Serial.available() inside serialEvent() (since you know data is there).

I see that there is a tutorial on the Arduino site that calls Serial.event(), but if you look at the example found under Examples => Communication => SerialEvent there is the same code with the calls removed.

4

You should never call serialEvent() directly, it is called when data arrives at the serial port.

It is NOT. It is called after loop() ends, IF there is serial data to be handled. It does NOT interrupt what loop() (or any other function) is doing when serial data arrives.

It is, in general, a completely useless "enhancement" to the Arduino repertoire.

5

PaulS:

SoftwareSerial BT(0, 1);

It is silly to use SoftwareSerial on the hardware serial pins.

  Serial.begin(9600);

It is impossible to use them for software serial AND hardware serial at the same time.

If I'm not mistaken, this is a requirement for the HC-06 module that I'm adding

PaulS:
What does "loop through the solid function" mean? There is a while loop in the solid function. That keeps the solid function executing until a new mode is selected. But, if loop() did all the serial IO, and changed the mode only when new data arrived, it would keep calling solid() over and over.

The stripe, sparkles and rainbow modes require loops to function, but generally speaking, setting all the leds to a single color doesn't require one. Without the while() loop, the leds would blink the color oh so briefly before executing the rainbowCycle function inexplicably.

6

If I'm not mistaken, this is a requirement for the HC-06 module that I'm adding

You are mistaken.

The stripe, sparkles and rainbow modes require loops to function

They do NOT. They require a series of steps, but that does NOT have to be in the form of a for loop or while loop.

7

I'm not sure I understand. Just plugging the BT module into those pins and starting Serial.begin(9600); is all that is necessary for the Uno to receive Serial commands from the module?

To your second point, how would you display those modes, until told otherwise, without an if or while loop?

8

Just plugging the BT module into those pins and starting Serial.begin(9600); is all that is necessary for the Uno to receive Serial commands from the module?

Yes.

To your second point, how would you display those modes, until told otherwise, without an if or while loop?

Here's a hint. The loop() function loops.

Suppose that the for/while loop index were a static or global variable. Could you figure out how to dispense with the for/while loop?

9

I guess I'm just not following. I purposefully kept the functions out of the loop() function because otherwise I wouldn't be able to run multiple patterns. The only thing that should constantly happen is the checking of the Serial port.

That said, if I'm wrong, I'm open to learning better ways and I DO appreciate the help.

10

Suppose you want to write a function to fade an LED, what does it need to know?

The pin number? Good idea.

The starting value? Maybe. Or maybe that will always be 0.

The ending value? Maybe. Or, maybe the ending value will always be 255.

The interval between brightness level changes? Maybe. Or, maybe that will be a constant or a global.

The direction - fade up or fade down? Maybe. Or maybe the function handles that.

Now, you have two choices. You can make the function blocking, where it will fade an LED from minimum to maximum, from maximum to minimum, or from min to max and back to min.

Or, you can have the function simply see if it is time to move, up or down, to the next level, and, if it is, move up or down, and record when the change happened.

If you want to be able to do other things, while the LED is fading, the blocking/single step choice is taken away from you.

You either have to change the function from fade() to fadeWhileCheckingSerialInput() and return when the function gets serial data or you change the function to be single stepping, and call it over and over, such as every time loop() iterates.

Well, you currently have a function that is blocking AND you need to make it non blocking. Which approach do you want to take? Make the function single-step and call it over and over? Or, change the name to indicate that the function does more than one thing, and replicate the reading of serial data in the function?