Re: [libgpiod][PATCH 10/16] core: provide gpiod_line_config_set_output_values()

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Mon, Jan 16, 2023 at 1:15 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>
> On Fri, Jan 13, 2023 at 10:52:04PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> >
> > Currently if user wants to use the same settings for a set of requested
> > lines with the exception of the output value - they need to go through
> > hoops by updating the line settings object and adding it one by one to
> > the line config. Provide a helper function that allows to set a global
> > list of output values that override the settings. For details on the
> > interface: see documentation in this commit.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> > ---
> >  include/gpiod.h            | 27 +++++++++++++++
> >  lib/line-config.c          | 60 +++++++++++++++++++++++++++++++---
> >  tests/gpiod-test-helpers.h | 10 ++++++
> >  tests/tests-line-config.c  | 67 ++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 160 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/gpiod.h b/include/gpiod.h
> > index 8cede47..c552135 100644
> > --- a/include/gpiod.h
> > +++ b/include/gpiod.h
> > @@ -785,6 +785,33 @@ struct gpiod_line_settings *
> >  gpiod_line_config_get_line_settings(struct gpiod_line_config *config,
> >                                   unsigned int offset);
> >
> > +/**
> > + * @brief Set output values for a number of lines.
> > + * @param config Line config object.
> > + * @param values Buffer containing the output values.
> > + * @param num_values Number of values in the buffer.
> > + * @return 0 on success, -1 on error.
> > + *
> > + * This is a helper that allows users to set multiple (potentially different)
> > + * output values at once while using the same line settings object. Instead of
> > + * modifying the output value in the settings object and calling
> > + * ::gpiod_line_config_add_line_settings multiple times, we can specify the
> > + * settings, add them for a set of offsets and then call this function to
> > + * set the output values.
> > + *
>
> A helper such as this did cross my mind when updating gpioset, but I
> didn't consider it worth the effort then, and still don't.
>
> So the user has to set ALL output values, or more correctly the first num_values,
> right? i.e. it is the block form.
>
> This helper is only helpful if the user already has ALL the values in the
> required array format, as is the case for gpioset, else they need to build
> the array to pass - in which case they may as well call
> gpiod_line_config_add_line_settings() for each line.
> So is it really that helpful?

It seems to me that building/adding a separate settings object for
every line is quite cumbersome (I know that C is cumbersome in itself
but still...). IMO more so than just storing values in an array. And
in many cases the user may already know the right set of values in
which case it boils down to `static const enum gpiod_line_value vals[]
= { ... };`

This is also pretty much the equivalent of the
gpiod_line_request_set_values() function just run at request-time.

>
> The sparse form would be more generally useful, particularly in the
> bindings. i.e. they should accept a mapping from offsets to values rather
> than the ordered list.  Though, again, those may be best implemented with
> multiple calls to ::gpiod_line_config_add_line_settings rather than
> jumping through the hoops of constructing the parameters for another helper
> like this one.
>

I'd argue that this should be implemented in bindings using the new function.

> > + * Values set by this function override whatever values were specified in the
> > + * regular line settings.
> > + *
> > + * The order of the output values in the array corresponds with the order in
> > + * which offsets were added by ::gpiod_line_config_add_line_settings. For
> > + * example calling add_settings([1, 3]) and add_settings([2, 4]) and then
> > + * calling this function with the following logicall values : [0, 1, 0, 1]
> > + * will result in the following offset->value mapping: 1->0, 2->0, 3->1, 4->1.
> > + */
> > +int gpiod_line_config_set_output_values(struct gpiod_line_config *config,
> > +                                     const enum gpiod_line_value *values,
> > +                                     size_t num_values);
> > +
>
> But if you do keep it...
>
> Use documentation from gpiod_line_request_set_values(), suitably modified,
> to describe ordering - it is clearer and does not imply that the user has
> to independently know the order lines were added - it is in the order
> provided by gpiod_line_config_get_offsets().
>

I'd like to have some version of that. Maybe keep what I proposed for
C but have something more elaborate (with mappings?) for bindings? I
need to think about it tomorrow.

Bart