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

samples: Bluetooth: Demonstrate MTU update #53593

Merged
merged 3 commits into from
Jan 16, 2023

Conversation

theob-pro
Copy link
Contributor

Add a new sample and a BabbleSim test for it. The goal of this sample is to demonstrate how to update the MTU to allow larger packet transmission as requested by #25648.

@theob-pro theob-pro force-pushed the large-mtu-sample branch 18 times, most recently from c497d86 to d95abf7 Compare January 11, 2023 12:55
@theob-pro theob-pro marked this pull request as ready for review January 11, 2023 13:18
Add a new sample that demonstrate how to exchange MTU to allow larger
packet transmission.

The sample is split in two applications. One is the Central, it will
initiate the MTU exchange. The other one is the Peripheral and will try to
send a large notification. If the MTU exchange fail or the new size is not
big enough, the Peripheral will not be able to send the notification.

Signed-off-by: Théo Battrel <theo.battrel@nordicsemi.no>
Test that the Central receive the rigtht data from the Peripheral
notification.

Signed-off-by: Théo Battrel <theo.battrel@nordicsemi.no>
@theob-pro theob-pro force-pushed the large-mtu-sample branch 2 times, most recently from 0aa5e33 to 16a7a07 Compare January 13, 2023 07:50
Update the README to add more informations on MTUs. Also add a diagram of
the different MTU and Kconfig symbols in the Host.

Signed-off-by: Théo Battrel <theo.battrel@nordicsemi.no>
@wiba-nordic
Copy link

I'm going over the Zephyr ⁠sample definition, and point 6 says that a sample should have minimal comments and not be explaining the WHY of something. But the sample currently has quite a bit of them, going into a lot the why and details of an MTU update. Would some of that be better served in a different part of the documentation instead of as part of a sample?

The first one should be flashed with the central and the second one with the
peripheral.

The two devices need to be close enough to be able to connect.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a bit hard to understand what is meant by this sentence. I think we should reword so it's clear it's a feature.

Suggested change
The two devices need to be close enough to be able to connect.
The two devices will connect only if they are close to each other, thanks to RSSI filtering.

@theob-pro
Copy link
Contributor Author

I'm going over the Zephyr ⁠sample definition, and point 6 says that a sample should have minimal comments and not be explaining the WHY of something. But the sample currently has quite a bit of them, going into a lot the why and details of an MTU update. Would some of that be better served in a different part of the documentation instead of as part of a sample?

I think that the point 6 refer to code comment and not the README documentation. Anyway, I don't think that limiting the comment on code make a lot of sense either, is there a good reason for that? @carlescufi
For the "why", you're right, I will remove the sentence citing reasons to update the MTU.

@carlescufi carlescufi merged commit 00a4a07 into zephyrproject-rtos:main Jan 16, 2023
@carlescufi
Copy link
Member

I'm going over the Zephyr ⁠sample definition, and point 6 says that a sample should have minimal comments and not be explaining the WHY of something. But the sample currently has quite a bit of them, going into a lot the why and details of an MTU update. Would some of that be better served in a different part of the documentation instead of as part of a sample?

I think that the point 6 refer to code comment and not the README documentation. Anyway, I don't think that limiting the comment on code make a lot of sense either, is there a good reason for that? @carlescufi For the "why", you're right, I will remove the sentence citing reasons to update the MTU.

In this particular case the documentation included in the sample is definitely helpful and belongs in it.

@alwa-nordic alwa-nordic added DNM This PR should not be merged (Do Not Merge) and removed DNM This PR should not be merged (Do Not Merge) labels Jan 16, 2023
theob-pro added a commit to theob-pro/zephyr that referenced this pull request Jan 16, 2023
Update the Readme file of the large mtu sample because the orginal PR
(zephyrproject-rtos#53593) has been merged
before the last comment could be addressed.

Signed-off-by: Théo Battrel <theo.battrel@nordicsemi.no>
theob-pro added a commit to theob-pro/zephyr that referenced this pull request Jan 16, 2023
Update the Readme file of the large mtu sample because the orginal PR
(zephyrproject-rtos#53593) has been merged
before the last comment could be addressed.

Signed-off-by: Théo Battrel <theo.battrel@nordicsemi.no>
carlescufi pushed a commit that referenced this pull request Jan 16, 2023
Update the Readme file of the large mtu sample because the orginal PR
(#53593) has been merged
before the last comment could be addressed.

Signed-off-by: Théo Battrel <theo.battrel@nordicsemi.no>
coreboot-org-bot pushed a commit to coreboot/zephyr-cros that referenced this pull request Jan 17, 2023
Update the Readme file of the large mtu sample because the orginal PR
(zephyrproject-rtos/zephyr#53593) has been merged
before the last comment could be addressed.

(cherry picked from commit 4314480)

Original-Signed-off-by: Théo Battrel <theo.battrel@nordicsemi.no>
GitOrigin-RevId: 4314480
Change-Id: I858888d9999864a7d7c9a739707c6136deff185a
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/4173730
Reviewed-by: Fabio Baltieri <fabiobaltieri@google.com>
Tested-by: Fabio Baltieri <fabiobaltieri@google.com>
Tested-by: CopyBot Service Account <copybot.service@gmail.com>
Commit-Queue: Fabio Baltieri <fabiobaltieri@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants