fix: filter for correct usage/usagePage when finding xkeys devices #93
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hey @michaelhatPIengineering ! function isValidXkeysUsage(device: HIDDevice): boolean {
return !!device.collections.find(
(collection) => collection.type === 1 && collection.usage === 1 && collection.usagePage === 12
)
}Does this filter look reasonable to you? Do you think we should go ahead and merge this fix right away or would you like to test it with various devices first? (I could prepare a test script for you) |
|
Johan,
To 100% and to future proof this, we should add && collection.writelength > 20
or what ever the way to see the write length of the "device"
There are some Multi-media devices that also use page 12 so we always check both.
Also, this problem is not just a browser problem, all other OSs could change how they enumerate and would not be out of spec.
So, this filter should be added to all.
MHH
…________________________________
From: Johan Nyman ***@***.***>
Sent: Wednesday, October 25, 2023 3:47 AM
To: SuperFlyTV/xkeys ***@***.***>
Cc: Michael Hetherington ***@***.***>; Mention ***@***.***>
Subject: Re: [SuperFlyTV/xkeys] fix: filter for correct usage/usagePage when finding xkeys devices (PR #93)
Hey @michaelhatPIengineering<https://github.com/michaelhatPIengineering> !
This PR basically adds the filter below (for browser WebHID only):
function isValidXkeysUsage(device: HIDDevice): boolean {
return !!device.collections.find(
(collection) => collection.type === 1 && collection.usage === 1 && collection.usagePage === 12
)
}
Does this filter look reasonable to you? Do you think we should go ahead and merge this fix right away or would you like to test it with various devices first? (I could prepare a test script for you)
—
Reply to this email directly, view it on GitHub<#93 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AS6L3QW5VG2XKKQXGPJQXPTYBC725AVCNFSM6AAAAAA6O4W4OGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONZYG4YDENBUGU>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
We are checking on how this works with all our device descriptors.
A test script might be good.
…________________________________
From: Johan Nyman ***@***.***>
Sent: Wednesday, October 25, 2023 3:47 AM
To: SuperFlyTV/xkeys ***@***.***>
Cc: Michael Hetherington ***@***.***>; Mention ***@***.***>
Subject: Re: [SuperFlyTV/xkeys] fix: filter for correct usage/usagePage when finding xkeys devices (PR #93)
Hey @michaelhatPIengineering<https://github.com/michaelhatPIengineering> !
This PR basically adds the filter below (for browser WebHID only):
function isValidXkeysUsage(device: HIDDevice): boolean {
return !!device.collections.find(
(collection) => collection.type === 1 && collection.usage === 1 && collection.usagePage === 12
)
}
Does this filter look reasonable to you? Do you think we should go ahead and merge this fix right away or would you like to test it with various devices first? (I could prepare a test script for you)
—
Reply to this email directly, view it on GitHub<#93 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AS6L3QW5VG2XKKQXGPJQXPTYBC725AVCNFSM6AAAAAA6O4W4OGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONZYG4YDENBUGU>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
it seems like the fix worked on Mac, but not on Windows |
|
looks like |
|
I reviewed with our firmware engineer; she recommends:
filter on:
VID
PID
Usage page 12
write length > 20
to get the "device" supported by this lib.
other things are redundant or not consistent over all OSs
…________________________________
From: Khang Li ***@***.***>
Sent: Wednesday, October 25, 2023 10:32 AM
To: SuperFlyTV/xkeys ***@***.***>
Cc: Michael Hetherington ***@***.***>; Mention ***@***.***>
Subject: Re: [SuperFlyTV/xkeys] fix: filter for correct usage/usagePage when finding xkeys devices (PR #93)
looks like collection.type === 0 for XKeys XK-80 on Windows
—
Reply to this email directly, view it on GitHub<#93 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AS6L3QVOF6GVOBTF7TXARXTYBEPINAVCNFSM6AAAAAA6O4W4OGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONZZGQYTOOBXGQ>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
I have stripped out some of the conditions, and left it checking I haven't added a writeLength check, as doing so requires digging deeply into multiple layers of arrays and objects and that makes me a little uneasy about the reliability of doing so. Although each of linux, macos and windows did produce what appeared to be the same data for it:
|
Julusian
force-pushed
the
fix/webhid-wrong-device
branch
from
7d09fe6
to
68f3e86
Compare
|
We have found that writeLenght is important to avoid confusion with multi-media which is also the same usage page.
…________________________________
From: Julian Waller ***@***.***>
Sent: Wednesday, November 1, 2023 11:41 AM
To: SuperFlyTV/xkeys ***@***.***>
Cc: Michael Hetherington ***@***.***>; Mention ***@***.***>
Subject: Re: [SuperFlyTV/xkeys] fix: filter for correct usage/usagePage when finding xkeys devices (PR #93)
I have stripped out some of the conditions, and left it checking usagePage and vendorId.
It is validating the productId later, so I don't think it is necessary to do at this early stage, especially as the user prompt isnt doing a filter on productId.
I haven't added a writeLength check, as doing so requires digging deeply into multiple layers of arrays and objects and that makes me a little uneasy about the reliability of doing so. Although each of linux, macos and windows did produce what appeared to be the same data for it:
[image]<https://user-images.githubusercontent.com/1327476/279719982-c1966e68-8300-45d2-96fc-55d6d6bc80c5.png>
—
Reply to this email directly, view it on GitHub<#93 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AS6L3QXNLNXCMQOKLYMOWF3YCJUTTAVCNFSM6AAAAAA6O4W4OGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOBZGE4DINBRHE>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
ok, I have added that check. The function now looks like: function isValidXkeysUsage(device: HIDDevice): boolean {
if (device.vendorId !== XKEYS_VENDOR_ID) return false
return !!device.collections.find((collection) => {
if (collection.usagePage !== 12) return false
// Check the write-length of the device is > 20
return !!collection.outputReports?.find((report) => !!report.items?.find((item) => item.reportCount ?? 0 > 20))
})
}I have tested that this new version still works on windows, macos and linux |
|
Looks good, we have found that those checks always work in all our other projects as well.
Thanks
…________________________________
From: Julian Waller ***@***.***>
Sent: Wednesday, November 1, 2023 12:40 PM
To: SuperFlyTV/xkeys ***@***.***>
Cc: Michael Hetherington ***@***.***>; Mention ***@***.***>
Subject: Re: [SuperFlyTV/xkeys] fix: filter for correct usage/usagePage when finding xkeys devices (PR #93)
ok, I have added that check. The function now looks like:
function isValidXkeysUsage(device: HIDDevice): boolean {
if (device.vendorId !== XKEYS_VENDOR_ID) return false
return !!device.collections.find((collection) => {
if (collection.usagePage !== 12) return false
// Check the write-length of the device is > 20
return !!collection.outputReports?.find((report) => !!report.items?.find((item) => item.reportCount ?? 0 > 20))
})
}
I have tested that this new version still works on windows, macos and linux
—
Reply to this email directly, view it on GitHub<#93 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AS6L3QQENVRQXM7GUFHYAO3YCJ3Q3AVCNFSM6AAAAAA6O4W4OGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOBZGI4DQNZSHA>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
nytamin
requested changes
Nov 2, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #92
The filtering might be overly strict, and needs to be verified on windows and macos to ensure the required properties are defined.
Tested with a XK24 and XK12-Jog