From bf1660abad0505b673272773a5693fa57203b413 Mon Sep 17 00:00:00 2001 From: Stine Akredalen Date: Mon, 25 Nov 2024 15:08:12 +0100 Subject: [PATCH] Bluetooth: fix GATT service reregistering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fixed issue with reregistering of GATT services. * Added unit tests covering the GATT reregistering scenario. Signed-off-by: Stine Åkredalen --- include/zephyr/bluetooth/gatt.h | 8 +- subsys/bluetooth/host/gatt.c | 8 ++ tests/bluetooth/gatt/src/main.c | 179 ++++++++++++++++++++++++++++++++ 3 files changed, 194 insertions(+), 1 deletion(-) diff --git a/include/zephyr/bluetooth/gatt.h b/include/zephyr/bluetooth/gatt.h index c72cb821a726ade..5114e11ab1cc1f9 100644 --- a/include/zephyr/bluetooth/gatt.h +++ b/include/zephyr/bluetooth/gatt.h @@ -296,8 +296,14 @@ struct bt_gatt_attr { * satisfied before calling read() or write(). * * @sa bt_gatt_discover_func_t about this field. + * + * @note The `_auto_assigned_handle` flag in this bitfield indicates if the attribute + * handle was automatically assigned by the stack (1) or manually set by the + * application (0). + * This flag must not be modified by the application. */ - uint16_t perm; + uint16_t perm: 15; + bool _auto_assigned_handle: 1; }; /** @brief GATT Service structure */ diff --git a/subsys/bluetooth/host/gatt.c b/subsys/bluetooth/host/gatt.c index 023216073e03f36..bef02ba50a6e2b6 100644 --- a/subsys/bluetooth/host/gatt.c +++ b/subsys/bluetooth/host/gatt.c @@ -1307,9 +1307,11 @@ static int gatt_register(struct bt_gatt_service *svc) populate: /* Populate the handles and append them to the list */ for (; attrs && count; attrs++, count--) { + attrs->_auto_assigned_handle = 0; if (!attrs->handle) { /* Allocate handle if not set already */ attrs->handle = ++handle; + attrs->_auto_assigned_handle = 1; } else if (attrs->handle > handle) { /* Use existing handle if valid */ handle = attrs->handle; @@ -1727,6 +1729,12 @@ static int gatt_unregister(struct bt_gatt_service *svc) if (is_host_managed_ccc(attr)) { gatt_unregister_ccc(attr->user_data); } + + /* The stack should not clear any handles set by the user. */ + if (attr->_auto_assigned_handle) { + attr->handle = 0; + attr->_auto_assigned_handle = 0; + } } return 0; diff --git a/tests/bluetooth/gatt/src/main.c b/tests/bluetooth/gatt/src/main.c index 3487fa6c036eddf..d510b97e2938a1c 100644 --- a/tests/bluetooth/gatt/src/main.c +++ b/tests/bluetooth/gatt/src/main.c @@ -142,6 +142,185 @@ ZTEST(test_gatt, test_gatt_unregister) "Test service unregister failed"); } +/* Test that a service A can be re-registered after registering it once, unregistering it, and then + * registering another service B. + * No pre-allocated handles. Repeat the process multiple times. + */ +ZTEST(test_gatt, test_gatt_reregister) +{ + struct bt_gatt_attr local_test_attrs[] = { + /* Vendor Primary Service Declaration */ + BT_GATT_PRIMARY_SERVICE(&test_uuid), + + BT_GATT_CHARACTERISTIC(&test_chrc_uuid.uuid, BT_GATT_CHRC_READ | BT_GATT_CHRC_WRITE, + BT_GATT_PERM_READ_AUTHEN | BT_GATT_PERM_WRITE_AUTHEN, + read_test, write_test, test_value), + }; + + struct bt_gatt_attr local_test1_attrs[] = { + /* Vendor Primary Service Declaration */ + BT_GATT_PRIMARY_SERVICE(&test1_uuid), + + BT_GATT_CHARACTERISTIC(&test1_nfy_uuid.uuid, BT_GATT_CHRC_NOTIFY, BT_GATT_PERM_NONE, + NULL, NULL, &nfy_enabled), + BT_GATT_CCC(test1_ccc_cfg_changed, BT_GATT_PERM_READ | BT_GATT_PERM_WRITE), + }; + struct bt_gatt_service local_test_svc = BT_GATT_SERVICE(local_test_attrs); + struct bt_gatt_service local_test1_svc = BT_GATT_SERVICE(local_test1_attrs); + + /* Checks that the procedure is successful for a few iterations. However, a single iteration + * was sufficient to trigger the reported bug. + */ + for (int i = 0; i < 10; i++) { + + zassert_false(bt_gatt_service_register(&local_test_svc), + "Test service A registration failed"); + + zassert_false(bt_gatt_service_unregister(&local_test_svc), + "Test service A unregister failed"); + + /* Check that the handles are the same as before registering the service */ + for (int j = 0; j < local_test_svc.attr_count; j++) { + zassert_equal(local_test_svc.attrs[j].handle, 0x0000, + "Test service A handle not reset"); + } + + zassert_false(bt_gatt_service_register(&local_test1_svc), + "Test service B registration failed"); + + zassert_false(bt_gatt_service_register(&local_test_svc), + "Test service A re-registering failed..."); + + /* Clean up */ + zassert_false(bt_gatt_service_unregister(&local_test_svc), + "Test service A unregister failed"); + zassert_false(bt_gatt_service_unregister(&local_test1_svc), + "Test service B unregister failed"); + } +} + +/* Test that a service A can be re-registered after registering it once, unregistering it, and then + * registering another service B. + * Service A and B both have pre-allocated handles for their attributes. + * Check that pre-allocated handles are the same after unregistering as they were before + * registering the service. + */ +ZTEST(test_gatt, test_gatt_reregister_pre_allocated_handles) +{ + struct bt_gatt_attr local_test_attrs[] = { + /* Vendor Primary Service Declaration */ + BT_GATT_PRIMARY_SERVICE(&test_uuid), + + BT_GATT_CHARACTERISTIC(&test_chrc_uuid.uuid, BT_GATT_CHRC_READ | BT_GATT_CHRC_WRITE, + BT_GATT_PERM_READ_AUTHEN | BT_GATT_PERM_WRITE_AUTHEN, + read_test, write_test, test_value), + }; + + struct bt_gatt_attr local_test1_attrs[] = { + /* Vendor Primary Service Declaration */ + BT_GATT_PRIMARY_SERVICE(&test1_uuid), + + BT_GATT_CHARACTERISTIC(&test1_nfy_uuid.uuid, BT_GATT_CHRC_NOTIFY, BT_GATT_PERM_NONE, + NULL, NULL, &nfy_enabled), + BT_GATT_CCC(test1_ccc_cfg_changed, BT_GATT_PERM_READ | BT_GATT_PERM_WRITE), + }; + + struct bt_gatt_service prealloc_test_svc = BT_GATT_SERVICE(local_test_attrs); + struct bt_gatt_service prealloc_test1_svc = BT_GATT_SERVICE(local_test1_attrs); + + /* Pre-allocate handles for both services */ + for (int i = 0; i < prealloc_test_svc.attr_count; i++) { + prealloc_test_svc.attrs[i].handle = 0x0100 + i; + } + for (int i = 0; i < prealloc_test1_svc.attr_count; i++) { + prealloc_test1_svc.attrs[i].handle = 0x0200 + i; + } + + zassert_false(bt_gatt_service_register(&prealloc_test_svc), + "Test service A registration failed"); + + zassert_false(bt_gatt_service_unregister(&prealloc_test_svc), + "Test service A unregister failed"); + + /* Check that the handles are the same as before registering the service */ + for (int i = 0; i < prealloc_test_svc.attr_count; i++) { + zassert_equal(prealloc_test_svc.attrs[i].handle, 0x0100 + i, + "Test service A handle not reset"); + } + + zassert_false(bt_gatt_service_register(&prealloc_test1_svc), + "Test service B registration failed"); + + zassert_false(bt_gatt_service_register(&prealloc_test_svc), + "Test service A re-registering failed..."); + + /* Clean up */ + zassert_false(bt_gatt_service_unregister(&prealloc_test_svc), + "Test service A unregister failed"); + zassert_false(bt_gatt_service_unregister(&prealloc_test1_svc), + "Test service B unregister failed"); +} + +/* Test that a service A can be re-registered after registering it once, unregistering it, and then + * registering another service B. + * Service A has pre-allocated handles for its attributes, while Service B has handles assigned by + * the stack when registered. + * Check that pre-allocated handles are the same after unregistering as they were before + * registering the service. + */ +ZTEST(test_gatt, test_gatt_reregister_pre_allocated_handle_single) +{ + struct bt_gatt_attr local_test_attrs[] = { + /* Vendor Primary Service Declaration */ + BT_GATT_PRIMARY_SERVICE(&test_uuid), + + BT_GATT_CHARACTERISTIC(&test_chrc_uuid.uuid, BT_GATT_CHRC_READ | BT_GATT_CHRC_WRITE, + BT_GATT_PERM_READ_AUTHEN | BT_GATT_PERM_WRITE_AUTHEN, + read_test, write_test, test_value), + }; + + struct bt_gatt_attr local_test1_attrs[] = { + /* Vendor Primary Service Declaration */ + BT_GATT_PRIMARY_SERVICE(&test1_uuid), + + BT_GATT_CHARACTERISTIC(&test1_nfy_uuid.uuid, BT_GATT_CHRC_NOTIFY, BT_GATT_PERM_NONE, + NULL, NULL, &nfy_enabled), + BT_GATT_CCC(test1_ccc_cfg_changed, BT_GATT_PERM_READ | BT_GATT_PERM_WRITE), + }; + + struct bt_gatt_service prealloc_test_svc = BT_GATT_SERVICE(local_test_attrs); + struct bt_gatt_service auto_test_svc = BT_GATT_SERVICE(local_test1_attrs); + + /* Pre-allocate handles for one service only */ + for (int j = 0; j < prealloc_test_svc.attr_count; j++) { + prealloc_test_svc.attrs[j].handle = 0x0100 + j; + } + + zassert_false(bt_gatt_service_register(&prealloc_test_svc), + "Test service A registration failed"); + + zassert_false(bt_gatt_service_unregister(&prealloc_test_svc), + "Test service A unregister failed"); + + /* Check that the handles are the same as before registering the service */ + for (int i = 0; i < prealloc_test_svc.attr_count; i++) { + zassert_equal(prealloc_test_svc.attrs[i].handle, 0x0100 + i, + "Test service A handle not reset"); + } + + zassert_false(bt_gatt_service_register(&auto_test_svc), + "Test service B registration failed"); + + zassert_false(bt_gatt_service_register(&prealloc_test_svc), + "Test service A re-registering failed..."); + + /* Clean up */ + zassert_false(bt_gatt_service_unregister(&prealloc_test_svc), + "Test service A unregister failed"); + zassert_false(bt_gatt_service_unregister(&auto_test_svc), + "Test service B unregister failed"); +} + static uint8_t count_attr(const struct bt_gatt_attr *attr, uint16_t handle, void *user_data) {