Re: add support for crc_enabled Elantech v3 touchpads
On Thu, 2 Apr 2015 18:51:38 +0200
Martin Pieuchot m...@openbsd.org wrote:
Even if that's true, nothing prevent us to commit this diff first, as
long as it does not introduce regression and then work on the possible
refactoring needed to support debounce packets :)
That is great to hear.
Because the way pms_sync_elantech_v3 now handles crc mode
pms_proc_elantech_v3 can't possibly receive a debounce packet. Therefore
I moved the debounce packet check in pms_proc_elantech_v3 into the part
that handles non-crc packets.
Below is the full diff for anyone that wants to try out these changes.
Index: sys/dev/pckbc/pms.c
===
RCS file: /cvs/src/sys/dev/pckbc/pms.c,v
retrieving revision 1.58
diff -u -p -r1.58 pms.c
--- sys/dev/pckbc/pms.c 26 Mar 2015 01:30:22 - 1.58
+++ sys/dev/pckbc/pms.c 2 Apr 2015 20:43:23 -
@@ -137,6 +137,7 @@ struct elantech_softc {
#define ELANTECH_F_HAS_ROCKER 0x02
#define ELANTECH_F_2FINGER_PACKET 0x04
#define ELANTECH_F_HW_V1_OLD 0x08
+#define ELANTECH_F_CRC_ENABLED 0x10
int fw_version;
int min_x, min_y;
@@ -1812,6 +1813,9 @@ elantech_get_hwinfo_v3(struct pms_softc
elantech-fw_version = fw_version;
elantech-flags |= ELANTECH_F_REPORTS_PRESSURE;
+ if ((fw_version 0x4000) == 0x4000)
+ elantech-flags |= ELANTECH_F_CRC_ENABLED;
+
if (elantech_set_absolute_mode_v3(sc))
return (-1);
@@ -2164,14 +2168,23 @@ pms_sync_elantech_v2(struct pms_softc *s
int
pms_sync_elantech_v3(struct pms_softc *sc, int data)
{
+ struct elantech_softc *elantech = sc-elantech;
+
switch (sc-inputstate) {
case 0:
+ if (elantech-flags ELANTECH_F_CRC_ENABLED)
+ break;
if ((data 0x0c) != 0x04 (data 0x0c) != 0x0c)
return (-1);
break;
case 3:
- if ((data 0xcf) != 0x02 (data 0xce) != 0x0c)
- return (-1);
+ if (elantech-flags ELANTECH_F_CRC_ENABLED) {
+ if ((data 0x09) != 0x08 (data 0x09) != 0x09)
+ return (-1);
+ } else {
+ if ((data 0xcf) != 0x02 (data 0xce) != 0x0c)
+ return (-1);
+ }
break;
}
@@ -2256,11 +2269,6 @@ pms_proc_elantech_v3(struct pms_softc *s
struct elantech_softc *elantech = sc-elantech;
int x, y, w, z;
- /* The hardware sends this packet when in debounce state.
-* The packet should be ignored. */
- if (!memcmp(sc-packet, debounce_pkt, sizeof(debounce_pkt)))
- return;
-
x = ((sc-packet[1] 0x0f) 8 | sc-packet[2]);
y = ((sc-packet[4] 0x0f) 8 | sc-packet[5]);
z = 0;
@@ -2271,10 +2279,19 @@ pms_proc_elantech_v3(struct pms_softc *s
* and a tail packet. We report a single event and ignore
* the tail packet.
*/
- if ((sc-packet[0] 0x0c) != 0x04
- (sc-packet[3] 0xcf) != 0x02) {
- /* not the head packet -- ignore */
- return;
+ if (elantech-flags ELANTECH_F_CRC_ENABLED) {
+ if ((sc-packet[3] 0x09) != 0x08)
+ return;
+ } else {
+ /* The hardware sends this packet when in debounce
state.
+* The packet should be ignored. */
+ if (!memcmp(sc-packet, debounce_pkt,
sizeof(debounce_pkt)))
+ return;
+ if ((sc-packet[0] 0x0c) != 0x04
+ (sc-packet[3] 0xcf) != 0x02) {
+ /* not the head packet -- ignore */
+ return;
+ }
}
}
Re: add support for crc_enabled Elantech v3 touchpads
This pms driver is becoming a nightmare, full of messy code that only a few people understand all the secrets behind. Can someone out there put a serious effort into refactoring this so that there is a layer inside it, with sub-drivers for specific models? I say this, because I senes this problem will get worse soon.
Re: add support for crc_enabled Elantech v3 touchpads
On 04/02/2015 03:39 AM, Fasse wrote: On Wed, 01 Apr 2015 21:23:15 +0200 Ulf Brosziewskiulf.brosziew...@t-online.de wrote: Yes, without some refactoring there won't be an elegant way. pms_sync_elantech_v2 encodes some sync state in the 'flags' field (ELANTECH_F_2FINGER_PACKET), but doing the same in the v3/CRC case might be ugly. Admittedly I am biased because I don't want to refactor ~2400 LOC to get my touchpad working but I don't think that crc enabled v3 touchpads use the debounce packet. I just installed Ubuntu and compiled the 3.19.3 linux kernel with added printk statements in the elantech_packet_check_v3 function on my laptop. In the linux kernel documentation [0] for elantech touchpads it says about the debounce packet: Note on debounce: In case the box has unstable power supply or other electricity issues, or when number of finger changes, F/W would send debounce packet to inform driver that the hardware is in debounce status. I could not reproduce the unstable power supply but when switching the number of fingers on the touchpad no debounce packet is issued. Instead just the head and tail packets are registered and processed (unlike the OpenBSD driver which ignores the tail packet). This leads me to belief that v3/crc does not use debounce packets. Do you think this is possible/likely? (...) Why not? You seem to have shown that it is possible, at least for your hardware and multiple touches. It might well be that the author(s) of the Linux driver just wanted to be on the safe side. (...) There doesn't appear to be any official documentation in regard to elantech touchpads, hence I can't be certain. Nonetheless a refactor would obviously be beneficial but in the interim the patch might do a sufficient job. [0] https://www.kernel.org/doc/Documentation/input/elantech.txt
Re: add support for crc_enabled Elantech v3 touchpads
On 02/04/15(Thu) 18:43, Ulf Brosziewski wrote: On 04/02/2015 03:39 AM, Fasse wrote: On Wed, 01 Apr 2015 21:23:15 +0200 Ulf Brosziewskiulf.brosziew...@t-online.de wrote: Yes, without some refactoring there won't be an elegant way. pms_sync_elantech_v2 encodes some sync state in the 'flags' field (ELANTECH_F_2FINGER_PACKET), but doing the same in the v3/CRC case might be ugly. Admittedly I am biased because I don't want to refactor ~2400 LOC to get my touchpad working but I don't think that crc enabled v3 touchpads use the debounce packet. I just installed Ubuntu and compiled the 3.19.3 linux kernel with added printk statements in the elantech_packet_check_v3 function on my laptop. In the linux kernel documentation [0] for elantech touchpads it says about the debounce packet: Note on debounce: In case the box has unstable power supply or other electricity issues, or when number of finger changes, F/W would send debounce packet to inform driver that the hardware is in debounce status. I could not reproduce the unstable power supply but when switching the number of fingers on the touchpad no debounce packet is issued. Instead just the head and tail packets are registered and processed (unlike the OpenBSD driver which ignores the tail packet). This leads me to belief that v3/crc does not use debounce packets. Do you think this is possible/likely? (...) Why not? You seem to have shown that it is possible, at least for your hardware and multiple touches. It might well be that the author(s) of the Linux driver just wanted to be on the safe side. Even if that's true, nothing prevent us to commit this diff first, as long as it does not introduce regression and then work on the possible refactoring needed to support debounce packets :)
Re: add support for crc_enabled Elantech v3 touchpads
On Wed, Apr 01, 2015 at 05:44:16PM +0200, Fasse wrote:
This diff adds support for Elantech v3 touchpads using the crc_enabled
integrity check. I tested this patch with my Elantech v3 touchpad using
firmware version 0x454f00, it now works correctly.
Other hardware versions should not be affected by this change. I could
not check if this introduces regression with other firmware versions.
If you have an elantech touchpad, particularly v3, could you please try
this diff and see if it causes any issues.
Interesting, thanks.
If you don't hear from anyone else about this please ask me again in
a week from now. I can test on v3 hardware by then but not any earlier.
Please write this as if-else instead of switch:
@@ -2271,11 +2284,18 @@ pms_proc_elantech_v3(struct pms_softc *s
* and a tail packet. We report a single event and ignore
* the tail packet.
*/
- if ((sc-packet[0] 0x0c) != 0x04
- (sc-packet[3] 0xcf) != 0x02) {
- /* not the head packet -- ignore */
- return;
+ switch (elantech-flags ELANTECH_F_CRC_ENABLED) {
+ case 0:
+ if ((sc-packet[0] 0x0c) != 0x04
+ (sc-packet[3] 0xcf) != 0x02) {
+ /* not the head packet -- ignore */
+ return;
+ }
+ case ELANTECH_F_CRC_ENABLED:
+ if ((sc-packet[3] 0x09) != 0x08)
+ return;
}
add support for crc_enabled Elantech v3 touchpads
This diff adds support for Elantech v3 touchpads using the crc_enabled
integrity check. I tested this patch with my Elantech v3 touchpad using
firmware version 0x454f00, it now works correctly.
Other hardware versions should not be affected by this change. I could
not check if this introduces regression with other firmware versions.
If you have an elantech touchpad, particularly v3, could you please try
this diff and see if it causes any issues.
Index: sys/dev/pckbc/pms.c
===
RCS file: /cvs/src/sys/dev/pckbc/pms.c,v
retrieving revision 1.58
diff -u -p -r1.58 pms.c
--- sys/dev/pckbc/pms.c 26 Mar 2015 01:30:22 - 1.58
+++ sys/dev/pckbc/pms.c 1 Apr 2015 14:22:10 -
@@ -137,6 +137,7 @@ struct elantech_softc {
#define ELANTECH_F_HAS_ROCKER 0x02
#define ELANTECH_F_2FINGER_PACKET 0x04
#define ELANTECH_F_HW_V1_OLD 0x08
+#define ELANTECH_F_CRC_ENABLED 0x10
int fw_version;
int min_x, min_y;
@@ -1812,6 +1813,9 @@ elantech_get_hwinfo_v3(struct pms_softc
elantech-fw_version = fw_version;
elantech-flags |= ELANTECH_F_REPORTS_PRESSURE;
+ if ((fw_version 0x4000) == 0x4000)
+ elantech-flags |= ELANTECH_F_CRC_ENABLED;
+
if (elantech_set_absolute_mode_v3(sc))
return (-1);
@@ -2164,14 +2168,23 @@ pms_sync_elantech_v2(struct pms_softc *s
int
pms_sync_elantech_v3(struct pms_softc *sc, int data)
{
+ struct elantech_softc *elantech = sc-elantech;
+
switch (sc-inputstate) {
case 0:
+ if (elantech-flags ELANTECH_F_CRC_ENABLED)
+ break;
if ((data 0x0c) != 0x04 (data 0x0c) != 0x0c)
return (-1);
break;
case 3:
- if ((data 0xcf) != 0x02 (data 0xce) != 0x0c)
- return (-1);
+ if (elantech-flags ELANTECH_F_CRC_ENABLED) {
+ if ((data 0x09) != 0x08 (data 0x09) != 0x09)
+ return (-1);
+ } else {
+ if ((data 0xcf) != 0x02 (data 0xce) != 0x0c)
+ return (-1);
+ }
break;
}
@@ -2271,11 +2284,18 @@ pms_proc_elantech_v3(struct pms_softc *s
* and a tail packet. We report a single event and ignore
* the tail packet.
*/
- if ((sc-packet[0] 0x0c) != 0x04
- (sc-packet[3] 0xcf) != 0x02) {
- /* not the head packet -- ignore */
- return;
+ switch (elantech-flags ELANTECH_F_CRC_ENABLED) {
+ case 0:
+ if ((sc-packet[0] 0x0c) != 0x04
+ (sc-packet[3] 0xcf) != 0x02) {
+ /* not the head packet -- ignore */
+ return;
+ }
+ case ELANTECH_F_CRC_ENABLED:
+ if ((sc-packet[3] 0x09) != 0x08)
+ return;
}
+
}
/* Prevent juming cursor if pad isn't touched or reports garbage. */
Re: add support for crc_enabled Elantech v3 touchpads
On Wed, 01 Apr 2015 20:05:46 +0200 Ulf Brosziewski ulf.brosziew...@t-online.de wrote: Hi, there might be a problem. The Linux driver for that touchpad type also accepts debounce packets, which have the same format as for the non-crc version. I have no idea whether that is correct and if those packets do occur in practice, but if they do they wouldn't pass this version of sync() (byte 3 would be 0x02 then). You are right. I haven't had issues so far with debounce packets missing but I obviously only have a very small sample size so there is that. Since the debounce packet is ignored anyways I don't know if it matters that it is thrown out early. Although if it didn't matter there probably wouldn't even be a debounce packet? We could allow byte 3 to pass if its value after the check is 0x02 but that kind of defeats the purpose of the integrity check I guess. Fortunately the values of debounce and head packets match up for non crc elantech v3 touchpads. I don't think there is an elegant way to solve this problem because pms_sync_elantech_v3 is called before the packet is complete and you need the whole packet to check if it is a debounce packet. To solve this we would have to do somthing similar to the linux driver by only checking complete packets.
Re: add support for crc_enabled Elantech v3 touchpads
On Wed, 1 Apr 2015 18:02:59 +0200
Stefan Sperling s...@stsp.name wrote:
Interesting, thanks.
If you don't hear from anyone else about this please ask me again in
a week from now. I can test on v3 hardware by then but not any earlier.
Please write this as if-else instead of switch:
@@ -2271,11 +2284,18 @@ pms_proc_elantech_v3(struct pms_softc *s
* and a tail packet. We report a single event and ignore
* the tail packet.
*/
- if ((sc-packet[0] 0x0c) != 0x04
- (sc-packet[3] 0xcf) != 0x02) {
- /* not the head packet -- ignore */
- return;
+ switch (elantech-flags ELANTECH_F_CRC_ENABLED) {
+ case 0:
+ if ((sc-packet[0] 0x0c) != 0x04
+ (sc-packet[3] 0xcf) != 0x02) {
+ /* not the head packet -- ignore */
+ return;
+ }
+ case ELANTECH_F_CRC_ENABLED:
+ if ((sc-packet[3] 0x09) != 0x08)
+ return;
}
Thanks for the offer I will propably take you up on that.
Below is the modified diff with the switch statement transformed.
Index: sys/dev/pckbc/pms.c
===
RCS file: /cvs/src/sys/dev/pckbc/pms.c,v
retrieving revision 1.58
diff -u -p -r1.58 pms.c
--- sys/dev/pckbc/pms.c 26 Mar 2015 01:30:22 - 1.58
+++ sys/dev/pckbc/pms.c 1 Apr 2015 16:31:33 -
@@ -137,6 +137,7 @@ struct elantech_softc {
#define ELANTECH_F_HAS_ROCKER 0x02
#define ELANTECH_F_2FINGER_PACKET 0x04
#define ELANTECH_F_HW_V1_OLD 0x08
+#define ELANTECH_F_CRC_ENABLED 0x10
int fw_version;
int min_x, min_y;
@@ -1812,6 +1813,9 @@ elantech_get_hwinfo_v3(struct pms_softc
elantech-fw_version = fw_version;
elantech-flags |= ELANTECH_F_REPORTS_PRESSURE;
+ if ((fw_version 0x4000) == 0x4000)
+ elantech-flags |= ELANTECH_F_CRC_ENABLED;
+
if (elantech_set_absolute_mode_v3(sc))
return (-1);
@@ -2164,14 +2168,23 @@ pms_sync_elantech_v2(struct pms_softc *s
int
pms_sync_elantech_v3(struct pms_softc *sc, int data)
{
+ struct elantech_softc *elantech = sc-elantech;
+
switch (sc-inputstate) {
case 0:
+ if (elantech-flags ELANTECH_F_CRC_ENABLED)
+ break;
if ((data 0x0c) != 0x04 (data 0x0c) != 0x0c)
return (-1);
break;
case 3:
- if ((data 0xcf) != 0x02 (data 0xce) != 0x0c)
- return (-1);
+ if (elantech-flags ELANTECH_F_CRC_ENABLED) {
+ if ((data 0x09) != 0x08 (data 0x09) != 0x09)
+ return (-1);
+ } else {
+ if ((data 0xcf) != 0x02 (data 0xce) != 0x0c)
+ return (-1);
+ }
break;
}
@@ -2271,10 +2284,15 @@ pms_proc_elantech_v3(struct pms_softc *s
* and a tail packet. We report a single event and ignore
* the tail packet.
*/
- if ((sc-packet[0] 0x0c) != 0x04
- (sc-packet[3] 0xcf) != 0x02) {
- /* not the head packet -- ignore */
- return;
+ if (elantech-flags ELANTECH_F_CRC_ENABLED) {
+ if ((sc-packet[3] 0x09) != 0x08)
+ return;
+ } else {
+ if ((sc-packet[0] 0x0c) != 0x04
+ (sc-packet[3] 0xcf) != 0x02) {
+ /* not the head packet -- ignore */
+ return;
+ }
}
}
Re: add support for crc_enabled Elantech v3 touchpads
On 04/01/2015 08:46 PM, Fasse wrote: On Wed, 01 Apr 2015 20:05:46 +0200 Ulf Brosziewskiulf.brosziew...@t-online.de wrote: Hi, there might be a problem. The Linux driver for that touchpad type also accepts debounce packets, which have the same format as for the non-crc version. I have no idea whether that is correct and if those packets do occur in practice, but if they do they wouldn't pass this version of sync() (byte 3 would be 0x02 then). You are right. I haven't had issues so far with debounce packets missing but I obviously only have a very small sample size so there is that. Since the debounce packet is ignored anyways I don't know if it matters that it is thrown out early. Although if it didn't matter there probably wouldn't even be a debounce packet? We could allow byte 3 to pass if its value after the check is 0x02 but that kind of defeats the purpose of the integrity check I guess. Fortunately the values of debounce and head packets match up for non crc elantech v3 touchpads. I don't think there is an elegant way to solve this problem because pms_sync_elantech_v3 is called before the packet is complete and you need the whole packet to check if it is a debounce packet. To solve this we would have to do somthing similar to the linux driver by only checking complete packets. Yes, without some refactoring there won't be an elegant way. pms_sync_elantech_v2 encodes some sync state in the 'flags' field (ELANTECH_F_2FINGER_PACKET), but doing the same in the v3/CRC case might be ugly.
Re: add support for crc_enabled Elantech v3 touchpads
Hi,
there might be a problem. The Linux driver for that touchpad type also
accepts debounce packets, which have the same format as for the
non-crc version. I have no idea whether that is correct and if those
packets do occur in practice, but if they do they wouldn't pass this
version of sync() (byte 3 would be 0x02 then).
On 04/01/2015 05:44 PM, Fasse wrote:
This diff adds support for Elantech v3 touchpads using the crc_enabled
integrity check. I tested this patch with my Elantech v3 touchpad using
firmware version 0x454f00, it now works correctly.
Other hardware versions should not be affected by this change. I could
not check if this introduces regression with other firmware versions.
If you have an elantech touchpad, particularly v3, could you please try
this diff and see if it causes any issues.
Index: sys/dev/pckbc/pms.c
===
RCS file: /cvs/src/sys/dev/pckbc/pms.c,v
retrieving revision 1.58
diff -u -p -r1.58 pms.c
--- sys/dev/pckbc/pms.c 26 Mar 2015 01:30:22 - 1.58
+++ sys/dev/pckbc/pms.c 1 Apr 2015 14:22:10 -
@@ -137,6 +137,7 @@ struct elantech_softc {
#define ELANTECH_F_HAS_ROCKER 0x02
#define ELANTECH_F_2FINGER_PACKET 0x04
#define ELANTECH_F_HW_V1_OLD 0x08
+#define ELANTECH_F_CRC_ENABLED 0x10
int fw_version;
int min_x, min_y;
@@ -1812,6 +1813,9 @@ elantech_get_hwinfo_v3(struct pms_softc
elantech-fw_version = fw_version;
elantech-flags |= ELANTECH_F_REPORTS_PRESSURE;
+ if ((fw_version 0x4000) == 0x4000)
+ elantech-flags |= ELANTECH_F_CRC_ENABLED;
+
if (elantech_set_absolute_mode_v3(sc))
return (-1);
@@ -2164,14 +2168,23 @@ pms_sync_elantech_v2(struct pms_softc *s
int
pms_sync_elantech_v3(struct pms_softc *sc, int data)
{
+ struct elantech_softc *elantech = sc-elantech;
+
switch (sc-inputstate) {
case 0:
+ if (elantech-flags ELANTECH_F_CRC_ENABLED)
+ break;
if ((data 0x0c) != 0x04 (data 0x0c) != 0x0c)
return (-1);
break;
case 3:
- if ((data 0xcf) != 0x02 (data 0xce) != 0x0c)
- return (-1);
+ if (elantech-flags ELANTECH_F_CRC_ENABLED) {
+ if ((data 0x09) != 0x08 (data 0x09) != 0x09)
+ return (-1);
+ } else {
+ if ((data 0xcf) != 0x02 (data 0xce) != 0x0c)
+ return (-1);
+ }
break;
}
@@ -2271,11 +2284,18 @@ pms_proc_elantech_v3(struct pms_softc *s
* and a tail packet. We report a single event and ignore
* the tail packet.
*/
- if ((sc-packet[0] 0x0c) != 0x04
- (sc-packet[3] 0xcf) != 0x02) {
- /* not the head packet -- ignore */
- return;
+ switch (elantech-flags ELANTECH_F_CRC_ENABLED) {
+ case 0:
+ if ((sc-packet[0] 0x0c) != 0x04
+ (sc-packet[3] 0xcf) != 0x02) {
+ /* not the head packet -- ignore */
+ return;
+ }
+ case ELANTECH_F_CRC_ENABLED:
+ if ((sc-packet[3] 0x09) != 0x08)
+ return;
}
+
}
/* Prevent juming cursor if pad isn't touched or reports garbage. */
