Bluetooth: has: Ensure that notifications are sent #61980

MariuszSkamra · 2023-08-28T14:14:46Z

This ensures active preset index, HAS features and partially control point notifications are sent.
The code retries sending notifications in case of lack of available buffers.
Partially addresses #57454

subsys/bluetooth/audio/has.c

tests/bsim/bluetooth/audio/src/common.c

tests/bsim/bluetooth/audio/src/has_client_test.c

subsys/bluetooth/audio/has.c

tests/bsim/bluetooth/audio/src/has_client_test.c

subsys/bluetooth/audio/has.c

Thalley · 2023-10-11T08:29:38Z

tests/bsim/bluetooth/audio/src/has_client_test.c

Why are we mostly re-implementing the HAS client again here?

Couldn't we reuse a lot of the code from the actual implementation?

As HAS client unsubscribes from the notifications on disconnection, I cannot use it for offline behavior test cases

As HAS client unsubscribes from the notifications on disconnection, I cannot use it for offline behavior test cases

Wouldn't it be easier to implement support for bonding in the client, than reimplementing a new HAS client that supports that? This just seems like much more effort than fixing the missing feature from the current HAS client :)

But can be done in a separate PR.

tests/bsim/bluetooth/audio/src/has_client_test.c

tests/bsim/bluetooth/audio/test_scripts/has_offline.sh

MSEEHenrik · 2023-10-12T12:41:28Z

subsys/bluetooth/audio/has.c

@@ -589,7 +687,7 @@ static int bt_has_cp_generic_update(struct has_client *client, const struct has_
 	preset_changed_prepare(&buf, BT_HAS_CHANGE_ID_GENERIC_UPDATE, is_last);

 	generic_update = net_buf_simple_add(&buf, sizeof(*generic_update));
-	generic_update->prev_index = get_prev_preset_index(preset);
+	generic_update->prev_index = preset_get_prev_index(preset);


This will provide a wrapped index. The first item in the list has to have a prevIndex of 0.

MariuszSkamra · 2023-10-12T15:32:15Z

Fixed preset list notifications after reconnection if the tail of the preset list have changed (the las one preset have been removed).

In such case:

/* The last preset known to the client has been removed.
 * As we do not hold the information about the deleted presets, we need to use
 * Generic Update procedure to:
 *   1. Notify the presets that have been removed in range
 *      (PrevIndex = current_preset_last, Index=previous_preset_last)
 *   2. Notify deletion of preset Index=previous_preset_last.
 */

subsys/bluetooth/audio/has.c

This fixes testing and clearing features flag. Signed-off-by: Mariusz Skamra <mariusz.skamra@codecoup.pl>

This fixes code indentation. Signed-off-by: Mariusz Skamra <mariusz.skamra@codecoup.pl>

This removes revering the features value back to previous state if work submission failes. Even if it fails it indicates an internal sysworkq issue, so even retry won't help. The client can read the features value anyway, thus it's sane to just log an error in such case. Signed-off-by: Mariusz Skamra <mariusz.skamra@codecoup.pl>

This fixes missing setting of FLAG_CONTROL_POINT_NOTIFY flag that indicate whether submit control_point_work. In case the there are more indications/notifications to sent (is_last flag is unset), the FLAG_CONTROL_POINT_NOTIFY shall be set to resubmit control_point_work. Signed-off-by: Mariusz Skamra <mariusz.skamra@codecoup.pl>

This moves common code to set_preset_availability function to be called from bt_has_preset_available and bt_has_preset_unavailable. Signed-off-by: Mariusz Skamra <mariusz.skamra@codecoup.pl>

Verify that a HAS Server IUT sends changed characteristic notifications or indications when the Lower Tester reconnects. Signed-off-by: Mariusz Skamra <mariusz.skamra@codecoup.pl>

Defer sending the features, active index, preset list and preset read response to sysworkq and retry sending in case failed due to buffers not available at the moment. Signed-off-by: Mariusz Skamra <mariusz.skamra@codecoup.pl>

This refactors the preset list to use sys_slist API. There have been various issues seen while iterating presets, thus it's more save to use well-defined and tested sys_slist API. Signed-off-by: Mariusz Skamra <mariusz.skamra@codecoup.pl>

Some of the data shall be persistent across connections to bonded clients. This includes notidication state flags that are used to determine whether notify bonded client after reconnection. Signed-off-by: Mariusz Skamra <mariusz.skamra@codecoup.pl>

This fixes sending proper Preset List notifications after reconnection. The issue was observed when the last preset known to the client has been removed. As we do not hold the information about the deleted presets, we need to use Generic Update procedure to: 1. Notify the presets that have been removed in range (PrevIndex = current_preset_last, Index=previous_preset_last) 2. Notify deletion of preset Index=previous_preset_last. Signed-off-by: Mariusz Skamra <mariusz.skamra@codecoup.pl>

This fixes missing memset of parameters used for indications and/or notifications. Signed-off-by: Mariusz Skamra <mariusz.skamra@codecoup.pl>

Thalley · 2023-10-18T14:16:05Z

subsys/bluetooth/audio/Kconfig.has

@@ -9,6 +9,7 @@ menuconfig BT_HAS
 	select EXPERIMENTAL
 	select UTF8
 	select BT_GATT_DYNAMIC_DB
+	select BT_GATT_AUTO_UPDATE_MTU


This is an interesting addition.

We've seen issues with Android, where Android (as the central) did not properly update the MTU on connection, which mean that some notifications sent by us (as peripheral) could not be done, due to MTU not matching the requirements from e.g. BAP.

HOWEVER
BT_GATT_AUTO_UPDATE_MTU depends on BT_GATT_CLIENT, which may not always be the case here for BT_HAS. So I don't think we should add select BT_GATT_AUTO_UPDATE_MTU here, but maybe we need to wait with the notifications until the MTU has been exchanged?

This is, btw, a general issue with all the LE Audio service implementations, where we cannot assume that the MTU has been exchanged when we are the server (and not can we expect it to do it ourself, if we are not the client).

This is an interesting addition.

We've seen issues with Android, where Android (as the central) did not properly update the MTU on connection, which mean that some notifications sent by us (as peripheral) could not be done, due to MTU not matching the requirements from e.g. BAP.

Can you elaborate? Was the MTU requested too small?

Yes, we've seen issues where we try to send notifications on reconnect of size e.g. 30 (assuming that the MTU is 64 as mandated by e.g. the BAP spec), because the phone did not request an update to the MTU before our first notification. I think it would make sense for all services in profiles that depend on BAP (CAP, TMAP, HAP, etc.) wait with sending notifications on reconnect until after the GATT MTU has been exchanged (at least the ones where it may be larger than the initial MTU of 23).

I think that we should probably add select BT_GATT_AUTO_UPDATE_MTU to any profile enabled where we act as the GATT client (e.g. BT_HAS_CLIENT), but we should not enable it for profiles where we are the GATT server (e.g. BT_HAS), as we cannot be sure that BT_GATT_CLIENT is enabled.

I am unsure what happens if a Kconfig selects another Kconfig without satisfying the dependencies.

Yes, we've seen issues where we try to send notifications on reconnect of size e.g. 30 (assuming that the MTU is 64 as mandated by e.g. the BAP spec), because the phone did not request an update to the MTU before our first notification. I think it would make sense for all services in profiles that depend on BAP (CAP, TMAP, HAP, etc.) wait with sending notifications on reconnect until after the GATT MTU has been exchanged (at least the ones where it may be larger than the initial MTU of 23).

I would say it's not phone's fault that the server wants to send the notification that does not fit MTU.
On the other hand the server does not know when the client is ready to receive the notifications (e.g. the profiles or some application is ready).
As there is no dedicated L2CAP channel PSM the client connects to, but ATT which is available right after connection this causes issues like that IMO.

I think that we should probably add select BT_GATT_AUTO_UPDATE_MTU to any profile enabled where we act as the GATT client (e.g. BT_HAS_CLIENT), but we should not enable it for profiles where we are the GATT server (e.g. BT_HAS), as we cannot be sure that BT_GATT_CLIENT is enabled.

BT_HAS_CLIENT selects BT_GATT_AUTO_UPDATE_MTU already. For profiles that are written on top of GATT Server I think that we should truncate the notifications, and apply some timeout waiting tor ATT exchange.

BTW, it would be helpful if we could have a function that will return true/false to know whether MTU was or was not already exchanged.

Like in HAS case in security_changed callback, I would like to check whether I still need to wait for MTU to be exchanged or it was exchanged already but the MTU stayed at 23 bytes

BTW, it would be helpful if we could have a function that will return true/false to know whether MTU was or was not already exchanged.

We have bt_gatt_get_mtu, and I guess you could simply do bool mtu_exchanged = bt_gatt_get_mtu() == BT_ATT_DEFAULT_LE_MTU

This won't work, because if bt_gatt_get_mtu returns 23, it means either the MTU was not exchanged, or it was exchanged and is 23. So one does not know whether it may still expect MTU Exchange or not

Good point.

IIRC the bt_gatt_cb.att_mtu_updated is even called on connection (bt_att_connected calls att_chan_mtu_updated), so we can't even use the callback.

I'll raise a GH issue for this.

I've created #64175 and #64172

subsys/bluetooth/audio/has_internal.h

This will truncate ATT notifications/indications if exceed ATT MTU size. It is up to the client to exchange MTU. Signed-off-by: Mariusz Skamra <mariusz.skamra@codecoup.pl>

This makes use of bt_conn_get_info function to access the conn address. Signed-off-by: Mariusz Skamra <mariusz.skamra@codecoup.pl>

This adds more logs for debugging purposes. Signed-off-by: Mariusz Skamra <mariusz.skamra@codecoup.pl>

MariuszSkamra force-pushed the has_notify_retry branch 3 times, most recently from b884dd3 to 085952f Compare August 31, 2023 11:44

MariuszSkamra force-pushed the has_notify_retry branch from 561273c to f771b20 Compare September 6, 2023 05:56

MariuszSkamra changed the title ~~Bluetooth: has: Retry sending notification on error~~ Bluetooth: has: Ensure that notifications are sent Sep 6, 2023

MariuszSkamra marked this pull request as ready for review September 6, 2023 07:10

MariuszSkamra requested review from jhedberg, Vudentz, wopu-ot, Thalley and sjanc as code owners September 6, 2023 07:10

zephyrbot added area: Bluetooth Audio area: Bluetooth labels Sep 6, 2023

zephyrbot requested review from asbjornsabo, Casper-Bonde-Bose, fredrikdanebjer, kruithofa and larsgk September 6, 2023 07:10

zephyrbot assigned Thalley Sep 6, 2023

Thalley requested changes Sep 6, 2023

View reviewed changes

fredrikdanebjer reviewed Sep 25, 2023

View reviewed changes

subsys/bluetooth/audio/has.c Show resolved Hide resolved

tests/bsim/bluetooth/audio/src/has_client_test.c Outdated Show resolved Hide resolved

tests/bsim/bluetooth/audio/src/has_client_test.c Outdated Show resolved Hide resolved

MariuszSkamra force-pushed the has_notify_retry branch from f771b20 to 6e485c4 Compare October 10, 2023 05:52

Thalley requested changes Oct 11, 2023

View reviewed changes

MSEEHenrik reviewed Oct 12, 2023

View reviewed changes

MariuszSkamra force-pushed the has_notify_retry branch from 6e485c4 to a2f2940 Compare October 12, 2023 15:28

MariuszSkamra force-pushed the has_notify_retry branch 3 times, most recently from 6efa790 to bc400b9 Compare October 12, 2023 22:19

Thalley requested changes Oct 13, 2023

View reviewed changes

MariuszSkamra added 6 commits October 17, 2023 11:58

Bluetooth: audio: has: Fix checking wrong flag

81d5dad

This fixes testing and clearing features flag. Signed-off-by: Mariusz Skamra <mariusz.skamra@codecoup.pl>

Bluetooth: audio: has: Fix indentation

52950ea

This fixes code indentation. Signed-off-by: Mariusz Skamra <mariusz.skamra@codecoup.pl>

Bluetooth: audio: has: Factor out set_preset_availability function

68cd2db

This moves common code to set_preset_availability function to be called from bt_has_preset_available and bt_has_preset_unavailable. Signed-off-by: Mariusz Skamra <mariusz.skamra@codecoup.pl>

tests: Bluetooth: has: Add test Preset Changed Offline Behavior

a3e4cca

Verify that a HAS Server IUT sends changed characteristic notifications or indications when the Lower Tester reconnects. Signed-off-by: Mariusz Skamra <mariusz.skamra@codecoup.pl>

MariuszSkamra force-pushed the has_notify_retry branch from 8087ca0 to d8478f8 Compare October 17, 2023 15:06

MariuszSkamra marked this pull request as draft October 18, 2023 08:47

MariuszSkamra added 5 commits October 18, 2023 14:20

Bluetooth: audio: has: Defer notifications to sysworkq

76fe66c

Defer sending the features, active index, preset list and preset read response to sysworkq and retry sending in case failed due to buffers not available at the moment. Signed-off-by: Mariusz Skamra <mariusz.skamra@codecoup.pl>

Bluetooth: audio: has: Fix missing memset of parameters

5c932a7

This fixes missing memset of parameters used for indications and/or notifications. Signed-off-by: Mariusz Skamra <mariusz.skamra@codecoup.pl>

MariuszSkamra force-pushed the has_notify_retry branch 2 times, most recently from b2b7135 to 85d80f3 Compare October 18, 2023 13:12

MariuszSkamra requested a review from Thalley October 18, 2023 13:14

MariuszSkamra marked this pull request as ready for review October 18, 2023 13:14

Thalley requested changes Oct 18, 2023

View reviewed changes

MariuszSkamra force-pushed the has_notify_retry branch from 85d80f3 to 8087ac2 Compare October 18, 2023 15:25

fredrikdanebjer reviewed Oct 19, 2023

View reviewed changes

subsys/bluetooth/audio/has_internal.h Outdated Show resolved Hide resolved

MariuszSkamra added 3 commits October 19, 2023 10:57

Bluetooth: audio: has: Truncate Control Point notifications to ATT MTU

e6a55ea

This will truncate ATT notifications/indications if exceed ATT MTU size. It is up to the client to exchange MTU. Signed-off-by: Mariusz Skamra <mariusz.skamra@codecoup.pl>

Bluetooth: audio: has: Remove include of conn_internal header

71574e8

This makes use of bt_conn_get_info function to access the conn address. Signed-off-by: Mariusz Skamra <mariusz.skamra@codecoup.pl>

Bluetooth: audio: has: Minor logging improvement

46f74aa

This adds more logs for debugging purposes. Signed-off-by: Mariusz Skamra <mariusz.skamra@codecoup.pl>

MariuszSkamra force-pushed the has_notify_retry branch from 8087ac2 to 46f74aa Compare October 19, 2023 09:51

Thalley approved these changes Oct 19, 2023

View reviewed changes

MariuszSkamra requested a review from fredrikdanebjer October 19, 2023 15:12

MariuszSkamra mentioned this pull request Oct 20, 2023

Bluetooth: audio: has: Add non-volatile settings #64164

Merged

fredrikdanebjer approved these changes Oct 20, 2023

View reviewed changes

carlescufi merged commit 49e7030 into zephyrproject-rtos:main Oct 20, 2023
21 checks passed

MariuszSkamra deleted the has_notify_retry branch October 20, 2023 13:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bluetooth: has: Ensure that notifications are sent #61980

Bluetooth: has: Ensure that notifications are sent #61980

MariuszSkamra commented Aug 28, 2023 •

edited by carlescufi

Loading

Thalley Oct 11, 2023

MariuszSkamra Oct 12, 2023

Thalley Oct 13, 2023

MSEEHenrik Oct 12, 2023

MariuszSkamra commented Oct 12, 2023 •

edited

Loading

Thalley Oct 18, 2023

MariuszSkamra Oct 18, 2023

Thalley Oct 18, 2023

MariuszSkamra Oct 19, 2023 •

edited

Loading

Thalley Oct 19, 2023

MariuszSkamra Oct 19, 2023 •

edited

Loading

Thalley Oct 20, 2023

Thalley Oct 20, 2023

Bluetooth: has: Ensure that notifications are sent #61980

Bluetooth: has: Ensure that notifications are sent #61980

Conversation

MariuszSkamra commented Aug 28, 2023 • edited by carlescufi 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

MariuszSkamra commented Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MariuszSkamra Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MariuszSkamra Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MariuszSkamra commented Aug 28, 2023 •

edited by carlescufi

Loading

MariuszSkamra commented Oct 12, 2023 •

edited

Loading

MariuszSkamra Oct 19, 2023 •

edited

Loading

MariuszSkamra Oct 19, 2023 •

edited

Loading