- 
                Notifications
    
You must be signed in to change notification settings  - Fork 8.2k
 
Bluetooth: Classic: add local name #96621
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
          
 updated  | 
    
| 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.
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.
There was a problem hiding this comment.
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
......
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
move bt local name commom hci to bt_br_write_local_name Signed-off-by: Kai Cheng <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meant to request changes along with my previous comments
| 
               | 
          ||
| #include "addr_internal.h" | ||
| #include "adv.h" | ||
| #include "classic/br.h" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove CONFIG_BT_CLASSIC marco of br.h  to fix build warrning  when add IS_ENABLED for bt_br_write_local_name
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;
}
}
INFO    - The following issues were found (showing the top 10 items):
INFO    - 1) bluetooth.host.att.retry_on_sec_err.server on nrf52_bsim/native error (Build failure - error: implicit declaration of function bt_br_write_local_name [-Werror=implicit-function-declaration])
INFO    - 2) bluetooth.host.att.timeout on nrf52_bsim/native error (Build failure - error: implicit declaration of function bt_br_write_local_name [-Werror=implicit-function-declaration])
INFO    - 3) bluetooth.host.att.open_close on nrf52_bsim/native error (Build failure - error: implicit declaration of function bt_br_write_local_name [-Werror=implicit-function-declaration])
INFO    - 4) bluetooth.host.att.read_fill_buf.server on nrf52_bsim/native error (Build failure - error: implicit declaration of function bt_br_write_local_name [-Werror=implicit-function-declaration])
INFO    - 5) bluetooth.host.att.long_read on nrf52_bsim/native error (Build failure - error: implicit declaration of function bt_br_write_local_name [-Werror=implicit-function-declaration])
INFO    - 6) bluetooth.host.gatt.device_name on nrf52_bsim/native error (Build failure - error: implicit declaration of function bt_br_write_local_name [-Werror=implicit-function-declaration])
INFO    - 7) bluetooth.host.misc.conn_stress.peripheral on nrf52_bsim/native error (Build failure - error: implicit declaration of function bt_br_write_local_name [-Werror=implicit-function-declaration])
        
          
                subsys/bluetooth/host/hci_core.c
              
                Outdated
          
        
      | 
               | 
          ||
| #if !defined(CONFIG_BT_CLASSIC) | ||
| static int bt_br_init(void) | ||
| static int br_init(void) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename local bt_br_init to br_init to avoid function conflicts with subsys/bluetooth/host/classic/br.h
int bt_br_init(void);
          
 done  | 
    
There was a problem hiding this 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.
zephyr/subsys/bluetooth/host/settings.c
Lines 183 to 196 in ad7beb5
| #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 | 
There was a problem hiding this comment.
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,
- 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.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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;
	}
}
There was a problem hiding this comment.
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 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.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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: - Initialize BR/EDR local name with configured device name during init - Update BR/EDR local name when bt_set_name() is called - Maintain consistency between LE and BR/EDR naming Signed-off-by: Kai Cheng <[email protected]>
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 <[email protected]>
          
 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.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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_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.
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 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. 



add BR/EDR local name