Maybe Faster Code Needed

I have this code below that is suppose to change the WS2812 LED colors using an analogue input. But, it seems that I don't get the variations in colors that I anticipated.

Being a novis to this, I am thinking that the 'IF' statement is too slow and too many samples are gathered before the 'IF' statement finishes. I changed the Serial.begin(57600); statement to several different abaud but that didn't seem to make a difference in any color changes.

Is there a more efficient way to code this (something faster than 'If')?

Color pitchConv(int p, int b) {
  Color c;
  double bright = convBrightness(b);

  if(p < 40) {
    setColor(&c, 255, 0, 0);
  }
  else if(p >= 40 && p <= 77) {
    int b = (p - 40) * (255/37.0000);
    setColor(&c, 255, 0, b);
  }
  else if(p > 77 && p <= 205) {
    int r = 255 - ((p - 78) * 2);
    setColor(&c, r, 0, 255);
  }
  else if(p >= 206 && p <= 238) {
    int g = (p - 206) * (255/32.0000);
    setColor(&c, 0, g, 255);
  }
  else if(p <= 239 && p <= 250) {
    int r = (p - 239) * (255/11.0000);
    setColor(&c, r, 255, 255);
  }
  else if(p >= 251 && p <= 270) {
    setColor(&c, 255, 255, 255);
  }
  else if(p >= 271 && p <= 398) {
    int rb = 255-((p-271)*2);
    setColor(&c, rb, 255, rb);
  }
  else if(p >= 398 && p <= 653) {
    setColor(&c, 0, 255-(p-398), (p-398));
  }
  else {
    setColor(&c, 255, 0, 0);
  }
  setColor(&c, c.r * bright, c.g * bright, c.b * bright);
  return c;
}

The if are the fastest part of the code you posted!

You need to post your complete code if you want the forum to help find what is making your code slow, because what you posted isn't the problem.

It will not save much time, but checking for p>=206 is not needed, as this must be true at this point...
(Assuming p is some kind of integer)

1 Like

What does this float help here?
Needed because you should have written

(p - 206)*255/32

?
Always multiply first. Then divide...

If this is time critical code, then casting to float and back is a bad idea...

So, it would speed up things if I were to reduce the number of logical 'IF' statements? OK, I'll try.

Attached is the code I'm using, not my original code but it almost does what I want.

Since my posting I played around with the baud more and this setting is just a bit too slow but the next setting (31250) is too fast. Is there a way to set something in the middle and still have Arduino IDE print the values?

#include "FastLED.h"

#define NUM_LEDS    30 //300      // How many leds in your strip?
#define updateLEDS  1  //7        // How many do you want to update every millisecond?
#define COLOR_SHIFT 180000  // Time for colors to shift to a new spectrum (in ms)
CRGB leds[NUM_LEDS];        // Define the array of leds

// Define the digital I/O PINS..
#define DATA_PIN    6        // led data transfer
#define PITCH_PIN  A0        // pitch input from frequency to voltage converter
#define BRIGHT_PIN A1 //4        // brightness input from amplified audio signal
#define PrintOn     1        // Turns on print to monitor

// Don't touch these, internal color variation variables
unsigned long setTime = COLOR_SHIFT;
int shiftC = 0;
int mulC = 2;

// Define color structure for rgb
struct color {
  int r;
  int g;
  int b;
};
typedef struct color Color;

void setup() { 
    Serial.begin(19200);
  	FastLED.addLeds<NEOPIXEL, DATA_PIN>(leds, NUM_LEDS);
    pinMode(A0, INPUT);
    pinMode(A4, INPUT);

    for(int i = 0; i < NUM_LEDS ; i++) {
      leds[i] = CRGB(0,0,0);
    }
    FastLED.show();
}

void loop() { 
  unsigned long time = millis();

  // Shift the color spectrum by 200 on set intervals (setTime)
  if(time / (double)setTime >= 1) {
    setTime = time + COLOR_SHIFT;
    if (PrintOn > 0) {
    Serial.println(setTime);
    }
    shiftC += 200;
    mulC++;
    if(shiftC >= 600) {
      shiftC = 0;
    }
    if(mulC > 3) {
      mulC = 2;
    }
  }

  // Shift all LEDs to the right by updateLEDS number each time
  for(int i = NUM_LEDS - 1; i >= updateLEDS; i--) {
    leds[i] = leds[i - updateLEDS];
  }

  // Get the pitch and brightness to compute the new color
  int newPitch = (analogRead(PITCH_PIN)*2) + shiftC;
  Color nc = pitchConv(newPitch, analogRead(BRIGHT_PIN));
      if (PrintOn > 0) {
      Serial.print("New Pitch  ");
      Serial.println(newPitch);
      Serial.print(" - ");
      //Serial.println(int color);
      Serial.print(" - ");
      //Serial.println(int nc);
    }

  // Set the left most updateLEDs with the new color
  for(int i = 0; i < updateLEDS; i++) {
    leds[i] = CRGB(nc.r, nc.g, nc.b);
  }
  FastLED.show();

  //printColor(nc);
  delay(1);
}

