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

[Bartosz Golaszewski <brgl@bgdev.pl>]

On Wed, Mar 2, 2022 at 3:32 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> 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@gmail.com> 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@gmail.com>
> > >
> > > with or without the suggested doco changes.
> > >
> > > > Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
> > > > ---
> > > >  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:
>

Sorry, it's my ESL.

> @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.
>

Yeah, I'll just resend what I have and let you handle the docs later.
I'm not a native English speaker so what am I even doing second
guessing you here. :)

> Cheers,
> Kent.