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

undefined behaviour in many methods #69

Open carandraug opened 5 years ago

carandraug commented 5 years ago

Behaviour for CameraDevice.set_exposure_time with values outside the valid range is undefined. The current behaviour is to clip it for AndorSDK3 while on PVCamera it is passed unchecked and left to libpvcam. The should have a defined behaviour for consistency.

This is similar to issue #51, which was defining behaviour for LaserDevice.set_power_mw for values outside the device valid range. I think the same logic and conclusion applies to camera exposure times. If so, we should clip the exposure time values. Anyone thinks otherwise?

mickp commented 5 years ago

We need to define the expected behaviour and limiting cases for most methods on devices in devices.py. This issue is now expanded for discussion of all theses methods.

carandraug commented 5 years ago

Here's some more cases

  1. setting exposure time within range but an inavlid number. Example, a camera supports time exposure increments of 100msec but users sets exposure to 150msec for example.

  2. calling shutdown in an enabled device. Should: 1) give an error because it needs to be disabled, 2) the Device base class calls disable and then shutdown, 3) the concrete class is responsible from disabling it if required. I'm leaning towards the third option because forcing disabling+shutdown might be quite slow, and the concrete class can decide what's fastest for the specific device.

carandraug commented 5 years ago

For Camera.set_exposure_time, I propose we clip the value to the camera valid range as happens with lasers power, and that we round the value to the nearest valid exposure time, changing what other settings are needed (for example, in the IDS camera we need to manually change the PixelClockto enable different ranges of exposure times). This has the problem that the result might be too different from what the user wants. Since we don't know that, the return value of set_exposure_time could be the final exposure time which the user can check afterwards and decide whether it's close enough for whatever he wants.

carandraug commented 5 years ago

Other cases:

carandraug commented 5 years ago

One more case:

carandraug commented 5 years ago

Proposal for methods that set values from a range

Methods that set things like exposure time, laser power, or a pattern should not fail because of invalid values. Values should be clamped if outside the valid range and rounded to the closest valid value. The method should return the value that the device was set to.

Using as example a camera which has valid exposure times of 0.1, 0.5, and 1 seconds only:

camera.set_exposure_time(1.2) # returns 1.0
camera.set_exposure_time(0.4) # returns 0.5
camera.set_exposure_time(0.2) # returns 0.1
camera.set_exposure_time(0.09) # returns 0.1

The returned value is what we set the value to, which may be different from what value is on the device. For example, a camera may have the exposure as milliseconds in an int. However, our Camera devices use seconds. So the camera may have 1 millisecond but we will return around 0.100000000000000088 since we return a double. A similar case is a lasers that only use a 12 bit integer to set laser power (the deepstar laser).


The reason for this behaviour is that this methods will often get invalid values. We don't want to be responsible to decide whether the rounding we can do is good enough. So we always round and use the rounded value.

Because we the value is always rounded, we move the job of checking if the set value is good enough to the user. The user should probably be doing some sort of checking on the set value. At that point, we might as well return the value to save the user from making two calls. Because we return the set value and not the internal value in the device, we also don't need to get the value from the device and so retuning the value should not cause a performance issue.

carandraug commented 5 years ago

Another undefined behaviour: calling FilterWheelbase.set_position with an invalid position number.

carandraug commented 3 years ago

We discussed recently what to do about shutdown and decided that shutdown is the method to call to ensure that the shutdown code is called for the object destruction. We advise users to always call it when done with the device. After shutdown, nothing else is likely to work. Reuse of the device requires reconstructing the device, calling initialize again might not work. Only exception is multiple calls to shutdown which should be handled. How to ensure that multiple calls to shutdown do not fail is device specific, it might require the class to keep around a _is_shutdown attribute but in most cases one will be able to check if the device/connection handle is already closed.