-
Notifications
You must be signed in to change notification settings - Fork 33
fix gattc iOS ANCS issues #453
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.
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
base: dev
Are you sure you want to change the base?
Conversation
| } | ||
|
|
||
| if (err) { | ||
| if (err && err != -EALREADY) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Call if_gattc_on_element_changed if -EALREADY
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you means if_gattc_on_element_subscribed
| } | ||
|
|
||
| if (err) { | ||
| if (err && err != -EALREADY) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Call if_gattc_on_element_changed if -EALREADY
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| struct bt_conn_info info; | ||
| bt_address_t addr; | ||
|
|
||
| get_le_addr_from_conn(conn, &addr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All APIs should start with the prefix bt_.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not in scope of this PR discussion; this is not a newly introduced API.
|
|
||
| addr.type = req->addr_type; | ||
| if (req->addr_type == BT_LE_ADDR_TYPE_UNKNOWN) { | ||
| addr.type = adapter_get_le_remote_address_type(&req->addr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does we store the addr_type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return false; | ||
| } | ||
|
|
||
| bt_uuid_to_uuid128(&uuid, u2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why only support 128 bit uuid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not only support 128, we can handle all type, but like old stack, all uuid from stack will convert to 128 Bits
and the log print is like
16Bits uuid like [0x00000000-0000-0000-0000-00000000xxxx]
now we show it as 0000xxxx-0000-1000-8000-00805F9B34FB.
So I change it to 128 to print pretty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a significant drawback if uuid16 must be handled as uuid128.
If we need to correct the printing in bttool, we can just go with #414 (see changes in gatt_client.c and bt_uuid.c).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this commit can drop, not necessary
| } | ||
|
|
||
| if (err) { | ||
| if (err && err != -EALREADY) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest return errno status code to app
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now cannot do it , no helper convert gatt err to gatt_service error status
| int err; | ||
|
|
||
| addr.type = req->addr_type; | ||
| if (req->addr_type == BT_LE_ADDR_TYPE_UNKNOWN) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggest check address type, and suggest app need to past address type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e7c573c to
e8794dc
Compare
| return false; | ||
| } | ||
|
|
||
| bt_uuid_to_uuid128(&uuid, u2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggest remove.
here just does uuid copy, but not uuid—>uuid128
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove done
|
|
||
| addr.type = req->addr_type; | ||
| if (req->addr_type == BT_LE_ADDR_TYPE_UNKNOWN) { | ||
| addr.type = adapter_get_le_remote_address_type(&req->addr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggest add error handle for get address type fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e8794dc to
58b1597
Compare
❌ CLA Signature Required@zhongzhijie1 Some contributors need to sign the CLA:
Please:
📋 View detailed check results: Action Run #21898142179 💡 Tip: All contributors must sign the CLA before the PR can be merged. |
| if (err && err != -EALREADY) { | ||
| BT_LOGE("%s, %s NOTIFY failed, err:%d", __func__, | ||
| enable ? "subscribe" : "unsubscribe", err); | ||
| if_gattc_on_element_subscribed(addr, element_id, BT_STATUS_FAIL, enable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BT_STATUS_FAIL -> GATT_STATUS_FAILURE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| } | ||
|
|
||
| if (err == -EALREADY) { | ||
| if_gattc_on_element_subscribed(addr, element_id, BT_STATUS_SUCCESS, enable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BT_STATUS_SUCCESS -> GATT_STATUS_SUCCESS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for other if_gattc_on_xxxxx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
@zhongzhijie1 please fix ci break |
3ae5674
58b1597 to
3ae5674
Compare
3ae5674 to
1d88ee6
Compare
7fe810a
1d88ee6 to
7fe810a
Compare
| memcpy(&addr.a, &req->addr, sizeof(addr.a)); | ||
| if (adapter_get_remote_identity_address(&req->addr, &id_addr) == BT_STATUS_SUCCESS) { | ||
| memcpy(&addr.a, &id_addr, sizeof(addr.a)); | ||
| addr.type = BT_LE_ADDR_TYPE_PUBLIC; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: identity address can be random.
Add TODO: or FIXME:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Fixes BLE iOS ANCS/GATTC address translation so upper layers receive notifications correctly, and adjusts LE whitelist + GATT subscribe handling to better align with identity/RPA behavior and non-fatal return codes.
Changes:
- Use
get_le_addr_from_conn()in GATT client callbacks to report the resolved/identity address rather than the raw connection destination. - Treat
-EALREADYfrom subscribe/indicate registration as non-fatal and propagate a successful subscribe result to the upper layer. - Update LE whitelist add/remove to prefer identity addresses and correct the remove path to call the proper Zephyr API.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| service/stacks/zephyr/sal_gatt_client_interface.c | Updates GATT client callbacks to use resolved LE address + refines subscribe error handling. |
| service/stacks/zephyr/sal_adapter_le_interface.c | Adjusts whitelist add/remove to use identity address where possible and fixes remove API usage. |
Comments suppressed due to low confidence (1)
service/stacks/zephyr/sal_gatt_client_interface.c:939
- The return value of
get_le_addr_from_conn()is ignored. If address resolution fails, notifications/indications will be reported with an invalid address. Check the return value and stop iteration on failure.
get_le_addr_from_conn(conn, &addr);
handle = params->value_handle;
if (data == NULL) {
BT_LOGE("[UNSUBSCRIBED] 0x%04X", params->value_handle);
return BT_GATT_ITER_STOP;
}
if_gattc_on_element_changed(&addr, handle, (uint8_t*)data, length);
return BT_GATT_ITER_CONTINUE;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| get_le_addr_from_conn(conn, &addr); | ||
| if_gattc_on_mtu_changed(&addr, att_payload, BT_STATUS_SUCCESS); | ||
| if_gattc_on_mtu_changed(&addr, att_payload, GATT_STATUS_SUCCESS); | ||
| } |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return value of get_le_addr_from_conn() is ignored. If it fails, if_gattc_on_mtu_changed() will be called with an undefined address. Check the return value and return early on failure.
| memcpy(&addr.a, &req->addr, sizeof(addr.a)); | ||
| if (adapter_get_remote_identity_address(&req->addr, &id_addr) == BT_STATUS_SUCCESS) { | ||
| memcpy(&addr.a, &id_addr, sizeof(addr.a)); | ||
| addr.type = BT_LE_ADDR_TYPE_PUBLIC; |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When an identity address is found, this forces addr.type = BT_LE_ADDR_TYPE_PUBLIC. BLE identity addresses can be public or random static (the service layer comment in adapter_service.c notes identity addr type may be public or random). Use the stored remote address type (e.g., adapter_get_le_remote_address_type(&req->addr)) or otherwise track identity type instead of hardcoding PUBLIC.
| addr.type = BT_LE_ADDR_TYPE_PUBLIC; | |
| addr.type = adapter_get_le_remote_address_type(&id_addr); |
| memcpy(&addr.a, &req->addr, sizeof(addr.a)); | ||
| if (adapter_get_remote_identity_address(&req->addr, &id_addr) == BT_STATUS_SUCCESS) { | ||
| memcpy(&addr.a, &id_addr, sizeof(addr.a)); | ||
| addr.type = BT_LE_ADDR_TYPE_PUBLIC; |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue as add: identity address type is hardcoded to BT_LE_ADDR_TYPE_PUBLIC, but identity addr type may be public or random. Use the device's stored address type (or track identity type) so bt_le_filter_accept_list_remove() uses the correct type.
| addr.type = BT_LE_ADDR_TYPE_PUBLIC; | |
| addr.type = adapter_get_le_remote_address_type(&id_addr); |
| get_le_addr_from_conn(conn, &addr); | ||
|
|
||
| if (err) { | ||
| BT_LOGE("%s, gatt read fail err:%d", __func__, err); | ||
| if_gattc_on_element_read(&addr, params->single.handle, (uint8_t*)data, length, BT_STATUS_FAIL); | ||
| if_gattc_on_element_read(&addr, params->single.handle, (uint8_t*)data, length, GATT_STATUS_FAILURE); | ||
| return BT_GATT_ITER_STOP; |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return value of get_le_addr_from_conn() is ignored. If it fails, addr may be uninitialized and the upper-layer callback will receive a garbage address. Check for BT_STATUS_SUCCESS and stop/return early (and optionally log) before calling if_gattc_on_element_read().
| get_le_addr_from_conn(conn, &addr); | ||
|
|
||
| if (err) { | ||
| BT_LOGE("%s, exchange MTU failed err: %u", __func__, err); | ||
| if_gattc_on_mtu_changed(&addr, 0, BT_STATUS_FAIL); | ||
| if_gattc_on_mtu_changed(&addr, 0, GATT_STATUS_FAILURE); | ||
| return; |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return value of get_le_addr_from_conn() is ignored. If address lookup fails, if_gattc_on_mtu_changed() will be invoked with an invalid address. Check for BT_STATUS_SUCCESS and return early on failure.
| addr.type = BT_LE_ADDR_TYPE_PUBLIC; | ||
| } else { | ||
| memcpy(&addr.a, &req->addr, sizeof(addr.a)); | ||
| addr.type = adapter_get_le_remote_address_type(&req->addr); |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the no-identity fallback, adapter_get_le_remote_address_type() can return BT_LE_ADDR_TYPE_UNKNOWN when the device isn't in the adapter device list. Passing UNKNOWN to bt_le_filter_accept_list_add() is likely invalid, and the PR description says UNKNOWN should be treated as PUBLIC. Add an explicit mapping for UNKNOWN -> BT_LE_ADDR_TYPE_PUBLIC here.
| addr.type = adapter_get_le_remote_address_type(&req->addr); | |
| addr.type = adapter_get_le_remote_address_type(&req->addr); | |
| if (addr.type == BT_LE_ADDR_TYPE_UNKNOWN) { | |
| addr.type = BT_LE_ADDR_TYPE_PUBLIC; | |
| } |
| memcpy(&addr.a, &req->addr, sizeof(addr.a)); | ||
| if (adapter_get_remote_identity_address(&req->addr, &id_addr) == BT_STATUS_SUCCESS) { | ||
| memcpy(&addr.a, &id_addr, sizeof(addr.a)); | ||
| addr.type = BT_LE_ADDR_TYPE_PUBLIC; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, add it
7fe810a to
e4eea5c
Compare
… handling. bug: v/83024 rootcause: This will result in the upper layer not receiving the GATTC notify. The `get_le_addr_from_conn` interface correctly retrieves the RPA address for the callback, not the identity address. The EALREADY return value indicates that ccc has already been subscribed to, so it is not an error. Signed-off-by: zhongzhijie1 <zhongzhijie1@xiaomi.com>
bug: v/85615 See adapter_service.c. When adding/removing from the whitelist, the unknown type is treated as public, and the random type is passed directly. The stack also needs a patch to convert random type addresses into identity addresses. Signed-off-by: zhongzhijie1 <zhongzhijie1@xiaomi.com>
b65504c
e4eea5c to
b65504c
Compare





fix address translation issue and improved subscribe API return value handling.
bug: v/83024
rootcause:
This will result in the upper layer not receiving the GATTC notify.
The
get_le_addr_from_conninterface correctly retrieves the RPA address for the callback,not the identity address.
The EALREADY return value indicates that ccc has already been subscribed to, so it is not an error.
Fix whitelist API typo, use the correct addr type rules.
bug: v/85615
See adapter_service.c. When adding/removing from the whitelist,
the unknown type now is treated as public, and the random type is passed directly.
So the addr_type is rely on service device list, call adapter_get_le_remote_address_type retrive it.
The stack also needs a patch to convert random type addresses into identity addresses.
Signed-off-by: zhongzhijie1 zhongzhijie1@xiaomi.com