search

adafruit / Adafruit_CircuitPython_CharLCD

Library code for character LCD interfacing
MIT License
69 stars 50 forks source link

enable non-PWM pins for backlight color, add kattni's pi-specific simpletest #16

Closed brennen closed 5 years ago

brennen commented 5 years ago

Rationale: pulseio isn't available on CPython Linux systems (i.e. the Pi), this mimics the design of the original Adafruit_Python_CharLCD.

Adds enable_pwm parameter (defaults to True) to Character_LCD_RGB constructor, and conditionalizes handling of the color pins on this value. If PWM is disabled, pins should be DigitalInOut instances.

It's entirely possible I've messed something up here, so close review is appreciated.

ladyada commented 5 years ago

rather than enabling pwm, check the attrs?

brennen commented 5 years ago

yeah, i wasn't super comfortable with the enable_pwm thing, but i'm also not sure what the idiomatic thing to do is here - isinstance on the pin objects? still feels sort of hacky...

ladyada commented 5 years ago

i know kattni's done a lot more of this with analog/pwm pins - i'll defer to her wisdom :D

kattni commented 5 years ago

I'm not super keen on this setup. We've discussed a couple of options such as creating a subclass for non-pwm pins or checking the object types (which also isn't great). Adding CircuitPython LIbrarians.

caternuson commented 5 years ago

Why no love for isinstance? Seems like an OK way to me.

brennen commented 5 years ago

i've gotten a "that's how you do that but you shouldn't want to do that" vibe every time i've googled this in the past, but if it doesn't raise alarm bells for people more steeped in Python Aesthetic than i am, i'll just go that route. threading a bunch of logic around that sort of type check can get weird, but i guess we're not dealing with all that complex a piece of code here...

sommersoft commented 5 years ago

I would say isinstance() too. Additionally, even if enable_pwm was kept, I would still check the objects to make sure they matched expected behavior (even though set_color() would still raise an exception).

brennen commented 5 years ago

wrote the isinstance() version, then did a bit more googling and realized what ladyada meant about checking the attrs.

caternuson commented 5 years ago

I like it. Do you need the check in __init__ at all? Just set direction to OUTPUT. The check in set_color takes care of it from there.

brennen commented 5 years ago

Do you need the check in init at all?

yeah, it at least needs to discern there between being a DigitalInOut or a PWMOut, since the latter doesn't have direction.

tannewt commented 5 years ago

hasattr looks good to me. 👍