Pins re-factor for remote SPI support

[waveform80]

This is turning out to be a pretty hefty PR, so I figured before I get too deep into it I should note down some of the goals of this to make sure everyone's happy with them. So, here's the goals:

• At the moment our software SPI support (regardless of pin implementation) constructs pins and bit-bangs on them. This is all well and good for all the strictly local implementations, but it's no good for remote pins in pigpiod because the latency involved ruins the SPI bandwidth (to the point where stuff starts malfunctioning).
• Now, pigpiod has built-in SPI bit-banging support so instead of constructing a bunch of PiGPIOPin instances, we should be constructing something that looks like SPISoftwareInterface (or SPIHardwareInterface) but which uses pigpiod's various SPI methods.
• This makes SPI pin implementation specific which means it needs to become part of the pin implementation itself.
• Something very similar to this is going to be required in the near future for I2C support (again, pigpiod supports this remotely).

Here's how I'm going about implementing this:

• The pin implementations are already too complex (messing around with __new__) and I don't fancy adding yet more class methods. The sensible thing to do here is to make an actual Factory instance with methods for returning a pin, an SPI interface, and probably in future an I2C interface (maybe 1-wire too eventually).
• This means that pin_factory will change from being a Pin sub-class to being an actual Factory instance.
• I'll also be introducing an abstract base SPI class. SPISoftwareInterface and SPIHardwareInterface will descend from this, and instances of these will be returned by the factories associated with local pin implementations (i.e. everything except pigpiod).
• Each pin implementation will wind up implementing at least two things: a Pin subclass and a Factory subclass, and optionally an SPI sub-class (as mentioned above, local pin factories will all use SPISoftwareInterface wrapping their respective pin subclasses, or SPIHardwareInterface).

Finally, this is what this means for end users:

• Because pin_factory is constructed up-front on library import, this implies that initialization of the pin factory will happen at import time rather than at time of first pin construction. This doesn't sound too important but it actually has a big impact on non-pi users as it means the NativeFactory tries to initialize straight away (and usually fails because neither /dev/gpiomem nor /dev/mem is accessible).
• This means on a PC you have to either setup the pigpio client properly or explicitly specify the mock factory to import gpiozero successfully; trying to import something that will only work on a Pi will fail at import time rather than the first time you try and use a pin.
• Because pin_factory is no longer a pin class I've taken the liberty of renaming the possible keys that can be used with GPIOZERO_PIN_FACTORY. They're now just named after the respective libraries and are case-insensitive. This is backward incompatible, but we haven't formally documented this yet and the new system is much friendlier: GPIOZERO_PIN_FACTORY=rpigpio.
• When a pin factory is not explicitly specified (via GPIOZERO_PIN_FACTORY) and the default ordering is attempted, any pin factories that fail (necessitating fallback) will fire a warning. This helps guard against people trying GPIO Zero in a virtualenv and forgetting to install RPi.GPIO.

Current state:

The basic coding work is done and works pretty well on a Pi. However, the vast majority of the test suite is quite broken because most references to MockPin (et al) need replacing with pin_factory (the test environment is configured by conftest prior to any attempt to import gpiozero to ensure it'll work on all platforms, sub-units will override pin_factory as required).

So, I anticipate this'll eventually be a mammoth patch, but the vast majority of it is going to be changes to the test suite (the actual coding work isn't that heavy - only really hits the pin implementations).

[lurch]

All sounds pretty reasonable to me 👍

the vast majority of it is going to be changes to the test suite

