areaDetector / ADGenICam

areaDetector base class for GenICam cameras.
https://areadetector.github.io/areaDetector/ADGenICam/ADGenICam.html
8 stars 17 forks source link

Added check for acquire state when acquisition is started. #32

Closed AbdallaDalleh closed 1 year ago

AbdallaDalleh commented 1 year ago

This prevents additional buffers allocation when setting ADAcquire = 1.

MarkRivers commented 1 year ago

@AbdallaDalleh have you tested this?

I think it might be better to test the current value of ADAcquire rather than ADStatus. Not all drivers update ADStatus when they should, since that field is mainly for users to see the status, not to control the operation.

You would need to save the current value of ADSAcquire before the call to setIntegerParam at the beginning.

AbdallaDalleh commented 1 year ago

@AbdallaDalleh have you tested this?

I think it might be better to test the current value of ADAcquire rather than ADStatus. Not all drivers update ADStatus when they should, since that field is mainly for users to see the status, not to control the operation.

You would need to save the current value of ADSAcquire before the call to setIntegerParam at the beginning.

@MarkRivers I tested it on a running Basler camera and it is working fine, the acquire command never allocates when acquiring. But what you are saying makes sense, I did not notice the setIntegerParam at the beginning, at first I tried ADAcquire but it was always 1 when pressing "Start" so the driver never allocates buffers. I will test it tomorrow and update you.

AbdallaDalleh commented 1 year ago

This also did not work, value of ADAcquire is 1 even before the setIntegerParam function, same for ADAcquireBusy. To overcome this, I added a flag which is set inside the if statement, this indicates whether the detector was acquiring or not. I tested it and it is working. Have a look at the fork here https://github.com/AbdallaDalleh/ADGenICam/commit/fc4cc39839a0846102105e29833251a51b478941

I noticed another similar flag "mWasAcquiring" which is used to pause/resume acquisition when setting image parameters, I thought to use a different flag to not confuse the functionality here.

AbdallaDalleh commented 1 year ago

To elaborate more, what happened is, when I press start, the GUI shows that it is collecting and acquire busy is set to Acquiring, but the driver never allocates buffers and so the detector state is always set to idle. This scenario happened when I checked the values of ADAcquire and ADAcquireBusy.

MarkRivers commented 1 year ago

To elaborate more, what happened is, when I press start, the GUI shows that it is collecting and acquire busy is set to Acquiring, but the driver never allocates buffers and so the detector state is always set to idle. This scenario happened when I checked the values of ADAcquire and ADAcquireBusy.

This does not make sense to me. Using a local variable and checking the value of ADAcquire at the top of writeInt32() is how other drivers do it, including ADSimDetector.

https://github.com/areaDetector/ADSimDetector/blob/4b236f4fa08a739868d569f39d3f633c3cd15d31/simDetectorApp/src/simDetector.cpp#L925

AbdallaDalleh commented 1 year ago

I just tested again and I get the same behavior, both the value and ADAcquire are 1 at the top of the function before setIntegerParam so there is no way it will ever start capturing.

MarkRivers commented 1 year ago

I now understand why this is happening. The derived class method, ADAravis::writeInt32 is called before ADGenICam::writeInt32. ADAravis::writeInt32 sets the value of ADAcquire in the parameter library, so as you observed it is already 1 when ADGenICam::writeInt32 is called, even though acquisition has not started.

Unfortunately I don't think your solution will work because your are only setting isAcquiring=false when ADAcquire is explicitly set to 0. But what if ADImageMode is set to Single or Multiple? Then acquisition will automatically stop, but isAcquiring will not be set to false. I am quite sure that if you try Single or Multiple mode and wait for acquisition to stop that you won't be able to restart it, because isAcquiring is still true.

MarkRivers commented 1 year ago

I believe I have fixed the problem on the master branch of ADGenICam and ADAravis with these commits. https://github.com/areaDetector/ADGenICam/commit/adae668603912b008447499309a3a7f48b3f8951 https://github.com/areaDetector/ADAravis/commit/d9140060b8c42eaaabd2b45a5e6b4e0549044a8f

The other 2 detectors that derive from ADGenICam are ADSpinnaker and ADVimba. They do not implement the ::writeInt32 method so I did not need to change them.

Please test the master branch of ADGenICam and ADAravis and see if the problem is fixed for you.

AbdallaDalleh commented 1 year ago

Thanks @MarkRivers for the update. I will test it next week and update you as soon as possible.

AbdallaDalleh commented 1 year ago

I pulled the master branches for both ADAravis and ADGenICam, the problem is still there. I will do more tests on the next shutdown.

MarkRivers commented 1 year ago

I pulled the master branches for both ADAravis and ADGenICam, the problem is still there.

Please enable ASYN_TRACEIO_DRIVER and post the output when you start acquire, both when it is not currently acquiring, and when it is currently acquiring.

MarkRivers commented 1 year ago

@AbdallaDalleh I tested the new version and I cannot reproduce the problem. Have you had a chance to test it?

AbdallaDalleh commented 1 year ago

Hello @MarkRivers , unfortunately I have not been able to test as the camera is running for some measurements. I will test it on the nearest shutdown.

AbdallaDalleh commented 1 year ago

Hello @MarkRivers , I did a new setup where I pulled the master branch for all the modules and it did not happen. Seems to me it is OK for now. But right now I have a new issue where the camera suddenly gets a disconnected state for some reason, it is ping-able but unable to fetch new images so I have to press "Reset Camera" button on the GUI.

MarkRivers commented 1 year ago

But right now I have a new issue where the camera suddenly gets a disconnected state for some reason, it is ping-able but unable to fetch new images so I have to press "Reset Camera" button on the GUI.

I think that must be another issue. I would like to close this PR and issue #31 because I think this issue is solved. You can open a new issue for the disconnect problem. @AbdallaDalleh is that OK with you?

If the disconnect issue is happening with a Basler camera then I suggest you might want to try the new ADPylon driver (https://github.com/areaDetector/ADPylon) which was just added. That uses the Basler Pylon SDK and it would be interesting to see if you have the same problem with that.

AbdallaDalleh commented 1 year ago

No problem at all. I will look into this new driver and let you know. Thank you very much.

MarkRivers commented 1 year ago

Fixed via adae668 and https://github.com/areaDetector/ADAravis/commit/d9140060b8c42eaaabd2b45a5e6b4e0549044a8f.