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

[Kent Gibson <warthog618@xxxxxxxxx>]

On Wed, Mar 02, 2022 at 01:59:31PM +0100, Bartosz Golaszewski wrote:
> On Tue, Feb 22, 2022 at 12:40 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> >
> > On Mon, Feb 21, 2022 at 04:40:54PM +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 identify
> > > offsets for which at least one setting is overriden.
> > >
> > > 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.
> > >
> >
> > TLDR: I've got a lot of nitpicks on the doco, but it might be simpler
> > for you to apply this patch as is and for me to submit a patch with doco
> > tweaks than for you to try to sort out my comments!
> >
> > The actual code looks good, so
> >
> > Reviewed-by: Kent Gibson <warthog618@xxxxxxxxx>
> >
> > with or without the suggested doco changes.
> >
> > > Signed-off-by: Bartosz Golaszewski <brgl@xxxxxxxx>
> > > ---
> > >  include/gpiod.h   | 509 ++++++++++++++++++++-----------
> > >  lib/line-config.c | 752 +++++++++++++++++++++++++++++-----------------
> > >  tools/gpioget.c   |   6 +-
> > >  tools/gpiomon.c   |   6 +-
> > >  tools/gpioset.c   |   9 +-
> > >  5 files changed, 821 insertions(+), 461 deletions(-)
> > >

[snip]

> > >  /**
> > > @@ -480,329 +505,424 @@ 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.
> >
> > Nitpick: "setting" feels redundant given the "Set".  Same thoughout,
> > including the "Get" cases.
> >
> 
> What would you prefer it to be? "@brief Direction of all lines"?
> Doesn't look right to me. It's normal for the doc to use a verb to say
> what the function does.
> 

"setting" is the redundant word to be removed:

@brief Set the default direction.

If you want to describe the scope as well then:

@brief Set the default direction for all requested lines.

Though the function scopes are described in the object documentation,
I'm good with the shorter version.

> > >   * @param config Line config object.
> > >   * @param direction New direction.
> > >   */
> > > -void gpiod_line_config_set_direction(struct gpiod_line_config *config,
> > > -                                  int direction);
> > > +void gpiod_line_config_set_direction_default(struct gpiod_line_config *config,
> > > +                                          int direction);
> > >
> > >  /**
> > > - * @brief Set the direction for a single line at given offset.
> > > + * @brief Set the direction override at given offset.
> >
> > Nitpick: "at given offset" -> "for the line" throughout.
> > Similarly "at this offset" -> "for this line",
> > and "line at given offset" -> "line",
> > and "for a given offset" -> "for a line".
> >
> 
> I think that speaking of offsets makes it more explicit and switching
> to your alternatives dilutes the meaning here. This is still the
> configuration and we don't have any real lines yet. Just offset ->
> value mappings.
> 

Are you just winding me up ;-)?

You don't have any __requested__ lines yet, but the lines physically
exist and here you are specifying the config to be applied TO THE LINE
when it is requested.  So the config is most certainly for the line
- the offset is just a line identifier (as is the line name).

If the name were the identifier then your documentation would read:

@brief Set the direction override with the given name.

which makes no sense.

whereas mine would still read:

@brief Set the direction override for the line.

Perhaps rename the function and object "gpiod_offset_config" for clarity??
(being sarcastic - please don't).

Still prefer my suggestion, or even the previous version to the v2 version.

> >
> > >   * @param config Line config object.
> > >   * @param direction New direction.
> > > - * @param offset Offset of the line for which to set the direction.
> > > + * @param offset Offset of the line for which to set the override.
> > >   */
> > > -void gpiod_line_config_set_direction_offset(struct gpiod_line_config *config,
> > > -                                         int direction, unsigned int offset);
> > > +void gpiod_line_config_set_direction_override(struct gpiod_line_config *config,
> > > +                                           int direction,
> > > +                                           unsigned int offset);
> > >
> > >  /**
> > > - * @brief Set the direction for a subset of lines.
> > > + * @brief Clear the direction override at given offset.
> > >   * @param config Line config object.
> > > - * @param direction New direction.
> > > - * @param num_offsets Number of offsets in the array.
> > > - * @param offsets Array of line offsets for which to set the direction.
> > > + * @param offset Offset of the line for which to clear the override.
> > > + * @note Does nothing if no override is set for this line.
> > >   */
> > > -void gpiod_line_config_set_direction_subset(struct gpiod_line_config *config,
> > > -                                         int direction,
> > > -                                         unsigned int num_offsets,
> > > -                                         const unsigned int *offsets);
> > > +void
> > > +gpiod_line_config_clear_direction_override(struct gpiod_line_config *config,
> > > +                                        unsigned int offset);
> > > +
> > > +/**
> > > + * @brief Check if the direction setting is overridden at given offset.
> > > + * @param config Line config object.
> > > + * @param offset Offset of the line for which to check the override.
> > > + * @return True if direction is overridden at this offset, false otherwise.
> > > + */
> > > +bool gpiod_line_config_direction_is_overridden(struct gpiod_line_config *config,
> > > +                                            unsigned int offset);
> > >
> > >  /**
> > > - * @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 non-overridden
> > > + *         offset.
> > > + */
> > > +int gpiod_line_config_get_direction_default(struct gpiod_line_config *config);
> > > +
> > > +/**
> > > + * @brief Get the direction setting for a given offset.
> >
> > Preferred "given line".  The direction is an attribute of the line, not
> > the offset.  The offset is the line identifier.
> > Same throughout.
> 
> If I were consistent with my own previous docs, it would be "for the
> line at given offset".
> 

Sure, but I find the "at given offset" redundant in the function @brief
since you repeat it in the @param offset below.  Once is enough, and
if anywhere then in the @param.

> >
> > >   * @param config Line config object.
> > >   * @param offset Line offset for which to read the direction setting.
> >
> > Nitpick: "Line offset" -> "Offset of the line"
> 
> Sounds good.
> 

I was on the fence about suggesting adding the "The" as well, so "The
offset of the line" to make it more grammatically correct, but it is
frequently dropped from documentation and missing in so many other places
that I went without for consistency.

But again, this is all just documentation tweaks, so don't get hung up on
anything - looking forward to v3.

Cheers,
Kent.