Re: [libgpiod] gpiomon loses events

[Kent Gibson <warthog618@xxxxxxxxx>]

On Mon, Sep 14, 2020 at 06:55:20PM +0300, Andy Shevchenko wrote:
> +Cc: libgpiod maintainers
> 
> On Mon, Sep 14, 2020 at 6:54 PM Andy Shevchenko
> <andy.shevchenko@xxxxxxxxx> wrote:
> >
> > On Mon, Sep 14, 2020 at 6:12 PM Andy Shevchenko
> > <andy.shevchenko@xxxxxxxxx> wrote:
> > >
> > > On Mon, Sep 14, 2020 at 1:40 PM Maxim Devaev <mdevaev@xxxxxxxxx> wrote:
> > > >
> > > > Hi. I noticed a strange behavior of gpiomon and gpiomon.py. It seems
> > > > that in some cases, if the signal arrives at the GPIO pin too quickly,
> > > > the last event on it may be rising, despite the fact that the actual
> > > > signal is already set to 0.
> > >

Hi Maxim,

It looks like you have run across a bug in libgpiod, that I recently submitted
a patch for, whereby reading events can discard events from the kfifo.
The discarded events are the most recent.
The problem is particularly bad for reading single events, as gpiomon
does - any other events in the kfifo will be lost.

The bug was introduced in libgpiod v1.5 so, depending on your
circumstances, I would revert to an earlier libgpiod or apply my patch.
(let me know if you can't find it and I'll send you a copy).

A workaround with the unpatched v1.5.x is to only read events using
gpiod_line_event_read_fd_multiple(), or one of the functions that wrap
it, with num_lines=16.
For the python binding that would be event_read_multiple() - as you have
discovered.

> > > I'm not sure I understood what's wrong here. 'Event' by definition is
> > > something that has already happened. If pin floats from 0 to 1 to 0
> > > you will get one rising and one falling event.
> > >
> > > > a Cursory study of the sources showed that
> > > > both of these utilities read only one (the first?) event from the
> > > > line. I changed gpiomon.py rby replacing read_event() to
> > > > read_event_multiply() and iterating by all events, and it looks like
> > > > the lost faling events were there.
> > > >
> > > > So, I have a few questions.
> > > >
> > > > 1) Is this really a bug in gpiomon, or is it intended to be?
> > > >
> > > > 2) If I use read_events_multiple(), can it skip some events? I noticed
> > > > that sometimes I can get several falling and rising in a row.
> > > > Why is
> > > > this happening?
> > >

Generated events are buffered in the kernel - to a point.  Beyond that
any new events are discarded.  The current limit is hard coded in the
kernel at 16.

Also, if you toggle the line faster than the kernel can service the
resulting interrupts then you can lose events.

> > > I can assume that IRQ handler is reentrant and since it has been run
> > > in the thread we will get it messed up. The timestamp of the event
> > > (when recorded) should be used for serialization of events. But if my
> > > assumption is the case, we have to record it in a hard IRQ context.
> >
> > ...but this is exactly what we are doing in the latest code (didn't
> > check from which kernel it's a default approach).
> >
> > So, do you have the timestamps not paired?
> >
> > > > Shouldn't they be paired? Can the state transition,
> > > > i.e. the final falling or rising, be lost?
> > > >
> > > > 3) It seems there can only be 16 events in a line. What happens if
> > > > more events occur in one iteration of the loop, such as 20? The last 4
> > > > events will be lost, they will be available in the next iteration of
> > > > event_wait(), or the first 4 events in the current iteration will be
> > > > discarded?
> > >
> > > It's how kfifo works, AFAIU it should rewrite first (older) ones.
> > >

The edge detection in the kernel only writes to the kfifo if it is NOT
full, so it actually discards the most recent - you will only get the
first 16 events. The last 4 events of your 20 will be lost.

I would actually prefer it worked as Andy thinks it does as the more
recent events are generally more valuable.
Bart, do you have any opinion on that?
(this would be for uAPI v2, not lineevent, so as not to alter existing
behaviour)

We are working on changes that will make debugging cases such as this
easier, as well as allowing some userspace control over the event
buffer size, but those wont be available until libgpiod v2.

Cheers,
Kent.