Arduino slows down at certain point in sketch

Using Arduino Programming Questions
1

Hello,

I'm trying to control a strand of WS2811 LEDs using an Arduino MEGA. For this I wanted to write a library called OctagonPanel that makes use of the very handy FastLED library to create my own methods and because I want to control other hardware later on as well.
For this I followed the tutorial on how to write your own library and classes. After some tinkering I got it to work but here's the catch: when i call a certain method, namely "update()", (which ironically is the most important one) the Arduino slows down so much that one loop cycle takes like 1.5 seconds.
I found out that it could be heap fragmentation. I read that the use of pointers can greatly decrease the risk of this happening but in the same article it's stated that pointers and such should only be used when really necessary.
Maybe it's something completely different.
Attempting to google this situation only yielded me results on slow compilation.

Here's my main:

#include "src\FastLED\FastLED.h"
#include <HardwareSerial.h> // HardwareSerial muss mit eingebunden werden, da sonst mit VSC Serial nicht funktioniert
#include "src\LightingControl\OctagonPanel.h"

const unsigned int NUM_LEDS = 160;
const int DELAY = 10, DATA_PIN = 6, HUE = 40;

int ledDefinerArray[NUM_LEDS]{0,0,0,0,1,1,1,1,2,2,2,2,0,0,0,0};
OctagonPanel panel1(DATA_PIN, NUM_LEDS, ledDefinerArray);

void setup(){
    Serial.begin(9600);
   
    panel1.begin();
    panel1.setHue(240);
    panel1.setLevel(50);
    panel1.testFunc();

}

void loop(){
    panel1.update();

}

The library is attached to this post.

The program executes at normal speed up until the 98th of 100 iterations of the last for loop of testFunc and then it slows down incredibly.

Here's a little explanation of what the library should be doing:
There's the single LED strand which will be mounted to a wall and will go up the wall in sections either vertically, diagonally or move left or right horizontally to create a nice pattern. The strip is supposed to fill from the bottom to the top with a constant vertical speed. This means that the diagonal LEDs have to fill faster than the vertical ones and the horizontal ones need to fill up simultaneously. The array called "ledDefinerArray" defines what type of LED each LED is. The array "ledTypes" saves the necessary values for the three types. The first value is the amount of iterations needed to fill the LED and the second value states how many iterations of th ecurrent LED must have been passed to start filling the next LED. Based on these pieces of information the "update()" method is repeatedly called and gradually fills the Strip up to the wanted level (_levelTarget)

P.S: my variable declarations are a bit scattered as I've been fooling around with different data types and figured that this might've been the culprit. Sadly it wasn't.

Thank you in advance for your time!

Cheers

Smagel

OctagonPanel.cpp (4.21 KB)

OctagonPanel.h (857 Bytes)

2

Just a quick look. But...

CRGB _leds[16];
int _ledDefinerArray[16];

...16 elements.

const unsigned int NUM_LEDS = 160;
OctagonPanel panel1(DATA_PIN, NUM_LEDS, ledDefinerArray);

...160 exceeds the array bounds.

const unsigned int _numLeds = numLeds;

You also have a private instance variable _numLeds. This is now a different, local _numLeds.
But the _numLeds in upate function will still be accessing the unassigned instance variable.

This is the programming equivalent of having 2 children and calling them both "Bob". It's perfectly legal, but you'd be stupid to do it. Never create global/instance and local variables with the same names.

What's with all the...

for(int j = 0; j <= _numLeds; j++)

If you are zero indexing, that should be < and not <= surely?

3

The discrepancy between the 16 and the 160 elements is somethin I'm not proud of and will be removed eventually but right now when I control the LED strip LEDs that aren't even in the array anymore light up as well. It appears that the FastLED library accesses the storage units behind the array containing the LED data and turns on those LEDs outside of the defined space with the values of completely unrelated data. Cranking the number of LEDs up to 160 when creating the FastLED instance circumvents this problem. In the future this will be obsolete as the LED strip will have exactly the desired length.

Thanks for the tip on the _numLeds!

The sketch is far from finished so there are a lot of bugs i suppose. Any ideas as to why it slows down upon entering the "update" method?

4

