Help with inefficient code. If/then or CASE?

Hello,
World’s second worse programmer here. I am making an art deco speaker which will feature a number of LEDs under the control of an arduino NANO. I have one 12 position switch, 3 potentiometers a memory button and a servo to indicate which mode it is in. Each position of the switch represents a different mode for the LEDs. That’s 11 actual modes plus “OFF.” I have “successfully” written snippets of code for the first 7 modes (see below).

What I’m noticing is when I switch into mode 7 things are getting a bit sluggish/out of order. For instance, when switching into this mode the servo should advance to the mode 7 position (angle = 112), then it should display LED number one for a short period, then turn it off, then turn on the next LED for a short period and so forth to create a chasing effect. This works. However, sometimes when I switch into mode 7, one LED will light which causes a slight delay before the servo indicates it’s in mode 7. Also, even though I told it to check the mode position switch each time it turns on an LED, if I switch to a different mode, it continues to display in turn all of the LEDs before recognizing the mode switch and changing the mode.

Bottom line, is I’m wondering if the fact that all of this code being in the loop is just causing things to run slow and possibly even slower as I add more modes. And if so, what is the appropriate way to handle this situation? As you can see I have used if statements. Does the use of CASE work differently/better?

I hope that covers it. Let me know if I left anything out. I do want to stress, no matter how inefficient this is, it actually does do what I want so far, just has this sort of sluggish behavior. Thanks in advance.

 11/12/20 Controlling the LED colors using Hue, Saturation and Value(brightness) and storing to memory plus servo
*/

#include <FastLED.h>
#include <EEPROM.h>
#include <Servo.h>
#include <Adafruit_DotStar.h>
#include <SPI.h>

#define NUM1_LEDS 8 //Num of LEDs in zone 1
#define NUM2_LEDS 8 //Num of LEDs in zone 2
#define NUM3_LEDS 2 //Num of LEDs in zone 3
#define NUM_LEDS 18 //Sum of all LEDs 
#define COLOR_ORDER BGR
#define MEM_PIN 4
Adafruit_DotStar strip = Adafruit_DotStar(NUM_LEDS, DOTSTAR_BGR);

//Servo stuff
int angle;
Servo servo;

CRGB leds[NUM_LEDS];
//addresses of the color data in memory
int memH1 = 0;
int memS1 = 1;
int memV1 = 2;
int memH2 = 3;
int memS2 = 4;
int memV2 = 5;
int memH3 = 6;
int memS3 = 7;
int memV3 = 8;

//Stored color data
int H1 = 126;
int S1 = 126;
int V1 = 126;
int H2 = 126;
int S2 = 126;
int V2 = 126;
int H3 = 126;
int S3 = 126;
int V3 = 126;

//Current color data
int range_H2;
int range_S2;
int range_V2;
int z2_min = NUM1_LEDS;
int z2_max = NUM1_LEDS + NUM2_LEDS - 1;

int range_H3;
int range_S3;
int range_V3;
int z3_min = z2_max + 1;
int z3_max = z3_min + NUM3_LEDS - 1;


void setup()
{
  strip.begin();
  strip.show();

  FastLED.addLeds<DOTSTAR, COLOR_ORDER>(leds, NUM_LEDS);
  servo.attach(3);
  servo.write (angle);
}

void loop()
{
  int mode = analogRead(A3);


    //Mode 7  **************************Chaser mode
    if (mode >= 725  && mode <= 745)
    {
      angle = 112;
      servo.write(angle);
      for (int i = 0; i < NUM_LEDS; i++)
      {
        int mode = analogRead(A3);
        if (mode < 725 || mode > 745)
        {
          for (int i = 0; i < NUM_LEDS; i++)
          {
            leds[i] = CHSV(0, 0, 0);
            i = NUM_LEDS;
            break;
          }
        }
        int pot_H = analogRead(A0);
        int pot_S = analogRead(A1);
        int pot_V = analogRead(A2);
        int range_H = map(pot_H, 0, 1023, 0, 255);
        int range_S = map(pot_S, 0, 1023, 0, 255);
        int range_V = map(pot_V, 0, 1023, 0, 255);
        leds[i] = CHSV(range_H, range_S, range_V);
        FastLED.show();
        int transitionTime = 300;
        unsigned long t = transitionTime * 2.4;
        unsigned long currentmillis = millis();
        while (millis() <= currentmillis + t)
        {
        }
        leds[i] = CHSV(0, 0, 0);
        FastLED.show();
        delay(1);
      }
    }

    //    //     if (mode >= 769  && mode<=789){
    //    //      int mode = 8;
    //    //    }
    //    //     if (mode >= 819  && mode<=839){
    //    //      int mode = 9;
    //    //    }
    //    //     if (mode >= 875  && mode<=895){
    //    //      int mode = 10;
    //    //    }
    //    //     if (mode >= 939  && mode<=959){
    //    //      int mode = 11;




  
  }
 while (millis() <= currentmillis + t)
        {
        }

