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

Reset properties while acquisition in Hamamatsu camera service #211

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

erinpougheon
Copy link
Collaborator

Fixes #194.

Changing the Hamamatsu Camera service so we will be able to change certain properties without having to stop the acquisition.

@erinpougheon erinpougheon self-assigned this Jun 4, 2024
@erinpougheon
Copy link
Collaborator Author

With inspiration from the FLIR camera (and Alied Vision Camera), I changed the way properties are created, so they can all be changed without having to stop mannualy the acquisition.
I will have to run some tests to verify when the Hamamatsu Camera will arrive (and also to be sure which property needs the acquisition to be stopped to be modified).
I had to make a distinction in the _create_propery method for the exposure time property, due to units that are used.
I have a doubt on how I'm using the getattr in the setter and getter (see l.29, 31, 47, 49).

A software review can be done, knowing this will require to be tested.

@erinpougheon erinpougheon requested a review from ivalaginja June 4, 2024 09:53
Copy link
Collaborator

@ivalaginja ivalaginja left a comment

Choose a reason for hiding this comment

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

Looking good! I think I know what tripped you over with the getters and setters, so here comes a solution.

def getter(self):
with self.mutex:
if property_name != 'EXPOSURETIME':
return getattr(self.cam, property_name).prop_getvalue(dcam.DCAM_IDPROP.property_name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since property_name is a string, we cannot call it - this is exactly why we use gettattr(). And property_name is a property of dcam.DCAM_IDPROP, so that is what we have to call getattr() on.

The correct way to write this is, for a getter:

return self.cam.prop_getvalue(getattr(dcam.DCAM_IDPROP, property_name))

and for a setter:

self.cam.prop_setvalue(getattr(dcam.DCAM_IDPROP, property_name), value)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thank you, I changed that and I understand better now !

@@ -21,6 +22,38 @@
raise


def _create_property(property_name, read_only=False, stopped_acquisition=True):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't point this out in the AV PR, but since this function _create_property() is specific to Hamamtsu cameras because of the getters and setters, lets call property_name -> hamamatsu_property_name instead. It's a bit long, but helps for understanding.

Also, could I maybe ask you to do a code-sneak and also replace that variable in the AV service with av_property_name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done for both services ;)

Copy link
Collaborator

@ivalaginja ivalaginja left a comment

Choose a reason for hiding this comment

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

This looks good to me now! We will just put this PR on standby until we can test it on hardware.

@ivalaginja ivalaginja added hardware Integrate new hardware collaborators Worked on by external collaborator. Might need some extra help on code integration. labels Jun 4, 2024
@erinpougheon erinpougheon force-pushed the feature/hamamatsu_cam_properties branch 3 times, most recently from 0147adc to 5cca964 Compare June 6, 2024 11:32
@erinpougheon erinpougheon force-pushed the feature/hamamatsu_cam_properties branch from 5cca964 to 632c0fc Compare June 28, 2024 14:32
@erinpougheon erinpougheon force-pushed the feature/hamamatsu_cam_properties branch from 632c0fc to a11e08b Compare July 9, 2024 12:12
@erinpougheon erinpougheon force-pushed the feature/hamamatsu_cam_properties branch from a11e08b to 8323087 Compare July 16, 2024 14:46
@ivalaginja
Copy link
Collaborator

@erinpougheon since the camera delivery will be delayed, could you maybe post a brief bullet-point list here what needs to be done in terms of tests once we have the new camera to wrap up this PR?

@erinpougheon
Copy link
Collaborator Author

Once the camera arrives:

• Determine which properties can be changed on the fly (go and test in an
ipython terminal, with the service open in another terminal). You need to
have to access the camera, try to change one of the properties, and then
check if it has changed. The properties to test are : exposure time, gain,
brightness, width, height, offset on x, offset on y.
• Check if the properties are well ’sorted’ in the Hamamatsu properties
branch on catkit2 (in this PR). 2 categories : the properties
that can be changed without having to stop the acquisition, or the ones
that can not.
• Test the service in the catkit2 branch, so see if we are now able to change
these properties

@ivalaginja
Copy link
Collaborator

As an update, we received the new camera and will be integrating it in the following weeks, which means we will be able to address this PR.

@ivalaginja
Copy link
Collaborator

We got our new camera and @johanmazoyer is working on it now.

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.

Stop acquisition when required to reset certain properties in Hamamatsu camera service
3 participants