search

ZhuangLab / storm-control

Microscope control software
Other
65 stars 67 forks source link

Upgraded hamamatsu camera classes to allow multiple cameras #100

Closed jeffmoffitt closed 4 years ago

jeffmoffitt commented 4 years ago

We made a few minor changes to Hamamatsu classes to configure these cameras so that one (or more) can be triggered from another.

HazenBabcock commented 4 years ago

Hm, I'm not sure about this one.

HazenBabcock commented 4 years ago

Sorry, regarding the 3rd point, I see now that you added this to the init() method. Still not sure it is a good idea to do it this way.

jeffmoffitt commented 4 years ago

We are certainly happy to revise in anyway you see fit.

A few comments on your points:

Honestly, this was a holdover from a larger change we were thinking about implementing. The idea was that there are some camera properties that the user probably should not be able to access. Happy to roll this change back.

Agreed. We were following the style of the Andor camera classes. Again, happy to roll this back if you like.

These changes followed a debate as to whether it was better to have the cameras completely configured in the config.xml file or whether there were some properties that we basically going to be always the same, in which case, it might be 'safer' to simply have the hamamatsu class make them the standard.

Do you have a preference?

On Sun, Feb 9, 2020 at 9:08 AM Hazen Babcock notifications@github.com wrote:

Sorry, regarding the 3rd point, I see now that you added this to the init() method. Still not sure it is a good idea to do it this way.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ZhuangLab/storm-control/pull/100?email_source=notifications&email_token=AAUJAXDBDQPD6ZUD7WHPNHTRCAE5TA5CNFSM4KRSDGI2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELGNL6Q#issuecomment-583849466, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAUJAXCL2VEW6YGICZSHO2LRCAE5TANCNFSM4KRSDGIQ .

HazenBabcock commented 4 years ago

On further reflection, in my opinion the right way to do this is to have camera properties specified in the XML for the camera. This would deal with several issues, such as not all cameras from the same manufacturer necessarily have all the same properties. In the XML you could also specify the initial values for the properties you are interested in and whether they are modifiable. I agree this is not as 'safe' as hard coding (and it would also probably break all existing config.xml files) but I think the benefit of the ability to more easily support more cameras outweighs the risk.

Anyway, that is probably too much work for now, so please keep output_trigger_polarity[0]. I assume it is there because someone needed it, and I'd prefer not to just yank it away. Maybe @emanuega has some additional thoughts?

Looking at the Andor camera class use of is_master... Not sure this is optimal either... I'm not sure why the user would want to change Exposure_Time for a slave camera, but I'm also not sure why we'd want to deny them that ability. Or maybe this is an Andor quirk? Externally timed cameras can't control their exposure time? I leave it up to you whether your want use is_master or self.is_master.

HazenBabcock commented 4 years ago

I just checked the history for hamamatsuCameraControl.py, looks like @leonardosepulveda wanted output_trigger_polarity[0] for some reason.

jeffmoffitt commented 4 years ago

I have incorporated these comments into a different version and different pull request. I am closing this request.