Convert NDPluginPva to use PVXS #532
Conversation
jsouter
changed the title
ericonr
left a comment
ericonr
left a comment
There was a problem hiding this comment.
The first commit breaks the build, because it uses the copied (pvAccess) versions of the files, but links everything against PVXS. Maybe it would be best to do "copy files" -> "convert into PVXS" -> "hook up build system"?
Updating headers before implementation also breaks the build... I think it's easier to understand to have a single commit for implementation and header that can be verified at once, instead of splitting it.
What do you think?
Thanks for the comments, I'll try and go through them soon. I think this is a reasonable change, also happy to squash things down more if needed. |
jsouter
force-pushed
the
pvxs
branch
3 times, most recently
from
f5a9c57 to
834fad7
Compare
ADApp/commonDriverMakefile
Outdated
| endif | ||
|
|
||
| ifeq ($(WITH_PVA),YES) | ||
| ifeq ($(WITH_PVXS),YES) |
There was a problem hiding this comment.
fyi. PVXS automatically provides some Make variables for testing presence and version.
https://github.com/epics-base/pvxs/blob/master/configure/CONFIG_PVXS_VERSION#L5-L14
eg.
ifdef PVXS_1_3_0There was a problem hiding this comment.
Thanks for the review!
This is good to know, though maybe we still want to be explicit about selecting the PVXS backend if both PVA and PVXS are provided, considering that it changes the public signature of the NTNDArrayConverter constructor. What do you think @ericonr?
I suppose with how the build works right now that any other modules using ADCore as a dependency (like pvaDriver) will only be able to see the libraries for one implementation at a time even though it can see both versions of the headers, probably I should fix it so that we build both libraries.
There was a problem hiding this comment.
The form I would encourage is:
# Link QSRV
ifdef PVXS_1_3_0 # prefer v2
myioc_DBD += pvxsIoc.dbd
myioc_LIBS += pvxsIoc pvxs
else
ifdef EPICS_QSRV_MAJOR_VERSION # fallback to v1
myioc_LIBS += qsrv
myioc_LIBS += $(EPICS_BASE_PVA_CORE_LIBS)
myioc_DBD += PVAServerRegister.dbd
myioc_DBD += qsrv.dbd
endif
endifWhich detects either PVXS or pva2pva.
ADApp/pluginSrc/NDPluginPva_pvxs.cpp
Outdated
| bool NTNDArrayRecord::init () | ||
| { | ||
| m_pv = pvxs::server::SharedPV(pvxs::server::SharedPV::buildReadonly()); | ||
| m_pv.open(m_value); | ||
| m_server = pvxs::server::Server::fromEnv(); | ||
| m_server.addPV(m_name, m_pv); | ||
| m_server.start(); // start is not blocking | ||
| m_converter.reset(new NTNDArrayConverter(m_value)); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
To discuss: do we want to open a server per plugin instance?
There was a problem hiding this comment.
You should not need to create a Server at all. Instead use pvxs::ioc::server().
There was a problem hiding this comment.
@mdavidsaver how does that interact with my comment about pvxsIoc.dbd #532 (comment) ? Is including pvxsIoc.dbd our responsibility, or the final integrator's when building the IOC?
And the documentation for the plugin should therefore state that the IOC needs to be built with QSRV2 and it shouldn't be disabled at runtime, right? Or should there be a fallback where if pvxs::ioc::server() throws, we create our own server?
Right now, wouldn't this throw anyway? The documentation states
This Server instance is created during a registrar function, started by the initHookAfterCaServerRunning phase of iocInit().
But NTNDArrayRecord::init is called by NTNDArrayRecord::create, which is called by the NDPluginPva initializer, which should always happen before iocInit.
| LIBRARY_IOC += ntndArrayConverter | ||
| INC += ntndArrayConverterAPI.h | ||
| ifeq ($(WITH_PVXS), YES) | ||
| INC += ntndArrayConverter.h | ||
| LIB_SRCS += ntndArrayConverter_pvxs.cpp | ||
| USR_CPPFLAGS += -DHAVE_PVXS | ||
| else | ||
| INC += ntndArrayConverter.h | ||
| LIB_SRCS += ntndArrayConverter.cpp | ||
| endif |
There was a problem hiding this comment.
Currently this builds only one implementation, should we be building both versions to allow users to switch between pvAccess and pvxs without rebuilding ADCore?
There was a problem hiding this comment.
I feel like that's too much flexibility without clear differences between versions. IMO choosing between one or the other should be left to the EPICS "distributor", to simplify things.
But I am biased, because I didn't even think it was necessary to keep supporting the old PVA libraries, since PVXS can always be installed as a separate module.
There was a problem hiding this comment.
yes, I'd vote for PVXS only unless there is a real need for the old libraries. The network protocol is backwards compatible (AFAIK)
There was a problem hiding this comment.
I would prefer compatibility with the old libraries to be retained, at least until PVXS is made part of base. We have local areaDetector applications that make use of PVAccess/PVData libraries as well as the pvaDriver and pvaPlugin modules packaged with areaDetector. So we'd need to port our local applications to PVXS in order to update areaDetector, although I suppose using both at the same time is possible. Ideally we would port our local applications, and it doesn't seem difficult to do this. It's just we have at least 60+ IOC applications spread across 30 beamlines, and it would be a major upgrade.
There was a problem hiding this comment.
I think this may also be the first required use of C++11 in ADCore (if a PVA plugin is needed), before moving my test build to a containerised environment I was seeing linker errors trying to compile this against Diamond's released support modules (asyn and busy iirc) which were compiled using an older version of GCC - I don't think pvAccessCPP et al. require C++11.
There was a problem hiding this comment.
I agree. Since the 2 PVA libraries have different features it makes sense to support both. It also does not require rebuilding ADCore when switching between them.
Yes. For example, APS DAQ system still uses old libraries, and if the support for those is dropped, there would have to be lots of work required on our end if we wanted to upgrade to the new ADCore version.
There was a problem hiding this comment.
The old plugin relies on PvDatabase server, which has the ability to distribute different data to different clients (e.g., image1 to client1, image2 to client2, etc.) if clients request that.
Is this a PVXS limitation that's being worked on, or just something nice to have that people will eventually be able to live without?
I could imagine emulating it by having a fanout to multiple NDPluginPVXS instances, but it definitely doesn't have the same ergonomics...
There was a problem hiding this comment.
The PVDatabase server has a pretty convenient plugin framework that allows one to develop different server side plugins, which in turn allows one to control what updates get returned to clients that monitor the given channel. In this particular case, the plugin I mentioned is this one: https://github.com/epics-base/pvDatabaseCPP/blob/master/documentation/dataDistributorPlugin.md
This feature has nothing to do with the PVA vs PVXS protocol itself. Just like the plugin framework in PVDatabase server, something like this could be added to PVXS-based servers. I do not know whether there are plans for doing so or not. I find it very useful for real-time processing of fast detector data, which could not be handled otherwise by a single client. Some examples of possible workflows are here: https://github.com/epics-base/pvaPy/blob/master/documentation/streamingFramework.md.
There was a problem hiding this comment.
Okay, so it sounds like the best course for the time being is to provide an NDPluginPVA, NDPluginPVXS and the required NTNDArrayConverter and NTNDArrayConverterPVXS (or some similar name), and maybe we have completely separate header files for each so that we can choose to provide one, both or neither simultaneously based on CONFIG flags.
There was a problem hiding this comment.
The old plugin relies on PvDatabase server, which has the ability to distribute different data to different clients ...
The PVXS SharedPV object specifically does not do this. Thus the "shared" part of its name. PVXS also provides a lower level API on which SharedPV is built (also p4p.gw). The spam.cpp test program is one example usage, providing a unique deluge to each subscriber to test per-subscription flow control.
NDPluginBasePixel in ADCore requires C++11. These are all of the modules in my areaDetector tree that have c++ flags: |
jsouter
force-pushed
the
pvxs
branch
2 times, most recently
from
4d3e6af to
3c16751
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
|
Docs will need updating. edit: okay, my container was doing some configure/RULES magic and automatically including and link pvxsIoc without me realising, but it is of course needed to instantiate pvxs::ioc::server, also have to fix a few more name collisions to allow both converters to be compiled simultaneously. |
|
This PR looks Ok to me. Do you want me to merge it? |
|
Just pushed some docs, not sure why that didn't get pushed before.
|
|
I think you can probably just link to pvxsIOC / include the pvxsIOC.dbd in the commonDriverMakefile without needing do it into the library |
|
@mdavidsaver, @MarkRivers, @sveseli, can you take a look in this final version of the PR? It should be ready for merging from @jsouter view. |
Looks okay to me. |
MarkRivers
left a comment
MarkRivers
left a comment
There was a problem hiding this comment.
Looks good to me. I will merge it.
|
I merged this PR and built pvxs with no errors. I modified the following files in areaDetector/configure/ CONFIG_SITE.local to have these lines: RELEASE_LIBS.local to have this line RELEASE_PRODS.local to have this line: ADCore builds fine. However, when I build ADSimDetector or any other driver I get these errors when linking the IOC. I have added line breaks to make the output more legible. Am I doing something wrong? |
|
I believe many of the errors are a result of the order of the pvxs and pvxsIoc libraries being in the incorrect order in commonLibraryMakefile. I reversed the order: That eliminates all of the undefined references. However, there is still a problem with multiply defined references to @jsouter did you test building and running detectors with this PR? |
These are symbol conflicts between libqsrv and pvxsIoc. I thought I had avoided these, but clearly not. It looks like they got in the PVA link support. At the moment, it will not be possible to link both QSRV1 and QSRV2 into the same executable. |
I have renamed the conflicting symbols, and re-verified that QSRV 1 and 2 can reasonably coexist. epics-base/pvxs#147 |
|
@mdavidsaver thanks for that. I pulled the master branch, and the link errors are fixed. I modified commonPlugins.cmd to add NDPluginPvxs and tried it with the simDetector. This is the output from the startup script when running with gdb: It appears that it fails to create the plugin. It crashes later, but the backtrace is not very useful. |
|
The failure above goes away if PVA is not included in the build, only PVXS. I am opening 2 new issues. |
A normal situation will be to link in one or the other. Removing the linking errors in about making way for a clearer runtime error/warning. The ADCore/ADApp/pluginSrc/NDPluginPvxs.cpp Line 56 in a44677e The |
|
Looking back to #532 (comment), I recommend using some Makefile ifdef PVXS_1_3_0
# QSRV2 present
else
# no QSRV2, test for QSRV1
ifdef EPICS_QSRV_MAJOR_VERSION # fallback to v1
# QSRV1 present
endif
endif |
This might be done in # detect presence of QSRV1 or 2. Prefer QSRV2.
ifdef PVXS_1_3_0
HAVE_PVXS = YES
else
ifdef EPICS_QSRV_MAJOR_VERSION
HAVE_PVA2PVA = YES
endif
endif
ifeq ($(HAVE_PVA2PVA)$(WITH_QSRV),YESYES)
# ... existing
endif
# unchanged
ifeq ($(WITH_PVA),YES)
# ... existing
endif
ifeq ($(HAVE_PVXS)$(WITH_PVXS),YESYES)
# ... existing
endifedit: updated logic to make clear that the conflict is between |
9 participants
Closes #528
Building with WITH_PVXS = YES (also require WITH_PVA = YES for time being) allows us to use pvxs, removing dependency on normativeTypesCPP, pvDataCPP, pvDatabaseCPP, pvAccessCPP.
WIP; currently testing against modified versions of ADSimDetector and pvaDriver, note that packages depending on this using PVXS will also need to be built with WITH_PVXS=YES in their config files. It may be that ultimately it would be better to move all the PVXS logic out of the separate *_pvxs.cpp files, but this was the simplest and most readable way to do it during development.
There are a few TODOs I will address, so making this a draft PR for now.