Ok so I will put this here for everyone else who is struggling with the same problem: I found the solution!

Have a look at the for loops in the "update()" method: They all define the counter variable EVERY TIME they are called. So every time a for loop starts looping, a new variable is created. And as I have multiple for loops in my method and i call the method repeatedly my storage is sure to be clogged with a pile of "i" "j" and "p" variables in a matter of milliseconds.

So how do I fix this?
Remove the defintion from all for loops and define them once in the header file. The finished thing will look like this:

for(_i = 0; _i < _numLeds, _i++){
magic();
moreMagic();
}

and in the header file I added this line:

int _i, _j, _p;

The underscores are more or less necessary. They just indicate that it's a private variable of the class. It's part of the name so the compiler doesn't really care for all I know.

5

FastLED library accesses the storage units behind the array containing the LED data and turns on those LEDs outside of the defined space with the values of completely unrelated data

So you’re fixing the fact that the library (for some reason) reads outside of the array bounds by wring outside of the array bounds yourself?

Ok, good luck with that.

Either your not using the library correctly, or you need a better library.

6

Smagel:
Have a look at the for loops in the "update()" method: They all define the counter variable EVERY TIME they are called. So every time a for loop starts looping, a new variable is created. And as I have multiple for loops in my method and i call the method repeatedly my storage is sure to be clogged with a pile of "i" "j" and "p" variables in a matter of milliseconds.

More nonsense. Sorry.

Local variables (loop or otherwise) are either on the stack or in registers. They are not dynamically allocated from the heap. As such local are automatically freed when the function ends.

You’ve changed where the variables are placed in memory. Your code as originally presented had so many array out of bounds errors and out-by-one errors that it’s no wonder in malfunctioned.

7

pcbbc:
So you’re fixing the fact that the library (for some reason) reads outside of the array bounds by wring outside of the array bounds yourself?

Ok, good luck with that.

Either your not using the library correctly, or you need a better library.

Thank you for your feedback.

Maybe I didn't understand my own code properly, but what I was trying to achieve is to create an array that is 160 elements big so that the unneccessary LEDs stay turned off because the memory for them isn't being used by something else as it's reserved/used for the LED. It did work. Plus I only wrote to the first 16 elements of the array so I don't quite get what you mean when you say that I was writing outside of the array bounds.

pcbbc:
More nonsense. Sorry.

Local variables (loop or otherwise) are either on the stack or in registers. They are not dynamically allocated from the heap. As such local are automatically freed when the function ends.

You’ve changed where the variables are placed in memory. Your code as originally presented had so many array out of bounds errors and out-by-one errors that it’s no wonder in malfunctioned.

Hmm that confuses me because I really only changed the things I described in my post and it fixed the issue. If it really had nothing to with the problem it wouldn't have fixed anything wouldn't it? Please explain it to me, I want to get better at this.
Could it be that what i did moved the i,p and j variable locations from the stack to the static data segment which now keeps the stack from growing too big when executing the "update()" method?

8

Both of these arrays are 16 elements...

       CRGB _leds[16];
        int _ledDefinerArray[16];

In many places in function update() you are iterating over these arrays using instance variable _numLeds as an upper bound. Edit: and also with loop for(int p = 0; p <= 80; p++)...

If instance variable _numLeds is set to what you pass in to the constructor as parameter numLeds, i.e. 160 (assuming you fixed the intitial problem where you had a local variable _numLeds instead), you are accessing array elements 16 through 159 of arrays with only 16 elements.

If you write outside of the array bounds you are writing to random memory locations. What area of the code is, or is not, affected by that depends not on what you change but on what those areas were used for. Often very minor changes to your code can cause memory allocation by the compiler to be radically different.

So you make a small change to your code, the illegal area of memory your code was previously writing to is now used by something else (less important to the stability of your program), or just free memory not used for anything at all, and your instability problems go away (for now). Doesn't in any way mean your code is "fixed", it could break again at the drop of a hat if you change anything.

I see you now have now started a new thread asking how to do dynamic memory allocation. That (and making sure you never access elements outside of the allocated size of an array) is indeed one way to address this problem.

9

Ok. I see. That makes sense. Thank you for the elaboration.