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

shutdown() should close connections and release resources #256

Open carandraug opened 1 year ago

carandraug commented 1 year ago

When we call shutdown we should release all the resources associated with the device. At the moment, we send all the required commands so that the device is down but on the case of serial devices we do not close the serial connection (the exact device that's causing issues at the moment is a Thorlabs Filter wheel). We've just been relying on Python's garbage collection to close the connection but not only is that no predictable, it also requires the object to get destroyed. We should be releasing the resources as soon as shutdown() is called.

The point is, this thing should work:

d1 = ThorlabsFilterWheel("COM1")
d1.shutdown()
d2 = ThorlabsFilterWheel("COM1")  # currently fails, because COM1 is still open in d1
carandraug commented 1 year ago

Adding to the above, the exception is on controller devices, i.e., if shutdown is called in one of the controlled devices, it should not close a serial connection because that would also close the connection for the other devices. Instead, it should only close the connection when shutdown is called on the controlled device itself.

carandraug commented 1 year ago

An argument against closing the connections on shutdown is that we have the shutdown method not so that users can shutdown whenever they want without destroying the object, but so that people can ensure that shutdown gets called when Python itself is exiting and there's multiple processes (the main case when we can't trust GC).

If so, then the recommendation is to destroy the object device right after calling shutdown and we don't have to change anything. If don't have to change anything there's less things for us to maintain (it gets a bit tricky when the device class itself does not hold the serial connection itself).

Anyway, a proposed fix is on my branch 256-close-conn-on-shutdown (if we agree that we should close connections/release resources on shutdown).

iandobbie commented 1 year ago

How does this reflect on situations like a required restart of cockpit but trying to avoid restarting microscope devices as this might require rehoming etc? It is a massive win that you can often start cockpit and not be required to re-init most devices as this can be time consuming and destructive of existing position states or other continuity information.

carandraug commented 1 year ago

How does this reflect on situations like a required restart of cockpit but trying to avoid restarting microscope devices as this might require rehoming etc?

This makes no difference to cockpit because cockpit never calls shutdown on any device (cockpit calls disable but only if the user does it explicitly on the window buttons and not when cockpit exits -- see also MicronOxford/cockpit#682).


We're already saying that after Device.shutdown the device is no longer usable, cannot be re-initialised, and that Device.shutdown "disables and disconnects the device". In the case of cameras and mirrors, where we use a C library and need to explicitly "release the device", we do it on shutdown. However, in the case of serial connections we never actually close the connection and rely on having it closed automatically when the device object is destructed (and the serial connection goes out of scope).

Having the connection closed when the device object is destructed works (I don't think that's an issue). The question here is whether we should make it happens when shutdown is called. And this is more than a serial connection, the decision is about ensuring that shutdown releases all resources - or at least those that can only be one entity holding to it.

If we decide "yes" that shutdown closes connections, then it will be more work for us since we need to do it manually and we'll have to do it for all devices. I'm struggling to come up with concrete examples where this is actually needed.

On the other hand, having it done explicitly on shutdown shouldn't be that much more work (and I already prepared something for all of our current devices in a couple of minutes) and I guess makes things clearer.

iandobbie commented 1 year ago

In general I think easy and simple is the way to go, but I do worry that this may come back to bite us later. However, that said, hardware always has weird behaviors and having to explicitly work out how to fully shutdown and disconnect every device and then code it into the module seem like quite some work, whereas just deleting the objects seems easy.

I think we should make a very explicit statement in the documentation about what we do on shutdown, both for clarity when questions come up and to help future writers of new device modules (probably us).