Re: [PATCH V2 2/4] libgpiod: Add rust wrappers

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Thu, Dec 2, 2021 at 12:23 PM Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
>
> Add rust wrappers around FFI bindings to provide a convenient interface
> for the users.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> ---

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]

> +
> +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]

> +
> +    /// 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.

> +
> +        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?

> +            _ => 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.


> +
> +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.
> +    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.

[snip]

I'll address the config objects once we actually have the final
version in the C API.

Bart