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

Rework some device and hci functions #514

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

zxzxwu
Copy link
Collaborator

@zxzxwu zxzxwu commented Jul 16, 2024

  • Add type hints
  • Add wait_for_complete behavior for async commands

* Add type hints
* Add wait_for_complete behavior for async commands
max_latency: int,
supervision_timeout: int,
use_l2cap: bool = False,
wait_for_complete: bool = False,
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe wait_for_completion would be a better name for this parameter.

phy_options: int = 0,
wait_for_complete: bool = False,
) -> None:
"""Sets PHY preference of this connection.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this docstring is essentially the same as the one for device.set_connection_phy, maybe there's a clever way to just say "See XYZ" and refer to that other location, rather than have a copy of the docstring, which is hard to maintain in both places.

supervision_timeout=supervision_timeout,
min_ce_length=min_ce_length,
max_ce_length=max_ce_length,
with closing(EventWatcher()) as watcher:
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: move the call to self.send_command() to a local function, which the code followibng with closing(...) ...: can invoke, but can also be invoked without the with close when wait_for_completion is False, so that we don't unnecessarily create the context and the future in that case.

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