Skip to content

Conversation

aurabindo
Copy link
Contributor

@aurabindo aurabindo commented Aug 9, 2019

SVDs available for DA1469x from https://www.keil.com/dd2/pack have few registers with missing <description> and this causes svd2rust to crash.

This PR enables to continue with a dummy description and print name of the
problematic register instead of just bailing out.

@aurabindo aurabindo requested a review from a team as a code owner August 9, 2019 09:46
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @therealprof (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-tools labels Aug 9, 2019
@therealprof
Copy link
Contributor

I haven't looked in detail but maybe we could add the no description text earlier than when generating the output. Oh, and would be great if the no-description description was a bit shorter; no need to be verbose here...

@Disasm
Copy link
Member

Disasm commented Aug 9, 2019

@therealprof That description field isn't mandatory in SVD spec, so it's ok to get None from svd parser. What do you mean by "earlier" then?

@therealprof
Copy link
Contributor

@Disasm It's a bit unfortunate that we're using the parsed SVD, patch it up and turn it into code all in the same place. Instead it'd be great if we had a pass which used the parsed SVD, checks whether it makes sense, cleans up the odds bits and fills in missing bits so we finally just dump the consistent information into code.

@aurabindo
Copy link
Contributor Author

@therealprof how about an empty String ("") or do you prefer No description ?

Copy link
Contributor

@therealprof therealprof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@therealprof A slight hint would be nice, so "" is not ideal. I'll leave it up to you to figure out what that is, surprise me with a short and descriptive comment. 😉

@aurabindo
Copy link
Contributor Author

aurabindo commented Aug 9, 2019

Well that was my version of Slight hint ;) I'll just put "Dummy description" ? Or maybe "Bad SVD" ?

@therealprof
Copy link
Contributor

Well, descriptions are optional so "Bad SVD" would be technically wrong. It should be something which hints to the "problem" at hand, so "Nothing" or just "No description" would work for me. While you are at it, please add a line to the CHANGELOG and rebase the commit.

@therealprof
Copy link
Contributor

Hm, coming to thing of it an empty String would also be acceptable (if Rust accepts that).

Continue with a dummy description  and print name of the
problematic register instead of just bailing out.

Signed-off-by: Aurabindo Jayamohanan <mail@aurabindo.in>
Copy link
Contributor

@therealprof therealprof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@therealprof
Copy link
Contributor

bors r+

bors bot added a commit that referenced this pull request Aug 9, 2019
369: Fix crash on missing register description in SVD file r=therealprof a=aurabindo

SVDs available for DA1469x from https://www.keil.com/dd2/pack have few registers with missing `<description>` and this causes svd2rust to crash.

This PR enables to continue with a dummy description  and print name of the
problematic register instead of just bailing out.

Co-authored-by: Aurabindo Jayamohanan <mail@aurabindo.in>
@bors
Copy link
Contributor

bors bot commented Aug 9, 2019

Build succeeded

@bors bors bot merged commit aa562e3 into rust-embedded:master Aug 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-tools

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants