Skip to contentSkip to conversation

bindings: rust: Add libgpiod-sys rust crate

PATCH Superseded Viresh Kumar 2 participants 19 messages Oct 31, 2022 – Nov 7, 2022 GPIO

v8 of 10 Superseded Latest is v10 →

lore
Share & tools
lore

Viresh Kumar

author
#0 lore raw

v8 cover bindings: rust: Add libgpiod-sys rust crate

Hello,

Here is another version of the rust bindings, based of the master branch.

Pushed here:

https://github.com/vireshk/libgpiod v8

V7->V8:
- Several updates to cargo.toml files, like license, version, etc.
- Removed Sync support for chip and gpiosim.
- Implemented, in a separate patch, iterator support for Events.
- Fixed missing SAFETY comments.
- Fixed build for 32 bit systems.
- Use errno::Errno.
- Removed Clone derive for many structures, that store raw pointers.
- line setting helpers return the object back, so another helper can be called directly on them. Also made all helpers public and used the same in tests and example for single configurations.
- Enums for gpiosim constants.
- New examples to demonstrate parallelism and event handling.
- Separated out HTE tests and marked as #[ignore] now.
- Updated commit subjects.
- Other minor 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 (9):
bindings: rust: Add libgpiod-sys rust crate
bindings: rust: Add pre generated bindings for libgpiod-sys
bindings: rust: Add gpiosim crate
bindings: rust: Add pre generated bindings for gpiosim
bindings: rust: Add libgpiod crate
bindings: rust: Add examples to libgpiod crate
bindings: rust: Add tests for libgpiod crate
bindings: rust: Integrate building of bindings with make
bindings: rust: Implement iterator for edge events

 .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              |   24 +
 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              |   79 ++
 bindings/rust/gpiosim/src/sim.rs              |  331 +++++
 bindings/rust/libgpiod-sys/Cargo.toml         |   22 +
 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             |   23 +
 .../rust/libgpiod/examples/gpio_events.rs     |   89 ++
 .../examples/gpio_threaded_info_events.rs     |  133 ++
 bindings/rust/libgpiod/examples/gpiodetect.rs |   31 +
 bindings/rust/libgpiod/examples/gpiofind.rs   |   37 +
 bindings/rust/libgpiod/examples/gpioget.rs    |   46 +
 bindings/rust/libgpiod/examples/gpioinfo.rs   |   98 ++
 bindings/rust/libgpiod/examples/gpiomon.rs    |   74 ++
 bindings/rust/libgpiod/examples/gpioset.rs    |   64 +
 bindings/rust/libgpiod/examples/gpiowatch.rs  |   54 +
 bindings/rust/libgpiod/src/chip.rs            |  317 +++++
 bindings/rust/libgpiod/src/edge_event.rs      |  110 ++
 bindings/rust/libgpiod/src/event_buffer.rs    |  179 +++
 bindings/rust/libgpiod/src/info_event.rs      |   69 +
 bindings/rust/libgpiod/src/lib.rs             |  480 +++++++
 bindings/rust/libgpiod/src/line_config.rs     |  135 ++
 bindings/rust/libgpiod/src/line_info.rs       |  162 +++
 bindings/rust/libgpiod/src/line_request.rs    |  227 ++++
 bindings/rust/libgpiod/src/line_settings.rs   |  297 +++++
 bindings/rust/libgpiod/src/request_config.rs  |   95 ++
 bindings/rust/libgpiod/tests/chip.rs          |   99 ++
 bindings/rust/libgpiod/tests/common/config.rs |  143 ++
 bindings/rust/libgpiod/tests/common/mod.rs    |   10 +
 bindings/rust/libgpiod/tests/edge_event.rs    |  299 +++++
 bindings/rust/libgpiod/tests/info_event.rs    |  167 +++
 bindings/rust/libgpiod/tests/line_config.rs   |   96 ++
 bindings/rust/libgpiod/tests/line_info.rs     |  276 ++++
 bindings/rust/libgpiod/tests/line_request.rs  |  510 +++++++
 bindings/rust/libgpiod/tests/line_settings.rs |  204 +++
 .../rust/libgpiod/tests/request_config.rs     |   39 +
 configure.ac                                  |   16 +
 48 files changed, 6548 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/gpio_events.rs
 create mode 100644 bindings/rust/libgpiod/examples/gpio_threaded_info_events.rs
 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
Signature · 1 line
2.31.1.272.g89b43f80a514
author
#1 lore raw

PATCH v8 1/9 bindings: rust: Add libgpiod-sys rust crate

This adds libgpiod-sys rust crate, which provides FFI (foreign function interface) bindings for libgpiod APIs.

Signed-off-by Viresh Kumar <[email protected]>
---
 .gitignore                            |  5 ++++
 bindings/rust/Cargo.toml              |  5 ++++
 bindings/rust/libgpiod-sys/Cargo.toml | 22 ++++++++++++++
 bindings/rust/libgpiod-sys/build.rs   | 41 +++++++++++++++++++++++++++
 bindings/rust/libgpiod-sys/src/lib.rs | 13 +++++++++
 5 files changed, 86 insertions(+)
 create mode 100644 bindings/rust/Cargo.toml
 create mode 100644 bindings/rust/libgpiod-sys/Cargo.toml
 create mode 100644 bindings/rust/libgpiod-sys/build.rs
 create mode 100644 bindings/rust/libgpiod-sys/src/lib.rs
.gitignore +5 -0
index 6c08415b390d..9f2fcf440c5d 100644
@@ -35,3 +35,8 @@ stamp-h1
 # profiling
 *.gcda
 *.gcno
+
+# Added by cargo
+
+target
+Cargo.lock
bindings/rust/Cargo.toml new file +5 -0
new file mode 100644
index 000000000000..c7bbcc798920
@@ -0,0 +1,5 @@
+[workspace]
+
+members = [
+    "libgpiod-sys"
+]
bindings/rust/libgpiod-sys/Cargo.toml new file +22 -0
new file mode 100644
index 000000000000..716dde551263
@@ -0,0 +1,22 @@
+[package]
+name = "libgpiod-sys"
+version = "0.1.0"
+authors = ["Viresh Kumar <[email protected]>"]
+description = "libgpiod public header bindings"
+repository = "https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git"
+categories = ["external-ffi-bindings", "os::linux-apis"]
+rust-version = "1.56"
+keywords = ["libgpiod", "gpio"]
+license = "Apache-2.0 OR BSD-3-Clause"
+edition = "2021"
+
+# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
+
+[dependencies]
+
+[features]
+generate = [ "bindgen" ]
+
+[build-dependencies]
+bindgen = { version = "0.59.1", optional = true }
+cc = "1.0.46"
bindings/rust/libgpiod-sys/build.rs new file +41 -0

41 lines not on this page · raw message

bindings/rust/libgpiod-sys/src/lib.rs new file +13 -0
new file mode 100644
index 000000000000..a1d1db19afe3
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: Apache-2.0 OR BSD-3-Clause
+
+#[allow(non_camel_case_types, non_upper_case_globals)]
+#[cfg_attr(test, allow(deref_nullptr, non_snake_case))]
+
+mod bindings_raw {
+    #[cfg(feature = "generate")]
+    include!(concat!(env!("OUT_DIR"), "/bindings.rs"));
+
+    #[cfg(not(feature = "generate"))]
+    include!("bindings.rs");
+}
+pub use bindings_raw::*;
Signature · 1 line
2.31.1.272.g89b43f80a514
author
#2 lore raw

PATCH v8 2/9 bindings: rust: Add pre generated bindings for libgpiod-sys

In reply to Viresh Kumar

This adds a copy of pre generated bindings and adds the suggested way of updating those in README.

Signed-off-by Viresh Kumar <[email protected]>
---
 bindings/rust/libgpiod-sys/README.md       |   11 +
 bindings/rust/libgpiod-sys/src/bindings.rs | 1173 ++++++++++++++++++++
 2 files changed, 1184 insertions(+)
 create mode 100644 bindings/rust/libgpiod-sys/README.md
 create mode 100644 bindings/rust/libgpiod-sys/src/bindings.rs
bindings/rust/libgpiod-sys/README.md new file +11 -0
new file mode 100644
index 000000000000..ecf75b31c41e
@@ -0,0 +1,11 @@
+# Generated libgpiod-sys Rust FFI bindings
+Automatically generated Rust FFI bindings via
+	[bindgen](https://github.com/rust-lang/rust-bindgen).
+
+## Updating bindings
+1. Clone the source from
+	<https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/>
+2. run `cd libgpiod/bindings/rust/libgpiod-sys/`
+2. run `cargo build --features generate`
+3. Copy the bindings 'cp ../target/debug/build/libgpiod-sys-###/out/bindings.rs src/bindings.rs'
+4. Commit changes in `src/bindings.rs`
bindings/rust/libgpiod-sys/src/bindings.rs new file +1173 -0

1,173 lines not on this page · raw message

Signature · 1 line
2.31.1.272.g89b43f80a514
author
#3 lore raw

PATCH v8 3/9 bindings: rust: Add gpiosim crate

In reply to Viresh Kumar

This adds gpiosim rust crate, which provides helpers to emulate GPIO chips.

Signed-off-by Viresh Kumar <[email protected]>
---
 bindings/rust/Cargo.toml         |   1 +
 bindings/rust/gpiosim/Cargo.toml |  24 +++
 bindings/rust/gpiosim/build.rs   |  43 ++++
 bindings/rust/gpiosim/src/lib.rs |  79 ++++++++
 bindings/rust/gpiosim/src/sim.rs | 331 +++++++++++++++++++++++++++++++
 5 files changed, 478 insertions(+)
 create mode 100644 bindings/rust/gpiosim/Cargo.toml
 create mode 100644 bindings/rust/gpiosim/build.rs
 create mode 100644 bindings/rust/gpiosim/src/lib.rs
 create mode 100644 bindings/rust/gpiosim/src/sim.rs
bindings/rust/Cargo.toml +1 -0
index c7bbcc798920..b9eea6b3a5ea 100644
@@ -1,5 +1,6 @@
 [workspace]
 
 members = [
+    "gpiosim",
     "libgpiod-sys"
 ]
bindings/rust/gpiosim/Cargo.toml new file +24 -0
new file mode 100644
index 000000000000..8ae6931f8a16
@@ -0,0 +1,24 @@
+[package]
+name = "gpiosim"
+version = "0.1.0"
+authors = ["Viresh Kumar <[email protected]>"]
+description = "gpiosim header bindings"
+repository = "https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git"
+categories = ["external-ffi-bindings", "os::linux-apis"]
+rust-version = "1.56"
+keywords = ["libgpiod", "gpio", "gpiosim"]
+license = "Apache-2.0 OR BSD-3-Clause"
+edition = "2021"
+
+# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
+
+[dependencies]
+errno = "0.2.8"
+libgpiod = { path = "../libgpiod" }
+
+[features]
+generate = [ "bindgen" ]
+
+[build-dependencies]
+bindgen = { version = "0.59.1", optional = true }
+cc = "1.0.46"
bindings/rust/gpiosim/build.rs new file +43 -0

43 lines not on this page · raw message

bindings/rust/gpiosim/src/lib.rs new file +79 -0

79 lines not on this page · raw message

bindings/rust/gpiosim/src/sim.rs new file +331 -0

331 lines not on this page · raw message

Signature · 1 line
2.31.1.272.g89b43f80a514
author
#4 lore raw

PATCH v8 4/9 bindings: rust: Add pre generated bindings for gpiosim

In reply to Viresh Kumar

This adds a copy of pre generated bindings and adds the suggested way of updating those in README.

Signed-off-by Viresh Kumar <[email protected]>
---
 bindings/rust/gpiosim/README.md       |  11 ++
 bindings/rust/gpiosim/src/bindings.rs | 180 ++++++++++++++++++++++++++
 2 files changed, 191 insertions(+)
 create mode 100644 bindings/rust/gpiosim/README.md
 create mode 100644 bindings/rust/gpiosim/src/bindings.rs
bindings/rust/gpiosim/README.md new file +11 -0
new file mode 100644
index 000000000000..acb84543dbdc
@@ -0,0 +1,11 @@
+# Generated gpiosim Rust FFI bindings
+Automatically generated Rust FFI bindings via
+	[bindgen](https://github.com/rust-lang/rust-bindgen).
+
+## Updating bindings
+1. Clone the source from
+	<https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/>
+2. run `cd libgpiod/bindings/rust/gpiosim/`
+2. run `cargo build --features generate`
+3. Copy the bindings 'cp ../target/debug/build/gpiosim-###/out/bindings.rs src/bindings.rs'
+4. Commit changes in `src/bindings.rs`
bindings/rust/gpiosim/src/bindings.rs new file +180 -0

180 lines not on this page · raw message

Signature · 1 line
2.31.1.272.g89b43f80a514
author
#5 lore raw

PATCH v8 5/9 bindings: rust: Add libgpiod crate

In reply to Viresh Kumar

Add rust wrapper crate, around the libpiod-sys crate added earlier, to provide a convenient interface for the users.

Signed-off-by Viresh Kumar <[email protected]>
---
 bindings/rust/Cargo.toml                     |   1 +
 bindings/rust/libgpiod/Cargo.toml            |  20 +
 bindings/rust/libgpiod/src/chip.rs           | 317 ++++++++++++
 bindings/rust/libgpiod/src/edge_event.rs     | 128 +++++
 bindings/rust/libgpiod/src/event_buffer.rs   | 102 ++++
 bindings/rust/libgpiod/src/info_event.rs     |  69 +++
 bindings/rust/libgpiod/src/lib.rs            | 478 +++++++++++++++++++
 bindings/rust/libgpiod/src/line_config.rs    | 135 ++++++
 bindings/rust/libgpiod/src/line_info.rs      | 162 +++++++
 bindings/rust/libgpiod/src/line_request.rs   | 224 +++++++++
 bindings/rust/libgpiod/src/line_settings.rs  | 297 ++++++++++++
 bindings/rust/libgpiod/src/request_config.rs |  95 ++++
 12 files changed, 2028 insertions(+)
 create mode 100644 bindings/rust/libgpiod/Cargo.toml
 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
bindings/rust/Cargo.toml +1 -0
index b9eea6b3a5ea..4fdf4e06ff90 100644
@@ -2,5 +2,6 @@
 
 members = [
     "gpiosim",
+    "libgpiod",
     "libgpiod-sys"
 ]
bindings/rust/libgpiod/Cargo.toml new file +20 -0
new file mode 100644
index 000000000000..ef52fdc198d7
@@ -0,0 +1,20 @@
+[package]
+name = "libgpiod"
+version = "0.1.0"
+authors = ["Viresh Kumar <[email protected]>"]
+description = "libgpiod wrappers"
+repository = "https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git"
+categories = ["command-line-utilities", "os::linux-apis"]
+rust-version = "1.56"
+keywords = ["libgpiod", "gpio"]
+license = "Apache-2.0 OR BSD-3-Clause"
+edition = "2021"
+
+# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
+
+[dependencies]
+errno = "0.2.8"
+intmap = "2.0.0"
+libc = "0.2.39"
+libgpiod-sys = { path = "../libgpiod-sys" }
+thiserror = "1.0"
bindings/rust/libgpiod/src/chip.rs new file +317 -0

317 lines not on this page · raw message

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

128 lines not on this page · raw message

bindings/rust/libgpiod/src/event_buffer.rs new file +102 -0

102 lines not on this page · raw message

bindings/rust/libgpiod/src/info_event.rs new file +69 -0

69 lines not on this page · raw message

bindings/rust/libgpiod/src/lib.rs new file +478 -0

478 lines not on this page · raw message

bindings/rust/libgpiod/src/line_config.rs new file +135 -0

135 lines not on this page · raw message

bindings/rust/libgpiod/src/line_info.rs new file +162 -0

162 lines not on this page · raw message

bindings/rust/libgpiod/src/line_request.rs new file +224 -0

224 lines not on this page · raw message

bindings/rust/libgpiod/src/line_settings.rs new file +297 -0

297 lines not on this page · raw message

bindings/rust/libgpiod/src/request_config.rs new file +95 -0

95 lines not on this page · raw message

Signature · 1 line
2.31.1.272.g89b43f80a514
#6 lore raw

Re: [libgpiod v2][PATCH V8 5/9] bindings: rust: Add libgpiod crate

Quoted 47 lines · Viresh Kumar

Add rust wrapper crate, around the libpiod-sys crate added earlier, to provide a convenient interface for the users.

Signed-off-by Viresh Kumar <[email protected]>
---
 bindings/rust/Cargo.toml                     |   1 +
 bindings/rust/libgpiod/Cargo.toml            |  20 +
 bindings/rust/libgpiod/src/chip.rs           | 317 ++++++++++++
 bindings/rust/libgpiod/src/edge_event.rs     | 128 +++++
 bindings/rust/libgpiod/src/event_buffer.rs   | 102 ++++
 bindings/rust/libgpiod/src/info_event.rs     |  69 +++
 bindings/rust/libgpiod/src/lib.rs            | 478 +++++++++++++++++++
 bindings/rust/libgpiod/src/line_config.rs    | 135 ++++++
 bindings/rust/libgpiod/src/line_info.rs      | 162 +++++++
 bindings/rust/libgpiod/src/line_request.rs   | 224 +++++++++
 bindings/rust/libgpiod/src/line_settings.rs  | 297 ++++++++++++
 bindings/rust/libgpiod/src/request_config.rs |  95 ++++
 12 files changed, 2028 insertions(+)
 create mode 100644 bindings/rust/libgpiod/Cargo.toml
 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
bindings/rust/Cargo.toml +1 -0
index b9eea6b3a5ea..4fdf4e06ff90 100644
@@ -2,5 +2,6 @@
 
 members = [
     "gpiosim",
+    "libgpiod",
     "libgpiod-sys"
 ]
bindings/rust/libgpiod/Cargo.toml new file +0 -0
new file mode 100644
index 000000000000..ef52fdc198d7

bindings/rust/libgpiod/Cargo.toml line 7

Quoted 7 lines
bindings/rust/libgpiod/Cargo.toml new file +7 -0
@@ -0,0 +1,20 @@
+[package]
+name = "libgpiod"
+version = "0.1.0"
+authors = ["Viresh Kumar <[email protected]>"]
+description = "libgpiod wrappers"
+repository = "https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git"
+categories = ["command-line-utilities", "os::linux-apis"]

The command line utilities being the examples?
That is a bit of a stretch.

How about "api-bindings", and maybe "hardware-support" and "embedded"?

bindings/rust/libgpiod/Cargo.toml line 14

Quoted 7 lines
bindings/rust/libgpiod/Cargo.toml new file +7 -0
+rust-version = "1.56"
+keywords = ["libgpiod", "gpio"]
+license = "Apache-2.0 OR BSD-3-Clause"
+edition = "2021"
+
+# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
+

That link is put there by cargo for YOU.
When you are satisfied with your keys you can delete it.

< --8<-- >

Quoted 7 lines
bindings/rust/libgpiod/Cargo.toml new file +7 -0
+    /// Get the file descriptor associated with the chip.
+    ///
+    /// The returned file descriptor must not be closed by the caller, else other methods for the
+    /// `struct Chip` may fail.
+    pub fn fd(&self) -> Result<u32> {
+        // SAFETY: `gpiod_chip` is guaranteed to be valid here.
+        let fd = unsafe { gpiod::gpiod_chip_get_fd(self.ichip.chip) };

bindings/rust/libgpiod/Cargo.toml

Quoted 10 lines
bindings/rust/libgpiod/Cargo.toml new file +10 -0
+
+        if fd < 0 {
+            Err(Error::OperationFailed(
+                OperationType::ChipGetFd,
+                errno::errno(),
+            ))
+        } else {
+            Ok(fd as u32)
+        }
+    }

Impl AsRawFd, as per Request.

< --8<-- >

Quoted 4 lines
bindings/rust/libgpiod/Cargo.toml new file +4 -0
+/// Line info
+///
+/// Exposes functions for retrieving kernel information about both requested and
+/// free lines.  Line info object contains an immutable snapshot of a line's status.

bindings/rust/libgpiod/Cargo.toml

Quoted 10 lines
bindings/rust/libgpiod/Cargo.toml new file +10 -0
+///
+/// The line info contains all the publicly available information about a
+/// line, which does not include the line value.  The line must be requested
+/// to access the line value.
+
+#[derive(Debug, Eq, PartialEq)]
+pub struct Info {
+    info: *mut gpiod::gpiod_line_info,
+    from_event: bool,
+}

The "from_event" flag indicates if the info needs to be freed, or not, when the Info is dropped, so call it something that indicates that, like "contained".

Admittedly I'm only skimming this one compared to my v7 review, but only minor knits that could always be picked up later.

Cheers,
Kent.

author
#7 lore raw

Re: [libgpiod v2][PATCH V8 5/9] bindings: rust: Add libgpiod crate

Quoted 18 lines · Kent Gibson
Quoted 7 lines
bindings/rust/libgpiod/src/chip.rs new file +7 -0
+    /// Get the file descriptor associated with the chip.
+    ///
+    /// The returned file descriptor must not be closed by the caller, else other methods for the
+    /// `struct Chip` may fail.
+    pub fn fd(&self) -> Result<u32> {
+        // SAFETY: `gpiod_chip` is guaranteed to be valid here.
+        let fd = unsafe { gpiod::gpiod_chip_get_fd(self.ichip.chip) };

bindings/rust/libgpiod/src/chip.rs line 153

Quoted 10 lines
bindings/rust/libgpiod/src/chip.rs new file +10 -0
+
+        if fd < 0 {
+            Err(Error::OperationFailed(
+                OperationType::ChipGetFd,
+                errno::errno(),
+            ))
+        } else {
+            Ok(fd as u32)
+        }
+    }
Quoted 1 line

Impl AsRawFd, as per Request.

You suggested it for line request earlier and I fixed that.

My bad for not updating this one, I completely missed that we have fd implementation here too :(

Signature · 1 line
viresh
author
#8 lore raw

PATCH v8 6/9 bindings: rust: Add examples to libgpiod crate

In reply to Viresh Kumar

Add examples for the usage of the rust bindings, quite similar to the ones in cxx bindings.

Signed-off-by Viresh Kumar <[email protected]>
---
 .../rust/libgpiod/examples/gpio_events.rs     |  88 ++++++++++++
 .../examples/gpio_threaded_info_events.rs     | 133 ++++++++++++++++++
 bindings/rust/libgpiod/examples/gpiodetect.rs |  31 ++++
 bindings/rust/libgpiod/examples/gpiofind.rs   |  37 +++++
 bindings/rust/libgpiod/examples/gpioget.rs    |  46 ++++++
 bindings/rust/libgpiod/examples/gpioinfo.rs   |  98 +++++++++++++
 bindings/rust/libgpiod/examples/gpiomon.rs    |  75 ++++++++++
 bindings/rust/libgpiod/examples/gpioset.rs    |  64 +++++++++
 bindings/rust/libgpiod/examples/gpiowatch.rs  |  54 +++++++
 9 files changed, 626 insertions(+)
 create mode 100644 bindings/rust/libgpiod/examples/gpio_events.rs
 create mode 100644 bindings/rust/libgpiod/examples/gpio_threaded_info_events.rs
 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
bindings/rust/libgpiod/examples/gpio_events.rs new file +88 -0
new file mode 100644
index 000000000000..77a7d2a5faa1
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: Apache-2.0 OR BSD-3-Clause
+//
+// Copyright 2022 Linaro Ltd. All Rights Reserved.
+//     Viresh Kumar <[email protected]>
+//
+// Simplified Rust implementation to show handling of events, when the buffer
+// is read into multiple times. Based on gpiomon example.
+
+use std::env;
+
+use libgpiod::{
+    chip::Chip,
+    line::{self, Edge, Offset},
+    request, Error, Result,
+};
+
+fn usage(name: &str) {
+    println!("Usage: {} <chip> <offset0> ...", name);
+}
+
+fn main() -> Result<()> {
+    let args: Vec<String> = env::args().collect();
+    if args.len() < 3 {
+        usage(&args[0]);
+        return Err(Error::InvalidArguments);
+    }
+
+    let mut lsettings = line::Settings::new()?;
+    let lconfig = line::Config::new()?;
+    let mut offsets = Vec::<Offset>::new();
+
+    for arg in &args[2..] {
+        let offset = arg.parse::<Offset>().map_err(|_| Error::InvalidArguments)?;
+        offsets.push(offset);
+    }
+
+    lsettings.set_edge_detection(Some(Edge::Both))?;
+    lconfig.add_line_settings(&offsets, lsettings)?;
+
+    let path = format!("/dev/gpiochip{}", args[1]);
+    let chip = Chip::open(&path)?;
+
+    let rconfig = request::Config::new()?;
+
+    let mut buffer = request::Buffer::new(1)?;
+    let request = chip.request_lines(&rconfig, &lconfig)?;
+
+    loop {
+        match request.wait_edge_event(None) {
+            Err(x) => {
+                println!("{:?}", x);
+                return Err(Error::InvalidArguments);
+            }
+
+            Ok(false) => {
+                // This shouldn't happen as the call is blocking.
+                panic!();
+            }
+            Ok(true) => (),
+        }
+
+        let count = request.read_edge_events(&mut buffer)?;
+        if count == 1 {
+            let event = buffer.event(0)?;
+            let cloned_event = request::Event::event_clone(&event)?;
+
+            // This is required before reading events again into the buffer.
+            drop(event);
+
+            let count = request.read_edge_events(&mut buffer)?;
+            if count == 1 {
+                let event = buffer.event(0)?;
+                println!(
+                    "line: {} type: {:?}, time: {:?}",
+                    cloned_event.line_offset(),
+                    cloned_event.event_type(),
+                    cloned_event.timestamp()
+                );
+                println!(
+                    "line: {} type: {:?}, time: {:?}",
+                    event.line_offset(),
+                    event.event_type(),
+                    event.timestamp()
+                );
+            }
+        }
+    }
+}
bindings/rust/libgpiod/examples/gpio_threaded_info_events.rs new file +133 -0

133 lines not on this page · raw message

bindings/rust/libgpiod/examples/gpiodetect.rs new file +31 -0
new file mode 100644
index 000000000000..e1d8f59268b5
@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: Apache-2.0 OR BSD-3-Clause
+//
+// Copyright 2022 Linaro Ltd. All Rights Reserved.
+//     Viresh Kumar <[email protected]>
+//
+// Simplified Rust implementation of gpiodetect tool.
+
+use std::env;
+use std::path::Path;
+
+use libgpiod::{self, Error, Result};
+
+fn main() -> Result<()> {
+    let args: Vec<String> = env::args().collect();
+    if args.len() > 1 {
+        println!("Usage: {}", args[0]);
+        return Err(Error::InvalidArguments);
+    }
+
+    for chip in libgpiod::gpiochip_devices(&Path::new("/dev"))? {
+        let info = chip.info()?;
+        println!(
+            "{} [{}] ({})",
+            info.name()?,
+            info.label()?,
+            info.num_lines(),
+        );
+    }
+
+    Ok(())
+}
bindings/rust/libgpiod/examples/gpiofind.rs new file +37 -0
new file mode 100644
index 000000000000..daaa93cc1bd2
@@ -0,0 +1,37 @@
+// SPDX-License-Identifier: Apache-2.0 OR BSD-3-Clause
+//
+// Copyright 2022 Linaro Ltd. All Rights Reserved.
+//     Viresh Kumar <[email protected]>
+//
+// Simplified Rust implementation of gpiofind tool.
+
+use std::env;
+use std::path::Path;
+
+use libgpiod::{self, Error, Result};
+
+fn main() -> Result<()> {
+    let args: Vec<String> = env::args().collect();
+    if args.len() != 2 {
+        println!("Usage: {} <line-name>", args[0]);
+        return Err(Error::InvalidArguments);
+    }
+
+    for chip in libgpiod::gpiochip_devices(&Path::new("/dev"))? {
+        let offset = chip.line_offset_from_name(&args[1]);
+        let info = chip.info()?;
+
+        if offset.is_ok() {
+            println!(
+                "Line {} found: Chip: {}, offset: {}",
+                args[1],
+                info.name()?,
+                offset?
+            );
+            return Ok(());
+        }
+    }
+
+    println!("Failed to find line: {}", args[1]);
+    Ok(())
+}
bindings/rust/libgpiod/examples/gpioget.rs new file +46 -0

46 lines not on this page · raw message

bindings/rust/libgpiod/examples/gpioinfo.rs new file +98 -0

98 lines not on this page · raw message

bindings/rust/libgpiod/examples/gpiomon.rs new file +75 -0

75 lines not on this page · raw message

bindings/rust/libgpiod/examples/gpioset.rs new file +64 -0

64 lines not on this page · raw message

bindings/rust/libgpiod/examples/gpiowatch.rs new file +54 -0

54 lines not on this page · raw message

Signature · 1 line
2.31.1.272.g89b43f80a514
author
#9 lore raw

PATCH v8 7/9 bindings: rust: Add tests for libgpiod crate

In reply to Viresh Kumar

Add tests for the rust bindings, quite similar to the ones in cxx bindings.

Signed-off-by Viresh Kumar <[email protected]>
---
 bindings/rust/libgpiod/Cargo.toml             |   3 +
 bindings/rust/libgpiod/tests/chip.rs          |  99 ++++
 bindings/rust/libgpiod/tests/common/config.rs | 143 +++++
 bindings/rust/libgpiod/tests/common/mod.rs    |  10 +
 bindings/rust/libgpiod/tests/edge_event.rs    | 300 +++++++++++
 bindings/rust/libgpiod/tests/info_event.rs    | 167 ++++++
 bindings/rust/libgpiod/tests/line_config.rs   |  96 ++++
 bindings/rust/libgpiod/tests/line_info.rs     | 276 ++++++++++
 bindings/rust/libgpiod/tests/line_request.rs  | 510 ++++++++++++++++++
 bindings/rust/libgpiod/tests/line_settings.rs | 204 +++++++
 .../rust/libgpiod/tests/request_config.rs     |  39 ++
 11 files changed, 1847 insertions(+)
 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
bindings/rust/libgpiod/Cargo.toml +3 -0
index ef52fdc198d7..e920eac61a34 100644
@@ -18,3 +18,6 @@ intmap = "2.0.0"
 libc = "0.2.39"
 libgpiod-sys = { path = "../libgpiod-sys" }
 thiserror = "1.0"
+
+[dev-dependencies]
+gpiosim = { path = "../gpiosim" }
bindings/rust/libgpiod/tests/chip.rs new file +99 -0

99 lines not on this page · raw message

bindings/rust/libgpiod/tests/common/config.rs new file +143 -0

143 lines not on this page · raw message

bindings/rust/libgpiod/tests/common/mod.rs new file +10 -0
new file mode 100644
index 000000000000..b795d8d8bfb1
@@ -0,0 +1,10 @@
+// SPDX-License-Identifier: Apache-2.0 OR BSD-3-Clause
+//
+// Copyright 2022 Linaro Ltd. All Rights Reserved.
+//     Viresh Kumar <[email protected]>
+
+#[allow(dead_code)]
+mod config;
+
+#[allow(unused_imports)]
+pub(crate) use config::*;
bindings/rust/libgpiod/tests/edge_event.rs new file +300 -0

300 lines not on this page · raw message

bindings/rust/libgpiod/tests/info_event.rs new file +167 -0

167 lines not on this page · raw message

bindings/rust/libgpiod/tests/line_config.rs new file +96 -0

96 lines not on this page · raw message

bindings/rust/libgpiod/tests/line_info.rs new file +276 -0

276 lines not on this page · raw message

bindings/rust/libgpiod/tests/line_request.rs new file +510 -0

510 lines not on this page · raw message

bindings/rust/libgpiod/tests/line_settings.rs new file +204 -0

204 lines not on this page · raw message

bindings/rust/libgpiod/tests/request_config.rs new file +39 -0
new file mode 100644
index 000000000000..9729fb17368b
@@ -0,0 +1,39 @@
+// SPDX-License-Identifier: Apache-2.0 OR BSD-3-Clause
+//
+// Copyright 2022 Linaro Ltd. All Rights Reserved.
+//     Viresh Kumar <[email protected]>
+
+mod common;
+
+mod request_config {
+    use libgpiod::{request, Error as ChipError, OperationType};
+
+    mod verify {
+        use super::*;
+
+        #[test]
+        fn default() {
+            let rconfig = request::Config::new().unwrap();
+
+            assert_eq!(rconfig.event_buffer_size(), 0);
+            assert_eq!(
+                rconfig.consumer().unwrap_err(),
+                ChipError::OperationFailed(
+                    OperationType::RequestConfigGetConsumer,
+                    errno::Errno(0),
+                )
+            );
+        }
+
+        #[test]
+        fn initialized() {
+            const CONSUMER: &str = "foobar";
+            let rconfig = request::Config::new().unwrap();
+            rconfig.set_consumer(CONSUMER).unwrap();
+            rconfig.set_event_buffer_size(64);
+
+            assert_eq!(rconfig.event_buffer_size(), 64);
+            assert_eq!(rconfig.consumer().unwrap(), CONSUMER);
+        }
+    }
+}
Signature · 1 line
2.31.1.272.g89b43f80a514
#10 lore raw

Re: [libgpiod v2][PATCH V8 7/9] bindings: rust: Add tests for libgpiod crate

Quoted 4 lines · Viresh Kumar

Add tests for the rust bindings, quite similar to the ones in cxx bindings.

Signed-off-by Viresh Kumar <[email protected]>
$ cargo clippy --tests
    Checking libgpiod v0.1.0 (/home/kent/work/libgpiod/bindings/rust/libgpiod)
error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing
   --> libgpiod/tests/edge_event.rs:103:13
    |
103 |             drop(event);
    |             ^^^^^^^^^^^
    |
    = note: `#[deny(clippy::drop_ref)]` on by default
note: argument has type `&libgpiod::request::Event`
   --> libgpiod/tests/edge_event.rs:103:18
    |
103 |             drop(event);
    |                  ^^^^^
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#drop_ref

error: calls to std::mem::drop with a reference instead of an owned value. Dropping a reference does nothing
--> libgpiod/tests/edge_event.rs:218:13

    |
218 |             drop(event);
    |             ^^^^^^^^^^^
    |
note: argument has type `&libgpiod::request::Event`
   --> libgpiod/tests/edge_event.rs:218:18
    |
218 |             drop(event);
    |                  ^^^^^

= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#drop_ref

error: could not compile libgpiod due to 2 previous errors

Those drops are now redundant as the events are returned by ref after patch 9.
I assume they are ignored in the build, as the tests build and run, but clippy complains.

Cheers,
Kent.

author
#11 lore raw

Re: [libgpiod v2][PATCH V8 7/9] bindings: rust: Add tests for libgpiod crate

Quoted 4 lines · Kent Gibson

Those drops are now redundant as the events are returned by ref after patch 9.
I assume they are ignored in the build, as the tests build and run, but clippy complains.

I have now added "--tests" option in my clippy's alias. I should have done it earlier, my bad.

Signature · 1 line
viresh
author
#12 lore raw

PATCH v8 8/9 bindings: rust: Integrate building of bindings with make

In reply to Viresh Kumar

Lets make build rust bindings as well.

Signed-off-by Viresh Kumar <[email protected]>
---
 README                    |  8 +++++---
 TODO                      |  8 --------
 bindings/Makefile.am      |  6 ++++++
 bindings/rust/Makefile.am | 18 ++++++++++++++++++
 configure.ac              | 16 ++++++++++++++++
 5 files changed, 45 insertions(+), 11 deletions(-)
 create mode 100644 bindings/rust/Makefile.am
README +5 -3
index 814a0f161fd2..68b5d69f9b66 100644
@@ -119,9 +119,9 @@ TOOLS
 BINDINGS
 --------
 
-High-level, object-oriented bindings for C++ and python3 are provided. They
-can be enabled by passing --enable-bindings-cxx and --enable-bindings-python
-arguments respectively to configure.
+High-level, object-oriented bindings for C++, python3 and Rust are provided.
+They can be enabled by passing --enable-bindings-cxx, --enable-bindings-python
+and --enable-bindings-rust arguments respectively to configure.
 
 C++ bindings require C++11 support and autoconf-archive collection if building
 from git.

@@ -132,6 +132,8 @@ the PYTHON_CPPFLAGS and PYTHON_LIBS variables in order to point the build
 system to the correct locations. During native builds, the configure script
 can auto-detect the location of the development files.
 
+Rust bindings require cargo support.
+
 TESTING
 -------
TODO +0 -8
index 8bb4d8f3ad56..cf4fd7b4a962 100644
@@ -28,14 +28,6 @@ and is partially functional.
 
 ----------
 
-* implement rust bindings
-
-With Rust gaining popularity as a low-level system's language and the
-possibility of it making its way into the linux kernel, it's probably time to
-provide Rust bindings to libgpiod as part of the project.
-
-----------
-
 * implement a simple daemon for controlling GPIOs in C together with a client
   program
bindings/Makefile.am +6 -0
index 8f8c762f254f..004ae23dbc58 100644
@@ -14,3 +14,9 @@ if WITH_BINDINGS_PYTHON
 SUBDIRS += python
 
 endif
+
+if WITH_BINDINGS_RUST
+
+SUBDIRS += rust
+
+endif
bindings/rust/Makefile.am new file +18 -0
new file mode 100644
index 000000000000..79a52bc691ae
@@ -0,0 +1,18 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+# SPDX-FileCopyrightText: 2022 Viresh Kumar <[email protected]>
+
+command = cargo build --release --lib
+
+if WITH_TESTS
+command += --tests
+endif
+
+if WITH_EXAMPLES
+command += --examples
+endif
+
+all:
+	$(command)
+
+clean:
+	cargo clean
configure.ac +16 -0
index 048b2ac7af8f..4a2cdb68b8b0 100644
@@ -212,6 +212,21 @@ then
 		[AC_SUBST(PYTHON_LIBS, [`$PYTHON-config --libs`])])
 fi
 
+AC_ARG_ENABLE([bindings-rust],
+	[AS_HELP_STRING([--enable-bindings-rust],[enable rust bindings [default=no]])],
+	[if test "x$enableval" = xyes; then with_bindings_rust=true; fi],
+	[with_bindings_rust=false])
+AM_CONDITIONAL([WITH_BINDINGS_RUST], [test "x$with_bindings_rust" = xtrue])
+
+if test "x$with_bindings_rust" = xtrue
+then
+	AC_CHECK_PROG([has_cargo], [cargo], [true], [false])
+	if test "x$has_cargo" = xfalse
+	then
+		AC_MSG_ERROR([cargo not found - needed for rust bindings])
+	fi
+fi
+
 AC_CHECK_PROG([has_doxygen], [doxygen], [true], [false])
 AM_CONDITIONAL([HAS_DOXYGEN], [test "x$has_doxygen" = xtrue])
 if test "x$has_doxygen" = xfalse

@@ -249,6 +264,7 @@ AC_CONFIG_FILES([Makefile
 		 bindings/python/examples/Makefile
 		 bindings/python/tests/Makefile
 		 bindings/python/tests/gpiosim/Makefile
+		 bindings/rust/Makefile
 		 man/Makefile])
 
 AC_OUTPUT
Signature · 1 line
2.31.1.272.g89b43f80a514
author
#13 lore raw

PATCH v8 9/9 bindings: rust: Implement iterator for edge events

In reply to Viresh Kumar

It would be much more convenient for the user to iterate over the events in the Rust idiomatic way (i.e. "for event in events"), instead of indexing into the vector.

This also lets us drop the hard requirement of explicitly dropping the event before reading new events from the buffer again.

Suggested-by Kent Gibson <[email protected]>
Signed-off-by Viresh Kumar <[email protected]>
---
 .../rust/libgpiod/examples/gpio_events.rs     |  47 +++----
 bindings/rust/libgpiod/examples/gpiomon.rs    |   5 +-
 bindings/rust/libgpiod/src/edge_event.rs      |  56 +++------
 bindings/rust/libgpiod/src/event_buffer.rs    | 119 ++++++++++++++----
 bindings/rust/libgpiod/src/lib.rs             |   2 +
 bindings/rust/libgpiod/src/line_request.rs    |   5 +-
 bindings/rust/libgpiod/tests/edge_event.rs    |  47 ++++---
 7 files changed, 172 insertions(+), 109 deletions(-)
bindings/rust/libgpiod/examples/gpio_events.rs +24 -23
index 77a7d2a5faa1..57dd5b4db546 100644
@@ -59,30 +59,31 @@ fn main() -> Result<()> {
             Ok(true) => (),
         }
 
-        let count = request.read_edge_events(&mut buffer)?;
-        if count == 1 {
-            let event = buffer.event(0)?;
-            let cloned_event = request::Event::event_clone(&event)?;
-
-            // This is required before reading events again into the buffer.
-            drop(event);
+        let events = request.read_edge_events(&mut buffer)?;
+        for event in events {
+            let cloned_event = request::Event::event_clone(event)?;
+            println!(
+                "line: {} type: {:?}, time: {:?}",
+                cloned_event.line_offset(),
+                cloned_event.event_type(),
+                cloned_event.timestamp()
+            );
+            println!(
+                "line: {} type: {:?}, time: {:?}",
+                event.line_offset(),
+                event.event_type(),
+                event.timestamp()
+            );
+        }
 
-            let count = request.read_edge_events(&mut buffer)?;
-            if count == 1 {
-                let event = buffer.event(0)?;
-                println!(
-                    "line: {} type: {:?}, time: {:?}",
-                    cloned_event.line_offset(),
-                    cloned_event.event_type(),
-                    cloned_event.timestamp()
-                );
-                println!(
-                    "line: {} type: {:?}, time: {:?}",
-                    event.line_offset(),
-                    event.event_type(),
-                    event.timestamp()
-                );
-            }
+        let events = request.read_edge_events(&mut buffer)?;
+        for event in events {
+            println!(
+                "line: {} type: {:?}, time: {:?}",
+                event.line_offset(),
+                event.event_type(),
+                event.timestamp()
+            );
         }
     }
 }
bindings/rust/libgpiod/examples/gpiomon.rs +2 -3
index a03ea31dfd53..4dea5d1c8dd7 100644
@@ -58,9 +58,8 @@ fn main() -> Result<()> {
             Ok(true) => (),
         }
 
-        let count = request.read_edge_events(&mut buffer)?;
-        if count == 1 {
-            let event = buffer.event(0)?;
+        let events = request.read_edge_events(&mut buffer)?;
+        for event in events {
             println!(
                 "line: {} type: {}, time: {:?}",
                 event.line_offset(),
bindings/rust/libgpiod/src/edge_event.rs +19 -37

87 lines not on this page · raw message

bindings/rust/libgpiod/src/event_buffer.rs +98 -21

164 lines not on this page · raw message

bindings/rust/libgpiod/src/lib.rs +2 -0
index 5452d47d51bc..3bcd3b97647c 100644
@@ -103,6 +103,8 @@ pub enum Error {
     OperationFailed(OperationType, errno::Errno),
     #[error("Invalid Arguments")]
     InvalidArguments,
+    #[error("Event count more than buffer capacity: {0} > {1}")]
+    TooManyEvents(usize, usize),
     #[error("Std Io Error")]
     IoError,
 }
bindings/rust/libgpiod/src/line_request.rs +4 -1
index 10d2197b876a..7101d098b6f5 100644
@@ -202,7 +202,10 @@ impl Request {
     /// Get a number of edge events from a line request.
     ///
     /// This function will block if no event was queued for the line.
-    pub fn read_edge_events(&self, buffer: &mut request::Buffer) -> Result<u32> {
+    pub fn read_edge_events<'a>(
+        &'a self,
+        buffer: &'a mut request::Buffer,
+    ) -> Result<request::Events> {
         buffer.read_edge_events(self)
     }
 }
bindings/rust/libgpiod/tests/edge_event.rs +23 -24

106 lines not on this page · raw message

Signature · 1 line
2.31.1.272.g89b43f80a514
#14 lore raw

Re: [libgpiod v2][PATCH V8 9/9] bindings: rust: Implement iterator for edge events

Quoted 9 lines · Viresh Kumar

It would be much more convenient for the user to iterate over the events in the Rust idiomatic way (i.e. "for event in events"), instead of indexing into the vector.

This also lets us drop the hard requirement of explicitly dropping the event before reading new events from the buffer again.

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

It is generally frowned upon to patch your own patch within the same series, so drop the Suggested-by and squash this into patch 5. In this instance it is very handy for reviewing though, as the other patches look pretty clean to me...

Quoted 31 lines
---
 .../rust/libgpiod/examples/gpio_events.rs     |  47 +++----
 bindings/rust/libgpiod/examples/gpiomon.rs    |   5 +-
 bindings/rust/libgpiod/src/edge_event.rs      |  56 +++------
 bindings/rust/libgpiod/src/event_buffer.rs    | 119 ++++++++++++++----
 bindings/rust/libgpiod/src/lib.rs             |   2 +
 bindings/rust/libgpiod/src/line_request.rs    |   5 +-
 bindings/rust/libgpiod/tests/edge_event.rs    |  47 ++++---
 7 files changed, 172 insertions(+), 109 deletions(-)
bindings/rust/libgpiod/examples/gpio_events.rs +6 -7
index 77a7d2a5faa1..57dd5b4db546 100644
@@ -59,30 +59,31 @@ fn main() -> Result<()> {
             Ok(true) => (),
         }
 
-        let count = request.read_edge_events(&mut buffer)?;
-        if count == 1 {
-            let event = buffer.event(0)?;
-            let cloned_event = request::Event::event_clone(&event)?;
-
-            // This is required before reading events again into the buffer.
-            drop(event);
+        let events = request.read_edge_events(&mut buffer)?;
+        for event in events {
+            let cloned_event = request::Event::event_clone(event)?;
+            println!(
+                "line: {} type: {:?}, time: {:?}",
+                cloned_event.line_offset(),

bindings/rust/libgpiod/examples/gpio_events.rs fn main() -> Result<()> { · line 77

Quoted 10 lines
bindings/rust/libgpiod/examples/gpio_events.rs +10 -0
+                cloned_event.event_type(),
+                cloned_event.timestamp()
+            );
+            println!(
+                "line: {} type: {:?}, time: {:?}",
+                event.line_offset(),
+                event.event_type(),
+                event.timestamp()
+            );
+        }

This is based on my example code, right?

So this:

        let mut events = request.read_edge_events(&mut buffer)?;
        let event = events.event(0)?;
        let cloned_event = event.event_clone()?;
        events = request.read_edge_events(&mut buffer)?;
        let event = events.event(0)?;
        println!(
            "line: {} type: {:?}, time: {:?}",
            cloned_event.line_offset(),
            cloned_event.event_type(),
            cloned_event.timestamp()
        );
        println!(
            "line: {} type: {:?}, time: {:?}",
            event.line_offset(),
            event.event_type(),
            event.timestamp()
        );

The purpose of that was to demonstrate that the cloned event can out live the original event - including across a read_edge_events(). This adaptation to the iterator misses that point.

You could use the iterator next() to return an event, rather than looping over all of them. Then do another read and get another event, as per my example code.

Might be good to add some more comments as to what is being demonstrated too, so we don't forget.

Quoted 58 lines
bindings/rust/libgpiod/examples/gpio_events.rs +8 -16
-            let count = request.read_edge_events(&mut buffer)?;
-            if count == 1 {
-                let event = buffer.event(0)?;
-                println!(
-                    "line: {} type: {:?}, time: {:?}",
-                    cloned_event.line_offset(),
-                    cloned_event.event_type(),
-                    cloned_event.timestamp()
-                );
-                println!(
-                    "line: {} type: {:?}, time: {:?}",
-                    event.line_offset(),
-                    event.event_type(),
-                    event.timestamp()
-                );
-            }
+        let events = request.read_edge_events(&mut buffer)?;
+        for event in events {
+            println!(
+                "line: {} type: {:?}, time: {:?}",
+                event.line_offset(),
+                event.event_type(),
+                event.timestamp()
+            );
         }
     }
 }
bindings/rust/libgpiod/examples/gpiomon.rs +2 -3
index a03ea31dfd53..4dea5d1c8dd7 100644
@@ -58,9 +58,8 @@ fn main() -> Result<()> {
             Ok(true) => (),
         }
 
-        let count = request.read_edge_events(&mut buffer)?;
-        if count == 1 {
-            let event = buffer.event(0)?;
+        let events = request.read_edge_events(&mut buffer)?;
+        for event in events {
             println!(
                 "line: {} type: {}, time: {:?}",
                 event.line_offset(),
bindings/rust/libgpiod/src/edge_event.rs +0 -1
index 95b05e947d26..dc71efcb2396 100644
@@ -8,7 +8,6 @@ use std::time::Duration;
 use super::{
     gpiod,
     line::{EdgeKind, Offset},
-    request::Buffer,
     Error, OperationType, Result,
 };
 

@@ -24,29 +23,33 @@ use super::{
 /// number of events are being read.

bindings/rust/libgpiod/src/edge_event.rs use super::{ · line 30

Quoted 10 lines
bindings/rust/libgpiod/src/edge_event.rs +3 -3
 
 #[derive(Debug, Eq, PartialEq)]
-pub struct Event<'b> {
-    buffer: Option<&'b Buffer>,
-    event: *mut gpiod::gpiod_edge_event,
+pub struct Event {
+    pub(crate) event: *mut gpiod::gpiod_edge_event,
+    pub(crate) cloned: bool,
 }

This can be simplified to a newtype:

pub struct Event(*mut gpiod::gpiod_edge_event);

see Buffer/Events below for more details....

Note that the cloned flag is not required. The only Events that CAN be dropped are those returned by event_clone(). The others are now returned by ref and so CANNOT be dropped - from the Rust PoV they are owned by the container that they are borrowed from.

Quoted 83 lines
bindings/rust/libgpiod/src/edge_event.rs +16 -33
-impl<'b> Event<'b> {
+impl Event {
     /// Get an event stored in the buffer.
-    pub(crate) fn new(buffer: &'b Buffer, index: usize) -> Result<Event<'b>> {
-        // SAFETY: The `gpiod_edge_event` returned by libgpiod is guaranteed to live as long
-        // as the `struct Event`.
-        let event = unsafe {
-            gpiod::gpiod_edge_event_buffer_get_event(buffer.buffer, index.try_into().unwrap())
-        };
+    pub(crate) fn new(event: *mut gpiod::gpiod_edge_event) -> Event {
+        Self {
+            event,
+            cloned: false,
+        }
+    }
+
+    pub fn event_clone(event: &Event) -> Result<Event> {
+        // 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::EdgeEventBufferGetEvent,
+                OperationType::EdgeEventCopy,
                 errno::errno(),
             ));
         }
 
         Ok(Self {
-            buffer: Some(buffer),
             event,
+            cloned: true,
         })
     }
 

@@ -95,32 +98,11 @@ impl<'b> Event<'b> {
     }
 }
 
-impl<'e, 'b> Event<'e> {
-    pub fn event_clone(event: &Event<'b>) -> Result<Event<'e>>
-    where
-        'e: 'b,
-    {
-        // 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::errno(),
-            ));
-        }
-
-        Ok(Self {
-            buffer: None,
-            event,
-        })
-    }
-}
-
-impl<'b> Drop for Event<'b> {
+impl Drop for Event {
     /// Free the edge event.
     fn drop(&mut self) {
-        // Free the event only if a copy is made
-        if self.buffer.is_none() {
+        // Only free the cloned event, others are freed with the buffer.
+        if self.cloned {
             // SAFETY: `gpiod_edge_event` is guaranteed to be valid here.
             unsafe { gpiod::gpiod_edge_event_free(self.event) };
         }
bindings/rust/libgpiod/src/event_buffer.rs +3 -1
index 16d7022034df..d1bfac54070c 100644
@@ -4,14 +4,74 @@
 //     Viresh Kumar <[email protected]>
 
 use std::os::raw::c_ulong;
+use std::ptr;
 
-use super::{gpiod, request, Error, OperationType, Result};
+use super::{
+    gpiod,

bindings/rust/libgpiod/src/event_buffer.rs line 20

Quoted 10 lines
bindings/rust/libgpiod/src/event_buffer.rs +10 -0
+    request::{Event, Request},
+    Error, OperationType, Result,
+};
+
+pub struct Events<'a> {
+    buffer: &'a mut Buffer,
+    events: Vec<&'a Event>,
+    index: usize,
+}
+

Events is pub but not documented.

The events attribute is expensive - requiring a dynamic allocation for each snapshot. And you create it unsized and push into it, which could require resizing and reallocation.
And it is unnecessary - just have Events iterate over the appropriate indicies and get the ref from the buffer.

The "index" field name is vague. Perhaps read_index?

And event_count moves here from Buffer - and can be used to terminate the iterator when read_index == event_count - rather than removing objects from events until it is empty.

Quoted 15 lines
bindings/rust/libgpiod/src/event_buffer.rs +15 -0
+impl<'a> Events<'a> {
+    pub fn new(buffer: &'a mut Buffer, len: usize) -> Self {
+        let mut events = Vec::new();
+
+        for i in 0..len {
+            // SAFETY: We could have simply pushed the reference of the event to the events vector
+            // here, but that causes a build error as buffer is borrowed as both mutable and
+            // immutable. Instead of a reference, perform unsafe pointer magic to do the same
+            // thing. It is safe as the events buffer will always outlive Events and the reference
+            // will always be valid.
+            events.push(unsafe {
+                (buffer.events.as_ptr().add(i) as *const Event)
+                    .as_ref()
+                    .unwrap()
+            });

bindings/rust/libgpiod/src/event_buffer.rs line 45

Quoted 10 lines
bindings/rust/libgpiod/src/event_buffer.rs +10 -0
+        }
+
+        Self {
+            buffer,
+            events,
+            index: 0,
+        }
+    }
+
+    /// Get the number of events the buffer contains.

Mentioning "buffer" is confusing.
Make it "Get the number of contained events."

Does this change as events are read by the iterator, so the number left, or is it locked to the size of the snapshot?

Quoted 8 lines
bindings/rust/libgpiod/src/event_buffer.rs +8 -0
+    pub fn len(&self) -> usize {
+        self.events.len()
+    }
+
+    /// Check if buffer is empty.
+    pub fn is_empty(&self) -> bool {
+        self.events.is_empty()
+    }

bindings/rust/libgpiod/src/event_buffer.rs line 63

Quoted 10 lines
bindings/rust/libgpiod/src/event_buffer.rs +10 -0
+}
+
+impl<'a> Iterator for Events<'a> {
+    type Item = &'a Event;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        if self.events.is_empty() || self.buffer.update_event(self.index).is_err() {
+            return None;
+        }
+

So an error reading an event will quietly terminate the iterator?

bindings/rust/libgpiod/src/event_buffer.rs line 68

Quoted 5 lines
bindings/rust/libgpiod/src/event_buffer.rs +4 -0
+        self.index += 1;
+        Some(self.events.remove(0))
+    }
+}

Still provide a get by index, so the caller has the option to use the iterator, or to iterate over indicies?

Else why provide the len()?

I'm fine either way, but it seems a bit odd to me as it stands.

bindings/rust/libgpiod/src/event_buffer.rs line 76

Quoted 9 lines
bindings/rust/libgpiod/src/event_buffer.rs +2 -1
 /// Line edge events buffer
 #[derive(Debug, Eq, PartialEq)]
 pub struct Buffer {
     pub(crate) buffer: *mut gpiod::gpiod_edge_event_buffer,
-    event_count: usize,
+    events: Vec<Event>,
+    capacity: usize,
 }

This can be simplified to:

pub struct Buffer {
buffer: *mut gpiod::gpiod_edge_event_buffer,
events: Vec<*mut gpiod::gpiod_edge_event>,
}

Storing a raw pointer here, rather than Event, so Buffer is simply the container for the raw C objects. The raw pointer can readily be converted into an Event ref, as the Event is now just a newtype based on that raw pointer.

The capacity is redundant - events.len(). The event_count should be part of the struct Events.

Quoted 79 lines
bindings/rust/libgpiod/src/event_buffer.rs +28 -19
 impl Buffer {

@@ -30,22 +90,32 @@ impl Buffer {
             ));
         }
 
+        // SAFETY: `gpiod_edge_event_buffer` is guaranteed to be valid here.
+        let capacity = unsafe { gpiod::gpiod_edge_event_buffer_get_capacity(buffer) as usize };
+        let mut events = Vec::new();
+        events.resize_with(capacity, || Event::new(ptr::null_mut()));
+
         Ok(Self {
             buffer,
-            event_count: 0,
+            events,
+            capacity,
         })
     }
 
-    /// Get a number of edge events from a line request.
+    /// Get edge events from a line request.
     ///
     /// This function will block if no event was queued for the line.
-    pub fn read_edge_events(&mut self, request: &request::Request) -> Result<u32> {
+    pub fn read_edge_events(&mut self, request: &Request) -> Result<Events> {
+        for i in 0..self.events.len() {
+            self.events[i].event = ptr::null_mut();
+        }
+
         // SAFETY: `gpiod_line_request` is guaranteed to be valid here.
         let ret = unsafe {
             gpiod::gpiod_line_request_read_edge_event(
                 request.request,
                 self.buffer,
-                self.capacity().try_into().unwrap(),
+                self.capacity.try_into().unwrap(),
             )
         };
 

@@ -55,30 +125,37 @@ impl Buffer {
                 errno::errno(),
             ))
         } else {
-            // Set count of events read in the buffer
-            self.set_event_count(ret as usize);
-            Ok(ret as u32)
-        }
-    }
+            let ret = ret as usize;
 
-    /// Set the number of events read into the event buffer.
-    pub(crate) fn set_event_count(&mut self, count: usize) {
-        self.event_count = count
+            if ret > self.capacity {
+                Err(Error::TooManyEvents(ret, self.capacity))
+            } else {
+                Ok(Events::new(self, ret))
+            }
+        }
     }
 
     /// Get the capacity of the event buffer.
     pub fn capacity(&self) -> usize {
-        // SAFETY: `gpiod_edge_event_buffer` is guaranteed to be valid here.
-        unsafe { gpiod::gpiod_edge_event_buffer_get_capacity(self.buffer) as usize }
+        self.capacity
     }
 
     /// Read an event stored in the buffer.
-    pub fn event(&self, index: usize) -> Result<request::Event> {
-        if index >= self.event_count {
-            Err(Error::InvalidArguments)
-        } else {
-            request::Event::new(self, index)
+    fn update_event(&mut self, index: usize) -> Result<()> {
+        // SAFETY: The `gpiod_edge_event` returned by libgpiod is guaranteed to live as long
+        // as the `struct Event`.
+        let event = unsafe {
+            gpiod::gpiod_edge_event_buffer_get_event(self.buffer, index.try_into().unwrap())
+        };

bindings/rust/libgpiod/src/event_buffer.rs impl Buffer { · line 159

Quoted 10 lines
bindings/rust/libgpiod/src/event_buffer.rs +8 -0
+        if event.is_null() {
+            return Err(Error::OperationFailed(
+                OperationType::EdgeEventBufferGetEvent,
+                errno::errno(),
+            ));
         }
+
+        self.events[index].event = event;
+        Ok(())
     }

Change update_event() to:

fn event(&self, index: usize) -> Result<&Event>

and Events should use that to get the ref for the event. (the iterator will then have to return Option<Result<&Event>>, so some additional unwrapping, but it doesn't hide errors from the caller.)

I've updated my mods branch[1] with some of those changes, if that is any clearer. Sorry, that is still based on v7, so no Events iterator etc, but hopefully the description above is sufficient.
If not let me know and I'll try to rebase it to v8.

Cheers,
Kent.

[1] https://github.com/warthog618/libgpiod/commit/d77d8b2a89ad4fbfde2070f827aa0dc007f1a92f

author
#15 lore raw

Re: [libgpiod v2][PATCH V8 9/9] bindings: rust: Implement iterator for edge events

Quoted 4 lines · Kent Gibson

It is generally frowned upon to patch your own patch within the same series, so drop the Suggested-by and squash this into patch 5. In this instance it is very handy for reviewing though, as the other patches look pretty clean to me...

I kept it separately for exactly this reason, I was planning to merge it eventually with the other commits.

Quoted 9 lines

bindings/rust/libgpiod/src/event_buffer.rs line 20

Quoted 6 lines
bindings/rust/libgpiod/src/event_buffer.rs +6 -0
+pub struct Events<'a> {
+    buffer: &'a mut Buffer,
+    events: Vec<&'a Event>,
+    index: usize,
+}
+

Events is pub but not documented.

Quoted 5 lines

The events attribute is expensive - requiring a dynamic allocation for each snapshot. And you create it unsized and push into it, which could require resizing and reallocation.
And it is unnecessary - just have Events iterate over the appropriate indicies and get the ref from the buffer.

I had to do it this way as I wasn't able to get around an error I think. I updated the code now based on suggestions, in my v9 branch, and I get it again:

error: lifetime may not live long enough
--> libgpiod/src/event_buffer.rs:51:9

   |
45 | impl<'a> Iterator for Events<'a> {
   |      -- lifetime `'a` defined here

...
48 | fn next(&mut self) -> Option<Self::Item> {
| - let's call the lifetime of this reference '1
...

51 |         event
   |         ^^^^^ associated function was supposed to return data with lifetime `'a` but it is returning data with lifetime `'1`

error: could not compile libgpiod due to previous error

I will try to fix this on Monday now, unless you have an answer for me by then :)

Thanks.

Signature · 1 line
viresh
#16 lore raw

Re: [libgpiod v2][PATCH V8 9/9] bindings: rust: Implement iterator for edge events

Quoted 36 lines · Viresh Kumar
Quoted 4 lines · Kent Gibson

It is generally frowned upon to patch your own patch within the same series, so drop the Suggested-by and squash this into patch 5. In this instance it is very handy for reviewing though, as the other patches look pretty clean to me...

I kept it separately for exactly this reason, I was planning to merge it eventually with the other commits.

Quoted 9 lines

bindings/rust/libgpiod/src/event_buffer.rs line 20

Quoted 6 lines
bindings/rust/libgpiod/src/event_buffer.rs +6 -0
+pub struct Events<'a> {
+    buffer: &'a mut Buffer,
+    events: Vec<&'a Event>,
+    index: usize,
+}
+

Events is pub but not documented.

Quoted 5 lines

The events attribute is expensive - requiring a dynamic allocation for each snapshot. And you create it unsized and push into it, which could require resizing and reallocation.
And it is unnecessary - just have Events iterate over the appropriate indicies and get the ref from the buffer.

I had to do it this way as I wasn't able to get around an error I think. I updated the code now based on suggestions, in my v9 branch, and I get it again:

error: lifetime may not live long enough
--> libgpiod/src/event_buffer.rs:51:9

   |
45 | impl<'a> Iterator for Events<'a> {
   |      -- lifetime `'a` defined here

...
48 | fn next(&mut self) -> Option<Self::Item> {
| - let's call the lifetime of this reference '1
...

Quoted 7 lines
51 |         event
   |         ^^^^^ associated function was supposed to return data with lifetime `'a` but it is returning data with lifetime `'1`

error: could not compile libgpiod due to previous error

I will try to fix this on Monday now, unless you have an answer for me by then :)

     /// Read an event stored in the buffer.
-    fn event(&mut self, index: usize) -> Result<&Event> {
+    fn event<'a>(&mut self, index: usize) -> Result<&'a Event> {
         if self.events[index].is_null() {
             // SAFETY: The `gpiod_edge_event` returned by libgpiod is guaranteed to live as long
             // as the `struct Event`.

There are still other things that need to be fixed after that, but that fixes that particular problem.

Cheers,
Kent.

#17 lore raw

Re: [libgpiod v2][PATCH V8 9/9] bindings: rust: Implement iterator for edge events

Quoted 53 lines · Kent Gibson
Quoted 36 lines · Viresh Kumar
Quoted 4 lines · Kent Gibson

It is generally frowned upon to patch your own patch within the same series, so drop the Suggested-by and squash this into patch 5. In this instance it is very handy for reviewing though, as the other patches look pretty clean to me...

I kept it separately for exactly this reason, I was planning to merge it eventually with the other commits.

Quoted 9 lines

bindings/rust/libgpiod/src/event_buffer.rs line 20

Quoted 6 lines
bindings/rust/libgpiod/src/event_buffer.rs +6 -0
+pub struct Events<'a> {
+    buffer: &'a mut Buffer,
+    events: Vec<&'a Event>,
+    index: usize,
+}
+

Events is pub but not documented.

Quoted 5 lines

The events attribute is expensive - requiring a dynamic allocation for each snapshot. And you create it unsized and push into it, which could require resizing and reallocation.
And it is unnecessary - just have Events iterate over the appropriate indicies and get the ref from the buffer.

I had to do it this way as I wasn't able to get around an error I think. I updated the code now based on suggestions, in my v9 branch, and I get it again:

error: lifetime may not live long enough
--> libgpiod/src/event_buffer.rs:51:9

   |
45 | impl<'a> Iterator for Events<'a> {
   |      -- lifetime `'a` defined here

...
48 | fn next(&mut self) -> Option<Self::Item> {
| - let's call the lifetime of this reference '1
...

Quoted 7 lines
51 |         event
   |         ^^^^^ associated function was supposed to return data with lifetime `'a` but it is returning data with lifetime `'1`

error: could not compile libgpiod due to previous error

I will try to fix this on Monday now, unless you have an answer for me by then :)

     /// Read an event stored in the buffer.
(quoted diff) +1 -1
-    fn event(&mut self, index: usize) -> Result<&Event> {
+    fn event<'a>(&mut self, index: usize) -> Result<&'a Event> {
         if self.events[index].is_null() {
             // SAFETY: The `gpiod_edge_event` returned by libgpiod is guaranteed to live as long
             // as the `struct Event`.
Quoted 3 lines

There are still other things that need to be fixed after that, but that fixes that particular problem.

I fixed up the other things (handling the iterator returning a Result, by either unwrapping or flattening as appropriate) in my rust_v9 branch[1]. Also changed Event to a newtype, to remove any possibility that the struct form could have different memory layout or alignment than the underlying raw pointer.

That works for me. Note that the flatten will skip over errors, so might not be what you want in practice, but my primary interest was to get everything compiling and the tests passing.

Cheers,
Kent.

[1]https://github.com/warthog618/libgpiod/tree/rust_v9

author
#18 lore raw

Re: [libgpiod v2][PATCH V8 9/9] bindings: rust: Implement iterator for edge events

In reply to Kent Gibson

Review

Quoted 3 lines · Kent Gibson
     /// Read an event stored in the buffer.
(quoted diff) +1 -1
-    fn event(&mut self, index: usize) -> Result<&Event> {
+    fn event<'a>(&mut self, index: usize) -> Result<&'a Event> {

I tried that earlier, but then I also modified self as "&'a mut self".

Quoted 3 lines
         if self.events[index].is_null() {
             // SAFETY: The `gpiod_edge_event` returned by libgpiod is guaranteed to live as long
             // as the `struct Event`.

Thanks.

Signature · 1 line
viresh