Change display.format

[mdavidsaver]

Replace printf-style format string with enum + integer. See #19. I'd like to hear from some developers of OPI clients prior to merging.

1. Is this better than the current 'display.format' string?
2. Are the choices for format type reasonable?
3. Is the interpretation of 'precision' clear? (negative values?)
4. Is there a better way?

With this change, a record

record(ai, "format") {
    field(VAL , "4.321")
    field(DESC, "testing")
    field(HOPR, "10.123")
    field(LOPR, "-10.123")
    field(EGU , "x")
    field(PREC, "2")
    info(Q:format, "Decimal")
}

would result in

...
    structure display
        double limitLow -10.123
        double limitHigh 10.123
        string description testing
        string units x
        int precision 2
        enum_t format (2) Decimal
            int index 2
            string[] choices ["Default", "String", "Decimal", "Exponential", "Hex", "Engineering"]
...

@shroffk @kasemir @georgweiss @hhslepicka

[kasemir]

Yes, much better than the current 'display.format' string.

Format type choices are good, mapping well to https://github.com/shroffk/phoebus/blob/master/core/ui/src/main/java/org/phoebus/ui/vtype/FormatOption.java.
Missing Sexagesimal, but even if everybody who needs paid a penny, we'd still not be able to retire early.

I thought the interpretation of 'precision' was clear.
For decimal it's the number of digits after the decimal point.
For exponential & engineering it's the same applied to the mantissa.
For hex, I suggest the same as in the CSS format option: Number of hex digits, like 8 for 0x00000000.

Since you mentioned negative values for the precision, don't know how that would be used.

[hhslepicka]

@mdavidsaver, I agree with @kasemir. It is much better this way! 👍
This match exactly what we offer at PyDM as well.
I would just add Binary as an additional format to the list if there is space for another one and folks think that it is an useful option.

I am also not sure how to deal with the negative values for precision and where it would be used. Can you give us an example?

[kasemir]

Binary as in 0b00010101011101 with precision = number of bits?
For the UI, that's typically displayed with a ByteMonitor widget that has one LED-type indicator per bit.
But I'd be OK with adding that as a format, so it could also be used in text update or even text entry widgets and thus allow users to enter binary numbers.

[ralphlange]

I think this should be about displaying a value, not about entering it.

When entering a numerical value in a text input, I would always expect reasonable conversions to be applied, so that entering in hex, octal, binary (for integers), or exponential (for floats) just works without additional configuration.

[kasemir]

Ralph, that's OK when you require people to enter "0x12" for hex.
With the text input widget we always allow that for the default option.
But when the field is formatted as Hex, you can enter just "12" and it's still interpreted as hex.

[ralphlange]

Fine.
The only thing I miss is knowing which base I am using when typing inside a Hex/Oct/Bin formatted field without using a prefix. But that's the last 2%...

[hhslepicka]

@kasemir that is exactly what I meant for the Binary... we have the Byte widget but we also allow users to display a value as binary at Textboxes as well as labels.

[mdavidsaver]

Missing Sexagesimal, but even if everybody who needs paid a penny, we'd still not be able to retire early.

Precisely. I don't expect (or really want) this list to be exhaustive. And I expect that there will always be a way for individual displays to override display.format. The main point is that once added, choices can't be readily removed. So I'm going to hold off on adding the three flavors of Sexagesimal (base 60). I will add Binary though, in part because "precision = number of bits" seems like such an obvious thing once you say it.

So that leaves me with the following ordered list:

• Default
• String
• Binary
• Decimal
• Hex
• Exponential
• Engineering

[anjohnson]

The Astronomy community may ask about Sexagesimal (for celestial coordinates) once they start to look at PVA, but given their recent progress that could be quite a few years off. Assuming it won't be too hard to add formats in the future I'm okay with the current list, although I note that it is missing Compact (%g) from Kay's Phoebus list. What will "Default" actually mean?

