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: Fix ISO Tx PDU buffer counts for fragmentation #82026

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+24 −30
Open

Bluetooth: Controller: Fix ISO Tx PDU buffer counts for fragmentation #82026

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
f8b00f6
Bluetooth: Controller: Fix ISO Tx PDU buffer counts for fragmentation
cvinayak Nov 26, 2024
6707b39
Bluetooth: Controller: Remove BT_CTLR_CONN_ISO_SDU_LEN_MAX
cvinayak Nov 26, 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
1 change: 0 additions & 1 deletion samples/bluetooth/hci_ipc/nrf5340_cpunet_cis-bt_ll_sw_split.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ CONFIG_BT_CTLR_PERIPHERAL_ISO=y
CONFIG_BT_CTLR_CONN_ISO_GROUPS=1
CONFIG_BT_CTLR_CONN_ISO_STREAMS=2
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

Expand Down
1 change: 0 additions & 1 deletion samples/bluetooth/hci_ipc/nrf5340_cpunet_iso-bt_ll_sw_split.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,6 @@ CONFIG_BT_CTLR_PERIPHERAL_ISO=y
CONFIG_BT_CTLR_CONN_ISO_GROUPS=1
CONFIG_BT_CTLR_CONN_ISO_STREAMS=2
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

Expand Down
1 change: 0 additions & 1 deletion samples/bluetooth/hci_ipc/nrf5340_cpunet_iso_central-bt_ll_sw_split.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ CONFIG_BT_CTLR_PERIPHERAL_ISO=n
CONFIG_BT_CTLR_CONN_ISO_GROUPS=1
CONFIG_BT_CTLR_CONN_ISO_STREAMS=2
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

Expand Down
1 change: 0 additions & 1 deletion samples/bluetooth/hci_ipc/nrf5340_cpunet_iso_peripheral-bt_ll_sw_split.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ CONFIG_BT_CTLR_PERIPHERAL_ISO=y
CONFIG_BT_CTLR_CONN_ISO_GROUPS=1
CONFIG_BT_CTLR_CONN_ISO_STREAMS=2
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

Expand Down
1 change: 0 additions & 1 deletion samples/bluetooth/hci_uart/overlay-all-bt_ll_sw_split.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,6 @@ 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

Expand Down
1 change: 0 additions & 1 deletion samples/bluetooth/hci_uart_3wire/overlay-all-bt_ll_sw_split.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,6 @@ 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

Expand Down
3 changes: 1 addition & 2 deletions samples/bluetooth/iso_central/overlay-bt_ll_sw_split.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,7 @@ CONFIG_BT_CTLR_SCAN_DATA_LEN_MAX=1650
# of PDUs
CONFIG_BT_CTLR_RX_BUFFERS=9

# Sufficient ISO SDU and PDU length for this sample with ISO_TX_MTU of 247
CONFIG_BT_CTLR_CONN_ISO_SDU_LEN_MAX=247
# Maximum PDU length for this sample with ISO_TX_MTU if set up to and over 247 bytes
CONFIG_BT_CTLR_CONN_ISO_PDU_LEN_MAX=251

# Number of supported streami sources and sinks
Expand Down
8 changes: 0 additions & 8 deletions subsys/bluetooth/controller/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1004,14 +1004,6 @@ config BT_CTLR_CONN_ISO_PDU_LEN_MAX
help
Maximum Connected Isochronous Channel PDU Length.

config BT_CTLR_CONN_ISO_SDU_LEN_MAX
int "Maximum Connected Isochronous Channel SDU Length"
depends on BT_CTLR_CONN_ISO
range 1 4095
default 251
help
Maximum Connected Isochronous Channel SDU Length.
Comment on lines -1007 to -1013
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to go through the deprecation process for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ISO is experimental upstream. And not sure if any downstream implementation use this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally we would select EXPERIMENTAL for all experiment controller Kconfigs as well then


config BT_CTLR_CONN_ISO_STREAMS_MAX_NSE
int "LE Connected Isochronous Streams max number of subevents"
depends on BT_CTLR_CONN_ISO
Expand Down
34 changes: 23 additions & 11 deletions subsys/bluetooth/controller/ll_sw/ull_iso_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,31 @@
* SPDX-License-Identifier: Apache-2.0
*/

#if defined(CONFIG_BT_CTLR_ADV_ISO) || defined(CONFIG_BT_CTLR_CONN_ISO)
/* Calculate ISO PDU buffers required considering SDU fragmentation */
/* FIXME: Calculation considering both Connected and Broadcast ISO PDU
* fragmentation.
*/
#if defined(CONFIG_BT_CTLR_CONN_ISO)
#define BT_CTLR_ISO_TX_BUFFERS (((CONFIG_BT_CTLR_CONN_ISO_SDU_LEN_MAX + \
CONFIG_BT_CTLR_CONN_ISO_PDU_LEN_MAX - 1U) / \
CONFIG_BT_CTLR_CONN_ISO_PDU_LEN_MAX) * \
#if defined(CONFIG_BT_CTLR_ADV_ISO) || defined(CONFIG_BT_CTLR_CONN_ISO)
#define BT_CTLR_ISO_SDU_LEN_MAX ((CONFIG_BT_CTLR_ISO_TX_BUFFER_SIZE * \
CONFIG_BT_CTLR_ISO_TX_BUFFERS) - \
BT_HCI_ISO_SDU_HDR_SIZE)
#if defined(CONFIG_BT_CTLR_ADV_ISO) && defined(CONFIG_BT_CTLR_CONN_ISO)
#define BT_CTLR_ISO_TX_BUFFERS (DIV_ROUND_UP(BT_CTLR_ISO_SDU_LEN_MAX, \
MIN((CONFIG_BT_CTLR_ISO_TX_BUFFER_SIZE - \
BT_HCI_ISO_SDU_TS_HDR_SIZE), \
MIN(CONFIG_BT_CTLR_ADV_ISO_PDU_LEN_MAX, \
CONFIG_BT_CTLR_CONN_ISO_PDU_LEN_MAX))) * \
CONFIG_BT_CTLR_ISO_TX_BUFFERS)
#elif defined(CONFIG_BT_CTLR_ADV_ISO)
#define BT_CTLR_ISO_TX_BUFFERS (DIV_ROUND_UP(BT_CTLR_ISO_SDU_LEN_MAX, \
MIN((CONFIG_BT_CTLR_ISO_TX_BUFFER_SIZE - \
BT_HCI_ISO_SDU_TS_HDR_SIZE), \
CONFIG_BT_CTLR_ADV_ISO_PDU_LEN_MAX)) * \
CONFIG_BT_CTLR_ISO_TX_BUFFERS)
#else /* CONFIG_BT_CTLR_CONN_ISO */
#define BT_CTLR_ISO_TX_BUFFERS (DIV_ROUND_UP(BT_CTLR_ISO_SDU_LEN_MAX, \
MIN((CONFIG_BT_CTLR_ISO_TX_BUFFER_SIZE - \
BT_HCI_ISO_SDU_TS_HDR_SIZE), \
CONFIG_BT_CTLR_CONN_ISO_PDU_LEN_MAX)) * \
CONFIG_BT_CTLR_ISO_TX_BUFFERS)
#else /* !CONFIG_BT_CTLR_CONN_ISO */
#define BT_CTLR_ISO_TX_BUFFERS CONFIG_BT_CTLR_ISO_TX_BUFFERS
#endif /* !CONFIG_BT_CTLR_CONN_ISO */
#endif /* CONFIG_BT_CTLR_CONN_ISO */

Check notice on line 31 in subsys/bluetooth/controller/ll_sw/ull_iso_internal.h

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

You may want to run clang-format on this change 

subsys/bluetooth/controller/ll_sw/ull_iso_internal.h:31 
-#define BT_CTLR_ISO_SDU_LEN_MAX ((CONFIG_BT_CTLR_ISO_TX_BUFFER_SIZE * \
-				  CONFIG_BT_CTLR_ISO_TX_BUFFERS) - \
-				 BT_HCI_ISO_SDU_HDR_SIZE)
+#define BT_CTLR_ISO_SDU_LEN_MAX                                                                    \
+	((CONFIG_BT_CTLR_ISO_TX_BUFFER_SIZE * CONFIG_BT_CTLR_ISO_TX_BUFFERS) -                     \
+	 BT_HCI_ISO_SDU_HDR_SIZE)
 #if defined(CONFIG_BT_CTLR_ADV_ISO) && defined(CONFIG_BT_CTLR_CONN_ISO)
-#define BT_CTLR_ISO_TX_BUFFERS (DIV_ROUND_UP(BT_CTLR_ISO_SDU_LEN_MAX, \
-					     MIN((CONFIG_BT_CTLR_ISO_TX_BUFFER_SIZE - \
-						  BT_HCI_ISO_SDU_TS_HDR_SIZE), \
-						 MIN(CONFIG_BT_CTLR_ADV_ISO_PDU_LEN_MAX, \
-						     CONFIG_BT_CTLR_CONN_ISO_PDU_LEN_MAX))) * \
-				CONFIG_BT_CTLR_ISO_TX_BUFFERS)
+#define BT_CTLR_ISO_TX_BUFFERS                                                                     \
+	(DIV_ROUND_UP(BT_CTLR_ISO_SDU_LEN_MAX,                                                     \
+		      MIN((CONFIG_BT_CTLR_ISO_TX_BUFFER_SIZE - BT_HCI_ISO_SDU_TS_HDR_SIZE),        \
+			  MIN(CONFIG_BT_CTLR_ADV_ISO_PDU_LEN_MAX,                                  \
+			      CONFIG_BT_CTLR_CONN_ISO_PDU_LEN_MAX))) *                             \
+	 CONFIG_BT_CTLR_ISO_TX_BUFFERS)
 #elif defined(CONFIG_BT_CTLR_ADV_ISO)
-#define BT_CTLR_ISO_TX_BUFFERS (DIV_ROUND_UP(BT_CTLR_ISO_SDU_LEN_MAX, \
-					     MIN((CONFIG_BT_CTLR_ISO_TX_BUFFER_SIZE - \
-						  BT_HCI_ISO_SDU_TS_HDR_SIZE), \
-						 CONFIG_BT_CTLR_ADV_ISO_PDU_LEN_MAX)) * \
-				CONFIG_BT_CTLR_ISO_TX_BUFFERS)
+#define BT_CTLR_ISO_TX_BUFFERS                                                                     \
+	(DIV_ROUND_UP(BT_CTLR_ISO_SDU_LEN_MAX,                                                     \
+		      MIN((CONFIG_BT_CTLR_ISO_TX_BUFFER_SIZE - BT_HCI_ISO_SDU_TS_HDR_SIZE),        \
+			  CONFIG_BT_CTLR_ADV_ISO_PDU_LEN_MAX)) *                                   \
+	 CONFIG_BT_CTLR_ISO_TX_BUFFERS)
 #else /* CONFIG_BT_CTLR_CONN_ISO */
-#define BT_CTLR_ISO_TX_BUFFERS (DIV_ROUND_UP(BT_CTLR_ISO_SDU_LEN_MAX, \
-					     MIN((CONFIG_BT_CTLR_ISO_TX_BUFFER_SIZE - \
-						  BT_HCI_ISO_SDU_TS_HDR_SIZE), \
-						 CONFIG_BT_CTLR_CONN_ISO_PDU_LEN_MAX)) * \
-				CONFIG_BT_CTLR_ISO_TX_BUFFERS)
+#define BT_CTLR_ISO_TX_BUFFERS                                                                     \
+	(DIV_ROUND_UP(BT_CTLR_ISO_SDU_LEN_MAX,                                                     \
+		      MIN((CONFIG_BT_CTLR_ISO_TX_BUFFER_SIZE - BT_HCI_ISO_SDU_TS_HDR_SIZE),        \
+			  CONFIG_BT_CTLR_CONN_ISO_PDU_LEN_MAX)) *                                  \
+	 CONFIG_BT_CTLR_ISO_TX_BUFFERS)
Comment on lines +8 to +31
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's gotta be better way than this :D

I don't have one on hand right now, but still...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree. And this is not feasible either, the RAM usage is too high (see CI failures) and need to limit the calculated tx buffer counts.

#else /* !CONFIG_BT_CTLR_ADV_ISO && !CONFIG_BT_CTLR_CONN_ISO */
#define BT_CTLR_ISO_TX_BUFFERS 0
#endif /* !CONFIG_BT_CTLR_ADV_ISO && !CONFIG_BT_CTLR_CONN_ISO */
Expand Down
1 change: 0 additions & 1 deletion tests/bsim/bluetooth/audio/overlay-bt_ll_sw_split.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ CONFIG_BT_CTLR_CENTRAL_ISO=y
CONFIG_BT_CTLR_PERIPHERAL_ISO=y
CONFIG_BT_CTLR_CONN_ISO_STREAMS=2
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

Expand Down
1 change: 0 additions & 1 deletion tests/bsim/bluetooth/ll/cis/prj.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ CONFIG_LOG=y
CONFIG_BT_LL_SW_SPLIT=y
CONFIG_BT_CTLR_CENTRAL_ISO=y
CONFIG_BT_CTLR_PERIPHERAL_ISO=y
CONFIG_BT_CTLR_CONN_ISO_SDU_LEN_MAX=247
CONFIG_BT_CTLR_CONN_ISO_PDU_LEN_MAX=251
CONFIG_BT_CTLR_CONN_ISO_STREAMS=9
CONFIG_BT_CTLR_CONN_ISO_STREAMS_PER_GROUP=9
Expand Down
1 change: 0 additions & 1 deletion ...s/bsim/bluetooth/ll/cis/sysbuild/hci_ipc/nrf5340_cpunet_iso_acl_group-bt_ll_sw_split.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,6 @@ CONFIG_BT_CTLR_SYNC_ISO_STREAM_MAX=2
# ISO Connection Oriented
CONFIG_BT_CTLR_CENTRAL_ISO=y
CONFIG_BT_CTLR_PERIPHERAL_ISO=y
CONFIG_BT_CTLR_CONN_ISO_SDU_LEN_MAX=247
CONFIG_BT_CTLR_CONN_ISO_PDU_LEN_MAX=251
CONFIG_BT_CTLR_CONN_ISO_STREAMS=4
CONFIG_BT_CTLR_CONN_ISO_STREAMS_PER_GROUP=4
Expand Down
Loading