Code Optimizing ...

I need some suggestions here guys. Need to make this run better. I have a string of RGB LEDs that I'm trying to fade up then back down. Sounds easy enough, however it's not. The string consists of 8 LEDs now, it will have more later. I want each LED to fade up and down on its own, and each one will be fading up to a random color, as well as on their own timing (meaning they're not all doing it at the same time.) The LEDs are controlled with WS2801 and the code I'm using maintains an array representing the LEDs. When I need the various colors to be pushed down the string, there's a specific routine that does that. The array looks like this:strip_colors{0xff0000,0xffff00,0x00ff00,0x00ffff,0x0000ff, etc., etc}

So what I thought of doing was this:
a) maintain an array of currentMillis for each individual LED
b) maintain an array of colors for each individual LED
c) maintain an array for how many steps up or down they've already faded

Now, the code itself looks like this:

//fill in the various array with relevant data
  if (!filled) {
    for (int px = 0; px < STRIP_LENGTH; px++) {
      // Pre-fill colors, delays, and timers
      randomColor(COLOR, r, g, b, 1);   // returns an RGB color value (and individual r, g, and b values), how, is not important for this exercise :)
      strip_fChase[px] = rColor;
      strip_delays[px] = random(0, 1000);  // set individual, random delays for each LED
      strip_pmills[px] = millis();  // keep track of when each LED was last updated
      strip_cnters[px] = 1;  // keep track of how many steps down or up the LEDs have faded
    }
    filled = 1;
  }

Now for the up/down part. The way I figured this was, first, stick random colors on the array strip_fChase (it's an arbitrary name). Then, take that value, manipulate it, and store it in the strip_colors array so I can push (shift out) the data down the string. In order to fade UP, I step through each color up to 8 steps till it's at its actual value that it needs to be, and then step back 8 steps down. Here's what I'm doing for the fade down part:

    for (int px = 0; px < STRIP_LENGTH; px++) {
      if (millis() - strip_pmills[px] > strip_delays[px]) {
        // Extract individual colors to manipulate
        oColor = strip_fChase[px];
          // This is the Fade Down part
          r=0xFF;
          g=0xFF;
          b=0xFF;
          b &= oColor;
          oColor >>= 8;
          g &= oColor;
          oColor >>= 8;
          r &= oColor;
          for (int steps = 0; steps < strip_cnters[px]; steps++) {
            r >>= 1;
            if (r < 0) r = 0;
            g >>= 1;
            if (g < 0) g = 0;
            b >>= 1;
            if (b < 0) b = 0;
          }
          nColor = r;
          nColor <<= 8;
          nColor |= g;
          nColor <<= 8;
          nColor |= b;
          strip_colors[px] = nColor;
          if (nColor == 0) {
            // if we've reached 0, pick a new random color, a new delay and store
            randomColor(COLOR, r, g, b, 1);
            strip_fChase[px] = rColor;
            strip_delays[px] = random(250, 1000);  // create an 'off' delay
          }
          strip_cnters[px]++;
          if (strip_cnters[px] > 7) strip_cnters[px] = 1;
        post_frame(0);
        delay(1);
        strip_pmills[px] = millis();
        strip_delays[px] = pause - 5;  // this 'pause' is the full loop pause (think Blink without delay), and it's set at 10ms
      }
    }

Basically, it steps through 0 to whatever the counter is (between 0 and 8 steps) and divides the color in half. So as the counter increases, it does less steps, causing the color to be brighter and brighter. I do the reverse when I have to fade up, the code is the same, except the counter is from -8 to 0 so the calculations are reversed.

Here's the problem, this is dog slow. The first one or two LEDs that come up are fine, but the moment the rest of them start to also blink, it crawls. I can set the delay to 0 for that matter and it's just too slow. My suspicion is because I'm stepping up to 8 times through each and every single LED as I loop through. 8 LEDs, up to 8 steps, that's a possible 64 loops for each time I go through the whole thing. Consider that I have 2 strings, that starts to really add up.

So, is there a better way of doing this?

Also, when shifting by 1 like I'm doing (r >>= 1), that effectively halves the value. Can I make that division even smaller? I realize 256 is 2^8, so halving it down to 0 is technically 8 steps ... how can I make that, say 16 steps?

Post the whole sketch perhaps?

instead of computing the brightness values as it runs you could make an array to use as a look up table. It would be easy to get 16 steps then and you could also compensate for the fact that as the values go up your eye will perceive less of a change.

iggykoopa:
instead of computing the brightness values as it runs you could make an array to use as a look up table. It would be easy to get 16 steps then and you could also compensate for the fact that as the values go up your eye will perceive less of a change.

Hmm, I'll try that. I still have to do that each time a new random color is selected ...

a) maintain an array of currentMillis for each individual LED

you might use - http://www.arduino.cc/playground/Code/StopWatchClass - as is or as inspiration ...

KirAsh4:

iggykoopa:
instead of computing the brightness values as it runs you could make an array to use as a look up table. It would be easy to get 16 steps then and you could also compensate for the fact that as the values go up your eye will perceive less of a change.

Hmm, I'll try that. I still have to do that each time a new random color is selected ...

I forgot ... I also have to do that for each individual LED. Each one has a different color, and each one will pick a different random color each time they fade off. That's several arrays that need to be re-filled each time.

So I'd have to define those arrays at the beginning. That becomes rather "static" ... if I want to run a longer string, that means having to change the code to accommodate those extra ones.

