search

EFeru / DbcParser

.NET CAN bus dbc file parser
MIT License
78 stars 28 forks source link

Inconsistent extended ID for Message and Signal #72

Closed mparge closed 3 months ago

mparge commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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