Closed rmsyn closed 1 month ago
Good point! I would like to discuss this with @rust-embedded/cortex-m and get to a similar design to make the developing experience more homogeneous.
I would like to discuss this with @rust-embedded/cortex-m and get to a similar design to make the developing experience more homogeneous.
Should we add this as a talking point for the next rust-embedded
WG meeting?
Sure! I didn't add it to this week's meeting because I'm currently in a conference, but we can give it a try next week.
In the last WG meeting, we discussed some trade-offs of having explicit panic
s in the code.
Some advantages of explicit panics are:
Some advantages of fallible functions:
Also, since we are discussing breaking changes, we should also consider using Rust's version of static_assert
, e.g.:
impl Mcounteren {
// ...
pub fn set_hpm<const N: usize>(&mut self, hpm: bool) {
const { assert!(3 <= N && N < 32) };
self.bits = bf_insert(self.bits, N, 1, hpm as usize);
}
// ...
}
This would then change the usage to:
let mcounteren = Mcounteren::from_bits(0);
mcounteren.set_hpm::<3>(true);
The API is much less flexible.
Given some tinkering, we might come up with ergonomic helper macros to make this usable in dynamic contexts (like loops).
It may not be worth the usability tradeoffs, and/or the engineering effort. :shrug:
Another possibillity is to use a ranged type wrapping the usize
primitive:
https://github.com/rust-embedded/riscv/compare/master...rmsyn:riscv:riscv/ranged-index
This avoids the const_assert
-style static assert, and the run-time assert currently being used.
It uses a "clamping" style of infallible conversion function, and a TryFrom
implementation for a fallible conversion.
use crate::error::Error;
/// Represents an index type restricted by a given valid range.
#[repr(C)]
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
pub struct RangedIndex<const MIN: usize, const MAX: usize>(usize);
impl<const MIN: usize, const MAX: usize> RangedIndex<MIN, MAX> {
/// Creates a new [RangedIndex].
pub const fn new() -> Self {
Self(MIN)
}
/// Infallible function that creates a [RangedIndex] from its inner representation.
///
/// Clamps the provided value to the restricted range.
///
/// If the value falls outside the valid range, it will be "clamped" to its nearest in-range
/// value.
pub const fn from_inner(val: usize) -> Self {
match val {
v if v <= MIN => Self(MIN),
v if v >= MAX => Self(MAX),
v => Self(v),
}
}
/// Converts the [RangedIndex] into its inner representation.
pub const fn into_inner(self) -> usize {
self.0
}
}
impl<const MIN: usize, const MAX: usize> TryFrom<usize> for RangedIndex<MIN, MAX> {
type Error = Error;
fn try_from(val: usize) -> Result<Self, Self::Error> {
match val {
v if (MIN..=MAX).contains(&v) => Ok(Self(v)),
_ => Err(Error::OutOfBounds),
}
}
}
Sorry, I took a few days off and couldn't step through this before.
From my experience, we should avoid using const generics, as as you mentioned it makes it more difficult to use loops and so on. Maybe it is something we can leave for future exploration.
I like the idea of moving from panics to Results, as I think it is the Rustacean way. In general, I would add a riscv::result
module where we can define a common interface for errors. Then, we could progressively update the crate to stop using assertions and using results.
Sorry, I took a few days off and couldn't step through this before.
No worries, thanks for reviewing it :)
In general, I would add a riscv::result module where we can define a common interface for errors.
This would contain the crate's Error
and Result
types, right? Should we re-export the modules types to make them available as top-level imports, or just make the result
module public?
We discussed this issue more in the meeting today, and I think the general consensus was to provide both an infallible function that panic
s on failure, and a fallible try_
version that returns a Result
.
Using that approach, we could also avoid a minor version bump, since we would be adding the fallible function without removing anything from the current API.
I like the try_*
thing, it is very Rustacean (and the Rust for Rustaceans book mentions this as a good practice in APIs). Regarding re-exporting it, I hacen't seen that on crates such as syn or tokio. They usually encapsulate error messages in a public crate::result
module.
From the Rustinomicon section on exception safety:
There are a number of locations in the code that use explicit panics (e.g.
unimplemented!
,assert.*!
macros). Sometime, thesepanic
ing calls are made withinsafe
functions, which users may call inside anunsafe
context.riscv
should convert all explicit and implicitpanic
ing code into fallible code that returns anOption
orResult
.For example,
Mcounteren::hpm
could be changed from:To the fallible:
Any strategy will almost definitely require breaking changes, and a version bump.