Arduino DigiSpark NeoPixel BlueTooth C++ Buffer Overflow

Question

i was able to make the following code after about a week of desk head banging, it works, kind of. the problem is that it's not that responsive, most of the times i have to spam the buttons on my phone to send the same command over and over again until it catches up.

Can you help me clean the code a bit?

As you will see, at times it seems i over complicated things but that is only cause i found it works better this way then what it seems to be a more "logical" simpler version. i will lay the code and then i will explain and ask my questions.

#include <Adafruit_NeoPixel.h> // NeoPixel Lib
#include <SoftSerial.h>  // Serial Lib
#define LED_PIN    2
#define LED_COUNT 30

SoftSerial bluetooth(4, 5); // RX TX
Adafruit_NeoPixel strip(LED_COUNT, LED_PIN, NEO_GRB + NEO_KHZ800);

const byte numChars = 32;
char receivedChars[numChars];
char tempChars[numChars];
boolean newData = false;
boolean goLED0 = false;
boolean goLED1 = true;
boolean goLED2 = false;
boolean goLED3 = false;
boolean goLED4 = false;


int eFx = 1;
int rC1 = 255;
int gC1 = 0;
int bC1 = 0;
int xS = 20;
int xB = 125;


void setup() {

  bluetooth.begin (9600);
  strip.begin();           // INITIALIZE NeoPixel strip object (REQUIRED)
  strip.show();            // Turn OFF all pixels ASAP

}


void loop() {
  checkLedData();
  delay(50);
  runLED();
  delay(50);
}


void recvWithStartEndMarkers() {
    static boolean recvInProgress = false;
    static byte ndx = 0;
    char startMarker = '<';
    char endMarker = '>';
    char rc;
 
    while (bluetooth.available() > 0 && newData == false) {
        rc = bluetooth.read();

        if (recvInProgress == true) {
            if (rc != endMarker) {
                receivedChars[ndx] = rc;
                ndx++;
                if (ndx >= numChars) {
                    ndx = numChars - 1;
                }
            } else {
                receivedChars[ndx] = '\0'; // terminate the string
                recvInProgress = false;
                ndx = 0;
                newData = true;
            }
        }   else if (rc == startMarker) {
            recvInProgress = true;
        }
    }
}


void parseData() {      // split the data into its parts

    char * strtokIndx; // this is used by strtok() as an index

    strtokIndx = strtok(tempChars, ",");      // get the first part - the string
    eFx = atoi(strtokIndx);     // convert this part to an integer

    strtokIndx = strtok(NULL, ",");      // get the first part - the string
    rC1 = atoi(strtokIndx);     // convert this part to an integer
 
    strtokIndx = strtok(NULL, ","); // this continues where the previous call left off
    gC1 = atoi(strtokIndx);     // convert this part to an integer

    strtokIndx = strtok(NULL, ","); // this continues where the previous call left off
    bC1 = atoi(strtokIndx);     // convert this part to an integer

    strtokIndx = strtok(NULL, ",");
    xS = atoi(strtokIndx);     // convert this part to an integer

    strtokIndx = strtok(NULL, ",");
    xB = atoi(strtokIndx);     // convert this part to an integer

    strtokIndx = strtok(NULL, NULL);
}



void checkLedData() {
  recvWithStartEndMarkers();
  if (newData == true) {
    strcpy(tempChars, receivedChars);
    parseData();
    newData = false;
    strip.setBrightness(xB);
   if (eFx == 0) { 
     goLED0 = true;
     goLED1 = false;
     goLED2 = false;
     goLED3 = false;
     goLED4 = false;
    }
   if (eFx == 1) { 
     goLED0 = false;
     goLED1 = true;
     goLED2 = false;
     goLED3 = false;
     goLED4 = false;
    }
   if (eFx == 2) { 
     goLED0 = false;
     goLED1 = false;
     goLED2 = true;
     goLED3 = false;
     goLED4 = false;
    }
   if (eFx == 3) { 
     goLED0 = false;
     goLED1 = false;
     goLED2 = false;
     goLED3 = true;
     goLED4 = false;
    }  
   if (eFx == 4) { 
     goLED0 = false;
     goLED1 = false;
     goLED2 = false;
     goLED3 = false;
     goLED4 = true;
    }}}
    

void runLED() {
  if (goLED0 == true) {
            
  } 
    if (goLED1 == true) {
          colorWipe(strip.Color(rC1, gC1, bC1), xS);
          delay(50);
          recvWithStartEndMarkers();
              
  } 
    if (goLED2 == true) {
          colorWipe(strip.Color(bC1,rC1,gC1), xS);
          colorWipe2(strip.Color(rC1, gC1, bC1), xS);
          delay(50);
          recvWithStartEndMarkers();
              
  } 
    if (goLED3 == true) {
          colorWipe(strip.Color(rC1, gC1, bC1), xS);
          colorWipe2(strip.Color(rC1/2, gC1/2, bC1/2), xS);
          colorWipe(strip.Color(rC1/5, gC1/5, bC1/5), xS); 
          colorWipe2(strip.Color(rC1/10, gC1/10, bC1/10), xS);
          delay(50);
          recvWithStartEndMarkers();  
  } 
      if (goLED4 == true) {
          colorWipe(strip.Color(gC1,rC1,bC1), xS);
          colorWipe2(strip.Color(bC1,gC1,rC1), xS);
          colorWipe(strip.Color(bC1,rC1, gC1), xS);
          colorWipe2(strip.Color(rC1, gC1, bC1), xS);
          delay(50);
          recvWithStartEndMarkers();
             
  }
  
  
}



void colorWipe(uint32_t color, int wait) {
  
  for(int i=0; i<strip.numPixels(); i++) { // For each pixel in strip...
    strip.setPixelColor(i, color);         //  Set pixel's color (in RAM)
    strip.show();                          //  Update strip to match
    delay(wait);                           //  Pause for a moment
  }
}

void colorWipe2(uint32_t color, int wait) {
  
  for(int i=29; i<strip.numPixels(); i--) { // For each pixel in strip...
    strip.setPixelColor(i, color);         //  Set pixel's color (in RAM)
    strip.show();                          //  Update strip to match
    delay(wait);                           //  Pause for a moment
  }
}

So what i'm doing is i send a code from phone, eg: <1,255,255,255,100,100> it gets picked up by void recvWithStartEndMarkers() parsed then all values get stored as INTs by void parseData()

inside the loop i got void checkLedData() that is calling the 2 functions above and depending on the first INT it activates or deactivates those booleans, then i have void runLED() that checks what booleans are true and starts blinking the LEDS

At first i had the switches activate by the INT eFx but for some reason it was working very poorly so i decided to use that part of the code to flip the booleans, in the runLED() you'll notice that i keep calling over and over this function recvWithStartEndMarkers(); as it was the only way to actually make the board respond.

I'm not sure what is going on, i believe that it works because of a bufferoverflow issue, it crashes then it can take a new command , the first version after it took 1 command it just got stuck with it, the void colorWipe was working as the Leds were going on and off, switching colors and so on, but when i was trying to change the effect or colors it didn't responded at all.

This next code i used before all this:

void loop() {
  recvWithStartEndMarkers();
  if (newData == true) {
        strcpy(tempChars, receivedChars);
            // this temporary copy is necessary to protect the original data
            //   because strtok() used in parseData() replaces the commas with \0
  parseData();
  strip.setBrightness(xbrithness);
  controlLed();
  newData = false;
  }
}

void controlLed() {
         colorWipe(strip.Color(redColor, greenColor, blueColor), xSpeed);
         colorWipe(strip.Color( greenColor, redColor, blueColor), xSpeed);
}

now this was very responsive but the problem was that it would go through void controlLed() once and it stopped while if i was calling the same function outside recvWithStartEndMarkers(); it would go in loop, like i wanted to cause i was trying to make a looping effect.

Does anyone have any idea what i could do to make it responsive but still loop the functions to make a "light show"?

Now that i posted all this i was thinking, not sure if arduino is multitasking i wonder if ATtiny85 is multi tasking, so that might be the issue, it's so busy processing the codes that it won't listen on serial what is coming in, any way to go around that?

Now that i posted all this i was thinking, not sure if arduino is multitaking i wonder if ATtiny is multi tasking, so that might be the issue, it's so busy processing the codes that it won't listen on serial what is coming in, any way to go around that? — RayShifter, Jul 20 '20 at 19:43

Delta_G · Accepted Answer · 2020-07-21T01:43:36.880

