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

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

[Kent Gibson]

Hi Bart,

I realise you want to keep libgpiod minimal, but in my latest survey of the
problems that new or potential users are finding with libgpiod the most
common one was that it is way too complicated to do simple things.
They just want to request an input or output line and play with it.
They think that should be an easy thing to do, and will completely write
off libgpiod because it is not - the learning curve is too steep.
And they have a point.

I've raised this before, but I think libgpiod is in need of a small (and I
emphasize small) set of helper functions to simplify basic use cases,
like just requesting a single input or output line.  Maybe functions to
control setting bias, edge detection and debounce on inputs (using
reconfigure after the initial request).
The functions would be separated from the existing functions by naming,
e.g. gpiod_helper_XXX, or some such, so there would be no confusion with
the existing.

Any chance your position has changed since last I asked?

Cheers,
Kent.

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

[Bartosz Golaszewski]

On Tue, May 7, 2024 at 4:21 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> Hi Bart,
>
> I realise you want to keep libgpiod minimal, but in my latest survey of the
> problems that new or potential users are finding with libgpiod the most
> common one was that it is way too complicated to do simple things.
> They just want to request an input or output line and play with it.
> They think that should be an easy thing to do, and will completely write
> off libgpiod because it is not - the learning curve is too steep.
> And they have a point.
>
> I've raised this before, but I think libgpiod is in need of a small (and I
> emphasize small) set of helper functions to simplify basic use cases,
> like just requesting a single input or output line.  Maybe functions to
> control setting bias, edge detection and debounce on inputs (using
> reconfigure after the initial request).
> The functions would be separated from the existing functions by naming,
> e.g. gpiod_helper_XXX, or some such, so there would be no confusion with
> the existing.
>
> Any chance your position has changed since last I asked?
>

Ugh... I really don't want the core libgpiod to grow... This sounds
like the old ctxless stuff that was awkward and I removed it in v2.

Do you think that users actually use it (core C libgpiod)? I would
think they stick to python and C++? Would providing the GLib bindings
help in this case? We could extend that no problem. Or maybe a
separate helper library linked against libgpiod but also existing kind
of adjacent to it?

Bart

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

[Kent Gibson]

On Fri, May 10, 2024 at 08:37:08PM +0200, Bartosz Golaszewski wrote:
> On Tue, May 7, 2024 at 4:21 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > Hi Bart,
> >
> > I realise you want to keep libgpiod minimal, but in my latest survey of the
> > problems that new or potential users are finding with libgpiod the most
> > common one was that it is way too complicated to do simple things.
> > They just want to request an input or output line and play with it.
> > They think that should be an easy thing to do, and will completely write
> > off libgpiod because it is not - the learning curve is too steep.
> > And they have a point.
> >
> > I've raised this before, but I think libgpiod is in need of a small (and I
> > emphasize small) set of helper functions to simplify basic use cases,
> > like just requesting a single input or output line.  Maybe functions to
> > control setting bias, edge detection and debounce on inputs (using
> > reconfigure after the initial request).
> > The functions would be separated from the existing functions by naming,
> > e.g. gpiod_helper_XXX, or some such, so there would be no confusion with
> > the existing.
> >
> > Any chance your position has changed since last I asked?
> >
>
> Ugh... I really don't want the core libgpiod to grow... This sounds
> like the old ctxless stuff that was awkward and I removed it in v2.
>

I understand - slippery slope and keeping minimal.
But I think we do need some solution for simple one line request cases that
can get users up and running with a smaller learning curve.
Then they can learn more if they want more.

> Do you think that users actually use it (core C libgpiod)?

Yeah, they do, believe it or not.  I'm mainly talking the vocal Pi crowd,
many of whom are used to going bare metal to the hardware, and are now
searching for an alternative.  libgpiod as it stands is too complicated for
them - they just want to drive a pin up and down or read a button.
There are a few other alternatives that let them do that, and they will
switch on that basis alone, and never look back, though they will happily
spread their rather toxic opinions of libgpiod vs those alternatives.

> I would think they stick to python and C++?

They are actually less likely to go C++ - more learning curve.
And Python can be considered too heavyweight in situations with limited
resources.

> Would providing the GLib bindings
> help in this case? We could extend that no problem. Or maybe a
> separate helper library linked against libgpiod but also existing kind
> of adjacent to it?
>

Sorry, I even haven't looked at the GLib bindings, but I will take a
look.  The core would be better if possible - they would be displeased
having to install yet another library just for basic commands.  But it
would be better than nothing.

This is what I currently have in mind for the C API:

/**
 * @}
 *
 * @defgroup olr One line requests - helper functions for basic use cases
 * @{
 *
 * Various functions that provide a simplified interface for basic use cases
 * where the request only contains one line.
 */

/**
 * @brief Request a single input line.
 * @param chip Path to the GPIO chip.
 * @param offset The offset of the GPIO line.
 * @return New line request object or NULL if an error occurred. The request
 *         must be released by the caller using ::gpiod_line_request_release.
 */
struct gpiod_line_request *
gpiod_olr_request_input(const char *path, unsigned int offset);

/**
 * @brief Request a single input line.
 * @param chip Path to the GPIO chip.
 * @param offset The offset of the GPIO line.
 * @param value The value to set the line.
 * @return New line request object or NULL if an error occurred. The request
 *         must be released by the caller using ::gpiod_line_request_release.
 */
struct gpiod_line_request *
gpiod_olr_request_output(const char *path,
			 unsigned int offset,
			 enum gpiod_line_value value);

/**
 * @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);

/**
 * @brief Set the debounce period of requested input line.
 * @param olr The request to reconfigure.
 * @param period The new debounce period to apply, in microseconds.
 * @return 0 on success, -1 on failure.
 */
int gpiod_olr_set_debounce_period_us(struct gpiod_line_request *olr,
				     unsigned long period);

/**
 * @brief Set the edge detection of requested input line.
 * @param olr The request to reconfigure.
 * @param edge The new edge detection setting.
 * @return 0 on success, -1 on failure.
 */
int gpiod_olr_set_edge_detection(struct gpiod_line_request * olr,
				 enum gpiod_line_edge edge);

/**
 * @}
 */

I think those 5 functions cover the basics.  That is it.  Done. No more.
Unless you can think of something I've missed.

I went with _olr_ as the prefix to clearly separate it from the existing,
but kept it as brief as possible.
It also chains with the trailing "d" in gpiod so it is pronouncable.

I was toying with adding get_value()/set_value(), but all they offered was
dropping the offset param, so not enough benefit and I dropped them.

I haven't put much thought into whether those should be carried through
to the bindings, but don't see any fundamental reason they couldn't.

Anyway, have a think about it.
And I'll go take a look at the GLib bindings.

Cheers,
Kent.

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

[Bartosz Golaszewski]

On Sat, 11 May 2024 03:11:44 +0200, Kent Gibson <warthog618@gmail.com> said:
> On Fri, May 10, 2024 at 08:37:08PM +0200, Bartosz Golaszewski wrote:
>> On Tue, May 7, 2024 at 4:21 AM Kent Gibson <warthog618@gmail.com> wrote:
>> >
>> > Hi Bart,
>> >
>> > I realise you want to keep libgpiod minimal, but in my latest survey of the
>> > problems that new or potential users are finding with libgpiod the most
>> > common one was that it is way too complicated to do simple things.
>> > They just want to request an input or output line and play with it.
>> > They think that should be an easy thing to do, and will completely write
>> > off libgpiod because it is not - the learning curve is too steep.
>> > And they have a point.
>> >
>> > I've raised this before, but I think libgpiod is in need of a small (and I
>> > emphasize small) set of helper functions to simplify basic use cases,
>> > like just requesting a single input or output line.  Maybe functions to
>> > control setting bias, edge detection and debounce on inputs (using
>> > reconfigure after the initial request).
>> > The functions would be separated from the existing functions by naming,
>> > e.g. gpiod_helper_XXX, or some such, so there would be no confusion with
>> > the existing.
>> >
>> > Any chance your position has changed since last I asked?
>> >
>>
>> Ugh... I really don't want the core libgpiod to grow... This sounds
>> like the old ctxless stuff that was awkward and I removed it in v2.
>>
>
> I understand - slippery slope and keeping minimal.
> But I think we do need some solution for simple one line request cases that
> can get users up and running with a smaller learning curve.
> Then they can learn more if they want more.
>

I don't disagree on this point but I still think core libgpiod is not the right
place for it.

>> Do you think that users actually use it (core C libgpiod)?
>
> Yeah, they do, believe it or not.  I'm mainly talking the vocal Pi crowd,
> many of whom are used to going bare metal to the hardware, and are now
> searching for an alternative.  libgpiod as it stands is too complicated for
> them - they just want to drive a pin up and down or read a button.
> There are a few other alternatives that let them do that, and they will
> switch on that basis alone, and never look back, though they will happily
> spread their rather toxic opinions of libgpiod vs those alternatives.
>
>> I would think they stick to python and C++?
>
> They are actually less likely to go C++ - more learning curve.
> And Python can be considered too heavyweight in situations with limited
> resources.
>

I see. We're being held hostage by the RPi crowd. :)

>> Would providing the GLib bindings
>> help in this case? We could extend that no problem. Or maybe a
>> separate helper library linked against libgpiod but also existing kind
>> of adjacent to it?
>>
>
> Sorry, I even haven't looked at the GLib bindings, but I will take a
> look.  The core would be better if possible - they would be displeased
> having to install yet another library just for basic commands.  But it
> would be better than nothing.
>
> This is what I currently have in mind for the C API:
>
> /**
>  * @}
>  *
>  * @defgroup olr One line requests - helper functions for basic use cases
>  * @{
>  *
>  * Various functions that provide a simplified interface for basic use cases
>  * where the request only contains one line.
>  */
>
> /**
>  * @brief Request a single input line.
>  * @param chip Path to the GPIO chip.
>  * @param offset The offset of the GPIO line.
>  * @return New line request object or NULL if an error occurred. The request
>  *         must be released by the caller using ::gpiod_line_request_release.
>  */
> struct gpiod_line_request *
> gpiod_olr_request_input(const char *path, unsigned int offset);
>
> /**
>  * @brief Request a single input line.
>  * @param chip Path to the GPIO chip.
>  * @param offset The offset of the GPIO line.
>  * @param value The value to set the line.
>  * @return New line request object or NULL if an error occurred. The request
>  *         must be released by the caller using ::gpiod_line_request_release.
>  */
> struct gpiod_line_request *
> gpiod_olr_request_output(const char *path,
> 			 unsigned int offset,
> 			 enum gpiod_line_value value);
>
> /**
>  * @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?

>
> /**
>  * @brief Set the debounce period of requested input line.
>  * @param olr The request to reconfigure.
>  * @param period The new debounce period to apply, in microseconds.
>  * @return 0 on success, -1 on failure.
>  */
> int gpiod_olr_set_debounce_period_us(struct gpiod_line_request *olr,
> 				     unsigned long period);
>
> /**
>  * @brief Set the edge detection of requested input line.
>  * @param olr The request to reconfigure.
>  * @param edge The new edge detection setting.
>  * @return 0 on success, -1 on failure.
>  */
> int gpiod_olr_set_edge_detection(struct gpiod_line_request * olr,
> 				 enum gpiod_line_edge edge);
>
> /**
>  * @}
>  */
>
> I think those 5 functions cover the basics.  That is it.  Done. No more.
> Unless you can think of something I've missed.
>

I'm afraid it never ends up being enough. We'd just open the door for all kinds
of extensions to libgpiod. Very soon someone would want a callback-based API
for reading edge events. Next you'll have people asking to be able to toggle
the direction of a pin without touching the config structures which will require
us to somehow store the context of the request. "That is it" never works.

> I went with _olr_ as the prefix to clearly separate it from the existing,
> but kept it as brief as possible.

I don't like this prefix. In fact I think it's even worse than the - already
crappy - _ctxless_ prefix we have in v1.6.x. It's not clear at first glace
what it stands for and it'll be confusing as soon as we add more such
extensions.

> It also chains with the trailing "d" in gpiod so it is pronouncable.
>
> I was toying with adding get_value()/set_value(), but all they offered was
> dropping the offset param, so not enough benefit and I dropped them.
>
> I haven't put much thought into whether those should be carried through
> to the bindings, but don't see any fundamental reason they couldn't.
>

In most cases bindings do allow you to do what you try to achieve here with
one-liners already. I think this really only applies to C.

> Anyway, have a think about it.
> And I'll go take a look at the GLib bindings.
>

I have thought about it. I agree we could use some simpler interfaces. I don't
agree their place is in core libgpiod. I understand we want to make this new
interface seamless to use for end-users of libgpiod.

How about introducing a new header and a separate shared object: gpiod-ext.h
and libgpiod-ext.so respectively? We could put all these new helpers in there,
use the gpiod_ext_ prefix for all of them and distros could package the
"extended" part of libgpiod together with the core library to avoid having to
install multiple packages?

We'd keep the clear distinction between the low-level, core C library wrapping
the kernel uAPI and the user-friendly C API. Though the user-friendly API in my
book could be the GLib library but I understand not everyone wants to use GLib
nor is familiar with GObject.

Bart

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

[Kent Gibson]

On Mon, May 13, 2024 at 01:28:19AM -0700, Bartosz Golaszewski wrote:
> On Sat, 11 May 2024 03:11:44 +0200, Kent Gibson <warthog618@gmail.com> said:
> > On Fri, May 10, 2024 at 08:37:08PM +0200, Bartosz Golaszewski wrote:
> >> On Tue, May 7, 2024 at 4:21 AM Kent Gibson <warthog618@gmail.com> wrote:
> >> >
> >
> > I understand - slippery slope and keeping minimal.
> > But I think we do need some solution for simple one line request cases that
> > can get users up and running with a smaller learning curve.
> > Then they can learn more if they want more.
> >
>
> I don't disagree on this point but I still think core libgpiod is not the right
> place for it.
>
> >> Do you think that users actually use it (core C libgpiod)?
> >
> > Yeah, they do, believe it or not.  I'm mainly talking the vocal Pi crowd,
> > many of whom are used to going bare metal to the hardware, and are now
> > searching for an alternative.  libgpiod as it stands is too complicated for
> > them - they just want to drive a pin up and down or read a button.
> > There are a few other alternatives that let them do that, and they will
> > switch on that basis alone, and never look back, though they will happily
> > spread their rather toxic opinions of libgpiod vs those alternatives.
> >
> >> I would think they stick to python and C++?
> >
> > They are actually less likely to go C++ - more learning curve.
> > And Python can be considered too heavyweight in situations with limited
> > resources.
> >
>
> I see. We're being held hostage by the RPi crowd. :)
>

You could look at it that way.

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

> >
> > /**
> >  * @brief Set the debounce period of requested input line.
> >  * @param olr The request to reconfigure.
> >  * @param period The new debounce period to apply, in microseconds.
> >  * @return 0 on success, -1 on failure.
> >  */
> > int gpiod_olr_set_debounce_period_us(struct gpiod_line_request *olr,
> > 				     unsigned long period);
> >
> > /**
> >  * @brief Set the edge detection of requested input line.
> >  * @param olr The request to reconfigure.
> >  * @param edge The new edge detection setting.
> >  * @return 0 on success, -1 on failure.
> >  */
> > int gpiod_olr_set_edge_detection(struct gpiod_line_request * olr,
> > 				 enum gpiod_line_edge edge);
> >
> > /**
> >  * @}
> >  */
> >
> > I think those 5 functions cover the basics.  That is it.  Done. No more.
> > Unless you can think of something I've missed.
> >
>
> I'm afraid it never ends up being enough. We'd just open the door for all kinds
> of extensions to libgpiod. Very soon someone would want a callback-based API
> for reading edge events. Next you'll have people asking to be able to toggle
> the direction of a pin without touching the config structures which will require
> us to somehow store the context of the request. "That is it" never works.
>

