python-microscope / microscope

Python library for control of microscope devices, supporting hardware triggers and distribution of devices over the network for performance and flexibility.
https://www.python-microscope.org
GNU General Public License v3.0
66 stars 39 forks source link

Kinetix exposure time can be reported incorrectly. #205

Open iandobbie opened 3 years ago

iandobbie commented 3 years ago

Latest updated from photmetrics.

Just a little update. We have been able to duplicate the same behaviour on our cameras as well. The camera is reporting exposure time in nanoseconds. At 100ms of exposure for the sensitivity port (this is observed only for the sensitivity port), the camera reports 99.997938ms. The PVCam doesn’t round it up but shows only value before the decimal point.

Engineering is looking into it. Unfortunately, the fix will take a while. We will update you again once we know something more.

I suggest until such a time as it is fixed we just add 1ms to all exposure times to prevent missing triggers.

dstoychev commented 3 years ago

I think we may be better off removing this entire feedback mechanism of reading the exposure time parameter. It doesn't seem to be done in PyVCAM either.

iandobbie commented 3 years ago

No that’s not a good idea as many cameras don’t actually set the exposure time you ask for but something close. I suggest we hack in the extra 1ms but try to get it actually fixed in the driver. We had a similar issue on the and or camera in the past.

Ian

Sent from my iPhone

On 7 May 2021, at 16:19, dstoychev @.***> wrote:



I think we may be better off removing this entire feedback mechanism of reading the exposure time parameter. It doesn't seem to be done in PyVCAM either.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/python-microscope/microscope/issues/205#issuecomment-834513743, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABJTBBMSI64N4JB7M5TCSWTTMQAF5ANCNFSM44JVIZZA.

carandraug commented 3 years ago

No that’s not a good idea as many cameras don’t actually set the exposure time you ask for but something close.

+1. The IDS camera is the same thing. You always need to read back the exposure time after setting it to know exactly what you got.


Is the problem that PVCam always rounds up or is it function of the actual exposure time? If we set exposure time to longer values, like 2 seconds, when it's off, is it also by only 1ms?

I think if the driver has that issue then I don't see what other option we have. But please, add a comment on the code saying why and pointing to this issue number. Also, maybe describe the issue in https://github.com/python-microscope/vendor-issues/issues

dstoychev commented 3 years ago

The problem is not with rounding but truncation. See Ian's first message for a more detailed explanation, but it seems that they truncate some internal float to the integer value of the PARAM_EXPOSURE_TIME parameter. The error is unrelated to the units and it seems it could happen for all three (s, ms, us).

I made a fix today: https://github.com/dstoychev/microscope/commit/f66812af6776dcb192fbd8485cad4c3101aa7f16. I did some quick testing with both Kinetix and PRIME-BSI and the fix seems to work as expected.

carandraug commented 3 years ago

The proposed fix only applies correction for Kinetics cameras. Is this issue specific to Kinetix cameras? The Photometrics reply mentions PVcam which to me suggests that it applies to all cameras making use of pvcam. I propose that during initialize you set exposure to 100 and then read the time back. If it's 99, assume that the camera is this bug and have an instance attribute (something like _suffers_issue_205, _suffers_exposure_truncation, etc) which you check during enable. This also means that when photometrics fix the issue on their side, our code stops applying the workaround.

Also, is t_exp - t_readback == 1 meant to catch when the value set is off by one? If so, I don't that's correct. We know that the value set by the camera might be different, which is why we need to read it back. This means that it might be off by one but the truncation. Suppose you set the exposure to 980. However, the closest value the camera accepts is 980.9. The camera is sets to that value but because of this bug in the firmware, it is truncated to 980. You will see no difference between the two but the real exposure time is closer to 981.

dstoychev commented 3 years ago

The proposed fix only applies correction for Kinetics cameras. Is this issue specific to Kinetix cameras?

As far as I am aware, yes. Only Kinetix and only the Sensitivity mode.

Also, is t_exp - t_readback == 1 meant to catch when the value set is off by one?

Yes. Due to the nature of the problem, I believe it will always manifest as a difference of one. The value set by the camera should not be different and I have verified this with the manufacturer. This is also why I suggested above to remove the reading of the exposure time parameter altogether. Also, I was reassured by the engineers that the code which computes the internal exposure time of the camera has been present for a long time, presumably remaining mostly the same, and that it should work the same for most cameras supported by PVCAM. The problem is not really with how the exposure is calculated, but that the calculated value is converted to the PARAM_EXPOSURE_TIME parameter, which is broken only on Kinetix and only on the Sensitivity mode.


I think it's a good idea to have the check in self.initialize() instead of self._do_enable(), as you suggest. I will try to modify my branch when I find the time.

iandobbie commented 3 years ago

The latest response from Teledyne is:

It’s not necessary read back the exposure time using the parameter. As you have mentioned, the camera in fact gives 100ms on the expose out line. The expose out value of the PARAM_EXPOSURE_TIME parameter is computed by region of code that has been present in PVCAM for a very long time so we expect small discrepancies in nanoseconds in more cameras supported by PVCAM.

Which implies to me that they are not going to fix it. We should follow up.

dstoychev commented 2 years ago

I said it's a good idea to move the check from _do_enable() to initialize(), but trying to implement this I realised that it's not possible. The reason is that the check needs to compare the two exposure times (demanded vs received) and this only happens during _do_enable(). As such, the commit from above remains valid.