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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Sat, 11 May 2024 03:11:44 +0200, Kent Gibson <warthog618@xxxxxxxxx> 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@xxxxxxxxx> 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