Bugs and improvements in WS2812B and streaming NRF52PWM

[kevinjwalters]

This is a concise summary of the likely bugs noted in lancaster-university/codal-microbit-v2#475 and originally reported in microbit-foundation/micropython-microbit-v2#227

Bugs

1. B1) Variables accessed from interrupt handler and normal code aren't marked volatile (identified by @finneyj)
2. B2) The blocking WS2812B::play() isn't fully blocking which means (atypical) code doing pixels.show() ; pixels.show() is likely to misbehave as the second execution of :neopixel_send_buffer() could overlap with data being sent by first one.
3. B3) WS2812B::pull sends one additional bogus pulse (noted by @martinwork in neopixels WS2812B PWM source reads one byte beyond the end of the data codal-microbit-v2#241)
4. B4) It looks like PWM.STOP = 1 fails very occasionally (~1 in 50000) despite passing the check while(PWM.EVENTS_STOPPED == 0);. Possible workarounds a) run it more than once and hope b) try a few stops if there's something which can be polled to indicate health c) after STOP set PWM.LOOP = 0 and turn off interrrupts after the stop and restore these just before the start of next bit PWMing d) set SHORTS to 0 before STOP like SDK does e) PWM.ENABLE = 0
5. B5 One or two PWM pulses are occasionally sent after emptyBuffer. Assuming this is due to small emptyBuffer and a late interrupt which occurs after that has been sent leaving old data in other buffer for PWM peripheral to unfortunately send. Larger emptyBuffer will reduce risk, E6 achieves same thing.
6. B6) Temporary statistics to look at behaviour of recent codal::neopixel_send_buffer() calls.
7. B7) DMESG printing of anomalies in statistics.
8. B8) Check/improve safety around malloc() failures in ManagedBuffer creation. Likely to need an enhancement in ManagedBuffer class in https://github.com/lancaster-university/codal-core.

Enhancements

1. E1) The code could be safer wrt negative values of dataReady (observed by @martinwork)
2. E2) An error flag in the PWM code would be useful for current and future debugging. It could be made accessible to the caller of pixels.show() somehow in the future.
3. E3) Replacing the current approach of buffer creation in the DataSink (WS2812B) with a pair of reused buffers made at the data preload stage ideally whilst preserving the current interfaces. Avoid explicit or implicit BufferInitialize::Zero as it's not needed. This follows the general principle of minimising code in interrupt handlers, avoids any risk of slow malloc() due to GC/other stuff, allows for handling of malloc() failures.
4. E4) Improve efficiency of WS2812B::pull to reduce risk of any slowness exceeding the time it takes to write buffer to wire (10240 cycles).
5. E5) Rather than use emptyBuffer (full of PWM zero DC values) fill the existing played buffer with these zero values if it's larger.
6. E6) Review priority of interrupts. Testing has shown that decreasing priority (increasing number) of capacitive touch interrupts improves the timing of events/interrupts in PWM code. Could be tempting to drop it to 2 as these interrupts only run for a few ms while pixel data is being sent.
7. E7) Look at options to free memory from the NRF52PWM pair of ManagedBuffers. Perhaps truncate(0) at end of transmission and enhance MangedBuffer to deallocate for this specific size.

Hygiene

1. H1 Use of setSampleRate() on NRF52PWM is redundant as value already set in constructor.
2. H2 Use of lock.wait() in WS2812B constructor is puzzling without a comment - better to create lock with 0 value.

Mysteries

1. M1 Why is microphone in use light on? Does this different between repro 14 and repro 24 code? (probably related to Microphone is permanently turned on (and LED on) when a programme never uses it codal-microbit-v2#499)
2. M2 Interrupts can appear to be too soon after start, for 128 sample buffer it should be 10240 cycles, 9500 and 7900 values are common. Perhaps there's more to measuring cycles than just DWT->CYCCNT?

Design

1. D1 Review the design here. Can two chained buffers be safely constantly filled in this pipeline for PWM at 800kHz with only 160us (10240 cycles) to fill the next buffer and in the context of loads of other stuff going on in MicroPython/CODAL.

Testing

Not under debugger, power-off micro:bit before test (including ZIP Halo HD power).

1. T1 Write a new test to check (visually?) that data is successful? Perhaps a colour change through 8 low brighness colours every second with different pixel counts which could run on multiple (synchronised) devices to look for a colour change whcih isn't applied across all.
2. T2 Run through the best of the reproduction tests, Any calculated (not measured) pixel.show() durations may need adjusting.
3. T3 Run the clock program to see if it runs without flicker and can run at least a week (needs USB power to LEDs).

---

All related tickets and discussions.

• Nordic Semiconductors: DevZone Forum: The finer points of PWM on nRF52 using two chained buffers
• Occasional corruption of neopixel wire data causing visual glitching codal-microbit-v2#475
• Rare glitches on WS2812B pixels using neopixel library on micro:bit V2 microbit-foundation/micropython-microbit-v2#227
• neopixels WS2812B PWM source reads one byte beyond the end of the data codal-microbit-v2#241
• periodUs incorrect due to int maths #58
• Improve efficiency of NRF52TouchSensor interrupt handler #60

[kevinjwalters]

I'm fiddling away in https://github.com/kevinjwalters/codal-nrf52/tree/kjwpwmfix and https://github.com/kevinjwalters/codal-core/tree/kjwpwmfix to see if I can address these.