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 --mark_range flag #576

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

henkkuli
Copy link

This flag allows implementing a marker trait for registers in specific memory addresses. These can then be used to implement methods on registers. One such example is providing atomic access to only some of the registers directly in the PAC. Currently this is done in HAL, but this loses the ability to use writer proxies. It is also finicky to use raw register addresses. Moreover this allows keeping atomic and non-atomic usage in HAL more similar.

For an example on how to use this, see my fork of rp2040-pac. The most interesting changes regarding the use reside in

Even though I have an example only for the RP2040, I believe that other architectures and devices could also benefit from this. For example on SMT32 MCUs this could be used to implement a nicer API for bit-banding.

Addresses #535

This flag allows implementing a marker trait for registers in specific
memory addresses. These can then be used to implement methods on
registers. One such example is providing atomic access to only some of
the registers directly in the PAC. Currently this is done in HAL, but
this loses the ability to use writer proxies. Moreover this allows
keeping atomic and non-atomic usage in HAL more similar.
@henkkuli henkkuli requested a review from a team as a code owner January 28, 2022 22:15
@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 @adamgreig (or someone else) soon.

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 Jan 28, 2022
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.

really neat!

src/main.rs Outdated
Comment on lines 113 to 116
"Mark registers in given range with a marker trait.
The first argument after the flag sets the name for the marker trait.
The second and third argument are the start and end addresses of registers
which this trait is applied for.",
Copy link
Member

Choose a reason for hiding this comment

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

I think this deserves an example of how it could look.

@adamgreig
Copy link
Member

I think this is a really nice way for specific PACs to implement chip-specific functionality like you've done, nice work!

Is there any reason in your update.sh you have each peripheral's address range spelled out, instead of just putting 0x40000000-0x40070000, 0x50200000-0x50204000, 0x50300000-0x50304000, 0x50400000-0x50404000? It seems like all the peripheral spaces are contiguous?

@henkkuli
Copy link
Author

Is there any reason in your update.sh you have each peripheral's address range spelled out, instead of just putting 0x40000000-0x40070000, 0x50200000-0x50204000, 0x50300000-0x50304000, 0x50400000-0x50404000?

Not really, other than to show that you can specify the same marker multiple times, and to list all peripherals that allow atomic access. I still need to go through that list once more; it seems that some USB peripheral registers allow atomic access, while now none implement it. It could still be simplified.

@adamgreig
Copy link
Member

Could you add an entry to the changelog for this, please?

Also, I wonder if it's a bit unnatural to have command line arguments like --flag a b c: it looks like only a is part of --flag and b and c are extra positional arguments... maybe they should be comma-separated? Maybe it's not a big deal, I just don't feel like I've seen that sort of command line arguments much before.

@burrbull
Copy link
Member

Even with new comments it is not obvious how it works. Please add more complete example to CI tests.

@henkkuli
Copy link
Author

Could you add an entry to the changelog for this, please?

Sure, I'll add an entry with next batch of changes.

Also, I wonder if it's a bit unnatural to have command line arguments like --flag a b c

I originally did that because I didn't want to write a parser for separating the marker name from the address, especially because I didn't know would this even be something that is wanted for svd2rust. Now that it seems that there is interest, I will write a proper parser. And now that I think of it, it shouldn't be anything more complicated than just a couple of splits.

I was thinking that maybe the following would be natural: --mark_range MarkerName:0x1234-0x5678

Even with new comments it is not obvious how it works. Please add more complete example to CI tests.

I can add a CI test, but I'm not exactly sure how. Should I make it a new option that is passed to rust2svd in ci/script.sh:13? And make it part of OPTIONS=all?

@burrbull
Copy link
Member

I can add a CI test, but I'm not exactly sure how. Should I make it a new option that is passed to rust2svd in ci/script.sh:13? And make it part of OPTIONS=all?

Not necessary. Any way you feel comfortable. You can just add separate job. We'll do refactoring some other day.

@adamgreig
Copy link
Member

I originally did that because I didn't want to write a parser for separating the marker name from the address, especially because I didn't know would this even be something that is wanted for svd2rust. Now that it seems that there is interest, I will write a proper parser. And now that I think of it, it shouldn't be anything more complicated than just a couple of splits.

I was thinking that maybe the following would be natural: --mark_range MarkerName:0x1234-0x5678

That suggested format looks good to me, thanks!

@Dirbaio
Copy link
Member

Dirbaio commented Aug 7, 2022

What does it do if there's several instances of the peripheral at different addrs, and only one is covered by the range in --mark_range?

@burrbull
Copy link
Member

burrbull commented Jan 4, 2023

@henkkuli Could you rebase this on current master branch?

@burrbull
Copy link
Member

burrbull commented Jan 4, 2023

I was thinking that maybe the following would be natural: --mark_range MarkerName:0x1234-0x5678

Maybe something like: --registers 0x1234-0x5678,0x11234-0x15678:Marker1,Marker2,Marker3 ?

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.

6 participants