Skip to content
This repository has been archived by the owner on Apr 20, 2022. It is now read-only.

Encapsulate buffer management into PixelBuf. #10

Closed
wants to merge 3 commits into from

Conversation

tannewt
Copy link
Member

@tannewt tannewt commented Jan 23, 2020

Also provide show() and fill() on the super class.

This is compatible with the previous PixelBuf API by accepting buf and
rawbuf in the constructor, allowing show() to be shadowed and providing
fill at the top level.

Libraries should not release based on the new API until the built-in _pixelbuf is also updated.

Also provide show() and fill() on the super class.

This is compatible with the previous PixelBuf API by accepting buf and
rawbuf in the constructor, allowing show() to be shadowed and providing
fill at the top level.
@caternuson
Copy link
Contributor

Looks good.

I also did a local update of NeoPixel_SPI based on these changes to test it out. Works as expected. I did not test anything related to DotStar.

I'll let you deal with the linter in whatever way best suits you. :)

@caternuson
Copy link
Contributor

Not part of your PR, but made me look - what's the purpose of called in the example?

@tannewt
Copy link
Member Author

tannewt commented Jan 27, 2020

@caternuson It's there to ensure that _transfer is called by the PixelBuf implementation. It is a bit tricky to do in the native code.

@tannewt
Copy link
Member Author

tannewt commented Apr 25, 2020

@dunkmann00 Just realized I had done this too. Anything here that would be interesting to move into your code?

@dunkmann00
Copy link
Contributor

dunkmann00 commented Apr 25, 2020

I actually noticed this this morning...really wish I had checked the pull requests before I made one, could've definitely saved myself some time!

Our changes were very similar, the one thing we went about differently was you used a memoryview to handle separating the color data from the header/footer in the buffer. I used the code that was there, which just stored the offset value and always performed the operations taking that into account.

So, if you prefer the use of the memoryview I could change that, but otherwise it looks like we both had very similar end products.

@tannewt
Copy link
Member Author

tannewt commented Apr 27, 2020

@dunkmann00 Either way is fine for me. Thanks again for picking this up!

@tannewt tannewt closed this Apr 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants