Skip to content

Bluetooth: Mesh: Fix pb-adv link close timeout #61551

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

Closed
wants to merge 1 commit into from
Closed

Bluetooth: Mesh: Fix pb-adv link close timeout #61551

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
103 changes: 72 additions & 31 deletions subsys/bluetooth/mesh/pb_adv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ LOG_MODULE_REGISTER(bt_mesh_pb_adv);

#define RETRANSMIT_TIMEOUT K_MSEC(CONFIG_BT_MESH_PB_ADV_RETRANS_TIMEOUT)
#define BUF_TIMEOUT K_MSEC(400)
#define CLOSING_TIMEOUT 3
#define CLOSING_TIMEOUT 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

5.3.1.4.3
A Link Close message is used to close a link. Since this message is not acknowledged, the sender shall repeat this message at least three times.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The message is sent three times. It is ensured by RETRANSMITS_LINK_CLOSE. CLOSING_TIMEOUT defines the, at least intended, wait time before the link should be closed in seconds. I think the default RETRANSMIT_TIMEOUT is 500 msec, so in testing the message is retransmitted 4-5 times.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps a counter could be used instead of timeout?

#define TRANSACTION_TIMEOUT 30

/* Acked messages, will do retransmissions manually, taking acks into account:
Expand Down Expand Up @@ -112,6 +112,15 @@ struct pb_adv {
struct k_work_delayable retransmit;
} tx;

struct {
/* Start timestamp of link close */
int64_t start;

/* Close reason */
enum prov_bearer_link_status reason;
} close_link;


/* Protocol timeout */
struct k_work_delayable prot_timer;
};
Expand All @@ -133,11 +142,18 @@ static void link_close(struct prov_rx *rx, struct net_buf_simple *buf);
static void prov_link_close(enum prov_bearer_link_status status);
static void close_link(enum prov_bearer_link_status status);

static inline bool close_timer_elapsed(void)
{
return k_uptime_get() - link.close_link.start > CLOSING_TIMEOUT * MSEC_PER_SEC;
}

static void buf_sent(int err, void *user_data)
{
enum prov_bearer_link_status reason = (enum prov_bearer_link_status)(int)user_data;

if (atomic_test_and_clear_bit(link.flags, ADV_LINK_CLOSING)) {
if (atomic_test_bit(link.flags, ADV_LINK_CLOSING) && close_timer_elapsed()) {
LOG_DBG("Close link timer elapsed - Closing link.");
atomic_clear_bit(link.flags, ADV_LINK_CLOSING);
close_link(reason);
return;
}
Expand Down Expand Up @@ -608,24 +624,6 @@ static void send_reliable(void)
k_work_reschedule(&link.tx.retransmit, RETRANSMIT_TIMEOUT);
}

static void prov_retransmit(struct k_work *work)
{
LOG_DBG("");

if (!atomic_test_bit(link.flags, ADV_LINK_ACTIVE)) {
LOG_WRN("Link not active");
return;
}

if (k_uptime_get() - link.tx.start > link.tx.timeout * MSEC_PER_SEC) {
LOG_WRN("Giving up transaction");
prov_link_close(PROV_BEARER_LINK_STATUS_TIMEOUT);
return;
}

send_reliable();
}

static struct net_buf *ctl_buf_create(uint8_t op, const void *data, uint8_t data_len,
uint8_t retransmits)
{
Expand Down Expand Up @@ -663,7 +661,7 @@ static int bearer_ctl_send(struct net_buf *buf)
return 0;
}

static int bearer_ctl_send_unacked(struct net_buf *buf, void *user_data)
static int bearer_ctl_send_unacked(struct net_buf *buf)
{
if (!buf) {
return -ENOMEM;
Expand All @@ -672,12 +670,57 @@ static int bearer_ctl_send_unacked(struct net_buf *buf, void *user_data)
prov_clear_tx();
k_work_reschedule(&link.prot_timer, PROTOCOL_TIMEOUT);

bt_mesh_adv_send(buf, &buf_sent_cb, user_data);
bt_mesh_adv_send(buf, NULL, NULL);
net_buf_unref(buf);

return 0;
}

static void bearer_ctl_send_link_close(void)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename to link_close_send?

{ /*
* According to mesh profile spec (5.3.1.4.3), the close message should
* be restransmitted at least three times.
*/
struct net_buf *buf =
ctl_buf_create(LINK_CLOSE, &link.close_link.reason, 1, RETRANSMITS_LINK_CLOSE);

if (!buf) {
LOG_ERR("Failed to create link close buffer");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we call close_link here in case of error or retry?

return;
}

prov_clear_tx();
k_work_reschedule(&link.prot_timer, PROTOCOL_TIMEOUT);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should it be started only once? And is there any need in it if protocol_timeout doesn't do anything if link is not ADV_LINK_ACTIVE?


bt_mesh_adv_send(buf, &buf_sent_cb, (void *)link.close_link.reason);
net_buf_unref(buf);
}

static void prov_retransmit(struct k_work *work)
{
LOG_DBG("");

if (atomic_test_bit(link.flags, ADV_LINK_CLOSING)) {
if (close_timer_elapsed()) {
LOG_DBG("Close link timer elapsed - Closing link.");
atomic_clear_bit(link.flags, ADV_LINK_CLOSING);
close_link(link.close_link.reason);
return;
}

bearer_ctl_send_link_close();
k_work_reschedule(&link.tx.retransmit, RETRANSMIT_TIMEOUT);
} else if (atomic_test_bit(link.flags, ADV_LINK_ACTIVE)) {
if (k_uptime_get() - link.tx.start > link.tx.timeout * MSEC_PER_SEC) {
LOG_WRN("Giving up transaction");
prov_link_close(PROV_BEARER_LINK_STATUS_TIMEOUT);
return;
}

send_reliable();
}
}

static int prov_send_adv(struct net_buf_simple *msg,
prov_bearer_send_complete_t cb, void *cb_data)
{
Expand Down Expand Up @@ -769,8 +812,7 @@ static void link_open(struct prov_rx *rx, struct net_buf_simple *buf)
LOG_DBG("Resending link ack");
/* Ignore errors, message will be attempted again if we keep receiving link open: */
(void)bearer_ctl_send_unacked(
ctl_buf_create(LINK_ACK, NULL, 0, RETRANSMITS_ACK),
(void *)PROV_BEARER_LINK_STATUS_SUCCESS);
ctl_buf_create(LINK_ACK, NULL, 0, RETRANSMITS_ACK));
return;
}

Expand All @@ -784,8 +826,7 @@ static void link_open(struct prov_rx *rx, struct net_buf_simple *buf)
net_buf_simple_reset(link.rx.buf);

err = bearer_ctl_send_unacked(
ctl_buf_create(LINK_ACK, NULL, 0, RETRANSMITS_ACK),
(void *)PROV_BEARER_LINK_STATUS_SUCCESS);
ctl_buf_create(LINK_ACK, NULL, 0, RETRANSMITS_ACK));
if (err) {
reset_adv_link();
return;
Expand Down Expand Up @@ -926,11 +967,11 @@ static void prov_link_close(enum prov_bearer_link_status status)
* be restransmitted at least three times. Retransmit the LINK_CLOSE
* message until CLOSING_TIMEOUT has elapsed.
Copy link
Collaborator

Choose a reason for hiding this comment

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

You moved CLOSING_TIMEOUT usage to another place but left a comment nonrelavant. Please keep the code clean for better maintainability.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sry, my bad 👍

*/
link.tx.timeout = CLOSING_TIMEOUT;
/* Ignore errors, the link will time out eventually if this doesn't get sent */
bearer_ctl_send_unacked(
ctl_buf_create(LINK_CLOSE, &status, 1, RETRANSMITS_LINK_CLOSE),
(void *)status);
link.close_link.reason = status;
link.close_link.start = k_uptime_get();

bearer_ctl_send_link_close();
k_work_reschedule(&link.tx.retransmit, RETRANSMIT_TIMEOUT);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move k_work_reschedule inside bearer_ctl_send_link_close?

}

void bt_mesh_pb_adv_init(void)
Expand Down