Regression in calcout setting constant links at runtime

[Andrew Johnson]

With the 3.15 branch, writing a literal number to a calcout.INP[A-L] field causes the corresponding [A-L] field to be set to that number. This works because the calcout record marks the INP fields as special(SPC_MOD) and the calcout::special() routine calls recGblInitConstantLink() to initialize the corresponding [A-L] field from the new link string. I suspect that a number of the synApps record types (transform, scalc, acalc etc.) probably use this same technique.

Keenan Lang reported that this doesn't currently work on the 7.0 branch; the value in the [A-L] field doesn't change, which I have confirmed.

This use of special() to reinitialize constant links is unusual, normally putting a literal into a link field after record initialization has no effect, but someone who worked on the calcout record discovered this way to get it to work.

I think I know what's going on and how to fix it (change dbPutFieldLink() to call dbAddLink() before the second call to dbPutSpecial()), but I need to check that this won't break other cases.

[Andrew Johnson (anj)]

Tricky issue, not quite as simple as I thought it would be.

I first wrote a regression test program which demonstrates the regression 4 times using a calcout record by putting different strings into its INP* fields and looking at the * and IN*V field values that resulted.

To fix it I created a fix that makes dbAccess.c's dbPutFieldLink() routine call dbAddLink() before the second dbPutSpecial() when the new link type is CONSTANT, PV_LINK or JSON_LINK, instead of lower down. This change fixes the bug, but unfortunately breaks the initialization of the dev*softCallback device supports, because they expect their add_record() routines to be called before dbAddLink() resolves a PV_LINK into either a DB_LINK or CA_LINK (they turn it into a PN_LINK instead).

I can resolve the CONSTANT or JSON_LINK types before the second dbPutSpecial(), but the calcout record's special() routine still notices the difference for DB and CA links because it calculates a new INAV field value, and if that happens before dbAddLink() gets called it's going to give the wrong result for them.

I need to back-port my regression test to 3.15 and 3.14 (or run it by hand) to confirm what broke when before I commit anything.

[Keenan Lang (klang)]

>I need to back-port my regression test to 3.15 and 3.14 (or run it by hand) to confirm what broke when before I commit anything.

If it helps at all, I'm pretty sure this bug was around at least as far back as two years ago. Saw this behavior in my lua record, but assumed it was something that I had done that broke things.

https://github.com/epics-modules/lua/issues/4

[Ralph Lange (ralph-lange)]

Is writing numbers to INP fields at run time a behavior that is documented by any sort?

I would feel somewhat reluctant to actively retain a behavior that is undocumented and marks an inconsistency between record types. Feels like cementing an unwanted situation.

[mdavidsaver (mdavidsaver)]

I also have to ask, is this something which should be supported?

What was written to 'INPA' was expected to appear in 'A'. Why not write to 'A' directly?

Was writing to INPA instead of A a deliberate decision? Having given database training classes, the relationship between eg. INPA and A is not obvious to most.

[Andrew Johnson (anj)]

@Keenan, please correct me if what I wrote below is wrong, and point to any documentation you guys have that describe writing constant values to INPx fields...

I was slightly surprised, but this is behavior that our beamlines have apparently been using for run-time database configuration, and the BCDA group here have been incorporating the necessary special() processing code into many of the synApps record types for years. There is a sentence in the calcout wiki-page that says changing constants in the INPx links works; from https://wiki-ext.aps.anl.gov/epics/index.php/RRM_3-14_Calcout#Read_Parameters

> If they are constants, they will be initialized with the value they are configured with and can be changed via dbPuts.

BCDA provide general-purpose tools for visiting scientists to use, including GUI screens for configuring run-time calculations using calcout and other similar record types. Those screens need to have input boxes pointing to the INPx fields to get values from other records, and it makes perfect sense to allow constant values to be typed into those boxes instead of PV names. They have separate text widgets showing the associated value fields (and indicators pointing to the INxV fields that show whether the input link is valid/connected or not), but it is much simpler for the scientist if they don't have to use a separate editable widget for typing in constants.

There might be another way to support this, but it's a bigger change which could break other databases and I'm not so keen on it: The idea would be to have the CONSTANT link type actually return a value to a getValue() request (currently that routine exists but just returns OK without writing anything to the buffer). Numeric conversion from the string could be cached, so it would only be done once and shouldn't affect performance much (although an empty INPx field is a constant with value 0 so there might be some effect, dirtying a cache-line etc.).

[Keenan Lang (klang)]

>Is writing numbers to INP fields at run time a behavior that is documented by any sort?

In the calc record documentation under read parameters is states that the INP links can be initialized with constants and that dbPuts of constants will change its value. Then, under Operands, it states that A-L get their value from the value of the INP links and that the INP fields can be constants.

>What was written to 'INPA' was expected to appear in 'A'. Why not write to 'A' directly?

Because it is also expected to overwrite any existing linkage in INPA. When I change from a DB_LINK to a CA_LINK, all I have to do is push a different value to INPA and the previous link is removed and values are received from the new link. If I want to change from a DB_LINK/CA_LINK to a constant value, it should conceivably be the same process, just write a value to INPA and the record will lose the old link and update to a new value.

If we restrict constants to only being written to the operand fields, then not only does it mean that you have to remember to work with a different field when dealing with a certain datatype, but also you have to manually remove any existing links in INPA or else writing to the 'A' field just doesn't work because the record processes and your changed value is overwritten by the existing link's value.

>I would feel somewhat reluctant to actively retain a behavior that is undocumented and marks an inconsistency between record types. Feels like cementing an unwanted situation.

That's reasonable, however, the synApps sCalcout, aCalcout, transform, and luascript records all duplicate this behavior and from a quick check of a couple of our beamlines, there are definitely IOCs that are reliant on this. While it represents an inconsistency between record types, as argued above, it represents consistency in how the calc/calcout record behaves.

[Andrew Johnson (anj)]

Actually the calc record from Base does *not* work that way; the internal inconsistency is between the calcout (and printf) records on one side and the calc, sub, sel, seq and all the other record types in Base on the other. That's why Ralph and Michael are questioning why I fixed this.

However since synApps has been using this technique for a long time I do think it needs to be kept working, and the tests I included in my associated Merge Request should help us do that.

[Ralph Lange (ralph-lange)]

Ah, sorry, I was misunderstanding the whole thing getting the impression that writing to INPA should immediately write-through to A.

So - to clarify my understanding - the idea is that constant input links can be changed at runtime (just like CA or DB input links) and the new constant value would be used at the next record processing (just like the new CA or DB input link would be executed).

I find this behavior reasonable and useful.
Still, I would like to see things being consistent. Couldn't *all* input links work this way?
Are there applications that rely on the fact that writing a value to an input link does not have an effect?

[Andrew Johnson (anj)]

We could change the code for constant links so they return a value from their lset::getValue() method (currently that routine exists but just returns 0). The string-to-number conversion result could be cached so this doesn't have to be done every time, but this could break any existing databases that provide an initial value using a constant link and later do puts to update the related value field.

I guess we could avoid that by coding getValue() so it would only return a value the first time it's called after the link has been set, and then only if the loadScalar(), routine that gets called from recGblInitConstantLink() API hasn't already been called.

However a lot of older code that reads links doesn't actually call dbGetLink() when the link type is constant (e.g. devAiSoft.c). I removed a lot of those checks from Base already since they aren't needed, but many probably still remain, and that pattern or its equivalent is probably universal in the synApps record types. I just found it in aCalcout, sCalcout, sseq and transform in the calc module, although not in swait. The code in those records is going to need updating to not break if someone gives them a JSON_LINK, but they should continue to work with just the older link types.

[Ben Franksen (bfrk)]

My immediate reaction was the same as Ralph's: if we agree that this is desired behavior then it should apply to all input links in a generic manner, independent of record support and not requiring special processing. I am not familiar with the internal workings of how writing to a link field is implemented, but given that redirection to another PV works, I guess it shouldn't be too hard to make it work for numeric constants?

[Ralph Lange (ralph-lange)]

I think that the consistency I would like to see is between DOL/VAL with OMSL=closed_loop and INPx/x in the calc class of records.

[Andrew Johnson (anj)]

An input link currently has no knowledge of the field to which its data is going to be written. In some cases (e.g. the printf record) there *is* no such field in the record, and the code reading the link may pass a different pointer on each call to getValue() — think double-buffering of arrays for example. The printf record passes a pointer to an auto variable on the stack, and the data type it requests can also vary because it depends on the format string in the FMT field. Thus when an INP field itself gets a new constant value the generic code in dbPutLinkValue() can have no way to know where to put that value. As a result we *cannot* implement a generic solution for this in the core IOC code (dbAccess.c).

The calcout record type was probably the first to implement the technique of making its INP fields special(SPC_MOD) and calling recGblInitConstantLink() from the special() routine; the other synApps records were developed following its example. Keeping that technique working is what this bug is all about — we broke it between 3.15 and 3.16. It does require help from the record to implement though, so if we like this behavior and want it to spread we just have to modify the other record types to do it. Doing that is outside the scope of this bug-fix.

I described an alternative approach earlier (have a constant link's getValue() method return the new value once), but that would still require changes to the record types, and thinking about it further it would cause interesting effects to any record type that uses double-buffering, so I don't recommend this approach. This would also fall outside the scope of this bug-fix.

[Ben Franksen (bfrk)]

> As a result we *cannot* implement a generic solution for this in the core IOC code (dbAccess.c).

If the link field can remember complicated stuff like which PV it points to, plus flags (CPP, MS, whatever) then why can't it remember a single numeric constant and use that constant as the data source when the record calls dbGetLink()?

> Keeping that technique working is what this bug is all about

I am trying to understand your reservation here and failing. I have never understood why in EPICS CONSTANT links can/should only be used for/during initialization, nor why there is a different call to initialize a field from a CONSTANT link instead of adding this functionality to the regular dbGetLink function. Which is why I thought fixing this bug would be a great opportunity to rectify that: get more functionality, less record diversity, more orthogonality.

Just my 2 cents.

[Andrew Johnson (anj)]

> why can't it remember a single numeric constant and use that constant
> as the data source when the record calls dbGetLink()?

I described this possibility earlier (see my comments #5 and #9 to this bug); many record types explicitly check and so don't call dbGetLink() when plink->type==CONSTANT. Thus for this approach to work will require changes to the record types, and the behavior that results is slightly different than that which the calcout's special() routine gives it (a GUI widget showing the A field won't show the value written to INPA until the record next processes).

[Ralph Lange (ralph-lange)]

For me, this behaviour (constant in INP link is the value taken the next time the link is processed) is the best option. (I don't like the "write-through" to the value field.)

I would support making this behaviour consistent over all INPx/x, DOL/VAL (in closed_loop) etc. combinations.

[Ben Franksen (bfrk)]

> constant in INP link is the value taken the next time the link is processed

Yes, this is the most sensible behavior.

> many record types explicitly check and so don't call dbGetLink() when plink->type==CONSTANT. Thus for this approach to work will require changes to the record types

This is true. However, I argue that this is benign for two reasons:

(1) Changes are only needed when and if it is desired that the record type in question make use of the proposed generalization. If no changes are made, the behavior does not differ from teh current one. So there is no pressing need for providers of external record types to adapt their code.

(2) The changes are simplifications: they eliminate a case distinction that is necessary to make now, but won't be in the future, instead will enable more functionality. The code becomes simpler and shorter and easier to maintain in the long run.