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

Make dbc format more closely match Vector CANdb++ 3.1.18 #493

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Wetmelon
Copy link
Contributor

  • Indicate a value is signed if it's a float
  • Reverse sort signals (SIG_VALTYPE_) by start bit

Newest Vector CANdb++ tool indicates a signal is signed (-) if it's a floating point value and has a different sorting for signals in .dbc files.

- Indicate a value is signed if it's a float
- Reverse sort signals (`SIG_VALTYPE_`) by start bit

Newest Vector CANdb++ tool indicates a signal is signed (`-`) if it's a floating point value and has a different sorting for signals in .dbc files.
@andlaus
Copy link
Member

andlaus commented Oct 15, 2022

besides the the fact that the unit tests need to be fixed and that I'm a bit skittish about changing the order of the signals here (this should possibly be done by specifying the sort_signals parameter of the database's constructor, this LGTM. @juleq calls the shots with the DBC code, though...

@juleq
Copy link
Collaborator

juleq commented Oct 15, 2022

I was thinking that it might not be desirable to let cantools make changes to an input file (of whatever origin) for no other reason other than to make it appear to have been produced by CANdb++.

The issue with that are:

  • User expectation: User loads a valid dbc with strange signal ordering, makes a minor change using cantools and dumps again. He will probably be surprised about the massive diff his minor change has produced.
  • Testing: The easiest testing strategy is to load/dump and check for equality. That does not work if cantools decides to reorder instead of retain.

So I would say: Ordering cantools internal data without API support is straightforward. Providing optional API to do so is good. Changing the default behavior is probably not so good.

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

3 participants