I'm bored and feeling generous so here you go. See if you can follow this. It's uncompiled and untested so I can't guarantee it does exactly what you want, but see if you can understand what I'm trying to do here. Nothing ever stops and waits for anything to happen. There are no delay calls. There is no waiting. Just cycling through and see if it is time to do something.

One change I made that doesn't affect this, just makes typing easier was to take all your goLED variables and make an array out of them. Anytime you catch yourself putting numbers on variable names, use an array instead so the compiler has access to those numbers and you don't have to repeat yourself. See how much simpler the checkLedData function got.

Then I made the colorWipe functions so that they will return a boolean, true if they've completed and false if they haven't. Then the runLED function can just check that to see if it is time to move on to the next step. For the first case it is easy, just set the goLED variable to whatever the colorWipe returns. As long as it is still working it returns true and goLED[1] stays true and you keep calling the same colorWipe function. For the others we have to make them state machines, so I added a state variable. When any of them get finished they set their goLED variable back to false. When all of those are false, meaning there's no effect currently running, then runLED will fall all the way through to the last else statement and go to see if there is another command.

Like I said, there may be a mistake or two in there. But see if you can understand how I am writing a checklist to see what needs to happen instead of a story to tell one thing right after another.

#include <Adafruit_NeoPixel.h> // NeoPixel Lib
#include <SoftSerial.h>  // Serial Lib
#define LED_PIN    2
#define LED_COUNT 30

SoftSerial bluetooth(4, 5); // RX TX
Adafruit_NeoPixel strip(LED_COUNT, LED_PIN, NEO_GRB + NEO_KHZ800);

const byte numChars = 32;
char receivedChars[numChars];
char tempChars[numChars];
boolean newData = false;

boolean goLED[5];
    
int eFx = 1;
int rC1 = 255;
int gC1 = 0;
int bC1 = 0;
int xS = 20;
int xB = 125;


void setup() {

  bluetooth.begin (9600);
  strip.begin();           // INITIALIZE NeoPixel strip object (REQUIRED)
  strip.show();            // Turn OFF all pixels ASAP

}


void loop() {
  recvWithStartEndMarkers();
  runLED();
}


void recvWithStartEndMarkers() {
  static boolean recvInProgress = false;
  static byte ndx = 0;
  char startMarker = '<';
  char endMarker = '>';
  char rc;

  while (bluetooth.available() > 0 && newData == false) {
    rc = bluetooth.read();

    if (recvInProgress == true) {
      if (rc != endMarker) {
        receivedChars[ndx] = rc;
        ndx++;
        if (ndx >= numChars) {
          ndx = numChars - 1;
        }
      } else {
        receivedChars[ndx] = '\0'; // terminate the string
        recvInProgress = false;
        ndx = 0;
        newData = true;
      }
    }   else if (rc == startMarker) {
      recvInProgress = true;
    }
  }
}


void parseData() {      // split the data into its parts

  char * strtokIndx; // this is used by strtok() as an index

  strtokIndx = strtok(tempChars, ",");      // get the first part - the string
  eFx = atoi(strtokIndx);     // convert this part to an integer

  strtokIndx = strtok(NULL, ",");      // get the first part - the string
  rC1 = atoi(strtokIndx);     // convert this part to an integer

  strtokIndx = strtok(NULL, ","); // this continues where the previous call left off
  gC1 = atoi(strtokIndx);     // convert this part to an integer

  strtokIndx = strtok(NULL, ","); // this continues where the previous call left off
  bC1 = atoi(strtokIndx);     // convert this part to an integer

  strtokIndx = strtok(NULL, ",");
  xS = atoi(strtokIndx);     // convert this part to an integer

  strtokIndx = strtok(NULL, ",");
  xB = atoi(strtokIndx);     // convert this part to an integer

  strtokIndx = strtok(NULL, NULL);
}



void checkLedData() {
  if (newData == true) {
    strcpy(tempChars, receivedChars);
    parseData();
    newData = false;
    strip.setBrightness(xB);
    for (int i = 0; i < 5; i++) {
      if (i == eFx) {
        goLED[i] = true;
      } else {
        goLED[i] = false;
      }
    }
  }
}





