att: Support deferred Read and Write Response handling.

[zhongzhijie1]

Now stack handles read_request with a sync callback. It needs value and length to be returned right away.

This patch enhances the GATT server implementation to support deferred response handling for both Read and Write operations. It enables applications to perform long-running or context-dependent attribute operations without blocking the protocol layer.

Signed-off-by: Zhijie Zhong [email protected]

[github-actions]

Hello @zhongzhijie1, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

[alwa-nordic]

• I'm missing an update to the APIs bt_gatt_attr_read_func_t and bt_gatt_attr_write_func_t.

• I'm missing an explanation for why bt_gatt_send_read_response and bt_gatt_send_write_response take a handle argument.

The above are the reasons for my NAK. The rest can be discussed. The discussion would benefit from a clearly updated API documentation. Points to address:

• This API change is breaking functionality. Currently bt_gatt_attr.read may be invoked locally.

• Are we going to be breaking backwards compatibility?

• The change is not compatible with our implementation when EATT is enabled.

[zhongzhijie1]

• I'm missing an update to the APIs bt_gatt_attr_read_func_t and bt_gatt_attr_write_func_t.
• I'm missing an explanation for why bt_gatt_send_read_response and bt_gatt_send_write_response take a handle argument.

The above are the reasons for my NAK. The rest can be discussed. The discussion would benefit from a clearly updated API documentation. Points to address:

• This API change is breaking functionality. Currently bt_gatt_attr.read may be invoked locally.
• Are we going to be breaking backwards compatibility?
• The change is not compatible with our implementation when EATT is enabled.

@alwa-nordic

Got it, thanks.

I will update the documentation for bt_gatt_attr_read_func_t and bt_gatt_attr_write_func_t to explain the new -EINPROGRESS behavior.

I will also remove the handle argument from bt_gatt_send_read_response and bt_gatt_send_write_response.

The change will not break existing synchronous behavior. Deferred response feature are skipped when EATT is enabled.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[alwa-nordic]

I think this is a nice prototype PR. Let's plan how to proceed.

I have added some discussion points on the API I would like us to address.

I think at this point, we should add automated tests to check the implementation against the API.

[alwa-nordic]

We should clarify how the user should determine whether EATT is active. I think the user has to check IS_ENABLED(CONFIG_BT_EATT). Any other suggestions?

[alwa-nordic]

This API allows any attribute type to defer reads. The Host uses the read function locally in places and is not compatible with deferred reads, e.g. here:

zephyr/subsys/bluetooth/host/gatt.c

Line 3549 in 3973aaa

len = attr->read(conn, attr, ccc_bits_encoded, sizeof(ccc_bits_encoded), 0);

Either the API has to prevent this, or better yet: The local reads in Host are updated respecting the API change.

Now in prototype stage, we can fix it in the API. But I would like local reads to work eventually, so we need to give it some though now to be forwards compatible. I'll give my thoughts below:

I think we should have a new API to do local reads. The requests can appear to originate from a pseudo-connection. A local reader might need to allocate a pseudo connection themselves.

The ATT database should not give out pointers to bt_gatt_attr, like it does with bt_gatt_find_by_uuid(). Instead, local reads should happen through a handle, just like a remote connection does it. bt_gatt_find_by_uuid() is fundamentally unsafe with bt_gatt_service_unregister().

Any thoughts?

[alwa-nordic]

I think it's worth explicitly saying that the lifetime of buf ends when the function returns, even if it returned -EINPROGRESS.

[alwa-nordic]

I think we need to discuses how this API can be extended (it the future?) to support EATT and local reads.

If we decide to handle ATT requests over multiple bearers (EATT) on a single connection serially (one at a time), then there is no problem, since the connection is enough to identify the associated request.

On the other hand, if we can pipeline requests to the application, then we have to allow the application to reply in any order. In this case, we need something to identify the request in bt_gatt_send_read_response. And, no, the handle is not enough.

[alwa-nordic]

We should have a note informing the developer that the application needs to be able to accept one concurrent request for every connection (or risk blocking Bluetooth after all).

If we add local reads through pseudo-connections, then those count as well.

If we pipeline EATT, then it grows to one request per ATT Bearer in the system.