Bluetooth: ISO: Make setting ISO data explicit #75549
Conversation
Thalley
force-pushed
the
iso_data_path_refactor
branch
11 times, most recently
from
7aac227 to
cbcdac0
Compare
zephyrbot
added
area: Bluetooth Controller
area: Bluetooth Audio
area: Bluetooth Host
zephyrbot
requested review from
alwa-nordic,
asbjornsabo,
Casper-Bonde-Bose,
fredrikdanebjer,
hermabe,
jhedberg,
jori-nordic,
kartben,
kruithofa,
larsgk,
MariuszSkamra and
nashif
Thalley
force-pushed
the
iso_data_path_refactor
branch
from
817b7dc to
9088297
Compare
Thalley
force-pushed
the
iso_data_path_refactor
branch
from
9088297 to
40bb978
Compare
doc/releases/migration-guide-4.0.rst
Outdated
| @@ -404,6 +404,10 @@ Bluetooth Audio | |||
| Bluetooth Host | |||
| ============== | |||
|
|
|||
| * The ISO data paths are not longer setup automatically, and shall explicitly be setup and removd | |||
There was a problem hiding this comment.
| * The ISO data paths are not longer setup automatically, and shall explicitly be setup and removd | |
| * The ISO data paths are not longer setup automatically, and shall explicitly be setup and removed |
Thalley
force-pushed
the
iso_data_path_refactor
branch
2 times, most recently
from
770a9c4 to
505cedd
Compare
|
ideally #83539 would get merged first to add additional TX and RX verification |
Thalley
force-pushed
the
iso_data_path_refactor
branch
from
505cedd to
bd0219c
Compare
Thalley
force-pushed
the
iso_data_path_refactor
branch
from
bd0219c to
3a1f2c7
Compare
Thalley
force-pushed
the
iso_data_path_refactor
branch
2 times, most recently
from
b80bdac to
2dd043a
Compare
Thalley
requested review from
rugeGerritsen
and removed request for
jori-nordic
Thalley
force-pushed
the
iso_data_path_refactor
branch
from
2dd043a to
abd66d0
Compare
| BT_ISO_CHAN_TYPE_CENTRAL, /**< Connected as central */ | ||
| BT_ISO_CHAN_TYPE_PERIPHERAL, /**< Connected as peripheral */ |
There was a problem hiding this comment.
This is also an API change which would need an entry in the migration guide
| */ | ||
| uint32_t delay; | ||
| /** Codec Configuration length */ | ||
| uint8_t cc_len; |
There was a problem hiding this comment.
Somewhat unrelated: Now that we are changing the API, would it make sense to change the name to make it easier to understand? cc_len -> codec_config_len
There was a problem hiding this comment.
I'd rather not. This PR is already large enough, and adding more API changes will just make it larger. If we want to do any renames, then I suggest to do that in a follow-up PR while the API is still considered unstable.
include/zephyr/bluetooth/iso.h
Outdated
| * If the channel was established then it still contain references to the BIS/CIS handles | ||
| * and is not completely free'd at this point. The memory of the channel shall not be | ||
| * memset to 0 or free'd. | ||
| * To avoid any issue it is recommended to use a @ref k_work_submit or similar to not | ||
| * overwrite any data while in the callback. | ||
| * | ||
| * For the above reason it is still possible to use bt_iso_chan_get_info() on the @p chan. |
There was a problem hiding this comment.
I didn't really understand what is meant with this section. Could you elaborate a bit on this?
From a controller point of view, the CIS Central handles remain valid after a disconnection, but this is not the case for a peripheral.
There was a problem hiding this comment.
Redid the text a bit. The main purpose was to document that this callback is similar to the bt_conn.disconnected callback, and the chan object is still valid in the host until after this function returns.
include/zephyr/bluetooth/iso.h
Outdated
| * | ||
| * Removes the ISO data path configured by bt_iso_setup_data_path() for the provided @p dir. | ||
| * | ||
| * As per Bluetooth Core specification 5.4, Vol 4, Part E, Section 7.7.5, the data paths of CIS for |
There was a problem hiding this comment.
Minor: Here we should reference the newest core spec (6.0.)
include/zephyr/bluetooth/iso.h
Outdated
| * | ||
| * Removes the ISO data path configured by bt_iso_setup_data_path() for the provided @p dir. | ||
| * | ||
| * As per Bluetooth Core specification 5.4, Vol 4, Part E, Section 7.7.5, the data paths of CIS for |
There was a problem hiding this comment.
The important part of the sentence comes at the very end of a long sentence. I would suggest to flip it around to make it easier to read:
| * As per Bluetooth Core specification 5.4, Vol 4, Part E, Section 7.7.5, the data paths of CIS for | |
| * The data paths for a CIS for a Peripheral are removed upon disconnection as per .... |
There was a problem hiding this comment.
Did that for all paragraphs
| @@ -1,12 +1,13 @@ | |||
| /* | |||
| * Copyright (c) 2021 Nordic Semiconductor ASA | |||
| * Copyright (c) 2021-2024 Nordic Semiconductor ASA | |||
There was a problem hiding this comment.
I guess we are in 2025 now :)
Thalley
force-pushed
the
iso_data_path_refactor
branch
from
abd66d0 to
8ab1ec8
Compare
The stack will no longer implicitly set the data path for ISO channel, and the responsibility for doing that is now for the upper layers/applications. This provides additional flexibility for the higher layers as they can better control the values and timing of the data path, as well as support removing and even reconfiguring the data path at will. This also removes some complexity from the stack. This commit also fixed a inconsistency in the disconnected handler. CIS for centrals as well as BIS were still valid bt_iso_chan channels in the disconnected callback, but CIS for peripherals were completely cleaned up at this point. This issue is fixed by moving the disconnected callback handling to before the code to cleanup the channel for peripherals. Since there is a difference in how you remove data paths depending on the GAP role (central/peripheral), the iso_info struct type has been expanded to be more concise of which type of CIS it is. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
Thalley
force-pushed
the
iso_data_path_refactor
branch
from
8ab1ec8 to
c65b2e6
Compare
The stack will no longer implicitly set the data path for ISO channel, and the responsibility for doing that is now for the upper layers/applications.
This provides additional flexibility for the higher layers as they can better control the values and timing of the data path, as well as support removing and even reconfiguring the data path at will.
Finally this also removes some complexity from the stack.
fixes #68359