Auracast#546
Conversation
5b5cbc1 to
0ba2c42
Compare
There was a problem hiding this comment.
Pull request overview
Adds Auracast Sink support (including LE Periodic Advertising Sync + BIG Sync) across the stack: Zephyr SAL, service layer, IPC/socket transport, framework APIs (sync + async), and bt_tools/feature integration.
Changes:
- Introduce PA Sync service + Zephyr SAL periodic advertising sync interface and related scan result enhancements (SID/interval/tx power/flags).
- Add Auracast Sink role end-to-end: Zephyr BIG sync SAL, service/profile state machine + IPC commands/callbacks + framework APIs.
- Extend tools and feature async bindings to expose Auracast sink operations (CLI + JIDL).
Reviewed changes
Copilot reviewed 67 out of 67 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/utils.h | Adds BTTOOL_STRCAT helper macro for bounded string appends. |
| tools/bt_tools.h | Declares auracast sink CLI command entrypoints (sync + async). |
| tools/bt_tools.c | Registers auracast sink command; adapts init/uninit logic for BLE-only adapter states. |
| tools/async/gap.c | Adds auracast sink async command; adapts init/uninit for BLE-only adapter states. |
| service/stacks/zephyr/sal_pa_sync_interface.c | New Zephyr SAL implementation for periodic advertising sync + events. |
| service/stacks/zephyr/sal_le_scan_interface.c | Updates scan receive path to use Zephyr scan callbacks and populate extended fields. |
| service/stacks/zephyr/sal_auracast_sink_interface.c | New Zephyr SAL for Auracast sink BIG sync and ISO data delivery. |
| service/stacks/include/sal_pa_sync_interface.h | New SAL header defining PA sync options/params and public SAL APIs. |
| service/stacks/include/sal_auracast_sink_interface.h | New SAL header defining Auracast sink BIG sync APIs/params. |
| service/src/scan_manager.h | Updates scan result adv_data pointer type to const uint8_t*. |
| service/src/scan_manager.c | Updates scan result handling; flags periodic advertising based on interval. |
| service/src/pa_sync_service.h | New service header for PA sync service and SAL→service callbacks. |
| service/src/pa_sync_service.c | New PA sync service: lifecycle, message dispatch, caching, auracast readiness detection. |
| service/src/pa_sync_event.h | New PA sync internal event/message definitions. |
| service/src/btservice.c | Registers auracast sink profile service when enabled. |
| service/src/adapter_service.c | Initializes/cleans up pa_sync service on LE enable/disable. |
| service/profiles/service_manager.h | Adds <stddef.h> include (size_t dependency). |
| service/profiles/include/auracast_sink_service.h | New auracast sink service interface + SAL callbacks for BIG/ISO data. |
| service/profiles/auracast/sink/auracast_sink_state_machine.h | New auracast sink state machine public API. |
| service/profiles/auracast/sink/auracast_sink_state_machine.c | Implements auracast sink state machine (sync lifecycle + data path placeholder). |
| service/profiles/auracast/sink/auracast_sink_service.c | New profile service: device tracking, callback fanout, startup/shutdown, message loop. |
| service/profiles/auracast/sink/auracast_sink_event.h | New message/event structs for auracast sink internal messaging. |
| service/profiles/auracast/sink/auracast_sink_event.c | Implements auracast sink message allocation/free helpers. |
| service/ipc/socket/src/bt_socket_server.c | Routes PA sync and auracast sink IPC code ranges to new handlers. |
| service/ipc/socket/src/bt_socket_scan.c | Adjusts scan result callback subcodes and dispatch for enum stability. |
| service/ipc/socket/src/bt_socket_pa_sync.c | New socket IPC server/client callbacks for PA sync service. |
| service/ipc/socket/src/bt_socket_client.c | Registers PA sync + auracast sink callbacks; widens async message code type to uint32_t. |
| service/ipc/socket/src/bt_socket_auracast_sink.c | New socket IPC server/client callbacks for auracast sink profile. |
| service/ipc/socket/include/bt_socket.h | Declares PA sync + auracast sink socket APIs; extends async send_with_reply signature. |
| service/ipc/socket/include/bt_message_scan.h | Adds stable scan callback IPC codes/subcodes and defines BT_LE_ON_SCAN_RESULT IPC macro. |
| service/ipc/socket/include/bt_message_pa_sync.h | New IPC codes and payload structs for PA sync commands/callbacks. |
| service/ipc/socket/include/bt_message_auracast_sink.h | New IPC codes and payload structs for auracast sink commands/callbacks. |
| service/ipc/socket/include/bt_message.h | Wires PA sync + auracast sink payloads/results into the unified message packet. |
| service/ipc/socket/include/bt_ipc_code.h | Adds IPC groups for PA sync and auracast profiles. |
| framework/socket/bt_pa_sync.c | New framework socket client API for PA sync create/terminate. |
| framework/socket/bt_auracast_sink.c | New framework socket client API for auracast sink callbacks and sync control. |
| framework/socket/async/bt_pa_sync_async.c | New async framework APIs for PA sync create/terminate. |
| framework/socket/async/bt_le_scan_async.c | Adjusts scan async reply behavior and callback invocation. |
| framework/socket/async/bt_le_advertiser_async.c | Fixes async callback macro usage typo. |
| framework/socket/async/bt_gattc_async.c | Fixes async callback macro usage; updates handle assignment type. |
| framework/socket/async/bt_device_async.c | Fixes async callback macro usage typo. |
| framework/socket/async/bt_auracast_sink_async.c | New async framework APIs for auracast sink callbacks and sync control. |
| framework/socket/async/bt_adapter_async.c | Fixes async callback macro usage typo and removes duplicate struct definition. |
| framework/include/bt_uuid.h | Adds UUIDs for (Broadcast) Audio Announcement services. |
| framework/include/bt_utils.h | Adds popcount helpers (bt_utils_count_ones). |
| framework/include/bt_profile.h | Adds auracast source/sink profile IDs and names. |
| framework/include/bt_pa_sync.h | New public framework API for PA sync + parsing helpers + async APIs. |
| framework/include/bt_le_scan.h | Extends scan result struct with SID/interval/tx power/flags; adds constants. |
| framework/include/bt_auracast_sink.h | New public framework API for auracast sink + parsing helpers + async APIs. |
| framework/include/bt_audio_numbers.h | New assigned-number constants and helpers for codec/context/location decoding. |
| framework/include/bt_async.h | Renames async callback macro and adds shared register-callback types. |
| framework/include/bt_adapter.h | Removes duplicate bt_register_callback_cb_t typedef (now in bt_async.h). |
| framework/include/bluetooth.h | Adds BT_POWER_UNAVAILABLE, broadcast constants, and auracast sink callback storage. |
| framework/include/advertiser_data.h | Adds generic advertiser_data_parse() API + parse callback typedef. |
| framework/common/bt_audio_numbers.c | Implements stringification helpers for codec/context/location (log-gated). |
| framework/common/advertiser_data_helper.c | Implements advertiser_data_parse() and PA sync / auracast advertising parsing. |
| framework/api/bt_pa_sync.c | Adds non-socket framework API passthrough to service for PA sync. |
| framework/api/bt_auracast_sink.c | Adds non-socket framework API passthrough to auracast sink profile interface. |
| feature/feature_async/src/feature_bluetooth_util.c | Adds aurasnk list initialization/cleanup to feature async context. |
| feature/feature_async/jidl/bluetooth_ble.jidl | Adds AuracastSink interface + data structs and callbacks to JIDL. |
| feature/feature_async/include/feature_bluetooth.h | Adds auracast sink feature structs/state and includes bt_auracast_sink.h. |
| Makefile | Adds build wiring for PA sync + auracast sink (service/SAL/tools/framework/socket/api). |
| Kconfig | Adds CONFIG_BLUETOOTH_PA_SYNC and CONFIG_BLUETOOTH_AURACAST_SINK options. |
| CMakeLists.txt | Adds build wiring for PA sync + auracast sink (mirrors Makefile). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| bt_list_remove(g_auracast_sink_service.sink_list, device); | ||
|
|
||
| if (g_auracast_sink_service.terminating) | ||
| auracast_sink_shutdown(g_auracast_sink_service.terminating); | ||
| } |
There was a problem hiding this comment.
auracast_sink_service_notify_sync_terminated() calls auracast_sink_shutdown() when a shutdown is pending. This enqueues a new SHUTDOWN message each time a device terminates; with multiple devices it can resend terminate requests and potentially invoke the shutdown callback multiple times. Consider instead checking whether the sink_list is empty after removal and, if so, complete the pending shutdown once (without re-enqueueing another shutdown request).
There was a problem hiding this comment.
This is safe by design. All messages are processed serially in the service loop. When notify_sync_terminated calls auracast_sink_shutdown(cb), it only enqueues a new SHUTDOWN message. When service_shutdown processes it, it checks bt_list_length(sink_list) > 0 — if devices remain, it re-sends terminate (idempotent); if the list is empty, it performs cleanup and calls cb once. After cleanup, sink_list is set to NULL and terminating is cleared, so subsequent SHUTDOWN messages are no-ops. No fix needed.
| scan = (bt_scan_remote_t*)data->scan; | ||
| if (!packet->scan_r.remote) { | ||
| status = BT_STATUS_FAIL; | ||
| goto error; | ||
| } | ||
|
|
||
| scan->remote = packet->scan_r.remote; | ||
| ret_cb(ins, packet->scan_r.status, data->scan, data->userdata); | ||
| ret_cb(ins, BT_STATUS_SUCCESS, data->scan, data->userdata); | ||
|
|
There was a problem hiding this comment.
le_start_scan_reply() always calls the user callback with BT_STATUS_SUCCESS when packet->scan_r.remote is non-null, ignoring packet->scan_r.status. This can report success to callers even when the server returned an error. Consider passing packet->scan_r.status through to ret_cb (and only treating it as success when it is BT_STATUS_SUCCESS).
There was a problem hiding this comment.
Intentional. At this point, packet->scan_r.remote is confirmed non-null, meaning the scanner object was successfully created at the IPC level. The actual scan start status is delivered asynchronously via the on_scan_start_status callback. Passing BT_STATUS_SUCCESS here correctly indicates that scanner creation (not scan start) succeeded. No fix needed.
| static void STACK_CALL(stop_scan)(void* args) | ||
| { | ||
| bt_le_scan_cb_unregister(&scan_cbs); | ||
| SAL_CHECK(bt_le_scan_stop(), 0); | ||
| } |
There was a problem hiding this comment.
In stop_scan(), the scan callback is unregistered before bt_le_scan_stop() is attempted. If bt_le_scan_stop() fails, scanning may continue but results will no longer be delivered to scan_recv_cb. Consider unregistering the callback only after bt_le_scan_stop() succeeds (or re-registering on failure).
There was a problem hiding this comment.
Good catch. Fixed — reordered to call bt_le_scan_stop() first, then bt_le_scan_cb_unregister(). This ensures the callback remains registered to handle any final events if stop fails.
| result_info.dev_type = BT_DEVICE_DEVTYPE_BLE; | ||
| result_info.addr_type = addr->type; | ||
| memcpy(&result_info.addr, &addr->a, sizeof(result_info.addr)); | ||
|
|
||
| scan_on_result_data_update(&result_info, eir.value); | ||
| result_info.adv_type = parse_adv_type(info); | ||
| result_info.length = ad->len; | ||
| result_info.rssi = info->rssi; |
There was a problem hiding this comment.
scan_recv_cb assigns result_info.length = ad->len, but ble_scan_result_t.length is a uint8_t. If Zephyr delivers extended advertising data >255 bytes, this will truncate silently and may break downstream AD parsing. Consider clamping/rejecting lengths > UINT8_MAX (and/or updating the public struct to carry a 16-bit length if extended payloads are intended).
There was a problem hiding this comment.
Per BLE spec, a single advertising data payload is at most 254 bytes (extended advertising), which fits within uint8_t. Zephyr's net_buf_simple.len is uint16_t for generality, but in the scan receive path it never exceeds 254. No fix needed.
| err = bt_le_per_adv_sync_delete(device->sync); | ||
| if (err) { | ||
| BT_LOGE("failed to terminate sync, err = %d", err); | ||
| bt_list_remove(g_sal_pa_sync_info->sync_list, device); | ||
| return; |
There was a problem hiding this comment.
If bt_le_per_adv_sync_delete() fails, the code removes the device from sync_list anyway, which triggers sync_removed()->device_delete()->pa_sync_on_terminated(). That reports termination even though the delete operation failed and the sync may still be active. Consider keeping the device in the list on delete failure (and report an error via a dedicated callback/status), or retry/handle the failure explicitly.
There was a problem hiding this comment.
Intentional. If bt_le_per_adv_sync_delete() fails, we still remove the device to prevent a stale entry from blocking subsequent operations (shutdown, re-sync). Keeping a failed-to-delete device in the list would be worse — it could cause the service to hang during shutdown waiting for a termination that never comes. Force-cleanup is the safer strategy. No fix needed.
| err = bt_iso_big_terminate(device->sink); | ||
| if (err) { | ||
| BT_LOGE("failed to terminate sync, err = %d", err); | ||
| bt_list_remove(g_sal_auracast_sink_info->sink_list, device); | ||
| return; |
There was a problem hiding this comment.
If bt_iso_big_terminate() fails, the code removes the device from sink_list anyway, which triggers sink_removed()->device_delete()->auracast_sink_on_terminated(). That reports termination even though termination failed and the BIG may still be active. Consider not removing from the list on failure; instead propagate the error and keep state consistent (or retry/force cleanup only when termination is confirmed).
There was a problem hiding this comment.
Same rationale as the PA sync case above. If bt_iso_big_terminate() fails, force-removing the device prevents residual state from interfering with other operations (shutdown, new sync creation, etc.). A lingering device entry with a failed BIG terminate would be a worse outcome than a clean removal with a terminated notification. No fix needed.
565ac82 to
c430220
Compare
chengkai15
left a comment
There was a problem hiding this comment.
PR #546 Review — Auracast (LE Audio Broadcast Sink)
规模: 67 文件,9817+/60−,全新 feature
🔴 阻断级问题
1. PR 描述为空
9817 行新增代码,body 为空。必须补充:架构设计、测试方案、已知限制、回退路径。
2. 代码中残留 FIXME/TODO
sal_auracast_sink_interface.c:68— FIXME: 不理解 QoS 参数用途auracast_sink_state_machine.c— TODO: derive_mse 未检查 iso interval、derive_sync_timeout 硬编码- FIXME/TODO 必须在合并前解决或转为 issue 跟踪
3. PR 规模过大
67 文件应拆分 PA sync / Auracast profile / tools / async 封装,分别评审。
⚠️ 改进建议
4. broadcast_code 缺少 IPC 边界校验(High)
IPC 层传入的 broadcast_code 应校验长度 == 16 字节。
5. PA sync 全局单例无线程保护(Medium)
g_pa_sync_info 无 mutex,多线程场景有竞态风险。
6. Releasing 状态无超时保护(Medium)
Stack 不回调 SYNC_TERMINATED 会卡死,建议加 releasing timeout。
7. iso_recv 无丢帧统计(Low)
建议加 rx_dropped 计数器。
📋 结论
架构合理(PA sync + sink + SAL 分层),但必须补充描述、清理 FIXME/TODO、拆分 PR。REQUEST_CHANGES
e5b83c0 to
b6c93c5
Compare
expliyh
left a comment
There was a problem hiding this comment.
Reviewed all 6 Copilot comments. 1 issue accepted and fixed (stop_scan ordering), 5 dismissed with rationale.
|
感谢逐条回复,force-cleanup 策略的理由可以接受。 但我评审中的阻断级问题仍未解决: 1. PR 描述为空 — 9817 行代码 body 为空字符串,这是合并阻断项。请补充:架构设计、测试方案、已知限制。 2. FIXME/TODO 残留 — 请更新 PR 描述并清理 FIXME/TODO,否则我无法 Approve。 |
Check commit message instead of PR description TODO and FIXME are by design and will be implemented in the future. |
zhongzhijie1
left a comment
There was a problem hiding this comment.
LGTM. Clean layered architecture for Auracast Sink: PA Sync → Auracast Sink profile → SAL → Feature/IPC/Tools. Proper Kconfig gating and error handling throughout.
fa71575 to
85f1818
Compare
1d87b94 to
7a1af25
Compare
| return msg; | ||
| } | ||
|
|
||
| void auracast_sink_msg_destory(auracast_sink_msg_t* msg) |
2b93bc5 to
4cf3538
Compare
ac7190f to
fd9fafd
Compare
The merge-base changed after approval.
bug: v/82275 Rootcause: Add LE Periodic Advertising (PA) sync functionality including: - PA sync service with create/terminate/transfer operations - PA sync report callback for receiving periodic advertising data - SAL PA sync interface with zephyr stack adaptation - Framework API, IPC socket layer, and async API support - Advertiser data parser helper (advertiser_data_parse) - Kconfig options: BLUETOOTH_PA_SYNC, BLUETOOTH_HCI_FILTER, BLUETOOTH_CONNECTION_MANAGER, BLUETOOTH_GATTS_CACHE_SUPPORT, BLUETOOTH_LOG_PRIVACY, BLUETOOTH_HFP_AG_LOCAL_TELEPHONY, ZBLUE_A2DP_SBC_MAX_BIT_POOL, ZBLUE_A2DP_SOURCE_BUF_SIZE - Build system integration (CMakeLists.txt, Makefile) Signed-off-by: liyuheng <liyuheng@xiaomi.com>
bug: v/82275 Implement Auracast Broadcast Sink profile across the full Bluetooth stack, enabling reception and playback of Auracast broadcast audio streams. Framework layer: - Public API (bt_auracast_sink.h) with LC3 codec config, BIS/subgroup info structures, and broadcast audio metadata definitions - Async socket wrappers (bt_auracast_sink_async.c) for non-blocking calls - Socket IPC transport (bt_socket_auracast_sink.c, bt_message_auracast_sink.h) - Audio codec number definitions and lookup tables (bt_audio_numbers.h/c) Service layer: - Auracast sink service with device lifecycle management, supporting multiple concurrent broadcast sources (auracast_sink_service.c) - State machine with Idle/Enabling/Streaming/Releasing states and BIS synchronization tracking (auracast_sink_state_machine.c) - Event definitions for stack-to-service callbacks (auracast_sink_event.h) Stack Adaptation Layer (SAL): - Zephyr-based BIG sync create/terminate and BIS data path setup (sal_auracast_sink_interface.c) Signed-off-by: Zihao Gao <gaozihao@xiaomi.com>
bug: v/82275 Add bttool CLI commands for Auracast Sink testing and debugging, registered under the "aurasnk" command group in both sync and async modes. Sync mode (tools/auracast_sink.c): - PA scan start/stop with nearby broadcast source discovery and aging - PA sync create/terminate with BASE info reporting - BIG sync (startReceive/stopReceive) with broadcast code support - Subgroup and BIS index selection via interactive commands Async mode (tools/async/auracast_sink.c): - Mirrors sync mode commands using the async API wrappers - Adds pending pointer sentinel (AURACAST_SINK_PTR_PENDING) for tracking in-flight async operations Common changes: - Adapt bt_tools.c adapter state callbacks to support BLE-only builds (BT_ADAPTER_STATE_BLE_ON/OFF) when CONFIG_BLUETOOTH_BREDR_SUPPORT is disabled - Register aurasnk command group in bt_tools.h and build system Signed-off-by: Zihao Gao <gaozihao@xiaomi.com>
No description provided.