Add support for firmware versions #620

whot · 2023-12-20T02:29:06Z

Completely untested, this is just the first draft

Add support for specifying a firmware version string in the DeviceMatch which is filled in by reading the proposed fw_name sysfs attribute.

DeviceMatch now uses | as separator, we have a few devices with : in the name and it's too painful to handle this otherwise.
some logic will be needed to do the right thing if we need a fw version but we don't have a fw_name attribute, i.e. running new libwacom on old kernels. Don't know how to handle this yet.

Fixes #610

cc @JoseExposito

libwacom/libwacom.c

jexposit · 2023-12-20T09:51:47Z

Nice! Thanks for working on this. I was planning to work on it when I had some time but you already made great progress.

I still need to get the firmware names for Gaomon so this PR actually fixes #609 and add the firmwares in the .tablet files.

I'll try to test it ASAP.

jexposit

Hi Peter,

Sorry for the embarrassing long time it took me to actually run this PR.

This is a first review after running the code, even though I notice 2 main issues:

g_udev_device_get_sysfs_attr always return NULL for fw_name: I didn't have time to investigate if a different function is required or there is an issue in my kernel code
The fw name is not optional when matching a tablet. For example, because I wasn't able to get the fw_name, this doesn't match usb|256c|006d|HUION Huion Tablet_H640P Pad|HUION_T203.

I'll keep investigating how to fix these issues.

libwacom/libwacom-database.c

libwacom/libwacom.c

jexposit · 2024-03-16T17:31:31Z

libwacom/libwacom.c


 	match->match = newmatch;
 	match->name = g_strdup(name);
+	match->fw = g_strdup(fw);


Memory leak in libwacom_match_unref:

WacomMatch* libwacom_match_unref(WacomMatch *match) { if (match == NULL || !g_atomic_int_dec_and_test(&match->refcnt)) return NULL; g_free (match->match); g_free (match->name); + g_free (match->fw); g_free (match); return NULL; }

libwacom/libwacom.sym

jexposit · 2024-03-16T18:53:22Z

libwacom/libwacom.c

+	/* Try getting the name/fw_name from the parent if that fails */
+	if (*name == NULL || *fw == NULL) {
+		GUdevDevice *parent = g_udev_device_get_parent (device);
+
+		while (parent && (*name == NULL || *fw == NULL)) {
+			if (*name == NULL)
+				*name = g_strdup (g_udev_device_get_sysfs_attr (parent, "name"));
+			if (*fw == NULL)
+				*fw = g_strdup (g_udev_device_get_sysfs_attr (device, "fw_name"));
+			g_object_unref (parent);
+			parent = g_udev_device_get_parent (device);
+		}
+
+		if (parent)
+			g_object_unref (parent);

-		parent = g_udev_device_get_parent (device);
-		if (!parent)
+		if (*name == NULL)
 			goto out;
-		*name = g_strdup (g_udev_device_get_sysfs_attr (parent, "name"));
-		g_object_unref (parent);
 	}


I'm still not able to get the fw name, but there are some minor issues with this block of code:

/* Try getting the name/fw_name from the parent if that fails */ if (*name == NULL || *fw == NULL) { GUdevDevice *prev_parent = NULL; GUdevDevice *parent = g_udev_device_get_parent (device); while (parent && (*name == NULL || *fw == NULL)) { if (*name == NULL) *name = g_strdup (g_udev_device_get_sysfs_attr (parent, "name")); if (*fw == NULL) *fw = g_strdup (g_udev_device_get_sysfs_attr (parent, "fw_name")); prev_parent = parent; parent = g_udev_device_get_parent (prev_parent); g_object_unref (prev_parent); } if (parent) g_object_unref (parent); if (*name == NULL) goto out; }

oops. all comments addressed and rebased, thanks!

Still untested because, well, we don't have the test suite to test this... :(

This needs to be printed to stdout, otherwise meson won't show it.

Some device names include a colon which makes it hard to add extra optional components to the device matches.

No functional changes, this just preps the way for more optional elements in the match string.

jexposit · 2024-03-18T09:51:07Z

libwacom/libwacom.c

+			if (*name == NULL)
+				*name = g_strdup (g_udev_device_get_sysfs_attr (parent, "name"));
+			if (*fw == NULL)
+				*fw = g_strdup (g_udev_device_get_sysfs_attr (device, "fw_name"));


This one is parent:

*fw = g_strdup (g_udev_device_get_sysfs_attr (parent, "fw_name"));

With this change (and adding a missing S_IROTH in the driver) libwacom is able to read the firmware name 🎉

Now I need to parse it to drop the version (HUION_T203_200720 -> HUION_T203) and make the match to the fw name optional for backwards compatibility.

This one is parent:

thanks, fixed (and renamed tmp to old_parent for consistency with some other code).

Some device manufacturers re-use the VID/PID but we can still get to the firmware versions if the kernel driver exports them. Add those to the match string as fifth component after the (possibly empty) name. A DeviceMatch=usb|1234|abcd|Foo Tablet|ABCD1234 now matches against a device with that firmware version.

whot · 2024-03-18T10:39:59Z

Now I need to parse it to drop the version (HUION_T203_200720 -> HUION_T203) and make the match to the fw name optional for backwards compatibility.

Moving this out from #620 (comment): if the version is encoded in the fw name then we might need a glob for the version match? That's possible but requires a bit of rework internally.

jexposit · 2024-03-18T11:03:46Z

Now I need to parse it to drop the version (HUION_T203_200720 -> HUION_T203) and make the match to the fw name optional for backwards compatibility.

Moving this out from #620 (comment): if the version is encoded in the fw name then we might need a glob for the version match? That's possible but requires a bit of rework internally.

Either we implement globs or we parse the well-known fw string. Since the fw names we are going to handle are going to be limited, I'm not sure if implementing globs is worth it.

I'll have a look to the matching mechanism and see how complex it could be.

jexposit · 2024-03-18T11:46:52Z

For reference, this is how a very simple fw name parsing could look like:

diff

diff --git a/data/huion-h640p.tablet b/data/huion-h640p.tablet
index fd01c22..d4135ae 100644
--- a/data/huion-h640p.tablet
+++ b/data/huion-h640p.tablet
@@ -16,7 +16,7 @@
[Device]
Name=Huion H640P
ModelName=
-DeviceMatch=usb|256c|006d|HUION Huion Tablet_H640P Pad;usb|256c|006d|HUION Huion Tablet_H640P Pen
+DeviceMatch=usb|256c|006d|HUION Huion Tablet_H640P Pad;usb|256c|006d|HUION Huion Tablet_H640P Pen;usb|256c|006d|HUION Huion Tablet_H640P Pad|HUION_T203;usb|256c|006d|HUION Huion Tablet_H640P Pen|HUION_T203
Class=Bamboo
Width=6
Height=4
diff --git a/libwacom/libwacom.c b/libwacom/libwacom.c
index 9442268..cf2574f 100644
--- a/libwacom/libwacom.c
+++ b/libwacom/libwacom.c
@@ -212,6 +212,28 @@ client_query_by_subsystem_and_device_file (GUdevClient *client,
  return ret;
}

+static char *
+get_device_fw_name(GUdevDevice *device)
+{
+	GRegex *regex;
+	GMatchInfo *match_info;
+	const gchar *fw = g_udev_device_get_sysfs_attr (device, "fw_name");
+
+	if (!fw)
+		return g_strdup (fw);
+
+	regex = g_regex_new ("(HUION_.*)_.*$", G_REGEX_DEFAULT, G_REGEX_MATCH_DEFAULT, NULL);
+	g_regex_match (regex, fw, 0, &match_info);
+
+	if (g_match_info_matches (match_info)) {
+		fw = g_match_info_fetch (match_info, 1);
+	}
+
+	g_match_info_free (match_info);
+	g_regex_unref (regex);
+	return fw;
+}
+
static gboolean
get_device_info (const char            *path,
  	 int                   *vendor_id,
@@ -285,7 +307,7 @@ get_device_info (const char            *path,
  }

  *name = g_strdup (g_udev_device_get_sysfs_attr (device, "name"));
-	*fw = g_strdup (g_udev_device_get_sysfs_attr (device, "fw_name"));
+	*fw = get_device_fw_name (device);
  /* Try getting the name/fw_name from the parent if that fails */
  if (*name == NULL || *fw == NULL) {
  	GUdevDevice *parent = g_udev_device_get_parent (device);
@@ -296,7 +318,7 @@ get_device_info (const char            *path,
  		if (*name == NULL)
  			*name = g_strdup (g_udev_device_get_sysfs_attr (parent, "name"));
  		if (*fw == NULL)
-				*fw = g_strdup (g_udev_device_get_sysfs_attr (device, "fw_name"));
+				*fw = get_device_fw_name (parent);
  		tmp = parent;
  		parent = g_udev_device_get_parent (parent);
  		g_object_unref (tmp);
@@ -566,7 +588,7 @@ libwacom_new_from_path(const WacomDeviceDatabase *db, const char *path, WacomFal
  WacomDevice *ret = NULL;
  WacomIntegrationFlags integration_flags;
  char *name, *match_name;
-	char *fw;
+	char *fw, *match_fw;
  WacomMatch *match;

  switch (fallback) {
@@ -592,10 +614,15 @@ libwacom_new_from_path(const WacomDeviceDatabase *db, const char *path, WacomFal
  	return NULL;

  match_name = name;
-	device = libwacom_new (db, match_name, fw, vendor_id, product_id, bus, error);
+	match_fw = fw;
+	device = libwacom_new (db, match_name, match_fw, vendor_id, product_id, bus, error);
  if (device == NULL) {
-		match_name = NULL;
-		device = libwacom_new (db, match_name, fw, vendor_id, product_id, bus, error);
+		match_fw = NULL;
+		device = libwacom_new (db, match_name, match_fw, vendor_id, product_id, bus, error);
+		if (device == NULL) {
+			match_name = NULL;
+			device = libwacom_new (db, match_name, match_fw, vendor_id, product_id, bus, error);
+		}
  }

  if (device == NULL) {
@@ -618,7 +645,7 @@ libwacom_new_from_path(const WacomDeviceDatabase *db, const char *path, WacomFal
  }

  /* for multiple-match devices, set to the one we requested */
-	match = libwacom_match_new(match_name, fw, bus, vendor_id, product_id);
+	match = libwacom_match_new(match_name, match_fw, bus, vendor_id, product_id);
  libwacom_set_default_match(ret, match);
  libwacom_match_unref(match);

It is way simpler than modifying the device_ht and stylus_ht hash tables to support globs, but I'm not convinced about it being the best solution long term.

whot · 2024-03-19T02:15:12Z

A few comments:

we're not exactly in a hot path here so we could replace the hashtable with a list and be probably fine.
looks like a simple g_str_has_prefix would work here too? should we keep the regex at bay until we definitely need it?
This bit here seems a bit excessive for a null pointer ;)

+	if (!fw)
+		return g_strdup (fw);
+

I'm not convinced about it being the best solution long term.

We don't need anything long term right now, chances are that the vendors throw us a curveball anyway. All this is internal-only API so we can go with what works and fix it later where it doesn't.

Side-note: I don't have any emotional attachment to this PR and since you're the one who can actually test it - feel free to push a PR that replaces this one and we close this one in favour of yours. Better then you having to give me diffs for my bugs :)

jexposit · 2024-03-21T12:16:13Z

looks like a simple g_str_has_prefix would work here too? should we keep the regex at bay until we definitely need it?

I'm afraid we will need the regex to match different vendor names, see #610 (comment) for examples.

Thankfully it is a very simple regex.

We don't need anything long term right now, chances are that the vendors throw us a curveball anyway. All this is internal-only API so we can go with what works and fix it later where it doesn't.

Then I guess it'd be better to stick with the simpler solution and less intrusive solution: parsing the firmware name.

Side-note: I don't have any emotional attachment to this PR and since you're the one who can actually test it - feel free to push a PR that replaces this one and we close this one in favour of yours. Better then you having to give me diffs for my bugs :)

Poor PR xD Let me share with you one more diff. If more changes are required, I promise I'll open another PR.

With these changes, libwacom-list-local-devices finds my device using the old driver (without fw_name) and the new one (with fw_name).

Also, it is possible to add the firmware version to the .tablet file:

DeviceMatch=usb|256c|006d|HUION Huion Tablet_H640P Pad;usb|256c|006d|HUION Huion Tablet_H640P Pen;usb|256c|006d|HUION Huion Tablet_H640P Pad|HUION_T203;usb|256c|006d|HUION Huion Tablet_H640P Pen|HUION_T203

To make the make it match taking into account the firmware name while keeping compatibility with older kernels.

I think this is everything we need?

diff

diff --git a/libwacom/libwacom.c b/libwacom/libwacom.c
index 6454a40..f97892d 100644
--- a/libwacom/libwacom.c
+++ b/libwacom/libwacom.c
@@ -212,6 +212,37 @@ client_query_by_subsystem_and_device_file (GUdevClient *client,
  return ret;
}

+static char *
+get_device_fw_name(GUdevDevice *device)
+{
+	GRegex *regex;
+	GMatchInfo *match_info;
+	gchar *fw = g_strdup (g_udev_device_get_sysfs_attr (device, "fw_name"));
+
+	if (!fw)
+		return NULL;
+
+	/* The UCLogic kernel driver returns firmware names with form
+	 * <vendor>_<model>_<version>. Remove the version from `fw` to avoid
+	 * mismatches on firmware updates. */
+	regex = g_regex_new ("(.*_.*)_.*$",
+				G_REGEX_DEFAULT,
+				G_REGEX_MATCH_DEFAULT,
+				NULL);
+	g_regex_match (regex, fw, 0, &match_info);
+
+	if (g_match_info_matches (match_info)) {
+		gchar *tmp = fw;
+		fw = g_match_info_fetch (match_info, 1);
+		g_free (tmp);
+	}
+
+	g_match_info_free (match_info);
+	g_regex_unref (regex);
+
+	return fw;
+}
+
static gboolean
get_device_info (const char            *path,
  	 int                   *vendor_id,
@@ -285,7 +316,7 @@ get_device_info (const char            *path,
  }

  *name = g_strdup (g_udev_device_get_sysfs_attr (device, "name"));
-	*fw = g_strdup (g_udev_device_get_sysfs_attr (device, "fw_name"));
+	*fw = get_device_fw_name (device);
  /* Try getting the name/fw_name from the parent if that fails */
  if (*name == NULL || *fw == NULL) {
  	GUdevDevice *parent = g_udev_device_get_parent (device);
@@ -296,7 +327,7 @@ get_device_info (const char            *path,
  		if (*name == NULL)
  			*name = g_strdup (g_udev_device_get_sysfs_attr (parent, "name"));
  		if (*fw == NULL)
-				*fw = g_strdup (g_udev_device_get_sysfs_attr (parent, "fw_name"));
+				*fw = get_device_fw_name (parent);
  		parent = g_udev_device_get_parent (parent);
  		g_object_unref (old_parent);
  	}
@@ -565,7 +596,7 @@ libwacom_new_from_path(const WacomDeviceDatabase *db, const char *path, WacomFal
  WacomDevice *ret = NULL;
  WacomIntegrationFlags integration_flags;
  char *name, *match_name;
-	char *fw;
+	char *fw, *match_fw;
  WacomMatch *match;

  switch (fallback) {
@@ -591,10 +622,15 @@ libwacom_new_from_path(const WacomDeviceDatabase *db, const char *path, WacomFal
  	return NULL;

  match_name = name;
-	device = libwacom_new (db, match_name, fw, vendor_id, product_id, bus, error);
+	match_fw = fw;
+	device = libwacom_new (db, match_name, match_fw, vendor_id, product_id, bus, error);
  if (device == NULL) {
-		match_name = NULL;
-		device = libwacom_new (db, match_name, fw, vendor_id, product_id, bus, error);
+		match_fw = NULL;
+		device = libwacom_new (db, match_name, match_fw, vendor_id, product_id, bus, error);
+		if (device == NULL) {
+			match_name = NULL;
+			device = libwacom_new (db, match_name, match_fw, vendor_id, product_id, bus, error);
+		}
  }

  if (device == NULL) {
@@ -617,7 +653,7 @@ libwacom_new_from_path(const WacomDeviceDatabase *db, const char *path, WacomFal
  }

  /* for multiple-match devices, set to the one we requested */
-	match = libwacom_match_new(match_name, fw, bus, vendor_id, product_id);
+	match = libwacom_match_new(match_name, match_fw, bus, vendor_id, product_id);
  libwacom_set_default_match(ret, match);
  libwacom_match_unref(match);

I'll review one more time my kernel patches and submit them, I'll post the link.

jexposit · 2024-03-22T10:19:27Z

As promised, I opened my own PR:
#648

It includes a minimal change to get the firmware name from uniq.

whot · 2024-03-26T08:29:43Z

Closing in favour of #648 since that one is actually tested :) Thanks!

jexposit reviewed Dec 20, 2023

View reviewed changes

libwacom/libwacom.c Outdated Show resolved Hide resolved

jexposit reviewed Dec 20, 2023

View reviewed changes

libwacom/libwacom.c Outdated Show resolved Hide resolved

whot force-pushed the wip/fw-version branch 3 times, most recently from f85481e to 6ad598d Compare December 20, 2023 10:28

jexposit suggested changes Mar 16, 2024

View reviewed changes

whot added 5 commits March 18, 2024 16:05

data: show the name matching in the example tablet file

32ca577

Improve error debugging for invalid matches

8342404

This needs to be printed to stdout, otherwise meson won't show it.

Correct a warning referring to the wrong key

dd77a64

data: switch DeviceMatch to use a pipe as separator

280e3dc

Some device names include a colon which makes it hard to add extra optional components to the device matches.

Rework match_from_string

2134c56

No functional changes, this just preps the way for more optional elements in the match string.

whot force-pushed the wip/fw-version branch from 6ad598d to f0f5109 Compare March 18, 2024 06:12

jexposit reviewed Mar 18, 2024

View reviewed changes

whot force-pushed the wip/fw-version branch from f0f5109 to 075393e Compare March 18, 2024 10:37

jexposit mentioned this pull request Mar 22, 2024

Add support for firmware versions #648

Merged

whot closed this Mar 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for firmware versions #620

Add support for firmware versions #620

whot commented Dec 20, 2023

jexposit commented Dec 20, 2023

jexposit left a comment

jexposit Mar 16, 2024

jexposit Mar 16, 2024

whot Mar 18, 2024

jexposit Mar 18, 2024

whot Mar 19, 2024

whot commented Mar 18, 2024

jexposit commented Mar 18, 2024

jexposit commented Mar 18, 2024

whot commented Mar 19, 2024

jexposit commented Mar 21, 2024

jexposit commented Mar 22, 2024

whot commented Mar 26, 2024

Add support for firmware versions #620

Add support for firmware versions #620

Conversation

whot commented Dec 20, 2023

jexposit commented Dec 20, 2023

jexposit left a comment

Choose a reason for hiding this comment

jexposit Mar 16, 2024

Choose a reason for hiding this comment

jexposit Mar 16, 2024

Choose a reason for hiding this comment

whot Mar 18, 2024

Choose a reason for hiding this comment

jexposit Mar 18, 2024

Choose a reason for hiding this comment

whot Mar 19, 2024

Choose a reason for hiding this comment

whot commented Mar 18, 2024

jexposit commented Mar 18, 2024

jexposit commented Mar 18, 2024

whot commented Mar 19, 2024

jexposit commented Mar 21, 2024

jexposit commented Mar 22, 2024

whot commented Mar 26, 2024