Re: [libgpiod v2][PATCH v4 2/2] bindings: cxx: implement C++ bindings for libgpiod v2.0

[Kent Gibson <warthog618@xxxxxxxxx>]

On Wed, Nov 10, 2021 at 02:42:09PM +0100, Bartosz Golaszewski wrote:
> On Mon, Nov 8, 2021 at 2:04 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> >
> > On Thu, Nov 04, 2021 at 08:22:52PM +0100, Bartosz Golaszewski wrote:
> > > This is the bulk of work implementing C++ bindings for the new libgpiod
> > > API. The tests are not converted yet but the examples are fully
> > > functional. More details in the cover letter as this patch will be
> > > squashed with the one for the core C library anyway.
> > >
> >
> > I dislike the squashing approach.  I would rather accept that this branch
> > is unstable and keep the patches separate.
> > If you want stability at each commit then disable the building of any
> > unstable sections until they are fit to be included in the build again,
> > rather than squashing everything into one humungous patch.
> >
> 
> Ok, sounds good, will change the approach.
> 
> > I'd rather see working tests than working examples :-(, unless you
> > think your API is still in flux?  Separate patches for API,
> > implementation, tests, and examples would work for me.
> >
> 
> I'd love to start writing tests but I just can't get the gpio-sim
> upstream. :( I consider dropping the blocking configfs changes and
> just implementing the "committing" part locally with a dedicated
> attribute.
> 

I suspected that might be a sticking point.

That doesn't stop you writing tests - just publishing them.
Even then you could make the patch contingent on your configfs patch on
the assumption it will make it into mainline soon.

And it would be good to exercise both interfaces, to better demonstrate
the utility of the gpio-sim, and to test libgpiod2 as well the gpio-sim
itself.

<snip>

> > > +GPIOD_CXX_API ::std::ostream& operator<<(::std::ostream& out, const line_config& config)
> > > +{
> > > +     out << "line_config(defaults=(direction=" << direction_names.at(config.direction()) <<
> > > +            "edge_detection=" << edge_names.at(config.edge_detection()) <<
> > > +            "bias=" << bias_names.at(config.bias()) <<
> > > +            "drive=" << drive_names.at(config.drive()) <<
> > > +            (config.active_low() ? "active-low" : "active-high") <<
> > > +            "debounce_period=" << config.debounce_period_us() << "us" <<
> > > +            "event_clock=" << clock_names.at(config.event_clock()) <<
> > > +            "default_output_value=" << config.output_value() <<
> > > +            ")";
> > > +
> > > +     if (config.num_overridden_offsets()) {
> > > +             out << "overrides=[";
> > > +
> > > +             for (const auto& offset: config.overridden_offsets()) {
> > > +                     out << "(offset=" << offset <<
> > > +                            "direction=" << direction_names.at(config.direction(offset)) <<
> > > +                            "edge_detection=" << edge_names.at(config.edge_detection(offset)) <<
> > > +                            "bias=" << bias_names.at(config.bias(offset)) <<
> > > +                            "drive=" << drive_names.at(config.drive(offset)) <<
> > > +                            (config.active_low(offset) ? "active-low" : "active-high") <<
> > > +                            "debounce_period=" << config.debounce_period_us(offset) <<
> > > +                            "event_clock=" << clock_names.at(config.event_clock(offset)) <<
> > > +                            "output_value=" << config.output_value(offset) <<
> > > +                            ")";
> > > +             }
> > > +
> > > +             out << "]";
> > > +     }
> > > +
> > > +     out << ")";
> > > +
> > > +     return out;
> > > +}
> > > +
> >
> > This stringification is both too verbose (reporting fields that may not
> > be overridden) and too terse (not reporting lines that are overridden
> > with the default value).
> >
> 
> Oh... I think I get it now. If we override the line with the current
> default value, then change the default value, the overridden line
> should keep it's overridden value. Is that it?
> 

Pretty much.  The general question is what the semantics of each
line_config function is and how they interact with each other.
Any reasonably sized command set is going to have some ambiguity - some
use case it handles badly that will require multiple function calls
to achieve the desired outcome.  e.g. in your current patch the only
way to clear an override is to start from scratch (or set it to the
default and assume the default wont change subsequently).
It is a question of how to minimise complexity and maximise utility.
Which is why I prefer a global "set all lines to this - and clear any
overrides".
But it is a corner case either way, and I don't have a compelling use
case either way, so not terribly fussed either way - just saying I'd do
it differently.

The specific problem here is the overridden_offsets() filters out useful
information, specifically which lines are overridden to the default, while
the streaming operator also doesn't filter other attributes which have NOT
been overridden. i.e. it reports ALL the attributes for a line if ANY
attribute is overridden from the default.
So problems in patch 1 as well as here.

> > What are you wanting to display here - the full state (what the user has
> > set) or the effective state (what the kernel will see)?
> > If the former then include the defaults and all overrides -
> > even when overridden to the default - but not attributes that are not
> > overridden.
> > If the latter then as per the full state but filter out overrides that
> > correspond to the default or that are ineffective (e.g. the default
> > edge_detection when all lines are outputs).  And maybe filter out the
> > defaults that match the kernel defaults?
> 
> The former of course, we don't want to expose the kernel logic to users.
> 

We do want to expose kernel functionality to users, and some kernel
logic tends to stick to that.
You don't think it would be useful to know what the actual effect of
your config will be?

But agreed - the streaming operator here should dump the contents of the
line_config so the user can see what they have set.  I see the other
as a nice to have.

Cheers,
Kent.