Re: [libgpiod] cxx bindings: time_point vs duration

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Thu, Oct 15, 2020 at 12:57 PM Helmut Grohne <helmut.grohne@xxxxxxxxxx> wrote:
>
> Hi,
>
> On Thu, Oct 15, 2020 at 12:05:23PM +0200, Bartosz Golaszewski wrote:
> > In case of the event timestamps - we get them from the kernel as
> > 64-bit unsigned integers. They're then converted to struct timespec as
> > defined by libc and eventually to ::std::chrono:duration. The
> > timestamps use the MONOTONIC kernel clock - the same that you would
> > use by calling clock_gettime(CLOCK_MONOTONIC, ...). Is there any way
> > to couple the C++ time_point to this clock?
>
> I got that wrong then as I thought they were wall clock time.

Well, it's a long story. It used to be what the kernel calls REALTIME
clock, then it was changed to MONOTONIC and now there's a suggestion
to make it configurable in v2. More on that here[1].

> CLOCK_MONOTONIC is the thing that backs std::chrono::steady_clock on
> Linux. At least gcc and clang's libcxx implement steady_clock using
> nanosecond resolution. I don't thing nanosecond resolution is
> guarantueed, but maybe this is good enough and you can just use
> steady_clock? That would certainly be most welcome by consuming client
> code.
>

One question is: even if on linux the steady_clock is backed by
CLOCK_MONOTONIC, is this a guarantee or just implementation? And can
we rely on this if it's not defined?

> > Me neither, but the new user API exposed by the kernel addresses a lot
> > of issues and adds new features and it's really impossible to keep the
> > current library API while also leveraging them. I'll keep supporting
> > the v1.6 stable branch for a long time though.
>
> Great news. Thank you.
>
> > Yes, it's what I intended indeed. I'm however worried that this isn't
> > the best approach. Having learned more, I now think that lines should
> > somehow weakly reference the chip - to emphasize that they don't
> > control its lifetime in any way (which is the case now with each line
> > storing a hard reference to the chip). Also what happens currently is
> > the fact that a new line object is created each time get_line() method
> > is called because we can't store lines in some array within the chip
> > because that would lead to circular references. Maybe a line should
> > store a weak_ptr to the underlying ::gpiod_chip? But then it would
> > create a new chip object every time get_chip() is called. Is there any
> > way around it? Making the chip and line non-copyable and expediting
> > any shared_ptr management to the user?
>
> First of all, I don't think any of these types need to be copyable.
> Keeping them moveable should be simple and makes handling them easy
> enough.
>

Right now we rely on them being copyable to store them in
::gpiod::line_bulk objects so that needs addressing too.

> It seems like you'd want a chip to include all lines as is the case for
> the C-api already. In that case, a line does not exist by itself, it
> only is an observable part of a chip. I'm not convinced that a line
> should weakly reference a chip. If you have a line and the underlying
> chip disappears, all the gpiod_lines get cleared and your line suddenly
> has a dangling reference. Actually, you line would be gone if it was
> part of the chip. That does not seem useful to me. So the current design
> of having lines control the lifetime of the chip seems useful to me.
>

Yes, except that with a weak_ptr we'd at least get a bad_weak_ptr
exception instead of a segfault if the programmer goofs and lets the
chip disappear before using a line.

> Why do you take issue with the fact that each get_line creates a new
> line object? This line object practically is a counted reference on the
> chip together with a line identifier. It kinda is a complex pointer. Why
> not have two distinct pointers point to the same thing?
>

It started with Python bindings actually. The pattern over there is
similar except that Python objects are much more complex to create and
initialize. Then I realized that C++ does the same. It also seems to
me like reversed logic where a line has the power to keep the chip
alive. Just doesn't feel good and I haven't seen this pattern
anywhere.

> Likewise creating new chip objects is not a problem in my book, because
> a chip object is a (counted) reference to a gpiod_chip. Having two
> pointers should be ok. Creating them is cheap.
>

Fair enough.

> > The thing with gpiod_line struct (the one from C libgpiod, not C++
> > class) is that the owner is the chip (struct gpiod_chip) - there's no
> > need to free any resources, they'll be freed when the chip goes out of
> > scope. You can copy the line pointer all you want, there's always a
> > single line behind stored in the opaque struct gpiod_chip. So in C++ -
> > I suppose - the chip should really own the C++ line (stored in a
> > vector maybe) and the line should at most weakly reference the chip
> > object. I'm just not sure how to correctly approach this so any advice
> > is welcome.
>
> That helps a lot with my understanding. I mostly concur up to the point
> where you say that a chip should own the C++ line. The C++ line
> currently is a counted reference on a gpiod_chip together with a line
> identifier. I don't see what's so precious about this to ensure it is
> not copied. That seems entirely fine with me.
>
> Now if you go the route and have chips own lines, then your back
> reference from line to chip can be a plain pointer. The forward
> reference ensures that the pointer is always valid. It's still circular,
> but you don't have to mess with weak_ptrs as long as destruction is
> careful.
>
> So this seems mostly fine as is. Having lines manage the lifetime of
> chips is very convenient for using the library.
>
> Helmut

That's interesting to read. I was under the impression that this is
bad C++ (or OOP in general). I have to think about it some more
though, because my gut is telling me this is just wrong on the logical
level - even if it's convenient.

Bartosz

[1] https://www.spinics.net/lists/linux-gpio/msg53796.html