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

[Kent Gibson <warthog618@gmail.com>]

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.

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.

> 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@bgdev.pl>
> ---
>  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@gmail.com> */
> +/* SPDX-FileCopyrightText: 2021 Bartosz Golaszewski <brgl@bgdev.pl> */


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.

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

> +/**
> + * @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.

And maybe add a gpiod_line_config_set_input()?

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