Re: [libgpiod v2.0][PATCH] core: extend config objects

[Kent Gibson <warthog618@xxxxxxxxx>]

On Thu, Aug 12, 2021 at 02:51:02PM +0200, Bartosz Golaszewski wrote:
> On Thu, Aug 12, 2021 at 12:29 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> >
> 
> [snip]
> 
> > > >
> > > > My preference would be for gpiod_line_config_set_output_value() and
> > > > variants to also set the direction for those lines to output.
> > > > And maybe rename it to gpiod_line_config_set_output().
> > > > And maybe add a set_input for symmetry.
> > > >
> > >
> > > Any naming idea for setting the direction to "AS_IS"?
> > >
> >
> > Since as-is is vague you need to include the field name.
> > So I would remove set_direction and replace it with set_input, set_output
> > and set_direction_as_is (which I would expect to see used very rarely in
> > the wild, as the only use case I can think of for it is undoing a
> > set_input or set_output call).
> >
> > > > But my concern above was actually the secondary array - that confused me.
> > > > And it's big - always. (OTOH it's on the heap so who cares?)
> > > > The array is of size GPIO_V2_LINE_NUM_ATTRS_MAX, yet each entry could
> > > > have multiple attributes set - so long as the offsets subsets match?
> > > > What happens if both debounce and flags are set for the same subset?
> > > > Looks like debounce wins and the flags get discarded in
> > > > gpiod_line_config_to_kernel().
> > > >
> > >
> > > Yeah, I aimed at ironing it out when writing tests. You're probably right here.
> > >
> >
> > Same reason I hadn't paid much attention to the implementation.
> >
> > > > What I had in mind for the config was an array of config for each line,
> > > > only performing the mapping to uAPI when the actual request or
> > > > reconfigure is performed, though that requires knowledge of the number
> > > > of lines in the request to be sized efficiently in the
> > > > gpiod_line_config_new().  Sizing it as GPIO_V2_LINES_MAX would be even
> > > > worse than your secondary array, so don't want that.
> > >
> > > Or would it? Currently the full config structure is 3784 bytes on a 64
> > > bit arch. The base config is 32 bytes. If we added the default value
> > > to base_config that would make it 36 bytes x GPIO_V2_LINES_MAX = 2304
> > > bytes. We'd need another base_config for default settings.
> > >
> > > Unless I'm missing something this does seem like the better option.
> > >
> >
> > Yikes, I overlooked the size of the offsets array in the secondary
> > config - that is a significant contributor to the config size as well.
> > For some reason I was thinking that was a bitmap, but that couldn't work.
> >
> > In that case a GPIO_V2_LINES_MAX sized array is clearly a better way to
> > go, and that is a surprise.
> > Though those array elements will require the line offset as well as the
> > base_config, unless you have some other way to map offset to config?
> >
> 
> No, but that's fine - see below.
> 
> > > > My Go library uses a map, but that isn't an option here.
> > > > Resizing it dynamically is the option I was last pondering,
> > > > but my preference would be to add a num_lines parameter to the new.
> > > > Anyway, that was what I was wondering about...
> > > >
> > >
> > > We could resize the array dynamically but we'd need to return error
> > > codes from getters.
> >
> > Why? If there is no config for the requested line then you return the
> > global default value, right?
> > Why does that change if the array is resizable?
> > Btw, I'm assuming that the gpiod_line_config would contain a pointer to
> > the dynamic array, so the handle the user has would remain unchanged.
> > And the getters all return ints, not pointers to internal fields.
> > What am I missing?
> 
> Memory allocation failures when resizing.
> 

Why are you resizing in a getter?
If you mean setters then fair enough - though you could just sit on
the error until the request or reconfigure and return it there instead.

Cheers,
Kent.

> >
> > Also, "global default value" is different from the primary, right?
> > Perhaps getters should return the primary value, which itself defaults
> > to the global defaults, if the line doesn't have specific config?
> >
> > > We could also define the size when allocating the
> > > config but it's a sub-par approach too.
> > >
> >
> > Sure, it's a trade-off, but the alternative is requiring a 2-3k block
> > even for a one line request, which seems a wee bit excessive.
> >
> 
> As you said - it's on the heap, so who cares. But this is also an
> internal structure and so we can use bit fields. That should reduce
> the memory footprint significantly as we now don't require more than 3
> bits for any given enum. That would leave us with the debounce period
> and offset as full size variables.
> 
> Bart
> 
> > With the proposed API, the only other alternative I can see to give a
> > small footprint is dynamic resizing, which I'm not thrilled by either.
> > So just wanted to double check that you are content to lock in the
> > gpiod_line_config_new API, as that will constrain any optimisation later
> > on.
> >