Add support for firmware versions

[whot]

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

[jexposit]

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 https://github.com/linuxwacom/libwacom/issues/609 and add the firmwares in the .tablet files.

I'll try to test it ASAP.

[whot]

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 https://github.com/linuxwacom/libwacom/pull/620#discussion_r1528223768: 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]

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]

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]

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]

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 https://github.com/linuxwacom/libwacom/issues/610#issuecomment-2012064189 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]

As promised, I opened my own PR: https://github.com/linuxwacom/libwacom/pull/648

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

[whot]

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