Skip to content

Bluetooth: Classic: add local name#96621

Open
chengkai15 wants to merge 4 commits intozephyrproject-rtos:mainfrom
chengkai15:dev_name
Open

Bluetooth: Classic: add local name#96621
chengkai15 wants to merge 4 commits intozephyrproject-rtos:mainfrom
chengkai15:dev_name

Conversation

@chengkai15
Copy link
Contributor

add BR/EDR local name

Copy link
Member

@jhedberg jhedberg left a comment

Choose a reason for hiding this comment

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

We already have CONFIG_BT_DEVICE_NAME and bt_set_name() APIs. I don't think it makes sense to do BR/EDR-specific APIs for this.

@HaavardRei HaavardRei removed their request for review September 30, 2025 13:13
@chengkai15
Copy link
Contributor Author

We already have CONFIG_BT_DEVICE_NAME and bt_set_name() APIs. I don't think it makes sense to do BR/EDR-specific APIs for this.

updated

Comment on lines 861 to 867
err = bt_br_write_local_name(CONFIG_BT_DEVICE_NAME);
if (err) {
return err;
}

strncpy(bt_dev.name, CONFIG_BT_DEVICE_NAME, CONFIG_BT_DEVICE_NAME_MAX);

Copy link
Contributor

Choose a reason for hiding this comment

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

The device name storing feature will be broken by this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NO, It would not storing CONFIG_BT_DEVICE_NAME , which would be load when stack init.

otherwise local name would be to twice, when bt_enable, it would try to store user name, and check (bt_dev.name[0] == '\0') , then set local name again

int bt_enable(bt_ready_cb_t cb)
{
......
} else if (IS_ENABLED(CONFIG_BT_DEVICE_NAME_DYNAMIC)) {
err = bt_set_name(CONFIG_BT_DEVICE_NAME);
if (err) {
LOG_WRN("Failed to set device name (%d)", err);
}
}
......
}

static int commit_settings(void)
{
......
#if defined(CONFIG_BT_DEVICE_NAME_DYNAMIC)
if (bt_dev.name[0] == '\0') {
bt_set_name(CONFIG_BT_DEVICE_NAME);
}

#endif
......

Copy link
Contributor

@lylezhu2012 lylezhu2012 Oct 16, 2025

Choose a reason for hiding this comment

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

I do not think so.

int bt_enable(bt_ready_cb_t cb) { ...... } else if (IS_ENABLED(CONFIG_BT_DEVICE_NAME_DYNAMIC)) { err = bt_set_name(CONFIG_BT_DEVICE_NAME); if (err) { LOG_WRN("Failed to set device name (%d)", err); } } ...... }

If the settings is not enabled and CONFIG_BT_DEVICE_NAME_DYNAMIC is enabled, the name is stored into bt_dev.name. The default value is CONFIG_BT_DEVICE_NAME since the settings is not enabled.

static int commit_settings(void) { ...... #if defined(CONFIG_BT_DEVICE_NAME_DYNAMIC) if (bt_dev.name[0] == '\0') { bt_set_name(CONFIG_BT_DEVICE_NAME); } #endif ......

Or if the settings is enabled and CONFIG_BT_DEVICE_NAME_DYNAMIC is enabled, the bt_dev.name is loaded from settings. If the name is not stored, the default value CONFIG_BT_DEVICE_NAME will be used and store it to settings.

Copy link
Contributor Author

@chengkai15 chengkai15 Oct 16, 2025

Choose a reason for hiding this comment

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

If the settings is not enabled and CONFIG_BT_DEVICE_NAME_DYNAMIC is enabled,that commit would meet the need. bt_dev.name would save last changed device name

and if the settings is enabled and CONFIG_BT_DEVICE_NAME_DYNAMIC is enabled, if controller name has been changed, we need to cache name to bt_dev.name and store to settings

these cases would be handled well.

Copy link
Contributor

@lylezhu2012 lylezhu2012 Oct 17, 2025

Choose a reason for hiding this comment

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

If the settings is not enabled and CONFIG_BT_DEVICE_NAME_DYNAMIC is enabled,that commit would meet the need. bt_dev.name would save last changed device name

So, do you mean the change of the line 866 is used to fix the issue that the device name is incorrect when calling bt_get_name() if the settings is not enabled but CONFIG_BT_DEVICE_NAME_DYNAMIC is enabled and function bt_set_name() is not called?

@cvinayak cvinayak removed their request for review October 15, 2025 06:42
@chengkai15
Copy link
Contributor Author

Meant to request changes along with my previous comments

done

jhedberg
jhedberg previously approved these changes Oct 23, 2025
gzh-terry
gzh-terry previously approved these changes Oct 24, 2025
Copy link
Contributor

@lylezhu2012 lylezhu2012 left a comment

Choose a reason for hiding this comment

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

I think there is a case missing in this PR.
The device name has been stored into NVM. And re-initialize the Bluetooth Host Stack.
The device name will not be written to the controller.

#if defined(CONFIG_BT_DEVICE_NAME_DYNAMIC)
if (!strncmp(name, "name", len)) {
len = read_cb(cb_arg, &bt_dev.name, sizeof(bt_dev.name) - 1);
if (len < 0) {
LOG_ERR("Failed to read device name from storage"
" (err %zd)", len);
} else {
bt_dev.name[len] = '\0';
LOG_DBG("Name set to %s", bt_dev.name);
}
return 0;
}
#endif

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the function name is changed here?

There are two cases if the classic feature is supported,

  1. If the classic is not enabled, call function bt_br_init() of file subsys/bluetooth/host/hci_core.c‎ to read the buffer size.
  2. Or, call function bt_br_init() of file subsys/bluetooth/host/classic/br.c

I only found the function name of bt_br_init() of file subsys/bluetooth/host/hci_core.c has been changed to br_hci_init(). The function bt_br_init() of file subsys/bluetooth/host/classic/br.c will not be called if Classic feature is enabled.

I think the changes will break the functionality of Bluetooth Classic.

Copy link
Contributor Author

@chengkai15 chengkai15 Oct 24, 2025

Choose a reason for hiding this comment

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

@lylezhu2012 it would not affect classic functionality
there was discussion about that #96621 (comment)

Copy link
Contributor

@lylezhu2012 lylezhu2012 Oct 24, 2025

Choose a reason for hiding this comment

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

Did you double confirm?

The function bt_br_init() of file subsys/bluetooth/host/classic/br.c is not called anymore in your PR.

Copy link
Contributor

@lylezhu2012 lylezhu2012 Oct 24, 2025

Choose a reason for hiding this comment

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

And I tried your PR. No any device discovered by shell command br discovery on.

Please confirm you have tested your changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lylezhu2012 name would only be get by RNR(remote name request), and EIR name feature would be add in later patch.

Copy link
Contributor Author

@chengkai15 chengkai15 Oct 24, 2025

Choose a reason for hiding this comment

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

Did you double confirm?

The function bt_br_init() of file subsys/bluetooth/host/classic/br.c is not called anymore in your PR.

Yes, it only fix the warning that bt_br_init was conflict with another global variable br.h,

and I had to #include<classic/br.h> header because I was going to use IS_ENABLED CONFIG_BT_CLASSIC condition
from @jhedberg ‘s suggestion

**if (IS_ENABLED(CONFIG_BT_CLASSIC)) {**
	err = bt_br_write_local_name(bt_dev.name);
	if (err) {
		LOG_WRN("Unable to set local name");
		return err;
	}
}
Copy link
Contributor

@lylezhu2012 lylezhu2012 Oct 24, 2025

Choose a reason for hiding this comment

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

name would only be get by RNR(remote name request), and EIR name feature would be add in later patch.

I do not think so. With the changes of PR, the initialisation of Bluetooth Classic will be bypassed (the function bt_br_init() will be not called anymore). It means in your PR, a function that was working properly was changed to not work anymore. I think it is a regression.

Yes, it only fix the warning that bt_br_init was conflict with another global variable br.h,

and I had to #include<classic/br.h> header because I was going to use IS_ENABLED CONFIG_BT_CLASSIC condition from @jhedberg ‘s suggestion

I understand why you change the function name. But I think he was just trying to give you some advice on the issue itself. I don't think he expects to solve the issue by introducing new issues.

I have some suggestions for you,

  • Put the implementation of function bt_br_write_local_name() to file hci_core.c,
  • Or, declare the prototype of the function bt_br_write_local_name() in file hci_core.h,
  • Or, create a new internal header file for Classic, just likes as br_internal.h or br_interface.h.
Copy link
Contributor Author

@chengkai15 chengkai15 Oct 24, 2025

Choose a reason for hiding this comment

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

@lylezhu2012 NO,there is not change about bt_br_init but the name. I do not think old bt_br_init is a good design.
which only work when CONFIG_BT_CLASSIC was not define. Function names and implementations are opposite

if you has any question about that. please make it more clear.

I think it was more friendly to do something about classic bt hci init in the future. such as HCI flow control、Authenticaition timeout etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and I think the changes about bt_br_init is reasonable. The previous comment is my opinion

If there is a disagreement on the location of the bt_br_write_local_name function, we could discuss a best solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

NO,there is not change about bt_br_init but the name. I do not think old bt_br_init is a good design. which only work when CONFIG_BT_CLASSIC was not define. Function names and implementations are opposite

Whether it is a good design or not, the regression cannot be accepted.

@chengkai15
Copy link
Contributor Author

I think there is a case missing in this PR. The device name has been stored into NVM. And re-initialize the Bluetooth Host Stack. The device name will not be written to the controller.

#if defined(CONFIG_BT_DEVICE_NAME_DYNAMIC)
if (!strncmp(name, "name", len)) {
len = read_cb(cb_arg, &bt_dev.name, sizeof(bt_dev.name) - 1);
if (len < 0) {
LOG_ERR("Failed to read device name from storage"
" (err %zd)", len);
} else {
bt_dev.name[len] = '\0';
LOG_DBG("Name set to %s", bt_dev.name);
}
return 0;
}
#endif

updated

if (bt_dev.name[0] == '\0') {
bt_set_name(CONFIG_BT_DEVICE_NAME);
} else {
bt_set_name(bt_dev.name);
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a potential issue that the life of Flash will be reduced. Since there is not any change of device name, I think it is better to write device name to controller directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that is to fix board reboot scenario.
I try to simulate the boot scenario here using settings_load to ensure commit_settings takes effect.

When I initiate the bt_set_name method, my log shows that commit_settings is not triggered.
Is this because my environment is not right?

do you means that if the name has been set before, and the same name would set again?
bt_set_name would check and will not take effect if it is set again

Copy link
Contributor

Choose a reason for hiding this comment

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

When I initiate the bt_set_name method, my log shows that commit_settings is not triggered. Is this because my environment is not right?

The commit_settings() would be called when the function settings_load() called.

do you means that if the name has been set before, and the same name would set again?

Uh-huh, that is what your change is doing.

bt_set_name would check and will not take effect if it is set again

In theory I think this can be done. But currently, the new name is not checked in function bt_set_name().

Copy link
Contributor Author

@chengkai15 chengkai15 Feb 26, 2026

Choose a reason for hiding this comment

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

hi @lylezhu2012 @jhedberg please help review

I have verified last changes with qemu_x86 platform that the relevant functions and hci seem working normally, but this platform seems to be a pure memory simulation, and all data is lost after rebooting.

uart:~$ bt init 
Bluetooth initialized
Settings Loaded
[00:00:04.600,000] <dbg> bt_settings: bt_settings_init: 
[00:00:04.600,000] <inf> fs_nvs: 8 Sectors of 1024 bytes
[00:00:04.600,000] <inf> fs_nvs: alloc wra: 0, 3e8
[00:00:04.600,000] <inf> fs_nvs: data wra: 0, 0
[00:00:04.600,000] <dbg> settings: settings_nvs_backend_init: Initialized
[00:00:04.760,000] <inf> bt_hci_core: No ID address. App must call settings_load()
[00:00:04.760,000] <dbg> bt_settings: commit_settings: 
[00:00:04.760,000] <inf> bt_hci_core: HCI transport: H:4
[00:00:04.760,000] <inf> bt_hci_core: Identity: 00:1B:DC:F4:B1:39 (public)
[00:00:04.760,000] <inf> bt_hci_core: HCI: version 4.2 (0x08) revision 0x30e8, manufacturer 0x000a
[00:00:04.760,000] <inf> bt_hci_core: LMP: version 4.2 (0x08) subver 0x30e8
[00:00:04.760,000] <dbg> bt_settings: commit_settings: Storing Identity Information
uart:~$ bt settings-load
Settings loaded
[00:00:37.000,000] <dbg> bt_settings: set_setting: IRK[0] 5eaa77b537b4b4965d1cae18858595dc
[00:00:37.000,000] <dbg> settings: settings_call_set_handler: set-value OK. key: bt/irk
[00:00:37.000,000] <dbg> bt_settings: set_setting: ID[0] 00:1B:DC:F4:B1:39 (public)
[00:00:37.000,000] <dbg> settings: settings_call_set_handler: set-value OK. key: bt/id
[00:00:37.000,000] <dbg> settings: settings_call_set_handler: set-value OK. key: bt/hash
[00:00:37.000,000] <dbg> bt_settings: commit_settings: 
uart:~$ bt name "v1234"
uart:~$ bt name
Bluetooth Local Name: v1234
uart:~$ bt settings-load
Settings loaded
[00:01:20.980,000] <dbg> bt_settings: set_setting: Name set to v1234
[00:01:20.980,000] <dbg> settings: settings_call_set_handler: set-value OK. key: bt/name
[00:01:20.980,000] <dbg> bt_settings: set_setting: IRK[0] 5eaa77b537b4b4965d1cae18858595dc
[00:01:20.980,000] <dbg> settings: settings_call_set_handler: set-value OK. key: bt/irk
[00:01:20.980,000] <dbg> bt_settings: set_setting: ID[0] 00:1B:DC:F4:B1:39 (public)
[00:01:20.980,000] <dbg> settings: settings_call_set_handler: set-value OK. key: bt/id
[00:01:20.980,000] <dbg> settings: settings_call_set_handler: set-value OK. key: bt/hash
[00:01:20.980,000] <dbg> bt_settings: commit_settings: 
uart:~$ QEMU: Terminated
Copy link
Contributor

@lylezhu2012 lylezhu2012 Oct 24, 2025

Choose a reason for hiding this comment

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

name would only be get by RNR(remote name request), and EIR name feature would be add in later patch.

I do not think so. With the changes of PR, the initialisation of Bluetooth Classic will be bypassed (the function bt_br_init() will be not called anymore). It means in your PR, a function that was working properly was changed to not work anymore. I think it is a regression.

Yes, it only fix the warning that bt_br_init was conflict with another global variable br.h,

and I had to #include<classic/br.h> header because I was going to use IS_ENABLED CONFIG_BT_CLASSIC condition from @jhedberg ‘s suggestion

I understand why you change the function name. But I think he was just trying to give you some advice on the issue itself. I don't think he expects to solve the issue by introducing new issues.

I have some suggestions for you,

  • Put the implementation of function bt_br_write_local_name() to file hci_core.c,
  • Or, declare the prototype of the function bt_br_write_local_name() in file hci_core.h,
  • Or, create a new internal header file for Classic, just likes as br_internal.h or br_interface.h.
@jhedberg jhedberg added this to the v4.4.0 milestone Oct 27, 2025
@jhedberg
Copy link
Member

@chengkai15 are you still working on this? There seems to be open review/discussion threads.

@chengkai15
Copy link
Contributor Author

chengkai15 commented Dec 25, 2025

@chengkai15 are you still working on this? There seems to be open review/discussion threads.

updated with last changes

@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Feb 24, 2026
move bt local name commom hci to bt_br_write_local_name

Signed-off-by: Kai Cheng <chengkai@xiaomi.com>
Implement dynamic local name update for BR/EDR connections. When
CONFIG_BT_DEVICE_NAME_DYNAMIC is enabled, the device name is now
properly synchronized between LE and BR/EDR transports. Includes:

- Update BR/EDR local name when bt_set_name() is called
- Maintain consistency between LE and BR/EDR naming

Signed-off-by: Kai Cheng <chengkai@xiaomi.com>
Add missing function declaration for bt_br_write_local_name() to
resolve compilation errors when CONFIG_BT_DEVICE_NAME_DYNAMIC
is enabled. The function is now properly declared in classic.h
header file to avoid implicit declaration warnings.

Signed-off-by: Kai Cheng <chengkai@xiaomi.com>
When the controller supports BR/EDR and CONFIG_BT_CLASSIC is enabled,
call bt_br_init() instead of br_hci_init() to perform the full
BR/EDR Classic initialization (SSP mode, inquiry mode, local name,
Class of Device, etc.).

br_hci_init() is a local helper that only reads the buffer size for
non-Classic configurations. Using it when CONFIG_BT_CLASSIC is enabled
skips all Classic-specific HCI setup, breaking Bluetooth Classic
functionality.

Signed-off-by: Kai Cheng <chengkai@xiaomi.com>
@jhedberg jhedberg removed the Stale label Feb 26, 2026
strncpy((char *)name_cp->local_name, CONFIG_BT_DEVICE_NAME, sizeof(name_cp->local_name));

err = bt_hci_cmd_send_sync(BT_HCI_OP_WRITE_LOCAL_NAME, buf, NULL);
err = bt_br_write_local_name(CONFIG_BT_DEVICE_NAME);
Copy link
Member

Choose a reason for hiding this comment

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

The CONFIG_BT_DEVICE_NAME_DYNAMIC=y case should be handled in the settings commit callback, so you're missing a condition here:

	/* The dynamic case is handled in commit_settings() */
	if (!IS_ENABLED(CONFIG_BT_DEVICE_NAME_DYNAMIC)) {
		bt_br_write_local_name(CONFIG_BT_DEVICE_NAME);
	}

#if !defined(CONFIG_BT_CLASSIC)
static int bt_br_init(void)
static int br_hci_init(void)
Copy link
Member

Choose a reason for hiding this comment

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

Could you just rename this to hci_read_buffer_size()? That should hopefully eliminate confusion of having seemigly multiple "BR" init routines (this function isn't really a BR init routine, rather is just used to get the buffer size for LE in case the controller has the BR/EDR feature and doesn't have dedicated LE buffers.

err = bt_br_init();
if (IS_ENABLED(CONFIG_BT_CLASSIC)) {
err = bt_br_init();
} else {
Copy link
Member

Choose a reason for hiding this comment

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

I suppse } else if (IS_ENABLED(CONFIG_BT_CONN)) { would be appropriate for this branch, to make it clear that it's only used for a specific combination of build time conditions. You should be able to remove any #ifdef guards from the called function itself, since it'll get stripped away by the linker if not called in practice.

}
}

if (IS_ENABLED(CONFIG_BT_CLASSIC)) {
Copy link
Member

Choose a reason for hiding this comment

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

Until now bt_set_name() hasn't required HCI initialization to have completed. Now that you're actually trying to send a HCI command from here you're introducing that requirement. I think you're missing a condition here:

	if (IS_ENABLED(CONFIG_BT_CLASSIC) &&
	    atomic_test_bit(bt_dev.flags, BT_DEV_READY)) {
CONFIG_BT_DEVICE_NAME_MAX);
bt_dev.name[CONFIG_BT_DEVICE_NAME_MAX] = '\0';
} else {
bt_set_name(bt_dev.name);
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look correct since the purpose of bt_set_name() is to copy the given value to bt_dev.name. Therefore, calling it by passing bt_dev.name as the argument is odd at best. I think the logic should go something like:

	if (bt_dev.name[0] == '\0') {
		bt_set_name(CONFIG_BT_DEVICE_NAME);
	}

	if (IS_ENABLED(CONFIG_BT_CLASSIC) {
		bt_br_write_local_name(bt_dev.name);
	}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Bluetooth Classic Bluetooth Classic (BR/EDR) area: Bluetooth Host Bluetooth Host (excluding BR/EDR) area: Bluetooth

5 participants