| Message ID | cover.1665744170.git.viresh.kumar@linaro.org |
|---|---|
| Headers | show |
| Series | libgpiod: Add Rust bindings | expand |
On Friday, October 14th, 2022 at 19:03, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > On Fri, Oct 14, 2022 at 12:47 PM Viresh Kumar viresh.kumar@linaro.org wrote: > > > Here is another version of rust bindings for libgpiod v2.0, based of the > > next/libgpiod-2.0. > > > > Pushed here: > > > > https://github.com/vireshk/libgpiod v7 > > > > [I have pushed v6 and v5 there too, in case someone wants to look at the > > changes]. It looks pretty good to me. I do have a couple of minor suggestions though. https://github.com/vireshk/libgpiod/blob/3e7fb99173856a3995360fc3fad51220c4b4e90e/bindings/rust/libgpiod/Cargo.toml#L10 should use 0.2.39 without >= to ensure that publishing a 0.3 version of libc with breaking changes won't break the build. Cargo treats "0.2.39" as ">=0.2.39, <0.3". At https://github.com/vireshk/libgpiod/blob/3e7fb99173856a3995360fc3fad51220c4b4e90e/bindings/rust/libgpiod/Cargo.toml#L13 the vmm-sys-utils dependency should be unpinned from "=0.10.0" to "0.10.0". Otherwise having any crate depend on a newer semver compatible version will cause a build error. While Cargo allows multiple semver incompatible versions of a crate, it doesn't allow multiple semver compatible versions as it wouldn't know which version to use for a crate that says it works with both versions. At https://github.com/vireshk/libgpiod/blob/3e7fb99173856a3995360fc3fad51220c4b4e90e/bindings/rust/libgpiod/src/lib.rs#L469 and elsewhere you might want to use `CStr::from_ptr(version)`. This does the `strlen` call for you and you can convert it to an `&str` using `.to_str()`. At https://github.com/vireshk/libgpiod/blob/3e7fb99173856a3995360fc3fad51220c4b4e90e/bindings/rust/libgpiod/src/chip.rs#L171 you could use `CString` and use the `.as_ptr()` method to get a null-terminated string. Not sure if it would be nicer that what you currently have though. At https://github.com/vireshk/libgpiod/blob/3e7fb99173856a3995360fc3fad51220c4b4e90e/bindings/rust/libgpiod/src/edge_event.rs#L46 the lifetimes are unclear. Is Event allowed to outlive the buffer? Can you add a lifetime annotation like fn event_clone<'a>(event: &Event<'a>) -> Result<Event<'a>> if it isn't allowed to outlive the buffer or fn event_clone<'a, 'b>(event: &Event<'a>) -> Result<Event<'b>> if it is allowed to outlive the buffer. I'm not sure which of the two the lifetime elision rules cause the current code to be equivalent of, but even if it is correct, explicitly stating the lifetime here is clearer IMHO. As for the question about test skipping elsewhere in this thread, rust has the #[ignore] attribute to ignore tests at compile time (with a command line flag to run ignored tests anyway), but nothing to ignore tests at runtime unfortunately. Cheers, Björn nb: I can't reply to a mail I didn't receive directly right now due to mail provider limitations. I'm working on sorting this out, but for now I'm going to reply to the mail I did receive directly.
On Friday, October 21st, 2022 at 11:39, Viresh Kumar <viresh.kumar@linaro.org> wrote: > Hi Björn, > > I have bounced (mutt's feature) the initial emails to your and other > email ids that Miguel added. The patches should be in your inbox now. Thanks! I receive the patches. > > On 20-10-22, 13:29, Björn Roy Baron wrote: > > > At > > https://github.com/vireshk/libgpiod/blob/3e7fb99173856a3995360fc3fad51220c4b4e90e/bindings/rust/libgpiod/src/lib.rs#L469 > > and elsewhere you might want to use `CStr::from_ptr(version)`. This > > does the `strlen` call for you and you can convert it to an `&str` > > using `.to_str()`. > > > At > > https://github.com/vireshk/libgpiod/blob/3e7fb99173856a3995360fc3fad51220c4b4e90e/bindings/rust/libgpiod/src/chip.rs#L171 > > you could use `CString` and use the `.as_ptr()` method to get a > > null-terminated string. Not sure if it would be nicer that what you > > currently have though. > > > These two were nice. Thanks. > > > At > > https://github.com/vireshk/libgpiod/blob/3e7fb99173856a3995360fc3fad51220c4b4e90e/bindings/rust/libgpiod/src/edge_event.rs#L46 > > the lifetimes are unclear. Is Event allowed to outlive the buffer? > > Can you add a lifetime annotation like fn event_clone<'a>(event: > > &Event<'a>) -> Result<Event<'a>> if it isn't allowed to outlive the > > buffer or fn event_clone<'a, 'b>(event: &Event<'a>) -> > > Result<Event<'b>> if it is allowed to outlive the buffer. I'm not > > sure which of the two the lifetime elision rules cause the current > > code to be equivalent of, but even if it is correct, explicitly > > stating the lifetime here is clearer IMHO. > > > An Event created using Event::new() can't outlive the buffer, though > an Event created using Event::event_clone() can. > > I tried to play around it based on your suggestion and here is the > diff, does it make sense ? > > diff --git a/bindings/rust/libgpiod/src/edge_event.rs b/bindings/rust/libgpiod/src/edge_event.rs > index b36c23601bb4..0d328ebb2b03 100644 > --- a/bindings/rust/libgpiod/src/edge_event.rs > +++ b/bindings/rust/libgpiod/src/edge_event.rs > @@ -33,7 +33,7 @@ pub struct Event<'b> { > > > impl<'b> Event<'b> { > > /// Get an event stored in the buffer. > - pub(crate) fn new(buffer: &'b Buffer, index: usize) -> Result<Self> { > > + pub(crate) fn new(buffer: &'b Buffer, index: usize) -> Result<Event<'b>> { This looks good to me. > > // SAFETY: The `gpiod_edge_event` returned by libgpiod is guaranteed to live as long > // as the `struct Event`. > let event = unsafe { > @@ -52,22 +52,6 @@ impl<'b> Event<'b> { > > }) > } > > - pub fn event_clone(event: &Event) -> Result<Self> { > > - // SAFETY: `gpiod_edge_event` is guaranteed to be valid here. > - let event = unsafe { gpiod::gpiod_edge_event_copy(event.event) }; > - if event.is_null() { > - return Err(Error::OperationFailed( > - OperationType::EdgeEventCopy, > - Errno::last(), > - )); > - } > - > - Ok(Self { > - buffer: None, > - event, > - }) > - } > - > /// Get the event type. > pub fn event_type(&self) -> Result<EdgeKind> { > > // SAFETY: `gpiod_edge_event` is guaranteed to be valid here. > @@ -105,6 +89,27 @@ impl<'b> Event<'b> { > > } > } > > +impl<'e, 'b> Event<'e> { > > + pub fn event_clone(event: &Event<'b>) -> Result<Event<'e>> > > + where > + 'e: 'b, Using `Event<'b>` on both sides should work fine. `Event` is covariant in it's lifetime parameter, so `Event<'b>` can be turned into `Event<'e>` with `'e` being a shorter lifetime than `'b`. What you wrote here is not incorrect, so if you prefer keeping it this way that is fine with me. > + { > + // SAFETY: `gpiod_edge_event` is guaranteed to be valid here. > + let event = unsafe { gpiod::gpiod_edge_event_copy(event.event) }; > + if event.is_null() { > + return Err(Error::OperationFailed( > + OperationType::EdgeEventCopy, > + Errno::last(), > + )); > + } > + > + Ok(Self { > + buffer: None, > + event, > + }) > + } > +} > + > > -- > viresh Cheers, Björn
On Tuesday, October 25th, 2022 at 08:42, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 21-10-22, 14:34, Björn Roy Baron wrote: > > > > impl<'b> Event<'b> { > > > > > > /// Get an event stored in the buffer. > > > - pub(crate) fn new(buffer: &'b Buffer, index: usize) -> Result<Self> { > > > > > > + pub(crate) fn new(buffer: &'b Buffer, index: usize) -> Result<Event<'b>> { > > > > This looks good to me. > > > > +impl<'e, 'b> Event<'e> { > > > > > > + pub fn event_clone(event: &Event<'b>) -> Result<Event<'e>> > > > > > > + where > > > + 'e: 'b, > > > > Using `Event<'b>` on both sides should work fine. `Event` is > > covariant in it's lifetime parameter, so `Event<'b>` can be turned > > into `Event<'e>` with `'e` being a shorter lifetime than `'b`. What > > you wrote here is not incorrect, so if you prefer keeping it this > > way that is fine with me. > > > That doesn't let the cloned event to live past read_edge_events(). > > error[E0502]: cannot borrow `buffer` as mutable because it is also borrowed as immutable > --> libgpiod/examples/gpio_events.rs:70:50 > > | > 64 | let event = buffer.event(0)?; > | --------------- immutable borrow occurs here > ... > 70 | let count = request.read_edge_events(&mut buffer)?; > | ^^^^^^^^^^^ mutable borrow occurs here > ... > 86 | } > | - immutable borrow might be used here, when `cloned_event` is dropped and runs the `Drop` code for type `libgpiod::request::Event` > > -- > viresh I would have expected that to work fine, but as it doesn't work keeping your code with two separate lifetimes is fine. Cheers, Bjorn
Hello, Here is another version of rust bindings for libgpiod v2.0, based of the next/libgpiod-2.0. Pushed here: https://github.com/vireshk/libgpiod v7 [I have pushed v6 and v5 there too, in case someone wants to look at the changes]. V6->V7: - Don't let buffer read new events if the earlier events are still referenced. - BufferIntenal is gone now, to make the above work. - Update example and tests too for the same. V5->V6: - Updates according to the new line-settings interface. - New file, line_settings.rs. - Renamed 'enum Setting' as 'SettingVal' to avoid conflicting names, as we also have 'struct Settings' now. - Support for HTE clock type. - Implement 'Eq' for public structure/enums (reported by build). - Remove 'SettingKindMap' and 'SettingMap' as they aren't required anymore. - Updated tests based on new interface. V4->V5: - Arrange as workspace with crates for libgpiod-sys, libgpiod, gpiosim. - Use static libgpiod and libgpiosim libraries instead of rebuilding again. - Arrange in modules instead of flattened approach. - New enums like Setting and SettingKind and new types based on them SettingMap and SettingKindMap. - New property independent helpers for line_config, like set_prop_default(). - Improved tests/examples, new example for gpiowatch. - Add pre-built bindings for gpiosim too. - Many other changes. V3->V4: - Rebased on top of new changes, and made changes accordingly. - Added rust integration tests with gpiosim. - Found a kernel bug with tests, sent a patch for that to LKML. V2->V3: - Remove naming redundancy, users just need to do this now use libgpiod:{Chip, Direction, LineConfig} now (Bartosz); - Fix lifetime issues between event-buffer and edge-event modules, the event buffer is released after the last edge-event reference is dropped (Bartosz). - Allow edge-event to be copied, and freed later (Bartosz). - Add two separate rust crates, sys and wrapper (Gerard). - Null-terminate the strings passed to libgpiod (Wedson). - Drop unnecessary checks to validate string returned from chip:name/label/path. - Fix SAFETY comments (Wedson). - Drop unnecessary clone() instances (Bartosz). V1->V2: - Added examples (I tested everything except gpiomon.rs, didn't have right hardware/mock device to test). - Build rust bindings as part of Make, update documentation. Thanks. -- Viresh Viresh Kumar (8): libgpiod: Add libgpiod-sys rust crate libgpiod-sys: Add pre generated rust bindings libgpiod: Add rust wrapper crate libgpiod: Add rust examples libgpiod: Add gpiosim rust crate gpiosim: Add pre generated rust bindings libgpiod: Add rust tests libgpiod: Integrate building of rust bindings with make .gitignore | 5 + README | 8 +- TODO | 8 - bindings/Makefile.am | 6 + bindings/rust/Cargo.toml | 7 + bindings/rust/Makefile.am | 18 + bindings/rust/gpiosim/Cargo.toml | 18 + bindings/rust/gpiosim/README.md | 11 + bindings/rust/gpiosim/build.rs | 43 + bindings/rust/gpiosim/src/bindings.rs | 180 +++ bindings/rust/gpiosim/src/lib.rs | 25 + bindings/rust/gpiosim/src/sim.rs | 323 +++++ bindings/rust/libgpiod-sys/Cargo.toml | 15 + bindings/rust/libgpiod-sys/README.md | 11 + bindings/rust/libgpiod-sys/build.rs | 41 + bindings/rust/libgpiod-sys/src/bindings.rs | 1173 +++++++++++++++++ bindings/rust/libgpiod-sys/src/lib.rs | 13 + bindings/rust/libgpiod/Cargo.toml | 16 + bindings/rust/libgpiod/examples/gpiodetect.rs | 30 + bindings/rust/libgpiod/examples/gpiofind.rs | 35 + bindings/rust/libgpiod/examples/gpioget.rs | 42 + bindings/rust/libgpiod/examples/gpioinfo.rs | 95 ++ bindings/rust/libgpiod/examples/gpiomon.rs | 73 + bindings/rust/libgpiod/examples/gpioset.rs | 60 + bindings/rust/libgpiod/examples/gpiowatch.rs | 54 + bindings/rust/libgpiod/src/chip.rs | 253 ++++ bindings/rust/libgpiod/src/edge_event.rs | 101 ++ bindings/rust/libgpiod/src/event_buffer.rs | 66 + bindings/rust/libgpiod/src/info_event.rs | 68 + bindings/rust/libgpiod/src/lib.rs | 471 +++++++ bindings/rust/libgpiod/src/line_config.rs | 118 ++ bindings/rust/libgpiod/src/line_info.rs | 180 +++ bindings/rust/libgpiod/src/line_request.rs | 246 ++++ bindings/rust/libgpiod/src/line_settings.rs | 277 ++++ bindings/rust/libgpiod/src/request_config.rs | 96 ++ bindings/rust/libgpiod/tests/chip.rs | 97 ++ bindings/rust/libgpiod/tests/common/config.rs | 134 ++ bindings/rust/libgpiod/tests/common/mod.rs | 10 + bindings/rust/libgpiod/tests/edge_event.rs | 296 +++++ bindings/rust/libgpiod/tests/info_event.rs | 125 ++ bindings/rust/libgpiod/tests/line_config.rs | 105 ++ bindings/rust/libgpiod/tests/line_info.rs | 279 ++++ bindings/rust/libgpiod/tests/line_request.rs | 519 ++++++++ bindings/rust/libgpiod/tests/line_settings.rs | 226 ++++ .../rust/libgpiod/tests/request_config.rs | 38 + configure.ac | 16 + 46 files changed, 6020 insertions(+), 11 deletions(-) create mode 100644 bindings/rust/Cargo.toml create mode 100644 bindings/rust/Makefile.am create mode 100644 bindings/rust/gpiosim/Cargo.toml create mode 100644 bindings/rust/gpiosim/README.md create mode 100644 bindings/rust/gpiosim/build.rs create mode 100644 bindings/rust/gpiosim/src/bindings.rs create mode 100644 bindings/rust/gpiosim/src/lib.rs create mode 100644 bindings/rust/gpiosim/src/sim.rs create mode 100644 bindings/rust/libgpiod-sys/Cargo.toml create mode 100644 bindings/rust/libgpiod-sys/README.md create mode 100644 bindings/rust/libgpiod-sys/build.rs create mode 100644 bindings/rust/libgpiod-sys/src/bindings.rs create mode 100644 bindings/rust/libgpiod-sys/src/lib.rs create mode 100644 bindings/rust/libgpiod/Cargo.toml create mode 100644 bindings/rust/libgpiod/examples/gpiodetect.rs create mode 100644 bindings/rust/libgpiod/examples/gpiofind.rs create mode 100644 bindings/rust/libgpiod/examples/gpioget.rs create mode 100644 bindings/rust/libgpiod/examples/gpioinfo.rs create mode 100644 bindings/rust/libgpiod/examples/gpiomon.rs create mode 100644 bindings/rust/libgpiod/examples/gpioset.rs create mode 100644 bindings/rust/libgpiod/examples/gpiowatch.rs create mode 100644 bindings/rust/libgpiod/src/chip.rs create mode 100644 bindings/rust/libgpiod/src/edge_event.rs create mode 100644 bindings/rust/libgpiod/src/event_buffer.rs create mode 100644 bindings/rust/libgpiod/src/info_event.rs create mode 100644 bindings/rust/libgpiod/src/lib.rs create mode 100644 bindings/rust/libgpiod/src/line_config.rs create mode 100644 bindings/rust/libgpiod/src/line_info.rs create mode 100644 bindings/rust/libgpiod/src/line_request.rs create mode 100644 bindings/rust/libgpiod/src/line_settings.rs create mode 100644 bindings/rust/libgpiod/src/request_config.rs create mode 100644 bindings/rust/libgpiod/tests/chip.rs create mode 100644 bindings/rust/libgpiod/tests/common/config.rs create mode 100644 bindings/rust/libgpiod/tests/common/mod.rs create mode 100644 bindings/rust/libgpiod/tests/edge_event.rs create mode 100644 bindings/rust/libgpiod/tests/info_event.rs create mode 100644 bindings/rust/libgpiod/tests/line_config.rs create mode 100644 bindings/rust/libgpiod/tests/line_info.rs create mode 100644 bindings/rust/libgpiod/tests/line_request.rs create mode 100644 bindings/rust/libgpiod/tests/line_settings.rs create mode 100644 bindings/rust/libgpiod/tests/request_config.rs