Bluetooth: GATT: Unable to re-register GATT service

[PavelVPV]

Describe the bug If try to register a GATT service again after registering another GATT service, bt_gatt_service_register() returns -EINVAL here: https://github.com/zephyrproject-rtos/zephyr/blob/91841587d6fd3dd5975a595ee819f429487b1ec1/subsys/bluetooth/host/gatt.c#L1222-L1225

To Reproduce Steps to reproduce the behavior:

1. Register service A using bt_gatt_service_register()
2. Unregister service A using bt_gatt_service_unregister()
3. Register service B using bt_gatt_service_register()
4. Register service A again using bt_gatt_service_register()

Example: https://github.com/zephyrproject-rtos/zephyr/commit/bd73ef768388e26150ae39c45040eaebb971ec76

Expected behavior Both services are eventually registered successfully.

Impact The handle field of each bt_gatt_attr struct needs to be cleared manually which is not mentioned anywhere. https://github.com/zephyrproject-rtos/zephyr/blob/91841587d6fd3dd5975a595ee819f429487b1ec1/include/zephyr/bluetooth/gatt.h#L177-L178

[alwa-nordic]

I opened https://github.com/zephyrproject-rtos/zephyr/pull/68290 which tries to fix this issue by ignoring the handle in the attr when registering. But it turns out that EDTT is using this feature. So I closed the PR.

As for alternative solutions:

• We can keep track of whether the handle was allocated by the stack, and unset it when de-registering.
  • This could be a flag in the attr. Or it could be a flag in the service assuming we don't want to support partially defined handles.
  • Or we could think of some way to divide the handle space into auto-allocated and app-provided. EDTT is using small handle values, so we could use high values for auto-allocated services.
• When auto-allocating handles, we start from highest previously allocated handle. So we never give out the same handle twice. The danger is that we could run out of handle-space if there is a lot of new dynamic services. We could fallback on allocating a free hole if that happens, but this is a rabbit-hole of hard-to-document behavior.
• We treat this as a documentation bug and intentional behavior. We can recommend all dynamic services that are possible to be registered once before bt_init. Then the app is free to remove and re-add them whenever, and it will keep the handles fixed.

[PavelVPV]

• We treat this as a documentation bug and intentional behavior. We can recommend all dynamic services that are possible to be registered once before bt_init. Then the app is free to remove and re-add them whenever, and it will keep the handles fixed.

The mesh stack needs to register and unregister its service when it gets provisioned and unprovisioned. When device boots up, the mesh stack can only register the service after loading settings (this is because it doesn't know whether it is provisioned or not before settings are loaded).

[alwa-nordic]

• We treat this as a documentation bug and intentional behavior. We can recommend all dynamic services that are possible to be registered once before bt_init. Then the app is free to remove and re-add them whenever, and it will keep the handles fixed.

The mesh stack needs to register and unregister its service when it gets provisioned and unprovisioned. When device boots up, the mesh stack can only register the service after loading settings (this is because it doesn't know whether it is provisioned or not before settings are loaded).

What I meant is that before bt_init, the app registers all services. Then, still before bt_init, it unregisters all services. This is done purely for the side effect of filling out all bt_gatt_service.attrs[i].handle. This has the effect of allocating unique handles for all the services, and the services that are removed are effectively just hidden. Since all handles are unique, registering the services later will not fail like described in this issue.

[PavelVPV]

What I meant is that before bt_init, the app registers all services. Then, still before bt_init, it unregisters all services. This is done purely for the side effect of filling out all bt_gatt_service.attrs[i].handle. This has the effect of allocating unique handles for all the services, and the services that are removed are effectively just hidden. Since all handles are unique, registering the services later will not fail like described in this issue.

Ah, I see. This will affect the mesh stack still as the first mesh function is called after bt_init.

[kruithofa]

@jhedberg please add your thoughts

[alwa-nordic]

@jhedberg (Ref Thursday meeting) If we want to rename the perms field in bt_gatt_attr without breaking API, we can make it an anonymous union with both the new and old names.

[jhedberg]

We discussed several options in today's Bluetooth meeting. We could simply document this and require all users of bt_gatt_service_unregister() to add explicit clearing of the handles of the service attributes. However, it would be nicer to the apps if the stack would just do this. That said, the stack should avoid clearing any pre-allocated handles, which means that the stack needs to store "auto vs pre-allocated" information in the service or the attribute structs themselves. Since we don't support a "hybrid" solution where only some attributes of a service are auto-allocated, having the information in the service struct would be most natural. However, just looking at the current struct definitions, we can't modify the service struct without increasing the struct size. We can modify the attribute struct though, since there are unused bits in the uint16_t perm member. Because of this, I'd propose to do the extension there with something like the following:

    uint16_t perm:15,
         _auto:1;

The underscore is intended to indicate that this is a private field, but might be good to document that clearly as well (the idea is that we could more easily then move this into the service struct in the future, if we want).

[jhedberg]

@jhedberg (Ref Thursday meeting) If we want to rename the perms field in bt_gatt_attr without breaking API, we can make it an anonymous union with both the new and old names.

Yes, an anonymous union would work (e.g. net_buf uses this), however the bitfields approach (as I outlined in my previous comment) is a reasonable option too (IMO).

[PavelVPV]

We discussed several options in today's Bluetooth meeting. We could simply document this and require all users of bt_gatt_service_unregister() to add explicit clearing of the handles of the service attributes. However, it would be nicer to the apps if the stack would just do this. That said, the stack should avoid clearing any pre-allocated handles, which means that the stack needs to store "auto vs pre-allocated" information in the service or the attribute structs themselves. Since we don't support a "hybrid" solution where only some attributes of a service are auto-allocated, having the information in the service struct would be most natural. However, just looking at the current struct definitions, we can't modify the service struct without increasing the struct size. We can modify the attribute struct though, since there are unused bits in the uint16_t perm member. Because of this, I'd propose to do the extension there with something like the following:

  uint16_t perm:15,
       _auto:1;

The underscore is intended to indicate that this is a private field, but might be good to document that clearly as well (the idea is that we could more easily then move this into the service struct in the future, if we want).

This solution is fine by me.

[github-actions[bot]]

This issue 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 issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.

[github-actions[bot]]

This issue 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 issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.

[PavelVPV]

Hi @alwa-nordic, @jhedberg, I've responded to what solution is fine by me as it was discussed. Should this be fixed?

[jhedberg]

Hi @alwa-nordic, @jhedberg, I've responded to what solution is fine by me as it was discussed. Should this be fixed?

I think so, yes. If someone wants to submit a pull request https://github.com/zephyrproject-rtos/zephyr/issues/67377#issuecomment-1959408409 has the summary of possible ways of fixing it. The discussion could then continue in the PR if necessary.

[github-actions[bot]]

This issue 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 issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.