redo conversion between DBD and pvData #96
Conversation
Get up to date with epics-base
|
except for the travis build for BRBASE=3.14 the following is true: I know that qsrv will fail for 3.14 release. I merged with latest from epics-base. But I see the same failure when I build in latest version in master branch of pvAccessCPP. I do not know what to do next. |
|
I think I fixed the BRBASE=3.14 build. |
|
Note that BRBASE=3.14 now only fails when running the tests. pvAccessCPP itself falis the same way when |
src/ca/dbdToPv.cpp
Outdated
| string properties; | ||
| if(alarmRequested && timeStampRequested) { | ||
| properties += "alarm,timeStamp"; | ||
| caRequestType += DBR_TIME_STRING + caValueType; |
There was a problem hiding this comment.
This sort of DBR type promotion is best done with dbf_type_to_DBR_TIME(caValueType).
| } | ||
|
|
||
|
|
||
| void DbdToPv::activate( |
There was a problem hiding this comment.
It looks to me that the bulk of this method is working with a native DBR type, and a pvRequest, to produce the actual DBR type(s) requested. This is logic which I would like to see being tested.
I can see two ways to do this. The first is to take advantage of the fact that local CA (aka dbContext.cpp) can be used in unit tests (since 3.16 I think). This would allow an fairly realistic end to end test. eg. Create a Provider and connect via libca to local records.
The second is to refactor a few methods (including this one) to split out a helper method which doesn't need a live chid and could be unit tested separately. In this case, such a (static) method would have a signature like:
void dbrMapRequest(short channelType, const PVStructurePtr& pvRequest, short *caRequestType, StructurePtr *structure);Where channelType is a native type DBF_. caRequestType is the promoted DBR_ type code, and structure is the PVD structure. This method could then be tested more or less exhaustively for all of the DBF_* code, and a pre-defined list of pvRequests.
These two options are not mutually exclusive. Ideally both would be done. My preference would be to start with the second, as i think this will more quickly expose any gaps in the logic.
Also, as proof that I'm eating my own dog food. The equivalent testing in QSRV of mapping between PVD and dbAccess.h is done here:
https://github.com/epics-base/pva2pva/blob/master/testApp/testpvif.cpp
src/ca/dbdToPv.cpp
Outdated
| return getPVDataCreate()->createPVStructure(structure); | ||
| } | ||
|
|
||
| Status DbdToPv::getFromDBD( |
There was a problem hiding this comment.
This method is another case which needs testing. As with DbdToPv::activate the second option will involve some refactoring. In this case, I would try to separate out the logic as:
void fromDBD(const void *dbr, short dbrType, long dbrCount, const PVStructurePtr& dest, const BitSetPtr& changed)
src/ca/dbdToPv.cpp
Outdated
| } | ||
| break; | ||
| } | ||
| case DBR_STRING: |
There was a problem hiding this comment.
Excepting enum and string, which continues to need special cases, the other DBF_* cases can be combined into two. Well, really two. One for scalars and one for array.
The array case will use PVScalarArray::putFrom(shared_vector<const void>&). A working example from QSRV is
For the scalar case, I'd like to say you could use PVScalar::putFrom(const AnyScalar&), but I realize that I'm missing a piece (ctor from type code and void*). Instead I'll recommend what is current use in QSRV, which is a union dbrbuf and "pv/typemap.h" to condense the switch statement.
src/ca/dbdToPv.cpp
Outdated
| dbr_short_t status = 0; | ||
| dbr_short_t severity = 0; | ||
| epicsTimeStamp stamp = {0,0}; | ||
| if(caRequestType>=DBR_CTRL_STRING) { |
There was a problem hiding this comment.
When I found myself writing code like this for QSRV, I stopped and asked. If I only need display/control as double, why am I asking the server (which also only stores doubles) to convert to something else for transport and I have to undo this conversion? The outcome is that QSRV always subscribes to display/control meta-data as DBR_CTRL_DOUBLE.
If you for some reason can't/don't want to do this, then this section cries out for a template'd helper to avoid this repetition.
src/ca/dbdToPv.cpp
Outdated
| return Status::Ok; | ||
| } | ||
|
|
||
| Status DbdToPv::putToDBD( |
There was a problem hiding this comment.
A third case for testing. Here I'd separate the logic as:
void toDBD(const PVStructurePtr& dest, const BitSetPtr& changed, void *dbr, short* dbrType, long* dbrCount)
src/ca/dbdToPv.cpp
Outdated
| dbr_long_t ivalue(0); | ||
| dbr_float_t fvalue(0); | ||
| dbr_double_t dvalue(0); | ||
| char *ca_stringBuffer(0); |
There was a problem hiding this comment.
FYI, std::vector<char> would be a great way to avoid leaking this local temp buffer.
src/ca/dbdToPv.cpp
Outdated
| } | ||
| break; | ||
| } | ||
| case DBR_CHAR: |
There was a problem hiding this comment.
As with getFromDBD, the non-DBR_STRING cases can be combined using PVScalarArray::getAs<shared_vector<const void> >().
Examples from QSRV
src/ca/dbdToPv.cpp
Outdated
| if(isArray) { | ||
| PVByteArrayPtr pvValue = pvStructure->getSubField<PVByteArray>("value"); | ||
| count = pvValue->getLength(); | ||
| pValue = pvValue->view().data(); |
There was a problem hiding this comment.
This is Evil! shared_vector::data() is returning an internal reference from the instance which is returned by view(). Strictly speaking, this internal reference is invalidated when it's instance is destroyed, which happens immediately.
Please make pValue a shared_vector<const void> instead of const void*.
src/ca/dbdToPv.cpp
Outdated
| dbr_short_t svalue(0); | ||
| dbr_long_t ivalue(0); | ||
| dbr_float_t fvalue(0); | ||
| dbr_double_t dvalue(0); |
There was a problem hiding this comment.
This would be a great situation for a C union.
This was my doing. You happened to start work from a point where I had unwittingly broken Timer is PVD while trying to find ref. leaks in PVA (cf. epics-base/pva2pva#12). Both are now fixed. You will need to do a re-base of your changes to get this fix. FYI, I try to remember to run the tests as a baseline before I start making changes. Similarly, checking the latest CI results can help to avoid finding someone else's bugs https://travis-ci.org/epics-base/pvAccessCPP/branches |
|
I need time to look at all of Michael's comments but I do have a comment(maybe question) about
The only reason for not just using DBR_CTRL_DOUBLE. is valueAlarm. Note that currently both qsrv and ca support the other types If ca is changed to use double for alarm limits then I think qsrv also should make the same change. Michael, |
Firstly, to clarify. QSRV uses dbAccess.h, where DBR_CTRL_DOUBLE refers to DBRctrlDouble. I also always use DBRgrDouble and DBRalDouble for all DBF types. So the limit values extracted via. dbGet() are always double. As I'm using the
(I'm assuming that "ca" means "caProvider" here) QSRV already uses PVScalar to interact with the limit values. So this particular change wouldn't break anything. Using StandardField has caused me a little pain in the past. Not in this way, but because it doesn't mesh well with FieldBuilder. Also, when I looked at adding other members under 'timeStamp' I had to stop using |
|
I am committing major changes to dbdToPv including using templates. I do not know how to fix issue #77, i.e. possible deadlock on termination. I am starting to create a document describing compatibility between
|
|
Michael, |
|
My original review:
With these changes 1) has been partially addressed via. templates. There is still no unit test coverage. I really want to see unit test coverage of the type and value handling code. All my experience with CA and PVD tells me that there will be bugs here. If not now, then if future. Manual testing is nice, and necessary, but doesn't help me with verification, nor the long term maintainability of this code. @mrkraimer Will you create unit tests for the 3 pieces of code I commented on? These need to cover the range of user inputs (pvRequest) and native field types (DBF_*).
|
|
I have added tests for provider ca. fails with But the following works: Any ideas? |
|
@mrkraimer We now have a lot of infrastructure which can cut way down on the boilerplate, and make tests like this work better. Instead of starting an IOC as a separate process, have a look at https://github.com/epics-base/epics-base/blob/3.16/src/ioc/db/test/dbPutLinkTest.c#L394 And I know you won't like this, but using "pva/client.h" will massively reduce the client boilerplate. This boilerplate accounts for ~600 of the ~700 lines of the test code. pvac::ClientProvider prov("ca");
pvac::Channel ch(prov.connect("pv:name"));
// ...
epics::pvData::PVStructure::const_shared_ptr val(ch.get(3.0, pvRequest));
// ...
ch.put(pvRequest)
.set<epics::pvData::uint32>("field", 42)
.exec(); |
|
I have moved the tests for ca provider from testApp to testCa, But when I run Does anyone know what is happening? |
|
Note that Now works for me but some travis builds still fail. |
|
I certainly know that dbUnitTest.h was introduced in 3.15. I thought that the local CA changes were on 3.15 as well, but I see that this is not the case (epics-base/epics-base@3e8ba7d). My concern is maintainability going forward, so I have no problem with only running this test when built against 3.16. I can't stress enough how much I dislike using |
Travis-CI fixes
|
Michael said
An argument for implementing a call to system is: I realize that this is a problem for RTEMS and vvWorks. |
|
IMHO, your argument supports starting a separate process, but not the use of the |
|
Ralph said
So how do I do this in RTEMS and vxWorks? |
|
Let's talk about that in the telecon. |
|
Note the name of this pull request. But then it was requested that I develop a test in pvAccessCPP for ca provider even though no test ever existed. |
|
Andrew said
Actually this is not true. The string values for the test are converted to the type for the channel. Marty |
|
@mrkraimer you're right, my apologies for forgetting that pvAccess only allows puts in the native type of the channel anyway, and for not reading your code carefully enough. My point about your always using a pvRequest of "value" for puts and "value,alarm,timestamp" for gets still stands though. IIRC you discussed a bug several months back that you found when the pvRequest fields were named in a different order, shouldn't there be some regression tests for that problem? |
|
I have been spending lots of time trying to fix the Deadlock in ca_clear_subscription problem. I did arrange to have an aux thread issue the ca_clear_subscription, but this did not fix the problem. I also created tests in Note that the tests that use pvaClientCPP do not fail but they also did not fail without the aux thread. The tests that use pvAccessCP/src/client/pva/client.h do fail when monitoring on more than one channel. |
|
I made some more changes for what happens when a monitor is destructed. |
|
I am going to push changes. Now works just fine. I am not ready for this pull request to be merged since I still want to do more testing. |
|
It is approaching three months since this pull request was issued. I really really want this pull request merged since it is a real improvement. |
|
Hi Marty, I'm seeing lots of warnings when testCaProvider.cpp gets built because it includes both diff --git a/src/ca/pv/caProvider.h b/src/ca/pv/caProvider.h
index a147d26..2a18acd 100644
--- a/src/ca/pv/caProvider.h
+++ b/src/ca/pv/caProvider.h
@@ -8,9 +8,10 @@
#define CAPROVIDER_H
#include <shareLib.h>
-#include <cadef.h>
#include <pv/pvAccess.h>
+struct ca_client_context;
+
namespace epics {
namespace pvAccess {
namespace ca {
@@ -39,7 +40,7 @@ public:
* This can be called by an application specific auxiliary thread.
* See ca documentation. Not for casual use.
*/
- static ca_client_context * get_ca_client_context();
+ static struct ca_client_context * get_ca_client_context();
/** @brief stop provider ca
*
* This does nothing since epicsAtExit is used to destroy the instance.
|
|
I have now implemented code to call channelGet and channelPut callbacks via separate threads. I have successfully run a test that connects to four records that process at maximum rate. I developed the code for the ca provider in https://github.com/mrkraimer/pva2ca I put the new code in a new repository because I do not know what the plans are the ca provider that is the subject of this pull request. But it would be easy for me to move the code back to pvAccess. The test is in https://github.com/mrkraimer/testClientCPP The name of the new test is getputmonitorrate. I think that it is important that the latest version of the ca provider be part of the next EPICS 7 release. If it is a separate repository, i. e. it is unbundled from pvAccess then I maintain that pvTools also be unbundled or else "pvget -p ca" and "pvput -p ca" will not work. If it is not separate repository will I be allowed to merge after I move code from mrkraimer/pva2ca back to pvAccess? |
|
mdavidsaver said It works with my latest in mrkraimer: |
I do not understand that question. For configuration-less adding of providers I would suggest a plug-in architecture, where the presence of a shared library in a well-known place is enough to use it. For the PVA client, any provider except PVA itself should be pluggable. |
|
Marty, do you need to push some recent changes to github? My copy of your branch still prints some debugging messages as I showed in #114 Andrew, The latest push should fix remove the extra messages. |
The code that converts between DVD and pvData is greatly modified.
The new implementation is provided by src/ca/dbdToPv.
To see how it supports pvRequest look at DbdToPv::activate.
Note that the new implementation no longer uses pvCopy but still properly implements bitSet for
channelGet and channelMonitor.
I tried to fix issue #77 but was not successful.
I did try changing code so that ca_clear_channel was called from a separate thread instead of from
CAChannel::~CAChannel thread.
But this did not work because CAChannel::~CAChannel was never called..
I have not tested dbr_char_t or dbr_short_t.
I am going to add DBRecords for those types in exampleCPP/database so that I can test those types before this pull request is merged.
In the meantime I am creating this pull request just in case someone has an interest in the new conversion and also has time.