prosilica.cpp: #4
Conversation
The following changes were made to frameCallback - check the return value of alloc before adding a newly created NDArray to the next frame - print an error message if a frame can't be requeued - if available, use the frame time to set the time structures in the associated NDArray - limit the number of PvAPI frames to the maximum allowed NDArrays
|
Do you forsee merging this pull request? I'm not sure if the epics time stamp is being set correctly. |
|
Yes, I do foresee merging it. I want to look at it more closely, and won’t have time for that before Wednesday. Mark From: jabrnthy [mailto:[email protected]] Do you forsee merging this pull request? I'm not sure if the epics time stamp is being set correctly. — |
|
Hi Mark, this is just a friendly reminder that this pull request awaits your review. |
|
Hi Jason, I am finally getting around to looking at your pull request.
I don’t think I agree with this change. It breaks the support I added for user-defined timestamps in asyn R4-22 and areaDetector R2-0. Please look at this document: ADCore/documentation/areaDetectorTimeStampSupport.html You are modifying the epicsTimeStamp via epicsTimeAddSeconds. But this will not work for sites like LCLS which are using the low-order bits of the epicsTimeStamp to encode the pulse ID. Now I guess if they never do PSResetTimer then it might be OK because lastSyncTime would be 0. But I don’t actually see where lastSyncTime is ever initialized to 0, do you? This line seems wrong to me:
What if maxBuffers is 0, as it often is when the user does not want to limit the maximum number of buffers? Is there a reason to limit maxPvAPIFrames_ to maxBuffers? Mark From: Mark Rivers Yes, I do foresee merging it. I want to look at it more closely, and won’t have time for that before Wednesday. Mark From: jabrnthy [mailto:[email protected]] Do you forsee merging this pull request? I'm not sure if the epics time stamp is being set correctly. — |
|
Hi Mark, Thanks for the feedback. | Please look at this document: After reviewing the document, I am unable to see how adding support for the hardware clock can be added. It seems like there is an "all or nothing" approach to using TSE = -2 because the functionality to specify that "only" the NDArray should have a user-defined timestamp is lacking. Perhaps you can see another way? | But this will not work for sites like LCLS which are using the low-order bits of the epicsTimeStamp to encode the pulse ID The epicsTimeStamp has a well-defined meaning. It's awkward to develop driver features around non-standard behaviour. Perhaps it would make more sense for the pulse ID to be set within the attribute list of the NDArray? Even better, within NDArray->timeStamp. | I don’t actually see where lastSyncTime is ever initialized to 0, do you? You're absolutely correct. My original implementation was based on the C++ data structure which initializes itself during construction. The "C" version should be initialized within the prosilica constructor. | What if maxBuffers is 0, as it often is when the user does not want to limit the maximum number of buffers? You're absolutely right. That line should read: if (maxPvAPIFrames_ > maxBuffers && (0 != maxBuffers) ) maxPvAPIFrames_ = maxBuffers; I added the check to prevent the driver from entering a strange stage where it cannot connect to the camera: https://github.com/jabrnthy/ADProsilica/blob/master/prosilicaApp/src/prosilica.cpp#L1387 If maxPvAPIFrames_ > maxBuffers then the NDArrayPool will run out of buffers and cause connectCamera to return an error. |
|
I'd like to make the changes which fix the undefined behaviour that results from lastSyncTime not being initialized, but I'm not sure what to do about setting the NDArray->epicsTS with the camera's hardware clock. I'll wait until I hear a response before making any modifications to my branch. |
EPICS base supports a General Time facility. This allows site-specific time sources that may use hardware to provide very precise timestamps across many IOCs. asynPortDriver::updateTimeStamp can retrieve that time. But your code is always modifying that time in a way that may be undesirable for a specific site. LCLS happens to use the low-order bits in a different way, but the point is that if a site has General Time source they are not going to want the time stamp to be modified by the driver, they want the time stamp that their site-specific General Time source provides. Using NDArray->timeStamp won’t work for 2 reasons:
The solution I would recommend that you investigate is to take advantage of this ability to write a site-specific timestamp function. As my document explained you can write your own timestamp routine which will be called by updateTimeStamp(). That routine can get the time from the camera frame and set the timestamp accordingly. Then you have complete freedom to set the timestamps as you would like for your installation. You need to call pasynManager->registerTimeStampSource(asynUser *pasynUser, void *userPvt, timeStampCallback callback). You would need to write a little C function that takes your Prosilica driver port name, connects an asynUser to it, and registers a callback function. The question is whether it would be possible to get access to the pFrame->Timestamp info in some way. Mark From: jabrnthy [mailto:[email protected]] Hi Mark, Thanks for the feedback. | Please look at this document: After reviewing the document, I am unable to see how adding support for the hardware clock can be added. It seems like there is an "all or nothing" approach to using TSE = -2 because the functionality to specify that "only" the NDArray should have a user-defined timestamp is lacking. Perhaps you can see another way? | But this will not work for sites like LCLS which are using the low-order bits of the epicsTimeStamp to encode the pulse ID The epicsTimeStamp has a well-defined meaning. It's awkward to develop driver features around non-standard behaviour. Perhaps it would make more sense for the pulse ID to be set within the attribute list of the NDArray? Even better, within NDArray->timeStamp. | I don’t actually see where lastSyncTime is ever initialized to 0, do you? You're absolutely correct. My original implementation was based on the C++ data structure which initializes itself during construction. The "C" version should be initialized within the prosilica constructor. | What if maxBuffers is 0, as it often is when the user does not want to limit the maximum number of buffers? You're absolutely right. That line should read: if (maxPvAPIFrames_ > maxBuffers && (0 != maxBuffers) ) maxPvAPIFrames_ = maxBuffers; I added the check to prevent the driver from entering a strange stage where it cannot connect to the camera: https://github.com/jabrnthy/ADProsilica/blob/master/prosilicaApp/src/prosilica.cpp#L1387 If maxPvAPIFrames_ > maxBuffers then the NDArrayPool will run out of buffers and cause connectCamera to return an error. — |
|
LCLS is using the EPICS General Time facility, but isn't it still in a non-standard way? The fields of epicsTimeStamp are documented: /* epics time stamp for C interface_/ It looks like General Time was meant to allow custom time providers, not custom definitions of the epicsTimeStamp. General Time queries each provider based on priority. If the LCLS timer fails, won't the insertion of a "fail safe" time cause undefined behaviour? That aside, I'm still unsatisfied with the proposal. Won't the custom routine set the time stamp for the entire port instead of only the NDArray? |
But if someone has a custom time provider they probably don't want that time modified by the Prosilica driver.
Only for functions in the areaDetector driver that call pasynManager->getTimeStamp. The only code that does that in any existing areaDetector driver is the code that sets the epicsTS in the NDArray. Mark From: jabrnthy [[email protected]] LCLS is using the EPICS General Time facility, but isn't it still in a non-standard way? The fields of epicsTimeStamp are documented: /* epics time stamp for C interface_/ It looks like General Time was meant to allow custom time providers, not custom definitions of the epicsTimeStamp. General Time queries each provider based on priority. If the LCLS timer fails, won't the insertion of a "fail safe" time cause undefined behaviour? That aside, I'm still unsatisfied with the proposal. Won't the custom routine set the time stamp for the entire port instead of only the NDArray? — |
Yes, that's a good point. How about adding a new parameter to the Prosilica driver: PSNDEpicsTSSource? Setting it to PSNDEpicsTSSourceDefault will use updateTimeStamp and PSNDEpicsTSSourceFrame will use the values in pFrame. |
The following changes were made to frameCallback