Bluetooth: Host: bt_gatt_cb.att_mtu_updated only called when MTU exchange completes #67858

ubieda · 2024-01-19T23:09:50Z

This PR attempts to address #64172, by issuing the following changes:

bt_gatt_cb.att_mtu_updated is only called when the MTU exchange is completed (only once per connection).
Modified tests/bsim/bluetooth/host/att/mtu_update to directly verify MTU exchange takes place both on Central and Peripheral.
Updated Migration Guide document to reflect the behavior change of att_mu_updated as it was previously also triggered upon connection establishment (before MTU exchange).

NOTE: att_mtu_updated is not triggered if an MTU exchange results in an ATT_ERROR_RSP (e.g: Not supported). There's a comment on the issue whether that should also result in calling att_mtu_updated.

Status on PR Actionable Items:

samples/bluetooth/mtu_update/central/src/central_mtu_update.c

samples/bluetooth/mtu_update/central/src/main.c

samples/bluetooth/mtu_update/peripheral/src/peripheral_mtu_update.c

samples/bluetooth/mtu_update/central/src/central_mtu_update.c

samples/bluetooth/mtu_update/peripheral/src/main.c

tests/bsim/bluetooth/host/att/mtu_update/src/main.c

ubieda · 2024-01-22T16:23:45Z

@Thalley thanks for your feedback!

I've addressed the following changes:

Restored APIs used for the mtu_exchange samples.
Added the bt_gatt_cb_register() on the tests before running the samples (both for peripheral and central).
Addressed formatting suggestions.

Follow-up Actions:

Make sure change works with EATT Bearers. Extend BabbleSIM test to do so.

I don't think this will work for EATT bearers, as the MTU is negotiated as par of the connection request, so if I understood correctly, this will not provide MTU information to the application on EATT connection established. We should probably extend one of the EATT babblesim tests to also care about the MTU exchange to ensure that the change we do here, works as expected for EATT as well.

@Thalley Do you have any feedback on Bluetooth: Host: Unable to easily determine if the GATT MTU has been exchanged #64172 (comment)?

alwa-nordic · 2024-01-24T15:39:26Z

I propose adding more test cases to nail down the behavior:

a test for MTU 23 case
a test that connects EATT but does not perform an ATT MTU exchange
a test for when the exchange results in an error

What do you think?

ubieda · 2024-01-24T15:40:59Z

I propose adding more test cases to nail down the behavior:

a test for MTU 23 case

a test that connects EATT but does not perform an ATT MTU exchange

a test for when the exchange results in an error

What do you think?

@alwa-nordic Sounds good. I'll capture them in the tests!

Per issue zephyrproject-rtos#64172, `att_mtu_updated` is expected to only notify once the MTU exchange has taken place in a Bluetooth connection, and not once connection is established. This allows each GATT profile to use this callback as a way to be signaled when higher MTU is available, which is expected to occur once per connection. There is the exception for EATT L2CAP channels, where an MTU exchange takes place at the L2CAP level, during ATT connection establishment. Signed-off-by: Luis Ubieda <[email protected]>

Now is not triggered upon connection establishment. Signed-off-by: Luis Ubieda <[email protected]>

Previously this sample verified the MTU exchange in an indirect manner by being able to send a larger notification size. Now this adjustment allows directly verifying both on the Central and the Peripheral that the expected MTU exchange takes place, and that the listeners subscribed through `bt_gatt_cb_register` are notified when this exchange takes place (once per connection). Signed-off-by: Luis Ubieda <[email protected]>

Thalley · 2024-01-25T07:03:40Z

doc/releases/migration-guide-3.6.rst

+* The handler `att_mtu_updated` registered over the :c:func:`bt_gatt_cb_register`
+  API is no longer triggered upon connection establishment. It is now only
+  called when the MTU exchange has taken place.


It's still called on EATT connection establishment though

Thalley · 2024-01-25T07:04:42Z

samples/bluetooth/mtu_update/central/src/central_mtu_update.c

+	if (info.role == BT_CONN_ROLE_CENTRAL) {
+		(void)mtu_exchange(conn);
+	}


Seems superfluous to guard a function call with BT_CONN_ROLE_CENTRAL in a central sample.

Furthermore, both the central and peripheral is allowed to do the MTU exchange

Thalley · 2024-01-25T07:10:20Z

tests/bsim/bluetooth/host/att/mtu_update/src/main.c

+	if (central_mtu_count == 1 &&
+		central_mtu_tx == expected_mtu &&
+		central_mtu_rx == expected_mtu) {
+		PASS("MTU Update test passed [Central]\n");


Some indentation is off here

Thalley · 2024-01-25T07:10:37Z

tests/bsim/bluetooth/host/att/mtu_update/src/main.c

+	if (peripheral_mtu_count == 1 &&
+		peripheral_mtu_tx == expected_mtu &&
+		peripheral_mtu_rx == expected_mtu) {


Some indentation is off here

Suggested change

if (peripheral_mtu_count == 1 &&

peripheral_mtu_tx == expected_mtu &&

peripheral_mtu_rx == expected_mtu) {

if (peripheral_mtu_count == 1 &&

peripheral_mtu_tx == expected_mtu &&

peripheral_mtu_rx == expected_mtu) {

Thalley · 2024-01-25T07:11:24Z

tests/bsim/bluetooth/host/att/mtu_update/src/main.c

+		FAIL("MTU Update test failed [Central] - Count: %u, MTU RX: %u, MTU TX: %u\n",
+			central_mtu_count, central_mtu_rx, central_mtu_tx);


Suggested change

FAIL("MTU Update test failed [Central] - Count: %u, MTU RX: %u, MTU TX: %u\n",

central_mtu_count, central_mtu_rx, central_mtu_tx);

FAIL("MTU Update test failed [Central] - Count: %u, MTU RX: %u, MTU TX: %u\n",

central_mtu_count, central_mtu_rx, central_mtu_tx);

Thalley · 2024-01-25T07:11:33Z

tests/bsim/bluetooth/host/att/mtu_update/src/main.c

+		FAIL("MTU Update test failed [Peripheral] - Count: %u, MTU RX: %u, MTU TX: %u\n",
+			peripheral_mtu_count, peripheral_mtu_rx, peripheral_mtu_tx);


Suggested change

FAIL("MTU Update test failed [Peripheral] - Count: %u, MTU RX: %u, MTU TX: %u\n",

peripheral_mtu_count, peripheral_mtu_rx, peripheral_mtu_tx);

FAIL("MTU Update test failed [Peripheral] - Count: %u, MTU RX: %u, MTU TX: %u\n",

peripheral_mtu_count, peripheral_mtu_rx, peripheral_mtu_tx);

Thalley · 2024-01-25T07:13:25Z

tests/bsim/bluetooth/host/att/mtu_update/src/main.c

-uint8_t notify_data[100] = {};
-uint8_t is_data_equal;


Does it make sense to remove the notification part of the test?

Wouldn't it make sense to both verify the MTU values and verify the notification length?

jori-nordic · 2024-01-25T10:42:36Z

@ubieda sorry for the super-short notice, but do you want to join the meeting today (in an hour or so) to discuss this PR?

ubieda · 2024-01-25T11:29:03Z

@ubieda sorry for the super-short notice, but do you want to join the meeting today (in an hour or so) to discuss this PR?

@jori-nordic Yeah - I'll see y'all in a few minutes!

ubieda · 2024-01-29T18:21:56Z

Just to capture current status on this PR: It's marked as blocked on #68107. Once that is addressed, we can re-evaluate this PR and follow-up with the change-requests.

henrikbrixandersen · 2024-02-24T18:08:36Z

doc/releases/migration-guide-3.6.rst

+* The handler `att_mtu_updated` registered over the :c:func:`bt_gatt_cb_register`
+  API is no longer triggered upon connection establishment. It is now only
+  called when the MTU exchange has taken place.


This needs to go into the migration guide for v3.7.0 instead.

github-actions · 2024-04-25T00:29:55Z

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.

ubieda · 2024-04-30T12:51:44Z

@Thalley @jori-nordic feel free to close if, after the latest changes in the BT Stack, this work needs to be restarted / no longer a suitable approach.

jori-nordic · 2024-04-30T12:59:43Z

Yeah let's close it. It's linked to the RFC issue, so we can always go back and re-open it later.
Thanks for putting in the work!

zephyrbot added area: Bluetooth Host Bluetooth Host (excluding BR/EDR) area: Bluetooth Release Notes To be mentioned in the release notes area: Samples Samples labels Jan 19, 2024

zephyrbot requested review from alwa-nordic, asbjornsabo, henrikbrixandersen, hermabe, jhedberg, jori-nordic, kartben, MaureenHelm, nashif, sjanc, Thalley, theob-pro and Vudentz January 19, 2024 23:10

zephyrbot assigned jhedberg Jan 19, 2024

ubieda force-pushed the fix/bt/gatt/notif-mtu-changed-only-after-exchange branch 2 times, most recently from e82a864 to 5b28e24 Compare January 19, 2024 23:24

Thalley requested changes Jan 22, 2024

View reviewed changes

ubieda force-pushed the fix/bt/gatt/notif-mtu-changed-only-after-exchange branch from 5b28e24 to 731b5d4 Compare January 22, 2024 16:14

ubieda force-pushed the fix/bt/gatt/notif-mtu-changed-only-after-exchange branch from 731b5d4 to b24fe02 Compare January 25, 2024 06:42

ubieda added 3 commits January 25, 2024 01:54

doc: releases: migration-guide: 3.6: noted att_mtu_updated change

489f723

Now is not triggered upon connection establishment. Signed-off-by: Luis Ubieda <[email protected]>

ubieda force-pushed the fix/bt/gatt/notif-mtu-changed-only-after-exchange branch from b24fe02 to 08e9a29 Compare January 25, 2024 06:54

Thalley reviewed Jan 25, 2024

View reviewed changes

ubieda mentioned this pull request Jan 25, 2024

Bluetooth: Host: Unable to easily determine if the GATT MTU has been exchanged #64172

Closed

henrikbrixandersen requested changes Feb 24, 2024

View reviewed changes

github-actions bot added the Stale label Apr 25, 2024

jori-nordic closed this Apr 30, 2024

ubieda mentioned this pull request Aug 7, 2024

bluetooth: host: Add APIs to track Unenhanced ATT MTU #76808

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bluetooth: Host: bt_gatt_cb.att_mtu_updated only called when MTU exchange completes #67858

Bluetooth: Host: bt_gatt_cb.att_mtu_updated only called when MTU exchange completes #67858

ubieda commented Jan 19, 2024 •

edited

Loading

ubieda commented Jan 22, 2024 •

edited

Loading

alwa-nordic commented Jan 24, 2024

ubieda commented Jan 24, 2024

Thalley Jan 25, 2024

Thalley Jan 25, 2024

Thalley Jan 25, 2024

Thalley Jan 25, 2024

Thalley Jan 25, 2024

Thalley Jan 25, 2024

Thalley Jan 25, 2024

jori-nordic commented Jan 25, 2024

ubieda commented Jan 25, 2024

ubieda commented Jan 29, 2024 •

edited

Loading

henrikbrixandersen Feb 24, 2024

github-actions bot commented Apr 25, 2024

ubieda commented Apr 30, 2024

jori-nordic commented Apr 30, 2024

		FAIL("MTU Update test failed [Central] - Count: %u, MTU RX: %u, MTU TX: %u\n",
		central_mtu_count, central_mtu_rx, central_mtu_tx);

		FAIL("MTU Update test failed [Peripheral] - Count: %u, MTU RX: %u, MTU TX: %u\n",
		peripheral_mtu_count, peripheral_mtu_rx, peripheral_mtu_tx);

Bluetooth: Host: bt_gatt_cb.att_mtu_updated only called when MTU exchange completes #67858

Bluetooth: Host: bt_gatt_cb.att_mtu_updated only called when MTU exchange completes #67858

Conversation

ubieda commented Jan 19, 2024 • edited Loading

ubieda commented Jan 22, 2024 • edited Loading

alwa-nordic commented Jan 24, 2024

ubieda commented Jan 24, 2024

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

jori-nordic commented Jan 25, 2024

ubieda commented Jan 25, 2024

ubieda commented Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Apr 25, 2024

ubieda commented Apr 30, 2024

jori-nordic commented Apr 30, 2024

ubieda commented Jan 19, 2024 •

edited

Loading

ubieda commented Jan 22, 2024 •

edited

Loading

ubieda commented Jan 29, 2024 •

edited

Loading