Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding more Ximea settings #262

Open
juliomateoslangerak opened this issue Jan 11, 2023 · 14 comments
Open

Adding more Ximea settings #262

juliomateoslangerak opened this issue Jan 11, 2023 · 14 comments

Comments

@juliomateoslangerak
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

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 microscope-cockpit/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
Copy link
Contributor Author

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
Copy link
Collaborator

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 microscope-cockpit/cockpit#838 that is no longer important.

@juliomateoslangerak
Copy link
Contributor Author

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

@juliomateoslangerak
Copy link
Contributor Author

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.

  • Some settings related to accessing the flash storage of the camera,
  • The "devide_manifest", which might be interesting but only implemented in the latest cameras. The manifest is a (rather large) xml string describing the settings of the camera. We might get more detailed description of the settings, but as it is only in a few models and I honestly dont feel like working with XML, I thought to just ignore it for the moment.
  • The "xiapi_context_list" producing settings for off line processing
  • And the "trigger_source" as it is implemented in a separate "add_setting" call. I do not know if we should change this.

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
Copy link
Collaborator

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
Copy link
Contributor

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 microscope-cockpit/cockpit#838

@iandobbie
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link
Collaborator

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:[email protected]: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
Copy link
Contributor

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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants