Skip to contentSkip to conversation

libgpiod v2: C++ bindings

PATCH Superseded Bartosz Golaszewski 3 participants 10 messages Jul 27, 2021 – Aug 2, 2021

Subsystems (2)

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

v3 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.

v1 -> v2:
Kent: I addressed most points from your review. Some are unaddressed due to personal preference (for instance: I still allow creating of empty line-request objects as they may be reused in subsequent requests). I also kept the 'watch' argument in get_line_info() as well as the boolean operators for chip and request - although with (hopefully) better documentation.

v2 -> v3:
- use scoped class enums instead of regular integer ones
- rename getters to get_<property>() in order to avoid name conflicts with the new enum types

The first two patches extend the core C API of libgpiod as discussed.

Bartosz Golaszewski (3):
API: add a function for retrieving the capacity of edge event buffers
API: extend the line request functionality
bindings: cxx: implement C++ bindings for libgpiod v2.0

 Doxyfile.in                                 |   4 +-
 bindings/cxx/Makefile.am                    |  16 +-
 bindings/cxx/chip.cpp                       | 214 +++--
 bindings/cxx/edge-event-buffer.cpp          | 103 +++
 bindings/cxx/edge-event.cpp                 | 123 +++
 bindings/cxx/examples/Makefile.am           |  12 +-
 bindings/cxx/examples/gpiodetectcxx.cpp     |   9 +-
 bindings/cxx/examples/gpiofindcxx.cpp       |   2 +-
 bindings/cxx/examples/gpiogetcxx.cpp        |  12 +-
 bindings/cxx/examples/gpioinfocxx.cpp       |  63 +-
 bindings/cxx/examples/gpiomoncxx.cpp        |  39 +-
 bindings/cxx/examples/gpiosetcxx.cpp        |  19 +-
 bindings/cxx/gpiod.hpp                      | 938 +-------------------
 bindings/cxx/gpiodcxx/Makefile.am           |  14 +
 bindings/cxx/gpiodcxx/chip.hpp              | 180 ++++
 bindings/cxx/gpiodcxx/edge-event-buffer.hpp | 115 +++
 bindings/cxx/gpiodcxx/edge-event.hpp        | 125 +++
 bindings/cxx/gpiodcxx/info-event.hpp        | 108 +++
 bindings/cxx/gpiodcxx/line-config.hpp       | 250 ++++++
 bindings/cxx/gpiodcxx/line-info.hpp         | 209 +++++
 bindings/cxx/gpiodcxx/line-request.hpp      | 207 +++++
 bindings/cxx/gpiodcxx/misc.hpp              |  49 +
 bindings/cxx/gpiodcxx/request-config.hpp    |  97 ++
 bindings/cxx/info-event.cpp                 |  89 ++
 bindings/cxx/internal.hpp                   | 168 +++-
 bindings/cxx/iter.cpp                       |  60 --
 bindings/cxx/line-config.cpp                | 226 +++++
 bindings/cxx/line-info.cpp                  | 150 ++++
 bindings/cxx/line-request.cpp               | 194 ++++
 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                             |  63 +-
 lib/edge-event.c                            |   6 +
 lib/line-request.c                          |  50 +-
 tools/gpioget.c                             |   3 +-
 38 files changed, 2854 insertions(+), 1849 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
#1 lore raw

PATCH v3 1/3 API: add a function for retrieving the capacity of edge event buffers

Add a getter for the edge-event buffer capacity property. This is useful if we want to read the maximum number of events that can be stored in a buffer object without externally storing its capacity.

---
 include/gpiod.h  | 9 +++++++++
 lib/edge-event.c | 6 ++++++
 2 files changed, 15 insertions(+)
include/gpiod.h +9 -0
index 66e6c12..8fc20ed 100644
@@ -989,6 +989,15 @@ unsigned long gpiod_edge_event_get_line_seqno(struct gpiod_edge_event *event);
 struct gpiod_edge_event_buffer *
 gpiod_edge_event_buffer_new(unsigned int capacity);
 
+/**
+ * @brief Get the capacity (the max number of events that can be stored) of
+ *        the event buffer.
+ * @param buffer Edge event buffer.
+ * @return The capacity of the buffer.
+ */
+unsigned int
+gpiod_edge_event_buffer_get_capacity(struct gpiod_edge_event_buffer *buffer);
+
 /**
  * @brief Free the edge event buffer and release all associated resources.
  * @param buffer Edge event buffer to free.
lib/edge-event.c +6 -0
index 4aece4b..ab484ad 100644
@@ -109,6 +109,12 @@ gpiod_edge_event_buffer_new(unsigned int capacity)
 	return buf;
 }
 
+GPIOD_API unsigned int
+gpiod_edge_event_buffer_get_capacity(struct gpiod_edge_event_buffer *buffer)
+{
+	return buffer->capacity;
+}
+
 GPIOD_API void
 gpiod_edge_event_buffer_free(struct gpiod_edge_event_buffer *buffer)
 {
Signature · 1 line
2.30.1
#2 lore raw

PATCH v3 2/3 API: extend the line request functionality

In reply to Bartosz Golaszewski

This adds new public functions for interacting with line request objects. First we implement getters for the number of lines in a request and for the array of offsets of requested lines. Next we rename the existing gpiod_line_request_get/set_values() functions to gpiod_line_request_get/set_values_subset() and add new functions that are named gpiod_line_request_get/set_values() but allow to get/set the values of all requested lines.

---
 include/gpiod.h    | 54 ++++++++++++++++++++++++++++++++++++++++------
 lib/line-request.c | 50 +++++++++++++++++++++++++++++++++---------
 tools/gpioget.c    |  3 +--
 3 files changed, 89 insertions(+), 18 deletions(-)
include/gpiod.h +48 -6
index 8fc20ed..51e93be 100644
@@ -814,6 +814,22 @@ gpiod_request_config_set_event_buffer_size(struct gpiod_request_config *config,
  */
 void gpiod_line_request_release(struct gpiod_line_request *request);
 
+/**
+ * @brief Get the number of lines in this request.
+ * @param request Line request object.
+ * @return Number of requested lines.
+ */
+unsigned int gpiod_line_request_get_num_lines(struct gpiod_line_request *request);
+
+/**
+ * @brief Get the hardware offsets of lines in this request.
+ * @param request Line request object.
+ * @param offsets Array to store offsets in. Must hold at least the number of
+ *                lines returned by ::gpiod_line_request_get_num_lines.
+ */
+void gpiod_line_request_get_offsets(struct gpiod_line_request *request,
+				    unsigned int *offsets);
+
 /**
  * @brief Read the value of a single line associated with this request.
  * @param request Line request object.
@@ -824,7 +840,7 @@ int gpiod_line_request_get_value(struct gpiod_line_request *request,
 				 unsigned int offset);
 
 /**
- * @brief Read values of lines associated with this request.
+ * @brief Read values of a subset of lines associated with this request.
  * @param request GPIO line request.
  * @param num_lines Number of lines for which to read values.
  * @param offsets Array of offsets corresponding with the lines associated with
@@ -832,9 +848,23 @@ int gpiod_line_request_get_value(struct gpiod_line_request *request,
  * @param values Array in which the values will be stored.
  * @return 0 on success, -1 on failure.
  */
+int gpiod_line_request_get_values_subset(struct gpiod_line_request *request,
+					 unsigned num_lines,
+					 const unsigned int *offsets,
+					 int *values);
+
+/**
+ * @brief Read values of all lines associated with this request.
+ * @param request GPIO line request.
+ * @param values Array in which the values will be stored. Must hold at least
+ *               the number of lines returned by
+ *               ::gpiod_line_request_get_num_lines. The index of each value
+ *               will be associated with the offset at the same index in the
+ *               offset array returned by ::gpiod_line_request_get_offsets.
+ * @return 0 on success, -1 on failure.
+ */
 int gpiod_line_request_get_values(struct gpiod_line_request *request,
-				  unsigned num_lines,
-				  const unsigned int *offsets, int *values);
+				  int *values);
 
 /**
  * @brief Set the value of a single line associated with this request.
@@ -846,7 +876,7 @@ int gpiod_line_request_set_value(struct gpiod_line_request *request,
 				 unsigned int offset, int value);
 
 /**
- * @brief Set values of lines associated with this line request.
+ * @brief Set values of a subset of lines associated with this line request.
  * @param request GPIO line request.
  * @param num_lines Number of lines for which to set values.
  * @param offsets Array of offsets corresponding with the lines associated with
@@ -855,9 +885,21 @@ int gpiod_line_request_set_value(struct gpiod_line_request *request,
  *               correspond with the offsets in the previous argument.
  * @return 0 on success, -1 on failure.
  */
+int gpiod_line_request_set_values_subset(struct gpiod_line_request *request,
+					 unsigned int num_lines,
+					 const unsigned int *offsets,
+					 const int *values);
+
+/**
+ * @brief Set values of all lines associated with this request.
+ * @param request GPIO line request.
+ * @param values Array containing the values to set. Must hold at least the
+ *               number of lines returned by ::gpiod_line_request_get_num_lines.
+ *               The index of each value be associated with the offset at the
+ *               same index in the offset array returned by
+ *               ::gpiod_line_request_get_offsets.
+ */
 int gpiod_line_request_set_values(struct gpiod_line_request *request,
-				  unsigned int num_lines,
-				  const unsigned int *offsets,
 				  const int *values);
 
 /**
lib/line-request.c +40 -10

85 lines not on this page · raw message

tools/gpioget.c +1 -2
index e0c630d..afa2ccb 100644
@@ -129,8 +129,7 @@ int main(int argc, char **argv)
 	if (!request)
 		die_perror("unable to request lines");
 
-	ret = gpiod_line_request_get_values(request, num_lines,
-					    offsets, values);
+	ret = gpiod_line_request_get_values(request, values);
 	if (ret)
 		die_perror("unable to read GPIO line values");
 
Signature · 1 line
2.30.1
#3 lore raw

PATCH v3 3/3 bindings: cxx: implement C++ bindings for libgpiod v2.0

In reply to 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.

---
 Doxyfile.in                                 |   4 +-
 bindings/cxx/Makefile.am                    |  16 +-
 bindings/cxx/chip.cpp                       | 214 +++--
 bindings/cxx/edge-event-buffer.cpp          | 103 +++
 bindings/cxx/edge-event.cpp                 | 123 +++
 bindings/cxx/examples/Makefile.am           |  12 +-
 bindings/cxx/examples/gpiodetectcxx.cpp     |   9 +-
 bindings/cxx/examples/gpiofindcxx.cpp       |   2 +-
 bindings/cxx/examples/gpiogetcxx.cpp        |  12 +-
 bindings/cxx/examples/gpioinfocxx.cpp       |  63 +-
 bindings/cxx/examples/gpiomoncxx.cpp        |  39 +-
 bindings/cxx/examples/gpiosetcxx.cpp        |  19 +-
 bindings/cxx/gpiod.hpp                      | 938 +-------------------
 bindings/cxx/gpiodcxx/Makefile.am           |  14 +
 bindings/cxx/gpiodcxx/chip.hpp              | 180 ++++
 bindings/cxx/gpiodcxx/edge-event-buffer.hpp | 115 +++
 bindings/cxx/gpiodcxx/edge-event.hpp        | 125 +++
 bindings/cxx/gpiodcxx/info-event.hpp        | 108 +++
 bindings/cxx/gpiodcxx/line-config.hpp       | 250 ++++++
 bindings/cxx/gpiodcxx/line-info.hpp         | 209 +++++
 bindings/cxx/gpiodcxx/line-request.hpp      | 207 +++++
 bindings/cxx/gpiodcxx/misc.hpp              |  49 +
 bindings/cxx/gpiodcxx/request-config.hpp    |  97 ++
 bindings/cxx/info-event.cpp                 |  89 ++
 bindings/cxx/internal.hpp                   | 168 +++-
 bindings/cxx/iter.cpp                       |  60 --
 bindings/cxx/line-config.cpp                | 226 +++++
 bindings/cxx/line-info.cpp                  | 150 ++++
 bindings/cxx/line-request.cpp               | 194 ++++
 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 +
 34 files changed, 2750 insertions(+), 1831 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
index 22d4721..53e8c2b 100644
@@ -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
index d9fa577..4f7eb47 100644
@@ -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 +143 -71

299 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
index 748b581..7e29616 100644
@@ -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 +4 -5
index 872cd96..09a32fa 100644
@@ -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)
@@ -20,9 +19,9 @@ int main(int argc, char **argv)
 		if (::gpiod::is_gpiochip_device(entry.path())) {
 			::gpiod::chip chip(entry.path());
 
-			::std::cout << chip.name() << " ["
-				    << chip.label() << "] ("
-				    << chip.num_lines() << " lines)" << ::std::endl;
+			::std::cout << chip.get_name() << " ["
+				    << chip.get_label() << "] ("
+				    << chip.get_num_lines() << " lines)" << ::std::endl;
 		}
 	}
 
bindings/cxx/examples/gpiofindcxx.cpp +1 -1
index ec4d79b..1df251e 100644
@@ -22,7 +22,7 @@ int main(int argc, char **argv)
 
 			auto offset = chip.find_line(argv[1]);
 			if (offset >= 0) {
-				::std::cout << chip.name() << " " << offset << ::std::endl;
+				::std::cout << chip.get_name() << " " << offset << ::std::endl;
 				return EXIT_SUCCESS;
 			}
 		}
bindings/cxx/examples/gpiogetcxx.cpp +3 -9
index 94b3dac..e9372c8 100644
@@ -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();
 
 	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 -21
index 4d6ac6e..9e56ec8 100644
@@ -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.get_type() == ::gpiod::edge_event::type::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.get_timestamp() / 1000000000;
 	::std::cout << ".";
-	::std::cout << event.timestamp.count() % 1000000000;
+	::std::cout << event.get_timestamp() % 1000000000;
 
-	::std::cout << " line: " << event.source.offset();
+	::std::cout << " line: " << event.get_line_offset();
 
 	::std::cout << ::std::endl;
 }
@@ -39,25 +37,24 @@ int main(int argc, char **argv)
 		return EXIT_FAILURE;
 	}
 
-	::std::vector<unsigned int> offsets;
+	::gpiod::line_offsets offsets;
 	offsets.reserve(argc);
 	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, "gpiomoncxx"),
+					  ::gpiod::line_config(::gpiod::line_config::direction::INPUT,
+							       ::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
index 71b27a9..935b438 100644
@@ -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(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
new file mode 100644
index 0000000..f5ef9ce
@@ -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 +180 -0

180 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 +125 -0

125 lines not on this page · raw message

bindings/cxx/gpiodcxx/info-event.hpp new file +108 -0

108 lines not on this page · raw message

bindings/cxx/gpiodcxx/line-config.hpp new file +250 -0

250 lines not on this page · raw message

bindings/cxx/gpiodcxx/line-info.hpp new file +209 -0

209 lines not on this page · raw message

bindings/cxx/gpiodcxx/line-request.hpp new file +207 -0

207 lines not on this page · raw message

bindings/cxx/gpiodcxx/misc.hpp new file +49 -0

13 lines not on this page · raw message

bindings/cxx/gpiodcxx/request-config.hpp new file +97 -0

97 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 +226 -0

226 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 +194 -0

194 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/misc.cpp new file +18 -0
new file mode 100644
index 0000000..a5639c6

Lines of this file are not shown. View the raw message.

bindings/cxx/request-config.cpp new file +80 -0

80 lines not on this page · raw message

configure.ac +1 -0
index e0a917f..a0fd45a 100644

Lines of this file are not shown. View the 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.

#4 lore raw

Re: [libgpiod][PATCH v3 3/3] 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.

My apologies for not getting around to reviewing v2, so here is a quick(??) review of v3...

I don't have any problems with the C API changes in patches 1 and 2. I would prefer that we didn't need the num_lines, offsets and capacity getters, but their addition is a reasonable compromise.

<snip>

Quoted 14 lines
bindings/cxx/chip.cpp +10 -3
-GPIOD_CXX_API bool is_gpiochip_device(const ::std::string& path)
+struct chip::impl
 {
-	return ::gpiod_is_gpiochip_device(path.c_str());
-}
+	impl(const ::std::string& path)
+		: chip(open_chip(path)),
+		  name(::gpiod_chip_get_name(this->chip.get())),
+		  label(::gpiod_chip_get_label(this->chip.get()))
+	{
+
+	}
+
+	impl(const impl& other) = delete;

bindings/cxx/chip.cpp line 64

Quoted 10 lines
bindings/cxx/chip.cpp +10 -0
+	impl(impl&& other) = delete;
+	impl& operator=(const impl& other) = delete;
+	impl& operator=(impl&& other) = delete;
+
+	void throw_if_closed(void) const
+	{
+		if (!this->chip)
+			throw ::std::logic_error("GPIO chip has been closed");
+	}
+

Perhaps throw something more specific than a logic_error?

<snip>

Quoted 15 lines
bindings/cxx/edge-event.cpp new file +9 -0
new file mode 100644
index 0000000..d6dce22
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: LGPL-3.0-or-later
+// SPDX-FileCopyrightText: 2021 Bartosz Golaszewski <[email protected]>
+
+#include <map>
+#include <stdexcept>
+#include <system_error>
+#include <utility>
+
+#include "internal.hpp"

bindings/cxx/edge-event.cpp line 19

Quoted 10 lines
bindings/cxx/edge-event.cpp new file +10 -0
+
+namespace gpiod {
+
+namespace {
+
+const ::std::map<int, edge_event::type> event_type_mapping = {
+	{ GPIOD_LINE_EVENT_RISING_EDGE,		edge_event::type::RISING_EDGE },
+	{ GPIOD_LINE_EVENT_FALLING_EDGE,	edge_event::type::FALLING_EDGE }
+};
+

Use of a map for just two event types seems like overkill. Wouldn't an if-else in edge_event::get_type(void) be simpler and cleaner? Also, as written, edge_event::get_type(void) can throw std::out_of_range which doesn't seem right. Wouldn't std::range_error be more appropriate? Or adding an unknown event type for failed mappings, so get_type() can never throw??

Same applies for mappings throughout. If the C++ values match the C values then you could just perform a range check and cast for the cases where there are more than two values.

<snip>

Quoted 8 lines
bindings/cxx/edge-event.cpp new file +8 -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.
+	 */

bindings/cxx/edge-event.cpp line 37

Quoted 10 lines
bindings/cxx/edge-event.cpp new file +10 -0
+	line_info get_line_info(unsigned int offset, bool watch = false) const;
+
+	/**
+	 * @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;
+

I repeat my objection to get_line_info() accepting a watch parameter and watch_line_info() wrapping it. While they both happen to return line_info, the purpose of the two methods are quite different and so should be kept separate.

Also, should be watch_line_info() really be allowed on a const chip? While it does not alter the C++ object, and so technically can be allowed on a const, it does alter the state of the underlying C object and so I would argue it should not be allowed.

<snip>

bindings/cxx/edge-event.cpp line 50

Quoted 13 lines
bindings/cxx/edge-event.cpp new file +13 -0
+	/**
+	 * @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;
+

A getter not prefixed with "get_" ;-).
(note that I'm kidding - I prefer it this way - more on that later in class line_info)

bindings/cxx/edge-event.cpp line 56

Quoted 6 lines
bindings/cxx/edge-event.cpp new file +6 -0
+	/**
+	 * @brief Maximum capacity of the buffer.
+	 * @return Buffer capacity.
+	 */
+	unsigned int capacity(void) const noexcept;
+

And again.

<snip>

bindings/cxx/edge-event.cpp line 64

Quoted 8 lines
bindings/cxx/edge-event.cpp new file +8 -0
+/**
+ * @brief Contains a set of line config options used in line requests and
+ *        reconfiguration.
+ */
+class line_config
+{
+public:
+

This class has setters but no getters.
There is no use case for getters?
Also applies to C API - just wondering given the addition of the getters in patch 1 and 2 - there may be cases where the user would like to read back the config?

bindings/cxx/edge-event.cpp line 77

Quoted 13 lines
bindings/cxx/edge-event.cpp new file +13 -0
+	/**
+	 * @brief Direction settings.
+	 */
+	enum class direction : unsigned int
+	{
+		AS_IS = 1,
+		/**< Request the line(s), but don't change current direction. */
+		INPUT,
+		/**< Request the line(s) for reading the GPIO line state. */
+		OUTPUT
+		/**< Request the line(s) for setting the GPIO line state. */
+	};
+

line_info and line_config should use common direction types (also applies to C API)? Perhaps in a ::gpiod::line namespace?

Same applies to other fields that map to GPIO_V2_LINE_FLAGs, so edge, drive, bias and event clock.
Oh, and the C API lacks a gpiod_line_info_get_event_clock(), and not surprisingly class line_info lacks a getter for event clock.

<snip>

Quoted 5 lines
bindings/cxx/edge-event.cpp new file +5 -0
+	/**
+	 * @brief Move assignment operator.
+	 * @param other Object to move.
+	 * @return Reference to self.
+	 */

bindings/cxx/edge-event.cpp line 92

Quoted 10 lines
bindings/cxx/edge-event.cpp new file +10 -0
+	line_config& operator=(line_config&& other) noexcept;
+
+	/**
+	 * @brief Set the direction, either globally or for a set of offsets.
+	 * @param dir_val New direction setting.
+	 * @param offsets Vector of line offsets to set the direction for. If
+	 *                empty, the setting will be global.
+	 */
+	void set_direction(direction dir_val, const line_offsets& offsets = line_offsets()) const;
+

Setters should not be const.
While your implementation may allow it now, conceptually it is wrong. And what happens if your implementation changes later? Removing the const later would break ABI compatibility.

This applies throughout.

<snip>

Quoted 9 lines
bindings/cxx/edge-event.cpp new file +9 -0
+/**
+ * @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:
+
+	/**

bindings/cxx/edge-event.cpp line 111

Quoted 10 lines
bindings/cxx/edge-event.cpp new file +10 -0
+	 * @brief Direction settings.
+	 */
+	enum class direction : unsigned int
+	{
+		INPUT = 1,
+		/**< Direction is input - we're reading the state of a GPIO line. */
+		OUTPUT
+		/**< Direction is output - we're driving the GPIO line. */
+	};
+

And as previously noted, I'd like to see the line_info and line_config use common types.

And I agree with Jack Winch on not prefixing getters with "get_". The only thing I would change from Jack's suggestion is to put the types in the line namespace so rather than his

    using direction_type = ::gpiod::direction;

I would have

    using direction_type = ::gpiod::line::direction;

- if you want to use an alias at all.

The types are used relatively rarely, so I'm no fussed if their names are a bit clunky. OTOH getters are frequently used and I much prefer the shorter form.

Where you have an "enum class type" that would conflict with a type() getter, such as in info_event and edge_event, I would rename the enum to event_type, or similar.

That applies throughout.

Wrt setters, I'm happy with them being prefixed with "set_" so they are clearly distinguished from getters.

<snip>

Quoted 7 lines
bindings/cxx/edge-event.cpp new file +7 -0
+/**
+ * @brief Stores the context of a set of requested GPIO lines.
+ */
+class line_request
+{
+public:
+

<snip>

bindings/cxx/edge-event.cpp

Quoted 7 lines
bindings/cxx/edge-event.cpp new file +7 -0
+
+	/**
+	 * @brief Get the number of requested lines.
+	 * @return Number of lines in this request.
+	 */
+	unsigned int num_lines(void) const;
+

More getters not prefixed with "get_" ;-).

bindings/cxx/edge-event.cpp

Quoted 6 lines
bindings/cxx/edge-event.cpp new file +6 -0
+	/**
+	 * @brief Get the list of offsets of requested lines.
+	 * @return List of hardware offsets of the lines in this request.
+	 */
+	line_offsets offsets(void) const;
+

And again.

<snip>

bindings/cxx/edge-event.cpp

Quoted 7 lines
bindings/cxx/edge-event.cpp new file +7 -0
+template<class enum_type, void global_func(::gpiod_line_config*, int),
+	 void subset_func(::gpiod_line_config*, int, unsigned int, const unsigned int*)>
+void set_mapped_value(::gpiod_line_config* config, enum_type value,
+		      const line_offsets& offsets, const ::std::map<enum_type, int>& mapping)
+{
+	int mapped_val = mapping.at(value);
+

This can throw ::std::out_of_range, which set_mapped_value() should catch and re-throw as ::std::invalid_argument.

The use of templating means mapping using ::std::map in this direction makes more sense than the reverse in the other modules, but I'm still not a fan.

<snip>

bindings/cxx/edge-event.cpp

Quoted 6 lines
bindings/cxx/edge-event.cpp new file +6 -0
+void line_request::impl::throw_if_released(void) const
+{
+	if (!this->request)
+		throw ::std::logic_error("GPIO lines have been released");
+}
+

Throw a more specific exception, rather than a generic logic_error.

<snip>

Hopefully I haven't snipped out too much...

Cheers,
Kent.

#5 lore raw

Re: [libgpiod][PATCH v3 3/3] bindings: cxx: implement C++ bindings for libgpiod v2.0

Quoted 35 lines · Kent Gibson
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.

My apologies for not getting around to reviewing v2, so here is a quick(??) review of v3...

I don't have any problems with the C API changes in patches 1 and 2. I would prefer that we didn't need the num_lines, offsets and capacity getters, but their addition is a reasonable compromise.

<snip>

Quoted 18 lines
bindings/cxx/chip.cpp +14 -3
-GPIOD_CXX_API bool is_gpiochip_device(const ::std::string& path)
+struct chip::impl
 {
-     return ::gpiod_is_gpiochip_device(path.c_str());
-}
+     impl(const ::std::string& path)
+             : chip(open_chip(path)),
+               name(::gpiod_chip_get_name(this->chip.get())),
+               label(::gpiod_chip_get_label(this->chip.get()))
+     {
+
+     }
+
+     impl(const impl& other) = delete;
+     impl(impl&& other) = delete;
+     impl& operator=(const impl& other) = delete;
+     impl& operator=(impl&& other) = delete;
+
Quoted 9 lines

bindings/cxx/chip.cpp line 64

Quoted 6 lines
bindings/cxx/chip.cpp +6 -0
+     void throw_if_closed(void) const
+     {
+             if (!this->chip)
+                     throw ::std::logic_error("GPIO chip has been closed");
+     }
+

Perhaps throw something more specific than a logic_error?

There's no standard exception that would inherit from std::logic_error that would fit here. I can define new exceptions (gpiod::chip_closed and gpiod::request_released) inheriting from logic_error if that works for you.

Quoted 26 lines

<snip>

Quoted 24 lines
bindings/cxx/edge-event.cpp new file +18 -0
new file mode 100644
index 0000000..d6dce22
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: LGPL-3.0-or-later
+// SPDX-FileCopyrightText: 2021 Bartosz Golaszewski <[email protected]>
+
+#include <map>
+#include <stdexcept>
+#include <system_error>
+#include <utility>
+
+#include "internal.hpp"
+
+namespace gpiod {
+
+namespace {
+
+const ::std::map<int, edge_event::type> event_type_mapping = {
+     { GPIOD_LINE_EVENT_RISING_EDGE,         edge_event::type::RISING_EDGE },
+     { GPIOD_LINE_EVENT_FALLING_EDGE,        edge_event::type::FALLING_EDGE }
+};
Quoted 9 lines

bindings/cxx/edge-event.cpp line 19

Quoted 1 line
bindings/cxx/edge-event.cpp new file +1 -0
+

Use of a map for just two event types seems like overkill. Wouldn't an if-else in edge_event::get_type(void) be simpler and cleaner? Also, as written, edge_event::get_type(void) can throw std::out_of_range which doesn't seem right. Wouldn't std::range_error be more appropriate? Or adding an unknown event type for failed mappings, so get_type() can never throw??

While it's true that we can throw the range_error, it's very unlikely and would occur only if a serious programming error was present in the C library or the kernel. I'm fine with this possibility - adding an unknown type, would suggest this is a normal situation and not an exception.

For the mapping: to me this looks more consistent and also allows to detect the rare and unlikely error mentioned above. In any case - this is an implementation detail and can be changed later.

Quoted 4 lines

Same applies for mappings throughout. If the C++ values match the C values then you could just perform a range check and cast for the cases where there are more than two values.

Casting in general is to be avoided in C++ - as defined in the document Jack linked recently. Mapping isn't expensive and looks more robust IMO. I would prefer to leave it like this.

Quoted 17 lines

<snip>

Quoted 15 lines
bindings/cxx/edge-event.cpp new file +15 -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;
+
+     /**
+      * @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.
Quoted 9 lines

bindings/cxx/edge-event.cpp

Quoted 3 lines
bindings/cxx/edge-event.cpp new file +3 -0
+      */
+     line_info watch_line_info(unsigned int offset) const;
+

I repeat my objection to get_line_info() accepting a watch parameter and watch_line_info() wrapping it. While they both happen to return line_info, the purpose of the two methods are quite different and so should be kept separate.

OK, I will change it.

Quoted 5 lines

Also, should be watch_line_info() really be allowed on a const chip? While it does not alter the C++ object, and so technically can be allowed on a const, it does alter the state of the underlying C object and so I would argue it should not be allowed.

Makes sense.

Quoted 31 lines

<snip>

bindings/cxx/edge-event.cpp

Quoted 13 lines
bindings/cxx/edge-event.cpp new file +13 -0
+     /**
+      * @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;
+

A getter not prefixed with "get_" ;-).
(note that I'm kidding - I prefer it this way - more on that later in class line_info)

bindings/cxx/edge-event.cpp

Quoted 6 lines
bindings/cxx/edge-event.cpp new file +6 -0
+     /**
+      * @brief Maximum capacity of the buffer.
+      * @return Buffer capacity.
+      */
+     unsigned int capacity(void) const noexcept;
+

And again.

<snip>

Quoted 15 lines

bindings/cxx/edge-event.cpp

Quoted 8 lines
bindings/cxx/edge-event.cpp new file +8 -0
+/**
+ * @brief Contains a set of line config options used in line requests and
+ *        reconfiguration.
+ */
+class line_config
+{
+public:
+

This class has setters but no getters.
There is no use case for getters?
Also applies to C API - just wondering given the addition of the getters in patch 1 and 2 - there may be cases where the user would like to read back the config?

IMO if a use-case like this appears, we can extend the interface. The reason for adding the new functions was because we added the all-lines getters and now the user would need to store the offsets outside the request if we didn't provide them. Adding getters for config structures now would unnecessarily bloat the interface. Let me know if you disagree though. I usually eventually get convinced by you. :)

Quoted 14 lines

bindings/cxx/edge-event.cpp

Quoted 13 lines
bindings/cxx/edge-event.cpp new file +13 -0
+     /**
+      * @brief Direction settings.
+      */
+     enum class direction : unsigned int
+     {
+             AS_IS = 1,
+             /**< Request the line(s), but don't change current direction. */
+             INPUT,
+             /**< Request the line(s) for reading the GPIO line state. */
+             OUTPUT
+             /**< Request the line(s) for setting the GPIO line state. */
+     };
+
Quoted 3 lines

line_info and line_config should use common direction types (also applies to C API)? Perhaps in a ::gpiod::line namespace?

So that would seem obvious to do this but the values stored in line info and passed to line config are not always the same. BIAS_UNKNOWN and BIAS_AS_IS are not the same and the former is only suitable for line_info while the latter only works for the config. I agree though that it may be a bit confusing for users. If we'd change it - where should these go wrt doxygen sections of the core API?

Quoted 5 lines

Same applies to other fields that map to GPIO_V2_LINE_FLAGs, so edge, drive, bias and event clock.
Oh, and the C API lacks a gpiod_line_info_get_event_clock(), and not surprisingly class line_info lacks a getter for event clock.

Yeah I noticed that and already have a patch for that in my dev branch for python bindings. I'll post it shortly.

Quoted 23 lines

<snip>

Quoted 5 lines
bindings/cxx/edge-event.cpp new file +5 -0
+     /**
+      * @brief Move assignment operator.
+      * @param other Object to move.
+      * @return Reference to self.
+      */

bindings/cxx/edge-event.cpp

Quoted 10 lines
bindings/cxx/edge-event.cpp new file +10 -0
+     line_config& operator=(line_config&& other) noexcept;
+
+     /**
+      * @brief Set the direction, either globally or for a set of offsets.
+      * @param dir_val New direction setting.
+      * @param offsets Vector of line offsets to set the direction for. If
+      *                empty, the setting will be global.
+      */
+     void set_direction(direction dir_val, const line_offsets& offsets = line_offsets()) const;
+

Setters should not be const.
While your implementation may allow it now, conceptually it is wrong. And what happens if your implementation changes later? Removing the const later would break ABI compatibility.

Quoted 2 lines

This applies throughout.

Indeed.

Quoted 29 lines

<snip>

Quoted 9 lines
bindings/cxx/edge-event.cpp new file +9 -0
+/**
+ * @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:
+
+     /**

bindings/cxx/edge-event.cpp

Quoted 10 lines
bindings/cxx/edge-event.cpp new file +10 -0
+      * @brief Direction settings.
+      */
+     enum class direction : unsigned int
+     {
+             INPUT = 1,
+             /**< Direction is input - we're reading the state of a GPIO line. */
+             OUTPUT
+             /**< Direction is output - we're driving the GPIO line. */
+     };
+

And as previously noted, I'd like to see the line_info and line_config use common types.

And I agree with Jack Winch on not prefixing getters with "get_". The only thing I would change from Jack's suggestion is to put the types in the line namespace so rather than his

Quoted 7 lines
    using direction_type = ::gpiod::direction;

I would have

    using direction_type = ::gpiod::line::direction;

- if you want to use an alias at all.

I think we'll be able to skip the aliases with the additional line namespace.

Quoted 5 lines

The types are used relatively rarely, so I'm no fussed if their names are a bit clunky. OTOH getters are frequently used and I much prefer the shorter form.

Fair enough.

Quoted 40 lines

Where you have an "enum class type" that would conflict with a type() getter, such as in info_event and edge_event, I would rename the enum to event_type, or similar.

That applies throughout.

Wrt setters, I'm happy with them being prefixed with "set_" so they are clearly distinguished from getters.

<snip>

Quoted 7 lines
bindings/cxx/edge-event.cpp new file +7 -0
+/**
+ * @brief Stores the context of a set of requested GPIO lines.
+ */
+class line_request
+{
+public:
+

<snip>

bindings/cxx/edge-event.cpp

Quoted 7 lines
bindings/cxx/edge-event.cpp new file +7 -0
+
+     /**
+      * @brief Get the number of requested lines.
+      * @return Number of lines in this request.
+      */
+     unsigned int num_lines(void) const;
+

More getters not prefixed with "get_" ;-).

bindings/cxx/edge-event.cpp

Quoted 6 lines
bindings/cxx/edge-event.cpp new file +6 -0
+     /**
+      * @brief Get the list of offsets of requested lines.
+      * @return List of hardware offsets of the lines in this request.
+      */
+     line_offsets offsets(void) const;
+

And again.

<snip>

Quoted 11 lines

bindings/cxx/edge-event.cpp

Quoted 7 lines
bindings/cxx/edge-event.cpp new file +7 -0
+template<class enum_type, void global_func(::gpiod_line_config*, int),
+      void subset_func(::gpiod_line_config*, int, unsigned int, const unsigned int*)>
+void set_mapped_value(::gpiod_line_config* config, enum_type value,
+                   const line_offsets& offsets, const ::std::map<enum_type, int>& mapping)
+{
+     int mapped_val = mapping.at(value);
+

This can throw ::std::out_of_range, which set_mapped_value() should catch and re-throw as ::std::invalid_argument.

Makes sense.

Quoted 15 lines

The use of templating means mapping using ::std::map in this direction makes more sense than the reverse in the other modules, but I'm still not a fan.

<snip>

bindings/cxx/edge-event.cpp

Quoted 6 lines
bindings/cxx/edge-event.cpp new file +6 -0
+void line_request::impl::throw_if_released(void) const
+{
+     if (!this->request)
+             throw ::std::logic_error("GPIO lines have been released");
+}
+

Throw a more specific exception, rather than a generic logic_error.

Quoted 6 lines

<snip>

Hopefully I haven't snipped out too much...

Cheers,
Kent.

Thanks Kent for the thorough - as always - review!

Bart

#6 lore raw

Re: [libgpiod][PATCH v3 3/3] bindings: cxx: implement C++ bindings for libgpiod v2.0

Quoted 46 lines · Bartosz Golaszewski
Quoted 35 lines · Kent Gibson
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.

My apologies for not getting around to reviewing v2, so here is a quick(??) review of v3...

I don't have any problems with the C API changes in patches 1 and 2. I would prefer that we didn't need the num_lines, offsets and capacity getters, but their addition is a reasonable compromise.

<snip>

Quoted 18 lines
bindings/cxx/chip.cpp +14 -3
-GPIOD_CXX_API bool is_gpiochip_device(const ::std::string& path)
+struct chip::impl
 {
-     return ::gpiod_is_gpiochip_device(path.c_str());
-}
+     impl(const ::std::string& path)
+             : chip(open_chip(path)),
+               name(::gpiod_chip_get_name(this->chip.get())),
+               label(::gpiod_chip_get_label(this->chip.get()))
+     {
+
+     }
+
+     impl(const impl& other) = delete;
+     impl(impl&& other) = delete;
+     impl& operator=(const impl& other) = delete;
+     impl& operator=(impl&& other) = delete;
+
Quoted 9 lines

bindings/cxx/chip.cpp line 64

Quoted 6 lines
bindings/cxx/chip.cpp +6 -0
+     void throw_if_closed(void) const
+     {
+             if (!this->chip)
+                     throw ::std::logic_error("GPIO chip has been closed");
+     }
+

Perhaps throw something more specific than a logic_error?

Quoted 5 lines

There's no standard exception that would inherit from std::logic_error that would fit here. I can define new exceptions (gpiod::chip_closed and gpiod::request_released) inheriting from logic_error if that works for you.

Exactly what I had in mind.

Quoted 36 lines
Quoted 26 lines

<snip>

Quoted 24 lines
bindings/cxx/edge-event.cpp new file +18 -0
new file mode 100644
index 0000000..d6dce22
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: LGPL-3.0-or-later
+// SPDX-FileCopyrightText: 2021 Bartosz Golaszewski <[email protected]>
+
+#include <map>
+#include <stdexcept>
+#include <system_error>
+#include <utility>
+
+#include "internal.hpp"
+
+namespace gpiod {
+
+namespace {
+
+const ::std::map<int, edge_event::type> event_type_mapping = {
+     { GPIOD_LINE_EVENT_RISING_EDGE,         edge_event::type::RISING_EDGE },
+     { GPIOD_LINE_EVENT_FALLING_EDGE,        edge_event::type::FALLING_EDGE }
+};
Quoted 9 lines

bindings/cxx/edge-event.cpp line 19

Quoted 1 line
bindings/cxx/edge-event.cpp new file +1 -0
+

Use of a map for just two event types seems like overkill. Wouldn't an if-else in edge_event::get_type(void) be simpler and cleaner? Also, as written, edge_event::get_type(void) can throw std::out_of_range which doesn't seem right. Wouldn't std::range_error be more appropriate? Or adding an unknown event type for failed mappings, so get_type() can never throw??

Quoted 3 lines

While it's true that we can throw the range_error, it's very unlikely and would occur only if a serious programming error was present in the C library or the kernel.

No - you will currently throw ::std::out_of_range, which is weird. I'd be ok with ::std::range_error, but that would involve a catch and re-throw.

And it could also happen if the kernel adds new event types that this version of the library is unaware of.
While that isn't terribly likely, don't box yourself in.

Quoted 4 lines

I'm fine with this possibility - adding an
unknown type, would suggest this is a normal situation and not an exception.

Fair enough.
That suggestion was coming from "a simple getter shouldn't throw" PoV, btw.

Quoted 4 lines

For the mapping: to me this looks more consistent and also allows to detect the rare and unlikely error mentioned above. In any case - this is an implementation detail and can be changed later.

Quoted 9 lines
Quoted 4 lines

Same applies for mappings throughout. If the C++ values match the C values then you could just perform a range check and cast for the cases where there are more than two values.

Casting in general is to be avoided in C++ - as defined in the document Jack linked recently. Mapping isn't expensive and looks more robust IMO. I would prefer to leave it like this.

If casting is so frowned upon then why have they defined so many types of casts? I thought the trick was to pick the right one ;-). And we are only talking about casting between int enums here, so really...

Quoted 84 lines
Quoted 17 lines

<snip>

Quoted 15 lines
bindings/cxx/edge-event.cpp new file +15 -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;
+
+     /**
+      * @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.
Quoted 9 lines

bindings/cxx/edge-event.cpp

Quoted 3 lines
bindings/cxx/edge-event.cpp new file +3 -0
+      */
+     line_info watch_line_info(unsigned int offset) const;
+

I repeat my objection to get_line_info() accepting a watch parameter and watch_line_info() wrapping it. While they both happen to return line_info, the purpose of the two methods are quite different and so should be kept separate.

OK, I will change it.

Quoted 5 lines

Also, should be watch_line_info() really be allowed on a const chip? While it does not alter the C++ object, and so technically can be allowed on a const, it does alter the state of the underlying C object and so I would argue it should not be allowed.

Makes sense.

Quoted 31 lines

<snip>

bindings/cxx/edge-event.cpp

Quoted 13 lines
bindings/cxx/edge-event.cpp new file +13 -0
+     /**
+      * @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;
+

A getter not prefixed with "get_" ;-).
(note that I'm kidding - I prefer it this way - more on that later in class line_info)

bindings/cxx/edge-event.cpp

Quoted 6 lines
bindings/cxx/edge-event.cpp new file +6 -0
+     /**
+      * @brief Maximum capacity of the buffer.
+      * @return Buffer capacity.
+      */
+     unsigned int capacity(void) const noexcept;
+

And again.

<snip>

Quoted 15 lines

bindings/cxx/edge-event.cpp

Quoted 8 lines
bindings/cxx/edge-event.cpp new file +8 -0
+/**
+ * @brief Contains a set of line config options used in line requests and
+ *        reconfiguration.
+ */
+class line_config
+{
+public:
+

This class has setters but no getters.
There is no use case for getters?
Also applies to C API - just wondering given the addition of the getters in patch 1 and 2 - there may be cases where the user would like to read back the config?

Quoted 7 lines

IMO if a use-case like this appears, we can extend the interface. The reason for adding the new functions was because we added the all-lines getters and now the user would need to store the offsets outside the request if we didn't provide them. Adding getters for config structures now would unnecessarily bloat the interface. Let me know if you disagree though. I usually eventually get convinced by you. :)

Agreed keep the interface minimal and easy enough to add later if the need arises. Just throwing it out there in case there are use cases I've overlooked.

Quoted 14 lines
Quoted 14 lines
Quoted 13 lines
bindings/cxx/edge-event.cpp new file +13 -0
+     /**
+      * @brief Direction settings.
+      */
+     enum class direction : unsigned int
+     {
+             AS_IS = 1,
+             /**< Request the line(s), but don't change current direction. */
+             INPUT,
+             /**< Request the line(s) for reading the GPIO line state. */
+             OUTPUT
+             /**< Request the line(s) for setting the GPIO line state. */
+     };
+
Quoted 8 lines
Quoted 3 lines

line_info and line_config should use common direction types (also applies to C API)? Perhaps in a ::gpiod::line namespace?

So that would seem obvious to do this but the values stored in line info and passed to line config are not always the same. BIAS_UNKNOWN and BIAS_AS_IS are not the same and the former is only suitable for line_info while the latter only works for the config.

Sure they are distinct - so give them different values in the C API. line_info will never return BIAS_AS_IS, and line_config will reject setting BIAS_UNKNOWN. So document that.

On a related note, the C API should have a GPIOD_LINE_CONFIG_BIAS_AS_IS, and that should be the default - as it is in the kernel. GPIOD_LINE_CONFIG_BIAS_DISABLED is actively disabling bias, which is a different thing.
Or are you intending for libgpiod to always actively specify bias?

Quoted 4 lines

I agree though
that it may be a bit confusing for users. If we'd change it - where should these go wrt doxygen sections of the core API?

You would think that doxygen should be subservient to the code, not the other way around :-|.

Given I'm suggesting the GPIOD_LINE_CONFIG_xxx enums get folded into the GPIOD_LINE_xxx enums, they would end up wherever those are?

Cheers,
Kent.

#7 lore raw
In reply to Bartosz Golaszewski

Hello Bartosz,

I am most certainly being *dense* here, but which libgpiod branch should I be applying this patch series to? Also, what other patch series is this particular set of patches dependent on? I tried to apply against master and next/libgpiod-2.0 without much success this weekend. Also, just for clarity, how is the next/libgpiod-2.0 branch being used at current? Which and at what point are you applying patches to this particular branch?

Once I've got over this basic hurdle, I'd like to do a 'holistic' review of this patch series. As I've not been following changes to either the kernel subsystem or user library as of late, I feel this approach to be more appropriate and thorough.

Finally, what is the target deadline for completion of the v2 API?

Best,
Jack

Quoted 108 lines · Bartosz Golaszewski

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.

v1 -> v2:
Kent: I addressed most points from your review. Some are unaddressed due to personal preference (for instance: I still allow creating of empty line-request objects as they may be reused in subsequent requests). I also kept the 'watch' argument in get_line_info() as well as the boolean operators for chip and request - although with (hopefully) better documentation.

v2 -> v3:
- use scoped class enums instead of regular integer ones
- rename getters to get_<property>() in order to avoid name conflicts with the new enum types

The first two patches extend the core C API of libgpiod as discussed.

Bartosz Golaszewski (3):
API: add a function for retrieving the capacity of edge event buffers
API: extend the line request functionality
bindings: cxx: implement C++ bindings for libgpiod v2.0

 Doxyfile.in                                 |   4 +-
 bindings/cxx/Makefile.am                    |  16 +-
 bindings/cxx/chip.cpp                       | 214 +++--
 bindings/cxx/edge-event-buffer.cpp          | 103 +++
 bindings/cxx/edge-event.cpp                 | 123 +++
 bindings/cxx/examples/Makefile.am           |  12 +-
 bindings/cxx/examples/gpiodetectcxx.cpp     |   9 +-
 bindings/cxx/examples/gpiofindcxx.cpp       |   2 +-
 bindings/cxx/examples/gpiogetcxx.cpp        |  12 +-
 bindings/cxx/examples/gpioinfocxx.cpp       |  63 +-
 bindings/cxx/examples/gpiomoncxx.cpp        |  39 +-
 bindings/cxx/examples/gpiosetcxx.cpp        |  19 +-
 bindings/cxx/gpiod.hpp                      | 938 +-------------------
 bindings/cxx/gpiodcxx/Makefile.am           |  14 +
 bindings/cxx/gpiodcxx/chip.hpp              | 180 ++++
 bindings/cxx/gpiodcxx/edge-event-buffer.hpp | 115 +++
 bindings/cxx/gpiodcxx/edge-event.hpp        | 125 +++
 bindings/cxx/gpiodcxx/info-event.hpp        | 108 +++
 bindings/cxx/gpiodcxx/line-config.hpp       | 250 ++++++
 bindings/cxx/gpiodcxx/line-info.hpp         | 209 +++++
 bindings/cxx/gpiodcxx/line-request.hpp      | 207 +++++
 bindings/cxx/gpiodcxx/misc.hpp              |  49 +
 bindings/cxx/gpiodcxx/request-config.hpp    |  97 ++
 bindings/cxx/info-event.cpp                 |  89 ++
 bindings/cxx/internal.hpp                   | 168 +++-
 bindings/cxx/iter.cpp                       |  60 --
 bindings/cxx/line-config.cpp                | 226 +++++
 bindings/cxx/line-info.cpp                  | 150 ++++
 bindings/cxx/line-request.cpp               | 194 ++++
 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                             |  63 +-
 lib/edge-event.c                            |   6 +
 lib/line-request.c                          |  50 +-
 tools/gpioget.c                             |   3 +-
 38 files changed, 2854 insertions(+), 1849 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

--
2.30.1

#8 lore raw
Quoted 11 lines · Jack Winch

Hello Bartosz,

I am most certainly being *dense* here, but which libgpiod branch should I be applying this patch series to? Also, what other patch series is this particular set of patches dependent on? I tried to apply against master and next/libgpiod-2.0 without much success this weekend. Also, just for clarity, how is the next/libgpiod-2.0 branch being used at current? Which and at what point are you applying patches to this particular branch?

It's actually my fault. Let me explain: the development of the v2 API for the entire project (C, C++ and Python parts) happens in the next/libgpiod-2.0 branch of
git://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git I squash the new changes into the big patch with the plan of resending it for a final review once we agree on the interface (more or less anyway). That will allow the repo to remain bisectable. Then we'll be able to fine-tune different elements.

Quoted 5 lines

Once I've got over this basic hurdle, I'd like to do a 'holistic' review of this patch series. As I've not been following changes to either the kernel subsystem or user library as of late, I feel this approach to be more appropriate and thorough.

Maybe you could wait until I post v4? I'm addressing a lot of issues in this revision. Previous small patches have already been squashed and they concerned the core C library anyway.

Quoted 2 lines

Finally, what is the target deadline for completion of the v2 API?

There's no target, it'll be ready when it's done.

Bart

PS How did the move go?

#9 lore raw
Quoted 8 lines · Bartosz Golaszewski

It's actually my fault. Let me explain: the development of the v2 API for the entire project (C, C++ and Python parts) happens in the next/libgpiod-2.0 branch of
git://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git I squash the new changes into the big patch with the plan of resending it for a final review once we agree on the interface (more or less anyway). That will allow the repo to remain bisectable. Then we'll be able to fine-tune different elements.

Understood and sensible. Thanks for clarifying.

Quoted 3 lines

Maybe you could wait until I post v4? I'm addressing a lot of issues in this revision. Previous small patches have already been squashed and they concerned the core C library anyway.

Sounds like a plan. I've noticed that, both in the mailing list and your recent commits into next/libgpiod-2.0 (which is why I wondered what the mode of operation is for these changes). Once v4 is out, I will look to review.

Quoted 1 line

There's no target, it'll be ready when it's done.

Again, understood and sensible.

Quoted 1 line

PS How did the move go?

Very well, thank you. Setup with everything I've brought over to Romania, with a little more to be shipped over very soon. On a somewhat related topic, I'll be reaching out to you directly over the next few days (mainly regarding your relationship / work with BayLibre, if that's okay).

Jack