support Neopixel about adafruit_nrf52_arduino HOT 19 CLOSED

Hi @hathach ! We just implemented a DMA version of the NeoPixel library in the Hackerspace San Salvador. In our implementation we don't need to turn off the interrupts or enter the critical section. The reason is that the Bluetooth SoftDevice handles several interrupts and is not a good idea to disable them even for short periods of time. Also, the critical section code must be really short and the WS2811/12 is a protocol just too slow to be put in a critical section.

You can test our version that is available here: https://github.com/hackerspacesv/Adafruit_NeoPixel

Also, we already sent a pull request to the Adafruit team. Hopefully they will accept it or at least check out our implementation.

Regards,
Mario.

from adafruit_nrf52_arduino.

Hi @hathach !

Yes I think that someone using 12 PWM is a corner case but must be supported and a UI process must not interfere with wherever they are doing with the peripheral. Doing a quick lookup in google I found a similar Neopixel DMA implementation using the I2S device. However is a little bit more complex to set-up than the PWM equivalent.

I could propose:

1. The Neopixel library looks for a free PWM device and uses it as proposed by @hathach. However it would help if the current analogWrite method is implemented in a way that doesn't use the whole device and the two sequencers for just setting up one output.

2. If no PWM device is available, try to use the I2S implementation (I'm trying to contact the person who wrote it to see if he wants to include it on the library).

3. If 1 or 2 are not possible then default to the bit-bang method even if it has a few glitches.

Personally I'm not in favor of implementing #3 in the critical section because... Well, a couple of blinking pixels is not something "critical".

I think that may be less than 1% of the use cases would default to 3.

However this is only valid if we are talking about a blocking method (because you lock only one PWM device at the time even when using several LED stripes). With the guys in the hackerspace were thinking on a more complex implementation that would allow us to control several stripes in parallel by defining a call-back function at the end of the sequence. However this kind of more complex behaviour could require to set-up a "driver" that manages the PWM devices and just notify if your sequence can be allocated or if there is no more slots available. This driver however should be implemented on the platform side and not at the library side. (That's why this issue becomes really interesting from the API design perspective)

What do you think?

Regards,
Mario.

P.D.: Talking with the guys in the hackerspace we concluded that the I2S could be the first option because is a device not commonly used on Arduino. The downside of it is that we don't have as many simultaneous channels as we could have with the PWM device. However if supporting simultaneous non-blocking instances of neopixels is not something on the development roadmap for the Neopixels library then we just implement I2S and forgot this issue jejejeje

from adafruit_nrf52_arduino.

Actually, the more I think, the more I feel that we should keep it simple for the library. The chance that people use more than 8 PWM channels + neopixel and complaining about packet loss sometime is not really that high. Maybe we just implement PWM+bitbang first, since it could solve >90% of the requirement and simple enough. With your current implementation, we only need a few line of if() in correct place to glue the PWM and bit-bangging method. We could always add the I2S option later if people start to complain :) .

PS: When user uses all 3 modules, even if the 3rd module only use 2 channels, I would suggest to use bit-banging anyway. Since reconfigure the sequencer to both control neopixel and keep user PWM setting will cost 2x, 3x the memory and too troublesome I think.

PS2: I agree that the critical section for neopixel show() is overkill. But that is all I could think of as equivalent to nointerrupt(). We will test and see if We could remove that, and retry show(). Currently we will try to re-show() if got interrupted by SD by calculating the total time e.g >1.5 us for each bit (nominal is 1.25us)

https://github.com/adafruit/Adafruit_NeoPixel/blob/master/Adafruit_NeoPixel.cpp#L1397

@mxgxw feel free to correct me, since I am new to nrf52 as well :)

from adafruit_nrf52_arduino.

Great @hathach ! We are going to make the changes and test them today we have been busy in another project. I expect to have the PR ready for tomorrow. Lets see how it works!

from adafruit_nrf52_arduino.

I'll add this in since the timing is tricky and the SD can interrupt the writes.

from adafruit_nrf52_arduino.

ESP8266 turns off interrupts for a short amount of time, basically just have a 'max' number of pixels you can write

from adafruit_nrf52_arduino.

from adafruit_nrf52_arduino.

ToDo: Add Timeslot support for NeoPixels since this seems like the best long term solution, although the current setup we have is OK writing short chunks between radio events.

from adafruit_nrf52_arduino.

@mxgxw Superb !!! PWM with DMA is indeed a better implementation without involving CPU. We will test it and merge this approach. Though we need to make sure neopixel use of PWM won't affect other PWM's configure by user (analogWrite/analogWriteResolution).

PS: we are still working on other issue for 0.5.5 next week on nrf52. Please expect a bit of delay.

from adafruit_nrf52_arduino.

Thanks @hathach! Indeed, our NeoPixel implementation is in conflict with the current analogWrite implementation because analogWrite takes the all the four channels for the designed device. For this chip you could have up to 12 different PWM settings played by the six sequencers (two by peripheral).

I think that the best way of doing it is to keep track of the device/channel used when analogWrite is called(in the current implementation only the channel is tracked). This way when the NeoPixel library is called it can look out for a free PWM slot and reconfigure-it in a way that doesn't conflict with the current PWM designation. However, you will lose one PWM channel because we need to play with the sequencer and if the PWM pin shares the same sequencer then you need to generate a new pattern that contain both sequences and this could use a lot (more) of memory.

