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

Bluetooth: Controller: ISO Broadcast/Receive interleaved packing support #67231

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Bluetooth: Controller: ISO Broadcast/Receive interleaved packing support #67231

wants to merge 10 commits into from
+981 −215

Conversation

cvinayak
Copy link
Contributor

@cvinayak cvinayak commented Jan 5, 2024 •
edited
Loading

Fixes #42892

@cvinayak cvinayak force-pushed the github_broadcast_iso_interleaved branch 5 times, most recently from b0bb1b4 to 8858f32 Compare January 11, 2024 13:48
@cvinayak cvinayak force-pushed the github_broadcast_iso_interleaved branch 10 times, most recently from a62e1f8 to c82573d Compare January 26, 2024 12:30
@cvinayak cvinayak marked this pull request as ready for review January 26, 2024 13:30
@cvinayak cvinayak force-pushed the github_broadcast_iso_interleaved branch 3 times, most recently from 152fe27 to d81d759 Compare May 2, 2024 12:55
@cvinayak cvinayak force-pushed the github_broadcast_iso_interleaved branch from d81d759 to 12bd3e0 Compare May 23, 2024 14:38
cvinayak
cvinayak commented May 23, 2024
View reviewed changes
Copy link
Contributor Author

@cvinayak cvinayak left a comment

Choose a reason for hiding this comment

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

Need more time to address all the comments, will try to see if some refactoring can be done.

subsys/bluetooth/controller/ll_sw/lll_adv.h
@@ -18,6 +18,17 @@ struct lll_adv_iso_stream {
uint16_t pkt_seq_num;
};

struct lll_adv_iso_data_chan {
uint16_t prn_s;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the variable documented in the BT Spec, as one of the pseudo random number used in the channel calculations.

subsys/bluetooth/controller/ll_sw/nordic/lll/lll_adv_iso.c
Comment on lines +397 to +400
iss_us = (lll->bis_spacing >= (lll->sub_interval * lll->nse)) ?
lll->sub_interval : lll->bis_spacing;
iss_us -= PDU_BIS_US(pdu->len, ((pdu->len) ? lll->enc : 0U),
lll->phy, lll->phy_flags);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

sub_interval or bis_spacing are calculated based on the max PDU lengths, hence the calculation does not lead to underflow.

subsys/bluetooth/controller/ll_sw/nordic/lll/lll_adv_iso.c
Comment on lines +455 to +461
} else if (lll->bis_spacing >= (lll->sub_interval * lll->nse)) {
next_chan_calc_seq(lll, event_counter, data_chan_id);
#endif /* CONFIG_BT_CTLR_ADV_ISO_SEQUENTIAL */

#if defined(CONFIG_BT_CTLR_ADV_ISO_INTERLEAVED)
} else if (lll->bis_spacing < (lll->sub_interval * lll->nse)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will leave this be as-is and let the compiler do the optimization as necessary based in the combination of selected conditional compilations.

subsys/bluetooth/controller/ll_sw/nordic/lll/lll_adv_iso.c
Comment on lines +805 to +824
iss_us = (lll->bis_spacing >= (lll->sub_interval * lll->nse)) ?
lll->sub_interval : lll->bis_spacing;
iss_us -= PDU_BIS_US(pdu->len, ((pdu->len) ? lll->enc : 0U),
lll->phy, lll->phy_flags);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no underflow based on the calculation of sub_interval or bis_spacing

subsys/bluetooth/controller/ll_sw/nordic/lll/lll_adv_iso.c
Comment on lines +834 to +886
#if defined(CONFIG_BT_CTLR_ADV_ISO_SEQUENTIAL)
} else if (lll->bis_spacing >= (lll->sub_interval * lll->nse)) {
next_chan_calc_seq(lll, event_counter, data_chan_id);
#endif /* CONFIG_BT_CTLR_ADV_ISO_SEQUENTIAL */

#if defined(CONFIG_BT_CTLR_ADV_ISO_INTERLEAVED)
} else if (lll->bis_spacing < (lll->sub_interval * lll->nse)) {
next_chan_calc_int(lll, event_counter);
#endif /* CONFIG_BT_CTLR_ADV_ISO_INTERLEAVED */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will leave this be as-is and let the compiler do the optimization as necessary based in the combination of selected conditional compilations.

subsys/bluetooth/controller/Kconfig
Comment on lines +731 to +846
config BT_CTLR_ADV_ISO_SEQUENTIAL
bool "LE Broadcast Isochronous Channel advertising sequential packing"
depends on BT_CTLR_ADV_ISO

config BT_CTLR_ADV_ISO_INTERLEAVED
bool "LE Broadcast Isochronous Channel advertising interleaved packing"
depends on BT_CTLR_ADV_ISO
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most broadcast application do not change the packing once set between create and terminate. These Kconfig provide the option to reduce code size. Both these would mostly be enabled under controller-only builds, like, hci_ipc/hci_uart.

subsys/bluetooth/controller/Kconfig
Comment on lines +754 to +871
config BT_CTLR_SYNC_ISO_SEQUENTIAL
bool "LE Broadcast Isochronous Channel advertising sync sequential packing"
depends on BT_CTLR_SYNC_ISO
default y

config BT_CTLR_SYNC_ISO_INTERLEAVED
bool "LE Broadcast Isochronous Channel advertising sync interleaved packing"
depends on BT_CTLR_SYNC_ISO
default y
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the case of receiver, by default both are enabled. Again when having applications with broadcaster and receiver solutions that get bundled together, then these options alway for either one of the packing be enabled to save code space.

subsys/bluetooth/controller/ll_sw/nordic/lll/lll_adv_iso.c
Comment on lines +575 to +582
bis = 0U;

LL_ASSERT(false);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to avoid compiler warning, as the compiler think that we exit from the assertion call and then later use an uninitialized bis value.

subsys/bluetooth/controller/ll_sw/nordic/lll/lll_sync_iso.c
Comment on lines +899 to +986
#endif /* CONFIG_BT_CTLR_SYNC_ISO_SEQUENTIAL */

goto isr_rx_ctrl;

#if defined(CONFIG_BT_CTLR_SYNC_ISO_INTERLEAVED)
isr_rx_interleaved:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, looks weird now that I have them functioning (developed iteratively, sequential first and interleaved next) and to try keep prior implementation not changed a lot.

This jigsaw of goto is used so that in case of looping to skip already received PDUs, I wanted to avoid checking for packing type (which is sufficient once checked).

I do not have allocated time to rethink the implementation, but surely will consider for refactoring as a separate task (not sure if it will ever get picked).

@cvinayak cvinayak force-pushed the github_broadcast_iso_interleaved branch 2 times, most recently from 8137e8f to 0bb387e Compare June 3, 2024 08:02
@cvinayak cvinayak added this to the v3.7.0 milestone Jun 4, 2024
@cvinayak cvinayak force-pushed the github_broadcast_iso_interleaved branch 2 times, most recently from 9a75fc5 to 5075a9a Compare June 14, 2024 15:57
@cvinayak cvinayak modified the milestones: v3.7.0, v4.0.0 Jun 15, 2024
@cvinayak cvinayak force-pushed the github_broadcast_iso_interleaved branch from 5075a9a to c8169be Compare June 27, 2024 23:52
@cvinayak cvinayak force-pushed the github_broadcast_iso_interleaved branch from c8169be to 968f730 Compare August 14, 2024 02:45
@zephyrbot zephyrbot added the area: Bluetooth ISO Bluetooth LE Isochronous Channels label Aug 14, 2024
@cvinayak
Bluetooth: Controller: Fix ISO Sync Receiver BIS payload dereferencing
b331030 
Fix incorrect use of BIS indices to dereference the payload
array, instead correctly use synchronised stream indices.

Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
@cvinayak
samples: Bluetooth: iso_broadcast: Restrict max stream sync
d8ed307 
Restrict possible maximum stream synchronization to
implementation defined 2 stream, as structure declarations
in the implementation is limited to 2.

Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
@cvinayak
samples: Bluetooth: iso_broadcast: Fix buf alloc timeout
e6dee9b 
Fix buf alloc timeout to 20 millisecond as the sent callback
can have a latency that is upto twice the ISO interval.

Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
@cvinayak
Bluetooth: Controller: Fix BIS interleaved packing sub_interval calc
640fb22 
Fix BIS sub_interval calculation for interleaved packing.

Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
@cvinayak
Bluetooth: Controller: ISO Broadcast interleaved packing support
505a96d 
Add ISO Broadcast interleaved packing support in Lower Link
Layer implementation.

Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
@cvinayak cvinayak force-pushed the github_broadcast_iso_interleaved branch from 968f730 to d1011b7 Compare September 11, 2024 03:53
@cvinayak
Bluetooth: Controller: ISO Receive interleaved packing support
84a2cad 
Add ISO Receive interleaved packing support in Lower Link
Layer implementation.

Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
@cvinayak
samples: Bluetooth: hci_uart hci_ipc configuration update
11af9ef 
Updates to project configuration extras to build with
Zephyr Controller.

Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
@cvinayak
samples: Bluetooth: Support interleaved packing in BIS samples
ba0a50a 
Add Kconfig options in Broadcast ISO related samples to
support enabling interleaved packing use in subevents.

Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
@cvinayak
tests: bsim: Bluetooth: Test for single BIS with interleaved packing
7502a87 
Test for single BIS with interleaved packing or sequential
packing requested.

Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
@cvinayak
tests: bsim: Bluetooth: broadcast_audio_source interleaved/sequential
cc13047 
Test broadcast audio source sample in interleaved and
sequential packing.

Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
@cvinayak cvinayak force-pushed the github_broadcast_iso_interleaved branch from d1011b7 to cc13047 Compare September 11, 2024 05:13
@cvinayak cvinayak mentioned this pull request Sep 26, 2024
Closed
@kruithofa kruithofa removed their request for review October 15, 2024 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Thalley Thalley Thalley requested changes

@carlescufi carlescufi Awaiting requested review from carlescufi

@erbr-ot erbr-ot Awaiting requested review from erbr-ot

@mtpr-ot mtpr-ot Awaiting requested review from mtpr-ot

@thoh-ot thoh-ot Awaiting requested review from thoh-ot

@wopu-ot wopu-ot Awaiting requested review from wopu-ot

@asbjornsabo asbjornsabo Awaiting requested review from asbjornsabo

@HoZHel HoZHel Awaiting requested review from HoZHel

@jhedberg jhedberg Awaiting requested review from jhedberg

@kartben kartben Awaiting requested review from kartben

@nashif nashif Awaiting requested review from nashif

@sjanc sjanc Awaiting requested review from sjanc

@theob-pro theob-pro Awaiting requested review from theob-pro

@Vudentz Vudentz Awaiting requested review from Vudentz

Requested changes must be addressed to merge this pull request.

Assignees

@cvinayak cvinayak

Labels
area: Bluetooth Audio area: Bluetooth Controller area: Bluetooth Host area: Bluetooth ISO Bluetooth LE Isochronous Channels area: Bluetooth area: Samples Samples platform: nRF BSIM Nordic Semiconductors, nRF BabbleSim
Projects
Bluetooth LE Audio
Status: In Review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bluetooth: Controller: Design and implementation of LLL BIS subevents (interleaved)
3 participants
@cvinayak @Thalley @zephyrbot