Re: [libgpiod v2.0][PATCH] core: extend config objects

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Mon, Aug 9, 2021 at 1:10 AM Kent Gibson <warthog618@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxx>
> > >
> > > 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