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

Add Hamamatsu camera service #163

Merged
merged 25 commits into from
May 23, 2024
Merged

Add Hamamatsu camera service #163

merged 25 commits into from
May 23, 2024

Conversation

ivalaginja
Copy link
Collaborator

@ivalaginja ivalaginja commented Mar 4, 2024

Fixes #162.

Contains a code sneak in which I delete a property from the AV cam service that is not set up or used.

Todo

  • Check library loading
  • Test on hardware
  • Check dimensions: sensor_width vs. sensor_heigth, width vs. height
  • Address all todos in code

@ivalaginja ivalaginja added hardware Integrate new hardware collaborators Worked on by external collaborator. Might need some extra help on code integration. labels Mar 4, 2024
@ivalaginja ivalaginja self-assigned this Mar 4, 2024
@ivalaginja ivalaginja force-pushed the feature/hamamatsu_camera branch 2 times, most recently from ba8122c to edc5337 Compare April 2, 2024 19:45
@ivalaginja ivalaginja force-pushed the feature/hamamatsu_camera branch 4 times, most recently from afb248f to 8068f38 Compare April 17, 2024 20:54
@ivalaginja
Copy link
Collaborator Author

For once a camera service that was simple and went smoothly. Tested on hardware on a Hamamatsu ORCA-Quest C15550-20UP as the science_camera with a 400 x 400 px ROI, and everything works as expected:

Screenshot 2024-04-18 at 13 42 07

@ivalaginja ivalaginja marked this pull request as ready for review April 18, 2024 11:55
@ivalaginja ivalaginja requested a review from ehpor April 18, 2024 11:55
@ivalaginja
Copy link
Collaborator Author

ivalaginja commented Apr 22, 2024

@ehpor note how this is the first camera in catkit2 that uses seconds as exposure time units, instead of microseconds as for the other cameras. I have formulated an issue around this here: #181

Edit: the service has been changed since to use microseconds as exposure time as well.

Comment on lines 211 to 214
if self.cam.buf_getframedata(-2) is False:
# If there is no second to last frame, it means we just acquired the first frame.
# In this case, drop the current frame since it always contains systematic errors.
break
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While testing in the lab, we learned that the first frame of each acquisition contains very obvious systematic errors, so it is unusable. It is probably rare that anyone would use the very first frame from a data stream, but it seemed more correct to me to never submit that faulty frame to the data stream in the first place.

I am not actually sure how to test this code block though... if you have any pointers @ehpor, please let me know.

@erinpougheon could I ask you to post a screenshot of the faulty frame here in the PR, for future reference?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: does this faulty frame reappear when you change ROI, exposure time, gain or other settings?

Yes, I'd never submit that frame to the data stream. I'm not sure what you mean by "test". Since this is hardware-only behaviour, you can't do unit tests unless you go for hardware-in-the-loop, which is a hassle to automate. If you mean, just testing that this behaves as expected, I'd put an info log message instead of just breaking.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good questions. I'll pass them on to @erinpougheon to test in the lab when she's back on Wednesday. FYI @johanmazoyer

I meant indeed to just test whether it behaves as expected. I'll do that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Faulty frame Here is a screenshot of the faulty frame.

@ivalaginja ivalaginja force-pushed the feature/hamamatsu_camera branch from a215f03 to 4e07258 Compare April 22, 2024 17:35
@ivalaginja ivalaginja marked this pull request as draft April 22, 2024 20:04
@ivalaginja
Copy link
Collaborator Author

ivalaginja commented Apr 24, 2024

Question: does this faulty frame reappear when you change ROI, exposure time, gain or other settings?

So it turns out that we haven't paid much attention to the proper way of property setting with this camera - or the Allied Vision cameras. Some of the properties need the camera acquisition to be temporarily stopped, so I opened an issue for this for the AV service here: #183

For the Hamamatsu camera, only the exposure time can be changed while the camera is acquiring, the other properties need a temporary stop of the acquisition as well. Since that retriggers the acquisition loop, yes the faulty frame would appear again, but it would also be automatically discarded again in the current implementation. As for resetting the exposure time, I don't actually know how to catch the exact first frame after resetting it to figure out whether it's fine or not...

We only have our loaner camera for one more day and we won't be able to implement and test this before we have to send it back. We won't get our own camera for a while. My idea would be to open an issue an put a rework of the properties that need an acquisition halt into it (#194), akin to the issue for the Allied Vision cameras. And to open a separate issue (#195) to figure out how to test the first frame upon an exposure time change to decide whether it's fine or whether we need to throw it away too.

What do you say?

@ivalaginja ivalaginja marked this pull request as ready for review April 24, 2024 12:57
@ivalaginja
Copy link
Collaborator Author

The tests on GitHub are failing for MacOS for some reason, but they are successful when I run them locally:

Screenshot 2024-04-24 at 15 01 21

@ehpor
Copy link
Collaborator

ehpor commented Apr 26, 2024

What do you say?

Yes, I'm fine with merging this early, with potentially incorrect first-frame handling and some other issues to be fixed in a later PR.

@ivalaginja ivalaginja force-pushed the feature/hamamatsu_camera branch 2 times, most recently from 6a89892 to f87f18d Compare April 26, 2024 13:12
@ehpor
Copy link
Collaborator

ehpor commented Apr 26, 2024

Let me know when this is ready for review, since you're still changing things.

@ivalaginja
Copy link
Collaborator Author

@ehpor I think this is actually ready for review now, unless I forgot anything that we talked about.

@ivalaginja ivalaginja force-pushed the feature/hamamatsu_camera branch from 7c804df to 153af19 Compare May 2, 2024 18:01
Copy link
Collaborator

@ehpor ehpor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@ivalaginja ivalaginja force-pushed the feature/hamamatsu_camera branch from 7793640 to d7edf0f Compare May 23, 2024 19:23
@ivalaginja ivalaginja enabled auto-merge May 23, 2024 19:23
@ivalaginja ivalaginja merged commit 4ae47e0 into develop May 23, 2024
6 checks passed
@ivalaginja ivalaginja deleted the feature/hamamatsu_camera branch May 23, 2024 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
collaborators Worked on by external collaborator. Might need some extra help on code integration. hardware Integrate new hardware
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add service for Hamamatsu camera
4 participants