I guess I meant restructuring your code a little. So you would have an array with possible brightness values, and then to set up the rgb leds:

uint8_t colors[16] = {1,10,20,40,60,100,130,140,160,200,210,220,240,250,255}; //the numbers are just off the top of my head, you will need to adjust
uint8_t leds[8][3];
for(int i=0; i < 8; i++){
  leds[i] = {random(0,16),random(0,16),random(0,16)};
}

that way to decrease the brightness of a color, and to write the value is just:

leds[1][0]--;
analogWrite(ledPin, colors[leds[1][0]]); //just an example, not sure how you are holding the pin values or writing out the values to the leds

it should simplify the code a lot and get rid of a lot of the math you are doing.

edit: noticed you were shifting out all the values as one big array, so you will still need to convert these values at the end instead of how you doing at the beginning. I still think it will simplify things, but that's personal coding style. Converted your shift down code would look something like this (not tested)

for (int px = 0; px < STRIP_LENGTH; px++) {
  if (millis() - strip_pmills[px] > strip_delays[px]) {
    for(int i=0; i < 3; i++){
      if(leds[px][i] > 0){
        leds[px][i] -= strip_cnters[px]; //this replaces your steps for loop
        if(leds[px][i] < 0)   leds[px][i] == 0;
      }
    }
    if (leds[px][0] == 0 && leds[px][1] == 0 && leds[px][2] == 0) {
      // if we've reached 0, pick a new random color, a new delay and store
      leds[px] = {random(0,16),random(0,16),random(0,16)};
      strip_delays[px] = random(250, 1000);  // create an 'off' delay
    }
    strip_cnters[px] == 8 ? strip_cnters[px] = 1 : strip_cnters[px]++;
    strip_pmills[px] = millis();
    strip_delays[px] = pause - 5;  // this 'pause' is the full loop pause (think Blink without delay), and it's set at 10ms
  }
}
// you will need to modify post_frame to work with the leds array, some of your slow down may be
// because you post the whole frame after only calculating the changes for each led... you should be fine
// posting just once after calculating all changes
post_frame(0);  
delay(1);

Why do you have a delay(1) inside your loop? A 1 msec delay may not seem like much, and even multiplying it times 8 LEDS makes for a still seemingly small 8 msec of total delay, but at 16Mhz clock speed, that is 128,000 cycles you are throwing away (16,000 cycles per msec). If you want to optimize your code, that's clearly the first place to start right there.

jraskell: Because without that delay, the update rate is too fast for the ICs to keep up with. They start to flicker random LEDs, colors don't match, and depending on how fast it is being updated, it gets to a point where it won't even display what you're sending it, just what it feels like displaying (usually just flickering white.) I have everything running at 8MHz ...

iggykoopa: Thanks for the coding exercise. You're right, everything is shifted out to the string, because that's what the LED drivers are expecting. I'm not using PWM to manipulate the LEDs, just sending them RGB color values. So I take r, g, and b, shift them into one variable and stuff it in the colors array. Once the array is updated, the whole thing is shifted out to the string.

The code that does the shifting is on SFE's website. It does shift the entire array out as the LED drivers are daisy chained (one per LED.) In the code below, CKI and SDI are just about any two pins:

void post_frame (void) {
  //Each LED requires 24 bits of data
  //MSB: R7, R6, R5..., G7, G6..., B7, B6... B0 
  //Once the 24 bits have been delivered, the IC immediately relays these bits to its neighbor
  //Pulling the clock low for 500us or more causes the IC to post the data.

  for(int LED_number = 0 ; LED_number < STRIP_LENGTH ; LED_number++) {
    long this_led_color = strip_colors[LED_number]; //24 bits of color data

    for(byte color_bit = 23 ; color_bit != 255 ; color_bit--) {
      //Feed color bit 23 first (red data MSB)
      
      digitalWrite(CKI, LOW); //Only change data when clock is low
      
      long mask = 1L << color_bit;
      //The 1'L' forces the 1 to start as a 32 bit number, otherwise it defaults to 16-bit.
      
      if(this_led_color & mask) 
        digitalWrite(SDI, HIGH);
      else
        digitalWrite(SDI, LOW);
  
      digitalWrite(CKI, HIGH); //Data is latched when clock goes high
    }
  }
  //Pull clock low to put strip into reset/post mode
  digitalWrite(CKI, LOW);
  delayMicroseconds(500); //Wait for 500us to go into reset

By the way, some of you might suggest moving to the FastSPI library for this, and I have actually tried it already (works beautifully as well.) The problem is that I have an RF module using the SPI already and rather than figuring out now how to handle that as well as the string using the same set of pins, based on my time constraints with this project, I opted to use SFE's code since that allowed me to use any two arbitrary pins, as well as easily add a second string using two different pins. With SPI, I guarantee you, I'd still be scratching my head, or worse, I would've dropped this project because I wouldn't have been able to finish in time (I have less than 3 weeks left to solder 300 tiny LED circuits, and create the individual controllers for them. Some of the PCBs are still in production!)

KirAsh4:
The problem is that I have an RF module using the SPI already and rather than figuring out now how to handle that as well as the string using the same set of pins, based on my time constraints with this project ...

Well this (the above problem) is wasting time isn't it?

The thing is, that SPI is really simple to do multiple devices. You just parallel up MOSI/MISO/SCK and have a dedicated SS (slave select) line per device. Just pull SS low for the currently-addressed device, and high again afterwards. That's all.