Bluetooth: Controller: Initial Frame Space Update support #99473

cvinayak · 2025-11-16T18:38:30Z

Initial Frame Space Update support.

Supercedes #82324.

Thank you @kruithofa for the contribution.

This a rebased PR, and is as-is for now.

Initial Frame Space Update support. Signed-off-by: Andries Kruithof <andries.kruithof@nordicsemi.no> Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>

sonarqubecloud · 2025-11-16T19:50:03Z

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Copilot

Pull Request Overview

This PR introduces initial Frame Space Update (FSU) support to the Bluetooth controller. Frame Space Update is a Bluetooth specification feature that allows devices to negotiate inter-frame spacing parameters during a connection, enabling optimization of timing intervals between packets.

Key changes:

Added FSU procedure handling in LLCP (Link Layer Control Protocol) state machines for both local and remote procedures
Implemented FSU PDU encoding/decoding functions for request and response messages
Integrated FSU initialization and updates into connection setup and PHY update procedures

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
ull_peripheral.c	Added FSU initialization call during peripheral setup
ull_central.c	Added FSU initialization call during central connection creation
ull_llcp_remote.c	Added FSU procedure handling in remote procedure state machine
ull_llcp_local.c	Added FSU procedure handling in local procedure state machine
ull_llcp_common.c	Implemented FSU request/response handling and notification logic
ull_llcp_phy.c	Added function to update effective TIFS based on FSU parameters during PHY updates
ull_llcp_pdu.c	Implemented FSU PDU encoding/decoding functions with debugging printk
ull_llcp.c	Added FSU procedure initiation and validation functions
ull_conn.c	Implemented FSU initialization, update logic, and per-PHY parameter management
ull_llcp_internal.h	Added FSU procedure enum and context data structure
ull_llcp_features.h	Added FSU feature check function
ull_conn_internal.h	Added FSU function declarations
ull_llcp.h	Added FSU public API declarations
pdu.h	Added FSU PDU structures and control type definitions
lll_conn.h	Added FSU data structures to connection state
ll_feat.h	Added FSU feature bit definition (temporarily set to 0)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-17T05:24:19Z

subsys/bluetooth/controller/ll_sw/ull_llcp_pdu.c

+	p->fsu = sys_cpu_to_le16(conn->lll.fsu.local.fsu_min);
+	p->phys = conn->lll.fsu.local.phys;
+	p->spacing_type = sys_cpu_to_le16(conn->lll.fsu.local.spacing_type);
+	printk("%s: fsu %u\n", __func__, p->fsu);


Debug printk statement should be removed or replaced with proper logging using LOG_DBG. This printk call is inappropriate for production code and will cause unwanted console output.

Copilot · 2025-11-17T05:24:19Z

subsys/bluetooth/controller/ll_sw/ull_llcp_pdu.c

+	conn->lll.fsu.local.phys = p->phys & 0x07; /* mask out RFU bits */
+	conn->lll.fsu.local.spacing_type =
+		sys_le16_to_cpu(p->spacing_type & 0x1F); /* mask out RFU bits */
+	/* nitpic, perphy is confusing, call it phy*/


Corrected spelling of 'nitpic' to 'nitpick'.

Suggested change

/* nitpic, perphy is confusing, call it phy*/

/* nitpick, perphy is confusing, call it phy*/

Copilot · 2025-11-17T05:24:19Z

subsys/bluetooth/controller/ll_sw/ull_llcp_phy.c

+		conn->lll.phy_tx == 4 ? 2 : conn->lll.phy_tx - 1; /* a bit tricky but should work */
+	phy_index_rx =
+		conn->lll.phy_rx == 4 ? 2 : conn->lll.phy_rx - 1; /* a bit tricky but should work */


The comment 'a bit tricky but should work' is vague and doesn't explain the PHY index mapping logic. Consider documenting why PHY value 4 maps to index 2 and the general mapping scheme (e.g., PHY_1M=1->index 0, PHY_2M=2->index 1, PHY_CODED=4->index 2).

Suggested change

conn->lll.phy_tx == 4 ? 2 : conn->lll.phy_tx - 1; /* a bit tricky but should work */

phy_index_rx =

conn->lll.phy_rx == 4 ? 2 : conn->lll.phy_rx - 1; /* a bit tricky but should work */

/* Map PHY values to perphy[] indices:

* PHY_1M (1) -> index 0

* PHY_2M (2) -> index 1

* PHY_CODED (4) -> index 2

* This works because PHY_CODED is 4, so (phy == 4 ? 2 : phy - 1)

* yields the correct index for each PHY value.

*/

conn->lll.phy_tx == 4 ? 2 : conn->lll.phy_tx - 1;

phy_index_rx =

/* See mapping above for explanation. */

conn->lll.phy_rx == 4 ? 2 : conn->lll.phy_rx - 1;

Copilot · 2025-11-17T05:24:19Z

subsys/bluetooth/controller/ll_sw/ull_llcp_phy.c

+	phy_index_tx =
+		conn->lll.phy_tx == 4 ? 2 : conn->lll.phy_tx - 1; /* a bit tricky but should work */
+	phy_index_rx =
+		conn->lll.phy_rx == 4 ? 2 : conn->lll.phy_rx - 1; /* a bit tricky but should work */


The comment 'a bit tricky but should work' is vague and doesn't explain the PHY index mapping logic. This duplicates the same unclear pattern from line 326. Consider documenting the mapping scheme clearly.

Suggested change

phy_index_tx =

conn->lll.phy_tx == 4 ? 2 : conn->lll.phy_tx - 1; /* a bit tricky but should work */

phy_index_rx =

conn->lll.phy_rx == 4 ? 2 : conn->lll.phy_rx - 1; /* a bit tricky but should work */

/* Map PHY values to perphy array indices:

* PHY values: 1 = LE 1M, 2 = LE 2M, 3 = LE Coded, 4 = LE Coded S2.

* perphy array indices: 0 = LE 1M, 1 = LE 2M, 2 = LE Coded (S8/S2).

* LE Coded S2 (phy value 4) shares the same index (2) as LE Coded S8 (phy value 3).

* For other PHYs, subtract 1 to get the array index.

*/

phy_index_tx =

conn->lll.phy_tx == 4 ? 2 : conn->lll.phy_tx - 1;

phy_index_rx =

conn->lll.phy_rx == 4 ? 2 : conn->lll.phy_rx - 1;

Copilot · 2025-11-17T05:24:20Z

subsys/bluetooth/controller/ll_sw/ull_conn.c

+	fsu_changed = ull_fsu_update_eff_from_local(conn);
+
+	if (fsu_changed) {
+		/* TODO: confirm that we do not need to do something here? */


TODO comment is unclear about what needs to be confirmed or what action might be needed. Consider either resolving this TODO before merging or documenting specifically what needs to be investigated (e.g., 'TODO: Verify if timing parameter recalculation is needed when FSU changes').

Suggested change

/* TODO: confirm that we do not need to do something here? */

/* TODO: Verify if additional actions (e.g., timing parameter recalculation or event notification) are needed when FSU changes. */

Copilot · 2025-11-17T05:24:20Z

subsys/bluetooth/controller/ll_sw/ull_llcp_common.c

+				/* llcp_tx_pause_data(conn, LLCP_TX_QUEUE_PAUSE_DATA_FRAME_SPACE);
+				 */


Commented-out code should be removed. If this functionality is planned for future implementation, add a TODO comment explaining when and why it should be uncommented.

Suggested change

/* llcp_tx_pause_data(conn, LLCP_TX_QUEUE_PAUSE_DATA_FRAME_SPACE);

*/

Copilot · 2025-11-17T05:24:20Z

subsys/bluetooth/controller/include/ll_feat.h

 	*/

+/* FIXME: Conditional Compilation for FSU feature, and handle feature bits > 64 bits */
+#define LL_FEAT_BIT_FRAME_SPACE 0U


LL_FEAT_BIT_FRAME_SPACE is defined as 0U which conflicts with existing feature bit 0. This should be assigned a unique, unused bit position in the feature mask. The comment on line 256 acknowledges this needs fixing for feature bits > 64 bits.

Suggested change

#define LL_FEAT_BIT_FRAME_SPACE 0U

#define LL_FEAT_BIT_FRAME_SPACE BIT64(BT_LE_FEAT_BIT_FRAME_SPACE)

Copilot · 2025-11-17T05:24:21Z

subsys/bluetooth/controller/ll_sw/lll_conn.h

 #endif /* CONFIG_BT_CTLR_DATA_LENGTH */
+	struct {
+		struct data_pdu_fsu local;
+		struct data_pdu_fsu perphy[4]; /* store frame-space for each PHY */


Array size hardcoded as 4 but code elsewhere uses size 3 (e.g., ull_conn.c lines 2383, 2427, 2965). This inconsistency could lead to bugs. Consider using a named constant (e.g., FSU_NUM_PHYS) or document why the sizes differ.

Copilot · 2025-11-17T05:24:21Z

subsys/bluetooth/controller/ll_sw/ull_llcp_local.c

+		break;
 #if defined(CONFIG_BT_CTLR_DF_CONN_CTE_REQ)
 	case PROC_CTE_REQ:
 		/* 3rd partam null? */


Corrected spelling of 'partam' to 'param'.

Suggested change

/* 3rd partam null? */

/* 3rd param null? */

cvinayak force-pushed the github_fsu branch from 41cf16b to b588711 Compare November 16, 2025 18:46

Bluetooth: Controller: Initial Frame Space Update support

f7358ed

Initial Frame Space Update support. Signed-off-by: Andries Kruithof <andries.kruithof@nordicsemi.no> Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>

cvinayak force-pushed the github_fsu branch from b588711 to f7358ed Compare November 16, 2025 19:23

cvinayak marked this pull request as ready for review November 17, 2025 05:19

zephyrbot added area: Bluetooth area: Bluetooth Controller labels Nov 17, 2025

zephyrbot requested review from Tronil, carlescufi, erbr-ot, ppryga, thoh-ot and wopu-ot November 17, 2025 05:21

zephyrbot assigned cvinayak Nov 17, 2025

cvinayak requested review from Thalley and Copilot November 17, 2025 05:21

Copilot started reviewing on behalf of cvinayak November 17, 2025 05:22 View session

Copilot finished reviewing on behalf of cvinayak November 17, 2025 05:22

Copilot AI reviewed Nov 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Bluetooth: Controller: Initial Frame Space Update support #99473

Bluetooth: Controller: Initial Frame Space Update support #99473

cvinayak commented Nov 16, 2025 •

edited

Loading

Uh oh!

sonarqubecloud bot commented Nov 16, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Nov 17, 2025

Uh oh!

Copilot AI Nov 17, 2025

Uh oh!

Copilot AI Nov 17, 2025

Uh oh!

Copilot AI Nov 17, 2025

Uh oh!

Copilot AI Nov 17, 2025

Uh oh!

Copilot AI Nov 17, 2025

Uh oh!

Copilot AI Nov 17, 2025

Uh oh!

Copilot AI Nov 17, 2025

Uh oh!

Copilot AI Nov 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	/* nitpic, perphy is confusing, call it phy*/
	/* nitpick, perphy is confusing, call it phy*/

-		conn->lll.phy_tx == 4 ? 2 : conn->lll.phy_tx - 1; /* a bit tricky but should work */
-	phy_index_rx =
-		conn->lll.phy_rx == 4 ? 2 : conn->lll.phy_rx - 1; /* a bit tricky but should work */
+		/* Map PHY values to perphy[] indices:
+		 * PHY_1M (1) -> index 0
+		 * PHY_2M (2) -> index 1
+		 * PHY_CODED (4) -> index 2
+		 * This works because PHY_CODED is 4, so (phy == 4 ? 2 : phy - 1)
+		 * yields the correct index for each PHY value.
+		 */
+		conn->lll.phy_tx == 4 ? 2 : conn->lll.phy_tx - 1;
+	phy_index_rx =
+		/* See mapping above for explanation. */
+		conn->lll.phy_rx == 4 ? 2 : conn->lll.phy_rx - 1;

	/* TODO: confirm that we do not need to do something here? */
	/* TODO: Verify if additional actions (e.g., timing parameter recalculation or event notification) are needed when FSU changes. */

		/* llcp_tx_pause_data(conn, LLCP_TX_QUEUE_PAUSE_DATA_FRAME_SPACE);
		*/

	#define LL_FEAT_BIT_FRAME_SPACE 0U
	#define LL_FEAT_BIT_FRAME_SPACE BIT64(BT_LE_FEAT_BIT_FRAME_SPACE)

Bluetooth: Controller: Initial Frame Space Update support #99473

Are you sure you want to change the base?

Bluetooth: Controller: Initial Frame Space Update support #99473

Conversation

cvinayak commented Nov 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonarqubecloud bot commented Nov 16, 2025

Quality Gate passed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cvinayak commented Nov 16, 2025 •

edited

Loading