search

COVESA / vss-tools

Software for working with VSS (https://github.com/COVESA/vehicle_signal_specification)
Mozilla Public License 2.0
54 stars 55 forks source link

VSSNode variable initializations #354

Closed nwesem closed 2 months ago

nwesem commented 4 months ago

Hi everyone,

I ran into an issue with the way variables are initialized in the VSSNode class. In this case variables min and max are the problem. For vspec2id we are creating a 4-byte hash using multiple attributes (name/path, unit, type, datatype, enum values/allowed, or minimum/maximum). This is the way min and max (and other) are initialized (line 79/80).

https://github.com/COVESA/vss-tools/blob/109b3edb5b2a2a0b0abec0852a23996da274e67a/vspec/model/vsstree.py#L35-L92

Now if the minimum value for e.g. a percentage is 0 and the min value is implicitly tansformed to an integer (which means it was set from a vehicle specification) and you want to export to a file using one of the exporters we mostly do something like if node.min: ....

https://github.com/COVESA/vss-tools/blob/109b3edb5b2a2a0b0abec0852a23996da274e67a/vspec/vssexporters/vss2id.py#L97-L100

These if-clauses will evaluate to False for an integer with value 0 which causes the 4-byte hash to be different. So in other words if we iterate on a static UID file (we impemented a validation to iterate static UID files), the min value would not be exported to file causing a different ID in the next iteration of the file.

What I suggest is we change all initializations of variables that are not strings e.g. min and max to use optional None so for python >= 3.10:

min: int | float | None = None
max: int | float | None = None

By doing so we know if the value has been set from a vehicle specification / overlay and if not it will stay None and therefore will not be exported. So on the export we would do sth like if node.min is not None: ...

I'm creating this issue because I'm wondering if any other exporters work the same way which could cause missing min and max values (or other?) on iterated vehicle specification files

erikbosch commented 4 months ago

Thanks for the issue. I think it is a good move to use None as default value.

It is very possible that some of the exporters can be affected, but if so it is likely quite easy fixed. We have some test-cases that use min/max but I do not think they are that thorough.

I suggest - do the change - if there are no regressions reported we assume that there are no generators/tools are affected. If you have a lot of time you can off course add some extra tests on min/max for all generators like we do for some other concepts to be able to catch more regressions but I do not see that as mandatory.

If you get any regression in a tool and it is not obvious for you how to fix it let me know and I can take a look