Re: [libgpiod][RFC v2] core: implement v2.0 API

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Thu, May 27, 2021 at 1:27 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>
> On Tue, May 18, 2021 at 09:18:55PM +0200, Bartosz Golaszewski wrote:
> > This is the second shot at the v2.0 C API for libgpiod.
> >
> > Special thanks go out to Kent for his valuable advice and suggestions.
> >
> > The biggest changes are:
> >
> > The concept of attributes has been removed - instead the translation from
> > configuration options to kernel request happens at the time of the line
> > request call and can only fail at this stage - the config mutators no
> > longer return any value.
> >
> > If the caller has passed a config that is too complicated, the request
> > function will set errno to E2BIG which stands for: "Argument list too
> > long".
> >
> > The direction and edge options have been split from the former
> > request_type.
> >
> > The objects are no longer reference counted and no longer can be the
> > responsibility for their lifetime shared.
> >
> > There are many other minor tweaks everywhere.
> >
> > One thing I've been contemplating is whether we should expose some
> > functions allowing callers to check if the line config has already
> > become too complex or values passed are invalid. This would allow
> > languages that have exceptions throw them before we actually make the
> > request call. Does this make sense?
> >
>
> Wrt passing invalid values, I suggested error-less mutators on the
> assumption they wouldn't have parameters that require range checking.
> e.g. my Go library has AsInput() and AsOutput(value)
> Your equivalents are:
>     gpiod_line_config_set_direction(cfg, GPIOD_LINE_CONFIG_DIRECTION_INPUT)
> and
>     gpiod_line_config_set_direction(cfg, GPIOD_LINE_CONFIG_DIRECTION_OUTPUT)
>
> If you need to perform range checking on the parameters then the mutator
> should return an error code.
>

Isn't this inconsistent with the complexity validation that only
happens at request-time? You could say that a config could also
transition from a state with an invalid value to a one where it's
fine. The error code would indicate the reason for the request
failure: either E2BIG for too complex config or EINVAL for invalid
values inside it.

> OTOH, for the active level you provide two error-less mutators,
> gpiod_line_config_set_active_low() and
> gpiod_line_config_set_active_high(), which is fine.
>
> Wrt exposing complexity validation functions, I don't see the benefit.
> The config may transition through complex states, so long as at the time
> the gpiod_chip_request_lines() or gpiod_line_request_reconfigure_lines()
> is called it isn't too complex and so can be translated to the uAPI.
> The check has to be performed as part of those functions either way, and
> validating a transitional config doesn't prove anything.
>

Indeed. OTOH with return values in mutators taking integer values we
would throw errors on invalid values passed. I need to think about it
some more.

> > This time the new API is in one big patch for easier review. This
> > changeset doesn't modify the bindings or tests but the tools compile
> > and pass all tests.
> >
> > Signed-off-by: Bartosz Golaszewski <brgl@xxxxxxxx>
> > ---
> >  include/gpiod.h            | 1222 ++++++++++++++++++-----------------
> >  lib/Makefile.am            |   13 +-
> >  lib/chip.c                 |  216 +++++++
> >  lib/core.c                 | 1240 ------------------------------------
> >  lib/edge-event.c           |  184 ++++++
> >  lib/helpers.c              |  302 ---------
> >  lib/info-event.c           |   83 +++
> >  lib/internal.c             |   58 ++
> >  lib/internal.h             |   28 +
> >  lib/line-config.c          |  674 ++++++++++++++++++++
> >  lib/line-info.c            |  164 +++++
> >  lib/line-request.c         |  192 ++++++
> >  lib/misc.c                 |   63 ++
> >  lib/request-config.c       |   98 +++
> >  tools/gpio-tools-test.bats |   12 +-
> >  tools/gpiodetect.c         |   13 +-
> >  tools/gpiofind.c           |    3 +-
> >  tools/gpioget.c            |   66 +-
> >  tools/gpioinfo.c           |   60 +-
> >  tools/gpiomon.c            |  133 ++--
> >  tools/gpioset.c            |   75 ++-
> >  tools/tools-common.c       |    8 +-
> >  tools/tools-common.h       |    2 +-
> >  23 files changed, 2607 insertions(+), 2302 deletions(-)
> >  create mode 100644 lib/chip.c
> >  delete mode 100644 lib/core.c
> >  create mode 100644 lib/edge-event.c
> >  delete mode 100644 lib/helpers.c
> >  create mode 100644 lib/info-event.c
> >  create mode 100644 lib/internal.c
> >  create mode 100644 lib/line-config.c
> >  create mode 100644 lib/line-info.c
> >  create mode 100644 lib/line-request.c
> >  create mode 100644 lib/request-config.c
> >
> > diff --git a/include/gpiod.h b/include/gpiod.h
> > index a4ce01f..bd4689f 100644
> > --- a/include/gpiod.h
> > +++ b/include/gpiod.h
> > @@ -1,12 +1,12 @@
> >  /* SPDX-License-Identifier: LGPL-2.1-or-later */
> >  /* SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <bartekgola@xxxxxxxxx> */
> > +/* SPDX-FileCopyrightText: 2021 Bartosz Golaszewski <brgl@xxxxxxxx> */
>
>
> I'm only going to look at the header here - I'm assuming the other changes
> just follow from the API changes.
>
> <snip>
>
> >  /**
> > - * @brief Get the handle to the GPIO line at given offset.
> > - * @param chip The GPIO chip object.
> > + * @brief Get the current snapshot of information about the line at given
> > + *        offset.
> > + * @param chip GPIO chip object.
> >   * @param offset The offset of the GPIO line.
> > - * @return Pointer to the GPIO line handle or NULL if an error occured.
> > - */
> > -struct gpiod_line *
> > -gpiod_chip_get_line(struct gpiod_chip *chip, unsigned int offset);
> > -
> > -/**
> > - * @brief Retrieve a set of lines and store them in a line bulk object.
> > - * @param chip The GPIO chip object.
> > - * @param offsets Array of offsets of lines to retrieve.
> > - * @param num_offsets Number of lines to retrieve.
> > - * @return New line bulk object or NULL on error.
> > + * @return New GPIO line info object or NULL if an error occurred.
> >   */
> > -struct gpiod_line_bulk *
> > -gpiod_chip_get_lines(struct gpiod_chip *chip, unsigned int *offsets,
> > -                  unsigned int num_offsets);
> > +struct gpiod_line_info *gpiod_chip_get_line_info(struct gpiod_chip *chip,
> > +                                              unsigned int offset);
> >
>
> Caller takes ownership of the gpiod_line_info, or are we peeking into
> the gpiod_chip internals?  Either way, needs a comment.
>

Caller takes ownership to underline the fact that it's a non-mutable
snapshot of the line config at the time when it was retrieved. I'll
add a comment.

> <snip>
>
> >  /**
> > - * @brief Get the handle to the GPIO chip controlling this line.
> > - * @param line The GPIO line object.
> > - * @return Pointer to the GPIO chip handle controlling this line.
> > + * @brief Get the pointer to the line-info object associated with this event.
> > + * @param event Line info event object.
> > + * @return Returns a pointer to the line-info object associated with this event
> > + *         whose lifetime is tied to the event object. It must not be freed by
> > + *         the caller.
> >   */
> > -struct gpiod_chip *gpiod_line_get_chip(struct gpiod_line *line);
> > +struct gpiod_line_info *
> > +gpiod_info_event_peek_line_info(struct gpiod_info_event *event);
> > +
>
> Rather than the peek/copy you use here, I would rename the peek to
> get...

Ha! I think we'll disagree here again. In most cases in low-level
linux C libraries, a "get" for complex objects returns a copy (or a
new reference if we're using shared objects). I would like to stay
consistent with that pattern. To me "peek" is a good way to
distinguish those functions that allow you to look into the internals
of the parent object. If anything - I'd go with the get/peek pattern
where the former returns a new object over whose lifetime the caller
takes responsibility and the latter returns a pointer to an object
stored in the parent. I went with copy/peek because it hints at a
different behavior than regular get but I can live with get/peek.

>
> > +/**
> > + * @brief Get a copy of the line-info object associated with this event.
> > + * @param event Line info event object.
> > + * @return Returns a copy of the line-info object associated with this event or
> > + *         NULL on error. The lifetime of the returned object must be managed
> > + *         by the caller and the line-info object must be freed using
> > + *         ::gpiod_line_info_free.
> > + */
> >
> > +struct gpiod_line_info *
> > +gpiod_info_event_copy_line_info(struct gpiod_info_event *event);
>
> ... and change this to
>
>  +struct gpiod_line_info *
>  +gpiod_line_info_copy(struct gpiod_line_info *info);
>
> as that is more generally useful.
>
> Similarly elsewhere you use the peek/copy pattern.
>
> <snip>
>
> > +/**
> > + * @brief Set the output value for a single offset.
> > + * @param config Line config object.
> > + * @param offset Offset of the line.
> > + * @param value Output value to set.
> > + */
> > +void gpiod_line_config_set_output_value(struct gpiod_line_config *config,
> > +                                     unsigned int offset, int value);
> > +
>
> I would rename this to gpiod_line_config_set_output() and have it set
> the direction (to GPIOD_LINE_CONFIG_DIRECTION_OUTPUT of course) as well
> as the value.
>

I like it but how would that plug into the global direction setting?

> And maybe add a gpiod_line_config_set_input()?
>

What about "as is" with this pattern? We agreed that input would be
the default so we need some way to set "as is" too.

Thanks a lot for the review!

Bart

> <snip>
>
> Those are the only things that jump out at me at the moment.
> I much prefer this version over the previous.
> Sorry I haven't had a chance to look at it earlier.
>
> Cheers,
> Kent.