0

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?

  • 1
    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. – Majenko May 7 '19 at 16:49
  • 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. – DevilWAH May 8 '19 at 9:46
2

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.

  • 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 May 8 '19 at 11:19
  • 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. – DevilWAH May 8 '19 at 11:27
  • @DevilWAH Yes, if LEDCount is 60, then the valid indexes for colors array are from 0 to 59. Index 60 is out of bounds. – AnT May 8 '19 at 18:31
  • Just to confirm that sorted it out :) thank you for taking the time out to help and explain. – DevilWAH May 9 '19 at 11:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.