Message-ID: <CAH5fLghthWr4r0v=2xNE_UJntG6o6qRzdqHj_nu8AKUwUWh2Aw@mail.gmail.com>
Date: Mon, 18 Nov 2024 11:19:21 +0100
From: Alice Ryhl <aliceryhl@...gle.com>
To: Marek Behún <kabel@...nel.org>
Cc: Fiona Behrens <me@...enk.dev>, Pavel Machek <pavel@....cz>, Lee Jones <lee@...nel.org>, 
	linux-leds@...r.kernel.org, Miguel Ojeda <ojeda@...nel.org>, 
	Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>, 
	Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Trevor Gross <tmgross@...ch.edu>, FUJITA Tomonori <fujita.tomonori@...il.com>, 
	linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [RFC PATCH 1/2] rust: LED abstraction

On Sat, Nov 16, 2024 at 4:47 PM Marek Behún <kabel@...nel.org> 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

Powered by blists - more mailing lists