Re: [libgpiod][RFC] helper functions for basic use cases

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Wed, 15 May 2024 11:18:48 +0200, Kent Gibson <warthog618@xxxxxxxxx> said:
>
> Sure thing.  This is what I have at the moment (the declarations are as
> per earlier, just renamed.  And I just noticed some variables I haven't
> renamed.  I'll add it to the todo list.):
>
> diff --git a/lib/line-request.c b/lib/line-request.c
> index b76b3d7..5af23e0 100644
> --- a/lib/line-request.c
> +++ b/lib/line-request.c
> @@ -305,3 +305,200 @@ gpiod_line_request_read_edge_events(struct gpiod_line_request *request,
>
>  	return gpiod_edge_event_buffer_read_fd(request->fd, buffer, max_events);
>  }
> +
> +static struct gpiod_line_request *
> +ext_request(const char  *path, unsigned int offset,
> +	    enum gpiod_line_direction direction,
> +	    enum gpiod_line_value value)
> +{
> +	struct gpiod_line_request *request = NULL;
> +	struct gpiod_line_settings *settings;
> +	struct gpiod_line_config *line_cfg;
> +	struct gpiod_chip *chip;
> +	int ret;
> +
> +	chip = gpiod_chip_open(path);
> +	if (!chip)
> +		return NULL;
> +
> +	settings = gpiod_line_settings_new();
> +	if (!settings)
> +		goto close_chip;
> +
> +	gpiod_line_settings_set_direction(settings, direction);
> +	if (direction == GPIOD_LINE_DIRECTION_OUTPUT)
> +		gpiod_line_settings_set_output_value(settings, value);
> +
> +	line_cfg = gpiod_line_config_new();
> +	if (!line_cfg)
> +		goto free_settings;
> +
> +	ret = gpiod_line_config_add_line_settings(line_cfg, &offset, 1,
> +						  settings);
> +	if (ret)
> +		goto free_line_cfg;
> +
> +	request = gpiod_chip_request_lines(chip, NULL, line_cfg);
> +
> +free_line_cfg:
> +	gpiod_line_config_free(line_cfg);
> +
> +free_settings:
> +	gpiod_line_settings_free(settings);
> +
> +close_chip:
> +	gpiod_chip_close(chip);
> +
> +	return request;
> +}
> +
> +GPIOD_API struct gpiod_line_request *
> +gpiod_ext_request_input(const char  *path, unsigned int offset)
> +{
> +	return ext_request(path, offset, GPIOD_LINE_DIRECTION_INPUT, 0);
> +}
> +
> +GPIOD_API struct gpiod_line_request *
> +gpiod_ext_request_output(const char  *path, unsigned int offset,
> +			 enum gpiod_line_value value)
> +{
> +	return ext_request(path, offset, GPIOD_LINE_DIRECTION_OUTPUT, value);
> +}
> +
> +static struct gpiod_line_settings *
> +ext_line_settings(struct gpiod_line_request * olr)
> +{
> +	struct gpiod_line_settings *settings = NULL;
> +	struct gpiod_line_info *line_info;
> +	struct gpiod_chip *chip;
> +	char path[32];
> +
> +	assert(olr);
> +
> +	if (olr->num_lines != 1) {
> +		errno = EINVAL;
> +		return NULL;
> +	}
> +
> +	/*
> +	 * This is all decidedly non-optimal, as generally the user has the
> +	 * config available from when they made the request, but here we need to
> +	 * rebuild it from the line info...
> +	 */

Yeah, I hate it...

I would assume hogging memory for config structs is still cheaper than all these
operations needed to reread the settings for a line. Not to mention the fact
that if we ever extend the settings, we'll need to remember to update this
routine too.

The users of the _ext_ functions most likely wouldn't care whether the context
is stored in struct gpiod_line_request or struct gpiod_ext_request or otherwise.

> +	memcpy(path, "/dev/", 5);
> +	strncpy(&path[5], olr->chip_name, 26);
> +	chip = gpiod_chip_open(path);
> +	if (!chip)
> +		return NULL;
> +
> +	// get info
> +	line_info = gpiod_chip_get_line_info(chip, olr->offsets[0]);
> +	gpiod_chip_close(chip);
> +	if (!line_info)
> +		return NULL;
> +
> +	if (gpiod_line_info_get_direction(line_info) != GPIOD_LINE_DIRECTION_INPUT) {
> +		errno = EINVAL;
> +		goto free_info;
> +	}
> +
> +	settings = gpiod_line_settings_new();
> +	if (!settings)
> +		goto free_info;
> +
> +	gpiod_line_settings_set_direction(settings, GPIOD_LINE_DIRECTION_INPUT);
> +	gpiod_line_settings_set_bias(settings,
> +		gpiod_line_info_get_bias(line_info));
> +	gpiod_line_settings_set_edge_detection(settings,
> +		gpiod_line_info_get_edge_detection(line_info));
> +	gpiod_line_settings_set_debounce_period_us(settings,
> +		gpiod_line_info_get_debounce_period_us(line_info));
> +
> +free_info:
> +	gpiod_line_info_free(line_info);
> +
> +	return settings;
> +}
> +
>
> Oh, I also added this:
>
> +/**
> + * @brief The size required to contain a single edge event.
> + * @return The size in bytes.
> + */
> +size_t gpiod_edge_event_size();
> +
>
> So users can perform the event read themselves without requiring any
> knowledge of event buffers (at the moment they can't determine the size
> required to perform the read).
>

Can you post an example of how this is used?

> I also intend to provide an updated set of examples that use the ext API.
> They should go in examples/ext?
>

I think the code should go into ext/, the gpiod-ext.h header can go right next
to gpiod.h in include/ and the examples can be in the same examples/ directory,
just call them something_something_ext.c to indicate they use the simpler API.

Does that sound right?

Bart