COVESA / vss-tools

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

Bug in DDSIDL when allowed string values could be a number #276

Closed erikbosch closed 1 year ago

erikbosch commented 1 year ago

vspec2ddsidl.py does not escape/quote enum values - this gives problem if VSS uses string values but the value actually could be a numeric

VSS


PidsA:
  datatype: string[]
  type: attribute
  allowed: ["01","02","03","04","05","06","07","08","09","0A","0B","0C","0D","0E","0F","10","11","12","13","14","15","16","17","18","19","1A","1B","1C","1D","1E","1F","20"]
  description: PID 00 - Array of the supported PIDs 01 to 20 in Hexadecimal.

Result:

module PidsA_M
{
enum PidsAValues{01,02,03,04,05,06,07,08,09,0A,0B,0C,0D,0E,0F,10,11,12,13,14,15,16,17,18,19,1A,1B,1C,1D,1E,1F,20};
};
struct PidsA
{
PidsA_M::PidsAValues value;
//const string type ="attribute";
//const string description="PID 00 - Array of the supported PIDs 01 to 20 in Hexadecimal.";
};

This should better be fixed for VSS 4.0. Could be seen as a regression as the signal is new for VSS 4.0

FYI @AkhilTThomas and others - If anyone have time and interest providing a fix you are welcome. if not I will try to find a fix next week

neil-rti commented 1 year ago

Tested the resulting IDL using RTI rtiddsgen for C++11: works without error when using prepended underscores (_01,_02,_03, etc) as above, but has errors if numerical enum members are either plain (01,02,03, etc) or quoted as strings ("01","02","03", etc).

erikbosch commented 1 year ago

PR merged to VSS 4.0. I will leave this one open until 4.0 branch has been merged to master.

And this fix off course does not prevent refactoring to a totally different representation of "allowed" in DSS IDL, if considered needed.

erikbosch commented 1 year ago

Turns out that underscore is not enough. At least not when generating python. For now - for 4.0 - I assume adding some other character/prefix like d33 or vspec2ddslidl33 would solve the critical problem, to get a *.idl file from VSS 4.0 that is correct and that also can generate correct Python. That the identifier then has different names compared to VSS is something users need to live with, and posssibly we can come up with a better solution for VSS 4.1

After it all it only affects 3 newly added signals, so should not cause any backward compatibility problems

erik@debian3:/tmp/vss-tools$ idlc -l py -x final out.idl
erik@debian3:/tmp/vss-tools$ more Vehicle/OBD/PidsB_M/_out.py 
"""
  Generated by Eclipse Cyclone DDS idlc Python Backend
  Cyclone DDS IDL version: v0.11.0
  Module: Vehicle.OBD.PidsB_M
  IDL file: out.idl

"""

from enum import auto
from typing import TYPE_CHECKING, Optional
from dataclasses import dataclass

import cyclonedds.idl as idl
import cyclonedds.idl.annotations as annotate
import cyclonedds.idl.types as types

# root module import for resolving types
import Vehicle

class PidsBValues(idl.IdlEnum, typename="Vehicle.OBD.PidsB_M.PidsBValues", default="21"):
    21 = auto()
    22 = auto()
    23 = auto()
    24 = auto()
erikbosch commented 1 year ago

Latest solution in #279 , adding d as prefix as not all dds tools can handle underscore correctly.

erikbosch commented 1 year ago

Closing it, but if any want to prepare a better mapping they are welcome to create a PR