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

Report routing revisited #1021

Closed
kareltucek opened this issue Nov 13, 2024 · 9 comments · Fixed by #1031
Closed

Report routing revisited #1021

kareltucek opened this issue Nov 13, 2024 · 9 comments · Fixed by #1031
Assignees

Comments

@kareltucek
Copy link
Collaborator

@benedekkupper, we would like to have more control over the bt/usb HID interfaces.

Until now, whenever an unknown bluetooth device connected, we have (very naively) assumed it is a HID device, and disabled USB HID, and consequently, have treated the connection states as mutually exclusive. This doesn't fit the bill for muliple reasons.

  • we would like to know the connection states independently (i.e., call the DeviceState_SetConnection in void hidmgr_set_transport(const hid::transport* tp) less naively)
  • when both are connected, we would like to be able to decide where a report is to be sent (basically implementing a kvm switch) (e.g., something like mouse_app.selec_destination(bt / usb); would fit the bill nicely.)
  • specifically, if I am not mistaken, in certain situations, we would like to send the commands app reports over USB, while sending keyboard app reports over bluetooth

(Do we already have such ability? If now, what course of action do you suggest?)

@benedekkupper
Copy link
Contributor

What you are asking for is to keep both connections alive at the same time, maintaining their state independently. That to me translates to having multiple instances of keyboard_app, etc, so when you swap between the transports, you take over the new transport's configuration (boot mode, more importantly caps/num/scroll lock, etc). Additionally, care should be taken that a switch happens at a time when the keys state is clear, so no key is stuck on the old transport.

That means instead of having a keyboard_app::handle(), you will have ble_handle() and usb_handle(), where you can hook into their start and stop methods to know which transport is available, and use the sending method of the appropriate handle.

@kareltucek
Copy link
Collaborator Author

That means instead of having a keyboard_app::handle(), you will have ble_handle() and usb_handle(), where you can hook into their start and stop methods to know which transport is available, and use the sending method of the appropriate handle.

Sounds about right.

It also sounds like it might result in a lot of code duplication. If that is the case, then we should probably think about it a bit more deeply. (Like, what if we decide we want to maintain multiple ble hid connections at the same time?)

(Please make it happen when time and priorities allow.)

@benedekkupper
Copy link
Contributor

Like, what if we decide we want to maintain multiple ble hid connections at the same time?

That's trickier actually, and another thing to consider altogether, it would need a significant rewrite of the BLE HID implementation in the c2usb library.

@kareltucek
Copy link
Collaborator Author

Alright, me and Laszlo have agreed that:

  • we definitely don't need simultaneous BLE HID connections at the moment. In the future, there is a slim chance we might like to support them, or maybe not, but either way, it is not an important feature for us, so the time investment is not justified.
  • the original request - being able to send keyboard/mouse over different HID than the command interface (Agent) holds.

If I understand you correctly:

  • implementing simultaneous BLE HIDS would be laborous
  • implementing the 1 usb hid + 1 ble hid variant is much less laborous
  • the two scenarios are more or less independent and do not impose major costs on each other.

To be clear, in case it simplifies matters, we don't strictly need simultaneous connection of the keyboard_app (mouse_app, control_app) over both HIDs. We just need for command_app to be connected to potentially different HID than the rest of the interfaces.

Should I clarify / elaborate on anything?

@benedekkupper
Copy link
Contributor

If I understand you correctly:

you do, in all areas

To be clear, in case it simplifies matters, we don't strictly need simultaneous connection of the keyboard_app (mouse_app, control_app) over both HIDs. We just need for command_app to be connected to potentially different HID than the rest of the interfaces.

It would simplify matters indeed, as in the case of BLE, (due to Android limitation) the HID interfaces are all or nothing, while on USB we can pick and choose which interfaces are desired - with the caveat that the command interface's index will be shifting around if we do switch things around at runtime. I'm hoping that Agent doesn't have this hardcoded anymore, and instead searches for the HID usage code. Is this what you want as a first step? Have the agent command interface be always available via USB, even when BLE HID is connected?

