Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ASI does not use abc.SerialDeviceMixIn #297

Open
juliomateoslangerak opened this issue Nov 5, 2023 · 4 comments
Open

ASI does not use abc.SerialDeviceMixIn #297

juliomateoslangerak opened this issue Nov 5, 2023 · 4 comments

Comments

@juliomateoslangerak
Copy link
Contributor

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?

@juliomateoslangerak juliomateoslangerak changed the title ASI has not a SerialMixIn ASI does not use abc.SerialDeviceMixIn Nov 5, 2023
@iandobbie
Copy link
Collaborator

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
Copy link
Collaborator

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 TopticaiBeam class represents the device itself. It never even deals with the serial connection directly it only deals with an _iBeamConnection instance and calls

  • the _iBeamConnection class handles the serial connection and one method for each of the functions we use on the device firmware. In a way, this provides a lower level Python interface to the device.

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
Copy link
Contributor Author

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.

  • Just by using inheritance and giving the mixin "parent" a mixin name (SerialDeviceMixin). Indeed this is kind of confusing.
class Device():
    ...

class SerialMixIn()
    ...

class MyController(Device, SerialMixIn):
    ...
  • Using an explicit "has a" property.
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
Copy link
Collaborator

[...] 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants