Register trait improvements #463
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
S-waiting-on-review
couchand
force-pushed
the
202008/register-traits
branch
from
ed5e817 to
6afe71a
Compare
|
@therealprof tagging you since you commented on #459. @burrbull tagging you because I was inspired by a number of your ideas in https://github.com/rust-embedded/svd2rust/blob/pa-traits/pa/src/lib.rs |
|
Well I'll be. Some architectures have registers named |
|
I see 3 big changes in your PR. Can you split them in different PRs, please? If it is possible. I will try to review tomorrow. |
This was referenced Aug 7, 2020
Merged
Merged
bors bot
added a commit
that referenced
this pull request
Aug 7, 2020
464: Register spec & traits r=therealprof a=couchand Part 1 of #463. - Publishes the register spec zero-sized type and moves all relevant register traits to that struct, which fixes #459 and improves generated documentation. - Removes the extra type parameter on `Reg`, making the register spec the sole authority on the shape of the register (but not its address). One thing that's changed since the previous PR, this renames the spec struct so that it won't cause a conflict on NXP, Freescale, and possibly other architectures. I've gone with `register::REGISTER_SPEC` since that seems reasonably clear and I can't think of how it would cause a conflict (but I'm ready to be proven wrong since I've tried several ideas only to be stymied by various architectures' oddball register naming patterns). I'd prefer the spec to be in the parent module, but I can't come up with a way to do that which guarantees that the generated code will be free from naming conflicts. This seems good enough, plus it maintains the rule that when we make up a name it belongs in the child mod (except for `RegisterBlock`). There is potential for breaking changes here, though the impact seems pretty minimal. Specifically: - If users referred to the traits in `mod generic`, their usage would break. Writing code referring to those traits was previously pretty awkward (see #463), and this PR makes such usage much more ergonomic, so it would seem to be a net benefit. - If users named the full reader/writer incantation the reference would break. It doesn't seem users are likely to name the readers/writers at all, but if they do it seems very likely they would use the type alias. - If users named the previously `#[docs(hidden)] ` spec struct the reference would break. I don't see why they would have done so. - If users patched something in to the generated module with the same name as our new register spec, it would break. That case seems highly unlikely. r? @burrbull Co-authored-by: Andrew Dona-Couch <[email protected]>
bors bot
added a commit
that referenced
this pull request
Aug 7, 2020
466: Avoid register type aliases r=therealprof a=couchand Part 3 of #463, builds on #465. - Removes generated use of the register type aliases in favor of directly referencing `Reg<REGISTER_SPEC>`. I think this helps clarify how the generated and generic types work together. No breaking change concerns on this change set. r? @burrbull Co-authored-by: Andrew Dona-Couch <[email protected]>
bors bot
added a commit
that referenced
this pull request
Aug 8, 2020
465: Accessor newtypes r=therealprof a=couchand Part 2 of #463, builds on #464. - Wraps register reader/writer and field readers in newtype wrappers, which significantly improves the documentation output. Again there's some minor potential for breaking changes: - If users named the complete type of the previous type alias, it would break. It seems more likely that they would use the alias, which will silently upgrade to a reference to the newtype. r? @burrbull Co-authored-by: Andrew Dona-Couch <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Shuffling around type and trait definitions with the goal of improving the generated API & documentation. Some highlights:
Reg<..>. While the type alias is convenient, it hides critical implementation details.My testing so far has been admittedly much too limited given the scope of the changes. However, for the binaries I've checked, the assembly output is identical before and after these changes, suggesting that they are all indeed only type-level cleanup. I've also not run into any examples of user code breakage, despite the public API changes (though I would not be suprised to find some). Spot checking with
scd2rust-regresshas found no issues so far.Some notable design decisions worth discussing:
RegisterBlocks) of the register type alias, as I think it adds a speed bump to understanding how the generated code works. Should the alias continue to be used there instead?Reg::write. Requiring the closure to return the input doesn't seem like the right thing to do. It sows doubt about how exactly the changes propagate, and it makes one-liners slightly shorter at the expense of making multi-line closures an extra line.Some documentation highlights:
Boolean readers now show accessor options directly.
Field readers and writers show
bitsaccessors.Generic
Rtype doesn't show every specific type's impls.