Re: [RFC PATCH 1/2] rust: LED abstraction

From: Alice Ryhl
Date: Mon Nov 18 2024 - 05:32:02 EST


On Sat, Nov 16, 2024 at 4:47 PM Marek Behún <kabel@xxxxxxxxxx> wrote:
>
> On Wed, Oct 09, 2024 at 12:57:58PM +0200, Fiona Behrens wrote:
>
> > +/// Color of an LED.
> > +#[derive(Copy, Clone)]
> > +pub enum Color {
> > + /// White
> > + White,
> > + /// Red
> > + Red,
> > + /// Green
> > + Green,
> > + /// Blue
> > + Blue,
> > + /// Amber
> > + Amber,
> > + /// Violet
> > + Violet,
> > + /// Yellow
> > + Yellow,
> > + /// Purple
> > + Purple,
> > + /// Orange
> > + Orange,
> > + /// Pink
> > + Pink,
> > + /// Cyan
> > + Cyan,
> > + /// Lime
> > + Lime,
>
> Why these repetitions?

My guess is that it's to silence the warning about undocumented public
items. It may make sense to just silence the warning in this case.

> > +impl TryFrom<u32> for Color {
> > + type Error = Error;
> > +
> > + fn try_from(value: u32) -> Result<Self, Self::Error> {
> > + Ok(match value {
> > + bindings::LED_COLOR_ID_WHITE => Color::White,
> > + bindings::LED_COLOR_ID_RED => Color::Red,
> > + bindings::LED_COLOR_ID_GREEN => Color::Green,
> > + bindings::LED_COLOR_ID_BLUE => Color::Blue,
> > + bindings::LED_COLOR_ID_AMBER => Color::Amber,
> > + bindings::LED_COLOR_ID_VIOLET => Color::Violet,
> > + bindings::LED_COLOR_ID_YELLOW => Color::Yellow,
> > + bindings::LED_COLOR_ID_PURPLE => Color::Purple,
> > + bindings::LED_COLOR_ID_ORANGE => Color::Orange,
> > + bindings::LED_COLOR_ID_PINK => Color::Pink,
> > + bindings::LED_COLOR_ID_CYAN => Color::Cyan,
> > + bindings::LED_COLOR_ID_LIME => Color::Lime,
> > + bindings::LED_COLOR_ID_IR => Color::IR,
> > + bindings::LED_COLOR_ID_MULTI => Color::Multi,
> > + bindings::LED_COLOR_ID_RGB => Color::RGB,
> > + _ => return Err(EINVAL),
> > + })
> > + }
> > +}
>
> How does Rust compile these? If these constants compile to the same
> numeric values, i.e. if
> LED_COLOR_ID_AMBER == Color::Amber,
> will the compiler compile away the function?

Well, it can't compile away the part where it returns EINVAL when the
u32 is not a valid color. But other than that, these matches are
usually optimized pretty well. I just tried a few different examples
in godbolt to confirm it. See e.g.:
https://rust.godbolt.org/z/WWM7891zW

That said, this relies on the assumption that they are represented
using the same values. We probably want to change the declaration to
this:

#[derive(Copy, Clone)]
pub enum Color {
White = bindings::LED_COLOR_ID_WHITE,
Red = bindings::LED_COLOR_ID_RED,
Green = bindings::LED_COLOR_ID_GREEN,
...
}

That way we are guaranteed that the enum uses the right values for the
enum to make the conversion free.

> How do enums work in Rust?

In this case, the enum has no fields. In that case, the enum is a
value that is only allowed to have certain values.

Enums are also allowed to have fields. In this case, you can think of
it as a discriminated union, though in some cases Rust will store it
in a more clever way. You can look up the "null pointer optimization"
for an example of that.

> > +impl<'a, T> Led<T>
>
> offtopic, what is 'a ? What does the ' mean? Is impl<> something like
> template in c++?

Things starting with a tick are lifetimes, so 'a is the name of a
lifetime. That said, this usage of lifetimes looks incorrect to me, so
I wouldn't look too closely at this instance.

As for impl<>, then yes sort of. It is the <> that makes it like a
template. When you have an `impl TypeName { ... }` block, then that
defines methods for `TypeName`, which you can call as either
`value.method(...)` or `TypeName::method(...)` depending on the
signature. When you write `impl<T>`, then this means that it is a
template (we use the word "generic" in Rust rather than "template"),
that is

impl<T> TypeName<T> { ... }

becomes the following infinite list of impl blocks:

impl TypeName<i32> { ... }
impl TypeName<u32> { ... }
impl TypeName<String> { ... }
impl TypeName<TcpStream> { ... }
// ... and so on for all possible types

This logic works anywhere that <T> appears. For example, `struct
TypeName<T> { ... }` is short-hand for the following infinite list of
structs:

struct TypeName<i32> { ... }
struct TypeName<u32> { ... }
struct TypeName<String> { ... }
struct TypeName<TcpStream> { ... }
// ... and so on for all possible types

Of course, only things in this infinite list that you actually use end
up in the final binary.

The `where T: Operations` part is a filter for the infinite list. It
restricts it so that only types `T` that implement the `Operations`
trait are present in the list; all other types are filtered out.

> > +where
> > + T: Operations + 'a,
>
> What does + mean here?

This is the same as:
where
T: Operations,
T: 'a
that is, apply two filters to the infinite list I mentioned above. The
meaning of `T: 'a` when the RHS is a lifetime is that `T` must not be
a type containing a lifetime annotation shorter than 'a.

> > +/// LED brightness.
> > +#[derive(Debug, Copy, Clone)]
> > +pub enum Brightness {
> > + /// LED off.
> > + Off,
> > + /// Led set to the given value.
> > + On(NonZeroU8),
> > +}
> > +
> > +impl Brightness {
> > + /// Half LED brightness
> > + // SAFETY: constant value non zero
> > + pub const HALF: Self = Self::On(unsafe { NonZeroU8::new_unchecked(127) });
> > + /// Full LED brightness.
> > + pub const FULL: Self = Self::On(NonZeroU8::MAX);
>
> These LED_OFF, LED_ON, LED_HALF and LED_FULL are deprecated constants
> that should not be used anymore. enum led_brightness will be either
> uint8_t or usigned int in the future.
>
> Is it possible to not infect Rust with these deprecated constants?
>
> Marek