Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 18 additions & 10 deletions subsys/bluetooth/host/classic/br.c
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

#include "common/bt_str.h"

#include "br.h"
#include "host/hci_core.h"
#include "host/conn_internal.h"
#include "host/keys.h"
Expand Down Expand Up @@ -879,7 +880,6 @@ int bt_br_init(void)
struct net_buf *buf;
struct bt_hci_cp_write_ssp_mode *ssp_cp;
struct bt_hci_cp_write_inquiry_mode *inq_cp;
struct bt_hci_write_local_name *name_cp;
struct bt_hci_rp_read_default_link_policy_settings *rp;
struct net_buf *rsp;
int err;
Expand Down Expand Up @@ -932,15 +932,7 @@ int bt_br_init(void)
}

/* Set local name */
buf = bt_hci_cmd_alloc(K_FOREVER);
if (!buf) {
return -ENOBUFS;
}

name_cp = net_buf_add(buf, sizeof(*name_cp));
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 (err) {
return err;
}
Expand Down Expand Up @@ -1626,3 +1618,19 @@ int bt_br_unpair(const bt_addr_t *addr)

return 0;
}

int bt_br_write_local_name(const char *name)
{
struct net_buf *buf;
struct bt_hci_write_local_name *name_cp;

buf = bt_hci_cmd_alloc(K_FOREVER);
if (!buf) {
return -ENOBUFS;
}

name_cp = net_buf_add(buf, sizeof(*name_cp));
strncpy((char *)name_cp->local_name, name, sizeof(name_cp->local_name));

return bt_hci_cmd_send_sync(BT_HCI_OP_WRITE_LOCAL_NAME, buf, NULL);
}
3 changes: 3 additions & 0 deletions subsys/bluetooth/host/classic/br.h
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/*
* Copyright (c) 2025 Xiaomi Corporation
* Copyright (c) 2017-2021 Nordic Semiconductor ASA
* Copyright (c) 2015-2016 Intel Corporation
*
Expand All @@ -10,3 +11,5 @@ int bt_br_init(void);
void bt_br_discovery_reset(void);

bool bt_br_update_sec_level(struct bt_conn *conn);

int bt_br_write_local_name(const char *name);
27 changes: 17 additions & 10 deletions subsys/bluetooth/host/hci_core.c
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@

#include "addr_internal.h"
#include "adv.h"
#include "classic/br.h"
#include "common/hci_common_internal.h"
#include "common/bt_str.h"
#include "common/rpa.h"
Expand All @@ -67,10 +68,6 @@
#include "settings.h"
#include "smp.h"

#if defined(CONFIG_BT_CLASSIC)
#include "classic/br.h"
#endif

#if defined(CONFIG_BT_DF)
#include "direction_internal.h"
#endif /* CONFIG_BT_DF */
Expand Down Expand Up @@ -3975,10 +3972,9 @@ static int le_init(void)
return le_set_event_mask();
}

#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.

{
#if defined(CONFIG_BT_CONN)
#if !defined(CONFIG_BT_CLASSIC) && defined(CONFIG_BT_CONN)
struct net_buf *rsp;
int err;

Expand All @@ -3994,11 +3990,10 @@ static int bt_br_init(void)

read_buffer_size_complete(rsp);
net_buf_unref(rsp);
#endif /* CONFIG_BT_CONN */
#endif /* !CONFIG_BT_CLASSIC && CONFIG_BT_CONN */

return 0;
}
#endif /* !defined(CONFIG_BT_CLASSIC) */

static int set_event_mask(void)
{
Expand Down Expand Up @@ -4314,7 +4309,11 @@ static int hci_init(void)
}

if (BT_FEAT_BREDR(bt_dev.features)) {
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.

err = br_hci_init();
}
if (err) {
return err;
}
Expand Down Expand Up @@ -4893,6 +4892,14 @@ int bt_set_name(const char *name)
}
}

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)) {
err = bt_br_write_local_name(bt_dev.name);
if (err) {
LOG_WRN("Unable to set local name");
return err;
}
}

return 0;
#else
return -ENOMEM;
Expand Down
10 changes: 9 additions & 1 deletion subsys/bluetooth/host/settings.c
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,15 @@ 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);
/* No name in flash — just populate bt_dev.name with the default.
* Skip bt_set_name() to avoid an unnecessary flash write;
* the controller already has the default name from init.
*/
strncpy(bt_dev.name, CONFIG_BT_DEVICE_NAME,
CONFIG_BT_DEVICE_NAME_MAX);
bt_dev.name[CONFIG_BT_DEVICE_NAME_MAX] = '\0';
} 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
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);
	}
}
#endif
if (!bt_dev.id_count) {
Expand Down
Loading