Bluetooth: attr->user_data is NULL when doing discovery with BT_GATT_DISCOVER_ATTRIBUTE

[asbjornsabo]

Describe the bug
Changing the discovery type from BT_GATT_DISCOVER_CHARACTERISTIC to BT_GATT_DISCOVER_ATTRIBUTE results in the attr->user_data for discovered characteristics being NULL in the discovery callback.

A clear and concise description of what the bug is.
I have the following code in a discovery callback:

if (!bt_uuid_cmp(attr->uuid, BT_UUID_GATT_CHRC)) {
	BT_DBG("It is a characteristic");
} else if (!bt_uuid_cmp(attr->uuid, BT_UUID_GATT_CCC)) {
	BT_DBG("It is a CCCD");
	return BT_GATT_ITER_CONTINUE;
} else {
	BT_DBG("It is something else");
	return BT_GATT_ITER_CONTINUE;
}

/* We have found an attribute, and it is a characteristic */
/* Find out which attribute, and subscribe if we should */
BT_DBG("attr->user_data: %p", attr->user_data);
chrc = (struct bt_gatt_chrc *)attr->user_data;
BT_DBG("UUID: %s", bt_uuid_str(chrc->uuid));

If the discovery is called with BT_GATT_DISCOVER_CHARACTERISTIC, this gives results as follows:

[00:00:55.879,455] <dbg> bt_mcc: discover_mcs_char_func: [ATTRIBUTE] handle 0x00BB
[00:00:55.879,486] <dbg> bt_mcc: discover_mcs_char_func: It is a characteristic
[00:00:55.879,547] <dbg> bt_mcc: discover_mcs_char_func: attr->user_data: 0x200176a0
[00:00:55.879,608] <dbg> bt_mcc: discover_mcs_char_func: UUID: 2b93

If instead the discovery is called with BT_GATT_DISCOVER_ATTRIBUTE, the results are:

[00:00:54.949,890] <dbg> bt_mcc: discover_mcs_char_func: [ATTRIBUTE] handle 0x00BB
[00:00:54.949,890] <dbg> bt_mcc: discover_mcs_char_func: It is a characteristic
[00:00:54.949,920] <dbg> bt_mcc: discover_mcs_char_func: attr->user_data: (nil)
[00:00:54.949,981] <dbg> bt_mcc: discover_mcs_char_func: UUID: 2000

Note that the attr->user_data is NULL. (And de-referencing it of course gives meaningless results.)

Please also mention any information which could help others to understand
the problem you're facing:

• What target platform are you using?

nRF52840

• What have you tried to diagnose or workaround this issue?

The two different types of discovery have different implementations - one uses gatt_find_info(), the other gatt_read_type().

Additional info from @Emil-Gydesen-Bose

The only place we use BT_GATT_DISCOVER_ATTRIBUTE is the BT shell, and that only prints attr->uuid and never casts the results.
BT_GATT_DISCOVER_ATTRIBUTE is furthermore the only place where gatt_find_info (and consequently gatt_find_info_rsp) is called

For BT_GATT_DISCOVER_CHARACTERISTIC we do

        value = (struct bt_gatt_chrc)BT_GATT_CHRC_INIT(
            &u.uuid, sys_le16_to_cpu(chrc->value_handle),
            chrc->properties);
        attr = (struct bt_gatt_attr)BT_GATT_ATTRIBUTE(
            BT_UUID_GATT_CHRC, 0, NULL, NULL, &value);
        attr.handle = handle;

        if (params->func(conn, &attr, params) == BT_GATT_ITER_STOP) {
            return 0;
        }

when constructing the callback data, but for BT_GATT_DISCOVER_ATTRIBUTE we only do

        attr = (struct bt_gatt_attr)BT_GATT_ATTRIBUTE(
            &u.uuid, 0, NULL, NULL, NULL);
        attr.handle = handle;

        if (params->func(conn, &attr, params) == BT_GATT_ITER_STOP) {
            return;
        }

To Reproduce
Steps to reproduce the behavior:

Check out https://github.com/asbjornsabo/zephyr/tree/mcc_discovery_bug
(This is very close to zephyr main)

Build the bluetooth shell with the audio configuration file:
zephyrproject/zephyr/tests/bluetooth/shell$ rm -rf build && cmake -H. -Bbuild -GNinja -DBOARD=nrf52840dk_nrf52840 -DCONF_FILE=audio.conf

zephyrproject/zephyr/tests/bluetooth/shell$ cd build/ && ninja

Run on two nRF5240 kits, initialize bt, mcc and mpl, connect, run mcc discover_mcs command.

Expected behavior
A clear and concise description of what you expected to happen.

I'd expect

chrc = (struct bt_gatt_chrc)attr->user_data;
BT_DBG("UUID: %s", bt_uuid_str(chrc->uuid));

To work the same regardless of the type of discovery as long as attr->uuid == BT_UUID_GATT_CHRC

Impact
What impact does this issue have on your progress (e.g., annoyance, showstopper)

Right now: Hindering my rewrite of the discovery to enqueue fewer subscriptions at a time so that it does not fail.
In principle: Inconsistent and undocumented behavior in callbacks. Not possible to discover characteristics and CCCDs in same discovery (?).

Environment (please complete the following information):

• OS: (e.g. Linux, MacOS, Windows)
  Linux

• Toolchain (e.g Zephyr SDK, ...)

• Zephyr SDK

• Commit SHA or Version used
  asbjornsabo@421e179
  from branch given above

[Thalley]

Useful addition from the bt_gatt_discover_func_t documentation:

 *  The attribute object as well as its UUID and value objects are temporary and
 *  must be copied to in order to cache its information.
 *  Only the following fields of the attribute contains valid information:
 *   - uuid      UUID representing the type of attribute.
 *   - handle    Handle in the remote database.
 *   - user_data The value of the attribute.
 *               Will be NULL when discovering descriptors
 *
 *  To be able to read the value of the discovered attribute the user_data
 *  must be cast to an appropriate type.
 *   - @ref bt_gatt_service_val when UUID is @ref BT_UUID_GATT_PRIMARY or
 *     @ref BT_UUID_GATT_SECONDARY.
 *   - @ref bt_gatt_include when UUID is @ref BT_UUID_GATT_INCLUDE.
 *   - @ref bt_gatt_chrc when UUID is @ref BT_UUID_GATT_CHRC.

The documentation states that it is NULL for descriptors, but should otherwise provide one of the above structs as the user_data

[Thalley]

Alternative way of reproducing it is also to simply use the bt shell and run gatt discover and output the attr->user_data value, as that also does discovery with BT_GATT_DISCOVER_ATTRIBUTE

[cvinayak]

@asbjornsabo @Emil-Gydesen-Bose as you have done a detailed analysis and may have solutions that can be discussed, may be you could send a PR?

[Thalley]

Either @asbjornsabo or I will take a crack at it :)