Skip to content

Comments

att over br modify patch#458

Open
liuX10 wants to merge 4 commits intoopen-vela:devfrom
liuX10:local_dev
Open

att over br modify patch#458
liuX10 wants to merge 4 commits intoopen-vela:devfrom
liuX10:local_dev

Conversation

@liuX10
Copy link
Contributor

@liuX10 liuX10 commented Feb 10, 2026

No description provided.

…ts limit

bug: v/84473

When the number of registered services exceeds the limit, records not captured by `gatt_sdp_records` will be returned. Subsequent attempts to locate pointer indices for release will fail.

Signed-off-by: liuxiang18 <liuxiang18@xiaomi.com>
…`bt_conn_lookup_addr_br`.

bug: v/84474

1. Add a call to bt_conn_unref(conn)
2. Use the locally stored conn in the sal layer to avoid executing `bt_conn_unref`

Signed-off-by: liuxiang18 <liuxiang18@xiaomi.com>
bug: v/84474

When the peer initiates a connection, the connection callback reporting logic lacks the `bt_conn_add` entry. This prevents the subsequent SAL layer from retrieving the corresponding `bt_conn_info_t` object for the address, leading to subsequent data transmission process failures.

Signed-off-by: liuxiang18 <liuxiang18@xiaomi.com>
bug: v/84474

When the SAL layer does not store the BREDR connection information, it returns info as NULL. At this point, attempting an erroneous address offset on info->conn will result in a null pointer return.

Signed-off-by: liuxiang18 <liuxiang18@xiaomi.com>
Copy link
Contributor

@zhongzhijie1 zhongzhijie1 left a comment

Choose a reason for hiding this comment

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

LGTM. The changes look good and address several issues:

  1. Memory Management: Fixed potential memory leaks in gatt_sdp_create_record (freeing attrs and record on failure) and do_gatts_disconnect (adding bt_conn_unref).
  2. Connection Tracking: zblue_gatts_connected_callback now uses bt_conn_add to ensure the connection is tracked and sets the GATT_ROLE_SERVER flag.
  3. ATT over BR/EDR Support: The changes in send_response, send_notification, and send_indication correctly implement a fallback to look up BR/EDR connections when an LE connection is not found, which is consistent with the PR title.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the Zephyr GATT server implementation to improve ATT-over-BR/EDR behavior, including connection bookkeeping and BR/EDR fallbacks when sending GATT responses/notifications/indications.

Changes:

  • Fix SDP record creation flow to return success only when a record slot is actually reserved; free allocations on failure.
  • Ensure BR/EDR disconnect path releases the bt_conn reference after lookup/disconnect.
  • Add BR/EDR connection fallback for server responses/notifications/indications when no LE connection is found, and ensure BR connection slots are created on connect.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +823 to +825
if (!slot->role) {
slot->role |= GATT_ROLE_SERVER;
}
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

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

slot->role is only updated when it is currently 0. If the connection slot already exists with GATT_ROLE_CLIENT set, this will skip setting GATT_ROLE_SERVER, leaving the role flags inconsistent for an ATT-over-BR server connection. Consider always OR-ing in GATT_ROLE_SERVER here (or using bt_conn_set_role() to merge roles consistently).

Suggested change
if (!slot->role) {
slot->role |= GATT_ROLE_SERVER;
}
slot->role |= GATT_ROLE_SERVER;

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

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

it is att over br, we have no gattc role to initial

Comment on lines +1319 to +1323
bt_conn_info_t* info;
BT_LOGW("%s, le conn null", __func__);

info = bt_conn_find(addr, BT_TRANSPORT_BREDR);
conn = info ? info->conn : NULL;
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

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

For BR/EDR-only operation, get_le_conn_from_addr() will commonly be NULL; logging this as a warning can spam logs on every response. Consider lowering this to debug, or only warning after both LE and BR lookups fail.

Copilot uses AI. Check for mistakes.
Comment on lines +1407 to +1411
bt_conn_info_t* info;
BT_LOGW("%s, le conn null", __func__);

context.conn = bt_conn_lookup_addr_br((bt_addr_t*)addr);
info = bt_conn_find(addr, BT_TRANSPORT_BREDR);
context.conn = info ? info->conn : NULL;
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

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

For BR/EDR-only operation, get_le_conn_from_addr() being NULL is expected; logging it as a warning can spam logs for frequent notifications. Consider lowering this to debug, or only warning after both LE and BR lookups fail.

Copilot uses AI. Check for mistakes.
Comment on lines +1506 to 1511
bt_conn_info_t* info;
BT_LOGW("%s, le conn null", __func__);

context.conn = bt_conn_lookup_addr_br((bt_addr_t*)addr);
info = bt_conn_find(addr, BT_TRANSPORT_BREDR);
context.conn = info ? info->conn : NULL;
if (!context.conn) {
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

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

For BR/EDR-only operation, get_le_conn_from_addr() being NULL is expected; logging it as a warning can spam logs for frequent indications. Consider lowering this to debug, or only warning after both LE and BR lookups fail.

Copilot uses AI. Check for mistakes.
}

return record;
free(attrs);
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest add err log

context.conn = get_le_conn_from_addr(addr);
if (!context.conn) {
bt_conn_info_t* info;
BT_LOGW("%s, le conn null", __func__);
Copy link
Contributor

Choose a reason for hiding this comment

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

no le conn, find bredr conn

slot = bt_conn_find(&addr, BT_TRANSPORT_BREDR);
slot = bt_conn_add(&addr, BT_TRANSPORT_BREDR);
if (!slot) {
BT_LOGE("%s, conn slot null", __func__);
Copy link
Contributor

Choose a reason for hiding this comment

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

br conn

context.conn = get_le_conn_from_addr(addr);
if (!context.conn) {
bt_conn_info_t* info;
BT_LOGW("%s, le conn null", __func__);

Choose a reason for hiding this comment

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

If this behavior is expected for BREDR, the LOG level here should not be set to warning; INFO is preferable.

Copy link
Contributor

@zhongzhijie1 zhongzhijie1 left a comment

Choose a reason for hiding this comment

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

There are remaining actions. The issue is not in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants