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?

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.