My "finished" code for an RGB LED controller box. Any suggestions?

Hello! I have "finished" this code for an RGB LED controller I built myself using mainly three MOSFETs and a teensy-LC. The LEDs are on my friends electric scooter. We basically have a bunch of voltage stepdowns and regulators to power the teensy and LEDs. I am wondering if there is any way to make this code more efficient. This is the best I could get it right now. I attached the file because the code itself exceeds the character limit. Thanks!

RGB controller.txt (10 KB)

I dislike your main strategy 'for loops with delay on steroids' (with interrupt flag exit) very much. :sob:

    while (millis() - prvMillis < 55) {
      if (yeild)
        break;
    }

Suggestion: throw it away, rewrite all that stuff.
No delay() and no while (millis()-xx<yy) in loop(), or in functions called by loop().

Suggestion for the big case in loop(): condense it to something like

typedef void (*fPtr)();
fPtr modes[] = { modeColorCycle, modeHeartbeat, .... };

void loop() {
  (*modes[mode-1])();
  yeild = false;
}

Thank you for that. It really helps make my code efficient and concise. I don't really understand what you mean by you first suggestion though...

What of

  • throw it away
  • rewrite all that stuff
  • no delay() and no while (millis()-xx<yy) in loop()
  • nor in functions called by loop()is hard to understand?

Shure, your sketch works, but it is very unflexible and hard to read and maintain.

I am wondering if there is any way to make this code more efficient.

Why? You say, or imply, that it is working. What will increased efficiency do for you?

Pete

You can argue 'if its not broken, why fix it?'

Try to add features like
some potentiometers to adjust colors/brightness/... and have them influence the running animations
stop the animation (via button), blink the red led 200ms on/off for 3 seconds, then continue with the animation

That wont be easy, caused by the code that is 'working'.

Your code is not the usual copied/gathered stuff, don't get me wrong,
you just follow a road of trouble with delay(), even if its on interrupt steroids.

If you do not want to change the functionality of your sketch, there is no reason to change it,
as nobody will ever see it just looking at your animations.

All else aside, if your project is working, and you don't plan on further modifications, what does it matter that the code is horrifying, hard to read, and hard to modify?

I would not even use the interrupt, unless the device is going to sleep, don't use interrupts for button presses unless you have a damned good reason; you have a reason, but it's a damned bad one. You're using blocking delays left right and center, and you've hackjob'ed your way out of it with that yield variable and an interrupt, instead of just using millis, and you followed that rabbit hole all the way to completion of your project.

I would:

record millis() when animation updated
in loop:

check if 4 milliseconds (or whatever you want to refresh at) have passed , if so, call a function to do the next step of the animation and record the current millis().
check if any buttons are pressed, and if so, change the current mode.

In this way, your main loop() will be running fast enough that it'll catch the button presses without an interrupt, and the code will be far more readable, and you'll be much better positioned to add on to it.

In this case (the for loop with delay animation) interrupts are a way to detect button while delay()ing.

The OP asked for suggestions for the code otherwise he could have posted a youtube link showing only the the animations (which I had probably ignored).

Yes, by "efficient" I mean I want to improve the way I do my timing / delaying with my animations. What would be the best to implement in my code?

Animations change values in a predefined/generated sequence on a timed schedule, right?

So I would use a TaskScheduler library for convenience.

You write a value generator/sequencer/set function and have it called on your schedule.

That would isolate the generation from the timing
and allows the animations to run independently and parallel.

You can start/stop/delay/ single tasks, there is builtin iteration support.

I use a modified version of GitHub - arkhipenko/TaskScheduler: Cooperative multitasking for Arduino, ESPx, STM32, nRF and other microcontrollers but there are many other variants available.

I think I thought of a solution that may work. If I make my "Delay" (while loop) into a function where instead of doing this / every time I needed it.

while (millis() - prvMillis < ms) {
    if (yeild)
      break;
  }
  prvMillis = millis();

I would just call a Delay(int ms) function. In that function I could take advantage of downtime by doing a quick task.

To be honest, I'm not sure how to implement a task scheduler into my program as is. I would have to re-write everything. And I feel it works good enough to where I don't really want to re-code the whole thing to make it just a little more time efficient. I did implement the function pointer suggestion though. :slight_smile:

TehProgrammer:
To be honest, I'm not sure how to implement a task scheduler into my program as is. I would have to re-write everything.

You remember my suggestion? :wink:

Shure, you can leave the sketch as it is, it works.

But:

How about writing a new one from scratch using a tasker?
Starting with just one of your animations?
Or create new ones?
Maybe just to learn how to tame that beast?

That may be a good weekend project for me. Thanks for the tip! :smiley: