[PATCH libevdev] tools: print the mean frequency together with the max frequency

Peter Hutterer peter.hutterer at who-t.net
Mon Sep 19 13:52:45 UTC 2016


On Mon, Sep 19, 2016 at 12:05:20PM +0200, Benjamin Tissoires wrote:
> On Mon, Sep 19, 2016 at 3:03 AM, Peter Hutterer
> <peter.hutterer at who-t.net> wrote:
> > And if they're 30% out, print a warning. On the ThinkPad X1 Wireless Touch
> > Mouse (when connected via bluetooth) we get a bunch of events at the start of
> > the movement, all less than 1ms apart. Best guess is that the device goes to
> > low-power, then notices the movement and buffers the event until the BT
> > connection is back up. Then it sends all events at once. Usually they're less
> 
> This reminds me the exact same bug we had on USB. The internal device
> output queue filled in while the device being put on suspend, and when
> resumed, the queue was emptied and spurious events were sent. We got
> it fixed by ignoring the first few events:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=b905811a49bcd6e6726ce5bbb591f57aaddfd3be
> 
> I wonder if the same should not be done for bluetooth devices.

could be done, but this would have to be something in the kernel, we can't
know what the magic timeout is for those devices.

> > than 1ms apart, but at one recording showed a 37ms delay before we go back to
> > the normal 70ms (~15Hz) the mouse has otherwise.
> >
> > This is unpredictable enough that we can't just work around it so instead
> > print a warning to the user so they can go investigate.
> 
> Well, if this is the same type of bug we had in USB, we could have the
> same in this tool. If the first reports are obviously too close to
> each other, discard them.

problem is, in userspace we don't know what the "first" reports are because
the timeout is undefined. and while it's easy to ignore events that are
within 1ms of each other, one of the recordings shows a 37ms (as opposed to
the average 70ms). This is something we couldn't detect.
 
> > https://bugs.freedesktop.org/show_bug.cgi?id=97812
> >
> > Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> > ---
> >  tools/mouse-dpi-tool.c | 43 +++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 39 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/mouse-dpi-tool.c b/tools/mouse-dpi-tool.c
> > index 30d689a..852ff07 100644
> > --- a/tools/mouse-dpi-tool.c
> > +++ b/tools/mouse-dpi-tool.c
> > @@ -44,6 +44,9 @@
> >  struct measurements {
> >         int distance;
> >         double max_frequency;
> > +       double *frequencies;
> > +       size_t frequencies_sz;
> > +       size_t nfrequencies;
> >         uint64_t us;
> >  };
> >
> > @@ -70,6 +73,21 @@ get_frequency(uint64_t last, uint64_t current)
> >         return 1000000.0/(current - last);
> >  }
> >
> > +static inline void
> > +push_frequency(struct measurements *m, double freq)
> > +{
> > +       if (m->nfrequencies == m->frequencies_sz) {
> > +               m->frequencies_sz += 100;
> > +               m->frequencies = realloc(m->frequencies,
> > +                                        m->frequencies_sz * sizeof *m->frequencies);
> 
> That's going to be a lot of realloc if the user goes slow :/

well, i can up it to 200 or so. note that this is just the mouse dpi tool,
"good enough" is very much good enough here ;)
 
> > +               if (!m->frequencies)
> > +                       abort();
> > +       }
> > +
> > +       m->frequencies[m->nfrequencies] = freq;
> > +       m->nfrequencies++;
> > +}
> > +
> >  static int
> >  print_current_values(const struct measurements *m)
> >  {
> > @@ -109,8 +127,8 @@ handle_event(struct measurements *m, const struct input_event *ev)
> >                         m->distance = 0;
> >                 } else {
> >                         double freq = get_frequency(last_us, m->us);
> > -                       if (freq < 1200)
> > -                               m->max_frequency = max(freq, m->max_frequency);
> > +                       push_frequency(m, freq);
> > +                       m->max_frequency = max(freq, m->max_frequency);
> >                         return print_current_values(m);
> >                 }
> >
> > @@ -166,12 +184,29 @@ mainloop(struct libevdev *dev, struct measurements *m) {
> >         return 0;
> >  }
> >
> > +static inline double
> > +mean_frequency(struct measurements *m)
> > +{
> > +       int idx;
> > +
> > +       idx = m->nfrequencies/2;
> > +       return m->frequencies[idx];
> > +}
> > +
> >  static void
> >  print_summary(struct measurements *m)
> >  {
> >         int res;
> > +       int max_freq = (int)m->max_frequency,
> > +           mean_freq = (int)mean_frequency(m);
> > +
> > +       printf("Estimated sampling frequency: %dHz (mean %dHz)\n",
> > +              max_freq, mean_freq);
> 
> Isn't the mean frequency the most interesting?

depends on what you want to do with it I guess. we're not using it anywhere,
so for now I'd like to stick to the max frequency actually used. mean
frequency may depend on speed of movement, etc.

> I am sure you can also directly discard some frequencies depending on
> the bus used: USB max should be around 1000Hz, maybe slightly more,
> Bluetooth not more than 500Hz I'd say (I'll need to check on the
> spec).
> So if you remove the obviously wrong ones (20000 is simply not
> possible, the mean frequency should be closer to the real one).

yes, but since this is a debugging tool only, I figured it's easier to just
print the warning and let the user catch the rest since I suspect that any
device that breaks the current bits needs manual intervention anyway. so I
don't want to spend too much time trying to catch the corner-cases here.

Cheers,
   Peter

> > +
> > +       if (max_freq > mean_freq * 1.3)
> > +               printf("WARNING: Max frequency is more than 30%% higher "
> > +                      "than mean frequency. Manual verification required!\n");
> >
> > -       printf("Estimated sampling frequency: %dHz\n", (int)m->max_frequency);
> >         printf("To calculate resolution, measure physical distance covered\n"
> >                "and look up the matching resolution in the table below\n");
> >
> > @@ -213,7 +248,7 @@ main (int argc, char **argv) {
> >         int fd;
> >         const char *path;
> >         struct libevdev *dev;
> > -       struct measurements measurements = {0, 0, 0};
> > +       struct measurements measurements = {0};
> >
> >         if (argc < 2)
> >                 return usage();
> > --
> > 2.7.4
> >
> > _______________________________________________
> > Input-tools mailing list
> > Input-tools at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/input-tools


More information about the Input-tools mailing list