On 17-12-21, 05:52, Wedson Almeida Filho wrote:
Some drive-by comments. I haven't reviewed the code in detail.
On Fri, 17 Dec 2021 at 05:01, Viresh Kumar viresh.kumar@linaro.org wrote:
On 16-12-21, 14:59, Bartosz Golaszewski wrote:
On Thu, Dec 2, 2021 at 12:23 PM Viresh Kumar viresh.kumar@linaro.org 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.?
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:
Right I did that kind of thing somewhere else too. This should work better.
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.
+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,
I guess it would, though my code works just fine right now and I am not sure why, maybe by chance the next byte is zero or something :)
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).
So I must do something like this then ?
pub(crate) fn open(path: &str) -> Result<Self> { let chip = unsafe { bindings::gpiod_chip_open(CStr::from_ptr(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?
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
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.)
The entire thing is returned to the caller, i.e. libgpiod::Error::OperationFailed("GpiodChip open", 22).
I think caller should be able to get the code out of it somehow, won't something like this work ?
let errno = match error { libgpiod::Error::OperationFailed(msg, errno) => errno, _ => 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.
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:
You are always welcome to provide feedback, I am the newbie here :)
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.
Right.
- /// 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.
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).
Ahh, this instance contains a clone by mistake here. It was correctly done at other places where I wanted the newly allocated structure to contain a reference to underlying ichip. I have removed clone() from two places where I was passing the reference instead.