bluetooth: host: Allow concurrent advertising with multiple ids #34848

rugeGerritsen · 2021-05-05T07:24:39Z

The HCI specification creates additional complexity to allow this
configuration:

When a connection gets established, we need to know which
identity the HCI_LE_Connection_Complete event corresponds to.
The identity is a property of the advertising set.
Therefore we need the advertising handle.
The advertising handle is part of the
HCI_LE_Advertising_Set_Terminated event and is not part of
the HCI_LE_Connection_Complete event. Therefore
the information of both events needs to be combined.

By spec the LE_Connection_Complete comes first. Therefore we cache
this event until the identity is available.
The event is only cached when a connection gets established as
that is the only case where we need to resolve the identity.

As the caching requires more resources, it is only enabled if the
application requires multiple advertising sets and multiple
identities.

The host maps the HCI_LE_Advertising_Set_Terminated event with
the HCI_LE_Connection_Complete event by comparing the connection
handles.

Fixes: #31588

jostn · 2021-05-05T11:37:40Z

subsys/bluetooth/host/hci_core.h

+	 * should be processed or not.
+	 */
+	bool conn_complete_received;
+	struct bt_hci_evt_le_enh_conn_complete cached_conn_complete;


I believe this can lead to a race condition if two advertisers manage to establish a connection before the TERMINATED is received for the first one.

Should this scale by CONFIG_BT_ID_MAX?

I'm assuming that the race condition you are considering is in step 5 and 6 below:

Start advertising set 0 using identity 0

Start advertising set 1 using identity 1

Receive LE Connection Complete for advertising set 0

Receive LE Connection Complete for advertising set 1

Receive LE Advertising Set terminated event for advertising set 0

Receive LE Advertising Set terminated event for advertising set 1

If that sequence occurs, the host has no way of mapping the LE Advertising Set Terminated event to the Connection Complete event. That is, it can't map an identity to the newly established connection.

Therefore, according to errata 14871, the controller should guarantee that the Advertising Set Terminated event follows the Connection Complete event. Hence, the controller should prevent sequences like the one above from happening.

what about the warning in hci_core:1058? will that be raised in such scenario?

Yes, this warning (https://github.com/zephyrproject-rtos/zephyr/pull/34848/files#diff-774b3de86adb275da54becf2a0958a2d4dae03d63e47840037ecc575e9d658b4R1058 ) will be printed in case the controller is raising another enhanced connection complete event before the LE Advertising Set Terminated event is processed

The idea of caching the connection complete event is sound I believe. Since the terminated event contains a connection handle, we can do a lookup from the adv set to the enhanced connection complete event.

The current implementation would not support the scenario above, where we receive 2 connection complete events before the advertising set terminated event, it only supports caching of one such even.

I think this could be implemented as

Only Enchance Connection Complete Event, store the event in an array (same size as MIN(CONFIG_BT_MAX_CONN, CONFIG_BT_EXT_ADV_MAX_ADV_SET))

On Advertising Set terminated lookup the connection handle to find the matching enhanced connection complete event

Finalize connection complete

Furthermore, I believe this PR is missing some crucial connection cancellation features, e.g. if the application stops the advertising set before advertising set terminated event, but after the connection complete event (corner case, but can still cause a desync).

@Thalley , thanks for your review. I believe I've addressed your comments by doing the following:

Checking if the advertiser is still running before processing the cached connection complete event to avoid that the application gets a connection complete callback after stopping the advertiser.

No longer depend on the order of events from the controller. Now the host caches all events that result in a connection being established. The host will compare the connection handles in those two events in order to create the mapping. The cost is some extra RAM usage.

Thalley · 2021-05-06T11:09:19Z

subsys/bluetooth/host/hci_core.h

+	 * should be processed or not.
+	 */
+	bool conn_complete_received;
+	struct bt_hci_evt_le_enh_conn_complete cached_conn_complete;


The idea of caching the connection complete event is sound I believe. Since the terminated event contains a connection handle, we can do a lookup from the adv set to the enhanced connection complete event.

The current implementation would not support the scenario above, where we receive 2 connection complete events before the advertising set terminated event, it only supports caching of one such even.

I think this could be implemented as

Only Enchance Connection Complete Event, store the event in an array (same size as MIN(CONFIG_BT_MAX_CONN, CONFIG_BT_EXT_ADV_MAX_ADV_SET))

On Advertising Set terminated lookup the connection handle to find the matching enhanced connection complete event

Finalize connection complete

Furthermore, I believe this PR is missing some crucial connection cancellation features, e.g. if the application stops the advertising set before advertising set terminated event, but after the connection complete event (corner case, but can still cause a desync).

Thalley

A few formatting comments.

Since this may introduce introduce issues with enhanced connections, I would be a lot happier to approve if it was accompanied by some test that validates the change.

subsys/bluetooth/host/hci_core.c

The HCI specification creates additional complexity to allow this configuration: - When a connection gets established, we need to know which identity the HCI_LE_Connection_Complete event corresponds to. - The identity is a property of the advertising set. Therefore we need the advertising handle. - The advertising handle is part of the HCI_LE_Advertising_Set_Terminated event and is not part of the HCI_LE_Connection_Complete event. Therefore the information of both events needs to be combined. By spec the LE_Connection_Complete comes first. Therefore we cache this event until the identity is available. The event is only cached when a connection gets established as that is the only case where we need to resolve the identity. As the caching requires more resources, it is only enabled if the application requires multiple advertising sets and multiple identities. The host maps the HCI_LE_Advertising_Set_Terminated event with the HCI_LE_Connection_Complete event by comparing the connection handles. Signed-off-by: Rubin Gerritsen <rubin.gerritsen@nordicsemi.no>

This will exercise caching of the HCI_LE_Connection_Complete event in the host. Signed-off-by: Rubin Gerritsen <rubin.gerritsen@nordicsemi.no>

Thalley · 2021-05-06T14:44:46Z

subsys/bluetooth/host/adv.c

+				 */
+				bt_hci_le_enh_conn_complete(&bt_dev.cached_conn_complete[i].evt);
+			}
+			bt_dev.cached_conn_complete[i].valid = false;


