On Thu, Dec 2, 2021 at 12:23 PM Viresh Kumar viresh.kumar@linaro.org wrote:
Add rust wrappers around FFI bindings to provide a convenient interface for the users.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Hi Viresh, thanks for the hard work.
Obviously this is not something we can merge yet but it's a good base to continue the work.
General comment on the naming convention:
The line from one of the examples: 'use libgpiod::chip::GpiodChip;' looks like it has a lot of redundancy in it. How about calling the crate gpiod and dropping the chip package entirely? Basically follow what C++ and python bindings do by having `use gpiod::Chip` etc.?
[snip]
+impl GpiodChipInternal {
- /// Find a GPIO chip by path.
- pub(crate) fn open(path: &str) -> Result<Self> {
let chip = unsafe { bindings::gpiod_chip_open(path.as_ptr() as *const c_char) };if chip.is_null() {return Err(Error::OperationFailed("GpiodChip open", IoError::last()));
One of my concerns last time was error handling. Does this now translate the error codes from the underlying C code to some kind of rust errors? Can you elaborate on how that works? I see it always returns IoError. In my WIP C++ and Python code I try to translate the errnos into some meaningful exceptions - for instance EINVAL -> ::std::invalid_argument etc. Can we have a similar thing here?
[snip]
- /// Get the GPIO chip name as represented in the kernel.
- pub fn get_name(&self) -> Result<&str> {
// SAFETY: The string returned by libgpiod is guaranteed to live as long// as the `struct GpiodChip`.let name = unsafe { bindings::gpiod_chip_get_name(self.ichip.chip()) };if name.is_null() {
This is not possible, the string can be empty at the very least but never null. Same for label and path.
match ret {-1 => Err(Error::OperationFailed("GpiodChip info-event-wait",IoError::last(),)),0 => Err(Error::OperationTimedOut),
Does it have to be an Error? It's pretty normal for an operation to time out, no?
_ => Ok(()),}- }
- /// Read a single line status change event from this chip. If no events are
- /// pending, this function will block.
- pub fn info_event_read(&self) -> Result<GpiodInfoEvent> {
GpiodInfoEvent::new(&self.ichip.clone())
Would you mind explaining what the clone() function does here to ichip and why it's needed.
+impl GpiodEdgeEvent {
- /// Get an event stored in the buffer.
- pub fn new(buffer: &GpiodEdgeEventBuffer, index: u64) -> Result<Self> {
let event = unsafe { bindings::gpiod_edge_event_buffer_get_event(buffer.buffer(), index) };if event.is_null() {return Err(Error::OperationFailed("GpiodEdgeEvent buffer-get-event",IoError::last(),));}Ok(Self { event })- }
- /// Get the event type.
- pub fn get_event_type(&self) -> Result<EdgeEvent> {
EdgeEvent::new(unsafe { bindings::gpiod_edge_event_get_event_type(self.event) } as u32)- }
- /// Get the timestamp of the event.
- pub fn get_timestamp(&self) -> Duration {
Duration::from_nanos(unsafe { bindings::gpiod_edge_event_get_timestamp(self.event) })- }
- /// Get the offset of the line on which the event was triggered.
- pub fn get_line_offset(&self) -> u32 {
unsafe { bindings::gpiod_edge_event_get_line_offset(self.event) }- }
- /// Get the global sequence number of this event.
- ///
- /// Returns sequence number of the event relative to all lines in the
- /// associated line request.
- pub fn get_global_seqno(&self) -> u64 {
unsafe { bindings::gpiod_edge_event_get_global_seqno(self.event) }- }
- /// Get the event sequence number specific to concerned line.
- ///
- /// Returns sequence number of the event relative to this line within the
- /// lifetime of the associated line request.
- pub fn get_line_seqno(&self) -> u64 {
unsafe { bindings::gpiod_edge_event_get_line_seqno(self.event) }- }
+}
AFAICT this object will not survive the parent buffer. Take a look at the current development version of the C++ bindings where I play with copy constructors to ensure that.
[snip]
I'll address the config objects once we actually have the final version in the C API.
Bart