Fix ExternalSeries acquisition when DataSource is None#95
Fix ExternalSeries acquisition when DataSource is None#95ericonr wants to merge 5 commits intoareaDetector:masterfrom
Conversation
file->refCount is already set to 'parse+save' in the pollTask loop that initializes all file objects.
ericonr
force-pushed
the
externalseries-datasource-none
branch
from
46640c4 to
d533dc0
Compare
|
When I call If I understand the eigerParam API correctly, |
|
You can call mState->fetch() to read from the detector in the parameter library first. You should have the lock held when accessing the parameter library. |
|
I think the code could be: |
|
Mark, that looks reasonable, thanks! I won't be able to test this on our detector until the 27th, unfortunately, so I will update the PR in the meantime but won't have results from it yet. I was worried about the ADEiger/eigerApp/src/eigerDetector.cpp Lines 1998 to 2004 in 84b9392 I wonder if this behavior could be made more flexible... as is, I only ever see |
eigerStatus is intended for polling auxiliary information like temperature, humidity, etc. that is not critical to acquisition. I agree that State_RBV should be moved from eigerStatus to a place where it is always updated during acquisition. We need to determine if there is a limit to how fast State can be queried without a performance impact. |
The original implementation had two main issues: - It called mFWState.get() without a corresponding fetch call, so it wasn't updating the value of the FileWriter state. Considering eigerStatus() doesn't update this value during an ongoing acquisition, it likely would be set to "ready" during this loop. - mFWState.get(), which reads a parameter from the param library, was called without holding the port lock. Both issues were solved with this commit. It was decided to keep the locking scheme around waiting for the FileWriter and Stream tasks the same, since these are conditional code paths, and it's simpler to reason about them by knowing we always start without holding the lock. We also added some rate limiting to the mFWState requests, using the same 0.1s delay as other driver tasks. The hardcoded 0.5s sleep, which was added in 960e6cc (Fix acquisition with External Enable mode, 2017-01-20) and increased in a4a4c30 (Major rewrite of how Eiger parameters are handled, 2017-04-17) was kept, since it's unclear if it can be removed.
The comment in the branch that handled TMExternalSeries and TMExternalEnable trigger modes was misleading, since it claimed the Eiger does not indicate when an acquisition is complete, even though the detector state parameter is available. This fix is important for the trigger modes above because otherwise they won't ever complete acquisitions where DataSource is None, because numImagesCounter won't be incremented. With this commit, such acquisitions can be completed. This logic, however, isn't specific to acquisitions with those trigger modes, so it can be applied to all of them equally, which simplifies the implementation. However, it also delays disarming the detector for the other trigger modes until the acquisition is over. Moving the disarm command to after waiting for the poll and stream tasks was considered, but that wouldn't have been enough if the driver was used to simply control the detector (DataSource is None and SaveFiles is Disabled). Doing this on top of the current commit, as a precaution, was also considered, but it would delay disarming and user feedback further for little gain. For the implementation, it was necessary to check for three possible states, because the detector state will be "configure" when sending the acquisition commands, then, when waiting for triggers, will be "ready", and ongoing acquisitions will indicate "acquiring". The change to the verification loop also allowed us to remove the epicsThreadSleep call and sleep while waiting for mStopEvent, because we can simply relinquish the lock for its entire duration and for the disarm command afterward.
mPollStop doesn't have a comment because access to it is currently not properly protected. mPollComplete is reset when writes to mPollQueue start, set before mPollDoneEvent is signaled, and read after waiting for mPollDoneEvent. mStreamComplete is reset before mStreamEvent is signaled, set before mStreamDoneEvent is signaled, and read after waiting for mStreamDoneEvent.
mPollStop is used from controlTask to signal pollTask to stop checking for data files, so reads and writes to it should be properly synchronized between these threads. Since it's only a flag, the simplest solution for this is making it atomic. To avoid complicating usage of the variable, we decided not to use stores and loads with explicit memory ordering, though it should be possible to use relaxed atomics for this purpose.
ericonr
force-pushed
the
externalseries-datasource-none
branch
from
d533dc0 to
e38b8dd
Compare
|
This PR now fixes #96 as well Pending testing on Monday, though! |
2 participants
Second commit is the main one, first one is just a small cleanup.
Reasoning is contained in the commit message fully.
Since this changes the logic to be shared for all trigger modes, I have managed to test it just fine for internal manual triggering. Will only be able to test it with external triggering tomorrow.