At the end of the day you are the arbiter of where that line is drawn, so
that is up to you.

>
> In most cases bindings do allow you to do what you try to achieve here with
> one-liners already. I think this really only applies to C.
>

Oh, you would be surprised - the C++ "one-liners" are still considered
too verbose and difficult to understand.

> > Anyway, have a think about it.
> > And I'll go take a look at the GLib bindings.
> >
>
> I have thought about it. I agree we could use some simpler interfaces. I don't
> agree their place is in core libgpiod. I understand we want to make this new
> interface seamless to use for end-users of libgpiod.
>
> How about introducing a new header and a separate shared object: gpiod-ext.h
> and libgpiod-ext.so respectively? We could put all these new helpers in there,
> use the gpiod_ext_ prefix for all of them and distros could package the
> "extended" part of libgpiod together with the core library to avoid having to
> install multiple packages?
>

That sounds good to me.

> We'd keep the clear distinction between the low-level, core C library wrapping
> the kernel uAPI and the user-friendly C API. Though the user-friendly API in my
> book could be the GLib library but I understand not everyone wants to use GLib
> nor is familiar with GObject.
>

Sorry, still haven't had a chance to look at the GLib API.
But if it involves any additional overhead or learning curve then it
wont be well received.

Cheers,
Kent.

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

[Bartosz Golaszewski]

On Mon, 13 May 2024 12:13:31 +0200, Kent Gibson <warthog618@gmail.com> 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.

>> We'd keep the clear distinction between the low-level, core C library wrapping
>> the kernel uAPI and the user-friendly C API. Though the user-friendly API in my
>> book could be the GLib library but I understand not everyone wants to use GLib
>> nor is familiar with GObject.
>>
>
> Sorry, still haven't had a chance to look at the GLib API.
> But if it involves any additional overhead or learning curve then it
> wont be well received.
>

Yes, unfortunately GLib & GObject are quite different from most of the regular
C programming.

Bart

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

[Kent Gibson]

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@gmail.com> 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.

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?

Maybe I'm missing something.

Cheers,
Kent.

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

[Bartosz Golaszewski]

On Tue, 14 May 2024 15:38:04 +0200, Kent Gibson <warthog618@gmail.com> 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@gmail.com> 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.

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

Bart

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

[Kent Gibson]

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@gmail.com> 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@gmail.com> 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.

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

[Kent Gibson]

On Wed, May 15, 2024 at 05:18:48PM +0800, Kent Gibson wrote:
> 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@gmail.com> 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@gmail.com> said:
> > >> >
> >
> > 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.):
>

You can now find my WIP branch here[1].  That includes examples for
comparison.

Cheers,
Kent.

[1] https://github.com/warthog618/libgpiod/tree/ext

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

[Bartosz Golaszewski]

On Wed, 15 May 2024 11:18:48 +0200, Kent Gibson <warthog618@gmail.com> 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

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

[Kent Gibson]

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@gmail.com> 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

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

[Kent Gibson]

On Wed, May 15, 2024 at 10:14:36PM +0800, Kent Gibson wrote:
> 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@gmail.com> said:
> > >
> >
> > Yeah, I hate it...
> >
>
> I wasn't expecting any love for it - it is ugly.
> But it does the job.
>

An alternative is to extend the struct gpiod_line_request to contain a
pointer to the config.  The ext constructors would store their config
there rather than free it.  The core constructors would leave it NULL,
though you would always have the option to keep a copy there later if
core wants to make use of it.
And gpiod_line_request_free() would free it.

The ext mutators would only accept requests with the config set.

That can all be conditional on extensions being part of the build.

So taking advantage of the pimpl interface.  Yay.

Would that work for you?

And I think I mis-interpreted one of your earlier mails - you want the
extensions to go in their own shared library, not bundled in libgpiod.so?
Though if that is the case it would be helpful to relax the use of pimpl
within the lib so the extensions can access any fields not made public
through the core API, as well as more efficiently access those that are.

Cheers,
Kent.

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

[Kent Gibson]

On Wed, May 15, 2024 at 10:14:36PM +0800, Kent Gibson wrote:
> 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@gmail.com> 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.):
> > >
> >
> > Can you post an example of how this is used?
> >
>
> Sure [1].
>

What was I thinking - you read the uAPI event and convert it to a libgpiod
type, so that wont work.  I obviously haven't tried running that
example.
Forget I suggested it - if the user wants to avoid the event buffer
complexity they have to read into a struct gpio_v2_line_event, so no
libgpiod involvement at all.

Cheers,
Kent.

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

[Bartosz Golaszewski]

On Wed, May 15, 2024 at 4:14 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Wed, May 15, 2024 at 06:54:15AM -0700, Bartosz Golaszewski wrote:
> >
> > 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?

I have a strong preference for that code to live in a separate .so
file (and by extension - a separate compilation unit).

Bart

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

[Kent Gibson]

On Fri, May 17, 2024 at 12:53:36PM +0200, Bartosz Golaszewski wrote:
> On Wed, May 15, 2024 at 4:14 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Wed, May 15, 2024 at 06:54:15AM -0700, Bartosz Golaszewski wrote:
> > >
> > > 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?
>
> I have a strong preference for that code to live in a separate .so
> file (and by extension - a separate compilation unit).
>

Oh, I agree - that makes total sense.  The direction I was headed felt wrong,
so I figured I must've misunderstood what you meant.

I'll re-organise it into a separate unit.

Does that unit have to act through the core API, or can we give it
access to the internals?
And where do you stand wrt the idea of adding a config pointer to struct
gpiod_line_request itself?

Cheers,
Kent.

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

[Bartosz Golaszewski]

On Fri, May 17, 2024 at 2:37 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Fri, May 17, 2024 at 12:53:36PM +0200, Bartosz Golaszewski wrote:
> > On Wed, May 15, 2024 at 4:14 PM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > On Wed, May 15, 2024 at 06:54:15AM -0700, Bartosz Golaszewski wrote:
> > > >
> > > > 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?
> >
> > I have a strong preference for that code to live in a separate .so
> > file (and by extension - a separate compilation unit).
> >
>
> Oh, I agree - that makes total sense.  The direction I was headed felt wrong,
> so I figured I must've misunderstood what you meant.
>
> I'll re-organise it into a separate unit.
>
> Does that unit have to act through the core API, or can we give it
> access to the internals?

If we can avoid it accessing the internals, that would be awesome.
Unless you hit a road-block, please try to keep the core separate.

> And where do you stand wrt the idea of adding a config pointer to struct
> gpiod_line_request itself?
>

We'd need to make a deep copy, otherwise it could be destroyed and the
pointer would be left dangling, right?

Bart

> Cheers,
> Kent.
>
>

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

[Kent Gibson]

On Wed, May 22, 2024 at 12:59:39PM +0200, Bartosz Golaszewski wrote:
> On Fri, May 17, 2024 at 2:37 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> >
> > Oh, I agree - that makes total sense.  The direction I was headed felt wrong,
> > so I figured I must've misunderstood what you meant.
> >
> > I'll re-organise it into a separate unit.
> >
> > Does that unit have to act through the core API, or can we give it
> > access to the internals?
>
> If we can avoid it accessing the internals, that would be awesome.
> Unless you hit a road-block, please try to keep the core separate.
>

If you want the ext functions to store config then that would rule out the
shortcut constructors returning a gpiod_line_request.

I was thinking that the ext could be a friend of core and get access to
some things not generally accessible, in this case allowing ext to store
the config inside the request.

Without that, the two options we have is to rebuild the config from line
info, which you don't like, or wrapping the gpiod_line_request and have
the wrapper provide an accessor to the contained gpiod_line_request if
the user wants to use core API, which I'm not thrilled about.
But that seems to be the only option.

> > And where do you stand wrt the idea of adding a config pointer to struct
> > gpiod_line_request itself?
> >
>
> We'd need to make a deep copy, otherwise it could be destroyed and the
> pointer would be left dangling, right?
>

No, as the config is constructed and added by ext - the user can't actually
see it.  It would only be accessed indirectly via the ext functions.
If using only the core API then it would always be NULL.

Well unless we were to provide accessors to make it accessible to the
user, and then which way to go would be open to debate.  There are other
functions in libgpiod that pass ownership, so copying isn't the only
option. I haven't put much thought into that though, as I wasn't planning
to go there.

Cheers,
Kent.

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

[Bartosz Golaszewski]

On Wed, May 22, 2024 at 2:41 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Wed, May 22, 2024 at 12:59:39PM +0200, Bartosz Golaszewski wrote:
> > On Fri, May 17, 2024 at 2:37 PM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > >
> > > Oh, I agree - that makes total sense.  The direction I was headed felt wrong,
> > > so I figured I must've misunderstood what you meant.
> > >
> > > I'll re-organise it into a separate unit.
> > >
> > > Does that unit have to act through the core API, or can we give it
> > > access to the internals?
> >
> > If we can avoid it accessing the internals, that would be awesome.
> > Unless you hit a road-block, please try to keep the core separate.
> >
>
> If you want the ext functions to store config then that would rule out the
> shortcut constructors returning a gpiod_line_request.
>

Actually this is why I initially proposed to have another object
wrapping gpiod_line_request. I don't really want to extend it unless
we make use of it in the core. We can always provide
gpiod_ext_state_get_request() or something. We could call this wrapper
gpiod_ext_state so that we can cram whatever persistence we will
require as the ext library grows (which it inevitably will).

> I was thinking that the ext could be a friend of core and get access to
> some things not generally accessible, in this case allowing ext to store
> the config inside the request.
>

I would rather it be a wrapper around libgpiod which simplifies
operating on the core API.

> Without that, the two options we have is to rebuild the config from line
> info, which you don't like, or wrapping the gpiod_line_request and have
> the wrapper provide an accessor to the contained gpiod_line_request if
> the user wants to use core API, which I'm not thrilled about.
> But that seems to be the only option.
>

If I were to rank the solutions here from best to worst (personal
opinion) I'd go:

1. Wrapper for extended state providing accessor to gpiod_line_request
and storing the config
2. Rebuilding the config from info
3. Storing the config in gpiod_line_request

> > > And where do you stand wrt the idea of adding a config pointer to struct
> > > gpiod_line_request itself?
> > >
> >
> > We'd need to make a deep copy, otherwise it could be destroyed and the
> > pointer would be left dangling, right?
> >
>
> No, as the config is constructed and added by ext - the user can't actually
> see it.  It would only be accessed indirectly via the ext functions.
> If using only the core API then it would always be NULL.
>

Ugh, I really don't like having a useless pointer in this struct. It's
not about memory - the 8 bytes are irrelevant - but the logic of it.
Making concessions in the very core of the library for what is
essentially an outer shell around it just doesn't sound right.

> Well unless we were to provide accessors to make it accessible to the
> user, and then which way to go would be open to debate.  There are other
> functions in libgpiod that pass ownership, so copying isn't the only
> option. I haven't put much thought into that though, as I wasn't planning
> to go there.
>