Rather than having all the unit-tests explicitly constructing MockPins, would it "make sense" to just have them constructing numbered pins, and then altering the Makefile so that 'make test' runs GPIOZERO_PIN_FACTORY=mockpin ... (that may well be a stupid question as I've not looked at the unit-tests for a while - and it might not 'fit' with Travis)

I guess if the allocating-of-pins to various interfaces (Pin/SPI/I2C) is being managed by Factory instances, maybe you could also have a method to tell the Factory to 'reserve' certain Pins? E.g. perhaps you have an addon board attached to some pins (managed by a different piece of software), and you want to tell GpioZero to never try allocating those pins. Not sure how that'd work with pigpiod and remote Pis though, so perhaps the 'reserved pins' should only be supported by strictly local pin implementations (rpigpio, rpio & nativepin) ?

While you're messing about with Pins, is it worth thinking about #385 at the same time too, or is that an orthogonal problem?

This means on a PC you have to either setup the pigpio client properly or explicitly specify the mock factory to import gpiozero successfully

Would this work?

import os
os.environ['GPIOZERO_PIN_FACTORY'] = "mockpin"
from gpiozero import *

[waveform80]

Rather than having all the unit-tests explicitly constructing MockPins, would it "make sense" to just have them constructing numbered pins, and then altering the Makefile so that 'make test' runs GPIOZERO_PIN_FACTORY=mockpin ... (that may well be a stupid question as I've not looked at the unit-tests for a while - and it might not 'fit' with Travis)

That's more or less what I'm intending to do. Just to get the test suite going again (to make sure I haven't broken too much) I've just done a bulk replace of MockPin(n) with pin_factory.pin(n) for now, and after a few tweaks I got all the tests working again. Of course, I've just broken all the SPI tests now I'm shifting everything over to pin_factory.spi(**args).

Ultimately I'll try to replace as much as I can with just straight pin references as that'll be much cleaner.

I guess if the allocating-of-pins to various interfaces (Pin/SPI/I2C) is being managed by Factory instances, maybe you could also have a method to tell the Factory to 'reserve' certain Pins? E.g. perhaps you have an addon board attached to some pins (managed by a different piece of software), and you want to tell GpioZero to never try allocating those pins. Not sure how that'd work with pigpiod and remote Pis though, so perhaps the 'reserved pins' should only be supported by strictly local pin implementations (rpigpio, rpio & nativepin) ?

Good thought. I'm discovering having the factory instance makes the code much cleaner. No more messing around with __new__, just a simple instance method which checks an internal "pins" cache. The Factory.close method (another ordinary method) handles pin clean-up at shutdown time ... it's all looking much nicer. I'm hoping this might also fix the issues with SPI being able to reserve pins properly - haven't got to that bit just yet, but that's my goal.

While you're messing about with Pins, is it worth thinking about #385 at the same time too, or is that an orthogonal problem?

Actually I've got another private branch where I was working on fixing that. However, it's almost certainly broken with all these changes. I'm seriously considering merging it into here though, because my goal is to make this the last backwards incompatible change in pins (i.e. after this the goal is only to add to pins with stuff like I2C).

Would this work?

import os
os.environ['GPIOZERO_PIN_FACTORY'] = "mockpin"
from gpiozero import *

Yes, that will work - and I know because that's almost precisely what conftest.py now does :). However it's not "mockpin", but "mock". I dropped the pin suffix because firstly the factory isn't a pin instance anymore (so that'd be misleading), and secondly this means the factory label is now just the library name (like "rpio", "pigpio", "native", etc.)

[bennuttall]

All looks good to me. Thanks Dave!

[waveform80]

No prob, given everyone sounds basically happy I'll press on. The SPI stuff's coming together right now (but the tests are all horribly broken). Might have something to push by tomorrow afternoon I think.

[waveform80]

Incidentally because this changes the names of the package entry points, when you want to test the PR that's eventually pushed you must run python setup.py develop before doing so (otherwise the new entry-points won't be registered and everything will fail horribly).

[lurch]

my goal is to make this the last backwards incompatible change in pins

Does that mean this latest refactoring would still work when you add IO-expanders, etc. ? ;-)
I guess they'd just be another Factory object? (which I guess implies that not all Factories will be able to construct SPI or I2C interfaces)

[waveform80]

Does that mean this latest refactoring would still work when you add IO-expanders, etc. ? ;-)

That's the plan.

I guess they'd just be another Factory object? (which I guess implies that not all Factories will be able to construct SPI or I2C interfaces)

Whether they form full blown factories that can be placed in pin_factory ... I'm not sure on that yet. The pin factory also contains the "pi_info" interface which really doesn't apply to expanders. Not wholly sure what to do about that yet so for now I've left it more or less as is (i.e. pi_info migrates to Factory, and that's about it). I might introduce a PiFactory sub-class (i.e. a factory specifically for pins on a Pi) which holds the pi_info interface. There's already a LocalFactory sub-class which more or less replaces LocalPin.

However, I have planned for the concept that some factories may not be capable of constructing certain interfaces. I've added some custom exceptions for these cases and they appear in the base Factory class. Factories are even permitted to be incapable of constructing pins (imagine a chip which purely provides multiplexed I2C interfaces, but no GPIOs).

