Rewrite ws2812 serialization code to use UART1 instead of bit-banging.

[Alkorin]

It allows keeping interrupts enabled but force to use GPIO2.

It's a first draft, some work still need to be done because it brokes compatibility.
What's the best solution ?

• Depreciate the old module and create a new one with new function signatures because bit-banging is just a bad idea and we should stop allowing people using it ?
• Offering the choice between bit-banging-which-breaks-wifi and uart-but-only-gpio2 ?
• if(pin == gpio2) { uart } else { bitbanging } with a notice which says that using another pin than GPIO2 is at your own risk ?

[TerryE]

@Alkorin Tomas, this really is a completely new approach. So my suggestion is not to try to PR too quickly. Find another WS2812 developer to buddy with and act as independent tester (@skycoders @kbeckmann ?) and hammer it so that you are both confident.

Also out of interest, why are you reallocating a temporary buffer to process the GRB -> RGB triples. why not just pass an RGB flag into ws2812_write()? Something like

  int i, j,  o[3];
  o[0] = rgb_flag ? 1: 0; o[1] = 1- o[0]; o[2] = 2;

  for (i = 0; i < length; i += 3) {
    for (j = 0 ; j <2; j++) {
      uint8_t value = pixels[i+o[j]];
      // . . . Wait for 4 free bytes and write out 4 x bit doubles
    }
  }

Also why wait at half full threshold, rather than 252? Also use UART1 rather than 1. And this is one point where you could bung in a delay between the tests of the fifo count.

[Alkorin]

Yes it needs to be tested. I stress test it here too. My current bench : 300 leds strip, 75 fps, 92% CPU time in user code (3.1ms to build the buffer + 8.9ms to stream it = 12ms repeated each 13ms) and 10 ping/s.

It "works" :)

1000 packets transmitted, 1000 received, 0% packet loss, time 100828ms

I monitor the stream with my scope too to see any failures.

For your second question, I don't reallocate a temporary buffer, I didn't touch this part of the code. It could be optimized... later ... one patch at a time ;)

For the third question, according to some documentation, UART FIFO is 128 bytes

I could #define UART1 1 at the top of the code, UART1 seems only be defined in app/driver/uart.c :/

[matgoebl]

@Alkorin Thanks a lot for your quick PR.
At a first try I was able to drive my ws2812 strip.
I will do more testing in a few days: re-run the tcp tests, that crashed before.

[kbeckmann]

Always interesting with a new approach! I don't mean to sound negative but what's the benefit of using UART? Handling interrupts while pushing pixels?

I see the CPU usage is still high.
Using I2S you can push pixels using DMA and offload the cpu, someone did that here https://github.com/cnlohr/esp8266ws2812i2s but it messes with the tx or rx pin (don't remember which one right now) so debugging becomes messy.

I would vote for two solutions, one which can handle any gpio but no interrups, and another solution that enables interrupts but is limited to a specific pin. This could also be a build flag if flash size is critical.

Edit: changed a few things after reading my post again.

[kbeckmann]

@Alkorin A bit off topic for this PR maybe since you're trying to move away from bit banging. But if you are concerned about the time it takes to push pixel data to the ledsrip, you could use two output pins simultaneously. I experimented with it and got it to work - resulted in double frame rate. http://kbeckmann.github.io/2015/07/25/Bit-banging-two-pins-simultaneously-on-ESP8266/

[Alkorin]

I2S uses U0RXD. So if you want to use your strip, you'll loose access to Lua's interpreter via serial :/ (and will need a little more deep patch ...)
UART uses U1TXD which is accessible via GPIO2.

The benefit of using UART instead of bit-banging is to let the wifi stack alive ! According to Espressif's documentation we should not disable interrupts more than 10µs. 10µs is the time used to serialize ... 8 bits, a third led... So the library will never be compliant with Espressif's recommendations :/

We could let the bit-banging version but with a notice which says "use at your own risks - wifi stack will die if you use it for more than 0 led... :)". If you don't need wifi, why not ;)

[Alkorin]

@kbeckmann I already saw your work to use two pins, here I tried to address #903. We have a more robust library compatible with Espressif's recommendations. The drawback is to have a specific pin :/ (on an esp-01... ☺️)

[skycoders]

Hi, first tests on my end look good as well, I will continue testing extensively. My build currently does not connect to WiFi but that might be a different story.
Over all, I'd prefer a single pin solution as well if you look at it from a usability point of view. It is much easier for people to connect their strip instead of trying to synchronize two strips in a way that keeps animations etc. working. My plan was to provide a simple http based control of nice WS2812 features on typical strips you can get at the dealer of your choice. I expect that if we get this working, the WS2812 library of NodeMCU will get much more useful for anyone. Nevertheless, leaving the PIN choice to people by maybe also keeping the bit-banging implementation active sounds reasonable as well. If we provide the new implementation as its own module, that could easily be integrated through the online NodeMCU firmware builder if people need it. Just as a guess...

[kbeckmann]

Oh, thanks for the explanaition @Alkorin. This probably explains the issues I've had when pushing pixel data coming from wifi for longer time (hours). It works surprisingly well considering the 10us recommendation, hehe.

If this patch makes the ESP more stable, then by all means, go for it! I'll also try the code and let you know how it goes.

[skycoders]

I have now set up my 300 LEDs strip, together with WiFi. I have this script https://github.com/geekscape/nodemcu_esp8266/blob/master/examples/ws2812.lua running at an update rate of 20ms and it runs great. Even at only 5ms timer interval before each update it runs. This never worked with the old driver. I have to stop for now, but tests will continue tomorrow evening. Thanks @Alkorin for implementing this so quickly!