/**
 * Converts the analog brightness reading into a percentage
 * 100% brightness is 614.. about 3 volts based on frequency to voltage converter circuit
 * The resulting percentage can simply be multiplied on the rgb values when setting our colors,
 * for example black is (0,0,0) so when volume is off we get 0v and all colors are black (leds are off)
 */
double convBrightness(int b) {
  double c = b / 614.0000;
  if( c < 0.2 ) {
    c = 0;
  }
  else if(c > 1) {
    c = 1.00;
  }
  return c;
}

/**
 * Creates a new color from pitch and brightness readings
 * int p         analogRead(pitch) representing the voltage between 0 and 5 volts
 * double b      analogRead(brightness) representing volume of music for LED brightness
 * returns Color structure with rgb values, which appear synced to the music
 */
Color pitchConv(int p, int b) {
  Color c;
  double bright = convBrightness(b);

  if(p < 40) {
    setColor(&c, 255, 0, 0);
  }
  else if(p >= 40 && p <= 77) {
    int b = (p - 40) * (255/37.0000);
    setColor(&c, 255, 0, b);
  }
  else if(p > 77 && p <= 205) {
    int r = 255 - ((p - 78) * 2);
    setColor(&c, r, 0, 255);
  }
  else if(p >= 206 && p <= 238) {
    int g = (p - 206) * (255/32.0000);
    setColor(&c, 0, g, 255);
  }
  else if(p <= 239 && p <= 250) {
    int r = (p - 239) * (255/11.0000);
    setColor(&c, r, 255, 255);
  }
  else if(p >= 251 && p <= 270) {
    setColor(&c, 255, 255, 255);
  }
  else if(p >= 271 && p <= 398) {
    int rb = 255-((p-271)*2);
    setColor(&c, rb, 255, rb);
  }
  else if(p >= 398 && p <= 653) {
    setColor(&c, 0, 255-(p-398), (p-398));
  }
  else {
    setColor(&c, 255, 0, 0);
  }
  setColor(&c, c.r * bright, c.g * bright, c.b * bright);
  return c;
}

void setColor(Color *c, int r, int g, int b) {
  c->r = r;
  c->g = g;
  c->b = b;
}

// Prints color structure data
void printColor(Color c) {
  if (PrintOn > 0) {
  Serial.print("( ");
  Serial.print(c.r);
  Serial.print(", ");
  Serial.print(c.g);
  Serial.print(", ");
  Serial.print(c.b);
  Serial.println(" )");
}}

Why is this not a method in your struct?
Same for printColor...
And pitchConv...

How can a baud rate be too fast? If you are trying to match some pace of output on the Serial monitor, you can always inject a delay(xxx) in your loop() to slow down an arbitrary amount. Or use millis() and track elapsed time and don't print the results until the proper amount of time has elapsed. Look at the Blink Without Delay example in the IDE (File->examples->02.digital->Blink Without Delay)

1 Like

As suggested, I changed the IF statements to this (see code). As indicated, there doesn't seem to be any noticeable change in the colors which is OK. I decided to spread the color values out some and that helped. I may have had some too close together.

About 'Same for printColor...'

I am not familiar with it or how it works. I tried looking for examples that would help me understand but I couldn't find any. I guess I'm not structuring my search correct.

Color pitchConv(int p, int b) {
  Color c;
  double bright = convBrightness(b);

  if(p < 40) {
    setColor(&c, 255, 0, 0);
  }
  else if(p <= 77) {
    int b = (p - 40) * (255/37.0000);
    setColor(&c, 255, 0, b);
  }
  else if(p <= 205) {
    int r = 255 - ((p - 78) * 2);
    setColor(&c, r, 0, 255);
  }
  else if(p <= 250) {
    int g = (p - 206) * (255/32.0000);
    setColor(&c, 0, g, 255);
  }
  else if(p <= 300) {
    int r = (p - 239) * (255/11.0000);
    setColor(&c, r, 255, 255);
  }
  else if(p <= 375) {
    setColor(&c, 255, 255, 255);
  }
  else if(p <= 500) {
    int rb = 255-((p-271)*2);
    setColor(&c, rb, 255, rb);
  }
  else if(p <= 700) {
    setColor(&c, 0, 255-(p-398), (p-398));
  }
  else {
    setColor(&c, 255, 0, 0);
  }
  setColor(&c, c.r * bright, c.g * bright, c.b * bright);
  return c;
}

It's a fools errand to try and performance tune a program without measuring where it spends its time. It's not like we're going to look at your code and say, "oh, on line 52 you set SlowMode=true."

