Tidying up drivers

[sde1000]

Comments originally from @rnixx in #68:

Hi Stephen,

I still don't think there's a "winner" amongst the several driver APIs

I don't think we should talk about a "winning" API. Actually we have 2 approaches. The "old style" implementation in dali.driver.base, with straight forward sync API and async implementation with callbacks. And the asyncio based implementation which uses the "modern" way of life. Both have their value and usecases. And finally there's the daliserver module which needs to be refactored to one kind of a driver or another, or both.

What we have in the driver package is some kind of a mess. Here are some thoughts from my side:

• We should make clear that we have asyncio and non asyncio drivers.
• We should share code among these drivers if possible. This primary includes the device specific frame extraction and construction code.
• We may share backend specific code if it makes sense. In any case we have asyncio style backends and old style backends.
• The hid module should be split up inside an asyncio package. We have a tridonic package where the non asyncio driver lives and the asyncio variant lives in hid.py. Same for hasseb driver. On the other hand the hid class is actually the hid device related asyncio backend.

This is what the hid module may be split up to:

driver
    asyncio
        backend.py
            hid
        tridonic.py
            tridonic
        hasseb.py
            hasseb
        ...

I always felt you do not like the driver API I wrote very much. Maybe I'm wrong, either way I'm totally fine with this. I am not able to switch over to asyncio based main loop anytime soon, and I think other people also will or cannot use asyncio for one reason or another. Therefor we should just cleanup the package and document what's there and why.

[rnixx]

Thanks for opening a dedicated issue. As low hanging fruit and for the most self documenting value we should start with splitting up the hid module into an asyncio subpackage. Are you fine with this? I can help on this one if you like.

[sde1000]

I don't think we should talk about a "winning" API.

Indeed. I don't want to mandate a particular API; I see the drivers as an "optional extra" to the library, rather than its core; I want the library to be useful in environments where these drivers aren't appropriate at all, for example running on MicroPython in an embedded controller.

All the library code outside the drivers package needs to work whatever concurrency model the caller is using.

straight forward sync API and async implementation with callbacks

I feel strongly that the simple synchronous API should be a wrapper around the strictly more capable async one, and that driver authors shouldn't have to implement both. I have to assume some drivers will be making use of threads internally (to wait for communication from their devices using a blocking call, for example) and it needs to be documented how the callback from the driver is handled: what thread is the code running in, and how thread-safe does the caller's code have to be?

We should make clear that we have asyncio and non asyncio drivers

Yes. How should we refer to these? "asyncio" and "threaded"?

I think it would be useful to write a threaded wrapper for asyncio-based drivers: run the asyncio event loop in a separate thread and pass commands to it. Writing an asyncio-based wrapper for threaded drivers should also be possible.

We should share code among these drivers if possible. This primary includes the device specific frame extraction and construction code.

Possibly, although since this code is generally a very small part of the driver I'm not sure how much there is to be gained by doing so. I think it would make the package structure more complicated.

The hid module should be split up inside an asyncio package.

Yes, I need to split it up: I'm going to add a simple asyncio driver that talks to DaliServer and it wouldn't make sense for that to be in the hid module.

I always felt you do not like the driver API I wrote very much.

My main issue with it is I feel there are too many API layers. I don't see what's gained by splitting it up into so many base classes. It's encouraging driver authors to write the "sync" version of their driver first (eg. the unipi driver) when really they should only ever have to write an async version and then get the sync version for free.

Apart from rearranging the drivers code, there are other things I think we should do to make finding and using drivers easier in application code. I'll open a separate issue for that, because it's largely unrelated to how the driver code is organised.

[rnixx]

My main issue with it is I feel there are too many API layers.

The idea behind the design was to have a similar concept like transport and protocol https://docs.python.org/3/library/asyncio-protocol.html, https://twistedmatrix.com/documents/19.10.0/api/twisted.internet.protocol.Protocol.html with a unified API for sending and receiving DALI frames. The several "transports" are represented by the Backend and Listener implementations while the "protocols" are implemented as sync and async DALIDriver objects.

It's encouraging driver authors to write the "sync" version of their driver first.

Valid point. We should merge Backend/Listener and SyncDALIDriver/AsyncDALIDriver APIs. If a device is not able to listen to the bus an error should be raised.

we should do to make finding and using drivers easier in application code

I totally agree. As stated in another issue I'm thinking about a driver registry/factory. It may look like so

class driver:
    """Driver registry and factory"""
    registry = dict()

    def __init__(self, name):
        self.name = name

    def __call__(self, driver):
        self.registry[self.name] = driver

    @classmethod
    def list_drivers(cls):
        for name, driver in cls.registry.items():
            yield (name, cls.title)

    @classmethod
    def create(cls, name, **kw):
        return cls[name](**kw)

@driver('my_driver')
class MyDriver(DALIDriver):
    title = 'My Driver'

We also should add some metadata to the driver implementation like a title and the driver capabilities, e.g. whether the driver supports listening to the bus.

[rnixx]

While discussing #74, the question arises whether read and write operations to the backend need a timeout per cycle or whether we might change read and write timeouts to be instance based.

Is there a real world use case where a read or write operation needs a dedicated timeout?

[abrasive]

Hi folks, is there still appetite for some unifying work here? I'd be keen to contribute. As a new user I found the mix of different APIs in the examples very confusing, especially as the serial and hid drivers both seem to be async but their API differs? (eg. async vs. sync connect(), dev_inst_map handling differs) And then I bit myself with it again after building my own interface - after I moved my own interface from using serial to hid, oops!

Is documenting the existing API still the most valuable place to start? Having written a couple of drivers now I feel like this would be a helpful jumping-off point for any changes.

I think you have the seed of a neat driver-finding interface hiding in the serial driver code already; I quite like the URI approach you have taken there. pySerial uses URIs quite nicely - when my interface had a serial driver I could open mydriver:socket://somehost:9999 and talk to my interface on a remote machine (the socket:// path handling being a pySerial thing). Being able to ship driver-specific parameters would be very civilised too (eg. mydevice://?serial=12345 to select amongst multiple interfaces attached to the same machine).