-
Notifications
You must be signed in to change notification settings - Fork 39
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
Add SK9822 support #29
Conversation
adafruit_dotstar.py
Outdated
@@ -157,7 +164,7 @@ def _set_item(self, index, value): | |||
if isinstance(value, int): | |||
rgb = (value >> 16, (value >> 8) & 0xff, value & 0xff) | |||
|
|||
if len(value) == 4: | |||
if len(rgb) == 4: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If value
was int
this fails with:
TypeError: object of type 'int' has no len()
When using int
color value instead of tuple
.
Looking at the code it seemed clear this was supposed to be rgb
all along as this would never work with an int
value.
9a15cfc
to
a20c4ca
Compare
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: Now, it's obviously because I added
So, I'll just hang back for now until I can get your opinion on how you would like things structured. |
a20c4ca
to
266da29
Compare
Removed fixed for |
@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 Thanks again for the addition! I'll try to get to it this week. |
No problem @kattni :) Let me know what, if anything I can change to make Travis happy and I'll get right on it. |
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 |
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 ***@***.***> 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
<adafruit/circuitpython#943>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#29 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA6a6krbk_9GLwFgZ-5QsBWGzAkR8rcFks5ugbaxgaJpZM4W7tnG>
.
|
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: 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. |
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 ***@***.***> 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
<#29 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA6a6h-p7MmzcRJIchxcejQPrq2NquHoks5ugjR2gaJpZM4W7tnG>
.
|
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. |
@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 calledSK9822
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 likeon_chip_brightness=True
?Also, I fixed a bug I found with setting pixel color values using anint
instead oftuple
. Noted in an inline comment.