Skip to content

att: Support deferred Read and Write Response handling. #97594

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+152 −0
Open

att: Support deferred Read and Write Response handling. #97594

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions include/zephyr/bluetooth/gatt.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,11 @@ struct bt_gatt_attr;
* @note The GATT server propagates the return value from this
* method back to the remote client.
*
* @note If this function returns ``-EINPROGRESS``, the read operation
* is deferred and the application must later send the response
* using @ref bt_gatt_send_read_response(). Deferred responses
* are only supported when Enhanced ATT (EATT) is not active.
*
Comment on lines +159 to +163
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment on lines +159 to +163
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment on lines +159 to +163
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +159 to +163
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +159 to +163
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* @param conn The connection that is requesting to read.
* NULL if local.
* @param attr The attribute that's being read
Expand Down Expand Up @@ -199,6 +204,11 @@ typedef ssize_t (*bt_gatt_attr_read_func_t)(struct bt_conn *conn,
* @note The GATT server propagates the return value from this
* method back to the remote client.
*
* @note If this function returns ``-EINPROGRESS``, the write operation
* is deferred and the application must later send the response
* using @ref bt_gatt_send_write_response(). Deferred responses
* are only supported when Enhanced ATT (EATT) is not active.
*
* @param conn The connection that is requesting to write
* @param attr The attribute that's being written
* @param buf Buffer with the data to write
Expand Down Expand Up @@ -2024,6 +2034,25 @@ struct bt_gatt_read_params {
*/
int bt_gatt_read(struct bt_conn *conn, struct bt_gatt_read_params *params);

/** @brief Send a deferred Read Response
*
* Used when an attribute read was deferred (attr->read returned
* -EINPROGRESS). The application later provides the
* response value via this API.
*
* @param conn Connection object.
* @param data Pointer to the attribute value buffer.
* @param length Length of the attribute value.
*
* @retval 0 Successfully queued response.
*
* @retval -ENOTCONN The connection is not established.
* @retval -EINVAL Invalid parameters.
* @retval -ENOMEM Out of memory.
* @retval -ENOTSUP EATT active and deferred responses not supported.
*/
int bt_gatt_send_read_response(struct bt_conn *conn, const void *data, uint16_t length);

struct bt_gatt_write_params;

/** @typedef bt_gatt_write_func_t
Expand Down Expand Up @@ -2073,6 +2102,24 @@ struct bt_gatt_write_params {
*/
int bt_gatt_write(struct bt_conn *conn, struct bt_gatt_write_params *params);

/**
* @brief Send a deferred Write Response
*
* Used when an attribute write was deferred (attr->write returned
* -EINPROGRESS). The application later
* responds using this API once the write has been processed.
*
* @param conn Connection object.
*
* @retval 0 Successfully queued response.
*
* @retval -ENOTCONN The connection is not established.
* @retval -EINVAL Invalid parameters.
* @retval -ENOMEM Out of memory.
* @retval -ENOTSUP EATT active and deferred responses not supported.
*/
int bt_gatt_send_write_response(struct bt_conn *conn);

/** @brief Write Attribute Value by handle without response with callback.
*
* This function works in the same way as @ref bt_gatt_write_without_response.
Expand Down
47 changes: 47 additions & 0 deletions subsys/bluetooth/host/att.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1401,6 +1401,17 @@ static ssize_t att_chan_read(struct bt_att_chan *chan,

read = attr->read(conn, attr, frag->data + frag->len, len,
offset);

if (read == -EINPROGRESS) {
#if defined(CONFIG_BT_EATT)
if (bt_att_is_enhanced(chan)) {
LOG_WRN("Async read not supported on EATT channel");
return BT_ATT_ERR_NOT_SUPPORTED;
}
#endif
return -EINPROGRESS;
}

if (read < 0) {
if (total) {
return total;
Expand Down Expand Up @@ -1586,6 +1597,7 @@ struct read_data {
uint16_t offset;
struct net_buf *buf;
uint8_t err;
bool async;
};

static uint8_t read_cb(const struct bt_gatt_attr *attr, uint16_t handle,
Expand Down Expand Up @@ -1618,6 +1630,12 @@ static uint8_t read_cb(const struct bt_gatt_attr *attr, uint16_t handle,

/* Read attribute value and store in the buffer */
ret = att_chan_read(chan, attr, data->buf, data->offset, NULL, NULL);

if (ret == -EINPROGRESS) {
data->async = true;
return BT_GATT_ITER_STOP;
}

if (ret < 0) {
data->err = err_to_att(ret);
return BT_GATT_ITER_STOP;
Expand Down Expand Up @@ -1666,6 +1684,12 @@ static uint8_t att_read_rsp(struct bt_att_chan *chan, uint8_t op, uint8_t rsp,
return 0;
}

if (data.async) {
net_buf_unref(data.buf);
/* Async read: release buffer, app send response later */
return 0;
}

bt_att_chan_send_rsp(chan, data.buf);

return 0;
Expand Down Expand Up @@ -2029,6 +2053,7 @@ struct write_data {
uint16_t len;
uint16_t offset;
uint8_t err;
bool async;
};

static uint8_t write_cb(const struct bt_gatt_attr *attr, uint16_t handle,
Expand Down Expand Up @@ -2063,6 +2088,12 @@ static uint8_t write_cb(const struct bt_gatt_attr *attr, uint16_t handle,
/* Write attribute value */
write = attr->write(data->conn, attr, data->value, data->len,
data->offset, flags);

if (write == -EINPROGRESS) {
data->async = true;
return BT_GATT_ITER_STOP;
}

if (write < 0 || write != data->len) {
data->err = err_to_att(write);
return BT_GATT_ITER_STOP;
Expand Down Expand Up @@ -2121,6 +2152,22 @@ static uint8_t att_write_rsp(struct bt_att_chan *chan, uint8_t req, uint8_t rsp,
return req == BT_ATT_OP_EXEC_WRITE_REQ ? data.err : 0;
}

if (data.async) {
#if defined(CONFIG_BT_EATT)
if (bt_att_is_enhanced(chan)) {
LOG_WRN("Async write not supported on EATT channel");
if (rsp) {
net_buf_unref(data.buf);
send_err_rsp(chan, req, handle, BT_ATT_ERR_NOT_SUPPORTED);
}
return req == BT_ATT_OP_EXEC_WRITE_REQ ? BT_ATT_ERR_NOT_SUPPORTED : 0;
}
#endif
net_buf_unref(data.buf);
/* Async write: release buffer, app send response later */
return 0;
}

if (data.buf) {
bt_att_chan_send_rsp(chan, data.buf);
}
Expand Down
58 changes: 58 additions & 0 deletions subsys/bluetooth/host/gatt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5155,6 +5155,64 @@ int bt_gatt_write_without_response_cb(struct bt_conn *conn, uint16_t handle,
return bt_att_send(conn, buf);
}

int bt_gatt_send_read_response(struct bt_conn *conn, const void *data, uint16_t length)
{
struct net_buf *buf;

__ASSERT(conn, "invalid connection\n");

if (conn->state != BT_CONN_CONNECTED) {
return -ENOTCONN;
}

#if defined(CONFIG_BT_EATT)
if (bt_eatt_count(conn) > 0) {
LOG_WRN("EATT active: async response not supported");
return -ENOTSUP;
}
#endif

buf = bt_att_create_pdu(conn, BT_ATT_OP_READ_RSP, length);
if (!buf) {
return -ENOMEM;
}

if (data && length) {
net_buf_add_mem(buf, data, length);
}

LOG_DBG("conn %p len %u", conn, length);

return bt_att_send(conn, buf);
}

int bt_gatt_send_write_response(struct bt_conn *conn)
{
struct net_buf *buf;

__ASSERT(conn, "invalid connection\n");

if (conn->state != BT_CONN_CONNECTED) {
return -ENOTCONN;
}

#if defined(CONFIG_BT_EATT)
if (bt_eatt_count(conn) > 0) {
LOG_WRN("EATT active: async write response not supported");
return -ENOTSUP;
}
#endif

buf = bt_att_create_pdu(conn, BT_ATT_OP_WRITE_RSP, 0);
if (!buf) {
return -ENOMEM;
}

LOG_DBG("conn %p", conn);

return bt_att_send(conn, buf);
}

static int gatt_exec_encode(struct net_buf *buf, size_t len, void *user_data)
{
struct bt_att_exec_write_req *req;
Expand Down
Loading