adafruit / Adafruit_CircuitPython_RGB_Display

Drivers for RGB displays for Adafruit CircuitPython.
MIT License
132 stars 53 forks source link

DummyPin doesn't reflect the pin API #1

Closed deshipu closed 5 years ago

deshipu commented 7 years ago

Trying to use DummyPin for CS that I have pulled down permanently on this breakout (because Trinket M0 doesn't have enough pins to handle CS), I get:

>>> cs = DummyPin()
>>> display = st7735.ST7735(spi, cs=cs, dc=dc, rst=rst)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "adafruit_rgb_display/st7735.py", line 98, in __init__
  File "adafruit_rgb_display/rgb.py", line 104, in __init__
  File "adafruit_bus_device/spi_device.py", line 59, in __init__
AttributeError: 'DummyPin' object has no attribute 'switch_to_output'
ladyada commented 7 years ago

btw, you will be better off tying RST to Reset and controlling CS as the falling edge is used to detect command start on many displays

deshipu commented 7 years ago

Thanks for the tip, I will try that, though so far I didn't manage to make any of my displays work without a RST pin — it seems the beginning of the SPI transmission is tied to it.

deshipu commented 7 years ago

You were right, I can tie the RST pin to the board's reset, and that works too. Thank you for the tip.

It would probably still be nice to make the DummyPin work. I can make a pull request adding switch_to_output method to it?

deshipu commented 7 years ago

With some more testing, the trick with connecting the RST pin to the board's RST works on the HUZZAH Feather, but not on the M0 boards. I suspect it's because the HUZZAH has a pull-up resistor on that pin, and the other boards don't.

tannewt commented 6 years ago

@deshipu I'll review a PR to update DummyPin. Should it be broken out too?

On the other hand, is changing the lib to handle None better?

deshipu commented 6 years ago

Using a dummy pin simplifies the code considerably, letting us skip all those ifs in the sections that are critical for speed. Even if we only use it internally, replacing any None that is initially passed in with it.

Then again, I think a dummy pin object would be generally useful, and perhaps should exist outside of this library, to be used by other drivers too? Note that Arduino has something like this — you can always specify pin as 0, and then nothing happens.

tannewt commented 6 years ago

I'm not convinced a DummyPin is faster than extra if checks. An if is an extra byte code or two in the current function while a dummy pin leads to an extra method call.

I'm ok with it existing outside of this library if we need it for something else.

Just because Arduino allows it doesn't mean we should. I'd prefer to raise an exception when no pin is mistakenly provided instead of silently doing nothing. Of course, where it makes sense to have no pin we should allow it.

deshipu commented 6 years ago

I'm not convinced a DummyPin is faster than extra if checks.

You are probably right. It's still cleaner code.

I'd prefer to raise an exception when no pin is mistakenly provided instead of silently doing nothing.

That means that simply setting the reset pin to None by default and adding ifs everywhere is out of the question. With a dummy pin, you have to explicitly create and pass the dummy pin, so that's definitely not "silently doing nothing".

tannewt commented 6 years ago

Ok, I'm happy with whatever you decide. If we want to keep DummyPin we should move it to a separate library. Perhaps we want a whole host of placeholder objects.

kattni commented 5 years ago

@deshipu Is this still something you're interested in implementing? If it's something you'd still like to see, but are not planning on doing yourself, please unassign yourself from this issue so others know it's available. If you're no longer interested in it, consider closing the issue. Thanks!

deshipu commented 5 years ago

@kattni I'd like to do it, but this week I'm travelling, so it can take a while.

kattni commented 5 years ago

@deshipu No worries! I wanted to make sure this was still on someone's radar. We can leave it assigned to you, no problem!

makermelissa commented 5 years ago

Hi @deshipu, is this still something you wanted to work on?

deshipu commented 5 years ago

I completely forgot about it (again). I can make a pull request this weekend, but feel free to have a go at it if you want to. The current methods on the DummyPin are all wrong (they were good for MicroPython but not for CircuitPython), they should be removed and replaced with the methods that DisplayInOut actually uses.

makermelissa commented 5 years ago

Ok thanks for the information. I may give it a try tomorrow during the sprint if somebody else doesn’t pick it up first.

makermelissa commented 5 years ago

I'm going to continue this on the presumption that you meant DigitalInOut.

deshipu commented 5 years ago

Ah, sorry, yes, of course I meant DigitalInOut.