sde1000 / python-dali

Library for controlling DALI lighting systems
Other
150 stars 73 forks source link

Command sequences #68

Closed sde1000 closed 4 years ago

sde1000 commented 4 years ago

I've never been happy with the bus module. I had an idea that it should be the API for dealing with all the items on a bus as a unit, but it would have involved settling on a particular driver API and calling convention and I still don't think there's a "winner" amongst the several driver APIs we have at the moment.

In practice, people have wanted to use the bus module for the commissioning code it contained. That's fair: commissioning is a non-trivial sequence of commands, and it's sensible for python-dali to include code to do this. There are also other things that people may commonly want to do that involve more than sending a single command to a bus and reading the response, like reading the list of device types supported by a particular item of control gear, that could usefully be included in the library.

So here's the idea: command sequences. These are co-routines (implemented as generators; see PEP 342) that produce a sequence of commands to send, and expect to be sent the responses to the commands. These command sequences are completely driver agnostic; I've provided a sample implementation of run_sequence() for async device drivers, but an implementation for any other type of device driver should be trivial.

This is a first cut of the idea. There are still several open questions, and there's no test code for the two example sequences yet. I'm looking for comments.

Open questions:

I believe command sequences also answer the question, "what should code that needs to execute as a DALI transaction look like?": if executing a sequence is a driver-level function, the driver can arrange for the sequence to execute as a transaction if the hardware is able to support it.

rnixx commented 4 years ago

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:

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.

So here's the idea: command sequences.

This sounds good. I like the idea.

These are co-routines

This sound good with the assumption that everyone is using asyncio.

What should the API for sequences look like? Most sequences will want to return a result.

I'd try to look about the implementation the other way around. The command sequence is the generator for the result, and whether the implementation uses asyncio or SyncDaliDriver or AsyncDaliDriver does not matter in the first place. So the usage may look like:

driver = WhateverDriverIWantToUse()
for device in sequence.FindDevices(driver):
   ...

For implementing concrete sequences this would require an intermediate API which can handle the different types of drivers. At least for asyncio based and SyncDaliDriver based drivers this would be easy. For AsyncDaliDriver there would be the need to "await" the result, but also here we may just wait for the result with a defined timeout.

But actually before diving deeper into this topic I'd prefer to solve the "drivers issue" stated above in the first place.

sde1000 commented 4 years ago

These are co-routines

This sound good with the assumption that everyone is using asyncio.

This style of co-routine is nothing to do with asyncio: it doesn't depend on it in any way, and was around long before asyncio was.

driver = WhateverDriverIWantToUse()
for device in sequence.FindDevices(driver):
   ...

For implementing concrete sequences this would require an intermediate API which can handle the different types of drivers. At least for asyncio based and SyncDaliDriver based drivers this would be easy. For AsyncDaliDriver there would be the need to "await" the result, but also here we may just wait for the result with a defined timeout.

Unfortunately this style of flow control is completely incompatible with async code. async code requires that the sequence.FindDevices(driver) code in your example there is able to yield control whenever it needs to wait for a result from the driver, and it just can't.

The motivation behind implementing command sequences as generator co-routines is to make the same code usable in both the traditional and async contexts. To use the QueryDeviceTypes sequence with a sync driver, for example, would look something like this:

driver = WhateverDriverIWantToUse()
qdt = QueryDeviceTypes(Short(0))
try:
    s = qdt.run()
    response = None
    while True:
        cmd = seq.send(response)
        if isinstance(cmd, sequence.sleep):
            time.sleep(cmd.delay)
        elif isinstance(cmd, Command):
            response = driver.send(cmd)
        else:
            pass # Ignore progress reports
except StopIteration:
    pass
device_types = qdt.device_types

In practice the loop over the sequence would itself be a library function, probably a method on the base class for the drivers, so the call would look like this:

driver = WhateverDriverIWantToUse()
qdt = QueryDeviceTypes(Short(0))
driver.run_sequence(qdt.run())
device_types = qdt.device_types

If we arranged for all command sequences to work in a consistent way (same attribute names to access the sequence and result), we could possibly shorten this to:

driver = WhateverDriverIWantToUse()
device_types = driver.run_sequence(QueryDeviceTypes(Short(0)))

Regarding drivers: I'll copy your comments into a separate issue; I agree we need to discuss.

