Fastled: SparkFun Pro nRF52840 Mini + Neopixels locking up when calling FastLED.show() too quickly

[masonevans]

I have a new SparkFun Pro nRF52840 MIni and I'm trying to get it up and running with Neopixels + FastLED. I've got it most of the way there but I'm running into a small issue with things locking up when I call FastLED.show() too quickly if I have more than 32 LEDs set up.

To start, I'm using the Adafruit nRF52 board definition (v0.10.1 because FastLED won't compile against the newest, v0.11.1, probably a separate issue). I'm using the board definition and variant available here: https://github.com/sparkfun/nRF52840_Breakout_MDBT50Q/tree/master/Firmware/Arduino

I made one change to the board definition, they define it as an NRF52840_FEATHER but I don't think that's correct because the pinouts are different, so I changed the board definition to NRF52840_PCA10056 as that matches the pinouts on this board. I am running the data line off a pin which support high speed GPIO.

Everything seems to work ok if I'm trying to drive up to about 32 pixels. But when I get beyond that, the board locks up relatively quickly if I call FastLED.show() without a rate limit. When it locks up, the LEDs stop refreshing and it stops writing to the Serial console (although Bluetooth DOES continue to work, I can connect and send commands). If I wrap FastLED.show() in an EVERY_N_MILLISECONDS(3) call then it seems to run fine forever.

I've tried adjusting the delay how quickly show() is called. It doesn't seem to be related to the number of pixels - if it locks up it will lock up at 1 or 2ms delay and it works with a 3ms delay, no matter how many pixels there are.

I am able to reproduce the issue without having any Bluetooth stuff turned on. The below code runs fine forever at 32 pixels. At 33 pixels it locked up after about 10 minutes. At 34 pixels it locks up after about 10 seconds. At 64 pixels it locks up immediately. Wrapping the FastLED.show() call in EVERY_N_MILLISECONDS(3) will let it run fine forever with 64 pixels.

I'm powering the LEDs of the 3.3V of the board but it seems unlikely to be a power issue. Turning the brightness from 10 to 100 does not cause any issues with 64 pixels if there is a 3ms delay around the show() call.

Edit: it did eventually lock up on 32 pixels

#include <FastLED.h>

#define NUM_LEDS 48
#define LED_PIN 5


CRGB leds[NUM_LEDS];

void setup() {
  delay(2000);

  pinMode(LED_PIN, OUTPUT);

  FastLED.addLeds<NEOPIXEL, LED_PIN>(leds, NUM_LEDS);
  FastLED.setBrightness(10);
  fill_solid(leds, NUM_LEDS, CRGB::Blue);
  FastLED.show();
}

void loop() {
  Serial.println("Showing");
  static int i = 0;
  if(i) {
    fill_solid(leds, NUM_LEDS, CRGB::Blue);
    i = 0;
  } else {
    fill_solid(leds, NUM_LEDS, CRGB::Red);
    i = 1;
  }
  FastLED.show();
}

[henrygab]

@masonevans Well written bug report. This sounds like it will either be devilishly hard, or relatively simple, to track down. I will not be able to do a deep investigation until mid-August. However, your data points do suggest there is a mutual-exclusion issue. I'll see what a code review turns up...

Two questions:

1. Does the lockup occur even if the LEDs are entirely disconnected? (no power, no board connection)
2. Does this occur on many pins? (e.g., not only pin 5)

For my own reference: (( NEOPIXEL == WS2812 == clockless ))

[masonevans]

Does the lockup occur even if the LEDs are entirely disconnected? (no power, no board connection)

I just tried it, I disconnected everything from the board except for a USB cable so I could monitor the Serial output, and I see the same behavior. 64 pixels will lock up immediately, fewer will take longer to lock up, but the Serial output does eventually stop.

Does this occur on many pins? (e.g., not only pin 5)

Yes, I just tried a few others pins, even with the pixels disconnected, and they all exhibited the same behavior.

[henrygab]

Ok, that's very helpful:

• hardware not required to repro (so not likely to be power related)
• large LED arrays increase likelihood
• ultra-simple repro ino provided

