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

[Kent Gibson <warthog618@xxxxxxxxx>]

On Wed, May 15, 2024 at 01:26:32AM -0700, Bartosz Golaszewski wrote:
> On Tue, 14 May 2024 15:38:04 +0200, Kent Gibson <warthog618@xxxxxxxxx> said:
> > On Tue, May 14, 2024 at 06:31:39AM -0700, Bartosz Golaszewski wrote:
> >> On Mon, 13 May 2024 12:13:31 +0200, Kent Gibson <warthog618@xxxxxxxxx> said:
> >> >
> >> >> > /**
> >> >> >  * @brief Set the bias of requested input line.
> >> >> >  * @param olr The request to reconfigure.
> >> >> >  * @param bias The new bias to apply to requested input line.
> >> >> >  * @return 0 on success, -1 on failure.
> >> >> >  */
> >> >> > int gpiod_olr_set_bias(struct gpiod_line_request * olr,
> >> >> > 		       enum gpiod_line_bias bias);
> >> >>
> >> >> For this to work, you'd most likely need a new struct wrapping the request
> >> >> and also storing the line config. Otherwise - how'd you keep the state of all
> >> >> the other line settings?
> >> >>
> >> >
> >> > Yeah, I realised that when I went to implement it :(.
> >> >
> >> > What I implemented was to read the line info and build the config from that.
> >> > So no impact on core.
> >> > Not the most efficient, but for this use case I wan't fussed.
> >> >
> >>
> >> I think those simplified requests should wrap the config structures, otherwise
> >> we'd have to readback the config from the kernel which would become quite
> >> complex for anything including more than one line.
> >>
> >
> > The whole point of the simplified requests is that they only deal with
> > a single line.  And the config mutators only deal with a single input.
> >
>
> For now anyway. :)
>
> > Wouldn't wrapping break the ability to use all the other
> > gpiod_line_request_XXX functions, and so require adding more functions
> > to make use of the simplified requests?
> >
>
> Not sure why? You need a request for a single line anyway and you need to store
> the config for it somewhere as toggling a single property will require a full
> gpiod_line_request_reconfigure() anyway.

But I don't intend to store the config for it, so the existing
gpiod_line_request is fine.

>
> I don't think it'll be enough to re-use struct gpiod_line_request here, you
> need some new structure. Unless you know how to do it. In that case: show me
> the code. :)
>

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...
+	 */
+	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;
+}
+
+static int
+ext_reconfigure(struct gpiod_line_request *request, struct gpiod_line_settings *settings)
+{
+	struct gpiod_line_config *line_cfg;
+	int ret;
+
+	line_cfg = gpiod_line_config_new();
+	if (!line_cfg)
+		return -1;
+
+	ret = gpiod_line_config_add_line_settings(line_cfg, request->offsets, 1,
+						  settings);
+	if (ret)
+		goto free_line_cfg;
+
+	ret = gpiod_line_request_reconfigure_lines(request, line_cfg);
+
+free_line_cfg:
+	gpiod_line_config_free(line_cfg);
+
+	return ret;
+}
+
+GPIOD_API int
+gpiod_ext_set_bias(struct gpiod_line_request * olr,
+		   enum gpiod_line_bias bias)
+{
+	int ret;
+
+	struct gpiod_line_settings *settings;
+
+	settings = ext_line_settings(olr);
+	if (!settings)
+		return -1;
+
+	ret = gpiod_line_settings_set_bias(settings, bias);
+	if (!ret)
+		ret = ext_reconfigure(olr, settings);
+
+	gpiod_line_settings_free(settings);
+
+	return ret;
+}
+
+GPIOD_API int
+gpiod_ext_set_debounce_period_us(struct gpiod_line_request * olr,
+				 unsigned long period)
+{
+	struct gpiod_line_settings *settings;
+	int ret;
+
+	settings = ext_line_settings(olr);
+	if (!settings)
+		return -1;
+
+	gpiod_line_settings_set_debounce_period_us(settings, period);
+	ret = ext_reconfigure(olr, settings);
+
+	gpiod_line_settings_free(settings);
+
+	return ret;
+}
+
+GPIOD_API int
+gpiod_ext_set_edge_detection(struct gpiod_line_request * olr,
+			     enum gpiod_line_edge edge)
+{
+	struct gpiod_line_settings *settings;
+	int ret;
+
+	settings = ext_line_settings(olr);
+	if (!settings)
+		return -1;
+
+	ret = gpiod_line_settings_set_edge_detection(settings, edge);
+	if (!ret)
+		ret = ext_reconfigure(olr, settings);
+
+	gpiod_line_settings_free(settings);
+
+	return ret;
+}


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

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

Cheers,
Kent.