Strange artifact in writing to serial while executing loop

Question

Hope you can see this.

https://gist.github.com/DevilWAH/60ad144d2858f53845ac2e80f84bb070

The main file in this is "morse-led-rfid" which is taking reading data of an RFID tag and then displaying the text as Morse code on to a APA102 / Dotstar LED strip.

The issue is when the function updatestrip(tempbuffer[i]); is called.

Serial.println(F("\n**Start Reading**\n"));
 for (uint8_t i = 1; i < 15; i++)
 {
 if (i != 3 && i!= 7 && i != 11 && i != 15 ) // Skip the non data blocks 
 {
 readblock (i, key);
 for (uint8_t i = 0; i < 16; i++)
 {
 updatestrip(tempbuffer[i]);
 Serial.write(tempbuffer[i]);
 }
 }
 }

This is what I am calling to scroll the LED strip and write the letter in morse. if in the code snip it above i comment out "updatestrip...." then i get the txt from the RFID tag printer to serial as i would expect.

as a soon as i un comment it, the strip displays the morse code but the serial output gets garbled, it misses letters, prints reversed "?'s" and repeats its self?

Good output = "My Bunny is the best" Bad output = "?Bunny is the ?t" (question marks reversed)

Further investigation i found if i leave it uncommented but go all the way to the bottom of the "more-strip" sheet and comment out the "else statement in the shiftLED function" the serial again displays correctly, but of course the led don't :)

void shiftLED() // shift all LED up 1 
 {
 for(uint16_t i = ledCount; i > 0; i--)
 {
 if ((colors[i - 1].red == 0 ) && (colors[i - 1].green == 0 ))
 {
 colors[i] = cblack;
 }
 else if (i > 19 && i < 40)
 {
 colors[i] = cred;
 } 
 // else 
 // {
 // colors[i] = cgreen;
 // }
 // }
 }

Can any one suggest why the else statement a few function down is affecting the "serial.write(tempbuffer[i])" in the main loop?

• Majenko: Using an i loop inside an i loop is very bad practice. Never nest the same variable like that - it makes it impossible to know what you intend.
• DevilWAH: Hi, I did actuality think that, and went though and changed this. I have to admit to being a very hacky programmer (not sure i can even call my self that, it an insult to the real ones). Will add to list of things not to do.

Answer 1

In your code colors is declared as

rgb_color colors[ledCount];

This means that the very first iteration of this cycle in shiftLED

for (uint16_t i = ledCount; i > 0; i--)
{
 // access colors[i]
}

will access ledCount[ledCount] which is obviously out of bounds (and will actually write into that location!). The behavior is undefined after that.

Using unsigned types for "backward" array iteration requires a bit of skill. The common idioms are

for (uint16_t i = ledCount; i > 0;)
{
 --i; // do this ASAP
 // access colors[i]
}

or

for (uint16_t i = ledCount; i-- > 0;)
{
 // access colors[i]
}

or

// Careful: this technique is not usable with unsigned types 
// narrower than `unsigned int`
for (uint16_t i = ledCount - 1; i != -1; --i)
{
 // access colors[i]
}

This is assuming that you want to iterate over the whole array: from [ledCount - 1] all the way back to [0].

But if you intended to iterate back to [1] and stop short of [0] (which is apparently the case), then a simple fix for your cycle would be

for (uint16_t i = ledCount - 1; i > 0; i--)
{
 // access colors[i]
}

I.e. simply start from ledCount - 1, not from ledCount.

• DevilWAH: OH! correct me is I am wrong but you mean when LEDCount = 60 the valid array values are 0 though 59? so accessing access colors[i] when i = LEDCount means i am writing to array position 60 not 59! A kind of buffer over flow?
• DevilWAH: And you got me to go look up more about signed and unsigned. When back at home tonight i will try out your suggestions, thanks for the help.
• AnT stands with Russia: @DevilWAH Yes, if LEDCount is 60, then the valid indexes for colors array are from 0 to 59. Index 60 is out of bounds.
• DevilWAH: Just to confirm that sorted it out :) thank you for taking the time out to help and explain.