labscript-suite / labscript-devices

A modular and extensible plugin architecture to control experiment hardware using the 𝘭𝘒𝘣𝘴𝘀𝘳π˜ͺ𝘱𝘡 𝘴𝘢π˜ͺ𝘡𝘦.
http://labscriptsuite.org
Other
5 stars 58 forks source link

Adding support for GeniCam-compatible cameras #106

Open TerryGeng opened 1 year ago

TerryGeng commented 1 year ago

This project adds a new labscript device GeniCam to support cameras compatible with the GeniCam interface.

GeniCam is a generic camera interface designed by the European Machine Vision Association, whose final goal is to unify the camera interfaces of different backends. It has been widely adopted by Allied Vision and many other camera manufacturers.

One of the targets of this project is to phase out the usage of IMAQdx since IMAQdx is a close-sourced black box and all supports are provided at the mercy of NI. The backend of this project is based on https://github.com/genicam/harvesters. However, it is worth noting that GeniCam interface requires a .cti file (provided by the camera manufacturer) to work and this .cti file is usually close-sourced.

On top of the functionalities provided by IMAQdxCamera, I added an attribute tree widget to browse and edit the camera attributes in manual mode.

Screenshot 2023-06-06 111601

A big part of the work is based on IMAQdxCamera but was carefully rewritten for better code quality.

It has been extensively tested on Allied Vision Mako G-131B for a while. We have plans to test more cameras in the next few months.

I intend to post this PR as a draft because there are still things on our wishlist:

I really wish other people may help test this branch and provide more input. Thanks!

dihm commented 1 year ago

@TerryGeng This looks really good. I'm excited to see it finished off and suspect it will get a lot of use from others in the community. I haven't had a chance to work directly with the code, but I'd like to add a few comments early on to help steer things helpfully.

Primarily, I would highly encourage you to split up generic Camera functionality and backend specific implementation details as a part of this PR. I agree that you shouldn't feel obligated to backport IMAQdxCamera to that interface (or any of the others), but doing this separation now will make it much easier for other backends to follow on your work. Hopefully this won't be too difficult as technically IMAQdxCamera was already doing this in all but name only (it being fairly easy to subclass to other backends by replacement rather than addition, if that makes sense).

On a related note, the typical paradigm for labscript widgets is to put them into labscript_utils, even if they are fairly uni-task. Mostly, I'm concerned about carrying a lot of QT boiler-plate with the device implementation itself. I appreciate that the current layout makes development a lot easier, but in the long term separating out the QT widget boiler-plate is extremely helpful for maintainability. Moving all of it to a generic Camera class is also an option, though I might still encourage ultimately moving QT widget code into labscript-utils. The base widgets you have made look quite useful and I could see them being re-employed elsewhere in the suite, which is very difficult if they are actually in a labscript-device (guaranteeing a circular dependence issue).

Anyway, this all looks super promising!

TerryGeng commented 1 year ago

@dihm Thanks for your interest!

Yes, I agree that it would be definitely better if I can separate the generic part and the GeniCam backend. It feels like the right thing to do to reduce the duplication of code. In the current implementation, I intentionally made the logical separation between the two parts anyway so in principle it should be straightforward to do. I will find some time to get it done.

Regarding where the widget should be kept, i.e. staying in its own code base or in labscript_utils. Since I don't foresee a need to use this widget outside blacs, so I'm a bit unsure. Would you say a few more words about the maintainability benefits?

dihm commented 1 year ago

@TerryGeng Apologies for the very slow response. My travel schedule has really thrown me for a loop.

The primary maintainability benefit to keeping widgets separate is that their primary dependency (QT) tends to deprecate and rot the widgets independently of the internal BLACS or device code. To the extent that they are truly "widgets" in the QT sense (ie no external dependencies or baked in user logic), keeping them separated doesn't hurt their use too much and it keeps them alongside many other widgets that tend to deprecate in similar ways at similar times.

Ultimately, the situation I want to avoid the most is having to debug a subtle QT issue that only effects the widgets, but having fire up all of BLACS and get a camera working so that the widgets will load and function. There are certainly ways to do that without splitting off the widgets to labscript-utils, but that is how most other devices work ATM and thus is my default recommendation for the sake of consistency. In the end, I'll probably defer to your judgement as the author (exception: unless those widgets have to be imported into other camera implementations, then they should definitely live in utils. Really want to limit cross-device imports to class inheritance only).