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

[Kent Gibson <warthog618@gmail.com>]

On Fri, Jul 30, 2021 at 10:41:49AM +0200, Bartosz Golaszewski wrote:
> On Fri, Jul 30, 2021 at 8:42 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Tue, Jul 27, 2021 at 04:34:00PM +0200, 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.
> > >
> >
> > My apologies for not getting around to reviewing v2, so here is a
> > quick(??) review of v3...
> >
> > I don't have any problems with the C API changes in patches 1 and 2.
> > I would prefer that we didn't need the num_lines, offsets and capacity
> > getters, but their addition is a reasonable compromise.
> >
> > <snip>
> >
> > > -GPIOD_CXX_API bool is_gpiochip_device(const ::std::string& path)
> > > +struct chip::impl
> > >  {
> > > -     return ::gpiod_is_gpiochip_device(path.c_str());
> > > -}
> > > +     impl(const ::std::string& path)
> > > +             : chip(open_chip(path)),
> > > +               name(::gpiod_chip_get_name(this->chip.get())),
> > > +               label(::gpiod_chip_get_label(this->chip.get()))
> > > +     {
> > > +
> > > +     }
> > > +
> > > +     impl(const impl& other) = delete;
> > > +     impl(impl&& other) = delete;
> > > +     impl& operator=(const impl& other) = delete;
> > > +     impl& operator=(impl&& other) = delete;
> > > +
> > > +     void throw_if_closed(void) const
> > > +     {
> > > +             if (!this->chip)
> > > +                     throw ::std::logic_error("GPIO chip has been closed");
> > > +     }
> > > +
> >
> > Perhaps throw something more specific than a logic_error?
> >
> 
> There's no standard exception that would inherit from std::logic_error
> that would fit here. I can define new exceptions (gpiod::chip_closed
> and gpiod::request_released) inheriting from logic_error if that works
> for you.
> 

Exactly what I had in mind.

> > <snip>
> >
> > > diff --git a/bindings/cxx/edge-event.cpp b/bindings/cxx/edge-event.cpp
> > > new file mode 100644
> > > index 0000000..d6dce22
> > > --- /dev/null
> > > +++ b/bindings/cxx/edge-event.cpp
> > > @@ -0,0 +1,123 @@
> > > +// SPDX-License-Identifier: LGPL-3.0-or-later
> > > +// SPDX-FileCopyrightText: 2021 Bartosz Golaszewski <brgl@bgdev.pl>
> > > +
> > > +#include <map>
> > > +#include <stdexcept>
> > > +#include <system_error>
> > > +#include <utility>
> > > +
> > > +#include "internal.hpp"
> > > +
> > > +namespace gpiod {
> > > +
> > > +namespace {
> > > +
> > > +const ::std::map<int, edge_event::type> event_type_mapping = {
> > > +     { GPIOD_LINE_EVENT_RISING_EDGE,         edge_event::type::RISING_EDGE },
> > > +     { GPIOD_LINE_EVENT_FALLING_EDGE,        edge_event::type::FALLING_EDGE }
> > > +};
> > > +
> >
> > Use of a map for just two event types seems like overkill.
> > Wouldn't an if-else in edge_event::get_type(void) be simpler and cleaner?
> > Also, as written, edge_event::get_type(void) can throw std::out_of_range
> > which doesn't seem right. Wouldn't std::range_error be more appropriate?
> > Or adding an unknown event type for failed mappings, so get_type() can
> > never throw??
> >
> 
> While it's true that we can throw the range_error, it's very unlikely
> and would occur only if a serious programming error was present in the
> C library or the kernel.

No - you will currently throw ::std::out_of_range, which is weird.
I'd be ok with ::std::range_error, but that would involve a catch
and re-throw.

And it could also happen if the kernel adds new event types that this
version of the library is unaware of.
While that isn't terribly likely, don't box yourself in.

> I'm fine with this possibility - adding an
> unknown type, would suggest this is a normal situation and not an
> exception.
> 

Fair enough.
That suggestion was coming from "a simple getter shouldn't throw" PoV,
btw.

> For the mapping: to me this looks more consistent and also allows to
> detect the rare and unlikely error mentioned above. In any case - this
> is an implementation detail and can be changed later.
> 
> > Same applies for mappings throughout.  If the C++ values match the C
> > values then you could just perform a range check and cast for the cases
> > where there are more than two values.
> >
> 
> Casting in general is to be avoided in C++ - as defined in the
> document Jack linked recently. Mapping isn't expensive and looks more
> robust IMO. I would prefer to leave it like this.
> 

If casting is so frowned upon then why have they defined so many types
of casts?  I thought the trick was to pick the right one ;-).
And we are only talking about casting between int enums here, so
really...

> > <snip>
> >
> > > +     /**
> > > +      * @brief Retrieve the current snapshot of line information for a
> > > +      *        single line.
> > > +      * @param offset Offset of the line to get the info for.
> > > +      * @param watch Indicates whether the caller wants to watch for line
> > > +      *              info changes.
> > > +      * @return New ::gpiod::line_info object.
> > > +      */
> > > +     line_info get_line_info(unsigned int offset, bool watch = false) const;
> > > +
> > > +     /**
> > > +      * @brief Wrapper around ::gpiod::chip::get_line_info that retrieves
> > > +      *        the line info and starts watching the line for changes.
> > > +      * @param offset Offset of the line to get the info for.
> > > +      * @return New ::gpiod::line_info object.
> > > +      */
> > > +     line_info watch_line_info(unsigned int offset) const;
> > > +
> >
> > I repeat my objection to get_line_info() accepting a watch parameter and
> > watch_line_info() wrapping it.  While they both happen to return
> > line_info, the purpose of the two methods are quite different and so
> > should be kept separate.
> >
> 
> OK, I will change it.
> 
> > Also, should be watch_line_info() really be allowed on a const chip?
> > While it does not alter the C++ object, and so technically can be allowed
> > on a const, it does alter the state of the underlying C object and so I
> > would argue it should not be allowed.
> >
> 
> Makes sense.
> 
> > <snip>
> >
> > > +     /**
> > > +      * @brief Get the constant reference to the edge event at given index.
> > > +      * @param index Index of the event in the buffer.
> > > +      * @return Constant reference to the edge event.
> > > +      */
> > > +     const edge_event& get_event(unsigned int index) const;
> > > +
> > > +     /**
> > > +      * @brief Get the number of edge events currently stored in the buffer.
> > > +      * @return Number of edge events in the buffer.
> > > +      */
> > > +     unsigned int num_events(void) const;
> > > +
> >
> > A getter not prefixed with "get_" ;-).
> > (note that I'm kidding - I prefer it this way - more on that later in
> > class line_info)
> >
> > > +     /**
> > > +      * @brief Maximum capacity of the buffer.
> > > +      * @return Buffer capacity.
> > > +      */
> > > +     unsigned int capacity(void) const noexcept;
> > > +
> >
> > And again.
> >
> > <snip>
> >
> > > +/**
> > > + * @brief Contains a set of line config options used in line requests and
> > > + *        reconfiguration.
> > > + */
> > > +class line_config
> > > +{
> > > +public:
> > > +
> >
> > This class has setters but no getters.
> > There is no use case for getters?
> > Also applies to C API - just wondering given the addition of the getters
> > in patch 1 and 2 - there may be cases where the user would like to read
> > back the config?
> >
> 
> IMO if a use-case like this appears, we can extend the interface. The
> reason for adding the new functions was because we added the all-lines
> getters and now the user would need to store the offsets outside the
> request if we didn't provide them. Adding getters for config
> structures now would unnecessarily bloat the interface. Let me know if
> you disagree though. I usually eventually get convinced by you. :)
> 

Agreed keep the interface minimal and easy enough to add later if the need
arises.  Just throwing it out there in case there are use cases I've
overlooked.

> > > +     /**
> > > +      * @brief Direction settings.
> > > +      */
> > > +     enum class direction : unsigned int
> > > +     {
> > > +             AS_IS = 1,
> > > +             /**< Request the line(s), but don't change current direction. */
> > > +             INPUT,
> > > +             /**< Request the line(s) for reading the GPIO line state. */
> > > +             OUTPUT
> > > +             /**< Request the line(s) for setting the GPIO line state. */
> > > +     };
> > > +
> >
> > line_info and line_config should use common direction types (also
> > applies to C API)? Perhaps in a ::gpiod::line namespace?
> >
> 
> So that would seem obvious to do this but the values stored in line
> info and passed to line config are not always the same. BIAS_UNKNOWN
> and BIAS_AS_IS are not the same and the former is only suitable for
> line_info while the latter only works for the config.

Sure they are distinct - so give them different values in the C API.
line_info will never return BIAS_AS_IS, and line_config will reject
setting BIAS_UNKNOWN.  So document that.

On a related note, the C API should have a GPIOD_LINE_CONFIG_BIAS_AS_IS,
and that should be the default - as it is in the kernel.
GPIOD_LINE_CONFIG_BIAS_DISABLED is actively disabling bias, which is a
different thing.
Or are you intending for libgpiod to always actively specify bias?

> I agree though
> that it may be a bit confusing for users. If we'd change it - where
> should these go wrt doxygen sections of the core API?
> 

You would think that doxygen should be subservient to the code, not the
other way around :-|.

Given I'm suggesting the GPIOD_LINE_CONFIG_xxx enums get folded into
the GPIOD_LINE_xxx enums, they would end up wherever those are?

Cheers,
Kent.