libgpiod: Add Rust bindings

Add rust wrappers around FFI bindings to provide a convenient interface for the users.

Signed-off-by Viresh Kumar <[email protected]>

---

bindings/rust/src/edge_event.rs new file +36 -0

+
+impl GpiodEdgeEvent {
+    /// Get an event stored in the buffer.
+    pub fn new(buffer: &GpiodEdgeEventBuffer, index: u64) -> Result<Self> {
+        let event = unsafe { bindings::gpiod_edge_event_buffer_get_event(buffer.buffer(), index) };
+        if event.is_null() {
+            return Err(Error::OperationFailed(
+                "GpiodEdgeEvent buffer-get-event",
+                IoError::last(),
+            ));
+        }
+
+        Ok(Self { event })
+    }
+
+    /// Get the event type.
+    pub fn get_event_type(&self) -> Result<EdgeEvent> {
+        EdgeEvent::new(unsafe { bindings::gpiod_edge_event_get_event_type(self.event) } as u32)
+    }
+
+    /// Get the timestamp of the event.
+    pub fn get_timestamp(&self) -> Duration {
+        Duration::from_nanos(unsafe { bindings::gpiod_edge_event_get_timestamp(self.event) })
+    }
+
+    /// Get the offset of the line on which the event was triggered.
+    pub fn get_line_offset(&self) -> u32 {
+        unsafe { bindings::gpiod_edge_event_get_line_offset(self.event) }
+    }
+
+    /// Get the global sequence number of this event.
+    ///
+    /// Returns sequence number of the event relative to all lines in the
+    /// associated line request.
+    pub fn get_global_seqno(&self) -> u64 {
+        unsafe { bindings::gpiod_edge_event_get_global_seqno(self.event) }

Quoted 6 lines · Viresh Kumar

Add rust wrappers around FFI bindings to provide a convenient interface for the users.

Signed-off-by Viresh Kumar <[email protected]>

---

Hi Viresh, thanks for the hard work.

Obviously this is not something we can merge yet but it's a good base to continue the work.

General comment on the naming convention:

The line from one of the examples: 'use libgpiod::chip::GpiodChip;' looks like it has a lot of redundancy in it. How about calling the crate gpiod and dropping the chip package entirely? Basically follow what C++ and python bindings do by having use gpiod::Chip etc.?

[snip]

bindings/rust/src/chip.rs line 35

Quoted 7 lines

bindings/rust/src/chip.rs new file +7 -0

+
+impl GpiodChipInternal {
+    /// Find a GPIO chip by path.
+    pub(crate) fn open(path: &str) -> Result<Self> {
+        let chip = unsafe { bindings::gpiod_chip_open(path.as_ptr() as *const c_char) };
+        if chip.is_null() {
+            return Err(Error::OperationFailed("GpiodChip open", IoError::last()));

One of my concerns last time was error handling. Does this now translate the error codes from the underlying C code to some kind of rust errors? Can you elaborate on how that works? I see it always returns IoError. In my WIP C++ and Python code I try to translate the errnos into some meaningful exceptions - for instance EINVAL -> ::std::invalid_argument etc. Can we have a similar thing here?

[snip]

bindings/rust/src/chip.rs line 71

Quoted 7 lines

bindings/rust/src/chip.rs new file +7 -0

+
+    /// Get the GPIO chip name as represented in the kernel.
+    pub fn get_name(&self) -> Result<&str> {
+        // SAFETY: The string returned by libgpiod is guaranteed to live as long
+        // as the `struct GpiodChip`.
+        let name = unsafe { bindings::gpiod_chip_get_name(self.ichip.chip()) };
+        if name.is_null() {

This is not possible, the string can be empty at the very least but never null. Same for label and path.

I'm of the opinion that a rust implementation would be better targetting the ioctls directly rather than libgpiod, as my Go library does and for essentially the same reasons.

To test that theory, and as an exercise to learn rust, I've been writing one, and so far I've been calling it gpiod :-|.

Since this crate targets libgpiod it would make sense for it to remain named libgpiod.

Quoted 3 lines · Kent Gibson

I'm of the opinion that a rust implementation would be better targetting the ioctls directly rather than libgpiod, as my Go library does and for essentially the same reasons.

I remember asking Linus initially about this and I was suggested to use libgpiod instead :)

Quoted 3 lines

To test that theory, and as an exercise to learn rust, I've been writing one, and so far I've been calling it gpiod :-|.

In case you are interested, the first pass of my take on a Rust GPIO library[1] has finally gotten to the point of being core feature complete, and may be usable, although it is sorely lacking any integration tests. And the documentation is still pretty light, or even non-existant for github.
And the event interface is either polled or blocking - haven't gotten to an async interface yet.

Anyway, there are three crates there:
- gpiod-uapi provides a thin and safe wrapper around the ioctls and file reads, for both uAPI versions.
- gpiod provides a more idiomatic abstraction, and hides the uAPI version being used - unless you need v2 specific features.
- gpiodctl provides a binary that bundles all the gpio tools into one.

There are a couple of minimal example apps in the gpiod crate, in addition to the example that gpiodctl provides.

Cheers,
Kent.

[1] https://github.com/warthog618/gpiod-rs

On Thu, Dec 2, 2021 at 12:23 PM Viresh Kumar <[email protected]> wrote:
General comment on the naming convention:

The line from one of the examples: 'use libgpiod::chip::GpiodChip;' looks like it has a lot of redundancy in it. How about calling the crate gpiod and dropping the chip package entirely? Basically follow what C++ and python bindings do by having use gpiod::Chip etc.?

bindings/rust/src/chip.rs line 35

Quoted 7 lines

bindings/rust/src/chip.rs new file +7 -0

+
+impl GpiodChipInternal {
+    /// Find a GPIO chip by path.
+    pub(crate) fn open(path: &str) -> Result<Self> {
+        let chip = unsafe { bindings::gpiod_chip_open(path.as_ptr() as *const c_char) };
+        if chip.is_null() {
+            return Err(Error::OperationFailed("GpiodChip open", IoError::last()));

One of my concerns last time was error handling. Does this now translate the error codes from the underlying C code to some kind of rust errors? Can you elaborate on how that works?

I see it always returns IoError.

In my WIP C++ and Python code I try to translate the errnos into some meaningful exceptions - for instance EINVAL -> ::std::invalid_argument etc. Can we have a similar thing here?

bindings/rust/src/chip.rs line 71

Quoted 6 lines

bindings/rust/src/chip.rs new file +6 -0

+    /// Get the GPIO chip name as represented in the kernel.
+    pub fn get_name(&self) -> Result<&str> {
+        // SAFETY: The string returned by libgpiod is guaranteed to live as long
+        // as the `struct GpiodChip`.
+        let name = unsafe { bindings::gpiod_chip_get_name(self.ichip.chip()) };
+        if name.is_null() {

This is not possible, the string can be empty at the very least but never null. Same for label and path.

bindings/rust/src/chip.rs line 162

Quoted 7 lines

bindings/rust/src/chip.rs new file +7 -0

+
+        match ret {
+            -1 => Err(Error::OperationFailed(
+                "GpiodChip info-event-wait",
+                IoError::last(),
+            )),
+            0 => Err(Error::OperationTimedOut),

Does it have to be an Error? It's pretty normal for an operation to time out, no?

bindings/rust/src/chip.rs line 170

Quoted 8 lines

bindings/rust/src/chip.rs new file +8 -0

+            _ => Ok(()),
+        }
+    }
+
+    /// Read a single line status change event from this chip. If no events are
+    /// pending, this function will block.
+    pub fn info_event_read(&self) -> Result<GpiodInfoEvent> {
+        GpiodInfoEvent::new(&self.ichip.clone())

Would you mind explaining what the clone() function does here to ichip and why it's needed.

Quoted 41 lines

bindings/rust/src/edge_event.rs new file +41 -0

+impl GpiodEdgeEvent {
+    /// Get an event stored in the buffer.
+    pub fn new(buffer: &GpiodEdgeEventBuffer, index: u64) -> Result<Self> {
+        let event = unsafe { bindings::gpiod_edge_event_buffer_get_event(buffer.buffer(), index) };
+        if event.is_null() {
+            return Err(Error::OperationFailed(
+                "GpiodEdgeEvent buffer-get-event",
+                IoError::last(),
+            ));
+        }
+
+        Ok(Self { event })
+    }
+
+    /// Get the event type.
+    pub fn get_event_type(&self) -> Result<EdgeEvent> {
+        EdgeEvent::new(unsafe { bindings::gpiod_edge_event_get_event_type(self.event) } as u32)
+    }
+
+    /// Get the timestamp of the event.
+    pub fn get_timestamp(&self) -> Duration {
+        Duration::from_nanos(unsafe { bindings::gpiod_edge_event_get_timestamp(self.event) })
+    }
+
+    /// Get the offset of the line on which the event was triggered.
+    pub fn get_line_offset(&self) -> u32 {
+        unsafe { bindings::gpiod_edge_event_get_line_offset(self.event) }
+    }
+
+    /// Get the global sequence number of this event.
+    ///
+    /// Returns sequence number of the event relative to all lines in the
+    /// associated line request.
+    pub fn get_global_seqno(&self) -> u64 {
+        unsafe { bindings::gpiod_edge_event_get_global_seqno(self.event) }
+    }
+
+    /// Get the event sequence number specific to concerned line.
+    ///
+    /// Returns sequence number of the event relative to this line within the
+    /// lifetime of the associated line request.

bindings/rust/src/edge_event.rs line 71

Quoted 4 lines

bindings/rust/src/edge_event.rs new file +4 -0

+    pub fn get_line_seqno(&self) -> u64 {
+        unsafe { bindings::gpiod_edge_event_get_line_seqno(self.event) }
+    }
+}

AFAICT this object will not survive the parent buffer. Take a look at the current development version of the C++ bindings where I play with copy constructors to ensure that.

Quoted 3 lines · Bartosz Golaszewski

On Thu, Dec 2, 2021 at 12:23 PM Viresh Kumar <[email protected]> wrote:
General comment on the naming convention:

Quoted 4 lines

The line from one of the examples: 'use libgpiod::chip::GpiodChip;' looks like it has a lot of redundancy in it. How about calling the crate gpiod and dropping the chip package entirely? Basically follow what C++ and python bindings do by having use gpiod::Chip etc.?

That would mean I move back all the code to lib.rs, which makes it contain too much code. I am okay though if you want it that way.

Quoted 5 lines

bindings/rust/src/chip.rs line 33

Quoted 5 lines

bindings/rust/src/chip.rs new file +5 -0

+
+impl GpiodChipInternal {
+    /// Find a GPIO chip by path.
+    pub(crate) fn open(path: &str) -> Result<Self> {
+        let chip = unsafe { bindings::gpiod_chip_open(path.as_ptr() as *const c_char) };

Quoted 6 lines

bindings/rust/src/chip.rs line 35

Quoted 2 lines

bindings/rust/src/chip.rs new file +2 -0

+        if chip.is_null() {
+            return Err(Error::OperationFailed("GpiodChip open", IoError::last()));

One of my concerns last time was error handling. Does this now translate the error codes from the underlying C code to some kind of rust errors? Can you elaborate on how that works?

You can think of it like wrapping around or concatenation of errors in a way. The above line will return this now:

Err(libgpiod::Error::OperationFailed("GpiodChip open", IoError::last())) and will print like for EINVAL:

Operation GpiodChip open Failed: 22

The user crate can further wrap it around with their own error enums, but this error information wouldn't be lost and won't be subject to interpretation as well, as we return the real underlying value now. This is the right/best way to do it I suppose in Rust, this is what I was suggested to do for the vhost-device Rust crate I merged in rust-vmm project.

Quoted 1 line

I see it always returns IoError.

This returns the underling errno value.

Quoted 3 lines

In my WIP C++ and Python code I try to translate the errnos into some meaningful exceptions - for instance EINVAL -> ::std::invalid_argument etc. Can we have a similar thing here?

I would avoid translating errors into some interpretation here and rather pass on the real error number returned by kernel. That can be more useful in debugging the failures later.

Quoted 7 lines

Quoted 6 lines

bindings/rust/src/chip.rs new file +6 -0

+    /// Get the GPIO chip name as represented in the kernel.
+    pub fn get_name(&self) -> Result<&str> {
+        // SAFETY: The string returned by libgpiod is guaranteed to live as long
+        // as the `struct GpiodChip`.
+        let name = unsafe { bindings::gpiod_chip_get_name(self.ichip.chip()) };
+        if name.is_null() {

Quoted 2 lines

This is not possible, the string can be empty at the very least but never null. Same for label and path.

Miguel, what's your take on stuff like this ? I am not sure if we should just drop this check altogether.

Quoted 10 lines

bindings/rust/src/chip.rs line 162

Quoted 7 lines

bindings/rust/src/chip.rs new file +7 -0

+
+        match ret {
+            -1 => Err(Error::OperationFailed(
+                "GpiodChip info-event-wait",
+                IoError::last(),
+            )),
+            0 => Err(Error::OperationTimedOut),

Does it have to be an Error? It's pretty normal for an operation to time out, no?

Right, but it is still a timeout and should be represented like that. And it is up for the caller to act based on it. For example in the gpiomon example, we check if the returned value is OperationTimedOut or not, and we retry if it is.

Quoted 11 lines

bindings/rust/src/chip.rs line 170

Quoted 8 lines

bindings/rust/src/chip.rs new file +8 -0

+            _ => Ok(()),
+        }
+    }
+
+    /// Read a single line status change event from this chip. If no events are
+    /// pending, this function will block.
+    pub fn info_event_read(&self) -> Result<GpiodInfoEvent> {
+        GpiodInfoEvent::new(&self.ichip.clone())

Would you mind explaining what the clone() function does here to ichip and why it's needed.

Normally clone() is like copying of the objects, but in this case ichip is declared as Arc (Atomic Reference Counting).

Invoking clone on Arc produces a new Arc instance, which points to the same allocation on the heap as the source Arc, while increasing a reference count.

In rust, when the last reference of an object goes away, it is freed automatically.

Quoted 41 lines

Quoted 41 lines

bindings/rust/src/edge_event.rs new file +41 -0

+impl GpiodEdgeEvent {
+    /// Get an event stored in the buffer.
+    pub fn new(buffer: &GpiodEdgeEventBuffer, index: u64) -> Result<Self> {
+        let event = unsafe { bindings::gpiod_edge_event_buffer_get_event(buffer.buffer(), index) };
+        if event.is_null() {
+            return Err(Error::OperationFailed(
+                "GpiodEdgeEvent buffer-get-event",
+                IoError::last(),
+            ));
+        }
+
+        Ok(Self { event })
+    }
+
+    /// Get the event type.
+    pub fn get_event_type(&self) -> Result<EdgeEvent> {
+        EdgeEvent::new(unsafe { bindings::gpiod_edge_event_get_event_type(self.event) } as u32)
+    }
+
+    /// Get the timestamp of the event.
+    pub fn get_timestamp(&self) -> Duration {
+        Duration::from_nanos(unsafe { bindings::gpiod_edge_event_get_timestamp(self.event) })
+    }
+
+    /// Get the offset of the line on which the event was triggered.
+    pub fn get_line_offset(&self) -> u32 {
+        unsafe { bindings::gpiod_edge_event_get_line_offset(self.event) }
+    }
+
+    /// Get the global sequence number of this event.
+    ///
+    /// Returns sequence number of the event relative to all lines in the
+    /// associated line request.
+    pub fn get_global_seqno(&self) -> u64 {
+        unsafe { bindings::gpiod_edge_event_get_global_seqno(self.event) }
+    }
+
+    /// Get the event sequence number specific to concerned line.
+    ///
+    /// Returns sequence number of the event relative to this line within the
+    /// lifetime of the associated line request.

Quoted 8 lines

bindings/rust/src/edge_event.rs line 71

Quoted 4 lines

bindings/rust/src/edge_event.rs new file +4 -0

+    pub fn get_line_seqno(&self) -> u64 {
+        unsafe { bindings::gpiod_edge_event_get_line_seqno(self.event) }
+    }
+}

AFAICT this object will not survive the parent buffer. Take a look at the current development version of the C++ bindings where I play with copy constructors to ensure that.

Adding back some of the removed code as well.

bindings/rust/src/edge_event.rs new file +6 -0

+impl Drop for GpiodEdgeEvent {
+    /// Free the edge event object and release all associated resources.
+    fn drop(&mut self) {
+        unsafe { bindings::gpiod_edge_event_free(self.event) }
+    }
+}

While testing this I found a bug few days back and I wonder why gpiod_edge_event_free() even exists. The memory for "events" is allocated with the buffer and gpiod_edge_event_free() shouldn't try to free a part of that. This looks buggy.

Yes I realize that the edge event shouldn't exist past the buffer itself, I will try to fix it in a Rusty way (maybe with Arc or something else).

--
viresh

Some drive-by comments. I haven't reviewed the code in detail.

Quoted 10 lines · Viresh Kumar

Quoted 7 lines · Bartosz Golaszewski

On Thu, Dec 2, 2021 at 12:23 PM Viresh Kumar <[email protected]> wrote:
General comment on the naming convention:

The line from one of the examples: 'use libgpiod::chip::GpiodChip;' looks like it has a lot of redundancy in it. How about calling the crate gpiod and dropping the chip package entirely? Basically follow what C++ and python bindings do by having use gpiod::Chip etc.?

Quoted 2 lines

That would mean I move back all the code to lib.rs, which makes it contain too much code. I am okay though if you want it that way.

You can keep things in different source files/modules and then expose them from lib.rs. For example, we have something like this in the kernel crate:

mod types;
pub use crate::types::{bit, bits_iter, Mode, Opaque, ScopeGuard};

So these are implemented in types.rs but exposed directly from the kernel crate as kernel::bit, kernel::bits_iter, etc.

Quoted 5 lines

Quoted 5 lines

bindings/rust/src/chip.rs line 33

Quoted 5 lines

bindings/rust/src/chip.rs new file +5 -0

+
+impl GpiodChipInternal {
+    /// Find a GPIO chip by path.
+    pub(crate) fn open(path: &str) -> Result<Self> {
+        let chip = unsafe { bindings::gpiod_chip_open(path.as_ptr() as *const c_char) };

Note that Rust strings are not necessarily null-terminated, so if gpiod_chip_open is expecting a null-terminated string,

this isn't
always going to work. In the kernel we have null-terminated strings as a CStr type, and they can be initialised with constants using the c_str! macro. You'll probably need something similar here if you want to avoid allocations (otherwise you can just allocate a null-terminated copy of path, use it, then free it).

Quoted 10 lines

Quoted 6 lines

bindings/rust/src/chip.rs line 35

Quoted 2 lines

bindings/rust/src/chip.rs new file +2 -0

+        if chip.is_null() {
+            return Err(Error::OperationFailed("GpiodChip open", IoError::last()));

One of my concerns last time was error handling. Does this now translate the error codes from the underlying C code to some kind of rust errors? Can you elaborate on how that works?

You can think of it like wrapping around or concatenation of errors in a way. The above line will return this now:

Quoted 4 lines

Err(libgpiod::Error::OperationFailed("GpiodChip open", IoError::last())) and will print like for EINVAL:

Operation GpiodChip open Failed: 22

Seems ok for printing. Is there a way to get the actual error code (22 in the example above) from the error returned? (As opposed to just a string.)

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

bindings/rust/src/chip.rs new file +6 -0

+    /// Get the GPIO chip name as represented in the kernel.
+    pub fn get_name(&self) -> Result<&str> {
+        // SAFETY: The string returned by libgpiod is guaranteed to live as long
+        // as the `struct GpiodChip`.
+        let name = unsafe { bindings::gpiod_chip_get_name(self.ichip.chip()) };
+        if name.is_null() {

Quoted 5 lines

Quoted 2 lines

This is not possible, the string can be empty at the very least but never null. Same for label and path.

Miguel, what's your take on stuff like this ? I am not sure if we should just drop this check altogether.

I'm obviously not Miguel :) But here's my take on this:

I think it's better to keep the check even if it's not possible today.

But if you choose to remove it, then you probably want to note in the next SAFETY comment for from_raw_parts (which I think you forgot to add) that name is always non-null.

Quoted 8 lines

Quoted 7 lines

bindings/rust/src/chip.rs line 170

Quoted 4 lines

bindings/rust/src/chip.rs new file +4 -0

+    /// Read a single line status change event from this chip. If no events are
+    /// pending, this function will block.
+    pub fn info_event_read(&self) -> Result<GpiodInfoEvent> {
+        GpiodInfoEvent::new(&self.ichip.clone())

Would you mind explaining what the clone() function does here to ichip and why it's needed.

Quoted 8 lines

Normally clone() is like copying of the objects, but in this case ichip is declared as Arc (Atomic Reference Counting).

Invoking clone on Arc produces a new Arc instance, which points to the same allocation on the heap as the source Arc, while increasing a reference count.

In rust, when the last reference of an object goes away, it is freed automatically.

Your description of Arc is accurate, but it doesn't look like the code does what you intended. Note that you're passing the _address_ of the cloned arc to GpiodInfoEvent::new, so what you're doing here is:
incrementing the refcount, calling GpiodInfoEvent::new, then immediately decrementing the count. If you want to keep the extra reference alive, you must transfer ownership of it (and hold on to it for as long as you want it alive).

Quoted 3 lines · Bartosz Golaszewski

AFAICT this object will not survive the parent buffer. Take a look at the current development version of the C++ bindings where I play with copy constructors to ensure that.

Adding back some of the removed code as well.

bindings/rust/src/edge_event.rs new file +6 -0

+impl Drop for GpiodEdgeEvent {
+    /// Free the edge event object and release all associated resources.
+    fn drop(&mut self) {
+        unsafe { bindings::gpiod_edge_event_free(self.event) }
+    }
+}

While testing this I found a bug few days back and I wonder why gpiod_edge_event_free() even exists. The memory for "events" is allocated with the buffer and gpiod_edge_event_free() shouldn't try to free a part of that. This looks buggy.

Yes I realize that the edge event shouldn't exist past the buffer itself, I will try to fix it in a Rusty way (maybe with Arc or something else).

bindings/rust/src/edge_event.rs new file +6 -0

+impl Drop for GpiodEdgeEvent {
+    /// Free the edge event object and release all associated resources.
+    fn drop(&mut self) {
+        unsafe { bindings::gpiod_edge_event_free(self.event) }
+    }
+}

While testing this I found a bug few days back and I wonder why gpiod_edge_event_free() even exists. The memory for "events" is allocated with the buffer and gpiod_edge_event_free() shouldn't try to free a part of that. This looks buggy.

Yes I realize that the edge event shouldn't exist past the buffer itself, I will try to fix it in a Rusty way (maybe with Arc or something else).

No, it's a different story altogether. In C the buffer allocates memory for events and when you "get" an event, you only have a pointer to the memory space in the buffer that you must not free. But you can "copy" an event with gpiod_edge_event_copy() which returns you a deep copy of the event that will survive the parent and that must be freed with gpiod_edge_event_free(). This is done so that by default we try to limit the number of allocations (as there can be a lot of events) unless the user decides to manually copy the event.

In C++ I used that mechanism together with the buffer's const event_get() and event's copy assignment operator. "Getting" an event returns a const reference to the event (still in buffer's memory) but copying it triggers a deep copy. The memory management is of course handled by the destructor.

This is not used in Python as speed is no longer a concern and we'd be creating new python objects anyway. But in Rust, I think it makes sense to reuse this mechanism.

Quoted 15 lines · Bartosz Golaszewski

No, it's a different story altogether. In C the buffer allocates memory for events and when you "get" an event, you only have a pointer to the memory space in the buffer that you must not free. But you can "copy" an event with gpiod_edge_event_copy() which returns you a deep copy of the event that will survive the parent and that must be freed with gpiod_edge_event_free(). This is done so that by default we try to limit the number of allocations (as there can be a lot of events) unless the user decides to manually copy the event.

In C++ I used that mechanism together with the buffer's const event_get() and event's copy assignment operator. "Getting" an event returns a const reference to the event (still in buffer's memory) but copying it triggers a deep copy. The memory management is of course handled by the destructor.

Quoted 3 lines

This is not used in Python as speed is no longer a concern and we'd be creating new python objects anyway. But in Rust, I think it makes sense to reuse this mechanism.

Ahh, what about this then, it just caches all the values when the event is requested ?

pub struct EdgeEvent {
    event_type: LineEdgeEvent,
    timestamp: Duration,
    line_offset: u32,
    global_seqno: u64,
    line_seqno: u64,
}

Quoted 24 lines · Viresh Kumar

Quoted 15 lines · Bartosz Golaszewski

No, it's a different story altogether. In C the buffer allocates memory for events and when you "get" an event, you only have a pointer to the memory space in the buffer that you must not free. But you can "copy" an event with gpiod_edge_event_copy() which returns you a deep copy of the event that will survive the parent and that must be freed with gpiod_edge_event_free(). This is done so that by default we try to limit the number of allocations (as there can be a lot of events) unless the user decides to manually copy the event.

In C++ I used that mechanism together with the buffer's const event_get() and event's copy assignment operator. "Getting" an event returns a const reference to the event (still in buffer's memory) but copying it triggers a deep copy. The memory management is of course handled by the destructor.

Quoted 3 lines

This is not used in Python as speed is no longer a concern and we'd be creating new python objects anyway. But in Rust, I think it makes sense to reuse this mechanism.

Ahh, what about this then, it just caches all the values when the event is requested ?

Quoted 8 lines

pub struct EdgeEvent {
    event_type: LineEdgeEvent,
    timestamp: Duration,
    line_offset: u32,
    global_seqno: u64,
    line_seqno: u64,
}

Then does it make sense to make it a binding to libgpiod? :)

I don't know Rust that well to be able to come up with an idea off the top of my head but I assume inheritance in one way or another exists in rust? Can you have an EdgeEvent interface implemented by two specialized structs, one that's stored in the buffer (stores a pointer to the internal event)

and one that stores the copy (and frees it when
it goes out of scope)?

Quoted 34 lines · Bartosz Golaszewski

Quoted 24 lines · Viresh Kumar

Quoted 15 lines · Bartosz Golaszewski

No, it's a different story altogether. In C the buffer allocates memory for events and when you "get" an event, you only have a pointer to the memory space in the buffer that you must not free. But you can "copy" an event with gpiod_edge_event_copy() which returns you a deep copy of the event that will survive the parent and that must be freed with gpiod_edge_event_free(). This is done so that by default we try to limit the number of allocations (as there can be a lot of events) unless the user decides to manually copy the event.

In C++ I used that mechanism together with the buffer's const event_get() and event's copy assignment operator. "Getting" an event returns a const reference to the event (still in buffer's memory) but copying it triggers a deep copy. The memory management is of course handled by the destructor.

Quoted 3 lines

This is not used in Python as speed is no longer a concern and we'd be creating new python objects anyway. But in Rust, I think it makes sense to reuse this mechanism.

Ahh, what about this then, it just caches all the values when the event is requested ?

Quoted 8 lines

pub struct EdgeEvent {
    event_type: LineEdgeEvent,
    timestamp: Duration,
    line_offset: u32,
    global_seqno: u64,
    line_seqno: u64,
}

Quoted 1 line

Then does it make sense to make it a binding to libgpiod? :)

Yes, because EdgeEvent::new() gets all these values from the internal C libgpiod library itself. We are going to make the same copy of data, either at the C library level, or here.

Quoted 5 lines

I don't know Rust that well to be able to come up with an idea off the top of my head but I assume inheritance in one way or another exists in rust? Can you have an EdgeEvent interface implemented by two specialized structs, one that's stored in the buffer (stores a pointer to the internal event)

This is what my initial patch had, right? It isn't safe. There is no way for libgpiod-rust to guarantee that the users don't access the EdgeEvent after the buffer is freed and this needs to be made safer at libgpiod itself.

Quoted 2 lines

and one that stores the copy (and frees it when
it goes out of scope)?

I don't understand what we will get out of such a scheme? Is there any benefit of getting these values via the pointer to the internal C event ?

Any user that wants to use the edge-event data, needs to call the edge-event APIs at least once and this is what the new version does, just calls them from new() itself.

Quoted 10 lines · Viresh Kumar

Quoted 5 lines

I don't know Rust that well to be able to come up with an idea off the top of my head but I assume inheritance in one way or another exists in rust? Can you have an EdgeEvent interface implemented by two specialized structs, one that's stored in the buffer (stores a pointer to the internal event)

This is what my initial patch had, right? It isn't safe. There is no way for libgpiod-rust to guarantee that the users don't access the EdgeEvent after the buffer is freed and this needs to be made safer at libgpiod itself.

I'm not sure it had the same thing. In libgpiod it's simple a matter of definition. The docs state that a pointer retrieved using gpiod_edge_event_buffer_get_event() must not be freed while one obtained with gpiod_edge_event_copy() does have to be disposed of. In C++ it's safe because either you hold a const reference to the object stored in the event buffer or you have copied it and now it will survive the parent meaning the user can access the event even after the buffer was deleted.

Quoted 3 lines

Quoted 2 lines

and one that stores the copy (and frees it when
it goes out of scope)?

Quoted 3 lines

I don't understand what we will get out of such a scheme? Is there any benefit of getting these values via the pointer to the internal C event ?

You avoid allocating memory for those objects. The memory exists in the C buffer. In order to keep a similar level of performance you'd need to replicate the behavior of the buffer in rust as well.

Miguel, what's your take on stuff like this ? I am not sure if we should just drop this check altogether.

Quoted 3 lines · Viresh Kumar

Miguel, what's your take on stuff like this ? I am not sure if we should just drop this check altogether.

Given the C side documents that the pointer is valid (I think; since others are documented as "or NULL" etc., but it is a bit implicit), then avoiding the check is a possibility.

However, to answer this, one needs to understand the risk profile of the project, the stability of the C API, the performance expectations (e.g. is the call supposed to be in the hot path of clients?), etc.

When in doubt (or when there is no reason to not do the check), it is usually better to err on the safe side, specially if you aren't close to the maintainers of the API etc. But since you have access to Linus/Bartosz here, it is best to discuss this with them :) And, after all, you are already trusting the C side to some degree anyway, and it would be good to make sure both sides understand and clearly document the contracts of the functions so that no bindings break in the future (Rust or otherwise).

The line from one of the examples: 'use libgpiod::chip::GpiodChip;' looks like it has a lot of redundancy in it. How about calling the crate gpiod and dropping the chip package entirely? Basically follow what C++ and python bindings do by having use gpiod::Chip etc.?

Does it have to be an Error? It's pretty normal for an operation to time out, no?

In any case, one can also re-export things more conveniently for users, so that is an option too.

Hi Bartosz,

This patch adds rust bindings for libgpiod v2.0, this is already partially tested with the virtio rust backend I am developing, which uses these to talk to the host kernel.

This is based of the next/post-libgpiod-2.0 branch.

I will be adding testing infrastructure later on, once other bindings are converted to use gpiosim.

Quoted 10 lines · Viresh Kumar

Hi Bartosz,

This patch adds rust bindings for libgpiod v2.0, this is already partially tested with the virtio rust backend I am developing, which uses these to talk to the host kernel.

This is based of the next/post-libgpiod-2.0 branch.

I will be adding testing infrastructure later on, once other bindings are converted to use gpiosim.

Hi Bartosz,

I can see a lots of patches related to V2 version making there way into libgpiod, but it is a bit hard for me to follow them currently.

Just wanted to request you, if you can ping me once the API is stable enough and most of the V2 changes you planned are merged, so I can rebase my stuff over those and send for review.

Thanks.

--
viresh