Merge ~anj/epics-base/+git/base-7.0:fix-1824277 into ~epics-core/epics-base/+git/epics-base:7.0

Looking for any final requests or veto's to this PR before tomorrow when I'm planning to merge it and work on the 7.0.2.2 release. Do you guys all understand it enough now (after the discussion that took place last week on the linked bug report)?

I'd like to see some test coverage for the more common case of writing to eg. '.A', both when INPA="" and when INPA="5" (or other non-empty).

Does this change effect all link fields now? (eg. INP for aiRecord)

Also, have you ever seen external record support special() looking for PV_LINK? It looks to me like such cases would instead see DB_LINK or CA_LINK.

My only objection is to rushing it out. lp:1824277 does not seem to effect many users. Let's not break something which does. Maybe we merge this just after 7.0.2.2?

There should be no change to the behavior of writing to the related value field, which for the calcout isn't marked as special although it is pp so doing so does trigger record processing. Nothing in that chain of processing has been touched though, it's only dbPutFieldLink() that I've modified, the routine that handles puts to a link field. However I just added a couple of testdbPutFieldOk() calls, writing to the two input-value fields that have const INP links.

The fix may slightly alter the code path taken anytime something writes to a link field at runtime, ai.INP fields included, but in most cases there should be no change to the result. It is unusual for a link field to be marked special, and in Base the calcout is the only type which has any special links.

There are a few references to PV_LINK in the epics-modules repo's, but I think you're right that most code won't normally see plink->type == PV_LINK; the only time that should happen with this fix is in a dsxt::add_record() routine where the link type in the device() entry in the DBD file is given as CONSTANT (which means "or DB/CA/PV/JSON LINK").

The bug hasn't affected many users because there hasn't been a release of synApps for EPICS 7 yet, and it's the synApps users who will see and complain about it. Keenan Lang is working to release synApps 6.1 by the end of May (see his question at https://github.com/epics-modules/iocStats/issues/33#issuecomment-484995327), but I don't know if that is intended to support EPICS 7 — synApps 6.0 was aimed at Base-3.15.5. If we're planning to merge other major changes after 7.0.2.2 that could preclude us putting out a patch release for this bug-fix unless we start a release branch.

I understand this seems like a rush, but a synApps release strikes me as a good reason to try and get this fix included.

> The bug hasn't affected many users because there hasn't been a release of synApps for EPICS 7 yet, and it's the synApps users who will see and complain about it.

I have been running 7.0.2.2 with synApps for 2 APS run cycles now. What is the bug?

Mark

________________________________
From: <email address hidden> <email address hidden> on behalf of Andrew Johnson via Core-talk <email address hidden>
Sent: Monday, April 22, 2019 11:40 PM
To: Andrew Johnson
Subject: Re: [Merge] ~anj/epics-base/+git/base-7.0:fix-1824277 into epics-base:7.0

There should be no change to the behavior of writing to the related value field, which for the calcout isn't marked as special although it is pp so doing so does trigger record processing. Nothing in that chain of processing has been touched though, it's only dbPutFieldLink() that I've modified, the routine that handles puts to a link field. However I just added a couple of testdbPutFieldOk() calls, writing to the two input-value fields that have const INP links.

The fix may slightly alter the code path taken anytime something writes to a link field at runtime, ai.INP fields included, but in most cases there should be no change to the result. It is unusual for a link field to be marked special, and in Base the calcout is the only type which has any special links.

There are a few references to PV_LINK in the epics-modules repo's, but I think you're right that most code won't normally see plink->type == PV_LINK; the only time that should happen with this fix is in a dsxt::add_record() routine where the link type in the device() entry in the DBD file is given as CONSTANT (which means "or DB/CA/PV/JSON LINK").

The bug hasn't affected many users because there hasn't been a release of synApps for EPICS 7 yet, and it's the synApps users who will see and complain about it. Keenan Lang is working to release synApps 6.1 by the end of May (see his question at https://github.com/epics-modules/iocStats/issues/33#issuecomment-484995327), but I don't know if that is intended to support EPICS 7 - synApps 6.0 was aimed at Base-3.15.5. If we're planning to merge other major changes after 7.0.2.2 that could preclude us putting out a patch release for this bug-fix unless we start a release branch.

I understand this seems like a rush, but a synApps release strikes me as a good reason to try and get this fix included.
--
https://code.launchpad.net/~anj/epics-base/+git/base-7.0/+merge/366247
Your team EPICS Core Developers is requested to review the proposed merge of ~anj/epics-base/+git/base-7.0:fix-1824277 into epics-base:7.0.

>Keenan Lang is working to release synApps 6.1 by the end of May (see his question at https://github.com/epics-modules/iocStats/issues/33#issuecomment-484995327), but I don't know if that is intended to support EPICS 7

I don't think this release will officially support EPICS 7, but I am definitely going to try to start getting all of our code compliant with newer versions. That's what ended up finding this issue as I was updating some of my own code for the changes to links.

>I understand this seems like a rush, but a synApps release strikes me as a good reason to try and get this fix included.

Since the whole of synApps isn't likely to use EPICSv7 at least for the next year or more, there's not really a rush. The only IOC's going in that use it will be for specific areaDetector cameras and they don't really use the record types affected.

>What is the bug?

Record types that are based off of calcout can no longer have a constant value written to their input fields and have the value of the correct data field change. For example, writing '5' to a calcout's INPA field no longer changes the A field to 5. This affects the calcout, aCalcout, sCalcout, transform, and luascript records.

> Since the whole of synApps isn't likely to use EPICSv7 at least for the next year or more, there's not really a rush. The only IOC's going in that use it will be for specific areaDetector cameras and they don't really use the record types affected.

synApps modules are in production use on APS sector 13, and I believe also sector 16, with base 7.0.2.2 right now

Mark

________________________________
From: <email address hidden> <email address hidden> on behalf of Keenan Lang via Core-talk <email address hidden>
Sent: Tuesday, April 23, 2019 9:43 AM
To: Andrew Johnson
Subject: Re: [Merge] ~anj/epics-base/+git/base-7.0:fix-1824277 into epics-base:7.0

>Keenan Lang is working to release synApps 6.1 by the end of May (see his question at https://github.com/epics-modules/iocStats/issues/33#issuecomment-484995327), but I don't know if that is intended to support EPICS 7

I don't think this release will officially support EPICS 7, but I am definitely going to try to start getting all of our code compliant with newer versions. That's what ended up finding this issue as I was updating some of my own code for the changes to links.

>I understand this seems like a rush, but a synApps release strikes me as a good reason to try and get this fix included.

Since the whole of synApps isn't likely to use EPICSv7 at least for the next year or more, there's not really a rush. The only IOC's going in that use it will be for specific areaDetector cameras and they don't really use the record types affected.

>What is the bug?

Record types that are based off of calcout can no longer have a constant value written to their input fields and have the value of the correct data field change. For example, writing '5' to a calcout's INPA field no longer changes the A field to 5. This affects the calcout, aCalcout, sCalcout, transform, and luascript records.
--
https://code.launchpad.net/~anj/epics-base/+git/base-7.0/+merge/366247
Your team EPICS Core Developers is requested to review the proposed merge of ~anj/epics-base/+git/base-7.0:fix-1824277 into epics-base:7.0.

Then I guess it's up to you guys how important it is to have this fix. Do you think you are likely to switch between links and constant values in user calculations? If you aren't switching between the two, you can just change the A-L fields directly.

Hi Keenan,

Does synApps have GUI screens that have a text-entry widget for the INP fields but only a text-display widget for the corresponding value field on these record types? That's the other case I'm most concerned about.

- Andrew

On 4/23/19 10:41 AM, Lang, Keenan C. wrote:

From a scan of our IOC's, it crops up in user calcs, pid calculations and limits, slit transforms, bpm calculations, cmdReply formatting, shutter arming, and a database for fastshutter limits.

________________________________
From: Mooney, Tim M.
Sent: Tuesday, April 23, 2019 10:11:53 AM
To: Johnson, Andrew N.; <email address hidden><mailto:<email address hidden>>; Lang, Keenan C.; <email address hidden><mailto:<email address hidden>>
Subject: Re: [Merge] ~anj/epics-base/+git/base-7.0:fix-1824277 into epics-base:7.0

>>What is the bug?

>Record types that are based off of calcout can no longer have a constant value written to their input fields and have the >value of the correct data field change. For example, writing '5' to a calcout's INPA field no longer changes the A field to >5. This affects the calcout, aCalcout, sCalcout, transform, and luascript records.

I'm sure I've never written a database that relies on this behavior.

Tim Mooney (<email address hidden><mailto:<email address hidden>>) (630)252-5417
Beamline Controls Group (www.aps.anl.gov<http://www.aps.anl.gov>)
Advanced Photon Source, Argonne National Lab

________________________________
From: <email address hidden><mailto:<email address hidden>> <email address hidden><mailto:<email address hidden>> on behalf of Keenan Lang via Core-talk <email address hidden><mailto:<email address hidden>>
Sent: Tuesday, April 23, 2019 9:43 AM
To: Johnson, Andrew N.
Subject: Re: [Merge] ~anj/epics-base/+git/base-7.0:fix-1824277 into epics-base:7.0

>Keenan Lang is working to release synApps 6.1 by the end of May (see his question at https://github.com/epics-modules/iocStats/issues/33#issuecomment-484995327), but I don't know if that is intended to support EPICS 7

I don't think this release will officially support EPICS 7, but I am definitely going to try to start getting all of our code compliant with newer versions. That's what ended up finding this issue as I was updating some of my own code for the changes to links.

>I understand this seems like a rush, but a synApps release strikes me as a good reason to try and get this fix included.

Since the whole of synApps isn't likely to use EPICSv7 at least for the next year or more, there's not really a rush. The only IOC's going in that use it will be for specific areaDetector cameras and they don't really use the record types affected.

>What is the bug?

Record types that are based off of calcout can no longer have a constant value written to their input fields and have the value of the correct data field change. For example, writing '5' to a calcout's INPA field no longer changes the A field to 5. This affects the calcout, aCalcout, sCalcout, transform, and luascript records.
--
https://code.launchpad.net/~anj/epics-base/+git/base-7.0/+merge/366247
Your team EPICS Core Developers is requested...

I believe all of the synApps screens have text-entry widgets for both the INPx and x value fields.

I checked the auto settings .req files and they save both the INPx and x value fields in autosave.

Changing the behavior so constant link fields cannot be changed at run-time should thus not break existing databases, since the x value fields will be autorestored. It would require training users to enter new constants into the value fields, not INP fields.

Mark

-----Original Message-----
From: <email address hidden> <email address hidden> On Behalf Of Andrew Johnson via Core-talk
Sent: Tuesday, April 23, 2019 10:57 AM
To: Andrew Johnson <email address hidden>
Subject: Re: [Merge] ~anj/epics-base/+git/base-7.0:fix-1824277 into epics-base:7.0

Hi Keenan,

Does synApps have GUI screens that have a text-entry widget for the INP fields but only a text-display widget for the corresponding value field on these record types? That's the other case I'm most concerned about.

- Andrew

On 4/23/19 10:41 AM, Lang, Keenan C. wrote:

>From a scan of our IOC's, it crops up in user calcs, pid calculations and limits, slit transforms, bpm calculations, cmdReply formatting, shutter arming, and a database for fastshutter limits.

________________________________
From: Mooney, Tim M.
Sent: Tuesday, April 23, 2019 10:11:53 AM
To: Johnson, Andrew N.; <email address hidden><mailto:<email address hidden>>; Lang, Keenan C.; <email address hidden><mailto:<email address hidden>>
Subject: Re: [Merge] ~anj/epics-base/+git/base-7.0:fix-1824277 into epics-base:7.0

>>What is the bug?

>Record types that are based off of calcout can no longer have a constant value written to their input fields and have the >value of the correct data field change. For example, writing '5' to a calcout's INPA field no longer changes the A field to >5. This affects the calcout, aCalcout, sCalcout, transform, and luascript records.

I'm sure I've never written a database that relies on this behavior.

Tim Mooney (<email address hidden><mailto:<email address hidden>>) (630)252-5417 Beamline Controls Group (www.aps.anl.gov<http://www.aps.anl.gov>)
Advanced Photon Source, Argonne National Lab

________________________________
From: <email address hidden><mailto:<email address hidden>> <email address hidden><mailto:<email address hidden>> on behalf of Keenan Lang via Core-talk <email address hidden><mailto:<email address hidden>>
Sent: Tuesday, April 23, 2019 9:43 AM
To: Johnson, Andrew N.
Subject: Re: [Merge] ~anj/epics-base/+git/base-7.0:fix-1824277 into epics-base:7.0

>Keenan Lang is working to release synApps 6.1 by the end of May (see
>his question at
>https://github.com/epics-modules/iocStats/issues/33#issuecomment-484995
>327), but I don't know if that is intended to support EPICS 7

I don't think this release will officially support EPICS 7, but I am definitely going to try to start getting all of our code compliant with newer versions. That's what ended up finding this issue as I was updating some of my own code for the changes to links.

>I understand this seems like a rush, but a synApps release str...

>Changing the behavior so constant link fields cannot be changed at run-time

That's already how it is working in 3.16 onward. This change would be to restore the previous operation.

Looking through the new code for dbPutFieldLink(), I'm suspicious that the allocated 'pdbaddr' would be leaked in some (probably error) cases. This may already be possible. Good practice might be to set 'pdbaddr=NULL' when it is consumed by dbAddLink(), then add a 'free(pdbaddr)' to the cleanup step. Also need to make dbAddLink() free it on error.

Looking at this again. What concerns me is that dbPutFieldLink() sits at the conjunction of three external interfaces (rset, dset, and lset). It isn't clear to me what the expectations and allowed actions are. Particularly of link support. If this is clear to anyone (Andrew) I would find it quite helpful to see some comments added to describe the state(s) that the target link may be in at, and after, the external interface calls.

As an example. I've just noticed that dbDbRemoveLink() always free()s the memory pointed to be 'plink->value.pv_link.pvt', but doesn't always NULL the pointer.

Group 10/4: Needs more thorough testing, extra cases. AJ to re-evaluate the fix.

Any progress on this issue in the last two years? I had users this Monday complaining that upgrading their IOCs from 3.14.12 to EPICS 7.0.5 broke their application.

@Dirk, This MR is waiting on review, expanded test coverage, and test results. Can you help with these? Does this MR fix the specific "breakage" seen by your users?

The specific breakage was: Changing calcout.INPx to a differenct constant does not work any longer.
The application is a user configureable calc where the users enter CALC and set INPx fields to other records or to constants on the fly. The very use case this fix has been made for. Thus I guess it will work, but before trying it, I wanted to know why it is on hold (priority "high", "In Progress", "needs fixing") for so long. Any severe doubts that it will work?

@Dirk That does sound like exactly the issue that I was working on fixing. My proposal for the fix was questioned by the other Core developers with the concerns described above, but due to the demands of my APS Upgrade development work I just haven't had time to address them yet. If you have time to pick this up I'd be very happy for you to do that.

There is one merge/rebase conflict in dbAccess.c which needs an understanding of both this branch and the other changes that have happened for it to be fixed (replacing pdbaddr with chan in the call to dbAddLink()), that will probably require some additional changes outside of the conflicting area. I suggest you first rebase this branch onto 7.0 and resolve that issue, then push it to your GitHub repo. Once you've started a PR there instead I will close this MR – the ability to review code and make comments works much better there.

I had re-based the change onto the latest 7.0 yesterday, including resolving the conflict in dbAccess.c which came from the change to use dbChannel.I have addressed the issue Michael mentioned:
"Good practice might be to set 'pdbaddr=NULL' when it is consumed by dbAddLink(), then add a 'free(pdbaddr)' to the cleanup step. Also need to make dbAddLink() free it on error."
I already did exactly that when changing dbAccess to used dbChannel and resolved that now in the merge.

It builds and fixes the problem with calcout. Of course it does not magically provide the same feature to all other records, e.g. calc. I still have to understand why the changes in *SoftCallback supports are needed.

The rebased version is here: https://github.com/dirk-zimoch/epics-base/tree/fix-1824277

Rebased again (because I'd forgotten that Dirk already did it once, and the number of tests in regressTest.c has gone up since) to https://github.com/anjohnson/epics-base/tree/fix-1824277
I will use that branch to address Michael's requests for additional comments.

Needs a PR on GitHub when ready for further review.