For now that doesn't seem required. At worst we can provide a function
to lookup individual per offset settings from the ext state object.
Settings can be copied alright.

Bart

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

[Martin Hundebøll]

On Mon, 2024-05-13 at 01:28 -0700, Bartosz Golaszewski wrote:
> > Anyway, have a think about it.
> > And I'll go take a look at the GLib bindings.
> > 
> 
> I have thought about it. I agree we could use some simpler
> interfaces. I don't
> agree their place is in core libgpiod. I understand we want to make
> this new
> interface seamless to use for end-users of libgpiod.
> 
> How about introducing a new header and a separate shared object:
> gpiod-ext.h
> and libgpiod-ext.so respectively? We could put all these new helpers
> in there,
> use the gpiod_ext_ prefix for all of them and distros could package
> the
> "extended" part of libgpiod together with the core library to avoid
> having to
> install multiple packages?
> 
> We'd keep the clear distinction between the low-level, core C library
> wrapping
> the kernel uAPI and the user-friendly C API. Though the user-friendly
> API in my
> book could be the GLib library but I understand not everyone wants to
> use GLib
> nor is familiar with GObject.

For our embedded use cases we would go far to avoid GLib in our root
filesystem (and our initrd too). This means relying on libgpiod only.

With the current core API, reading a single gpio line feels cumbersome.
Especially because we often use gpio labels to run the same binaries on
multiple hardware variants.

So we would really like to see an "extended" API, with wrappers to:
 * request a single gpio line from chip + offset
 * request a single gpio line from a (unique) label
 * read the current value of the requested gpio line
 * set the current value of the requested gpio line

Having those functionalities in a separate shared object is fine.

// Martin

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

[Kent Gibson]

On Wed, May 15, 2024 at 09:06:50AM +0200, Martin Hundebøll wrote:
> On Mon, 2024-05-13 at 01:28 -0700, Bartosz Golaszewski wrote:
> > > Anyway, have a think about it.
> > > And I'll go take a look at the GLib bindings.
> > >
> >
> > I have thought about it. I agree we could use some simpler
> > interfaces. I don't
> > agree their place is in core libgpiod. I understand we want to make
> > this new
> > interface seamless to use for end-users of libgpiod.
> >
> > How about introducing a new header and a separate shared object:
> > gpiod-ext.h
> > and libgpiod-ext.so respectively? We could put all these new helpers
> > in there,
> > use the gpiod_ext_ prefix for all of them and distros could package
> > the
> > "extended" part of libgpiod together with the core library to avoid
> > having to
> > install multiple packages?
> >
> > We'd keep the clear distinction between the low-level, core C library
> > wrapping
> > the kernel uAPI and the user-friendly C API. Though the user-friendly
> > API in my
> > book could be the GLib library but I understand not everyone wants to
> > use GLib
> > nor is familiar with GObject.
>
> For our embedded use cases we would go far to avoid GLib in our root
> filesystem (and our initrd too). This means relying on libgpiod only.
>
> With the current core API, reading a single gpio line feels cumbersome.
> Especially because we often use gpio labels to run the same binaries on
> multiple hardware variants.
>
> So we would really like to see an "extended" API, with wrappers to:
>  * request a single gpio line from chip + offset

That is in my proposal.

>  * request a single gpio line from a (unique) label

That one would require migrating a fair chunk of code from tools into
core.  Though that deals with multiple lines in parallel, whereas this
would only have to deal with a single line, so could just be a simple
interation to find the line.

Would a function that finds the (chip,offset) for a name be sufficient?

>  * read the current value of the requested gpio line
>  * set the current value of the requested gpio line

I did consider these, but didn't see a huge advantage over the existing.
gpiod_line_request_get_value(req, offset).  Too complicated?

OTOH, if you want to get the line by name then you don't know the offset
:(.

>
> Having those functionalities in a separate shared object is fine.
>

Good to know, but my intent is to make these ext functions still use
the core object so users can still use the existing core functions.

Cheers,
Kent.