@kareltucek
Copy link
Collaborator Author

Sorry 😅.

with the caveat that the command interface's index will be shifting around if we do switch things around at runtime. I'm hoping that Agent doesn't have this hardcoded anymore, and instead searches for the HID usage code.

No idea. @ert78gb ?

Is this what you want as a first step? Have the agent command interface be always available via USB, even when BLE HID is connected?

Yes, I think so.

So we are talking about:

  • command interface always available via USB
  • knowing independently whether BLE HID is available and whether USB HID is available
  • being able to switch or pick the target connection for HID reports one way or another

Or just part of them?

@ert78gb
Copy link
Member

ert78gb commented Nov 24, 2024

Agent use usage, usePage USB descriptors to identify the communication USB interface.

https://github.com/UltimateHackingKeyboard/agent80/blob/22cd23cf578d5de82c83498043812fa833e42f9b/packages/uhk-usb/src/util.ts#L89

benedekkupper referenced this issue in UltimateHackingKeyboard/firmware80 Nov 28, 2024
benedekkupper referenced this issue in UltimateHackingKeyboard/firmware80 Dec 4, 2024
benedekkupper referenced this issue in UltimateHackingKeyboard/firmware80 Dec 4, 2024
benedekkupper referenced this issue in UltimateHackingKeyboard/firmware80 Dec 5, 2024
benedekkupper referenced this issue in UltimateHackingKeyboard/firmware80 Dec 5, 2024
benedekkupper referenced this issue in UltimateHackingKeyboard/firmware80 Dec 5, 2024
@mondalaci mondalaci transferred this issue from UltimateHackingKeyboard/firmware80 Dec 12, 2024
@kareltucek
Copy link
Collaborator Author

kareltucek commented Dec 26, 2024

@benedekkupper what is the state of affairs?

From my point of view, current blockers are:

  • reliable way to tell c2usb to switch to usb or ble hid when both are connected.
    • I am still not sure what the USB_EnableHid/USB_DisableHid is exactly supposed to do - whether to switch between usb hid and ble hid operations, or something else? Would you either elaborate (best in code comment), or refactor it somehow?
    • In any case, when Ble Hid is connected and I call USB_EnableHid, the firmware tends to crash, so I guess I am not supposed to do that while hogp is active.
  • When ble hid is connected, what is the recommended way to tell whether usb hid is available?
  • void hidmgr_set_transport(const hid::transport *tp) is unreliable. Just like in the UHK80 with Windows is in disconnected state after waking up from sleep #1060 it often ends up in NULL getting reported as the last state even though usb connection is available.

Other less important thoughts - if you have any comment, it is highly welcome:

  • I am not sure how c2usb actually takes over the ble hid connections. I would expect the bt_conn.c code to explicitly call c2usb, telling it "here you have a ble connection handle, please establish hid over it".
  • It would be nice to actually check somewhere that the device that connects is asking for ble hid rather than nus. At the moment the code assumes that if an address is not known, then it is ble hid does a bunch of guesses based on host connections contained in the user config and a gatt discovery.

@benedekkupper
Copy link
Contributor

What I implemented in the previous PR was only the concurrent agent operation over USB and BLE. Both can be used at the same time, since it's fully a request-response protocol, the response is always sent where the request comes from.

@kareltucek Now on the parallel-host-routing branch I have added the changes I proposed earlier, where both USB and BLE HID are live in parallel, and the UHK itself needs to make the decision which way to go. This is WIP, since the actual decision whether to send via usb_handle or ble_handle needs to be made by the routing logic - see the TODOs I made in usb_compatibility.cpp. Besides that, the keyboard LED management also needs to track the USB and BLE host states in parallel independently, so KeyboardLedsState needs to be redesigned to accommodate this. (Also note that I moved Connections_SetState calls to keyboard_app.cpp, please review it.)

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 a pull request may close this issue.

3 participants