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 value lists #23

Merged
merged 6 commits into from
Dec 4, 2018

Conversation

martonmiklos
Copy link
Contributor

Hi Richard,

This PR add support for parsing the VAL_ elements. Vector uses these fields for assigning textual representation to to signals values.

I think it would be useful to generate enums for the C output. To perform that generation we will need to implement sanitization on the strings to make sure that all enum items are C compilant. To make it easier to review I have separated my patch at this point.

@howerj
Copy link
Owner

howerj commented Dec 4, 2018

Looks good, shall I merge it, or are you still making changes?

@howerj
Copy link
Owner

howerj commented Dec 4, 2018

It might be best to signal an error if the strings are not a valid C identifier ('' in the grammar could be used to reject invalid strings). At the very least a warning and that entire enumeration should be skipped if any one enumeration is invalid.

I generally keep my enumerations in the following format (although I have not done it in this project):

typedef enum {
TYPE_VALUE_1_E,
TYPE_VALUE_2_E,
} type_e;

It might be worth generating functions for encoding/decoding and validating that specific enumeration, and then using those functions in the pack/print/unpack/encode/decode routines.

Thanks for the work!
Richard

@martonmiklos
Copy link
Contributor Author

I do not expect big changes regarding to this topic (the enum code generation to the C source will be an another PR). If something left out I can always submit an another PR.

What I have been thinking about now: it would be useful to add a VAL to the end of the example dbc files to be able to have minimal test coverage. I do not have Vector tools at hand at the moment, but I can add this tomorrow.

@howerj
Copy link
Owner

howerj commented Dec 4, 2018

That sounds good, I'll merge this, and you can add examples if/when you have the time.

@howerj howerj merged commit ce968d8 into howerj:master Dec 4, 2018
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

2 participants