Re: [libgpiod][RFC 0/6] first draft of libgpiod v2.0 API

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Thu, Apr 22, 2021 at 4:32 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>
> On Wed, Apr 21, 2021 at 10:04:04PM +0200, Bartosz Golaszewski wrote:
> > On Mon, Apr 19, 2021 at 3:17 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> > >
> >
> > [snip -> long discussion on the libgpiod C API]
> >
> > Hi Kent,
> >
> > I was working on the next iteration of the code and I'm struggling
> > with the implementation of some elements without the concept of
> > attributes.
> >
> > So I initially liked the idea of variadic functions but they won't
> > work for language bindings so that's a no go. On that note: I wanted
> > to get some inspiration from your go library but your elegant API
> > makes use of go features (like interfaces) we don't have in C.
> >
>
> It is the functional options that is the big difference between my Go
> implementation and what I would do in C.  I happen to use interfaces to
> implement those options.  You could do something similar in C (cos what
> can't you do in C) but it would be weird, so lets not go there.
>
> You can still provide the variadic forms for C users.
> And couldn't the language bindings use the "v" variant, as libabc
> suggests?:
>
> "Be careful with variadic functions
>   - It's great if you provide them, but you must accompany them with
>     "v" variants (i.e. functions taking a va_arg object), and provide
>     non-variadic variants as well. This is important to get language
>     wrappers right."
>

The "v" functions do nothing for language bindings - you can't
construct the required va_list out of thin air. What you do usually is
this (example taken from GLib):

gpointer
g_object_new(GType object_type,
             const gchar *first_property_name,
             ...);

has a non-variadic counterpart:

GObject *
g_object_new_with_properties(GType object_type,
                             guint n_properties,
                             const char *names[],
                             const GValue values[]);

I would prefer to simply not do variadic functions. The low-level API
should in general be wrapped in high-level languages. Most people that
mail me on libgpiod seem to use Python anyway.

> > The problem we have is basically about implementing primary and
> > secondary line configuration objects - where the primary contains the
> > default config for all requested lines and the secondary can override
> > certain config options (should zeroed values for enumerated types mean
> > - don't override?) for certain lines.
> >
>
> Yep, use the 0 value to mean "defaulted".
> For the secondary that means use the primary value.
> For the primary that means use the kernel default, so the primary is
> initialised with the kernel defaults.
>
> Note that accessors, if provided, generally wouldn't return the 0 value to
> the user - they follow the secondary -> primary chain and return the
> effective setting.
>
> > The basic line config structure (let's call it struct
> > gpiod_line_config) can be very simple and have the following mutators:
> >
>
> This is where you are immediately off into the weeds, so I obviously
> didn't communicate my suggestion very well...
> .
> The opaque config object presented to the user is not the simple line
> config object - it is the container for the whole request config (which
> is different from the request options - which is exactly why I would like
> the options not to be called request_config).
>
> The user never sees the simple line config, which is internal, only the
> request config.
> The accessors always work on the request config at attribute level, and
> there is never a need to apply or return the whole config for a particular
> line. You could - but it is not necessary for core functionality, so for
> now don't.
>
> I realise this makes the internal modelling of config much more
> complicated, but the goal is to provide a simplified interface for the
> user - so it should be expected that the majority of the complexity will
> end up in the library rather than user code.
>

Yeah, I understood this alright but I figured that this will bloat the
library interface with three variants of mutators per every config
option (or more if we provide two functions for direction and active
level) more than having simpler config objects and applying them
globally or per line.

> In my Go implementation I merge the request config into the request
> object itself. You could also do that, though the semantics might be
> clearer if you keep them separate (more on that later).
>

My goal was to logically split the config into elements that can be
reconfigured and those that cannot. More on that later.

> > struct gpiod_line_config *cfg = gpiod_line_config_new();
> >
> > gpiod_line_config_set_active_low(cfg, true);
>
> I would provide two functions for active level - set_active_high() and
> set_active_low().  Even if you don't, the function here should be
> set_active_level(), in case you want to add them later.
>
> > gpiod_line_config_set_direction(cfg, GPIOD_LINE_CONFIG_DIRECTION_OUTPUT);
> >
>
> That is the A case, i.e. the whole request.  How would you name the B
> and C cases?
>
> _set_<attr>_line and _set_<attr>_lines?
>
> or _set_line_<attr> and _set_lines_<attr>?
>
> or something else?
>

These are what I had in mind and also: _offset and _offsets.

> It might be worthwhile providing separate set_input() and
> set_output() functions - as the output case requires the initial value.
> You could have the user call set_output_values(), but why force them to
> make another call?
>
> > and so on for for drive, bias, edge, debounce, realtime clock.
> >
> > Output values would be set like this:
> >
> > unsigned int values[] = { 0, 1, 1, 0 }, num_lines = 4;
> > gpiod_line_config_set_output_values(cfg, num_lines, values);
> >
>
> I don't like the implicit identification of lines based on request
> ordering here.  I realise my Go API does that, but that is an option to
> the request itself, so the requested lines are also present and visible
> to the casual reader, whereas this is a standalone call.
>
> So it should require the list of lines that you are setting the values
> for. i.e. it is the mutator for a subset of lines, so the C case.
>
> And it implicitly sets those lines to outputs, so it can be more clearly
> named set_output(value) (that is the A case, btw).
>

I can imagine the B case like:

gpiod_line_config_set_output_value(config, offset, value);

But how would exactly the call for the A case look like? Two arrays
with offset -> value mapping?

unsigned int offsets[] = { 0, 2, 5 }, values[] = { 0, 1 ,1 };
gpiod_line_config_set_output_values_offsets(config, 3, offsets, values);

?

> > One can imagine a simple request with the same config for all lines as:
> >
> > gpiod_chip_request_lines(chip, req_cfg, line_cfg);
> >
> > Where req_cfg configures request-specific options, and line_cfg
> > contains the above line config. I'm still not convinced that
> > gpiod_request_options is the better name, I think I prefer the
> > juxtaposition of the two names: line_config and request_config.
> >
>
> That's ok - I'm pretty sure you'll get there eventually ;-).
>
> > Now how do we pass a composite line config with overridden values in C
> > without interfaces etc.?
> >
>
> As above, the req_cfg is the composite line config, so
>
> req = gpiod_chip_request_lines(chip, req_options, req_cfg);
>
> Or if you were to merge the request config, and even the options, into the
> request:
>
> unsigned int lines[] = { 0, 4, 12, 54 }, num_lines = 4;
> req = gpiod_line_request_new(num_lines, lines); // also variadic forms??
> // call req option and config mutators here...
> gpiod_line_request_set_active_low(req);
> gpiod_line_request_set_output(req, 1);
> gpiod_line_request_set_line_input(req, 12);
> gpiod_line_request_set_event_buffer_size(req, 42);
> ...
> // then actually request the lines...
> err = gpiod_chip_request_lines(chip, req);
>
> which may error for various reasons, such as lines already being
> requested or overly complex config.
>
> Merging everything into the request means fewer opaque objects and
> interactions for the user to have to deal with, which is always a good
> thing.
> The downside is that changes to options and config, such as the
> gpiod_line_request_set_active_low() etc here, are not applied until
> either the gpiod_chip_request_lines() or the set_config() call, which
> could be confusing.  Though the more I think about it the more I think
> the resulting simplification of the API wins out.  i.e. these objects:
>
> struct gpiod_line_attr;
> struct gpiod_line_config;
> struct gpiod_request_config;
> struct gpiod_request_handle;
>
> all get collapsed into:
>
> struct gpiod_line_request;
>
> which significantly reduces the cognitive load on the user.
>
> The set_config() would probably be called something like:
>
> err = gpiod_line_request_reconfigure(req)
>

This lack of splitting of options into configurable and constant ones
visually suggests that you can change all request options later on
which is not true. I think that at least for the C API, we should
split the responsibilities of objects and keep the division into
request config, line config *and* the line handle whose lifetime is
from the moment the lines get requested until they're released.

> to distinguish it from the mutators which use the _set_ naming.
> (and it would also align with my Go library ;-)
>
> > One idea I have is to add a new object called struct
> > gpiod_line_config_ext (for extended) that would take one primary
> > config and an arbitrary number of secondary configs with the following
> > example use-case:
> >
> > struct gpiod_line_config_ext *ext_cfg = gpiod_line_config_ext_new();
> > unsigned int offsets[] = { 2, 3 };
> >
> > /* Add the default config for this request. */
> > gpiod_line_config_ext_set_primary_config(ext_cfg, line_cfg);
> > /* Add a secondary config for 2 lines with offsets: 2 and 3. */
> > gpiod_line_config_ext_add_secondary_config(ext_cfg, other_line_cfg, 2, offsets);
> >
> > gpiod_chip_request_lines_ext(chip, req_cfg, ext_cfg);
> >
>
> Please, no _ext objects - that is an admission of failure right there.
>

I wanted to protest but then realized that if you need _ext interfaces
then it means your non-extended, initial design is already flawed. :)

Ok so let's try again.

How about:

Three structs:

struct gpiod_line_config;
struct gpiod_request_config;
struct gpiod_line_request;

The first one holds the composite of primary and secondary configs and
is modified using mutators according to this scheme:

gpiod_line_config_set_<attr>(config, attr);
gpiod_line_config_set_<attr>_offset(config, attr, offset);
gpiod_line_config_set_<attr>_offsets(config, attr, num_offsets, offsets);

With notable exceptions for:

gpiod_line_config_set_[input|active_low|active_high](config);
gpiod_line_config_set_[input|active_low|active_high]_offset(config, offset);
gpiod_line_config_set_[input|active_low|active_high]_offsets(config,
num_offsets, offsets);

and:

gpiod_line_config_set_output(config, num_lines, offsets, values);
gpiod_line_config_set_output_offset(config, offset, value);

The request function takes a single line config and a request config
and returns a new gpiod_line_request like in the first iteration.

Then the lines can be set like this:

// num_lines refers to the number of lines to set, not the number of
// lines in the request
gpiod_line_request_set_values(req, num_lines, offsets, values);

Bartosz

> > Does this make sense? I'm worried about the resource management here.
> > Who should be responsible for freeing the config structures? Should
> > the extended config take ownership? Should the user remain
> > responsible? Back to reference counting for these objects? Is this
> > even a good idea?
> >
>
> The user is responsible for freeing the request config.
> The request config is responsible for freeing the line configs, if there
> are any - the user isn't even aware of them so they obviously can't.
>
> Similarly if you merge the config into the request.
>
> > Please let me know what you think, I could use some advice.
>
> Hopefully I've communicated my meaning a little more clearly this time?
>
> Cheers,
> Kent.