It's probably an error in the mutual exclusion class, or I forgot a wait somewhere. I am still without my equipment for another week and a half, but this is top of mind.

[henrygab]

I'm going to use this post to store my ongoing notes, since I'm without my normal equipment for a while longer....

Importantly, the current clockless implementation does _not_ currently wait for the PWM hardware to be done sending the bits. Rather, it uses PWM hardware and an ISR with callback to indicate completion.

So far, the manual code review hasn't found an issue that is likely related to the hard-lock described.

Roadmap / Overview

• LED Hardware is not required to repro, so unlikely to be power-related.
• Larger LED arrays increase likelihood of repro
• SparkFun Pro's Arduino port 5 corresponds to nRF52 port 0.5
• Clockless on nRF52 uses the PWM DMA-based hardware

There are three important concepts in use:

1. Exclusive access to the sequence buffer used by PWM hardware:
   https://github.com/FastLED/FastLED/blob/13a9e0cc0999025b7ffdf92932bc6efb2199f8ea/platforms/arm/nrf52/clockless_arm_nrf52.h#L49
2. Exclusive access to use PWM hardware:
   https://github.com/FastLED/FastLED/blob/13a9e0cc0999025b7ffdf92932bc6efb2199f8ea/platforms/arm/nrf52/arbiter_nrf52.h#L37-L38
3. Ensuring that there's sufficient pause to cause clockless LEDs to latch the data: https://github.com/FastLED/FastLED/blob/13a9e0cc0999025b7ffdf92932bc6efb2199f8ea/platforms/arm/nrf52/clockless_arm_nrf52.h#L50

Sequence buffer exclusive access

showPixels() immediately infinite-loops until s_SequenceBufferInUse is zero:
https://github.com/FastLED/FastLED/blob/13a9e0cc0999025b7ffdf92932bc6efb2199f8ea/platforms/arm/nrf52/clockless_arm_nrf52.h#L162-L164

It sets this value to one in a cache-safe manner, on that same thread:
https://github.com/FastLED/FastLED/blob/13a9e0cc0999025b7ffdf92932bc6efb2199f8ea/platforms/arm/nrf52/clockless_arm_nrf52.h#L241-L246

And sets the value to zero in the ISR, when the PWM hardware stops:
https://github.com/FastLED/FastLED/blob/13a9e0cc0999025b7ffdf92932bc6efb2199f8ea/platforms/arm/nrf52/clockless_arm_nrf52.h#L134-L138

Based on the above, it does not appear that the sequence buffer could ever be used by more than a single in-progress showPixels() call. However, the ISR does (purposely) set this to zero prior to releasing the PWM class object, so there _is_ a timing window when the sequence buffer is not in use, but the PWM arbiter is still marked in use.

Does mWait properly ensuring wait time?

As a first item, showPixels() does attempt to ensure it's not called too rapidly:
https://github.com/FastLED/FastLED/blob/13a9e0cc0999025b7ffdf92932bc6efb2199f8ea/platforms/arm/nrf52/clockless_arm_nrf52.h#L162-L166

However, it never actually updates this value from the ISR.

This appears to be an _unrelated bug_?

PWM arbiter internals

PWM arbiter public APIs:

1. isAcquired() -- returns true if PWM acquired, without __sync
2. acquire( FASTLED_NRF52_PWM_INTERRUPT_HANDLER isr ) -- spins until success
3. tryAcquire( FASTLED_NRF52_PWM_INTERRUPT_HANDLER isr ) -- meat of acquire here
4. releaseFromIsr() -- owner calls to release
5. getPWM() -- used to expose the underlying pointer to the PWM peripheral
6. getIRQn() -- used to expose the IRQ for the underlying PWM peripheral

Default is to use PWM0, but all the IRQ handlers are identical:
https://github.com/FastLED/FastLED/blob/13a9e0cc0999025b7ffdf92932bc6efb2199f8ea/platforms.cpp#L20

The ISR callback defaults to NULL:
https://github.com/FastLED/FastLED/blob/13a9e0cc0999025b7ffdf92932bc6efb2199f8ea/platforms/arm/nrf52/arbiter_nrf52.h#L110

