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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

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.

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