libgpiod v2: C++ bindings
Subsystems (2)
cover libgpiod v2: C++ bindings
This series contains the implementation of the C++ bindings for libgpiod v2. In general the C++ library follows the data structure model as defined by the C library with one notable exception: objects that represent immutable snapshots of kernel data (line_info and edge & info events) are copyable (or rather shared behind the scenes using ::std::shared_ptr). The rest of the classes delete their copy constructors and assignment operators and are only move constructible and move assignable.
All classes follow the pimpl idiom - using either shared_ptr or unique_ptr - and all implementations are hidden from the user for easier maintenance and less ABI breakage in the future.
The edge_event class is a bit of a special case. While it looks the same as other copyable objects to the user, the implementation uses a tiny bit of polymorphism (although it never crosses the ABI boundary). This is done to make it possible to use the edge_event_buffer without any memory allocations like what the C API enables. The edge_event objects stored in the buffer only contain a raw pointer to the C object stored in the underlying C edge_event_buffer. The event is copied into a fully managed object once the copy assignment operator is called.
I'm Cc'ing people who showed interest and helped me with C++ bindings before for review.
Bartosz Golaszewski (4):
events: hide the *_read_fd() symbols from the API header
API: drop "peek" functions
API: add an AS_IS value for bias setting
bindings: cxx: implement C++ bindings for libgpiod v2.0
Doxyfile.in | 4 +- bindings/cxx/Makefile.am | 16 +- bindings/cxx/chip.cpp | 213 +++-- bindings/cxx/edge-event-buffer.cpp | 103 +++ bindings/cxx/edge-event.cpp | 123 +++ bindings/cxx/examples/Makefile.am | 12 +- bindings/cxx/examples/gpiodetectcxx.cpp | 3 +- bindings/cxx/examples/gpiogetcxx.cpp | 12 +- bindings/cxx/examples/gpioinfocxx.cpp | 63 +- bindings/cxx/examples/gpiomoncxx.cpp | 38 +- bindings/cxx/examples/gpiosetcxx.cpp | 19 +- bindings/cxx/gpiod.hpp | 938 +------------------- bindings/cxx/gpiodcxx/Makefile.am | 14 + bindings/cxx/gpiodcxx/chip.hpp | 172 ++++ bindings/cxx/gpiodcxx/edge-event-buffer.hpp | 115 +++ bindings/cxx/gpiodcxx/edge-event.hpp | 124 +++ bindings/cxx/gpiodcxx/info-event.hpp | 107 +++ bindings/cxx/gpiodcxx/line-config.hpp | 242 +++++ bindings/cxx/gpiodcxx/line-info.hpp | 205 +++++ bindings/cxx/gpiodcxx/line-request.hpp | 170 ++++ bindings/cxx/gpiodcxx/misc.hpp | 49 + bindings/cxx/gpiodcxx/request-config.hpp | 95 ++ bindings/cxx/info-event.cpp | 89 ++ bindings/cxx/internal.hpp | 168 +++- bindings/cxx/iter.cpp | 60 -- bindings/cxx/line-config.cpp | 222 +++++ bindings/cxx/line-info.cpp | 150 ++++ bindings/cxx/line-request.cpp | 161 ++++ bindings/cxx/line.cpp | 321 ------- bindings/cxx/line_bulk.cpp | 366 -------- bindings/cxx/misc.cpp | 18 + bindings/cxx/request-config.cpp | 80 ++ configure.ac | 1 + include/gpiod.h | 79 +- lib/edge-event.c | 39 +- lib/info-event.c | 10 +- lib/internal.h | 4 +- lib/line-config.c | 1 + lib/line-info.c | 24 +- tools/gpiomon.c | 4 +- 40 files changed, 2701 insertions(+), 1933 deletions(-) create mode 100644 bindings/cxx/edge-event-buffer.cpp create mode 100644 bindings/cxx/edge-event.cpp create mode 100644 bindings/cxx/gpiodcxx/Makefile.am create mode 100644 bindings/cxx/gpiodcxx/chip.hpp create mode 100644 bindings/cxx/gpiodcxx/edge-event-buffer.hpp create mode 100644 bindings/cxx/gpiodcxx/edge-event.hpp create mode 100644 bindings/cxx/gpiodcxx/info-event.hpp create mode 100644 bindings/cxx/gpiodcxx/line-config.hpp create mode 100644 bindings/cxx/gpiodcxx/line-info.hpp create mode 100644 bindings/cxx/gpiodcxx/line-request.hpp create mode 100644 bindings/cxx/gpiodcxx/misc.hpp create mode 100644 bindings/cxx/gpiodcxx/request-config.hpp create mode 100644 bindings/cxx/info-event.cpp delete mode 100644 bindings/cxx/iter.cpp create mode 100644 bindings/cxx/line-config.cpp create mode 100644 bindings/cxx/line-info.cpp create mode 100644 bindings/cxx/line-request.cpp delete mode 100644 bindings/cxx/line.cpp delete mode 100644 bindings/cxx/line_bulk.cpp create mode 100644 bindings/cxx/misc.cpp create mode 100644 bindings/cxx/request-config.cpp
Signature · 1 line
2.30.1
PATCH 1/4 events: hide the *_read_fd() symbols from the API header
While exposing file descriptors for polling (for use by event loops) is a common pattern, allowing to read data directly from file descriptors isn't really a thing in most linux libraries. Sane event loops allow to associate data with polled file descriptors so users know which object to use for receiving data when it's available.
--- include/gpiod.h | 24 ------------------------ lib/edge-event.c | 5 ++--- lib/info-event.c | 2 +- lib/internal.h | 3 +++ 4 files changed, 6 insertions(+), 28 deletions(-)
include/gpiod.h +0 -24
@@ -410,14 +410,6 @@ gpiod_info_event_peek_line_info(struct gpiod_info_event *event);struct gpiod_line_info *gpiod_info_event_get_line_info (struct gpiod_info_event *event); -/** -* @brief Read the line info event from a file descriptor. -* @param fd File descriptor to read from. -* @return New info event object or NULL on error. The returned object must be -* freed using ::gpiod_info_event_free. -*/ -struct gpiod_info_event *gpiod_info_event_read_fd (int fd); -/** * @} *
@@ -1036,22 +1028,6 @@ gpiod_edge_event_buffer_get_event(struct gpiod_edge_event_buffer *buffer,unsigned int gpiod_edge_event_buffer_num_events (struct gpiod_edge_event_buffer *buffer); -/** -* @brief Read GPIO edge events directly from a file descriptor. -* @param fd File descriptor. -* @param buffer Line event buffer. -* @param max_events Maximum number of events to read. -* @return On success returns the number of events read from the file -* descriptor, on failure return -1. -* -* Users who directly poll the file descriptor for incoming events can also -* directly read the event data from it using this routine. This function -* translates the kernel representation of the event to the libgpiod format. -*/ -int gpiod_edge_event_buffer_read_fd (int fd, -struct gpiod_edge_event_buffer *buffer, -unsigned int max_events); -/** * @} *
lib/edge-event.c +2 -3
@@ -146,9 +146,8 @@ gpiod_edge_event_buffer_num_events(struct gpiod_edge_event_buffer *buffer)return buffer->num_events; } -GPIOD_API int -gpiod_edge_event_buffer_read_fd (int fd,struct gpiod_edge_event_buffer *buffer, -unsigned int max_events) +int gpiod_edge_event_buffer_read_fd (int fd,struct gpiod_edge_event_buffer *buffer, +unsigned int max_events) {struct gpio_v2_line_event *curr;struct gpiod_edge_event *event;
lib/info-event.c +1 -1
@@ -86,7 +86,7 @@ gpiod_info_event_get_line_info(struct gpiod_info_event *event)return gpiod_line_info_copy (event->info); } -GPIOD_API struct gpiod_info_event *gpiod_info_event_read_fd(int fd) +struct gpiod_info_event *gpiod_info_event_read_fd (int fd) {struct gpio_v2_line_info_changed evbuf;ssize_t rd;
lib/internal.h +3 -0
@@ -25,8 +25,11 @@ int gpiod_line_config_to_kernel(struct gpiod_line_config *config, const unsigned int *offsets);struct gpiod_line_request *gpiod_line_request_from_kernel (struct gpio_v2_line_request *reqbuf); +int gpiod_edge_event_buffer_read_fd (int fd,struct gpiod_edge_event_buffer *buffer, +unsigned int max_events);struct gpiod_info_event *gpiod_info_event_from_kernel (struct gpio_v2_line_info_changed *evbuf); +struct gpiod_info_event *gpiod_info_event_read_fd (int fd);int gpiod_poll_fd (int fd,uint64_t timeout);
Signature · 1 line
2.30.1
PATCH 2/4 API: drop "peek" functions
In reply to Bartosz GolaszewskiWhen I took the new API for a ride writing the C++ bindings, it turned out that Kent was right all along and the split into get and peek functions isn't really useful, while copy functions operating on copied objects are what we need. This removes the peek functions in favor of "get" functions everywhere and simply documents the lifetime of each object returned by said get functions. It also adds copy functions for objects where it makes sense - line_info and edge_event as they can be stored in other objects (info_event and edge_event_buffer respectively) and the user may want to decouple their lifetime from the parent.
This is done because in real-life situations, the user of the C API will most likely only access the edge_event to inspect its properties while it's still stored in the buffer and only on rare occasions will there be a need for a deep copy.
For C++ bindings this is useful because we want to make it possible to copy the edge_event object but also not require new allocations for every received object for better performance.
--- include/gpiod.h | 51 +++++++++++++++++------------------------------- lib/edge-event.c | 34 ++++++++++++-------------------- lib/info-event.c | 8 +------- lib/internal.h | 1 - lib/line-info.c | 24 +++++++++++------------ tools/gpiomon.c | 4 ++-- 6 files changed, 46 insertions(+), 76 deletions(-)
include/gpiod.h +18 -33
@@ -38,11 +38,6 @@ extern "C" {* users pass valid pointers to objects as arguments. An exception to this rule * are the functions that free/close/release resources - which work when passed * a NULL-pointer as argument. Other exceptions are documented. -* -* <p>When a function returns a pointer to an opaque, libgpiod-specific object, -* the caller is tasked with its lifetime management except where explicitly -* documented. Returned const char * pointers must not be freed by the caller -* as they are managed by the containing objects. */ struct gpiod_chip ;
@@ -249,6 +244,14 @@ enum {*/ void gpiod_line_info_free (struct gpiod_line_info *info); +/** +* @brief Copy the line info object. +* @param info Line info to copy. +* @return Copy of the line info or NULL on error. The returned object must +* be freed by the caller using :gpiod_line_info_free. +*/ +struct gpiod_line_info *gpiod_line_info_copy (struct gpiod_line_info *info); +/** * @brief Get the hardware offset of the line. * @param info GPIO line info object.
@@ -397,17 +400,6 @@ uint64_t gpiod_info_event_get_timestamp(struct gpiod_info_event *event);* the caller. */ struct gpiod_line_info * -gpiod_info_event_peek_line_info (struct gpiod_info_event *event); - -/** -* @brief Get a copy of the line-info object associated with this event. -* @param event Line info event object. -* @return Returns a copy of the line-info object associated with this event or -* NULL on error. The lifetime of the returned object must be managed -* by the caller and the line-info object must be freed using -* ::gpiod_line_info_free. -*/ -struct gpiod_line_info *gpiod_info_event_get_line_info (struct gpiod_info_event *event);/**
@@ -936,11 +928,17 @@ enum {/** * @brief Free the edge event object. * @param event Edge event object to free. -* @note Must not be used with events stored in the buffer and retrieved using -* ::gpiod_edge_event_buffer_peek_event. */ void gpiod_edge_event_free (struct gpiod_edge_event *event); +/** +* @brief Copy the edge event object. +* @param event Edge event to copy. +* @return Copy of the edge event or NULL on error. The returned object must +* be freed by the caller using :gpiod_edge_event_free. +*/ +struct gpiod_edge_event *gpiod_edge_event_copy (struct gpiod_edge_event *event); +/** * @brief Get the event type. * @param event GPIO edge event.
@@ -1000,21 +998,8 @@ void gpiod_edge_event_buffer_free(struct gpiod_edge_event_buffer *buffer);* @param buffer Line event buffer. * @param index Index of the event in the buffer. * @return Pointer to an event stored in the buffer. The lifetime of this -* event is tied to the buffer object. Users must not call event -* reference counting functions on pointers retrieved using this -* function. -*/ -struct gpiod_edge_event * -gpiod_edge_event_buffer_peek_event (struct gpiod_edge_event_buffer *buffer, -unsigned long index); - -/** -* @brief Get a copy of an edge event. -* @param buffer Line event buffer. -* @param index Index of the event in the buffer. -* @return Returns a copy of the edge event stored in this buffer. The event's -* lifetime is managed by the caller. The event must be released using -* ::gpiod_edge_event_free. +* event is tied to the buffer object. Users must not free the event +* returned by this function. */ struct gpiod_edge_event * gpiod_edge_event_buffer_get_event(struct gpiod_edge_event_buffer *buffer,
lib/edge-event.c +13 -21
@@ -35,6 +35,18 @@ GPIOD_API void gpiod_edge_event_free(struct gpiod_edge_event *event)free (event); } +GPIOD_API struct gpiod_edge_event * +gpiod_edge_event_copy (struct gpiod_edge_event *event) +{ +struct gpiod_edge_event *copy; + + copy =malloc (sizeof (*event)); +if (!copy) +return NULL ; + +return copy; +} + GPIOD_API int gpiod_edge_event_get_event_type(struct gpiod_edge_event *event) {return event->event_type;
@@ -108,36 +120,16 @@ gpiod_edge_event_buffer_free(struct gpiod_edge_event_buffer *buffer)free (buffer); } -GPIOD_API struct gpiod_edge_event * -gpiod_edge_event_buffer_peek_event (struct gpiod_edge_event_buffer *buffer, -unsigned long index) -{ -if (index >= buffer->num_events) { - errno =EINVAL ; -return NULL ; - } - -return &buffer->events[index]; -} - GPIOD_API struct gpiod_edge_event *gpiod_edge_event_buffer_get_event (struct gpiod_edge_event_buffer *buffer,unsigned long index) { -struct gpiod_edge_event *event; -if (index >= buffer->num_events) { errno =EINVAL ;return NULL ; } - event =malloc (sizeof (*event)); -if (!event) -return NULL ; - -memcpy (event, &buffer->events[index],sizeof (*event)); - -return event; +return &buffer->events[index]; }GPIOD_API unsigned int
lib/info-event.c +1 -7
@@ -74,16 +74,10 @@ gpiod_info_event_get_timestamp(struct gpiod_info_event *event)return event->timestamp; } -GPIOD_API struct gpiod_line_info * -gpiod_info_event_peek_line_info (struct gpiod_info_event *event) -{ -return event->info; -} - GPIOD_API struct gpiod_line_info *gpiod_info_event_get_line_info (struct gpiod_info_event *event) { -return gpiod_line_info_copy (event->info); +return event->info; }struct gpiod_info_event *gpiod_info_event_read_fd (int fd)
lib/internal.h +0 -1
@@ -16,7 +16,6 @@struct gpiod_line_info *gpiod_line_info_from_kernel (struct gpio_v2_line_info *infobuf); -struct gpiod_line_info *gpiod_line_info_copy (struct gpiod_line_info *info);int gpiod_request_config_to_kernel (struct gpiod_request_config *config,struct gpio_v2_line_request *reqbuf); int gpiod_line_config_to_kernel(struct gpiod_line_config *config,
lib/line-info.c +12 -12
@@ -29,6 +29,18 @@ GPIOD_API void gpiod_line_info_free(struct gpiod_line_info *info)free (info); } +GPIOD_API struct gpiod_line_info * +gpiod_line_info_copy (struct gpiod_line_info *info) +{ +struct gpiod_line_info *copy; + + copy =malloc (sizeof (*info)); +if (!copy) +return NULL ; + +return copy; +} + GPIOD_API unsigned int gpiod_line_info_get_offset(struct gpiod_line_info *info) {return info->offset;
@@ -153,15 +165,3 @@ gpiod_line_info_from_kernel(struct gpio_v2_line_info *infobuf)return info; } - -struct gpiod_line_info *gpiod_line_info_copy (struct gpiod_line_info *info) -{ -struct gpiod_line_info *new_info; - - new_info =malloc (sizeof (*new_info)); -if (!new_info) -return NULL ; - -memcpy (new_info, info,sizeof (*new_info)); -return new_info; -}
tools/gpiomon.c +2 -2
@@ -291,8 +291,8 @@ int main(int argc, char **argv)die_perror ("error reading line events" );for (i =0 ; i < ret; i++) { - event =gpiod_edge_event_buffer_peek_event (event_buffer, - i); + event =gpiod_edge_event_buffer_get_event (event_buffer, + i);if (!event)die_perror ("unable to retrieve the event from the buffer" );
Signature · 1 line
2.30.1
PATCH 3/4 API: add an AS_IS value for bias setting
In reply to Bartosz GolaszewskiWe don't have an explicit default "as-is" value for the bias setting. This adds it.
--- include/gpiod.h | 4 +++- lib/line-config.c | 1 + 2 files changed, 4 insertions(+), 1 deletion(-)
include/gpiod.h +3 -1
@@ -451,7 +451,9 @@ enum {* @brief Available internal bias settings for line requests. */ enum { -GPIOD_LINE_CONFIG_BIAS_DISABLED =1 , +GPIOD_LINE_CONFIG_BIAS_AS_IS =1 , +/**< Don't change the current bias setting. */ +GPIOD_LINE_CONFIG_BIAS_DISABLED ,/**< The internal bias should be disabled (the default). */ GPIOD_LINE_CONFIG_BIAS_PULL_UP ,/**< The internal pull-up bias is enabled. */
lib/line-config.c +1 -0
@@ -496,6 +496,7 @@ static int gpiod_make_kernel_flags(uint64_t *flags, struct base_config *config)case GPIOD_LINE_CONFIG_BIAS_PULL_DOWN : *flags |=GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN ;break ; +case GPIOD_LINE_CONFIG_BIAS_AS_IS :case 0 :break ;default :
Signature · 1 line
2.30.1
PATCH 4/4 bindings: cxx: implement C++ bindings for libgpiod v2.0
In reply to Bartosz GolaszewskiThis is the bulk of work implementing C++ bindings for the new libgpiod API. The tests are not converted yet but the examples are fully functional. More details in the cover letter as this patch will be squashed with the one for the core C library anyway.
--- Doxyfile.in | 4 +- bindings/cxx/Makefile.am | 16 +- bindings/cxx/chip.cpp | 213 +++-- bindings/cxx/edge-event-buffer.cpp | 103 +++ bindings/cxx/edge-event.cpp | 123 +++ bindings/cxx/examples/Makefile.am | 12 +- bindings/cxx/examples/gpiodetectcxx.cpp | 3 +- bindings/cxx/examples/gpiogetcxx.cpp | 12 +- bindings/cxx/examples/gpioinfocxx.cpp | 63 +- bindings/cxx/examples/gpiomoncxx.cpp | 38 +- bindings/cxx/examples/gpiosetcxx.cpp | 19 +- bindings/cxx/gpiod.hpp | 938 +------------------- bindings/cxx/gpiodcxx/Makefile.am | 14 + bindings/cxx/gpiodcxx/chip.hpp | 172 ++++ bindings/cxx/gpiodcxx/edge-event-buffer.hpp | 115 +++ bindings/cxx/gpiodcxx/edge-event.hpp | 124 +++ bindings/cxx/gpiodcxx/info-event.hpp | 107 +++ bindings/cxx/gpiodcxx/line-config.hpp | 242 +++++ bindings/cxx/gpiodcxx/line-info.hpp | 205 +++++ bindings/cxx/gpiodcxx/line-request.hpp | 170 ++++ bindings/cxx/gpiodcxx/misc.hpp | 49 + bindings/cxx/gpiodcxx/request-config.hpp | 95 ++ bindings/cxx/info-event.cpp | 89 ++ bindings/cxx/internal.hpp | 168 +++- bindings/cxx/iter.cpp | 60 -- bindings/cxx/line-config.cpp | 222 +++++ bindings/cxx/line-info.cpp | 150 ++++ bindings/cxx/line-request.cpp | 161 ++++ bindings/cxx/line.cpp | 321 ------- bindings/cxx/line_bulk.cpp | 366 -------- bindings/cxx/misc.cpp | 18 + bindings/cxx/request-config.cpp | 80 ++ configure.ac | 1 + 33 files changed, 2645 insertions(+), 1828 deletions(-) create mode 100644 bindings/cxx/edge-event-buffer.cpp create mode 100644 bindings/cxx/edge-event.cpp create mode 100644 bindings/cxx/gpiodcxx/Makefile.am create mode 100644 bindings/cxx/gpiodcxx/chip.hpp create mode 100644 bindings/cxx/gpiodcxx/edge-event-buffer.hpp create mode 100644 bindings/cxx/gpiodcxx/edge-event.hpp create mode 100644 bindings/cxx/gpiodcxx/info-event.hpp create mode 100644 bindings/cxx/gpiodcxx/line-config.hpp create mode 100644 bindings/cxx/gpiodcxx/line-info.hpp create mode 100644 bindings/cxx/gpiodcxx/line-request.hpp create mode 100644 bindings/cxx/gpiodcxx/misc.hpp create mode 100644 bindings/cxx/gpiodcxx/request-config.hpp create mode 100644 bindings/cxx/info-event.cpp delete mode 100644 bindings/cxx/iter.cpp create mode 100644 bindings/cxx/line-config.cpp create mode 100644 bindings/cxx/line-info.cpp create mode 100644 bindings/cxx/line-request.cpp delete mode 100644 bindings/cxx/line.cpp delete mode 100644 bindings/cxx/line_bulk.cpp create mode 100644 bindings/cxx/misc.cpp create mode 100644 bindings/cxx/request-config.cpp
Doxyfile.in +3 -1
@@ -44,7 +44,9 @@ WARNINGS = YES WARN_IF_UNDOCUMENTED = YES WARN_FORMAT = WARN_LOGFILE = -INPUT = @top_srcdir@/include/gpiod.h @top_srcdir@/bindings/cxx/gpiod.hpp +INPUT = @top_srcdir@/include/gpiod.h \ + @top_srcdir@/bindings/cxx/gpiod.hpp \ + @top_srcdir@/bindings/cxx/gpiodcxx/ SOURCE_BROWSER = YES INLINE_SOURCES = NO REFERENCED_BY_RELATION = YES
bindings/cxx/Makefile.am +13 -3
@@ -2,8 +2,18 @@ # SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <[email protected]> lib_LTLIBRARIES = libgpiodcxx.la -libgpiodcxx_la_SOURCES = chip.cpp internal.h iter.cpp line.cpp line_bulk.cpp -libgpiodcxx_la_CPPFLAGS = -Wall -Wextra -g -std=gnu++11 +libgpiodcxx_la_SOURCES = \ + chip.cpp \ + edge-event-buffer.cpp \ + edge-event.cpp \ + info-event.cpp \ + internal.hpp \ + line-config.cpp \ + line-info.cpp \ + line-request.cpp \ + misc.cpp \ + request-config.cpp +libgpiodcxx_la_CPPFLAGS = -Wall -Wextra -g -std=gnu++17 libgpiodcxx_la_CPPFLAGS += -fvisibility=hidden -I$(top_srcdir)/include/ libgpiodcxx_la_LDFLAGS = -version-info $(subst .,:,$(ABI_CXX_VERSION)) libgpiodcxx_la_LDFLAGS += -lgpiod -L$(top_builddir)/lib
@@ -13,7 +23,7 @@ include_HEADERS = gpiod.hpp pkgconfigdir = $(libdir)/pkgconfig pkgconfig_DATA = libgpiodcxx.pc -SUBDIRS = . +SUBDIRS = gpiodcxx . if WITH_TESTS
bindings/cxx/chip.cpp +140 -73
296 lines not on this page · raw message
bindings/cxx/edge-event-buffer.cpp new file +103 -0
103 lines not on this page · raw message
bindings/cxx/edge-event.cpp new file +123 -0
123 lines not on this page · raw message
bindings/cxx/examples/Makefile.am +6 -6
@@ -5,12 +5,12 @@ AM_CPPFLAGS = -I$(top_srcdir)/bindings/cxx/ -I$(top_srcdir)/include AM_CPPFLAGS += -Wall -Wextra -g -std=gnu++17 AM_LDFLAGS = -lgpiodcxx -L$(top_builddir)/bindings/cxx/ -lstdc++fs -noinst_PROGRAMS = \ - gpiodetectcxx \ - gpiofindcxx \ - gpiogetcxx \ - gpioinfocxx \ - gpiomoncxx \ +noinst_PROGRAMS = \ + gpiodetectcxx \ + gpiofindcxx \ + gpiogetcxx \ + gpioinfocxx \ + gpiomoncxx \ gpiosetcxx gpiodetectcxx_SOURCES = gpiodetectcxx.cpp
bindings/cxx/examples/gpiodetectcxx.cpp +1 -2
@@ -3,10 +3,9 @@ /* C++ reimplementation of the gpiodetect tool. */ -#include <gpiod.hpp> - #include <cstdlib> #include <filesystem> +#include <gpiod.hpp> #include <iostream> int main(int argc, char **argv)
bindings/cxx/examples/gpiogetcxx.cpp +3 -9
@@ -15,21 +15,15 @@ int main(int argc, char **argv) return EXIT_FAILURE; } - ::std::vector<unsigned int> offsets; + ::gpiod::line_offsets offsets; for (int i = 2; i < argc; i++) offsets.push_back(::std::stoul(argv[i])); ::gpiod::chip chip(argv[1]); - auto lines = chip.get_lines(offsets); + auto request = chip.request_lines(::gpiod::request_config(offsets, "gpiogetcxx")); - lines.request({ - argv[0], - ::gpiod::line_request::DIRECTION_INPUT, - 0 - }); - - auto vals = lines.get_values(); + auto vals = request.get_values(offsets); for (auto& it: vals) ::std::cout << it << ' ';
bindings/cxx/examples/gpioinfocxx.cpp +36 -27
78 lines not on this page · raw message
bindings/cxx/examples/gpiomoncxx.cpp +18 -20
@@ -3,29 +3,27 @@ /* Simplified C++ reimplementation of the gpiomon tool. */ -#include <gpiod.hpp> - +#include <chrono> #include <cstdlib> +#include <gpiod.hpp> #include <iostream> namespace { -void print_event(const ::gpiod::line_event& event) +void print_event(const ::gpiod::edge_event& event) { - if (event.event_type == ::gpiod::line_event::RISING_EDGE) + if (event.event_type() == ::gpiod::edge_event::RISING_EDGE) ::std::cout << " RISING EDGE"; - else if (event.event_type == ::gpiod::line_event::FALLING_EDGE) - ::std::cout << "FALLING EDGE"; else - throw ::std::logic_error("invalid event type"); + ::std::cout << "FALLING EDGE"; ::std::cout << " "; - ::std::cout << ::std::chrono::duration_cast<::std::chrono::seconds>(event.timestamp).count(); + ::std::cout << event.timestamp() / 1000000000; ::std::cout << "."; - ::std::cout << event.timestamp.count() % 1000000000; + ::std::cout << event.timestamp() % 1000000000; - ::std::cout << " line: " << event.source.offset(); + ::std::cout << " line: " << event.line_offset(); ::std::cout << ::std::endl; }
@@ -45,19 +43,19 @@ int main(int argc, char **argv) offsets.push_back(::std::stoul(argv[i])); ::gpiod::chip chip(argv[1]); - auto lines = chip.get_lines(offsets); + auto request = chip.request_lines(::gpiod::request_config(offsets, "gpiomoncxx"), + ::gpiod::line_config(::gpiod::line_config::DIRECTION_INPUT, + ::gpiod::line_config::no_output_values, + ::gpiod::line_config::EDGE_BOTH)); - lines.request({ - argv[0], - ::gpiod::line_request::EVENT_BOTH_EDGES, - 0, - }); + ::gpiod::edge_event_buffer buffer; for (;;) { - auto events = lines.event_wait(::std::chrono::seconds(1)); - if (events) { - for (auto& it: events) - print_event(it.event_read()); + if (request.edge_event_wait(::std::chrono::seconds(5))) { + request.edge_event_read(buffer); + + for (const auto& event: buffer) + print_event(event); } }
bindings/cxx/examples/gpiosetcxx.cpp +9 -10
@@ -11,12 +11,13 @@ int main(int argc, char **argv) { if (argc < 3) { - ::std::cerr << "usage: " << argv[0] << " <chip> <line_offset0>=<value0> ..." << ::std::endl; + ::std::cerr << "usage: " << argv[0] << + " <chip> <line_offset0>=<value0> ..." << ::std::endl; return EXIT_FAILURE; } - ::std::vector<unsigned int> offsets; - ::std::vector<int> values; + ::gpiod::line_offsets offsets; + ::gpiod::line_values values; for (int i = 2; i < argc; i++) { ::std::string arg(argv[i]);
@@ -27,20 +28,18 @@ int main(int argc, char **argv) ::std::string value(arg.substr(pos + 1, ::std::string::npos)); if (offset.empty() || value.empty()) - throw ::std::invalid_argument("invalid argument: " + ::std::string(argv[i])); + throw ::std::invalid_argument("invalid offset=value mapping: " + + ::std::string(argv[i])); offsets.push_back(::std::stoul(offset)); values.push_back(::std::stoul(value)); } ::gpiod::chip chip(argv[1]); - auto lines = chip.get_lines(offsets); + auto request = chip.request_lines(::gpiod::request_config(offsets, "gpiosetcxx"), + ::gpiod::line_config(::gpiod::line_config::DIRECTION_OUTPUT)); - lines.request({ - argv[0], - ::gpiod::line_request::DIRECTION_OUTPUT, - 0 - }, values); + request.set_values(offsets, values); ::std::cin.get();
bindings/cxx/gpiod.hpp +12 -926
952 lines not on this page · raw message
bindings/cxx/gpiodcxx/Makefile.am new file +14 -0
@@ -0,0 +1,14 @@ +# SPDX-License-Identifier: GPL-2.0-or-later +# SPDX-FileCopyrightText: 2021 Bartosz Golaszewski <[email protected]> + +otherincludedir = $(includedir)/gpiodcxx +otherinclude_HEADERS = \ + chip.hpp \ + edge-event-buffer.hpp \ + edge-event.hpp \ + info-event.hpp \ + line-config.hpp \ + line-info.hpp \ + line-request.hpp \ + misc.hpp \ + request-config.hpp
bindings/cxx/gpiodcxx/chip.hpp new file +172 -0
172 lines not on this page · raw message
bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +115 -0
115 lines not on this page · raw message
bindings/cxx/gpiodcxx/edge-event.hpp new file +124 -0
124 lines not on this page · raw message
bindings/cxx/gpiodcxx/info-event.hpp new file +107 -0
107 lines not on this page · raw message
bindings/cxx/gpiodcxx/line-config.hpp new file +242 -0
242 lines not on this page · raw message
bindings/cxx/gpiodcxx/line-info.hpp new file +205 -0
205 lines not on this page · raw message
bindings/cxx/gpiodcxx/line-request.hpp new file +170 -0
170 lines not on this page · raw message
bindings/cxx/gpiodcxx/misc.hpp new file +49 -0
49 lines not on this page · raw message
bindings/cxx/gpiodcxx/request-config.hpp new file +95 -0
52 lines not on this page · raw message
bindings/cxx/info-event.cpp new file +89 -0
89 lines not on this page · raw message
bindings/cxx/internal.hpp +164 -4
168 lines not on this page · raw message
bindings/cxx/iter.cpp deleted +0 -60
60 lines not on this page · raw message
bindings/cxx/line-config.cpp new file +222 -0
222 lines not on this page · raw message
bindings/cxx/line-info.cpp new file +150 -0
150 lines not on this page · raw message
bindings/cxx/line-request.cpp new file +161 -0
161 lines not on this page · raw message
bindings/cxx/line.cpp deleted +0 -321
321 lines not on this page · raw message
bindings/cxx/line_bulk.cpp deleted +0 -366
366 lines not on this page · raw message
bindings/cxx/request-config.cpp new file +80 -0
80 lines not on this page · raw message
The rest of this diff is not shown. View the raw message.
Signature · 1 line
2.30.1
Part of the diff is not shown here. View the raw message.
Re: [libgpiod][PATCH 4/4] bindings: cxx: implement C++ bindings for libgpiod v2.0
Quoted 5 lines · Bartosz Golaszewski
This is the bulk of work implementing C++ bindings for the new libgpiod API. The tests are not converted yet but the examples are fully functional. More details in the cover letter as this patch will be squashed with the one for the core C library anyway.
Firstly, I don't have any problems with patches 1-3.
Secondly, I'll be focussing on the API here, but I'm not up on current C++ best practice, so take my comments, or the absence of them, with a suitable dose of salt.
A general comment - you use retrieve/read/get/return for essentially the same thing in the accessor method documentation.
Perhaps you could settle on one?
Variety may be the spice of life, but sometimes boring is better.
<snip to the gpiodcxx headers...>
Quoted 31 lines
+++ b/bindings/cxx/gpiodcxx/chip.hpp
bindings/cxx/gpiodcxx/chip.hpp new file +29
-0@@ -0,0 +1,172 @@ +/* SPDX-License-Identifier: LGPL-3.0-or-later */ +/* SPDX-FileCopyrightText: 2021 Bartosz Golaszewski <[email protected]> */ + +/** + * @file chip.hpp + */ + +#ifndef __LIBGPIOD_CXX_CHIP_HPP__ +#define __LIBGPIOD_CXX_CHIP_HPP__ + +#include <chrono> +#include <memory> +#include <string> + +namespace gpiod { + +class info_event; +class line_config; +class line_info; +class line_request; +class request_config; + +/** + * @ingroup gpiod_cxx + * @{ + */ + +/** + * @brief Represents a GPIO chip.
bindings/cxx/gpiodcxx/chip.hpp line 39
Quoted 10 lines
bindings/cxx/gpiodcxx/chip.hpp new file +10
-0+ */ +class chip +{ +public: + + /** + * @brief Default line config object. + */ + static const line_config default_line_config; +
Document what the defaults are?
Quoted 9 lines
bindings/cxx/gpiodcxx/chip.hpp new file +9
-0+ /** + * @brief Instantiates a new chip object by opening the device file + * indicated by the path argument. + * @param path Path to the device file to open. + */ + chip(const ::std::string& path); + + chip(const chip& other) = delete; +
bindings/cxx/gpiodcxx/chip.hpp line 58
Quoted 10 lines
bindings/cxx/gpiodcxx/chip.hpp new file +10
-0+ /** + * @brief Move constructor. + * @param other Object to move. + */ + chip(chip&& other) noexcept; + + ~chip(void); + + chip& operator =(const chip& other) = delete; +
Not sure if the space between the "operator" and the operator is usual for C++ now, but it looks odd to me.
I find it easier to read without the space.
bindings/cxx/gpiodcxx/chip.hpp line 71
Quoted 13 lines
bindings/cxx/gpiodcxx/chip.hpp new file +13
-0+ /** + * @brief Move assignment operator. + * @param other Object to move. + * @return Reference to self. + */ + chip& operator =(chip&& other) noexcept; + + /** + * @brief Check if this object's internal GPIO chip handle is open. + * @return + */ + explicit operator bool(void) const noexcept; +
Return doco?
And dislike converting the chip to a bool - it isn't intuitive that it should mean the underlying handle is open. Provide a suitably named method instead, say is_open() or is_closed()?
Similar comment for line_request.
Quoted 25 lines
bindings/cxx/gpiodcxx/chip.hpp new file +25
-0+ /** + * @brief Close the GPIO chip device file and free associated resources. + * @note The chip object can live after calling this method but any of + * the chip's mutators will throw a logic_error exception. + */ + void close(void); + + /** + * @brief Get the name of this GPIO chip. + * @return Reference to the string containing the chip name. + */ + const ::std::string& name(void) const; + + /** + * @brief Get the label of this GPIO chip. + * @return Reference to the string containing the label of this chip. + */ + const ::std::string& label(void) const; + + /** + * @brief Return the number of lines exposed by this chip. + * @return Number of lines. + */ + unsigned int num_lines(void) const; +
bindings/cxx/gpiodcxx/chip.hpp line 106
Quoted 10 lines
bindings/cxx/gpiodcxx/chip.hpp new file +10
-0+ /** + * @brief Retrieve the current snapshot of line information for a + * single line. + * @param offset Offset of the line to get the info for. + * @param watch Indicates whether the caller wants to watch for line + * info changes. + * @return New ::gpiod::line_info object. + */ + line_info get_line_info(unsigned int offset, bool watch = false) const; +
What is the benefit of the watch option here?
It is clearer to keep the get_line_info() and watch_line_info() distinct, as they are in the C API.
Quoted 92 lines
bindings/cxx/gpiodcxx/chip.hpp new file +66
-0+ /** + * @brief Wrapper around ::gpiod::chip::get_line_info that retrieves + * the line info and starts watching the line for changes. + * @param offset Offset of the line to get the info for. + * @return New ::gpiod::line_info object. + */ + line_info watch_line_info(unsigned int offset) const; + + /** + * @brief Stop watching the line at given offset for info events. + * @param offset Offset of the line to get the info for. + */ + void unwatch_line_info(unsigned int offset) const; + + /** + * @brief Get the file descriptor associated with this chip. + * @return File descriptor number. + */ + int get_fd(void) const; + + /** + * @brief Wait for line status events on any of the watched lines + * exposed by this chip. + * @param timeout Wait time limit in nanoseconds. + * @return True if at least one event is ready to be read. False if the + * wait timed out. + */ + bool info_event_wait(const ::std::chrono::nanoseconds& timeout) const; + + /** + * @brief Read a single line status change event from this chip. + * @return New info_event object. + */ + info_event info_event_read(void) const; + + /** + * @brief Map a GPIO line's name to its offset within the chip. + * @param name Name of the GPIO line to map. + * @return Offset of the line within the chip or -1 if the line with + * given name is not exposed by this chip. + */ + int find_line(const ::std::string& name) const; + + /** + * @brief Request a set of lines for exclusive usage. + * @param req_cfg Request config object. + * @param line_cfg Line config object. + * @return New line_request object. + */ + line_request request_lines(const request_config& req_cfg, + const line_config& line_cfg = default_line_config); + +private: + + struct impl; + + ::std::unique_ptr<impl> _m_priv; +}; + +/** + * @} + */ + +} /* namespace gpiod */ + +#endif /* __LIBGPIOD_CXX_CHIP_HPP__ */bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +20
-0@@ -0,0 +1,115 @@ +/* SPDX-License-Identifier: LGPL-3.0-or-later */ +/* SPDX-FileCopyrightText: 2021 Bartosz Golaszewski <[email protected]> */ + +/** + * @file edge-event-buffer.hpp + */ + +#ifndef __LIBGPIOD_CXX_EDGE_EVENT_BUFFER_HPP__ +#define __LIBGPIOD_CXX_EDGE_EVENT_BUFFER_HPP__ + +#include <memory> +#include <vector> + +namespace gpiod { + +class edge_event; +class line_request; + +/** + * @ingroup gpiod_cxx
bindings/cxx/gpiodcxx/edge-event-buffer.hpp line 30
Quoted 10 lines
bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +10
-0+ * @{ + */ + +/** + * @brief Object into which edge events are read for better performance. + * + * The edge_event_buffer allows to read edge_event objects into an existing + * buffer which improves the performance by avoiding needless memory + * allocations. + */
"allows to read" -> "allows reading"
Quoted 113 lines
bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +85
-0+class edge_event_buffer +{ +public: + + /** + * @brief Constant iterator for iterating over edge events stored in + * the buffer. + */ + using const_iterator = ::std::vector<edge_event>::const_iterator; + + /** + * @brief Constructor. Creates a new edge event buffer with given + * capacity. + * @param capacity Capacity of the new buffer. + */ + edge_event_buffer(unsigned int capacity = 64); + + edge_event_buffer(const edge_event_buffer& other) = delete; + + /** + * @brief Move constructor. + * @param other Object to move. + */ + edge_event_buffer(edge_event_buffer&& other) noexcept; + + ~edge_event_buffer(void); + + edge_event_buffer& operator =(const edge_event_buffer& other) = delete; + + /** + * @brief Move assignment operator. + * @param other Object to move. + * @return Reference to self. + */ + edge_event_buffer& operator =(edge_event_buffer&& other) noexcept; + + /** + * @brief Get the constant reference to the edge event at given index. + * @param index Index of the event in the buffer. + * @return Constant reference to the edge event. + */ + const edge_event& get_event(unsigned int index) const; + + /** + * @brief Get the number of edge events currently stored in the buffer. + * @return Number of edge events in the buffer. + */ + unsigned int num_events(void) const; + + /** + * @brief Maximum capacity of the buffer. + * @return Buffer capacity. + */ + unsigned int capacity(void) const noexcept; + + /** + * @brief Retrieve a constant iterator to the first edge event + * currently stored in the buffer. + * @return Constant iterator to the first element. + */ + const_iterator begin(void) const noexcept; + + /** + * @brief Retrieve a constant iterator to the element after the last + * edge event in the buffer. + * @return Constant iterator to the element after the last edge event. + */ + const_iterator end(void) const noexcept; + +private: + + struct impl; + + ::std::unique_ptr<impl> _m_priv; + + friend line_request; +}; + +/** + * @} + */ + +} /* namespace gpiod */ + +#endif /* __LIBGPIOD_CXX_EDGE_EVENT_BUFFER_HPP__ */bindings/cxx/gpiodcxx/edge-event.hpp new file +22
-0@@ -0,0 +1,124 @@ +/* SPDX-License-Identifier: LGPL-3.0-or-later */ +/* SPDX-FileCopyrightText: 2021 Bartosz Golaszewski <[email protected]> */ + +/** + * @file edge-event.hpp + */ + +#ifndef __LIBGPIOD_CXX_EDGE_EVENT_HPP__ +#define __LIBGPIOD_CXX_EDGE_EVENT_HPP__ + +#include <cstdint> +#include <memory> + +namespace gpiod { + +class edge_event_buffer; + +/** + * @ingroup gpiod_cxx + * @{ + */ +
bindings/cxx/gpiodcxx/edge-event.hpp line 32
Quoted 10 lines
bindings/cxx/gpiodcxx/edge-event.hpp new file +10
-0+/** + * @brief Immutable object containing data about a single edge event. + */ +class edge_event +{ +public: + + /** + * @brief Possible edge event types. + */
As opposed to the impossible ;)?
Elsewhere you use "Available", which is better.
Though both are probably redundant.
Quoted 320 lines
bindings/cxx/gpiodcxx/edge-event.hpp new file +92
-0+ enum : int { + RISING_EDGE = 1, + /**< This is a rising edge event. */ + FALLING_EDGE, + /**< This is falling edge event. */ + }; + + /** + * @brief Copy constructor. + * @param other Object to copy. + */ + edge_event(const edge_event& other); + + /** + * @brief Move constructor. + * @param other Object to move. + */ + edge_event(edge_event&& other) noexcept; + + /** + * @brief Destructor. + */ + ~edge_event(void); + + /** + * @brief Copy assignment operator. + * @param other Object to copy. + * @return Reference to self. + */ + edge_event& operator =(const edge_event& other); + + /** + * @brief Move assignment operator. + * @param other Object to move. + * @return Reference to self. + */ + edge_event& operator =(edge_event&& other) noexcept; + + /** + * @brief Retrieve the event type. + * @return Event type (rising or falling edge). + */ + int event_type(void) const; + + /** + * @brief Retrieve the event time-stamp. + * @return Time-stamp in nanoseconds as registered by the kernel using + * the configured edge event clock. + */ + uint64_t timestamp(void) const noexcept; + + /** + * @brief Read the offset of the line on which this event was + * registered. + * @return Line offset. + */ + unsigned int line_offset(void) const noexcept; + + /** + * @brief Get the global sequence number of this event. + * @return Sequence number of the event relative to all lines in the + * associated line request. + */ + unsigned long global_seqno(void) const noexcept; + + /** + * @brief Get the event sequence number specific to the concerned line. + * @return Sequence number of the event relative to this line within + * the lifetime of the associated line request. + */ + unsigned long line_seqno(void) const noexcept; + +private: + + edge_event(void); + + struct impl; + struct impl_managed; + struct impl_external; + + ::std::shared_ptr<impl> _m_priv; + + friend edge_event_buffer; +}; + +/** + * @} + */ + +} /* namespace gpiod */ + +#endif /* __LIBGPIOD_CXX_EDGE_EVENT_HPP__ */bindings/cxx/gpiodcxx/info-event.hpp new file +107
-0@@ -0,0 +1,107 @@ +/* SPDX-License-Identifier: LGPL-3.0-or-later */ +/* SPDX-FileCopyrightText: 2021 Bartosz Golaszewski <[email protected]> */ + +/** + * @file gpiod.h + */ + +#ifndef __LIBGPIOD_CXX_INFO_EVENT_HPP__ +#define __LIBGPIOD_CXX_INFO_EVENT_HPP__ + +#include <cstdint> +#include <memory> + +namespace gpiod { + +class chip; +class line_info; + +/** + * @ingroup gpiod_cxx + * @{ + */ + +/** + * @brief Immutable object containing data about a single line info event. + */ +class info_event +{ +public: + + /** + * @brief Possible types of info events. + */ + enum : int { + LINE_REQUESTED = 1, + /**< Line has been requested. */ + LINE_RELEASED, + /**< Previously requested line has been released. */ + LINE_CONFIG_CHANGED, + /**< Line configuration has changed. */ + }; + + /** + * @brief Copy constructor. + * @param other Object to copy. + */ + info_event(const info_event& other); + + /** + * @brief Move constructor. + * @param other Object to move. + */ + info_event(info_event&& other) noexcept; + + ~info_event(void); + + /** + * @brief Copy assignment operator. + * @param other Object to copy. + * @return Reference to self. + */ + info_event& operator =(const info_event& other); + + /** + * @brief Move assignment operator. + * @param other Object to move. + * @return Reference to self. + */ + info_event& operator =(info_event&& other) noexcept; + + /** + * @brief Type of this event. + * @return Event type. + */ + int event_type(void) const; + + /** + * @brief Timestamp of the event as returned by the kernel. + * @return Timestamp as a 64-bit unsigned integer. + */ + uint64_t timestamp(void) const noexcept; + + /** + * @brief Get the new line information. + * @return Constant reference to the line info object containing the + * line data as read at the time of the info event. + */ + const line_info& get_line_info(void) const noexcept; + +private: + + info_event(void); + + struct impl; + + ::std::shared_ptr<impl> _m_priv; + + friend chip; +}; + +/** + * @} + */ + +} /* namespace gpiod */ + +#endif /* __LIBGPIOD_CXX_INFO_EVENT_HPP__ */bindings/cxx/gpiodcxx/line-config.hpp new file +109
-0@@ -0,0 +1,242 @@ +/* SPDX-License-Identifier: LGPL-3.0-or-later */ +/* SPDX-FileCopyrightText: 2021 Bartosz Golaszewski <[email protected]> */ + +/** + * @file line-config.hpp + */ + +#ifndef __LIBGPIOD_CXX_LINE_CONFIG_HPP__ +#define __LIBGPIOD_CXX_LINE_CONFIG_HPP__ + +#include <memory> +#include <utility> + +#include "misc.hpp" + +namespace gpiod { + +class chip; +class line_request; + +/** + * @ingroup gpiod_cxx + * @{ + */ + +/** + * @brief Contains a set of line config options used in line requests and + * reconfiguration. + */ +class line_config +{ +public: + + /** + * @brief Available direction settings. + */ + enum : int { + DIRECTION_AS_IS = 1, + /**< Request the line(s), but don't change current direction. */ + DIRECTION_INPUT, + /**< Request the line(s) for reading the GPIO line state. */ + DIRECTION_OUTPUT + /**< Request the line(s) for setting the GPIO line state. */ + }; + + /** + * @brief Available edge event detection settings. + */ + enum : int { + EDGE_NONE = 1, + /**< Don't report edge events. */ + EDGE_FALLING, + /**< Only watch falling edge events. */ + EDGE_RISING, + /**< Only watch rising edge events. */ + EDGE_BOTH + /**< Monitor both types of events. */ + }; + + /** + * @brief Available internal bias settings for line requests. + */ + enum : int { + BIAS_AS_IS = 1, + /**< Don't change the current bias setting. */ + BIAS_DISABLED, + /**< The internal bias should be disabled (the default). */ + BIAS_PULL_UP, + /**< The internal pull-up bias is enabled. */ + BIAS_PULL_DOWN + /**< The internal pull-down bias is enabled. */ + }; + + /** + * @brief Available drive settings for line requests. + */ + enum : int { + DRIVE_PUSH_PULL = 1, + /**< Drive setting should be set to push-pull (the default). */ + DRIVE_OPEN_DRAIN, + /**< Line output should be set to open-drain. */ + DRIVE_OPEN_SOURCE + /**< Line output should be set to open-source. */ + }; + + /** + * @brief Available clock types used for event timestamps. + */ + enum : int { + EVENT_CLOCK_MONOTONIC = 1, + /**< Use the monotonic clock. */ + EVENT_CLOCK_REALTIME + /**< Use the realtime clock. */ + }; + + /** + * @brief Convenience helper value for passing an empty list of + * offset-to-value mappings to methods of this class. + */ + static const line_value_mappings no_output_values; + + /** + * @brief Constructor. + * @param direction Global direction setting. + * @param output_values Default output values. + * @param edge Global edge detection. + * @param bias Global bias setting. + * @param drive Global drive setting. + * @param active_low Global active_low setting.
bindings/cxx/gpiodcxx/line-config.hpp line 119
Quoted 10 lines
bindings/cxx/gpiodcxx/line-config.hpp new file +10
-0+ * @param debounce_period Global debounce period in microseconds. + * @param event_clock Default event clock mode. + */ + line_config(int direction = DIRECTION_INPUT, + const line_value_mappings& output_values = no_output_values, + int edge = EDGE_NONE, int bias = BIAS_AS_IS, + int drive = DRIVE_PUSH_PULL, bool active_low = false, + unsigned long debounce_period = 0, + int event_clock = EVENT_CLOCK_MONOTONIC); +
The parameter ordering should follow the probability of use? So I would make active_low earlier, before edge and output_values. And maybe output_values after edge?
Quoted 11 lines
bindings/cxx/gpiodcxx/line-config.hpp new file +11
-0+ line_config(const line_config& other) = delete; + + /** + * @brief Move constructor. + * @param other Object to move. + */ + line_config(line_config&& other) noexcept; + + ~line_config(void); + + line_config& operator =(const line_config& other) = delete;
bindings/cxx/gpiodcxx/line-config.hpp line 140
Quoted 10 lines
bindings/cxx/gpiodcxx/line-config.hpp new file +10
-0+ + /** + * @brief Move assignment operator. + * @param other Object to move. + * @return Reference to self. + */ + line_config& operator =(line_config&& other) noexcept; + + /** + * @brief Set the direction (globally or for a set of offsets).
"Set the direction, either globally or for a set of offsets."?
Quoted 45 lines
bindings/cxx/gpiodcxx/line-config.hpp new file +45
-0+ * @param direction New direction setting. + * @param offsets Vector of line offsets to set the direction for. If + * empty, the setting will be global. + */ + void set_direction(int direction, const line_offsets& offsets = line_offsets()) const; + + /** + * @brief Set the edge detection (globally or for a set of offsets). + * @param edge New edge detection setting. + * @param offsets Vector of line offsets to set the edge detection for. + * If empty, the setting will be global. + */ + void set_edge_detection(int edge, const line_offsets& offsets = line_offsets()) const; + + /** + * @brief Set the bias (globally or for a set of offsets). + * @param bias New bias setting. + * @param offsets Vector of line offsets to set the bias for. If empty, + * the setting will be global. + */ + void set_bias(int bias, const line_offsets& offsets = line_offsets()) const; + + /** + * @brief Set the drive (globally or for a set of offsets). + * @param drive New drive setting. + * @param offsets Vector of line offsets to set the drive for. If empty, + * the setting will be global. + */ + void set_drive(int drive, const line_offsets& offsets = line_offsets()) const; + + /** + * @brief Set the active setting to low (globally or for a set of + * offsets). + * @param offsets Vector of line offsets to set to active-low. If empty, + * the setting will be global. + */ + void set_active_low(const line_offsets& offsets = line_offsets()) const noexcept; + + /** + * @brief Set the active setting to high (globally or for a set of + * offsets). + * @param offsets Vector of line offsets to set to active-high. If + * empty, the setting will be global. + */ + void set_active_high(const line_offsets& offsets = line_offsets()) const noexcept;
bindings/cxx/gpiodcxx/line-config.hpp line 195
Quoted 10 lines
bindings/cxx/gpiodcxx/line-config.hpp new file +10
-0+ + /** + * @brief Set the debounce period (globally or for a set of offsets). + * @param period New debounce period setting. + * @param offsets Vector of line offsets to set the debounce period for. + * If empty, the setting will be global. + */ + void set_debounce_period(unsigned long period, + const line_offsets& offsets = line_offsets()) const noexcept; +
period == 0 means not debounced? e.g. when overridding the global setting for a particular line.
Quoted 20 lines
bindings/cxx/gpiodcxx/line-config.hpp new file +20
-0+ /** + * @brief Set the type of clock used to timestamp edge events (globally + * or for a set of offsets). + * @param clock New event clock setting. + * @param offsets Vector of line offsets to set the debounce period for. + * If empty, the setting will be global. + */ + void set_event_clock(int clock, const line_offsets& offsets = line_offsets()) const; + + /** + * @brief Set the output value for a single offset. + * @param offset Line offset to associate the value with. + * @param value New value. + */ + void set_output_value(unsigned int offset, int value) const; + + /** + * @brief Set the output values for a set of line offsets. + * @param values Vector of offset->value mappings. + */
bindings/cxx/gpiodcxx/line-config.hpp line 225
Quoted 10 lines
bindings/cxx/gpiodcxx/line-config.hpp new file +10
-0+ void set_output_values(const line_value_mappings& values) const; + + /** + * @brief Set the output values for a set of line offsets. + * @param offsets Vector of line offsets for which to set output values. + * @param values Vector of new line values with indexes of values + * corresponding to the indexes of offsets. + */ + void set_output_values(const line_offsets& offsets, const line_values& values) const; +
Painful that the user has to keep track of the offsets independent of the request. Any way to provide a method that applies values to the requested lines without explicitly providing the offsets? See related get_values() comments below...
And in general, what happens if the offsets provided aren't a subset of the requested offsets? That applies to the C API as well.
Quoted 192 lines
bindings/cxx/gpiodcxx/line-config.hpp new file +17
-0+private: + + struct impl; + + ::std::unique_ptr<impl> _m_priv; + + friend chip; + friend line_request; +}; + +/** + * @} + */ + +} /* namespace gpiod */ + +#endif /* __LIBGPIOD_CXX_LINE_CONFIG_HPP__ */bindings/cxx/gpiodcxx/line-info.hpp new file +169
-0@@ -0,0 +1,205 @@ +/* SPDX-License-Identifier: LGPL-3.0-or-later */ +/* SPDX-FileCopyrightText: 2021 Bartosz Golaszewski <[email protected]> */ + +/** + * @file line-info.hpp + */ + +#ifndef __LIBGPIOD_CXX_LINE_INFO_HPP__ +#define __LIBGPIOD_CXX_LINE_INFO_HPP__ + +#include <memory> +#include <string> + +namespace gpiod { + +class chip; +class info_event; + +/** + * @ingroup gpiod_cxx + * @{ + */ + +/** + * @brief Contains an immutable snapshot of the line's state at the + * time when the object of this class was instantiated. + */ +class line_info +{ +public: + + /** + * @brief Possible direction settings. + */ + enum : int { + DIRECTION_INPUT = 1, + /**< Direction is input - we're reading the state of a GPIO line. */ + DIRECTION_OUTPUT + /**< Direction is output - we're driving the GPIO line. */ + }; + + /** + * @brief Possible internal bias settings. + */ + enum : int { + BIAS_UNKNOWN = 1, + /**< The internal bias state is unknown. */ + BIAS_DISABLED, + /**< The internal bias is disabled. */ + BIAS_PULL_UP, + /**< The internal pull-up bias is enabled. */ + BIAS_PULL_DOWN + /**< The internal pull-down bias is enabled. */ + }; + + /** + * @brief Possible drive settings. + */ + enum : int { + DRIVE_PUSH_PULL = 1, + /**< Drive setting is push-pull. */ + DRIVE_OPEN_DRAIN, + /**< Line output is open-drain. */ + DRIVE_OPEN_SOURCE, + /**< Line output is open-source. */ + }; + + /** + * @brief Possible edge detection settings. + */ + enum : int { + EDGE_NONE = 1, + /**< Line edge detection is disabled. */ + EDGE_RISING, + /**< Line detects rising edge events. */ + EDGE_FALLING, + /**< Line detect falling edge events. */ + EDGE_BOTH + /**< Line detects both rising and falling edge events. */ + }; + + /** + * @brief Copy constructor. + * @param other Object to copy. + */ + line_info(const line_info& other) noexcept; + + /** + * @brief Move constructor. + * @param other Object to move. + */ + line_info(line_info&& other) noexcept; + + ~line_info(void); + + /** + * @brief Copy assignment operator. + * @param other Object to copy. + * @return Reference to self. + */ + line_info& operator =(const line_info& other) noexcept; + + /** + * @brief Move assignment operator. + * @param other Object to move. + * @return Reference to self. + */ + line_info& operator =(line_info&& other) noexcept; + + /** + * @brief Get the hardware offset of the line. + * @return Offset of the line within the parent chip. + */ + unsigned int offset(void) const noexcept; + + /** + * @brief Get the GPIO line name. + * @return Name of the GPIO line as it is represented in the kernel. + * This routine returns an empty string if the line is unnamed. + */ + const ::std::string& name(void) const noexcept; + + /** + * @brief Check if the line is currently in use. + * @return True if the line is in use, false otherwise. + * + * The user space can't know exactly why a line is busy. It may have + * been requested by another process or hogged by the kernel. It only + * matters that the line is used and we can't request it. + */ + bool used(void) const noexcept; + + /** + * @brief Read the GPIO line consumer name. + * @return Name of the GPIO consumer name as it is represented in the + * kernel. This routine returns an empty string if the line is + * not used. + */ + const ::std::string& consumer(void) const noexcept; + + /** + * @brief Read the GPIO line direction setting. + * @return Returns DIRECTION_INPUT or DIRECTION_OUTPUT. + */ + int direction(void) const; + + /** + * @brief Check if the signal of this line is inverted. + * @return True if this line is "active-low", false otherwise. + */ + bool active_low(void) const noexcept; + + /** + * @brief Read the GPIO line bias setting. + * @return Returns BIAS_PULL_UP, BIAS_PULL_DOWN, BIAS_DISABLE or + * BIAS_UNKNOWN. + */ + int bias(void) const; + + /** + * @brief Read the GPIO line drive setting. + * @return Returns DRIVE_PUSH_PULL, DRIVE_OPEN_DRAIN or + * DRIVE_OPEN_SOURCE. + */ + int drive(void) const; + + /** + * @brief Read the current edge detection setting of this line. + * @return Returns EDGE_NONE, EDGE_RISING, EDGE_FALLING or EDGE_BOTH.
bindings/cxx/gpiodcxx/line-info.hpp line 179
Quoted 10 lines
bindings/cxx/gpiodcxx/line-info.hpp new file +10
-0+ */ + int edge_detection(void) const; + + /** + * @brief Check if this line is debounced (either by hardware or by the + * kernel software debouncer). + * @return True if the line is debounced, false otherwise. + */ + bool debounced(void) const noexcept; +
Not sure if I had the same comment for the C API, but what is the benefit of this given it is equivalent to debounce_period() == 0?
Quoted 79 lines
bindings/cxx/gpiodcxx/line-info.hpp new file +26
-0+ /** + * @brief Read the current debounce period in microseconds. + * @return Current debounce period in microseconds, 0 if the line is + * not debounced. + */ + unsigned long debounce_period(void) const noexcept; + +private: + + line_info(void); + + struct impl; + + ::std::shared_ptr<impl> _m_priv; + + friend chip; + friend info_event; +}; + +/** + * @} + */ + +} /* namespace gpiod */ + +#endif /* __LIBGPIOD_CXX_LINE_INFO_HPP__ */bindings/cxx/gpiodcxx/line-request.hpp new file +47
-0@@ -0,0 +1,170 @@ +/* SPDX-License-Identifier: LGPL-3.0-or-later */ +/* SPDX-FileCopyrightText: 2021 Bartosz Golaszewski <[email protected]> */ + +/** + * @file line-request.hpp + */ + +#ifndef __LIBGPIOD_CXX_LINE_REQUEST_HPP__ +#define __LIBGPIOD_CXX_LINE_REQUEST_HPP__ + +#include <chrono> +#include <memory> + +#include "misc.hpp" + +namespace gpiod { + +class chip; +class edge_event; +class edge_event_buffer; +class line_config; + +/** + * @ingroup gpiod_cxx + * @{ + */ + +/** + * @brief Stores the context of a set of requested GPIO lines. + */ +class line_request +{ +public: + + line_request(const line_request& other) = delete; + + /** + * @brief Move constructor. + * @param other Object to move. + */ + line_request(line_request&& other) noexcept; + + ~line_request(void); + + line_request& operator =(const line_request& other) = delete; + + /**
bindings/cxx/gpiodcxx/line-request.hpp line 57
Quoted 10 lines
bindings/cxx/gpiodcxx/line-request.hpp new file +10
-0+ * @brief Move assignment operator. + * @param other Object to move. + */ + line_request& operator =(line_request&& other) noexcept; + + /** + * @brief Check if the object is associated with an open GPIO chip. + * @return True if the chip is open, false if it has been closed. + */ + explicit operator bool(void) const noexcept;
This is even less intuitive than for chip.
It could mean chip is open, or line is requested, or ... So make it a method with an appropriate name instead.
Quoted 14 lines
bindings/cxx/gpiodcxx/line-request.hpp new file +14
-0+ + /** + * @brief Release the GPIO chip and free all associated resources. + * @note The object can still be used after this method is called but + * using any of the mutators will result in throwing + * a logic_error exception. + */ + void release(void); + + /** + * @brief Read the value of a single requested line. + * @param offset Offset of the line to read within the chip. + * @return Current line value. + */
bindings/cxx/gpiodcxx/line-request.hpp line 81
Quoted 10 lines
bindings/cxx/gpiodcxx/line-request.hpp new file +10
-0+ int get_value(unsigned int offset) const; + + /** + * @brief Read the values of a subset of requested lines. + * @param offsets Vector of line offsets + * @return Vector of lines values with indexes of values corresponding + * to those of the offsets. + */ + line_values get_values(const line_offsets& offsets) const; +
A common use case would be to get all lines in the request. As written, to do that the user needs to maintain their own copy of the offsets.
Maybe if offsets is empty then returns all requested lines? And provide line_offsets() as a default parameter?
Same argument applies for the C API.
In gpiod.h you could rename the existing _get_values() to
int gpiod_line_request_get_values_subset(struct gpiod_line_request *request,
unsigned num_lines, const unsigned int *offsets, int *values);
and add
int gpiod_line_request_get_values(struct gpiod_line_request *request, int *values);
Or interpret a zero offsets param to mean use the requested offsets??
Quoted 14 lines
bindings/cxx/gpiodcxx/line-request.hpp new file +14
-0+ /** + * @brief Read the values of a subset of requested lines into a vector + * supplied by the caller. + * @param offsets Vector of line offsets. + * @param values Vector for storing the values. Its size must be at + * least that of the offsets vector. The indexes of read + * values will correspond with those in the offsets + * vector. + */ + void get_values(const line_offsets& offsets, line_values& values) const; + + /** + * @brief Set the value of a single requested line. + * @param offset Offset of the line to set within the chip.
bindings/cxx/gpiodcxx/line-request.hpp line 105
Quoted 10 lines
bindings/cxx/gpiodcxx/line-request.hpp new file +10
-0+ * @param value New line value. + */ + void set_value(unsigned int offset, int value) const; + + /** + * @brief Set the values of a subset of requested lines. + * @param values Vector containing a set of offset->value mappings. + */ + void set_values(const line_value_mappings& values) const; +
Same comment as for get_values.
Quoted 158 lines
bindings/cxx/gpiodcxx/line-request.hpp new file +65
-0+ /** + * @brief Set the values of a subset of requested lines. + * @param offsets Vector containing the offsets of lines to set. + * @param values Vector containing new values with indexes + * corresponding with those in the offsets vector. + */ + void set_values(const line_offsets& offsets, const line_values& values) const; + + /** + * @brief Apply new config options to requested lines. + * @param config New configuration. + */ + void reconfigure_lines(const line_config& config) const; + + /** + * @brief Retrieve the file descriptor number associated with this + * line request. + * @return File descriptor number. + */ + int get_fd(void) const; + + /** + * @brief Wait for edge events on any of the lines requested with edge + * detection enabled. + * @param timeout Wait time limit in nanoseconds. + * @return True if at least one event is ready to be read. False if the + * wait timed out. + */ + bool edge_event_wait(const ::std::chrono::nanoseconds& timeout) const; + + /** + * @brief Read a number of edge events from this request up to the + * maximum capacity of the buffer. + * @param buffer Edge event buffer to read events into. + * @return Number of events read. + */ + unsigned int edge_event_read(edge_event_buffer& buffer) const; + + /** + * @brief Read a number of edge events from this request. + * @param buffer Edge event buffer to read events into. + * @param max_events Maximum number of events to read. Limited by the + * capacity of the buffer. + * @return Number of events read. + */ + unsigned int edge_event_read(edge_event_buffer& buffer, unsigned int max_events) const; + +private: + + line_request(void); + + struct impl; + + ::std::unique_ptr<impl> _m_priv; + + friend chip; +}; + +/** + * @} + */ + +} /* namespace gpiod */ + +#endif /* __LIBGPIOD_CXX_LINE_REQUEST_HPP__ */bindings/cxx/gpiodcxx/misc.hpp new file +49
-0@@ -0,0 +1,49 @@ +/* SPDX-License-Identifier: LGPL-3.0-or-later */ +/* SPDX-FileCopyrightText: 2021 Bartosz Golaszewski <[email protected]> */ + +/** + * @file misc.hpp + */ + +#ifndef __LIBGPIOD_CXX_MISC_HPP__ +#define __LIBGPIOD_CXX_MISC_HPP__ + +#include <string> +#include <utility> +#include <vector> + +namespace gpiod { + +/** + * @brief Vector of line offsets. + */ +using line_offsets = ::std::vector<unsigned int>; + +/** + * @brief Vector of line values. + */ +using line_values = ::std::vector<int>; + +/** + * @brief Vector of offset->value mappings. Each mapping is defined as a pair + * of an unsigned and signed integers. + */ +using line_value_mappings = ::std::vector<::std::pair<unsigned int, int>>; + +/** + * @brief Check if the file pointed to by path is a GPIO chip character device. + * @param path Path to check. + * @return True if the file exists and is a GPIO chip character device or a + * symbolic link to it. + */ +bool is_gpiochip_device(const ::std::string& path); + +/** + * @brief Get the human readable version string for libgpiod API + * @return String containing the library version. + */ +::std::string version_string(void); + +} /* namespace gpiod */ + +#endif /* __LIBGPIOD_CXX_MISC_HPP__ */bindings/cxx/gpiodcxx/request-config.hpp new file +32
-0@@ -0,0 +1,95 @@ +/* SPDX-License-Identifier: LGPL-3.0-or-later */ +/* SPDX-FileCopyrightText: 2021 Bartosz Golaszewski <[email protected]> */ + +/** + * @file request-config.hpp + */ + +#ifndef __LIBGPIOD_CXX_REQUEST_CONFIG_HPP__ +#define __LIBGPIOD_CXX_REQUEST_CONFIG_HPP__ + +#include <memory> +#include <string> + +#include "misc.hpp" + +namespace gpiod { + +class chip; + +/** + * @ingroup gpiod_cxx + * @{ + */ + +/** + * @brief Stores a set of options passed to the kernel when making a line + * request. + */ +class request_config +{ +public: +
bindings/cxx/gpiodcxx/request-config.hpp line 42
Quoted 10 lines
bindings/cxx/gpiodcxx/request-config.hpp new file +10
-0+ /** + * @brief Constructor. + * @param offsets Vector of line offsets to request. + * @param consumer Consumer name. + * @param event_buffer_size Event buffer size. + */ + request_config(const line_offsets& offsets = line_offsets(), + const ::std::string& consumer = ::std::string(), + unsigned int event_buffer_size = 0); +
When would you call this without providing offsets?
Quoted 27 lines
bindings/cxx/gpiodcxx/request-config.hpp new file +27
-0+ request_config(const request_config& other) = delete; + + /** + * @brief Move constructor. + * @param other Object to move. + */ + request_config(request_config&& other) noexcept; + + ~request_config(void); + + request_config& operator =(const request_config& other) = delete; + + /** + * @brief Move assignment operator. + * @param other Object to move. + * @return Reference to self. + */ + request_config& operator =(request_config&& other) noexcept; + + /** + * @brief Set the consumer name. + * @param consumer New consumer name. + */ + void set_consumer(const ::std::string& consumer) const noexcept; + + /** + * @brief Set line offsets for this request.
bindings/cxx/gpiodcxx/request-config.hpp line 79
Quoted 10 lines
bindings/cxx/gpiodcxx/request-config.hpp new file +10
-0+ * @param offsets Vector of line offsets to request. + */ + void set_offsets(const line_offsets& offsets) const noexcept; + + /** + * @brief Set the size of the event buffer. + * @param event_buffer_size New event buffer size. + */ + void set_event_buffer_size(unsigned int event_buffer_size) const noexcept; +
Highlight that this is the kernel buffer size, as per the C API doco, and is totally unrelated to userspace edge-event-buffer capacity?
bindings/cxx/info-event.cpp
Quoted 18 lines
bindings/cxx/gpiodcxx/request-config.hpp new file +16
-0+private: + + struct impl; + + ::std::unique_ptr<impl> _m_priv; + + friend chip; +}; + +/** + * @} + */ + +} /* namespace gpiod */ + +#endif /* __LIBGPIOD_CXX_REQUEST_CONFIG_HPP__ */bindings/cxx/info-event.cpp new file +0
-0
<snip>
Cheers,
Kent.
Re: [libgpiod][PATCH 4/4] bindings: cxx: implement C++ bindings for libgpiod v2.0
Quoted 8 lines · Kent Gibson
Quoted 4 lines · Bartosz Golaszewski
This is the bulk of work implementing C++ bindings for the new libgpiod API. The tests are not converted yet but the examples are fully functional. More details in the cover letter as this patch will be squashed with the one for the core C library anyway.
Firstly, I don't have any problems with patches 1-3.
Quoted 8 lines
Secondly, I'll be focussing on the API here, but I'm not up on current C++ best practice, so take my comments, or the absence of them, with a suitable dose of salt.
A general comment - you use retrieve/read/get/return for essentially the same thing in the accessor method documentation.
Perhaps you could settle on one?
Variety may be the spice of life, but sometimes boring is better.
It's a weak point of many documentation files that I saw :-) Nevertheless, it's good practice to define terms in the preface pages to be sure everybody is on the same page with meaning of them.
Kent, do I understand correctly that Go bindings use C library?
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [libgpiod][PATCH 4/4] bindings: cxx: implement C++ bindings for libgpiod v2.0
Quoted 18 lines · Andy Shevchenko
Quoted 8 lines · Kent Gibson
Quoted 4 lines · Bartosz Golaszewski
This is the bulk of work implementing C++ bindings for the new libgpiod API. The tests are not converted yet but the examples are fully functional. More details in the cover letter as this patch will be squashed with the one for the core C library anyway.
Firstly, I don't have any problems with patches 1-3.
Quoted 8 lines
Secondly, I'll be focussing on the API here, but I'm not up on current C++ best practice, so take my comments, or the absence of them, with a suitable dose of salt.
A general comment - you use retrieve/read/get/return for essentially the same thing in the accessor method documentation.
Perhaps you could settle on one?
Variety may be the spice of life, but sometimes boring is better.
Quoted 6 lines
It's a weak point of many documentation files that I saw :-) Nevertheless, it's good practice to define terms in the preface pages to be sure everybody is on the same page with meaning of them.
Kent, do I understand correctly that Go bindings use C library?
No, the Go library is pure Go - it makes the ioctl calls itself.
Cheers,
Kent.
Re: [libgpiod][PATCH 4/4] bindings: cxx: implement C++ bindings for libgpiod v2.0
Quoted 2 lines · Kent Gibson
Quoted 1 line · Andy Shevchenko
On Sun, Jun 27, 2021 at 04:47:57PM +0800, Kent Gibson wrote:
...
Quoted 3 lines
Quoted 1 line
Kent, do I understand correctly that Go bindings use C library?
No, the Go library is pure Go - it makes the ioctl calls itself.
Hmm... Interesting. Why is it so? Performance wise? Other points?
(It means you are basically replicating what libgpiod does, right?)
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [libgpiod][PATCH 4/4] bindings: cxx: implement C++ bindings for libgpiod v2.0
Quoted 6 lines · Andy Shevchenko
Quoted 2 lines · Kent Gibson
Quoted 1 line · Andy Shevchenko
On Sun, Jun 27, 2021 at 04:47:57PM +0800, Kent Gibson wrote:
...
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
Kent, do I understand correctly that Go bindings use C library?
No, the Go library is pure Go - it makes the ioctl calls itself.
Hmm... Interesting. Why is it so? Performance wise? Other points?
(It means you are basically replicating what libgpiod does, right?)
Yeah, I ended up replicating libgpiod in Go. Which actually came in quite handy when it came to developing and testing the v2 uAPI - the first support and test cases for that were written in Go.
A bit of history...
Initially I wrote a Go GPIO library for the Raspberry Pi for a project that required better performance than was available via sysfs. This pre-dated cdev/libgpiod availability on the Raspberry Pi. That library manipulated the hardware directly via mapped memory, which at the time was the only alternative to sysfs. It is hideous and horrible, but it works, and if you can live with the downsides it is also quite performant.
Once cdev started being supported in Pi kernels I wanted to port the library to that. I did consider porting to libgpiod, but that had a few downsides:
My original library was pure Go, which is trivial to cross-compile for the Raspberry Pi (just call 'GOARCH=arm GOARM=6 go build' on my PC and I have an executable that I can run on a Pi).
If you do call C libraries from Go and want to cross-compile then you get sucked back into requiring the whole C cross-compilation toolchain. Plus ease of deployment takes a hit - you now have to consider the dependent libraries, not just the executable.
At that point you might as well just write in C.
Ease of cross-compilation and deployment was important to me (I'm lazy), so this was a major factor.
Also, I had less trouble getting my head around the kernel uAPI than I did the libgpiod API. The uAPI was a smaller and simpler target, and was accessible from Go, so I went directly to that.
(I think the libgpiod v2 API is much more approachable than v1, btw.) Anyway, neither API at the time could implement my existing Go GPIO API, so I ended up writing a new library instead - the one that replicates the core functionality of libgpiod.
Ironically, I probably could implement that old API using the v2 uAPI...
So, in short, cos it was easier.
Cheers,
Kent.
Re: [libgpiod][PATCH 4/4] bindings: cxx: implement C++ bindings for libgpiod v2.0
In reply to Kent GibsonQuoted 1 line · Kent Gibson
[snip]
Quoted 7 lines
And dislike converting the chip to a bool - it isn't intuitive that it should mean the underlying handle is open. Provide a suitably named method instead, say
is_open()oris_closed()?Similar comment for
line_request.
It's actually a common pattern in STL. I find overloaded operators more intuitive for checking the state of the wrapping object as opposed to getters for reading the state and properties of the associated GPIO chip character device stored in said object.
[snip]
Quoted 7 lines
Quoted 7 lines
bindings/cxx/gpiodcxx/chip.hpp new file +7
-0+ + /** + * @brief Retrieve the current snapshot of line information for a + * single line. + * @param offset Offset of the line to get the info for. + * @param watch Indicates whether the caller wants to watch for line + * info changes.
Quoted 9 lines
bindings/cxx/gpiodcxx/chip.hpp line 106
Quoted 4 lines
bindings/cxx/gpiodcxx/chip.hpp new file +4
-0+ * @return New ::gpiod::line_info object. + */ + line_info get_line_info(unsigned int offset, bool watch = false) const; +What is the benefit of the watch option here?
It is clearer to keep theget_line_info()andwatch_line_info()distinct, as they are in the C API.
Actually I would have used this pattern in the C API if I could overload functions or provide default values for arguments. watch_line_info(0) is just a helper for get_line_info(0, true). I would even be fine with just get_line_info() alone with a switch for watching the lines but since we already have gpiod_watch_line_info(), I decided to replicate it here.
[snip]
Quoted 15 lines
Quoted 15 lines
bindings/cxx/gpiodcxx/line-config.hpp new file +15
-0+ + /** + * @brief Constructor. + * @param direction Global direction setting. + * @param output_values Default output values. + * @param edge Global edge detection. + * @param bias Global bias setting. + * @param drive Global drive setting. + * @param active_low Global active_low setting. + * @param debounce_period Global debounce period in microseconds. + * @param event_clock Default event clock mode. + */ + line_config(int direction = DIRECTION_INPUT, + const line_value_mappings& output_values = no_output_values, + int edge = EDGE_NONE, int bias = BIAS_AS_IS,
Quoted 9 lines
bindings/cxx/gpiodcxx/line-config.hpp line 119
Quoted 4 lines
bindings/cxx/gpiodcxx/line-config.hpp new file +4
-0+ int drive = DRIVE_PUSH_PULL, bool active_low = false, + unsigned long debounce_period = 0, + int event_clock = EVENT_CLOCK_MONOTONIC); +The parameter ordering should follow the probability of use? So I would make
active_lowearlier, before edge andoutput_values. And maybeoutput_valuesafter edge?
Yeah this was my goal. active_low could come before bias and drive but after output_values IMO. edge could come earlier too (before output_values).
[snip]
Quoted 6 lines
Quoted 6 lines
bindings/cxx/gpiodcxx/line-config.hpp new file +6
-0+ + /** + * @brief Set the output values for a set of line offsets. + * @param offsets Vector of line offsets for which to set output values. + * @param values Vector of new line values with indexes of values + * corresponding to the indexes of offsets.
Quoted 9 lines
bindings/cxx/gpiodcxx/line-config.hpp line 225
Quoted 3 lines
bindings/cxx/gpiodcxx/line-config.hpp new file +3
-0+ */ + void set_output_values(const line_offsets& offsets, const line_values& values) const; +Painful that the user has to keep track of the offsets independent of the request. Any way to provide a method that applies values to the requested lines without explicitly providing the offsets? See related
get_values()comments below...
I think we already discussed this under the C API patches. The line config object can live independently from the request config. We could theoretically pass an array containing just values (with the value at given index later assigned to the offset at the same index in the request config) but that would make it less clear IMO. In most cases the user will already have an array containing the offsets ready in order to pass it to the request method. I think you mentioned previously some Go feature that allowed you to do it differently but in C it's not really possible.
Quoted 3 lines
And in general, what happens if the offsets provided aren't a subset of the requested offsets? That applies to the C API as well.
At request-time we'll return EINVAL from set_kernel_output_values() (or in the case of C++ - we'll throw a system_error exception).
[snip]
Quoted 12 lines
bindings/cxx/gpiodcxx/line-info.hpp line 179
Quoted 8 lines
bindings/cxx/gpiodcxx/line-info.hpp new file +8
-0+ + /** + * @brief Check if this line is debounced (either by hardware or by the + * kernel software debouncer). + * @return True if the line is debounced, false otherwise. + */ + bool debounced(void) const noexcept; +Not sure if I had the same comment for the C API, but what is the benefit of this given it is equivalent to
debounce_period()== 0?
Clarity of the resulting code.
if (info.debounced())
is clearer than
if (info.debounce_period())
if you're only checking if debouncing is enabled and not what the period is.
[snip]
Quoted 11 lines
bindings/cxx/gpiodcxx/line-request.hpp line 57
Quoted 6 lines
bindings/cxx/gpiodcxx/line-request.hpp new file +6
-0+ + /** + * @brief Check if the object is associated with an open GPIO chip. + * @return True if the chip is open, false if it has been closed. + */ + explicit operator bool(void) const noexcept;This is even less intuitive than for chip.
It could mean chip is open, or line is requested, or ... So make it a method with an appropriate name instead.
Well it would be much less confusing if I didn't stupidly copy-paste the comment from the chip... It was supposed to say: Check if this request hasn't been released or something along those lines.
[snip]
Quoted 21 lines
bindings/cxx/gpiodcxx/line-request.hpp line 81
Quoted 9 lines
bindings/cxx/gpiodcxx/line-request.hpp new file +9
-0+ + /** + * @brief Read the values of a subset of requested lines. + * @param offsets Vector of line offsets + * @return Vector of lines values with indexes of values corresponding + * to those of the offsets. + */ + line_values get_values(const line_offsets& offsets) const; +A common use case would be to get all lines in the request. As written, to do that the user needs to maintain their own copy of the offsets.
Maybe if offsets is empty then returns all requested lines? And provide
line_offsets()as a default parameter?Same argument applies for the C API.
Ingpiod.hyou could rename the existing_get_values()toint gpiod_line_request_get_values_subset(struct
gpiod_line_request*request,
Quoted 9 lines
unsigned num_lines, const unsigned int *offsets, int *values);and add
int
gpiod_line_request_get_values(struct gpiod_line_request *request, int *values);Or interpret a zero offsets param to mean use the requested offsets??
Hmm, I can't find it now but I could swear we discussed this previously and you raised some issue with this. I'm definitely in favor of that so let me add it in the next iteration.
[snip]
Quoted 5 lines
Quoted 5 lines
bindings/cxx/gpiodcxx/request-config.hpp new file +5
-0+ + /** + * @brief Constructor. + * @param offsets Vector of line offsets to request. + * @param consumer Consumer name.
Quoted 8 lines
bindings/cxx/gpiodcxx/request-config.hpp line 42
Quoted 6 lines
bindings/cxx/gpiodcxx/request-config.hpp new file +6
-0+ * @param event_buffer_size Event buffer size. + */ + request_config(const line_offsets& offsets = line_offsets(), + const ::std::string& consumer = ::std::string(), + unsigned int event_buffer_size = 0); +When would you call this without providing offsets?
This is to mirror the C API which allows to create an empty request config. If the user never sets the offsets, we'll catch that at request-time.
[snip]
For any issues I didn't address explicitly - I'll fix them in v2.
Bart
Re: [libgpiod][PATCH 4/4] bindings: cxx: implement C++ bindings for libgpiod v2.0
Quoted 10 lines · Bartosz Golaszewski
Quoted 1 line · Kent Gibson
[snip]
Quoted 5 lines
And dislike converting the chip to a bool - it isn't intuitive that it should mean the underlying handle is open. Provide a suitably named method instead, say
is_open()oris_closed()?
Quoted 8 lines
Quoted 2 lines
Similar comment for
line_request.It's actually a common pattern in STL. I find overloaded operators more intuitive for checking the state of the wrapping object as opposed to getters for reading the state and properties of the associated GPIO chip character device stored in said object.
I understand it for simple wrappers like the smart pointers and streams, but those are very single purpose - chips and line_requests aren't.
If you are using it as a "good to go" indicator then document it as that, rather than specifically tying to some internal state like the handle being open. Does the user even need to know about the handle - isn't that implementation detail in the pimpl?
Quoted 19 lines
[snip]
Quoted 7 lines
Quoted 7 lines
bindings/cxx/gpiodcxx/chip.hpp new file +7
-0+ + /** + * @brief Retrieve the current snapshot of line information for a + * single line. + * @param offset Offset of the line to get the info for. + * @param watch Indicates whether the caller wants to watch for line + * info changes.Quoted 9 lines
bindings/cxx/gpiodcxx/chip.hpp line 106
Quoted 4 lines
bindings/cxx/gpiodcxx/chip.hpp new file +4
-0+ * @return New ::gpiod::line_info object. + */ + line_info get_line_info(unsigned int offset, bool watch = false) const; +What is the benefit of the watch option here?
It is clearer to keep theget_line_info()andwatch_line_info()distinct, as they are in the C API.
Quoted 7 lines
Actually I would have used this pattern in the C API if I could overload functions or provide default values for arguments.
watch_line_info(0)is just a helper forget_line_info(0, true). I would even be fine with justget_line_info()alone with a switch for watching the lines but since we already havegpiod_watch_line_info(), I decided to replicate it here.
Fair enough if you have multiple parameter values, like for bias or drive, but for a bool it is frequently clearer and simpler to split it into two functions - IMHO.
Further, watch is a very uncommon use case, so I'd prefer to keep them separate - the average user doesn't even need to know that the watch capability exists.
Quoted 27 lines
[snip]
Quoted 15 lines
Quoted 15 lines
bindings/cxx/gpiodcxx/line-config.hpp new file +15
-0+ + /** + * @brief Constructor. + * @param direction Global direction setting. + * @param output_values Default output values. + * @param edge Global edge detection. + * @param bias Global bias setting. + * @param drive Global drive setting. + * @param active_low Global active_low setting. + * @param debounce_period Global debounce period in microseconds. + * @param event_clock Default event clock mode. + */ + line_config(int direction = DIRECTION_INPUT, + const line_value_mappings& output_values = no_output_values, + int edge = EDGE_NONE, int bias = BIAS_AS_IS,Quoted 9 lines
bindings/cxx/gpiodcxx/line-config.hpp line 119
Quoted 4 lines
bindings/cxx/gpiodcxx/line-config.hpp new file +4
-0+ int drive = DRIVE_PUSH_PULL, bool active_low = false, + unsigned long debounce_period = 0, + int event_clock = EVENT_CLOCK_MONOTONIC); +The parameter ordering should follow the probability of use? So I would make
active_lowearlier, before edge andoutput_values. And maybeoutput_valuesafter edge?
Quoted 4 lines
Yeah this was my goal.
active_lowcould come before bias and drive but afteroutput_valuesIMO. edge could come earlier too (beforeoutput_values).
So you suggest direction, edge, output_values, active_low, bias, drive?
Perhaps you use active low a lot less than I do :-).
I figured that, even if active low is used less frequently than output, forcing output users to provide active level was less demanding than having active level users provide empty ouputs, so I bumped it before output. Similarly edge, though I was more on the fence on that one.
Quoted 106 lines
[snip]
Quoted 6 lines
Quoted 6 lines
bindings/cxx/gpiodcxx/line-config.hpp new file +6
-0+ + /** + * @brief Set the output values for a set of line offsets. + * @param offsets Vector of line offsets for which to set output values. + * @param values Vector of new line values with indexes of values + * corresponding to the indexes of offsets.Quoted 9 lines
bindings/cxx/gpiodcxx/line-config.hpp line 225
Quoted 3 lines
bindings/cxx/gpiodcxx/line-config.hpp new file +3
-0+ */ + void set_output_values(const line_offsets& offsets, const line_values& values) const; +Painful that the user has to keep track of the offsets independent of the request. Any way to provide a method that applies values to the requested lines without explicitly providing the offsets? See related
get_values()comments below...I think we already discussed this under the C API patches. The line config object can live independently from the request config. We could theoretically pass an array containing just values (with the value at given index later assigned to the offset at the same index in the request config) but that would make it less clear IMO. In most cases the user will already have an array containing the offsets ready in order to pass it to the request method. I think you mentioned previously some Go feature that allowed you to do it differently but in C it's not really possible.
Quoted 3 lines
And in general, what happens if the offsets provided aren't a subset of the requested offsets? That applies to the C API as well.
At request-time we'll return EINVAL from
set_kernel_output_values()(or in the case of C++ - we'll throw asystem_errorexception).[snip]
Quoted 12 lines
bindings/cxx/gpiodcxx/line-info.hpp line 179
Quoted 8 lines
bindings/cxx/gpiodcxx/line-info.hpp new file +8
-0+ + /** + * @brief Check if this line is debounced (either by hardware or by the + * kernel software debouncer). + * @return True if the line is debounced, false otherwise. + */ + bool debounced(void) const noexcept; +Not sure if I had the same comment for the C API, but what is the benefit of this given it is equivalent to
debounce_period()== 0?Clarity of the resulting code.
if (
info.debounced())is clearer than
if (
info.debounce_period())if you're only checking if debouncing is enabled and not what the period is.
[snip]
Quoted 11 lines
bindings/cxx/gpiodcxx/line-request.hpp line 57
Quoted 6 lines
bindings/cxx/gpiodcxx/line-request.hpp new file +6
-0+ + /** + * @brief Check if the object is associated with an open GPIO chip. + * @return True if the chip is open, false if it has been closed. + */ + explicit operator bool(void) const noexcept;This is even less intuitive than for chip.
It could mean chip is open, or line is requested, or ... So make it a method with an appropriate name instead.Well it would be much less confusing if I didn't stupidly copy-paste the comment from the chip... It was supposed to say: Check if this request hasn't been released or something along those lines.
[snip]
Quoted 26 lines
bindings/cxx/gpiodcxx/line-request.hpp line 81
Quoted 9 lines
bindings/cxx/gpiodcxx/line-request.hpp new file +9
-0+ + /** + * @brief Read the values of a subset of requested lines. + * @param offsets Vector of line offsets + * @return Vector of lines values with indexes of values corresponding + * to those of the offsets. + */ + line_values get_values(const line_offsets& offsets) const; +A common use case would be to get all lines in the request. As written, to do that the user needs to maintain their own copy of the offsets.
Maybe if offsets is empty then returns all requested lines? And provide
line_offsets()as a default parameter?Same argument applies for the C API.
Ingpiod.hyou could rename the existing_get_values()toint gpiod_line_request_get_values_subset(struct
gpiod_line_request*request,unsigned num_lines, const unsigned int *offsets, int *values);and add
Quoted 9 lines
Quoted 4 lines
int
gpiod_line_request_get_values(struct gpiod_line_request *request, int *values);Or interpret a zero offsets param to mean use the requested offsets??
Hmm, I can't find it now but I could swear we discussed this previously and you raised some issue with this. I'm definitely in favor of that so let me add it in the next iteration.
Perhaps you are thinking of the discussion we had regarding line_config all/offset/subset variants?
I think we just forgot to apply the same logic to the get/sets here. I don't recall having any problem with it, but if anything comes to me I'll let you know.
Quoted 16 lines
[snip]
Quoted 5 lines
Quoted 5 lines
bindings/cxx/gpiodcxx/request-config.hpp new file +5
-0+ + /** + * @brief Constructor. + * @param offsets Vector of line offsets to request. + * @param consumer Consumer name.Quoted 8 lines
bindings/cxx/gpiodcxx/request-config.hpp line 42
Quoted 6 lines
bindings/cxx/gpiodcxx/request-config.hpp new file +6
-0+ * @param event_buffer_size Event buffer size. + */ + request_config(const line_offsets& offsets = line_offsets(), + const ::std::string& consumer = ::std::string(), + unsigned int event_buffer_size = 0); +When would you call this without providing offsets?
Quoted 4 lines
This is to mirror the C API which allows to create an empty request config. If the user never sets the offsets, we'll catch that at request-time.
Yeah, I get that, but when would a user ever need an empty request_config? If they never need it then why give them the option? That also applies to the C API.
Is there ever a need to mutate the offsets in the request_config? If not, just make the offsets manditory in the new/constructor? Just putting the idea out there.
All minor points, so fine whichever way you go with them.
Cheers,
Kent.