Bug 4147

Reassign to the new evdev maintainer.

I'm planning on doing this sometime, but there are several other things that I'm
probably going to get to first.

But I should get to it eventually.

Remind me about it if I don't get to it in the next few weeks.

(In reply to comment #2)
> I'm planning on doing this sometime, but there are several other things that I'm
> probably going to get to first.
> 
> But I should get to it eventually.
> 
> Remind me about it if I don't get to it in the next few weeks.

A few weeks have come and gone, EmulateWheel still isn't possible with evdev.

Created attachment 6125 [details] [review]
First alpha version of my wheel emulation implementation for the evdev driver

So I went away and hacked the evdev to support EmulateWheel.. This is really a
alpha version, there are some known button bugs (they sometimes get stuck, but
just press them again to fix the problem).. but generally it works ok so I
thought I'd post it here since I'm not sure whether I'll fix the bugs soon..

Sorry it's not against the newest version, I just took and worked on what was
marked stable on my distro i.e. version 1.0.0.5. But it works fine for me, so I
don't see a desperate need to use any newer version at the moment :)

I'll post any further versions here as soon as I have them.

Hope it works for you too :)

- xkr47

Oh, and that evdev driver is what I got when I installed xorg x11 7.0. I have 
no idea about whether it would work for 7.1 or 6.x..

The config I use for my Logitech TrackMan Marble FX mouse:

Section "InputDevice"
        Identifier  "Mouse0"
        Driver      "evdev"
        Option      "Device" "/dev/input/event1"
        Option      "EmulateWheel"  "1"
        Option      "EmulateWheelButton"  "8"
EndSection

(In reply to comment #6)
> The config I use for my Logitech TrackMan Marble FX mouse:
> 
> Section "InputDevice"
>         Identifier  "Mouse0"
>         Driver      "evdev"
>         Option      "Device" "/dev/input/event1"
>         Option      "EmulateWheel"  "1"
>         Option      "EmulateWheelButton"  "8"
> EndSection
> 

I'm afraid that 1.0.0.5 is so close to being a completely different code base
that, as much as I'd like it to not be so, patches against it are not useful for
the current code base.

The best general bet when working on patches is to get the most current git tree
and do the work on that, because 1.0.0.5 is not even remotely a code base that I
can rebase from.

Sorry.
Zephaniah E. Hull.

I see that still doesn't work. The worse thing is - I was using the EmulateWheel
not for my external USB mouse that's using evdev (for hotplug support only), but
for the internal TrackPoint which is using the normal mouse driver, but still
doesn't work (because of the evdev usb mouse - if I comment it out in xorg.conf,
EmulateWheel for the TrackPoint works fine). If not add the EmulateWheel
support, is it at least possible to circumvent this issue I'm having?

Sorry about the phenomenal bug spam, guys.  Adding xorg-team@ to the QA contact so bugs don't get lost in future.

Zephaniah, are you (or smb else) gonna fix this anytime soon? Pleeeeease )

The mystery 'xf86-input-mouse breaks in odd ways when xf86-input-evdev is loaded' bug is a complete mystery, it shouldn't be possible and I probably can't fix it.

Suggestions on how to reproduce it easily are welcome, but it's a basic 'this can't happen', so.

EmulateWheel support, is going to be a pain, and it's not going to happen immediately.

It's on my todo list for the next time I do major work on the xf86-input-evdev code base, but that's probably the 'xserver handles hotplug' rewrite, dunno if it will come back to a branch for prior to that xserver.

RedHat bug 446627
https://bugzilla.redhat.com/show_bug.cgi?id=446627

FWIW, on my ThinkPad I keep input hotplugging enabled and just disable it for the trackpoint. Then I can keep the trackpoint configured in xorg.conf and any external mice be hotpluggable. Below is the fdi I added to do this. I've considered just merging it into the x11-input.fdi on my system since the trackpoint is pretty useless without the mouse driver.

$ cat /etc/hal/fdi/policy/no-trackpoint-evdev.fdi 
<?xml version="1.0" encoding="UTF-8"?>
<deviceinfo version="0.2">
  <device>
    <match key="input.product" string="TPPS/2 IBM TrackPoint">
      <remove key="input.x11_driver"/>
    </match>
  </device>
</deviceinfo>

Well, this one annoyed me enough that I decided to take a shot at it.  I've worked out a patch that supports everything (* I think *) except EmulateWheelTimeout.  That particular feature is not one that I use.  A good chunk of this is a copy/rewrite of the mouse wheel emulation code form xf86-input-mouse.  This patch should be good against the most recent git ( commit 53e7525744cd7c47707c7339f0b771f59f99b97c ).  It seems to work nicely for me.

Created attachment 17940 [details] [review]
Mouse Wheel Emulation Patch

Chris:

Thanks for doing this work!  A few comments to your patch:
- please attach the bug as a git formatted patch, so it's easier to just apply
  it. git-format-patch HEAD~1 gets you the last commit as patch.
  git-commit --amend allows you to change the last commit, so you can apply
  changes and resubmit it as another formatted patch instead of a series of
  patches.
- watch out for indentation. there's a number of lines with tabs where there
  should be spaces.
- watch out for needless changes, hunks 3, 4 and 6 in evdev.c are just noise.
- please separate the wheel emulation a bit more

for example, instead of
---
if ((ev.code-BTN_LEFT+5)== pEvdev->emulateWheel.button) {
    pEvdev->emulateWheel.button_state=value;
} else {
  ... lots of indentation changes
---
you could do something like

---
if (EvdevWheelEmuFilterEvent(...))
   break;

... no indentation changes necessary.
--

inside EvdevWheelEmuFilterEvent you can check for REL_X, BTN_XYZ, etc.,
reducing the noise in the main evdev.c file and keeping the driver core easy
to read.

(In reply to comment #16)
> Chris:
> 
> Thanks for doing this work!  A few comments to your patch:
> - please attach the bug as a git formatted patch, so it's easier to just apply
>   it. git-format-patch HEAD~1 gets you the last commit as patch.
>   git-commit --amend allows you to change the last commit, so you can apply
>   changes and resubmit it as another formatted patch instead of a series of
>   patches.
> - watch out for indentation. there's a number of lines with tabs where there
>   should be spaces.
> - watch out for needless changes, hunks 3, 4 and 6 in evdev.c are just noise.
> - please separate the wheel emulation a bit more
> 


Ok, I went through and cleaned this up a good deal.  There should be significantly less 'noise' in the main driver.  I've added calls to EvdevWheelEmuFilterEvent in two places and added the necessary configuration code.

Limitations:
- This code will not work with a mouse that uses absolute positioning, I don't have a device handy to test that with.  If I have this right, it should ignore absolute motion events.
- EmulateWheelTimeout is not supported
- I have not done any of the ABI_XINPUT < 3 protection mentioned in commit 5d13259a5ddab31dbb2158975c8ccbb1f3c99046.
- probably others ;)

Let me know how that looks.

Created attachment 17983 [details] [review]
Mouse Wheel Emulation Patch take 2

Hopefully this is cleaner than the last one.

On Tue, Jul 29, 2008 at 10:52:25PM -0700, bugzilla-daemon@freedesktop.org wrote:
> Ok, I went through and cleaned this up a good deal.  There should be
> significantly less 'noise' in the main driver.  I've added calls to
> EvdevWheelEmuFilterEvent in two places and added the necessary configuration
> code.

thanks, looking much better. Again, a few comments:
- indentation and coding style. Indentation is still messed up. (e.g.
  EvdevWheelEmuInertia). Also, I'm not a big fan of the "no spaces" coding
  style. Please use the form "a = b" instead of "a=b", and try to be
  consistent (e.g. HandleButtonMap uses "if( condition)" instead of "if
  (condition)"
- Parenthesis around if conditionsa "if ((a > b) && (c < d)" make it easier to
  read (noticed that in HandleButtonMap
- EvdevWheelHandleButtonMapping - I'd much rather prefer to see that as a
  separate patch to evdev.c. This is code we need anyway, and it shouldn't be
  tied to the wheel emulation.
- MSE_MAXBUTTONS is undefined. It used to be part of the server, but is now
  mouse-driver only. A separate patch to #define it to 32 is welcomed :)
- any reason why we couldn't consolidate EmulateWheel and EmulateWheelButton
  into one setting? such as EmulateWheel switches it on and at the same time
  defines the buttonswitches it on and at the same time.
- why do we need "configured". If Wheel is enabled, then it should be
  automatically configured to sane defaults (or to whatever the user
  specified).
- xf86Msg in PreInit should be X_CONFIG instead of X_DEFAULT.
- If we don't do timeout, then we shouldn't provide the option (and definitely
  not print it). just leads to confusion.
- Inertia is only called from within the wheelEmu code, should be static. this
  allows us to move the WheelAxis struct out of evdev.h too.

> - This code will not work with a mouse that uses absolute positioning, I don't
> have a device handy to test that with.  If I have this right, it should ignore
> absolute motion events.

You can post relative events from an absolute device too, so that may just
work (don't have an abs device either).

> - EmulateWheelTimeout is not supported
see above

> - I have not done any of the ABI_XINPUT < 3 protection mentioned in commit
> 5d13259a5ddab31dbb2158975c8ccbb1f3c99046.

not needed yet, we'll do them when we (i.e. you ;) expose the wheel as property.

(In reply to comment #19)
> On Tue, Jul 29, 2008 at 10:52:25PM -0700, bugzilla-daemon@freedesktop.org
> wrote:
> > Ok, I went through and cleaned this up a good deal.  There should be
> > significantly less 'noise' in the main driver.  I've added calls to
> > EvdevWheelEmuFilterEvent in two places and added the necessary configuration
> > code.
> 
> thanks, looking much better. Again, a few comments:
> - indentation and coding style. Indentation is still messed up. (e.g.
>   EvdevWheelEmuInertia). Also, I'm not a big fan of the "no spaces" coding
>   style. Please use the form "a = b" instead of "a=b", and try to be
>   consistent (e.g. HandleButtonMap uses "if( condition)" instead of "if
>   (condition)"
> - Parenthesis around if conditionsa "if ((a > b) && (c < d)" make it easier to
>   read (noticed that in HandleButtonMap
-Coding style differences can be so much fun ;)  I'll go back through this and clean it up some more and hammer it into shape.  Does the Xorg project have a style guide?  What line length should I be shooting for?

> - EvdevWheelHandleButtonMapping - I'd much rather prefer to see that as a
>   separate patch to evdev.c. This is code we need anyway, and it shouldn't be
>   tied to the wheel emulation.

On this front, I was following the code in xf86-input-mouse and attempting to duplicate/enhance its behavior.  The X and Y axis configuration code looked to be specific to EmulateWheel,  I did not add in Z axis mapping, which looked like the appropriate configuration for the 'real' mouse wheel.  Hence I kept this code separate.  It could be easily adapted to handle Z axis mapping in a similar manner to the emulated axis's.

> - MSE_MAXBUTTONS is undefined. It used to be part of the server, but is now
>   mouse-driver only. A separate patch to #define it to 32 is welcomed :)

Well drat, that's what I get for using a non bit xorg-server to dev against :( I thought I had found that value defined in xf86OSmouse.h, oh well.  I'll add it back as a defined constant in the emuWheel.c file for the moment . . . 


> - any reason why we couldn't consolidate EmulateWheel and EmulateWheelButton
>   into one setting? such as EmulateWheel switches it on and at the same time
>   defines the buttonswitches it on and at the same time.

I did it this way to copy the behavior of xf86-input-mouse with the goal of being able to copy a config block setup for xf86-input-mouse and use it as is in xf86-input-evdev (at least the emulate wheel portion).  It could make sense to consolidate those into one setting though. . .

> - why do we need "configured". If Wheel is enabled, then it should be
>   automatically configured to sane defaults (or to whatever the user
>   specified).

This patch allows for the emulation of a horizontal as well as a vertical wheel.  The vertical wheel has sane defaults if it is turned on and not given a YAxisMapping, ( theoretically to correspond to the same button events as a standard mouse wheel).  The X axis or horizontal wheel emulation defaults to off.  This is a feature that I had no clue was there until I started digging around in xf86-input-mouse.  To duplicate that drivers behavior as much as possible this option defaults to off.  While it is possible to use a crafted value for one of the buttons for this I usually perfer explicit flags for turning things on and off.  Truthfully, the Y axis does not a configured flag, just the X axis.

If this code were split out into a separate entity, as you suggested earlier.  I would very likely change the WheelAxis struct into something more like:

struct {
   int up_button;
   int down_button;
} WheelAxis, *WheelAxis;

Then use an enum like:

enum {
   WHEEL_X = 0,
   WHEEL_Y,
   WHEEL_Z
};

and an array of WheelAxis to handle the button mappings.  

> - xf86Msg in PreInit should be X_CONFIG instead of X_DEFAULT.

Ok, that should be switched in my next patch version.

> - If we don't do timeout, then we shouldn't provide the option (and definitely
>   not print it). just leads to confusion.

Good point, I'll strip it out.

> - Inertia is only called from within the wheelEmu code, should be static. this
>   allows us to move the WheelAxis struct out of evdev.h too.
> 
  
How does making this function static allow the WheelAxis struct out of evdev.h?  I wasn't planning on doing any memory allocation within the wheel emulation code.  Are you suggesting putting WheelAxis into an emuWheel.h and including that in evdev.h?  The wheel emulation code should, depending on how rusty my C has gotten, allow for per device configuration.  


I'll attach the patch here shortly.

> > - Inertia is only called from within the wheelEmu code, should be static. this
> >   allows us to move the WheelAxis struct out of evdev.h too.
> > 
> 
> How does making this function static allow the WheelAxis struct out of evdev.h?
>  I wasn't planning on doing any memory allocation within the wheel emulation
> code.  Are you suggesting putting WheelAxis into an emuWheel.h and including
> that in evdev.h?  The wheel emulation code should, depending on how rusty my C
> has gotten, allow for per device configuration.  
> 

I think I see what you were talking about . . .  I'm not sure why I left the EvdevWheelEmuHandleButtonMap prototype in evdev.h  That was there initially when I was calling it from evdev.c.  Foo.

Created attachment 18025 [details] [review]
Mouse Wheel Emulation Patch take 3

Ok, here's a third take on this.  I've gone through and cleaned up emuWheel.c a good deal and rethought a few things there per your suggestions.  This patch should also include man page descriptions of the Emulate Wheel options, slightly modified, mostly copied, from xf86-input-mouse (The git version rather than the 1.3.0 version Gentoo is shipping at the moment).

- I pulled the inertia out of WheelAxis and into emulateWheel, does anyone really need to specify their inertia on a per axis basis?
- Stripped out the timeout option.
- Added man page text describing the behavior that this code aims to duplicate (from the standard mouse driver, does this need a attribution?) 
- Added statics where they looked appropriate.
- Cleaned up spacing and indentation a bit.

> -Coding style differences can be so much fun ;)  I'll go back through this and
> clean it up some more and hammer it into shape.  Does the Xorg project have a
> style guide?  What line length should I be shooting for?

http://wiki.x.org/wiki/CodingStyle
in addition, it's also "use the style of the remainder of the file", if
appropriate. In some cases that's not pretty though :)
 
> On this front, I was following the code in xf86-input-mouse and attempting to
> duplicate/enhance its behavior.  The X and Y axis configuration code looked to
> be specific to EmulateWheel,  I did not add in Z axis mapping, which looked
> like the appropriate configuration for the 'real' mouse wheel.  Hence I kept
> this code separate.  It could be easily adapted to handle Z axis mapping in a
> similar manner to the emulated axis's.

there's two parts to button mapping.
One is the mapping of physical buttons to virtual buttons. and then there's
the wheel buttons.
 
> Well drat, that's what I get for using a non bit xorg-server to dev against :(
> I thought I had found that value defined in xf86OSmouse.h, oh well.  I'll add
> it back as a defined constant in the emuWheel.c file for the moment . . . 

you're correct, it was in xf86OSmouse.h, but this file has since moved (as of
last week or so).

> I did it this way to copy the behavior of xf86-input-mouse with the goal of
> being able to copy a config block setup for xf86-input-mouse and use it as is
> in xf86-input-evdev (at least the emulate wheel portion).  It could make sense
> to consolidate those into one setting though. . .

fair enough. I'll let you decide on that.
 
> > - Inertia is only called from within the wheelEmu code, should be static. this
> >   allows us to move the WheelAxis struct out of evdev.h too.
> 
> How does making this function static allow the WheelAxis struct out of evdev.h?
>  I wasn't planning on doing any memory allocation within the wheel emulation
> code.  Are you suggesting putting WheelAxis into an emuWheel.h and including
> that in evdev.h?  The wheel emulation code should, depending on how rusty my C
> has gotten, allow for per device configuration.  

With that I meant we can make EvdevEmuWheelInertia static, so we don't need
it's declaration in the evdev.h file. Which in turn means we don't need the
WheelAxis struct in the evdev.h file but can just put it into the emuWheel.c -
just an extra bit of  separation between core and extras.

Take 3 looks fine, I applied it locally and fix up the two or three minor
things myself. Expect it upstream next week.

> Take 3 looks fine, I applied it locally and fix up the two or three minor
> things myself. Expect it upstream next week.
> 

Looks like there is a bug in EvdevEmuWheelFilterEvent.  I spotted it looking at bug #15961.  The code around line 81 in emuWheel.c:

        /* Check for EmulateWheelButton */
        if((pEv->code - BTN_LEFT + 5) == pEvdev->emulateWheel.button) {
            pEvdev->emulateWheel.button_state = value;
            return TRUE;
        }

        break;

This block is suppoesd to detect the button to activate wheel emulation.  Unfortunately, it seems to only be able to detect the higher mouse buttons ( I normally use button 8 ).  So, if for some reason some one wanted to use button 1, 2, or 3 say to emulate a mouse wheel, it wouldn't work.  This code calls button 1 button 5, button 2 button 6 and so on.  

Would it make sense to add a utility function that handles decoding button events to a button index/number?  This would reduce code duplication, especially when drag lock is brought into play, and reduce some of the complexity in evdev.c 

Say something like:

int
EvdevButtonEventToIndex(int code) {
    int button=0;

    // mapping logic (probably the similar to what's in ReadInput)

    return button;
}

Properly crafted it would return a 0 or -1 one say on non-button events.  Or something like that.

I might have such a function here shortly as part of a fix for bug #15961 I can submit it as a separate patch if you would like.

> --- Comment #24 from Chris Salch <chrissalch@letu.edu>  2008-08-03 01:15:07 PST ---
> This block is suppoesd to detect the button to activate wheel emulation. 
> Unfortunately, it seems to only be able to detect the higher mouse buttons ( I
> normally use button 8 ).  So, if for some reason some one wanted to use button
> 1, 2, or 3 say to emulate a mouse wheel, it wouldn't work.  This code calls
> button 1 button 5, button 2 button 6 and so on.  

I saw that one and figured it's not a dealbreaker for now though. If you have
a patch, I'm willing to wait before pushing upstream though.

> Would it make sense to add a utility function that handles decoding button
> events to a button index/number?  This would reduce code duplication,
> especially when drag lock is brought into play, and reduce some of the
> complexity in evdev.c 

you can give it a try, just make sure that you cover all the corner-cases
(especially the tool handling).
Please submit that patch as a separate one, it should only touch evdev.c after
all

(In reply to comment #26)

> you can give it a try, just make sure that you cover all the corner-cases
> (especially the tool handling).
> Please submit that patch as a separate one, it should only touch evdev.c after
> all
> 

It might be tricky to separate the utility function from the ability to handle more buttons for emulate wheel as that would be my solution to handling all of the buttons as well as my solution for draglock handling.  (Having the button number as PostButtonEvent wants it is easier to deal with than the kernel events.)

Created attachment 18115 [details] [review]
ButtonToNumber patch

This patch should be a simple function that returns a button number, faithfully reproduced as the original code in ReadInput does or 0 for non-button events.  It returns 0 on the tool events.  This helps to consolidate the code into one place that can be called from anywhere.

Created attachment 18116 [details] [review]
Evdev.c patch to clean up duplication.

This patch modifies ReadInput to make use of the EventToButton function.  It clarifies the flow of ReadInput and allows emulation code that only cares about button events to deal with X button numbers rather duplicate the event translation code (Wheel Emulation and Drag Locking bug #15961).

Doing things this way would lead to emulate wheel's filter event being split into two functions and move the actual event parsing back into evdev rather than duplicating it in EmuWheelFilterEvent.

Created attachment 18118 [details] [review]
Take 4

If I've created this patch right, which I'm not entirely sure I have, it should contain mouse wheel emulation code built against ButtonToNumber.patch.  I'll wait to see if that is accepted before taking the more advanced step ;)

If neither of those are accepted, the take three patch needs one addition:

    WheelAxisPtr pAxis = NULL;
    int value = pEv->value;

+    /* Has wheel emulation been configured to be enabled? */
+    if (!pEvdev->emulateWheel.enabled)
+       return FALSE;

    /* Handle our motion events */


I completely forgot to chec wether the wheel emulation was enabled or not.

The two button map patches are fine, but I merged them into one patch, seemed
more appropriate.

I don't understand this comment though:
/* Add filter logic not requiring explicit button events here to avoid doing
   this conversion over and over again. */

(In reply to comment #31)
> The two button map patches are fine, but I merged them into one patch, seemed
> more appropriate.
> 
> I don't understand this comment though:
> /* Add filter logic not requiring explicit button events here to avoid doing
>    this conversion over and over again. */
> 


Sorry about that, this comment may need to be rewritten.  I was referring to being able to split and simplify the EmuWheelFilter event function.  One function to handle motion events only and one function that is essentially a toggle for the active flag, only.  Two functions but simpler code and the event decoding can be left in evdev.h rather than here.  Having the button number code allows a one line inserting in evdev for the button decoding.  (Putting it in a function makes reading a little easier ;)

This came to mind as I was working through it last night but I didn't want to dive off the deep end if the second button patch wasn't accepted.

Created attachment 18149 [details] [review]
0001-Adding-a-function-to-map-button-events-to-button-num.patch

Please look at this patch. It merges both of yours into one, with cleanup. Also, the patch you provided didn't work with button 1 presses when MB emulation was enabled. Should be fixed with this one.

(In reply to comment #33)
> Created an attachment (id=18149) [details]
> 0001-Adding-a-function-to-map-button-events-to-button-num.patch
> 
> Please look at this patch. It merges both of yours into one, with cleanup.
> Also, the patch you provided didn't work with button 1 presses when MB
> emulation was enabled. Should be fixed with this one.
> 

Nice :)  I'll work from this and update my Wheel emulation code to work sanely with it (Splitting out the event handling cleanly)

Comment on attachment 18118 [details] [review]
Take 4

Needs to be Updated for the buttonNumbers patch

Created attachment 18166 [details] [review]
This should be the last one.

Hopefully this should have the works all ready to go.  This version uses the button to number mapping function rather than trying to do it's own mapping.  The only actual event parsing is for motion events.

Looks good, thanks. Applied, will be pushed after testing.

I applied one change: if the wheel button is invalid, emulation is disabled
and the server prints a warning.
Added one more warning about built-in inertia being used (if inertia is
invalid).

Also one FIXME, if we're changing numButtons, we should notify the server -
but there aren't any mechanisms to do so yet (since the server would then have
to notify the clients).

(In reply to comment #37)
> Looks good, thanks. Applied, will be pushed after testing.
> 
> I applied one change: if the wheel button is invalid, emulation is disabled
> and the server prints a warning.
> Added one more warning about built-in inertia being used (if inertia is
> invalid).
> 
> Also one FIXME, if we're changing numButtons, we should notify the server -
> but there aren't any mechanisms to do so yet (since the server would then have
> to notify the clients).
> 

All sounds good to me :)  I'll wait to see it in git.

One small thing I've noticed.  The Emulate3Button needs to be the last filter function.  I think the timeout code is causing it to hijack button presses for the first 3 buttons.  Something like this should do the trick:

diff --git a/src/evdev.c b/src/evdev.c
index 5bc562a..3fa6b2d 100644
--- a/src/evdev.c
+++ b/src/evdev.c
@@ -288,10 +288,10 @@ EvdevReadInput(InputInfoPtr pInfo)
             default:
                button = EvdevUtilButtonEventToButtonNumber(ev.code);

-               if (EvdevMBEmuFilterEvent(pInfo, button, value))
+               if (EvdevWheelEmuFilterButton(pInfo, button, value))
                   break;

-               if (EvdevWheelEmuFilterButton(pInfo, button, value))
+               if (EvdevMBEmuFilterEvent(pInfo, button, value))
                   break;

                if (button)

Emulation pushed as a9d72b40fbe178fa4fbb9d0e7c02dc6c5250969a.
Filter-swapping pushed as 555f5a7cbf3c980c436c205e9b23a78f3e19bdfe.

Thanks for your efforts!