Skip to content

Commit

Permalink
Revert "Replace display.format with .form and .precision"
Browse files Browse the repository at this point in the history 
This reverts commit 4ffddfa.
  • Loading branch information
@mdavidsaver
mdavidsaver committed May 6, 2019
1 parent 3ae2d09 commit cd24363
Show file tree
Hide file tree
Showing 6 changed files with 37 additions and 6 deletions.
3 changes: 1 addition & 2 deletions src/factory/StandardField.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,7 @@ StandardField::StandardField()
->add("limitLow", pvDouble)
->add("limitHigh", pvDouble)
->add("description", pvString)
->add("precision", pvInt)
->add("form", enumerated())
->add("format", pvString)
->add("units", pvString)
->createStructure())

Expand Down
15 changes: 14 additions & 1 deletion src/property/pv/display.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ class epicsShareClass Display {
* Constructor
*/
Display()
:low(0.0),high(0.0) {}
: description(std::string("")),format(std::string("")),units(std::string("")),
low(0.0),high(0.0) {}
//default constructors and destructor are OK
/**
* Get the current value of limitLow.
Expand Down Expand Up @@ -74,6 +75,17 @@ class epicsShareClass Display {
* @param value The value.
*/
void setDescription(std::string const & value) {description = value;}
/**
* Get the current value of format.
* @return The current value.
*/
std::string getFormat() const {return format;}
/**
* Set format to a new value.
* @param value The value.
* The rules for a valid syntax has not been specified.
*/
void setFormat(std::string const & value) {format = value;}
/**
* Get the current value of units.
* @return The current value.
Expand All @@ -86,6 +98,7 @@ class epicsShareClass Display {
void setUnits(std::string const & value) {units = value;}
private:
std::string description;
std::string format;
std::string units;
double low;
double high;
Expand Down
1 change: 1 addition & 0 deletions src/property/pv/pvDisplay.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ class epicsShareClass PVDisplay {
static std::string noDisplayFound;
static std::string notAttached;
PVStringPtr pvDescription;
PVStringPtr pvFormat;
PVStringPtr pvUnits;
PVDoublePtr pvLow;
PVDoublePtr pvHigh;
Expand Down
14 changes: 13 additions & 1 deletion src/property/pvDisplay.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@ bool PVDisplay::attach(PVFieldPtr const & pvField)
PVStructurePtr pvStructure = static_pointer_cast<PVStructure>(pvField);
pvDescription = pvStructure->getSubField<PVString>("description");
if(pvDescription.get()==NULL) return false;
pvFormat = pvStructure->getSubField<PVString>("format");
if(pvFormat.get()==NULL) {
detach();
return false;
}
pvUnits = pvStructure->getSubField<PVString>("units");
if(pvUnits.get()==NULL) {
detach();
Expand All @@ -50,6 +55,7 @@ bool PVDisplay::attach(PVFieldPtr const & pvField)
void PVDisplay::detach()
{
pvDescription.reset();
pvFormat.reset();
pvUnits.reset();
pvLow.reset();
pvHigh.reset();
Expand All @@ -66,6 +72,7 @@ void PVDisplay::get(Display & display) const
throw std::logic_error(notAttached);
}
display.setDescription(pvDescription->get());
display.setFormat(pvFormat->get());
display.setUnits(pvUnits->get());
display.setLow(pvLow->get());
display.setHigh(pvHigh->get());
Expand All @@ -76,7 +83,7 @@ bool PVDisplay::set(Display const & display)
if(pvDescription.get()==NULL) {
throw std::logic_error(notAttached);
}
if(pvDescription->isImmutable()) return false;
if(pvDescription->isImmutable() || pvFormat->isImmutable()) return false;
if(pvUnits->isImmutable() || pvLow->isImmutable() || pvHigh->isImmutable())
{
return false;
Expand All @@ -89,6 +96,11 @@ bool PVDisplay::set(Display const & display)
pvDescription->put(display.getDescription());
returnValue = true;
}
if(current.getFormat()!=display.getFormat())
{
pvFormat->put(display.getFormat());
returnValue = true;
}
if(current.getUnits()!=display.getUnits())
{
pvUnits->put(display.getUnits());
Expand Down
4 changes: 3 additions & 1 deletion testApp/property/testProperty.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,13 +172,15 @@ static void testDisplay()
dy.setLow(-10.0);
dy.setHigh(-1.0);
dy.setDescription(string("testDescription"));
dy.setFormat(string("%f10.0"));
dy.setUnits(string("volts"));
result = pvDisplay.set(dy);
testOk1(result);
pvDisplay.get(display);
testOk1(dy.getLow()==display.getLow());
testOk1(dy.getHigh()==display.getHigh());
testOk1(dy.getDescription().compare(display.getDescription())==0);
testOk1(dy.getFormat().compare(display.getFormat())==0);
testOk1(dy.getUnits().compare(display.getUnits())==0);
double low = display.getLow();
double high = display.getHigh();
Expand Down Expand Up @@ -215,7 +217,7 @@ static void testEnumerated()

MAIN(testProperty)
{
testPlan(26);
testPlan(27);
testDiag("Tests property");
fieldCreate = getFieldCreate();
pvDataCreate = getPVDataCreate();
Expand Down
6 changes: 5 additions & 1 deletion testApp/pv/testPVData.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,10 @@ static void testPVScalarWithProperties(
string("display.description"));
testOk1(desc.get()!=0);
desc->put(string("this is a description"));
PVStringPtr format = pvStructure->getSubField<PVString>(
string("display.format"));
testOk1(format.get()!=0);
format->put(string("f10.2"));
PVStringPtr units = pvStructure->getSubField<PVString>(
string("display.units"));
testOk1(units.get()!=0);
Expand Down Expand Up @@ -736,7 +740,7 @@ static void testSubField()

MAIN(testPVData)
{
testPlan(261);
testPlan(271);
try{
fieldCreate = getFieldCreate();
pvDataCreate = getPVDataCreate();
Expand Down

12 comments on commit cd24363

@kasemir
Copy link

@kasemir kasemir commented on cd24363 May 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this affect the data provided by QSRV, where EPICS 7.0.2.2 now gets us the desired 'form' and 'precision'?

@mdavidsaver
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. #62 and epics-base/pva2pva#24 are independent from each other.

@kasemir
Copy link

@kasemir kasemir commented on cd24363 May 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good. Do I have this correct:

The EPICS IOC, using QSRV, does and will continue to provide values with form and precision.
The pvDataCPP, normative type, ... libraries can't easily be updated because of tests and paperwork.

@mdavidsaver
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difficulty is with clients, as in AD, which use the isCompatible() tests from the ntCPP module (epics-base/normativeTypesCPP#15). These tests are extremely strict. Changing any aspect of the display sub-struct will cause eg. the AD client to reject the whole NTNDArray. So even were ntCPP updated, older clients would be unable to use newer servers.

So we're in a situation where some degree of compatibility break seems inevitable...

@mrkraimer
Copy link
Contributor

@mrkraimer mrkraimer commented on cd24363 May 7, 2019 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kasemir
Copy link

@kasemir kasemir commented on cd24363 May 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest CS-Studio code checks if there's a "format" or the new "form" and "precision", so it handles both.
I think we should move forward, meaning the AD client needs to adjust to the new display info in a similar way.

@anjohnson
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Section 2 of the Normative Types specification says:

The Normative Types definitions in this document each have the following general form:

  1. They are defined as structures, composed of fields.
  2. They usually have one primary field called "value", which encodes the most important data of the type.
  3. They are composed of required fields, and optional fields. The required fields come first, the optional fields follow.
  4. The order of fields matters. Although the Normative Types pvData binding allows for access though an introspection API, senders must encode the fields in the order described in this document.

The combination of points 3 and 4 above explains why isCompatible() is checking index numbers for the fields — the spec says that all the required fields must be present in the order given, and any additional fields must come later on. I see no wiggle room there (which was probably intentional).

ISTR suggesting that we might need to include both display.format and display.form fields for at least one release. In the meantime perhaps we should come up with a revised specification that says what we want it to say for future releases.

For those that aren't aware, one type (maybe the only one, not sure) for which the order of fields really matters is the set of columns in an NTTable. The value field is a structure containing a series of scalar array sub-fields, one for each table column. The separate labels field is an array of strings that provides the column labels for those columns, and the indices for the sub-field and its label must be the same, so implementations must store the column fields using an order-preserving container such as an ordered dictionary in Python.

@mdavidsaver
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ISTR suggesting that we might need to include both display.format and display.form

I had this thought as well, but sadly this will not avoid a compatibility break. One of the tests in NTField::isDisplay() is:

size_t n = structurePtr->getNumberFields();
if(n!=5) return false;

@mrkraimer
Copy link
Contributor

@mrkraimer mrkraimer commented on cd24363 May 8, 2019 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kasemir
Copy link

@kasemir kasemir commented on cd24363 May 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not change NTField::isDisplay?

I think that goes into the right direction.

On one extreme, you can argue that this is a change to the "normative types". So need to create paperwork to define a new version "epics:nt/NTScalar:2.0" with updated display definition.

On the other hand, the latest writeup I can find is
http://epics-pvdata.sourceforge.net/alpha/normativeTypes/normativeTypes.html,
"Editors Draft, 16-Mar-2015", which states

format
   A format for converting the value field to a string
   Needs work: What is display.format?
   What's it for and what are examples?
   If it's a sprintf pattern, which syntax must it conform to - C or Java?

So even in the definition it was never fully defined.
Now it is being defined in a more practical way, and implementations naturally need to be updated, just like CS-Studio which has already been updated.

@mrkraimer
Copy link
Contributor

@mrkraimer mrkraimer commented on cd24363 May 8, 2019 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mdavidsaver
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not change NTField::isDisplay ?

cf. epics-base/normativeTypesCPP#15

Please sign in to comment.