Skip to content

Updates to pixelbuf API - Addresses #884 #2034

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

Merged
merged 55 commits into from
Jan 3, 2020

Conversation

rhooper
Copy link

@rhooper rhooper commented Aug 3, 2019

This work-in-progress still requires some thorough testing. It switches over _pixelbuf to the same string based API as pypixelbuf.

Early feedback would be highly welcome, @dhalbert @tannewt

Also thoughts on what a good test suite looks like would help me.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Looking good here! Just a couple comments. I'm a bit torn about having the constants for every order still but it does give us the ability to change it later.

@ladyada
Copy link
Member

ladyada commented Dec 30, 2019

@rhooper hi - what are ya currently blocking on for this PR - we can get some help to wrap up this PR.

@rhooper
Copy link
Author

rhooper commented Dec 30, 2019

@ladyada

From the top of my head, these are the remaining tasks:

  • Free up time somehow!
  • Sort out making it so native method calls can call python subclass overrides (specifically so the native .fill() can call .show() on the python subclass). This is needed for auto_write to work properly with the native .fill(). We could skip native .fill() for now.
  • Finish writing pypixelbuf so that implementations without pixelbuf work
  • Test and merge the neopixel changes that work with pixelbuf (and pypixelbuf)
  • Test and merge adafruit_dotstar changes that work with pixelbuf (and pypixelbuf)
  • Test Test Test
  • Update which neopixel and dotstar libs are frozen in

@ladyada
Copy link
Member

ladyada commented Dec 30, 2019

@rhooper would it be ok for other folks to implement/test some of these TODOs?

@rhooper
Copy link
Author

rhooper commented Dec 30, 2019

@ladyada absolutely fine with me. the more people that know the changes the better!
all i’d ask is that work be coordinated so i don’t spend time doing something someone else is working on.

@ladyada
Copy link
Member

ladyada commented Dec 30, 2019

sure, they'll post here in this thread

@rhooper rhooper changed the title WIP: Switch to the new string based pixelbuf API WIP: Switch to the new string based pixelbuf API - Addresses #884 Jan 1, 2020
@rhooper
Copy link
Author

rhooper commented Jan 1, 2020

I worked around the native .fill() method being unable to call .show() on subclasses using a helper function in _pixelbuf instead.

@rhooper
Copy link
Author

rhooper commented Jan 1, 2020

Dotstar and Neopixel changes appear to work, but I need some assistance gathering up examples to test compatibility.

adafruit/Adafruit_CircuitPython_NeoPixel#59
adafruit/Adafruit_CircuitPython_DotStar#42

@rhooper rhooper changed the title WIP: Switch to the new string based pixelbuf API - Addresses #884 Updates to pixelbuf API - Addresses #884 Jan 2, 2020
@rhooper
Copy link
Author

rhooper commented Jan 2, 2020

Pypixelbuf has had some testing done - seems to work - but needs the same kind of testing as the Dotstar and NeoPixel PR. It also needs putting in pypi so that the Dotstar and Neopixel builds can pass.

adafruit/Adafruit_CircuitPython_Pypixelbuf#3

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Really really close. The show stuff looks fine except the name. Another questions about unused param but good otherwise.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

This looks good to me! Thank you for your incredible persistence. @dhalbert you will need to approve as well or at least dismiss your review.

@dhalbert dhalbert requested review from tannewt and dhalbert January 3, 2020 22:18
Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

👍 Thank you for all this work!!

@dhalbert dhalbert removed the request for review from tannewt January 3, 2020 22:18
@dhalbert dhalbert merged commit fc5f776 into adafruit:master Jan 3, 2020
@ladyada
Copy link
Member

ladyada commented Jan 3, 2020

💯 💯 🔥 🏅 💯 💯 to everyone who helped get this over the finish line - you are heros! many glowy projects will thank u :)

@siddacious
Copy link

:🎉🎉🎉Whoooo🎉🎉🎉🥳

Awesome contribution, thank you! Much glow will be had

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

Successfully merging this pull request may close these issues.

5 participants