COVESA / vehicle_signal_specification

Vehicle Signal Specification - standardized way to describe automotive data
Mozilla Public License 2.0
325 stars 168 forks source link

Use of string constants in VSS #323

Closed erikbosch closed 2 years ago

erikbosch commented 3 years ago

When looking at enum documentation I realized that we are quite inconsistent concerning strings in VSS, if we use single quotes, no quotes or double quotes. This might possibly make life difficult for tools, and I think it would be good to align and document it. As part of this it would be good to document what special characters that are allowed for enums and/or string constants. If we allow special characters like line breaks, blank space, commas or quotes, we also need a defined escape-mechanism, possibly like "hi\"there!" or similar.

A few examples from VSS:

enum: [ "Inactive", "Close", "Open", "OneShotClose", "OneShotOpen" ]
enum: [ 'type 1', 'type 2', 'ccs', 'chademo' ]
enum: [ good, flat spot, low, uneven, separated, other damage ]

default: "unknown"
default: ccs

Some ideas:

oliveirarleo commented 3 years ago

I think this is a valid discussion and ends up showing that some information is missing comparing to enums in almost every programming language.

Usually, enums come as ways to describe categories, or limited amounts of options to values. Here follows an example in C#:

enum ErrorCode : ushort
{
    None = 0,
    Unknown = 1,
    ConnectionLost = 100,
    OutlierReading = 200
}

We have described here an association between symbolic values and values, like in the line ConnectionLost = 100,. As discussed in our meeting, I believe now enum: follows with the values, not the symbolic values. We were using the strings received as the symbolic values, internally converting flat spot to FLAT_SPOT for example, which is not ideal.

Maybe a way to solve this would be having a structure similar to the following one:

MyLeaf:
  type: actuator
  datatype: int32
  enum: [ "None", "Unknown", "ConnectionLost", "OutlierReading" ]
  enumvalues: [ 0, 1, 100, 200]

And if enumvalues (or any other better name) is not present, to assume it is the integers from zero on ([ 0, 1, 2, 3] in case of 4 elements for example).

alexanderdomin commented 3 years ago

