Sync v3.4.1 #18
Sync v3.4.1 #18
Conversation
Initial ADPimega MEDM screens Approved-by: Guilherme Paulino
By default the precision of ai records is 3, which is not enough to represent the minimum acquire time available (0.0005). This commit increases the precision to 6 decimal places.
…uest #140) Increase default precision for AcquireTime_RBV record Approved-by: Gabriel Amorim
handle high perf stream data 12 and 24 bits Approved-by: Álvaro Costa
EPICS_BASE_VERSION updated to 7.0.8.1 Approved-by: Álvaro Costa
Merge tag '3.4.0'
…t #144) Set PVs providing detector and vendor info Approved-by: Gabriel Amorim
initializeBufferPool was being initialized inside the connect function, make inaccessible outside it, and needing reinitialization to be able to receive the data from the high performance stream. Now it is called at the construction of pimegaDetector, correcting its initialization.
fix buffer initialization Approved-by: Álvaro Costa
…the Capture function and it was without any return, other gcc versions may ignore this lack of return but on gcc (Ubuntu 13.3.0-6ubuntu2~24.04) 13.3.0 this causes a sigill and crash the software
Added a return to the configureAlignment Approved-by: Álvaro Costa
Release/3.4.1
There was a problem hiding this comment.
This PR currently does not follow the commit and code standards we apply to our repositories. Since there's a certain urgency in merging, we decided to take the opportunity to present an example of a PR following our standards, which is available at #19 . This new PR would ideally have been split into multiple ones as well, but it was decided to match the PR 1 to 1.
The commit message diff is available below.
Further PRs will only be merged if they follow our code standards. We would also like to highlight that rectifying this PR took less than 1hr of development.
We weren't able to build the PR with all applied commits, because we don't have access to the new version of pimega-api; but we were able to test the parts we changed.
1: 976ee82 ! 1: 0fb900f Initial ADPimega MEDM screens
@@ Metadata
Author: Álvaro Costa <[email protected]>
## Commit message ##
- Initial ADPimega MEDM screens
+ Add initial ADPimega MEDM screens
## pimegaApp/op/Makefile (new) ##
@@
2: ca85f72 ! 2: 2428f67 Increase default precision for AcquireTime_RBV record
@@ Metadata
Author: Álvaro Costa <[email protected]>
## Commit message ##
- Increase default precision for AcquireTime_RBV record
+ Increase PREC for AcquireTime_RBV
- By default the precision of ai records is 3, which is not enough to
- represent the minimum acquire time available (0.0005). This commit
- increases the precision to 6 decimal places.
+ The precision of AcquireTime and AcquireTime_RBV is set to 3 by ADBase,
+ which is not enough to represent the minimum acquire time available
+ (0.0005). This commit increases the precision of AcquireTime_RBV to 6
+ decimal places, which is the same value already used by AcquireTime.
## pimegaApp/Db/pimega.template ##
@@ pimegaApp/Db/pimega.template: record(ao, "$(P)$(R)AcquireTime")
3: 3fd192e < -: ------- handle high perf stream data 12 and 24 bits
4: 2bf72a4 < -: ------- removed global counter depth
5: 543992d < -: ------- new class for handle the circullar buffer
-: ------- > 3: a7a9797 Remove PimegaNDArray class member
-: ------- > 4: 09be27d Handle high perf stream data for 12 and 24 bit images
6: 6779b84 ! 5: 48d83ab EPICS_BASE_VERSION updated to 7.0.8.1
@@ Metadata
Author: gamorim <[email protected]>
## Commit message ##
- EPICS_BASE_VERSION updated to 7.0.8.1
+ Update epics-base to 7.0.8.1
+
+ This is necessary in order to allow building on Ubuntu 24.04, since this
+ version includes workarounds for issues with FORTIFY_SOURCE.
## install/install_epics.sh ##
@@
7: 7e7e714 < -: ------- Set PVs providing detector and vendor info
8: f48265d < -: ------- Fix: initializeBufferPool not being properly initialized.
9: 420e488 < -: ------- ran pre-commit to pass jenkins build
-: ------- > 6: 8e422cf Set PVs providing detector and vendor info
10: 830959e ! 7: 5c34acf Added a return to the configureAlignment, this function is called by the Capture function and it was without any return, other gcc versions may ignore this lack of return but on gcc (Ubuntu 13.3.0-6ubuntu2~24.04) 13.3.0 this causes a sigill and crash the software
@@ Metadata
Author: gamorim <[email protected]>
## Commit message ##
- Added a return to the configureAlignment, this function is called by the Capture function and it was without any return, other gcc versions may ignore this lack of return but on gcc (Ubuntu 13.3.0-6ubuntu2~24.04) 13.3.0 this causes a sigill and crash the software
+ Add return statement to configureAlignment
+
+ This function is called by the Capture function and it didn't return
+ anything. Other GCC versions may ignore this lack of return but on GCC
+ 13.3.0 (Ubuntu 13.3.0-6ubuntu2~24.04) this causes a sigill and crashes
+ the software.
## pimegaApp/src/pimegaDetector.cpp ##
@@ pimegaApp/src/pimegaDetector.cpp: asynStatus pimegaDetector::configureAlignment(bool alignment_mode) {
11: 5845a3f ! 8: 40e36b0 added error handling to configureAlignment
@@ Metadata
Author: gamorim <[email protected]>
## Commit message ##
- added error handling to configureAlignment
+ Add error handling to configureAlignment
+
+ Co-authored-by: Érico Nogueira <[email protected]>
## pimegaApp/src/pimegaDetector.cpp ##
@@ pimegaApp/src/pimegaDetector.cpp: asynStatus pimegaDetector::configureAlignment(bool alignment_mode) {
- int numExposuresVar;
int numCaptureVar;
int max_num_capture = 2147483647;
-+ int rc = 0;
-+ int rc_NumCapture = 0;
if (alignment_mode) {
- set_numberExposures(pimega, max_num_capture);
- SetAcqParamCameraNumCapture(pimega, max_num_capture);
-+ rc = set_numberExposures(pimega, max_num_capture);
++ int rc = set_numberExposures(pimega, max_num_capture);
+ if (rc != PIMEGA_SUCCESS) {
+ error("Invalid number of exposures: %s\n", pimega_error_string(rc));
+ return asynError;
+ }
+ rc = SetAcqParamCameraNumCapture(pimega, max_num_capture);
-+ if (rc_NumCapture != PIMEGA_SUCCESS) {
++ if (rc != PIMEGA_SUCCESS) {
+ error("Invalid number of capture: %s\n", pimega_error_string(rc));
+ return asynError;
+ }
@@ pimegaApp/src/pimegaDetector.cpp: asynStatus pimegaDetector::configureAlignment(
getIntegerParam(ADNumExposures, &numExposuresVar);
- set_numberExposures(pimega, numExposuresVar);
getParameter(NDFileNumCapture, &numCaptureVar);
-+ rc = set_numberExposures(pimega, numExposuresVar);
++ int rc = set_numberExposures(pimega, numExposuresVar);
+ if (rc != PIMEGA_SUCCESS) {
+ error("Invalid number of exposures: %s\n", pimega_error_string(rc));
+ return asynError;
pimegaApp/src/pimegaDetector.cpp
Outdated
| @@ -37,22 +39,75 @@ static void acquisitionTaskC(void *drvPvt) { | |||
| pPvt->acqTask(); | |||
| } | |||
|
|
|||
| void pimegaDetector::updateEpicsFrame(vis_dtype *data) { | |||
| std::vector<NDArray *> ndArrayBuffer; | |||
There was a problem hiding this comment.
ADCore already implements a free array pool. It also implements functionality to preallocate a user-defined amount of buffers, which was released in ADCore R3-14. This has the advantage of being controllable by the user, who can select how many buffers make sense for their application and machine specifications.
Therefore, including your own circular buffer requires a lot of justification. Are your requirements so different from the rest of AreaDetector? If not, why can't improvements be done to the free pool already present in AreaDetector?
Beyond that, there's also the implementation issues with the design in use here. 20 is a small amount of buffers, especially for the max 2000 fps of a Pimega detector. If the data from the detector is fed into plugins, it is easily possible for those plugins to be unable to cope with 2000 fps directly, and have to keep frames in their queue to process at a lower rate; once that queue passes 20 frames, the same NDArray object will have to be in the queue twice (20 is the value used currently, but this would be an issue for any value used). Your circular buffer doesn't check if the NDArray objects it returns are still in use, so it will overwrite the data that might still be in use, corrupting the final acquisition result (especially relevant for users saving into files using AreaDetector!). To guarantee safety, it's better to have a memory pool with an explicit free pool, at which point we return to the implementation already in use and made available by ADCore.
pimegaApp/src/pimegaDetector.cpp
Outdated
| this->updateEpicsFrame(reinterpret_cast<vis_dtype *>(data)); | ||
| if (counter_depth == 3) { | ||
| this->updateEpicsFrame(reinterpret_cast<uint32_t *>(data)); | ||
| } else { | ||
| this->updateEpicsFrame(reinterpret_cast<uint16_t *>(data)); | ||
| } |
There was a problem hiding this comment.
It's simpler to pass an NDDataType_t argument to updateEpicsFrame than to duplicate its implementation simply to receive different kinds of pointer, especially considering that the function simply treats it as a void pointer anyway.
| setParameter(NDDataType, NDUInt32); | ||
| setParameter(NDDataType, NDUInt16); |
There was a problem hiding this comment.
Careless find-replace
This one is of little consequence, since this parameter is only set here and not actually used by the driver.
| #define DRIVER_VERSION_MAJOR 3 | ||
| #define DRIVER_VERSION_MINOR 4 | ||
| #define DRIVER_VERSION_PATCH 0 |
There was a problem hiding this comment.
Hardcoding the driver version is error prone and involves manual work. Case in point, this branch is tag 3.4.1 but the file still says 3.4.0
| DRIVER_VERSION_MINOR, DRIVER_VERSION_PATCH); | ||
|
|
||
| setParameter(NDDriverVersion, version); | ||
| setParameter(ADSDKVersion, version); |
There was a problem hiding this comment.
The SDK here is pimega-api, not the driver itself. This one should be left blank until pimega-api provides a way to query its version.
pimegaApp/src/pimegaDetector.cpp
Outdated
| int rc_NumCapture = 0; | ||
| if (alignment_mode) { | ||
| set_numberExposures(pimega, max_num_capture); | ||
| SetAcqParamCameraNumCapture(pimega, max_num_capture); | ||
| rc = set_numberExposures(pimega, max_num_capture); | ||
| if (rc != PIMEGA_SUCCESS) { | ||
| error("Invalid number of exposures: %s\n", pimega_error_string(rc)); | ||
| return asynError; | ||
| } | ||
| rc = SetAcqParamCameraNumCapture(pimega, max_num_capture); | ||
| if (rc_NumCapture != PIMEGA_SUCCESS) { |
There was a problem hiding this comment.
rc_NumCapture is never set, but is used in the conditional as if it was.
| PimegaDacCountScanResult = | ||
| this->pNDArrayPool->alloc(2, shape, NDUInt32, 0, NULL); | ||
| this->pNDArrayPool->alloc(2, shape, NDUInt16, 0, NULL); | ||
| memcpy(PimegaDacCountScanResult->pData, counts, | ||
| PimegaDacCountScanResult->dataSize); |
There was a problem hiding this comment.
Careless find-replace
This one has greater significance. counts is an uint32_t *, so allocating a vector with the same length, but for 16-bit variables, means it's half the size (in bytes), and that the data will be interpreted incorrectly (each element will be part of a 32-bit value). Thankfully, the memcpy call used PimegaDacCountScanResult->dataSize, so there wasn't a memory corruption issue.
| @@ -1444,14 +1472,20 @@ void pimegaDetector::connect(const char *address[10], unsigned short port, | |||
|
|
|||
| char connection_address[1024]; | |||
| sprintf(connection_address, "tcp://127.0.0.1:%d", vis_frame_port); | |||
| const std::string visualizer_topic = "pimega_frame_visualizer"; | |||
| const size_t max_frame_size = maxSizeX * maxSizeY * sizeof(vis_dtype); | |||
| const std::string visualizer_topic = "pimega_high_perf"; | |||
There was a problem hiding this comment.
The usage of the pimega_high_perf subscription adds a requirement to switch to a different backend port. Since the dac count scan functions use the same configured port to receive acquired frames, they will break, because the dac count scan functions use the pimega_frame_visualizer subscription, which is only available from the original backend port.
Release Notes
Feature: Added a basic MEDM screen for the ADPimega driver.
This screen allows basic operations and access to the plugins but
will be incremented with more operations later.
It provides a standard interface to match what is offered by other
detectors on AreaDetector repository.
Feature: Support for handle new high perf stream data.
The new high performance stream from the api implements a variable size
of the data considering the counter depth used, this grants optimize the
size transferred and a better performance, because of this a selection of
the data type is needed, and a internal circular buffer help on the optimization
and speed.
Feature: EPICS_BASE_VERSION updated to 7.0.8.1 on install script
This update is nescessary to be compatible with ubuntu 24.04
there is a fix on this epics version regarding FORTIFY_SOURCE
to avoid fail on build EPICS base on this ubuntu version.
Fix: Increase default precision for AcquireTime_RBV
By default the precision of ai records is 3, which is not enough to
represent the minimum acquire time available (0.0005). This commit
increases the precision to 6 decimal places.
Fix: Added missing detector info pvs
Set PVs providing detector and vendor info. Information usually present on the MEDM's screens
Fix: Added a return and error management to the configureAlignment function, avoiding sigill
Added a return to the configureAlignment, this function is called by the
Capture function and it was without any return, other
gcc versions may ignore this lack of return but on gcc 13.3.0 (Ubuntu
13.3.0-6ubuntu2~24.04) this causes a sigill and crash the software