void runLED() {
  static int whichStep = 0;
  if (goLED[0] == true) {
    goLED[0] = false;
  }
  else if (goLED[1] == true) {
    goLED[1] = !colorWipe(strip.Color(rC1, gC1, bC1), xS)
  }

  else if (goLED[2] == true) {
    if (whichStep == 0) {
      if (colorWipe(strip.Color(bC1, rC1, gC1), xS)) {
        whichStep = 1;
      }
    }
    else if (whichStep == 1) {
      goLED[2] = !colorWipe2(strip.Color(rC1, gC1, bC1), xS));

    }
  }

  else if (goLED[3] == true) {

    if (whichStep == 0) {
      if ((colorWipe(strip.Color(rC1, gC1, bC1), xS)) {
      WhichStep = 1;
      }
    }
    else if (whichStep == 1) {
      if ((colorWipe2(strip.Color(rC1 / 2, gC1 / 2, bC1 / 2), xS)) {
        whichStep = 2;
      }
    }
    else if (whichStep == 2) {
      if ((colorWipe(strip.Color(rC1 / 5, gC1 / 5, bC1 / 5), xS)) {
        whichStep = 3;
      }
    }
    else if (whichStep == 3) {
      if ((colorWipe2(strip.Color(rC1 / 10, gC1 / 10, bC1 / 10), xS)) {
        goLED[3] = false;
      }
    }
  }
  
  else if (goLED[4] == true) {
    // You write this one
    goLED[4] = false;
  }
  else {
    checkLedData();  // get next command
    whichStep = 0;
  }

}



boolean colorWipe(uint32_t colot, int wait) {
  static unsigned long lastMillis = millis();
  static int state = 0;
  static int ledIndex = 0;

  if (state == 0) {
    lastMillis = millis();
    ledIndex = 0;
    strip.setPixelColor(ledIndex, color);
    strip.show();
    state = 1;
  }
  if (state == 1) {
    if (millis() - lastMillis >= wait) {
      lastMillis = millis();
      ledIndex++;
      strip.setPixelColor(ledIndex, color);
      strip.show();
      if (ledIndex == strip.numPixels() - 1) {
        state = 0;
        return true;
      }
    }
  }
  return false;
}


boolean colorWipe2(uint32_t colot, int wait) {
  static unsigned long lastMillis = millis();
  static int state = 0;
  static int ledIndex = 0;

  if (state == 0) {
    lastMillis = millis();
    ledIndex = strip.numPixels() - 1;
    strip.setPixelColor(ledIndex, color);
    strip.show();
    state = 1;
  }
  if (state == 1) {
    if (millis() - lastMillis >= wait) {
      lastMillis = millis();
      ledIndex--;
      strip.setPixelColor(ledIndex, color);
      strip.show();
      if (ledIndex == 0) {
        state = 0;
        return true;
      }
    }
  }
  return false;
}

Sorry, got my true and false backwards, just edited to make it right. So when colorWipe returns true, it's done with one wipe and the runLED function can either set it's goLED to false or it can move on to the next step in that effect. — Delta_G, Jul 20 '20 at 22:29
I also wasn't sure what colorWipe2 was supposed to do. As it was it would have run forever. I think you had that for loop wrong. I think you want to light them up backwards, but that's not quite what you had there. — Delta_G, Jul 20 '20 at 22:30
The important point is that all we kept from the colorWipe example code is the line that sets the pixel to the right color. That's all the example code was there to show you. Using a for loop to do that in a blocking manner is fine for an example code, but as you just learned it doesn't work if you want the code to be able to do anything else. — Delta_G, Jul 20 '20 at 22:32
wwwwoooowwww, thank you so much, i'll check it in an hour or so, i've just noticed you answered! thank you so much Delta_G — RayShifter, Jul 20 '20 at 23:07
i cleanead up the code, there were a few things here and there missing or extra like ; ) caps and other stuff that prevented compiling / 5390 bytes (89%) used. It's not working though i'm looking deeper into it to see if i can find the issues. — RayShifter, Jul 21 '20 at 00:23
Yeah sorry about that, I don't have the library for the leds so I couldn't compile it. I figured there would be mistakes in there. That's good though, I don't want you to just copy from me. Learn from me instead. Get the idea of non-blocking code and then write your own version. — Delta_G, Jul 21 '20 at 00:25
recvWithStartEndMarkers() should probably be called somewhere... From loop. I kinda left that out it looks like. Can't work if we don't check serial. — Delta_G, Jul 21 '20 at 00:27
void checkLedData() { if (newData == true) { strcpy(tempChars, receivedChars); parseData(); newData = false; strip.setBrightness(xB); for (int i = 0; i < 5; i++) { if (i == eFx) { goLED[i] = true; } else { goLED[i] = false; } } } } — RayShifter, Jul 21 '20 at 00:30
There's also an issue in there if you send a new command while an effect is running and then send a second one. Since we are only looking at the commands if the effect has ended we won't see one that comes in the middle if another one comes in to replace it. Like I said, there are several holes in this, don't just copy it. The point was to see how I'm having these functions called over and over and looking for steps instead of just calling it once and having it go all the way through the animation before moving on to something else. — Delta_G, Jul 21 '20 at 00:30
ok so here the last part with for, i is defined as 0 then if i is smaller then 5 it keeps adding by increments of 1 then it checks if i = efx it sets goLED[whatever i value it reached to goLED[that value] sych an wierd laguage... anyway i is used in otehr parts if i remeber corectly, is it a global thing or it may generate a conflict? — RayShifter, Jul 21 '20 at 00:33
Well you had if eFx is 0 then goLED0 is true and everything else is false. Same for the other numbers. This rolls through all 5 numbers 0 to 4 and if it matched eFx then set that goLED true and all the others get set to false because they don’t match i and eFx. — Delta_G, Jul 21 '20 at 00:49
The i is used elsewhere but in that for loop it is local to that for loop. The most local variable is the one used. Really single letters should only be used for very narrow scopes like for loops and while loops. You wouldn’t want a global variable with a single letter name. It’s not illegal just bad form. You should be able to read a variables name and know what information it holds. — Delta_G, Jul 21 '20 at 00:51
i called recvWithStartEndMarkers(); in void loop, it gets the data and it rolls, goLED1 works great, goLED2 runs only he second part, i'll drop the code then expain what it does — RayShifter, Jul 21 '20 at 01:19
else if (goLED[2] == true) { if (whichStep = 0) { if (colorWipe(strip.Color(bC1, rC1, gC1), xS)) { whichStep = 1; } } else if (whichStep = 1) { goLED[2] = !colorWipe2(strip.Color(rC1, gC1, bC1), xS)); } — RayShifter, Jul 21 '20 at 01:20
ok, so when i do the second effect, it runs only colorwipe2, it jumps over colorwipe1 entirely, the scope of this efect is to role colorwipe1 then 2 then 1 back again.... — RayShifter, Jul 21 '20 at 01:21
else if (goLED[2] == true) { if (whichStep = 0) { goLED[2] = !colorWipe(strip.Color(rC1, gC1, bC1), xS); whichStep = 1; } else if (whichStep = 1) { goLED[2] = !colorWipe2(strip.Color(rC1, gC1, bC1), xS); whichStep = 0; } } — RayShifter, Jul 21 '20 at 01:22
i changed the code to the one above, i don't understand why the second if was called with the boolean, anyway, i was tried to define the witchstep 0 / 1 to do a go between them, it does the same as the first code, it only runs colorwipe2 — RayShifter, Jul 21 '20 at 01:23
Newbie mistake, what I get for snapping off code without compiling. Look at this: if (whichStep = 0) that sets whichStep to 0 It should be if (whichStep == 0). I'lll edit the answer to fix those. — Delta_G, Jul 21 '20 at 01:43
lol, i didin' noticed either, = sets == compares...now i get a strange effect, unexpected....when i go to goled2 the first time it changes the first led to a color, the second time makes half the strip 1 color and the other one another color, (i changed the valuse for color wipe2 like this: colorWipe2(strip.Color( gC1, rC1, bC1), xS); / red value is fed into green and reverse, since i can only update that many things....the memory is getting close to 100% it's at 95 now :D. i'll go to sleep and see what i can do tommorrow, thank you so much for all the help — RayShifter, Jul 21 '20 at 02:07
If it’s reporting 95% at compile then you’re out of memory. That just counts global and static locals. Any local variables aren’t counted in that. Once you run out of memory weird things happen cause it goes to do whatever expecting one thing to be there and it’s overwritten with something else. So there’s no way to explain what’s happening if it’s running out of memory. I still say you should bump up to a pro-mini or some other tiny ATmega 328 based board. — Delta_G, Jul 21 '20 at 02:33

Arduino DigiSpark NeoPixel BlueTooth C++ Buffer Overflow

1 Answers1