Refactors HID connections to support busy response #699
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
The main intended difference is that OpenSK now responds with ERR_CHANNEL_BUSY when it receives a packet while waiting for touch in in a CTAP2 command. To support this more elegantly, we changed `HidConnection` to have a `send` and `recv` instead of `send_and_maybe_recv` for a cleaner API. This also resolves an older TODO to respond to incoming channels on the other USB endpoint. It doesn't process the incoming traffic correctly, but unblock the host at least, and tells it to come back later. We still only allow one active channel at the same time. We now don't need the `TRANSMIT_AND_RECEIVE` syscall anymore. I didn't remove it from the Tock pack for simplicity, but cleaned up libtock-drivers. The Env API changes from having one connection per endpoint, to having one send function that takes an endpoint, and a receive function that receives on all endpoints at the same time. The `HidConnection` already received on all endpoints before, which was inconsistent with the existance of, e.g., `VendorHidConnection` being able to receive on the main endpoint. Since we now have a cleaner send and receive API, we use this in `src/main.rs` and simplify it a bit, while making it more consistent with other calls to the HID API. I found an additional inaccuracy in our implementation while refactoring: We want to send keepalives every 100 ms. To do so, we first wait for a button callback for 100 ms. Then we send the keepalive and check if a cancel packet appeared. What should have happened instead is that we listen for HID packets and button presses at the same time during these 100 ms. If nothing happens that stops the loop, we send the keepalive. The old implementation would, in some implementations, wait 200 ms for each keepalive: First 100 for touch, then 100 for `send_and_maybe_receive`. The new implementation can loop faster in case there is a lot of unrelated HID traffic. To make the touch timeout last 30 seconds, we introduce an extra timer and loop until time is up. This might still make us blink the LEDs too fast, but that is already the case in the main loop, and generally would need a bigger refactoring. The only simple workaround to make the LEDs slightly more precise is to wait for touch and packets until it is time to flip the LEDs, and if we return too early within some thresholds, wait again. If we care enough, we can fix that in a later PR. Not sure how to make the test work that I removed in ctap/mod.rs. I'd need to advance the time while the loop is running. Setting a user presence function that advances time seems hard with the Rust borrowing rules.
kaczmarczyck
force-pushed
the
send-recv-busy
branch
from
f533ae0
to
214b004
Compare
ia0
reviewed
Sep 2, 2024
kaczmarczyck
commented
Sep 2, 2024
There are a few small logic fixes hidden in this comit: - We always call `check_complete`, even if we received a cancel. - We only accept Cancel packets from the active channel. - Send timeouts now return `Ctap2StatusCode::CTAP1_ERR_TIMEOUT`.
|
This fix surfaced a bug in the USB stack: Keeping this PR open until I looked into the problem. |
Sounds good. Click the |
Before, if the device gets a CTAP2 UP request and another channel sends packets, the blinking pattern would have sped up. The patch for the Tock USB implementation fixes a panic after a timeout in send.
ia0
previously approved these changes
Sep 5, 2024
ia0
approved these changes
Sep 5, 2024
Comment on lines
+63
to
+64
| let mut locked_now_ms = self.now_ms.lock().unwrap(); | ||
| *locked_now_ms += milliseconds; |
There was a problem hiding this comment.
fyi, this could have just been *self.now_ms.lock().unwrap() += milliseconds
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.
The main intended difference is that OpenSK now responds with ERR_CHANNEL_BUSY when it receives a packet while waiting for touch in in a CTAP2 command.
To support this more elegantly, we changed
HidConnectionto have asendandrecvinstead ofsend_and_maybe_recvfor a cleaner API. This also resolves an older TODO to respond to incoming channels on the other USB endpoint.It doesn't process the incoming traffic correctly, but unblock the host at least, and tells it to come back later. We still only allow one active channel at the same time.
We now don't need the
TRANSMIT_AND_RECEIVEsyscall anymore. I didn't remove it from the Tock pack for simplicity, but cleaned up libtock-drivers.The Env API changes from having one connection per endpoint, to having one send function that takes an endpoint, and a receive function that receives on all endpoints at the same time.
The
HidConnectionalready received on all endpoints before, which was inconsistent with the existance of, e.g.,VendorHidConnectionbeing able to receive on the main endpoint.Since we now have a cleaner send and receive API, we use this in
src/main.rsand simplify it a bit, while making it more consistent with other calls to the HID API.I found an additional inaccuracy in our implementation while refactoring: We want to send keepalives every 100 ms. To do so, we first wait for a button callback for 100 ms. Then we send the keepalive and check if a cancel packet appeared. What should have happened instead is that we listen for HID packets and button presses at the same time during these 100 ms. If nothing happens that stops the loop, we send the keepalive.
The old implementation would, in some implementations, wait 200 ms for each keepalive: First 100 for touch, then 100 for
send_and_maybe_receive.The new implementation can loop faster in case there is a lot of unrelated HID traffic. To make the touch timeout last 30 seconds, we introduce an extra timer and loop until time is up. This might still make us blink the LEDs too fast, but that is already the case in the main loop, and generally would need a bigger refactoring.
The only simple workaround to make the LEDs slightly more precise is to wait for touch and packets until it is time to flip the LEDs, and if we return too early within some thresholds, wait again. If we care enough, we can fix that in a later PR.
Not sure how to make the test work that I removed in ctap/mod.rs. I'd need to advance the time while the loop is running. Setting a user presence function that advances time seems hard with the Rust borrowing rules.