vehicle_signal_specification icon indicating copy to clipboard operation
vehicle_signal_specification copied to clipboard

proposal for stringlength keyword

Open neil-rti opened this issue 1 year ago • 11 comments

This proposes adding a stringlength keyword to the VSS vspec files that use string data types -- intended as a starting point for discussion.

neil-rti avatar Mar 17 '23 22:03 neil-rti

I think the argument, why as an implementor , you would need/want a bounded length in certain conditions is obvious.

Let's assume we would agree on some way to describe this (i.e. adding a stringlength attribute to the description of signals in the spec), I feel the larger discussion is, how to use it:

  • would we "use" it in default service catalogue? (I guess not, as well, mostly there are no good defaults. Well Maybe in DTCList because maybe those are always Pxxx or Uxxx, but that is maybe the only corner case
  • would we define it as "official" keyword (like arraylength, like aggregate), i.e. supported/generated by tooling as a default, or would we define it as an attribute for an overlay (i.e. how dbc mpppings are epxressed in KUKSA)

I think this is a relevant discussion (although maybe not exactly the same as this one), because due to historic reasons we are not very consistent. If we look at the current state of "deployment" information defined (e.g. information that might change with each implmentation of VSS in a specific vehicle, we have

Deployment Information Used in std catalogue Supported in std tools Comment/Opinion
instance yes yes Should maybe not be defined in std catalgue (no good default), on the other hand, might still want to define some rules (i.e. consider left/right passenger/driver discussion). (Side note: Android seems to have generic - albeit not very amazing - concept of zones of "Positions)
aggregate no (I think) yeah -sort of? I think tools largely ignore it? Here it is a bit unclear whether it "should" be used in std catalgue (i.e. wouldn't it be super helpful for GPS) or whether this is more a marker you can use in your impl, to define what belongs together. Also a question, with struct support, whether this has relevance, and what it would be
arraysize no yes I think this is a sane choice right here: Might be needed for use cases, implementations, but no good default choice available in std catalogue
default yes yes I think this is still used too much in std catalogue. For VSS version it is fine, for Wheelbase nad Door.Count not so much
example: dbc no no One example for some thirdparty overly-attributes: KUKSA defines some keys to allow mapping of DBC defined CAN signals to VSS signals. Obviously not used in std catalogue and not specifically supported in std tools (except via generic overlay format). And this seems the correct choice (Probably even if in COVESA at one point we would agreee to define a "recommended" way to map CAN)

Where does this leave something like stringlength. In my view

Deployment Information Used in std catalogue Supported in std tools Comment/Opinion
stringlength no yes This falls in the same category like arraysize/default: Probably often used/useful for specififc deplyometns, so should be handled in a meaningful way by tools, but no good defaults

Coming back to the orginal discussion, it may be worth thinking about having a "generic" format string for strings, i.e. restrict things by a RegExp? That gives you a more powerful way to describe string limitations, that includes setting the length if you wish - but would of course be a PITA to parse if your only use case actually is limiting the size of some C array

SebastianSchildt avatar Mar 21 '23 07:03 SebastianSchildt

Some comments on the last comment from Sebastian:

  • I agree on that in general instance-definition in standard catalog does not make sense as they often need to be changed to fit number of doors/seats/axles, at least if we intend to have them optimized for individual vehicles. On the other hand, we need some instance-information to be able to generate yaml/json/csv in the same way as we do today, and as they are part of release we need a "default instantiation"
  • Even if we in general not will need to specify stringlength or arraysize in standard catalog, I would rather like them to be described as "rarely" rather than "no", as there might be special cases like VIN and possibly timestamps in standardized format where a special number of characters are expected

But I like the idea of having a table of keywords and state how they are used/checked in different contexts. For the proposed stringlength keyword I can think of the following behavior in VSS-tools:

  • VSS-tools shall give an error if stringlength is used on something not a string
  • VSS-tools shall for Yaml/JSOn exporter just print the keyword as is, for CSV add in new column
  • Behavior for other exporters needs to be defined/investigated - can it be supported? Silently ignored or warning, or even error?

erikbosch avatar Mar 21 '23 08:03 erikbosch

Meeting: Please review/discuss until next meeting

erikbosch avatar Mar 21 '23 16:03 erikbosch

That is the intent, and note that I'm looking at this through the lens of working with DDS, and with constrained or critical systems where byte allocations need to be accounted for. There may be other use cases that need both numbers. This is a good clarifying question. Thanks for bringing it up.

On Mon, Mar 27, 2023 at 3:16 PM Krishna Chaitanya Koppolu < @.***> wrote:

@.**** commented on this pull request.

In stringlength.md https://github.com/COVESA/vehicle_signal_specification/pull/566#discussion_r1149840545 :

+## Proposed Implementation + +The proposed implementation would add in the .vspec files a new key:value pair (herein called stringlength:) +to set the string length of any signal with a string or string[] datatype. +When used with arrays-of-strings (string[]), this keyword sets the max length of the strings in the array, while +the existing arraysize keyword sets the number of strings in the array. + + +## Examples from VSS + +An example implementation might look like: +``` +VehicleIdentification.VIN:

  • datatype: string
  • stringlength: 17

Strings (or collections) have two properties:

  1. capacity - Maximum allowed size
  2. length - Occupied size

Taking the example of C++ basic string, these are modeled as max_size - https://en.cppreference.com/w/cpp/string/basic_string/max_size size/length - https://en.cppreference.com/w/cpp/string/basic_string/size

Size/length seem to be implementation detail as they may vary at the data sample level.

I assume what we are trying to model here is the capacity/max size. Is that correct?

— Reply to this email directly, view it on GitHub https://github.com/COVESA/vehicle_signal_specification/pull/566#pullrequestreview-1359944032, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHNC2KQHSDBD33S3SRYCWOLW6IGT7ANCNFSM6AAAAAAV7CEA5A . You are receiving this because you authored the thread.Message ID: @.*** com>

neil-rti avatar Mar 27 '23 22:03 neil-rti

Meeting notes:

  • Important to clarify if it concerns max capacity, implementation may use flexible string
  • Some exporters may not support it.
  • Neil to start work on updating documentation and updating tools.
  • Keep this PR open as reference and for discussion while developing

erikbosch avatar Mar 28 '23 14:03 erikbosch

I think we should make a decision on how to progress with this one, so that development can start.

  • Decide if we shall introduce a keyword stringlength that specifies max size of strings for this signal. (My understanding is that it is max size, not fixed size)
  • Agree that if not given we anyway have a limit, implicitly or explicitly set by implementation/deployment.
  • Agree if length shall exclude trailing null (if any)
  • Verify that we have common understanding of arraysize - current documentation is a bit ambiguous - does it refer to fix size or max size?

If we agree on introducing it, we need to align on "minimal handling" in vss-tools. Points to agree on:

  • Shall it be included as new column in csv.

erikbosch avatar May 15 '23 08:05 erikbosch

Meeting notes:

  • Erik: propose to make decision next week
  • Sebastian: Where to use? DBC error codes, any more?.
  • Erik: Some examples- VIN, ACRISS, Time in UTC format (as string)
  • Sebastian: Good if we can mention examples.

erikbosch avatar May 16 '23 14:05 erikbosch

6.6.23 @adobekan to check details.

adobekan avatar Jun 06 '23 14:06 adobekan

Discussion:

  • Way forward : @neil-rti (or someone else) to create "real" PR. One for VSS - update doc and add stringlength where reasonable, one for vss-tools so it accepts keyword
  • Only include it for those exporters where it makes sense (i.e. optional for exporters to take it into account)

erikbosch avatar Jun 27 '23 14:06 erikbosch

Converted PR to draft as refactoring is required

erikbosch avatar Sep 12 '23 11:09 erikbosch

Anyone interested in driving the topic of creating PRs (as described above) @neil-rti ?

erikbosch avatar Nov 14 '23 13:11 erikbosch