Created attachment 20028 [details] [review]
VertAdjustmentFactor option to alter Y sensitivity

This patch adds an option to set an adjustment factor for vertical movement. At its default value of 1.0 there is no change, but with a setting of 0.6 it corrects the touchpad of the HP 2133 (for example) to have similar sensitivity in the x and y directions.

I made the patch on a git branch just using "git diff master". If it is not in the correct format just let me know and I'll be happy to make another one.

See also
http://lists.freedesktop.org/archives/xorg/2008-November/040092.html

Same thing for my new Lenovo S10 netbook -- the synaptics touchpad works just fine, but the vertical axis is way too sensitive.  I'm fairly certain that a whole host of newer netbooks are using this same touchpad and have similar issues (some models are mentioned in the discussion linked above).

Is this the official bug tracking the patch into the system, or does that just happen informally on the list?

is there any reason not to include the same option for X scaling as well? 
other than that, patch is looking good, please resubmit as git formatted patch.

I don't think a separate X adjustment factor is helpful as you can correct any aspect ratio with the Y factor alone, then adjust the overall movement with the regular settings.

For the patch, sorry, could you tell me what options to pass to git or where there is a patch submission guideline? I use git for my personal work all the time but I'm still unclear on how sharing patches is generally done, other than a straight diff.

I'd say that the right name is something like "AspectRatio" not "VertAdjustmentFactor" and this would remove any confusion over X vs. Y.  (although one could wonder whether the aspect ratio is X/Y or Y/X)

Does anyone know what the Windows drivers are doing to fix this problem?  I would assume that you could query the device for both the resolution in pixels as well as the resolution in mm, and then compute the AspectRatio from those numbers.  I'm poking around on my hardware, but I don't expect to see anything obvious...

Let me know if there's anything I can try out here.  For example, xinput list-properties, lsusb, lspci, etc...  

> --- Comment #6 from Steve Lacy <slacy@slacy.com>  2008-12-09 21:29:23 PST ---
> I'd say that the right name is something like "AspectRatio" not
> "VertAdjustmentFactor" and this would remove any confusion over X vs. Y. 
> (although one could wonder whether the aspect ratio is X/Y or Y/X)

I agree, AspectRatio is probably better, and it needs to be documented
accordingly in the man page.

> Does anyone know what the Windows drivers are doing to fix this problem?  I
> would assume that you could query the device for both the resolution in pixels
> as well as the resolution in mm, and then compute the AspectRatio from those
> numbers.  I'm poking around on my hardware, but I don't expect to see anything
> obvious...

I don't think the kernel exposes this information (if it even has it).


Regarding git patch submission: git commit, then git format-patch HEAD~1 (for
the last commit) and attach the patch file (usually
0001-whatever-commit-msg.patch).

If you do rename it "AspectRatio" I think you should use the convention of X/Y as is used in the TV & movie industry. 

In other words: 

A screen that's 4 units wide and 3 units high has an aspect ratio of "1.33" 
A screen that's 16 units wide and 9 units high has an aspect ratio of "1.77" 
etc.
Assuming a pad whose width is 5 units and height is 3 units, the AspectRatio will be 5/3 or 1.66

On the implementation side, I don't think you should never multiply input coordinates by any number greater than 1.0, because it will cause odd rounding / aliasing issues, so I think the logic should be something like this:


if (AspectRatio > 1.0) { 
  y /= AspectRatio; 
} else { 
  x *= AspectRatio; 
}

Although I'm not familiar enough with X input semantics to know if this will always be the correct thing to do. 

On Wed, Dec 10, 2008 at 03:47:17PM -0800, bugzilla-daemon@freedesktop.org wrote:
> --- Comment #8 from Steve Lacy <slacy@slacy.com>  2008-12-10 15:47:16 PST ---
> If you do rename it "AspectRatio" I think you should use the convention of X/Y
> as is used in the TV & movie industry. 

agreed.
 
> On the implementation side, I don't think you should never multiply input
> coordinates by any number greater than 1.0, because it will cause odd rounding
> / aliasing issues, so I think the logic should be something like this:
> 
> if (AspectRatio > 1.0) { 
>   y /= AspectRatio; 
> } else { 
>   x *= AspectRatio; 
> }

agreed.

> Although I'm not familiar enough with X input semantics to know if this will
> always be the correct thing to do. 

this effect is purely within the driver, so there is no semantics to worry
about other than "make it not suck".

I've been working with another member of the xorg mailing list, and have found that the required issue for most of these trackpads is a missing call to SYN_QUE_RESOLUTION. 

The description of the query can be found in http://www.synaptics.com/sites/default/files/ACF126.pdf sections 2.4.3 and 3.5.1.  

The query $08 returns the X and Y resolutions in pixels per MM, as a single byte. 

The other list member has a patch that he's tried and says that it returns the proper (non-square) resolution values for his touchpad.  I'm going to point him at this bug and maybe he can chime in. 

I looked at the code for both the Synaptics and evdev kernel drivers, and found that it was a significant amount of work to add X and Y resolutions (or aspect ratio correction based on the SYN_QUE_RESOLUTION values) and decided that this was beyond my ability to craft up a patch.  Ideally, this would be patched into evdev somehow, or exported via the synaptics driver in the shared memory segment.  (Both would be non-compatible changes as far as I can tell). 

> I looked at the code for both the Synaptics and evdev kernel drivers, and found
> that it was a significant amount of work to add X and Y resolutions (or aspect
> ratio correction based on the SYN_QUE_RESOLUTION values) and decided that this
> was beyond my ability to craft up a patch.  Ideally, this would be patched into
> evdev somehow, or exported via the synaptics driver in the shared memory
> segment.  (Both would be non-compatible changes as far as I can tell). 

If I understand you correctly, we need to get the kernel driver to issue the
call first. Then it can export it to the X driver, which can do stuff with
this information. Without an updated kernel patch, this one is a no-go.

Nonetheless. One possibility is to add the information is through explicit
configuration options instead of autodetecting it. If you want to prepare a
patch towards this, please feel free to do so.

Yes, I think it would be ideal to have the kernel query & export the resolution values as a long term solution. 

Another possibility is to have the kernel do the resolution query and adjustment for the aspect ratio before passing back any of its values.  Although I'm fairly certain I could get this working on my non-square device, I don't have access to enough other trackpads to know if that level of change would be compatible with the wide array of supported devices.  In other words, I'd be afraid of breaking input on some class of synaptics pads. 

When Steve pointed out SYN_QUE_RESOLUTION I tested it in two laptops.  Wide touchpad returned values infoXupmm=74 infoYupmm=160 while a normal touchpad returned 81 and 95.

I didn't find any obvious mechanism either for passing this kind of information back to user space.  If there isn't one it would make things quite simple as the only place compensation can be done would be the kernel.  I posted a question about this to linux-input list recently http://thread.gmane.org/gmane.linux.kernel.input/6243. 

I did a simple patch for synaptics kernel driver that scales the absolute coordinates before reporting them.  I suppose it's a bit out of topic so I do not attach it here but just give a link instead for the ones who like to experiment http://pastebin.com/f3b5bf09b

Created attachment 22280 [details] [review]
Vertical and horizontal multiplier options

Comment on attachment 22280 [details] [review]
Vertical and horizontal multiplier options

Useful to set size of touchpad in percent (like VertMultiplier=60 means that height or touchpad is 60% from width)

Some time ago I linked here a simple version of a patch for Synaptics driver in the kernel.  It adjusts the coordinates according to the resolution reported by the touchpad and should make it unnecessary to expose individual x and y sensitivity variables to users.

I would really like to hear opinions on what would be the preferred approach for you, especially from Xorg devels, since I do not want to try submit the patch to kernel if you are against the idea! :)

In the current version [1] I've adjusted the coordinates and also the minimum and maximum values that are used as basis e.g. for calculating edge scroll limits.

Can you see any problems in this approach?  


[1] 
preliminary patch for kernel's synaptics driver 
http://pastebin.com/f7e8505c

Having the resolution provided by the kernel is good for autoconfiguration of
sensitivity. I don't think the kernel should do coordinate adjustment though.
It should be something up to userspace.

(In reply to comment #17)
> Having the resolution provided by the kernel is good for autoconfiguration of
> sensitivity. I don't think the kernel should do coordinate adjustment though.
> It should be something up to userspace.

Ok, I will not try submitting the patch. 

I could try to dig into details on how to provide the resolution from kernel input layer to userspace. However I would appreciate a lot if you can point me into right direction, maybe giving me an example on how it should look from xorg perspective (for example, introduce new ioctl primitive similar to ABS query).

Best to ask on linux-input or lkml, or look through the archives.
Henrik Rydberg recently posted patches for multi-touch support, they may serve
as a guide for how to get new features exposed to userland.

Any news about this bug?

I just want to say that I'm using Nathan Hamblen's patch right now and it works nice here. IMHO the VertAdjustmentFactor option is a good idea and it should be merged.

I promised to take a try at this but didn't until now, sorry.

I still haven't asked the kernel folks how they recommend exporting the new resolution values for automatic configuration but looking at the interface again I think I might be getting quite near with a solution like this http://pastebin.com/f7bc1e633. 

On Xorg side I've added two new configurable variables: "VertResolution" and "HorizResolution".  The default values are read from the (patched!) kernel.  If not running patched kernel the variables are set to 1, making the scaling practically a no-op.  I'll attach an experimental patch here.

Could this be something people feel worth of pushing forward?  

I should note the obvious about theae patches: I'm not very familiar with either Xorg or kernel and the code is almost not tested at all.  

Created attachment 25322 [details] [review]
Experimental patch with autoconfiguration by reading resolution from (patched) kernel, see comment #21

basic approach is correct, a few nitpicks on a quick glance:
- don't calculate scale for each event, do it once and store it somewhere (and recalculate when the property changes)
- ABS_RX/RY is AFAIK for "rotary x/y", not for resolution
- document what unit is used for the resolution (units/mm, units/inch, bananas/tree?)

Before we can put that into the driver, please make sure that there's at least a plan to go forward with the kernel support.

Tero, I'm assigning this bug to you for now, please reassign back to me when the patch is done from your point of view (I'm still on the CC list, so I'll see further comments)>

(In reply to comment #23)
> Before we can put that into the driver, please make sure that there's at least
> a plan to go forward with the kernel support.

Just reporting that I still haven't had success to get the kernel patch forward:
http://www.spinics.net/lists/linux-input/index.html#03327
The single maintainer of the input subsystem seems to be rather busy and the queue of people waiting for review of their stuff looks overwhelming :-(

Created attachment 27390 [details] [review]
patch with autoconfiguration by reading resolution from kernel (2.6.31 required)

(In reply to comment #25)
> Created an attachment (id=27390) [details]
> patch with autoconfiguration by reading resolution from kernel (2.6.31
> required)
> 

Just to clarify, the patch works also with older kernels but obviously without autoconfiguration support.  HorizResolution and VertResolution can be set manually to override default values.

The scaling multipliers are now calculated only when properties change like suggested but the solution is not very elegant.  All functions in synaptics.c were declared as static so I didn't want to add public function that properties.c could call when the values change.

Please comment if there's something else I should change!

thanks again for the patch! a few comments:
- the man page should state the default value/behaviour if the resolution is unset.
- you allow for negative resolutions in the property handler. is this intended? If not, s/INT32/CARD32/.
- wouldn't it be best to calculate the coefficient when the property is changed (or at startup) instead of the checks in ScaleCoordinate? I'm fine with having two functions, ScaleCoordinate and CalculateRation (or somesuch).
- InitValuatorAxisStruct takes a resolution parameter (in units/m, not mm!), please fill this in appropriately.


The patch sets the aspect ration based on the resolution (which makes sense). I'm wondering if there should be a way of setting the aspect ratio independent of the resolution?

Created attachment 27525 [details] [review]
Add configurable x/y resolution to fix sensitivity on wide touchpads

Thanks for the comments Peter!

> the man page should state the default value/behaviour if the resolution is 
> unset.

Added some text about default values.

> you allow for negative resolutions in the property handler. is this intended?
> If not, s/INT32/CARD32/.

It was not the intention.  I replaced the type but it still allows negative values since there's no actual checks in the set property handler... 

> wouldn't it be best to calculate the coefficient when the property is 
> changed (or at startup) instead of the checks in ScaleCoordinate? 
> I'm fine with having two functions, ScaleCoordinate and CalculateRation 

I agree, added CalculateScalingCoeffs().  

I'm not very clear on the coding rules.  Should I add new header file synaptics.h for prototype of this function?  As far as I can see it's the first function in synaptics.c that is called outside file scope.  For now I just declared it in both synaptics.c and properties.c.

> InitValuatorAxisStruct takes a resolution parameter (in units/m, not mm!)

I filled the min and max parameters with the resolution * 1000.  

How are these values used?  Will somebody do calculations according to resolution and/or coordinates after the x/y values leave synaptics driver?  Scaling could potentially cause unexpected behavior somewhere.  I had to be careful to scale at the correct place in the code in order not to break e.g. edge detection which depends on unscaled coordinates (I added comment about this to the code too). 

> The patch sets the aspect ration based on the resolution (which makes sense).
> I'm wondering if there should be a way of setting the aspect ratio 
> independent of the resolution?

To do that wouldn't we need to define "ideal touchpad" that has equal resolution on both axes and use the aspect ratio of that as a reference for all calculations.  I don't know if this is so easy to do...  My old laptop has touchpad with physical aspect ratio of 7:5 and VertResolution=81 and HorizResolution=92 which I never noticed as they are nearly equal.  My current one has 2:1 and VertResolution=74 and HorizResolution=160 which is impossible to use without scaling.

I wonder if it's worth of adding configuration option for setting aspect ratio.  Personally I think user should never need to be bothered with details like x/y sensitivity anyway.  It should be always auto-configured.

(In reply to comment #28)
> > you allow for negative resolutions in the property handler. is this intended?
> > If not, s/INT32/CARD32/.
> 
> It was not the intention.  I replaced the type but it still allows negative
> values since there's no actual checks in the set property handler... 

CARD32 is unsigned int, so if you update the 
SynapticsParameters to take unsigned ints as well anything you send in will be automatically positive.

Please add the unsigned comment to the synaptics-property.h as well.

While reading your other comment about the aspect ration setting a thought struck me though: is there even a reason to make the resolutions property writable? it's these properties that you'd have little reason to actually change at runtime. If you ever have to adjust it you have a 'broken' touchpad and then you need to add it to the xorg.conf anyway.

> I'm not very clear on the coding rules.  Should I add new header file
> synaptics.h for prototype of this function?  As far as I can see it's the first
> function in synaptics.c that is called outside file scope.  For now I just
> declared it in both synaptics.c and properties.c.

synaptics.h is the shared header for synclient and syndaemon as well. synapticsstr.h is the better choice here.


> > InitValuatorAxisStruct takes a resolution parameter (in units/m, not mm!)
> 
> I filled the min and max parameters with the resolution * 1000.

I think it's better to use a default resolution value of 0 here. The server doesn't actually do anything with it other than report it to clients when asked. I didn't find any convention but a default resolution of 0 is IMO better than 1 (which might be a valid value).

Created attachment 27542 [details] [review]
Add configurable x/y resolution to fix sensitivity on wide touchpads

> CARD32 is unsigned int, so if you update the 
> SynapticsParameters to take unsigned ints as well anything you send in will be
> automatically positive.
> 
> Please add the unsigned comment to the synaptics-property.h as well.

Ok, done this. 

> While reading your other comment about the aspect ration setting a thought
> struck me though: is there even a reason to make the resolutions property
> writable? it's these properties that you'd have little reason to actually
> change at runtime. If you ever have to adjust it you have a 'broken' touchpad
> and then you need to add it to the xorg.conf anyway.

I guess the value will be set only when running on earlier kernels.  Don't know if there are touchpads that report invalid resolution...  So do you mean the value could still be writable but through xorg.conf and not synclient?  Would it then be removed from properties completely or just from SetProperty()?  Will that be confusing for user if a configurable option is missing or read only in synclient's perspective but writable on xorg.conf? 

Basically my reason for adding it as a property was only that I thought properties should map one to one with configurable options.  They also seemed to be writable in general.  

I myself have stopped modifying xorg.conf completely since touchpad is nowadays the only thing requiring manual configuration.  So I run synclient at desktop session startup.  In a way it's convenient when everything related to touchpad is at one place.  

> synaptics.h is the shared header for synclient and syndaemon as well.
> synapticsstr.h is the better choice here.

Moved the function prototype to synapticsstr.h.

> I think it's better to use a default resolution value of 0 here.

Ok, now it should be reported as 0 if we do not know the resolution.

Just to add...  
Basically I would be ready to rely on auto-configuration only and remove the configurable property/option since it is just a fallback anyway.  I hope that when we have more test results and see that this works for all laptops, people running latest xorg will also be running 2.6.31.  There's no need for fallbacks then.

my 2 cts:

Please do not remove the manual configuration! While this feature is now available in the Linux kernel, it will not be available in other free *nixes, such as the *bsds for a while.

Thank you all for the contributions! I am looking forward to being able to finally use my touchpad in Linux!

> I guess the value will be set only when running on earlier kernels.  Don't know
> if there are touchpads that report invalid resolution...  So do you mean the
> value could still be writable but through xorg.conf and not synclient?  Would
> it then be removed from properties completely or just from SetProperty()?  Will
> that be confusing for user if a configurable option is missing or read only in
> synclient's perspective but writable on xorg.conf? 

The value would be available as a read-only property. the 'Synaptics
Capabilities' property is one example for a write-only only property. It
serves a similar purpose - inform clients about the hardware capabilities.
(There's little point changing the flag that specifies whether the hardware
can do multi-touch)

the resolution property seems similar, it makes little sense to modify the
resolution at run-time. Note that this doesn't rule out manual
configuration, just run-time modification.

implementation-wise: just return BadValue from the SetProperty handler.

> I myself have stopped modifying xorg.conf completely since touchpad is nowadays
> the only thing requiring manual configuration.  So I run synclient at desktop
> session startup.  In a way it's convenient when everything related to touchpad
> is at one place.  

of course, this makes sense for user-specific settings (tapping, scroll
method, etc.) but not really for hardware properties IMO.

Created attachment 27608 [details] [review]
Add configurable x/y resolution to fix sensitivity on wide touchpads

> the resolution property seems similar, it makes little sense to modify the
> resolution at run-time. Note that this doesn't rule out manual
> configuration, just run-time modification.
>
> implementation-wise: just return BadValue from the SetProperty handler.

I've updated the patch.  Since HorizResolution and VertResolution are now read-only I removed them from synclient too but I left them in the manual page. 
Please comment if there's something I could sill make better. 

Actually I'm not up to date if input device parameters can still be set in xorg.conf but at least in Xorg version shipped with Ubuntu 9.04 I can set HorizResolution and VertResolution values now using fdi file.

Created attachment 27609 [details] [review]
Add configurable x/y resolution to fix sensitivity on wide touchpads

Made the patch apply to current head. Removed the prototype for CalculateScalingCoeffs() from synapticsstr.h and changed it back to static.

pushed as 0c3fbceb1b2a18f92166fe75c44b5aaada693c4b. thanks for your work.

(In reply to comment #34)
> Actually I'm not up to date if input device parameters can still be set in
> xorg.conf but at least in Xorg version shipped with Ubuntu 9.04 I can set
> HorizResolution and VertResolution values now using fdi file.


from the driver's POV, options set in the fdi are the same as those set in the xorg.conf