Skip to content

Expand derived clusters #485

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 7, 2020
Merged

Expand derived clusters #485

merged 1 commit into from
Dec 7, 2020

Conversation

smindinvern
Copy link
Contributor

This PR addresses two issues encountered when attempting to process the svd file here.

  1. The SVD file defines a cluster in the 'R_GPT_ODC' peripheral that derives from a sibling cluster. AFAICT from the SVD schema, this is allowed, and we should expand the cluster definition in the same way registers are handled.
  2. The SVD file contains many EnumeratedValues that are "reserved", but aren't filtered out because they're named "other". They have an <isDefault> child element, which is mutually exclusive with the <value> child element according to the SVD schema, i.e. they don't represent a value that we would want to define in the generated code, and so can be skipped.

When running svd2rust-regress locally 3 tests fail, but the same tests also fail on master, so it seems there's no indication of a regression here.

This seems to fix #462.

@smindinvern smindinvern requested a review from a team as a code owner December 4, 2020 17:11
@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 @ryankurte (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 Dec 4, 2020
@burrbull
Copy link
Member

burrbull commented Dec 4, 2020

LGTM.
Rustfmt, please.

@smindinvern
Copy link
Contributor Author

I noticed that this causes invalid rust code to be generated for fields which only have a single isDefault value. I have a patch for that, but I'm not sure that it's correct. Should I include that here for consideration, or address that separately?

@burrbull
Copy link
Member

burrbull commented Dec 5, 2020

I think it's better to split this PR. And place c9b18c7 in other one.

Emilgardis
Emilgardis requested changes Dec 5, 2020
View reviewed changes
Copy link
Member

@Emilgardis Emilgardis left a comment

Choose a reason for hiding this comment

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

Nice!

Rebase to squash rustfmt and remove c9b18c7

@smindinvern
Expand clusters that specify a valid derivedFrom attribute
c7d6061 
This follows the same process as derived registers, and doesn't cover
the case where the `derivedFrom` cluster is in a different scope.
@burrbull
Copy link
Member

burrbull commented Dec 6, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented Dec 6, 2020

👎 Rejected by code reviews

@burrbull
Copy link
Member

burrbull commented Dec 6, 2020

@Emilgardis

@Emilgardis
Copy link
Member

bors r=burrbull

@bors
Copy link
Contributor

bors bot commented Dec 7, 2020

Build succeeded:

@bors bors bot merged commit 461078b into rust-embedded:master Dec 7, 2020
@smindinvern smindinvern deleted the fix/derived-clusters branch December 7, 2020 13:47
@Emilgardis Emilgardis mentioned this pull request Dec 9, 2020
Merged
@therealprof
Copy link
Contributor

@smindinvern And of course I missed that this is missing a CHANGELOG entry. Would you mind PRing one? 😅

smindinvern added a commit to smindinvern/svd2rust that referenced this pull request Feb 2, 2021
@smindinvern
Update CHANGELOG for rust-embedded#485 and rust-embedded#486
93d118f
@smindinvern smindinvern mentioned this pull request Feb 2, 2021
Merged
bors bot added a commit that referenced this pull request Feb 3, 2021
@bors @smindinvern
Merge #490
6457c30 
490: Update CHANGELOG for #485 and #486 r=burrbull a=smindinvern

As requested in #485 

Co-authored-by: Nickolas Lloyd <smindinvern@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Emilgardis Emilgardis Emilgardis approved these changes

@burrbull burrbull burrbull approved these changes

Assignees

@ryankurte ryankurte

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-tools
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unexpected token error processing SVD file
6 participants
@smindinvern @rust-highfive @burrbull @Emilgardis @therealprof @ryankurte