Re: Stairstep mouse motion
On Sat, Oct 26, 2013 at 10:13:11PM +0600, Alexandr Shadchin wrote: On Fri, Oct 25, 2013 at 11:41:25AM +0100, Edd Barrett wrote: On Thu, Oct 24, 2013 at 10:33:22PM +0300, Henri Kemppainen wrote: What happens when priv-swap_axes is set, and the ax ay branch is taken along with the wsWheelEmuFilterMotion() branch. Following continue another event is processed and now the axes are swapped again (ax and ay were not reset after use) and then what? Not very likely I know. Ah, yes, there is the possibility of posting an inconsistent pointer sample in this case. Perhaps we should only update the old_ax/old_ay if the wsWheelEmuFilterMotion branch is not taken? What do you think? And yes, this is a very very unlikely case. You could argue it wouldn't matter even if it did happen. -- Best Regards Edd Barrett http://www.theunixzoo.co.uk Alternative solution without extra magic (need rebuild kernel). Before (on example pms(4)): * user move mouse * pms(4) read state mouse and process it * pms(4) send dx, dy and buttons in wscons * wscons generate simple events * ws(4) reads one event and process it immediately After applying diff: * user move mouse * pms(4) read state mouse and process it * pms(4) send dx, dy and buttons in wscons * wscons generate simple events and adds SYNC event * ws(4) reads events until it receives SYNC, and only then begins processing Tested on mouse. Comments ? PS: synaptics(4) is working on a similar basis -- Alexandr Shadchin I've tested this diff with some hours of QuakeWorld, an old and very fast/demanding FPS game. Works well. This staircase fix + KMS have improved these types of games on OpenBSD a lot. However, mouse input still isn't there yet, it still feels slightly delayed and not precise enough compared to running the same game and game client on Linux. This is only subtle and will most likely not be noticed without playing quite a lot though. Modern Quake engines rely on high sleep granuality so I always have to rip out any usleep from the clients since in OpenBSD atm you can't sleep below tic rate (roughly 1/100 sec on my machine) AFAIK. This may add to the aforementioned felt latency on input. Alf OpenBSD 5.4-current (GENERIC.MP) #0: Tue Oct 29 14:32:26 CET 2013 r...@obsd.rechners.lemarit.com:/usr/src/sys/arch/amd64/compile/GENERIC.MP real mem = 8555986944 (8159MB) avail mem = 8320094208 (7934MB) mainbus0 at root bios0 at mainbus0: SMBIOS rev. 2.4 @ 0xf0100 (37 entries) bios0: vendor Award Software International, Inc. version F9 date 03/21/2012 bios0: Gigabyte Technology Co., Ltd. PH67A-UD3-B3 acpi0 at bios0: rev 0 acpi0: sleep states S0 S3 S4 S5 acpi0: tables DSDT FACP MSDM HPET MCFG ASPT SSPT EUDS MATS TAMG APIC SSDT MATS acpi0: wakeup devices PEX0(S5) PEX1(S5) PEX2(S5) PEX3(S5) PEX4(S5) PEX5(S5) PEX6(S5) PEX7(S5) HUB0(S5) UAR1(S3) USBE(S3) USE2(S3) AZAL(S5) PCI0(S5) acpitimer0 at acpi0: 3579545 Hz, 24 bits acpihpet0 at acpi0: 14318179 Hz acpimcfg0 at acpi0 addr 0xf400, bus 0-63 acpimadt0 at acpi0 addr 0xfee0: PC-AT compat cpu0 at mainbus0: apid 0 (boot processor) cpu0: Intel(R) Core(TM) i5-2500 CPU @ 3.30GHz, 3396.09 MHz cpu0: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,POPCNT,DEADLINE,AES,XSAVE,AVX,NXE,LONG,LAHF,PERF,ITSC cpu0: 256KB 64b/line 8-way L2 cache cpu0: smt 0, core 0, package 0 cpu0: apic clock running at 99MHz cpu0: mwait min=64, max=64, C-substates=0.2.1.1.0, IBE cpu1 at mainbus0: apid 2 (application processor) cpu1: Intel(R) Core(TM) i5-2500 CPU @ 3.30GHz, 3395.56 MHz cpu1: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,POPCNT,DEADLINE,AES,XSAVE,AVX,NXE,LONG,LAHF,PERF,ITSC cpu1: 256KB 64b/line 8-way L2 cache cpu1: smt 0, core 1, package 0 cpu2 at mainbus0: apid 4 (application processor) cpu2: Intel(R) Core(TM) i5-2500 CPU @ 3.30GHz, 3395.56 MHz cpu2: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,POPCNT,DEADLINE,AES,XSAVE,AVX,NXE,LONG,LAHF,PERF,ITSC cpu2: 256KB 64b/line 8-way L2 cache cpu2: smt 0, core 2, package 0 cpu3 at mainbus0: apid 6 (application processor) cpu3: Intel(R) Core(TM) i5-2500 CPU @ 3.30GHz, 3395.56 MHz cpu3: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,POPCNT,DEADLINE,AES,XSAVE,AVX,NXE,LONG,LAHF,PERF,ITSC cpu3: 256KB 64b/line 8-way L2 cache cpu3: smt 0, core 3, package 0
Re: Stairstep mouse motion
On Tue, Oct 29, 2013 at 11:07:29PM +0600, Alexandr Shadchin wrote: Look good to me. However I've a concern about compatibility with NetBSD. The kernel change should be documented in the commit message for xf86-input-ws so that they can catch up with the kernel change before they update xf86-input-ws... Update diff (add small hack for NetBSD). Having spent ten minutes scribbling in xournal using my stylus, I can say that this appears to work well. Diagonal lines are smooth even when drawn fast. ws.c is looking much tidier too. If the others are happy with the kernel portion of your diff, then I reckon this is good to go in. -- Best Regards Edd Barrett http://www.theunixzoo.co.uk
Re: Stairstep mouse motion
On Tue, Oct 29, 2013 at 11:07:29PM +0600, Alexandr Shadchin wrote:
Update diff (add small hack for NetBSD).
Thanks. This (plus the kernel diff) is ok matthieu@.
--
Alexandr Shadchin
Index: ws.c
===
RCS file: /cvs/xenocara/driver/xf86-input-ws/src/ws.c,v
retrieving revision 1.58
diff -u -p -r1.58 ws.c
--- ws.c 20 Jul 2013 13:24:50 - 1.58
+++ ws.c 29 Oct 2013 16:57:05 -
@@ -428,13 +428,6 @@ wsDeviceOn(DeviceIntPtr pWS)
}
}
}
- priv-buffer = XisbNew(pInfo-fd,
- sizeof(struct wscons_event) * NUMEVENTS);
- if (priv-buffer == NULL) {
- xf86IDrvMsg(pInfo, X_ERROR, cannot alloc xisb buffer\n);
- wsClose(pInfo);
- return !Success;
- }
xf86AddEnabledDevice(pInfo);
wsmbEmuOn(pInfo);
pWS-public.on = TRUE;
@@ -462,170 +455,157 @@ wsDeviceOff(DeviceIntPtr pWS)
xf86RemoveEnabledDevice(pInfo);
wsClose(pInfo);
}
- if (priv-buffer) {
- XisbFree(priv-buffer);
- priv-buffer = NULL;
- }
pWS-public.on = FALSE;
}
-static void
-wsReadInput(InputInfoPtr pInfo)
+static Bool
+wsReadEvent(InputInfoPtr pInfo, struct wscons_event *event)
{
- WSDevicePtr priv = (WSDevicePtr)pInfo-private;
- static struct wscons_event eventList[NUMEVENTS];
- int n, c, dx, dy;
- struct wscons_event *event = eventList;
- unsigned char *pBuf;
-
- XisbBlockDuration(priv-buffer, -1);
- pBuf = (unsigned char *)eventList;
- n = 0;
- while (n sizeof(eventList) (c = XisbRead(priv-buffer)) = 0) {
- pBuf[n++] = (unsigned char)c;
+ Bool rc = TRUE;
+ ssize_t len;
+
+ len = read(pInfo-fd, event, sizeof(struct wscons_event));
+ if (len = 0) {
+ if (errno != EAGAIN)
+ xf86IDrvMsg(pInfo, X_ERROR, read error %s\n,
+ strerror(errno));
+ rc = FALSE;
+ } else if (len != sizeof(struct wscons_event)) {
+ xf86IDrvMsg(pInfo, X_ERROR,
+ read error, invalid number of bytes\n);
+ rc = FALSE;
}
- if (n == 0)
- return;
+ return rc;
+}
- dx = dy = 0;
- n /= sizeof(struct wscons_event);
- while (n--) {
- int buttons = priv-lastButtons;
- int newdx = 0, newdy = 0, dz = 0, dw = 0, ax = 0, ay = 0;
- int zbutton = 0, wbutton = 0;
+static Bool
+wsReadHwState(InputInfoPtr pInfo, wsHwState *hw)
+{
+ WSDevicePtr priv = (WSDevicePtr)pInfo-private;
+ struct wscons_event event;
+
+ bzero(hw, sizeof(wsHwState));
+
+ hw-buttons = priv-lastButtons;
+ hw-ax = priv-old_ax;
+ hw-ay = priv-old_ay;
- switch (event-type) {
+ while (wsReadEvent(pInfo, event)) {
+ switch (event.type) {
case WSCONS_EVENT_MOUSE_UP:
- buttons = ~(1 event-value);
- DBG(4, ErrorF(Button %d up %x\n, event-value,
- buttons));
+ hw-buttons = ~(1 event.value);
+ DBG(4, ErrorF(Button %d up %x\n, event.value,
+ hw-buttons));
break;
case WSCONS_EVENT_MOUSE_DOWN:
- buttons |= (1 event-value);
- DBG(4, ErrorF(Button %d down %x\n, event-value,
- buttons));
+ hw-buttons |= (1 event.value);
+ DBG(4, ErrorF(Button %d down %x\n, event.value,
+ hw-buttons));
break;
case WSCONS_EVENT_MOUSE_DELTA_X:
- if (!dx)
- dx = event-value;
- else
- newdx = event-value;
- DBG(4, ErrorF(Relative X %d\n, event-value));
+ hw-dx = event.value;
+ DBG(4, ErrorF(Relative X %d\n, event.value));
break;
case WSCONS_EVENT_MOUSE_DELTA_Y:
- if (!dy)
- dy = -event-value;
- else
- newdy = -event-value;
- DBG(4, ErrorF(Relative Y %d\n, event-value));
+ hw-dy = -event.value;
+ DBG(4, ErrorF(Relative Y %d\n, event.value));
+ break;
+ case WSCONS_EVENT_MOUSE_DELTA_Z:
+ hw-dz = event.value;
+ DBG(4, ErrorF(Relative Z %d\n, event.value));
+ break;
+ case WSCONS_EVENT_MOUSE_DELTA_W:
+ hw-dw = event.value;
+ DBG(4, ErrorF(Relative W %d\n,
Re: Stairstep mouse motion
On Sun, Oct 27, 2013 at 04:31:37PM +0100, Matthieu Herrb wrote:
On Sat, Oct 26, 2013 at 10:13:11PM +0600, Alexandr Shadchin wrote:
On Fri, Oct 25, 2013 at 11:41:25AM +0100, Edd Barrett wrote:
On Thu, Oct 24, 2013 at 10:33:22PM +0300, Henri Kemppainen wrote:
What happens when priv-swap_axes is set, and the ax ay branch is
taken along with the wsWheelEmuFilterMotion() branch. Following
continue another event is processed and now the axes are swapped again
(ax and ay were not reset after use) and then what? Not very likely
I know.
Ah, yes, there is the possibility of posting an inconsistent pointer
sample
in this case. Perhaps we should only update the old_ax/old_ay if the
wsWheelEmuFilterMotion branch is not taken?
What do you think? And yes, this is a very very unlikely case. You could
argue it wouldn't matter even if it did happen.
--
Best Regards
Edd Barrett
http://www.theunixzoo.co.uk
Alternative solution without extra magic (need rebuild kernel).
Before (on example pms(4)):
* user move mouse
* pms(4) read state mouse and process it
* pms(4) send dx, dy and buttons in wscons
* wscons generate simple events
* ws(4) reads one event and process it immediately
After applying diff:
* user move mouse
* pms(4) read state mouse and process it
* pms(4) send dx, dy and buttons in wscons
* wscons generate simple events and adds SYNC event
* ws(4) reads events until it receives SYNC, and only then begins processing
Tested on mouse.
Comments ?
Look good to me. However I've a concern about compatibility with
NetBSD. The kernel change should be documented in the commit message
for xf86-input-ws so that they can catch up with the kernel change
before they update xf86-input-ws...
Update diff (add small hack for NetBSD).
--
Alexandr Shadchin
Index: ws.c
===
RCS file: /cvs/xenocara/driver/xf86-input-ws/src/ws.c,v
retrieving revision 1.58
diff -u -p -r1.58 ws.c
--- ws.c20 Jul 2013 13:24:50 - 1.58
+++ ws.c29 Oct 2013 16:57:05 -
@@ -428,13 +428,6 @@ wsDeviceOn(DeviceIntPtr pWS)
}
}
}
- priv-buffer = XisbNew(pInfo-fd,
- sizeof(struct wscons_event) * NUMEVENTS);
- if (priv-buffer == NULL) {
- xf86IDrvMsg(pInfo, X_ERROR, cannot alloc xisb buffer\n);
- wsClose(pInfo);
- return !Success;
- }
xf86AddEnabledDevice(pInfo);
wsmbEmuOn(pInfo);
pWS-public.on = TRUE;
@@ -462,170 +455,157 @@ wsDeviceOff(DeviceIntPtr pWS)
xf86RemoveEnabledDevice(pInfo);
wsClose(pInfo);
}
- if (priv-buffer) {
- XisbFree(priv-buffer);
- priv-buffer = NULL;
- }
pWS-public.on = FALSE;
}
-static void
-wsReadInput(InputInfoPtr pInfo)
+static Bool
+wsReadEvent(InputInfoPtr pInfo, struct wscons_event *event)
{
- WSDevicePtr priv = (WSDevicePtr)pInfo-private;
- static struct wscons_event eventList[NUMEVENTS];
- int n, c, dx, dy;
- struct wscons_event *event = eventList;
- unsigned char *pBuf;
-
- XisbBlockDuration(priv-buffer, -1);
- pBuf = (unsigned char *)eventList;
- n = 0;
- while (n sizeof(eventList) (c = XisbRead(priv-buffer)) = 0) {
- pBuf[n++] = (unsigned char)c;
+ Bool rc = TRUE;
+ ssize_t len;
+
+ len = read(pInfo-fd, event, sizeof(struct wscons_event));
+ if (len = 0) {
+ if (errno != EAGAIN)
+ xf86IDrvMsg(pInfo, X_ERROR, read error %s\n,
+ strerror(errno));
+ rc = FALSE;
+ } else if (len != sizeof(struct wscons_event)) {
+ xf86IDrvMsg(pInfo, X_ERROR,
+ read error, invalid number of bytes\n);
+ rc = FALSE;
}
- if (n == 0)
- return;
+ return rc;
+}
- dx = dy = 0;
- n /= sizeof(struct wscons_event);
- while (n--) {
- int buttons = priv-lastButtons;
- int newdx = 0, newdy = 0, dz = 0, dw = 0, ax = 0, ay = 0;
- int zbutton = 0, wbutton = 0;
+static Bool
+wsReadHwState(InputInfoPtr pInfo, wsHwState *hw)
+{
+ WSDevicePtr priv = (WSDevicePtr)pInfo-private;
+ struct wscons_event event;
+
+ bzero(hw, sizeof(wsHwState));
+
+ hw-buttons = priv-lastButtons;
+ hw-ax = priv-old_ax;
+ hw-ay = priv-old_ay;
- switch (event-type) {
+ while (wsReadEvent(pInfo, event)) {
+ switch (event.type) {
case WSCONS_EVENT_MOUSE_UP:
- buttons = ~(1 event-value);
- DBG(4, ErrorF(Button %d up %x\n, event-value,
- buttons));
+ hw-buttons =
Re: Stairstep mouse motion
On Fri, Oct 25, 2013 at 11:41:25AM +0100, Edd Barrett wrote:
On Thu, Oct 24, 2013 at 10:33:22PM +0300, Henri Kemppainen wrote:
What happens when priv-swap_axes is set, and the ax ay branch is
taken along with the wsWheelEmuFilterMotion() branch. Following
continue another event is processed and now the axes are swapped again
(ax and ay were not reset after use) and then what? Not very likely
I know.
Ah, yes, there is the possibility of posting an inconsistent pointer sample
in this case. Perhaps we should only update the old_ax/old_ay if the
wsWheelEmuFilterMotion branch is not taken?
What do you think? And yes, this is a very very unlikely case. You could
argue it wouldn't matter even if it did happen.
--
Best Regards
Edd Barrett
http://www.theunixzoo.co.uk
Alternative solution without extra magic (need rebuild kernel).
Before (on example pms(4)):
* user move mouse
* pms(4) read state mouse and process it
* pms(4) send dx, dy and buttons in wscons
* wscons generate simple events
* ws(4) reads one event and process it immediately
After applying diff:
* user move mouse
* pms(4) read state mouse and process it
* pms(4) send dx, dy and buttons in wscons
* wscons generate simple events and adds SYNC event
* ws(4) reads events until it receives SYNC, and only then begins processing
Tested on mouse.
Comments ?
PS:
synaptics(4) is working on a similar basis
--
Alexandr Shadchin
Index: dev/pckbc/pms.c
===
RCS file: /cvs/src/sys/dev/pckbc/pms.c,v
retrieving revision 1.48
diff -u -p -r1.48 pms.c
--- dev/pckbc/pms.c 20 Sep 2013 14:07:30 - 1.48
+++ dev/pckbc/pms.c 26 Oct 2013 15:09:53 -
@@ -1155,8 +1155,7 @@ pms_proc_synaptics(struct pms_softc *sc)
if (syn-wsmode == WSMOUSE_NATIVE) {
wsmouse_input(sc-sc_wsmousedev, buttons, x, y, z, w,
WSMOUSE_INPUT_ABSOLUTE_X | WSMOUSE_INPUT_ABSOLUTE_Y |
- WSMOUSE_INPUT_ABSOLUTE_Z | WSMOUSE_INPUT_ABSOLUTE_W |
- WSMOUSE_INPUT_SYNC);
+ WSMOUSE_INPUT_ABSOLUTE_Z | WSMOUSE_INPUT_ABSOLUTE_W);
} else {
dx = dy = 0;
if (z SYNAPTICS_PRESSURE) {
@@ -1470,8 +1469,7 @@ pms_proc_alps(struct pms_softc *sc)
wsmouse_input(sc-sc_wsmousedev, buttons, x, y, z, w,
WSMOUSE_INPUT_ABSOLUTE_X | WSMOUSE_INPUT_ABSOLUTE_Y |
- WSMOUSE_INPUT_ABSOLUTE_Z | WSMOUSE_INPUT_ABSOLUTE_W |
- WSMOUSE_INPUT_SYNC);
+ WSMOUSE_INPUT_ABSOLUTE_Z | WSMOUSE_INPUT_ABSOLUTE_W);
alps-old_fin = fin;
} else {
@@ -2321,8 +2319,7 @@ elantech_send_input(struct pms_softc *sc
WSMOUSE_INPUT_ABSOLUTE_X |
WSMOUSE_INPUT_ABSOLUTE_Y |
WSMOUSE_INPUT_ABSOLUTE_Z |
- WSMOUSE_INPUT_ABSOLUTE_W |
- WSMOUSE_INPUT_SYNC);
+ WSMOUSE_INPUT_ABSOLUTE_W);
} else {
dx = dy = 0;
Index: dev/wscons/wsmouse.c
===
RCS file: /cvs/src/sys/dev/wscons/wsmouse.c,v
retrieving revision 1.24
diff -u -p -r1.24 wsmouse.c
--- dev/wscons/wsmouse.c18 Oct 2013 13:54:09 - 1.24
+++ dev/wscons/wsmouse.c26 Oct 2013 15:09:55 -
@@ -452,13 +452,11 @@ wsmouse_input(struct device *wsmousedev,
ub ^= d;
}
- if (flags WSMOUSE_INPUT_SYNC) {
- NEXT;
- ev-type = WSCONS_EVENT_SYNC;
- ev-value = 0;
- TIMESTAMP;
- ADVANCE;
- }
+ NEXT;
+ ev-type = WSCONS_EVENT_SYNC;
+ ev-value = 0;
+ TIMESTAMP;
+ ADVANCE;
/* XXX fake wscons_event notifying wsmoused(8) to close mouse device */
if (flags WSMOUSE_INPUT_WSMOUSED_CLOSE) {
Index: dev/wscons/wsmousevar.h
===
RCS file: /cvs/src/sys/dev/wscons/wsmousevar.h,v
retrieving revision 1.6
diff -u -p -r1.6 wsmousevar.h
--- dev/wscons/wsmousevar.h 22 Jul 2012 18:28:36 - 1.6
+++ dev/wscons/wsmousevar.h 26 Oct 2013 15:09:55 -
@@ -74,7 +74,6 @@ int wsmousedevprint(void *, const char *
#define WSMOUSE_INPUT_WSMOUSED_CLOSE (13) /* notify wsmoused(8) to close
mouse device */
#define WSMOUSE_INPUT_ABSOLUTE_W (14)
-#define WSMOUSE_INPUT_SYNC (15)
void wsmouse_input(struct device *kbddev, u_int btns,
int x, int y, int z, int w, u_int flags);
Index: ws.c
===
RCS file: /cvs/xenocara/driver/xf86-input-ws/src/ws.c,v
retrieving revision 1.58
diff -u -p -r1.58 ws.c
--- ws.c20 Jul 2013 13:24:50 - 1.58
+++ ws.c26 Oct 2013 16:03:01 -
@@
Re: Stairstep mouse motion
From: Alexandr Shadchin Before (on example pms(4)): * user move mouse * pms(4) read state mouse and process it * pms(4) send dx, dy and buttons in wscons * wscons generate simple events * ws(4) reads one event and process it immediately After applying diff: * user move mouse * pms(4) read state mouse and process it * pms(4) send dx, dy and buttons in wscons * wscons generate simple events and adds SYNC event * ws(4) reads events until it receives SYNC, and only then begins processing Tested on mouse. Comments ? PS: synaptics(4) is working on a similar basis Absolutely yes for this. This is one of the approaches I originally considered, but then feared it'd be too intrusive. I didn't realize WS_INPUT_SYNC was already a thing and that we're doing this with synaptics. This'll also fix another downside which I mentioned with the previous approach (that it could join two unrelated x y events if they follow each other). I'm busy crossing the time_t chasm and compiling ports because the packages on mirrors haven't gotten across the libstdc++ bump (damnit). I'll try take a better look at your diff and test it tomorrow.
Re: Stairstep mouse motion
On Thu, Oct 24, 2013 at 10:33:22PM +0300, Henri Kemppainen wrote: What happens when priv-swap_axes is set, and the ax ay branch is taken along with the wsWheelEmuFilterMotion() branch. Following continue another event is processed and now the axes are swapped again (ax and ay were not reset after use) and then what? Not very likely I know. Ah, yes, there is the possibility of posting an inconsistent pointer sample in this case. Perhaps we should only update the old_ax/old_ay if the wsWheelEmuFilterMotion branch is not taken? What do you think? And yes, this is a very very unlikely case. You could argue it wouldn't matter even if it did happen. -- Best Regards Edd Barrett http://www.theunixzoo.co.uk
Re: Stairstep mouse motion
On Wed, Oct 16, 2013 at 11:45:34PM +0100, Edd Barrett wrote: Tested on my x230t and will continue to test. No regrssions noticed on relative pointing devices. OK? Anyone? I appreciate that I am probably the only one using OpenBSD on a tablet, but a looks OK and no regressions for relative pointing devices would be great. Keen to get this in so that I don't have to rebuild the ws driver every time I update my snapshot. -- Best Regards Edd Barrett http://www.theunixzoo.co.uk
Re: Stairstep mouse motion
Tested on my x230t and will continue to test. No regrssions noticed on relative pointing devices. OK? Anyone? I appreciate that I am probably the only one using OpenBSD on a tablet, but a looks OK and no regressions for relative pointing devices would be great. What happens when priv-swap_axes is set, and the ax ay branch is taken along with the wsWheelEmuFilterMotion() branch. Following continue another event is processed and now the axes are swapped again (ax and ay were not reset after use) and then what? Not very likely I know. In hindsight it's possible that wheel emulation has been broken in the previous revision; a glance at wsWheelEmuFilterMotion suggests it doesn't like to handle both x and y axes at once, and will only do the former (resetting the latter) if both are set. I've never used this thing though, and I didn't dive deeper. Other than that I guess your diff is about as reasonable as one can be with this hairy code. I was waiting and hoping someone else with an absolute pointing device could've checked and tested it because I really didn't enjoy working on this file when I did my patch :-)
Re: Stairstep mouse motion
On Thu, Jul 18, 2013 at 09:23:00PM +0100, Edd Barrett wrote:
After applying your diff:
Touchpad: smooth lines.
Nipple: smooth lines.
Pen: jagged lines.
I wonder if it is because the pen is an absolute pointing device. You
probably need extra magic in the WSCONS_EVENT_MOUSE_ABSOLUTE_* cases.
The following diff fixes the jagged diagonal lines for absolute pointing
devices. I think I might be able to simplify some of the relative pointer
code here too, but that should be a separate diff.
Tested on my x230t and will continue to test. No regrssions noticed on
relative pointing devices.
OK?
Index: src/ws.c
===
RCS file: /home/edd/cvsync/cvs/xenocara/driver/xf86-input-ws/src/ws.c,v
retrieving revision 1.58
diff -u -p -u -r1.58 ws.c
--- src/ws.c20 Jul 2013 13:24:50 - 1.58
+++ src/ws.c16 Oct 2013 22:36:21 -
@@ -474,7 +474,7 @@ wsReadInput(InputInfoPtr pInfo)
{
WSDevicePtr priv = (WSDevicePtr)pInfo-private;
static struct wscons_event eventList[NUMEVENTS];
- int n, c, dx, dy;
+ int n, c, dx, dy, ax, ay;
struct wscons_event *event = eventList;
unsigned char *pBuf;
@@ -488,11 +488,11 @@ wsReadInput(InputInfoPtr pInfo)
if (n == 0)
return;
- dx = dy = 0;
+ dx = dy = ax = ay = 0;
n /= sizeof(struct wscons_event);
while (n--) {
int buttons = priv-lastButtons;
- int newdx = 0, newdy = 0, dz = 0, dw = 0, ax = 0, ay = 0;
+ int newdx = 0, newdy = 0, dz = 0, dw = 0;
int zbutton = 0, wbutton = 0;
switch (event-type) {
@@ -595,25 +595,17 @@ wsReadInput(InputInfoPtr pInfo)
ax = ay;
ay = tmp;
}
- if (ax) {
+ if (ax ay) {
int xdelta = ax - priv-old_ax;
- priv-old_ax = ax;
- if (wsWheelEmuFilterMotion(pInfo, xdelta, 0))
- continue;
-
- /* absolute position event */
- DBG(3, ErrorF(postMotionEvent X %d\n, ax));
- xf86PostMotionEvent(pInfo-dev, 1, 0, 1, ax);
- }
- if (ay) {
int ydelta = ay - priv-old_ay;
+ priv-old_ax = ax;
priv-old_ay = ay;
- if (wsWheelEmuFilterMotion(pInfo, 0, ydelta))
+ if (wsWheelEmuFilterMotion(pInfo, xdelta, ydelta))
continue;
-
/* absolute position event */
- DBG(3, ErrorF(postMotionEvent y %d\n, ay));
- xf86PostMotionEvent(pInfo-dev, 1, 1, 1, ay);
+ DBG(3, ErrorF(postMotionEvent X %d Y %d\n, ax, ay));
+ xf86PostMotionEvent(pInfo-dev, 1, 0, 2, ax, ay);
+ ax = ay = 0; /* prevent second post below */
}
}
if (dx || dy) {
@@ -624,6 +616,24 @@ wsReadInput(InputInfoPtr pInfo)
DBG(3, ErrorF(postMotionEvent dX %d dY %d\n,
dx, dy));
xf86PostMotionEvent(pInfo-dev, 0, 0, 2, dx, dy);
+ }
+ if (ax) {
+ int xdelta = ax - priv-old_ax;
+ priv-old_ax = ax;
+ if (wsWheelEmuFilterMotion(pInfo, xdelta, 0))
+ return;
+ /* absolute position event */
+ DBG(3, ErrorF(postMotionEvent X %d\n, ax));
+ xf86PostMotionEvent(pInfo-dev, 1, 0, 1, ax);
+ }
+ if (ay) {
+ int ydelta = ay - priv-old_ay;
+ priv-old_ay = ay;
+ if (wsWheelEmuFilterMotion(pInfo, 0, ydelta))
+ return;
+ /* absolute position event */
+ DBG(3, ErrorF(postMotionEvent Y %d\n, ay));
+ xf86PostMotionEvent(pInfo-dev, 1, 1, 1, ay);
}
return;
}
--
Best Regards
Edd Barrett
http://www.theunixzoo.co.uk
Re: Stairstep mouse motion
On Mon, Jul 08, 2013 at 08:26:56PM +0300, Henri Kemppainen wrote:
I do fear that with some devices your patch will collapse too many
events and make it harder to follow small radius curves.
Right, I did not consider this case. If this is a problem, perhaps
the code could be changed to only collapse a pair of DELTA_X and
DELTA_Y events, but never more than that. This way it might still
incorrectly merge two independent motions along the axes into one
diagonal motion, but I would expect that to be rare, and the deltas
to be so small that it has very little impact on anything.
Here's a diff that does just that. If ws receives more than one
delta along an axis, it will not sum these; each will go in a separate
event. But if it gets an X delta followed by an Y delta (or vice versa),
these will be combined into a single event.
I've committed this patch. Thanks again for the contribution.
Index: xenocara/driver/xf86-input-ws/src/ws.c
===
RCS file: /cvs/xenocara/driver/xf86-input-ws/src/ws.c,v
retrieving revision 1.57
diff -u -p -r1.57 ws.c
--- xenocara/driver/xf86-input-ws/src/ws.c8 Jul 2012 14:22:03 -
1.57
+++ xenocara/driver/xf86-input-ws/src/ws.c8 Jul 2013 17:12:27 -
@@ -474,7 +474,7 @@ wsReadInput(InputInfoPtr pInfo)
{
WSDevicePtr priv = (WSDevicePtr)pInfo-private;
static struct wscons_event eventList[NUMEVENTS];
- int n, c;
+ int n, c, dx, dy;
struct wscons_event *event = eventList;
unsigned char *pBuf;
@@ -488,10 +488,11 @@ wsReadInput(InputInfoPtr pInfo)
if (n == 0)
return;
+ dx = dy = 0;
n /= sizeof(struct wscons_event);
while (n--) {
int buttons = priv-lastButtons;
- int dx = 0, dy = 0, dz = 0, dw = 0, ax = 0, ay = 0;
+ int newdx = 0, newdy = 0, dz = 0, dw = 0, ax = 0, ay = 0;
int zbutton = 0, wbutton = 0;
switch (event-type) {
@@ -506,11 +507,17 @@ wsReadInput(InputInfoPtr pInfo)
buttons));
break;
case WSCONS_EVENT_MOUSE_DELTA_X:
- dx = event-value;
+ if (!dx)
+ dx = event-value;
+ else
+ newdx = event-value;
DBG(4, ErrorF(Relative X %d\n, event-value));
break;
case WSCONS_EVENT_MOUSE_DELTA_Y:
- dy = -event-value;
+ if (!dy)
+ dy = -event-value;
+ else
+ newdy = -event-value;
DBG(4, ErrorF(Relative Y %d\n, event-value));
break;
case WSCONS_EVENT_MOUSE_ABSOLUTE_X:
@@ -548,14 +555,20 @@ wsReadInput(InputInfoPtr pInfo)
}
++event;
- if (dx || dy) {
- if (wsWheelEmuFilterMotion(pInfo, dx, dy))
+ if ((newdx || newdy) || ((dx || dy)
+ event-type != WSCONS_EVENT_MOUSE_DELTA_X
+ event-type != WSCONS_EVENT_MOUSE_DELTA_Y)) {
+ int tmpx = dx, tmpy = dy;
+ dx = newdx;
+ dy = newdy;
+
+ if (wsWheelEmuFilterMotion(pInfo, tmpx, tmpy))
continue;
/* relative motion event */
DBG(3, ErrorF(postMotionEvent dX %d dY %d\n,
- dx, dy));
- xf86PostMotionEvent(pInfo-dev, 0, 0, 2, dx, dy);
+ tmpx, tmpy));
+ xf86PostMotionEvent(pInfo-dev, 0, 0, 2, tmpx, tmpy);
}
if (dz priv-Z.negative != WS_NOMAP
priv-Z.positive != WS_NOMAP) {
@@ -583,9 +596,9 @@ wsReadInput(InputInfoPtr pInfo)
ay = tmp;
}
if (ax) {
- dx = ax - priv-old_ax;
+ int xdelta = ax - priv-old_ax;
priv-old_ax = ax;
- if (wsWheelEmuFilterMotion(pInfo, dx, 0))
+ if (wsWheelEmuFilterMotion(pInfo, xdelta, 0))
continue;
/* absolute position event */
@@ -593,15 +606,24 @@ wsReadInput(InputInfoPtr pInfo)
xf86PostMotionEvent(pInfo-dev, 1, 0, 1, ax);
}
if (ay) {
- dy = ay - priv-old_ay;
+ int ydelta = ay - priv-old_ay;
priv-old_ay = ay;
- if (wsWheelEmuFilterMotion(pInfo, 0, dy))
+ if (wsWheelEmuFilterMotion(pInfo, 0, ydelta))
continue;
Re: Stairstep mouse motion
Hi, On Mon, Jul 08, 2013 at 08:26:56PM +0300, Henri Kemppainen wrote: Here's a diff that does just that. If ws receives more than one delta along an axis, it will not sum these; each will go in a separate event. But if it gets an X delta followed by an Y delta (or vice versa), these will be combined into a single event. I have also been experiencing these jagged staircase lines on my x230t. FWIW here are the outcomes of my experiments: Prior to applying your diff (the most recent one): Touchpad: smooth lines. Nipple: jagged lines. Pen: jagged lines. After applying your diff: Touchpad: smooth lines. Nipple: smooth lines. Pen: jagged lines. I wonder if it is because the pen is an absolute pointing device. You probably need extra magic in the WSCONS_EVENT_MOUSE_ABSOLUTE_* cases. -- Best Regards Edd Barrett http://www.theunixzoo.co.uk
Re: Stairstep mouse motion
I do fear that with some devices your patch will collapse too many
events and make it harder to follow small radius curves.
Right, I did not consider this case. If this is a problem, perhaps
the code could be changed to only collapse a pair of DELTA_X and
DELTA_Y events, but never more than that. This way it might still
incorrectly merge two independent motions along the axes into one
diagonal motion, but I would expect that to be rare, and the deltas
to be so small that it has very little impact on anything.
Here's a diff that does just that. If ws receives more than one
delta along an axis, it will not sum these; each will go in a separate
event. But if it gets an X delta followed by an Y delta (or vice versa),
these will be combined into a single event.
Index: xenocara/driver/xf86-input-ws/src/ws.c
===
RCS file: /cvs/xenocara/driver/xf86-input-ws/src/ws.c,v
retrieving revision 1.57
diff -u -p -r1.57 ws.c
--- xenocara/driver/xf86-input-ws/src/ws.c 8 Jul 2012 14:22:03 -
1.57
+++ xenocara/driver/xf86-input-ws/src/ws.c 8 Jul 2013 17:12:27 -
@@ -474,7 +474,7 @@ wsReadInput(InputInfoPtr pInfo)
{
WSDevicePtr priv = (WSDevicePtr)pInfo-private;
static struct wscons_event eventList[NUMEVENTS];
- int n, c;
+ int n, c, dx, dy;
struct wscons_event *event = eventList;
unsigned char *pBuf;
@@ -488,10 +488,11 @@ wsReadInput(InputInfoPtr pInfo)
if (n == 0)
return;
+ dx = dy = 0;
n /= sizeof(struct wscons_event);
while (n--) {
int buttons = priv-lastButtons;
- int dx = 0, dy = 0, dz = 0, dw = 0, ax = 0, ay = 0;
+ int newdx = 0, newdy = 0, dz = 0, dw = 0, ax = 0, ay = 0;
int zbutton = 0, wbutton = 0;
switch (event-type) {
@@ -506,11 +507,17 @@ wsReadInput(InputInfoPtr pInfo)
buttons));
break;
case WSCONS_EVENT_MOUSE_DELTA_X:
- dx = event-value;
+ if (!dx)
+ dx = event-value;
+ else
+ newdx = event-value;
DBG(4, ErrorF(Relative X %d\n, event-value));
break;
case WSCONS_EVENT_MOUSE_DELTA_Y:
- dy = -event-value;
+ if (!dy)
+ dy = -event-value;
+ else
+ newdy = -event-value;
DBG(4, ErrorF(Relative Y %d\n, event-value));
break;
case WSCONS_EVENT_MOUSE_ABSOLUTE_X:
@@ -548,14 +555,20 @@ wsReadInput(InputInfoPtr pInfo)
}
++event;
- if (dx || dy) {
- if (wsWheelEmuFilterMotion(pInfo, dx, dy))
+ if ((newdx || newdy) || ((dx || dy)
+ event-type != WSCONS_EVENT_MOUSE_DELTA_X
+ event-type != WSCONS_EVENT_MOUSE_DELTA_Y)) {
+ int tmpx = dx, tmpy = dy;
+ dx = newdx;
+ dy = newdy;
+
+ if (wsWheelEmuFilterMotion(pInfo, tmpx, tmpy))
continue;
/* relative motion event */
DBG(3, ErrorF(postMotionEvent dX %d dY %d\n,
- dx, dy));
- xf86PostMotionEvent(pInfo-dev, 0, 0, 2, dx, dy);
+ tmpx, tmpy));
+ xf86PostMotionEvent(pInfo-dev, 0, 0, 2, tmpx, tmpy);
}
if (dz priv-Z.negative != WS_NOMAP
priv-Z.positive != WS_NOMAP) {
@@ -583,9 +596,9 @@ wsReadInput(InputInfoPtr pInfo)
ay = tmp;
}
if (ax) {
- dx = ax - priv-old_ax;
+ int xdelta = ax - priv-old_ax;
priv-old_ax = ax;
- if (wsWheelEmuFilterMotion(pInfo, dx, 0))
+ if (wsWheelEmuFilterMotion(pInfo, xdelta, 0))
continue;
/* absolute position event */
@@ -593,15 +606,24 @@ wsReadInput(InputInfoPtr pInfo)
xf86PostMotionEvent(pInfo-dev, 1, 0, 1, ax);
}
if (ay) {
- dy = ay - priv-old_ay;
+ int ydelta = ay - priv-old_ay;
priv-old_ay = ay;
- if (wsWheelEmuFilterMotion(pInfo, 0, dy))
+ if (wsWheelEmuFilterMotion(pInfo, 0, ydelta))
continue;
/* absolute position event */
DBG(3,
Re: Stairstep mouse motion
On Sun, Jul 7, 2013 at 12:22 PM, Henri Kemppainen ducl...@guu.fi wrote:
So I needed to see my thoughts on paper but my desk was so full of stuff
I couldn't make room for pen and paper. Instead I fired up Gimp, and
drawing with the mouse worked fine until I realized it's next to impossible
to draw diagonal lines that look like lines.
Instead of straight lines, I got waves. The faster I draw the mouse, the
deeper the waves.
That is a very cool effect (referring to your image). Having not seen
this issue, I tried drawing a few diagonal lines; this was using the
synaptics touchpad. The lines were pretty smooth looking. Then I
connected a USB mouse and reattempted drawing the diagonal lines.
That's when I see the stair-step effect; still not as wave-like as
yours are.
Just adding a data-point.
--patrick
It looked like diagonal mouse motion generated a pair
of pointer motion events, one for the X axis and another for Y. And Gimp
smoothed that stairstep motion, resulting in waves. Xev(1) confirmed my
hypothesis.
It turns out wsmouse(4) is breaking the mouse input into multiple events.
This isn't necessarily a bug, since it allows for a very small and simple
event structure which works without modification as new properties (such
as button states, axes, etc.) are added.
Now wsmouse generates all the events it can in a loop before waking up
the process that waits for these events. So on the receiving side (i.e.
in the xenocara ws(4) driver) we can sum all the consecutive X and Y
deltas from a single read() before issuing a pointer motion event. This
eliminates the stairsteps as long as the events generated by wsmouse fit
in the buffers involved.
Other approaches would be either extending the event structure, or perhaps
adding a new event type that somehow communicates the intended grouping
for events that follow/precede (but ugh...). I felt the ws(4) fix was
the least intrusive, and it appears to work just fine here. What do you
think?
This image (drawn in Gimp) illustrates the problem. The last two lines
were drawn with my diff applied.
http://guu.fi/mousebug.png
Index: xenocara/driver/xf86-input-ws/src/ws.c
===
RCS file: /cvs/xenocara/driver/xf86-input-ws/src/ws.c,v
retrieving revision 1.57
diff -u -p -r1.57 ws.c
--- xenocara/driver/xf86-input-ws/src/ws.c 8 Jul 2012 14:22:03 -
1.57
+++ xenocara/driver/xf86-input-ws/src/ws.c 7 Jul 2013 18:33:57 -
@@ -474,7 +474,7 @@ wsReadInput(InputInfoPtr pInfo)
{
WSDevicePtr priv = (WSDevicePtr)pInfo-private;
static struct wscons_event eventList[NUMEVENTS];
- int n, c;
+ int n, c, dx, dy;
struct wscons_event *event = eventList;
unsigned char *pBuf;
@@ -488,10 +488,11 @@ wsReadInput(InputInfoPtr pInfo)
if (n == 0)
return;
+ dx = dy = 0;
n /= sizeof(struct wscons_event);
while (n--) {
int buttons = priv-lastButtons;
- int dx = 0, dy = 0, dz = 0, dw = 0, ax = 0, ay = 0;
+ int dz = 0, dw = 0, ax = 0, ay = 0;
int zbutton = 0, wbutton = 0;
switch (event-type) {
@@ -506,11 +507,11 @@ wsReadInput(InputInfoPtr pInfo)
buttons));
break;
case WSCONS_EVENT_MOUSE_DELTA_X:
- dx = event-value;
+ dx += event-value;
DBG(4, ErrorF(Relative X %d\n, event-value));
break;
case WSCONS_EVENT_MOUSE_DELTA_Y:
- dy = -event-value;
+ dy -= event-value;
DBG(4, ErrorF(Relative Y %d\n, event-value));
break;
case WSCONS_EVENT_MOUSE_ABSOLUTE_X:
@@ -548,14 +549,18 @@ wsReadInput(InputInfoPtr pInfo)
}
++event;
- if (dx || dy) {
- if (wsWheelEmuFilterMotion(pInfo, dx, dy))
+ if ((dx || dy) event-type != WSCONS_EVENT_MOUSE_DELTA_X
+ event-type != WSCONS_EVENT_MOUSE_DELTA_Y) {
+ int tmpx = dx, tmpy = dy;
+ dx = dy = 0;
+
+ if (wsWheelEmuFilterMotion(pInfo, tmpx, tmpy))
continue;
/* relative motion event */
DBG(3, ErrorF(postMotionEvent dX %d dY %d\n,
- dx, dy));
- xf86PostMotionEvent(pInfo-dev, 0, 0, 2, dx, dy);
+ tmpx, tmpy));
+ xf86PostMotionEvent(pInfo-dev, 0, 0, 2, tmpx, tmpy);
}
if (dz priv-Z.negative != WS_NOMAP
priv-Z.positive != WS_NOMAP) {
@@ -583,9 +588,9 @@
