On Fri, Dec 17, 2021 at 10:54 AM Viresh Kumar [off-list ref] wrote:

On 17-12-21, 10:43, Bartosz Golaszewski wrote:

quoted

On Fri, Dec 17, 2021 at 10:32 AM Viresh Kumar [off-list ref] wrote:

quoted

On 17-12-21, 10:12, Bartosz Golaszewski wrote:

quoted

No, it's a different story altogether. In C the buffer allocates
memory for events and when you "get" an event, you only have a pointer
to the memory space in the buffer that you must not free. But you can
"copy" an event with gpiod_edge_event_copy() which returns you a deep
copy of the event that will survive the parent and that must be freed
with gpiod_edge_event_free(). This is done so that by default we try
to limit the number of allocations (as there can be a lot of events)
unless the user decides to manually copy the event.

In C++ I used that mechanism together with the buffer's const
event_get() and event's copy assignment operator. "Getting" an event
returns a const reference to the event (still in buffer's memory) but
copying it triggers a deep copy. The memory management is of course
handled by the destructor.

This is not used in Python as speed is no longer a concern and we'd be
creating new python objects anyway. But in Rust, I think it makes
sense to reuse this mechanism.

Ahh, what about this then, it just caches all the values when the event is
requested ?

pub struct EdgeEvent {
    event_type: LineEdgeEvent,
    timestamp: Duration,
    line_offset: u32,
    global_seqno: u64,
    line_seqno: u64,
}

Then does it make sense to make it a binding to libgpiod? :)

Yes, because EdgeEvent::new() gets all these values from the internal C libgpiod
library itself. We are going to make the same copy of data, either at the C
library level, or here.

quoted

I don't know Rust that well to be able to come up with an idea off the
top of my head but I assume inheritance in one way or another exists
in rust? Can you have an EdgeEvent interface implemented by two
specialized structs, one that's stored in the buffer (stores a pointer
to the internal event)

This is what my initial patch had, right? It isn't safe. There is no way for
libgpiod-rust to guarantee that the users don't access the EdgeEvent after the
buffer is freed and this needs to be made safer at libgpiod itself.

I'm not sure it had the same thing. In libgpiod it's simple a matter
of definition. The docs state that a pointer retrieved using
gpiod_edge_event_buffer_get_event() must not be freed while one
obtained with gpiod_edge_event_copy() does have to be disposed of. In
C++ it's safe because either you hold a const reference to the object
stored in the event buffer or you have copied it and now it will
survive the parent meaning the user can access the event even after
the buffer was deleted.

quoted

and one that stores the copy (and frees it when
it goes out of scope)?

I don't understand what we will get out of such a scheme? Is there any benefit
of getting these values via the pointer to the internal C event ?

You avoid allocating memory for those objects. The memory exists in
the C buffer. In order to keep a similar level of performance you'd
need to replicate the behavior of the buffer in rust as well.

Bart

Any user that wants to use the edge-event data, needs to call the edge-event
APIs at least once and this is what the new version does, just calls them from
new() itself.