Bug 8583 - patch for mouse speed and acceleration issues
patch for mouse speed and acceleration issues
Status: RESOLVED FIXED
Product: xorg
Classification: Unclassified
Component: Server/Input/Core
unspecified
All Linux (All)
: high enhancement
Assigned To: Daniel Stone
: patch
: Niklas (view as bug list)
Depends on: 9156
Blocks: 2927 3908
  Show dependency treegraph
 
Reported: 2006-10-10 04:08 UTC by Simon Thum
Modified: 2008-07-20 13:46 UTC (History)
12 users (show)

See Also:


Attachments
Documentation for the patch (9.86 KB, text/plain)
2006-10-10 04:09 UTC, Simon Thum
no flags Details
Patch for mouse speed/acceleration (15.93 KB, patch)
2006-10-10 04:13 UTC, Simon Thum
no flags Details | Splinter Review
updated documentation (11.27 KB, text/plain)
2006-10-23 08:31 UTC, Simon Thum
no flags Details
patch for enhanced mouse acceleration (16.07 KB, patch)
2006-10-23 08:35 UTC, Simon Thum
no flags Details | Splinter Review
Patch for enhanced acceleration (21.71 KB, patch)
2006-11-13 08:33 UTC, Simon Thum
no flags Details | Splinter Review
Documentation for the patch (11.06 KB, text/plain)
2006-11-13 08:34 UTC, Simon Thum
no flags Details
patch against master (23.50 KB, patch)
2006-11-21 17:37 UTC, Simon Thum
no flags Details | Splinter Review
slightly revised doc. (11.22 KB, text/plain)
2006-11-21 17:37 UTC, Simon Thum
no flags Details
patch against master (14.77 KB, patch)
2006-12-05 14:33 UTC, Simon Thum
no flags Details | Splinter Review
Documentation for the patch (11.24 KB, text/plain)
2006-12-05 14:41 UTC, Simon Thum
no flags Details
revised patch (22.90 KB, patch)
2006-12-22 02:23 UTC, Simon Thum
no flags Details | Splinter Review
Patch agains 1.2.0 (16.52 KB, patch)
2007-03-21 08:39 UTC, Simon Thum
no flags Details | Splinter Review
patch against current git (33.26 KB, patch)
2007-03-30 09:47 UTC, Simon Thum
no flags Details | Splinter Review
documentation (12.32 KB, text/plain)
2007-03-30 09:51 UTC, Simon Thum
no flags Details
patch against master (27.64 KB, patch)
2007-04-11 14:25 UTC, Simon Thum
no flags Details | Splinter Review
revised patch against master (28.86 KB, patch)
2007-05-28 09:08 UTC, Simon Thum
no flags Details | Splinter Review
documentation (13.56 KB, text/plain)
2007-05-28 09:21 UTC, Simon Thum
no flags Details
patch against xorg-server 1.2.0 (17.99 KB, patch)
2007-06-03 07:47 UTC, Simon Thum
no flags Details | Splinter Review
updated patch against current master (32.19 KB, patch)
2007-09-18 14:43 UTC, Simon Thum
no flags Details | Splinter Review
updated documentation (16.06 KB, text/plain)
2007-09-18 14:45 UTC, Simon Thum
no flags Details
preliminary patch (34.45 KB, patch)
2007-09-25 20:18 UTC, Simon Thum
no flags Details | Splinter Review
patch agaist X server (42.54 KB, patch)
2008-01-13 06:37 UTC, Simon Thum
no flags Details | Splinter Review
Xinput proto - needed for Xserver (2.81 KB, patch)
2008-01-13 06:38 UTC, Simon Thum
no flags Details | Splinter Review
lib/libXi: proof-of concept runtime parameter adjustment (1.60 KB, patch)
2008-01-13 06:40 UTC, Simon Thum
no flags Details | Splinter Review
app/xinput: proof-of concept runtime parameter adjustment (5.04 KB, patch)
2008-01-13 06:41 UTC, Simon Thum
no flags Details | Splinter Review
User documentation (16.24 KB, text/plain)
2008-01-13 09:26 UTC, Simon Thum
no flags Details
patch for Xserver (43.84 KB, patch)
2008-01-18 02:21 UTC, Simon Thum
no flags Details | Splinter Review
patch: X server (48.99 KB, patch)
2008-02-09 17:58 UTC, Simon Thum
no flags Details | Splinter Review
patch: libXi (2.88 KB, patch)
2008-02-09 17:59 UTC, Simon Thum
no flags Details | Splinter Review
patch: xinput tool (9.02 KB, patch)
2008-02-09 17:59 UTC, Simon Thum
no flags Details | Splinter Review
patch: Xinput proto (4.86 KB, patch)
2008-02-09 18:00 UTC, Simon Thum
no flags Details | Splinter Review
Documentation (16.73 KB, text/plain)
2008-02-09 18:04 UTC, Simon Thum
no flags Details
patch against X server (15.93 KB, patch)
2008-07-04 18:15 UTC, Simon Thum
no flags Details | Splinter Review
documentation (15.69 KB, text/plain)
2008-07-04 18:18 UTC, Simon Thum
no flags Details
revised patch (41.52 KB, patch)
2008-07-10 14:10 UTC, Simon Thum
no flags Details | Splinter Review
updated developer documentation (17.15 KB, text/plain)
2008-07-20 13:42 UTC, Simon Thum
no flags Details
patch for Xorg server 1.4.x (32.18 KB, patch)
2008-07-20 13:46 UTC, Simon Thum
no flags Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Thum 2006-10-10 04:08:13 UTC
This is not a bug peport but a patch proposal for enhanced mouse acceleration.
It 'fixes' bugs 2927, 3908 and related issues, as well as giving a more natural
feel for the mouse IMHO. I write 'fixes' in parentheses since it is not a
magical sword, but might need some settings.

It would be nice if a more experienced dev could help me crush the TODOs!
BTW, I use it and its rock stable.

Attached are documentation and patch.
Comment 1 Simon Thum 2006-10-10 04:09:45 UTC
Created attachment 7319 [details]
Documentation for the patch
Comment 2 Simon Thum 2006-10-10 04:13:05 UTC
Created attachment 7322 [details] [review]
Patch for mouse speed/acceleration
Comment 3 Simon Thum 2006-10-23 08:31:42 UTC
Created attachment 7492 [details]
updated documentation
Comment 4 Simon Thum 2006-10-23 08:35:22 UTC
Created attachment 7493 [details] [review]
patch for enhanced mouse acceleration

Some code cleanup, removed c++-comments, some optimization
Comment 5 Daniel Stone 2006-11-04 11:05:56 UTC
hi,
the algorithms and implementation look pretty good to me at a first glance. 
could you please redo this against master, though?  dix/getevents.c is what you
want to be looking at: GetPointerEvents will call acceleratePointer if
POINTER_ACCELERATE is set in flags.  so what you want to be doing is storing all
your information in the ValuatorClassRec (include/inputstr.h), and then changing
which acceleration function you call.

also, please wrap your lines at 80 characters.

thanks!
Comment 6 Simon Thum 2006-11-09 16:21:49 UTC
Raising the code to dix is entirely possible but i order to get configuration
done I will have to extend the dix interface (export functions and a struct). If
this is not a good idea please write soon.
Comment 7 Daniel Stone 2006-11-09 16:47:36 UTC
that's fine.  i suggest you just add an acceleration type integer to
ValuatorClassRec, and then a void *accelData or so, that the acceleration method
of your choice can use.  that way, we only ever need to break abi once.

thanks!
Comment 8 Simon Thum 2006-11-13 08:33:25 UTC
Created attachment 7772 [details] [review]
Patch for enhanced acceleration

