BUG: fix wrong DVF parameters in class constants - #1242
Conversation
henriquesimoes
left a comment
henriquesimoes
left a comment
There was a problem hiding this comment.
I noticed this is still a draft, but I'm commenting on everything I noticed nevertheless.
You probably haven't had time to actually implement the use of the new IMAGE_NR_PIXEL_MULTP parameter, but I left comment showing where I would expect it to be used.
|
|
||
| def cam_roi_calc(self, roix_fwhm_factor, roiy_fwhm_factor): | ||
| """Return ROI based on FWHM factors.""" | ||
| multp = 4 |
There was a problem hiding this comment.
I was expecting that the new device specific constant IMAGE_NR_PIXEL_MULTP would be used here.
| DEVICES.CAX_DVF1: _get_namedtuple( | ||
| # Basler acA1300-75gm (version 106755-24) | ||
| 'DVFParameters', | ||
| _dvfparam_fields, | ||
| (16, 0.5, 0.5, 0.100, 2064, 3088, 2.4, 5.0), | ||
| (16, 16, 0.5, 0.5, 0.100, 2064, 3088, 2.4, 5.0), | ||
| ), |
There was a problem hiding this comment.
As I mentioned in the deprecated PR, this camera (acA1300-75gm) has a
Can't we always use the existing cam_max_sizex and cam_max_sizey methods (which read the size of the sensor from the IOC) instead of redefining those constants? As of now, it seems dvfimgproc/csdev.py and dvfimgproc/meas.py consume these IMAGE_SIZE_Y and IMAGE_SIZE_X constants defined here.
I just noticed the pixel size is also wrong, because it was replicated from DVF2. It is
TL;DR: If we keep redefining the size constants, it should be at least updated to the following:
| DEVICES.CAX_DVF1: _get_namedtuple( | |
| # Basler acA1300-75gm (version 106755-24) | |
| 'DVFParameters', | |
| _dvfparam_fields, | |
| (16, 0.5, 0.5, 0.100, 2064, 3088, 2.4, 5.0), | |
| (16, 16, 0.5, 0.5, 0.100, 2064, 3088, 2.4, 5.0), | |
| ), | |
| DEVICES.CAX_DVF1: _get_namedtuple( | |
| # Basler acA1300-75gm (version 106755-24) | |
| 'DVFParameters', | |
| _dvfparam_fields, | |
| (16, 16, 0.5, 0.5, 0.100, 1024, 1280, 4.8, 5.0), | |
| ), |
| DEVICES.CAX_DVF2: _get_namedtuple( | ||
| 'DVFParameters', | ||
| _dvfparam_fields, | ||
| (16, 0.5, 0.5, 0.100, 2064, 3088, 2.4, 5.0), | ||
| (16, 16, 0.5, 0.5, 0.100, 2064, 3088, 2.4, 5.0), | ||
| ), |
There was a problem hiding this comment.
This camera model is acA3088-16gm (version 107406-12), and its IMAGE_NR_PIXEL_MULTP equals to 4, not 16. (Yeah, that's the original one we used for developing the device, and where the constant 4 came from in cam_roi_calc).
Thus:
| DEVICES.CAX_DVF2: _get_namedtuple( | |
| 'DVFParameters', | |
| _dvfparam_fields, | |
| (16, 0.5, 0.5, 0.100, 2064, 3088, 2.4, 5.0), | |
| (16, 16, 0.5, 0.5, 0.100, 2064, 3088, 2.4, 5.0), | |
| ), | |
| DEVICES.CAX_DVF2: _get_namedtuple( | |
| 'DVFParameters', | |
| _dvfparam_fields, | |
| (16, 4, 0.5, 0.5, 0.100, 2064, 3088, 2.4, 5.0), | |
| ), |
|
|
||
| def cmd_reset(self, timeout=None): | ||
| """Reset DVF to a standard configuration.""" | ||
| # TODO: is reseting BASLER roi necessary? |
There was a problem hiding this comment.
If you want to ensure it will always be in a valid state, I think so. Otherwise, the client of the DVF class will need to know the image might be cut to an arbitrary and possibly irrelevant portion of the sensor.
Why do you think it might not be needed?
| 'cam1:PixelFormat': 1, # Mono12 | ||
| # ROI1 takes images from camera driver | ||
| 'ROI1:NDArrayPort': self['cam1:PortName_RBV'], | ||
| # TODO: check if these properties are necessary to be set on reset | ||
| # 'cam1:DataType': 1, # UInt16 (maybe unnecessary) |
There was a problem hiding this comment.
Because we set cam1:PixelFormat to Mono12, the IOC will automatically update the cam1:DataType to UInt16.
| # TODO: check if these properties are necessary to be set on reset | ||
| # 'cam1:DataType': 1, # UInt16 (maybe unnecessary) | ||
| # 'cam1:NumImages': 1, # number of sequential acquired images | ||
| # 'cam1:ExposureMode': 0, # TIMED |
There was a problem hiding this comment.
It seems our cameras (both models relevant here) don't support any ExposureMode other than "timed". So this one in fact isn't currently needed.
| 'ROI1:NDArrayPort': self['cam1:PortName_RBV'], | ||
| # TODO: check if these properties are necessary to be set on reset | ||
| # 'cam1:DataType': 1, # UInt16 (maybe unnecessary) | ||
| # 'cam1:NumImages': 1, # number of sequential acquired images |
There was a problem hiding this comment.
If we use ImageMode as Continuous, this PV is irrelevant. Is there any scenario which we use other acquisition mode?
| # 'cam1:DataType': 1, # UInt16 (maybe unnecessary) | ||
| # 'cam1:NumImages': 1, # number of sequential acquired images | ||
| # 'cam1:ExposureMode': 0, # TIMED | ||
| # 'cam1:TriggerMode': 0, # Off |
There was a problem hiding this comment.
If we ever change the TriggerMode, it seems relevant to ensure this is off. Otherwise, the camera won't acquire because no external trigger will come, right?
I've never configured these cameras to use external trigger, but it seems that's one of the many parameters required to use that mode of operation.
2 participants
replacing #1032