Bluetooth: Classic: add local name#96621
Bluetooth: Classic: add local name#96621chengkai15 wants to merge 4 commits intozephyrproject-rtos:mainfrom
Conversation
jhedberg
left a comment
There was a problem hiding this comment.
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 |
subsys/bluetooth/host/classic/br.c
Outdated
| 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); | ||
|
|
There was a problem hiding this comment.
The device name storing feature will be broken by this change.
There was a problem hiding this comment.
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
......
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
done |
lylezhu2012
left a comment
There was a problem hiding this comment.
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.
zephyr/subsys/bluetooth/host/settings.c
Lines 183 to 196 in ad7beb5
subsys/bluetooth/host/hci_core.c
Outdated
There was a problem hiding this comment.
Why is the function name is changed here?
There are two cases if the classic feature is supported,
- If the classic is not enabled, call function
bt_br_init()of file subsys/bluetooth/host/hci_core.c to read the buffer size. - 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.
There was a problem hiding this comment.
@lylezhu2012 it would not affect classic functionality
there was discussion about that #96621 (comment)
There was a problem hiding this comment.
Did you double confirm?
The function bt_br_init() of file subsys/bluetooth/host/classic/br.c is not called anymore in your PR.
There was a problem hiding this comment.
And I tried your PR. No any device discovered by shell command br discovery on.
Please confirm you have tested your changes.
There was a problem hiding this comment.
@lylezhu2012 name would only be get by RNR(remote name request), and EIR name feature would be add in later patch.
There was a problem hiding this comment.
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;
}
}
There was a problem hiding this comment.
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 filehci_core.c, - Or, declare the prototype of the function
bt_br_write_local_name()in filehci_core.h, - Or, create a new internal header file for Classic, just likes as
br_internal.horbr_interface.h.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
updated |
|
| if (bt_dev.name[0] == '\0') { | ||
| bt_set_name(CONFIG_BT_DEVICE_NAME); | ||
| } else { | ||
| bt_set_name(bt_dev.name); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
When I initiate the
bt_set_namemethod, my log shows thatcommit_settingsis 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_namewould 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().
There was a problem hiding this comment.
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
subsys/bluetooth/host/hci_core.c
Outdated
There was a problem hiding this comment.
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 filehci_core.c, - Or, declare the prototype of the function
bt_br_write_local_name()in filehci_core.h, - Or, create a new internal header file for Classic, just likes as
br_internal.horbr_interface.h.
|
@chengkai15 are you still working on this? There seems to be open review/discussion threads. |
updated with last changes |
|
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. |
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>
|
| 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); |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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)) { |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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);
}


add BR/EDR local name