adafruit / Adafruit_CircuitPython_NeoPixel

CircuitPython drivers for neopixels.
MIT License
308 stars 99 forks source link

Update for friendly tuple support. #8

Closed ntoll closed 7 years ago

ntoll commented 7 years ago

This PR does several related things that fix backwards compatibility related problems in as "light touch" manner as possible (i.e. we retain all the awesome improvements you've made). :-) It fixes the issues highlighted in #7.

Discussion:

I notice you've implemented the update function as show and aliased write. I like how it's consistent with the micro:bit version of the neopixel module yet also backwards compatible. I believe this is a good thing (it's a more obvious name for such a function, especially if you're a beginner). This also suggests that you care about consistency and compatibility between implementations. Unfortunately, the awesome changes you've made (slicing, auto_write, yay!) also introduce other inconsistencies between different implementations of the module.

Ergo, this PR retains all the good stuff while also making this neopixel module consistently consistent with all the other neopixel modules. :-)

It also means that the code examples in my book are no longer knackered. :-P

As always, comments, constructive criticism and ideas are most welcome!

ladyada commented 7 years ago

@tannewt wanna check this one out or i can?

tannewt commented 7 years ago

All of the tuple readback changes look good to me.

I don't want to switch auto_show to False by default though. Its a huge win for beginners who are starting with this version because it clearly reinforces the connection between setting state and seeing state change.

Having this difference between neopixel libraries isn't a huge deal for those going between them because the behavior is almost identical. It'll be a little slower and maybe a little less perfect but its really hard to tell. Thats ok given the huge win it is.

So, please remove the auto_show changes and we should be good to go.

ntoll commented 7 years ago

Fantastic. I'll update the auto_show default as requested. Hurrah and many thanks for the fast turn-around. It'll save me lots of potential book related work. ;-)

tannewt commented 7 years ago

Thanks for fixing this up! I'll release a new bundle this week with it.