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
73 stars 42 forks source link

Adding more Ximea settings #262

Open juliomateoslangerak opened 1 year ago

juliomateoslangerak commented 1 year ago

While using the Ximea cameras for a number of purposes I came across some limitations in the number of settings that are implemented. I worked on some of them and I'd like to add a few more. In particular we will have to add the transform, bitdepth, roi,... @iandobbie @carandraug @thomasmfish any wishes? I could also try to get them all in using a settings factory (I don't know if you may call it like that).

carandraug commented 1 year ago

xiAPI parameters can be of type float, integer, enumerator, or string. They all use the same function to set them. If you are planning on a bunch of them, I think it will make sense to make something that works for all. The python api should already have all the stuff there on the ximea.xidefs module. Don't do the mistake I did of dynamically looking for the methods with the property names, you should be able to use self._handle.set_param and self._handle.get_param`. I think the most tricky thing is to handle the enums, but I'd guess along the lines of the following should do it:

def initialize(self):
    ...

    prm_type_to_add_method = {
        "xiTypeEnum" : self._add_enum_prm_to_setting,
        "xiTypeFloat" : self._add_float_prm_to_setting,
        ...
    }
    for prm_name, prm_type in ximea.xidefs.VAL_TYPE.items():
        prm_type_to_add_method[prm_type](prm_name)
    ...

    def _add_enum_prm_to_settings(self, prm_name):
        values: Dict[str, c_int] = ximea.xidefs.ASSOC_ENUM[prm_name] 
        ...

    def _add_float_prm_to_settings(self, name):
        ...
juliomateoslangerak commented 1 year ago

Thanks for the advice. My only concern is that there are 226 parameters. Really a bit too much to be practical in the UI. I can define a list (probably better a set) of parameters to be considered. Adding a new parameter should be as easy as adding the parameter name to the list.

carandraug commented 1 year ago

My only concern is that there are 226 parameters. Really a bit too much to be practical in the UI.

While there are a lot of parameters, each camera will only support a subset of them (I think the way to find out if it is supported is to try and read the value and check if it returned an error).

The UI is an issue for the GUI. In the context of cockpit I guess the recently discussed panel on MicronOxford/cockpit#838 would help.

I can define a list (probably better a set) of parameters to be considered. Adding a new parameter should be as easy as adding the parameter name to the list.

From the perspective of end-user, I don't think we can say it's easy to just add another property on that list. They'll probably be users of Cockpit or some other GUI (not Microscope directly) and may not even be Python developers (or software developers at all).

juliomateoslangerak commented 1 year ago

Agreed. One question. How is the UI going to sort the parameters. Is there something has to be done to, for example, sort them alphabetically? If I understand well dictionaries in the later versions of Python are sorted.

carandraug commented 1 year ago

One question. How is the UI going to sort the parameters. Is there something has to be done to, for example, sort them alphabetically? If I understand well dictionaries in the later versions of Python are sorted.

In Microscope, the settings dict is an ordered dict (ordered by insertion order) and cockpit displays in that order. The reason to use an ordered dict (even before Python's default dict was ordered) was so that the most important settings could appear first. To be honest, I never liked that approach because what the developer in Microscope and the end-user in Cockpit think are the most important settings is not necessarily the same. But we do need a way in Microscope to show most important (whatever most important means) settings easier. Again, maybe with the quick access settings panel proposed in MicronOxford/cockpit#838 that is no longer important.

juliomateoslangerak commented 1 year ago

Indeed. I think that if #838 works, the best for 100 settings is alphabetical.

juliomateoslangerak commented 1 year ago

I have a branch where I've been working on this. https://github.com/juliomateoslangerak/microscope/tree/add_ximea_settings

@carandraug, could you review this?

I finally made a blacklist with some of the settings _UNSUPPORTED_SETTINGS.

I tested this to some extent and there are still some errors produced (I presume) by the API. Problems arise sometimes when using update_settings. I do not see a way to improve this if it is not trying every single setting combination, exploiting the device_manifest or going directly to the C bindings, which are richer in detail. Many functions in the python API are just implemented in batch and are not tested. eg. the buffer allocated to the "device_manifest" is 256 (like all the others) while it should be many thousands. The c bindings provide ways to calculate the buffers that are necessary.

There are a few more comments in the code and things could be compacted at some places like the functions to get ints, floats and strings

iandobbie commented 1 year ago

To be honest seems like many python implementations we have seen that it is a very basic facia over the c-code but with silly limits (eg buffer size mentioned above) imposed. As usual coding around the limits probably makes it more work than just re-implementing in c-types. Unfortunately we (you and David! I did a bit honest.) have already sunk substantial effort into getting the crappy python bindings to sort of work.

I understand that these multi-device libraries are difficult to build and maintain, but why waste their effort and our effort on a half hearted python skin over the C? All that said it is worth lodging a bug report over issues you have found as this will help people think that the python bindings need to be first class citizens.

thomasmfish commented 1 year ago

I'm late to the party here but I'm not aware of anything we need. The main thing that would be valuable to us is saved settings, either through Cockpit defaults that get sent to Microscope or via a config on the Microscope side, as suggested in https://github.com/MicronOxford/cockpit/issues/838

iandobbie commented 1 year ago

What settings do you want to save as some of them should get stored with the channel info. Maybe that is the manner to get more stored?

thomasmfish commented 1 year ago

Users don't consistently use channels, and we want the camera settings to be consistent regardless of channel, so I don't think that storing it with channel info is the best approach - I think having a default setting per-camera is more useful. The settings we change are the fan, temperature, and readout mode because those settings get cleared every time Microscope is restarted on the Ixon PC.

iandobbie commented 1 year ago

You can specify these in the depot file so they reset every time that cockpit is started. I think this is the best place for fan and temperature to be specified. I thought this was done, I cirtainly tried to get that to work and we have had previous issues where this didn't work.

I just went and looked and at one point the b24 config had this in it eg....

[transmitted] type: cockpit.devices.microscopeCamera.MicroscopeCamera uri: PYRO:AndorAtmcd@ixon1.b24:7777 triggerSource: dsp triggerLine: 0 transform: (0,1,0) TemperatureSetPoint: -80 Fan mode: off isWaterCooled: True targetTemperature: -80

If this isnt working we need to work out why.

thomasmfish commented 1 year ago

I think some of that may have been removed in the past when it wasn't working... It's in use at the moment, so I can't check

iandobbie commented 1 year ago

I should also add that if you restart the remote microscope instance talking to the camera then this config will be lost. You should also be able to set these on the microscope side but I don't think this is a very good idea as the setting explicitly force the camera to turn off its fan and set the temperature so low that the water cooling is needed.