Skip to content

Register access traits #306

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
Closed
wants to merge 11 commits into from
Closed

Register access traits #306

wants to merge 11 commits into from

Conversation

burrbull
Copy link
Member

Move read/write/modify and set/clear bits into traits:

japaric/vcell#8

@burrbull burrbull requested a review from a team as a code owner July 12, 2019 15:06
@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 Jul 12, 2019
@burrbull burrbull mentioned this pull request Jul 12, 2019
Closed
therealprof
therealprof reviewed Jul 12, 2019
View reviewed changes
src/util.rs Outdated
9...16 => Ident::new("u16"),
17...32 => Ident::new("u32"),
33...64 => Ident::new("u64"),
2..=8 => Ident::new("u8"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind moving out all those unrelated cleanups into a separate PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done #307

burrbull
burrbull commented Jul 13, 2019
View reviewed changes
src/generate/register.rs
mod_items.push(quote! {
/// Wiggle in the specified value into the given bits with mask and the offset and return the new value
#[inline]
pub const fn set_bits(bits: #rty, mask: #rty, offset: u8, value: #rty) -> #rty {
Copy link
Member Author

@burrbull burrbull Jul 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@therealprof I moved set_bits here. I think it is the best trade-off: one const set_bits per writable register as they can have different bit-size. Now only traits in vcell

@burrbull burrbull force-pushed the regtraits branch 2 times, most recently from b238406 to edf23b3 Compare July 13, 2019 16:51
@burrbull
Copy link
Member Author

I've done everything I could.

@therealprof
Copy link
Contributor

I think it should be possible to have the traits defined in vcell directly in the generated code if you put them into a module.

@burrbull
Copy link
Member Author

Where is the use of traits if each generated crate have own incompatible traits?
We will need to re-export them several times:

  1. In header of common (family) pac crate
  2. In header of hal trait
  3. In the place where they are used

With vcell (or other common crate) only 3 item is actual. And potentially they can be used for inheritance.

@therealprof
Copy link
Contributor

Where is the use of traits if each generated crate have own incompatible traits?

Each RegisterBlock (and everything underneath) is completely freestanding and incompatible anyway.

With vcell (or other common crate) only 3 item is actual. And potentially they can be used for inheritance.

Sure but first you'll need to get your PR included there and I'm somewhat suspicious this is going to happen. 😏

For development it's a lot easier if everything is self-contained...

I'm not too hot on having traits at all; I only care about real source code and compile time reduction, so macro tricks do not apply.

@burrbull
Copy link
Member Author

Speaking of speed. How you tested it? Clean after each attempt? Can you repeat your tests with this variant?

@burrbull
Copy link
Member Author

burrbull commented Jul 15, 2019 •
edited
Loading

Sure but first you'll need to get your PR included there

It does not have to be exactly vcell. It can be crate in workspace of svd2rust.

@therealprof
Copy link
Contributor

No, I can't test this "as-is" due to the custom vcell repository. That's exactly what I'm talking about, it makes everything so much more complicated... I might be able to patch it but the version resolution plays havoc if you're trying to use github repositories in multiple crates...

@burrbull
Copy link
Member Author

https://doc.rust-lang.org/nightly/edition-guide/rust-2018/cargo-and-crates-io/replacing-dependencies-with-patch.html

@therealprof
Copy link
Contributor

Thanks, I know. You probably missed the important bit here (emphasis mine):

I might be able to patch it but the version resolution plays havoc if you're trying to use github repositories in multiple crates

@burrbull
Copy link
Member Author

Sorry. I'll try do something tomorrow.

@burrbull
Copy link
Member Author

Try to test this branch:
https://github.com/burrbull/svd2rust/tree/internal-traits

@burrbull burrbull closed this Jul 20, 2019
@burrbull burrbull deleted the regtraits branch July 24, 2019 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@therealprof therealprof therealprof left review comments

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.
4 participants
@burrbull @rust-highfive @therealprof @ryankurte