Allow device plugins to offer asynchronous opening

[alexdewar]

Description

I think I've done enough tinkering with this now and should probably stop 😛.

Background

Currently, the process of opening devices (which can include making a USB connection, moving a motor, etc.) all takes place on the main thread. The advantage of this model is that it's simple to work with -- the frontend doesn't have to worry about any intermediate stage where a device is still initialising, but can't actually be used for anything. The disadvantage is that it means that either we have to "finish" the device opening process early (e.g. before we've received a response from a server) or block the main (GUI) thread until it completes, both of which have drawbacks (e.g. #668, #247, #415).

Implementation

The backend changes were not especially complicated. The frontend is informed that we've started connecting to a device with a new device.before_opening message. I've added another class parameter, async_open, which device classes can use to indicate that they open asynchronously. It defaults to False, so classes have to opt in to this new behaviour. (Currently I haven't implemented this for any actual devices, though I have tested it.) Device classes with async_open==True just need to explicitly call signal_is_opened when they've finished opening (in practice, this will be from another thread).

Making the frontend work with these changes was a bit more fiddly. You should be able to disconnect from devices that are still connecting (though this might require some small changes to actual device code). I've added a new widget to show the connection status of individual devices to the device management dialog. Everything else should (hopefully) just work as before.

I'm afraid this PR is a little on the long side (sorry!) but a lot of the changes are to the tests, so I hope it's not too painful to review.

Closes #638.

[codecov]

Codecov Report

Attention: Patch coverage is 94.51220% with 9 lines in your changes missing coverage. Please review.

Project coverage is 85.31%. Comparing base (d6cbde8) to head (60af840).
Report is 21 commits behind head on main.

Files with missing lines	Patch %	Lines
finesse/gui/led_icon.py	57.14%	3 Missing ⚠️
finesse/gui/hardware_set/device_view.py	96.22%	2 Missing ⚠️
finesse/gui/temperature_controller_view.py	0.00%	2 Missing ⚠️
finesse/gui/hardware_set/device.py	96.42%	1 Missing ⚠️
finesse/gui/hardware_set/hardware_sets_view.py	97.29%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #682      +/-   ##
==========================================
+ Coverage   84.97%   85.31%   +0.34%     
==========================================
  Files          69       69              
  Lines        3461     3549      +88     
==========================================
+ Hits         2941     3028      +87     
- Misses        520      521       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dalonsoa]

I haven't check the tests, but the actual code looks ok to me.

It is a bit difficult to follow who does what as there are a lot of classes knot together to handle the status of the devices, and the connection of the devices, etc. so I think that at some point you will need to write a thorough developers guide explaining all of these relationships and why they are the way they are.

I don't see where a new threads are created to open a device. Could you point me in the right place. Also, my only initial concern was updating the GUI in the main thread from messages sent from another thread, but I guess this is not a problem if you have tested it and it works.

[dalonsoa]

What's the reason of being None? Should be either async or not?

[alexdewar]

If it's None, then it just leaves the _device_async_open property as whatever it was before, so if it was set by a parent class, it won't be overridden.

[alexdewar]

It is a bit difficult to follow who does what as there are a lot of classes knot together to handle the status of the devices, and the connection of the devices, etc. so I think that at some point you will need to write a thorough developers guide explaining all of these relationships and why they are the way they are.

Yeah, it's a bit of a tangle 😞. If I have time, I'll add a bit more documentation before merging this PR. Otherwise I'll do it later.

I don't see where a new threads are created to open a device. Could you point me in the right place. Also, my only initial concern was updating the GUI in the main thread from messages sent from another thread, but I guess this is not a problem if you have tested it and it works.

This part of the code doesn't actually create any threads. (I did consider an implementation where we left things the same as before, but just ran the __init__ methods on separate threads, but decided against it in the end.) Any class with async_open=True is just responsible for calling signal_is_opened() at some later stage, which in practice will probably involve other threads. This doesn't mean they have to explicitly make Python threads though: e.g. all the devices which send HTTP requests use Qt for this, which makes its own background threads (and sends a signal when you get a response).

That's a good point about updating the GUI from a different thread. It's not a problem for the devices I've been working on, because they will use QThreads + Qt signals to pass messages back to the main thread, which then invokes signal_is_opened(), but it could be a problem for other devices. I'll make sure to document it properly.

[dc2917]

Code looks good, and the functionality sensibly implemented to me. Let's hope it works with the real devices!

[alexdewar]

Thanks for reviews 😄