zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.88k stars 6.63k forks source link

Bluetooth: Common Discover All feature #31306

Open Thalley opened 3 years ago

Thalley commented 3 years ago

Is your feature request related to a problem? Please describe. The idea behind this feature is that it is possible for multiple applications get the results of a single discovery procedure. If we have a couple of applications (or even modules in the BT subsys) that act like GATT clients and may want to discover several different (or even the same) services or characteristics on a GATT server, then currently all the discovery has to be either implemented in the same place by the application, or the application(s) will have to call multiple individual discovery functions.

Having a common discover (all) feature in Zephyr for the GATT client, which could offload a lot of the work from the application(s), has a lot of benefits in terms of:

1) Code size (could perhaps do with just one discover_param and callback function) 2) Only a single discovery ATT command for a specific handle (in case multiple apps need the same handle) 3) A single callback would allow for a central place to handle GATT caching etc. 4) Potentially faster discovery as the server and client only need to iterate on the GATT database on the server once

Describe the solution you'd like A procedure where it is possible for an application to register the UUIDs that it wants to get the handles, with a callback that is called once the UUIDs have been found on the GATT server. It should also be possible to automatically subscribe to specified characteristics. Some services may be optionally writable or have different levels of security, so the application(s) would need to be notified about that too.

The feature should (perhaps later) be able to utilize the full knowledge of the UUIDs and handles to provide a GATT cache solution (using the GATT cache feature) so that whenever the device reconnects the application could get the handles right away without having to do a discovery again.

The goal is that for an (GATT client) application, getting the handles it requires (if available on the server) should be a trivial, seamless and fast thing to do.

Describe alternatives you've considered None so far

Additional context Challenges to keep in mind: 1) Some services have a variable number of characteristics (and some may even have multiples of the same characteristic in a single service) 2) Some servers may have multiple instances of the same service 3) Some (primary services) may include other (secondary services)

Thalley commented 2 years ago

Proposal 1: Specific UUIDs

client_1:

static uint8_t client_1_service_cb(struct bt_conn *conn,
                   const struct bt_gatt_attr *attr,
                   struct bt_gatt_discover_params *params)
{
    ...
}

static uint8_t client_1_characteristic_cb(struct bt_conn *conn,
                      const struct bt_gatt_attr *attr,
                      struct bt_gatt_discover_params *params)
{
    ...
}

static client_1_init(void)
{
    bt_gatt_discover_register(SERVICE_UUID_1, client_1_service_cb);
    bt_gatt_discover_register(CHARACTERISTIC_UUID_1, client_1_characteristic_cb);
}

client_2:

static uint8_t client_2_discover_cb(struct bt_conn *conn,
                    const struct bt_gatt_attr *attr,
                    struct bt_gatt_discover_params *params)
{
    if (bt_uuid_cmp(attr->uuid, BT_UUID_XXX) {
        ...
    } else {
        ...
    }
}

static client_2_init(void)
{
    bt_gatt_discover_register(SERVICE_UUID_2, client_2_discover_cb);
    bt_gatt_discover_register(CHARACTERISTIC_UUID_2, client_2_discover_cb);
}

Proposal 2: Register service UUID

client_3:

static uint8_t client_3_discover_cb(struct bt_conn *conn,
                    const struct bt_gatt_attr *attr,
                    struct bt_gatt_discover_params *params)
{
    if (bt_uuid_cmp(attr->uuid, BT_UUID_XXX) {
        ...
    } else {
        ...
    }
}

static client_3_init(void)
{
    /* Callback called for all attributes in service */
    bt_gatt_discover_register(SERVICE_UUID_3, client_3_discover_cb);
}

Proposal 3: Register service UUID but with individual callbacks

client_4:

static uint8_t client_4_service_cb(struct bt_conn *conn,
                   const struct bt_gatt_attr *attr,
                   struct bt_gatt_discover_params *params)
{
    ...
}

static uint8_t client_4_characteristic_cb(struct bt_conn *conn,
                      const struct bt_gatt_attr *attr,
                      struct bt_gatt_discover_params *params)
{
    ...
}

static uint8_t client_4_include_cb(struct bt_conn *conn,
                   const struct bt_gatt_attr *attr,
                   struct bt_gatt_discover_params *params)
{
    ...
}

static uint8_t client_4_descriptor_cb(struct bt_conn *conn,
                      const struct bt_gatt_attr *attr,
                      struct bt_gatt_discover_params *params)
{
    ...
}

static client_4_init(void)
{
    bt_gatt_discover_register(SERVICE_UUID_1,
                  client_1_service_cb,
                  client_1_characteristic_cb,
                  client_4_include_cb,
                  client_4_descriptor_cb);
}
Thalley commented 2 years ago

There has been a consensus that this is a good idea.

The suggestion has been to do a primary service discovery, and for each service found check if anything has registered for the service UUID. If so, then we should perform a characteristic (or perhaps an attribute) discovery, and then provide the attributes in the services to the registering modules.

For that we can either A) For each service, do a discovery for just the service handles (i.e. one discovery procedure for each service). Pro: Less data sent over air. Con: Takes at least X * CI time, where X is the number of services and CI is the connection interval. B) Do a full characteristic/attribute discovery and use the service handles to quickly parse the results. Pro: Fewer discovery requests. Con: More data sent over air.

The registering modules should only register for the "main" UUID, and any included UUID discoveries will have to be handled separate. The overall order should then be something like

  1. Discover primary services
  2. Discover attributes for registered services
  3. If discovered attributes contain included services, do another discovery procedure for those
  4. If discovered attributes contain CCCDs, subscribe to those

How subscriptions should be handled is still a TBD.

Thalley commented 2 years ago

@asbjornsabo @MariuszSkamra @rymanluk @alwa-nordic @runegrunnet Please provide input on the above if my memory has failed me in terms of what we discussed.

KAGA164 commented 2 years ago

Do you plan to implement the GATT caching on the client side as a part of this issue ? @Thalley

Thalley commented 2 years ago

Do you plan to implement the GATT caching on the client side as a part of this issue ? @Thalley

We discussed this, and it would be a great opportunity to cache the handles etc.

Not sure if that part would be done in the first iteration of adding this, as that would require even more rewrite in the existing client implementations (to avoid caching the handles twice).

We did not talk much about the design of the caching either, and how/when/if it should be stored in flash as well.

zephyrbot commented 9 months ago

Hi @kruithofa,

This issue, marked as an Feature Request, was opened a while ago and did not get any traction. Please confirm the issue is correctly assigned and re-assign it otherwise.

Please take a moment to review if the issue is still relevant to the project. If it is, please provide feedback and direction on how to move forward. If it is not, has already been addressed, is a duplicate, or is no longer relevant, please close it with a short comment explaining the reason.

@Thalley you are also encouraged to help moving this issue forward by providing additional information and confirming this request/issue is still relevant to you.

Thanks!

Thalley commented 9 months ago

This issue is fine as is