[lurch]

imagine a chip which purely provides multiplexed I2C interfaces, but no GPIOs

Yup https://www.kickstarter.com/projects/land-boards/rpi-i2c-hub/description

[lurch]

And presumably there's a separate Factory for each remote Pi that a pigpio instance might be talking to?

[waveform80]

And presumably there's a separate Factory for each remote Pi that a pigpio instance might be talking to?

Precisely. That's a bit different to how things are now (constructing pins individually if you want multiple hosts) but I think it's actually simpler: define the factories you need and then either construct pins from them directly or assign them as the current pin_factory while you construct devices.

[waveform80]

Okay, this is about to land. I've finally got all the tests working, the docs are building nicely and everything seems good. Now, it's a pretty gigantic patch so I'll lay out the story of what changes are in here working from the bottom up, so people can find their way around it. Obviously I won't merge this until some review's been done! A little warning up front: there's a contentious change in here that Ben and I discussed at PyConUK; more on that later!

So, first let's quickly cover what doesn't change:

• Basically everything in the device hierarchy. That's all almost exactly as it was. The only changes here were down in the Device constructor
• The actual pin implementations themselves. All their prior stuff (function, state, pull, etc.) is more or less untouched; there's just a lot more machinery surrounding the pin implementations now

Now starting from the bottom let's work our way up:

• The Pin base class is still there but now there's a PiPin descendent which represents pins attached directly to the Raspberry Pi. PiGPIOPin descends from this. LocalPiPin descends from PiPin and all other pin implementations (RPiGPIOPin, etc.) descend from this. As discussed, not much else worth mentioning here except the new "address" stuff; I'll get to that in a bit.
• There's a new Factory abstract base class. Instances of Factory (or rather concrete descendants of it) have methods for constructing new GPIO pins (pin(spec)) and SPI interfaces (spi(**spi_args)). Firstly this replaces the horrible convoluted pin constructors we had before. Secondly it forms a nice expansion point that we can build I2C and possible 1-wire support upon in future without messing up backwards compatibility.
• Instances of Factory are obviously what pin_factory now point to ... except pin_factory has moved a little. Instead of being a global in gpiozero.devices it's now a class attribute of the Device class. This gets rid of a global (yay!) but you may be wondering "Dave's messing with pin_factory again?!". Yes, sorry - there's a circular dep issue here in that devices depend on pin factories which implement SPI which (in the software bit-banging variant) requires devices to implement. Oh dear. Making the pin_factory a (initially blank) class attribute solves this as it's now essentially a forward declaration that we can fill in once the devices module has loaded.
• There's a new Device._set_pin_factory method which can be used to handle replacing the active pin factory. Frankly, though, with the GPIOZERO_PIN_FACTORY env var (which uses this) I'm not really expecting anyone to use it directly; I've bunged a mention in the docs though. Oh, and I've added GPIOZERO_PIN_FACTORY to the docs.
• As mentioned above: pin factory instantiation now means you can't accidentally use the native pin implementation on a PC; import gpiozero will immediately fail. Also, pin factory fallback fires warnings for each default pin factory that fails. This should help users who forget to install RPi.GPIO in virtual envs.
• Pin reservation is now implemented properly (it used to be completely broken between SPI and GPIO devices). There's no more _PINS global (yay!) and no more re-entrant locking (double yay!). Instead, there's new Device._reserve_pins and Device._release_pins methods which are called by descendants during construction. The _reserve_pins method in turn uses a new _conflicts_with method which is called on the device being constructed with any device it shares a pin with, to test whether the two conflict. In GPIODevice this simply returns True (no two straight-forward GPIO devices can share a pin). But other descendants have more nuanced implementations, e.g. SPI devices can all share clock, MOSI, and MISO lines, just not select lines.
• SPI changes significantly in this patch. The actual software implementation is more or less the same, in the core, but the gpiozero.spi module becomes gpiozero.pins.spi and a lot of the wrapper stuff moves into gpiozero.pins.local. The major change is that the pigpiod backend does something different to the rest in that we now rely on pigpiod's SPI support directly. This means remote SPI support works (and works really fast with the hardware implementation!).
• The remote SPI bit-bang implementation has proven a little tricky to test because unfortunately it looks like the current Raspbian pigpio package is lagging behind the upstream GitHub: i.e. the SPI bit-banging methods are missing from the current Raspbian package. @bennuttall any chance we can get Serge to rebuild the packages from the latest source at some point in the near future? It's not terribly urgent: just means anyone using the pigpio backend won't have software SPI until that happens (but everything else, including hardware SPI, should work). I think @joan2937 must've added the SPI bit-banging stuff fairly recently because I can see bit-bang stuff for I2C and serial in the current Raspbian package; it's just SPI bit-bang which is missing but then I'm not sure which source the Raspbian packages were built from. Anyway, like I said, nothing terribly urgent.
• There's one issue I still need to track down with remote SPI, but which I haven't had time to dig into seriously yet: I can get the client to wedge fairly easily by interrupting it. Not sure what's going on there yet (my immediately thought was some sort of race, but then I realized there were no background threads in the test ... hmmm). Anyway, if you don't interrupt it, it'll place nice, and it's not (yet) the default back-end so I'll track that down in another ticket later.