The way I see it you cannot have the cake and eat it too, if you want to use the 12 PWM outputs you will not have a way to use the Neopixels because you will have all the PWM devices busy. I can modify the current Neopixel implementation to try to interfeer the less as possible with analogWrite, however when someone calls analogWrite it will destroy the configuration for the Neopixels.

Let us think about it and make a proposal of how it could work through a pull request.

Regards,
Mario.

from adafruit_nrf52_arduino.

@mxgxw Normally people won't use that many of PWM channels (more than 8 to use all 3 PWM modules). But when they do, I think we've better not mess with their PWM configuration. They are possibly controlling multiple stepper motors in a coordination task. Even if we implement PWM registers push/pop inside neopixel .show(), those few millisecond interfere could cause a more severe problem. Neopixel is after all an UI whose priority should be lower than other critical task !!!!

I would suggest we combine PWM + DWT method (without using compiler macro)

1. Look for a available PWM module (disabled, SEQ.PTR == NULL, SEQ.CNT == 0) + there is enough memory for pixels_pattern ( ~1k for 100 pixel)
2. If (1) is not satisfied, we will use DWT as the fall-back. A few packet loss is not the end of the world, provided (1) is not satisfied.

Does it make any senses !

@ladyada @microbuilder do you think this approach is OK ?? If not I will try to find another way :)

from adafruit_nrf52_arduino.

@mxgxw that is an awesome idea to use I2S since it is much less common than PWM. However It is a bit complicated, and to be honest the 3 options (I2S, PWM, bit-banging) is a bit over engineered. And the limitation not controlling multiple instances of Neopixel strips does raise my concern though. @microbuilder what do you think ??

For PWM, we already addressed the issue you mentioned (1 channel use the whole module) in the develop branch by introducing HardwarePWM class. It will be merged shortly this week for next software release for Feather nrf52.

https://github.com/adafruit/Adafruit_nRF52_Arduino/blob/develop/cores/nRF5/HardwarePWM.h

(implement analogWrite() using HardwarePWM)
https://github.com/adafruit/Adafruit_nRF52_Arduino/blob/develop/cores/nRF5/wiring_analog.cpp#L65

It is still in early stage, without much of advance feature but use as least PWM module as possible.

from adafruit_nrf52_arduino.

@mxgxw I forgot the part about the callback with multiple strips. It is indeed need a bit difficult and need some set-up and out of the scope of library. In my opition, however, you can try to define your own class to inherit the NeoPixel class that acts as a collection class. It could take multiple strips, mutiple pins and define showAll() method. That will allow you to control up to 4 LED strip with only a PWM module ( if you could find one ), the tradeoff is of course more SRAM required !!!!

from adafruit_nrf52_arduino.

Hi @hathach ! I think we all are pretty new on the NRF52, we have learned a lot about this microcontroller in the last months.

Ok. Let me modify our implementation of the NeoPixel library to take in consideration the way that the PWM are assigned. I'm going to use your suggestion to detect the peripherals based on the registers and if there is no devices available it will fall back to bitbang. I expect that the new HardwarePWM class would be compatible with this implementation.

The way that the protocol works makes PWM the "natural" path to follow to write a driver for them. The DMA of the NRF52, even with it limited capabilities, appears to handle it well and I think the chip has enough RAM to handle even very long patterns. However you are right that trying to keep the current PWM settings would at least duplicate the amount of memory used.

When we tested our bitbang implementation the glitches were not a problem unless you want a high frame-rate. Actually the idea of sending again the full frame was something that we discussed but we decided that DMA was actually something feasible may be we can reach some middle ground that combines both solutions.

Mario.

from adafruit_nrf52_arduino.

Personally, I think PWM + DMA as the default option, with a fallback to DWT makes the most sense. I don't even know if I'd bother with bit-banging, though I guess it might be worth having as a third option. Honestly, I think PWM + DMA will work for 90% of people who likely won't use more than 8 PWM (4*2) in the real world anyway.

I2S is interesting, but I think there are better uses for I2S on this chip and it's probably worth reserving it for audio use. Just my opinion, though.

from adafruit_nrf52_arduino.

@mxgxw Great !!!!! thanks a lot. That would be awesome. We are looking forward to the next pull request. Don't worry about the feather nrf52 if you don't got one. We will merge it to a develop branch. Do a bit of testing and merge it to master when done.

PWM is such a brilliant idea. Although, I just messed up with the PWM, I couldn't think of that. You, guys must be doing something awesome there :)

PS: hardwarePWM is our repo modifications only, If you are using other core repo for nrf52 (e.g sandeep's), you may need to do some manual work yourself. Our core repo is too localized for feather52 and wouldn't pass requirement for pull request to original base :)

from adafruit_nrf52_arduino.

just make HardwarePWM ready for co-exist with Neopixel PWM. @mxgxw for detecting available PWM module. After re-reading the spec, I think just a simple check with

• ENABLE == 0
• all PSEL.OUT[0..3] is not connected ( highest bit is '1' )

Feel free to do any checks that you feel appropriate. It is just my opinion

from adafruit_nrf52_arduino.

no crush, we are busy with central API as well :)

from adafruit_nrf52_arduino.

Hi @mxgxw,

We are doing the merge. We notice that you are using the WS2812 (rev A) specs of timing.

REV A

As far as I know, since most of our current neopixel is using WS2812 revB, we will adjust the timing to rev B chip's. Luckily, both chips will work with either one since they are well in limit range of each other. So we have no worries at all.

REV B

https://cdn-shop.adafruit.com/datasheets/WS2812B.pdf

from adafruit_nrf52_arduino.