How is that different from delay()?

Single letter variable names suck. I want to find where t gets its value. Do a search for t and you will see why single letter variable names suck.

for (int i = 0; i < NUM_LEDS; i++)
          {
            leds[i] = CHSV(0, 0, 0);
            i = NUM_LEDS;
            break;

This I really don’t understand.

Well, I understand it, in that I know what it does, I just wonder “why?”

Because I have too few posts, I cannot post often enough so I will answer both of you in one post.

  1. The delay command ties up the processor, millis doesn't. Otherwise it performs the same function.
  2. I usually only use single letter variables to run through for loops. In this case "t" stands for time. It's the least of my concerns.
  3. The command that sets the CSV values to zero is needed to cause the LEDs to chase. If I didn't turn the LED off that I just turned on, the LEDs would just sort of stack up, one turning on, then another, then another instead of just one LED that moves from position 1 to 2 to 3, etc. Did that answer you question?

profozone:

  1. The delay command ties up the processor, millis doesn't. Otherwise it performs the same function.

delay calls yield. That snippet does does not.

delay uses micros. That snippet uses millis. delay is a tiny bit more accurate.

That snippet is coded incorrectly. delay correctly handles the wrap.

That snippet is a lower quality version of delay. That's the difference. You are better served replacing that snippet with a call to delay.

I don't understand that. All over the internet people say not to use "delay" because the processor can't do anything during the delay and to use 'millis' instead and now you are telling me the opposite. I pretty much just copied that code from one of those examples. Are you sure that the 'millis' command shouldn't just be coded differently?
To the other person who responded:
Sorry, I answered the wrong CSV to 0 question. That just turns them all off before exiting the loop. If I don't do that when I move to another mode, it leaves the values of the LEDs from the old mode on. That may not be an issue going from mode 7 to a lower mode like 6 and below, but when I'm working on mode 8, it will linger with the mode 7 LEDs as they were.

The idea of using millis is like having a clock on the wall.
You look at the clock every now and then to see if a certain time has elapsed.
If it hasn’t, you go off and do something different.

The way you have it, the “something different” doesn’t exist, and you just stare at the clock.

All over the internet people say not to use "delay" because the processor can't do anything during the delay and to use 'millis' instead

But you have to use millis() properly

Take a look at Using millis() for timing. A beginners guide, Several things at the same time and the BlinkWithoutDelay example in the IDE

@profozone the others know what they are talking about. They don't want you to explain why you did it wrong, they want you to step out of the way you think about the sketch, take some distance, and try in a better way.

You will be a better programmer by fixing just these two things:

  • The biggest mistake is of course that it is not a full sketch. Probably missing the first line with: /*
  • There is an extra indent in the loop(). Press Ctrl+T in the Arduino IDE to get rid of it.

The delay() is a software-waiting-loop.
Using millis() in a software-waiting-loop has no advantages. It has disadvantages for some Arduino boards in some situations because delay() calls yield() as Coding Badly already wrote.

"They don't want you to explain why you did it wrong, they want you to step out of the way you think about the sketch, take some distance, and try in a better way."

Ok. I do appreciate the help, really, but I'm not sure I fully understand. Tell me if this is correct.

The reason for the delay is because I believe when the LED comes on and goes off at the 'natural' rate it is happening so fast that it isn't really visible, so I wanted to leave the LED on long enough to be seen. Because of the warnings about delay I chose to use 'millis' (apparently incorrectly) but as it turns out, the only thing I needed to do is leave the LED on a little longer anyway. So in this case, it is perfectly acceptable to use 'delay' because there really isn't anything for the processor to miss during that time. Is that correct?

Heli and everyone really:
The code is too long so yeah, I probably left something in when chopped it up. I was trying to just post the declaration of variables, the setup and just mode 7, leaving out the other 6 modes. The program, basically runs this way:

User switches the knob, which runs through a resistor network. The NANO reads the value of the resistance to decide which mode it is in. Each mode is under an 'if' statement. If mode 1 do this, if mode 2 do this and so on. One of my questions was, is that ok or is that part of the sluggish problem?

User formerly known as:
I don't think I explained your earlier question correctly. I might be able to remove the CSV to 0 command eventually, but the problem I was having was that if I wanted to work on a new mode that dealt with just 3 LEDs, but the last mode turned on all 18 LEDs then when I was messing around with the code, all 18 would be on, instead of the 3 I wanted. So I wanted to turn them all off. I think when I'm finished, I won't need this command. Or were you talking about using i = NUM_LEDS when break sends me out anyway?

The correct use of millis() is in the Blink Without Delay example.

Your use of millis() is not “wrong”, but a delay() is easier and you can use a small delay() to make a nice transition of the leds. That is perfectly acceptable.

When a single led is used, them millis() is easy to implement. Suppose a 25Hz update of that led is good enough, then a certain led fading can be programmed for that single led. See my example: millis_soft_pulsating_led.ino.
You could try that and replace the single led with a single led of your ledstrip.
By using millis() for the 25Hz led update, there is no delay and the sketch can do others things at the same time.
However, using millis() for all the leds of your ledstrip is very hard (I have not even tried it). So you better use a small delay.

When a sketch is 5000 lines, then it is still medium size :wink:

You read pin A3 more than once in the loop(). I don’t know why. It could be a bad structure of the code.
Can you do a analogRead(A3); just once at the beginning of the loop() ?
I even prefer to read the others in the begin of the loop() as well.

void loop()
{
  // -----------------------------
  // Gather all data from potmeters, sensors, and so on
  // -----------------------------
  int pot_H = analogRead(A0);
  int pot_S = analogRead(A1);
  int pot_V = analogRead(A2);
  int mode = analogRead(A3);


  // -----------------------------
  // Process the information, make decisions
  // -----------------------------


  // -----------------------------
  // Send data to outputs
  // -----------------------------
}

Sending data to outputs is often combined with the part that processes the information.

When doing a for-loop, and use a break that stops the for-loop, then why do you use a for-loop ?
When doing a for-loop, and set the variable beyond the maximum to stop the for-loop, then why do you use a for-loop ?
These are the same:

for (int i = 0; i < NUM_LEDS; i++)
{
  leds[i] = CHSV(0, 0, 0);  // i starts with 0
  i = NUM_LEDS;     // the for-loop ends
  break;             // exit the for-loop
}

// It is the same as this:
leds[0] = CHSV(0, 0, 0);

When I tried to post my full code, I got a warning that I exceeded 9000 characters and it wouldn't let me post it. So I stripped the other modes out. The first analog read line actually belongs to a different mode. So for mode 7, it actually only reads once.

I have switched the millis thing to delay, but I'll probably put it back in because I've decided to make the delay variable according to the state of one of the pots. Actually that's why all of that 'transition time *2.4' stuff is in there to begin with. I could have just said t= 300 or something like that to begin with.

When I switch out of mode 7, it still wants to complete lighting all 18 LEDs before jumping out, but when I look through the code, it still seems to me that it should only complete the LED it's on and then jump out. It checks the state of the mode switch every pass through the loop, doesn't it?

I hear you about having the 'break' command and 'i' setting in there. I can remove them. I actually had it in there because in a previous use of that loop, it wasn't jumping out. When I put it in, it did jump out, but now I'm wondering if some other change must have solve the problem convincing me that I needed it. Not sure.

Thanks for the help.

delay() in it self is not the end all if evil. Its the thinking process that it fosters in new programmers that is evil.


You hire the neighbor kid to water your lawn once a week and feed your cat every day.

The kid goes out waters the lawn then spends the rest of the week standing out on your lawn looking at his watch to see what time it is so he can water it again.

Meanwhile, your cat has died.

This is how you structured your code.

What would a better use of the neighbor kid's time be?


-jim lee

Wish I had never posted.

This topic was automatically closed 120 days after the last reply. New replies are no longer allowed.