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

When parsing DBC, make sure there are no unused VAL_ and SIG_VALTYPE_ entries #661

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

SebKuzminsky
Copy link

This makes DBC parsing a little more strict, rejecting DBCs with some common(?) errors.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 8840594508

Details

  • 14 of 20 (70.0%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.06%) to 93.489%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/cantools/database/can/formats/dbc.py 14 20 70.0%
Files with Coverage Reduction New Missed Lines %
src/cantools/database/can/formats/dbc.py 1 96.7%
Totals Coverage Status
Change from base Build 8024886298: -0.06%
Covered Lines: 7266
Relevant Lines: 7772

💛 - Coveralls

@SebKuzminsky
Copy link
Author

@andlaus do you want me to rebase this on top of master to pick up the linter fix that you just merged?

@zariiii9003
Copy link
Collaborator

I'm not sure about this. I do not see the benefit of raising an exception on unused values. If CANdb++ can open these files, then they are valid and we should accept them.

@andlaus
Copy link
Member

andlaus commented May 3, 2024

@andlaus do you want me to rebase this on top of master to pick up the linter fix that you just merged?

that would probably a good idea...

@SebKuzminsky
Copy link
Author

SebKuzminsky commented May 3, 2024

I'm not sure about this. I do not see the benefit of raising an exception on unused values. If CANdb++ can open these files, then they are valid and we should accept them.

CANDB++ fails to open these files (and prints a very unhelpful error message). Current cantools (39.4.5) accepts these bogus/questionable files. This PR brings cantools into closer alignment with the CANDB++ parser.

@SebKuzminsky SebKuzminsky reopened this May 3, 2024
@SebKuzminsky
Copy link
Author

(Oops, didn't mean to close this PR!)

Sebastian Kuzminsky added 2 commits May 3, 2024 08:40
I came across a DBC in the wild which contained a VAL_ line that specified
the wrong frame ID for its signal.  cantools accepted the DBC file,
but failed to interpret the signal correctly.

This commit tries to detect this DBC error by removing VAL_ choices
as they are used by Messages, and raising an exception if any choices
remain after parsing the entire DBC.
I came across a DBC in the wild which contained a SIG_VALTYPE_ line that
specified the wrong frame ID for its signal.  cantools accepted the DBC
file, but failed to interpret the signal correctly.

This commit tries to detect this DBC error by removing SIG_VALTYPE_
choices from the signal_types dict as they are used by Messages, and
raising an exception if any entries remain after parsing the entire DBC.
@SebKuzminsky
Copy link
Author

And this latest force-push should fix the ruff linter error i introduced. 🙄

Sorry for all the churn...

@SebKuzminsky
Copy link
Author

Here are some small DBC files that demonstrate the issue. They both define two packets with one signal each. In bad.dbc the VAL_ and SIG_VALTYPE_ lines have incorrect frame ids (COB IDs?) so they don't get applied to the signals. In good.dbc the frame ids have been fixed so the lines do get applied to the signals.

good.dbc.txt
bad.dbc.txt

The diff from bad to good is this:

$ diff -u bad.dbc good.dbc 
--- bad.dbc     2024-05-03 15:10:47.601981628 -0600
+++ good.dbc    2024-05-03 15:10:53.985999971 -0600
@@ -42,6 +42,6 @@
  SG_ signal2 : 0|32@1- (1,0) [0|0] "" Vector__XXX
 
 
-VAL_ 99 signal1 3 "High" 2 "Standard" 1 "Low" 0 "Unknown" ;
+VAL_ 1 signal1 3 "High" 2 "Standard" 1 "Low" 0 "Unknown" ;
 
-SIG_VALTYPE_ 345 signal2 : 1;
+SIG_VALTYPE_ 2 signal2 : 1;

Here is CANdb++ trying to load bad.dbc:
Screenshot from 2024-05-03 15-11-24
Screenshot from 2024-05-03 15-11-29

Here is cantools accepting bad.dbc and just ignoring the invalid VAL_ and SIG_VALTYPE_ lines, so the signals are interpreted as "default" integers:

$ python3 -m cantools list --all bad.dbc 
msg1:
  Frame ID: 0x1 (1)
  Size: 1 bytes
  Is extended frame: False
  Is CAN-FD frame: False
  Signal tree:

    -- {root}
       +-- signal1

  Signal details:
    signal1:
      Internal type: Integer
      Start bit: 0
      Length: 8 bits
      Byte order: little_endian
      Is signed: False
msg2:
  Frame ID: 0x2 (2)
  Size: 4 bytes
  Is extended frame: False
  Is CAN-FD frame: False
  Signal tree:

    -- {root}
       +-- signal2

  Signal details:
    signal2:
      Internal type: Integer
      Start bit: 0
      Length: 32 bits
      Byte order: little_endian
      Is signed: True

Here is CANdb++ successfully loading good.dbc:
Screenshot from 2024-05-03 15-11-57

And for completeness, here is cantools parsing good.dbc:

$ python3 -m cantools list --all good.dbc 
msg1:
  Frame ID: 0x1 (1)
  Size: 1 bytes
  Is extended frame: False
  Is CAN-FD frame: False
  Signal tree:

    -- {root}
       +-- signal1

  Signal details:
    signal1:
      Internal type: Integer
      Start bit: 0
      Length: 8 bits
      Byte order: little_endian
      Is signed: False
      Named values:
        3: High
        2: Standard
        1: Low
        0: Unknown
msg2:
  Frame ID: 0x2 (2)
  Size: 4 bytes
  Is extended frame: False
  Is CAN-FD frame: False
  Signal tree:

    -- {root}
       +-- signal2

  Signal details:
    signal2:
      Internal type: Float
      Start bit: 0
      Length: 32 bits
      Byte order: little_endian
      Is signed: True

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants