ws2812: write inline assembly using C instead of Go

tinygo-org / drivers

TinyGo drivers for sensors, displays, wireless adaptors, and other devices that use I2C, SPI, GPIO, ADC, and UART interfaces.

https://tinygo.org

BSD 3-Clause "New" or "Revised" License

617 stars 195 forks source link

ws2812: write inline assembly using C instead of Go #401

Closed aykevl closed 2 years ago

aykevl commented 2 years ago

This is better for various reasons:

I don't like the inline assembly implementation in TinyGo. It kinda works, but it's hard to use correctly.
The Go version had a subtle bug: the i and value variables weren't marked as modified. The compiler produced correct code by chance. In GCC-style inline assembly, it's possible to correctly mark it as an input+output operand.
LLVM 14 has some changes to inline assembly that change how memory operands can be created. Instead of supporting that, I'd like to get rid of memory operands entirely (and possibly inline assembly in general).

I did some light testing: the ARM assembly changed a bit but not in any way that should have a practical effect, and the RISC-V assembly didn't change at all.

This PR is possible thanks to ThinLTO, which enables inlining across C and Go and performs some more dead code elimination. And it might also require #391.

deadprogram commented 2 years ago

I tried testing this on my Digispark. It compiled but did not work. Then I discovered that TinyGo v0.22 does not work either. v0.20 does, but I did not get a chance to bisect further.

aykevl commented 2 years ago

It works only halfway for me (the lights work but have the wrong color etc). I'll bisect this a bit.

aykevl commented 2 years ago

Bisected to https://github.com/tinygo-org/tinygo/commit/92150bd1c57aa78f21d41c55eaf75c04d5cbe18e. Not sure why that has an effect, because interrupts are disabled while updating the LEDs.

aykevl commented 2 years ago

The weird thing is that I've used an Arduino Nano in a project with ws2812 LEDs (192 LEDs!) and it worked just fine.

aykevl commented 2 years ago

https://github.com/tinygo-org/tinygo/pull/2791 fixes the bug for the Arduino Uno (I didn't test the DigiSpark).

deadprogram commented 2 years ago

Digispark works somewhat better, but now seems like all the commands to the LEDs in the strip are only changing the color of the first LED. The others are unlit.

PXL_20220420_161102770

aykevl commented 2 years ago

Thanks for testing! I'm afraid I need to investigate this more thoroughly.

deadprogram commented 2 years ago

I think the Digispark issue being unrelated should not delay merging this PR, which does work as advertised on the other boards I have tested.

Thanks @aykevl now merging.