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
69 stars 41 forks source link

stop "conf"s all being the same dict #212

Closed thomasmfish closed 3 years ago

thomasmfish commented 3 years ago

When multiple devices are being created without a conf set, all dev["conf"]s are pointing to the same dictionary, meaning that they all have the same _index. This was making it impossible to load both AndorAtmcd cameras being loaded at once on the cryoSIM at B24, Diamond Light Source as it hit this within the class AndorAtmcd:

    def initialize(self):
        """Initialize the library and hardware and create Setting objects."""
        _logger.info("Initializing ...")
        num_cams = GetAvailableCameras()
        if self._index >= num_cams:
            msg = "Requested camera %d, but only found %d cameras" % (
                self._index,
                num_cams,
            )
            raise microscope.InitialiseError(msg)
        self._handle = GetCameraHandle(self._index)
carandraug commented 3 years ago

The proposed fix changes the default from an empty dict to a None. This also changes the function signature so the type annotation needs to be fixed accordingly, i.e., from Mapping[str, Any] to Optional[Mapping[str, Any]]

However, I don't think this is the right fix. The default really is an empty dict, and the use of None here is a workaround. The root problem is that in microscope.device_server.serve_devices the dictionary of arguments is being modified and that behaviour is unexpected.

Suppose that instead of wanting the default empty dict, we do have two devices with the same list of arguments, so we do:

conf = {'arg1': some_value}
DEVICES = [
    device(SomeFloatingDevice, .., conf=conf, uid='B'),
    device(SomeFloatingDevice, .., conf=conf, uid='A'),
]

Because this does not trigger if conf is None: conf = {}, both devices will end up with the same index value. The issue here remains.

I believe the right fix is to have make a copy of the conf dict (possibly in serve_devices I'm not sure).

thomasmfish commented 3 years ago

@carandraug presumably any original dict is thrown away after device is created anyway, so I've made a different fix at #217 (unfortunately untested for now)