It still seems that in case that the application disables the advertising set after the connection complete event is received, but before the adv terminated event, then we don't clean up the cached connection complete event, as there won't be a adv terminated event.

Thalley

I still think it's missing handling of a corner case.

rugeGerritsen · 2021-05-06T19:16:19Z

@Thalley , I have thought a bit more about the race conditions you have mentioned. I'm not convinced there are race conditions that need to be handled. LE_Set_Advertising_Enable is handled synchronously,
It may succeed or fail if it is issued before the LE_Connection_Complete event is fetched, and will fail if it is fetched afterwards. The controller will either send both the events LE_Connection_Complete and LE_Advertising_Set_Terminated or none of them. That will prevent the host and controller from ending up in an out-of-sync state.

Let me try to illustrate this by providing some examples:

Case 1: Adv is stopped with success:

HCI_LE_Set_Advertising_Enable (disable) -> Success
<controller>_evt_get() -> No connection complete events

Case 2: Adv is stopped but fails because the controller has a pending event:

A connection is established, an event is prepared.
HCI_LE_Set_Advertising_Enable (disable) -> Disallowed
bt_recv(LE_Connection_Complete)
bt_recv(LE_Advertising_Set_Terminated)

Case 3: Adv is stopped after the enhanced connection complete event is processed:

bt_recv(LE_Connection_Complete)
HCI_LE_Set_Advertising_Enable (disable) -> Disallowed. The controller connection complete
event has already been fetched from the controller.
bt_recv(LE_Advertising_Set_Terminated)

Furthermore, when restarting the advertiser will not result in race conditions as the flag BT_ADV_ENABLED is cleared when processing the LE_Advertising_Set_Terminated event.

Case 4: Advertiser is restared after fetching both events:

bt_le_adv_start()
bt_recv(LE_Connection_Complete)
bt_recv(LE_Advertising_Set_Terminated)
bt_le_adv_start(). No issue. All previous state has been cleaned up.

Case 5: Advertiser is restarted after fetching the first event:

bt_le_adv_start()
bt_recv(LE_Connection_Complete)
bt_le_adv_start() -> fails because the flag BT_ADV_ENABLED is not yet cleared.
bt_recv(LE_Advertising_Set_Terminated)

From this analysis I conclude that we don't need to validate the advertiser flags when processing these events. That is, remove the check if (atomic_test_bit(adv->flags, BT_ADV_ENABLED)

Is there something I've misunderstood or overlooked?

Thalley · 2021-05-06T20:33:33Z

@Thalley , I have thought a bit more about the race conditions you have mentioned. I'm not convinced there are race conditions that need to be handled. LE_Set_Advertising_Enable is handled synchronously,
It may succeed or fail if it is issued before the LE_Connection_Complete event is fetched, and will fail if it is fetched afterwards. The controller will either send both the events LE_Connection_Complete and LE_Advertising_Set_Terminated or none of them. That will prevent the host and controller from ending up in an out-of-sync state.

Let me try to illustrate this by providing some examples:

Case 1: Adv is stopped with success:

HCI_LE_Set_Advertising_Enable (disable) -> Success

_evt_get() -> No connection complete events

Case 2: Adv is stopped but fails because the controller has a pending event:

A connection is established, an event is prepared.

HCI_LE_Set_Advertising_Enable (disable) -> Disallowed

bt_recv(LE_Connection_Complete)

bt_recv(LE_Advertising_Set_Terminated)

Case 3: Adv is stopped after the enhanced connection complete event is processed:

bt_recv(LE_Connection_Complete)

HCI_LE_Set_Advertising_Enable (disable) -> Disallowed. The controller connection complete
event has already been fetched from the controller.

