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

Inconsistent extended ID for Message and Signal #72

Closed
mparge opened this issue Aug 13, 2024 · 4 comments
Closed

Inconsistent extended ID for Message and Signal #72

mparge opened this issue Aug 13, 2024 · 4 comments
Labels
bug Something isn't working

Comments

@mparge
Copy link

mparge commented Aug 13, 2024

When parsing a dbc file which uses extended IDs, the MSB of the ID is set to 0 (in AdjustExtendedId)

But the IDs of the related signals aren't updated:

image

@Uight
Copy link
Contributor

Uight commented Aug 13, 2024

@mparge you right the signal.ID is set to the message.ID when the signal is added but when the dbc is finally build the message.ID is updated with the extendend logic and due to the ID being uint and therefor a value type the ID in the Signal is not updated.

  1. Quik solution would be to update the signalIDs in AdjustExtendedId aswell (loop over signals).
  2. But the nicer solution would propably be to do the change right when the message is parsed which would then automaticall be used correctly in the signal. It would also mean that the internal dictionary would contain the right value.
    This will fuck up one Test (ExtendedMessageIsAdded) but that test could be moved to the parser tests.
  3. Or make the ID in signal a ref field but that woudl require using c#11 but should work in this case (as the adjust extended is a inplace operation) and would remove the duplication completly.
    (A rename could also be good here as the ID is certainly the message ID an not an ID of the signal in any way)

I think you could implement either of the first two options and create a pull request if you want. The italian guys would be happy i guess ;)

Uight added a commit to Uight/DbcParser that referenced this issue Aug 20, 2024
…eadonly for public access. Use FinishUp method to calc useful properties.
@Adhara3 Adhara3 added the bug Something isn't working label Aug 26, 2024
@Uight
Copy link
Contributor

Uight commented Aug 27, 2024

@Adhara3 which solution woudl you prefer:

  1. Apply extendedID directly when parsing
  2. Update the signal IDs too when applying Extended to the Messages?

I would also recommend to change the name of ID in Signal to MessageID as a Signal has no ID (ID for a signal is the name). I think having an ID in there isnt really clear

@Adhara3
Copy link
Collaborator

Adhara3 commented Aug 27, 2024

I would also recommend to change the name of ID in Signal to MessageID as a Signal has no ID (ID for a signal is the name). I think having an ID in there isnt really clear

What if we remove the property completely? I mean, the dbc is browsable, if you iterate messages you get signals so the ID is there. I mean, this is a duplicated stuff!
I would do one of the following:

  1. remove the property
  2. create the Signal passing its Message in the constructor so that Signal.MessageId => m_parentMessage.Id (no duplication)

I would push for the number 1 honestly.

Cheers
Andrea

Adhara3 pushed a commit that referenced this issue Aug 28, 2024
…rent message itself. This helps avoiding duplication (and the need to update the extended parent ID in signals too)
@Adhara3
Copy link
Collaborator

Adhara3 commented Aug 28, 2024

I opted for solution number 2, for 2 reasons:

  1. I have added the new Parent property so that the signal is linked to the parent message. This allow linking even if flattening the signal list in the Dbc class.
  2. This does not break the API, ID is still available (not even renamed, but marked as obsolete) but is readonly and refers to the new Parent property

In the next major version we will get rid of ID property.

Cheers
A

@Adhara3 Adhara3 closed this as completed Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants