Logitech high-resolution scrolling..

Logitech high-resolution scrolling..

[Linus Torvalds]

So I use a Logitech MX Anywhere 2S mouse, and really like it. I have
the scroll-wheel unlocked, because I like flicking once to scroll a
lot.

However, the new high-res scroll code means that the scroll wheel
action is now much too sensitive. It's not even stable - it will
scroll back-and-forth a bit occasionally, and in fact it sometimes
seems to scroll not because I touch the scroll-wheel, but because the
movement of the mouse itself causes the scroll wheel to move slightly
and wiggle the scroll action.

So the recent change to enable the high-res scrolling really seems a
bit *too* extreme.

Is there some middle ground that turns the mouse from "look at it
sideways and it starts scrolling" to something slightly more
reasonable?

                      Linus

Re: Logitech high-resolution scrolling..

[Linus Torvalds]

On Sun, Oct 28, 2018 at 12:13 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So the recent change to enable the high-res scrolling really seems a
> bit *too* extreme.
>
> Is there some middle ground that turns the mouse from "look at it
> sideways and it starts scrolling" to something slightly more
> reasonable?

Actually, I think the bug may be in the generic HID high-resolution
scrolling code, and I only notice because the Logitech support means
that now I see it.

In particular, if you look at hid_scroll_counter_handle_scroll(),
you'll notice that it tries to turn a high-res scroll event into a
regular wheel event by using the resolution_multiplier.

But that code looks really broken. It tries to react to a "half
multiplier" thing:

        int threshold = counter->resolution_multiplier / 2;
   ..
        counter->remainder += hi_res_value;
        if (abs(counter->remainder) >= threshold) {

and that's absolutely and entirely wrong.

Imagine that the high-res wheel counter has just moved a bit up (by
one high-res) tick, so now it's at the half-way mark to the
resolution_multiplier, and we scroll up by one:

                low_res_scroll_amount =
                        counter->remainder / counter->resolution_multiplier
                        + (hi_res_value > 0 ? 1 : -1);
                input_report_rel(counter->dev, REL_WHEEL,
                                 low_res_scroll_amount);

and then correct for it:

                counter->remainder -=
                        low_res_scroll_amount * counter->resolution_multiplier;

now we went from "half resolution multiplier positive" to "half negative".

Which means that next time that the high-res event happens by even
just one high-resolution tick in the other direction, we'll now
generate a low-resolution scroll event in the other direction.

In other words, that function results in unstable behavior. Tiny tiny
movements back-and-forth in the high-res wheel events (which could be
just because either the sensor is unstable, or the wheel is wiggling
imperceptibly) can result in visible movement in the low-res
("regular") wheel reporting.

There is no "damping" function, in other words. Noise in the high
resolution reading can result in noise in the regular wheel reporting.

So that threshold handling needs to be fixed, I feel. Either get rid
of it entirely (you need to scroll a *full* resolution_multiplier to
get a regular wheel event), or the counter->remainder needs to be
*cleared* when a wheel event has been sent so that you don't get into
the whole "back-and-forth" mode.

Or some other damping model. I suspect there are people who have
researched what the right answer is, but I guarantee that the current
code is not the right answer.

I suspect this also explains why I *sometimes* see that "just moving
the mouse sends wheel events", and at other times don't. It needs to
get close to that "half a resolution multiplier" stage to get into the
bad cases, but then tiny tiny perturbations can cause unstable
behavior.

I can't be the only person seeing this, but I guess the Logitech mouse
is right now the only one that uses the new generic HID code, and I
guess not a lot of people have been *using* it.

Harry?

                 Linus

Re: Logitech high-resolution scrolling..

[Jiri Kosina]

On Sun, 28 Oct 2018, Linus Torvalds wrote:

> So I use a Logitech MX Anywhere 2S mouse, and really like it. I have
> the scroll-wheel unlocked, because I like flicking once to scroll a
> lot.
> 
> However, the new high-res scroll code means that the scroll wheel
> action is now much too sensitive. It's not even stable - it will
> scroll back-and-forth a bit occasionally, and in fact it sometimes
> seems to scroll not because I touch the scroll-wheel, but because the
> movement of the mouse itself causes the scroll wheel to move slightly
> and wiggle the scroll action.
> 
> So the recent change to enable the high-res scrolling really seems a
> bit *too* extreme.
> 
> Is there some middle ground that turns the mouse from "look at it
> sideways and it starts scrolling" to something slightly more
> reasonable?

Benjamin indicated that Peter probably has found the issue in the code 
(failure to properly reset on direction change) that might be causing 
this. Adding to CC.

Peter?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: Logitech high-resolution scrolling..

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 941 bytes --]

On Mon, Oct 29, 2018 at 6:18 AM Jiri Kosina <jikos@kernel.org> wrote:
>
> Benjamin indicated that Peter probably has found the issue in the code
> (failure to properly reset on direction change) that might be causing
> this.

So honestly, once I looked at that hid_scroll_counter_handle_scroll()
function, that's the first thing I tried - get rid of the "half-way
threshold" thing, and reset on direction changes.

It fixes the instability, and I don't see the "back-and-forth"
movements and I don't get the "move the mouse and it generates mouse
wheel events" any more.

It basically makes the wheel _work_ for me.

I'm not entirely convinced it's as good as it used to be, though. It
still feels like it might be a bit over-sensitive, but that may be
because now I'm just looking for it..

Patch I'm using attached. I'm inclined to just commit it, but if
somebody has a better idea, I can test alternatives too.

                    Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 2208 bytes --]

 drivers/hid/hid-input.c | 43 +++++++++++++++++++++----------------------
 1 file changed, 21 insertions(+), 22 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 567c3bf64515..a2f74e6adc70 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1855,31 +1855,30 @@ EXPORT_SYMBOL_GPL(hidinput_disconnect);
 void hid_scroll_counter_handle_scroll(struct hid_scroll_counter *counter,
 				      int hi_res_value)
 {
-	int low_res_scroll_amount;
-	/* Some wheels will rest 7/8ths of a notch from the previous notch
-	 * after slow movement, so we want the threshold for low-res events to
-	 * be in the middle of the notches (e.g. after 4/8ths) as opposed to on
-	 * the notches themselves (8/8ths).
-	 */
-	int threshold = counter->resolution_multiplier / 2;
+	int low_res_value, remainder, multiplier;
 
 	input_report_rel(counter->dev, REL_WHEEL_HI_RES,
 			 hi_res_value * counter->microns_per_hi_res_unit);
 
-	counter->remainder += hi_res_value;
-	if (abs(counter->remainder) >= threshold) {
-		/* Add (or subtract) 1 because we want to trigger when the wheel
-		 * is half-way to the next notch (i.e. scroll 1 notch after a
-		 * 1/2 notch movement, 2 notches after a 1 1/2 notch movement,
-		 * etc.).
-		 */
-		low_res_scroll_amount =
-			counter->remainder / counter->resolution_multiplier
-			+ (hi_res_value > 0 ? 1 : -1);
-		input_report_rel(counter->dev, REL_WHEEL,
-				 low_res_scroll_amount);
-		counter->remainder -=
-			low_res_scroll_amount * counter->resolution_multiplier;
-	}
+	/*
+	 * Update the low-res remainder with the high-res value,
+	 * but reset if the direction has changed.
+	 */
+	remainder = counter->remainder;
+	if ((remainder ^ hi_res_value) < 0)
+		remainder = 0;
+	remainder += hi_res_value;
+
+	/*
+	 * Then just use the resolution multiplier to see if
+	 * we should send a low-res (aka regular wheel) event.
+	 */
+	multiplier = counter->resolution_multiplier;
+	low_res_value = remainder / multiplier;
+	remainder -= low_res_value * multiplier;
+	counter->remainder = remainder;
+
+	if (low_res_value)
+		input_report_rel(counter->dev, REL_WHEEL, low_res_value);
 }
 EXPORT_SYMBOL_GPL(hid_scroll_counter_handle_scroll);

Re: Logitech high-resolution scrolling..

[Linus Torvalds]

On Mon, Oct 29, 2018 at 8:16 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Patch I'm using attached. I'm inclined to just commit it, but if
> somebody has a better idea, I can test alternatives too.

I committed my patch, as it at least makes the scroll wheel usable.

I'm more than open to further improvements, but I wasn't willing to
live with the status quo, and carrying this around in my tree as a
patch kept confusing me while doing merges whenever a conflict
happened (because the "git diff" that I use to see the conflicts
obviously also showed my own local modifications).

                 Linus

Re: Logitech high-resolution scrolling..

[Harry Cutts]

As I explained in the comments, the reason for triggering at the half
multiplier point and then setting a negative remainder is to cater for
wheels that sometimes rest just before they complete a whole "notch"
in clicky mode. On those mice, setting the threshold at a full notch
means that you frequently have to move the wheel a little past its
resting point to trigger the low-res scroll event.

I don't really understand why the half-multiplier thing would cause
the instability; there's still a low-res threshold every 8 high-res
units whichever way you do it, it's just that with the half-multiplier
method it's 4 closer to the point where the wheel was when the device
was connected. Once you've scrolled around a bit in smooth mode that
should be irrelevant, unless you're somehow managing to move the wheel
in whole-notch increments without the help of the ratchet.

Maybe the long-term solution is to use more than just the value of the
current scroll event when deciding whether to send a low-res event. In
the meantime, I'll write a patch putting the threshold at 7/8ths of a
notch, to see if that can solve both issues.

Thanks,

Harry Cutts
Chrome OS Touch/Input team
Harry Cutts
Chrome OS Touch/Input team


On Mon, 29 Oct 2018 at 11:33, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Mon, Oct 29, 2018 at 8:16 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Patch I'm using attached. I'm inclined to just commit it, but if
> > somebody has a better idea, I can test alternatives too.
>
> I committed my patch, as it at least makes the scroll wheel usable.
>
> I'm more than open to further improvements, but I wasn't willing to
> live with the status quo, and carrying this around in my tree as a
> patch kept confusing me while doing merges whenever a conflict
> happened (because the "git diff" that I use to see the conflicts
> obviously also showed my own local modifications).
>
>                  Linus

Re: Logitech high-resolution scrolling..

[Linus Torvalds]

On Mon, Oct 29, 2018 at 12:17 PM Harry Cutts <hcutts@chromium.org> wrote:
>
> I don't really understand why the half-multiplier thing would cause
> the instability; there's still a low-res threshold every 8 high-res
> units whichever way you do it

No there isn't.

So what the half-multiplier did, assuming a multiplier of 8 (which is
what my MX Anywhere 2S reports) would be:

 - remainder starts at 3
 - high-res is +1
 - now remainder is 3+1, and it triggers the >= half logic
 - 4/8 is 0, but then the code added 1 because high-res was positive,
so the code decides to add 1
 - the code does a wheel update of 1, and updates remainder with -8,
so now it's -4

Next time around, if the high-res update is 0 or -1, it will go the
other direction. And then it will oscillate.

Notice how tiny movements of +1/-1 in the *high-res* count can
translate into +1/-1 in the regular wheel movement.

And those tiny movements very definitely happen. Maybe it's just my
mouse, but the undeniable fact is that the old algorithm was simply
not stable.

It was literally unusable. I had to be careful not to even touch the
wheel at all, or it would scroll randomly.

I do not believe that you actually ever *used* that code, or if you
did, you only did so with applications that were high-res aware and
ignored the regular wheel entirely because you were testing in an
environment with other changes than just the kernel.

                    Linus

Re: Logitech high-resolution scrolling..

[Harry Cutts]

On Mon, 29 Oct 2018 at 14:12, Linus Torvalds <torvalds@linux-foundation.org>
> So what the half-multiplier did, assuming a multiplier of 8 (which is
> what my MX Anywhere 2S reports) would be:
>
>  - remainder starts at 3
>  - high-res is +1
>  - now remainder is 3+1, and it triggers the >= half logic
>  - 4/8 is 0, but then the code added 1 because high-res was positive,
> so the code decides to add 1
>  - the code does a wheel update of 1, and updates remainder with -8,
> so now it's -4
>
> Next time around, if the high-res update is 0 or -1, it will go the
> other direction. And then it will oscillate.
>
> Notice how tiny movements of +1/-1 in the *high-res* count can
> translate into +1/-1 in the regular wheel movement.

Ah, I see what you mean. So, if we move the threshold to (multiplier -
1)/multiplier (7/8) in this case, I think the equivalent scenario
would be:

- remainder starts at 7
- high-res is +1
- remainder is now 7+1, triggering a low-res update
- 7/8 is 0, but we add one to the remainder in the check making it (7+1)/8 == 1
- we update remainder to -1

This way we're still at least 7/8ths of a notch from the threshold in
either direction, so we shouldn't get the oscillation problem. Does
that sound reasonable, or do you think I've missed something?

> I do not believe that you actually ever *used* that code, or if you
> did, you only did so with applications that were high-res aware and
> ignored the regular wheel entirely because you were testing in an
> environment with other changes than just the kernel.

I tested these changes with 5 different Logitech mice (see the
Logitech high-res support patch [0] for details), and did so mainly
with applications that were *not* high-res aware, using a mix of
clicky and smooth modes. Admittedly the MX Anywhere 2S was not one of
my test devices; I had assumed that its behaviour would be
sufficiently similar to that of the MX Anywhere 2 and the MX Master
2S.

Harry Cutts
Chrome OS Touch/Input team

[0]: https://patchwork.kernel.org/patch/10582935/

Re: Logitech high-resolution scrolling..

[Linus Torvalds]

On Mon, Oct 29, 2018 at 2:42 PM Harry Cutts <hcutts@chromium.org> wrote:
>
> Ah, I see what you mean. So, if we move the threshold to (multiplier -
> 1)/multiplier (7/8) in this case, I think the equivalent scenario
> would be:

That would work, yes.

Except I think you *do* want the "reset on direction change" logic,
because otherwise we still end up having the:

> - we update remainder to -1

where it now gets easier to next time go the wrong way, for no good
reason.  So now you only need another 6/8ths the other way to get to
within 7/8ths of -8 and scroll back.

In other words, the whole "round partial scrolling" also causes that
whole "now the other direction is closer" issue.

At 7/8's it is less obviously a problem than it was at 1/2, but I
still think it's a sign of an unstable algorithm, where changes get
triggered too easily in the non-highres world.

Also, honestly, I'm not sure I see the point. *IF* you actually scroll
more in one direction, it doesn't matter one whit whether you pick
1/2, 7/8, or whole multipliers: the *next* step is still always going
to be one whole multiplier away.

So I think the whole rounding is actually misguided. I think it may
come from the very fact that you did *not* reset the remainder on
direction changes, so you could scroll in one direction to -3, and
then you change direction and go a "whole" tick the other way, but now
it's just at +5, so you think you need to round up.

With the whole "reset when changing direction", I don't think the
rounding is necessary, and I don't think it makes sense.

But I'm willing to test patches. I would suggest looking at the "oops,
direction changed" issue, though, because it really was very annoying.

> I tested these changes with 5 different Logitech mice (see the
> Logitech high-res support patch [0] for details), and did so mainly
> with applications that were *not* high-res aware, using a mix of
> clicky and smooth modes. Admittedly the MX Anywhere 2S was not one of
> my test devices; I had assumed that its behaviour would be
> sufficiently similar to that of the MX Anywhere 2 and the MX Master
> 2S.

I happen to have a MX Master 2S too, but I don't use it because I find
I like the smaller and lightweight "anywhere" mice.

I didn't try the broken case with it, but one thing I notice with the
Master 2S is that it seems to have a "heftier" feel to its wheel. It
may simply have more mass and not be as flighty, and thus show the
issue less.

But that's just a theory. It could just be something that is
individual to some mice.

                       Linus

Re: Logitech high-resolution scrolling..

[Harry Cutts]

On Mon, 29 Oct 2018 at 15:01, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> That would work, yes.

OK, I'll write a patch for this. (It may be next week, though, as I
have a deadline on a separate project this week.)

> Except I think you *do* want the "reset on direction change" logic,
> because otherwise we still end up having the:
>
> > - we update remainder to -1
>
> where it now gets easier to next time go the wrong way, for no good
> reason.  So now you only need another 6/8ths the other way to get to
> within 7/8ths of -8 and scroll back.
>
> In other words, the whole "round partial scrolling" also causes that
> whole "now the other direction is closer" issue.
>
> At 7/8's it is less obviously a problem than it was at 1/2, but I
> still think it's a sign of an unstable algorithm, where changes get
> triggered too easily in the non-highres world.
>
> Also, honestly, I'm not sure I see the point. *IF* you actually scroll
> more in one direction, it doesn't matter one whit whether you pick
> 1/2, 7/8, or whole multipliers: the *next* step is still always going
> to be one whole multiplier away.
>
> So I think the whole rounding is actually misguided. I think it may
> come from the very fact that you did *not* reset the remainder on
> direction changes, so you could scroll in one direction to -3, and
> then you change direction and go a "whole" tick the other way, but now
> it's just at +5, so you think you need to round up.
>
> With the whole "reset when changing direction", I don't think the
> rounding is necessary, and I don't think it makes sense.

Resetting on direction change would certainly make complete sense in
smooth mode. The reason that I'm reluctant to do it is for clicky
mode, where we think it's important that the low-res event happen at a
consistent point in the movement between notches (the resting
positions of the wheel). For example, imagine the following scenario
with a wheel multiplier of 8 and the threshold initially at 7/8ths of
a notch:

- I scroll one notch down. The low-res event occurs just before the
wheel settles in to its notch, leaving a -1/8th remainder, and then
(on most wheels) the ratchet mechanism settles the wheel 1/8th further
into its resting position, eliminating the remainder.
- I move the wheel 3/8ths further down, then change my mind and start
scrolling upwards.

If we reset on direction change at this point, then the "zero point"
will have moved, so that we trigger the low-res movement at -4/8ths
(at the peak of resistance between the two notches) instead of at
7/8ths. If we don't reset but allow the 3/8ths remainder to be
cleared, the trigger point stays at 7/8ths. It's a minor thing, to be
sure, but we think that keeping the on-screen response consistent with
the tactile feel of the wheel is important for the user experience.

Harry Cutts
Chrome OS Touch/Input team

Re: Logitech high-resolution scrolling..

[Peter Hutterer]

On Mon, Oct 29, 2018 at 04:03:54PM -0700, Harry Cutts wrote:
> On Mon, 29 Oct 2018 at 15:01, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > That would work, yes.
> 
> OK, I'll write a patch for this. (It may be next week, though, as I
> have a deadline on a separate project this week.)
> 
> > Except I think you *do* want the "reset on direction change" logic,
> > because otherwise we still end up having the:
> >
> > > - we update remainder to -1
> >
> > where it now gets easier to next time go the wrong way, for no good
> > reason.  So now you only need another 6/8ths the other way to get to
> > within 7/8ths of -8 and scroll back.
> >
> > In other words, the whole "round partial scrolling" also causes that
> > whole "now the other direction is closer" issue.
> >
> > At 7/8's it is less obviously a problem than it was at 1/2, but I
> > still think it's a sign of an unstable algorithm, where changes get
> > triggered too easily in the non-highres world.
> >
> > Also, honestly, I'm not sure I see the point. *IF* you actually scroll
> > more in one direction, it doesn't matter one whit whether you pick
> > 1/2, 7/8, or whole multipliers: the *next* step is still always going
> > to be one whole multiplier away.
> >
> > So I think the whole rounding is actually misguided. I think it may
> > come from the very fact that you did *not* reset the remainder on
> > direction changes, so you could scroll in one direction to -3, and
> > then you change direction and go a "whole" tick the other way, but now
> > it's just at +5, so you think you need to round up.
> >
> > With the whole "reset when changing direction", I don't think the
> > rounding is necessary, and I don't think it makes sense.
> 
> Resetting on direction change would certainly make complete sense in
> smooth mode. The reason that I'm reluctant to do it is for clicky
> mode, where we think it's important that the low-res event happen at a
> consistent point in the movement between notches (the resting
> positions of the wheel). For example, imagine the following scenario
> with a wheel multiplier of 8 and the threshold initially at 7/8ths of
> a notch:
> 
> - I scroll one notch down. The low-res event occurs just before the
> wheel settles in to its notch, leaving a -1/8th remainder, and then
> (on most wheels) the ratchet mechanism settles the wheel 1/8th further
> into its resting position, eliminating the remainder.
> - I move the wheel 3/8ths further down, then change my mind and start
> scrolling upwards.
> 
> If we reset on direction change at this point, then the "zero point"
> will have moved, so that we trigger the low-res movement at -4/8ths
> (at the peak of resistance between the two notches) instead of at
> 7/8ths. If we don't reset but allow the 3/8ths remainder to be
> cleared, the trigger point stays at 7/8ths. It's a minor thing, to be
> sure, but we think that keeping the on-screen response consistent with
> the tactile feel of the wheel is important for the user experience.

IMO this is a lost battle because you cannot know when the ratchet is
enabled or not (at least not on all mice). Users switch between ratchet and
freewheeling time and once you're out of one mode, you have no reference 
to the other mode's reset point anymore.

you could guess it with heuristics. if you get multiple scroll sequences
with $multiplier events, then you're probably back in ratchet mode. Of
course, it's just guesswork...

fwiw, here's a writeup of the issues that I found in the current code,
before Linus' patch. This is as much my note-taking as it is an email.

Let's assume free-wheeling for now, and I'm using high-res values of
2 to reduce typing. multiplier is 8 like the default in the code.

- the first event comes earlier than the second on a consistent scroll
  motion, you get one event after a half movement, the second, third, ...
  events after n + half. Not a huge issue since it only ever happens once
  after plug.  And this is by design as you said, so let's live with
  that :)
- The scroll wheel emulation is unpredictable across scroll events. Let's
  assume multiple sequences of events, with a pause long enough to make the
  user think they are independent scroll motions:
    [2, 2, 2, 2] [2, 2, 2, 2] ← input events
        x            x
    [2, 2] [2, 2, 2, 2, 2, 2] ← input events
        x            x
    [2, 2, 2, 2, 2] [2, 2, 2] ← input events
        x            x
  x marks the spot where the low-res event is sent.
  in the first case, everything is fine, second case has the first sequence
  react quickly, the second one slower. third case is the opposite. The only
  reason this isn't very obvious is because the scroll distance is very
  small either way. we'd need a timeout to avoid this issue, a basic "reset
  remainder after N ms".
- the directional change is what Linus triggered
              [2, 2, -2, 2, -2 ...] ← input events
  remainders:  0  4              r - 8
                 -4  -6          r + 8
                      2  4       r - 8
                        -4  -6
                  x   x  x   x
  if you get in the right state you get to trigger the events on every small
  motion. Note that the issue here isn't the half-way trigger, but the
  missing reset. we could have the half-way trigger in place:
  [2, 2, -2, 2, -2 ...] ← input events
   0  4
     -4   0 ← reset 
         -2  0 ← reset 
             2   0 ← reset 
      x          
  so that would work just fine, that's also what the patch below does.
- fast motion can trigger some odd calculations because of the +1 added.
    [2, 8, -2, 2 ...] ← input events
  r: 0 10               r - 16 
       -6  -8           r + 16 (incorrect)
            8 10        r - 16 (incorrect)
              -6  
       xx  xx xx
  All of these would trigger a double scroll event. Admittedly this is
  physically hard to trigger so it's more a theoretical concern. What's
  easier to trigger is:
    [2, 6, 2, 2 ...] ← input events
     0  8               r - 16
       -8 -6            r + 8 (incorrect)
           2 4          r - 8
            -4  
       xx  x x
  The xx and y events are incorrect here, total movement was 10 units but we
  got 3 units instead of the expected 1. Result is that on fast scrolling we
  may end up with the occasional extra event. 

  The fix for this is in the patch below, by only adding the extra ±1 if
  we're below the multiplier, we get this instead:
    [2, 6, 2, 2 ...] ← input events
     0  8               r - 8
        0  2  4         r + 8
             -4         r - 8
       xx     x

  or on a similar sequence:
    [2, 8, 6, 2 ...] ← input events
     0  8               r - 8
        0  6  4         r + 8
          -2 -4         r - 8
       xx  x  x


Other issues I found with an MX Anywhere 2S is that on slow scroll and in
ratchet mode we get some scroll jitter.  In ratchet mode  we can get this
sequence if you scroll just past the notch and it snaps back:
    [1, 1, 1, 1, 1, 1, 1, 1, -1]
That's quite easy to trigger. In free-wheel mode we may get the same for
slow motion due to human finger jitter (the Anywhere 2S didn't have HW
jitter, but other devices may). So a perceived-consistent scroll motion may
really look like this:
    [1, 1, 1, 1, 1, -1, 1, 1]
Hard to triggger but when it does, it feels like we're dropping events.
The former isn't that much of an issue as long as the ratchet is enabled so
you get the haptic feedback and we (usually) don't drop events.

Finally: I reproduced the issue you mentioned with the 7/8th of a
notch. A slow ratchet scroll does not always give me the same number of
events per notch, there are cases where I get 7 or 9 events instead of the
expected 8 (all with a hi-res value 1). With Linus patch I ended up getting
weirdly ouf of sync here as to when the low res event was triggered during
the motion.

So my suggestion is to combine Linus' reset with your approach and use the
center-point for the trigger. This gives us a few events to slide and still
do the right thing, and any direction change will reset anyway. Biggest
drawback is that the first event after a direction change is triggered
faster than the next event. Otherwise it feels correct to me, both in
free-wheeling and in ratchet mode now.

The timeout I mentioned above is probably something better kept in userspace
if needed.

Cheers,
   Peter

Also, WTF moment: I managed to get the mouse into a state where it would
only give me 1 hi-res event per notch movement but failed to reproduce that
again.


From a9cb724159cc9515ce9ee1aff15184a19731d80b Mon Sep 17 00:00:00 2001
From: Peter Hutterer <peter.hutterer@who-t.net>
Date: Tue, 30 Oct 2018 14:10:53 +1000
Subject: [PATCH] HID: input: restore the hi-res scroll emulation to work on
 the midpoint

A single notch movement does not always cause exactly $multiplier events in
the hardware. Setting the trigger at the midpoint allows us to slide a few
events either direction while still triggering exactly one scroll event per
multiplier.

Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
---
 drivers/hid/hid-input.c | 25 ++++++++++++++++++-------
 include/linux/hid.h     |  1 +
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index a2f74e6adc70..a170a6823072 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1855,7 +1855,7 @@ EXPORT_SYMBOL_GPL(hidinput_disconnect);
 void hid_scroll_counter_handle_scroll(struct hid_scroll_counter *counter,
 				      int hi_res_value)
 {
-	int low_res_value, remainder, multiplier;
+	int low_res_value, remainder, multiplier, direction;
 
 	input_report_rel(counter->dev, REL_WHEEL_HI_RES,
 			 hi_res_value * counter->microns_per_hi_res_unit);
@@ -1865,20 +1865,31 @@ void hid_scroll_counter_handle_scroll(struct hid_scroll_counter *counter,
 	 * but reset if the direction has changed.
 	 */
 	remainder = counter->remainder;
-	if ((remainder ^ hi_res_value) < 0)
+	direction = hi_res_value > 0 ? 1 : -1;
+	if (direction != counter->direction)
 		remainder = 0;
+	counter->direction = direction;
 	remainder += hi_res_value;
 
 	/*
 	 * Then just use the resolution multiplier to see if
 	 * we should send a low-res (aka regular wheel) event.
+	 * Threshold is at the mid-point because we'll slide a few events
+	 * back/forth when the mouse gives us more or less than multiplier
+	 * events for a single notch movement.
 	 */
 	multiplier = counter->resolution_multiplier;
-	low_res_value = remainder / multiplier;
-	remainder -= low_res_value * multiplier;
-	counter->remainder = remainder;
-
-	if (low_res_value)
+	if (abs(remainder) >= multiplier/2) {
+		low_res_value = remainder / multiplier;
+                /* Move at minimum 1/-1 because we want to trigger when the wheel
+                 * is half-way to the next notch (i.e. scroll 1 notch after a
+                 * 1/2 notch movement).
+                 */
+		if (low_res_value == 0)
+			low_res_value = (hi_res_value > 0 ? 1 : -1);
+		remainder -= low_res_value * multiplier;
 		input_report_rel(counter->dev, REL_WHEEL, low_res_value);
+	}
+	counter->remainder = remainder;
 }
 EXPORT_SYMBOL_GPL(hid_scroll_counter_handle_scroll);
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 2827b87590d8..9752b8a2ee42 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -1162,6 +1162,7 @@ struct hid_scroll_counter {
 	int resolution_multiplier;
 
 	int remainder;
+	int direction;
 };
 
 void hid_scroll_counter_handle_scroll(struct hid_scroll_counter *counter,
-- 
2.19.1

Re: Logitech high-resolution scrolling..

[Mauro Carvalho Chehab]

Em Sun, 28 Oct 2018 14:08:31 -0700
Linus Torvalds <torvalds@linux-foundation.org> escreveu:

> On Sun, Oct 28, 2018 at 12:13 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > So the recent change to enable the high-res scrolling really seems a
> > bit *too* extreme.
> >
> > Is there some middle ground that turns the mouse from "look at it
> > sideways and it starts scrolling" to something slightly more
> > reasonable?  
> 
> Actually, I think the bug may be in the generic HID high-resolution
> scrolling code, and I only notice because the Logitech support means
> that now I see it.
> 
> In particular, if you look at hid_scroll_counter_handle_scroll(),
> you'll notice that it tries to turn a high-res scroll event into a
> regular wheel event by using the resolution_multiplier.
> 
> But that code looks really broken. It tries to react to a "half
> multiplier" thing:
> 
>         int threshold = counter->resolution_multiplier / 2;
>    ..
>         counter->remainder += hi_res_value;
>         if (abs(counter->remainder) >= threshold) {
> 
> and that's absolutely and entirely wrong.
> 
> Imagine that the high-res wheel counter has just moved a bit up (by
> one high-res) tick, so now it's at the half-way mark to the
> resolution_multiplier, and we scroll up by one:
> 
>                 low_res_scroll_amount =
>                         counter->remainder / counter->resolution_multiplier
>                         + (hi_res_value > 0 ? 1 : -1);
>                 input_report_rel(counter->dev, REL_WHEEL,
>                                  low_res_scroll_amount);
> 
> and then correct for it:
> 
>                 counter->remainder -=
>                         low_res_scroll_amount * counter->resolution_multiplier;
> 
> now we went from "half resolution multiplier positive" to "half negative".
> 
> Which means that next time that the high-res event happens by even
> just one high-resolution tick in the other direction, we'll now
> generate a low-resolution scroll event in the other direction.
> 
> In other words, that function results in unstable behavior. Tiny tiny
> movements back-and-forth in the high-res wheel events (which could be
> just because either the sensor is unstable, or the wheel is wiggling
> imperceptibly) can result in visible movement in the low-res
> ("regular") wheel reporting.
> 
> There is no "damping" function, in other words. Noise in the high
> resolution reading can result in noise in the regular wheel reporting.
> 
> So that threshold handling needs to be fixed, I feel. Either get rid
> of it entirely (you need to scroll a *full* resolution_multiplier to
> get a regular wheel event), or the counter->remainder needs to be
> *cleared* when a wheel event has been sent so that you don't get into
> the whole "back-and-forth" mode.
> 
> Or some other damping model. I suspect there are people who have
> researched what the right answer is, but I guarantee that the current
> code is not the right answer.
> 
> I suspect this also explains why I *sometimes* see that "just moving
> the mouse sends wheel events", and at other times don't. It needs to
> get close to that "half a resolution multiplier" stage to get into the
> bad cases, but then tiny tiny perturbations can cause unstable
> behavior.
> 
> I can't be the only person seeing this, but I guess the Logitech mouse
> is right now the only one that uses the new generic HID code, and I
> guess not a lot of people have been *using* it.

I remember I submitted in the past some patches adding a different event
for the high scroll mode. I have myself a MX Anywhere 2, and even wrote
some patches for Solaar[1] in order to allow selecting between low
res and high res wheel modes.

The problem I faced, on that time, was similar to yours: when the
high res wheel was enabled, it was very hard to control the mouse,
specially when using the wheel in "free" mode (with I do).

I remember that the patchset I sent was not actually applied, but I
didn't followed what happened after that (got sidetracked by
something else).

[1] https://github.com/pwr/Solaar

Thanks,
Mauro

Re: Logitech high-resolution scrolling..

[Linus Torvalds]

On Mon, Oct 29, 2018 at 11:27 PM Peter Hutterer
<peter.hutterer@who-t.net> wrote:
>
> Other issues I found with an MX Anywhere 2S is that on slow scroll and in
> ratchet mode we get some scroll jitter.  In ratchet mode  we can get this
> sequence if you scroll just past the notch and it snaps back:
>     [1, 1, 1, 1, 1, 1, 1, 1, -1]
> That's quite easy to trigger. In free-wheel mode we may get the same for
> slow motion due to human finger jitter (the Anywhere 2S didn't have HW
> jitter, but other devices may). So a perceived-consistent scroll motion may
> really look like this:
>     [1, 1, 1, 1, 1, -1, 1, 1]
> Hard to triggger but when it does, it feels like we're dropping events.
> The former isn't that much of an issue as long as the ratchet is enabled so
> you get the haptic feedback and we (usually) don't drop events.

Both of these actually argue that doing the reset on direction change
can be a real problem.

But equally clearly, _not_ doing the reset is unacceptable too.

I wonder if there's some docs on what Logitech does internally in the
mouse. It might involve a timeout (ie "if not moving for a while, do
the rounding _and_ reset), which would probably be too expensive to do
on the host side.

                    Linus

Re: Logitech high-resolution scrolling..

[Harry Cutts]

Thanks for the analysis, Peter.

On Mon, 29 Oct 2018 at 23:27, Peter Hutterer <peter.hutterer@who-t.net> wrote:
> IMO this is a lost battle because you cannot know when the ratchet is
> enabled or not (at least not on all mice). Users switch between ratchet and
> freewheeling time and once you're out of one mode, you have no reference
> to the other mode's reset point anymore.

It would be a lost battle, if it weren't for the fact that on all the
mice I've tested, putting the wheel back into clicky mode causes the
wheel to jump to the nearest notch resting point, which should mean
that the remainder resets to 0 (or maybe ±1 if the mechanism is worn).

> So my suggestion is to combine Linus' reset with your approach and use the
> center-point for the trigger. This gives us a few events to slide and still
> do the right thing, and any direction change will reset anyway. Biggest
> drawback is that the first event after a direction change is triggered
> faster than the next event. Otherwise it feels correct to me, both in
> free-wheeling and in ratchet mode now.

This sounds like a reasonable approach if we find that we can't keep
the triggering point consistent.

> Also, WTF moment: I managed to get the mouse into a state where it would
> only give me 1 hi-res event per notch movement but failed to reproduce that
> again.

Interesting; let me know if you manage to reliably reproduce it. The
only time I've encountered this in the past was when connecting to the
mouse over BLE, where we don't seem to be able to detect if the mouse
is power cycled (meaning that the mouse resets to low-res mode but the
kernel is still expecting high-res reports). I held off on enabling
high-res scrolling over Bluetooth for this reason.

On Tue, 30 Oct 2018 at 09:29, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> I wonder if there's some docs on what Logitech does internally in the
> mouse. It might involve a timeout (ie "if not moving for a while, do
> the rounding _and_ reset), which would probably be too expensive to do
> on the host side.

I've been wondering this as well. Nestor (CCed), is there anything you
can tell us about this?

Harry Cutts
Chrome OS Touch/Input team

Re: Logitech high-resolution scrolling..

[Nestor Lopez Casado]

Hi guys,

I've read the discussion, I think I understand the problem and I'll
get back to this thread with more information as soon as I've got some
internal feedback.

BTW, lovely to see so many MX Anywhere  2 users :)
-nestor

On Tue, Oct 30, 2018 at 6:48 PM Harry Cutts <hcutts@chromium.org> wrote:
>
> Thanks for the analysis, Peter.
>
> On Mon, 29 Oct 2018 at 23:27, Peter Hutterer <peter.hutterer@who-t.net> wrote:
> > IMO this is a lost battle because you cannot know when the ratchet is
> > enabled or not (at least not on all mice). Users switch between ratchet and
> > freewheeling time and once you're out of one mode, you have no reference
> > to the other mode's reset point anymore.
>
> It would be a lost battle, if it weren't for the fact that on all the
> mice I've tested, putting the wheel back into clicky mode causes the
> wheel to jump to the nearest notch resting point, which should mean
> that the remainder resets to 0 (or maybe ±1 if the mechanism is worn).
>
> > So my suggestion is to combine Linus' reset with your approach and use the
> > center-point for the trigger. This gives us a few events to slide and still
> > do the right thing, and any direction change will reset anyway. Biggest
> > drawback is that the first event after a direction change is triggered
> > faster than the next event. Otherwise it feels correct to me, both in
> > free-wheeling and in ratchet mode now.
>
> This sounds like a reasonable approach if we find that we can't keep
> the triggering point consistent.
>
> > Also, WTF moment: I managed to get the mouse into a state where it would
> > only give me 1 hi-res event per notch movement but failed to reproduce that
> > again.
>
> Interesting; let me know if you manage to reliably reproduce it. The
> only time I've encountered this in the past was when connecting to the
> mouse over BLE, where we don't seem to be able to detect if the mouse
> is power cycled (meaning that the mouse resets to low-res mode but the
> kernel is still expecting high-res reports). I held off on enabling
> high-res scrolling over Bluetooth for this reason.
>
> On Tue, 30 Oct 2018 at 09:29, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > I wonder if there's some docs on what Logitech does internally in the
> > mouse. It might involve a timeout (ie "if not moving for a while, do
> > the rounding _and_ reset), which would probably be too expensive to do
> > on the host side.
>
> I've been wondering this as well. Nestor (CCed), is there anything you
> can tell us about this?
>
> Harry Cutts
> Chrome OS Touch/Input team