Skip to content

Bluetooth: samples: Add ISO connected benchmark sample #36752

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 14, 2021
Merged

Bluetooth: samples: Add ISO connected benchmark sample #36752

merged 1 commit into from
Jul 14, 2021

Conversation

@Thalley
Copy link
Contributor

@Thalley Thalley commented Jul 6, 2021

Add a sample that measures packet loss in a connected
ISO setup.

Signed-off-by: Emil Gydesen emil.gydesen@nordicsemi.no

Was originally #35602 but then reverted in #36750 due to a CI error.

@Thalley Thalley self-assigned this Jul 6, 2021
@Thalley Thalley mentioned this pull request Jul 6, 2021
Closed
nashif
nashif requested changes Jul 6, 2021
View reviewed changes
@Thalley Thalley force-pushed the iso_connected_throughput_new branch from a483607 to 0ccfb90 Compare July 6, 2021 11:31
@Thalley Thalley requested a review from nashif July 6, 2021 11:33
@Thalley Thalley force-pushed the iso_connected_throughput_new branch from 0ccfb90 to 6826ce1 Compare July 6, 2021 12:50
asbjornsabo
asbjornsabo reviewed Jul 6, 2021
View reviewed changes
Copy link
Contributor

@asbjornsabo asbjornsabo left a comment

Choose a reason for hiding this comment

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

Preliminary comments (a bit nit-picking, I know, I think the README can be improved)

samples/bluetooth/iso_connected_benchmark/README.rst Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

mode? Whether the application is in the central role or the peripheral role?

Not used as both at the same time, I guess?

The application can be used as either a central or a peripheral, and can easily switch between these roles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering whether it would be better to actually use the word "role", rather than "mode", when talking about switching to the other role.

@Thalley Thalley force-pushed the iso_connected_throughput_new branch from 6826ce1 to 3f3d0e4 Compare July 6, 2021 13:43
@Thalley Thalley requested a review from asbjornsabo July 6, 2021 13:44
ppryga-nordic
ppryga-nordic reviewed Jul 9, 2021
View reviewed changes
Copy link
Contributor

@ppryga-nordic ppryga-nordic left a comment

Choose a reason for hiding this comment

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

I know that the sample was already approved in PR that was reverted, so requesting changes may be frustrating to you. I put few comments, they are nit-picking. Generally the code looks ok.

@asbjornsabo asbjornsabo added the area: Bluetooth ISO Bluetooth LE Isochronous Channels label Jul 13, 2021
@Thalley Thalley force-pushed the iso_connected_throughput_new branch 2 times, most recently from 750e651 to d2aef30 Compare July 13, 2021 10:23
@Thalley Thalley requested a review from ppryga-nordic July 13, 2021 10:23
@Thalley
Copy link
Contributor Author

Thalley commented Jul 13, 2021

@ppryga thanks for the review, I think I've fixed all of your comments. I'll apply the same changed to the broadcast PR (#35178)

@Thalley Thalley force-pushed the iso_connected_throughput_new branch from d2aef30 to 45d2cdd Compare July 13, 2021 10:36
asbjornsabo
asbjornsabo reviewed Jul 13, 2021
View reviewed changes
@Thalley
Bluetooth: samples: Add ISO connected benchmark sample
40d02fc 
Add a sample that measures packet loss in a connected
ISO setup.

Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
@Thalley Thalley force-pushed the iso_connected_throughput_new branch from 45d2cdd to 40d02fc Compare July 13, 2021 11:12
@Thalley Thalley requested a review from asbjornsabo July 13, 2021 11:13
ppryga-nordic
ppryga-nordic approved these changes Jul 13, 2021
View reviewed changes
samples/bluetooth/iso_connected_benchmark/src/main.c Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to use printk wherever the message is mandatory from point of view of the sample application. I leave the decision if and where apply changes up to you.

@cfriedt
Copy link
Member

cfriedt commented Jul 13, 2021

@Thalley - is this one ready to be merged? Are comments resolved?

@Thalley
Copy link
Contributor Author

Thalley commented Jul 14, 2021

@cfriedt I'd like to get an approval from @asbjornsabo before we merge, so he can verify that his comments were resolved

asbjornsabo
asbjornsabo approved these changes Jul 14, 2021
View reviewed changes
Copy link
Contributor

@asbjornsabo asbjornsabo left a comment

Choose a reason for hiding this comment

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

My comments have been resolved to my satisfaction

@cfriedt cfriedt merged commit 884ec71 into zephyrproject-rtos:main Jul 14, 2021
@Thalley Thalley deleted the iso_connected_throughput_new branch July 14, 2021 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nashif nashif nashif approved these changes

@asbjornsabo asbjornsabo asbjornsabo approved these changes

@jhedberg jhedberg Awaiting requested review from jhedberg

@joerchan joerchan Awaiting requested review from joerchan

@Vudentz Vudentz Awaiting requested review from Vudentz

@koffes koffes Awaiting requested review from koffes

@rick1082 rick1082 Awaiting requested review from rick1082

@alexsven alexsven Awaiting requested review from alexsven

+1 more reviewer

@ppryga-nordic ppryga-nordic ppryga-nordic approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Thalley Thalley

Labels

area: Bluetooth ISO Bluetooth LE Isochronous Channels area: Documentation area: Samples Samples

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Thalley @cfriedt @nashif @asbjornsabo @ppryga-nordic