CSSTUDIO-1920 Add alarm limit info to tool tip

[abrahamwolk]

This pull-request adds runtime properties for the alarm limits of widgets that are instances of PVWidget.

It adds the following four runtime properties to instances of PVWidget:

• pv_alarm_lolo
• pv_alarm_low
• pv_alarm_high
• pv_alarm_hihi

These variables are assigned the limits received with incoming data from the PV.

As a consequence, these values can be accessed in macros, and easily displayed in tooltips.

This pull-request adds a substitution of $(pv_alarm_limits) with the PV alarm limits if these are defined to tooltip.setOnShowing().

[kasemir]

I think that's the wrong direction. The PVWidget has a "value", which is a VType, and that comes with time stamp, units, precision, alarm state, maybe alarm limits.
What's in there exactly depends on the data type. Numerics like VDouble will have alarm limits, but binaries like VEnum won't.
If you want to use some of that data in scripts, you can use the existing VType API. Maybe add some formulas to extract the info in a more convenient way. But adding properties pv_alarm_lolo, pv_alarm_low, pv_alarm_high, pv_alarm_hihi to the widget blows up the widget. What's next? Add pv_units, pv_precision, pv_timestamp, pv_alarm_severity?? Also misses the fact that many PVs don't have these properties.

[kasemir]

Misses the point of VType

[abrahamwolk]

I think that's the wrong direction. The PVWidget has a "value", which is a VType, and that comes with time stamp, units, precision, alarm state, maybe alarm limits. What's in there exactly depends on the data type. Numerics like VDouble will have alarm limits, but binaries like VEnum won't.

The code checks that the incoming data point is an instance of DisplayProvider before updating the variables. If it is not, the incoming data point is ignored (for the purpose of updating these variables).

If you want to use some of that data in scripts, you can use the existing VType API. Maybe add some formulas to extract the info in a more convenient way. But adding properties pv_alarm_lolo, pv_alarm_low, pv_alarm_high, pv_alarm_hihi to the widget blows up the widget. What's next? Add pv_units, pv_precision, pv_timestamp, pv_alarm_severity?? Also misses the fact that many PVs don't have these properties.

The idea is to be able to access this information with less overhead than with a script. Using a script to update a tool-tip on incoming data points adds much overhead.

[kasemir]

Still no. Updating tool tips to show limits is an edge case that I've never seen as an operational requirement.
Adding 4 properties to each widget for that is total overkill.
Just display "PVNAME.HIHI" in a text update next to the widget to show the limits.

Or use something like Display.of(pvs[0].read()).getAlarmRange().... in a script for the one tooltip where you want this.

[georgweiss]

We have examples of widgets where users want alarm limits in the tooltip. So they add a script executed on each monitor update. End result is - to give one single example - an OPI executing 46 scripts per second to compute a tooltip that never changes.

My idea of binding alarm limits to runtime properties would eliminate the need for such scripts.

[kasemir]

Still, we can't add any potential VType aspect as a widget property.
Also, why do you turn the tooltip into another mini display with key information about the PV?
What you add to the tooltip is what you get from Right-click -> Probe.

[kasemir]

BOY had the idea of an "OPI Probe". That's just a display, but it's accessible from the context menu similar to Right Click -> Probe, receiving the PV name as a macro.

With something like that you can create an "Inspection" display that shows value, limits, ... in a configurable way, not with a predefined layout as in Probe, and reach that from the context menu.

Maybe add that functionality, a display reachable from the context menu?

[abrahamwolk]

@kasemir What do you think about a more systematic inclusion of all or at least more VTYPE fields as macro values?

Executing a script adds computational overhead, and opening a separate window in order to determine the alarm limits adds cognitive overhead for the operator compared to seeing the information in a tool-tip.

[kasemir]

The VType fields depend on the type. We can't add low/high warnings and alarm thresholds, units, precision, enum labels, image encoding, image data type, image encryption, image width/height, .. all as widget properties, and then keep doing that as new vtypes are added. That blows up widget size for everybody, for thousands of existing displays, just because you might have a few screens where you want some of the information to appear in a tool tip. The widget properties would be updated with each value change even if nobody ever opens the tool tip. You'll also end up with displays where somebody put $(pv_alarm_lolo) into the tool tip on PVs that don't have alarm limits.

If users really need to look into a PV, then opening Probe or the PV Tree is a solid solution that offers access to all the detail.

The tool tip is always a little finicky: Need to place mouse on widget and then take care to not move it. Hard to copy values out or take a screenshot. If you really need the PV name or value etc, you can't conveniently get it from a tool tip. Still, it can be a first check of a PV name and value.
If you look into ToolTipSupport, what $(pv_value) means is really defined in there:

phoebus/app/display/representation-javafx/src/main/java/org/csstudio/display/builder/representation/javafx/widgets/TooltipSupport.java

Line 92 in e6e7b0f

