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

Add support for keyword BO_TX_BU_ #86

Closed
Adhara3 opened this issue Sep 6, 2024 · 2 comments · Fixed by #87
Closed

Add support for keyword BO_TX_BU_ #86

Adhara3 opened this issue Sep 6, 2024 · 2 comments · Fixed by #87
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@Adhara3
Copy link
Collaborator

Adhara3 commented Sep 6, 2024

Currently BO_TX_BU_ is not parsed

@Adhara3 Adhara3 added this to the 1.8 milestone Sep 6, 2024
@Adhara3 Adhara3 added the enhancement New feature or request label Sep 6, 2024
@Uight
Copy link
Contributor

Uight commented Sep 7, 2024

Ill take care of this one.
Allready have the regex for it:
private readonly string m_extraTransmitterRegex = $@"BO_TX_BU_ (?<{MessageId}>\d+)\s*:\s*(?<{TransmitterGroup}>(\s*(?:[a-zA-Z_][\w])\s(?:,)?)+);";
Spec says ";" is not optional which is nice for parsing handling but the Transmitter seperator is not defined. i assume it is ',' here but im not sure as in my production dbc theres only ever one extra transmitter defined and im not to sure about the definition from https://github.com/commaai/opendbc in acura_ilx_2016_nidec.dbc.
Nodes are seperated by " " so why , here?
https://github.com/stefanhoelzl/CANpy/blob/master/docs/DBC_Specification.md says its , too so i will take that.

@Adhara3 one question though how would we like to add the additional transmitters?
Message contains a string Transmitter. i could add the additional ones seperated with , but i would probably change it to a IEnumarable so its similar to the rest of the current public api?

The errors i would handle is: parsing fail, message not found and duplicate transmitter (would still add all non duplicates in that case)

@Adhara3
Copy link
Collaborator Author

Adhara3 commented Sep 7, 2024

Spec says ";" is not optional which is nice for parsing handling

Can confirm that CANdb++ requires final semicolon, otherwise syntax error, parsing failed. I would follow this, which is coherent with specs.

Nodes are seperated by " " so why , here?

Oh well, another proof that dbc and specs are shitty as fu*k. Jokes apart, BU_ is very old and probably at that time they weren't strict and space was fine, going on they realised that having a specific separator was very useful and so they forced it in new keywords' syntax but couldn't break backward compatibility.
CANdb++ requires commas otherwise parsing fails. I would follow this, which is coherent with specs.

one question though how would we like to add the additional transmitters?

In v2.0 Transmitter and receivers is signal are Nodes, no longer strings. If Transmitter cardinality can be > 1, then Transmitters should also be a collection of some kind.
To smooth the transition we could do the following

// Version 1.8, do not brek API
string Transmitter;
string[] AdditionalTransmitters;

// Version 2.0
Node Transmitter;
IReadOnlyCollection<Node> AdditionalTransmitters;

// Version 2.0b, since we are already breaking the API
IReadOnlyCollection<Node> Transmitters; // We lose the info about the main one.... needed?

Probably keeping the distinction is unnecessary but at least for version 1.8 would help with having a backward compatible API

@Adhara3 Adhara3 linked a pull request Sep 7, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants