libgpiod v2: C++ bindings

We've already added getters for line-config but without exposing some parts of the internal logic of the object, the user can't really get the full picture and inspect the contents. This patch reworks the accessors further by providing three flavors of each setter and two of each getter.

Up until now, the setters that set the default (fall-back) value for various line settings didn't state explicitly that the values they set have a lower priority than the one coming from setters that take offsets as argument. The docs were vague too and suggested that the last call counts which is not true.

We therefore expose a setter that explicitly sets the "default" value and make sure to stress that this will be used only by lines for which no "specific" override was defined.

We also provide getters that allow to read both the default value as well as the actual value that would be used for each offset if the object was passed to a request call at this moment. We also complete the API by providing functions that allow to identify offsets for which at least one setting differs from the defaults.

This way the caller can fully inspect the line_config and high-level language bindings can provide stringification methods.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 include/gpiod.h   | 255 ++++++++++++++++++++++++++++-----------
 lib/line-config.c | 295 ++++++++++++++++++++++++++++++++--------------
 tools/gpioget.c   |   6 +-
 tools/gpiomon.c   |   6 +-
 tools/gpioset.c   |   9 +-
 5 files changed, 404 insertions(+), 167 deletions(-)

include/gpiod.h +4 -6

index 2a41fca..ee4a067 100644
@@ -450,12 +450,38 @@ gpiod_info_event_get_line_info(struct gpiod_info_event *event);
  *
  * The line-config object stores the configuration for lines that can be used
  * in two cases: when making a line request and when reconfiguring a set of
- * already requested lines. The mutators for the line request don't return
- * errors. If the set of options is too complex to be translated into kernel
- * uAPI structures - the error will be returned at the time of the request or
- * reconfiguration. If an invalid value was passed to any of the getters - the
- * default value will be silently used instead. Each option can be set
- * globally, for a single line offset or for multiple line offsets.
+ * already requested lines.
+ *
+ * A new line-config object is instantiated containing a set of sane defaults
+ * for all supported configuration settings. Those defaults can be modified by

include/gpiod.h +6 -0

+ *
+ * The mutators don't return errors. If the set of options is too complex to
+ * be translated into kernel uAPI structures - the error will be returned at
+ * the time of the request or reconfiguration. If an invalid value was passed
+ * to any of the mutators - the default value will be silently used instead.
+ *

include/gpiod.h +3 -1

+ * For simple cases the offsets can be entirely ignored when preparing the
+ * line configuration.
  */
 
 /**

@@ -480,12 +506,12 @@ void gpiod_line_config_free(struct gpiod_line_config *config);
 void gpiod_line_config_reset(struct gpiod_line_config *config);
 
 /**
- * @brief Set the direction of all lines.
+ * @brief Set the default direction setting.

include/gpiod.h +15 -8

  * @param config Line config object.
  * @param direction New direction.
  * @param num_offsets Number of offsets in the array.

@@ -509,24 +535,31 @@ void gpiod_line_config_set_direction_subset(struct gpiod_line_config *config,
 					    const unsigned int *offsets);
 
 /**
- * @brief Get the direction setting for a given line.
+ * @brief Get the default direction setting.
+ * @param config Line config object.
+ * @return Direction setting that would have been used for any offset not
+ *         assigned its own direction value.
+ */
+int gpiod_line_config_get_direction_default(struct gpiod_line_config *config);
+
+/**
+ * @brief Get the direction setting for a given offset.
  * @param config Line config object.
  * @param offset Line offset for which to read the direction setting.
  * @return Direction setting that would have been used for given offset if the
  *         config object was used in a request at the time of the call.
- * @note If an offset is used for which no config was provided, the function
- *       will return the global default value.
  */
-int gpiod_line_config_get_direction(struct gpiod_line_config *config,
-				    unsigned int offset);
+int gpiod_line_config_get_direction_offset(struct gpiod_line_config *config,
+					   unsigned int offset);
 
 /**
- * @brief Set the edge event detection for all lines.
+ * @brief Set the default edge event detection.
  * @param config Line config object.
  * @param edge Type of edge events to detect.
  */
-void gpiod_line_config_set_edge_detection(struct gpiod_line_config *config,
-					  int edge);
+void
+gpiod_line_config_set_edge_detection_default(struct gpiod_line_config *config,
+					     int edge);
 
 /**
  * @brief Set the edge event detection for a single line at given offset.

include/gpiod.h +41 -19

  * @param config Line config object.
  * @param edge Type of edge events to detect.
  * @param num_offsets Number of offsets in the array.

@@ -551,24 +584,33 @@ gpiod_line_config_set_edge_detection_subset(struct gpiod_line_config *config,
 					    const unsigned int *offsets);
 
 /**
- * @brief Get the edge event detection setting for a given line.
+ * @brief Get the default edge detection setting.
+ * @param config Line config object.
+ * @return Edge detection setting that would have been used for any offset not
+ *         assigned its own direction value.
+ */
+int
+gpiod_line_config_get_edge_detection_default(struct gpiod_line_config *config);
+
+/**
+ * @brief Get the edge event detection setting for a given offset.
  * @param config Line config object.
  * @param offset Line offset for which to read the edge event detection setting.
  * @return Edge event detection setting that would have been used for given
  *         offset if the config object was used in a request at the time of
  *         the call.
- * @note If an offset is used for which no config was provided, the function
- *       will return the global default value.
  */
-int gpiod_line_config_get_edge_detection(struct gpiod_line_config *config,
-					 unsigned int offset);
+int
+gpiod_line_config_get_edge_detection_offset(struct gpiod_line_config *config,
+					    unsigned int offset);
 
 /**
- * @brief Set the bias of all lines.
+ * @brief Set the default bias setting.
  * @param config Line config object.
  * @param bias New bias.
  */
-void gpiod_line_config_set_bias(struct gpiod_line_config *config, int bias);
+void gpiod_line_config_set_bias_default(struct gpiod_line_config *config,
+					int bias);
 
 /**
  * @brief Set the bias for a single line at given offset.

@@ -591,23 +633,30 @@ void gpiod_line_config_set_bias_subset(struct gpiod_line_config *config,
 				       const unsigned int *offsets);
 
 /**
- * @brief Get the bias setting for a given line.
+ * @brief Get the default bias setting.
+ * @param config Line config object.
+ * @return Bias setting that would have been used for any offset not assigned
+ *         its own direction value.
+ */
+int gpiod_line_config_get_bias_default(struct gpiod_line_config *config);
+
+/**
+ * @brief Get the bias setting for a given offset.
  * @param config Line config object.
  * @param offset Line offset for which to read the bias setting.
  * @return Bias setting that would have been used for given offset if the
  *         config object was used in a request at the time of the call.
- * @note If an offset is used for which no config was provided, the function
- *       will return the global default value.
  */
-int gpiod_line_config_get_bias(struct gpiod_line_config *config,
+int gpiod_line_config_get_bias_offset(struct gpiod_line_config *config,
 			       unsigned int offset);
 
 /**
- * @brief Set the drive of all lines.
+ * @brief Set the default drive setting.
  * @param config Line config object.
  * @param drive New drive.
  */
-void gpiod_line_config_set_drive(struct gpiod_line_config *config, int drive);
+void gpiod_line_config_set_drive_default(struct gpiod_line_config *config,
+					 int drive);
 
 /**
  * @brief Set the drive for a single line at given offset.

@@ -619,7 +668,7 @@ void gpiod_line_config_set_drive_offset(struct gpiod_line_config *config,
 					int drive, unsigned int offset);
 
 /**
- * @brief Set the drive for a subset of lines.
+ * @brief Set the drive for a subset of offsets.
  * @param config Line config object.
  * @param drive New drive.
  * @param num_offsets Number of offsets in the array.

@@ -630,22 +679,28 @@ void gpiod_line_config_set_drive_subset(struct gpiod_line_config *config,
 					const unsigned int *offsets);
 
 /**
- * @brief Get the drive setting for a given line.
+ * @brief Get the default drive setting.
+ * @param config Line config object.
+ * @return Drive setting that would have been used for any offset not assigned
+ *         its own direction value.
+ */
+int gpiod_line_config_get_drive_default(struct gpiod_line_config *config);
+
+/**
+ * @brief Get the drive setting for a given offset.
  * @param config Line config object.
  * @param offset Line offset for which to read the drive setting.
  * @return Drive setting that would have been used for given offset if the
  *         config object was used in a request at the time of the call.
- * @note If an offset is used for which no config was provided, the function
- *       will return the global default value.
  */
-int gpiod_line_config_get_drive(struct gpiod_line_config *config,
-				unsigned int offset);
+int gpiod_line_config_get_drive_offset(struct gpiod_line_config *config,
+				       unsigned int offset);
 

include/gpiod.h +81 -30

@@ -665,23 +720,30 @@ void gpiod_line_config_set_active_low_subset(struct gpiod_line_config *config,
 					     unsigned int num_offsets,
 					     const unsigned int *offsets);
 
+/**
+ * @brief Check if active-low is the default setting.
+ * @param config Line config object.
+ * @return Active-low setting that would have been used for any offset not
+ *         assigned its own value.
+ */
+bool gpiod_line_config_get_active_low_default(struct gpiod_line_config *config);
+
 /**
  * @brief Check if the line at given offset was configured as active-low.
  * @param config Line config object.
  * @param offset Line offset for which to read the active-low setting.
  * @return Active-low setting that would have been used for given offset if the
  *         config object was used in a request at the time of the call.
- * @note If an offset is used for which no config was provided, the function
- *       will return the global default value.
  */
-bool gpiod_line_config_get_active_low(struct gpiod_line_config *config,
-				      unsigned int offset);
+bool gpiod_line_config_get_active_low_offset(struct gpiod_line_config *config,
+					     unsigned int offset);
 
 /**
- * @brief Set all lines as active-high.
+ * @brief Set lines to active-high by default.
  * @param config Line config object.
  */
-void gpiod_line_config_set_active_high(struct gpiod_line_config *config);
+void
+gpiod_line_config_set_active_high_default(struct gpiod_line_config *config);
 
 /**
  * @brief Set a single line as active-high.

@@ -702,12 +764,12 @@ void gpiod_line_config_set_active_high_subset(struct gpiod_line_config *config,
 					      const unsigned int *offsets);
 
 /**
- * @brief Set the debounce period for all lines.
+ * @brief Set the default debounce period.
  * @param config Line config object.
  * @param period New debounce period. Disables debouncing if 0.
  */
-void gpiod_line_config_set_debounce_period_us(struct gpiod_line_config *config,
-					      unsigned long period);
+void gpiod_line_config_set_debounce_period_us_default(
+		struct gpiod_line_config *config, unsigned long period);
 
 /**
  * @brief Set the debounce period for a single line at given offset.

@@ -722,7 +784,7 @@ gpiod_line_config_set_debounce_period_us_offset(
 					unsigned int offset);
 
 /**
- * @brief Set the debounce period for a subset of lines.
+ * @brief Set the debounce period for a subset of offsets.
  * @param config Line config object.
  * @param period New debounce period. Disables debouncing if 0.
  * @param num_offsets Number of offsets in the array.

@@ -736,25 +798,33 @@ gpiod_line_config_set_debounce_period_us_subset(
 					const unsigned int *offsets);
 
 /**
- * @brief Get the debounce period for a given line.
+ * @brief Get the default debounce period.
+ * @param config Line config object.
+ * @return Debounce period that would have been used for any offset not
+ *         assigned its own debounce period. 0 if not debouncing is disabled.
+ */
+unsigned long gpiod_line_config_get_debounce_period_us_default(
+					struct gpiod_line_config *config);
+
+/**
+ * @brief Get the debounce period for a given offset.
  * @param config Line config object.
  * @param offset Line offset for which to read the debounce period.
  * @return Debounce period that would have been used for given offset if the
- *         config object was used in a request at the time of the call.
- * @note If an offset is used for which no config was provided, the function
- *       will return the global default value.
+ *         config object was used in a request at the time of the call. 0 if
+ *         debouncing is disabled.
  */
 unsigned long
-gpiod_line_config_get_debounce_us_period(struct gpiod_line_config *config,
-					 unsigned int offset);
+gpiod_line_config_get_debounce_period_us_offset(
+			struct gpiod_line_config *config, unsigned int offset);
 
 /**
- * @brief Set the event timestamp clock for all lines.
+ * @brief Set the default event timestamp clock.
  * @param config Line config object.
  * @param clock New clock to use.
  */
-void gpiod_line_config_set_event_clock(struct gpiod_line_config *config,
-				       int clock);
+void gpiod_line_config_set_event_clock_default(struct gpiod_line_config *config,
+					       int clock);
 
 /**
  * @brief Set the event clock for a single line at given offset.

@@ -766,7 +836,7 @@ void gpiod_line_config_set_event_clock_offset(struct gpiod_line_config *config,
 					      int clock, unsigned int offset);
 
 /**
- * @brief Set the event clock for a subset of lines.
+ * @brief Set the event clock for a subset of offsets.
  * @param config Line config object.
  * @param clock New event clock to use.
  * @param num_offsets Number of offsets in the array.

@@ -778,16 +848,31 @@ void gpiod_line_config_set_event_clock_subset(struct gpiod_line_config *config,
 					      const unsigned int *offsets);
 
 /**
- * @brief Get the event clock setting for a given line.
+ * @brief Get the default event clock setting.
+ * @param config Line config object.
+ * @return Event clock setting that would have been used for any offset not
+ *         assigned its own direction value.
+ */
+int gpiod_line_config_get_event_clock_default(struct gpiod_line_config *config);
+
+/**
+ * @brief Get the event clock setting for a given offset.
  * @param config Line config object.
  * @param offset Line offset for which to read the event clock setting.
  * @return Event clock setting that would have been used for given offset if
  *         the config object was used in a request at the time of the call.
- * @note If an offset is used for which no config was provided, the function
- *       will return the global default value.
  */
-int gpiod_line_config_get_event_clock(struct gpiod_line_config *config,
-				      unsigned int offset);
+int gpiod_line_config_get_event_clock_offset(struct gpiod_line_config *config,
+					     unsigned int offset);
+
+/**
+ * @brief Set the default output value.
+ * @param config Line config object.
+ * @param value New value.
+ */
+void
+gpiod_line_config_set_output_value_default(struct gpiod_line_config *config,
+					   int value);
 
 /**
  * @brief Set the output value for a single offset.

@@ -795,8 +880,8 @@ int gpiod_line_config_get_event_clock(struct gpiod_line_config *config,
  * @param offset Offset of the line.
  * @param value Output value to set.
  */
-void gpiod_line_config_set_output_value(struct gpiod_line_config *config,
-					unsigned int offset, int value);
+void gpiod_line_config_set_output_value_offset(struct gpiod_line_config *config,
+					       unsigned int offset, int value);
 
 /**
  * @brief Set the output values for a set of offsets.

@@ -811,14 +896,44 @@ void gpiod_line_config_set_output_values(struct gpiod_line_config *config,
 					 const unsigned int *offsets,
 					 const int *values);
 
+/**
+ * @brief Get the default output value.
+ * @param config Line config object.
+ * @return Output value that would have been used for any offset not
+ *         assigned its own output value.
+ */
+int
+gpiod_line_config_get_output_value_default(struct gpiod_line_config *config);
+
 /**
  * @brief Get the output value configured for a given line.
  * @param config Line config object.
  * @param offset Line offset for which to read the value.
- * @return 1 or 0 if the value was configured for this line, -1 otherwise.
+ * @return Output value that would have been used for given offset if the
+ *         config object was used in a request at the time of the call.
  */
-int gpiod_line_config_get_output_value(struct gpiod_line_config *config,
-				       unsigned int offset);
+int gpiod_line_config_get_output_value_offset(struct gpiod_line_config *config,
+					      unsigned int offset);
+
+/**
+ * @brief Get the number of offsets for which this config object contains
+ *        at least one setting that is different from the defaults.
+ * @param config Line config object.
+ * @return Number of offsets with at least one overridden setting.
+ */
+unsigned int
+gpiod_line_config_get_num_overridden_offsets(struct gpiod_line_config *config);
+
+/**

Quoted 16 lines · Bartosz Golaszewski

We've already added getters for line-config but without exposing some parts of the internal logic of the object, the user can't really get the full picture and inspect the contents. This patch reworks the accessors further by providing three flavors of each setter and two of each getter.

Up until now, the setters that set the default (fall-back) value for various line settings didn't state explicitly that the values they set have a lower priority than the one coming from setters that take offsets as argument. The docs were vague too and suggested that the last call counts which is not true.

We therefore expose a setter that explicitly sets the "default" value and make sure to stress that this will be used only by lines for which no "specific" override was defined.

Quoted 6 lines

We also provide getters that allow to read both the default value as well as the actual value that would be used for each offset if the object was passed to a request call at this moment. We also complete the API by providing functions that allow to identify offsets for which at least one setting differs from the defaults.

This command set is incomplete. There is no way to unset an attribute, i.e. set it back to tracking the default.

It is also difficult to determine if a line attribute is tracking the default or has been overridden to the current default value. This would be very useful to know for stringification. (also see comment on gpiod_line_config_get_overridden_offsets())

Changing the default AFTER overriding particular lines should be uncommon, so I would've gone with making the "default" setter remove any overrides so ALL lines in the config are set to the new default. Not that that doesn't also treat some use cases badly.

Quoted 2 lines

This way the caller can fully inspect the line_config and high-level language bindings can provide stringification methods.

Quoted 28 lines

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 include/gpiod.h   | 255 ++++++++++++++++++++++++++++-----------
 lib/line-config.c | 295 ++++++++++++++++++++++++++++++++--------------
 tools/gpioget.c   |   6 +-
 tools/gpiomon.c   |   6 +-
 tools/gpioset.c   |   9 +-
 5 files changed, 404 insertions(+), 167 deletions(-)

include/gpiod.h +4 -6

index 2a41fca..ee4a067 100644
@@ -450,12 +450,38 @@ gpiod_info_event_get_line_info(struct gpiod_info_event *event);
  *
  * The line-config object stores the configuration for lines that can be used
  * in two cases: when making a line request and when reconfiguring a set of
- * already requested lines. The mutators for the line request don't return
- * errors. If the set of options is too complex to be translated into kernel
- * uAPI structures - the error will be returned at the time of the request or
- * reconfiguration. If an invalid value was passed to any of the getters - the
- * default value will be silently used instead. Each option can be set
- * globally, for a single line offset or for multiple line offsets.
+ * already requested lines.
+ *
+ * A new line-config object is instantiated containing a set of sane defaults
+ * for all supported configuration settings. Those defaults can be modified by

include/gpiod.h gpiod_info_event_get_line_info(struct gpiod_info_event *event); · line 466

Quoted 10 lines

include/gpiod.h +10 -0

+ * the caller. Default values can be overridden by applying different values
+ * for specific line offsets. When making a request or reconfiguring an
+ * existing one, the overridden settings for specific offsets will be taken
+ * into account first and for every other offset and setting the defaults will
+ * be used.
+ *
+ * For every setting, the mutators come in three flavors: one setting the
+ * default value (to which we fallback for every offset not explicitly
+ * overridden), one setting an override for a single offset and one that
+ * overrides settings for a subset of offsets.

I prefer the term "line" over "offset" where referring to a line. The offset is a unique identifer for a line, but is isn't a line, and you are setting the config of a line, not an offset.

Maybe add something about offsets being used as line identifiers if it isn't already sufficiently clear.

Quoted 6 lines

include/gpiod.h +6 -0

+ *
+ * The mutators don't return errors. If the set of options is too complex to
+ * be translated into kernel uAPI structures - the error will be returned at
+ * the time of the request or reconfiguration. If an invalid value was passed
+ * to any of the mutators - the default value will be silently used instead.
+ *

include/gpiod.h gpiod_info_event_get_line_info(struct gpiod_info_event *event); · line 482

Quoted 10 lines

include/gpiod.h +10 -0

+ * We also expose getters - these come in two flavors for every settings: one
+ * that returns the value that would have been used for given offset if the
+ * request was made at this moment and one that always returns the default
+ * value.
+ *
+ * It's important to not think of the offsets used in the offset->value mapping
+ * as linked to actual hardware offsets of the lines exposed by a GPIO chip as
+ * config objects live separately from any chips and are only analyzed at the
+ * time the request is being made.
+ *

I'm not sure what you are trying to say in this paragraph. The offsets in the API are the hardware offsets, right? You are saying that the config doesn't necessarily correspond to the hardware setting unless is is unchanged since the last request or reconfiguration call?

Either way you might want to reword that paragraph.>

And gramatically it probably should be "important not to think".

Quoted 17 lines

include/gpiod.h +17 -0

+
+/**
+ * @brief Get the number of offsets for which this config object contains
+ *        at least one setting that is different from the defaults.
+ * @param config Line config object.
+ * @return Number of offsets with at least one overridden setting.
+ */
+unsigned int
+gpiod_line_config_get_num_overridden_offsets(struct gpiod_line_config *config);
+
+/**
+ * @brief Get the offsets for which this config object contains at least one
+ *        setting that is different from the defaults.
+ * @param config Line config object.
+ * @param offsets Array to store the offsets. Must hold at least the number
+ *                of offsets returned by
+ *                ::gpiod_line_config_get_num_overridden_offsets.

include/gpiod.h void gpiod_line_config_set_output_values(struct gpiod_line_config *config, · line 936

Quoted 4 lines

+ */

include/gpiod.h +2 -0

+void gpiod_line_config_get_overridden_offsets(struct gpiod_line_config *config,
+                                           unsigned int *offsets);

The "that is different from the defaults" filtering performed by these two functions is premature and prevents the caller determining the actual state of the current config.

It could also produce weird stringification. Consider a set of output lines. The stringification will vary wildly depending on the output value of each line. Those with the default value are filtered, the remainder displayed. I would prefer the stringification to show all the output lines being explicitly set, but the filtering makes that impossible.

Let the caller do any filtering themselves.
Maybe expose override_differs_from_defaults(), suitably renamed of course.
A method to check if a line attribute is overridden or defaulted would be useful too.

Cheers,
Kent.

Quoted 25 lines · Kent Gibson

Quoted 16 lines · Bartosz Golaszewski

We've already added getters for line-config but without exposing some parts of the internal logic of the object, the user can't really get the full picture and inspect the contents. This patch reworks the accessors further by providing three flavors of each setter and two of each getter.

Up until now, the setters that set the default (fall-back) value for various line settings didn't state explicitly that the values they set have a lower priority than the one coming from setters that take offsets as argument. The docs were vague too and suggested that the last call counts which is not true.

We therefore expose a setter that explicitly sets the "default" value and make sure to stress that this will be used only by lines for which no "specific" override was defined.

Quoted 6 lines

We also provide getters that allow to read both the default value as well as the actual value that would be used for each offset if the object was passed to a request call at this moment. We also complete the API by providing functions that allow to identify offsets for which at least one setting differs from the defaults.

Quoted 8 lines

This command set is incomplete. There is no way to unset an attribute, i.e. set it back to tracking the default.

It is also difficult to determine if a line attribute is tracking the default or has been overridden to the current default value. This would be very useful to know for stringification. (also see comment on gpiod_line_config_get_overridden_offsets())

Actually the subsequent patch implements the stream insertion operator for the line_config class and does the stringification. You can always get the list of overridden offsets and compare the values of the default and per-offset attributes. I didn't want to add more functions to this struct but if you think it's a good idea then I can do it.

Quoted 5 lines

Changing the default AFTER overriding particular lines should be uncommon, so I would've gone with making the "default" setter remove any overrides so ALL lines in the config are set to the new default. Not that that doesn't also treat some use cases badly.

Yeah, I disagree with this one. It's not very intuitive IMO. I prefer that the offsets be overridden one by one and the overrides have higher priority.

Quoted 2 lines

Quoted 2 lines

This way the caller can fully inspect the line_config and high-level language bindings can provide stringification methods.

I already have that both for C++ and for Python (Work-in-Progress), but it can make it easier indeed.

Quoted 63 lines

Quoted 28 lines

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 include/gpiod.h   | 255 ++++++++++++++++++++++++++++-----------
 lib/line-config.c | 295 ++++++++++++++++++++++++++++++++--------------
 tools/gpioget.c   |   6 +-
 tools/gpiomon.c   |   6 +-
 tools/gpioset.c   |   9 +-
 5 files changed, 404 insertions(+), 167 deletions(-)

include/gpiod.h +4 -6

index 2a41fca..ee4a067 100644
@@ -450,12 +450,38 @@ gpiod_info_event_get_line_info(struct gpiod_info_event *event);
  *
  * The line-config object stores the configuration for lines that can be used
  * in two cases: when making a line request and when reconfiguring a set of
- * already requested lines. The mutators for the line request don't return
- * errors. If the set of options is too complex to be translated into kernel
- * uAPI structures - the error will be returned at the time of the request or
- * reconfiguration. If an invalid value was passed to any of the getters - the
- * default value will be silently used instead. Each option can be set
- * globally, for a single line offset or for multiple line offsets.
+ * already requested lines.
+ *
+ * A new line-config object is instantiated containing a set of sane defaults
+ * for all supported configuration settings. Those defaults can be modified by

include/gpiod.h gpiod_info_event_get_line_info(struct gpiod_info_event *event); · line 466

Quoted 10 lines

include/gpiod.h +10 -0

+ * the caller. Default values can be overridden by applying different values
+ * for specific line offsets. When making a request or reconfiguring an
+ * existing one, the overridden settings for specific offsets will be taken
+ * into account first and for every other offset and setting the defaults will
+ * be used.
+ *
+ * For every setting, the mutators come in three flavors: one setting the
+ * default value (to which we fallback for every offset not explicitly
+ * overridden), one setting an override for a single offset and one that
+ * overrides settings for a subset of offsets.

I prefer the term "line" over "offset" where referring to a line. The offset is a unique identifer for a line, but is isn't a line, and you are setting the config of a line, not an offset.

Maybe add something about offsets being used as line identifiers if it isn't already sufficiently clear.

Quoted 6 lines

include/gpiod.h +6 -0

+ *
+ * The mutators don't return errors. If the set of options is too complex to
+ * be translated into kernel uAPI structures - the error will be returned at
+ * the time of the request or reconfiguration. If an invalid value was passed
+ * to any of the mutators - the default value will be silently used instead.
+ *

include/gpiod.h gpiod_info_event_get_line_info(struct gpiod_info_event *event); · line 482

Quoted 10 lines

include/gpiod.h +10 -0

+ * We also expose getters - these come in two flavors for every settings: one
+ * that returns the value that would have been used for given offset if the
+ * request was made at this moment and one that always returns the default
+ * value.
+ *
+ * It's important to not think of the offsets used in the offset->value mapping
+ * as linked to actual hardware offsets of the lines exposed by a GPIO chip as
+ * config objects live separately from any chips and are only analyzed at the
+ * time the request is being made.
+ *

Quoted 7 lines

I'm not sure what you are trying to say in this paragraph. The offsets in the API are the hardware offsets, right? You are saying that the config doesn't necessarily correspond to the hardware setting unless is is unchanged since the last request or reconfiguration call?

Either way you might want to reword that paragraph.>

A line_config object contains configuration that can be reused across multiple chips. When you set an attribute for an offset, then use the config object on two different chips, it will affect two separate "real/physical" lines with the same offset on their chips. As long as you don't use the config, it only stores abstract mapping. That's why I chose to speak of offsets here and of lines in the chip docs. Does it make sense? If not, I'm open to change but it sounds more intuitive for me.

Quoted 2 lines

And gramatically it probably should be "important not to think".

[snip]

Quoted 17 lines

Quoted 17 lines

include/gpiod.h +17 -0

+
+/**
+ * @brief Get the number of offsets for which this config object contains
+ *        at least one setting that is different from the defaults.
+ * @param config Line config object.
+ * @return Number of offsets with at least one overridden setting.
+ */
+unsigned int
+gpiod_line_config_get_num_overridden_offsets(struct gpiod_line_config *config);
+
+/**
+ * @brief Get the offsets for which this config object contains at least one
+ *        setting that is different from the defaults.
+ * @param config Line config object.
+ * @param offsets Array to store the offsets. Must hold at least the number
+ *                of offsets returned by
+ *                ::gpiod_line_config_get_num_overridden_offsets.

Quoted 9 lines

include/gpiod.h void gpiod_line_config_set_output_values(struct gpiod_line_config *config, · line 936

Quoted 4 lines

+ */

include/gpiod.h +2 -0

+void gpiod_line_config_get_overridden_offsets(struct gpiod_line_config *config,
+                                           unsigned int *offsets);

The "that is different from the defaults" filtering performed by these two functions is premature and prevents the caller determining the actual state of the current config.

I'm not sure what you mean here. Could you give an example?

Quoted 7 lines

It could also produce weird stringification. Consider a set of output lines. The stringification will vary wildly depending on the output value of each line. Those with the default value are filtered, the remainder displayed. I would prefer the stringification to show all the output lines being explicitly set, but the filtering makes that impossible.

So here's the deal: you can't speak of "all the lines" in the config object. You can have a config object that defines output values for offset 2, 3 and 8 and the default value of 1. Then you use it to export 64 lines from a chip that has 128 lines total. What are "all the lines" in this case? The stringification for the line_config should output the defaults + any explicitly overridden offset.

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.

bindings/cxx/chip.cpp +3 -2

-GPIOD_CXX_API bool chip::operator!=(const chip& rhs) const noexcept
+GPIOD_CXX_API int chip::find_line(const ::std::string& name) const
 {
-	return this->_m_chip.get() != rhs._m_chip.get();
+	this->_m_priv->throw_if_closed();
+

bindings/cxx/gpiodcxx/chip.hpp new file +6 -0

+	/**
+	 * @brief Check if this object is valid.
+	 * @return True if this object's methods can be used, false otherwise.
+	 *         False usually means the chip was closed. If the user calls
+	 *         any of the methods of this class on an object for which this
+	 *         operator returned false, a logic_error will be thrown.

bindings/cxx/gpiodcxx/chip.hpp new file +30 -0

+	/**
+	 * @brief Get the name of this GPIO chip.
+	 * @return C string pointer to the chip name.
+	 */
+	const char* name(void) const;
+
+	/**
+	 * @brief Get the label of this GPIO chip.
+	 * @return C string pointer to the chip name.
+	 */
+	const char* label(void) const;
+
+	/**
+	 * @brief Get the filesystem path that was used to open this GPIO chip.
+	 * @return C string pointer to the chip's path.
+	 */
+	const char* path(void) const;
+
+	/**
+	 * @brief Return the number of lines exposed by this chip.
+	 * @return Number of lines.
+	 */
+	unsigned int num_lines(void) const;
+
+	/**
+	 * @brief Retrieve the current snapshot of line information for a
+	 *        single line.
+	 * @param offset Offset of the line to get the info for.
+	 * @return New ::gpiod::line_info object.
+	 */

bindings/cxx/gpiodcxx/chip.hpp new file +11 -0

+	/**
+	 * @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.

bindings/cxx/gpiodcxx/chip.hpp new file +9 -0

+	 * The default line configuration is: input, active-high, bias as-is,
+	 * no edge detection, push-pull drive (unused in input mode), no
+	 * debounce, monotonic event clock (unused with no edge detection).
+	 */
+	static const line_config& default_line_config(void);
+
+private:
+
+	struct impl;

+::std::ostream& operator<<(::std::ostream& out, const chip& chip);

bindings/cxx/gpiodcxx/chip.hpp new file +8 -0

+
+/**
+ * @}
+ */
+
+} /* namespace gpiod */
+
+#endif /* __LIBGPIOD_CXX_CHIP_HPP__ */

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +25 -0

new file mode 100644
index 0000000..87da8ac
@@ -0,0 +1,122 @@
+/* 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__
+
+#if !defined(__LIBGPIOD_GPIOD_CXX_INSIDE__)
+#error "Only gpiod.hpp can be included directly."
+#endif
+
+#include <iostream>
+#include <memory>
+#include <vector>
+
+namespace gpiod {
+
+class edge_event;
+class line_request;
+
+/**
+ * @ingroup gpiod_cxx

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +33 -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.

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +14 -0

+class line_config
+{
+public:
+
+	/**
+	 * @brief Constructor.
+	 * @param direction Global direction setting.
+	 * @param edge Global edge detection.
+	 * @param active_low Global active_low setting.
+	 * @param output_values Default output values.
+	 * @param bias Global bias setting.
+	 * @param drive Global drive setting.
+	 * @param debounce_period Global debounce period in microseconds.
+	 * @param clock Default event clock mode.

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +20 -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;
+
+	/**
+	 * @brief Move assignment operator.
+	 * @param other Object to move.
+	 * @return Reference to self.
+	 */
+	line_config& operator=(line_config&& other) noexcept;
+
+	/**

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +4 -0

+	/**
+	 * @brief Set the direction for a single line at given offset.
+	 * @param direction New direction.
+	 * @param offset Offset of the line for which to set the direction.

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +25 -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 Get the number of requested lines.
+	 * @return Number of lines in this request.
+	 */
+	unsigned int num_lines(void) const;
+
+	/**
+	 * @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;
+
+	/**
+	 * @brief Get 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/edge-event-buffer.hpp new file +7 -0

+	/**
+	 * @brief Get the values of all requested lines.
+	 * @return List of read values.
+	 */
+	line::values get_values(void);
+
+	/**

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +4 -0

+	/**
+	 * @brief Set the values of a subset of requested lines.
+	 * @param values Vector containing a set of offset->value mappings.
+	 */

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +27 -0

+	/**
+	 * @brief Set the values of all requested lines.
+	 * @param values Array of new line values. The size must be equal to
+	 *               the value returned by line_request::num_lines.
+	 */
+	void set_values(const line::values& values);
+
+	/**
+	 * @brief Apply new config options to requested lines.
+	 * @param config New configuration.
+	 */
+	void reconfigure_lines(const line_config& config);
+
+	/**
+	 * @brief Get the file descriptor number associated with this line
+	 *        request.
+	 * @return File descriptor number.
+	 */
+	int 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.
+	 */

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +26 -0

+GPIOD_CXX_API ::std::ostream& operator<<(::std::ostream& out, const line_config& config)
+{
+	out << "line_config(defaults=(direction=" << direction_names.at(config.direction()) <<
+	       "edge_detection=" << edge_names.at(config.edge_detection()) <<
+	       "bias=" << bias_names.at(config.bias()) <<
+	       "drive=" << drive_names.at(config.drive()) <<
+	       (config.active_low() ? "active-low" : "active-high") <<
+	       "debounce_period=" << config.debounce_period_us() << "us" <<
+	       "event_clock=" << clock_names.at(config.event_clock()) <<
+	       "default_output_value=" << config.output_value() <<
+	       ")";
+
+	if (config.num_overridden_offsets()) {
+		out << "overrides=[";
+
+		for (const auto& offset: config.overridden_offsets()) {
+			out << "(offset=" << offset <<
+			       "direction=" << direction_names.at(config.direction(offset)) <<
+			       "edge_detection=" << edge_names.at(config.edge_detection(offset)) <<
+			       "bias=" << bias_names.at(config.bias(offset)) <<
+			       "drive=" << drive_names.at(config.drive(offset)) <<
+			       (config.active_low(offset) ? "active-low" : "active-high") <<
+			       "debounce_period=" << config.debounce_period_us(offset) <<
+			       "event_clock=" << clock_names.at(config.event_clock(offset)) <<
+			       "output_value=" << config.output_value(offset) <<
+			       ")";

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.

I dislike the squashing approach. I would rather accept that this branch is unstable and keep the patches separate.
If you want stability at each commit then disable the building of any unstable sections until they are fit to be included in the build again, rather than squashing everything into one humungous patch.

I'd rather see working tests than working examples :-(, unless you think your API is still in flux? Separate patches for API, implementation, tests, and examples would work for me.

A lot of the comments below are nitpicking documentation wording, and may well apply to the C API as well.

<snip>

Quoted 7 lines

bindings/cxx/chip.cpp +3 -2

-GPIOD_CXX_API bool chip::operator!=(const chip& rhs) const noexcept
+GPIOD_CXX_API int chip::find_line(const ::std::string& name) const
 {
-     return this->_m_chip.get() != rhs._m_chip.get();
+     this->_m_priv->throw_if_closed();
+

bindings/cxx/chip.cpp namespace gpiod { · line 197

Quoted 10 lines

bindings/cxx/chip.cpp +9 -0

+     int ret = ::gpiod_chip_find_line(this->_m_priv->chip.get(), name.c_str());
+     if (ret < 0) {
+             if (errno == ENOENT)
+                     return -1;
+
+             throw_from_errno("error looking up line by name");
+     }
+
+     return ret;
 }

        if (ret < 0 && errno != ENOENT)
                throw_from_errno("error looking up line by name");

unless you are guarding against ret < -1??

<snip>

bindings/cxx/gpiodcxx/chip.hpp line 65

Quoted 8 lines

bindings/cxx/gpiodcxx/chip.hpp new file +8 -0

+
+     /**
+      * @brief Move assignment operator.
+      * @param other Object to move.
+      * @return Reference to self.
+      */
+     chip& operator=(chip&& other) noexcept;
+

s/Reference to self/Updated object/g

Quoted 6 lines

bindings/cxx/gpiodcxx/chip.hpp new file +6 -0

+     /**
+      * @brief Check if this object is valid.
+      * @return True if this object's methods can be used, false otherwise.
+      *         False usually means the chip was closed. If the user calls
+      *         any of the methods of this class on an object for which this
+      *         operator returned false, a logic_error will be thrown.

bindings/cxx/gpiodcxx/chip.hpp line 81

Quoted 10 lines

bindings/cxx/gpiodcxx/chip.hpp new file +10 -0

+      */
+     explicit operator bool(void) const noexcept;
+
+     /**
+      * @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);
+

It actually throws chip_closed.

Quoted 30 lines

bindings/cxx/gpiodcxx/chip.hpp new file +30 -0

+     /**
+      * @brief Get the name of this GPIO chip.
+      * @return C string pointer to the chip name.
+      */
+     const char* name(void) const;
+
+     /**
+      * @brief Get the label of this GPIO chip.
+      * @return C string pointer to the chip name.
+      */
+     const char* label(void) const;
+
+     /**
+      * @brief Get the filesystem path that was used to open this GPIO chip.
+      * @return C string pointer to the chip's path.
+      */
+     const char* path(void) const;
+
+     /**
+      * @brief Return the number of lines exposed by this chip.
+      * @return Number of lines.
+      */
+     unsigned int num_lines(void) const;
+
+     /**
+      * @brief Retrieve the current snapshot of line information for a
+      *        single line.
+      * @param offset Offset of the line to get the info for.
+      * @return New ::gpiod::line_info object.
+      */

bindings/cxx/gpiodcxx/chip.hpp line 121

Quoted 10 lines

bindings/cxx/gpiodcxx/chip.hpp new file +10 -0

+     line_info get_line_info(unsigned int offset) 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;
+

Does a user care that this is a wrapper?
Just describe what the method does, from the user's PoV, not how. If the user really cares about how they can look at the implementation.

@brief Start watching a line for info events and return the current line info.
@param offset Offset of the line.
@note Info events are returned via info_event_read().

bindings/cxx/gpiodcxx/chip.hpp line 127

Quoted 6 lines

bindings/cxx/gpiodcxx/chip.hpp new file +6 -0

+     /**
+      * @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;
+

@param offset Offset of the line.
(The method doesn't get info, and even if it did this would be redundant as that should be described in @brief.)

s/line at given offset/line/g
(The @param makes it clear that the offset identifies the line.)

bindings/cxx/gpiodcxx/chip.hpp line 136

Quoted 9 lines

bindings/cxx/gpiodcxx/chip.hpp new file +9 -0

+     /**
+      * @brief Get the file descriptor associated with this chip.
+      * @return File descriptor number.
+      */
+     int fd(void) const;
+
+     /**
+      * @brief Wait for line status events on any of the watched lines
+      *        exposed by this chip.

"watched lines of this chip."

bindings/cxx/gpiodcxx/chip.hpp line 148

Quoted 12 lines

bindings/cxx/gpiodcxx/chip.hpp new file +12 -0

+      * @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;
+

@note This method can block unless info_event_wait() returns true.

Quoted 11 lines

bindings/cxx/gpiodcxx/chip.hpp new file +11 -0

+     /**
+      * @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.

bindings/cxx/gpiodcxx/chip.hpp line 169

Quoted 10 lines

bindings/cxx/gpiodcxx/chip.hpp new file +10 -0

+      * @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());
+
+     /**
+      * @brief Get the default line config object.
+      * @return Reference to the default line config object.
+      *

@return Default line config object.

Returning a const& is a standard pattern.
Omit that it is reference, just as you omitted it is const. i.e. describe the object being passed, not how it is being passed - that is clear from the type.

Similarly elsewhere...

Quoted 9 lines

bindings/cxx/gpiodcxx/chip.hpp new file +9 -0

+      * The default line configuration is: input, active-high, bias as-is,
+      * no edge detection, push-pull drive (unused in input mode), no
+      * debounce, monotonic event clock (unused with no edge detection).
+      */
+     static const line_config& default_line_config(void);
+
+private:
+
+     struct impl;

bindings/cxx/gpiodcxx/chip.hpp line 188

Quoted 10 lines

bindings/cxx/gpiodcxx/chip.hpp new file +10 -0

+
+     ::std::unique_ptr<impl> _m_priv;
+};
+
+/**
+ * @brief Stream insertion operator for GPIO chip objects.
+ * @param out Output stream to write to.
+ * @param chip GPIO chip to print.
+ * @return Reference to out.
+ */

... like here.

s/Reference to out/Output stream/g

Quoted 40 lines

+::std::ostream& operator<<(::std::ostream& out, const chip& chip);

bindings/cxx/gpiodcxx/chip.hpp new file +8 -0

+
+/**
+ * @}
+ */
+
+} /* namespace gpiod */
+
+#endif /* __LIBGPIOD_CXX_CHIP_HPP__ */

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +25 -0

new file mode 100644
index 0000000..87da8ac
@@ -0,0 +1,122 @@
+/* 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__
+
+#if !defined(__LIBGPIOD_GPIOD_CXX_INSIDE__)
+#error "Only gpiod.hpp can be included directly."
+#endif
+
+#include <iostream>
+#include <memory>
+#include <vector>
+
+namespace gpiod {
+
+class edge_event;
+class line_request;
+
+/**
+ * @ingroup gpiod_cxx

bindings/cxx/gpiodcxx/edge-event-buffer.hpp line 35

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 reading edge_event objects into an existing
+ * buffer which improves the performance by avoiding needless memory
+ * allocations.
+ */

"buffer to improve performance"

Quoted 33 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +33 -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.

bindings/cxx/gpiodcxx/edge-event-buffer.hpp line 78

Quoted 10 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +10 -0

+      */
+     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 an edge event.

@return Edge event.

<snip>

bindings/cxx/gpiodcxx/edge-event-buffer.hpp line 85

Quoted 7 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +7 -0

+     /**
+      * @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;
+

@return Line info object ...

<snip>

Quoted 14 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +14 -0

+class line_config
+{
+public:
+
+     /**
+      * @brief Constructor.
+      * @param direction Global direction setting.
+      * @param edge Global edge detection.
+      * @param active_low Global active_low setting.
+      * @param output_values Default output values.
+      * @param bias Global bias setting.
+      * @param drive Global drive setting.
+      * @param debounce_period Global debounce period in microseconds.
+      * @param clock Default event clock mode.

bindings/cxx/gpiodcxx/edge-event-buffer.hpp line 109

Quoted 10 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +10 -0

+      */
+     line_config(line::direction direction = line::direction::INPUT,
+                 line::edge edge = line::edge::NONE,
+                 bool active_low = false,
+                 const line::value_mappings& output_values = no_output_values(),
+                 line::bias bias = line::bias::AS_IS,
+                 line::drive drive = line::drive::PUSH_PULL,
+                 unsigned long debounce_period = 0,
+                 line::clock clock = line::clock::MONOTONIC);
+

output_values should be "int output_value = 0".
It is the default for all lines, not the setting for each line.

Quoted 20 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +20 -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;
+
+     /**
+      * @brief Move assignment operator.
+      * @param other Object to move.
+      * @return Reference to self.
+      */
+     line_config& operator=(line_config&& other) noexcept;
+
+     /**

bindings/cxx/gpiodcxx/edge-event-buffer.hpp

Quoted 10 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +10 -0

+      * @brief Reset the line config object.
+      */
+     void reset(void) noexcept;
+
+     /**
+      * @brief Set the default direction setting.
+      * @param direction New direction.
+      */
+     void set_direction(line::direction direction);
+

As per my comments in patch 01, this should set the direction for all lines - clearing any overrides.

Similarly other "default" setters.

Quoted 5 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +4 -0

+     /**
+      * @brief Set the direction for a single line at given offset.
+      * @param direction New direction.
+      * @param offset Offset of the line for which to set the direction.

bindings/cxx/gpiodcxx/edge-event-buffer.hpp

Quoted 10 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +10 -0

+      */
+     void set_direction(line::direction direction, unsigned int offset);
+
+     /**
+      * @brief Set the direction for a subset of offsets.
+      * @param direction New direction.
+      * @param offsets Vector of line offsets for which to set the direction.
+      */
+     void set_direction(line::direction direction, const line::offsets& offsets);
+

s/subset of offsets/subset of lines/g

@param Offsets of the lines to set.

Similarly elsewhere, i.e.
s/Vector of line offsets for which to set the <attr>/Offsets of the lines to set/g

bindings/cxx/gpiodcxx/edge-event-buffer.hpp

Quoted 7 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +7 -0

+     /**
+      * @brief Get the default direction setting.
+      * @return Direction setting that would have been used for any offset
+      *         not assigned its own direction value.
+      */
+     line::direction direction(void) const;
+

s/used for any offset/used for any line/g

bindings/cxx/gpiodcxx/edge-event-buffer.hpp

Quoted 9 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +9 -0

+     /**
+      * @brief Get the direction setting for a given offset.
+      * @param offset Line offset for which to read the direction setting.
+      * @return Direction setting that would have been used for given offset
+      *         if the config object was used in a request at the time of
+      *         the call.
+      */
+     line::direction direction(unsigned int offset) const;
+

s/setting for a given offset/setting for a line/g

@param offset Offset of the line to read.
Similarly elsewhere.

s/used for given offset/used for the line/g

bindings/cxx/gpiodcxx/edge-event-buffer.hpp

Quoted 6 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +6 -0

+     /**
+      * @brief Set the default edge event detection.
+      * @param edge Type of edge events to detect.
+      */
+     void set_edge_detection(line::edge edge);
+

s/edge event detection/edge detection/g

<snip>

bindings/cxx/gpiodcxx/edge-event-buffer.hpp

Quoted 8 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +8 -0

+
+     /**
+      * @brief Get the offsets for which this config object contains at
+      *        least one setting that is different from the defaults.
+      * @return Vector of line offsets with at least one overridden setting.
+      */
+     line::offsets overridden_offsets(void) const;
+

As per patch 01, shouldn't filter overrides that match the defaults.

bindings/cxx/gpiodcxx/edge-event-buffer.hpp

Quoted 7 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +7 -0

+     /**
+      * @brief Convenience helper value for passing an empty list of
+      *        offset-to-value mappings to methods of this class.
+      * @return Empty line->value mapping vector.
+      */
+     static const line::value_mappings& no_output_values(void);
+

Does this still have a purpose?

<snip>

bindings/cxx/gpiodcxx/edge-event-buffer.hpp

Quoted 6 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +6 -0

+     /**
+      * @brief Move assignment operator.
+      * @param other Object to move.
+      */
+     line_request& operator=(line_request&& other) noexcept;
+

@return ?

bindings/cxx/gpiodcxx/edge-event-buffer.hpp

Quoted 10 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +10 -0

+     /**
+      * @brief Check if this object is valid.
+      * @return True if this object's methods can be used, false otherwise.
+      *         False usually means the request was released. If the user
+      *         calls any of the methods of this class on an object for
+      *         which this operator returned false, a logic_error will be
+      *         thrown.
+      */
+     explicit operator bool(void) const noexcept;
+

It should throw request_released. That exception is defined but not used.

Quoted 25 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +25 -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 Get the number of requested lines.
+      * @return Number of lines in this request.
+      */
+     unsigned int num_lines(void) const;
+
+     /**
+      * @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;
+
+     /**
+      * @brief Get 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/edge-event-buffer.hpp

Quoted 10 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +10 -0

+     int get_value(unsigned int offset);
+
+     /**
+      * @brief Get 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);
+

@return Line values in the same order as offsets.

Quoted 7 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +7 -0

+     /**
+      * @brief Get the values of all requested lines.
+      * @return List of read values.
+      */
+     line::values get_values(void);
+
+     /**

bindings/cxx/gpiodcxx/edge-event-buffer.hpp

Quoted 10 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +10 -0

+      * @brief Get 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);
+

s/vector/container/

Similarly elsewhere - don't specify the nature of the container unless you really need to.

bindings/cxx/gpiodcxx/edge-event-buffer.hpp

Quoted 8 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +8 -0

+     /**
+      * @brief Get the values of all requested lines.
+      * @param values Array in which the values will be stored. Must hold
+      *               at least the number of elements returned by
+      *               line_request::num_lines.
+      */
+     void get_values(line::values& values);
+

Container in which

bindings/cxx/gpiodcxx/edge-event-buffer.hpp

Quoted 7 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +7 -0

+     /**
+      * @brief Set the value of a single requested line.
+      * @param offset Offset of the line to set within the chip.
+      * @param value New line value.
+      */
+     void set_value(unsigned int offset, int value);
+

s/within the chip//

Quoted 4 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +4 -0

+     /**
+      * @brief Set the values of a subset of requested lines.
+      * @param values Vector containing a set of offset->value mappings.
+      */

bindings/cxx/gpiodcxx/edge-event-buffer.hpp

Quoted 10 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +10 -0

+     void set_values(const line::value_mappings& values);
+
+     /**
+      * @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);
+

@param values New values in same order as offsets.

Quoted 27 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +27 -0

+     /**
+      * @brief Set the values of all requested lines.
+      * @param values Array of new line values. The size must be equal to
+      *               the value returned by line_request::num_lines.
+      */
+     void set_values(const line::values& values);
+
+     /**
+      * @brief Apply new config options to requested lines.
+      * @param config New configuration.
+      */
+     void reconfigure_lines(const line_config& config);
+
+     /**
+      * @brief Get the file descriptor number associated with this line
+      *        request.
+      * @return File descriptor number.
+      */
+     int 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.
+      */

bindings/cxx/gpiodcxx/edge-event-buffer.hpp

Quoted 10 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +10 -0

+     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);
+

@note This method can block unless edge_event_wait() returns true.

bindings/cxx/gpiodcxx/edge-event-buffer.hpp

Quoted 9 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +9 -0

+     /**
+      * @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);
+

and again here.

<snip>

bindings/cxx/gpiodcxx/edge-event-buffer.hpp

Quoted 6 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +6 -0

+     /**
+      * @brief Set line offsets for this request.
+      * @param offsets Vector of line offsets to request.
+      */
+     void set_offsets(const line::offsets& offsets) noexcept;
+

@brief Set the offsets of the lines for this request config.

Leaning towards renaming this set_lines(), and similarly elsewhere including the C API, but I can live with it as all it is setting is the offsets of the lines.

bindings/cxx/gpiodcxx/edge-event-buffer.hpp

Quoted 12 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +12 -0

+     /**
+      * @brief Get the number of offsets configured in this request config.
+      * @return Number of line offsets in this request config.
+      */
+     unsigned int num_offsets(void) const noexcept;
+
+     /**
+      * @brief Get the hardware offsets of lines in this request config.
+      * @return List of line offsets.
+      */
+     line::offsets offsets(void) const;
+

Drop "hardware" as it implies there is more than one type of offset.

<snip>

Quoted 32 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +32 -0

+GPIOD_CXX_API ::std::ostream& operator<<(::std::ostream& out, const line_config& config)
+{
+     out << "line_config(defaults=(direction=" << direction_names.at(config.direction()) <<
+            "edge_detection=" << edge_names.at(config.edge_detection()) <<
+            "bias=" << bias_names.at(config.bias()) <<
+            "drive=" << drive_names.at(config.drive()) <<
+            (config.active_low() ? "active-low" : "active-high") <<
+            "debounce_period=" << config.debounce_period_us() << "us" <<
+            "event_clock=" << clock_names.at(config.event_clock()) <<
+            "default_output_value=" << config.output_value() <<
+            ")";
+
+     if (config.num_overridden_offsets()) {
+             out << "overrides=[";
+
+             for (const auto& offset: config.overridden_offsets()) {
+                     out << "(offset=" << offset <<
+                            "direction=" << direction_names.at(config.direction(offset)) <<
+                            "edge_detection=" << edge_names.at(config.edge_detection(offset)) <<
+                            "bias=" << bias_names.at(config.bias(offset)) <<
+                            "drive=" << drive_names.at(config.drive(offset)) <<
+                            (config.active_low(offset) ? "active-low" : "active-high") <<
+                            "debounce_period=" << config.debounce_period_us(offset) <<
+                            "event_clock=" << clock_names.at(config.event_clock(offset)) <<
+                            "output_value=" << config.output_value(offset) <<
+                            ")";
+             }
+
+             out << "]";
+     }
+
+     out << ")";

bindings/cxx/gpiodcxx/edge-event-buffer.hpp

Quoted 4 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +4 -0

+
+     return out;
+}
+

This stringification is both too verbose (reporting fields that may not be overridden) and too terse (not reporting lines that are overridden with the default value).

What are you wanting to display here - the full state (what the user has set) or the effective state (what the kernel will see)?
If the former then include the defaults and all overrides - even when overridden to the default - but not attributes that are not overridden.
If the latter then as per the full state but filter out overrides that correspond to the default or that are ineffective (e.g. the default edge_detection when all lines are outputs). And maybe filter out the defaults that match the kernel defaults?

<snip>

bindings/cxx/gpiodcxx/edge-event-buffer.hpp

Quoted 6 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp 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");
+}
+

Should throw request_released.

<snip>

bindings/cxx/gpiodcxx/edge-event-buffer.hpp

Quoted 4 lines

bindings/cxx/gpiodcxx/edge-event-buffer.hpp new file +4 -0

+GPIOD_CXX_API int line_request::get_value(unsigned int offset)
+{
+     return this->get_values({ offset }).front();
+}

The one-liner is nice and brief, but is it really better than just calling ::gpiod_line_request_get_value() directly? That would avoid the temp vectors and exercise a bit more of the C API.

Similarly set_value().

Cheers,
Kent.

Quoted 8 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.

Quoted 6 lines

I dislike the squashing approach. I would rather accept that this branch is unstable and keep the patches separate.
If you want stability at each commit then disable the building of any unstable sections until they are fit to be included in the build again, rather than squashing everything into one humungous patch.

Ok, sounds good, will change the approach.

Quoted 4 lines

I'd rather see working tests than working examples :-(, unless you think your API is still in flux? Separate patches for API, implementation, tests, and examples would work for me.

I'd love to start writing tests but I just can't get the gpio-sim upstream. :( I consider dropping the blocking configfs changes and just implementing the "committing" part locally with a dedicated attribute.

Quoted 32 lines

Quoted 32 lines

bindings/cxx/line-config.cpp new file +32 -0

+GPIOD_CXX_API ::std::ostream& operator<<(::std::ostream& out, const line_config& config)
+{
+     out << "line_config(defaults=(direction=" << direction_names.at(config.direction()) <<
+            "edge_detection=" << edge_names.at(config.edge_detection()) <<
+            "bias=" << bias_names.at(config.bias()) <<
+            "drive=" << drive_names.at(config.drive()) <<
+            (config.active_low() ? "active-low" : "active-high") <<
+            "debounce_period=" << config.debounce_period_us() << "us" <<
+            "event_clock=" << clock_names.at(config.event_clock()) <<
+            "default_output_value=" << config.output_value() <<
+            ")";
+
+     if (config.num_overridden_offsets()) {
+             out << "overrides=[";
+
+             for (const auto& offset: config.overridden_offsets()) {
+                     out << "(offset=" << offset <<
+                            "direction=" << direction_names.at(config.direction(offset)) <<
+                            "edge_detection=" << edge_names.at(config.edge_detection(offset)) <<
+                            "bias=" << bias_names.at(config.bias(offset)) <<
+                            "drive=" << drive_names.at(config.drive(offset)) <<
+                            (config.active_low(offset) ? "active-low" : "active-high") <<
+                            "debounce_period=" << config.debounce_period_us(offset) <<
+                            "event_clock=" << clock_names.at(config.event_clock(offset)) <<
+                            "output_value=" << config.output_value(offset) <<
+                            ")";
+             }
+
+             out << "]";
+     }
+
+     out << ")";

Quoted 9 lines

bindings/cxx/edge-event-buffer.cpp line 117

Quoted 4 lines

bindings/cxx/edge-event-buffer.cpp new file +4 -0

+
+     return out;
+}
+

This stringification is both too verbose (reporting fields that may not be overridden) and too terse (not reporting lines that are overridden with the default value).

Oh... I think I get it now. If we override the line with the current default value, then change the default value, the overridden line should keep it's overridden value. Is that it?

Quoted 9 lines

What are you wanting to display here - the full state (what the user has set) or the effective state (what the kernel will see)?
If the former then include the defaults and all overrides - even when overridden to the default - but not attributes that are not overridden.
If the latter then as per the full state but filter out overrides that correspond to the default or that are ineffective (e.g. the default edge_detection when all lines are outputs). And maybe filter out the defaults that match the kernel defaults?

The former of course, we don't want to expose the kernel logic to users.