rewrite the neopixel_send code to more elegant and faster asm.

[DaveAtKitronik]

Fixes #486 and additionally adds correct timing support for WS2812Mini. ASM based on WS2812 driver from PXT / Makecode Initial testing using attached test_file.py and various python games written for Kitronik :GAME ZIP64. Also tested on Kitronik :MOVE Servo:Lite board. Will be testing on Kitronik ZIP Halo today

[DaveAtKitronik]

Forgot to attach the test file test_file.py.txt

[AlasdairAtKitronik]

Update: Now also successfully tested on the Kitronik ZIP Halo

[AlasdairAtKitronik]

This is the latest test file used on all 3 Kitronik products test_file.py.txt

[dpgeorge]

Thanks for this! But the main issue would be:

ASM based on WS2812 driver from PXT / Makecode

We need to be careful about licensing here if the code is taken from another project. Is the code here substantially copied from PXT? If so what is the licensing conditions of code from PXT?

[DaveAtKitronik]

The PXT code is MIT and open. Its here: https://github.com/Microsoft/pxt-ws2812b The assembly code actually waggling the pins is substantially the same as in sendbuffer.asm, but the loading of the various registers etc is different.

[dpgeorge]

The PXT code is MIT and open. Its here: https://github.com/Microsoft/pxt-ws2812b

Thanks for the pointer. We would need to include their copyright.

The assembly code actually waggling the pins is substantially the same as in sendbuffer.asm, but the loading of the various registers etc is different.

It's going to get messy (with license/copyright, and general code structure) trying to merge that code with the existing source/lib/neopixel.c driver in this repo. I think the best way forward is to just discard the existing neopixel.c code completely and replace it with a pure assembler file based on sendbuffer.asm. All that is needed is just a single function that takes a buffer and outputs the data on the pin, which is what sendbuffer.asm does. The pin would be already configured, the ASM code just needs as input the pin pointer/mask, the buffer pointer and the buffer length. You've already pretty much done this in the PR but I think it would make things a lot cleaner if the code in this PR was factored into a pure ASM file (eg neopixelsend.s). It would need the PXT license header, and also would benefit from retaining the comments in sendbuffer.asm.

The rest of the functionality for setting/clearing pixels can go in source/microbit/modneopixel.cpp.

What do you think about this approach? I can help do the work on the source/microbit/modneopixel.cpp side.

[DaveAtKitronik]

The merge should be very simple. I have only touched 1 function in existing code. There are no new functions. All the existing APIs are the same. I have cleaned up the no longer used #defines in neopixel.h which we inserting the original assembler into the send function. MIT license only requires copyright inclusion for 'substantial portions'. whilst that is a judgement call I don't think that using around 30 lines of assembler as a basis for some new code is substantial. Even if it is why is including the words: 'portions of this code base on code provided under MIT license from Microsoft MIT License Copyright (c) Microsoft Corporation. All rights reserved. Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the "Software"), to deal in the Software without restriction, including without limitation the rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons to whom the Software is furnished to do so, subject to the following conditions: The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.'

messy? It sounds like you a proposing to throw away existing, functioning code and refactor the whole thing rather than include 11 lines of text in the comment portion at the head of neopixel.c ?

[dpgeorge]

It sounds like you a proposing to throw away existing, functioning code and refactor the whole thing rather than include 11 lines of text in the comment portion at the head of neopixel.c ?

Yes. And the reason is because the existing code is (mostly) a code drop of the upstream library found at https://github.com/lavallc/nrf51-neopixel . And this PR here is effectively replacing this driver with a different driver. The core part of the existing neopixel driver is the neopixel_show() function (the other functions are pretty trivial and a couple of them aren't even used), and this PR completely replaces this core function.

From a code maintenance point of view it is much cleaner to replace the existing driver with the PXT one. I'm happy to do the leg work to integrate the new driver.

[DaveAtKitronik]

Following a conversation with @carlosperate I have refactored the inline assembly into a separate .s file which is then an 'extern' from the neopixel.c file. I'll run some tests and then push it to GIT

[DaveAtKitronik]

Tested ok and pushed

[microbit-carlos]

First of all, thank you so much @DaveAtKitronik and @AlasdairAtKitronik for looking into the neopixel module, update it and test it to make it compatible with WS2812Minis!

@dpgeorge does the current refactor to the neopixelsend.s provide what you would need to easily refactor the current c module?

I think Kitronik has a micro:bit product already out with this type of neopixels and some resources they are ready to publish. So once we have something working on the master branch I can update http://python.microbit.org/v/beta to have something available for some beta testing (with an attached warning).

[dpgeorge]

Thanks @DaveAtKitronik that looks good! It's now both an improvement to the existing driver and it makes it possible to easily swap out the old driver for a new one based on the new ASM code.

@carlosperate I'll look at merging this tomorrow.

[microbit-carlos]

Fantastic, thanks @dpgeorge!

[dpgeorge]

Ok, I squashed and merged this PR in 4fc103da6e612569de53dbdb61edf843a3677320. I made some changes to white space (convert tabs to spaces and aligned things a bit) and put the full PXT license header in the neopixelsend.s file. But the actual code was merged unchanged.

Then in 1986322ca8239e54d14243392ade952e662e31ef I consolidated the neopixel code into the neopixel module/class to optimise things (see comment of that commit for more info), and removed the old neopixel driver in e10a5ffdbaf1cc40a82a665d79343c7b6b78d13b.

Space savings moving to the new ASM driver are 512 bytes of code, and an extra savings of 112 bytes code in consolidating the support code into the module and removing the old driver.

Great work @DaveAtKitronik, @AlasdairAtKitronik!