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 implementation of XKeysWatcher for browser environments under web package #105

Merged
merged 5 commits into from
Aug 26, 2024

Conversation

loucadufault
Copy link
Contributor

@loucadufault loucadufault commented Jul 30, 2024

#104

Largely based on the existing implementation in https://github.com/SuperFlyTV/xkeys/blob/838adffd0b24b9a749e131855ea5d934f1c6c086/packages/node/src/watcher.ts

Key differences:

  • tracks seen devices by the actual HIDDevice instance references (stored in a Set), as opposed to the devicePath (since it is not available for web). Per the spec, these instances should remain stable across calls to the underlying navigator.hid.getDevices() (see https://wicg.github.io/webhid/#dom-hid-getdevices "6." > https://wicg.github.io/webhid/#dfn-devices).
  • in general, does not call the "hidden" methods _handleDeviceReconnected, _handleDeviceDisconnected. I was not sure if/why these calls were needed.
  • slightly simplified polling logic uses setTimeout callbacks calling each other, rather than setInterval. This lends itself to a slightly less verbose implementation, and has the added benefit of not flooding the event loop if the polling interval causes a timer rate higher than the system can handle.

Missing features:

  • automaticUnitIdMode

I tried to keep the code stylistically similar: comments, names, etc. Also included the "dead" debug logging code for convenience.

@loucadufault loucadufault changed the title add implementation of XKeysWatcher for browser environments under web package Add implementation of XKeysWatcher for browser environments under web package Jul 30, 2024
@loucadufault
Copy link
Contributor Author

loucadufault commented Jul 30, 2024

Full disclosure: this is completely untested. I'm hoping to be able to do some testing in the near future.

@Julusian
Copy link
Member

@loucadufault
Copy link
Contributor Author

loucadufault commented Jul 30, 2024

would the https://developer.mozilla.org/en-US/docs/Web/API/HID/connect_event and https://developer.mozilla.org/en-US/docs/Web/API/HID/disconnect_event be usable for this instead of polling?

Thanks for the suggestion.

Did some testing on my end with an X-Keys 128. Findings:

  • the browser permission dialog flow described here does not seem to remember the keyboard after un-plugging/re-plugging on the same tab. (EDIT: it does seem to be able to remember a device across page refresh so long as it remains connected).
  • the disconnect event is fired consistently (and only fires when the device is considered connected which happens only after calling requestDevice)
  • the connect event is never fired

The navigator.hid.connect event not being fired can be explained by the browser's inability to remember the keyboard, and an important fact missing from the MDN docs, that this chrome dev article alludes to:

When the website has been granted permission to access an HID device, it can actively receive connection and disconnection events by listening to "connect" and "disconnect" events.

Which suggests the events are only fired for devices which have been granted permission through a call to requestDevices. This makes sense, as it would otherwise leak devices to non-permissioned pages.

At least with my X-Keys device this suggests the connect event is not suitable. The disconnect event might be useful, remains to be seen how it compares to the library XKeys instance disconnected event. Perhaps both can be used to cover different cases.

Also, through testing the polling approach works quite well (have yet to validate the entire implementation).

@nytamin
Copy link
Member

nytamin commented Aug 12, 2024

Thanks for this contribution! Since this is a work-in-progress, I'll mark this PR as a draft for now.

I gave this an initial test in Chrome, these are my findings:

  • The basic logic in the XKeysWatcher seems to work (I have not run any extensive tests, though)
  • When I pull the plug of an xkeys panel and then reconnect it, the browser doesn't seem to remember the panel, so I need to request permissions for it again for it to show up in the list of devices.

Do you know if it's possible to get a browser to remember past connected panels? If not, I don't really see the point of adding the XKeysWatcher for browsers, since it won't add any functionality anyway? Or do you have a use case I'm missing?

@nytamin nytamin marked this pull request as draft August 12, 2024 07:25
Comment on lines +60 to +70
this.pollingTimeout = (setTimeout as Window['setTimeout'])(
async () => {
try {
await this.updateConnectedDevices()
} catch (e) {
console.error(e)
}
this.triggerUpdateConnectedDevices(false)
},
immediate ? 0 : this.options?.pollingInterval ?? DEFAULT_POLLING_INTERVAL_MS
)
Copy link
Member

Choose a reason for hiding this comment

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

You must not pass an async funtion to setTimeout, as a thrown error will result in an uncaught Error.

Suggested change
this.pollingTimeout = (setTimeout as Window['setTimeout'])(
async () => {
try {
await this.updateConnectedDevices()
} catch (e) {
console.error(e)
}
this.triggerUpdateConnectedDevices(false)
},
immediate ? 0 : this.options?.pollingInterval ?? DEFAULT_POLLING_INTERVAL_MS
)
this.pollingTimeout = (setTimeout as Window['setTimeout'])(
() => {
this.updateConnectedDevices()
.then(() => {
this.triggerUpdateConnectedDevices(false)
})
.catch((e) => {
this.emit('error', e)
})
},
immediate ? 0 : this.options?.pollingInterval ?? DEFAULT_POLLING_INTERVAL_MS
)

@loucadufault
Copy link
Contributor Author

Thanks for the feedback and review @nytamin .

When I pull the plug of an xkeys panel and then reconnect it, the browser doesn't seem to remember the panel, so I need to request permissions for it again for it to show up in the list of devices.

This matches the testing I had done.

Do you know if it's possible to get a browser to remember past connected panels?

I believe it has to do with the XKeys devices (at least the ones you and I have tested) not having a serial number https://issues.chromium.org/issues/40625708:

The change in #6 adds persistent permissions for USB devices that expose a serial number

And here is the log from chrome://device-log/:

[09:24:36] USB device added: vendor=1523 "P. I. Engineering", product=1227 "XK128Pi4", serial="", guid=3b3b6407-6770-4806-b0d7-17d1335120a4

It would seem this info would need to be set on the XKeys panels devices themselves... I am not sure whether this is achievable.


Regarding use cases for this implementation, the two use cases I have for this are:

  • to poll for new devices in a different part of the application as the requestXKeysPanels call was made (which itself returns the devices). In my case specifically, the call is made by some React button, while the device management logic happens much deeper in the pure JS code.
  • to manage disconnections from multiple sources; the Navigator.hid object, and XKeys objects themselves. In practice I am not sure whether it is necessary to handle both events, or if the XKeys 'disconnected' event even fires at all, so maybe this is not necessary.

@nytamin
Copy link
Member

nytamin commented Aug 14, 2024

if the XKeys 'disconnected' event even fires at all

Oh, that's a good spot that I haven't noticed before. The disconnect event doesn't fire in the WebHID version, which makes it inconsistent to the node-version. I addressed this in a separate PR: #107

And here is the log from chrome://device-log/ [...] serial="" [...]

Yeah, unfortunately the xkeys panels do not have any serial numbers in their hardware.

use case: to poll for new devices in a different part of the application as the requestXKeysPanels call was made

Ah, yes that's a reasonable use case.


I think, in order to make documentation easier and the code maintainable, that I'd like to refactor this so that most of the code is shared between the node and the webhid versions.

I've started to look into the refactoring, so I'll open another PR in a little while with those changes.

Note to self: Before merging, make sure to update the documentation to make it clear that the watcher won't work "out of the box" in a browser, and that it must be paired with a requestXkeysPanels() call to work.

@loucadufault
Copy link
Contributor Author

Sounds good to the above, and thanks for the patch in #107.

Let me know if you would like me to make changes to this PR once the refactors are opened.

@nytamin nytamin merged commit b47559a into SuperFlyTV:master Aug 26, 2024
5 of 6 checks passed
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.

3 participants