libgpiod: rust bindings and bindgen issue with C enums

libgpiod: rust bindings and bindgen issue with C enums

[Bartosz Golaszewski]

Hi!

The rust bindings are now in the master branch and I started working
on some additional changes for v2. Among other: using C enum types
explicitly in the core library as suggested by Kent as even though the
C standard says:

---
The expression that defines the value of an enumeration constant shall
be an integer constant expression that has a value representable as an
int.
---

and virtually all compilers store enum variables as signed integers,
modern compilers can spot some errors when using explicit types and it
also benefits IDEs which can interpret them better than ints.

That however led to a problem I'm seeing with rust bindings: bindgen
seems to set the type of an enum constant to ::std::os::raw::c_uint
unless that enum has at least one value explicitly set to a negative
number, in which case the type is ::std::os::raw::c_int.

In order to allow gpiod_line_request_get_value() to still return an
error, I added an additional value to enum gpiod_line_value:
GPIOD_LINE_VALUE_INVALID = -1. Now this one enum in bindgen generated
code has become c_int while all others are still c_uint which - as you
can imagine - leads to all kinds of build-time errors due to strong
typing.

As enums are naturally signed integers in the C world - can we somehow
make bindgen default to c_int for all enum types?

Bartosz

Re: libgpiod: rust bindings and bindgen issue with C enums

[Miguel Ojeda]

On Tue, Nov 22, 2022 at 4:38 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> ---
> The expression that defines the value of an enumeration constant shall
> be an integer constant expression that has a value representable as an
> int.
> ---
>
> and virtually all compilers store enum variables as signed integers,

I don't think this is true. Both GCC and Clang seem to pick an
unsigned one if possible (for the enum, not the constants), e.g.
https://godbolt.org/z/6zjzMdP3T. I assume bindgen is using the one
decided by clang.

Note that the quote of the standard is a constraint, i.e. the values
of the constants need to fit in an `int` (and the compiler is required
to issue a diagnostic if they don't, under `-Wpedantic` in GCC/Clang).

> As enums are naturally signed integers in the C world - can we somehow
> make bindgen default to c_int for all enum types?

This would be https://github.com/rust-lang/rust-bindgen/issues/1966,
where it has been suggested as an option (as well as the fact that the
constants are not being generated as `c_int`).

Cheers,
Miguel

Re: libgpiod: rust bindings and bindgen issue with C enums

[Bartosz Golaszewski]

On Tue, Nov 22, 2022 at 5:55 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Tue, Nov 22, 2022 at 4:38 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > ---
> > The expression that defines the value of an enumeration constant shall
> > be an integer constant expression that has a value representable as an
> > int.
> > ---
> >
> > and virtually all compilers store enum variables as signed integers,
>
> I don't think this is true. Both GCC and Clang seem to pick an
> unsigned one if possible (for the enum, not the constants), e.g.
> https://godbolt.org/z/6zjzMdP3T. I assume bindgen is using the one
> decided by clang.
>
> Note that the quote of the standard is a constraint, i.e. the values
> of the constants need to fit in an `int` (and the compiler is required
> to issue a diagnostic if they don't, under `-Wpedantic` in GCC/Clang).
>
> > As enums are naturally signed integers in the C world - can we somehow
> > make bindgen default to c_int for all enum types?
>
> This would be https://github.com/rust-lang/rust-bindgen/issues/1966,
> where it has been suggested as an option (as well as the fact that the
> constants are not being generated as `c_int`).
>

IIUC this is not done yet.

Viresh' code makes the assumption that all Enums are unsigned
integers. This would be easy to just convert to signed ints but having
both types supported seems impossible. For instance there's the
InvalidEnumValue error that takes an u32. There are more instances
where the enum's type matters.

What should I do in this case to accommodate two types for enums? I'm
bad at rust so I don't even know. In C++ I'd probably use templates if
I had this problem. So generics?

Bart

Re: libgpiod: rust bindings and bindgen issue with C enums

[Bartosz Golaszewski]

On Tue, Nov 22, 2022 at 8:08 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Tue, Nov 22, 2022 at 5:55 PM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
> >
> > On Tue, Nov 22, 2022 at 4:38 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > ---
> > > The expression that defines the value of an enumeration constant shall
> > > be an integer constant expression that has a value representable as an
> > > int.
> > > ---
> > >
> > > and virtually all compilers store enum variables as signed integers,
> >
> > I don't think this is true. Both GCC and Clang seem to pick an
> > unsigned one if possible (for the enum, not the constants), e.g.
> > https://godbolt.org/z/6zjzMdP3T. I assume bindgen is using the one
> > decided by clang.
> >
> > Note that the quote of the standard is a constraint, i.e. the values
> > of the constants need to fit in an `int` (and the compiler is required
> > to issue a diagnostic if they don't, under `-Wpedantic` in GCC/Clang).
> >
> > > As enums are naturally signed integers in the C world - can we somehow
> > > make bindgen default to c_int for all enum types?
> >
> > This would be https://github.com/rust-lang/rust-bindgen/issues/1966,
> > where it has been suggested as an option (as well as the fact that the
> > constants are not being generated as `c_int`).
> >
>
> IIUC this is not done yet.
>
> Viresh' code makes the assumption that all Enums are unsigned
> integers. This would be easy to just convert to signed ints but having
> both types supported seems impossible. For instance there's the
> InvalidEnumValue error that takes an u32. There are more instances
> where the enum's type matters.
>
> What should I do in this case to accommodate two types for enums? I'm
> bad at rust so I don't even know. In C++ I'd probably use templates if
> I had this problem. So generics?
>
> Bart

Viresh,

Could you take a look at https://github.com/brgl/libgpiod-private?
There's a branch called topic/further-libgpiod-v2-updates. Can you
check out commit 5a4e08d546a8ec32757e6c9cc59d7a16939721ea and tell me
how you'd make rust bindings work with it because I'm out of ideas
(and my comfort zone)?

Thanks in advance,
Bartosz

Re: libgpiod: rust bindings and bindgen issue with C enums

[Viresh Kumar]

On 23-11-22, 19:37, Bartosz Golaszewski wrote:
> Could you take a look at https://github.com/brgl/libgpiod-private?
> There's a branch called topic/further-libgpiod-v2-updates. Can you
> check out commit 5a4e08d546a8ec32757e6c9cc59d7a16939721ea and tell me
> how you'd make rust bindings work with it because I'm out of ideas
> (and my comfort zone)?

https://github.com/vireshk/libgpiod brgl/fix

For the benefit of others, I am pasting the entire diff of Rust changes required
to make the C library enums named.

The part that can be improved, but I am not sure how, is the Error enum. Maybe
Miguel or Kent can help ?

The problem is that the InvalidEnumValue Error needs to be generic, which makes
it:

"
pub enum Error<E> {
    ...
    InvalidEnumValue(&'static str, E),
};

pub type Result<T, E> = std::result::Result<T, Error<E>>;
"

Where E can be i32 or u32. Currently I just cast it everywhere as i32 to make
it work.

With above generics change, we need modifications like below everywhere, which I
don't really like.

"
-pub fn gpiochip_devices<P: AsRef<Path>>(path: &P) -> Result<Vec<chip::Chip>> {
+pub fn gpiochip_devices<P: AsRef<Path>, E>(path: &P) -> Result<Vec<chip::Chip>, Error<E>> {
"

What's the best way to make Error accept generics and still have a simpler
Result prototype ?

-- 
viresh

-------------------------8<-------------------------

diff --git a/bindings/rust/gpiosim-sys/src/lib.rs b/bindings/rust/gpiosim-sys/src/lib.rs
index 5391dbd62f44..4b0c8cc6262f 100644
--- a/bindings/rust/gpiosim-sys/src/lib.rs
+++ b/bindings/rust/gpiosim-sys/src/lib.rs
@@ -29,7 +29,7 @@ impl Value {
         match val {
             GPIOSIM_VALUE_INACTIVE => Ok(Value::InActive),
             GPIOSIM_VALUE_ACTIVE => Ok(Value::Active),
-            _ => Err(Error::InvalidEnumValue("Value", val as u32)),
+            _ => Err(Error::InvalidEnumValue("Value", val as i32)),
         }
     }
 }
diff --git a/bindings/rust/libgpiod/src/lib.rs b/bindings/rust/libgpiod/src/lib.rs
index 161de164dddd..0cff45a432ab 100644
--- a/bindings/rust/libgpiod/src/lib.rs
+++ b/bindings/rust/libgpiod/src/lib.rs
@@ -24,6 +24,35 @@ use thiserror::Error as ThisError;

 use libgpiod_sys as gpiod;

+use gpiod::{
+    gpiod_edge_event_type_GPIOD_EDGE_EVENT_FALLING_EDGE as GPIOD_EDGE_EVENT_FALLING_EDGE,
+    gpiod_edge_event_type_GPIOD_EDGE_EVENT_RISING_EDGE as GPIOD_EDGE_EVENT_RISING_EDGE,
+    gpiod_info_event_type_GPIOD_INFO_EVENT_LINE_CONFIG_CHANGED as GPIOD_INFO_EVENT_LINE_CONFIG_CHANGED,
+    gpiod_info_event_type_GPIOD_INFO_EVENT_LINE_RELEASED as GPIOD_INFO_EVENT_LINE_RELEASED,
+    gpiod_info_event_type_GPIOD_INFO_EVENT_LINE_REQUESTED as GPIOD_INFO_EVENT_LINE_REQUESTED,
+    gpiod_line_bias_GPIOD_LINE_BIAS_AS_IS as GPIOD_LINE_BIAS_AS_IS,
+    gpiod_line_bias_GPIOD_LINE_BIAS_DISABLED as GPIOD_LINE_BIAS_DISABLED,
+    gpiod_line_bias_GPIOD_LINE_BIAS_PULL_DOWN as GPIOD_LINE_BIAS_PULL_DOWN,
+    gpiod_line_bias_GPIOD_LINE_BIAS_PULL_UP as GPIOD_LINE_BIAS_PULL_UP,
+    gpiod_line_bias_GPIOD_LINE_BIAS_UNKNOWN as GPIOD_LINE_BIAS_UNKNOWN,
+    gpiod_line_direction_GPIOD_LINE_DIRECTION_AS_IS as GPIOD_LINE_DIRECTION_AS_IS,
+    gpiod_line_direction_GPIOD_LINE_DIRECTION_INPUT as GPIOD_LINE_DIRECTION_INPUT,
+    gpiod_line_direction_GPIOD_LINE_DIRECTION_OUTPUT as GPIOD_LINE_DIRECTION_OUTPUT,
+    gpiod_line_drive_GPIOD_LINE_DRIVE_OPEN_DRAIN as GPIOD_LINE_DRIVE_OPEN_DRAIN,
+    gpiod_line_drive_GPIOD_LINE_DRIVE_OPEN_SOURCE as GPIOD_LINE_DRIVE_OPEN_SOURCE,
+    gpiod_line_drive_GPIOD_LINE_DRIVE_PUSH_PULL as GPIOD_LINE_DRIVE_PUSH_PULL,
+    gpiod_line_edge_GPIOD_LINE_EDGE_BOTH as GPIOD_LINE_EDGE_BOTH,
+    gpiod_line_edge_GPIOD_LINE_EDGE_FALLING as GPIOD_LINE_EDGE_FALLING,
+    gpiod_line_edge_GPIOD_LINE_EDGE_NONE as GPIOD_LINE_EDGE_NONE,
+    gpiod_line_edge_GPIOD_LINE_EDGE_RISING as GPIOD_LINE_EDGE_RISING,
+    gpiod_line_event_clock_GPIOD_LINE_EVENT_CLOCK_HTE as GPIOD_LINE_EVENT_CLOCK_HTE,
+    gpiod_line_event_clock_GPIOD_LINE_EVENT_CLOCK_MONOTONIC as GPIOD_LINE_EVENT_CLOCK_MONOTONIC,
+    gpiod_line_event_clock_GPIOD_LINE_EVENT_CLOCK_REALTIME as GPIOD_LINE_EVENT_CLOCK_REALTIME,
+    gpiod_line_value_GPIOD_LINE_VALUE_ACTIVE as GPIOD_LINE_VALUE_ACTIVE,
+    gpiod_line_value_GPIOD_LINE_VALUE_INACTIVE as GPIOD_LINE_VALUE_INACTIVE,
+    gpiod_line_value_GPIOD_LINE_VALUE_INVALID as GPIOD_LINE_VALUE_INVALID,
+};
+
 /// Operation types, used with OperationFailed() Error.
 #[derive(Copy, Clone, Debug, Eq, PartialEq)]
 pub enum OperationType {
@@ -96,7 +125,7 @@ pub enum Error {
     #[error("Invalid String")]
     InvalidString,
     #[error("Invalid enum {0} value: {1}")]
-    InvalidEnumValue(&'static str, u32),
+    InvalidEnumValue(&'static str, i32),
     #[error("Operation {0} Failed: {1}")]
     OperationFailed(OperationType, errno::Errno),
     #[error("Invalid Arguments")]
@@ -150,18 +179,24 @@ pub mod line {
     pub type ValueMap = IntMap<Value>;

     impl Value {
-        pub fn new(val: i32) -> Result<Self> {
+        pub fn new(val: gpiod::gpiod_line_value) -> Result<Self> {
             Ok(match val {
-                0 => Value::InActive,
-                1 => Value::Active,
-                _ => return Err(Error::InvalidEnumValue("Value", val as u32)),
+                GPIOD_LINE_VALUE_INACTIVE => Value::InActive,
+                GPIOD_LINE_VALUE_ACTIVE => Value::Active,
+                GPIOD_LINE_VALUE_INVALID => {
+                    return Err(Error::OperationFailed(
+                        OperationType::LineRequestGetVal,
+                        errno::errno(),
+                    ))
+                }
+                _ => return Err(Error::InvalidEnumValue("Value", val as i32)),
             })
         }

-        pub(crate) fn value(&self) -> i32 {
+        pub(crate) fn value(&self) -> gpiod::gpiod_line_value {
             match self {
-                Value::Active => 1,
-                Value::InActive => 0,
+                Value::Active => GPIOD_LINE_VALUE_ACTIVE,
+                Value::InActive => GPIOD_LINE_VALUE_INACTIVE,
             }
         }
     }
@@ -181,20 +216,20 @@ pub mod line {
     }

     impl Direction {
-        pub(crate) fn new(dir: u32) -> Result<Self> {
+        pub(crate) fn new(dir: gpiod::gpiod_line_direction) -> Result<Self> {
             Ok(match dir {
-                gpiod::GPIOD_LINE_DIRECTION_AS_IS => Direction::AsIs,
-                gpiod::GPIOD_LINE_DIRECTION_INPUT => Direction::Input,
-                gpiod::GPIOD_LINE_DIRECTION_OUTPUT => Direction::Output,
-                _ => return Err(Error::InvalidEnumValue("Direction", dir)),
+                GPIOD_LINE_DIRECTION_AS_IS => Direction::AsIs,
+                GPIOD_LINE_DIRECTION_INPUT => Direction::Input,
+                GPIOD_LINE_DIRECTION_OUTPUT => Direction::Output,
+                _ => return Err(Error::InvalidEnumValue("Direction", dir as i32)),
             })
         }

-        pub(crate) fn gpiod_direction(&self) -> u32 {
+        pub(crate) fn gpiod_direction(&self) -> gpiod::gpiod_line_direction {
             match self {
-                Direction::AsIs => gpiod::GPIOD_LINE_DIRECTION_AS_IS,
-                Direction::Input => gpiod::GPIOD_LINE_DIRECTION_INPUT,
-                Direction::Output => gpiod::GPIOD_LINE_DIRECTION_OUTPUT,
+                Direction::AsIs => GPIOD_LINE_DIRECTION_AS_IS,
+                Direction::Input => GPIOD_LINE_DIRECTION_INPUT,
+                Direction::Output => GPIOD_LINE_DIRECTION_OUTPUT,
             }
         }
     }
@@ -211,24 +246,24 @@ pub mod line {
     }

     impl Bias {
-        pub(crate) fn new(bias: u32) -> Result<Option<Self>> {
+        pub(crate) fn new(bias: gpiod::gpiod_line_bias) -> Result<Option<Self>> {
             Ok(match bias {
-                gpiod::GPIOD_LINE_BIAS_UNKNOWN => None,
-                gpiod::GPIOD_LINE_BIAS_AS_IS => None,
-                gpiod::GPIOD_LINE_BIAS_DISABLED => Some(Bias::Disabled),
-                gpiod::GPIOD_LINE_BIAS_PULL_UP => Some(Bias::PullUp),
-                gpiod::GPIOD_LINE_BIAS_PULL_DOWN => Some(Bias::PullDown),
-                _ => return Err(Error::InvalidEnumValue("Bias", bias)),
+                GPIOD_LINE_BIAS_UNKNOWN => None,
+                GPIOD_LINE_BIAS_AS_IS => None,
+                GPIOD_LINE_BIAS_DISABLED => Some(Bias::Disabled),
+                GPIOD_LINE_BIAS_PULL_UP => Some(Bias::PullUp),
+                GPIOD_LINE_BIAS_PULL_DOWN => Some(Bias::PullDown),
+                _ => return Err(Error::InvalidEnumValue("Bias", bias as i32)),
             })
         }

-        pub(crate) fn gpiod_bias(bias: Option<Bias>) -> u32 {
+        pub(crate) fn gpiod_bias(bias: Option<Bias>) -> gpiod::gpiod_line_bias {
             match bias {
-                None => gpiod::GPIOD_LINE_BIAS_AS_IS,
+                None => GPIOD_LINE_BIAS_AS_IS,
                 Some(bias) => match bias {
-                    Bias::Disabled => gpiod::GPIOD_LINE_BIAS_DISABLED,
-                    Bias::PullUp => gpiod::GPIOD_LINE_BIAS_PULL_UP,
-                    Bias::PullDown => gpiod::GPIOD_LINE_BIAS_PULL_DOWN,
+                    Bias::Disabled => GPIOD_LINE_BIAS_DISABLED,
+                    Bias::PullUp => GPIOD_LINE_BIAS_PULL_UP,
+                    Bias::PullDown => GPIOD_LINE_BIAS_PULL_DOWN,
                 },
             }
         }
@@ -246,20 +281,20 @@ pub mod line {
     }

     impl Drive {
-        pub(crate) fn new(drive: u32) -> Result<Self> {
+        pub(crate) fn new(drive: gpiod::gpiod_line_drive) -> Result<Self> {
             Ok(match drive {
-                gpiod::GPIOD_LINE_DRIVE_PUSH_PULL => Drive::PushPull,
-                gpiod::GPIOD_LINE_DRIVE_OPEN_DRAIN => Drive::OpenDrain,
-                gpiod::GPIOD_LINE_DRIVE_OPEN_SOURCE => Drive::OpenSource,
-                _ => return Err(Error::InvalidEnumValue("Drive", drive)),
+                GPIOD_LINE_DRIVE_PUSH_PULL => Drive::PushPull,
+                GPIOD_LINE_DRIVE_OPEN_DRAIN => Drive::OpenDrain,
+                GPIOD_LINE_DRIVE_OPEN_SOURCE => Drive::OpenSource,
+                _ => return Err(Error::InvalidEnumValue("Drive", drive as i32)),
             })
         }

-        pub(crate) fn gpiod_drive(&self) -> u32 {
+        pub(crate) fn gpiod_drive(&self) -> gpiod::gpiod_line_drive {
             match self {
-                Drive::PushPull => gpiod::GPIOD_LINE_DRIVE_PUSH_PULL,
-                Drive::OpenDrain => gpiod::GPIOD_LINE_DRIVE_OPEN_DRAIN,
-                Drive::OpenSource => gpiod::GPIOD_LINE_DRIVE_OPEN_SOURCE,
+                Drive::PushPull => GPIOD_LINE_DRIVE_PUSH_PULL,
+                Drive::OpenDrain => GPIOD_LINE_DRIVE_OPEN_DRAIN,
+                Drive::OpenSource => GPIOD_LINE_DRIVE_OPEN_SOURCE,
             }
         }
     }
@@ -276,23 +311,23 @@ pub mod line {
     }

     impl Edge {
-        pub(crate) fn new(edge: u32) -> Result<Option<Self>> {
+        pub(crate) fn new(edge: gpiod::gpiod_line_edge) -> Result<Option<Self>> {
             Ok(match edge {
-                gpiod::GPIOD_LINE_EDGE_NONE => None,
-                gpiod::GPIOD_LINE_EDGE_RISING => Some(Edge::Rising),
-                gpiod::GPIOD_LINE_EDGE_FALLING => Some(Edge::Falling),
-                gpiod::GPIOD_LINE_EDGE_BOTH => Some(Edge::Both),
-                _ => return Err(Error::InvalidEnumValue("Edge", edge)),
+                GPIOD_LINE_EDGE_NONE => None,
+                GPIOD_LINE_EDGE_RISING => Some(Edge::Rising),
+                GPIOD_LINE_EDGE_FALLING => Some(Edge::Falling),
+                GPIOD_LINE_EDGE_BOTH => Some(Edge::Both),
+                _ => return Err(Error::InvalidEnumValue("Edge", edge as i32)),
             })
         }

-        pub(crate) fn gpiod_edge(edge: Option<Edge>) -> u32 {
+        pub(crate) fn gpiod_edge(edge: Option<Edge>) -> gpiod::gpiod_line_edge {
             match edge {
-                None => gpiod::GPIOD_LINE_EDGE_NONE,
+                None => GPIOD_LINE_EDGE_NONE,
                 Some(edge) => match edge {
-                    Edge::Rising => gpiod::GPIOD_LINE_EDGE_RISING,
-                    Edge::Falling => gpiod::GPIOD_LINE_EDGE_FALLING,
-                    Edge::Both => gpiod::GPIOD_LINE_EDGE_BOTH,
+                    Edge::Rising => GPIOD_LINE_EDGE_RISING,
+                    Edge::Falling => GPIOD_LINE_EDGE_FALLING,
+                    Edge::Both => GPIOD_LINE_EDGE_BOTH,
                 },
             }
         }
@@ -358,20 +393,20 @@ pub mod line {
     }

     impl EventClock {
-        pub(crate) fn new(clock: u32) -> Result<Self> {
+        pub(crate) fn new(clock: gpiod::gpiod_line_event_clock) -> Result<Self> {
             Ok(match clock {
-                gpiod::GPIOD_LINE_EVENT_CLOCK_MONOTONIC => EventClock::Monotonic,
-                gpiod::GPIOD_LINE_EVENT_CLOCK_REALTIME => EventClock::Realtime,
-                gpiod::GPIOD_LINE_EVENT_CLOCK_HTE => EventClock::HTE,
-                _ => return Err(Error::InvalidEnumValue("Eventclock", clock)),
+                GPIOD_LINE_EVENT_CLOCK_MONOTONIC => EventClock::Monotonic,
+                GPIOD_LINE_EVENT_CLOCK_REALTIME => EventClock::Realtime,
+                GPIOD_LINE_EVENT_CLOCK_HTE => EventClock::HTE,
+                _ => return Err(Error::InvalidEnumValue("Eventclock", clock as i32)),
             })
         }

-        pub(crate) fn gpiod_clock(&self) -> u32 {
+        pub(crate) fn gpiod_clock(&self) -> gpiod::gpiod_line_event_clock {
             match self {
-                EventClock::Monotonic => gpiod::GPIOD_LINE_EVENT_CLOCK_MONOTONIC,
-                EventClock::Realtime => gpiod::GPIOD_LINE_EVENT_CLOCK_REALTIME,
-                EventClock::HTE => gpiod::GPIOD_LINE_EVENT_CLOCK_HTE,
+                EventClock::Monotonic => GPIOD_LINE_EVENT_CLOCK_MONOTONIC,
+                EventClock::Realtime => GPIOD_LINE_EVENT_CLOCK_REALTIME,
+                EventClock::HTE => GPIOD_LINE_EVENT_CLOCK_HTE,
             }
         }
     }
@@ -388,12 +423,12 @@ pub mod line {
     }

     impl InfoChangeKind {
-        pub(crate) fn new(kind: u32) -> Result<Self> {
+        pub(crate) fn new(kind: gpiod::gpiod_info_event_type) -> Result<Self> {
             Ok(match kind {
-                gpiod::GPIOD_INFO_EVENT_LINE_REQUESTED => InfoChangeKind::LineRequested,
-                gpiod::GPIOD_INFO_EVENT_LINE_RELEASED => InfoChangeKind::LineReleased,
-                gpiod::GPIOD_INFO_EVENT_LINE_CONFIG_CHANGED => InfoChangeKind::LineConfigChanged,
-                _ => return Err(Error::InvalidEnumValue("InfoChangeKind", kind)),
+                GPIOD_INFO_EVENT_LINE_REQUESTED => InfoChangeKind::LineRequested,
+                GPIOD_INFO_EVENT_LINE_RELEASED => InfoChangeKind::LineReleased,
+                GPIOD_INFO_EVENT_LINE_CONFIG_CHANGED => InfoChangeKind::LineConfigChanged,
+                _ => return Err(Error::InvalidEnumValue("InfoChangeKind", kind as i32)),
             })
         }
     }
@@ -408,11 +443,11 @@ pub mod line {
     }

     impl EdgeKind {
-        pub(crate) fn new(kind: u32) -> Result<Self> {
+        pub(crate) fn new(kind: gpiod::gpiod_edge_event_type) -> Result<Self> {
             Ok(match kind {
-                gpiod::GPIOD_EDGE_EVENT_RISING_EDGE => EdgeKind::Rising,
-                gpiod::GPIOD_EDGE_EVENT_FALLING_EDGE => EdgeKind::Falling,
-                _ => return Err(Error::InvalidEnumValue("EdgeEvent", kind)),
+                GPIOD_EDGE_EVENT_RISING_EDGE => EdgeKind::Rising,
+                GPIOD_EDGE_EVENT_FALLING_EDGE => EdgeKind::Falling,
+                _ => return Err(Error::InvalidEnumValue("EdgeEvent", kind as i32)),
             })
         }
     }
diff --git a/bindings/rust/libgpiod/src/line_info.rs b/bindings/rust/libgpiod/src/line_info.rs
index 1784cde27e2f..b45878c523c9 100644
--- a/bindings/rust/libgpiod/src/line_info.rs
+++ b/bindings/rust/libgpiod/src/line_info.rs
@@ -100,7 +100,7 @@ impl Info {
     /// Get the GPIO line's direction.
     pub fn direction(&self) -> Result<Direction> {
         // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
-        Direction::new(unsafe { gpiod::gpiod_line_info_get_direction(self.info) } as u32)
+        Direction::new(unsafe { gpiod::gpiod_line_info_get_direction(self.info) })
     }

     /// Returns true if the line is "active-low", false otherwise.
@@ -112,25 +112,25 @@ impl Info {
     /// Get the GPIO line's bias setting.
     pub fn bias(&self) -> Result<Option<Bias>> {
         // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
-        Bias::new(unsafe { gpiod::gpiod_line_info_get_bias(self.info) } as u32)
+        Bias::new(unsafe { gpiod::gpiod_line_info_get_bias(self.info) })
     }

     /// Get the GPIO line's drive setting.
     pub fn drive(&self) -> Result<Drive> {
         // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
-        Drive::new(unsafe { gpiod::gpiod_line_info_get_drive(self.info) } as u32)
+        Drive::new(unsafe { gpiod::gpiod_line_info_get_drive(self.info) })
     }

     /// Get the current edge detection setting of the line.
     pub fn edge_detection(&self) -> Result<Option<Edge>> {
         // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
-        Edge::new(unsafe { gpiod::gpiod_line_info_get_edge_detection(self.info) } as u32)
+        Edge::new(unsafe { gpiod::gpiod_line_info_get_edge_detection(self.info) })
     }

     /// Get the current event clock setting used for edge event timestamps.
     pub fn event_clock(&self) -> Result<EventClock> {
         // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
-        EventClock::new(unsafe { gpiod::gpiod_line_info_get_event_clock(self.info) } as u32)
+        EventClock::new(unsafe { gpiod::gpiod_line_info_get_event_clock(self.info) })
     }

     /// Returns true if the line is debounced (either by hardware or by the
diff --git a/bindings/rust/libgpiod/src/line_settings.rs b/bindings/rust/libgpiod/src/line_settings.rs
index cedf7cabafcc..1c5ac66f9ce7 100644
--- a/bindings/rust/libgpiod/src/line_settings.rs
+++ b/bindings/rust/libgpiod/src/line_settings.rs
@@ -102,10 +102,7 @@ impl Settings {
     pub fn set_direction(&mut self, direction: Direction) -> Result<&mut Self> {
         // SAFETY: `gpiod_line_settings` is guaranteed to be valid here.
         let ret = unsafe {
-            gpiod::gpiod_line_settings_set_direction(
-                self.settings,
-                direction.gpiod_direction() as i32,
-            )
+            gpiod::gpiod_line_settings_set_direction(self.settings, direction.gpiod_direction())
         };

         if ret == -1 {
@@ -121,17 +118,14 @@ impl Settings {
     /// Get the direction setting.
     pub fn direction(&self) -> Result<Direction> {
         // SAFETY: `gpiod_line_settings` is guaranteed to be valid here.
-        Direction::new(unsafe { gpiod::gpiod_line_settings_get_direction(self.settings) } as u32)
+        Direction::new(unsafe { gpiod::gpiod_line_settings_get_direction(self.settings) })
     }

     /// Set the edge event detection setting.
     pub fn set_edge_detection(&mut self, edge: Option<Edge>) -> Result<&mut Self> {
         // SAFETY: `gpiod_line_settings` is guaranteed to be valid here.
         let ret = unsafe {
-            gpiod::gpiod_line_settings_set_edge_detection(
-                self.settings,
-                Edge::gpiod_edge(edge) as i32,
-            )
+            gpiod::gpiod_line_settings_set_edge_detection(self.settings, Edge::gpiod_edge(edge))
         };

         if ret == -1 {
@@ -147,15 +141,14 @@ impl Settings {
     /// Get the edge event detection setting.
     pub fn edge_detection(&self) -> Result<Option<Edge>> {
         // SAFETY: `gpiod_line_settings` is guaranteed to be valid here.
-        Edge::new(unsafe { gpiod::gpiod_line_settings_get_edge_detection(self.settings) } as u32)
+        Edge::new(unsafe { gpiod::gpiod_line_settings_get_edge_detection(self.settings) })
     }

     /// Set the bias setting.
     pub fn set_bias(&mut self, bias: Option<Bias>) -> Result<&mut Self> {
         // SAFETY: `gpiod_line_settings` is guaranteed to be valid here.
-        let ret = unsafe {
-            gpiod::gpiod_line_settings_set_bias(self.settings, Bias::gpiod_bias(bias) as i32)
-        };
+        let ret =
+            unsafe { gpiod::gpiod_line_settings_set_bias(self.settings, Bias::gpiod_bias(bias)) };

         if ret == -1 {
             Err(Error::OperationFailed(
@@ -170,15 +163,14 @@ impl Settings {
     /// Get the bias setting.
     pub fn bias(&self) -> Result<Option<Bias>> {
         // SAFETY: `gpiod_line_settings` is guaranteed to be valid here.
-        Bias::new(unsafe { gpiod::gpiod_line_settings_get_bias(self.settings) } as u32)
+        Bias::new(unsafe { gpiod::gpiod_line_settings_get_bias(self.settings) })
     }

     /// Set the drive setting.
     pub fn set_drive(&mut self, drive: Drive) -> Result<&mut Self> {
         // SAFETY: `gpiod_line_settings` is guaranteed to be valid here.
-        let ret = unsafe {
-            gpiod::gpiod_line_settings_set_drive(self.settings, drive.gpiod_drive() as i32)
-        };
+        let ret =
+            unsafe { gpiod::gpiod_line_settings_set_drive(self.settings, drive.gpiod_drive()) };

         if ret == -1 {
             Err(Error::OperationFailed(
@@ -193,7 +185,7 @@ impl Settings {
     /// Get the drive setting.
     pub fn drive(&self) -> Result<Drive> {
         // SAFETY: `gpiod_line_settings` is guaranteed to be valid here.
-        Drive::new(unsafe { gpiod::gpiod_line_settings_get_drive(self.settings) } as u32)
+        Drive::new(unsafe { gpiod::gpiod_line_settings_get_drive(self.settings) })
     }

     /// Set active-low setting.
@@ -236,7 +228,7 @@ impl Settings {
     pub fn set_event_clock(&mut self, clock: EventClock) -> Result<&mut Self> {
         // SAFETY: `gpiod_line_settings` is guaranteed to be valid here.
         let ret = unsafe {
-            gpiod::gpiod_line_settings_set_event_clock(self.settings, clock.gpiod_clock() as i32)
+            gpiod::gpiod_line_settings_set_event_clock(self.settings, clock.gpiod_clock())
         };

         if ret == -1 {

Re: libgpiod: rust bindings and bindgen issue with C enums

[Kent Gibson]

On Thu, Nov 24, 2022 at 04:15:01PM +0530, Viresh Kumar wrote:
> On 23-11-22, 19:37, Bartosz Golaszewski wrote:
> > Could you take a look at https://github.com/brgl/libgpiod-private?
> > There's a branch called topic/further-libgpiod-v2-updates. Can you
> > check out commit 5a4e08d546a8ec32757e6c9cc59d7a16939721ea and tell me
> > how you'd make rust bindings work with it because I'm out of ideas
> > (and my comfort zone)?
> 
> https://github.com/vireshk/libgpiod brgl/fix
> 
> For the benefit of others, I am pasting the entire diff of Rust changes required
> to make the C library enums named.
> 
> The part that can be improved, but I am not sure how, is the Error enum. Maybe
> Miguel or Kent can help ?
> 
> The problem is that the InvalidEnumValue Error needs to be generic, which makes
> it:
> 
> "
> pub enum Error<E> {
>     ...
>     InvalidEnumValue(&'static str, E),
> };
> 
> pub type Result<T, E> = std::result::Result<T, Error<E>>;
> "
> 
> Where E can be i32 or u32. Currently I just cast it everywhere as i32 to make
> it work.
> 

I don't see this as a problem for generics.  Whether the enum is signed
or unsigned doesn't need to affect the Error variant, much less the whole
Error type.  The Error doesn't need to respresent the type of the source
of the error, it needs to represent the type required to convey
information to the user.
Just accepting that the InvalidEnumValue variant expects i32, and casting
from u32 if necessary, seems appropriate to me.  Unless there are some
extreme values you are concerned about - but then you always switch it
up to  i64 ;-).

What is the problem that generics solve - that a subsequent bindgen or
gpiod.h change might change signage on you?  If so then cast them all
- even if the cast isn't necessary at present.

Cheers,
Kent.

Re: libgpiod: rust bindings and bindgen issue with C enums

[Bartosz Golaszewski]

On Thu, Nov 24, 2022 at 2:32 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Thu, Nov 24, 2022 at 04:15:01PM +0530, Viresh Kumar wrote:
> > On 23-11-22, 19:37, Bartosz Golaszewski wrote:
> > > Could you take a look at https://github.com/brgl/libgpiod-private?
> > > There's a branch called topic/further-libgpiod-v2-updates. Can you
> > > check out commit 5a4e08d546a8ec32757e6c9cc59d7a16939721ea and tell me
> > > how you'd make rust bindings work with it because I'm out of ideas
> > > (and my comfort zone)?
> >
> > https://github.com/vireshk/libgpiod brgl/fix
> >
> > For the benefit of others, I am pasting the entire diff of Rust changes required
> > to make the C library enums named.
> >
> > The part that can be improved, but I am not sure how, is the Error enum. Maybe
> > Miguel or Kent can help ?
> >
> > The problem is that the InvalidEnumValue Error needs to be generic, which makes
> > it:
> >
> > "
> > pub enum Error<E> {
> >     ...
> >     InvalidEnumValue(&'static str, E),
> > };
> >
> > pub type Result<T, E> = std::result::Result<T, Error<E>>;
> > "
> >
> > Where E can be i32 or u32. Currently I just cast it everywhere as i32 to make
> > it work.
> >
>
> I don't see this as a problem for generics.  Whether the enum is signed
> or unsigned doesn't need to affect the Error variant, much less the whole
> Error type.  The Error doesn't need to respresent the type of the source
> of the error, it needs to represent the type required to convey
> information to the user.
> Just accepting that the InvalidEnumValue variant expects i32, and casting
> from u32 if necessary, seems appropriate to me.  Unless there are some
> extreme values you are concerned about - but then you always switch it
> up to  i64 ;-).
>

In that case: Viresh: can I include your changes in my patch (giving
you credit as a co-author)?

Bart

> What is the problem that generics solve - that a subsequent bindgen or
> gpiod.h change might change signage on you?  If so then cast them all
> - even if the cast isn't necessary at present.
>
> Cheers,
> Kent.

Re: libgpiod: rust bindings and bindgen issue with C enums

[Viresh Kumar]

On 24-11-22, 15:27, Bartosz Golaszewski wrote:
> In that case: Viresh: can I include your changes in my patch (giving
> you credit as a co-author)?

Yes please. Thanks.

-- 
viresh

Re: libgpiod: rust bindings and bindgen issue with C enums

[Miguel Ojeda]

On Thu, Nov 24, 2022 at 2:32 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> I don't see this as a problem for generics.  Whether the enum is signed
> or unsigned doesn't need to affect the Error variant, much less the whole
> Error type.  The Error doesn't need to respresent the type of the source
> of the error, it needs to represent the type required to convey
> information to the user.
> Just accepting that the InvalidEnumValue variant expects i32, and casting
> from u32 if necessary, seems appropriate to me.  Unless there are some
> extreme values you are concerned about - but then you always switch it
> up to  i64 ;-).

Yeah, I am not sure what a generic `Error` buys us here.

If one really wants to preserve whether it is signed or not, that is
only two possibilities, not an open set of them. Moreover, I imagine
one wants to constraint them for users, rather than let users provide
the type `E`.

Thus one could have a sum type with 2 variants like
`InvalidEnumValue(..., Either<i32, u32>)` or something more explicit.

But that is assuming there is a need to preserve it. What is the
variant meant to be used for by users? e.g. if it is just for
reporting, it probably doesn't matter. Actually, does the user even
need the number? Could `InvalidArguments` be enough?

Looking at it a bit more, I am confused why the error is possible to
begin with. There is `Value::new()` which appears to be public and can
return `InvalidEnumValue`, but why should it be used by users? They
should create the `enum` directly, no? And for the other `new()`s that
also return `InvalidEnumValue`s, I see they are `pub(crate)`. That is
what I would expect, but still, why is the error a possibility to
begin with?

For instance, somewhere else the library does `Direction::new(...)`
with a value from the C side. The values from the C side must be
correct, i.e. it is a bug otherwise, right? Thus one can trust them,
or assert them, or if one wants to avoid panics for something that is
a bug, one could return a `InternalLibraryError` with no extra
information, because there is really not much users can do with the
error details apart from knowing there is a bug in either the Rust or
the C side.

I have taken a quick look at the C++ side for that same case, and from
a quick look, C++ appears to throw if the mappings are wrong, so it
sounds to me like you can similarly assert the validity in Rust and
remove the `InvalidEnumValue` variant altogether.

Cheers,
Miguel

Re: libgpiod: rust bindings and bindgen issue with C enums

[Kent Gibson]

On Thu, Nov 24, 2022 at 03:46:27PM +0100, Miguel Ojeda wrote:
> On Thu, Nov 24, 2022 at 2:32 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > I don't see this as a problem for generics.  Whether the enum is signed
> > or unsigned doesn't need to affect the Error variant, much less the whole
> > Error type.  The Error doesn't need to respresent the type of the source
> > of the error, it needs to represent the type required to convey
> > information to the user.
> > Just accepting that the InvalidEnumValue variant expects i32, and casting
> > from u32 if necessary, seems appropriate to me.  Unless there are some
> > extreme values you are concerned about - but then you always switch it
> > up to  i64 ;-).
> 
> Yeah, I am not sure what a generic `Error` buys us here.
> 
> If one really wants to preserve whether it is signed or not, that is
> only two possibilities, not an open set of them. Moreover, I imagine
> one wants to constraint them for users, rather than let users provide
> the type `E`.
> 
> Thus one could have a sum type with 2 variants like
> `InvalidEnumValue(..., Either<i32, u32>)` or something more explicit.
> 
> But that is assuming there is a need to preserve it. What is the
> variant meant to be used for by users? e.g. if it is just for
> reporting, it probably doesn't matter. Actually, does the user even
> need the number? Could `InvalidArguments` be enough?
> 

AIUI, it is just for reporting.  The value itself is helpful to
understand the root cause of the problem.  Not critical, but nice to
have.

> Looking at it a bit more, I am confused why the error is possible to
> begin with. There is `Value::new()` which appears to be public and can
> return `InvalidEnumValue`, but why should it be used by users? They
> should create the `enum` directly, no? And for the other `new()`s that
> also return `InvalidEnumValue`s, I see they are `pub(crate)`. That is
> what I would expect, but still, why is the error a possibility to
> begin with?
> 

The possibility for error can arise from running against a later
libgpiod that has additional values for the enum that these bindings are
obviously unaware of. e.g. the hte event clock recently added.  If you
had bindings built prior to that addition there is no Rust variant
in the event clock enum for that to map to.

Cheers,
Kent.

> For instance, somewhere else the library does `Direction::new(...)`
> with a value from the C side. The values from the C side must be
> correct, i.e. it is a bug otherwise, right? Thus one can trust them,
> or assert them, or if one wants to avoid panics for something that is
> a bug, one could return a `InternalLibraryError` with no extra
> information, because there is really not much users can do with the
> error details apart from knowing there is a bug in either the Rust or
> the C side.
> 
> I have taken a quick look at the C++ side for that same case, and from
> a quick look, C++ appears to throw if the mappings are wrong, so it
> sounds to me like you can similarly assert the validity in Rust and
> remove the `InvalidEnumValue` variant altogether.
> 
> Cheers,
> Miguel

Re: libgpiod: rust bindings and bindgen issue with C enums

[Miguel Ojeda]

On Thu, Nov 24, 2022 at 3:58 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> The possibility for error can arise from running against a later
> libgpiod that has additional values for the enum that these bindings are
> obviously unaware of. e.g. the hte event clock recently added.  If you
> had bindings built prior to that addition there is no Rust variant
> in the event clock enum for that to map to.

If using older bindings against newer libraries is intended to be
supported (and not just the other way around), then I would expect at
least some of the enums to be stable (i.e. exhaustive), no? Say,
`Direction` or `EdgeKind`. Are new variants expected in those (before
a major 2.0)?

And for those that aren't designed to be stable, `InvalidEnumValue` is
a bit misleading. It is just that it is unknown to the bindings (which
is more useful for users: "I should update my bindings"), or
"unexposed" (a term some bindings use when the value is still usable
even if opaque).

Now, the C++ side throws even for the clock mapping (and the exception
says "thrown when the core C library returns an invalid value"), which
to me it sounds like the C++ bindings are not intended to be used if
there is a mismatch. Thus Rust could panic too.

On the other hand, if the bindings are actually intended to be used
even when encountering unknown values, then I would say C++ shouldn't
throw, Rust shouldn't panic, and the enums should be marked as stable
or not as needed. Then, for the unstable ones, depending on whether an
unknown value can still be useful, it could be made an `Unknown`
variant on the particular `enum` (e.g. `EventClock::Unknown`) instead
of an error (the "unexposed" idea above).

Cheers,
Miguel

Re: libgpiod: rust bindings and bindgen issue with C enums

[Kent Gibson]

On Thu, Nov 24, 2022 at 05:18:20PM +0100, Miguel Ojeda wrote:
> On Thu, Nov 24, 2022 at 3:58 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > The possibility for error can arise from running against a later
> > libgpiod that has additional values for the enum that these bindings are
> > obviously unaware of. e.g. the hte event clock recently added.  If you
> > had bindings built prior to that addition there is no Rust variant
> > in the event clock enum for that to map to.
> 
> If using older bindings against newer libraries is intended to be
> supported (and not just the other way around), then I would expect at
> least some of the enums to be stable (i.e. exhaustive), no? Say,
> `Direction` or `EdgeKind`. Are new variants expected in those (before
> a major 2.0)?
> 

Not sure on the policy for Rust bindings, just going by what has
happened in the past for C++ and Python, which are linked dynamically,
and assuming the same for Rust.
Hmmm, the examples in the current Rust bindings are statically linked
to libgpiod, which isn't what I expected, so there you go.
Not sure if that will always be the case.

Anyway, for statically linked Rust bindings, the source of the error can
be a newer kernel (e.g. an extra value in the enum gpio_v2_line_attr_id),
or a bug in libgpiod itself.

The enums drawn from flags are effectively stable even if new flags get
added to the kernel - libgpiod will just ignore them.  Though I'm not
sure there is any advantage in treating them differently, particularly
if we do want to support dynamic linking.

> And for those that aren't designed to be stable, `InvalidEnumValue` is
> a bit misleading. It is just that it is unknown to the bindings (which
> is more useful for users: "I should update my bindings"), or
> "unexposed" (a term some bindings use when the value is still usable
> even if opaque).
> 

The naming is probably a carry over from the C/C++ where the invalid
values can originate from the user, but a valid point wrt Rust.

What is "invalid" depends on perspective.  It is invalid from the user's
PoV, even if it may be valid in an absolute sense.
Is `UnknownEnumValue` a little clearer?  Though that implies you don't
know the actual value passed - and you do.
`UnexpectedEnumValue`?

> Now, the C++ side throws even for the clock mapping (and the exception
> says "thrown when the core C library returns an invalid value"), which
> to me it sounds like the C++ bindings are not intended to be used if
> there is a mismatch. Thus Rust could panic too.
> 

C++ is a slightly different case, as the invalid values can originate from
the user as well.

Disagree on the equivalence between C++ exceptions and Rust panics.
In C++, exceptions are used for general error handling whereas Rust panics,
similar to Go panics, are used for unrecoverable errors.
In Rust, general errors are handled with Results.

I don't see these errors as unrecoverable.  In practice, any lines you
request can only take values that you understand, so you would only see
such errors when reading info about lines you didn't request, and then
the app should just note "well that's odd" and move on.

> On the other hand, if the bindings are actually intended to be used
> even when encountering unknown values, then I would say C++ shouldn't
> throw, Rust shouldn't panic, and the enums should be marked as stable
> or not as needed. Then, for the unstable ones, depending on whether an
> unknown value can still be useful, it could be made an `Unknown`
> variant on the particular `enum` (e.g. `EventClock::Unknown`) instead
> of an error (the "unexposed" idea above).
> 

That approach certainly makes sense if the accessors don't already return
Results - but they do as there are other sources of error.
Given that they do return Results, the decision is whether the invalid
value should be handled in the error path or the normal path.
The error path makes more sense to me as this case is quite unlikely
and forcing the user to always deal with potential Unknown variants in
the normal path is a bit unfriendly.  Much nicer to bunch it in with
the "well that didn't work as expected" path.

So, in short, I agree that the naming could be improved, but I'm otherwise
ok with the approach the bindings are taking here.

Cheers,
Kent.

Re: libgpiod: rust bindings and bindgen issue with C enums

[Viresh Kumar]

On 24-11-22, 21:32, Kent Gibson wrote:
> I don't see this as a problem for generics.  Whether the enum is signed
> or unsigned doesn't need to affect the Error variant, much less the whole
> Error type.  The Error doesn't need to respresent the type of the source
> of the error, it needs to represent the type required to convey
> information to the user.
> Just accepting that the InvalidEnumValue variant expects i32, and casting
> from u32 if necessary, seems appropriate to me.

Right.

> Unless there are some
> extreme values you are concerned about - but then you always switch it
> up to  i64 ;-).

I was just looking to avoid explicit casts, but as you mentioned, it is probably
the right thing to do.

> What is the problem that generics solve - that a subsequent bindgen or
> gpiod.h change might change signage on you?  If so then cast them all
> - even if the cast isn't necessary at present.

That's what I have done here, to avoid future breakage.

Thanks.

-- 
viresh