rnixx commented 4 years ago

This style of co-routine is nothing to do with asyncio

ok

Unfortunately this style of flow control is completely incompatible with async code

Correct me if I totally miss something, but making the sequence run a generator on it's own if the expected result is an iterable can be done like so:

def sequence_result_iter():
    ...
    qdt = QueryDeviceTypes(Short(0))  # QueryDeviceTypes.results = Queue()
    try:
        s = qdt.run()  # fills results with Queue.put()
        ...
        while True:
            ...
            while True:
                try:
                    res = qdt.results.get(True, 0)
                    yield res
                except Queue.Empty:
                    break
    except StopIteration:
        pass
rnixx commented 4 years ago

In practice the loop over the sequence would itself be a library function, probably a method on the base class for the drivers

this sounds good

sde1000 commented 4 years ago

Aargh — no f-strings in python 3.5! I wonder if anyone is still using it?

ferdinandkeil commented 4 years ago

As mentioned elsewhere I really like this idea. You asked for feedback on the API, so here are my 2 cents.

I'm not a fan of co-routines, as I find dealing with them it not intuitive. Why not implement sequences as iterables:

class Sequence:

    def __init__(self, param1, param2):
        self.param1 = param1.
        self.param2 = param2
        self.__result = []

    @property
    def result(self):
        return self.__result

    def __iter__(self):
        c = MyDALICommand()
        yield c
        if c.response is None:
            return
        c = MyOtherDALICommand()
        yield c

Consuming the sequence is as easy as iterating over it:

seq = Sequence('param1', 'param2')
for cmd in seq:
    send(cmd)
return seq.result

Sending back data to the sequence is not necessary, as the response is neatly stowed away in the command instance. result is declared as a property, so a sequence implementation can do some processing before returning the result.

sde1000 commented 4 years ago

Sending back data to the sequence is not necessary, as the response is neatly stowed away in the command instance.

That's not how responses work! Command instances are an immutable representation of a particular command. There's no concept of "Command instance that hasn't been sent yet" and "Command instance with a response bound to it".

Even if Commands were modified to work this way, isn't

    def __iter__(self):
        c = MyDALICommand()
        yield c
        if c.response is None:
            return
        c = MyOtherDALICommand()
        yield c

more verbose and less clear than

    def __iter__(self):
        if yield MyDALICommand() is None:
            return
        yield MyOtherDALICommand()

I do like the idea of declaring that sequences are iterables. I'll see if I can make the change.

sde1000 commented 4 years ago

I tried, but no: although generators follow the iterator protocol (they have a __next__() method), iterators don't follow the generator protocol (they don't have a send() method).

ferdinandkeil commented 4 years ago

Sending back data to the sequence is not necessary, as the response is neatly stowed away in the command instance.

That's not how responses work! Command instances are an immutable representation of a particular command. There's no concept of "Command instance that hasn't been sent yet" and "Command instance with a response bound to it".

You are correct about the response, I got confused there. However, commands are only almost immutable, as they do store the destination address.

Coroutines it is, I guess.

sde1000 commented 4 years ago

However, commands are only almost immutable, as they do store the destination address.

No. Command instances with destination addresses have the address passed in at instance creation time, and it may not be changed afterwards.

Compare:

from dali.address import Short
from dali.gear.general import Off
off = Off(Short(5))

and

from decimal import Decimal
fifteen = Decimal(15)

off and fifteen are immutable in the same way: although you can write fifteen.foo = "bar" you shouldn't expect it to work.

ferdinandkeil commented 4 years ago

However, commands are only almost immutable, as they do store the destination address.

No. Command instances with destination addresses have the address passed in at instance creation time, and it may not be changed afterwards.

Yes, makes sense.

sde1000 commented 4 years ago

Today I Learned: generators can have a return value. (In other words, I read PEP 380 properly.)

If you yield from a generator then the value of the yield from is the return value of the generator.

Otherwise, the return value is available as the value attribute of the StopIteration exception.

So I can make the calling convention for command sequences much simpler. A command sequence is simply a generator that yields a sequence of commands, sleep and progress objects, and optionally returns a result. No need for a class and a convention regarding return values. I'll push an update shortly.

sde1000 commented 4 years ago

Committed as 169d42b1db1d5ff6151f4509205b3e771aa31529 which is rebased to current master and squashed.