Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for the Nordic nRF52832 #133

Merged
merged 11 commits into from
Mar 31, 2017

Conversation

mxgxw
Copy link
Contributor

@mxgxw mxgxw commented Mar 20, 2017

-This pull request adds support for the nRF52832 MCU, it was tested on the Sparkfun nRF52832 breakout board, however it could possibly work on any other board based on the nRF52. The implementation is based on the Teensy examples but using the syntax and registers of the NRF52DK. You'll need to add support for the board from the following URL: https://raw.githubusercontent.com/sparkfun/Arduino_Boards/nrf5/IDE_Board_Manager/package_sparkfun_index.json

  • This implementation doesn't disable the interrupts when running the code. This is because enabling interrupts breaks the Bluetooth Softdevice. Currently we are working on an implementation that doesn't require disabling the interrupts. However, you can disable the interrupts using #define NRF5_DISABLE_INT. When interrupts are enabled you cannot use the Bluetooth softdevice. For more info check the comments on the lines 142 and 1210.

  • It was tested to work sucessfully with the "strandtest" example provided with the library. Also yo can see it working over https://www.instagram.com/teubico

@ladyada
Copy link
Member

ladyada commented Mar 20, 2017

thanks - we're actually in the middle of adding generic nrf52 support so may not merge this in particular but we'll get something going and maybe you can test it :) stay tuned!

@mxgxw mxgxw changed the title Added support for the Sparkfun nRF52832 Breakout Board Added support for the Nordic nRF52832 Mar 26, 2017
@mxgxw
Copy link
Contributor Author

mxgxw commented Mar 26, 2017

Hi @ladyada ! We just finished our implementation using the DMA and the PWM module. To summarize the changes made:

  1. This update adds generic support for any board that defines "NRF52" at compile time. Because this I changed the name of the Pull request to something more generic.

  2. This implementation is based on DMA transfers. It works converting the pixel buffer to a pattern stored in RAM that is then sent to the PWM device. Because it uses DMA it doesn't interfeers with the Bluetooth SoftDevice. You can find more details and the process used to set-up the DMA in the source code.

  3. We added an example that uses the NRF52 SoftDevice.

  4. We left the original implementation that uses the cycle counter from the ARM core just in case someone wants to use it and to document the process. We do not recommend to use this implementation. However if you feel brave enough and want to enable it you'll need to comment the line "#define NRF52_NEOPIXEL_USEDMA" on the file "Adafruit_NeoPixel.cpp".

  5. Using the DMA transfers allow us to use this library in a non-blocking fashion. However to make this pull compatible with the current API we have a while loop that waits for the event flag. However if you are interested we can add a call-back handler that would enable to use this lib in non-blocking contexts in a way that we dont break the current use cases of the library.

@mxgxw
Copy link
Contributor Author

mxgxw commented Mar 26, 2017

We added more detalied comments to run the StrandtestBLE example and also added a skip file for it. The current travis script doesn't include support for the NRF52 based boards. We'll wait for this support to be added to remove the skip file.

@microbuilder
Copy link
Contributor

This is an interesting approach, thanks for the PR. We'll test it out locally and let you know the results.

@mxgxw
Copy link
Contributor Author

mxgxw commented Mar 30, 2017

Hi @microbuilder, @ladyada ! I've just pushed the changes based on the comments from the discussion at: adafruit/Adafruit_nRF52_Arduino#21

To summarize the changes:

  • We added a small code to detect a free PWM device and if no PWM devices are available it will fall back to the DWT implementation.
  • This commit includes a little bit of code cleanup and a little bit of formatting to the comments to improve the documentation of the algorithm.
  • It must work on any nRF52 based board where NRF52 is defined on compilation.
  • This commit was not tested with RGBW LEDs however the algorithm should work without changes.
  • This code was not tested with the protocol running at 400KHz. However it should work. If not, it would only need small adjusts to the timing constants on the nRF52 section.

@hathach hathach changed the base branch from master to develop March 31, 2017 08:24
@microbuilder microbuilder merged commit f65f002 into adafruit:develop Mar 31, 2017
@microbuilder
Copy link
Contributor

I merged this into a new 'develop' branch for now so that it's easier to work on, and we can move it into Master shortly.

pixels_pattern[++pos] = 0 | (0x8000); // Seq end

// Enable the PWM
PWM[device]->ENABLE =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ENABLE should be set after PSEL.OUT

