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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Fri, Apr 23, 2021 at 3:39 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>

[snip]

>
> > > 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?
> >
>
> No - the A case sets ALL lines to one value.

Apart from a single line request - what could possibly be the use-case for that?

> B is one line to one value.
> C is a set of lines to one value.

This makes a bit more sense but ...

>
> A set of lines to a set of values is a new case.
> And yes - two arrays as per your set_values() below.
>

... to me this and B are the most sensible options. Do we really need
A & C for line reading and setting? Do you find use for that in your
Go lib?

> > 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.
>
> Yup, as I said, the semantics for the unified object are more confusing.
>
> In the Go implementation, the request options can be passed to the
> request_lines(), but not the set_config(), cos interfaces.
>
> There is no good way to flag that in C at compile time. For a runtime
> check you could add a return code to the option mutators and return an
> error if the lines have already been requested.
>

I agree that it doesn't map well to C and this is why I think it would
be less confusing if we went with two structs instead.

> > 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 user manages the lifecycle of all three?

Yes.

>
> I can live with that, though I would probably still lean towards the
> unified object approach - with the option mutators getting return codes.
>
> > The first one holds the composite of primary and secondary configs and
> > is modified using mutators according to this scheme:
> >
>
> Which is why it should be called request_config, not line_config.
> line_config is misleading - it says line but its scope is request.
>

It depends on how you look at it really. Its scope are *the lines* in
the request, not the request itself (unlike the event buffer size or
line offsets). It says line because gpiod_lines_config would look
bizarre.

> And of course request_config should be request_options ;-).

I'm still not there yet.

>
> > 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);
> >
>
> I personally prefer the _set_line_<attr> style as it reads better, but I
> can live with this - I know you prefer suffixes for variants.
>
> > 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.
> >
>
> Where are the set of requested lines specified?
>

They map the uAPI in that the offsets are set in struct gpiod_request_config:

gpiod_request_config_set_line_offsets(config, num_lines, offsets); :)

or

gpiod_request_options_set_line_offsets(config, num_lines, offsets); :(

> Do null config ptrs result in something useful, but guaranteed harmless,
> such as requesting lines as input?  Or are they required non-null?
>

I normally expect users to pass a valid pointer and don't make the
functions null aware. In this case - it's a good question. I'm
wondering if it is useful at all? The user should IMO specify what
they want to do with the lines? I would lean towards non-null line
configfs.

> > 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);
> >
>
> At first glance that feels a bit odd, being on the request while the
> others all operate on the config, but it maps to the SET_VALUES ioctl(),
> not the SET_CONFIG, so that makes sense.
>
> There is a corresponding get_values(req, num_lines, offsets, values)?
>

Sure, just didn't include it in the example.

> And a reconfigure(req, cfg)?
>

Sure, just haven't decided on the name yet.
gpiod_line_request_reconfigure() would be what you're suggesting but
it sounds like it would reconfigure the request and not modify the
line configuration. I would prefer something closer to
gpiod_line_request_set_line_config() which is as verbose as it gets.
Maybe even gpiod_line_request_change_line_config()? Or
gpiod_line_request_modify_line_config()?

Bartosz

> Cheers,
> Kent.
>