riscv: add fallible functions #222

rmsyn · 2024-06-28T05:20:31Z

Adds fallible variants to all functions that explicitly panic in the riscv crate.

Resolves: #212

rmsyn · 2024-06-28T05:23:33Z

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.

romancardenas

Looks great! Thank you so much. We can keep working in this PR

riscv/src/register/macros.rs

riscv/src/register/satp.rs

romancardenas

It looks good to me. What do you think about more details in invalid values? Is it overcomplicating stuff maybe?

riscv/src/register/macros.rs

riscv/src/register/pmpcfgx.rs

riscv/src/result.rs

rmsyn · 2024-06-30T21:32:01Z

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.

romancardenas

It is looking great! The only comments I have are:

Adding an InvalidVariant error for those cases in which there are reserved values within a bitmask.
Updating "infallible" functions to use their "fallible" counterpart followed by an unwrap(). In this way, new panic messages will show a more comprehensive outcome.

riscv/src/result.rs

riscv/src/register/mcounteren.rs

riscv/src/register/pmpcfgx.rs

riscv/src/result.rs

rmsyn · 2024-07-02T02:33:50Z

Ok, I added the next round of fixup changes.

romancardenas

So far so good!

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. Crates such as riscv or riscv-peripheral would re-export the result module. Moving it to riscv-pac will allow us to bump riscv versions without breaking other crates, as there will not be version issues.

riscv/src/register/mcounteren.rs

riscv/src/register/satp.rs

rmsyn · 2024-07-03T04:48:39Z

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).

romancardenas

LGTM

romancardenas · 2024-07-03T08:41:39Z

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.

rmsyn · 2024-07-03T23:55:08Z

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.

romancardenas

LGTM

romancardenas · 2024-07-04T06:19:58Z

riscv/src/lib.rs

@@ -40,6 +40,7 @@ pub(crate) mod bits;
 pub mod delay;
 pub mod interrupt;
 pub mod register;
+pub use riscv_pac::result;


It could be handy to reexport the riscv-pac crate

Suggested change

pub use riscv_pac::result;

pub use riscv_pac::{self as pac, result};

... or even pub use riscv_pac::*?

Adds the `result` module to contain types for fallible functions. For discussion, see: <rust-embedded#212 (comment)>

Adds fallible `try_` function variants to most `register` module macros.

Adds fallible access functions for `Mcounteren` HPM fields.

Adds fallible access functions for `Scounteren` HPM fields.

Adds fallible access functions for `Mcountinhibit` HPM fields.

Adds fallible access functions for `Satp` ASID and PPN fields.

Adds fallible conversion functions for `Pmpcfgx` permission and range fields.

romancardenas

LGTM

romancardenas · 2024-07-05T09:15:45Z

I think this is done. The main new features are:

New result module in riscv-pac, in which we define the typical error cases for embedded RISC-V.
Now, in 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 Errors), 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.

romancardenas · 2024-07-10T07:57:22Z

@rmsyn could you please update the riscv-pac traits so they use riscv_pac::result::Error?

Uses the new `Result` type for library traits.

romancardenas

LGTM. Now we need to update riscv-peripheral 😁

rmsyn · 2024-07-12T00:41:06Z

LGTM. Now we need to update riscv-peripheral 😁

😅 I looked for current usage of the traits, and must have missed riscv-peripheral. Updating now.

Uses the `riscv_pac::result::*` types for unsafe trait implementations.

romancardenas

Let me know what you think about my comments

romancardenas · 2024-07-12T07:17:04Z

riscv-pac/src/result.rs

+    /// Invalid field value.
+    InvalidValue {
+        field: &'static str,
+        value: usize,
+        bitmask: usize,
+    },
+    /// Invalid value of a register field that does not match any known variants.
+    InvalidVariant { field: &'static str, value: usize },


Maybe we could differentiate errors regarding fields of a register and errors regarding the whole register/value. The idea is avoiding defining the field in the riscv-peripheral examples, as I think it is not necessary.

Suggested change

/// Invalid field value.

InvalidValue {

field: &'static str,

value: usize,

bitmask: usize,

},

/// Invalid value of a register field that does not match any known variants.

InvalidVariant { field: &'static str, value: usize },

/// Invalid value of a register field.

InvalidFieldValue {

field: &'static str,

value: usize,

bitmask: usize,

},

/// Invalid value of a register field that does not match any known variants.

InvalidFieldVariant { field: &'static str, value: usize },

/// Invalid value.

InvalidValue {

value: usize,

bitmask: usize,

},

/// Invalid value that does not match any known variants.

InvalidVariant(usize),

Maybe we could differentiate errors regarding fields of a register and errors regarding the whole register/value.

This makes sense to me. The value covers the entire structure, so this separation could be useful in other cases without subfields.

romancardenas · 2024-07-12T07:20:45Z

riscv-peripheral/examples/e310x.rs

        if number > Self::MAX_HART_ID_NUMBER {
-            Err(number)
+            Err(Error::InvalidVariant {
+                field: "hart_id",


Adding field to these errors feels unnecessary to me.

Changes current `Invalid*` variants to `InvalidField*` variants to indicate their use for subfields in an outer struct. Adds the field-less counterparts for use in a context without subfields.

rmsyn · 2024-07-13T01:52:48Z

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.

romancardenas

Let me know what you think of my review. It feels to me like the errors containing field names are not that necessary.

romancardenas · 2024-07-15T08:17:52Z

riscv/src/register/pmpcfgx.rs

+            _ => Err(Error::InvalidFieldValue {
+                field: "permission",
+                value: val as usize,
+                bitmask: 0b111,


Suggested change

_ => Err(Error::InvalidFieldValue {

field: "permission",

value: val as usize,

bitmask: 0b111,

_ => Err(Error::InvalidValue {

value: val as usize,

bitmask: 0b111,

romancardenas · 2024-07-15T08:18:32Z

riscv/src/register/pmpcfgx.rs

+            _ => Err(Error::InvalidFieldValue {
+                field: "range",
+                value: val as usize,
+                bitmask: 0b11,


Suggested change

_ => Err(Error::InvalidFieldValue {

field: "range",

value: val as usize,

bitmask: 0b11,

_ => Err(Error::InvalidValue {

value: val as usize,

bitmask: 0b11,

rmsyn · 2024-07-16T03:05:49Z

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/panicing?

romancardenas · 2024-07-16T07:09:01Z

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!

rmsyn · 2024-07-16T17:44:20Z

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.

almindor

LGTM

romancardenas · 2024-07-17T06:58:24Z

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

rmsyn requested a review from a team as a code owner June 28, 2024 05:20

romancardenas reviewed Jun 28, 2024

View reviewed changes

riscv/src/register/macros.rs Outdated Show resolved Hide resolved

rmsyn force-pushed the riscv/result branch from b7b9267 to c2e12f7 Compare June 29, 2024 02:36

rmsyn commented Jun 29, 2024

View reviewed changes

riscv/src/register/satp.rs Outdated Show resolved Hide resolved

romancardenas reviewed Jun 29, 2024

View reviewed changes

rmsyn force-pushed the riscv/result branch from c2e12f7 to 1d4fa05 Compare June 30, 2024 21:26

romancardenas reviewed Jul 1, 2024

View reviewed changes

rmsyn force-pushed the riscv/result branch 2 times, most recently from 7347b51 to 768f825 Compare July 2, 2024 02:31

romancardenas reviewed Jul 2, 2024

View reviewed changes

riscv/src/register/mcounteren.rs Outdated Show resolved Hide resolved

riscv/src/register/mcounteren.rs Outdated Show resolved Hide resolved

riscv/src/register/satp.rs Show resolved Hide resolved

rmsyn force-pushed the riscv/result branch from 768f825 to b2f0c34 Compare July 3, 2024 04:53

romancardenas reviewed Jul 3, 2024

View reviewed changes

romancardenas reviewed Jul 4, 2024

View reviewed changes

rmsyn added 7 commits July 4, 2024 18:11

riscv: add result module

014bc42

Adds the `result` module to contain types for fallible functions. For discussion, see: <rust-embedded#212 (comment)>

riscv: add fallible functions to register macros

0d23dc1

Adds fallible `try_` function variants to most `register` module macros.

riscv: add fallible functions to mcounteren

5165db3

Adds fallible access functions for `Mcounteren` HPM fields.

riscv: add fallible functions to scounteren

66fa6a4

Adds fallible access functions for `Scounteren` HPM fields.

riscv: add fallible functions to mcountinhibit

56f30bb

Adds fallible access functions for `Mcountinhibit` HPM fields.

riscv: add fallible functions to satp

54bdc4e

Adds fallible access functions for `Satp` ASID and PPN fields.

riscv: add fallible functions to pmpcfgx

77b692f

Adds fallible conversion functions for `Pmpcfgx` permission and range fields.

rmsyn force-pushed the riscv/result branch from 8a384b4 to 77b692f Compare July 4, 2024 18:12

romancardenas reviewed Jul 5, 2024

View reviewed changes

romancardenas mentioned this pull request Jul 5, 2024

Support "non-standard" interrupts and exceptions #211

Closed

riscv-pac: use Result type for traits

62a417a

Uses the new `Result` type for library traits.

romancardenas reviewed Jul 11, 2024

View reviewed changes

riscv-peripheral: use riscv-pac result types

fea2e0b

Uses the `riscv_pac::result::*` types for unsafe trait implementations.

romancardenas reviewed Jul 12, 2024

View reviewed changes

riscv-pac: add field and field-less error variants

2b35502

Changes current `Invalid*` variants to `InvalidField*` variants to indicate their use for subfields in an outer struct. Adds the field-less counterparts for use in a context without subfields.

romancardenas reviewed Jul 15, 2024

View reviewed changes

romancardenas approved these changes Jul 16, 2024

View reviewed changes

almindor approved these changes Jul 16, 2024

View reviewed changes

romancardenas added this pull request to the merge queue Jul 17, 2024

Merged via the queue into rust-embedded:master with commit 5612138 Jul 17, 2024
95 checks passed

romancardenas mentioned this pull request Oct 9, 2024

cortex-m-pac crate: standard traits and result handling for Cortex-M targets rust-embedded/cortex-m#560

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

riscv: add fallible functions #222

riscv: add fallible functions #222

rmsyn commented Jun 28, 2024

rmsyn commented Jun 28, 2024

romancardenas left a comment

romancardenas left a comment

rmsyn commented Jun 30, 2024

romancardenas left a comment

rmsyn commented Jul 2, 2024

romancardenas left a comment •

edited

Loading

rmsyn commented Jul 3, 2024

romancardenas left a comment

romancardenas commented Jul 3, 2024 •

edited

Loading

rmsyn commented Jul 3, 2024

romancardenas left a comment

romancardenas Jul 4, 2024

romancardenas Jul 4, 2024

romancardenas left a comment

romancardenas commented Jul 5, 2024

romancardenas commented Jul 10, 2024

romancardenas left a comment

rmsyn commented Jul 12, 2024

romancardenas left a comment •

edited

Loading

romancardenas Jul 12, 2024

rmsyn Jul 13, 2024

romancardenas Jul 12, 2024

rmsyn commented Jul 13, 2024

romancardenas left a comment

romancardenas Jul 15, 2024

romancardenas Jul 15, 2024

rmsyn commented Jul 16, 2024

romancardenas commented Jul 16, 2024 •

edited

Loading

rmsyn commented Jul 16, 2024

almindor left a comment

romancardenas commented Jul 17, 2024

	pub use riscv_pac::result;
	pub use riscv_pac::{self as pac, result};

riscv: add fallible functions #222

riscv: add fallible functions #222

Conversation

rmsyn commented Jun 28, 2024

rmsyn commented Jun 28, 2024

romancardenas left a comment

Choose a reason for hiding this comment

romancardenas left a comment

Choose a reason for hiding this comment

rmsyn commented Jun 30, 2024

romancardenas left a comment

Choose a reason for hiding this comment

rmsyn commented Jul 2, 2024

romancardenas left a comment • edited Loading

Choose a reason for hiding this comment

rmsyn commented Jul 3, 2024

romancardenas left a comment

Choose a reason for hiding this comment

romancardenas commented Jul 3, 2024 • edited Loading

rmsyn commented Jul 3, 2024

romancardenas left a comment

Choose a reason for hiding this comment

romancardenas Jul 4, 2024

Choose a reason for hiding this comment

romancardenas Jul 4, 2024

Choose a reason for hiding this comment

romancardenas left a comment

Choose a reason for hiding this comment

romancardenas commented Jul 5, 2024

romancardenas commented Jul 10, 2024

romancardenas left a comment

Choose a reason for hiding this comment

rmsyn commented Jul 12, 2024

romancardenas left a comment • edited Loading

Choose a reason for hiding this comment

romancardenas Jul 12, 2024

Choose a reason for hiding this comment

rmsyn Jul 13, 2024

Choose a reason for hiding this comment

romancardenas Jul 12, 2024

Choose a reason for hiding this comment

rmsyn commented Jul 13, 2024

romancardenas left a comment

Choose a reason for hiding this comment

romancardenas Jul 15, 2024

Choose a reason for hiding this comment

romancardenas Jul 15, 2024

Choose a reason for hiding this comment

rmsyn commented Jul 16, 2024

romancardenas commented Jul 16, 2024 • edited Loading

rmsyn commented Jul 16, 2024

almindor left a comment

Choose a reason for hiding this comment

romancardenas commented Jul 17, 2024

romancardenas left a comment •

edited

Loading

romancardenas commented Jul 3, 2024 •

edited

Loading

romancardenas left a comment •

edited

Loading

romancardenas commented Jul 16, 2024 •

edited

Loading