The ISR is set when the PWM arbiter is acquired successfully:
https://github.com/FastLED/FastLED/blob/13a9e0cc0999025b7ffdf92932bc6efb2199f8ea/platforms/arm/nrf52/arbiter_nrf52.h#L59-L66

Note: The ISR is NOT cleared when the PWM arbiter is released.... _consider changing this_?

In-depth review of arbiter's tryAcquire()

Per the GCC docs, the intrinsics used are considered a _full barrier_, so the compiler cannot optimize memory access across this call, and speculative loads / queued stores should also be disabled across this barrier. _((This is probably excessive, but that's better than a latent optimization-dependent bug...))_

__sync_fetch_and_or() performs the operation unconditionally, and returns the prior value. Here, it's logically OR'ing the lowest bit to ensure it's set to one. The prior value is returned and stored in oldValue:
https://github.com/FastLED/FastLED/blob/13a9e0cc0999025b7ffdf92932bc6efb2199f8ea/platforms/arm/nrf52/arbiter_nrf52.h#L60

If the old value did not have that bit set to one, then the function will succeed:
https://github.com/FastLED/FastLED/blob/13a9e0cc0999025b7ffdf92932bc6efb2199f8ea/platforms/arm/nrf52/arbiter_nrf52.h#L61

Prior to returning in the success path, the ISR is stored in the class for future use:
https://github.com/FastLED/FastLED/blob/13a9e0cc0999025b7ffdf92932bc6efb2199f8ea/platforms/arm/nrf52/arbiter_nrf52.h#L62-L63

In-depth review of arbiter's releaseFromIsr()

Per the GCC docs, the intrinsics used are considered a _full barrier_, so the compiler cannot optimize memory access across this call, and speculative loads / queued stores should also be disabled across this barrier. _((This is probably excessive, but that's better than a latent optimization-dependent bug...))_

__sync_fetch_and_and() performs the operation unconditionally, and returns the prior value. Here, it's logically AND'ing all except the lowest bit. The prior value is returned and stored in oldValue.

https://github.com/FastLED/FastLED/blob/13a9e0cc0999025b7ffdf92932bc6efb2199f8ea/platforms/arm/nrf52/arbiter_nrf52.h#L68

The prior value MUST be one, which indicates that the arbiter was already acquired.

nRF52840 shared peripheral ID?

This is based on the nRF52840 product specification, v1.1 (nRF52840_PS_v1.1.pdf).

The nRF52840 peripherals have an associated "Peripheral ID", which is directly related to the base address of the registers for controlling the peripheral. Some of the Peripheral IDs are shared. Only one peripheral can safely use a single peripheral ID at a time, because the registers may overlap or otherwise be shared.

The sample also used the Arduino Serial library. Since the PWM output occurs asynchronously, it's possible that both the serial port and the PWM were both in use at the same time.

However, the table on pages 23-24 shows the peripheral IDs of all peripherals. The UART0/UARTE0 peripherals are the only peripherals at ID 2. The PWM instances are each at distinct IDs that are not shared with any other peripheral (PWM0 == 28, PWM1 == 33, PWM2 == 34, PWM3 == 45). Therefore, the shared peripheral ID is not a concern for this issue.

nRF52840 events correct?

The PWM instances have the following events:

• STOPPED -- when PWM pulses are no longer generated
• SEQSTARTED[0] -- When first PWM period for SEQ0 starts
• SEQSTARTED[1] -- When first PWM period for SEQ1 starts
• SEQEND[0] -- at end of SEQ0, when last value from RAM applied
• SEQEND[1] -- at end of SEQ1, when last value from RAM applied
• PWMPERIODEND -- When _each_ PWM period ends
• LOOPSDONE -- when looping, when sequences played LOOP.CNT times (i.e., done)

The only event that is watched for is the STOPPED event:
https://github.com/FastLED/FastLED/blob/13a9e0cc0999025b7ffdf92932bc6efb2199f8ea/platforms/arm/nrf52/clockless_arm_nrf52.h#L134

This seems right?

nRF52840 spurious interrupt?

Per note on page 101, disabling or clearing an interrupt could take up to four clock cycles to take effect, due to the APB. To prevent the interrupt firing immediately, there must be at least four clock cycles between the disabling of the interrupt and exiting the ISR. It appears that the clockless isr_handler() function (the callback registered during acquire()) ensures at least four cycles pass:
https://github.com/FastLED/FastLED/blob/13a9e0cc0999025b7ffdf92932bc6efb2199f8ea/platforms/arm/nrf52/clockless_arm_nrf52.h#L147

TODO: verify compiler does not optimize away the no-ops?

Correct PWM configuration?

Starting PWM playback sets the buffers as being in use, and then setup of the PWM peripheral is broken into five functions:
https://github.com/FastLED/FastLED/blob/13a9e0cc0999025b7ffdf92932bc6efb2199f8ea/platforms/arm/nrf52/clockless_arm_nrf52.h#L246-L252

startPwmPlayback_InitializePinState() just sets the pin to output, and sets the initial state to high/low (based on _FLIP).

startPwmPlayback_InitializePwmInstance() ensures the PWM instance is in a known-good initial state. It ensures the output pin is configured for the peripheral, clears all shortcuts, disables interrupts, and ensures all events are cleared. Based on example at page 260 of the functional specification, it's correct to enable the PWM after setting the pins that will be used.

startPwmPlayback_ConfigurePwmSequence() sets up the sequence buffer, zero repeats, and no end delay, applying it to both sequence 0 and sequence 1, and ensures zero loops. In other words, a one-shot play of the sequence.

startPwmPlayback_EnableInterruptsAndShortcuts() sets up the shortcuts to ensure that, when either SEQ0 or SEQ1 are done, (or loop.cnt loops are done) the peripheral will immediately self-set the STOPPED task. This is redundant, but functionally correct. The use of a shortcut is necessary to avoid an (up to) 4-cycle delay from interrupt handler to actually stopping the PWM peripheral (due to potential delay in memory updates).

The code also enables interrupts for SEQEND0, SEQEND1, LOOPSDONE, and STOPPED. This is also a bit redundant, but functionally correct.

startPwmPlayback_StartTask() simply triggers the task that starts SEQ0, and thus causes the PWM peripheral to output the clockless control signal.

This looks correct?

[henrygab]

Update:

1. I have locally reproduced the issue, using adafruit's nrf52840 feather express.
2. If set to 40 LEDs, the repro locks up after ~ 0x2000 iterations of showLed().
3. If set to 80 LEDs, the code fails substantially immediately, leaving the NEOPIXEL a solid green... which is really odd and suggests more than one issue. (I've not put a scope on this yet.)
4. If I fix the _(unrelated?)_ CMinWait<> issue, by calling .mark() in the ISR, then the 40 LED strand continues to run overnight. However, this _may_ just be hiding the issue rather than fixing it.

Next steps:

1. Get scope trace from 80-LED strand
2. Add infinite loops where runtime assertions are noted as comments in code
3. Use J-Link to see state of locked-up system

If the above does not resolve the issue, then I can change the code to be synchronous, and not return from showLeds() until the PWM peripheral is finished. This is no worse than most chipsets, but I would prefer to avoid this....

[masonevans]

@henrygab - I can confirm that I have also see the "green first pixel" issue. I have noticed that doing something like immediately filling the strip in my setup() function will cause the first pixel to be colored green. From what I can tell, the next time I do anything with the strip the first pixel gets updated to the correct color and that issue doesn't come up again.

[henrygab]

@masonevans Potentially very good news....

I have found potential cause(s) and have a private branch that appears (_at least preliminarily_) to fix this problem.

The long road...

After the manual reviews listed earlier didn't shed light on the problem, and because some of my equipment is still not setup, I needed more data from a solid repro.

As I have a J-Link, I used Segger's SysView, which AdaFruit appears to have provided built-in support for on the nRF52. So I added custom events in SysView.

This was not as simple as it should have been. Pain points include:

• Documentation on how to add a module is great, in the same way a jumble of jigsaw puzzle pieces is a picture. Each piece exists, but how they fit together is unclear.
• Documentation presumes use of Segger Embedded Studio, which doesn't translate well to Arduino.
• Custom events … The SystemView tool itself wants files to be written to the app installation directory, which is not writable on Windows. Luckily, there's a way to provide the format strings programmatically when registering a module.
• The SYSVIEW API limits string length to 128 bytes (by default). This is not normally a real issue. Except that this limit is also applied to strings used when registering a module … which is the way that format strings are registered for the module. Therefore, *ALL module information and format strings must (combined) be under 128 bytes.
• If too many events defined, the output wouldn't be formatted properly (silently failing).
• If defined events fired too often, events would be lost.

Eventually I got lucky: a build happened to have just the right events and a minimal repro, suggesting what close to the "last thing" was that happened, just before the hard-lock....

When does the hard-lock occur?

The following sequence of events caused a repro:

1. showPixels() is called first time, starts the PWM peripheral on the task, and returns
2. showPixels() is called a second time, and stalls because the PWM peripheral is still sending bits across the wire
3. the PWM peripheral finishes and thus the interrupt fires, and the ISR frees the PWM arbiter and sequence buffers for re-use.
4. When the ISR exits, it goes back to the loop (the second showPixels() call) … and then nothing happens.

Potential cause?

I really wish it was some complex, hardware-based bug. It now seems much more likely a combination of a few things.

The following may be the minimum related items:

1. I failed to mark the s_SequenceBufferInUse variable as volatile:
   https://github.com/FastLED/FastLED/blob/13a9e0cc0999025b7ffdf92932bc6efb2199f8ea/platforms/arm/nrf52/clockless_arm_nrf52.h#L49
   I knew it should be volatile. This is a classic example of when volatile is necessary:
   https://github.com/FastLED/FastLED/blob/13a9e0cc0999025b7ffdf92932bc6efb2199f8ea/platforms/arm/nrf52/clockless_arm_nrf52.h#L162-L164

1. I failed to call mWait.mark() in the ISR handler (potentially failing to allow latching of data before another update is sent).
   https://github.com/FastLED/FastLED/blob/13a9e0cc0999025b7ffdf92932bc6efb2199f8ea/platforms/arm/nrf52/clockless_arm_nrf52.h#L134

2. The order of operations in the ISR may need tweaking.

3. changes to s_SequenceBufferInUse may need memory barrier semantics. I can't be sure, but I'm going to add full barrier semantics here anyways.

Anyways, I have a branch that has run for over an hour with 140 LED strands (none actually attached, but it's still running....). I am looking to reduce / minimize the code changes needed, before generating a PR.

Let me know if you want early access and can test from either / both the dirty dev branch, or the cleaner PR branch before I submit.

[henrygab]

https://github.com/henrygab/FastLED/tree/MinFix840

@masonevans I think this is the minimal fix needed.
I've constrained the changes to a single file … clockless_arm_nrf52.h.

Are you willing to give it a go to verify it resolves this issue for you?

[masonevans]

@henrygab Just tested this out and it seems promising so far. I let it run for a bit with 64 LEDs configured but without any LEDs attached and that ran fine. I'm now letting it run with a strip plugged in with 60 LEDs (all I have handy at the moment) and it's been running for about 15 minutes with no lock ups.

I'll let it run for a few hours and leave a comment if it locks up.

[henrygab]

@masonevans I'm thrilled to hear that -- awesome!

Can you please give positive confirmation if your setup is still running after at least eight hours? I'm also running this set to 140 LEDs (none actually connected). Since both previously reproduced the issue essentially immediately, having those two data points in combination with the SysView trace will provide extremely high confidence in the fix.

[masonevans]

@henrygab Running for 10 hours with no issues now

[henrygab]

and nearly 16 hours here, I think it's about time for a PR.

@masonevans : Thank you for your patience and clear/minimal repro. 👍

[masonevans]

@henrygab I'm at about 20 hours now as well with no problems. Thanks for having a look at this!