[PATCH libevdev] tools: print the mean frequency together with the max frequency
Benjamin Tissoires
benjamin.tissoires at gmail.com
Mon Sep 19 14:06:20 UTC 2016
On Mon, Sep 19, 2016 at 3:52 PM, Peter Hutterer
<peter.hutterer at who-t.net> wrote:
> 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.
Yes, I was talking about a kernel tweak, not in user space.
>
>> > 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.
To me, "first reports", were the firsts in the recording, when we
actually start counting.
If it's triggered also in the middle (if the user stops for a few
secs), then yes, we can't detect those.
>
>> > 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 ;)
k, good enough then
>
>> > + 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.
K.
Acked-By: Benjamin Tissoires <benjamin.tissoires at gmail.com>
(I think I reviewed it properly today, by lack of brain currently
gives me a simple acked-by :-P )
Cheers,
Benjamin
>
> 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