Add plugin system for hardware and use for serial devices

[alexdewar]

Background

FINESSE connects to various devices: a stepper motor, two temperature controllers, a temperature monitor and the EM27 spectrometer. Except for the spectrometer, these devices are connected via USB serial. For each device, there is a "real" implementation for connecting to actual hardware as well as a dummy version for development. To use the dummy version of a serial device, you select "Dummy" as the COM port from the dropdown menu and for the EM27 you pass an extra command-line argument. Apart from being somewhat ugly, this strategy is also not particularly extensible, which will especially be a problem when we come to add new device types for UNIRAS. It would be better if the user could choose which device to use for a given type, whether that's the "real" one using USB serial, the dummy device or a new type of device added for UNIRAS. (This was suggested by @dalonsoa when the original code was merged, but we ran out of time to implement it.)

As different devices have different parameters (e.g. USB serial devices need a port and baudrate specified), the choices for parameters could be shown dynamically in the GUI based on what the currently selected device requires.

Changes

This PR adds a plugin system for defining different types of hardware and refactors the existing serial devices to use this system. While the backend code has changed considerably, the basic design of the program is the same, i.e. users still have to manually connect to serial devices using a panel in the bottom left of the GUI. Eventually we will want the code to automatically connect to devices (#323) at which point this panel can be removed (and possibly repurposed into a separate configuration dialog). I have only made the serial devices plugins for now. The plan is to do the same thing for the EM27 code too eventually, but it will require some additional changes (e.g. there are EM27-specific GUI components which should also probably be part of the plugin).

Implementation

There are two types of plugin, device base types and device types, both of which are represented with classes that inherit from DeviceBase. The base types are abstract classes which define the interface for a particular kind of hardware (e.g. a stepper motor). The device types are concrete classes which must inherit from one of the base types (i.e. they cannot inherit from DeviceBase directly). They must also be defined using one of the plugin decorators in order to be discoverable. Base types should use @register_device_base_type and device types can use @register_device_type or @register_serial_device_type. The decorators add some extra class variables (e.g. device parameters) and add the classes to common registries.

All plugins must live in finesse.hardware.plugins. The code obtains a list of device types by recursively importing the submodules from finesse.hardware.plugins and then checking the device registry. Note that this step is only necessary to obtain a list of all available device types, which we have to do to populate the GUI, but you can just import device classes directly to use them.

Each connected device is uniquely identified by a string representing its base type (e.g. "stepper_motor") and where there can be more than one instance of a base type, an additional name (e.g. "temperature_controller.hot_bb").

Rough edges

• Currently there are no tests
• It's a bit ugly to pass instance into "device.error.*" messages
• I kept the current hack to report the temperature as NaNs when there is no temperature monitor connected (we need this for the data file recording) but there are definitely cleaner solutions

Fixes #319.

[codecov]

Codecov Report

Merging #336 (bc6ed35) into main (03dde3c) will decrease coverage by 1.48%.
Report is 25 commits behind head on main.
The diff coverage is 72.40%.

@@            Coverage Diff             @@
##             main     #336      +/-   ##
==========================================
- Coverage   76.46%   74.98%   -1.48%     
==========================================
  Files          47       50       +3     
  Lines        2379     2614     +235     
==========================================
+ Hits         1819     1960     +141     
- Misses        560      654      +94     

Files	Coverage Δ
finesse/config.py	100.00% <ø> (ø)
finesse/event_counter.py	100.00% <100.00%> (ø)
finesse/gui/error_message.py	50.00% <100.00%> (ø)
finesse/gui/measure_script/script.py	99.53% <100.00%> (+<0.01%)	⬆️
finesse/gui/serial_device_panel.py	100.00% <100.00%> (ø)
finesse/gui/stepper_motor_view.py	100.00% <100.00%> (ø)
finesse/hardware/plugins/__init__.py	100.00% <100.00%> (ø)
finesse/hardware/plugins/stepper_motor/dummy.py	97.77% <100.00%> (ø)
...rdware/plugins/stepper_motor/stepper_motor_base.py	97.91% <100.00%> (ø)
...lugins/temperature/dummy_temperature_controller.py	92.30% <100.00%> (ø)
... and 18 more

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[dalonsoa]

I've just made a very lightweigth review and point to a couple of questions, suggestions. A much deeper one will come as soon as possible.

[dalonsoa]

Why is the temperature monitor treaded in a different way?

[alexdewar]

It's a hack -- I mentioned it briefly in the PR description.

The temperatures are requested from the temperature monitor every 2s and they are used to a) update the plot and b) save into the data file (if recording is happening). The problem is that at some point Jon decided that he wanted data files to carry on recording regardless of whether the temperature monitor is active. In general this is a better design, because it makes things more resilient in the case of the temperature monitor breaking, but the way I implemented it was to add a hack that NaNs are broadcast in place of real temperatures if the device isn't working. A better way to do it would be to have all the sensors whose data can be recorded into data files respond to a different, specific message used for this purpose only.

