search

lancaster-university / codal-microbit-v2

CODAL target for the micro:bit v2.x series of devices
MIT License
43 stars 52 forks source link

ws2812 timing is incorrect when using `HARDWARE_NEOPIXEL` #294

Closed jimmo closed 1 year ago

jimmo commented 1 year ago

I'm helping a school group figure out an issue related to neopixels. The background is:

Digging into this a bit more, it appears the issue is specifically with the CODAL implementation, I think likely only when HARDWARE_NEOPIXEL is enabled (which MicroPython uses the default, enabled). It appears that this isn't generating an 800kHz waveform, and although WS2812 seems to work, the other chips such as APA104 (and maybe SK6812) don't like it.

I was surprised that make:code was fine though, but seems that they use their own driver https://github.com/microsoft/pxt-neopixel which uses https://github.com/microsoft/pxt-ws2812b/ which appears to be a common implementation shared by v1 and v2 micro:bit. Curious why they don't use CODAL also?

v1 MicroPython is also fine because it uses the hand-written assembly code in https://github.com/bbcmicrobit/micropython/blob/master/source/lib/neopixelsend.s

I captured some traces, which are attached (can be opened in PulseView). microbit-traces.zip The summary below is:

v2-micropython:
0 (H/LL): 380 / 1615   501kHz
1 (HH/L): 625 / 1370   501kHz

v2-makecode:
0 (H/LL): 340 / 780    892kHz
1 (HH/L): 810 / 590    714kHz

v1-micropython:
0 (H/LL): 250 / 1000   800kHz
1 (HH/L): 810 / 440    800kHz

v1-makecode:
0 (H/LL): 250 / 1000   800kHz
1 (HH/L): 810 / 440    800kHz

v2-micropython-bitbang:
0 (H/LL): 270 / 920    840kHz
0 (H/LL): 370 / 910    781kHz
1 (H/LL): 830 / 480    760kHz

HH/L means the relatively long H followed by the short L (i.e. a zero bit), and H/LL means the one bit. Note the last one is for CODAL with HARDWARE_NEOPIXEL disabled and appears to have a small amount of jitter for the zero bit, hence two lines for the 0.

I noticed that the hardware implementation has #define WS2812B_PWM_FREQ 500000 https://github.com/lancaster-university/codal-nrf52/blob/master/inc/WS2812B.h#L37

Changed this to 800kHz and I think the results look more correct, although I think the T1H needs to be slightly longer? It does still work on a WS2812, need to test on APA104.

v2-micropython-800kHz:
0 (H/LL): 380 / 870   800kHz
1 (H/LL): 630 / 620   800kHz

Happy to send a PR, but would be good to understand if the 500kHz is intentional? @finneyj @microbit-carlos @dpgeorge Thanks!

microbit-carlos commented 1 year ago

Hi @jimmo, Thanks for the detailed analysis, a PR would be very welcomed!

I don't remember the details, but I suspect we probably gathered the timings of the types of Neopixels we knew about at the time, and got it working for those values. Maybe @finneyj remembers more?

(I'll move this issue to the codal-microbit-v2 repository, codal-nrf52 was definitely the correct/logical place to open the issue, but we manage a lot of the micro:bit-related work from that issue tracker, so it's easier for us to triage it over there)

jimmo commented 1 year ago

Thanks @microbit-carlos -- I have sent a PR at https://github.com/lancaster-university/codal-nrf52/pull/47

This works on all the neopixel and neopixel-like strips I have access to (which includes WS2812B, SK6812 RGBW, APA104), including the previously-not-working robots mentioned in the original message.

JohnVidler commented 1 year ago

Merged this in to -nrf52 - thanks for the PR :)

jimmo commented 1 year ago

hi @microbit-carlos & @JohnVidler, thanks for merging this.

Damien has just updated the MicroPython repo to use CODAL 0.2.58 (https://github.com/microbit-foundation/micropython-microbit-v2/commit/f47cbe1d531a636c435083a3b15ae2edac846ebd) which includes this fix. Could you please let me know a rough ETA for when the online editor at python.microbit.org will incorporate this so I can let the schools know (there are hundred of these affected robots in use across several schools and they're looking forward to the fix!). Thanks!