Closed rmsyn closed 1 month ago
We probably also want to cover the riscv::register::satp
and riscv::register::pmpcfgx
modules, too.
Wanted to submit the initial work to get feedback on the approach.
I've added suggested fixes as fixup
commits for easier review.
If you're happy with them, I'll squash them into their respective parent commits before a merge.
I've also left conversations open to continue discussion on some of the style-related suggestions.
Ok, I added the next round of fixup
changes.
I was thinking about the correct location of the result module and maybe we should consider moving it to riscv-pac. Ideally, riscv-pac will have way less activity and act as a dependency for all the riscv ecosystem.
I'm not sure about the "correct" location for the result
module. Putting it in riscv-pac
would require adding riscv-pac
as a riscv
dependency, where keeping it in riscv
requires the opposite.
I think the latter makes more sense, and follows with riscv-rt
having a riscv
dependency.
Moving it to riscv-pac will allow us to bump riscv versions without breaking other crates, as there will not be version issues.
We could still bump riscv
versions, we would just need to do semver
bumps for any rust-embedded/riscv
workspace crates that have a riscv
dependency.
Maybe I'm misunderstanding the architecture, but I thought riscv
was the "core" crate that all the riscv-*
crates would bring in as a dependency (if they need it).
You can get more information about the motivation of the riscv-pac
crate here.
You can think of riscv-pac
as a "special" module of riscv
that is completely isolated from the rest of the crate. In future implementations, I foresee riscv
doing a public reexport of riscv-pac
(in fact, it is already a dependency in this PR). riscv-pac
will experience little to no breaking changes, while we can keep improving riscv
without too big impacts in the ecosystem.
Let us assume that we have the ExternalInterruptNumber
trait in riscv
instead of riscv-pac
. The riscv_peripheral::PLIC
peripheral implementation relies on this trait to provide safe access to different registers. If we add breaking changes to riscv
, we then should also release a new version of riscv-peripheral
, even if no changes were made in the ExternalInterruptNumber
trait. This scales to PACs, third-party peripheral drivers, and so on.
If we place the result
module in riscv-pac
, it will be available in riscv
with the reexport. Additionally, we can improve the current riscv-pac
traits by using your new Result
enum.
If we place the result module in riscv-pac, it will be available in riscv with the reexport. Additionally, we can improve the current riscv-pac traits by using your new Result enum.
I think this makes sense. If you are planning to have riscv-pac
be a dependency in riscv
anyway, then moving result
into riscv-pac
doesn't cause any issues.
In fact, if that's the plan, then moving result
into riscv-pac
is a requirement to avoid a circular dependency.
A similar approach would still need to be taken for any additions to the Error
enum, where breaking changes would require a semver
bump in riscv-pac
, and everything that re-exports riscv_pac::result
.
I'll move it in a fixup
commit, and if you like it, squash the commit before merge.
I think this is done. The main new features are:
result
module in riscv-pac
, in which we define the typical error cases for embedded RISC-V.riscv
, we do pub use riscv_pac::*
. Thus, riscv-pac
could be considered just an "extension" of riscv
that isolates traits and error enums from other breaking changes. Also, in this way, PACs would not need to add an extra dependency to riscv-pac
.While there are other aspects to polish (e.g., modifying riscv-pac
traits so they use the new Error
s), I would like to wait for the next WG meeting (we are already on the agenda). The idea is to get some consensus so the cortex-m
ecosystem could replicate this work. Also, there are very knowledgeable folks who have been working on embedded-hal
and all sorts of interfaces that could help us improve the user experience and avoid potential issues in the future.
@rmsyn could you please update the riscv-pac
traits so they use riscv_pac::result::Error
?
LGTM. Now we need to update
riscv-peripheral
😁
:sweat_smile: I looked for current usage of the traits, and must have missed riscv-peripheral
. Updating now.
Let me know what you think about my comments
Added the recommended changes. Hopefully they cover the contexts you were concerned with.
Agree that for contexts where the value covers the entire structure, we can get the necessary context from the conversion function, e.g. in a backtrace / panic dump.
Let me know what you think of my review. It feels to me like the errors containing field names are not that necessary.
I can appreciate that reviews are incremental, but this could have been made clearer in the last set of changes.
The field
entry is useful when viewing the error messages, e.g. if users have some form of logging enabled.
It also clarifies situations where multiple fields could return/bubble-up an error. For instance, in a function parsing the permission
AND range
field, how would you differentiate which field errors without attaching a debugger/panic
ing?
Hey @rmsyn sorry if it annoyed you. After the last changes, I thought that backtracking would be enough to find out which field changed, with no need to add static strings to the binary. In any case, if you think that variants that indicate the invalid field are necessary/valuable, we can definitely leave them as is, I have no strong feelings against them.
I will wait a couple of days in case anyone from @rust-embedded/riscv wants to leave their review and merge it to master. Again, thank you so much for your contribution!
After the last changes, I thought that backtracking would be enough to find out which field changed, with no need to add static strings to the binary. In any case, if you think that variants that indicate the invalid field are necessary/valuable, we can definitely leave them as is, I have no strong feelings against them.
I think the field
is helpful, but not exactly necessary. I can see the argument that including static strings in riscv
could lead to an increase in static memory. An alternative that would take up less space would be an enum of all CSRs with fields. However, that seemed even more burdensome for maintenance, and downstream users.
If we did use an enum, or follow svd
to use a module-specific error type, maybe we can use minimal formatting in riscv
. Then users can decide how verbose they would like to get with error messages. The module-specific approach would require a reworking of the entire result
module.
We could also just drop the field
like you suggested, and have less directly helpful error messages. This may not be that large of a trade-off. My main concern is when working on a platform with only printf
-style debugging available, e.g. I'm currently working on a platform without a working JTAG interface.
Let's leave the field
thing for now and open a new issue to discuss this in more depth. I'll add this PR to the merge queue tonight, so we can also advance in #211
Adds fallible variants to all functions that explicitly panic in the
riscv
crate.Resolves: https://github.com/rust-embedded/riscv/issues/212