Bluetooth: Classic: Add role change event cb #90126

MarkWangChinese · 2025-05-19T06:49:30Z

add role_changed to struct bt_conn_cb to notify the HCI_Role_Change event to application.
add BT_HCI_OP_SWITCH_ROLE and struct bt_hci_cp_switch_role.
~~add br switch-role shell cmd and test the role_changed callback.~~

lylezhu2012 · 2025-05-19T07:45:16Z

subsys/bluetooth/host/classic/shell/bredr.c

+
+	if (!strcmp(action, "central")) {
+		/* 0 - Change own Role to Central for this BD_ADDR. */
+		role = 0;


It is not readable. I suggest defining two marcos.

#define BT_HCI_SWITCH_ROLE_CENTRAL 0 #define BT_HCI_SWITCH_ROLE_PERIPHERAL 1

lylezhu2012 · 2025-05-19T07:48:00Z

subsys/bluetooth/host/classic/shell/bredr.c

+	err = bt_conn_get_info(default_conn, &info);
+	if (err) {
+		shell_error(sh, "Failed to get info (err %d)", err);
+		return err;
+	}


There is a function bt_conn_get_dst_br() can be used to get the peer address.

lylezhu2012 · 2025-05-19T07:49:44Z

subsys/bluetooth/host/classic/shell/bredr.c

+	if (buf != NULL) {
+		cp = net_buf_add(buf, sizeof(*cp));
+		memcpy(&cp->bdaddr, &info.br.dst->val[0], sizeof(cp->bdaddr));
+		cp->role = role;


What if the new role is the same as the old one?

From my test, the controller gives error for the result.

lylezhu2012 · 2025-05-19T07:50:42Z

subsys/bluetooth/host/classic/shell/bredr.c

+	buf = bt_hci_cmd_create(BT_HCI_OP_SWITCH_ROLE, sizeof(*cp));
+	if (buf != NULL) {
+		cp = net_buf_add(buf, sizeof(*cp));
+		memcpy(&cp->bdaddr, &info.br.dst->val[0], sizeof(cp->bdaddr));
+		cp->role = role;
+		err = bt_hci_cmd_send_sync(BT_HCI_OP_SWITCH_ROLE, buf, NULL);
+		if (err) {
+			shell_print(sh, "fail to send hci cmd (err %d)", err);
+		}
+	} else {
+		shell_error(sh, "hi cmd fail to create");
+	}


If can we add an API for BR role switch function?

I think it is better to add an API. I think another pr for the API is better because the current pr is mainly for adding new callback.

Well. I suggest removing this shell command. And adding it when the API is added.

OK, Even I don't agree it, but I still removed the added shell test codes.

lylezhu2012 · 2025-05-19T07:51:45Z

subsys/bluetooth/host/classic/shell/bredr.c

@@ -1041,6 +1116,7 @@ SHELL_STATIC_SUBCMD_SET_CREATE(br_cmds,
 	SHELL_CMD_ARG(oob, NULL, NULL, cmd_oob, 1, 0),
 	SHELL_CMD_ARG(pscan, NULL, "<value: on, off>", cmd_connectable, 2, 0),
 	SHELL_CMD_ARG(sdp-find, NULL, "<HFPAG, HFPHF>", cmd_sdp_find_record, 2, 0),
+	SHELL_CMD_ARG(switch-role, NULL, "<central, peripheral", cmd_switch_role, 2, 0),


> is missing in <central, peripheral.

lylezhu2012 · 2025-05-19T09:01:11Z

subsys/bluetooth/host/conn_internal.h

@@ -478,6 +478,10 @@ void bt_conn_set_state(struct bt_conn *conn, bt_conn_state_t state);

 void bt_conn_connected(struct bt_conn *conn);

+#if defined(CONFIG_BT_CLASSIC)


As my understand, function declarations do not need to be controlled by conditional compilation.

I see similar code as https://github.com/zephyrproject-rtos/zephyr/pull/90126/files#diff-58c6dddbce7d7fd58c9999b995f884c5d46d1f4c79cb17288167595afb389b6aL525

@MarkWangChinese thanks! @lylezhu2012 is correct. If you have the time, please submit a PR to remove those other #ifdefs too. It's a bad idea to have that in header files since it eliminates the possibility of using IS_ENABLED() in the calling code.

Thanks for the explain. I will do it.

Fixed this PR.

lylezhu2012 · 2025-05-28T02:57:55Z

include/zephyr/bluetooth/conn.h

+	 *  connection has changed.
+	 *
+	 *  @param conn Connection object.
+	 *  @param status The HCI_Role_Change event's Status.


Suggested change

* @param status The HCI_Role_Change event's Status.

* @param status HCI status of role change event.

lylezhu2012 · 2025-05-28T03:00:53Z

include/zephyr/bluetooth/conn.h

+	/** @brief The role of the connection has changed.
+	 *
+	 *  This callback notifies the application that the role of the
+	 *  connection has changed.


Suggest changing to This callback notifies the application that the role switch procedure has completed.

jhedberg · 2025-06-10T10:43:06Z

include/zephyr/bluetooth/conn.h

+	 *  @param conn Connection object.
+	 *  @param status HCI status of role change event.
+	 */
+	void (*role_changed)(struct bt_conn *conn, uint8_t status);


This seems like it has limited usability. Why not at least include the new role as a parameter of the callback?

That said, do we even have an API to query the current role of a connection?

bt_conn_get_info can get the current role.

Right, maybe that's sufficient then. Another question: why do you bother including a status parameter? If a role change fails then then role didn't change, so why would you call the callback then?

If the role switch is initiated by the DUT, the DUT needs to know the result whether it success or fail. If the role switch is initiated by peer side, this callback can give the indication that the peer tries to switch role even status is fail. So this callback is defined as HCI status of role change event.

As I said that I fully agree to add one API. But I am afraid that I don't think the callback can only make sense when the new API exist. Because the role switch can be initialized from the peer (it can run any bluetooth stack), then DUT (run zephyr stack) needs the callback to know the role is changed (when role switch success, the DUT host receive the HCI event from controller). Without the callback, the DUT application can only poll the conn info to know the role is changed if DUT application want to know it.

As I said that I fully agree to add one API. But I am afraid that I don't think the callback can only make sense when the new API exist. Because the role switch can be initialized from the pee

I meant the status parameter, not the callback as a whole.

OK, let me summary. Sorry that I may not get your point.

Case1. DUT is the initiator of the role switch: DUT use the new API/hci_cmd to send the hci cmd to let controller do the role switch. Controller will give the result of role switch no matter it success or fail. DUT application needs the callback to know the result no matter success or fail.
@jhedberg Do you mean that we don't have the new API, so the case1 is invalid, so only the follow Case2 is valid case, then the status is not needed? If yes, I will create the new API pr later, could we let this pr merged?

Case2. The peer is the initiator of the role switch: Controller will give the HCI event to host if the role switch success, DUT application needs the callback to know the role is changed.

@MarkWangChinese I mean that the callback as it's currently defined in this PR is fine (makes sense) as long as it's combined with an API to initiate the role switch from our side, i.e. I'd propose to add a new commit here to introduce bt_conn_br_change_role() or similar.

Hi @jhedberg I have added the new API. I prefer the API as bt_conn_br_switch_role(const struct bt_conn *conn, uint8_t role) because the application want to switch as dedicated role not change the current role. For example: application wants to switch as central after connected by peer, give the dedicated role to stack/controller in avoid that the peer initiate the role switch too at the same time, I think the controller can handle the corner case.

jhedberg

A couple of initial comments. I'll do a more thorough review in the next iteration.

jhedberg · 2025-06-16T08:06:55Z

include/zephyr/bluetooth/hci_types.h

+struct bt_hci_rp_read_link_policy_settings {
+	uint8_t  status;
+	uint16_t handle;
+	uint16_t  link_policy_settings;


Should be just one space between uint16_t and variable name

jhedberg · 2025-06-16T08:07:01Z

include/zephyr/bluetooth/hci_types.h

+#define BT_HCI_OP_WRITE_LINK_POLICY_SETTINGS    BT_OP(BT_OGF_LINK_POLICY, 0x000d)
+struct bt_hci_cp_write_link_policy_settings {
+	uint16_t handle;
+	uint16_t  link_policy_settings;


jhedberg · 2025-06-16T08:07:07Z

include/zephyr/bluetooth/hci_types.h

+#define BT_HCI_OP_READ_DEFAULT_LINK_POLICY_SETTINGS  BT_OP(BT_OGF_LINK_POLICY, 0x000e)
+struct bt_hci_rp_read_default_link_policy_settings {
+	uint8_t  status;
+	uint16_t  default_link_policy_settings;


jhedberg · 2025-06-16T08:07:15Z

include/zephyr/bluetooth/hci_types.h

+
+#define BT_HCI_OP_WRITE_DEFAULT_LINK_POLICY_SETTINGS BT_OP(BT_OGF_LINK_POLICY, 0x000f)
+struct bt_hci_cp_write_default_link_policy_settings {
+	uint16_t  default_link_policy_settings;


jhedberg · 2025-06-16T08:08:10Z

subsys/bluetooth/host/classic/Kconfig

+config BT_DEFAULT_ROLE_SWITCHABLE
+	bool "Default Bluetooth Role Switch Enable/Disable State"
+	help
+	  This option sets the controller's default link policy to enable/disable the role switch.


Please use similar line width as for help texts for other options, like BT_COD above.

jhedberg · 2025-06-16T08:16:36Z

subsys/bluetooth/host/classic/conn_br.c

+	}
+
+	cp = net_buf_add(buf, sizeof(*cp));
+	cp->handle = conn->handle;


Missing sys_cpu_to_le16()

jhedberg · 2025-06-16T08:18:04Z

subsys/bluetooth/host/classic/conn_br.c

+	is_enabled = link_policy_settings & BT_HCI_LINK_POLICY_SETTINGS_ENABLE_ROLE_SWITCH;
+
+	if (enable == is_enabled) {


To make it clear that you mean & and not && I think it's better to put the operation inside parenthesis. Also, no empty line between the assignment and the test, please (this is also inconsistent with yourself, since you don't do it e.g. on line 261/262 either).

jhedberg · 2025-06-16T08:18:36Z

subsys/bluetooth/host/classic/conn_br.c

+	err = bt_conn_br_write_link_policy_settings(conn, link_policy_settings);
+
+	return err;


I'd just replace this with return bt_conn_br_write_...();

jhedberg · 2025-06-16T08:19:59Z

subsys/bluetooth/host/classic/br.c

+	bool is_enabled = default_link_policy_settings &
+			  BT_HCI_LINK_POLICY_SETTINGS_ENABLE_ROLE_SWITCH;


Use () here. Added benefit is that editors will do the split line alignment more correctly.

jhedberg · 2025-06-16T08:21:15Z

subsys/bluetooth/host/classic/br.c

@@ -801,7 +801,11 @@ int bt_br_init(void)
 	struct bt_hci_cp_write_inquiry_mode *inq_cp;
 	struct bt_hci_write_local_name *name_cp;
 	struct bt_hci_cp_write_class_of_device *cod;
+	struct bt_hci_rp_read_default_link_policy_settings *rp;
+	struct bt_hci_cp_write_default_link_policy_settings *policy_cp;


Move this into the if-branch where it's actually used (variables should always be declared in the smallest possible scope)

MarkWangChinese · 2025-06-17T13:16:12Z

A couple of initial comments. I'll do a more thorough review in the next iteration.

Hi @jhedberg Thanks, all are resolved. please help to review again.

lylezhu2012 · 2025-07-02T04:52:47Z

subsys/bluetooth/host/classic/br.c

-		conn->role = BT_CONN_ROLE_PERIPHERAL;
-	} else {
-		conn->role = BT_CONN_ROLE_CENTRAL;
+	if (!evt->status) {


Suggested change

if (!evt->status) {

if (evt->status == 0) {

lylezhu2012 · 2025-07-02T04:58:33Z

subsys/bluetooth/host/classic/br.c

-	} else {
-		conn->role = BT_CONN_ROLE_CENTRAL;
+	if (!evt->status) {
+		if (evt->role) {


I suggest adding the following macros for event BT_HCI_EVT_ROLE_CHANGE,

#define BT_ROLE_CHANGE_CENTRAL 0 #define BT_ROLE_CHANGE_PERIPHERAL 1

And

Suggested change

if (evt->role) {

if (evt->role == BT_ROLE_CHANGE_PERIPHERAL) {

It is useful for reading.

Or, we may use the macros BT_HCI_ROLE_CENTRAL and BT_HCI_ROLE_PERIPHERAL. But I do not make sure it is reasonable. At lest the value of the BT_HCI_ROLE_PERIPHERAL is 1. And the value of BT_HCI_ROLE_CENTRAL is 0.

Updated with the HCI role MACRO.

lylezhu2012 · 2025-07-02T05:13:25Z

subsys/bluetooth/host/classic/conn_br.c

+	return bt_hci_cmd_send_sync(BT_HCI_OP_WRITE_LINK_POLICY_SETTINGS, buf, NULL);
+}
+
+int bt_conn_br_set_role_switch_enable(const struct bt_conn *conn, bool enable)


Whether is it better to change function name to bt_conn_br_role_switch?

Suggested change

int bt_conn_br_set_role_switch_enable(const struct bt_conn *conn, bool enable)

int bt_conn_br_role_switch(const struct bt_conn *conn, bool enable)

It is talked here #90126 (comment) and it is more aligned with the other bluetooth APIs.

lylezhu2012 · 2025-07-02T05:20:01Z

subsys/bluetooth/host/classic/shell/bredr.c

+	if (!strcmp(action, "enable")) {
+		enable = true;
+	} else if (!strcmp(action, "disable")) {
+		enable = false;
+	} else {
+		shell_help(sh);
+		return SHELL_CMD_HELP_PRINTED;
+	}


There is a function shell_strtobool(). It can be used to parse the argument from string to bool.

add `role_changed` to `struct bt_conn_cb` to notify the HCI_Role_Change event to application. Signed-off-by: Mark Wang <[email protected]>

add bt_conn_br_switch_role and bt_conn_br_set_role_switchable to control the role switch, add DEFAULT_ROLE_SWITCHABLE Kconfig to control the default role switch state. Signed-off-by: Mark Wang <[email protected]>

sonarqubecloud · 2025-07-03T06:44:03Z

Quality Gate passed

Issues
0 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

github-actions bot added area: Bluetooth area: Bluetooth Classic Bluetooth Classic (BR/EDR) area: Bluetooth Host Bluetooth Host (excluding BR/EDR) labels May 19, 2025

github-actions bot requested review from alwa-nordic, cvinayak, gzh-terry, hermabe, jhedberg, lylezhu2012, makeshi, rugeGerritsen, sjanc, Thalley and theob-pro May 19, 2025 06:50

github-actions bot assigned jhedberg and alwa-nordic May 19, 2025

MarkWangChinese force-pushed the feature/add_role_change_cb branch from 43ed4cc to 2009ed9 Compare May 19, 2025 07:18

Thalley removed their request for review May 19, 2025 07:32

lylezhu2012 requested changes May 19, 2025

View reviewed changes

MarkWangChinese force-pushed the feature/add_role_change_cb branch from 2009ed9 to 7c6719d Compare May 19, 2025 13:39

alwa-nordic assigned lylezhu2012 and unassigned alwa-nordic May 27, 2025

alwa-nordic removed their request for review May 27, 2025 14:01

lylezhu2012 reviewed May 28, 2025

View reviewed changes

lylezhu2012 changed the title ~~Feature/add role change cb~~ Bluetooth: Classic: Add role change event cb May 28, 2025

MarkWangChinese force-pushed the feature/add_role_change_cb branch from 7c6719d to f60b13c Compare May 28, 2025 13:41

MarkWangChinese requested a review from lylezhu2012 May 28, 2025 13:42

lylezhu2012 previously approved these changes Jun 5, 2025

View reviewed changes

jhedberg reviewed Jun 10, 2025

View reviewed changes

MarkWangChinese dismissed lylezhu2012’s stale review via 07bd0f8 June 10, 2025 13:02

MarkWangChinese force-pushed the feature/add_role_change_cb branch from f60b13c to 07bd0f8 Compare June 10, 2025 13:02

MarkWangChinese requested review from lylezhu2012 and jhedberg June 10, 2025 13:03

MarkWangChinese force-pushed the feature/add_role_change_cb branch 3 times, most recently from ab7bb7d to 9190f88 Compare June 16, 2025 02:34

jhedberg requested changes Jun 16, 2025

View reviewed changes

MarkWangChinese force-pushed the feature/add_role_change_cb branch from 9190f88 to 0dacc61 Compare June 17, 2025 12:52

MarkWangChinese requested a review from jhedberg June 17, 2025 13:16

lylezhu2012 reviewed Jul 2, 2025

View reviewed changes

bluetooth: classic: add role changed callback

0ff1b1f

add `role_changed` to `struct bt_conn_cb` to notify the HCI_Role_Change event to application. Signed-off-by: Mark Wang <[email protected]>

MarkWangChinese force-pushed the feature/add_role_change_cb branch from 0dacc61 to dd4c6ff Compare July 3, 2025 02:05

MarkWangChinese requested a review from lylezhu2012 July 3, 2025 02:08

bluetooth: classic: add role switch API

ebe2c91

add bt_conn_br_switch_role and bt_conn_br_set_role_switchable to control the role switch, add DEFAULT_ROLE_SWITCHABLE Kconfig to control the default role switch state. Signed-off-by: Mark Wang <[email protected]>

MarkWangChinese force-pushed the feature/add_role_change_cb branch from dd4c6ff to ebe2c91 Compare July 3, 2025 06:18

jhedberg approved these changes Jul 3, 2025

View reviewed changes

		@@ -478,6 +478,10 @@ void bt_conn_set_state(struct bt_conn *conn, bt_conn_state_t state);

		void bt_conn_connected(struct bt_conn *conn);

		#if defined(CONFIG_BT_CLASSIC)

	* @param status The HCI_Role_Change event's Status.
	* @param status HCI status of role change event.

		is_enabled = link_policy_settings & BT_HCI_LINK_POLICY_SETTINGS_ENABLE_ROLE_SWITCH;

		if (enable == is_enabled) {

		err = bt_conn_br_write_link_policy_settings(conn, link_policy_settings);

		return err;

		bool is_enabled = default_link_policy_settings &
		BT_HCI_LINK_POLICY_SETTINGS_ENABLE_ROLE_SWITCH;

	if (evt->role) {
	if (evt->role == BT_ROLE_CHANGE_PERIPHERAL) {

	int bt_conn_br_set_role_switch_enable(const struct bt_conn *conn, bool enable)
	int bt_conn_br_role_switch(const struct bt_conn *conn, bool enable)

Bluetooth: Classic: Add role change event cb #90126

Are you sure you want to change the base?

Bluetooth: Classic: Add role change event cb #90126

Uh oh!

Conversation

MarkWangChinese commented May 19, 2025 • edited by lylezhu2012 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lylezhu2012 May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhedberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

MarkWangChinese commented May 19, 2025 •

edited by lylezhu2012

Loading

lylezhu2012 May 28, 2025 •

edited

Loading