Bluetooth: Audio: race hazard bt_audio_discover() callback vs unicast_client_ase_cp_discover()

[Chris-StJohn-Bose]

bt_audio_discover() has a callback when an ASE is discovered on a remote device
unicast_client_ase_cp_discover() asynchronously subscribes to notifications of ASE state changes

There's no way of the application knowing when unicast_client_ase_cp_discover() has completed so that it is safe to call bt_audio_stream_config()

This causes occasional race hazards when stream configuration occurs before state changes are notified.

I'm working on a PR to resolve this, probably calling the discovery callback after notifications have been enabled.

Relevant lines in mainline:
https://github.com/zephyrproject-rtos/zephyr/blob/main/subsys/bluetooth/audio/unicast_client.c#L1882
https://github.com/zephyrproject-rtos/zephyr/blob/main/subsys/bluetooth/audio/unicast_client.c#L1799

[Chris-StJohn-Bose]

Example of that happens over-the-air

[Chris-StJohn-Bose]

Adding a cp_subscribe[index].subscribe callback and re-using bt_gatt_discover_params seems to work plus I got rid of cp_disc[]

[Thalley]

@Chris-StJohn-Bose please follow the bug reporting template when reporting bugs :)

In any case, this is actually a known issue (but haven't really been an issue reported before), and is actually fixed by my commit a5f435f in #53484.

Perhaps I should pull that commit out and see if I can get it through separately.

[Thalley]

Moved the commit to a separate PR to get it in ASAP: #54283

[Chris-StJohn-Bose]

Moved the commit to a separate PR to get it in ASAP: #54283

Yes that looks a lot like the fix I'm using. I'll stop preparing a PR and watch for #54283 merge. Thanks!

[Thalley]

Moved the commit to a separate PR to get it in ASAP: #54283

Yes that looks a lot like the fix I'm using. I'll stop preparing a PR and watch for #54283 merge. Thanks!

Feel free to add your review to #54283 :)

[Chris-StJohn-Bose]

Note sure about the priority:low as this is currently preventing streaming to a 3rd party receiver that doesn't automatically notify on CCC write, but it's being dealt with so the priority is fairly irrelevant.

[Thalley]

Note sure about the priority:low as this is currently preventing streaming to a 3rd party receiver that doesn't automatically notify on CCC write, but it's being dealt with so the priority is fairly irrelevant.

It's something that has been in the code for more than 2 years and haven't seen any reports about it until now, so arguably it isn't that big of an issue. Aside from that, it would be quite trivial for an application to add a small waiting period between the discover callback and the next procedure.

Finally, until #53484 is merged, none of the notifications does anything besides being logged