Also I would like to add my comments to this topic and address following three aspects:

  1. General guidline to document enumerations in VSS (yaml)

    Following naming convention for enumeration keys can be followed in the VSS specification. This is nothing I was inventing but teking as a best practis from OSS and our Intrnal projects:

    • Use only upper case characters from [A-Z][A-Z0-9_]*. e.g.: [ UNKNOWN, STRAIGHT, V, BOXER, W, ROTARY, RADIAL, SQUARE, H, U, OPPOSED, X ] e.g.: [ ONE, TWO, THREE ]
    • Use "_" to separate characters/words. e.g.: [ YYYY_MM_DD, DD_MM_YYYY, MM_DD_YYYY, YY_MM_DD, DD_MM_YY, MM_DD_YY e.g.: [ MI_KWH, KM_KWH, KWH_100MI, KWH_100KM, WH_MI, WH_KM ]
    • An apostrophe is not needed. YAML parser is creating the element as a string already.
  2. Enable interface designers to specify values for enumerations:

    In our Franca service descriptions we are using enumerations. For some of the enumerations we are not specifying any values, this will be done by the CommonAPI tooling in the generation step. But I also see a lot of examples where interface designers precisly define the values thay need for enumerations. Dow to this fact I see a need to definde such velues also for enumerations in VSS. To cover this aspect I propose to crate following structure:

MyLeaf:
    type: actuator
    datatype: uint8
    enum:
        - UNKNOWN:
            value: 100
        - STRAIGHT:
            value: 200
  1. Description of enumerations

    In the current VSS specification I see some examples to document description of enumerations in the general description. I see folowing issues with this appoach:

    • First of all the description is growing and it is hard to find the right enumeration in it.
    • Secondly you have to do the work twice. You list the name of the enumeration key twice in the enumeration array itself and additionly in the description. This can cause some issues.

    To cover this aspect I propose to extend proposed structure with following attributes:

MyLeaf:
    type: actuator
    datatype: uint8
    enum:
        - UNKNOWN:
            value: 100
            description: "Write your description here"
        - STRAIGHT:
            value: 200
            description: "Write your description here"
Similar appoache we have and use in interfaces specified in Franca
UlfBj commented 2 years ago

I would like an enum syntax as shown in the example below.

AspirationType:
  datatype: enum
  type: attribute
  enum: [ UNKNOWN = 0, NATURAL=1, SUPERCHARGER=2, TURBOCHARGER=3 ]
  default: UNKNOWN
  description: Type of aspiration (natural, turbocharger, supercharger etc).

The enum number datatype must be int32. The proposed comment field could be used if enum values needs further explanation.

oliveirarleo commented 2 years ago

I would like an enum syntax as shown in the example below.

AspirationType:
  datatype: enum
  type: attribute
  enum: [ UNKNOWN = 0, NATURAL=1, SUPERCHARGER=2, TURBOCHARGER=3 ]
  default: UNKNOWN
  description: Type of aspiration (natural, turbocharger, supercharger etc).

The enum number datatype must be int32. The proposed comment field could be used if enum values need further explanation.

So this approach is interesting, but it has the disadvantage of the enum attribute being parsed as a list of "UNKNOWN = 0" strings, which would have to be parsed separately by this tool after the yaml parsing. An alternative is to have:

AspirationType:
  datatype: enum
  type: attribute
  enum: [ UNKNOWN: 0, NATURAL: 1, SUPERCHARGER: 2, TURBOCHARGER: 3 ]
  default: UNKNOWN
  description: Type of aspiration (natural, turbocharger, supercharger etc).

Which would generate a list of attributes.

Another disadvantage would be having no way of describing the enum datatype, which would have to be fixed.

alexanderdomin commented 2 years ago

I would like an enum syntax as shown in the example below.

AspirationType:
  datatype: enum
  type: attribute
  enum: [ UNKNOWN = 0, NATURAL=1, SUPERCHARGER=2, TURBOCHARGER=3 ]
  default: UNKNOWN
  description: Type of aspiration (natural, turbocharger, supercharger etc).

The enum number datatype must be int32. The proposed comment field could be used if enum values needs further explanation.

In your proposal I miss the description. It has a big advantage do document a description for each enumeration. We use it in our current projects with Franca. Doing it inside the general description will make the documentation unmaintainable and complicated due to repeated writing of the values in the description.

adobekan commented 2 years ago

I think this is a valid discussion and ends up showing that some information is missing comparing to enums in almost every programming language.

@oliveirarleo Not sure if something is missing or it just might be the case that enum as such is just wrong in model specification. Did you think about scenario if the values are not the same in every vehicle or data source? Also, if you would implement let's say web api, what would you provide as an answer to the request where field is described with enum? I think that enums are great but they should be realized if needed in the implementation and not in model specification, it is something really specific to each use case.

danielwilms commented 2 years ago

IMHO to have restrictions of what is allowed is a useful thing and to have rules for them, too (like in 1. in https://github.com/COVESA/vehicle_signal_specification/issues/323#issuecomment-906990746).

We do call it now enums, and I think the term is misleading. I don't see the need in the standard catalogue have "real" enums with symbolic and real values, because:

  1. It's an implementation detail to make it more efficient. Different language handle it in different way and it can be easily generated.
  2. when it comes to overhead, I would see that as the job of the serialisation. If you use plain json and send funny numbers around instead of the string, I don't see the point so much.

I think it mainly adds complexity to the model and taking up implementation details, which we don't wanna have there. My proposal would be to rename enum as a keyword into allowedValues or validInput or something like that and leave it to the implementation to optimize.

alexanderdomin commented 2 years ago

Sorry to say so but with the argument "enum values are project specific" we can close the VSS discussion. I thought VSS is an agreement between OEMs to have have a common understanding of data keys, types and values. But if we only agree on keys the usage of VSS is useless. Any Web App developer will have to guess which data is coming.

In general I think we have to void usage of enumerations as fare as possible. But if we use them we have to be able to defined the ID, the value and the description. In Franca it is possibly why it is not possible to do in VSS?

danielwilms commented 2 years ago

Sorry to say so but with the argument "enum values are project specific" we can close the VSS discussion.

I didn't say it's project specific. I said it's an implementation detail how to handle those input restrictions efficiently.

I thought VSS is an agreement between OEMs to have have a common understanding of data keys, types and values. But if we only agree on keys the usage of VSS is useless. Any Web App developer will have to guess which data is coming.

Let's look at an example: https://github.com/COVESA/vehicle_signal_specification/blob/e4adabe9b66258b5632e85cc03e89cc4251fa2cb/spec/Cabin/SingleSliderSwitch.vspec#L13-L17

Now I modify it a little

Switch:
  datatype: string
  type: actuator
  allowedValues: [ "Inactive", "Close", "Open", "OneShotClose", "OneShotOpen" ]
  description: Switch controlling sliding action such as window, sunroof, or blind.

Now I think it's clear for the developer you mention on what to expect. The name is there, the datatype and the allowed values using the very same datatype and the type. I don't see what is missing. I see that the keyword enum is misleading.

In general I think we have to void usage of enumerations as fare as possible. But if we use them we have to be able to defined the ID, the value and the description. In Franca it is possibly why it is not possible to do in VSS?

Franca has a very different scope than VSS. If you need the ID for your code generation or serialization, use layers. If you need further description, use the description field. Up to now I didn't see any argument against the points I raised earlier (https://github.com/COVESA/vehicle_signal_specification/issues/323#issuecomment-983570608)

alexanderdomin commented 2 years ago
  1. So if the community agrees to defined only allowed values and delete Enumerations is fine for me. Someone has just to think about how and where to write the mapping between the value and the key to bring things together. I think its not user friendly but its your decision.
  2. To descriptions: I was hoping to use Yaml to structure elements and to allow the computer to distinguish things for the generation. But as I understand now we are using one description to describe everything. Personally I would do it moor formal but its also fine for me.
SebastianSchildt commented 2 years ago

I think there are separate concerns here.

If I am looking at how we use enums today, I agree with Daniel, this really only restrict inputs. I would change the keyword for that, such as

Charging.ChargePlugType:
 datatype: string
  type: attribute
  default: ccs
  acceptedValues: [ 'type 1', 'type 2', 'ccs', 'chademo' ]
  description: Type of charge plug available on the vehicle (CSS includes Type2)

this is useful and clean. If so desired (I do not think we should do in the core spec), this might even be extended to support more complex restrictions such as providing a JSON schema/XSD/Regexp...

I think enums in the "programming language" sense as in "Mapping a symbolic name to a number" is too low level for VSS. This is an implementation thing. If the group disagrees, of course we need to discuss how to do that.

Before voting for enums, consider where would you need such mapping? "Below" VSS, i.e. when feeding data from lower parts of the software stacks, "numbers" are highly implementation specific anyway (whether I need to map the pattern 0b11 to "open" or to "1" which then gets a symbolic name in VSS doesn't change anything). If we are talking about efficient transport/serialisation protocols (i.e. when you are not living JSON world), consider that YAML sequences are ordered: https://yaml.org/spec/1.2.2/#10112-generic-sequence , so for a given version of vspec, tooling/generators can unambigously map "accpetedValues" to numbers if so desired, without vspec manually enforcing a mapping.

erikbosch commented 2 years ago

With https://github.com/COVESA/vehicle_signal_specification/pull/404 merged, I now consider this one closed