Merge #370

370: Add a simple way to convert a CommandReturn into a Result r=jrvanwhy a=kupiakos

There are a few changes that this makes:
- `CommandReturn` is `#[must_use]`
- `ErrorCode` includes `BadRVal`
- `CommandReturn` has a `to_result` method

I experimented with a few methods of structuring `to_result` to have the
smallest overhead per monomorphization. What you see here is the best I
could do. I also tried:

- Providing a `try_to_result` and using that (lots of overhead)
- Preserving the error code of the failure variant on `BADRVAL`
- Associated constant `FailureData::BADRVAL`
- Associated fn `FailureData::from_wrong_variant(r1: ErrorCode) -> Self`
- A helper function `fn to_result_helper(self, success_variant: ReturnVariant, error_variant: ReturnVariant) -> (bool, u32, u32, u32)` that would do the branches and replacing `r1` with `BadRVal` and other registers with `0` in the case of the wrong variant. I thought this would reduce things further but it seemed to make it worse in my tests. Then again - I only had one instance of each monomorphization.

Co-authored-by: Alyssa Haroldsen <kupiakos@google.com>

Author: bors[bot]

apis/leds/src/lib.rs

@@ -1,6 +1,6 @@
 #![no_std]
 
-use libtock_platform::Syscalls;
+use libtock_platform::{ErrorCode, Syscalls};
 
 /// The LEDs driver
 ///
@@ -9,30 +9,30 @@ use libtock_platform::Syscalls;
 /// use libtock2::Leds;
 ///
 /// // Turn on led 0
-/// Leds::on(0);
+/// let _ = Leds::on(0);
 /// ```
 pub struct Leds<S: Syscalls>(S);
 
 impl<S: Syscalls> Leds<S> {
     /// Run a check against the leds capsule to ensure it is present.
     ///
-    /// Returns `Some(number_of_leds)` if the driver was present. This does not necessarily mean
+    /// Returns `Ok(number_of_leds)` if the driver was present. This does not necessarily mean
     /// that the driver is working, as it may still fail to allocate grant
     /// memory.
-    pub fn count() -> Option<u32> {
-        S::command(DRIVER_ID, LEDS_COUNT, 0, 0).get_success_u32()
+    pub fn count() -> Result<u32, ErrorCode> {
+        S::command(DRIVER_ID, LEDS_COUNT, 0, 0).to_result()
     }
 
-    pub fn on(led: u32) {
-        S::command(DRIVER_ID, LED_ON, led, 0);
+    pub fn on(led: u32) -> Result<(), ErrorCode> {
+        S::command(DRIVER_ID, LED_ON, led, 0).to_result()
     }
 
-    pub fn off(led: u32) {
-        S::command(DRIVER_ID, LED_OFF, led, 0);
+    pub fn off(led: u32) -> Result<(), ErrorCode> {
+        S::command(DRIVER_ID, LED_OFF, led, 0).to_result()
     }
 
-    pub fn toggle(led: u32) {
-        S::command(DRIVER_ID, LED_TOGGLE, led, 0);
+    pub fn toggle(led: u32) -> Result<(), ErrorCode> {
+        S::command(DRIVER_ID, LED_TOGGLE, led, 0).to_result()
     }
 }
 

apis/leds/src/tests.rs

@@ -1,11 +1,12 @@
+use libtock_platform::ErrorCode;
 use libtock_unittest::fake;
 
 type Leds = super::Leds<fake::Syscalls>;
 
 #[test]
 fn no_driver() {
     let _kernel = fake::Kernel::new();
-    assert_eq!(Leds::count(), None);
+    assert_eq!(Leds::count(), Err(ErrorCode::NoDevice));
 }
 
 #[test]
@@ -14,7 +15,7 @@ fn driver_check() {
     let driver = fake::Leds::<10>::new();
     kernel.add_driver(&driver);
 
-    assert!(Leds::count().is_some());
+    assert_eq!(Leds::count(), Ok(10));
     for led in 0..10 {
         assert_eq!(driver.get_led(led), Some(false));
     }
@@ -34,7 +35,7 @@ fn on() {
     let driver = fake::Leds::<10>::new();
     kernel.add_driver(&driver);
 
-    Leds::on(0);
+    assert_eq!(Leds::on(0), Ok(()));
     assert_eq!(driver.get_led(0), Some(true));
 }
 
@@ -44,7 +45,7 @@ fn off() {
     let driver = fake::Leds::<10>::new();
     kernel.add_driver(&driver);
 
-    Leds::off(0);
+    assert_eq!(Leds::off(0), Ok(()));
     assert_eq!(driver.get_led(0), Some(false));
 }
 
@@ -54,9 +55,9 @@ fn toggle() {
     let driver = fake::Leds::<10>::new();
     kernel.add_driver(&driver);
 
-    Leds::toggle(0);
+    assert_eq!(Leds::toggle(0), Ok(()));
     assert_eq!(driver.get_led(0), Some(true));
-    Leds::toggle(0);
+    assert_eq!(Leds::toggle(0), Ok(()));
     assert_eq!(driver.get_led(0), Some(false));
 }
 
@@ -66,9 +67,9 @@ fn on_off() {
     let driver = fake::Leds::<10>::new();
     kernel.add_driver(&driver);
 
-    Leds::on(0);
+    assert_eq!(Leds::on(0), Ok(()));
     assert_eq!(driver.get_led(0), Some(true));
-    Leds::off(0);
+    assert_eq!(Leds::off(0), Ok(()));
     assert_eq!(driver.get_led(0), Some(false));
 }
 
@@ -78,7 +79,7 @@ fn no_led() {
     let driver = fake::Leds::<10>::new();
     kernel.add_driver(&driver);
 
-    Leds::on(11);
+    assert_eq!(Leds::on(11), Err(ErrorCode::Invalid));
     for led in 0..Leds::count().unwrap_or_default() {
         assert_eq!(driver.get_led(led), Some(false));
     }

libtock2/examples/leds.rs

@@ -10,9 +10,9 @@ set_main! {main}
 stack_size! {0x100}
 
 fn main() {
-    if let Some(leds_count) = Leds::count() {
+    if let Ok(leds_count) = Leds::count() {
         for led_index in 0..leds_count {
-            Leds::on(led_index as u32);
+            let _ = Leds::on(led_index as u32);
         }
     }
 }

platform/src/command_return.rs

@@ -2,8 +2,63 @@ use crate::{return_variant, ErrorCode, ReturnVariant};
 
 use core::mem::transmute;
 
-/// The response type from `command`. Can represent a successful value or a
-/// failure.
+/// The response type from the [`command`](crate::Syscalls::command) syscall.
+/// Can represent a success or a failure with or without associated data.
+///
+/// After a syscall is made, registers `r1`-`r3` contain the output as
+/// described by [TRD 104][trd-104]. Some syscalls only return success/failure,
+/// while others provide associated data. This is done by placing the _return
+/// variant_ in `r0`, which specifies how the output registers should be
+/// interpreted. For syscalls other than `command`, the possible output
+/// variants are fixed; you always know which variants are expected given the
+/// syscall class.
+///
+/// However, the `command` syscall is flexible - there must be one success
+/// variant and one failure variant for a given driver/command ID, but
+/// which variants those are, and what data is expected, cannot be checked
+/// statically. Capsules and userspace APIs must agree on the expected
+/// variants for success and failure.
+///
+/// # Example
+///
+/// This uses the [`to_result`] method to implicitly check variants and convert
+/// to a `Result`.
+///
+/// ```ignore
+/// let res: Result<(u32, u32), ErrorCode> = Syscalls::command(314, 1, 1, 2).to_result();
+/// match res {
+///     Ok((val1, val2)) => {
+///         // Success with associated data in val1, val2.
+///     }
+///     Err(ErrorCode::BadRVal) => {
+///         // Incorrect return variant! We may choose to handle this
+///         // explicitly or propagate upwards without branching.
+///     }
+///     Err(ec) => {
+///         // The driver returned an error (or it doesn't exist).
+///     }
+/// }
+/// ```
+///
+/// This uses the `get_*` methods to check the variants explicitly and extract
+/// the associated data.
+///
+/// ```ignore
+/// let command_return = Syscalls::command(314, 1, 1, 2);
+/// if let Some((val1, val2)) = command_return.get_success_2_u32() {
+///     // If there was a success, there is an associated data (u32, u32).
+/// } else if let Some(error_code) = command_return.get_failure() {
+///     // If there was a failure, there's no associated data and we only
+///     // have an error code.
+/// } else {
+///     // Incorrect return variant! If this occurs, your capsule and userspace
+///     // API do not agree on what the return variants should be.
+///     // An application may want to panic in this case to catch this early.
+/// }
+/// ```
+///
+/// [trd-104]: https://github.com/tock/tock/blob/master/doc/reference/trd104-syscalls.md#32-return-values
+#[must_use = "this `CommandReturn` may represent an error, which should be handled"]
 #[derive(Clone, Copy, Debug)]
 pub struct CommandReturn {
     return_variant: ReturnVariant,
@@ -26,16 +81,6 @@ impl CommandReturn {
             r3,
         }
     }
-    // I generally expect CommandReturn to be used with pattern matching, e.g.:
-    //
-    //     let command_return = Syscalls::command(314, 1, 1, 2);
-    //     if let Some((val1, val2)) = command_return.get_success_2_u32() {
-    //         // ...
-    //     } else if let Some(error_code) = command_return.get_failure() {
-    //         // ...
-    //     } else {
-    //         // Incorrect return variant
-    //     }
 
     /// Returns true if this CommandReturn is of type Failure. Note that this
     /// does not return true for other failure types, such as Failure with u32.
@@ -177,4 +222,152 @@ impl CommandReturn {
     pub fn return_variant(&self) -> ReturnVariant {
         self.return_variant
     }
+
+    /// Interprets this `CommandReturn` as a `Result`, checking the success and
+    /// failure variants, as well as extracting the relevant data.
+    ///
+    /// If neither the success or failure variants match what is required by
+    /// `T` and `E`, this function will return `Err(ErrorCode::BadRVal)`.
+    /// If `E` contains non-`ErrorCode` data in this case, the data will be 0.
+    ///
+    /// It is recommended to use type ascription or `::<>` to make the types
+    /// for `T` and `E` explicit at call-site.
+    pub fn to_result<T, E>(self) -> Result<T, E>
+    where
+        T: SuccessData,
+        E: FailureData,
+    {
+        let (return_variant, r1, mut r2, mut r3) = self.raw_values();
+        if return_variant == T::RETURN_VARIANT {
+            return Ok(T::from_raw_values(r1, r2, r3));
+        }
+        let ec: ErrorCode = if return_variant == E::RETURN_VARIANT {
+            // Safety: E::RETURN_VARIANT must be a failure variant, and
+            // failure variants must contain a valid ErrorCode in r1.
+            unsafe { core::mem::transmute(r1 as u16) }
+        } else {
+            r2 = 0;
+            r3 = 0;
+            ErrorCode::BadRVal
+        };
+        Err(E::from_raw_values(ec, r2, r3))
+    }
+}
+
+mod sealed {
+    pub trait Sealed {}
+}
+
+/// Output from a successful `command` syscall.
+///
+/// This trait is [sealed], meaning foreign implementations cannot be defined,
+/// even though it can be referenced by foreign crates.
+///
+/// [sealed]: https://rust-lang.github.io/api-guidelines/future-proofing.html#sealed-traits-protect-against-downstream-implementations-c-sealed
+pub trait SuccessData: sealed::Sealed {
+    /// The return variant for this success type, stored in `r0` after
+    /// performing a `command` syscall.
+    const RETURN_VARIANT: ReturnVariant;
+
+    /// Constructs the success data given the raw register values.
+    fn from_raw_values(r1: u32, r2: u32, r3: u32) -> Self;
+}
+
+impl sealed::Sealed for () {}
+impl SuccessData for () {
+    const RETURN_VARIANT: ReturnVariant = return_variant::SUCCESS;
+
+    fn from_raw_values(_r1: u32, _r2: u32, _r3: u32) -> Self {}
+}
+impl sealed::Sealed for u32 {}
+impl SuccessData for u32 {
+    const RETURN_VARIANT: ReturnVariant = return_variant::SUCCESS_U32;
+
+    fn from_raw_values(r1: u32, _r2: u32, _r3: u32) -> Self {
+        r1
+    }
+}
+impl sealed::Sealed for u64 {}
+impl SuccessData for u64 {
+    const RETURN_VARIANT: ReturnVariant = return_variant::SUCCESS_U64;
+
+    fn from_raw_values(r1: u32, r2: u32, _r3: u32) -> Self {
+        r1 as u64 | ((r2 as u64) << 32)
+    }
+}
+impl sealed::Sealed for (u32, u32) {}
+impl SuccessData for (u32, u32) {
+    const RETURN_VARIANT: ReturnVariant = return_variant::SUCCESS_2_U32;
+
+    fn from_raw_values(r1: u32, r2: u32, _r3: u32) -> Self {
+        (r1, r2)
+    }
+}
+impl sealed::Sealed for (u32, u64) {}
+impl SuccessData for (u32, u64) {
+    const RETURN_VARIANT: ReturnVariant = return_variant::SUCCESS_U32_U64;
+
+    fn from_raw_values(r1: u32, r2: u32, r3: u32) -> Self {
+        (r1, r2 as u64 | ((r3 as u64) << 32))
+    }
+}
+impl sealed::Sealed for (u32, u32, u32) {}
+impl SuccessData for (u32, u32, u32) {
+    const RETURN_VARIANT: ReturnVariant = return_variant::SUCCESS_3_U32;
+
+    fn from_raw_values(r1: u32, r2: u32, r3: u32) -> Self {
+        (r1, r2, r3)
+    }
+}
+
+/// Output from a failed `command` syscall.
+///
+/// This trait is [sealed], meaning foreign implementations cannot be defined,
+/// even though it can be referenced by foreign crates.
+///
+/// [sealed]: https://rust-lang.github.io/api-guidelines/future-proofing.html#sealed-traits-protect-against-downstream-implementations-c-sealed
+pub unsafe trait FailureData: sealed::Sealed {
+    /// The return variant for this failure type, stored in `r0` after
+    /// performing a `command` syscall.
+    ///
+    /// # Safety
+    /// This must represent a failure variant, such that `r1` will always be
+    /// a valid [`ErrorCode`].
+    const RETURN_VARIANT: ReturnVariant;
+
+    /// Constructs the error data given the raw register values.
+    fn from_raw_values(r1: ErrorCode, r2: u32, r3: u32) -> Self;
+}
+
+impl sealed::Sealed for ErrorCode {}
+unsafe impl FailureData for ErrorCode {
+    const RETURN_VARIANT: ReturnVariant = return_variant::FAILURE;
+
+    fn from_raw_values(r1: ErrorCode, _r2: u32, _r3: u32) -> Self {
+        r1
+    }
+}
+impl sealed::Sealed for (ErrorCode, u32) {}
+unsafe impl FailureData for (ErrorCode, u32) {
+    const RETURN_VARIANT: ReturnVariant = return_variant::FAILURE_U32;
+
+    fn from_raw_values(r1: ErrorCode, r2: u32, _r3: u32) -> Self {
+        (r1, r2)
+    }
+}
+impl sealed::Sealed for (ErrorCode, u32, u32) {}
+unsafe impl FailureData for (ErrorCode, u32, u32) {
+    const RETURN_VARIANT: ReturnVariant = return_variant::FAILURE_2_U32;
+
+    fn from_raw_values(r1: ErrorCode, r2: u32, r3: u32) -> Self {
+        (r1, r2, r3)
+    }
+}
+impl sealed::Sealed for (ErrorCode, u64) {}
+unsafe impl FailureData for (ErrorCode, u64) {
+    const RETURN_VARIANT: ReturnVariant = return_variant::FAILURE_U64;
+
+    fn from_raw_values(r1: ErrorCode, r2: u32, r3: u32) -> Self {
+        (r1, r2 as u64 | ((r3 as u64) << 32))
+    }
 }