[TerryE]

All (probably less Tomas who gets this) the wider architectural issue arises because the ESP8266 contains a single core LX106, that also needs to run the Wifi stack. Yes the chip has a Wifi controller on the die, but this is pretty low level and needs a general purpose CPU to handle interrupts and Wifi / IP / TCP housekeeping tasks. If you mask off interrupts so that you can bit bang out to the WS2812, or any GPIO, then the Wifi stack dies, and recovery isn't straight forward.

The UART FIFO is latched out under H/W control and a 128 byte FIFO gives you a H/W buffer of 32×9µS = 288 µSec worth of banged bits, which mean that you don't need to mask interrupts and thus the network ISRs can be served without causing a ws2812 reset.

The only other option as I see it would be to mask interrupts; accept that the wifi is going to die during the ws2812 output and restart the CPU when done to recover the Wifi (storing any context in the RTC memory), but this restart would mean that you'd get a pretty crappy frame rate!

[matgoebl]

Fantastic results - very big thanx to @Alkorin 👍
I've been running a load test for over 26.9 hours now: 300 LEDs updated at about 69.4fps, with 24k tcp connections in between. Im on the dev branch with this ws2812.c by @Alkorin.
Using the default ws2812.c I got an reboot after a few seconds.

My test setup:
In the ESP side I have a telnethttpd.lua running and in addition I run the command:
counter=0 function count() counter=counter+1 ws2812.writergb(1,string.char(counter%256):rep(300*3)) tmr.alarm(6,1,0,count) end count()
On the unix side I run:
while sleep 1;do while sleep 1;do echo =counter|nc -w1 $ESPIP 8266;echo;done

I'd stronly suggest to provide this improved driver to our users.

[TerryE]

Given the enthusiastic feedback, I don't think that we should delay moving this into dev further. However we do need to do usual checks on backwards compatability, documentation, etc. @Alkorin Thomas, can I ask you to do this?

[marcelstoer]

Thomas, thanks a lot for all you efforts.

I don't think that we should delay moving this into dev further. However we do need to do usual checks on backwards compatability, documentation, etc.

For what it's worth I disagree with this procedure. The whole point of moving the docs back to the repository was to ensure consistency with the code. Hence, if PRs change API or behavior they should include doc updates before we merge them. Authors shouldn't think about backwards compatibility after code was merged but before. Right now we're no better than before because the documentation says X but the code does Y.
Sooner or later someone will report a "bug" claiming that ws2812.writegrb(pin, string) didn't work anymore. Not what we want; certainly not what I want.

[Alkorin]

I agree @marcelstoer!
That's why I opened #966 to discuss/fix this asap... !

[skycoders]

@Alkorin I saw that you are also working on an extended version of the ws2812 module which allows to send tables instead of strings. Maybe in the course of making the ws2812 module better, this could be included as well? I realized that building a 300 LEDs string by appending characters to a string is not working well, having a table and then create the string by concat works, but wastes memory. As the charming part of these strips is individual addressing, a table would be a great fit from my point of view. I'd love to see it :)

[TerryE]

@marcelstoer, I agree that this should be an absolute gate for hoisting from dev to master, but this thread is extremely active and the main WS2812 users are now actively participating. Why not just revert the change?

[TerryE]

@Alkorin, Incidentally if you agree that this shouldn't have been pulled, then why did you issue a PR in the first place? If you have a change that is not ready for being integrated, then just push it to you local fork and reference the commit in your issue discussions.

[Alkorin]

Hum, I could be wrong, but for me it's how PR works. As per github documentation, Pull requests let you tell others about changes you've pushed to a repository on GitHub. Once a pull request is sent, interested parties can review the set of changes, discuss potential modifications, and even push follow-up commits if necessary.

It's what I've done here, tell what I've done and discuss about the final road. Next time I can open an issue to discuss about it, but ... issues are not made to track code changes.

Anyway, we have to choose how we handle this compatibility change in #966 and we will close this asap :)

[TerryE]

Thomas, I am just getting moderator fatigue. Sorry for this. I clearly made a mistake. Marcel can sort this out. He has my blessing. Any work that I do on nodemcu over the next few weeks at least will relate to my own contributions.

[devsaurus]

Hum, I could be wrong, but for me it's how PR works.

As always, there are two sides of the same coin. The intention in this project is that design discussions and evolution of an implementation is handled in an issue and not in a PR. There's a related recommendation in CONTRIBUTION.md.
Personally, I made similar experiences like you did and took with me back then: Place a PR if I really want the code to be merged; now. But open an issue in case I need constructive feedback to polish the code towards a PR.

On to technical topics...
I've included this update in my project and found the following (maybe already reported somewhere else). After reset, the first call to ws2812.writergb() always switches the first LED to green. The next call works then as expected.

node.restart()
-- wait for reboot
-- all LEDs off

ws2812.writergb(2, string.char(0,0,0, 0,0,0, 0,0,0))
-- 1st LED is green

ws2812.writergb(2, string.char(0,0,0, 0,0,0, 0,0,0))
-- 1st LED is now off

Do you see the same?

[skycoders]

@devsaurus yes, this is the same here, and also noted by @matgoebl in the backwards-compat ticket #966. Something must be different with the first call

[Alkorin]

Will take a look this evening

[Alkorin]

Here seems to be the bug:

Pin is high at reboot... I will force it to 0 and wait 10µs (reset) before starting to stream. It should fix the problem.

[Alkorin]

@devsaurus I've pushed a fix on my branch