Bluetooth: ascs: Handle ASE control point operations in separate thread #59775
Conversation
MariuszSkamra
added
area: Bluetooth Audio
DNM
MariuszSkamra
requested review from
jhedberg,
Vudentz,
Thalley and
sjanc
as code owners
zephyrbot
requested review from
asbjornsabo,
Casper-Bonde-Bose,
fredrikdanebjer,
kruithofa,
larsgk and
szymon-czapracki
|
@MariuszSkamra Before I dive too deep into the implementation of this, what is the advantage of using a new thread for this compared to the system workqueue thread? |
subsys/bluetooth/audio/ascs.c
Outdated
| @@ -2905,4 +2993,7 @@ void bt_ascs_cleanup(void) | |||
| unicast_server_cb = NULL; | |||
| } | |||
| } | |||
|
|
|||
| K_THREAD_DEFINE(ascs, CONFIG_BT_ASCS_STACK_SIZE, ascs_thread, NULL, NULL, NULL, | |||
There was a problem hiding this comment.
I am wondering if we can get this working without the need for a separate thread (thinking memory constraints)?
There was a problem hiding this comment.
I decided to make use of the System Workqueue thread
Being slightly cautious adding to the memory budget, I am wondering if this can be done without a separate thread. Also, do we have some real life tests/observations where this causes a clear issue (sans thread)? |
MariuszSkamra
force-pushed
the
ascs_notifications
branch
from
5a2902e
to
3a47272
Compare
MariuszSkamra
force-pushed
the
ascs_notifications
branch
from
3a47272
to
656221e
Compare
subsys/bluetooth/audio/ascs.c
Outdated
| err = k_work_submit(&ase->state_transition_work); | ||
| if (err < 0) { | ||
| return err; |
There was a problem hiding this comment.
Maybe we should log this?
If this indeed fails, then we can't really recover from it, can we?
There was a problem hiding this comment.
I'll add a log, but I'm not sure about how to recover if this one fails.
k_work_submit API defines below errors that might be returned:
* @retval -EBUSY
* * if work submission was rejected because the work item is cancelling; or
* * @p queue is draining; or
* * @p queue is plugged.
* @retval -EINVAL if @p queue is null and the work item has never been run.
* @retval -ENODEV if @p queue has not been started.
I think that in case of System Workqueue, EINVAL and ENODEV if returned indicate some failure in the system. Not sure about EBUSY
There was a problem hiding this comment.
Indeed. We can't really do much besides logging this as an LOG_ERR; I don't think we can do anything to recover from it
|
@MariuszSkamra please rebase |
The `ops` variable is unused, thus can be removed. Signed-off-by: Mariusz Skamra <[email protected]>
This adds handling of ASE control point operations in separate thread so that the notifications of ASE state changes are sent from non-BT thread. This ensures bt_gatt_notify_cb to be blocking waiting for available buffers to send the notifications. Signed-off-by: Mariusz Skamra <[email protected]>
MariuszSkamra
force-pushed
the
ascs_notifications
branch
from
656221e
to
3264836
Compare
This adds handling of ASE control point operations in separate thread
so that the notifications of ASE state changes are sent from non-BTRX
thread. This ensures bt_gatt_notify_cb to be blocking waiting for
available buffers to send the notifications.