display.format was originally a string; this PR makes it an enum so display.format.index will be an integer and display.format.choices an array of strings. What could happen when code that was written for the old definition sees data formatted with the new one? Would it be wise to rename the enum (some ideas, I kind of like display.form although for UI dev's it might be confusing) to avoid triggering problems with older clients? Servers/IOCs might then be able to provide both if the developer wishes (requiring a flag-day change-over is always hard).

[mdavidsaver]

What will "Default" actually mean?

I see the purpose of this exercise as providing a hint in a form specified by the consumers of this hint. I'm not going to try to tell them how to use it. The decision I'm willing to make is to delay adding choices in the interest of preventing a premature proliferation. eg. I'm not saying "no" to Sexagesimal, I'm saying "not yet".

[mdavidsaver]

Would it be wise to rename the enum

I forgot to repeat myself from #19 (comment)

The re-use of 'display.format' with a different type (enum vs. string) seems likely to cause problems. An alternative name will be needed.

I kind of like display.form

What should it be? Time to put on your name czar hat!

[mdavidsaver]

Servers/IOCs might then be able to provide both if the developer wishes (requiring a flag-day change-over is always hard).

Show me an OPI client which currently uses the display.format string. cs-studio/BOY used to, but at some point stopped.

[kasemir]

Show me an OPI client which currently uses the display.format string.

Well the "VTypes" do, but that was a consequence of the V4 API only providing a format string.
So even the DBR_CTRL.. precision was turned into a format string as the least common denominator, and later we try to introspect that format to get the numeric precision back out.

Would be much happier with just the numeric precision, plus a hint how to use that: decimal, engineering, hex, ...

[anjohnson]

What should it be?

The enum provides an answer to the question "how should the value of this channel be displayed" so I suggest naming it display.how, giving display.how.choices and display.how.index.

Any objections to or problems with this?

One more question: Do/did the pvtools programs support display.format and if so will they continue to do so when presented with older PVs?

[ralphlange]

I strongly object against using interrogative words and particles as names for structure elements.
Unless we plan to support what.when for timestamps and what.showme.how.howmany for the display precision.

[mdavidsaver]

Would be much happier with just the numeric precision, plus a hint how to use that: decimal, engineering, hex, ...

In my mind, this is a done deal at this point. We're just haggling over the details.

Well the "VTypes" do

From my test with cs-studio (NSLS2) 4.3.3, and Timo's original report using 4.6.1, I concluded that recent versions of cs-studio/BOY effectively ignored display.format.

Am I wrong in this?

I know that the format string was used at some point (circa 2016). Early on I set display.format="0" and spent a confused hour trying to figure out why all of my PV values were rendered as 0.

[mdavidsaver]

Do/did the pvtools programs support display.format and if so will they continue to do so when presented with older PVs?

No, and never have. Thinking about how to do this safely is part of what led me to conclude that the display.format string idea was dangerously broken.

[kasemir]

recent versions of cs-studio/BOY effectively ignored display.format.

Since we have to use the VType, which has a format and no precision, the precision received from Channel Access it turned into a format, and the format received from PVAccess is copied as such.

The format is then used in a few places like the PV Tree, but we also have a lot of code like this that tries to recover the precision from the format:

https://github.com/ControlSystemStudio/cs-studio/blob/master/applications/opibuilder/opibuilder-plugins/org.csstudio.simplepv/src/org/csstudio/simplepv/VTypeHelper.java#L642

https://github.com/kasemir/org.csstudio.display.builder/blob/master/org.csstudio.display.builder.model/src/org/csstudio/display/builder/model/util/FormatOptionHandler.java#L72

https://github.com/ControlSystemStudio/cs-studio/blob/master/applications/opibuilder/opibuilder-plugins/org.csstudio.opibuilder.widgets/src/org/csstudio/opibuilder/widgets/editparts/SpinnerEditpart.java#L130

[mdavidsaver]

@kasemir I think we're having two different conversations. I'm trying to find out if simply removing display.format would have a practical effect for any users. As @anjohnson points out, this would be a client visible change. However, before I commit to maintaining this backwards compatibility I'd want to hear from user(s) who

A) Use QSRV
B) Clear qsrvDisableFormat so that a format string is actually provided
C) Have an OPI client which uses display.format.

It sounds like your answer to C for cs-studio is a conditional yes. That some parts do. I admit I don't often use PV Tree.

I don't believe that SNS uses QSRV, so I'm guessing you wouldn't be effected?

As for the fact that the format string idea has been baked into the VType interfaces. All I can do is apologize and sympathize.

[georgweiss]

ESS depends on QSRV.

[kasemir]

I'm trying to find out if simply removing display.format would have a practical effect for any users

From the SNS perspective, I suggest to simply remove the display.format.

Has ESS written custom clients that depend on the display.format string?
If you're basically using CSS as a client, that'll be handled by a CSS update.

As far as I can tell the current CSS vtype.PV and phoebus.pv code that decodes the pvdata checks for a subfield "format". So right now removing the format wouldn't result in a crash. You'd simply get some default number format.
@shroffk has recently updated the VTypes, so I assume he could update the VType.Display to carry the new precision and enum.
I'll then update the vtype.PV and all code that uses it in CSS/RCP as well as the phoebus.pv and all code that uses it in CSS/Phoebus so we not only won't crash, but actually use the precision & format suggestion.

[ttkorhonen]

I am not aware of any ESS custom clients that use the format string. Even if we had, we would fix them to work with this proposal. I would also vote for removing the display.format, leaving it in would just invite confusion.

[thomascobb]

This sounds like a good idea to me, but what is rationale behind making it an enum rather than a string? I can't imagine a time that a client would need to know what all the different possible values of the enum...

[kasemir]

A client like CS-Studio would like to offer a 'default' option that uses the formatting hint from the PV.
For that to function, the PV needs to provide a known set of options like an enum { Decimal, Engineering, .. }.

[anjohnson]

Final decision: display.form — this is not an abbreviation of display.format. The server is suggesting the outline/style/guise/pose in which the value should be displayed, but we aren't letting it specify that in explicit detail. If the server wants to control exactly which characters should be shown it should represent the value as a string and do the conversion itself.

This term could be confused with a fill-out-form (e.g. a tax form), but that is a different sense of the word than the one I'm using.

[mdavidsaver]

Updated structure

    structure display
        double limitLow -10.123
        double limitHigh 10.123
        string description testing
        string units x
        int precision 2
        enum_t form (3) Decimal
            int index 3
            string[] choices ["Default", "String", "Binary", "Decimal", "Hex", "Exponential", "Engineering"]

@thomascobb says:

I can't imagine a time that a client would need to know what all the different possible values

It also allows QSRV to report typos. eg. info(Q:form, "Enginering").