Skip to content

riscv-peripheral rework #288

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

Merged
merged 8 commits into from
Jun 4, 2025
Merged

riscv-peripheral rework #288

merged 8 commits into from
Jun 4, 2025

Conversation

romancardenas
Copy link
Contributor

Instead of using associated functions, this PR proposes using methods. The benefits of this change are:

  1. It follows the same approach as in svd2rust. Thus, the PACs using this tool will benefit from a more uniform API.
  2. It allows using the Deref trait to simplify the peripheral_codegen! macros.

I also removed the embedded-hal-async partial support. The reason for removing this is that while implementing a few asynchronous functions, I noticed that it is difficult to provide a generic and optimized implementation for any RISC-V target. Instead, I think it is more reasonable to implement a custom chip-hal-async crate that makes the most of the characteristics of a given chip.

This is still a work-in-progress PR. I want to make sure that everything works as expected by updating svd2rust and e310x.

@romancardenas romancardenas requested a review from a team as a code owner May 16, 2025 16:10
Copy link

This PR is being prevented from merging because it presents one of the blocking labels: work in progress, do not merge.

@romancardenas romancardenas force-pushed the riscv-peripheral-rework branch from 3145c81 to 66026e1 Compare May 16, 2025 16:14
Copy link
Contributor

@rmsyn rmsyn left a comment

Choose a reason for hiding this comment

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

So far, these changes look really good. Having these common peripherals match with the svd2rust generated code is a great idea.

Implementing a lot of the functionality as traits is also really nice, since it allows providing platform-specific quirks as extension traits.

Feel free to ping when you want a more in-depth review.

@romancardenas romancardenas force-pushed the riscv-peripheral-rework branch from ef8e38c to 769da06 Compare May 21, 2025 18:01
@romancardenas
Copy link
Contributor Author

@rmsyn I think it is ready to go. You can check how the code generated by svd2rust would look like here: https://github.com/riscv-rust/e310x/tree/rvp-rework

Copy link
Contributor

@rmsyn rmsyn left a comment

Choose a reason for hiding this comment

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

LGTM. The rework is very nice, and the generated code fits well with the rest of the svd2rust code generation in e310x. Having to call the explicit unsafe { Clint::steal() } also makes it clear to users that they should have exclusive access to the peripheral for these calls.

I think the separation of the async stuff into a separate HAL implementation is also a good move. Allows for those who want an async-first implementation to do that, and expose a non-async crate that hides all of the async calls in blocking functions. While letting users who only want a non-async interface to use the generated code, or implement the async interfaces on top of the blocking ones.

@romancardenas romancardenas added this pull request to the merge queue Jun 4, 2025
Merged via the queue into master with commit 09dc978 Jun 4, 2025
139 of 140 checks passed
@romancardenas romancardenas deleted the riscv-peripheral-rework branch June 4, 2025 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants