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

Add support for Zephyr HCI VSC set TX power command #258

Merged
merged 1 commit into from
Sep 12, 2023

Conversation

mogenson
Copy link
Collaborator

@mogenson mogenson commented Aug 25, 2023

Add HCI Zephyr vendor commands to read and write TX power

Create platforms/zephyr/hci.py with definitions of vendor HCI commands
to read and write TX power.

Add documentation for how to prepare an nRF52840 dongle with a Zephyr
HCI USB firmware application that includes dynamic TX power support and
how to send a write TX power vendor HCI command from Bumble

README.md Outdated
@@ -63,3 +63,23 @@ This is not an official Google product.
This library is in alpha and will be going through a lot of breaking changes. While releases will be stable enough for prototyping, experimentation and research, we do not recommend using it in any production environment yet.
Expect bugs and sharp edges.
Please help by trying it out, reporting bugs, and letting us know what you think!

# TX Power Control for nrf52840 dongles
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this doc should live in a section under docs/mkdocs/src (maybe platforms/zephyr.md?) rather than the top-level README, which should remain very generic.

bumble/hci.py Outdated
@@ -557,6 +557,13 @@ def phy_list_to_bits(phys):
HCI_LE_SET_DEFAULT_SUBRATE_COMMAND = hci_command_op_code(0x08, 0x007D)
HCI_LE_SUBRATE_REQUEST_COMMAND = hci_command_op_code(0x08, 0x007E)

# Vendor specific HCI commands
HCI_VSC_TX_POWER_HANDLE_TYPE_ADV = 0x00
Copy link
Collaborator

Choose a reason for hiding this comment

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

Those 3 constants should be class members of the HCI_VSC_Write_Tx_Power_Level_Command class I think.

bumble/hci.py Outdated
('selected_tx_power_level', -1),
],
)
class HCI_VSC_Write_Tx_Power_Level_Command(HCI_Command):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a doc/spec we can point to for this vendor command?

hci_usb.zip Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should find a way to host this type of pre-built firmware images on the static pages rather than in the Git repo.

@mogenson
Copy link
Collaborator Author

Hi @barbibulle thanks for the comments. The vendor HCI command for TX power is here: https://github.com/zephyrproject-rtos/zephyr/blob/a15e7d2326d9e8896401be479fae56f30dde71bb/include/zephyr/bluetooth/hci_vs.h#L163

However, I'm going to close this PR because I don't think it makes sense to have vendor commands for specific hardware that require custom built firmware as part of Bumble. I will add the definition for these commands at the application level for our Python project. I just wanted to use this PR to demonstrate the feasibility of setting TX power.

@mogenson mogenson closed this Aug 30, 2023
@barbibulle
Copy link
Collaborator

Actually, I think it would be great to make these HCI commands and tool extension part of the main project. I’m sure others will find it useful, even if Nordic/Zephyr specific.

@barbibulle barbibulle reopened this Aug 30, 2023
@mogenson
Copy link
Collaborator Author

Ok then! Is a vendor command section of hci.py a good location to place these command definitions?

Is any sort of Device level API needed, or is host.send_command() good enough?

Should we gate the presence of these commands with some sort of parameter in a device.json config file?

@barbibulle
Copy link
Collaborator

barbibulle commented Aug 30, 2023

It might be good to put the vendor HCI packet definitions in their own file. Maybe hci_vendor.py or hci_vsc.py. I’d leave it as low level commands for now. For normal HCI commands, there’s a way to query the controller if the command is supported, but I don’t know of a way to do that for vendor commands (the answer comes in the form of a long bit mask, so not extensible to custom extensions). It’s probably safe to let users decide if they want/need to send those commands, and let them catch errors if they send the command to a controller without knowing if their controller supports it. Since the high level APIs would not send those commands directly, we wouldn’t need a config entry to enable/disable them.

@barbibulle
Copy link
Collaborator

It might be good to put the vendor HCI packet definitions in their own file. Maybe hci_vendor.py or hci_vsc.py.

I will push a PR for review with a structure and supporting functions for separating vendor HCI commands in discrete modules.

@mogenson
Copy link
Collaborator Author

mogenson commented Aug 31, 2023

I will push a PR for review with a structure and supporting functions for separating vendor HCI commands in discrete modules.

Great thanks!!

@uael
Copy link
Contributor

uael commented Sep 1, 2023

It might be good to put the vendor HCI packet definitions in their own file. Maybe hci_vendor.py or hci_vsc.py. I’d leave it as low level commands for now. For normal HCI commands, there’s a way to query the controller if the command is supported, but I don’t know of a way to do that for vendor commands (the answer comes in the form of a long bit mask, so not extensible to custom extensions). It’s probably safe to let users decide if they want/need to send those commands, and let them catch errors if they send the command to a controller without knowing if their controller supports it. Since the high level APIs would not send those commands directly, we wouldn’t need a config entry to enable/disable them.

I suggest to put them in their own files like in bumble/vendor/{zephyr,rtk}.py and for ex. with realtek use the commands in bumble/drivers/rtk.py

@barbibulle
Copy link
Collaborator

It might be good to put the vendor HCI packet definitions in their own file. Maybe hci_vendor.py or hci_vsc.py. I’d leave it as low level commands for now. For normal HCI commands, there’s a way to query the controller if the command is supported, but I don’t know of a way to do that for vendor commands (the answer comes in the form of a long bit mask, so not extensible to custom extensions). It’s probably safe to let users decide if they want/need to send those commands, and let them catch errors if they send the command to a controller without knowing if their controller supports it. Since the high level APIs would not send those commands directly, we wouldn’t need a config entry to enable/disable them.

I suggest to put them in their own files like in bumble/vendor/{zephyr,rtk}.py and for ex. with realtek use the commands in bumble/drivers/rtk.py

PR up for review: #269

@mogenson mogenson changed the title DO NOT MERGE. Add support for Zephyr HCI VSC set TX power command Add support for Zephyr HCI VSC set TX power command Sep 8, 2023
@mogenson
Copy link
Collaborator Author

mogenson commented Sep 8, 2023

@barbibulle @uael This PR is now ready to be reviewed and can be merged.

I was searching for a good way to host the prebuilt hci_usb.zip file as part of the online docs. I ended up creating a discussion thread here and uploading the file. It appears that there is a static Github URL that can be include in the mkdocs.

Create platforms/zephyr/hci.py with definitions of vendor HCI commands
to read and write TX power.

Add documentation for how to prepare an nRF52840 dongle with a Zephyr
HCI USB firmware application that includes dynamic TX power support and
how to send a write TX power vendor HCI command from Bumble.
@mogenson
Copy link
Collaborator Author

@barbibulle Changed the Zephyr firmware link to point to downloads/zephyr/hci_usb.zip from PR #276.

@barbibulle barbibulle merged commit 56139c6 into google:main Sep 12, 2023
16 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.

None yet

3 participants