Skip to content

Bluetooth: Controller: Fix spurious ISO Sync receiver stall #80775

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

Closed
wants to merge 2 commits into from
Closed

Bluetooth: Controller: Fix spurious ISO Sync receiver stall #80775

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c9545f8
tests: bsim: Bluetooth: Align audio test Controller Kconfig
cvinayak Nov 2, 2024
20490c3
Bluetooth: Controller: Fix spurious ISO Sync receiver stall
cvinayak Nov 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions subsys/bluetooth/controller/ll_sw/nordic/lll/lll_sync_iso.c
View file
Open in desktop
Copy link
Contributor

@Thalley Thalley Nov 2, 2024

Choose a reason for hiding this comment

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

I had a feeling it was related to uninitialzed data, but valgrind didn't show anything for me. How did you find this?

Copy link
Contributor Author

@cvinayak cvinayak Nov 3, 2024

Choose a reason for hiding this comment

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

Had to debug inside the lower link layer to check when the reception stopped, there were calls to ticker_stop and lead to the caller which used this uninitialized value (recently added struct member). Since the value is from inside the Rx buffer node allocated from pool, the overall content get assigned something sometime before when receiving other packets.

Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,7 @@ static void abort_cb(struct lll_prepare_param *prepare_param, void *param)
LL_ASSERT(e);

e->type = EVENT_DONE_EXTRA_TYPE_SYNC_ISO;
e->estab_failed = 0U;
e->trx_cnt = 0U;
e->crc_valid = 0U;

Expand Down Expand Up @@ -1281,6 +1282,7 @@ static void isr_rx_done(void *param)

/* Calculate and place the drift information in done event */
e->type = EVENT_DONE_EXTRA_TYPE_SYNC_ISO;
e->estab_failed = 0U;
Copy link
Contributor

@Thalley Thalley Nov 3, 2024

Choose a reason for hiding this comment

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

This value isn't assigned after line 1252 - Is that intentional?

Copy link
Contributor Author

@cvinayak cvinayak Nov 4, 2024

Choose a reason for hiding this comment

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

Yes, intentional. Only relevant structure members are assigned for each type being passed around.

Controller has large Rx data flow (in struct node_rx) overheads (two parts: fixed overhead of unions, and configurable PDU length payload). Generally, not all members of the overhead part (respective to the type of rx node) are assigned.

When adding new struct member, typically all sources of the rx node type in the data flow need to be inspected :-(

Copy link
Contributor Author

@cvinayak cvinayak Nov 4, 2024

Choose a reason for hiding this comment

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

On second thought i need to investigate further today...#80788 pass locally, but fails in CI :-(

e->trx_cnt = trx_cnt;
e->crc_valid = crc_ok_anchor;

Expand Down
93 changes: 81 additions & 12 deletions tests/bsim/bluetooth/audio/overlay-bt_ll_sw_split.conf
View file
Open in desktop
Copy link
Contributor

@Thalley Thalley Nov 2, 2024

Choose a reason for hiding this comment

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

Could we instead just use the nRF53 BSIM .conf file?

Copy link
Contributor Author

@cvinayak cvinayak Nov 3, 2024

Choose a reason for hiding this comment

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

Will investigate as part of enabling some high reliability CAP tests. Reusing the target conf file is challenging as high reliability test need lot more tx and rx buffers to handle worst case retransmissions.

Trying out fixes and Kconfig changes here: #80788

Original file line number Diff line number Diff line change
@@ -1,45 +1,114 @@
# Controller Settings
CONFIG_BT_CTLR_SCAN_DATA_LEN_MAX=255
# Host and Controller common dependencies
CONFIG_BT_BROADCASTER=y
CONFIG_BT_OBSERVER=y
CONFIG_BT_EXT_ADV=y
CONFIG_BT_PER_ADV=y
CONFIG_BT_PER_ADV_SYNC=y
CONFIG_BT_PER_ADV_SYNC_MAX=2
CONFIG_BT_CENTRAL=y
CONFIG_BT_PERIPHERAL=y
CONFIG_BT_MAX_CONN=3

# Broadcast and Connected ISO
CONFIG_BT_ISO_BROADCASTER=y
CONFIG_BT_ISO_SYNC_RECEIVER=y
CONFIG_BT_ISO_CENTRAL=y
CONFIG_BT_ISO_PERIPHERAL=y

# ISO Streams
CONFIG_BT_ISO_TX_MTU=310
CONFIG_BT_ISO_RX_MTU=310
CONFIG_BT_ISO_MAX_CHAN=4
# CONFIG_BT_ISO_TX_BUF_COUNT=1
# CONFIG_BT_ISO_RX_BUF_COUNT=1

# Controller
CONFIG_BT_LL_SW_SPLIT=y

# Rx ACL and Adv Reports
CONFIG_BT_CTLR_RX_BUFFERS=9
CONFIG_BT_CTLR_DATA_LENGTH_MAX=251

# Coded PHY support
CONFIG_BT_CTLR_PHY_CODED=y

# Advertising Sets and Extended Scanning
CONFIG_BT_CTLR_ADV_EXT=y
CONFIG_BT_CTLR_ADV_SET=3
CONFIG_BT_CTLR_ADV_DATA_LEN_MAX=191
CONFIG_BT_CTLR_SCAN_DATA_LEN_MAX=1650

# Controller advanced options
CONFIG_BT_CTLR_ADVANCED_FEATURES=y
CONFIG_BT_CTLR_ADV_AUX_SET=3
CONFIG_BT_CTLR_ADV_AUX_PDU_BACK2BACK=y
CONFIG_BT_CTLR_ADV_SYNC_SET=3
CONFIG_BT_CTLR_ADV_SYNC_PDU_BACK2BACK=y
CONFIG_BT_CTLR_ADV_DATA_BUF_MAX=6

# Increase the below to receive interleaved advertising chains
CONFIG_BT_CTLR_SCAN_AUX_SET=1
# CONFIG_BT_CTLR_SCAN_AUX_USE_CHAINS=y
# CONFIG_BT_CTLR_SCAN_AUX_CHAIN_COUNT=1

CONFIG_BT_CTLR_ADV_RESERVE_MAX=n
CONFIG_BT_CTLR_ADV_ISO_RESERVE_MAX=y
CONFIG_BT_CTLR_SCAN_AUX_SYNC_RESERVE_MIN=y
CONFIG_BT_CTLR_SYNC_PERIODIC_SKIP_ON_SCAN_AUX=n
CONFIG_BT_CTLR_SYNC_ISO_RESERVE_MAX=n
CONFIG_BT_CTLR_CENTRAL_RESERVE_MAX=n
CONFIG_BT_CTLR_PERIPHERAL_RESERVE_MAX=n
CONFIG_BT_CTLR_PERIPHERAL_ISO_RESERVE_MAX=n
CONFIG_BT_CTLR_EVENT_OVERHEAD_RESERVE_MAX=y
CONFIG_BT_CTLR_SLOT_RESERVATION_UPDATE=n
CONFIG_BT_CTLR_SCAN_UNRESERVED=y
CONFIG_BT_TICKER_NEXT_SLOT_GET_MATCH=y
CONFIG_BT_TICKER_EXT=y
CONFIG_BT_TICKER_EXT_SLOT_WINDOW_YIELD=y

# Control Procedure
CONFIG_BT_CTLR_LLCP_LOCAL_PROC_CTX_BUF_NUM=6

# ISO Broadcaster Controller
CONFIG_BT_CTLR_ADV_EXT=y
CONFIG_BT_CTLR_ADV_PERIODIC=y
CONFIG_BT_CTLR_SYNC_TRANSFER_SENDER=y
CONFIG_BT_CTLR_ADV_ISO=y
CONFIG_BT_CTLR_ADV_ISO_PDU_LEN_MAX=247
CONFIG_BT_CTLR_ADV_ISO_SET=2
CONFIG_BT_CTLR_ADV_ISO_STREAM_COUNT=4
CONFIG_BT_CTLR_ADV_ISO_STREAM_MAX=2
CONFIG_BT_CTLR_ADV_ISO_STREAM_COUNT=2
CONFIG_BT_CTLR_ADV_ISO_PDU_LEN_MAX=247

# ISO Receive Controller
CONFIG_BT_CTLR_ADV_EXT=y
CONFIG_BT_CTLR_SYNC_PERIODIC=y
CONFIG_BT_CTLR_SYNC_TRANSFER_RECEIVER=y
CONFIG_BT_CTLR_SYNC_ISO=y
CONFIG_BT_CTLR_SYNC_ISO_PDU_LEN_MAX=251
CONFIG_BT_CTLR_SCAN_SYNC_ISO_SET=1
CONFIG_BT_CTLR_SYNC_ISO_STREAM_COUNT=2
CONFIG_BT_CTLR_SYNC_ISO_STREAM_MAX=2
CONFIG_BT_CTLR_SYNC_ISO_PDU_LEN_MAX=251

# ISO Connection Oriented
CONFIG_BT_CTLR_CENTRAL_ISO=y
CONFIG_BT_CTLR_PERIPHERAL_ISO=y
CONFIG_BT_CTLR_CONN_ISO_STREAMS=2
CONFIG_BT_CTLR_CONN_ISO_GROUPS=1
CONFIG_BT_CTLR_CONN_ISO_STREAMS_PER_GROUP=2
CONFIG_BT_CTLR_CONN_ISO_SDU_LEN_MAX=247
CONFIG_BT_CTLR_CONN_ISO_PDU_LEN_MAX=251
CONFIG_BT_CTLR_CONN_ISO_LOW_LATENCY_POLICY=y

# ISO Transmissions
CONFIG_BT_ISO_TX_MTU=310
CONFIG_BT_ISO_TX_BUF_COUNT=4
CONFIG_BT_CTLR_ISO_TX_BUFFERS=8
CONFIG_BT_CTLR_ISOAL_SOURCES=4
CONFIG_BT_CTLR_ISO_TX_BUFFERS=12
CONFIG_BT_CTLR_ISO_TX_BUFFER_SIZE=255
CONFIG_BT_CTLR_ISOAL_SOURCES=2

# ISO Receptions
CONFIG_BT_ISO_RX_MTU=310
CONFIG_BT_CTLR_ISO_RX_BUFFERS=8
CONFIG_BT_CTLR_ISOAL_SINKS=2
CONFIG_BT_CTLR_ISO_RX_BUFFERS=8

# Tx Power Dynamic Control
CONFIG_BT_CTLR_TX_PWR_DYNAMIC_CONTROL=y

# Ignore HCI ISO data Tx sequence numbers
# CONFIG_BT_CTLR_ISOAL_PSN_IGNORE=y
Loading