-
Notifications
You must be signed in to change notification settings - Fork 8.2k
Bluetooth: Userchan: Add support for TCP Connection #60711
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
Conversation
88bd883 to
6f11257
Compare
carlescufi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
aescolar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @vChavezB
Just a few relatively minor things that caught my eye.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just 2 trivial nits from me, which do not really warrant another pass unless you want to.
4d31894 to
8f50abb
Compare
|
I will start adding the documentation about how to use the virtual controller here Edit: I have added the first documentation on how to use the virtual controller with an example. |
fdb082b to
eef22a9
Compare
aescolar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for the update @vChavezB
|
Thanks for the input @aescolar will modify accordingly. |
|
I have added the changes proposed by @aescolar for the docs and reverted the condition return value for |
drivers/bluetooth/hci/userchan.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since bt_dev_index is global, I suggest to make arg_hci_idx local, and do this check where arg_hci_idx is assigned. IMO there should be no reason to keep arg_hci_idx as a global value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made arg_hci_idx global to leave it for discussion. The question for me is where should user argument values be checked?
Directly when cmd_bt_dev_found is called or btuserchan_check_arg ?
If the bt index would be checked in cmd_bt_dev_found would it not make sense to also check the other arguments (i.e., ip address, port number) in this function ?
It was not clear to me if btuserchan_check_arg was just used to check if the argument --bt-dev was set or if it should also check the argument values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Directly when cmd_bt_dev_found is called or btuserchan_check_arg ?
It is a bit better to check directly in cmd_bt_dev_found, no need to keep going if there is a bad cmd line argument.
would it not make sense to also check the other arguments (i.e., ip address, port number) in this function ?
Yep
|
aescolar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm even happier with the PR now :)
|
Sorry there was a build error, hopefully it should now work 👍 Forgot a local variable that was used in |
|
There seems to be some errors but I am not sure if they are related to the change of the hci user channel source file. |
Not related to this PR, main is broken right now, I queued a fix here: #61733 |
Added support to connect to an HCI TCP Server. This allows to do integration tests with other frameworks that support a virtual hci interface. Signed-off-by: Victor Chavez <[email protected]>
|
ok thanks for the update, I have just updated a condition which was incorrect As I was not considering hci interfaces with index 0 i.e., |
Thalley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR and the quick updates :)
|
No problem, thanks a lot for the input ! |
Added support to connect to an HCI TCP Server. This allows to do integration tests with other frameworks that support a virtual hci interface.
The use cases I see for this are:
functionality of a Bluetooth gateway by assigning a virtual hci controller bridge.
Attached is a proof of concept to show case emulating an Android device and connecting to a Zephyr Bluetooth sample (peripheral_hr). I am using the experimental python software from Google called bumble for connecting the Zephyr TCP HCI client and the Android netsim
https://github.com/google/bumble
Upper left screen is the HCI traffice between zephyr bluetooth stack (host) and the android-netsim (Controller). Lower window is the zephyr binary running the peripheral_hr example in the Windows Subsystem for Linux. And on the right is the Android emulator running with Android API 34 and the NRF Connect App to test the GATT notification of the peripheral_hr sample.
pof_tcp_hci_zephyr.mp4