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.

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

Bt iso data fixes #33766

Merged
merged 3 commits into from
May 27, 2021
Merged

Bt iso data fixes #33766

merged 3 commits into from
May 27, 2021

Conversation

Casper-Bonde-Bose
Copy link
Collaborator

@Casper-Bonde-Bose Casper-Bonde-Bose commented Mar 29, 2021 •
edited by carlescufi
Loading

A few fixes required for proper ISO data processing.

Fixes #31179.

@github-actions github-actions bot added area: Bluetooth area: Bluetooth Host area: Boards area: native port Host native arch port (native_sim) area: Networking area: Shell Shell subsystem area: Tests Issues related to a particular existing or missing test labels Mar 29, 2021
aescolar
aescolar previously requested changes Mar 29, 2021
View reviewed changes
Copy link
Member

@aescolar aescolar left a comment

Choose a reason for hiding this comment

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

Commits' titles should be prefixed by the subsystem the commit targets: https://docs.zephyrproject.org/latest/contribute/index.html#commit-guidelines

For the native posix build the sleep calls used i tasks will stall the zephyr instance

native_posix is not meant for hard real time interaction with host hardware. The real time support is only there to support some level of interactive testing, and testing with real host peripherals.
So if you find yourself needing to reconfigure it to awake every ms (as you are doing here), you are beyond the limit of what it ever was meant for. Even if native_posix tries to get rescheduled every ms, the host OS is going to schedule it with whatever priority at whatever tick it has. So you are going to see a lot of jitter in the scheduling.
If you want to interact with something sending a lot of data, you should, hopefully, mostly only need more buffering.

which sets an upper limit on the data processing speed to one packet every 20-30 millisecond.

This sounds a bit funny. @jhedberg can the BT user channel not queue several packets at a time?

boards/posix/native_posix/native_posix_defconfig Outdated Show resolved Hide resolved
subsys/shell/shell.c Outdated Show resolved Hide resolved
@Casper-Bonde-Bose
Copy link
Collaborator Author

Even if native_posix tries to get rescheduled every ms, the host OS is going to schedule it with whatever priority at whatever tick it has. So you are going to see a lot of jitter in the scheduling.

True, I think many people use this on a Linux PC, where the tick is typically 250 ticks per second. I do see some jitter, but it seems to be within one tick. My thinking of setting it to 1000 was that if multiple zephyr tasks was using sleep (at leas two does for the current ISO data test application), you could have zephyr execute 4 ticks for every host OS tick. When you start to add tasks to read audio data as well you would have more sleep calls.

Thalley
Thalley requested changes Mar 29, 2021
View reviewed changes
subsys/net/Kconfig Outdated Show resolved Hide resolved
joerchan
joerchan requested changes Mar 30, 2021
View reviewed changes
subsys/net/Kconfig Outdated Show resolved Hide resolved
@github-actions github-actions bot added the area: API Changes to public APIs label Mar 31, 2021
Thalley
Thalley requested changes May 11, 2021
View reviewed changes
Copy link
Collaborator

@Thalley Thalley left a comment

Choose a reason for hiding this comment

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

Two minor documentation comments, and then some questions regarding the peripheral command.

include/bluetooth/iso.h Outdated Show resolved Hide resolved
include/bluetooth/iso.h Outdated Show resolved Hide resolved
subsys/bluetooth/shell/iso.c Outdated Show resolved Hide resolved
subsys/bluetooth/shell/iso.c Outdated Show resolved Hide resolved
@Casper-Bonde-Bose Casper-Bonde-Bose force-pushed the bt_iso_data_fixes branch 2 times, most recently from 73047e6 to a9bc555 Compare May 11, 2021 11:58
Thalley
Thalley requested changes May 12, 2021
View reviewed changes
Copy link
Collaborator

@Thalley Thalley left a comment

Choose a reason for hiding this comment

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

One minor final comment, otherwise LGTM

tests/bluetooth/shell/boards/native_posix.conf Outdated Show resolved Hide resolved
Thalley
Thalley approved these changes May 12, 2021
View reviewed changes
Copy link
Collaborator

@Thalley Thalley left a comment

Choose a reason for hiding this comment

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

LGTM

@Casper-Bonde-Bose
Copy link
Collaborator Author

This sounds a bit funny. @jhedberg can the BT user channel not queue several packets at a time?

@aescolar Just to conclude on this. Yes it can process multiple packets per tick, but if you have an ISO interval of 5ms and only have a few ISO buffers in the controller you will get gabs in the data if you only process data every 20-30ms. I did move the configuration of this to the BT-shell native posix test application configuration rather than the global configuration.

aescolar
aescolar reviewed May 12, 2021
View reviewed changes
Copy link
Member

@aescolar aescolar left a comment

Choose a reason for hiding this comment

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

@aescolar Just to conclude on this. Yes it can process multiple packets per tick, but if you have an ISO interval of 5ms and only have a few ISO buffers in the controller you will get gabs in the data if you only process data every 20-30ms. I did move the configuration of this to the BT-shell native posix test application configuration rather than the global configuration.

Fair enough. I think at this point it is @jhedberg (and maybe also @Vudentz @joerchan) who should approve or request changes.

tests/bluetooth/shell/boards/native_posix.conf Outdated Show resolved Hide resolved
@carlescufi
Copy link
Member

@joerchan can you please re-review?

@carlescufi carlescufi added this to the v2.6.0 milestone May 20, 2021
@carlescufi
Copy link
Member

carlescufi commented May 25, 2021 •
edited
Loading

@Casper-Bonde-Bose do you want this in for 2.6? if so please address the review comments.
@asmk-ot, @Vudentz and @joerchan please approve if happy.

@Casper-Bonde-Bose
Bluetooth: ISO: Speed up BT ISO data processing
95afb99 
For the native posix build the sleep calls used in tasks will stall
the zephyr instance which sets an upper limit on the data processing
interval to once every 20-30 millisecond.
This change reduces the duration of the sleep calls and increases the
TICKS_PER_SECOND to allow for shorter sleeps. This is needed to
support the data rates needed for LE Audio streaming. The rate is
tuned to support up to bidirectional 5ms ISO-intervals.
This change also increases the ISO buffer count from 1 to 5 to
allow for some buffering in the controller, which is needed for
gapless playback and/or use of burst number larger than 1.

Signed-off-by: Casper Bonde <casper_bonde@bose.com>
@Casper-Bonde-Bose
Bluetooth: ISO: Add sequence number to ISO data packets
26db447 
The sequence number is needed in the appliaction layer to detect lost
packets in the ISO stream in cases where the timestamp is not included.
(Sequence number is mandatory to include where timestamp is optional).
The API for the public metadata have been moved to a public header file.

As the size of the ISO meta data exceeds the default 4 octets net_buf
user_data the public ISO metadata have been moved into a seperate array.
The internal metadata is still stored in net_buf user_data.

This also fixed the user_data overflow on 32 bit systems, caused by
writing the ts into user_data on index 4 to 7, which is outside the 4
allocated bytes.

Signed-off-by: Casper Bonde <casper_bonde@bose.com>
@Casper-Bonde-Bose
Copy link
Collaborator Author

@Casper-Bonde-Bose do you want this in for 2.6? if so please address the review comments.
@carlescufi There was a single optional comment - not something I expected would block the this patch, but now I made the requested change. Do I need to do something to indicate I have addressed all requested changes?

@Casper-Bonde-Bose
Bluetooth: ISO: Make it possible to setup unidirectional CIS
8fbec49 
The current iso commands in the shell only supports setting up
a bidirectional stream or unidirectional for central role.
Adding rx/tx/rxtx option to iso listen command to allow for
peripheral side configuration of an iso connection.

Signed-off-by: Casper Bonde <casper_bonde@bose.com>
@Thalley
Copy link
Collaborator

Thalley commented May 26, 2021

One thing that would be nice to add to the bt_iso_recv_info struct is also the packet flags (retrieved with the line flags = bt_iso_pkt_flags(len);), as that will tell the application if a packet is malformed or even lost.

Can be appended in this PR, or added in a new PR.

@jukkar jukkar removed their request for review May 26, 2021 15:39
@carlescufi carlescufi merged commit 3af1f4b into zephyrproject-rtos:main May 27, 2021
@Casper-Bonde-Bose Casper-Bonde-Bose deleted the bt_iso_data_fixes branch May 27, 2021 11:33
@Casper-Bonde-Bose
Copy link
Collaborator Author

One thing that would be nice to add to the bt_iso_recv_info struct is also the packet flags (retrieved with the line flags = bt_iso_pkt_flags(len);), as that will tell the application if a packet is malformed or even lost.

Can be appended in this PR, or added in a new PR.

@Thalley I like this idea, and agree it would be a natural extension.

@Thalley
Copy link
Collaborator

Thalley commented May 27, 2021

One thing that would be nice to add to the bt_iso_recv_info struct is also the packet flags (retrieved with the line flags = bt_iso_pkt_flags(len);), as that will tell the application if a packet is malformed or even lost.
Can be appended in this PR, or added in a new PR.

@Thalley I like this idea, and agree it would be a natural extension.

@Casper-Bonde-Bose : #35723

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

@asmk-ot asmk-ot asmk-ot left review comments

@aescolar aescolar aescolar left review comments

@Thalley Thalley Thalley approved these changes

@joerchan joerchan joerchan approved these changes

@jhedberg jhedberg jhedberg approved these changes

@daor-oti daor-oti Awaiting requested review from daor-oti

@jakub-uC jakub-uC Awaiting requested review from jakub-uC

@nordic-krch nordic-krch Awaiting requested review from nordic-krch

@asbjornsabo asbjornsabo Awaiting requested review from asbjornsabo

@carlescufi carlescufi Awaiting requested review from carlescufi

@mengxianglinx mengxianglinx Awaiting requested review from mengxianglinx

@tbursztyka tbursztyka Awaiting requested review from tbursztyka

@Vudentz Vudentz Awaiting requested review from Vudentz

Assignees

@jhedberg jhedberg

Labels
area: API Changes to public APIs area: Bluetooth Host area: Bluetooth area: Boards area: native port Host native arch port (native_sim) area: Networking area: Samples Samples area: Shell Shell subsystem area: Tests Issues related to a particular existing or missing test
Projects
None yet
Milestone
v2.6.0
Development

Successfully merging this pull request may close these issues.

iso bind for slave not working as intended
8 participants
@Casper-Bonde-Bose @carlescufi @Thalley @jhedberg @joerchan @Vudentz @aescolar @asmk-ot