Skip to content
This repository has been archived by the owner on Apr 7, 2022. It is now read-only.

WIP for the Properties RFC-5 #30

Closed
wants to merge 9 commits into from
Closed

WIP for the Properties RFC-5 #30

Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
52e5214
WIP
gwen-soleil Jul 10, 2019
7902371
WIP on properties
gwen-soleil Jul 10, 2019
b2b0422
WIP (reformulations, typos, e-mail format)
bourtemb Jul 10, 2019
2d6a471
Added Andy Gotz as contributor in properties rfc 5 (#1)
andygotz Jul 16, 2019
ec3172e
Update introduction of 5/README.md
gwen-soleil Jul 18, 2019
9b00d8d
integrate comments from PR #30 + WIP
gwen-soleil Aug 22, 2019
e01dcb8
WIP
gwen-soleil Aug 22, 2019
ab4ed16
Class specification is part of the device RFC
bourtemb Sep 13, 2019
da3d37f
Take into account Vincent Hardion comments from https://github.com/bo…
bourtemb Sep 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
86 changes: 72 additions & 14 deletions 5/README.md
View file
Open in desktop
@@ -1,14 +1,18 @@
---
domain: rfc.tango-controls.org
shortname: 5/PROPERTY
name:Property
name: Property
status: raw
editor: Vincent Hardion (vincent.hardion@maxiv.lu.se)
editor: Gwenaëlle Abeillé `<at synchrotron-soleil.fr - gwenaelle.abeille>`
contributors:
- Vincent Hardion `<at maxiv.lu.se - vincent.hardion>`
- Reynald Bourtembourg `<at esrf.fr - bourtemb>`
- Andy Gotz <andy.gotz@esrf.fr>
---

This document describes the Property, a Tango concept representing an element of configuration in order to customise a Tango element. This document describes version 1.0 of the Property.
This document describes the Property, a Tango concept representing one or more values to configure one of the following Tango elements namely Tango Device, Class, Attribute, System or a not associated to any Tango element, so called free properties. This document describes version 1.0 of the Property.

See also: Y/OtherTemplate
See also: 2/Device, X/Database, X/Attribute, X/Class

## Preamble

Expand All @@ -22,7 +26,12 @@ The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "S

## Tango Property Specification

A Property is designed to represent any information of configuration in Tango.
A Property is a persisted item of Tango that is mainly used for configuration purposes. 4 types of properties exist:

* Class property: a property that is attached to a Device Class (cf RFC-9). This property is accessible from all devices of the class
* Device property: a property that is attached to a Device (RFC-2). This property is accessible from only one device
Copy link
Member Author

Choose a reason for hiding this comment

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

This property is accessible from all devices of the class
This property is accessible from only one device
I find these sentences a bit confusing... Technically, nothing prevents another device or a client to have access to a device property value defined in the DB or even to the default value of a device property.

* Attribute property: a property that is attached to an Attribute (RFC-4).
* Free property: TODO

### Goals

Expand All @@ -45,23 +54,72 @@ There are many use cases for the usage of a Property:

* To change the representation of an information in order to be more user friendly a Property can define a new data format for the graphical user interface to convert the raw data.

* TODO: Memorized Attribute

## Specification

A Tango Property is a strict definition of a pair of key/value
* configuration are represented in the form of properties.
* data are represented in the form of attributes.
* actions are represented in the form of commands.

Its model can be represented as a defined tree which each elements are from a defined types: Class, Device, Property, Attribute, Command following the rules below:

## Specification

A Tango Property is a strict definition of a pair of key/value
* The Property SHALL have one key, called Property Name
* The Property SHALL have one value
* The Property SHALL have one value, which could be empty, called Property Value
* If the device is started upon a Tango Database, Class, Device, Attribute and Free Properties MAY be persisted into the Tango Database (RFC6)
* If the device is started without a Tango Database, the device and class properties MAY be persisted in a file (but no support for the attribute properties)
* All device and class properties MAY be directly modified and accessed through the database device or its file
* Attribute properties SHOULD be modified and accessed at any time through the device object (RFC-2)
* All default attribute and device properties are OPTIONAL
* The attribute, class and device properties MAY be defined in the device code and they MAY be OPTIONAL OR REQUIRED (mandatory)
* TODO memorized attributes, system properties
* TODO : properties history
* TODO: error management
* All device and class properties MUST be loaded at init command or device start-up
* If a device property does not exist, the device MUST load the class property having the same name
Copy link

Choose a reason for hiding this comment

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

The current flow is:
1 - check if class property exist, load it
2 - check if default value exist, load it
3 - check if value is defined in database, load it
This guarantees that, whenever defined, default value will override class value, and db value will override default value (and class as well).
Propose to reword line 74 something like:
"For each Property, the device checks and extracts Property value from Class, default device value and database defined value, with increasing priority"

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 current flow is:
1 - check if class property exist, load it
2 - check if default value exist, load it
3 - check if value is defined in database, load it

A more correct way to express it would be:
The current flow is:
1 - check if class property exist, load it, else check if default value exist, load it
2 - (executed in any case) check if value is defined in database, load it

The way you wrote it suggested that the default property would override the class property (at least this is the way I interpreted it)

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 way you wrote it suggested that the default property would override the class property (at least this is the way I interpreted it)

And it looks like this is the way you interpreted it too.. This is not the case in the current implementations. A default value does not override a class property.

Copy link
Member Author

Choose a reason for hiding this comment

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

So, starting from your sentence proposal, I would change it to:
For each Property, the device checks and extracts default device value, Property value from Class and database defined value, with increasing priority.

Copy link

Choose a reason for hiding this comment

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

You're right. Had a quick look and missed the "else". But what about "For each Property, the device checks and extracts Property value from default device value, Class and database defined value, with increasing priority" ?

Copy link
Member Author

Choose a reason for hiding this comment

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

But what about "For each Property, the device checks and extracts Property value from default device value, Class and database defined value, with increasing priority" ?

Looks better to me indeed.

Copy link

Choose a reason for hiding this comment

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

What about replacing "database defined value" with "device specific value". Or we should say that the Device specific value can be defined in multiple places e.g. database, file or in the code (aie!).

Copy link

Choose a reason for hiding this comment

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

The way I was reading this is:
default device Property value = if specified in the Pogo dialog... this ends up in the XXXClass.cpp file
default Class Property value = if specified in the Pogo dialog... this ends up in the XXXClass.cpp file
database Class Property value = if Class Property defined in the Tango db...
database device Property value = if device Property defined in the Tango db...
Not sure about your reference to "file", does this refers to running a device with db on file? Isn't this just a special case of database?

Copy link

Choose a reason for hiding this comment

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

To simplify the reading, I would propose to add pseudo-code. Something like:

  • A device or class property MAY have an OPTIONAL default value
  • The device property loading flow in pseudo-code is:
property-value = getDevicePropertyFromTangoDBorFile(device-name, property-name)
if(property-value is empty)
  property-value = getClassPropertyFromTangoDBorFile(class-name, property-name)
if(property-value is empty && default-value is not empty)
  property-value = default-value
  • The class property loading flow property-name is:
property-value = getClassPropertyFromTangoDBorFile(class-name, property-name)
if(property-value is empty && default-value is not empty)
  property-value = default-value`

* All device and class properties MAY be modified and accessed through the database device when using a Tango Database (RFC-6)
* A Tango device MUST manage the following default device properties: cmd_min_poll_period, min_poll_period, attr_min_poll_period, poll_ring_depth, cmd_poll_ring_depth, attr_poll_ring_depth, polled_attr, logging_target, logging_level, logging_rft (TODO: description of each one)
* An attribute MUST manage the following default attribute properties (TODO: descriptions):
hardion marked this conversation as resolved.
Show resolved Hide resolved
* String **label** default value "";
* String **description** default value "No description"
* String **unit** default value "No unit"
* String **standard_unit** default value "No standard unit"
* String **display_unit** default value "No display unit"
* String **format** default value :
* attribute type is a string, "%s"
* attribute type is float or double, "%6.2f"
* "Not specified" otherwise
* String **min_value** default value "Not specified"
* String **max_value** default value "Not specified"
* String **min_alarm** default value "Not specified"
* String **max_alarm** default value "Not specified"
* String **min_warning** default value "Not specified"
* String **max_warning** default value "Not specified"
* String **delta_t** default value "Not specified"
* String **delta_val** default value "Not specified"
* String **abs_change** default value "Not specified"
* String **rel_change** default value "Not specified"
* String **event_period** default value "Not specified"
* String **archive_period** default value "Not specified"
* String **archive_rel_change** default value "Not specified"
* String **archive_abs_change** default value "Not specified"
* String **enum_labels** default value "Not specified"
* String **__root_att** default value "Not specified"
* String **enum_labels**
* String **__value**


### Naming convention
* The Property's Name SHALL use the following convention:
``` ABNF
property-name = 1*VCHAR
```
Copy link

Choose a reason for hiding this comment

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

From my testing with Jive I've found that I can add a property with an empty name as well. And I can also just use a space ( ) as name. There are even no checks for duplicated names so two properties with empty names are possible as well. So I think this is more like property-name = 0*CHAR.

Looking into the database schema, https://github.com/tango-controls/TangoDatabase/blob/master/create_db_tables.sql.in, the name is limited to 255 characters so we can write it as.

property-name = 0*255CHAR

Copy link
Member Author

Choose a reason for hiding this comment

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

From my testing with Jive I've found that I can add a property with an empty name as well. And I can also just use a space ( ) as name. There are even no checks for duplicated names so two properties with empty names are possible as well. So I think this is more like property-name = 0*CHAR.

This would not be true if we consider properties defined from a file (without Tango Database).
POGO does some checks on the property name and won't accept an empty property name and would remove any space. POGO also forbids device property names with some special characters like '%' for instance which could be interpreted as a wildcard by a Database engine.

Looking into the database schema, https://github.com/tango-controls/TangoDatabase/blob/master/create_db_tables.sql.in, the name is limited to 255 characters so we can write it as.

Indeed, there is this limitation by default.
This is also defined in cppTango for instance:
https://github.com/tango-controls/cppTango/blob/tango-9-lts/cppapi/server/tango_const.h.in#L221

property-name = 0*255CHAR

As we said this morning during the teleconf, this might be true with the current jive implementation (not true when using POGO or a file DB). We will make a note that the current implementation allows this kind of property name (empty name of with spaces and tabs) but this is due to issues with the current implementation. empty property names, property names with tabs, spaces and some special characters (we need to find the list) should be forbidden.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here is the method which does the check in POGO:
https://github.com/tango-controls/pogo/blob/Pogo-Release-9.6.23/org.tango.pogo.pogo_gui/src/org/tango/pogo/pogo_gui/tools/Utils.java#L404

As I understand this code, only alphanumeric characters are allowed [a-z][A-Z][0-9] and the underscore.
The property shall start with a letter.
An attribute property (and only an attribute property) can start with an underscore.

@srgblnch , @hardion , this could be useful for #7: this POGO piece of code (combined with a slightly deeper analysis) also highlights that "State" and "Status" command names are reserved. Command names seem to be case sensitive because command names "state" and "status" seem to be accepted by POGO.
I've done a quick test generating a C++ device server with a command named "state" returning a DevState and it seems that this use case is not well supported. Pogo generated the source code, but created a method
Tango::DevState MyClassName::state()

AtkPanel did not recognize this "state" command. tg_devtest (jive test device) listed "state" and "State" as 2 commands available on the device but executing the "state" command from tg_devtest did not execute Tango::DevState MyClassName::state() method, but MyClass::dev_state() was executed instead, as for the "State" command.

Similar behaviour when adding a command named "status".

So it would probably be good to specify that "state" and "status" should be reserved|forbidden too as command names.

Copy link

Choose a reason for hiding this comment

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

This is indeed a deficiency of Pogo and should be avoided. To be added to RFC on Commands, Attributes and State.

Copy link

Choose a reason for hiding this comment

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

Since the specifications were not there at the time of implementation, I think it is normal that we fall into those kind of inconsistencies. So I think we should compromise on a specification even though the current implementation does not reflect 100% the specification. I propose:
property-name = 1*255ALPHA

Copy link
Collaborator

@hardion hardion Jul 23, 2019 •
edited

Choose a reason for hiding this comment

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

@srgblnch , @hardion , this could be useful for #7: this POGO piece of code (combined with a slightly deeper analysis) also highlights that "State" and "Status" command names are reserved. Command names seem to be case sensitive because command names "state" and "status" seem to be accepted by POGO.
I've done a quick test generating a C++ device server with a command named "state" returning a DevState and it seems that this use case is not well supported. Pogo generated the source code, but created a method
Tango::DevState MyClassName::state()

Indeed a good point. The draft of the device RFC already define to have a State and Status attributes. Would it make sense to define what is a State and a Status command in the Command RFC and make mandatory the State and Status command in the Device RFC?

Copy link
Member Author

Choose a reason for hiding this comment

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

Here is the method which does the check in POGO:
https://github.com/tango-controls/pogo/blob/Pogo-Release-9.6.23/org.tango.pogo.pogo_gui/src/org/tango/pogo/pogo_gui/tools/Utils.java#L404

As I understand this code, only alphanumeric characters are allowed [a-z][A-Z][0-9] and the underscore.
The property name shall start with a letter.
An attribute property (and only an attribute property) can start with an underscore.

So if we follow the rules defined by code in POGO, I think we would get something like that for the property names in abnf:

alphanum = ALPHA / DIGIT 
underscore = %x5F
device_property_name = 1*ALPHA *(alphanum / underscore)
class_property_name = 1*ALPHA *(alphanum / underscore)
free_property_name = 1*ALPHA *(alphanum / underscore)
attribute_property_name = 1*(ALPHA / underscore) *(alphanum / underscore)

Copy link
Member Author

Choose a reason for hiding this comment

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

And if we take into account that the maximum property name length is 255 we would get something like:

alphanum = ALPHA / DIGIT 
underscore = %x5F
device_property_name = 1*1ALPHA 0*254(alphanum / underscore)
class_property_name = 1*1ALPHA 0*254(alphanum / underscore)
free_property_name = 1*1ALPHA 0*254(alphanum / underscore)
attribute_property_name = 1*1(ALPHA / underscore) 0*254(alphanum / underscore)

* The property name is case insensitive
* The Property value MUST use the following convention:
single value that may contain any character
or an array with carriage return
special values: "NaN", "inf"
Copy link

Choose a reason for hiding this comment

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

So the property values are something like

propery-value = CHAR / 1*CHAR CR / "NaN" / "inf"

?

Copy link

Choose a reason for hiding this comment

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

I would say:

property-value = 1*[1*VCHAR | "nan" | "inf"]

where VCHAR is the visible character set and nan and inf are case in-sensitive. The CR is part of the implementation in the database. I am not sure it belongs in the property value definition.

Copy link

Choose a reason for hiding this comment

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

property-value = 1*[1*VCHAR | "nan" | "inf"] does not respect the ABNF standard for me (https://tools.ietf.org/html/rfc5234 or https://en.wikipedia.org/wiki/Augmented_Backus%E2%80%93Naur_form). If the CR is part of the implementation in the database, how can we define arrays?

* TODO nodbproperties file convention
```
# --- 1/1/1 properties
1/1/1->myProp:titi


CLASS/MyClass->myClassProp: 10
```