Re: [libgpiod v2][PATCH 2/6] API: rename gpiod_request_config_get_num_offsets to gpiod_request_config_get_num_lines to match line_request pattern

[Kent Gibson <warthog618@xxxxxxxxx>]

On Tue, Mar 15, 2022 at 02:43:28PM +0100, Bartosz Golaszewski wrote:
> On Tue, Mar 15, 2022 at 12:59 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> >
> > On Tue, Mar 15, 2022 at 12:39:56PM +0100, Bartosz Golaszewski wrote:
> > > On Tue, Mar 15, 2022 at 12:23 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> > > >
> > > > On Tue, Mar 15, 2022 at 11:52:21AM +0100, Bartosz Golaszewski wrote:
> > > > > On Fri, Mar 11, 2022 at 8:40 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> > > > > >
> > > > > > Both gpiod_request_config and gpiod_line_request contain a number of
> > > > > > lines, but the former has a get_num_offsets accessor, while the latter
> > > > > > has get_num_lines.  Make them consistent by switching request_config to
> > > > > > get_num_lines.
> > > > > >
> > > > > > Signed-off-by: Kent Gibson <warthog618@xxxxxxxxx>
> > > > > > ---
> > > > >
> > > > > IMO this doesn't make much sense because we still have
> > > > > gpiod_request_config_set_offsets(). So now you have set_offsets but
> > > > > get_lines. At the time of preparing the request_config we're still
> > > > > talking about offsets of lines to request, while once the request has
> > > > > been made, we're talking about requested lines.
> > > > >
> > > >
> > > > Oh FFS we are always talking about lines.  Whether you have requested
> > > > them yet or not is irrelevant.  You WANT to request lines.
> > > > If we had globally unique line names we wouldn't give a rats about the
> > > > offset.
> > > >
> > > > And take another look - you have actually have get_offsets and
> > > > get_num_lines functions.
> > > >
> > > > Offsets is just one of the attributes of the lines, and this approach
> > > > still works if there is another fields of interest. e.g. values:
> > > >
> > > > int gpiod_line_request_set_values_subset(struct gpiod_line_request *request,
> > > >                                          size_t num_lines,
> > > >                                          const unsigned int *offsets,
> > > >                                          const int *values);
> > > >
> > > > which you then complain about in patch 4 as I'm writing this.... <sigh>.
> > > >
> > > > You could equally argue that one should be num_values.
> > > >
> > > > While we are still preparing the configuration, we are preparing the
> > > > config for LINES, not offsets.  Using num_lines is a reminder that you
> > > > need to provide the offset for each line - the two are inextricably
> > > > linked.  Using num_offsets could be taken to imply that
> > > > gpiod_request_config_set_offsets() can be called multiple times to add
> > > > offsets.
> > > >
> > > > > I would leave it as it is personally.
> > > > >
> > > >
> > > > I know, I know :-|.
> > > >
> > > > Cheers,
> > > > Kent.
> > >
> > > I didn't know I would see the whole extend of Kent's wrath after that
> > > comment. :)
> > >
> >
> > We're still way way off the full extent.
> >
> > Though "libgpiod v2 - the Wrath of Kent" does have a certain ring to it.
> >
> 
> Love it, let's make it official. :)
> 

Maybe not - one of the good guys dies at the end of that one, as does
the eponymous character :-(.

> > > Anyway let me try to defend myself before I wave the white flag and
> > > surrender as usual.
> > >
> > > We're setting VALUES so it's only normal to speak about NUMBER of VALUES.
> > >
> >
> > But you are happy to call it num_offsets?  I'm confused.
> >
> 
> Wat? No, the only num_offsets are present in get/set_offsets in request_config.
> 

My bad - you were being abstract and on first reading I took it literally.

My perspective is that you are setting the ATTRs on a NUMBER of OBJECTS,
so looking at it with the scope of the config, not the individual function.

But I see your point.

> > > It's like when you have an array of of X that is an attribute of Y,
> > > that array still carries a number of X and not Y.
> > >
> >
> > I get that, but in this case the offset is identifier for line.
> > The mapping is 1-1.
> >
> > > This is for patch 4 but this one has another problem. What if we
> > > extend this structure to - besides offsets - accept string identifiers
> > > for a request? Then we would want to use get_offsets and get_names (or
> > > get_ids) and the corresponding get_num_offsets and get_num_names
> > > accesors and in this case get_num_lines would become confusing.
> > >
> >
> > Good luck with that - no matter how you name things.
> > If you allow multiple identifiers then you have to deal with the
> > overlap case.  Just don't go there.
> > And what happens to gpiod_line_request_get_offsets where the size of
> > the buffer is determined by gpiod_line_request_get_num_lines()?
> >
> 
> The string identifiers would be translated to offsets at some point.
> Here it would make sense to retrieve the number of lines ACTUALLY
> requested and get their OFFSETS of which there are NUM_LINES.
> 

For the tool prototyping I've been doing I went with stringified id -> line,
with the stringified id mapped to line depending on the other
command line options, so it may be a name or an offset, depending.
Behind the scenes the line is always (chip,offset).

But that is really a higher level of abstraction that should be built
over libgpiod core, not builtin to it.  Unless it were also added to the
uAPI.

> > Much simpler to stick to a single type of identifier for the request.
> > Oh, and them the 1-1 mapping still holds, so you can still use num_lines.
> > No multi-dimensional thinking.
> >
> 
> I don't see a problem with current naming. You set offsets ->
> num_offsets, values -> num_values. Also: unlike function names this is
> not even part of ABI. We can even name it `n`, `nelem`, `num_elem`
> everywhere for all I care.
> 

A generic might be the way to go for the (offset,value) pairs split over
arrays case.

Cheers,
Kent.