adafruit / Adafruit_CircuitPython_DotStar

Dotstarrin' away in CircuitPython land
MIT License
47 stars 38 forks source link

Add SK9822 support #29

Closed adammhaile closed 6 years ago

adammhaile commented 6 years ago

@kattni - As discussed at Maker Faire, this adds support for SK9822 LED strips.

SK9822 data sheet here: https://cpldcpu.files.wordpress.com/2016/12/sk9822-led-datasheet.pdf Much of the protocol tweaks in this PR from the work done here: https://cpldcpu.wordpress.com/2016/12/13/sk9822-a-clone-of-the-apa102/

TL;DR version: APA102 superimposes a 440Hz PWM on the 19kHz base PWM to control brightness. SK9822 uses a base 4.7kHz PWM but controls brightness with a controllable constant current source. Because of this SK9822 will have much less flicker at lower levels and is still suitable for POV applications while maintaining full color depth even while dimmed. The on-chip brightness control is identical as for the APA102, however the end frame must be switched to all zeros and with 32 bits extra to better handle the SK9822 while still working for the APA102.

I would certainly love to see Adafruit carry the SK9822 LEDs (DotStar Plus maybe?). Far superior in my opinion :)

Notes on my changes In my own usage of this combined protocol (I've done ports into both FastLED and my own python library) I didn't have the option to send a 4 value tuple to a pixel that included brightness. Specifically that in the case of this library, that 4th value is applied to the on-chip brightness level instead of internal scaling. So that made my approach here a little different in order to not change the API.

Because of the per-pixel brightness I implemented it such that when SK9822 mode is enabled (meaning brightness control is always on-chip and not locally scaled, the per-pixel brightness will override the global brightness for that individual pixel. My change simply updates the brightness byte for each pixel whenever the global brightness is changed. Mainly I did it this way to save CPU cycles by not doing it on every call to show(). But, because of that, the per-pixel brightness overrides global when in this mode.

As for the new param in __init__, I couldn't really decide what to call it, hence it just being called SK9822 at the moment. Open to suggestions. There's no reason this mode isn't valid for APA102 other than it may cause added flicker. But that may be fine for some applications. You'll still get the benefit of no loss of color-depth compared to local color scaling. So, maybe something more like on_chip_brightness=True?

Also, I fixed a bug I found with setting pixel color values using an int instead of tuple. Noted in an inline comment.

adammhaile commented 6 years ago

Ah, ok... lesson learned. I use flake8, you use pylint. So, I've cleaned up 2 of the 3 pylint failures but admit I'm not 100% about what to do with this one: adafruit_dotstar.py:49:0: R0902: Too many instance attributes (12/11) (too-many-instance-attributes)

Now, it's obviously because I added self._sk9822 as an instance attribute to the class. I found the limit in .pylintrc but don't want to just go increasing that value without understanding why you've chosen to limit it in the first place. (Though I assume it's to keep CircuitPython modules more tame than regular python modules.) And I don't currently have a great solution for creating that new mode without either:

So, I'll just hang back for now until I can get your opinion on how you would like things structured.

adammhaile commented 6 years ago

Removed fixed for int color values as to not steal the thunder from #28

kattni commented 6 years ago

@adammhaile Thanks so much for doing this! You were too quick for me :)

I want to test this myself and make sure it's working with our DotStars before merging. I also want to discuss with @tannewt the best way to go about this.

We have, when it makes sense, included a pylint: disable= in this case. I want to discuss the changes with Scott before making a decision as to whether a disable is warranted here.

Thanks again for the addition! I'll try to get to it this week.

adammhaile commented 6 years ago

No problem @kattni :) Regarding testing, I meant to mention I tested it with three APA102 and four SK9822 strips I had on hand. Though the only one from Adafruit was the DotStar Disk. Obviously, test away with your own, just wanted to note what I was able to test with. I imagine, aside from the disk, that all my suppliers are different.

Let me know what, if anything I can change to make Travis happy and I'll get right on it.

tannewt commented 6 years ago

I don't think this is the best way to do this because its not a DotStar. Instead, lets create a separate driver for the SK9822. I know there will be a lot of code duplication initially but we want to introduce a shared PixelBuffer to factor out the common parts. The PR for it is here: https://github.com/adafruit/circuitpython/pull/943

adammhaile commented 6 years ago

Ok, so should I just create and brand new module repo and have you fork it when ready to pull into CircuitPython? Not sure what the procedure for a completely new module is.

On Mon, Oct 1, 2018, 2:31 AM Scott Shawcroft notifications@github.com wrote:

I don't think this is the best way to do this because its not a DotStar. Instead, lets create a separate driver for the SK9822. I know there will be a lot of code duplication initially but we want to introduce a shared PixelBuffer to factor out the common parts. The PR for it is here: adafruit/circuitpython#943 https://github.com/adafruit/circuitpython/pull/943

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/adafruit/Adafruit_CircuitPython_DotStar/pull/29#issuecomment-425802509, or mute the thread https://github.com/notifications/unsubscribe-auth/AA6a6krbk_9GLwFgZ-5QsBWGzAkR8rcFks5ugbaxgaJpZM4W7tnG .

kattni commented 6 years ago

Thanks so much for doing this!

We've done it the way you suggested in the past, but in this case, since we're already involved, I'll create a new repo and then you can fork/clone it and populate it from there.

There's a whole process to creating a new driver, which we have outlined here: https://learn.adafruit.com/creating-and-sharing-a-circuitpython-library/overview Please take a look at the driver creation process guide and let me know if you have any questions.

You already seem to have Git and GitHub figured out, but if you have any questions about our workflow, feel free to ask here or on Discord, or check out this guide here: https://learn.adafruit.com/contribute-to-circuitpython-with-git-and-github/overview

I will leave this PR open until I've created the new repo, link it here, and then close the PR.

adammhaile commented 6 years ago

Cool. I've been reading up on your whole process but just wanted to confirm what you'd rather :) once the new repo is ready I'll get to work.

On Mon, Oct 1, 2018, 11:27 AM Kattni notifications@github.com wrote:

Thanks so much for doing this!

We've done it the way you suggested in the past, but in this case, since we're already involved, I'll create a new repo and then you can fork/clone it and populate it from there.

There's a whole process to creating a new driver, which we have outlined here:

https://learn.adafruit.com/creating-and-sharing-a-circuitpython-library/overview Please take a look at the driver creation process guide and let me know if you have any questions.

You already seem to have Git and GitHub figured out, but if you have any questions about our workflow, feel free to ask here or on Discord, or check out this guide here: https://learn.adafruit.com/contribute-to-circuitpython-with-git-and-github/overview

I will leave this PR open until I've created the new repo, link it here, and then close the PR.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/adafruit/Adafruit_CircuitPython_DotStar/pull/29#issuecomment-425950250, or mute the thread https://github.com/notifications/unsubscribe-auth/AA6a6h-p7MmzcRJIchxcejQPrq2NquHoks5ugjR2gaJpZM4W7tnG .

kattni commented 6 years ago

Alright! Here's the new repo: https://github.com/adafruit/Adafruit_CircuitPython_SK9822

We can start a discussion in an issue on that repo. Please create a new issue there with questions and we can go from there.