feat: hid rpc channel #755
Conversation
IDisposable
left a comment
IDisposable
left a comment
There was a problem hiding this comment.
Wow, that was fast :) suggestions/questions are not blockers
| CapsLock bool `json:"caps_lock"` | ||
| ScrollLock bool `json:"scroll_lock"` | ||
| Compose bool `json:"compose"` | ||
| Kana bool `json:"kana"` |
There was a problem hiding this comment.
I wished I had killed off these bools already :(
There was a problem hiding this comment.
This was kept for backwards compatibility.
ui/src/hooks/useKeyboard.ts
Outdated
| if (rpcHidReady) { | ||
| console.debug("Sending keypress event via HidRPC"); | ||
| reportKeypressEvent(key, press); | ||
| return; | ||
| } | ||
|
|
||
| if (keyPressReportApiAvailable) { | ||
| // if the keyPress api is available, we can just send the key press event | ||
| sendKeypressEvent(key, press); |
There was a problem hiding this comment.
There is a bit of duplicate logic:
keyPressReportApiAvailableisn’t useful, since it’s effectively the same as rpcHidReady.sendKeypressEvent()duplicates the preceding if statement in this function. Just replacekeyPressReportApiAvailablewithrpcHidReadyand remove the preceding if statement.
ui/src/components/WebRTCVideo.tsx
Outdated
| send("relMouseReport", { dx: calcDelta(x), dy: calcDelta(y), buttons }); | ||
| const dx = calcDelta(x); | ||
| const dy = calcDelta(y); | ||
| if (rpcHidReady) { |
There was a problem hiding this comment.
I’d suggest not using useHidRPC() directly for mouse and keyboard events. Instead, let hidRPC serve purely as a transport layer, with dedicated hooks handling input logic.
We use useKeyboard() for keyboard input and useRPC() for mouse reports. Mixing approaches like this adds confusion and clutters an already complex HID JS API surface.
A clearer approach would be to introduce both useMouse() and useKeyboard() hooks. That way, consumption is consistent and obvious, while each hook can encapsulate its own complexity.
We added useKeyboard() out of necessity because its fallback logic was big. That isn’t the case for the mouse, where the fallback is rather simple -- but, I would say the added clarity and consistency still make a useMouse() hook worthwhile.
There was a problem hiding this comment.
Good point, moving mouse logic to useMouse
ui/src/hooks/useMouse.ts
Outdated
| const getRelMouseMoveHandler = useCallback( | ||
| ({ isPointerLockActive, isPointerLockPossible }: RelMouseMoveHandlerProps) => (e: MouseEvent) => { | ||
| if (mouseMode !== "relative") return; | ||
| if (isPointerLockActive === false && isPointerLockPossible) return; |
There was a problem hiding this comment.
I know you didn't actually change this (just moved here from WebRTVideo.tsx), but this seems odd... do we only support relative mouse mode if we can lock the pointer, AND have locked the pointer?
If that's the case, then what happens if we haven't locked? We ignore all mouse events?
There was a problem hiding this comment.
Yes, it's definitely odd ... Let me just remove it
ui/src/hooks/stores.ts
Outdated
|
|
||
| keyPressReportApiAvailable: boolean; | ||
| setkeyPressReportApiAvailable: (available: boolean) => void; | ||
| // keyPressReportApiAvailable is no longer needed, we'll simply use hidChannel available to |
There was a problem hiding this comment.
Could probably just delete this commented-out code.
| }; | ||
|
|
||
| document.addEventListener("fullscreenchange ", handleFullscreenChange); | ||
| document.addEventListener("fullscreenchange", handleFullscreenChange); |
There was a problem hiding this comment.
WOW, nice catch... this could explain some of the oddities of focus when going in and out of fullscreen repeatedly
IDisposable
left a comment
IDisposable
left a comment
There was a problem hiding this comment.
(we can discuss the relative mouse locked mode elsewhere)
|
|
* feat: use hidRpcChannel to save bandwidth * chore: simplify handshake of hid rpc * add logs * chore: add timeout when writing to hid endpoints * fix issues * chore: show hid rpc version * refactor hidrpc marshal / unmarshal * add queues for keyboard / mouse event * chore: change logging level of JSONRPC send event to trace * minor changes related to logging * fix: nil check * chore: add comments and remove unused code * add useMouse * chore: log msg data only when debug or trace mode * chore: make tslint happy * chore: unlock keyboardStateLock before calling onKeysDownChange * chore: remove keyPressReportApiAvailable * chore: change version handle * chore: clean up unused functions * remove comments
This PR moved HID events to a dedicated queue and switched from JSONRPC to a custom binary format to improve performance under poor network conditions.
It's also worth noting that the
/dev/hidg*endpoints can sometimes hang indefinitely, so a timeout was introduced to prevent the queue from being blocked forever in such cases.