FastLED / FastLED

The FastLED library for colored LED animation on Arduino. Please direct questions/requests for help to the FastLED Reddit community: http://fastled.io/r We'd like to use github "issues" just for tracking library bugs / enhancements.
http://fastled.io
MIT License
6.43k stars 1.63k forks source link

RFduino/NRF51/Simblee + APA102 timing issue? #294

Closed jasoncoon closed 8 years ago

jasoncoon commented 8 years ago

There seems to be an issue on the Simblee (and possibly RFduino and other NRF51 boards) after commit https://github.com/FastLED/FastLED/commit/720f339e3ca5d60ccc1217c0aa8c70e4f3314852. That commit works fine (with my changes to led_sysdefs.h from commit https://github.com/FastLED/FastLED/commit/023baa3b063d85fabe7ee05c97dfa67d997ffaa1), but the next commit doesn't.

Something like a simple color fade is glitchy and erratic. Here's some video of the problem, if it helps: https://goo.gl/photos/2SB4m18DuKrECMAv6

A guy I've been working with on a Simblee project had his working fine, but I could not get mine working. After a few weeks of troubleshooting back and forth, we finally compared FastLED versions. I diffed his copy to master, and tracked his version back to October 2015.

I'll keep digging and see if I can figure out how to fix it. Any help you can offer would be greatly appreciated. Thanks!

soamaven commented 8 years ago

I see similar issues with current master branch and RFduino. Reverting to 720f339 fixes these timing issues

I diff'd the two commits, and there are significant changes, beyond what I think I could help reconcile at this point in my knowledge of this library and LED controlling :/

jasoncoon commented 8 years ago

Thanks for confirming! Yeah, I'm afraid I'm still not skilled enough either, but I'm going to try to help at least. :)

focalintent commented 8 years ago

Thanks for the digging/tracking this down (both here and in the g+ post) - I just need to get some time to get one of my rfduino's set up and dig in to what's going on here (especially since it appears to only be the nrf* that's impacted by this)

soamaven commented 8 years ago

Hey Guys,

I did some digging into the code. I think i tracked part of the problem to commit a0d7f9 where the APA102 chipset was given the option of being written as 8 or 16 bit.

All I did was revert to using 8 bit output on writeByte(uint8_t b ) and forcing the NRF51 chipset to use writeWord(uint16_t w) (which calls writeByte() twice on the separate bytes) by commenting out FAST_LED_SPI_BYTEONLY definition in "led_sysdefs_arm_nrf51.h". I also had to change the strip to BGR in the addLeds() method during setup. For some reason, using the uint8_t version of writeByte() along with defining FAST_LED_SPI_BYTEONLY does not fix the timing. Diff Here

I don't really understand why the current master is using 32 bit input on writeByte(). I don't want to lose speed by reverting, but its all I could get working.

One last thing, for my strip, pixel[0] is not being set correctly, so this patch is incomplete for me. But the timing seems better...

@jasoncoon care to try this patch?

focalintent commented 8 years ago

I don't remember why i had those two as uint32_t -- it isn't that way anywhere else (I wonder if I was having compiler/platform issues - this code was originally written for a non-arduino environment as it was on contract for an embedded systems project).

(I still don't have a setup where I can test this, and the way work is going, it's going to be a few more days before I can put any time into this).

As far as pixel[0] not being set correctly - what do you mean? Do you mean it's left dark? Do you mean that the color is wrong? do you mean that everything is shifted down by one pixel?

(Also - as for the timing thing - one thing that might be causing a problem is NRF's mechanism for determing whether or not it's ok to write out the next byte is pretty shitty - and so right now, I'm not using it - which means that if data is getting pushed to the SPI subsystem too quickly (which may happen if you're setting a lower data rate) then things could end up getting corrupted).

soamaven commented 8 years ago

I forgot to mention above [updated now], the patch only works when the color order is changed from from RGB, to BGR in the addLeds method.

Pixel[0] is the wrong color. One specific case is being Red when it should be black. Yellow when it should be Red. I dont think it is the whole pixel that is shifted, but I think that the last 8 bits are bleeding from pixel[i+1] which could be a contributing factor to having to switch to BGR.

What code determines the timing? I couldn't track it down.

No worries on the being busy -- I can keep poking around, project goal-line is mid-June :)

focalintent commented 8 years ago

In a pinch - try adding:

#define FASTLED_FORCE_SOFTWARE_SPI

before:

#include "FastLED.h"

that'll bypass the hardware SPI support - it sounds like I need to do some more work with it (your patches basically sound like they juggle things around enough to get things to a point of more likely to work - but not actually fixing the problem - i think I need to revisit the nrf51 datasheet and dig into how to determine when it is safe to write data into the SPI TX register).

soamaven commented 8 years ago

#define FASTLED_FORCE_SOFTWARE_SPI fixes the bit-bleeding, but color order still must be BGR. So the colors are correct now, the timing seems correct, and it makes no difference if my patch is used or not.

focalintent commented 8 years ago

The color order thing makes sense - not all led chipsets are RGB ordered (and it's not even about chipsets - I have some WS2812 chips that are RGB ordered and others that are GRB ... and then there's one strip i have that is BRG ordered).

jasoncoon commented 8 years ago

#define FASTLED_FORCE_SOFTWARE_SPI seems to have fixed all of my issues, thanks!

soamaven commented 8 years ago

I found the bug in the Hardware SPI usage. The writeByte() method was not clearing the EVENTS_READY register after writing to the TX register, and so every byte was off by some delay at the beginning of each pixel (it would clear the EVENTS_READY register after each pixel, per the APA102 controller calling waitFully(), which clears the reg.). Here is a blurb from the NRF52 Product Specification:

The SPI master will move the incoming byte to the RXD register after a short delay following the SCK clock period of the last bit in the byte. This also means that the READY event will be delayed accordingly, see Figure 3. Therefore, it is important that you always clear the READY event, even if the RXD register and the data that is being received is not used.

So this is technically from nrf52, but applys for nrf51 also. I can't find this in the nrf51 product specs, but found this write behavior in the RFduino files as well (i now see the macros in nrf51.h).

I updated my branch with the patch. I also changed some code that seemed unnecessary and tried to include some info in comments; the RXD data was being read into a variable on the wait() methods also, which we don't need to do.

I think we should do some testing with other chipsets before pulling? I only own WS2812Bs in addition to my APA102Cs, so I can't be the one to do this.

Also, an interesting thing I learned about the nrf52, is that the TXD has a two-byte buffer. Curious if/how that could be used for speed one day.

PS. this was kinda fun way to learn the library, more C/C++, and about hardware. I hope this helps

LeonardoGuimaraes commented 8 years ago

Hi @jasoncoon ,

How are you compiling the code to the Simblee? I am trying to use the Arduino IDE (1.6.5), puting the library into the libraries folder, but i got an error! fatal error: avr/io.h: No such file or directory

jasoncoon commented 8 years ago

@LeonardoGuimaraes, have you followed the Simblee setup instructions? I had to put FastLED in the portable/sketchbook/libraries directory.

LeonardoGuimaraes commented 8 years ago

Hi @jasoncoon, Yes I ahve followed the tutorial! Today i tried to download everything again, and now it is working and I do not know why! Thanks for the attention!!