Skip to content

Conversation

@patstew
Copy link
Contributor

@patstew patstew commented Mar 15, 2024

Keep reading from the HCI socket when a packet is incomplete. The other end may not write entire packets, or TCP could fragment in the middle of a packet.
Also fix a potential infinite loop by advancing to the next packet before any continue statements.

@zephyrbot zephyrbot added area: Bluetooth area: Bluetooth Host Bluetooth Host (excluding BR/EDR) labels Mar 15, 2024
@github-actions
Copy link

Hello @patstew, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

Copy link
Member

@jhedberg jhedberg left a comment

Choose a reason for hiding this comment

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

What does Linux HCI user channel sockets have to do with TCP? The Linux kernel should ensure that you always get entire packet frames when reading from a userchan socket. The userchan HCI driver in Zephyr takes advantage of this, and there are also BlueZ tools which assume the same. Could you describe your setup a little more in detail, assuming that you're actually seeing the need for this change in practice?

@patstew
Copy link
Contributor Author

patstew commented Mar 15, 2024

That code is also used in native_sim for TCP HCI emulation. e.g.

west build -b native_sim samples/bluetooth/peripheral
build/zephyr/zephyr.exe --bt-dev=127.0.0.1:9123

See https://docs.zephyrproject.org/latest/boards/native/native_sim/doc/index.html#peripherals
user_chan_open in userchan.c can open a TCP socket. It was introduced in #60711 and partially fixed in #61764

I'm using that to connect to a controller, the controller in question doesn't write packets in their entireity at once, so Zephyr fails to read the responses from it. But assuming that one write == one read in TCP is completely wrong anyway.

@patstew patstew requested a review from jhedberg March 15, 2024 22:00
@jhedberg jhedberg requested a review from aescolar March 16, 2024 10:14
@jhedberg
Copy link
Member

@patstew thanks for the clarification. I had completely forgotten that our userchan driver had been extended with TCP support (curiously, even though I was the assignee in the original feature PR, it got merged without my approval). Btw, I think we might want to consider renaming the driver since its name still implies that it's only about user channel sockets.

@aescolar
Copy link
Member

CC @vChavezB

@jhedberg jhedberg requested a review from LingaoM March 25, 2024 08:04
@jhedberg jhedberg changed the title Bluetooth: Support TCP fragmentation of HCI packets Bluetooth: userchan: Support TCP fragmentation of HCI packets Mar 25, 2024
@Thalley Thalley removed their request for review March 25, 2024 12:24
@patstew patstew force-pushed the bluetooth-tcp-fix branch from 6495603 to ddff897 Compare March 25, 2024 20:24
@vChavezB
Copy link
Contributor

vChavezB commented Mar 25, 2024

Just added two comments on the changes. They are minor details but hope they help. Thanks for the contribution 👍 I am happy to know that other users are taking advantage of connecting Zephyr BT applications to other external controllers.

@patstew patstew force-pushed the bluetooth-tcp-fix branch 2 times, most recently from ae1ef3b to 4f03938 Compare March 25, 2024 21:54
Keep reading from the HCI socket when a packet is incomplete. The other
end may not write entire packets, or TCP could fragment in the middle of a
packet.
Also fix a potential infinite loop by advancing to the next packet before
any continue statements.

Signed-off-by: Patrick Stewart <[email protected]>
@patstew patstew force-pushed the bluetooth-tcp-fix branch from 4f03938 to 1eaebf0 Compare March 25, 2024 23:19
@vChavezB
Copy link
Contributor

Looks good to me, I think you just need to await feedback from the code owners or collaborators of the project.

@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label May 29, 2024
@patstew
Copy link
Contributor Author

patstew commented May 29, 2024

I'd like the stale label removed (and for someone else to review it)

@github-actions github-actions bot removed the Stale label May 30, 2024
@carlescufi carlescufi merged commit 1868987 into zephyrproject-rtos:main May 30, 2024
@github-actions
Copy link

Hi @patstew!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Bluetooth Host Bluetooth Host (excluding BR/EDR) area: Bluetooth

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants