Re: [libgpiod] Rethinking struct gpiod_line_bulk

[Bartosz Golaszewski <brgl@xxxxxxxx>]

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.

> 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.

>
> 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.

Bartosz