Right ... contentious change time! One thing I didn't mention above is that pin reservation turned out to be quite tricky. In order to do pin reservation you need some means of uniquely identifying each pin in the system. "Easy!" says Dave and charges in by using the identify of each Pin object. Except that comes with a problem:

1. User sets up an SPI device (GPIOs 8, 9, 10, and 11) by doing something like MCP3008(channel=1)
2. User erroneously attempts to set up an LED on GPIO11 with LED(11)
3. Down at the Device level, we construct a pin on 11, but it doesn't conflict with anything because the hardware SPI implementation hasn't constructed any Pin objects that conflict with it.

"Okay!" says Dave, "we need some other unique identifier for a pin that can exist independently of the Pin object itself". Cue long internal debate about what forms this could take (imagine much blackboard scribbling here):

• Must support hierarchy of GPIO extenders.
• So identifier must have arbitrary number of components.
• What's a good delimiter for a hierarchy? Commas? Dots like objects? Slash like paths? Hmm.
• How about URLs? Maybe.
• Could "/" reasonably appear in a factory address? Potentially.
• Idiot! Why bother with URLs at all: these aren't going to be (directly) user-accessible addresses, so why not make an address a tuple of strings and avoid the whole delimiter issue.

So, slightly weirdly, pins have an address attribute which returns a tuple of strings (tuples are used because the addresses need to be immutable for the reservation system's dict key). Factories likewise have an address from which their pin addresses are derived. Currently it's a pretty simple system:

• All local factories (RPiGPIOFactory etc.) simply have the address ('localhost',)
• So all pins constructed by local factories have addresses that look like ('localhost', 'GPIO11')
• Remote factories (PiGPIOFactory) have an address like ('raspberrypi:8888',)
• So pins constructed by remote factories have addresses that look like ('raspberrypi:8888', 'GPIO10')

In future, this will likely become more complex as we add GPIO extenders. Pin representations don't change as far as users are concerned; __repr__ just returns the last component of address.

Now the next issue reared its head... In several pin implementations, when we construct a pin, the only way to get it into a "known good" state is to set its function, and the function is often limited to input/output. Unfortunately, hardware SPI demands that its pins remain in "alternate function" mode in order to operate. Flip 'em to an input and things break. So, consider the scenario again:

1. User sets up an SPI device (GPIOs 8, 9, 10, and 11) by doing something like MCP3008(channel=1). This reserves the pin addresses for GPIOs 8, 9, 10, and 11 against the SPI device.
2. User erroneously attempts to set up an LED on GPIO11 with LED(11).
3. Down at the Device level, we construct a pin on 11 to get its address ... and break the hardware SPI implementation.

Oh dear. So, whilst Pin.address is an attribute that exists for querying once a pin is constructed (in case you're passing in a straight Pin object rather than a pin specification), there's also a Factory.pin_address method which is used by the reservation system to query what address a pin would have if we were to go ahead and construct it. This is kinda horrid but I haven't come up with a better way yet. Now, because what Factory.pin_address is effectively doing is translating from a pin specification (a GPIO number) to a pin address I thought I'd throw in the following contentious change because, if we do this, this is exactly where it should go.

No, you know what: this is already way too long. Let's leave discussion of the stuff above for this ticket because I hope that despite some odd bits, it's all relatively uncontentious (albeit long). I'll open another ticket for the contentious change where we can chin-wag about it separately.