python-microscope / microscope

Python library for control of microscope devices, supporting hardware triggers and distribution of devices over the network for performance and flexibility.
https://www.python-microscope.org
GNU General Public License v3.0
66 stars 38 forks source link

ASI does not use abc.SerialDeviceMixIn #297

Open juliomateoslangerak opened 8 months ago

juliomateoslangerak commented 8 months ago

Is there any reason why the MS2000 is not using the abc.SerialDeviceMixin?

I saw the deprecation warning on the abc.SerialDeviceMixin but I don't really understand it. Aren't mixin classes for composition? Is this deprecation warning from a time where the SerialDevice was not a mixin yet?

iandobbie commented 8 months ago

Probably just my incompetence. I have no idea why I didn't start with the SerialDeviceMixin. I will investigate. Will also need to change the ludl code as well.

carandraug commented 8 months ago

At some point, things start becoming personal preference, but I really think that having device classes subclassing from SerialDeviceMixin is a case where we should be using composition instead of inheritance. These devices "have" a serial connection, and "are" not a serial connection/device. It's not like the SerialDeviceMixin was saving us a lot of typing, it was mainly wrapping _readline and _write, we still had to overload those thins wraps in some devices, and we still had to do the locking in the device class. It also prevents separating the sending of commands and parsing of replies into a separate class, so things become messier and more difficult to share with other classes. For stages and controller devices, it's also messier having to share the same connection when using the mixin. Also, one of the devices can have both a serial and ethernet connection. All those things are still possible, it's just that things get hairy with the mixin.

Asked by email:

So what do you think we should do, use the deprecated function or just implement the locking and flushing on a case by case basis?

Currently, the documentation for creating new devices has:

Create a class that wraps the serial connection and provides the different commands as Python methods. The device object then “has a” device connection object, and the device connection object “has a” serial connection object. This will greatly simplify the code reducing most [device] methods to 1-2 lines of code.

Which I think makes things quite simple, even if sometimes a bit more verbose. I suggest microscope.lights.toptic.TopticaiBeam as a simple example of that design:

The sending of commands and parsing of the reply, with any weirdnesses of it, are contained in the iBeamConnection class. The mixing and conversion of those commands into the behaviour that the ABC documents is contained in the TopticaiBeam class.

With regards to locking, there is microscope._utils.SharedSerial which provides a serial.Serial implementation with locking.

juliomateoslangerak commented 8 months ago

That is clear. Thanks.

Just to summarize and for my own clarification cause when I was looking into this concepts it was never clear to me how python implements inheritance and mixins. My takeaway was that python does not explicitly implement this difference. From what I could see, and you are confirming, is that there are different ways to implement this.

class SerialMixIn() ...

class MyController(Device, SerialMixIn): ...

- Using an explicit "has a" property.
```python
class Device():
    ...

class SerialMixIn()
    ...

class MyController(Device):
    def __init__(conn_args):
        self._conn = SerialMixIn(conn_args):
        ...

Any other way to conceptualize these relationships?

I think that for these use cases, using Protocol from the typing library might be an appropriate way to structure things. In this case the _iBeamConnection might implement a Serial protocol just by defining the _write and _read methods. Agreed that maybe for the concrete case of the serial, as it is so simple it does not add up much, but still think it is a very interesting concept.

carandraug commented 8 months ago

[...] when I was looking into this concepts it was never clear to me how python implements inheritance and mixins. My takeaway was that python does not explicitly implement this difference. From what I could see, and you are confirming, is that there are different ways to implement this.

Python supports mixins simply because it supports multiple inheritance. In Python, a mixin is syntactically no different from other classes but while the "normal" classes represent an object on themselves that make sense to create, a mixin class make no sense on their own and if you instantiate an object of the mixin class you will get something that is not useful.

[...] Any other way to conceptualize these relationships?

Juts a minor issue: in the composition case it is no longer a "MixIn", it would be a SerialConnection (or similar)

I think that for these use cases, using Protocol from the typing library might be an appropriate way to structure things

Protocol would bump our requirement to Python 3.8.

In this case the _iBeamConnection might implement a Serial protocol just by defining the _write and _read methods.

I would split things a bit more. If you look into the Toptica iBeam code, the _iBeamConnection does not have read or write methods. It kinda represents the firmware to which you send commands and get back answers. The read and write methods are in a separate Serial object.