Re: [libgpiod] Rethinking struct gpiod_line_bulk

[Kent Gibson <warthog618@xxxxxxxxx>]

On Tue, Oct 20, 2020 at 12:47:02PM +0200, Bartosz Golaszewski wrote:
> On Mon, Oct 19, 2020 at 6:21 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> >
> > On Mon, Oct 19, 2020 at 03:31:07PM +0200, Bartosz Golaszewski wrote:
> > > On Tue, Oct 13, 2020 at 10:53 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> > > >
> > >
> > > [snip]
> > >
> > > > >
> > > > > struct gpiod_line_bulk {
> > > > >     struct gpiod_chip *owner;
> > > > >     unsigned int num_lines;
> > > > >     uint64_t lines;
> > > > > };
> > > > >
> > > > > And the 'lines' bitmap should actually refer to offsets at which the
> > > > > owning chip stores the line pointers in its own 'lines' array - up to
> > > > > 64 lines.
> > > > >
> > > > > But we'd still have to sanitize the values when adding lines to a bulk
> > > > > object and probably check the return value. I'm wondering if there's a
> > > > > better way to store group references to lines on the stack but I'm out
> > > > > of ideas.
> > > > >
> > > >
> > > > So you are proposing keeping the bulk of the bulk in the background and
> > > > passing around a flyweight in its place.  Makes sense.
> > > >
> > >
> > > So this won't fly of course because a bitmap doesn't hold enough
> > > information to reference an arbitrary number of lines in the chip in
> > > any meaningful way.
> > >
> >
> > Yeah, that is what I was trying to get at earlier, but from your replies
> > I assumed you would have an array mapping from bitmap index to line
> > hidden somewhere behind the scenes on a per-bulk basis...
> >
> > > I have another idea. I can live with struct gpiod_bulk being allocated
> > > dynamically whenever users of the library use it - because it's quite
> > > unlikely they'd do it all that often. What I'd like to avoid is
> > > allocating a new bulk object whenever we want to package a single line
> > > passed to e.g. gpiod_line_request() before we propagate it to
> > > gpiod_line_request_bulk().
> > >
> > > How about we define struct gpiod_line_bulk as:
> > >
> > > struct gpiod_line_bulk {
> > >     unsigned int num_lines;
> > >     unsigned int max_num_lines;
> > >     struct gpiod_line *lines[1];
> > > };
> > >
> > > And expose it in gpiod.h header?
> > >
> >
> > I don't have a problem with that definition, appropriately documented,
> > but I wouldn't want to expose that to the user, so my preference would
> > be to make it opaque, but....
> >
> > ... that will make the API more awkward to use, as the methods that
> > populate a bulk will now have to have that bulk constructed beforehand.
> > Or you change them to accept a struct gpiod_line_bulk ** to return the
> > bulk constructed by libgpiod.  Either way the user will have to free the
> > bulk afterwards.  But there are only a few of those of those methods.
> >
> 
> Or we can even make these methods return struct gpiod_line_bulk *
> which is cleaner than passing struct gpiod_line_bulk ** as argument
> IMO. I'm fine with that.
> 

Returning NULL for the error cases, as per gpiod_chip_find_line()?

> > And all the bulk accesor methods are currently inline - that would also
> > have to change if you go opaque, so you will get a code size and
> > performance hit as well.
> 
> This may slightly increase the size of the library indeed but it will
> decrease the size of the user programs. Anyway: we're talking about
> miniscule changes. Performance hit isn't that much of a concern either
> - function calls are fast and we're not adding new system calls that
> could lead to more context switches.
> 

Those inlines are just simple field accessors and mutators, and you'd hope
that has less overhead than a function call.

Opaque is certainly preferable - it reduces the API surface and you are
also free to change the implementation later.

Can we also add in range checking to functions that accept offsets while
you are at it? e.g. gpiod_line_bulk_get_line() should return NULL if the
offset is out of range. Or are we assuming that the user will always
ensure offset < num_lines first?  If the latter can we at least document
the bad behaviour if offset >= num_lines?

> >
> > Maybe keeping it exposed is the best - just heavily documented?
> > At least start out that way, since it is closest to what is already there.
> >
> 
> I'm now actually leaning more towards making it opaque but I need to
> find a way to make gpiod_line_bulk_foreach_line work with hidden bulk
> struct.
> 

Why not just drop it in favour of gpiod_line_bulk_foreach_line_off()?

Cheers,
Kent.