pimoroni / st7735-python

Python library to control an ST7735 TFT LCD display. Allows simple drawing on the display without installing a kernel module.
MIT License
58 stars 29 forks source link

[Feature Request] Untie from spidev #13

Open jrmoserbaltimore opened 3 years ago

jrmoserbaltimore commented 3 years ago

This library requires spidev, which is not available everywhere. It would be more broadly useful if it accepted an spi instance already created, with a read() and write() member, which is available in MicroPython. A client program may need to extend spidev with a write() member for this library to work, import spidev, create the SPI object themselves, and pass it to the constructor; but that would work, and so would using machine.SPI, and other things.

This is a breaking change. The dependency on spidev and the import line itself prevents the use of this library on the Pi Pico. There are already three ST7735 libraries, all based on the same code base. It's a trivial modification and I could send a PR, but this is a pretty major decision as it is a mandatory API break.

See work done at my fork. I have not finished updating the examples to use spidev or MicroPython objects, but notice in framerate.py:

if sys.implementation.name == "micropython":
    # Hardware SPI device
    spi = machine.SPI(0)
    sck = machine.Pin(18)
    smosi = machine.Pin(19)
    smiso = machine.pin(20)
    scs = machine.pin(21)
    spi.init(baudrate=SPI_SPEED_MHZ * 1000000, sck=sck, mosi=smosi, miso=smiso)
    dc = machine.Pin(2)
    backlight = machine.Pin(3)
else:
    # Set up the spidev device
    spi = spidev.SpiDev(0, ST7735.BG_SPI_CS_FRONT)
    spi.mode = 0
    spi.lsbfirst = False
    spi.max_speed_hz = SPI_SPEED_MHZ * 1000000
    # add the send() method as alias to xfer3()
    spi.send = spi.xfer3
    # gpiozero pins
    dc = OutputDevice(9)
    backlight = OutputDevice(19)
# Create ST7735 LCD display class.
disp = ST7735.ST7735(
    port=spi,
    dc=dc,
    backlight=backlight,               # 18 for back BG slot, 19 for front BG slot.
    rotation=90,
)

The above uses micropython's facilities or spidev. Because spidev uses xfer3() instead of send(), it makes spi.send() a pointer to spi.xfer3(), so the spidev SPI object behaves the same as the micropython one when called the same way.

This approach, again, removes gpio and spidev stuff from the library itself, so it is now portable to the Raspberry Pi Pico, but also runs on the Raspberry Pi Linux series using spidev, just as long as the client hands it a duck.

Gadgetoid commented 3 years ago

I think separating device drivers from their transports is a generally great idea for many reasons, but I can't spare the time to give this any proper attention at the moment. That said:

Right now our MicroPython approach is to build C++ drivers that can be used for Pico C++ and wrap those up for use in MicroPython, baking the result into our custom .uf2. I know this isn't an ideal approach, but it was the path of least resistance for supporting the two different paths of Pico. IE here's the ST7735 driver - https://github.com/pimoroni/pimoroni-pico/tree/breakouts-dev/drivers/st7735

Additionally since our st7735-python driver was written specifically for Linux/Raspberry Pi I'm apprehensive to make any changes that break the API. IE: existing Pi users should be able to continue using the library as-is.

Between this and other differences in MicroPython vs full-blown-Python I would probably approach this by writing a MicroPython-specific driver since it could:

  1. Include things like const and other considerations around memory usage
  2. Focus on MicroPython-only support to avoid contrivances, complications and accommodations for other usages which cause problems with 1.
  3. Be packaged up and distributed according to MicroPython's way of doing things, so it's easy for people to install/use
  4. Include a concise set of MicroPython-only examples that don't confuse users with additional setup code (code accessibility and simplicity is pretty important and we serve a lot of beginners)

Duet to different size and performance complications and different language features I'm not totally convinced trying to twist existing drivers to work with MicroPython is a feasible approach and I really hope to get the time and headspace to write MicroPython specific drivers for things in future. There's otherwise always going to be a compromise to make a driver that works in both environments.

At the risk of sounding like I want you to do my job my way for free (in fact I'm really keen to embrace the MicroPython ecosystem and do things properly but we're painted into a corner here), I'd encourage you to:

  1. Keep your forked changes and rename your repository to micropython-st7735
  2. Double down on MicroPython, discarding CPython compatibility
  3. Evaluate the use of const() and other MicroPython features to make it work better
  4. Ship this result as a MicroPython library - https://docs.micropython.org/en/latest/reference/packages.html - ie: micropython-st7735
jrmoserbaltimore commented 3 years ago

I figured breaking the API was iffy.

Not sure that's the right direction. Instead I'm looking at de-CircuitPythoning adafruit_rgb_display. There's nothing to gain around memory usage considerations, and not really anything to make it specific to micropython; the approach I'm using is instead:

Since this works the same way regardless of whether it's an RPi or MicroPython, the API becomes generic—albeit, again, incompatible with the current method of creating these objects within the class itself.

The setup as-is is the following process:

The setup I'm using is the following process:

In any case, I'll fork it off from Adafruit's library and make the small tweaks to detangle it from CircuitPython. There's no real gains (and not actually anything to do) to discard CPython compatibility for MicroPython; rather, the library as-is has discarded compatibility with anything that's not spidev, including native soft/hard SPI devices, and really could be generalized to UART and i2c because it only requires a send() method on the port object (not that all displays have that, but there's no reason to have separate base classes for most of them).

We'll see where I get with that.

Gadgetoid commented 3 years ago

Awesome, keen to see where you end up with it.

I'm broadly for decoupling drivers from their underlying transports for the portability benefits, plus it makes it easier to simulate hardware without weird shimming, or redirect I2C/SPI/UART to an external peripheral (like CircuitPython does with the USB IO expanders), but I also have to be wary of breaking APIs we already have customers/software using.

if I'd been more proactive about this in the first place, we wouldn't have such a mountain of work on our hands to make MicroPython drivers... hindsight is 20/20 eh!

Thanks for chiming in here, nonetheless.

jrmoserbaltimore commented 3 years ago

This is untested (I haven't received any of these devices yet), but I've forked adafruit_rgb_display and renamed it to rgb_display. You can see how I initialize SPI devices for various platforms and frameworks in one of the example files. It's possible to write a function that decides how to create the device and pins based on the platform, but that's for another module or the user to decide.

Interesting bits are on lines 52-60 and 71, where I added on() and off() methods to digitalio pins and made send() an alias of write() or xfer3() on the various types of SPI devices. Same deal if you have a chip that accepts I2C or UART: make sure the object you hand off has a send() function and is pointed to a valid port and it'll work, although the protocol might be different on different buses for the same device, so mind the datasheet.

It's also possible to from rgb_display import DisplayDevice and implement further display drivers as separate modules.

I'll play around with this when I get my hardware and see if I can get the examples to work on multiple platforms in this way. If it works, I'll figure on how to get it up on PyPI and you can point your customers at using that for MicroPython (or even on RPi). That'll avoid breaking the API here.