I've opened a new issue and copied this description into it: #337

[dalonsoa]

This file mixes stuff for the stepper motor and the temperature controller, which defeats a bit the modularity aims as someone adding a new device might need to edit this file to add its own stuff. If it makes sense for each hardware to have more than one module for some reason, maybe they can live in their own packages.

[alexdewar]

That's a good point. As I said in a previous comment, this is all basically a bit of a hack. It would be nicer if the data file module could figure out what columns should exist and what values they should be populated with dynamically. Maybe it could be resolved along with #337

[dalonsoa]

Decorators are great for registering functionality because they are explicit. But when it comes to registering classes that need to be inheriting from a specific subclass, they are redundant: you can achieve identical functionality with less code simply by adding the registration step to the __init_subclass__ method of the relevant base class. Not only it will be less code but also saves the user/developer to use both the decorator and the base class.

Here you have an example illustrating the process:

register = {}

class Base:

    def __init_subclass__(cls, long_name):
        cls.long_name = long_name
        if cls.__name__ not in register:
            register[cls.__name__] = cls

class Inherited(Base, long_name="A very long name"):
    pass

a = register["Inherited"]
print(a.long_name)

[alexdewar]

Ahh I didn't know you could do this. That's much nicer! I'll have a go at doing it this way

[alexdewar]

I've had a go at this... let me know what you think. I had to do some nasty shenanigans in various __init_subclass__ methods in order to get it to work the way I wanted, but I think the interface is at least a bit nicer now.

[dalonsoa]

Maybe "exclude" instead of "include"?

[alexdewar]

Ohh dear 😆. It turns out it should be ignore instead.

[dalonsoa]

I've focused only on the __init__subclass__ business. Will review the rest once that is out of the way. As I point out in one comment, I don't fuly understand the need of a Device class and a DeviceBaseType. I think they can be combined easily, but maybe I'm missing something?

[dalonsoa]

Why not making cls.device_base_type_info public and accessing it directly? I think sometimes we are (me included) paranoid about someone changing attributes that should not be change, making the code unnecesary complex...

[alexdewar]

I guess it just felt a bit odd to have get_device_type_info as a classmethod and
then let users access this attribute directly. I take your point but I'm a bit on the
fence about this.

[dalonsoa]

Include this information in the docstring, not as a comment. It is relevant to be able to use this and should appear in the documentation!

[dalonsoa]

I think I might be missing something. Is there any reason why you cannot do the following and skip the DeviceBaseType class below altogether?

