Merge lp:~epics-core/epics-base/pcas-fake-dynamic into lp:~epics-core/epics-base/3.16

[mdavidsaver]

Yet another (though hopefully last) attempt to fake support for dynamic arrays in PCAS. This is a very minimal change which mangles the received packet near the start of processing to reduce the number of places where changes are needed.

[Ralph Lange (ralph-lange)]

I am fine with the faking approach, as it is minimally invasive and still "fixes" things.

The CA Gateway (aka PCAS power user) always uses variable size subscriptions on its connections to the IOCs, so the "precious" IOC network already uses optimized traffic.
A "real" full implementation could follow at any time - the only difference would be the PCAS (CA Gateway) actually sending variable size arrays instead of always pushing the full size. No changes in protocol version, IOCs or clients would be required.

review: Approve

[Ralph Lange (ralph-lange)]

This should go on the 3.14 branch, I guess.

[Andrew Johnson (anj)]

No objections, I agree we should merge this into 3.14.

Can someone explain which other merge requests this one replaces, and hence which other pcas-related ones (if any) we will still need.

[Ralph Lange (ralph-lange)]

This branch is a direct replacement for ~matej-sekoranja/epics-base/pcas-variable-length-arrays,
including the proposed change in ~bhill/epics-base/pcas-variable-length-arrays.

Functionally equivalent, very similar, with fewer changes in the code.

(I don't see any other public branches that might be related.)

[Andrew Johnson (anj)]

Thanks Ralph.

I have just tested and merged this onto 3.14 with a Release Note, but my wording doesn't explain what changes have to be made to a cas application to add full support for dynamic arrays. If someone who knows would like to document it my release note it would be one spot. Another place would be to modify src/makeBaseApp/top/caServerApp/exServer.cc to generate variable length arrays for one or more of the PVs it serves.

review: Approve

[Bruce Hill (bhill)]

I don't understand why you guys pushed this through so quickly. As near as I can tell it doesn't accomplish anything. True, pcas now can claim to support CA 4.13 during connection so the client can request zero elements and think they're getting dynamic arrays, but it's still sending the full array.

Please note that this is not the same as the pcas-variable-length-arrays proposal which does support variable sized arrays w/ my patch.

I"m also concerned that this could result in clients compiled w/ CA 4.13 that request zero elements but always copy the full array from a gdd that wouldn't be zero padded if the CA 4.13 compliant PCAS server actually sent the dynamic size.

On 2nd thought, since this change steps on the m_count field in the msg, it would probably keep that app from benefitting when the PCAS server does send the dynamic size as it would always request the full array.

Please reconsider merging this into base.

[Bruce Hill (bhill)]

I don't understand why you guys pushed this through so quickly. As near as I can tell it doesn't accomplish anything. True, pcas now can claim to support CA 4.13 during connection so the client can request zero elements and think they're getting dynamic arrays, but it's still sending the full array.

Please note that this is not the same as the pcas-variable-length-arrays proposal which does support variable sized arrays w/ my patch.

I"m also concerned that this could result in clients compiled w/ CA 4.13 that request zero elements but always copy the full array from a gdd that wouldn't be zero padded if the CA 4.13 compliant PCAS server actually sent the dynamic size.

On 2nd thought, since this change steps on the m_count field in the msg, it would probably keep that app from benefitting when the PCAS server does send the dynamic size as it would always request the full array.

Please reconsider merging this into base.

review: Disapprove

[Ralph Lange (ralph-lange)]

@Michael: I know you had tests for this. Are these on a different branch? Can you please add them (either as a new branch or merging them into 3.14)?

[Ralph Lange (ralph-lange)]

@Bruce:
We are preparing new releases for 3.14 and 3.15 that are supposed to be done by end of October, so we need the code changes to be in last Friday latest.

Channel Access functionality and protocol version numbers are linear; there is no bitset of features that a server may decide to implement or not. PCAS needs to be in sync with rsrv and the client in terms of functionality i.e. protocol version numbers.

Client and rsrv implemented variable length arrays as CA version 4.13; the existing PCAS (4.12) was rejecting zero-length requests.
We have other proposed changes for Channel Access (i.e. CA version 4.14) that cannot go into PCAS unless PCAS handles 4.13 correctly (because 4.14 implicitly declares 4.13).

So, for the immanent releases of 3.14 and 3.15, we need a quick fix that allows us to step up the CA version number in PCAS, minimally invasive, with the least number of side effects. This is exactly what this branch implements.
You are right with your statements about this implementation not being complete and just claiming that PCAS supports dynamic arrays. (Note its name being "pcas-fake-dynamic".)
But: this is exactly the minimal workaround we need at this point.

Once we merged Michael's tests, please resubmit your proposal as a new branch. At that point we should be able to better verify - using those tests - that your implementation is complete and working.

[Andrew Johnson (anj)]

So this branch does *not* replace Matej & Bruce's work, it merely pretends to do so?

My apologies Bruce, I didn't realize the significance of Michael's word "fake", which was why I asked for more information about it last week. I thought this was an alternative and less intrusive way to implement the dynamic array support.

I do *not* like the "faking it" approach if that's what this actually does, and I'm not very happy about this right now. I am currently inclined to revert my merge commit of this branch to 3.14, but where we go from there I don't know; I have other stuff I have to work on here for the next day or two.

review: Disapprove

[mdavidsaver (mdavidsaver)]

So Matej and/or Bruce has fixed gdd track actual array size?

[Bruce Hill (bhill)]

Hi Ralph,

Sorry I didn't know we were up against a deadline on this.

I'll resubmit my version as a new proposal that will just provide variable array
support to PCAS w/ CA 4.13.

What other changes are being proposed for CA 4.14? I have a pending
merge request ca-vararray-zero-pad-in-client that proposed bumping to 4.14
to reduce network bandwidth for clients which haven't been updated to request
zero elements in their ca array calls. It needs some more work though
to satisfy Jeff's objections.

Regards,
- Bruce

On 10/11/2016 01:41 AM, Ralph Lange wrote:
> @Bruce:
> We are preparing new releases for 3.14 and 3.15 that are supposed to be done by end of October, so we need the code changes to be in last Friday latest.
>
> Channel Access functionality and protocol version numbers are linear; there is no bitset of features that a server may decide to implement or not. PCAS needs to be in sync with rsrv and the client in terms of functionality i.e. protocol version numbers.
>
> Client and rsrv implemented variable length arrays as CA version 4.13; the existing PCAS (4.12) was rejecting zero-length requests.
> We have other proposed changes for Channel Access (i.e. CA version 4.14) that cannot go into PCAS unless PCAS handles 4.13 correctly (because 4.14 implicitly declares 4.13).
>
> So, for the immanent releases of 3.14 and 3.15, we need a quick fix that allows us to step up the CA version number in PCAS, minimally invasive, with the least number of side effects. This is exactly what this branch implements.
> You are right with your statements about this implementation not being complete and just claiming that PCAS supports dynamic arrays. (Note its name being "pcas-fake-dynamic".)
> But: this is exactly the minimal workaround we need at this point.
>
> Once we merged Michael's tests, please resubmit your proposal as a new branch. At that point we should be able to better verify - using those tests - that your implementation is complete and working.

--
Bruce Hill
Member Technical Staff
SLAC National Accelerator Lab
2575 Sand Hill Road M/S 10
Menlo Park, CA 94025

[Bruce Hill (bhill)]

Not exactly. What my fix does is have gdd always allocate buffers to accomadate
the full array size, but memory is cheap and network bandwidth is not.

Cheers,
- Bruce

On 10/11/2016 10:48 AM, mdavidsaver wrote:
> So Matej and/or Bruce has fixed gdd track actual array size?

--
Bruce Hill
Member Technical Staff
SLAC National Accelerator Lab
2575 Sand Hill Road M/S 10
Menlo Park, CA 94025

=== modified file 'src/ca/legacy/pcas/generic/caHdrLargeArray.h'
--- src/ca/legacy/pcas/generic/caHdrLargeArray.h	2016-05-22 03:43:09 +0000
+++ src/ca/legacy/pcas/generic/caHdrLargeArray.h	2016-09-22 13:16:55 +0000
@@ -30,7 +30,7 @@
 #   include "shareLib.h"
 #endif
 
-static const unsigned char CA_MINOR_PROTOCOL_REVISION = 12;
+static const unsigned char CA_MINOR_PROTOCOL_REVISION = 13;
 
 typedef ca_uint32_t caResId;
 

=== modified file 'src/ca/legacy/pcas/generic/casCtx.h'
--- src/ca/legacy/pcas/generic/casCtx.h	2016-05-22 03:43:09 +0000
+++ src/ca/legacy/pcas/generic/casCtx.h	2016-09-22 13:16:55 +0000
@@ -18,6 +18,8 @@
 
 #include "caHdrLargeArray.h"
 
+class casStrmClient;
+
 class casCtx {
 public:
 	casCtx();
@@ -41,6 +43,7 @@
 	casChannelI * pChannel;
 	casPVI * pPV;
 	unsigned nAsyncIO; // checks for improper use of async io
+    friend class casStrmClient;
 };
 
 inline const caHdrLargeArray * casCtx::getMsg() const 

=== modified file 'src/ca/legacy/pcas/generic/casStrmClient.cc'
--- src/ca/legacy/pcas/generic/casStrmClient.cc	2014-05-30 17:36:50 +0000
+++ src/ca/legacy/pcas/generic/casStrmClient.cc	2016-09-22 13:16:55 +0000
@@ -388,14 +388,12 @@
 //
 // casStrmClient::verifyRequest()
 //
-caStatus casStrmClient::verifyRequest ( casChannelI * & pChan )
+caStatus casStrmClient::verifyRequest (casChannelI * & pChan , bool allowdyn)
 {
-    const caHdrLargeArray * mp = this->ctx.getMsg();
-
     //
     // channel exists for this resource id ?
     //
-    chronIntId tmpId ( mp->m_cid );
+    chronIntId tmpId ( ctx.msg.m_cid );
     pChan = this->chanTable.lookup ( tmpId );
     if ( ! pChan ) {
         return ECA_BADCHID;
@@ -404,14 +402,22 @@
     //
     // data type out of range ?
     //
-    if ( mp->m_dataType > ((unsigned)LAST_BUFFER_TYPE) ) {
+    if ( ctx.msg.m_dataType > ((unsigned)LAST_BUFFER_TYPE) ) {
         return ECA_BADTYPE;
     }
 
     //
     // element count out of range ?
     //
-    if ( mp->m_count > pChan->getPVI().nativeCount() || mp->m_count == 0u ) {
+    if ( allowdyn && ctx.msg.m_count==0 && CA_V413 ( this->minor_version_number ) ) {
+        // Hack
+        // Since GDD interface doesn't support variable sized arrays
+        // we present dynamic requests as max size.
+        // this allows PCAS to claim support for dynamic arrays w/o
+        // going to the trouble of fixing GDD.
+        ctx.msg.m_count = pChan->getPVI().nativeCount();
+    } else
+    if ( ctx.msg.m_count > pChan->getPVI().nativeCount() || ctx.msg.m_count == 0u ) {
         return ECA_BADCOUNT;
     }
 
@@ -444,7 +450,7 @@
     casChannelI * pChan;
 
     {
-        caStatus status = this->verifyRequest ( pChan );
+        caStatus status = this->verifyRequest ( pChan, true );
         if ( status != ECA_NORMAL ) {
             if ( pChan ) {
                 return this->sendErr ( guard, mp, pChan->getCID(), 
@@ -585,7 +591,7 @@
     casChannelI * pChan;
 
     {
-        caStatus status = this->verifyRequest ( pChan );
+        caStatus status = this->verifyRequest ( pChan, true );
         if ( status != ECA_NORMAL ) {
             return this->readNotifyFailureResponse ( guard, * mp, status );
         }
@@ -1940,7 +1946,7 @@
 
     casChannelI *pciu;
     {
-        caStatus status = casStrmClient::verifyRequest ( pciu );
+        caStatus status = casStrmClient::verifyRequest ( pciu, true );
         if ( status != ECA_NORMAL ) {
             if ( pciu ) {
                 return this->sendErr ( guard, mp, 

=== modified file 'src/ca/legacy/pcas/generic/casStrmClient.h'
--- src/ca/legacy/pcas/generic/casStrmClient.h	2010-08-10 21:05:46 +0000
+++ src/ca/legacy/pcas/generic/casStrmClient.h	2016-09-22 13:16:55 +0000
@@ -69,7 +69,7 @@
     bool responseIsPending;
 
     caStatus createChannel ( const char * pName );
-    caStatus verifyRequest ( casChannelI * & pChan );
+    caStatus verifyRequest ( casChannelI * & pChan, bool allowdyn = false );
     typedef caStatus ( casStrmClient :: * pCASMsgHandler ) 
         ( epicsGuard < casClientMutex > & );
     static pCASMsgHandler const msgHandlers[CA_PROTO_LAST_CMMD+1u];