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

[Kent Gibson <warthog618@xxxxxxxxx>]

On Wed, May 15, 2024 at 06:54:15AM -0700, Bartosz Golaszewski wrote:
> 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 wasn't expecting any love for it - it is ugly.
But it does the job.

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

It shouldn't be used often, so I'm fine with the overhead.
The benefit is it uses a standard line-request.
If the user isn't hapopy with the overhead then they can use the core API.

And the function only has to deal with the config that is settable via
ext.  If you have set config via the core API then what the hell are you
doing calling one of these?

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

Yeah, but then they can't use the existing gpiod_line_request_XXX functions
can they?  At least not without going through an accessor to pull the
line_request from the ext_request.

> > +	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?
>

Sure [1].

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

At the moment I've made the code a conditionally compiled block in
line-request.c, so it can directly use the line-request internals.
Pretty sure that can be changed to use the core API, but isn't pimpl within
the library itself a tad extreme?

Cheers,
Kent.

[1] https://github.com/warthog618/libgpiod/blob/ext/examples/ext/async_watch_line_value.c