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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Wed, Apr 28, 2021 at 12:34 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>
> On Wed, Apr 28, 2021 at 11:19:05AM +0200, Bartosz Golaszewski wrote:
> > 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?
> >
>
> The A, B, C nomenclature originated for attributes, for which blanket
> sets (A) make more sense.
>
> The case for using A for outputs would be if the vast majority of your
> lines default to one value.  You would then only have to use the other
> (B, C or whatever) for the non-default lines.
>
> > > 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.
> > >
>
> For future reference, lets call this case D.
>
> >
> > ... 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?
> >
>
> No need for all four - two would be sufficient - probably B and D as you
> suggest.
>

Agreed.

[snip]

> > > >
> > > > 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 know.
>
> My concern is that the simplest get use case is 7 function calls:
> 1. create request_config
> 2. create line_config
> 3. request lines
> 4. do the actual work
> 5. release request
> 6. free line_config
> 7. free request_config
>
> And that is ignoring the chip open/close functions.
>

This doesn't bother me all that much. This is the low-level C API.
It's supposed to be a bit clunky. It'll get way simpler in the
high-level bindings.

In Python a simple case would look something like this:

with gpiod.Chip("/dev/gpiochip0") as chip:
    with chip.request_lines(gpiod.RequestConfig(offsets=( 0, 2, 3 )),

gpiod.LineConfig(direction=gpiod.DIRECTION_INPUT)) as req:
        values = req.get_values() # Read all values
        values = req.get_values(offsets=( 2, 3)) # Read values of specific lines

[snip]

> > >
> > > 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); :(
> >
>
> Alternatively it could be in one of the constructors -
> gpiod_request_config_new(), gpiod_line_config_new() or
> gpiod_chip_request_lines()?
>

I want to make the struct reusable/modifiable so setting it only in
the constructor would disallow it and having both would be redundant.
And the last one: the less function arguments the better IMO. So I'm
for having it in the request_config.

> > > 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.
> >
>
> Sure doesn't if you have to set the offsets in the config.
> But might do if you provide them to the gpiod_chip_request_lines().
>
> The null case could then request the lines as-is?
>
> There seems to be a bit of interest in as-is of late, and the simplest
> case would then be three function calls.
>

Ok, makes sense. Null line-config -> request lines as is.

> > > > 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()?
> >
>
> gpiod_line_request_reconfigure(req, cfg) is not modifying the line_config
> - it is modifying the request WITH the modified config.
>
> If gpiod_line_request_<splat>_line_config(req, cfg) works better for you
> then I'd prefer something along the lines of _apply_ or _commit_, as it is
> taking the line_config, modified by sets, and applying it to the
> requested lines.
>

How about gpiod_line_request_reconfigure_lines()?

Bart