adafruit/Adafruit_CircuitPython_RGB_Display

DummyPin doesn't reflect the pin API

deshipu opened this issue · 17 comments

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'

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

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.

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?

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.

@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?

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.

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.

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".

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.

@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!

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

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

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

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.

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

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

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