Suggested change

	def __init_subclass__(cls, description: str | None = None, **kwargs: Any) -> None:
	"""Initialise a device type class.
	While the description argument has to be optional to allow for non-concrete
	classes to inherit from Device, it is mandatory to include it, else the device
	type class will not be added to the registry.
	Args:
	description: Human-readable name for this device type.
	"""
	# Forward keyword args to allow for multiple inheritance
	super().__init_subclass__(**kwargs)
	# **HACK**: Allow callers to omit this arg in the case that they are
	# non-concrete
	if not description:
	return
	# Set device description for this class
	cls._device_description = description
	# Add the class to the registry of device types
	_device_types.add(cls)
	def __init_subclass__(cls, is_base_type: bool = False, **kwargs: Any) -> None:
	"""Initialise a device type class.
	While the description argument has to be optional to allow for non-concrete
	classes to inherit from Device, it is mandatory to include it, else the device
	type class will not be added to the registry.
	Args:
	description: Human-readable name for this device type.
	"""
	# If it is a base class, we initialise it as such
	if is_base_type:
	cls._init_base_type(**kwargs)
	return
	# If it is not, it should inherit from one.
	# Not sure how robust this will be to differences in the registration order...
	if not set(cls.__bases__).intersection(_base_types):
	raise ValueError(f"Class {cls.__name__} must be a device base type or inherit from one.")
	# And we initialise it as a concrete device class
	cls._init_device_type(**kwargs)
	@classmethod
	def _init_base_type(
	cls,
	name: str,
	description: str,
	names_short: Sequence[str] = (),
	names_long: Sequence[str] = (),
	**kwargs,
	) -> None:
	super().__init_subclass__(**kwargs)
	# Store metadata about this base class
	cls._device_base_type_info = DeviceBaseTypeInfo(
	name, description, names_short, names_long
	)
	# Add the class to the registry of base types
	_base_types.add(cls)
	@classmethod
	def _init_device_type(
	cls,
	name: str,
	description: str,
	**kwargs,
	) -> None:
	super().__init_subclass__(**kwargs)
	# Set device description for this class
	cls._device_description = description
	# Add the class to the registry of device types
	_device_types.add(cls)

[dalonsoa]

You can even go one step further and take the decision of a class being base or not depending on the class name instead of an input parameter (although that's more explicit): if it ends in Base it is a base class and not otherwise. StepMotorBase will be considered a base class, so will TempControllerBase, but not TempController or MyFunnyClassWithOddName.

[alexdewar]

Interesting! I like the idea of specifying is_base_type explicitly. I'll have a tinker
with this.

The reason for having a separate DeviceBaseType class was partly because
SerialDevice inherits from Device but not DeviceBaseType, because it isn't a
concrete device implementation (it isn't a device base type either actually). I guess we
could either a) make an AbstractDevice parent class which includes just the bits actually
shared with SerialDevice or b) make SerialDevice not inherit from anything (not sure
if this will break something or not).

[dalonsoa]

So, how this goes? I think I'm missing the overall architecture of classes you are implementing.

In the current implementation, I thought you would have a, lets say, StepperMotorBase inheriting from DeviceBaseType, and a ModelXYZStepperMotor as a concrete, usable device, inheriting from StepperMotorBase (because it is a stepper motor) and SerialDevice (because it is a serial device). Why does SerialDevice need to inherit from Device without being a DeviceBaseType? Or why it needs to inherit from Device at all?

It seems to me you might be mixing two orthogonal concepts here: device types (stepper motor, temp controller, etc.) with connection types (serial, GPIB, ethernet...). It makes sense to me that a concrete device needs to inherit from both (or use composition, instead), but I don't see any reason why these two concepts need to have a common ancestor to inherit from.

[alexdewar]

Right, so I've finally finished doing this.

I did end up making an AbstractDevice class as a separate base class. The reason is that the SerialDevice class needs to modify a class attribute (device parameters) which is shared with other device types.

[dalonsoa]

Looks good! I'm approving this PR, but I've left a comment that will need addressing.

[dalonsoa]

You probably have though og this, but as it is now, if the concrete device class adds its own parameters (and depending on the order of the parent classes) these parameters will be overwritten. You are going to need to put in place some checks to make sure that parameters added in the multiple inheritance process can all live with each other.

[alexdewar]

I thought about it and was like "ah I'll deal with it later", then promptly forgot to do so 😛. You're right, it does need fixing.

[alexdewar]

Right, I think this is all done now!

@dc2917 Do you think you'll have a chance to have a look at this or shall I just go ahead and merge it?

[dc2917]

Right, I think this is all done now!

@dc2917 Do you think you'll have a chance to have a look at this or shall I just go ahead and merge it?

Yep, I'll take a look

[dc2917]

I've only made a couple of super minor comments, otherwise this all looks really good to me. The new "Device control" panel on the GUI looks cool!

[alexdewar]

Seeing as I'm going into the lab on Monday anyway I think I'll hold off on merging this until I've tested it (again) with the actual hardware.

[alexdewar]

I've tested it on the real hardware and everything still seems to be working.