[libgpiod,v2.0] core: extend config objects

Kent suggested that we may want to add getters for the config objects in
his reviews under the C++ patches. Indeed when working on Python bindings
I noticed it would be useful for implementing __str__ and __repr__
callbacks. In C++ too we could use them for overloading stream operators.

This extends the config objects with getters. They are straightforward for
the request config but for the line config, they allow to only read
per-offset values that would be used if the object was used in a request
at this moment. We also add getters for the output values: both taking
the line offset as argument as well as ones that take the index and allow
to iterate over all configured output values.

The sanitization of input for the getters has subsequently been changed
so that we never return invalid values. The input values are verified
immediately and if an invalid value is passed, it's silently replaced
by the default value for given setting.

This patch also adds the reset function for the line config object - it
can be used to reset all stored configuration if - for example - the
config has become too complex.

As this patch will be squashed into the big v2 patch anyway, I allowed
myself to include some additional changes: some variable renames and
other minor tweaks.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 include/gpiod.h      | 189 +++++++++++++++++++++++-
 lib/line-config.c    | 340 ++++++++++++++++++++++++++++++++++---------
 lib/request-config.c |  42 +++++-
 3 files changed, 487 insertions(+), 84 deletions(-)

On Fri, Aug 06, 2021 at 03:28:10PM +0200, Bartosz Golaszewski wrote:
> Kent suggested that we may want to add getters for the config objects in
> his reviews under the C++ patches. Indeed when working on Python bindings
> I noticed it would be useful for implementing __str__ and __repr__
> callbacks. In C++ too we could use them for overloading stream operators.
> 
> This extends the config objects with getters. They are straightforward for
> the request config but for the line config, they allow to only read
> per-offset values that would be used if the object was used in a request
> at this moment. We also add getters for the output values: both taking
> the line offset as argument as well as ones that take the index and allow
> to iterate over all configured output values.
> 
> The sanitization of input for the getters has subsequently been changed
> so that we never return invalid values. The input values are verified
> immediately and if an invalid value is passed, it's silently replaced
> by the default value for given setting.
> 
> This patch also adds the reset function for the line config object - it
> can be used to reset all stored configuration if - for example - the
> config has become too complex.
> 
> As this patch will be squashed into the big v2 patch anyway, I allowed
> myself to include some additional changes: some variable renames and
> other minor tweaks.
> 
> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>

A few minor nit-picks in the gpiod.h documentation below...

Cheers,
Kent.

> ---
>  include/gpiod.h      | 189 +++++++++++++++++++++++-
>  lib/line-config.c    | 340 ++++++++++++++++++++++++++++++++++---------
>  lib/request-config.c |  42 +++++-
>  3 files changed, 487 insertions(+), 84 deletions(-)
> 
> diff --git a/include/gpiod.h b/include/gpiod.h
> index 885b472..d186df7 100644
> --- a/include/gpiod.h
> +++ b/include/gpiod.h
> @@ -451,11 +451,11 @@ 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 configuration is invalid - the set of options is too complex
> - * to be translated into kernel uAPI structures or invalid values have been
> - * passed to any of the functions - the error will be returned at the time of
> - * the request or reconfiguration. Each option can be set globally, for
> - * a single line offset or for multiple line offsets.
> + * 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.
>   */
>  
>  /**
> @@ -470,6 +470,15 @@ struct gpiod_line_config *gpiod_line_config_new(void);
>   */
>  void gpiod_line_config_free(struct gpiod_line_config *config);
>  
> +/**
> + * @brief Reset the line config object.
> + * @param config Line config object to free.
> + *
> + * Resets the entire configuration stored in this object. This is useful if
> + * the user wants to reuse the object without reallocating it.
> + */
> +void gpiod_line_config_reset(struct gpiod_line_config *config);
> +
>  /**
>   * @brief Set the direction of all lines.
>   * @param config Line config object.
> @@ -499,6 +508,18 @@ void gpiod_line_config_set_direction_subset(struct gpiod_line_config *config,
>  					    unsigned int num_offsets,
>  					    const unsigned int *offsets);
>  
> +/**
> + * @brief Get the direction setting for the line at given offset.

"for a given line" reads better for me, especially when combined with a
minor tweak to the offset description below.

> + * @param config Line config object.
> + * @param offset Line offset for which to read the direction setting.

"Offset of the line from which to read".  Throughout.

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

No comma necessary - "global default value" is fine. Throughout.

> +int gpiod_line_config_get_direction(struct gpiod_line_config *config,
> +				    unsigned int offset);
> +
>  /**
>   * @brief Set the edge event detection for all lines.
>   * @param config Line config object.
> @@ -529,6 +550,19 @@ gpiod_line_config_set_edge_detection_subset(struct gpiod_line_config *config,
>  					    int edge, unsigned int num_offsets,
>  					    const unsigned int *offsets);
>  
> +/**
> + * @brief Get the edge event detection setting for the line at 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);
> +
>  /**
>   * @brief Set the bias of all lines.
>   * @param config Line config object.
> @@ -556,6 +590,18 @@ void gpiod_line_config_set_bias_subset(struct gpiod_line_config *config,
>  				       int bias, unsigned int num_offsets,
>  				       const unsigned int *offsets);
>  
> +/**
> + * @brief Get the bias setting for the line at 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,
> +			       unsigned int offset);
> +
>  /**
>   * @brief Set the drive of all lines.
>   * @param config Line config object.
> @@ -583,6 +629,18 @@ void gpiod_line_config_set_drive_subset(struct gpiod_line_config *config,
>  					int drive, unsigned int num_offsets,
>  					const unsigned int *offsets);
>  
> +/**
> + * @brief Get the drive setting for the line at 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);
> +
>  /**
>   * @brief Set all lines as active-low.
>   * @param config Line config object.
> @@ -607,6 +665,18 @@ void gpiod_line_config_set_active_low_subset(struct gpiod_line_config *config,
>  					     unsigned int num_offsets,
>  					     const unsigned int *offsets);
>  
> +/**
> + * @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_is_active_low(struct gpiod_line_config *config,
> +				     unsigned int offset);
> +
>  /**
>   * @brief Set all lines as active-high.
>   * @param config Line config object.
> @@ -663,6 +733,19 @@ gpiod_line_config_set_debounce_period_subset(struct gpiod_line_config *config,
>  					     unsigned int num_offsets,
>  					     const unsigned int *offsets);
>  
> +/**
> + * @brief Get the debounce period for the line at 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.
> + */
> +unsigned long
> +gpiod_line_config_get_debounce_period(struct gpiod_line_config *config,
> +				      unsigned int offset);
> +
>  /**
>   * @brief Set the event timestamp clock for all lines.
>   * @param config Line config object.
> @@ -692,6 +775,18 @@ void gpiod_line_config_set_event_clock_subset(struct gpiod_line_config *config,
>  					      unsigned int num_offsets,
>  					      const unsigned int *offsets);
>  
> +/**
> + * @brief Get the event clock setting for the line at 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);
> +
>  /**
>   * @brief Set the output value for a single offset.
>   * @param config Line config object.
> @@ -704,16 +799,63 @@ void gpiod_line_config_set_output_value(struct gpiod_line_config *config,
>  /**
>   * @brief Set the output values for a set of offsets.
>   * @param config Line config object.
> - * @param num_offsets Number of offsets for which to set values.
> + * @param num_values Number of offsets for which to set values.
>   * @param offsets Array of line offsets to set values for.
>   * @param values Array of output values associated with the offsets passed in
>   *               the previous argument.
>   */
>  void gpiod_line_config_set_output_values(struct gpiod_line_config *config,
> -					 unsigned int num_offsets,
> +					 unsigned int num_values,
>  					 const unsigned int *offsets,
>  					 const int *values);
>  
> +/**
> + * @brief Get the number of line offsets for which this config object stores
> + *        output values.
> + * @param config Line config object.
> + * @return Number of output values currently configured for this object.
> + */
> +unsigned int
> +gpiod_line_config_num_output_values(struct gpiod_line_config *config);
> +
> +/**
> + * @brief Get the output value configured for the line at given offset.
> + * @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.
> + */
> +int gpiod_line_config_get_output_value(struct gpiod_line_config *config,
> +				       unsigned int offset);
> +
> +/**
> + * @brief Get the output value mapping (offset -> value) at given index.
> + * @param config Line config object.
> + * @param index Position of the mapping in the internal array.
> + * @param offset Buffer for storing the offset of the line.
> + * @param value Buffer for storing the value mapped for the offset.

"value corresponding to the offset"

> + * @return Returns 0 on success, -1 if the index is out of range.
> + *
> + * This function together with ::gpiod_line_config_num_output_values allows to
> + * iterate over all output value mappings currently held by this object.
> + */
> +int gpiod_line_config_get_output_value_index(struct gpiod_line_config *config,
> +					     unsigned int index,
> +					     unsigned int *offset, int *value);
> +
> +/**
> + * @brief Get all output value mappings stored in this config object.
> + * @param config Line config object.
> + * @param offsets Buffer in which offsets will be stored.
> + * @param values Buffer in which values will be stored.
> + * @note Both the offsets and values buffers must be able to hold at least the
> + *       number of elements returned by ::gpiod_line_config_num_output_values.
> + *
> + * Each offset in the offsets array corresponds with the value in the values
> + * array at the same index.
> + */

"corresponds to", not with.

> +void gpiod_line_config_get_output_values(struct gpiod_line_config *config,
> +					 unsigned int *offsets, int *values);
> +
>  /**
>   * @}
>   *
> @@ -750,6 +892,14 @@ void gpiod_request_config_free(struct gpiod_request_config *config);
>  void gpiod_request_config_set_consumer(struct gpiod_request_config *config,
>  				       const char *consumer);
>  
> +/**
> + * @brief Get the consumer string.
> + * @param config Request config object.
> + * @return Current consumer string stored in this request config.
> + */
> +const char *
> +gpiod_request_config_get_consumer(struct gpiod_request_config *config);
> +
>  /**
>   * @brief Set line offsets for this request.
>   * @param config Request config object.
> @@ -762,6 +912,23 @@ void gpiod_request_config_set_offsets(struct gpiod_request_config *config,
>  				      unsigned int num_offsets,
>  				      const unsigned int *offsets);
>  
> +/**
> + * @brief Get the number of offsets configured in this request config.
> + * @param config Request config object.
> + * @return Number of line offsets in this request config.
> + */
> +unsigned int
> +gpiod_request_config_get_num_offsets(struct gpiod_request_config *config);
> +
> +/**
> + * @brief Get the hardware offsets of lines in this request config.
> + * @param config Request config object.
> + * @param offsets Array to store offsets in. Must hold at least the number of
> + *                lines returned by ::gpiod_request_config_get_num_offsets.
> + */

"Array to store offsets."

> +void gpiod_request_config_get_offsets(struct gpiod_request_config *config,
> +				      unsigned int *offsets);
> +
>  /**
>   * @brief Set the size of the kernel event buffer.
>   * @param config Request config object.
> @@ -773,6 +940,14 @@ void
>  gpiod_request_config_set_event_buffer_size(struct gpiod_request_config *config,
>  					   unsigned int event_buffer_size);
>  
> +/**
> + * @brief Get the edge event buffer size from this request config.
> + * @param config Request config object.
> + * @return Current edge event buffer size setting.
> + */
> +unsigned int
> +gpiod_request_config_get_event_buffer_size(struct gpiod_request_config *config);
> +
>  /**
>   * @}
>   *

On Sat, Aug 7, 2021 at 10:48 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Fri, Aug 06, 2021 at 03:28:10PM +0200, Bartosz Golaszewski wrote:
> > Kent suggested that we may want to add getters for the config objects in
> > his reviews under the C++ patches. Indeed when working on Python bindings
> > I noticed it would be useful for implementing __str__ and __repr__
> > callbacks. In C++ too we could use them for overloading stream operators.
> >
> > This extends the config objects with getters. They are straightforward for
> > the request config but for the line config, they allow to only read
> > per-offset values that would be used if the object was used in a request
> > at this moment. We also add getters for the output values: both taking
> > the line offset as argument as well as ones that take the index and allow
> > to iterate over all configured output values.
> >
> > The sanitization of input for the getters has subsequently been changed
> > so that we never return invalid values. The input values are verified
> > immediately and if an invalid value is passed, it's silently replaced
> > by the default value for given setting.
> >
> > This patch also adds the reset function for the line config object - it
> > can be used to reset all stored configuration if - for example - the
> > config has become too complex.
> >
> > As this patch will be squashed into the big v2 patch anyway, I allowed
> > myself to include some additional changes: some variable renames and
> > other minor tweaks.
> >
> > Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
>
> A few minor nit-picks in the gpiod.h documentation below...
>
> Cheers,
> Kent.
>

Thanks,

With that fixed, do you think it's good to be applied?

Bart

On Sun, Aug 08, 2021 at 09:11:14PM +0200, Bartosz Golaszewski wrote:
> On Sat, Aug 7, 2021 at 10:48 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Fri, Aug 06, 2021 at 03:28:10PM +0200, Bartosz Golaszewski wrote:
> > > Kent suggested that we may want to add getters for the config objects in
> > > his reviews under the C++ patches. Indeed when working on Python bindings
> > > I noticed it would be useful for implementing __str__ and __repr__
> > > callbacks. In C++ too we could use them for overloading stream operators.
> > >
> > > This extends the config objects with getters. They are straightforward for
> > > the request config but for the line config, they allow to only read
> > > per-offset values that would be used if the object was used in a request
> > > at this moment. We also add getters for the output values: both taking
> > > the line offset as argument as well as ones that take the index and allow
> > > to iterate over all configured output values.
> > >
> > > The sanitization of input for the getters has subsequently been changed
> > > so that we never return invalid values. The input values are verified
> > > immediately and if an invalid value is passed, it's silently replaced
> > > by the default value for given setting.
> > >
> > > This patch also adds the reset function for the line config object - it
> > > can be used to reset all stored configuration if - for example - the
> > > config has become too complex.
> > >
> > > As this patch will be squashed into the big v2 patch anyway, I allowed
> > > myself to include some additional changes: some variable renames and
> > > other minor tweaks.
> > >
> > > Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
> >
> > A few minor nit-picks in the gpiod.h documentation below...
> >
> > Cheers,
> > Kent.
> >
> 
> Thanks,
> 
> With that fixed, do you think it's good to be applied?
> 

Sure.

I was also wondering if anything could be done to simplify the
structures in line-config.c, but that isn't specific to this patch.
Not having access to the offsets, or even num_lines, and doing the
allocation up-front makes it rather painful.  Especially if the most
common case is only one or two lines.
But, as long as you are happy with the external API, that is just
implementation detail that can be optimised later.

Cheers,
Kent.

On Mon, Aug 9, 2021 at 1:10 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Sun, Aug 08, 2021 at 09:11:14PM +0200, Bartosz Golaszewski wrote:
> > On Sat, Aug 7, 2021 at 10:48 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > On Fri, Aug 06, 2021 at 03:28:10PM +0200, Bartosz Golaszewski wrote:
> > > > Kent suggested that we may want to add getters for the config objects in
> > > > his reviews under the C++ patches. Indeed when working on Python bindings
> > > > I noticed it would be useful for implementing __str__ and __repr__
> > > > callbacks. In C++ too we could use them for overloading stream operators.
> > > >
> > > > This extends the config objects with getters. They are straightforward for
> > > > the request config but for the line config, they allow to only read
> > > > per-offset values that would be used if the object was used in a request
> > > > at this moment. We also add getters for the output values: both taking
> > > > the line offset as argument as well as ones that take the index and allow
> > > > to iterate over all configured output values.
> > > >
> > > > The sanitization of input for the getters has subsequently been changed
> > > > so that we never return invalid values. The input values are verified
> > > > immediately and if an invalid value is passed, it's silently replaced
> > > > by the default value for given setting.
> > > >
> > > > This patch also adds the reset function for the line config object - it
> > > > can be used to reset all stored configuration if - for example - the
> > > > config has become too complex.
> > > >
> > > > As this patch will be squashed into the big v2 patch anyway, I allowed
> > > > myself to include some additional changes: some variable renames and
> > > > other minor tweaks.
> > > >
> > > > Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
> > >
> > > A few minor nit-picks in the gpiod.h documentation below...
> > >
> > > Cheers,
> > > Kent.
> > >
> >
> > Thanks,
> >
> > With that fixed, do you think it's good to be applied?
> >
>
> Sure.
>
> I was also wondering if anything could be done to simplify the
> structures in line-config.c, but that isn't specific to this patch.
> Not having access to the offsets, or even num_lines, and doing the
> allocation up-front makes it rather painful.  Especially if the most
> common case is only one or two lines.
> But, as long as you are happy with the external API, that is just
> implementation detail that can be optimised later.
>
> Cheers,
> Kent.

I'm fine with how it works now for most part. I understand your
concerns about splitting the output value configuration from the
requested offsets but I think we can live with it. If anything: we can
add a function to set an array of output values in line_config where
the offsets would be assigned automatically based on the index in the
array. Something like this:

int values[] = { 0, 0, 1, 1, 0, 1 };
unsigned int offsets[] = { 0, 3, 4, 7, 12, 13 };

gpiod_line_config_set_output_values_auto(line_cfg, 6, values);
gpiod_request_config_set_offsets(req_cfg, 6, offsets);

request = gpiod_chip_request_lines(chip, line_cfg, req_cfg);

This would result in the following mapping: 0 -> 0, 3 -> 0, 4 -> 1, 7
-> 1, 12 -> 0, 13 -> 1.

We could store the auto array as a separate array in line_config and
the offset -> value mappings would take precedence. For
reconfiguration we would do the same. Does this make sense?

Bart

On Tue, Aug 10, 2021 at 09:52:04AM +0200, Bartosz Golaszewski wrote:
> On Mon, Aug 9, 2021 at 1:10 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Sun, Aug 08, 2021 at 09:11:14PM +0200, Bartosz Golaszewski wrote:
> > > On Sat, Aug 7, 2021 at 10:48 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > >
> > > > On Fri, Aug 06, 2021 at 03:28:10PM +0200, Bartosz Golaszewski wrote:
> > > > > Kent suggested that we may want to add getters for the config objects in
> > > > > his reviews under the C++ patches. Indeed when working on Python bindings
> > > > > I noticed it would be useful for implementing __str__ and __repr__
> > > > > callbacks. In C++ too we could use them for overloading stream operators.
> > > > >
> > > > > This extends the config objects with getters. They are straightforward for
> > > > > the request config but for the line config, they allow to only read
> > > > > per-offset values that would be used if the object was used in a request
> > > > > at this moment. We also add getters for the output values: both taking
> > > > > the line offset as argument as well as ones that take the index and allow
> > > > > to iterate over all configured output values.
> > > > >
> > > > > The sanitization of input for the getters has subsequently been changed
> > > > > so that we never return invalid values. The input values are verified
> > > > > immediately and if an invalid value is passed, it's silently replaced
> > > > > by the default value for given setting.
> > > > >
> > > > > This patch also adds the reset function for the line config object - it
> > > > > can be used to reset all stored configuration if - for example - the
> > > > > config has become too complex.
> > > > >
> > > > > As this patch will be squashed into the big v2 patch anyway, I allowed
> > > > > myself to include some additional changes: some variable renames and
> > > > > other minor tweaks.
> > > > >
> > > > > Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
> > > >
> > > > A few minor nit-picks in the gpiod.h documentation below...
> > > >
> > > > Cheers,
> > > > Kent.
> > > >
> > >
> > > Thanks,
> > >
> > > With that fixed, do you think it's good to be applied?
> > >
> >
> > Sure.
> >
> > I was also wondering if anything could be done to simplify the
> > structures in line-config.c, but that isn't specific to this patch.
> > Not having access to the offsets, or even num_lines, and doing the
> > allocation up-front makes it rather painful.  Especially if the most
> > common case is only one or two lines.
> > But, as long as you are happy with the external API, that is just
> > implementation detail that can be optimised later.
> >
> > Cheers,
> > Kent.
> 
> I'm fine with how it works now for most part. I understand your
> concerns about splitting the output value configuration from the
> requested offsets but I think we can live with it. If anything: we can
> add a function to set an array of output values in line_config where
> the offsets would be assigned automatically based on the index in the
> array. Something like this:
> 
> int values[] = { 0, 0, 1, 1, 0, 1 };
> unsigned int offsets[] = { 0, 3, 4, 7, 12, 13 };
> 
> gpiod_line_config_set_output_values_auto(line_cfg, 6, values);
> gpiod_request_config_set_offsets(req_cfg, 6, offsets);
> 
> request = gpiod_chip_request_lines(chip, line_cfg, req_cfg);
> 
> This would result in the following mapping: 0 -> 0, 3 -> 0, 4 -> 1, 7
> -> 1, 12 -> 0, 13 -> 1.
> 
> We could store the auto array as a separate array in line_config and
> the offset -> value mappings would take precedence. For
> reconfiguration we would do the same. Does this make sense?
>

My preference would be for gpiod_line_config_set_output_value() and
variants to also set the direction for those lines to output.
And maybe rename it to gpiod_line_config_set_output().
And maybe add a set_input for symmetry.

But my concern above was actually the secondary array - that confused me.
And it's big - always. (OTOH it's on the heap so who cares?)
The array is of size GPIO_V2_LINE_NUM_ATTRS_MAX, yet each entry could
have multiple attributes set - so long as the offsets subsets match?
What happens if both debounce and flags are set for the same subset?
Looks like debounce wins and the flags get discarded in
gpiod_line_config_to_kernel().

What I had in mind for the config was an array of config for each line,
only performing the mapping to uAPI when the actual request or
reconfigure is performed, though that requires knowledge of the number
of lines in the request to be sized efficiently in the
gpiod_line_config_new().  Sizing it as GPIO_V2_LINES_MAX would be even
worse than your secondary array, so don't want that.
My Go library uses a map, but that isn't an option here.
Resizing it dynamically is the option I was last pondering,
but my preference would be to add a num_lines parameter to the new.
Anyway, that was what I was wondering about...

Cheers,
Kent.

On Tue, Aug 10, 2021 at 06:31:13PM +0800, Kent Gibson wrote:
> On Tue, Aug 10, 2021 at 09:52:04AM +0200, Bartosz Golaszewski wrote:
> > On Mon, Aug 9, 2021 at 1:10 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > On Sun, Aug 08, 2021 at 09:11:14PM +0200, Bartosz Golaszewski wrote:
> > > > On Sat, Aug 7, 2021 at 10:48 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > > >
> > > > > On Fri, Aug 06, 2021 at 03:28:10PM +0200, Bartosz Golaszewski wrote:
> > > > > > Kent suggested that we may want to add getters for the config objects in
> > > > > > his reviews under the C++ patches. Indeed when working on Python bindings
> > > > > > I noticed it would be useful for implementing __str__ and __repr__
> > > > > > callbacks. In C++ too we could use them for overloading stream operators.
> > > > > >
> > > > > > This extends the config objects with getters. They are straightforward for
> > > > > > the request config but for the line config, they allow to only read
> > > > > > per-offset values that would be used if the object was used in a request
> > > > > > at this moment. We also add getters for the output values: both taking
> > > > > > the line offset as argument as well as ones that take the index and allow
> > > > > > to iterate over all configured output values.
> > > > > >
> > > > > > The sanitization of input for the getters has subsequently been changed
> > > > > > so that we never return invalid values. The input values are verified
> > > > > > immediately and if an invalid value is passed, it's silently replaced
> > > > > > by the default value for given setting.
> > > > > >
> > > > > > This patch also adds the reset function for the line config object - it
> > > > > > can be used to reset all stored configuration if - for example - the
> > > > > > config has become too complex.
> > > > > >
> > > > > > As this patch will be squashed into the big v2 patch anyway, I allowed
> > > > > > myself to include some additional changes: some variable renames and
> > > > > > other minor tweaks.
> > > > > >
> > > > > > Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
> > > > >
> > > > > A few minor nit-picks in the gpiod.h documentation below...
> > > > >
> > > > > Cheers,
> > > > > Kent.
> > > > >
> > > >
> > > > Thanks,
> > > >
> > > > With that fixed, do you think it's good to be applied?
> > > >
> > >
> > > Sure.
> > >
> > > I was also wondering if anything could be done to simplify the
> > > structures in line-config.c, but that isn't specific to this patch.
> > > Not having access to the offsets, or even num_lines, and doing the
> > > allocation up-front makes it rather painful.  Especially if the most
> > > common case is only one or two lines.
> > > But, as long as you are happy with the external API, that is just
> > > implementation detail that can be optimised later.
> > >
> > > Cheers,
> > > Kent.
> > 
> > I'm fine with how it works now for most part. I understand your
> > concerns about splitting the output value configuration from the
> > requested offsets but I think we can live with it. If anything: we can
> > add a function to set an array of output values in line_config where
> > the offsets would be assigned automatically based on the index in the
> > array. Something like this:
> > 
> > int values[] = { 0, 0, 1, 1, 0, 1 };
> > unsigned int offsets[] = { 0, 3, 4, 7, 12, 13 };
> > 
> > gpiod_line_config_set_output_values_auto(line_cfg, 6, values);
> > gpiod_request_config_set_offsets(req_cfg, 6, offsets);
> > 
> > request = gpiod_chip_request_lines(chip, line_cfg, req_cfg);
> > 
> > This would result in the following mapping: 0 -> 0, 3 -> 0, 4 -> 1, 7
> > -> 1, 12 -> 0, 13 -> 1.
> > 
> > We could store the auto array as a separate array in line_config and
> > the offset -> value mappings would take precedence. For
> > reconfiguration we would do the same. Does this make sense?
> >
> 
> My preference would be for gpiod_line_config_set_output_value() and
> variants to also set the direction for those lines to output.
> And maybe rename it to gpiod_line_config_set_output().
> And maybe add a set_input for symmetry.
> 
> But my concern above was actually the secondary array - that confused me.
> And it's big - always. (OTOH it's on the heap so who cares?)
> The array is of size GPIO_V2_LINE_NUM_ATTRS_MAX, yet each entry could
> have multiple attributes set - so long as the offsets subsets match?
> What happens if both debounce and flags are set for the same subset?
> Looks like debounce wins and the flags get discarded in
> gpiod_line_config_to_kernel().
> 
> What I had in mind for the config was an array of config for each line,
> only performing the mapping to uAPI when the actual request or
> reconfigure is performed, though that requires knowledge of the number
> of lines in the request to be sized efficiently in the
> gpiod_line_config_new().  Sizing it as GPIO_V2_LINES_MAX would be even
> worse than your secondary array, so don't want that.
> My Go library uses a map, but that isn't an option here.
> Resizing it dynamically is the option I was last pondering,
> but my preference would be to add a num_lines parameter to the new.
> Anyway, that was what I was wondering about...
> 

Also, what is the state of the config after this call sequence:

gpiod_line_config_set_active_high_offset(cfg,1)
gpiod_line_config_set_active_low(cfg)

Is line 1 active high or low?

The docs indicate that it should be active low, but the current
line-config.c implementation will leave it active high as the secondary
config is not changed by gpiod_line_config_set_active_low()?

Cheers,
Kent.

On Tue, Aug 10, 2021 at 12:31 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Tue, Aug 10, 2021 at 09:52:04AM +0200, Bartosz Golaszewski wrote:
> > On Mon, Aug 9, 2021 at 1:10 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > On Sun, Aug 08, 2021 at 09:11:14PM +0200, Bartosz Golaszewski wrote:
> > > > On Sat, Aug 7, 2021 at 10:48 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > > >
> > > > > On Fri, Aug 06, 2021 at 03:28:10PM +0200, Bartosz Golaszewski wrote:
> > > > > > Kent suggested that we may want to add getters for the config objects in
> > > > > > his reviews under the C++ patches. Indeed when working on Python bindings
> > > > > > I noticed it would be useful for implementing __str__ and __repr__
> > > > > > callbacks. In C++ too we could use them for overloading stream operators.
> > > > > >
> > > > > > This extends the config objects with getters. They are straightforward for
> > > > > > the request config but for the line config, they allow to only read
> > > > > > per-offset values that would be used if the object was used in a request
> > > > > > at this moment. We also add getters for the output values: both taking
> > > > > > the line offset as argument as well as ones that take the index and allow
> > > > > > to iterate over all configured output values.
> > > > > >
> > > > > > The sanitization of input for the getters has subsequently been changed
> > > > > > so that we never return invalid values. The input values are verified
> > > > > > immediately and if an invalid value is passed, it's silently replaced
> > > > > > by the default value for given setting.
> > > > > >
> > > > > > This patch also adds the reset function for the line config object - it
> > > > > > can be used to reset all stored configuration if - for example - the
> > > > > > config has become too complex.
> > > > > >
> > > > > > As this patch will be squashed into the big v2 patch anyway, I allowed
> > > > > > myself to include some additional changes: some variable renames and
> > > > > > other minor tweaks.
> > > > > >
> > > > > > Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
> > > > >
> > > > > A few minor nit-picks in the gpiod.h documentation below...
> > > > >
> > > > > Cheers,
> > > > > Kent.
> > > > >
> > > >
> > > > Thanks,
> > > >
> > > > With that fixed, do you think it's good to be applied?
> > > >
> > >
> > > Sure.
> > >
> > > I was also wondering if anything could be done to simplify the
> > > structures in line-config.c, but that isn't specific to this patch.
> > > Not having access to the offsets, or even num_lines, and doing the
> > > allocation up-front makes it rather painful.  Especially if the most
> > > common case is only one or two lines.
> > > But, as long as you are happy with the external API, that is just
> > > implementation detail that can be optimised later.
> > >
> > > Cheers,
> > > Kent.
> >
> > I'm fine with how it works now for most part. I understand your
> > concerns about splitting the output value configuration from the
> > requested offsets but I think we can live with it. If anything: we can
> > add a function to set an array of output values in line_config where
> > the offsets would be assigned automatically based on the index in the
> > array. Something like this:
> >
> > int values[] = { 0, 0, 1, 1, 0, 1 };
> > unsigned int offsets[] = { 0, 3, 4, 7, 12, 13 };
> >
> > gpiod_line_config_set_output_values_auto(line_cfg, 6, values);
> > gpiod_request_config_set_offsets(req_cfg, 6, offsets);
> >
> > request = gpiod_chip_request_lines(chip, line_cfg, req_cfg);
> >
> > This would result in the following mapping: 0 -> 0, 3 -> 0, 4 -> 1, 7
> > -> 1, 12 -> 0, 13 -> 1.
> >
> > We could store the auto array as a separate array in line_config and
> > the offset -> value mappings would take precedence. For
> > reconfiguration we would do the same. Does this make sense?
> >
>
> My preference would be for gpiod_line_config_set_output_value() and
> variants to also set the direction for those lines to output.
> And maybe rename it to gpiod_line_config_set_output().
> And maybe add a set_input for symmetry.
>

Any naming idea for setting the direction to "AS_IS"?

> But my concern above was actually the secondary array - that confused me.
> And it's big - always. (OTOH it's on the heap so who cares?)
> The array is of size GPIO_V2_LINE_NUM_ATTRS_MAX, yet each entry could
> have multiple attributes set - so long as the offsets subsets match?
> What happens if both debounce and flags are set for the same subset?
> Looks like debounce wins and the flags get discarded in
> gpiod_line_config_to_kernel().
>

Yeah, I aimed at ironing it out when writing tests. You're probably right here.

> What I had in mind for the config was an array of config for each line,
> only performing the mapping to uAPI when the actual request or
> reconfigure is performed, though that requires knowledge of the number
> of lines in the request to be sized efficiently in the
> gpiod_line_config_new().  Sizing it as GPIO_V2_LINES_MAX would be even
> worse than your secondary array, so don't want that.

Or would it? Currently the full config structure is 3784 bytes on a 64
bit arch. The base config is 32 bytes. If we added the default value
to base_config that would make it 36 bytes x GPIO_V2_LINES_MAX = 2304
bytes. We'd need another base_config for default settings.

Unless I'm missing something this does seem like the better option.

> My Go library uses a map, but that isn't an option here.
> Resizing it dynamically is the option I was last pondering,
> but my preference would be to add a num_lines parameter to the new.
> Anyway, that was what I was wondering about...
>

We could resize the array dynamically but we'd need to return error
codes from getters. We could also define the size when allocating the
config but it's a sub-par approach too.

Bart

On Thu, Aug 12, 2021 at 09:24:28AM +0200, Bartosz Golaszewski wrote:
> On Tue, Aug 10, 2021 at 12:31 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Tue, Aug 10, 2021 at 09:52:04AM +0200, Bartosz Golaszewski wrote:
> > > On Mon, Aug 9, 2021 at 1:10 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > >
> > > > On Sun, Aug 08, 2021 at 09:11:14PM +0200, Bartosz Golaszewski wrote:
> > > > > On Sat, Aug 7, 2021 at 10:48 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > > > >
> > > > > > On Fri, Aug 06, 2021 at 03:28:10PM +0200, Bartosz Golaszewski wrote:
> > > > > > > Kent suggested that we may want to add getters for the config objects in
> > > > > > > his reviews under the C++ patches. Indeed when working on Python bindings
> > > > > > > I noticed it would be useful for implementing __str__ and __repr__
> > > > > > > callbacks. In C++ too we could use them for overloading stream operators.
> > > > > > >
> > > > > > > This extends the config objects with getters. They are straightforward for
> > > > > > > the request config but for the line config, they allow to only read
> > > > > > > per-offset values that would be used if the object was used in a request
> > > > > > > at this moment. We also add getters for the output values: both taking
> > > > > > > the line offset as argument as well as ones that take the index and allow
> > > > > > > to iterate over all configured output values.
> > > > > > >
> > > > > > > The sanitization of input for the getters has subsequently been changed
> > > > > > > so that we never return invalid values. The input values are verified
> > > > > > > immediately and if an invalid value is passed, it's silently replaced
> > > > > > > by the default value for given setting.
> > > > > > >
> > > > > > > This patch also adds the reset function for the line config object - it
> > > > > > > can be used to reset all stored configuration if - for example - the
> > > > > > > config has become too complex.
> > > > > > >
> > > > > > > As this patch will be squashed into the big v2 patch anyway, I allowed
> > > > > > > myself to include some additional changes: some variable renames and
> > > > > > > other minor tweaks.
> > > > > > >
> > > > > > > Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
> > > > > >
> > > > > > A few minor nit-picks in the gpiod.h documentation below...
> > > > > >
> > > > > > Cheers,
> > > > > > Kent.
> > > > > >
> > > > >
> > > > > Thanks,
> > > > >
> > > > > With that fixed, do you think it's good to be applied?
> > > > >
> > > >
> > > > Sure.
> > > >
> > > > I was also wondering if anything could be done to simplify the
> > > > structures in line-config.c, but that isn't specific to this patch.
> > > > Not having access to the offsets, or even num_lines, and doing the
> > > > allocation up-front makes it rather painful.  Especially if the most
> > > > common case is only one or two lines.
> > > > But, as long as you are happy with the external API, that is just
> > > > implementation detail that can be optimised later.
> > > >
> > > > Cheers,
> > > > Kent.
> > >
> > > I'm fine with how it works now for most part. I understand your
> > > concerns about splitting the output value configuration from the
> > > requested offsets but I think we can live with it. If anything: we can
> > > add a function to set an array of output values in line_config where
> > > the offsets would be assigned automatically based on the index in the
> > > array. Something like this:
> > >
> > > int values[] = { 0, 0, 1, 1, 0, 1 };
> > > unsigned int offsets[] = { 0, 3, 4, 7, 12, 13 };
> > >
> > > gpiod_line_config_set_output_values_auto(line_cfg, 6, values);
> > > gpiod_request_config_set_offsets(req_cfg, 6, offsets);
> > >
> > > request = gpiod_chip_request_lines(chip, line_cfg, req_cfg);
> > >
> > > This would result in the following mapping: 0 -> 0, 3 -> 0, 4 -> 1, 7
> > > -> 1, 12 -> 0, 13 -> 1.
> > >
> > > We could store the auto array as a separate array in line_config and
> > > the offset -> value mappings would take precedence. For
> > > reconfiguration we would do the same. Does this make sense?
> > >
> >
> > My preference would be for gpiod_line_config_set_output_value() and
> > variants to also set the direction for those lines to output.
> > And maybe rename it to gpiod_line_config_set_output().
> > And maybe add a set_input for symmetry.
> >
> 
> Any naming idea for setting the direction to "AS_IS"?
> 

Since as-is is vague you need to include the field name.
So I would remove set_direction and replace it with set_input, set_output
and set_direction_as_is (which I would expect to see used very rarely in
the wild, as the only use case I can think of for it is undoing a
set_input or set_output call).

> > But my concern above was actually the secondary array - that confused me.
> > And it's big - always. (OTOH it's on the heap so who cares?)
> > The array is of size GPIO_V2_LINE_NUM_ATTRS_MAX, yet each entry could
> > have multiple attributes set - so long as the offsets subsets match?
> > What happens if both debounce and flags are set for the same subset?
> > Looks like debounce wins and the flags get discarded in
> > gpiod_line_config_to_kernel().
> >
> 
> Yeah, I aimed at ironing it out when writing tests. You're probably right here.
> 

Same reason I hadn't paid much attention to the implementation.

> > What I had in mind for the config was an array of config for each line,
> > only performing the mapping to uAPI when the actual request or
> > reconfigure is performed, though that requires knowledge of the number
> > of lines in the request to be sized efficiently in the
> > gpiod_line_config_new().  Sizing it as GPIO_V2_LINES_MAX would be even
> > worse than your secondary array, so don't want that.
> 
> Or would it? Currently the full config structure is 3784 bytes on a 64
> bit arch. The base config is 32 bytes. If we added the default value
> to base_config that would make it 36 bytes x GPIO_V2_LINES_MAX = 2304
> bytes. We'd need another base_config for default settings.
> 
> Unless I'm missing something this does seem like the better option.
> 

Yikes, I overlooked the size of the offsets array in the secondary
config - that is a significant contributor to the config size as well.
For some reason I was thinking that was a bitmap, but that couldn't work.

In that case a GPIO_V2_LINES_MAX sized array is clearly a better way to
go, and that is a surprise.
Though those array elements will require the line offset as well as the
base_config, unless you have some other way to map offset to config?

> > My Go library uses a map, but that isn't an option here.
> > Resizing it dynamically is the option I was last pondering,
> > but my preference would be to add a num_lines parameter to the new.
> > Anyway, that was what I was wondering about...
> >
> 
> We could resize the array dynamically but we'd need to return error
> codes from getters.

Why? If there is no config for the requested line then you return the
global default value, right?
Why does that change if the array is resizable?
Btw, I'm assuming that the gpiod_line_config would contain a pointer to
the dynamic array, so the handle the user has would remain unchanged.
And the getters all return ints, not pointers to internal fields.
What am I missing?

Also, "global default value" is different from the primary, right?
Perhaps getters should return the primary value, which itself defaults
to the global defaults, if the line doesn't have specific config?

> We could also define the size when allocating the
> config but it's a sub-par approach too.
> 

Sure, it's a trade-off, but the alternative is requiring a 2-3k block
even for a one line request, which seems a wee bit excessive.

With the proposed API, the only other alternative I can see to give a
small footprint is dynamic resizing, which I'm not thrilled by either.
So just wanted to double check that you are content to lock in the
gpiod_line_config_new API, as that will constrain any optimisation later
on.

Cheers,
Kent.

On Thu, Aug 12, 2021 at 12:29 PM Kent Gibson <warthog618@gmail.com> wrote:
>

[snip]

> > >
> > > My preference would be for gpiod_line_config_set_output_value() and
> > > variants to also set the direction for those lines to output.
> > > And maybe rename it to gpiod_line_config_set_output().
> > > And maybe add a set_input for symmetry.
> > >
> >
> > Any naming idea for setting the direction to "AS_IS"?
> >
>
> Since as-is is vague you need to include the field name.
> So I would remove set_direction and replace it with set_input, set_output
> and set_direction_as_is (which I would expect to see used very rarely in
> the wild, as the only use case I can think of for it is undoing a
> set_input or set_output call).
>
> > > But my concern above was actually the secondary array - that confused me.
> > > And it's big - always. (OTOH it's on the heap so who cares?)
> > > The array is of size GPIO_V2_LINE_NUM_ATTRS_MAX, yet each entry could
> > > have multiple attributes set - so long as the offsets subsets match?
> > > What happens if both debounce and flags are set for the same subset?
> > > Looks like debounce wins and the flags get discarded in
> > > gpiod_line_config_to_kernel().
> > >
> >
> > Yeah, I aimed at ironing it out when writing tests. You're probably right here.
> >
>
> Same reason I hadn't paid much attention to the implementation.
>
> > > What I had in mind for the config was an array of config for each line,
> > > only performing the mapping to uAPI when the actual request or
> > > reconfigure is performed, though that requires knowledge of the number
> > > of lines in the request to be sized efficiently in the
> > > gpiod_line_config_new().  Sizing it as GPIO_V2_LINES_MAX would be even
> > > worse than your secondary array, so don't want that.
> >
> > Or would it? Currently the full config structure is 3784 bytes on a 64
> > bit arch. The base config is 32 bytes. If we added the default value
> > to base_config that would make it 36 bytes x GPIO_V2_LINES_MAX = 2304
> > bytes. We'd need another base_config for default settings.
> >
> > Unless I'm missing something this does seem like the better option.
> >
>
> Yikes, I overlooked the size of the offsets array in the secondary
> config - that is a significant contributor to the config size as well.
> For some reason I was thinking that was a bitmap, but that couldn't work.
>
> In that case a GPIO_V2_LINES_MAX sized array is clearly a better way to
> go, and that is a surprise.
> Though those array elements will require the line offset as well as the
> base_config, unless you have some other way to map offset to config?
>

No, but that's fine - see below.

> > > My Go library uses a map, but that isn't an option here.
> > > Resizing it dynamically is the option I was last pondering,
> > > but my preference would be to add a num_lines parameter to the new.
> > > Anyway, that was what I was wondering about...
> > >
> >
> > We could resize the array dynamically but we'd need to return error
> > codes from getters.
>
> Why? If there is no config for the requested line then you return the
> global default value, right?
> Why does that change if the array is resizable?
> Btw, I'm assuming that the gpiod_line_config would contain a pointer to
> the dynamic array, so the handle the user has would remain unchanged.
> And the getters all return ints, not pointers to internal fields.
> What am I missing?

Memory allocation failures when resizing.

>
> Also, "global default value" is different from the primary, right?
> Perhaps getters should return the primary value, which itself defaults
> to the global defaults, if the line doesn't have specific config?
>
> > We could also define the size when allocating the
> > config but it's a sub-par approach too.
> >
>
> Sure, it's a trade-off, but the alternative is requiring a 2-3k block
> even for a one line request, which seems a wee bit excessive.
>

As you said - it's on the heap, so who cares. But this is also an
internal structure and so we can use bit fields. That should reduce
the memory footprint significantly as we now don't require more than 3
bits for any given enum. That would leave us with the debounce period
and offset as full size variables.

Bart

> With the proposed API, the only other alternative I can see to give a
> small footprint is dynamic resizing, which I'm not thrilled by either.
> So just wanted to double check that you are content to lock in the
> gpiod_line_config_new API, as that will constrain any optimisation later
> on.
>

On Thu, Aug 12, 2021 at 02:51:02PM +0200, Bartosz Golaszewski wrote:
> On Thu, Aug 12, 2021 at 12:29 PM Kent Gibson <warthog618@gmail.com> wrote:

> As you said - it's on the heap, so who cares. But this is also an
> internal structure and so we can use bit fields. That should reduce
> the memory footprint significantly as we now don't require more than 3
> bits for any given enum. That would leave us with the debounce period
> and offset as full size variables.

Side note (I believe you know about): bit fields are the beasts when you have
to have a locked access.

On Thu, Aug 12, 2021 at 02:51:02PM +0200, Bartosz Golaszewski wrote:
> On Thu, Aug 12, 2021 at 12:29 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> 
> [snip]
> 
> > > >
> > > > My preference would be for gpiod_line_config_set_output_value() and
> > > > variants to also set the direction for those lines to output.
> > > > And maybe rename it to gpiod_line_config_set_output().
> > > > And maybe add a set_input for symmetry.
> > > >
> > >
> > > Any naming idea for setting the direction to "AS_IS"?
> > >
> >
> > Since as-is is vague you need to include the field name.
> > So I would remove set_direction and replace it with set_input, set_output
> > and set_direction_as_is (which I would expect to see used very rarely in
> > the wild, as the only use case I can think of for it is undoing a
> > set_input or set_output call).
> >
> > > > But my concern above was actually the secondary array - that confused me.
> > > > And it's big - always. (OTOH it's on the heap so who cares?)
> > > > The array is of size GPIO_V2_LINE_NUM_ATTRS_MAX, yet each entry could
> > > > have multiple attributes set - so long as the offsets subsets match?
> > > > What happens if both debounce and flags are set for the same subset?
> > > > Looks like debounce wins and the flags get discarded in
> > > > gpiod_line_config_to_kernel().
> > > >
> > >
> > > Yeah, I aimed at ironing it out when writing tests. You're probably right here.
> > >
> >
> > Same reason I hadn't paid much attention to the implementation.
> >
> > > > What I had in mind for the config was an array of config for each line,
> > > > only performing the mapping to uAPI when the actual request or
> > > > reconfigure is performed, though that requires knowledge of the number
> > > > of lines in the request to be sized efficiently in the
> > > > gpiod_line_config_new().  Sizing it as GPIO_V2_LINES_MAX would be even
> > > > worse than your secondary array, so don't want that.
> > >
> > > Or would it? Currently the full config structure is 3784 bytes on a 64
> > > bit arch. The base config is 32 bytes. If we added the default value
> > > to base_config that would make it 36 bytes x GPIO_V2_LINES_MAX = 2304
> > > bytes. We'd need another base_config for default settings.
> > >
> > > Unless I'm missing something this does seem like the better option.
> > >
> >
> > Yikes, I overlooked the size of the offsets array in the secondary
> > config - that is a significant contributor to the config size as well.
> > For some reason I was thinking that was a bitmap, but that couldn't work.
> >
> > In that case a GPIO_V2_LINES_MAX sized array is clearly a better way to
> > go, and that is a surprise.
> > Though those array elements will require the line offset as well as the
> > base_config, unless you have some other way to map offset to config?
> >
> 
> No, but that's fine - see below.
> 
> > > > My Go library uses a map, but that isn't an option here.
> > > > Resizing it dynamically is the option I was last pondering,
> > > > but my preference would be to add a num_lines parameter to the new.
> > > > Anyway, that was what I was wondering about...
> > > >
> > >
> > > We could resize the array dynamically but we'd need to return error
> > > codes from getters.
> >
> > Why? If there is no config for the requested line then you return the
> > global default value, right?
> > Why does that change if the array is resizable?
> > Btw, I'm assuming that the gpiod_line_config would contain a pointer to
> > the dynamic array, so the handle the user has would remain unchanged.
> > And the getters all return ints, not pointers to internal fields.
> > What am I missing?
> 
> Memory allocation failures when resizing.
> 

Why are you resizing in a getter?
If you mean setters then fair enough - though you could just sit on
the error until the request or reconfigure and return it there instead.

Cheers,
Kent.

> >
> > Also, "global default value" is different from the primary, right?
> > Perhaps getters should return the primary value, which itself defaults
> > to the global defaults, if the line doesn't have specific config?
> >
> > > We could also define the size when allocating the
> > > config but it's a sub-par approach too.
> > >
> >
> > Sure, it's a trade-off, but the alternative is requiring a 2-3k block
> > even for a one line request, which seems a wee bit excessive.
> >
> 
> As you said - it's on the heap, so who cares. But this is also an
> internal structure and so we can use bit fields. That should reduce
> the memory footprint significantly as we now don't require more than 3
> bits for any given enum. That would leave us with the debounce period
> and offset as full size variables.
> 
> Bart
> 
> > With the proposed API, the only other alternative I can see to give a
> > small footprint is dynamic resizing, which I'm not thrilled by either.
> > So just wanted to double check that you are content to lock in the
> > gpiod_line_config_new API, as that will constrain any optimisation later
> > on.
> >

On Thu, Aug 12, 2021 at 3:53 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Thu, Aug 12, 2021 at 02:51:02PM +0200, Bartosz Golaszewski wrote:
> > On Thu, Aug 12, 2021 at 12:29 PM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> >
> > [snip]
> >
> > > > >
> > > > > My preference would be for gpiod_line_config_set_output_value() and
> > > > > variants to also set the direction for those lines to output.
> > > > > And maybe rename it to gpiod_line_config_set_output().
> > > > > And maybe add a set_input for symmetry.
> > > > >
> > > >
> > > > Any naming idea for setting the direction to "AS_IS"?
> > > >
> > >
> > > Since as-is is vague you need to include the field name.
> > > So I would remove set_direction and replace it with set_input, set_output
> > > and set_direction_as_is (which I would expect to see used very rarely in
> > > the wild, as the only use case I can think of for it is undoing a
> > > set_input or set_output call).
> > >
> > > > > But my concern above was actually the secondary array - that confused me.
> > > > > And it's big - always. (OTOH it's on the heap so who cares?)
> > > > > The array is of size GPIO_V2_LINE_NUM_ATTRS_MAX, yet each entry could
> > > > > have multiple attributes set - so long as the offsets subsets match?
> > > > > What happens if both debounce and flags are set for the same subset?
> > > > > Looks like debounce wins and the flags get discarded in
> > > > > gpiod_line_config_to_kernel().
> > > > >
> > > >
> > > > Yeah, I aimed at ironing it out when writing tests. You're probably right here.
> > > >
> > >
> > > Same reason I hadn't paid much attention to the implementation.
> > >
> > > > > What I had in mind for the config was an array of config for each line,
> > > > > only performing the mapping to uAPI when the actual request or
> > > > > reconfigure is performed, though that requires knowledge of the number
> > > > > of lines in the request to be sized efficiently in the
> > > > > gpiod_line_config_new().  Sizing it as GPIO_V2_LINES_MAX would be even
> > > > > worse than your secondary array, so don't want that.
> > > >
> > > > Or would it? Currently the full config structure is 3784 bytes on a 64
> > > > bit arch. The base config is 32 bytes. If we added the default value
> > > > to base_config that would make it 36 bytes x GPIO_V2_LINES_MAX = 2304
> > > > bytes. We'd need another base_config for default settings.
> > > >
> > > > Unless I'm missing something this does seem like the better option.
> > > >
> > >
> > > Yikes, I overlooked the size of the offsets array in the secondary
> > > config - that is a significant contributor to the config size as well.
> > > For some reason I was thinking that was a bitmap, but that couldn't work.
> > >
> > > In that case a GPIO_V2_LINES_MAX sized array is clearly a better way to
> > > go, and that is a surprise.
> > > Though those array elements will require the line offset as well as the
> > > base_config, unless you have some other way to map offset to config?
> > >
> >
> > No, but that's fine - see below.
> >
> > > > > My Go library uses a map, but that isn't an option here.
> > > > > Resizing it dynamically is the option I was last pondering,
> > > > > but my preference would be to add a num_lines parameter to the new.
> > > > > Anyway, that was what I was wondering about...
> > > > >
> > > >
> > > > We could resize the array dynamically but we'd need to return error
> > > > codes from getters.
> > >
> > > Why? If there is no config for the requested line then you return the
> > > global default value, right?
> > > Why does that change if the array is resizable?
> > > Btw, I'm assuming that the gpiod_line_config would contain a pointer to
> > > the dynamic array, so the handle the user has would remain unchanged.
> > > And the getters all return ints, not pointers to internal fields.
> > > What am I missing?
> >
> > Memory allocation failures when resizing.
> >
>
> Why are you resizing in a getter?
> If you mean setters then fair enough - though you could just sit on
> the error until the request or reconfigure and return it there instead.
>

I meant setters of course. My bad.

Bart

On Thu, Aug 12, 2021 at 3:03 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Aug 12, 2021 at 02:51:02PM +0200, Bartosz Golaszewski wrote:
> > On Thu, Aug 12, 2021 at 12:29 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> > As you said - it's on the heap, so who cares. But this is also an
> > internal structure and so we can use bit fields. That should reduce
> > the memory footprint significantly as we now don't require more than 3
> > bits for any given enum. That would leave us with the debounce period
> > and offset as full size variables.
>
> Side note (I believe you know about): bit fields are the beasts when you have
> to have a locked access.
>

The library isn't thread-safe. AFAIK a simple mutex around every
operation on a struct containing bitfields will be enough for a user
to guarantee no race conditions in user-space.

Bart

On Thu, Aug 12, 2021 at 02:51:02PM +0200, Bartosz Golaszewski wrote:
> On Thu, Aug 12, 2021 at 12:29 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> 

<snip>

> >
> > Sure, it's a trade-off, but the alternative is requiring a 2-3k block
> > even for a one line request, which seems a wee bit excessive.
> >
> 
> As you said - it's on the heap, so who cares. But this is also an
> internal structure and so we can use bit fields. That should reduce
> the memory footprint significantly as we now don't require more than 3
> bits for any given enum. That would leave us with the debounce period
> and offset as full size variables.
> 

Rather than introducing a new collection of enums and bitfields, why not
just store the v2 flags for the line?

Cheers,
Kent.

On Thu, Aug 12, 2021 at 4:23 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Thu, Aug 12, 2021 at 02:51:02PM +0200, Bartosz Golaszewski wrote:
> > On Thu, Aug 12, 2021 at 12:29 PM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> >
>
> <snip>
>
> > >
> > > Sure, it's a trade-off, but the alternative is requiring a 2-3k block
> > > even for a one line request, which seems a wee bit excessive.
> > >
> >
> > As you said - it's on the heap, so who cares. But this is also an
> > internal structure and so we can use bit fields. That should reduce
> > the memory footprint significantly as we now don't require more than 3
> > bits for any given enum. That would leave us with the debounce period
> > and offset as full size variables.
> >
>
> Rather than introducing a new collection of enums and bitfields, why not
> just store the v2 flags for the line?
>

Why a new collection of enums? It wouldn't change anything, we'd just
make sure in the setters we never overflow.

I would prefer to limit the use of kernel symbols (and types!) to the
minimum for clarity.

Bart

On Thu, Aug 12, 2021 at 04:43:46PM +0200, Bartosz Golaszewski wrote:
> On Thu, Aug 12, 2021 at 4:23 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Thu, Aug 12, 2021 at 02:51:02PM +0200, Bartosz Golaszewski wrote:
> > > On Thu, Aug 12, 2021 at 12:29 PM Kent Gibson <warthog618@gmail.com> wrote:
> > > >
> > >
> >
> > <snip>
> >
> > > >
> > > > Sure, it's a trade-off, but the alternative is requiring a 2-3k block
> > > > even for a one line request, which seems a wee bit excessive.
> > > >
> > >
> > > As you said - it's on the heap, so who cares. But this is also an
> > > internal structure and so we can use bit fields. That should reduce
> > > the memory footprint significantly as we now don't require more than 3
> > > bits for any given enum. That would leave us with the debounce period
> > > and offset as full size variables.
> > >
> >
> > Rather than introducing a new collection of enums and bitfields, why not
> > just store the v2 flags for the line?
> >
> 
> Why a new collection of enums? It wouldn't change anything, we'd just
> make sure in the setters we never overflow.
> 

Yeah, my bad - you already have the enums.

> I would prefer to limit the use of kernel symbols (and types!) to the
> minimum for clarity.
> 

OK, but you need to map the libgpiod config to kernel flags at some
point...
And this is all internal to line-config.c...

Cheers,
Kent.

On Thu, Aug 12, 2021 at 5:02 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Thu, Aug 12, 2021 at 04:43:46PM +0200, Bartosz Golaszewski wrote:
> > On Thu, Aug 12, 2021 at 4:23 PM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > On Thu, Aug 12, 2021 at 02:51:02PM +0200, Bartosz Golaszewski wrote:
> > > > On Thu, Aug 12, 2021 at 12:29 PM Kent Gibson <warthog618@gmail.com> wrote:
> > > > >
> > > >
> > >
> > > <snip>
> > >
> > > > >
> > > > > Sure, it's a trade-off, but the alternative is requiring a 2-3k block
> > > > > even for a one line request, which seems a wee bit excessive.
> > > > >
> > > >
> > > > As you said - it's on the heap, so who cares. But this is also an
> > > > internal structure and so we can use bit fields. That should reduce
> > > > the memory footprint significantly as we now don't require more than 3
> > > > bits for any given enum. That would leave us with the debounce period
> > > > and offset as full size variables.
> > > >
> > >
> > > Rather than introducing a new collection of enums and bitfields, why not
> > > just store the v2 flags for the line?
> > >
> >
> > Why a new collection of enums? It wouldn't change anything, we'd just
> > make sure in the setters we never overflow.
> >
>
> Yeah, my bad - you already have the enums.
>
> > I would prefer to limit the use of kernel symbols (and types!) to the
> > minimum for clarity.
> >
>
> OK, but you need to map the libgpiod config to kernel flags at some
> point...

Yep, right before passing the arguments to the kernel. Having separate
fields for each setting is much clearer IMO than storing flags.

> And this is all internal to line-config.c...
>

This code still needs to be maintained and using enums makes it easier.

Anyway, this is implementation detail really as with bit fields we'll
fit in an 32-bit integer anyway with all those enums. Does the general
idea sound good? If so, then I'll rework it.

Bart

On Fri, Aug 13, 2021 at 02:59:43PM +0200, Bartosz Golaszewski wrote:
> On Thu, Aug 12, 2021 at 5:02 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Thu, Aug 12, 2021 at 04:43:46PM +0200, Bartosz Golaszewski wrote:
> > > On Thu, Aug 12, 2021 at 4:23 PM Kent Gibson <warthog618@gmail.com> wrote:
> > > >
> > > > On Thu, Aug 12, 2021 at 02:51:02PM +0200, Bartosz Golaszewski wrote:
> > > > > On Thu, Aug 12, 2021 at 12:29 PM Kent Gibson <warthog618@gmail.com> wrote:
> > > > > >
> > > > >
> > > >
> > > > <snip>
> > > >
> > > > > >
> > > > > > Sure, it's a trade-off, but the alternative is requiring a 2-3k block
> > > > > > even for a one line request, which seems a wee bit excessive.
> > > > > >
> > > > >
> > > > > As you said - it's on the heap, so who cares. But this is also an
> > > > > internal structure and so we can use bit fields. That should reduce
> > > > > the memory footprint significantly as we now don't require more than 3
> > > > > bits for any given enum. That would leave us with the debounce period
> > > > > and offset as full size variables.
> > > > >
> > > >
> > > > Rather than introducing a new collection of enums and bitfields, why not
> > > > just store the v2 flags for the line?
> > > >
> > >
> > > Why a new collection of enums? It wouldn't change anything, we'd just
> > > make sure in the setters we never overflow.
> > >
> >
> > Yeah, my bad - you already have the enums.
> >
> > > I would prefer to limit the use of kernel symbols (and types!) to the
> > > minimum for clarity.
> > >
> >
> > OK, but you need to map the libgpiod config to kernel flags at some
> > point...
> 
> Yep, right before passing the arguments to the kernel. Having separate
> fields for each setting is much clearer IMO than storing flags.
> 
> > And this is all internal to line-config.c...
> >
> 
> This code still needs to be maintained and using enums makes it easier.
> 

Fair enough.

> Anyway, this is implementation detail really as with bit fields we'll
> fit in an 32-bit integer anyway with all those enums.

Agreed - I thought of that after I posted.

> Does the general idea sound good? If so, then I'll rework it.
> 

Sounds like a plan - rework away.

Cheers,
Kent.