tooltip.setOnShowing(event ->

If the VType includes alarm info, it's added. If the VType includes a timestamp, it's added. If the VType includes display info, it's description (only set for PVA) is added. So you already have a track record of adding stuff to the $(pv_value).

phoebus/app/display/representation-javafx/src/main/java/org/csstudio/display/builder/representation/javafx/widgets/TooltipSupport.java

Lines 112 to 116 in e6e7b0f

	String display = Display.displayOf(vtype).getDescription();
	// Description is non-null only for pva.
	if(display != null && !display.isEmpty()){
	buf.append(System.lineSeparator()).append(display);
	}

--> Why don't you go in there and add the alarm thresholds from the display info? This will show the alarm thresholds for PVs that provide them, it's in the tool tip, the code only runs when a tool tip activates. No overhead at all for the widget size and runtime in general. Nothing an end user needs to configure, so no chance to mess it up. The default tool tip will work just fine.

[abrahamwolk]

--> Why don't you go in there and add the alarm thresholds from the display info? This will show the alarm thresholds for PVs that provide them, it's in the tool tip, the code only runs when a tool tip activates. No overhead at all for the widget size and runtime in general. Nothing an end user needs to configure, so no chance to mess it up. The default tool tip will work just fine.

Thanks for the pointer to this code, I was not aware of it. Adding the information in tooltip.setOnShowing() is a good solution, I think. I will consider how to add the functionality to tooltip.setOnShowing() and update the PR.

[abrahamwolk]

I have updated the PR so that tooltip.setOnShowing() substitutes $(pv_alarm_limits) with the PV Alarm Limits if they are present in the VTYPE.

[kasemir]

This is MUCH better.

Still two comments:

To get/check a Display, there's Display.displayOf(vtype) as already used when checking for the description.

You now require tooltips to be updated to include $(pv_alarm_limits), and we're still stuck with the user having to edit the tooltips while being aware of the data type.
Why not update the replacement of $(pv_value) to be overall better?
Just add the alarm limits (if they are defined) to that text.

[abrahamwolk]

I tried to not change the existing behavior unless the user requested it. However, I agree that adding this information to the default tool-tip would make it overall better. I will update the PR with your suggestions.

[abrahamwolk]

I have updated the PR to use the function Display.displayOf(vtype) to retrieve a Display instance.

There are two problems is one problem, I think, with the proposed solution to add alarm limit information to occurrences of $(pv_name) $(pv_value):

1. Custom values for $(pv_name) $(pv_value) is an optional feature of a widget, and it is only implemented by the TextUpdate-, TextEntry-, and the Spinner widgets. Other widgets expand $(pv_name) $(pv_value) using the macro system.
2. Right now, the default tool-tip is $(pv_name)\n$(pv_value). To me, it seems most natural that the alarm limits would be placed after $(pv_value). If they are placed after $(pv_name), however, the ordering would be $(pv_name)\n$(pv_alarm_limits)\n$(pv_value).

One solution to both points would be to instead of appending the alarm limits to $(pv_name), to append them to $(pv_value) instead for any PV that supports alarm limits.

Another possible solution would be to update the default tool-tip from $(pv_name)\n$(pv_value) to $(pv_name)\n$(pv_value)\n$(pv_alarm_limits).

[kasemir]

Not sure how pv_name enters this discussion. The value, units, time stamp, alarm state/severity and now alarm limits should be in the replacement of pv_value. So simply add the alarm limits to the pv_value replacement.

[kasemir]

In other words: Don't add a new pv_alarm_limits replacement, don't update the default tooltip, simply replace the pv_value with value, units, time stamp, alarm state/severity as before AND add the alarm limits to that replacement.

[abrahamwolk]

Sorry, I was confusing the variable names $(pv_value) and $(pv_name).

I have updated my previous comment to reflect the correction.

[kasemir]

You lost me.
The tooltip replaces $(pv_value) with value, timestamp, alarm state, .. You also want to see the alarm limit, which would make sense to show with the alarm state. So why can't you just add the alarm limits (after checking if there is a Display, and maybe only showing alarm thresholds that are not NaN/Infinity) to the expansion of $(pv_value) in the tool tip code?
Why is there a need for a new pv_alarm_limits replacement, why is there a need to change the default tooltip?

[abrahamwolk]

The tooltip replaces $(pv_value) with value, timestamp, alarm state, ..

Only when pv_value (i.e. the Java-variable pv_value, not the macro variable $(pv_value)) is not equal to null, which is not the case for most widgets. For most widgets, $(pv_value) is resolved using a macro-substitution:

phoebus/app/display/representation-javafx/src/main/java/org/csstudio/display/builder/representation/javafx/widgets/TooltipSupport.java

Line 124 in e6e7b0f

expanded = MacroHandler.replace(macros, spec);

(Somehow the the alarm status etc is bound in the corresponding macro-variable, but not in the call to tooltip.setOnShowing().)

I have now update the PR to append the alarm limits to $(pv_value) when they are available.

[abrahamwolk]

I chose to print "Undefined" when a limit is not defined, since I think it is clearer for the user.

[kasemir]

That looks very reasonable. As for "Undefined", not sure what the best wording would be. "Undefined" might suggest that something is not quite right, like the undefined/UDF error of PVs that have not been processed. For limits, it's perfectly normal to only set the high but not the hihi etc. "Undefined" as in not-defined is correct, but maybe "not set", "unspecified", or the like are good alternatives to avoid confusion with the very common EPICS error of "Undefined" on PVs.

[georgweiss]

Agree, "Undefined" I fear will cause misunderstandings and confusion. My vote is on "not set".

[abrahamwolk]

I have now changed "Undefined" to "Not set".

There is one more issue that I can see: Preferences.tooltip_length can be too small, so that only some of the limits are printed in the tool-tip. What would be the best way of fixing this? I can see three options:

1. Increase the default tool-tip length.
2. Not count the alarm limits towards the tool-tip length (i.e., effectively increasing the maximum tool-tip length to Preferences.tooltip_length + length_of_alarm_limits_representation).
3. Leave the default as it is and let the user set a length in Phoebus.ini.

[abrahamwolk]

I have now implemented option 1 by increasing the default tool-tip length from 150 to 200.

I have also changed the labels in the tool-tip to uppercase (i.e., they are now "HIHI:", "HIGH:", etc. instead of "HiHi:", "High", etc.).

I think that this PR is ready to be merged now.