-
Notifications
You must be signed in to change notification settings - Fork 172
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
Conversation
This PR is being prevented from merging because it presents one of the blocking labels: work in progress, do not merge. |
3145c81
to
66026e1
Compare
There was a problem hiding this 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.
ef8e38c
to
769da06
Compare
@rmsyn I think it is ready to go. You can check how the code generated by |
There was a problem hiding this 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.
Instead of using associated functions, this PR proposes using methods. The benefits of this change are:
svd2rust
. Thus, the PACs using this tool will benefit from a more uniform API.Deref
trait to simplify theperipheral_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 customchip-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
ande310x
.