Re: [libgpiod v2][PATCH v3 2/3] line-config: expose the override logic to users

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Sat, Mar 5, 2022 at 6:51 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>
> On Thu, Mar 03, 2022 at 10:18:35AM +0100, Bartosz Golaszewski wrote:
> > 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 access to the underlying override
> > mechanism.
> >
> > For every setting, we expose a getter and setter for the default value
> > as well as a set of four functions for setting, getting, clearing and
> > checking per-offset overrides.
> >
> > An override can initially have the same value as the defaults but will
> > retain the overridden value should the defaults change.
> >
> > We also complete the API by providing functions that allow to retrieve
> > the overridden offsets and their corresponding property types.
> >
> > This way the caller can fully inspect the line_config and high-level
> > language bindings can provide stringification methods.
> >
> > While at it: we fix a couple bugs in the implementation of struct
> > line_config and add new constructors that take a variable list of
> > arguments.
> >
>
> The variadic constructor is new for patch v3.
> It bundles default constructor + default mutators, so doesn't add
> functionality that wasn't already available - it just makes it
> accessible via a single function call.
> Is the variadic form beneficial for bindings, say Python, where you
> would prefer not to be making a bunch of C calls?
> Or is this just sugar?
>

No, there's no benefit for the bindings as they can't possibly
construct a va_list. I very much intend to have keyword arguments in
Python but the only reason I added the new constructors is to justify
adding the GPIOD_LINE_CONFIG_PROP_* enum because without them it would
only be used for retrieving the override list.

> No major objection, just curious about the rationale for adding it.
>

Actually looking at them now, I think they don't make much sense and I
think about dropping them from the next iteration. It's not like the
enum increases the executable size really.

> [snip]
>
> > +/**
> > + * @brief Get the total number of overridden settings currently stored by this
> > + *        line config object.
> > + * @param config Line config object.
> > + * @return Number of individual overridden settings.
> > + */
> > +unsigned int
> > +gpiod_line_config_get_num_overrides(struct gpiod_line_config *config);
> > +
> > +/**
> > + * @brief Get the list of overridden offsets and the corresponding types of
> > + *        overridden settings.
> > + * @param config Line config object.
> > + * @param offsets Array to store the overidden offsets. Must hold at least the
> > + *                number of unsigned integers returned by
> > + *                ::gpiod_line_config_get_output_value_offset.
> > + * @param props Array to store the types of overridden settings. Must hold at
> > + *              least the number of integers returned by
> > + *              gpiod_line_config_get_output_value_offset.
> > + */
>
> The purpose of the offsets and props arrays is not clear.
> Clarify that you are returning a list of (offset,prop), split across the
> two arrays.
> Replace them with a single array of (offset,prop) unless there is
> a good reason to keep them separate?

The only (but good) reason is to not introduce another data structure.
Especially a public one. This function is not going to be used a lot I
suppose so I don't care if it's awkward to use.

>
> Guessing it should be gpiod_line_config_get_num_overrides(), not
> gpiod_line_config_get_output_value_offset() which returns 0 or 1, or
> even better -1 for inputs?
>

Yep, just a wrong copy/paste.

> [snip]
>
> > +GPIOD_API unsigned int
> > +gpiod_line_config_get_num_overrides(struct gpiod_line_config *config)
> > +{
> > +     struct override_config *override;
> > +     unsigned int i, j, count = 0;
> >
> > -     errno = ENXIO;
> > +     for (i = 0; i < GPIO_V2_LINES_MAX; i++) {
> > +             override = &config->overrides[i];
> > +
> > +             if (override_used(override)) {
> > +                     for (j = 0; j < NUM_OVERRIDE_FLAGS; j++) {
> > +                             if (override->override_flags &
> > +                                 override_flag_list[j])
> > +                                     count++;
> > +                     }
> > +             }
> > +     }
> > +
> > +     return count;
> > +}
> > +
>
> Using GPIO_V2_LINES_MAX for the size of the overrides array is
> confusing, and the two should be de-coupled so you can more easily resize
> the array if necessary.
> Provide a NUM_OVERRIDES_MAX, or similar, and use that when referring
> to the size of the overrides array.
>

Makes sense.

> > +static int override_flag_to_prop(int flag)
> > +{
> > +     switch (flag) {
> > +     case OVERRIDE_FLAG_DIRECTION:
> > +             return GPIOD_LINE_CONFIG_PROP_DIRECTION;
> > +     case OVERRIDE_FLAG_EDGE:
> > +             return GPIOD_LINE_CONFIG_PROP_EDGE;
> > +     case OVERRIDE_FLAG_BIAS:
> > +             return GPIOD_LINE_CONFIG_PROP_BIAS;
> > +     case OVERRIDE_FLAG_DRIVE:
> > +             return GPIOD_LINE_CONFIG_PROP_DRIVE;
> > +     case OVERRIDE_FLAG_ACTIVE_LOW:
> > +             return GPIOD_LINE_CONFIG_PROP_ACTIVE_LOW;
> > +     case OVERRIDE_FLAG_DEBOUNCE_PERIOD:
> > +             return GPIOD_LINE_CONFIG_PROP_DEBOUNCE_PERIOD;
> > +     case OVERRIDE_FLAG_CLOCK:
> > +             return GPIOD_LINE_CONFIG_PROP_EVENT_CLOCK;
> > +     case OVERRIDE_FLAG_OUTPUT_VALUE:
> > +             return GPIOD_LINE_CONFIG_PROP_OUTPUT_VALUE;
> > +     }
> > +
> > +     /* Can't happen. */
> >       return -1;
> >  }
> >
> > +GPIOD_API void
> > +gpiod_line_config_get_overrides(struct gpiod_line_config *config,
> > +                             unsigned int *offsets, int *props)
> > +{
> > +     struct override_config *override;
> > +     unsigned int i, j, count = 0;
> > +
> > +     for (i = 0; i < GPIO_V2_LINES_MAX; i++) {
> > +             override = &config->overrides[i];
> > +
> > +             if (override_used(override)) {
> > +                     for (j = 0; j < NUM_OVERRIDE_FLAGS; j++) {
> > +                             if (override->override_flags &
> > +                                 override_flag_list[j]) {
> > +                                     offsets[count] = override->offset;
> > +                                     props[count] = override_flag_to_prop(
> > +                                                     override_flag_list[j]);
> > +                                     count++;
> > +                             }
> > +                     }
> > +             }
> > +     }
> > +}
> > +
>
> Return the count?
> Would be a bit redundant, as the user needs to call
> gpiod_line_config_get_num_overrides() to size the offsets and props
> arrays beforehand, but the usual patten when writing into a passed array
> is to return the number of elements written.
>

So there are two typical approaches: take the size of the passed
buffer as argument and return the number of elements actually written
or define the required size of the buffer (in this case: the size
returned by the get_num_overrides func) and don't return that number.
I don't think it would be of any use so let's not return a value
nobody would check.

> Cheers,
> Kent.

Bart