Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for firmware versions #648

Merged
merged 3 commits into from
Apr 17, 2024
Merged

Conversation

jexposit
Copy link
Contributor

Copy link
Member

@whot whot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One typo found, would be great if you could squash that in. Otherwise this LGTM, we can merge this once upstream accepts your kernel patch. Can you ping me when that happens? Thanks

data/wacom.example Outdated Show resolved Hide resolved
G_REGEX_DEFAULT,
G_REGEX_MATCH_DEFAULT,
NULL);
g_regex_match (regex, fw, 0, &match_info);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: this may become an issue if we ever get a fw exported that has a different underscorify pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong preference for this regex (I usually have a strong preference against using regexes), but in this case I think that makes easier to parse the fw name.

If you prefer to match against a list of well known fw names, I'm happy to change this function.

@whot
Copy link
Member

whot commented Mar 26, 2024

ftr I have merged the first few cleanup patches, so a rebase should half the commit list

@jexposit
Copy link
Contributor Author

jexposit commented Apr 5, 2024

@whot the kernel patch has been merged.

In the end, instead of creating a custom sysfs property, we ended up using hid->uniq. Since this a generic attribute all devices has, I wonder if it makes sense to refactor a little bit the series to use uniq instead of fw as variable names and update the documentation.

Something else I have in my to-do list is displaying that information with libwacom-list-local-devices so user can actually discover it.

@whot
Copy link
Member

whot commented Apr 10, 2024

I wonder if it makes sense to refactor a little bit the series to use uniq instead of fw as variable names and update the documentation.

yes, definitely. This also leaves room for a future firmware attribute if uniq is no longer sufficient.

Something else I have in my to-do list is displaying that information with libwacom-list-local-devices so user can actually discover it.

I think --format=datafile should work automatically, the yaml format will need an extra entry though.

@jexposit
Copy link
Contributor Author

I wonder if it makes sense to refactor a little bit the series to use uniq instead of fw as variable names and update the documentation.

yes, definitely. This also leaves room for a future firmware attribute if uniq is no longer sufficient.

Refactored the last patch to use uniq instead of fw

Something else I have in my to-do list is displaying that information with libwacom-list-local-devices so user can actually discover it.

I think --format=datafile should work automatically, the yaml format will need an extra entry though.

Also added an extra entry to tools/list-devices.c

Copy link
Member

@whot whot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one indentation error, rest looks good, thanks!

libwacom/libwacom.c Outdated Show resolved Hide resolved
libwacom/libwacom.sym Outdated Show resolved Hide resolved
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
Copy link
Contributor Author

Resolved and rebased. Thanks for the review!

@whot
Copy link
Member

whot commented Apr 16, 2024

../libwacom/libwacom.c:234:33: error: ‘G_REGEX_DEFAULT’ undeclared (first use in this function); did you mean ‘G_REGEX_DOTALL’?

Introduced in 2.74 and that's too new for Ubuntu 22.04. Looking at glib commit 2.73.1-26-g879b9cd669f0 we should be fine with an

#ifndef G_REGEX_DEFAULT
#define G_REGEX_DEFAULT 0
#endif

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.
@jexposit
Copy link
Contributor Author

Ops, missed the CI emails. I replaced compile_options and match_options with 0 as in the docs:
https://docs.gtk.org/glib/ctor.Regex.new.html

@jexposit
Copy link
Contributor Author

Is the FreeBSD failure expected?

@whot
Copy link
Member

whot commented Apr 16, 2024

Is the FreeBSD failure expected?

yeah, it's very unreliable and fails often (all the time?) but I'm ETIME on debugging that. cc @jbeich

@whot whot merged commit cb5b570 into linuxwacom:master Apr 17, 2024
12 of 13 checks passed
@whot
Copy link
Member

whot commented Apr 17, 2024

I think we're good here, let's merge this! thanks for all the effort

@jbeich
Copy link
Contributor

jbeich commented Apr 20, 2024

it's very unreliable

Try applying labwc/labwc@00ec29c834b7. I suspect v0 isn't maintained anymore or emulating on macOS is still unstable.

@whot
Copy link
Member

whot commented Apr 22, 2024

@jbeich thanks, that was it, fixed in #662

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fixing vendors that re-use USB IDs
3 participants