epics-base / pva2pva

PV Access gateway/proxy and EPICS Process Database integration
https://epics-base.github.io/pva2pva/
Other
4 stars 13 forks source link

qsrv returns fields not requeste by user #21

Closed mrkraimer closed 5 years ago

mrkraimer commented 5 years ago

A pvAccess server is required 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.

But qsrv creates a structure that has additional fields.

For example:

mrk> pvget -p ca -r control DBRdouble
DBRdouble
structure 
    control_t control
        double limitLow -9
        double limitHigh 9
        double minStep 0

mrk> pvget -p pva -r control DBRdouble
DBRdouble
epics:nt/NTScalar:1.0 
    double value 0
    alarm_t alarm INVALID DRIVER UDF
    time_t timeStamp <undefined> 0
    display_t display
        double limitLow -10
        double limitHigh 10
        string description 
        string format 
        string units volts
    control_t control
        double limitLow -9
        double limitHigh 9
        double minStep 0
    valueAlarm_t valueAlarm
        boolean active false
        double lowAlarmLimit -8
        double lowWarningLimit -6
        double highWarningLimit 6
        double highAlarmLimit 8
        int lowAlarmSeverity 0
        int lowWarningSeverity 0
        int highWarningSeverity 0
        int highAlarmSeverity 0
        double hysteresis 0
mrk> 
mdavidsaver commented 5 years ago

A pvAccess server is required

This is a mis-leading statement. The PVA server (aka. epics::pvAccess::ServerContext) has never enforced this sort of behavior.

Secondly, these are not requirements at all in my vocabulary. A requirement might be that "a client must be able to avoid the extra bandwidth of transferring fields which it doesn't want".

These "requirements" constitute a convention which you like, and I don't. (see also #1)

anjohnson commented 5 years ago

There are many written and unwritten 'rules' that EPICS has never enforced, e.g. don't use record names that are valid strtod() numbers (even Inf and NaN count) — they are legal, but if someone does that they may have difficulty pointing a link at such a record (you have to add a flag like NPP to the link string to stop the link parser making it into a CONSTANT link). These 'rules' often only become apparent some time after the code has been written when someone gets a result they didn't expect, looks into why that happened and works out what the limitations are (just like Science really).

This particular issue is really an argument about what the rules should be for a server's behavior. They may even have been described in the old reference documentation which @mdavidsaver deleted from his modules' repo's. Looking at the pvAccessCPP documentation for the 4.6.0 release I see wording like this:

channelPutConnect This is called as a result of calling Channel::createChannelPut. If status is OK, then channelPut is the interface to ChannelPut and structure is the introspection interface that must be used for the pvStructure passed to each ChannelPut::put and will be used for the data returned by every call to ChannelPut::get. If status shows a failure then the client should NOT use either channelPut or structure.

I haven't gone through too much of that document and the above wording doesn't explicitly contradict @mdavidsaver 's position, but the fact that each and every class Channel<x>Requester has an equivalent routine to the above that hands out a structure introspection interface, and that the client's channelPutGetConnect() method is given both a putStructure and a get Structure tells me that the original intention was that these structures are likely to be different.

As the original architect of much of EPICS I would normally ask @mrkraimer for his opinion in a situation where the rules aren't clear, and I think it's unfortunate that you seem to assume that anything which works that the code doesn't actively prevent must be allowed (without any discussion). That isn't how we have worked in the past, although it probably helped me that Marty was only 2 offices away and we also talked every lunchtime.

mdavidsaver commented 5 years ago

I think it's unfortunate that you seem to assume that anything which works that the code doesn't actively prevent must be allowed

Replace "code" with "protocol" and you're closer to the mark. pvAccessCPP is fundamentally a library built around a network protocol, and its feature set is necessarily based around how that protocol works, what it can, and what it can't do. My starting point with such a library is to expose everything which is possible to do with the protocol, even if I don't want it to be used (eg. ChannelArray). Any restrictions should be carefully considered.

I make a distinction between what should be done, and what could be done. Talking about what should be done is all well and good, but when I'm writing a network client or server, I want it to be able to handle everything which a peer could do. This may be with an error, but crashing is out of the question.

eg. when designing the pva/client.h API, my decision to not allowing re-use of get/put/rpc, was made only after combing through all of the existing user code I could find. I didn't find any examples of re-use. Also, this is a decision which the client side can make. My simplified server API (pva/sharedstate.h) fully supports re-use of get/put/rpc, because a client might do this.

(I have in mind a way that re-use might be added if it turns out that someone was using this feature)