bt_recv(LE_Advertising_Set_Terminated)

Furthermore, when restarting the advertiser will not result in race conditions as the flag BT_ADV_ENABLED is cleared when processing the LE_Advertising_Set_Terminated event.

Case 4: Advertiser is restared after fetching both events:

bt_le_adv_start()

bt_recv(LE_Connection_Complete)

bt_recv(LE_Advertising_Set_Terminated)

bt_le_adv_start(). No issue. All previous state has been cleaned up.

Case 5: Advertiser is restarted after fetching the first event:

bt_le_adv_start()

bt_recv(LE_Connection_Complete)

bt_le_adv_start() -> fails because the flag BT_ADV_ENABLED is not yet cleared.

bt_recv(LE_Advertising_Set_Terminated)

From this analysis I conclude that we don't need to validate the advertiser flags when processing these events. That is, remove the check if (atomic_test_bit(adv->flags, BT_ADV_ENABLED)

Is there something I've misunderstood or overlooked?

You might a good point, and I think you are right that it should work. The sentence

The controller will either send both the events LE_Connection_Complete and LE_Advertising_Set_Terminated or none of them.
Makes sure of that. Thanks for checking! :)

Thalley

LGTM

github-actions bot added area: API Changes to public APIs area: Bluetooth area: Bluetooth Host labels May 5, 2021

rugeGerritsen force-pushed the ext-adv-multiple-ids branch 2 times, most recently from 4346318 to 38d95be Compare May 5, 2021 07:42

rugeGerritsen marked this pull request as ready for review May 5, 2021 09:44

rugeGerritsen requested review from jhedberg, joerchan and Vudentz as code owners May 5, 2021 09:44

trond-snekvik added this to the v2.6.0 milestone May 5, 2021

jostn reviewed May 5, 2021

View reviewed changes

jostn approved these changes May 5, 2021

View reviewed changes

carlescufi requested a review from Thalley May 5, 2021 12:26

zephyrbot requested a review from asbjornsabo May 5, 2021 18:20

zephyrbot assigned jhedberg May 5, 2021

Thalley requested changes May 6, 2021

View reviewed changes

rugeGerritsen force-pushed the ext-adv-multiple-ids branch from 38d95be to 3635534 Compare May 6, 2021 12:21

rugeGerritsen requested a review from Thalley May 6, 2021 12:25

rugeGerritsen force-pushed the ext-adv-multiple-ids branch from 3635534 to 6016c3d Compare May 6, 2021 12:26

Thalley requested changes May 6, 2021

View reviewed changes

rugeGerritsen force-pushed the ext-adv-multiple-ids branch from 6016c3d to 1e71dd5 Compare May 6, 2021 13:18

rugeGerritsen added 2 commits May 6, 2021 15:43

tests: bluetooth: advx: Support multiple sets and identies

7b9c14b

This will exercise caching of the HCI_LE_Connection_Complete event in the host. Signed-off-by: Rubin Gerritsen <rubin.gerritsen@nordicsemi.no>

rugeGerritsen force-pushed the ext-adv-multiple-ids branch from 1e71dd5 to 7b9c14b Compare May 6, 2021 13:45

rugeGerritsen requested review from aescolar and wopu-ot as code owners May 6, 2021 13:45

github-actions bot added the area: Tests Issues related to a particular existing or missing test label May 6, 2021

Thalley reviewed May 6, 2021

View reviewed changes

Thalley requested changes May 6, 2021

View reviewed changes

jhedberg approved these changes May 6, 2021

View reviewed changes

Thalley approved these changes May 6, 2021

View reviewed changes

jhedberg merged commit f05ea67 into zephyrproject-rtos:master May 7, 2021

carlescufi mentioned this pull request Aug 19, 2022

Bluetooth: IRK handling issue when using multiple local identities #22049

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bluetooth: host: Allow concurrent advertising with multiple ids #34848

bluetooth: host: Allow concurrent advertising with multiple ids #34848

rugeGerritsen commented May 5, 2021 •

edited by joerchan

Loading

jostn May 5, 2021

rugeGerritsen May 5, 2021

minkargar-zz May 5, 2021

rugeGerritsen May 5, 2021

Thalley May 6, 2021

rugeGerritsen May 6, 2021

Thalley May 6, 2021

Thalley left a comment

Thalley May 6, 2021

Thalley left a comment

rugeGerritsen commented May 6, 2021

Thalley commented May 6, 2021

Thalley left a comment

bluetooth: host: Allow concurrent advertising with multiple ids #34848

bluetooth: host: Allow concurrent advertising with multiple ids #34848

Conversation

rugeGerritsen commented May 5, 2021 • edited by joerchan Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thalley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thalley left a comment

Choose a reason for hiding this comment

rugeGerritsen commented May 6, 2021

Thalley commented May 6, 2021

Thalley left a comment

Choose a reason for hiding this comment

rugeGerritsen commented May 5, 2021 •

edited by joerchan

Loading