search

epics-extensions / ca-gateway

Channel Access PV Gateway
http://www.aps.anl.gov/epics/extensions/gateway/
Other
17 stars 17 forks source link

Support for caPutLog >= R3.5 #22

Closed hhslepicka closed 5 years ago

hhslepicka commented 5 years ago

This patch was originally made by @bhill-slac .

@bfrk changed the caPutLog and renamed the members of the VALUE union (See: https://github.com/epics-modules/caPutLog/commit/1fbe2ed1f7fe26ed9edc98fada7f96ecf033ab64) This means that the ca-gateway will not build with versions of caPutLog greater than R3-4.

This Pull Request also adds the note at the README file that EPICS 7 needs PCAS as an external module to be added to the RELEASE file.

bfrk commented 5 years ago

The changes that actually do what the comment says look fine to me. However, the second patch "FIX: Adjust caPutLog dependent struct..." should, IMHO, be cleaned up before merging, as it contains lots of changes not related to the issue at hand (e.g. adding support for 64 bit request types). My recommendation would be to split the changes into semantically related sets and record them separately, for easier review.

ralphlange commented 5 years ago

@bfrk Thanks for lurking around! Question: does the caPutLog change come with a preprocessor-testable thing (numerical version number or a new macro being defined)? I would prefer to avoid a versioned dependency to caPutLog in the Gateway, if possible.

bfrk commented 5 years ago

No, caPutLog doesn't have that (yet). Shouldn't be too hard to add, but of course it would be too late to help with the problem at hand.

hhslepicka commented 5 years ago

Ok... changes were split now into smaller commits. It is ready for a new review.

bfrk commented 5 years ago

This is a clear improvement, thanks. I recommend all changes for merging except "ENH: Add ca putLog support. " which I think still needs a bit of cleanup. The addition of gateResources::putLog in src/gateResources.cc/h is fine but the changes to src/gateVc.cc where the new method is called still confuse me. There are some layout/whitespace changes in there, not all of which look justified to me. More importantly, I am unclear about the how and why of the re-grouping of #ifdefs. Perhaps someone more familiar with this part of the gateway code can make more useful comments here. My impression is that some of the changes made to this file are meant as cleanup of the existing code, independent of the addition of the call to gateResources::putLog. If this is the case, these could be separated out and perhaps given a bit of explanation in the comment to the patch/commit.

I wonder why the Travix CI build fails. Perhaps this is due to unmet dependencies in the builder? Have you been abe to run the test suite (I assume there is one) locally?

hhslepicka commented 5 years ago

@bfrk thank you for taking a look at it again. I will try to make the layout/whitespaces more uniform. This patch was extracted from a patch made locally here at SLAC that involves a some other local build changes.

For the Add caputLog support patch, I believe that the regrouping of #ifdefs was because now putLog can be used to write to the file descriptor when caPutLog is used and that is why there is a #ifndef and an #else statement invoking the caPutLog. I will try to contact Bruce Hill (original patch author) and ask more details about the implementation and reason. As soon as I have more information I will update the comments.

For Travis CI, looks like the last time it passed was 3 years ago so I think there is a bit more to it than simply unmet dependencies.

I have not tested it with test suite. We tested it here at SLAC with our IOCs in development.

ralphlange commented 5 years ago

Travis fails because I added tests that show issue #11 but that was never fixed. There is a partial fix in PR #21 - that fixes one of the three tests that are failing. Maybe the tests should be skipped?

bfrk commented 5 years ago

@ralphlange if there is a way to disable/skip only this one (failing) test until someone has an idea how to fix it, this would be the way to go, I guess.

ralphlange commented 5 years ago

OK, will do.

bhill-slac commented 5 years ago

Re the changes to gateVc.cc in 9d76941 ENH: Add ca putLog support The #ifdef WITH_CAPUTLOG changes could be separated out into their own commit. The gateway supports a simple put log w/o caPutLog support which doesn't show the value. I changed this to replace the simple put log entry w/ the new global_resources->putLog() call which shows the same info w/ user, host, and value as caPutLog_Send().

The changes further down related to smartCopy and #if DEBUG_GDD and DEBUG_EVENT_DATA fix crashes if DEBUG_GDD is enabled. There's also a #if logic fix for when to call heading("*** gateVcData::copyState: end", name()); There are also a couple of style/whitespace changes near the end that could also be broken out.

ralphlange commented 5 years ago

Thanks. This pull request has been integrated manually.