In the scheme of things, an if() is not normally slow, it's unlikely the problem is too many if's.

Here's a quick and dirty way of profiling your code:

First, define a bunch of variables of type DWORD to hold the time spent in each section:

DWORD pitchConvTime=0; 
DWORD loopTime=0;

etc.
Define two macros:

#define BEGINTIMINGBLOCK(timingvariable) timingvariable-=millis()
#define ENDTIMINGBLOCK(timingvariable) timingvariable+=millis()


Bracket each section of code you want to profile with BEGINTIMINGBLOCK()/ENDTIMINGBLOCK():

Color pitchconv()
{
   BEGINTIMINGBLOCK(pitchConvTime);
...

   ENDTIMINGBLOCK(pitchConvTime);
}

void loop()
{

   BEGINTIMINGBLOCK(loopTime);
...

   ENDTIMINGBLOCK(loopTime);
}

Then put code in loop() to periodically dump the value of the timing variables to the serial monitor so you can see where the program is spending its time. Once you see that you can put timing blocks around smaller and smaller sections of code and find the sections that are taking time.

dccontrarian,

Thanks for your input. I was asking for ideas from you guys that have done this before in some way or another. Again, I'm a novias.

I really like your suggestion and will set out making code and using it to point out where I can change things.

Your code could be improved by using methods.
Usung classes, you can keep things that belong together in one class.
You can also do this in a struct. Works exactly the same (but all members are default public with structs and default private with classes and maybe some other subtle differences).

It will probably not make your code faster...
But it will make your code clearer, more robust and more reusable.

Your speed problem is most probably not caused by some ifs.

But your improved code is easier to read and understand now... in general: do not optimize for speed. Optimize for readability and robustness. That saves your time (needed for debugging) instead of mcu time...

W3schools and geekstogeeks have nice websites where you can learn about classes. And probably also about struct methods...

The only affect the baud rate has on the speed of your code is when you are printing enough to fill the Serial transmit buffer. Once the buffer is full, Serial.print() has to wait for characters to be sent so that free space becomes available in the buffer. At 31250 buad each character takes 0.32mS to send. If you set the baud rate to the highest reliable speed, that will minimize the possibility of causing delays in the code.

Something else that isn't a problem in your code, but could be if the LED strip had more LEDs, is that FastLED (as well as Adafruit_NeoPixel) disables interrupts while sending data to the LED strip. This interferes with the interrupt for the millis() counter when it takes more than 1mS to update the LEDs, which occurs with approximately 34 LEDs.

Good info to know. My testing is with 30 LEDs but final will be with 148 LEDs. Is there any other than FastLED or Adafruit_NeoPixel that I should consider if I were to go to 300 or more LEDs?

Is there a limit for the max number of leds for those libraries?
Depending on other parts of your code you might run out of ram.
Most libraries reserve an array of 3 bytes per led to store the color. So at 300 leds, that would need 900 byte.

The bit sequence to drive "neopixels" is tightly specified, so essentially there is no way to make the update go "faster."

Some of the more advanced 32bit CPUs have libraries that pre-compute the bitstream and then use DMA to do the actual output, which at least in theory permits the CPU to resume calculations while the DMA happens in "the background/" In reality some of them wait for the output to complete anyway, to avoid concurrency issues. (also, the pre-computation typically consumes a lot of RAM - one scheme creates 3 output bits for each bit of the actual data, so it takes a total of 4x the RAM of a bit-banged solution.)

Placement of FastLED.show() will change the "speed" of the animation. Experiment.

What exactly do you mean the placement of the FastLED.show() will change the speed?

I can't be exact, but here is what I have experienced (I am still a novice).

If you call FastLED.show() for each color/intensity change of each individual pixel, it will be much slower (measurable in seconds) than a color/intensity change of the group of pixels before calling FastLED.show() (all changes will look nearly instantaneous). If I don't make sense, I will try to get some minimal code to show it.

[edit]

The code in this simulation chugs along smoothly. Here are lines 65, 66, 67, 68. If line 68 were to be inside the parenthesis on line 66, after led[j]... (loading the buffer), FastLED.show() will make the code much slower... even if you remove line 67 (delay(DELAY);) which was inserted to slow the speed of the "fade"

/* 65 */          led[j] = CRGB(red, grn, blu);
/* 66 */    }
/* 67 */    delay(DELAY); // fade speed, larger value for slower fade
/* 68 */    FastLED.show(); // display neopixel buffer

Of course it will. FastLED.show() clocks all the data out to the LEDS. By moving it inside the parenthesis, you are calling it 50x the number of times (NUMPIX). You are changing only the value of the first LED, clocking out all the data to all 50 LEDS, updating the value of the second LED, clocking out all the data, ...

That was the intent. Placing it where each pixel gets displayed (slow), rather than displaying all pixels at one time (fast). Answering the question "what do you mean?" as a response to the statement...