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

[Bug] Edimax BLE Dongle Fails After Teardown and Re-Instantiation #534

Merged
merged 3 commits into from
Sep 11, 2024

Conversation

hkpeprah
Copy link
Contributor

Bug: Edimax BLE Dongle Fails After Teardown and Re-Instantiation

Overview

Replacement for #497 with the logic for the HCI parser removed now that packets are handled without a parser for USB. This bug was observed with some RTK BLE dongles (specifically cheaper CSR dongles as well as the Edimax BT-8500). Sometimes, when a reset is attempted, the host will fail to receive a result, instead receiving an invalid packet or corrupt data. This results in blocking forever in driver_info_for_host(). In testing, I found that allowing that to time out, then trying again can work with dongles like the Edimax.

With cheaper CSR dongles, sometimes the first packet in response to a command after the reset will be an invalid packet. This occurs when the Local Version request is sent. The result is similar to the above in which case we wait forever on a packet that isn't coming in. In testing, I found that re-sending the command can sometimes avoid this, but not in all cases, so in the case that we do not get the local version information, we should skip over it and return upstream.

Summary of Changes

  1. In RTK driver_info_for_host attempt a HCI reset with a timeout; on time out, retry the HCI reset and assume the host is ready.
  2. In RTK driver_info_for_host exit if the Local Version Information response is invalid.
  3. In bumble/host.py, for send_command() allow an optional response_timeout to allow the caller to time out if a response is not received.
  4. In bumble/host.py, for on_packet(), do not crash if from_bytes() fails to parse an HCI packet as may be the case with invalid data.

@barbibulle
Copy link
Collaborator

This PR is good to go. Can you please just fix the linter/formatter errors before we can merge it?

@hkpeprah
Copy link
Contributor Author

This PR is good to go. Can you please just fix the linter/formatter errors before we can merge it?

Done!

@barbibulle
Copy link
Collaborator

This PR is good to go. Can you please just fix the linter/formatter errors before we can merge it?

Done!

There are still some type checker errors (see the output of the GitHub actions that run on the PR, or runinvoke project.pre-commit to run the checks locally.
Those are not type checking errors that were introduced by your change, but they are only detected now, because your change adds type annotations to the send_command functions (which is good, as we're adding type annotations throughout the code base as time goes), which means that now that function will be type-checked, whereas it wasn't before.
The type checker output says:

bumble/transport/usb.py:118: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
bumble/host.py:526: error: Incompatible types in assignment (expression has type "Future[Any]", variable has type "None")  [assignment]
bumble/host.py:531: error: Argument 1 to "wait_for" has incompatible type "None"; expected "Union[Future[Never], Generator[Any, None, Never], Awaitable[Never]]"  [arg-type]
bumble/host.py:532: error: "None" has no attribute "result"  [attr-defined]
bumble/host.py:537: error: "HCI_Command_Status_Event" has no attribute "status"  [attr-defined]

Those errors can be fixed easily:
self.pending_response should be explicitly typed, so that the assignment to a non-None value doesn't generate an error (probably something like self.pending_response: Future = None on line 174 would work fine.

This patch addresses an issue where the some RTK BLE dongles fail to perform
an HCI reset after the transport is torn down and re-instantiated. To address
that, we prevent crashing the background threads when invalid data comes in,
and time out if no response is received within a fixed amount of time. When
the timeout occurs, we retry the reset, and ultimately skip over reading the
local version information if that fails.
@hkpeprah
Copy link
Contributor Author

hkpeprah commented Sep 9, 2024

I'm not sure how to work around this one:

bumble/host.py:537: error: "HCI_Command_Status_Event" has no attribute "status"  [attr-defined]

It seems like MyPy doesn't like how the constructors for the events are done, as it can't tell the class instance has an attribute called status.

@barbibulle
Copy link
Collaborator

I'm not sure how to work around this one:

bumble/host.py:537: error: "HCI_Command_Status_Event" has no attribute "status"  [attr-defined]

It seems like MyPy doesn't like how the constructors for the events are done, as it can't tell the class instance has an attribute called status.

The comment # type: ignore[attr-defined] should solve the issue I think.

@barbibulle barbibulle merged commit 0f71a63 into google:main Sep 11, 2024
57 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.

2 participants