The code is now in dix, but needs to be enabled for every server, which is
exemplary in hw/xfree86.
However, I have not yet managed to build Xorg, so the whole thing is untested
and might not even compile. Could someone (Daniel?) please apply it and give me
some feedback?
Comment 9 Simon Thum 2006-11-13 08:34:58 UTC
Created attachment 7773 [details]
Documentation for the patch

Documentation for the patch in general and configuration options
Comment 10 Simon Thum 2006-11-13 08:40:14 UTC
Just noted there is a small error:
This line:
https://bugs.freedesktop.org/attachment.cgi?id=7772&action=diff#a/dix/getevents.c_sec4
is superfluous and should not compile. Going to respost later.
Comment 11 Simon Thum 2006-11-21 17:37:05 UTC
Created attachment 7858 [details] [review]
patch against master

I finally managed to get the code to dix so one can see the effects.
Sadly, there is a bug somewhere in the server that prevents (adaptive)
deceleration from working, which IMHO ist the most appealing feature.
Also, the integration is sort of a hack, but a more involved dev should easily
be able to improve that.

@Daniel: It seems that there is a Bug which shows up when you do e.g.
xset m 2 0
then slow movements are screwed. It seems that if the acceleration does not
accelerate but rather slow down strange things happen. You should not need to
apply my patch to reproduce.
Comment 12 Simon Thum 2006-11-21 17:37:58 UTC
Created attachment 7859 [details]
slightly revised doc.
Comment 13 Simon Thum 2006-11-25 11:56:35 UTC
I've been able to check my clain there is a bug in current git and it is (bug#9156).
simply do 
>xset m 2 0
and see youself.
Since by default the proposed algo does not decelerate, it hides the problem, as
well as not using polynomial acceleration does.
I tried to fix but it seemingly depends on other factors. IMHO, the acc function
should be able to decelerate, but currently it's a mess.
Comment 14 Simon Thum 2006-12-05 14:33:04 UTC
Created attachment 7969 [details] [review]
patch against master

This should now work and also fix the classic implementation.
Integration is still suboptimal, but it works fine.
Comment 15 Simon Thum 2006-12-05 14:41:04 UTC
Created attachment 7970 [details]
Documentation for the patch
Comment 16 Bryan Hoyt 2006-12-21 16:09:53 UTC
Hi,

I hope this is a good place to leave this comment. I'm not a dev, but I'm very
interested in this patch.

I just want to say thank you very much to Simon for his work in implementing it. 

I would encourage the developers to think seriously about including this in the
mainstream code as soon as possible; I've seen a fair bit of wishing on the web
(often among laptop users, because of their more restricted mouse requirements)
that X had a smoother mouse acceleration function.
Comment 17 Simon Thum 2006-12-22 02:23:17 UTC
Created attachment 8201 [details] [review]
revised patch

This patch should again apply in current master branch.
Comment 18 Simon Thum 2006-12-22 02:44:32 UTC
I am still seeking feedback on this. Especially, I need hints on improving
integration in xfree86 backend:
1) how to suppress those carryover_ fields (see ApplyExtendedValuatorSettings)
2) should the dix api be changed to support the new valuator fields
3) where is a good place to apply the extended settings (or to call
ApplyExtendedValuatorSettings)? Must be after DEVICE_INIT (for valuators to
exist), and I'd like to stay device driver independent. Also, hotplug is an issue.

Thanks!
Comment 19 Peter Hutterer 2007-03-06 18:29:10 UTC
some cosmetics:
Use #defines for scheme names. Scheme 0 or 1 is not very explanatory.

Can you write one acceleratePointer and then split from there into accelPointerDumb, accelPointerSmart or whatever you want to name them? GPE shouldn't decide on the accel scheme and it'll make it more readable. 

Don't call it carryover_accelScheme, don't be too apologetic. You might want to put scheme and data into a struct, it'll make it easier to add additional stuff at a later point in time.

please don't mix tabs and spaces. http://wiki.x.org/wiki/CodingStyle

xf86ProcessCommonOptions: bit hacky. don't strcmp, alloc memory inside PVC and call it dependent on option AccelerationScheme. PVC can then decide what to do. 
Comment 20 Simon Thum 2007-03-07 14:07:02 UTC
Thanks for the comments!

Those carryover_ fields are in effect a hack I got rid of already. When it works (current Git won't on my machine even vanilla) youl'll see a post :)
I'll see what I can do about the rest.
Comment 21 Alex Deucher 2007-03-15 06:49:59 UTC
*** Bug 10295 has been marked as a duplicate of this bug. ***
Comment 22 Simon Thum 2007-03-21 08:39:06 UTC
Created attachment 9248 [details] [review]
Patch agains 1.2.0

This patch is against a 7.2 release / 1.2.0. It contains some improvements in config handling and velocity approximation.
The patch against git is not currently stable.
Comment 23 Simon Thum 2007-03-30 09:47:46 UTC
Created attachment 9389 [details] [review]
patch against current git

This patch contains some of peters suggestions. I haven't introduced #defines but made up a function pointer to exchange schemes, so it can be implemented compile-time checked. Init/shutdown still needs improvement.
Comment 24 Simon Thum 2007-03-30 09:51:37 UTC
Created attachment 9390 [details]
documentation

Revised documentation
Comment 25 Peter Hutterer 2007-04-09 16:37:43 UTC
There's a load of needless whitespace diffs in that patch, seems your whitespacing differs to what the file already has. Makes it very hard to read. Apart from that it looks good from the little I know.
Comment 26 Simon Thum 2007-04-10 14:06:03 UTC
(In reply to comment #25)
> There's a load of needless whitespace diffs in that patch, seems your
> whitespacing differs to what the file already has. Makes it very hard to read.
> Apart from that it looks good from the little I know.
Got that one. In your previous post you said I shouldn't strcmp, but what am I supposed to do to match pointing devices?
Comment 27 Daniel Stone 2007-04-10 14:28:31 UTC
(In reply to comment #26)
> (In reply to comment #25)
> > There's a load of needless whitespace diffs in that patch, seems your
> > whitespacing differs to what the file already has. Makes it very hard to read.
> > Apart from that it looks good from the little I know.
> Got that one. In your previous post you said I shouldn't strcmp, but what am I
> supposed to do to match pointing devices?

if dev->valuator exists, then that's enough to set up acceleration.

the meat of the patch seems okay, but i have a few style issues with it (mainly adding two new headers and source files just for acceleration), so i'll clean it up and push it to an api-break branch this week.
Comment 28 Simon Thum 2007-04-11 14:25:49 UTC
Created attachment 9565 [details] [review]
patch against master

this patch contains some minor improvements and works against current git.
The 2 headers are overkill, mainly a failed attempt to integrate the 1.2.0 and current git patch versions. IMHO putting ptraccel.h into input[str].h should be best, since that would preserve the fact you only need ptrveloc.h if you actually fiddle with it.
I have some improvements in queue[0], but apart from the small mem hole in CloseDevice() I don't know why it shouldn't be applied now.
Comment 29 Simon Thum 2007-05-28 09:08:13 UTC
Created attachment 10106 [details] [review]
revised patch against master

This version of the patch has some improvements:
 - it now has only one additional header
 - slow diagonal moves are measured more precise
 - no known memory leaks
 - improved structure so adding arbitrary acceleration functions should be easier to do in an ABI-compatible manner
Comment 30 Simon Thum 2007-05-28 09:21:35 UTC
Created attachment 10107 [details]
documentation
Comment 31 Simon Thum 2007-06-03 07:47:50 UTC
Created attachment 10162 [details] [review]
patch against xorg-server 1.2.0

This patch contains an improvement backported from the git version so slow diagonal movements are handled better. It is also tested with multiple devices (mouse and synaptics).
Comment 32 Simon Thum 2007-09-18 14:43:16 UTC
Created attachment 11617 [details] [review]
updated patch against current master

I've updated the patch to include profile selection, added a few acceleration profiles, and made the header structure more concise.
Comment 33 Simon Thum 2007-09-18 14:45:18 UTC
Created attachment 11618 [details]
updated documentation

updated documentation.
Comment 34 Peter Hutterer 2007-09-19 04:50:38 UTC
Can velocity data be freed through a callback function? Better than checking for the AccelSheme in CloseDevice. 

I did not understand why xf86InitValuatorAxisStruct only applies the new settings if there are zero axis? typo? 
It is better if you move this into InitValuatorAxisStruct, I guess xf86IVAS will die out over time.

aside from that, I like it. I haven't checked the actual algorithms for the acceleration though. 

One thing: hotplugging should be covered with your code once you move to IVAS instead of xf86IVAS. I recommend trying this scheme with both the mouse and the evdev driver. These are the most common ones in use.

Cosmetics:
#include "ptrveloc.h" instead of #include <ptrveloc.h>
Indendation seems screwed in ApplySofteningAndConstantDeceleration, ClassicProfile
Please avoid // comments
For javadoc-like comments preceding each line with an asterisk makes the code easier to read (admittedly subjectively)
/**
 * foobar
 */ 

ptrveloc.h: can you move the comments to be next to the declarations instead of above? It's horrendous to read right now, especially w/o syntax highlighting.
Comment 35 Simon Thum 2007-09-19 15:19:59 UTC
(In reply to comment #34)
> Can velocity data be freed through a callback function? Better than checking
> for the AccelSheme in CloseDevice. 
> 
> I did not understand why xf86InitValuatorAxisStruct only applies the new
> settings if there are zero axis? typo? 
> It is better if you move this into InitValuatorAxisStruct, I guess xf86IVAS
> will die out over time.
You've exactly hit the spots I consider suboptimal too :)
It's not a typo, but makes sure AEVS is called only once, as xf86IVAS is called once for each axis.
The reason I rely on all this is simply I need to evalute the config file, which makes that part viable only in xfree86 (AFAIK). I'm just not sure how to treat the issue such that I don't introduce more problems.
E.g. moving it in IVAS would make that fn take more parameters, and suddenly all its callers get involved. Any thoughts?

> aside from that, I like it. I haven't checked the actual algorithms for the
> acceleration though. 
I don't feel like a math whiz, but I'm pretty sure they're fine :) Basically I think there should be some research but I don't know who would pay _that_.

> One thing: hotplugging should be covered with your code once you move to IVAS
> instead of xf86IVAS. I recommend trying this scheme with both the mouse and the
> evdev driver. These are the most common ones in use.
Regarding hotplug, I have not idea how to deal with config here. A XI DeviceControl API is some work, as I've seen.

> Cosmetics:
When it compiles again :)
Comment 36 Peter Hutterer 2007-09-25 16:42:08 UTC
(In reply to comment #35)
> It's not a typo, but makes sure AEVS is called only once, as xf86IVAS is called
> once for each axis.
> The reason I rely on all this is simply I need to evalute the config file,
> which makes that part viable only in xfree86 (AFAIK). I'm just not sure how to
> treat the issue such that I don't introduce more problems.
> E.g. moving it in IVAS would make that fn take more parameters, and suddenly
> all its callers get involved. Any thoughts?

two options:
- you modify all the callers. depends on when you want it to be in the server. there's more input ABI breakage down the road anyway.
- you create a second interface InitValuatorAxisStructWithAccel() and let IVAS call IVASWA with sane defaults. this is probably the better idea for now.
 
> Regarding hotplug, I have not idea how to deal with config here. A XI
> DeviceControl API is some work, as I've seen.

not as much as you'd think. we (well, daniels preferrably :) can probably knock something up in quite short time. 
Comment 37 Simon Thum 2007-09-25 20:18:23 UTC
Created attachment 11756 [details] [review]
preliminary patch

I chose option three: I applied your suggestion on cleanup in CloseDevice to init, and from the dix/ddx decoupling perspective I am quite satisfied now. The patch *DOES NOT WORK* but it shows how I intend to do it. However I need to modify ActivateDecice() or something like that. Are there any objections?

I've seen there was some cleanup in the DeviceControl handling, maybe I'll take a a second chance an that. If we head for 1.5, there should be some time left, right?
Comment 38 Peter Hutterer 2007-10-15 17:40:44 UTC
(In reply to comment #37)
> Created an attachment (id=11756) [details]
> preliminary patch
> 
> I chose option three: I applied your suggestion on cleanup in CloseDevice to
> init, and from the dix/ddx decoupling perspective I am quite satisfied now. The
> patch *DOES NOT WORK* but it shows how I intend to do it. However I need to
> modify ActivateDecice() or something like that. Are there any objections?
> 

had a look at it on the last plane, looks good. One thing is left, which is more style than anything again: can you put accel stuff into a struct?
so that in the end we get something like
  dev->valuator->accell.clean() 
  dev->valuator->accell.init()
instead of 
  dev->valuator->AccelCleanupProc
  dev->valuator->AccelInitProc

I think it makes the code more readable. aside from that, I like it.
Comment 39 Simon Thum 2008-01-13 06:37:48 UTC
Created attachment 13692 [details] [review]
patch agaist X server
Comment 40 Simon Thum 2008-01-13 06:38:48 UTC
Created attachment 13693 [details] [review]
Xinput proto - needed for Xserver
Comment 41 Simon Thum 2008-01-13 06:40:32 UTC
Created attachment 13694 [details] [review]
lib/libXi: proof-of concept runtime parameter adjustment
Comment 42 Simon Thum 2008-01-13 06:41:34 UTC
Created attachment 13695 [details] [review]
app/xinput: proof-of concept runtime parameter adjustment
Comment 43 Simon Thum 2008-01-13 06:47:54 UTC
The structure is now OK, but changing parameters at runtime is not quite ready. It is however enough to try out and adjust the most important settings via the xinput tool. Comments encouraged!
Comment 44 Simon Thum 2008-01-13 09:26:28 UTC
Created attachment 13697 [details]
User documentation

updated documentation
Comment 45 Peter Hutterer 2008-01-17 00:58:43 UTC
(In reply to comment #39)
> Created an attachment (id=13692) [details]
> patch agaist X server
> 

This patch is a bit messy.

CopySwapDeviceAccel() shouldn't even compile due to a stray "e->operation"

SendErrorToClient is not used in current master anymore. just BadMatch directly.

InitDefaultPointerAccelerationScheme(): you could just move the three lines to InitValuatorClassDeviceStruct and just have InitPointerAccelerationScheme. Or move them to the latter and have it as the default action if NULLs are provided.

what also makes me wonder is the high amount of commented lines. if they're not needed, just remove them.

can you fix these things please, then I can have another look at it.
Comment 46 Peter Hutterer 2008-01-17 01:03:29 UTC
(In reply to comment #40)
> Created an attachment (id=13693) [details]
> Xinput proto - needed for Xserver
> 

I'd be better if DEVICE_PTRACCEL only takes up one spot and then you have a sub-variable for the scheme in the struct.

The range from DEVICE_PTRACCEL to DEVICE_PTRACCEL_MAX is a bit messy. 
Comment 47 Simon Thum 2008-01-18 02:21:53 UTC
Created attachment 13771 [details] [review]
patch for Xserver

I'm sorry!

I accidentally uploaded an old development snapshot of the Xserver patch. I meant this one :) Sorry for wasting your time with this!

> I'd be better if DEVICE_PTRACCEL only takes up one spot and then you have a
sub-variable for the scheme in the struct.

> The range from DEVICE_PTRACCEL to DEVICE_PTRACCEL_MAX is a bit messy.

Agreed. It was like this in the first place. I changed the design in order to support getting parameters (GetDeviceControl doesn't upload a struct to the server) plus keeping the possibility to get/set (rather) arbitrary parameters.

I thought it to be better than a loaded struct from which something will always be missing, especially if someone draws up additional schemes or the algorithm gets new tweaks. Maybe there is another way to do it?
Comment 48 Peter Hutterer 2008-01-18 03:09:52 UTC
(In reply to comment #47) 
> > I'd be better if DEVICE_PTRACCEL only takes up one spot and then you have a
> sub-variable for the scheme in the struct.
> 
> > The range from DEVICE_PTRACCEL to DEVICE_PTRACCEL_MAX is a bit messy.
> 
> Agreed. It was like this in the first place. I changed the design in order to
> support getting parameters (GetDeviceControl doesn't upload a struct to the
> server) plus keeping the possibility to get/set (rather) arbitrary parameters.
> 
> I thought it to be better than a loaded struct from which something will always
> be missing, especially if someone draws up additional schemes or the algorithm
> gets new tweaks. Maybe there is another way to do it?

there is a simple reason why the current approach is unsuitable. you're essentially making anybody else who may need to add a device ctrl dependent on your DEVICE_PTRACCEL_MAX. 
Now in the worst case s.b. adds a DevCtl as MAX + 1. Clients depend on it and then when you add an accel scheme suddenly the absolute value of MAX + 1 has changed. This is bad!
Remember, #defines are compiled in so once they are in binary you can't change them anymore w/o recompilation.

I'm also not sure why the current approach gets around the problem of an "incomplete" struct. Maybe I'm misunderstanding you here?
What is wrong with

typedef struct {
    XID            control; /* DEVIC_PTRACCEL */
    int            length;
    int            type;
} XDeviceAccelerationControl, XDeviceAccelerationState;

for the default, and then depending on the type you have a struct for each accel scheme that needs it. type determines which struct, and since length is in bytes anyway, nothing breaks by adding new schemes.


I'll have a look at the server patch but the inputproto stuff in its current state is a dealbreaker.
Comment 49 Simon Thum 2008-01-18 04:44:02 UTC
(In reply to comment #48)
> typedef struct {
>     XID            control; /* DEVIC_PTRACCEL */
>     int            length;
>     int            type;
> } XDeviceAccelerationControl, XDeviceAccelerationState;
> 
> for the default, and then depending on the type you have a struct for each
> accel scheme that needs it. type determines which struct, and since length is
> in bytes anyway, nothing breaks by adding new schemes.
Looks good, absolutely on option. Let's call it option (1).
What it doesn't do is provide a simple way to add a parameter. You'll have to modify libXi, inputproto and modify marshalling (client and server). The current approach provides simple end-to-end transport - and by reserving a range incl. surplus we could be ABI safe too. (2)

OTOH, I'm not sure if it is desirable to have this ability at all.

> I'll have a look at the server patch but the inputproto stuff in its current
> state is a dealbreaker.
Would that still hold if the set of parameters was considerably smaller and with fixed codes assigned (and of course not resorting to range checks in switch() default: paths) ?
This would however mean not every knob is tweakable, a thing I'm fine with. (3)

Since libXi is yours, I'll leave the choice to you. My vote is for (2).
Comment 50 Peter Hutterer 2008-01-20 22:56:09 UTC
(In reply to comment #49)
> (In reply to comment #48)
> > typedef struct {
> >     XID            control; /* DEVIC_PTRACCEL */
> >     int            length;
> >     int            type;
> > } XDeviceAccelerationControl, XDeviceAccelerationState;
> > 
> > for the default, and then depending on the type you have a struct for each
> > accel scheme that needs it. type determines which struct, and since length is
> > in bytes anyway, nothing breaks by adding new schemes.
> Looks good, absolutely on option. Let's call it option (1).
> What it doesn't do is provide a simple way to add a parameter. You'll have to
> modify libXi, inputproto and modify marshalling (client and server). The
> current approach provides simple end-to-end transport - and by reserving a
> range incl. surplus we could be ABI safe too. (2)

Ah. now I finally get it. I interpreted the parameters as different accel schemes rather than parameters for the same scheme. 

Nevertheless, my proposal still holds. The length field is 16 bit, which should be enough to stack device controls. Something along the line of 

typedef struct {
     XID            control; /* DEVIC_PTRACCEL */
     int            length;
     int            type; /* which accel scheme */
     int            parameters; /* number of parameters following */
} XDeviceAccelerationControl, XDeviceAccelerationState;

parameters simply states how many of the following.
length encloses the whole control (inc. all parameters) to be compatible with older versions. 

typedef struct {
   int param_type;
   int length;
} XAccelParameterControl;

and then you have multiple versions of

typedef struct {
    int          param_type; /* DevicePtrAccel_Coeff /*
    int          length;
    int            integer;
    int            rational_num;
    int            rational_den;
} XDevicePtrAccelCoeff;


this is a similar approach to what the ListInputDevices reply does with the classes.

advantages: 
 we don't mess up the CONTROL range,
 we have _a huge_ range of parameters. 
 the type field allows for multiple different accel types
 parameters are re-usable.

and 16 bit for the length should be enough for even multiple parameters.

does that make sense? any comments?


Comment 51 Simon Thum 2008-02-09 17:58:38 UTC
Created attachment 14254 [details] [review]
patch: X server
Comment 52 Simon Thum 2008-02-09 17:59:10 UTC
Created attachment 14255 [details] [review]
patch: libXi
Comment 53 Simon Thum 2008-02-09 17:59:38 UTC
Created attachment 14256 [details] [review]
patch: xinput tool
Comment 54 Simon Thum 2008-02-09 18:00:14 UTC
Created attachment 14257 [details] [review]
patch: Xinput proto
Comment 55 Simon Thum 2008-02-09 18:04:08 UTC
Created attachment 14258 [details]
Documentation

The XDeviceConrol interface has been reworked and should conform better now. The server-side implementation is considered complete. xinput has some rough edges but is useable.
Comment 56 Daniel Stone 2008-04-01 06:10:53 UTC
FWIW, my final intention for this is 1.6, once I've finished the property code, since I'm loathe to add an API and then swiftly remove it.  Currently working on XKB stuff, but should be back on to Xi by the end of the month.  I don't have any real concerns with the patches, so any distros that want to merge this are welcome to.
Comment 57 Simon Thum 2008-07-04 18:15:00 UTC
Created attachment 17544 [details] [review]
patch against X server

This patch should apply against current git.
Small changelog:
-A minimal interface for device drivers has been added (fully optional)
-The velocity guessing is cleaner and potentially even more precise
-the runtime parameter changing stuff has been removed
-'classic' compatibility functions are now selectable as own profiles
Comment 58 Simon Thum 2008-07-04 18:18:20 UTC
Created attachment 17545 [details]
documentation

the user-unfriendly documentation ;)
might miss a few bits, but correct in general
Comment 59 Simon Thum 2008-07-10 14:10:14 UTC
Created attachment 17626 [details] [review]
revised patch

didn't include important parts, this patch should be complete now.
Comment 60 Peter Hutterer 2008-07-13 04:32:16 UTC
Pushed as c9eb0e870c87d291311491452adf7f91a911e24b
Comment 61 Simon Thum 2008-07-20 13:42:44 UTC
Created attachment 17772 [details]
updated developer documentation

The attached doc should match with current git servers.

Contrary to what some people linking here stated, this effort was not commited until recently. This makes it a likely item in X11R7.5.

Until then, you have to use the patches here if you want it.
Comment 62 Simon Thum 2008-07-20 13:46:46 UTC
Created attachment 17773 [details] [review]
patch for Xorg server 1.4.x

This patch should apply against 1.4 servers.