Put fails over ca

[brunoseivam]

(Following discussion on #114)

I found the problem, and it is related to the ca provider. I can't create an issue on Marty's repository.

Proof of Concept: https://github.com/brunoseivam/testPut https://gist.github.com/brunoseivam/7e861757d3bf1edf3e79dadd2dcd6aff

When running the test I get a Provided put value with wrong type, even though the provided type is supposed to match the server's type.

If I comment these lines out, the test seems to work:

https://github.com/mrkraimer/pvAccessCPP/blob/df45c70149ec8f87bdea37f637b9ca4eea95100c/src/ca/dbdToPv.cpp#L148-L159

[brunoseivam]

Oh, also, if you (@mrkraimer) are running straight from your fork, you probably need to apply this patch: a4e9730

[mdavidsaver]

Provided put value with wrong type

This error means what it says. The PVStructure which is assigned to args.root must be exactly identical to the build argument as it will be then passed directly to the Serialize code, which will fault if this isn't the case.

Can you visualize the difference between two Structures? eg.

    virtual void putBuild(const StructureConstPtr &build,
                          ClientChannel::PutCallback::Args& args) OVERRIDE FINAL
    {
        args.root = value;
        args.tosend = tosend;
        if(value->getStructure()!=build) {
              std::cout<<"=== Structure mis-match\n===\n"<<*build<<"\n===\n"<<*value->getStructure()<<"\n===\n";
        }
    }

[mdavidsaver]

The putBuild() callback is defined this way to enforce handling of type change, while allowing for the possibility of reusing a PVStructure, though imo. there is little reason unless a high rate of Put operations is anticipated.

What this means in practice is that, rather than having a class member PVStructure value;, to in your example store double value; (or whatever your internal representation is). Then define the putBuild() like:

    virtual void putBuild(const StructureConstPtr &build,
                          ClientChannel::PutCallback::Args& args) OVERRIDE FINAL
    {
        PVStructurePtr top(getPVCreate()->createStructure(build));
        PVScalarPtr val(top->getSubFieldT<PVScalar>("value"));
        val->putFrom(value);
        args.tosend.set(val->getFieldOffset());
        args.root = val;
    }

Also, please for the sake of sanity don't have more than one using namespace in a source file. This defeats the isolation which is the purpose of namespaces! When my fingers get tried, I use namespace aliases like namespace pvd = epics::pvData;.

[mdavidsaver]

Also, notice the use of PVScalar::putFrom() makes this example insensitive to change from one scalar type to another (eg. float -> int or vis. versa).

[brunoseivam]

Hi @mdavidsaver,

Please consider the piece of code I sent as a quick and dirty way to trigger the issue at hand. It is not representative of the actual code that I am developing (where namespaces are properly used and the put part is supposed to handle any structure).

Yes, the types are different. Here's build:

structure
    double value

The actual issue is that I think build is wrong! I am passing "field()" as a request and it still shows me only the value field, where it should show the entire (converted from dbd) structure.

[brunoseivam]

Also, keep in mind this issue happens when @mrkraimer's fork is merged. The code as it is on the master branch doesn't trigger this issue.

[mdavidsaver]

The actual issue is that I think build is wrong!

For Put/Get the pvac:: classes are a thin wrapper.

Here you can see that aside from providing a default, a pvRequest is passed unchanged to createChannelPut()

pvAccessCPP/src/client/clientGet.cpp

Lines 217 to 235 in 8644f42

	Operation
	ClientChannel::put(PutCallback* cb,
	epics::pvData::PVStructure::const_shared_pointer pvRequest)
	{
	if(!impl) throw std::logic_error("Dead Channel");
	if(!pvRequest)
	pvRequest = pvd::createRequest("field()");
	std::tr1::shared_ptr<GetPutter> ret(GetPutter::build(cb));
	{
	Guard G(ret->mutex);
	ret->op = getChannel()->createChannelPut(ret->internal_shared_from_this(),
	std::tr1::const_pointer_cast<pvd::PVStructure>(pvRequest));
	}
	return Operation(ret);
	}

On the other side you can see that the Structure passed to the builder comes directly from channelPutConnect().

pvAccessCPP/src/client/clientGet.cpp

Lines 89 to 93 in 8644f42

	virtual void channelPutConnect(
	const epics::pvData::Status& status,
	pva::ChannelPut::shared_pointer const & channelPut,
	epics::pvData::Structure::const_shared_pointer const & structure) OVERRIDE FINAL
	{

... error checking omitted

pvAccessCPP/src/client/clientGet.cpp

Lines 116 to 117 in 8644f42

	UnGuard U(G);
	cb->putBuild(structure, args);

This goes make me think of adding another "example" server, which would print pvRequest, value, and bitset from the put handler.

[mrkraimer]

In the original message Bruno said:
When running the test I get a Provided put value with wrong type, even though the provided type is supposed to match the server's type.

Then the fix was commenting out the code:

switch(ioType)
    {
         case getIO : break;
         case putIO:
              alarmRequested = false;
              timeStampRequested = false;
              displayRequested = false;
              controlRequested = false;
              valueAlarmRequested = false;
              break;
         case monitorIO: break;
    }

But what a pvAccess server is required to do is to create a structure that has

1. only the fields the client requests and that the server supports.
2. each field type is determined by the server.

For channel access

1. A put can only access a single field.
2. A client can pass a type other than the native type and the server will convert from the client type to the native type.

For ca provider,:

1. the above code ensures that a client put can only access a single field.
2. The type is the native type.
3. The client must convert from any other type to the native type.

[brunoseivam]

I understand the reasoning, but I believe that the current implementation is inconsistent, since request=field() ends up not being honored for Put.

Note that, in my example, while I expect the whole structure to be there, I only put to the value field (as specified in the BitSet that I am passing in).

So I think the correct thing to do (to ensure that only the value field is accessed) is to apply both request and bitset to the structure and then check if the resulting structure is what you expect.

[mrkraimer]

Above it says

What a pvAccess server is required to do is to create a structure that has

1. only the fields the client requests and that the server supports.
2. each field type is determined by the server.

For a put channel access only allows the client to specify single field, i. e. the server only supports a single field.
ca provider gives the name value to this field.

[brunoseivam]

The pva provider gives me the whole structure. I really don't want to have a special case for the ca provider in my code...

[mdavidsaver]

What a pvAccess server is required to do is to create a structure that has

These are not requirements as far as I'm concerned. The handling of pvRequest (or not) is left to individual ChannelProvider. caProvider may choose to do things this way, but others may not.

The pva provider gives me the whole structure.

I think you really mean "QSRV"? For my own sanity, QSRV has only one Structure definition per PV for get/put/monitor irrespective of pvRequest. (so pvinfo is always accurate) However, it only supports Put to some fields (mainly '.value', but Group PVs can be more complicated). Attempts to write to other fields (determined by checking the bitset) are ignored, sometimes with a warning.

[brunoseivam]

I think you really mean "QSRV"?

Yes. I meant that I get the whole structure when using softIocPVA -d test.db and the pva provider on the other end.

For my own sanity, QSRV has only one Structure definition per PV for get/put/monitor irrespective of pvRequest.

Here's my use case:

I create a monitor on a PV. This monitor already has an underlying channel and an associated pvRequest. When I want to put to this PV, I reuse the channel and the pvRequest (that's why I first reported this issue - the build Structure returned by the ca provider was different than the Structure for the respective monitor).

So, in the end, I believe Get, Put and Monitor operations should all operate on the same Structure for a given (pv, pvRequest) pair. Any restrictions on what can be the subject of Put should take the given BitSet into consideration.

[mrkraimer]

Note that pvRequest is an argument to createGet, createPut, monitor,etc.
It is not a argument to createChannel.

Thus the API, which has existed since about 2007, means that the server can create different structures for get, put, monitor, etc.
Looking at

https://github.com/epics-base/pvAccessCPP/blob/master/src/client/pva/client.h

I can see why Bruno came to his conclusion.
It has the client creating a put, get, monitor without first connecting to a channel.

Look at
https://github.com/epics-base/pvaClientCPP
and for examples
https://github.com/epics-base/exampleCPP
In the latter look especially at exampleClient (put get and monitor).

With pvaClient a get, put, etc is only created after a channel connects.

[brunoseivam]

Hi Marty, I understand what you are saying but my point was a little bit different. Please correct me if I am wrong in my following assertions:

For the sake of argument, say that I create a monitor on the TEST:ai channel, and pass it pvRequest="field(value,alarm)"

1. The subsequent monitor data events will give me PVStructures with only value and alarm fields

   1. True for ca provider.
   2. False for pva provider over QSRV, according to what Michael said:

For my own sanity, QSRV has only one Structure definition per PV for get/put/monitor irrespective of pvRequest.

2. Structure of Monitor(channel="TEST:ai", pvRequest="field(value,alarm)") == Structure of Put(channel="TEST:ai", pvRequest="field(value,alarm)").

   1. True for pva over QSRV (ignores pvRequest in both cases).
   2. False for ca (ignores pvRequest for Put).

I opened this issue because assertion 2.ii is false and I wish it was true. If ca is not respecting the wish of the client w.r.t. pvRequest then I, as a client developer, have to special case it into my code when writing a generic client.

I also think that 1.ii should be true, but that in particular doesn't hurt the client since it gets more data than it needs, not less data as is the case with 2.ii

[anjohnson]

Hi Bruno,

Doing a put over CA that tries to write anything except for the value field will never work; inside the IOC only the addressed field of the record (or VAL if a field name is not given) is ever written to by the RSRV (CA server) code. I don't know what QSRV does for regular channels (groups are a different matter) but it calls the same dbChannel API to talk to the IOC database so I think it has to follow the same rules (@mdavidsaver can you confirm that?). Thus the CA provider is being correct when it tells you that the data structure associated with a put can only contain a value field.

Note that there is currently a significant disagreement between @mrkraimer and @mdavidsaver as to whether the CA model or the QSRV model is correct/better. @mdavidsaver 's implementation is simpler than @mrkraimer 's original vision of how this should work, but by using the same pvStructure for all get/put/monitor operations QSRV relies on the user knowing that they can't change a record's alarm status & severity say by doing a put that supplies data for the alarm structure. Michael explained above that for some cases like that there may be a warning, but in others the server silently discards the extra information.

[mdavidsaver]

some cases like that there may be a warning, but in others the server silently discards the extra information.

To be clear. When I looked yesterday I realized that I had only done so in the wait=true case. (Presumable where I first made this mistake) I intend to warn in all cases.

[mdavidsaver]

It has the client creating a put, get, monitor without first connecting to a channel.

What benefit is derived from doing otherwise? (I see none)

[mdavidsaver]

Ok, so there one clear incompatibility between pva/client.h and caProvider. I assume that ChannelGet::get() and ChannelPut::get() behave the same. So pva/client.h only uses ChannelPut::get(). However, caProvider treats them differently wrt. pvRequest.

[brunoseivam]

The way I see it, the Structure should describe the channel, according to pvRequest, independent of the Operation being performed on the channel. If I am not allowed to write to a field, this should be caught at the time of the actual writing, and consider the bitset that I am passing to it. See:

$ caput TEST:ai.SEVR 1
Old : TEST:ai.SEVR                   NO_ALARM
Error from put operation: Write access denied

Presumably the IOC is the one that prevented the write in this case. Maybe putBuild could pass a new BitSet argument to indicate writable fields to allow errors to be caught by the client earlier, but that's not necessary.

What happens when channel access security is involved? Should putBuild pass an empty Structure for read-only channels?

In any case, I won't insist. If the current behavior is the correct one by design, then I'll have to work around it in the client.

[mdavidsaver]

The way I see it, the Structure should describe the channel, according to pvRequest, independent of the Operation being performed on the channel

I wish it were so, but the PVA protocol wasn't designed this way. A prudent client will have to take into account the possibility that get/put/monitor may have different types.

Maybe putBuild could pass a new BitSet argument to indicate writable fields

This would be a reasonable interface. The issue at present is that the PVA protocol has no place for this granular information (another regrettable design issue). Let alone a way to communicate changes in permissions.

[anjohnson]

Marty wrote:

It has the client creating a put, get, monitor without first connecting to a channel.

To which Michael replied:

What benefit is derived from doing otherwise? (I see none)

AIUI the idea was that the client can find out what the data structure from a channel will look like before it has to commit to even fetching any data from it. If the new API lets/makes a client create a put/get/monitor object before it knows what channel to connect to, how does the client find out what it's going to get from the channel?

The kind of case I think we've always been concerned about is a client with limited RAM connecting by mistake to a PV that serves large image arrays. If the client can see in advance that this channel isn't going to return the kind of structure it wants it can avoid starting a channel monitor on it, thus saving both sides from problems. I don't know if the design completely achieved that goal (can a client limit how many array elements it will be sent?) but that was what we wanted.

BTW: The terse mode output from pvget (pvget -t) exactly matches that of caget and makes perfect sense using the ca provider or pvaSrv. QSRV's 'deliver everything' is what breaks it:

tux% caget anj:BaseVersion
anj:BaseVersion                7.0.1.2-DEV
tux% caget -t anj:BaseVersion
7.0.1.2-DEV
tux% pvget -p ca -t anj:BaseVersion
7.0.1.2-DEV
tux% pvget -t pvaSrv:BaseVersion
3.15.5-DEV
tux% pvget -t anj:BaseVersion
7.0.1.2-DEV NO_ALARM NO_STATUS NO_ALARM 2018-07-18T17:55:26.961 0 0 0 EPICS Base Version   0 0 0

(I'm probably mixing up issues with that point, sorry!)

[mdavidsaver]

... how does the client find out what it's going to get from the channel?

For Put, this is the role of the putBuild() callback already discussed.

For Get/RPC/Monitor, I didn't think this would ever be useful. The use case you mention seems like one better satisfied by adding a condition to the request (a la server side filters).

can a client limit how many array elements it will be sent?

Not by any general mechanism. What the server wants to send is what the client gets.

The terse mode output ... QSRV's 'deliver everything' is what breaks it:

Not surprising considering that I didn't test '-t'. The output modes of the pvtools need to be gone over. imo. there are too many ('-t', '-i', '-v').

[anjohnson]

@mdavidsaver Isn't a pvRequest like field(value,timeStamp) pretty close to the server-side filter that you're suggesting? Send me just the data from these named fields?

I agree about reviewing the pvtools, although I don't know who would do that or when it could happen.

[mdavidsaver]

pretty close to the server-side filter that you're suggesting?

Only if I'm completely misunderstanding your use case of returning part, but not all of an array. I was thinking of something like pv:name.VAL[:400] to return no more than the first 400 elements.

I don't know who would do that or when it could happen.

Yup. This sort of maintenance work doesn't really fit with my current feature driven funding. Looks boring on a slide, but high impact to those in daily contact with the tools.