Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP for the Properties RFC-5 #30

Open
wants to merge 9 commits into
base: draft
from

Conversation

@bourtemb
Copy link
Member

commented Jul 10, 2019

Related to issue #6

gwen-soleil and others added 3 commits Jul 10, 2019
WIP
- reformulate sentences
- change e-mail addresses format
- fix some typos
@bourtemb

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2019

I attach a presentation I made for Icalepcs 2015 Tango workshop on Tango properties TangoProperties.zip
This could provide some ideas on this very wide topic.

* 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
```

This comment has been minimized.

Copy link
@t-b

t-b Jul 10, 2019

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

This comment has been minimized.

Copy link
@bourtemb

bourtemb Jul 11, 2019

Author Member

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.

This comment has been minimized.

Copy link
@bourtemb

bourtemb Jul 11, 2019

Author Member

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.

This comment has been minimized.

Copy link
@andygotz

andygotz Jul 16, 2019

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

This comment has been minimized.

Copy link
@gwen-soleil

gwen-soleil Jul 17, 2019

Contributor

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

This comment has been minimized.

Copy link
@hardion

hardion Jul 23, 2019

Collaborator

@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?

This comment has been minimized.

Copy link
@bourtemb

bourtemb Aug 7, 2019

Author Member

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)

This comment has been minimized.

Copy link
@bourtemb

bourtemb Aug 8, 2019

Author Member

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)
5/README.md Outdated
* 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"

This comment has been minimized.

Copy link
@t-b

t-b Jul 10, 2019

So the property values are something like

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

?

This comment has been minimized.

Copy link
@andygotz

andygotz Jul 16, 2019

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.

This comment has been minimized.

Copy link
@gwen-soleil

gwen-soleil Jul 16, 2019

Contributor

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?

5/README.md Outdated Show resolved Hide resolved
5/README.md Outdated
* 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

This comment has been minimized.

Copy link
@lorenzopivetta

lorenzopivetta Jul 11, 2019

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"

This comment has been minimized.

Copy link
@bourtemb

bourtemb Jul 11, 2019

Author Member

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)

This comment has been minimized.

Copy link
@bourtemb

bourtemb Jul 11, 2019

Author Member

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.

This comment has been minimized.

Copy link
@bourtemb

bourtemb Jul 11, 2019

Author Member

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.

This comment has been minimized.

Copy link
@lorenzopivetta

lorenzopivetta Jul 11, 2019

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" ?

This comment has been minimized.

Copy link
@bourtemb

bourtemb Jul 11, 2019

Author Member

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.

This comment has been minimized.

Copy link
@andygotz

andygotz Jul 16, 2019

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!).

This comment has been minimized.

Copy link
@lorenzopivetta

lorenzopivetta Jul 16, 2019

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?

This comment has been minimized.

Copy link
@gwen-soleil

gwen-soleil Jul 17, 2019

Contributor

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`
5/README.md Outdated Show resolved Hide resolved
@bourtemb bourtemb requested a review from gwen-soleil Jul 11, 2019
@andygotz

This comment has been minimized.

Copy link

commented Jul 16, 2019

@bourtemb just pointed out there is a default property in the Class and Device supported by pogo.

TODO make sure this is captured in the RFC

Co-Authored-By: Andy Gotz <andy.gotz@esrf.fr>
5/README.md Outdated
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

This comment has been minimized.

Copy link
@bourtemb

bourtemb Aug 7, 2019

Author Member

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.

gwen-soleil added a commit to bourtemb/rfc that referenced this pull request Aug 22, 2019
gwen-soleil and others added 4 commits Aug 22, 2019
Update copyright to tango-controls
Improved system properties and logging properties sections
Added some TODOs
* 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:
* To memorize the Attribute value each time it changes.

This comment has been minimized.

Copy link
@lorenzopivetta

lorenzopivetta Oct 15, 2019

Does this means "marking an attribute as memorized" ? I'd rewrite this as "To mark an attribute as memorized, and store the value each time it changes".

This comment has been minimized.

Copy link
@bourtemb

bourtemb Oct 16, 2019

Author Member

@lorenzopivetta , I agree it is currently a bit ambiguous. I would not use "To mark an attribute as memorized" because this part is done in the code, not in the DB.
I would reformulate to something like:

  • to memorize the Attribute value of a Memorized Attribute when it changes

The idea here was to talk about the role of the __value attribute property.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.