// operation we enable the interruption for the end of sequence
// and block the execution thread until the event flag is set by
// the peripheral.
PWM[device]->INTEN |=
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EVENTS_SEQEND[0] is generated without the need to set INTEN

// TODO: Check if disabling the device causes performance issues.
PWM[device]->ENABLE &=
~(PWM_ENABLE_ENABLE_Enabled << PWM_ENABLE_ENABLE_Pos);
PWM[device]->PSEL.OUT[0] &= ~(PWM_PSEL_OUT_CONNECT_Connected <<
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bit31 is set mean disconnected

@hathach
Copy link
Member

hathach commented Apr 14, 2017

Hi @mxgxw,

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

REV A
image

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

image

@hathach
Copy link
Member

hathach commented Apr 14, 2017

Hi @mxgxw ,

Thank you for the great pull request. We merged and add some modificaion 68a516d as follows

  1. Adjust the timing to the WS2818 Rev B which is 400us and 800us for T0H and T1H

https://github.com/adafruit/Adafruit_NeoPixel/blob/develop/Adafruit_NeoPixel.cpp#L1234

  1. pixels_pattern is malloc and free within .show() to remove the need to modify Adafruit_Neopixel header. In case PWM are all used up, this will save memory since it is not malloc in such as case. This potentially add dynamic issue, we will see if there is a need to revert to malloc in begin()

https://github.com/adafruit/Adafruit_NeoPixel/blob/develop/Adafruit_NeoPixel.cpp#L1275
Note: feather52 uses freeRTOS, it must use thread-safe malloc rtos_malloc

  1. Comment out INTEN since SEQEND will be set without this. PSEL should be configured before enabling PWM.

https://github.com/adafruit/Adafruit_NeoPixel/blob/develop/Adafruit_NeoPixel.cpp#L1371
https://github.com/adafruit/Adafruit_NeoPixel/blob/develop/Adafruit_NeoPixel.cpp#L1377

  1. Interrupt disable/enable is only executed when DWT is used. In case of Bluefruit52, it is critical section. Critical Section will mask out interrupts used by freeRTOS but leave SoftDevice as it is. I.e Radio event happens within SD, just not handled by application().

https://github.com/adafruit/Adafruit_NeoPixel/blob/develop/Adafruit_NeoPixel.cpp#L1412
Note: Critical Section is a must for Bluefruit52 to send data reliably

  1. Merge 400Khz with 800Khz using CYCLE_X00. This increase overhead a bit to access variable instead of immediate number. The result is a slightly smaller magic number of cycle. But we verified it on an scope, no worries.

https://github.com/adafruit/Adafruit_NeoPixel/blob/develop/Adafruit_NeoPixel.cpp#L1431

Please let us know if you have any suggestions regarding those modifications. If possible, please check out the merged code to see if it works with your board over there. Once it is confirmed, we will merge it to master and release it out.

Thank you again for the brilliant PWM pull request 👍

@hathach
Copy link
Member

hathach commented Apr 14, 2017

These are screenshot from my scope testing 800/400 Khz with both PWM and DWT implementation

800 KHz PWM ONE

800kz pwm one

800 KHz PWM ZERO

800kz pwm zero

800 KHz DWT ONE

800khz dwt one

800 KHz DWT ZERO

800khz dwt zero

@hathach
Copy link
Member

hathach commented Apr 14, 2017

400 KHz PWM ONE

400khz pwm one

400 KHz PWM ZERO

400khz pwm zero

400 KHz DWT ONE

400khz dwt one

400 KHz DWT ZERO
400khz dwt zero

@mxgxw
Copy link
Contributor Author

mxgxw commented Apr 15, 2017

Hi @hathach ! Great, I've just tested the changes and they appear to work perfectly! It's good to see that the final implementation only required minimum changes. We are really happy here that you found the code useful :)

@hathach
Copy link
Member

hathach commented Apr 18, 2017

Thank you very much, version 1.1.0 that incorporates your brilliant implementation is released 👍

@gpambrozio
Copy link

@mxgxw Thanks for the contribution. I have a qq. I'm using this with one strand and it works fine but if I add another neopixel object to control another strand using a different pin it seems to lock up during setup. Does the implementation support using more than one strip? I'm not using DWT, maybe using PWM is the issue? I appreciate any help, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants