ControlSystemStudio / cs-studio

Control System Studio is an Eclipse-based collections of tools to monitor and operate large scale control systems, such as the ones in the accelerator community.
https://controlsystemstudio.org/
Eclipse Public License 1.0
111 stars 96 forks source link

Possible metadata issue #2716

Closed ttkorhonen closed 1 year ago

ttkorhonen commented 2 years ago

When I use probe to look at a PV, it reported that alarm limits are "NaN". Screenshot 2022-03-07 at 10 03 00

However, pvget as well as caget tell me that the limits are set: Screenshot 2022-03-07 at 17 07 54

(lower limits are indeed not set but upper limits are.)

Am I interpreting wrong what probe is telling me, or is this a bug?

kasemir commented 2 years ago

Is this for phoebus, not the older Eclipse-based CS-Studio? Can you paste a demo record with the exact settings?

ttkorhonen commented 2 years ago

This is for Phoebus. I have to create a demo record, this particular record has a bit complicated configuration. Anyway, the macro string looks like this: "P=DTL-010, IDX=001, LOLO=0, LLSV=NO_ALARM, LOW=0, LSV=NO_ALARM, HIGH=30, HSV=MINOR, HIHI=50, HHSV=MAJOR, HYST=5, HOPR=150, LOPR=10"

kasemir commented 2 years ago

OK, here's an example record with those values that exhibits the issue:

record(ao, "limits")
{
  field("HHSV", "MAJOR")
  field("HSV", "MINOR")
  field("LSV", "NO_ALARM")
  field("LLSV", "NO_ALARM")
  field("HIHI", "50")
  field("HIGH", "30")
  field("LOW", "0")
  field("LOLO", "0")
  field("HYST", "5")
  field("HOPR", "150")
  field("LOPR", "10")
}

There are two things going on.

One is that the "0" for LOW and LOLO are reported by the IOC as NaN, when they were indeed configured in the record. Should report zero, not NaN.

The other problem is in the 'VType' Range class that I think goes back to the PVManager. This record has upper limits HIGH and HIHI, while the lower LOW and LOLO limits are NaN/not set. The 'VType' Display, however, doesn't look at them as an upper/lower pair of thresholds. It combines HIHI and LOLO into the 'alarm' range, and the HIGH and LOW thresholds into the 'warning' range. Each Range must be valid at both start and end:

https://github.com/epics-base/epicsCoreJava/blob/71b384b6092ee0311a46447e90bac3e1dc50a229/epics-util/src/main/java/org/epics/util/stats/Range.java#L215

    public static Range of(final double minValue, final double maxValue) {
        if (Double.isNaN(minValue) || Double.isNaN(maxValue)) {
            return Range.UNDEFINED;
        }

Do one end of the range being NaN turns the complete range NaN.

@shroffk : What do we do about that? Remove that check?

kasemir commented 2 years ago

One is that the "0" for LOW and LOLO are reported by the IOC as NaN, when they were indeed configured in the record. Should report zero, not NaN.

Timo, do you want to bring that up as a separate EPICS base issue? In fact if that was fixed, CS-Studio would be happy as well.

ttkorhonen commented 2 years ago

Good point, I agree that the fix belongs to EPICS base. I will report that. Thanks for the quick feedback!

kasemir commented 2 years ago

Well, in this example EPICS needs to be fixed first. But we'd still have this case where I really only set the HIGH limit:

record(ao, "limits")
{
  field("HSV", "MINOR")
  field("HIGH", "30")
}

For that, the IOC would be OK to report

    Lo alarm limit:   nan
    Lo warn limit:    nan
    Hi warn limit:    30
    Hi alarm limit:   nan

and CS-Studio would then turn the warning range of lower=NaN, upper=30 into lower=upper=NaN.

Of course a range from NaN to 30 is somewhat iffy, like how large is it?? I guess there simply isn't a "warning range", the lower and upper warning are simply thresholds that don't need to combine into a range.

ralphlange commented 2 years ago

The 'VType' Display, however, doesn't look at them as an upper/lower pair of thresholds. It combines HIHI and LOLO into the 'alarm' range, and the HIGH and LOW thresholds into the 'warning' range.

That doesn't make sense at all, does it? You can't combine HIHI and LOLO for an alarm range, unless you work with inverse ranges, in the sense of "everything outside the range between LOLO and HIHI is an alarm".

For a record with HIGH and HIHI set (and HIHI > HIGH), valid ranges would be: -inf ... HIGH = no alarm HIGH ... HIHI = warning HIHI ... +inf = alarm

ttkorhonen commented 2 years ago

I fully agree with -inf and +inf, but how should one interpret NaN?

kasemir commented 2 years ago

That doesn't make sense at all, does it?

Correct. LOLO, LOW, HIGH, HIHI are 4 thresholds (with optional HYSTeresis). The VType Range is cute but doesn't fully apply. Could simply use it as a plain old data structure, a pair of a 'lower' and 'upper' threshold where any of them may be NaN, doesn't have to be a valid 'range'.

anjohnson commented 2 years ago

As I just posted to core-talk, alarm limits will be reported by the IOC as NaN if the associated severity field is NO_ALARM.

kasemir commented 2 years ago

The EPICS base code in the aoRecord looks like this:

static long get_alarm_double(DBADDR *paddr, struct dbr_alDouble *pad)
{
    aoRecord    *prec=(aoRecord *)paddr->precord;

    if(dbGetFieldIndex(paddr) == indexof(VAL)){
        pad->upper_alarm_limit = prec->hhsv ? prec->hihi : epicsNAN;
        pad->upper_warning_limit = prec->hsv ? prec->high : epicsNAN;
        pad->lower_warning_limit = prec->lsv ? prec->low : epicsNAN;
        pad->lower_alarm_limit = prec->llsv ? prec->lolo : epicsNAN;
    } else recGblGetAlarmDouble(paddr,pad);
    return(0);
}

I think that's OK. Any limit where the severity is NO_ALARM is set to NaN, while for the rest we get the number. So the issue here is solely within the VType Range second-guessing the numbers and trying to enforce a true range, when in fact we just have 4 thresholds.

ttkorhonen commented 1 year ago

I close this issue as there was no issue (other than lack of understanding on my side) after all. Thank you for the clarification!

kasemir commented 1 year ago

No worries, you reported a valid issue. The VType Range that caused problems when one of the endpoints was not set (NaN) has been updated, https://github.com/epics-base/epicsCoreJava/commit/718607332adb6f301d5db975c81ea8787a3047e2 , which should fix this issue.