Unreachable code

[ahmedmoheb-nordic]

I think the following part of the code isn't reachable

zephyr/subsys/bluetooth/host/id.c

Line 298 in 7c3bf20

if (!(IS_ENABLED(CONFIG_BT_EXT_ADV) &&

It contradicts with another condition at the beginning of the function

zephyr/subsys/bluetooth/host/id.c

Line 288 in 7c3bf20

if (!(IS_ENABLED(CONFIG_BT_EXT_ADV) &&

[ahmedmoheb-nordic]

@alwa-nordic @jori-nordic could you check this?

[alwa-nordic]

You are correct. That is dead code.

[alwa-nordic]

The condition was changed recently in 6c40427. @cvinayak, do you think we introduced a bug?

The dead code specifies the requirement "Make sure that a Legacy advertiser using default ID has same RPA address as scanner roles.". I'm not sure where this requirement comes from. Do you have this at the top of you head, @joerchan? If we don't have a reason for the requirement, I suggest we delete the dead code.

[joerchan]

It's about making sure that OOB get local works as expected.

I suspect that if you start an advertiser using the legacy API and extended advertising is enabled and supported by the controller.
If you then call bt_le_oob_get_local you will not get the advertiser RPA address being transmitted on air.
This could lead to a failure to bond using OOB information.

[alwa-nordic]

I am still a bit confused after your reply. Does the scanner role have anything to do with this?

It seems to me like you are saying that bt_le_oob_get_local would not work correctly in some cases, and this is a workaround. Do you think it's possible to remove this code and fix bt_le_oob_get_local so that it gets the correct address?

[cvinayak]

The condition was changed recently in 6c40427. @cvinayak, do you think we introduced a bug?

My change indeed seems to introduce dead code, fixing the issue #49014 with the reason for the change mentioned in the commit message of 6c40427.

[joerchan]

Looking at the supposed the change in 6c40427 I'm wondering if this was actually just a wrong assumption about how the code is supposed to work, and possibly that commit should be reverted and another fix is needed to address the RPA timeout issue that it was trying to address.

[cvinayak]

Looking at the supposed the change in 6c40427 I'm wondering if this was actually just a wrong assumption about how the code is supposed to work, and possibly that commit should be reverted and another fix is needed to address the RPA timeout issue that it was trying to address.

6c40427 is fixing #49014. bond using OOB information related changes being the cause of #49014, could you point me to the section in BT spec where private address to be not changed for legacy advertising with default identity when Extended HCI commands are used and the Controller supports Extended Advertising?

[joerchan]

@cvinayak I can see that 49014 is a correct bug report. This appears to be a regression since zephyr 3.0.0, where this works correctly.
However your fix introduced another bug.

The best thing would be to revert your fix, find the regression and apply the fix based on that.

could you point me to the section in BT spec where private address to be not changed for legacy advertising with default identity when Extended HCI commands are used and the Controller supports Extended Advertising?

This has nothing to do with the BT spec, but the zephyr Bluetooth API.

Let's clear up some things related to the word "legacy".
The legacy API is the old advertising API: bt_le_adv_*
The extendand advertising API: bt_le_ext_adv_*.
We're not talking about a legacy advertiser here, but the API.

When you start an advertiser using the legacy API then the OOB information is provided by bt_le_oob_get_local
When you start an advertiseng using the extended API then the OOB informartion is provided by bt_le_ext_adv_oob_get_local.

The new bug now is that bt_le_oob_get_local does not return the RPA address of the advertiser.

You can verify this bug like this:
west build -b nrf52840dk_nrf52840 tests/bluetooth/shell -- -DCONFIG_BT_PRIVACY=y -DCONFIG_BT_LOG_SNIFFER_INFO=y -DCONFIG_BT_RPA_TIMEOUT=30

Using the nRF Sniffer for Bluetooth LE.
Add the printed IRK to the sniffer, and select follow IRK.

uart:~$ bt advertise on
Advertising started
[00:00:10.378,570] <inf> bt_id: bt_id_set_adv_private_addr: RPA: 5E:0B:7D:F9:9A:D1
uart:~$ bt oob
OOB data:
addr                          random                           confirm                         
   4E:44:19:A9:44:12 (random) 015ce2d5e52416dfc3f54a562822b10d c47cb850fd05737e21eb502cd65c8003
[00:00:23.610,137] <inf> bt_id: bt_id_set_adv_private_addr: RPA: 6D:28:FE:FE:A3:61
[00:00:23.610,717] <inf> bt_id: bt_id_set_private_addr: RPA: 4E:44:19:A9:44:12

The advertiser starts using 5E:0B:7D:F9:9A:D1. When calling bt_le_oob_get_local it returns 4E:44:19:A9:44:12 (random), but is advertising using 6D:28:FE:FE:A3:61.

On zephyr 3.0.0 release this works as expected.

Reverting the fix 6c40427 shows that the RPA is not updated, neither on calling bt_le_oob_get_local, or on an RPA timeout.

[joerchan]

Bisect shows this as the commit that broke the RPA timeout invalidation: 4e924b6

[cvinayak]

Bisect shows this as the commit that broke the RPA timeout invalidation: 4e924b6

Yes, bisect leads to 4e924b6. Do you know where the le_rpa_invalidate should now be?