Draft bap thread introduction. #97326
Conversation
This PR adds the initial base functionality of the broadcast sink. The functionality is test on a NRF5340 audio. Signed-off-by: Rasmus Moeller <[email protected]>
This PR adds the initial base functionality of the broadcast sink. The functionality is test on a NRF5340 audio. Signed-off-by: Rasmus Moeller <[email protected]>
Added bap_thread in broadcast_snk.c for feedback while working on it. Draft to be deleted once feedback is incorperated in the add_bass_to_hap_ha branch.
There was a problem hiding this comment.
I would consider renaming these enums. Since BAP is a keyword from the spec, it might come across as if there is a BAP_STATE in the spec, which there is not.
BAP is a spec that defines several profiles, among them you have both broadcast sink, which you refer to in this file - but also unicast server, which you address in bap_unicast_sr.c. With this in mind, I would also suggest to rename the function completely. bap thread might imply that this thread handles everything BAP, but it does not, it handles the Broadcast Sink specifically.
From what I currently can see, this thread is actually not being started from anywhere (yet?) ?
There was a problem hiding this comment.
Shouldn't the state be set to BAP_STATE_RESET here? The if-statement on line 850 checks whether we k_sem_take is a success, and if so we put state into BAP_STATE_PA_SYNC and break - so here I assume we have a failare and should reset?
There was a problem hiding this comment.
You are mixing how you check k_sem_take a bit. In some places you first assigned the return value to err, which you then check, while in others you check it directly. Also, sometimes you check whether its a success (== 0), and sometimes whether its a failure. For optimization lenient readability I would suggest to go through this whole function and try make it as uniform as possible, without jeopardizing logic ofc.
There was a problem hiding this comment.
Did you try this before converting to a thread? I'm just wondering if 10 second sleep at the end might be slightly annoying when running - basically means we have to wait 10 second every time we break - causing long delays in some transitions, right? Or am I wrong?
There was a problem hiding this comment.
10 milisec :)
Tho i do not mind removing it.
There was a problem hiding this comment.
Oh, my bad. Having a 10 ms wait does make sense though
There was a problem hiding this comment.
This function is also called "init_bap_sink" - and I know you didnt name this. But since this file now lives in a sample where both unicast and broadcast coexist, I would consider renaming it to better reflect that this refers to Broadcast, instead of relying on it being implied.
This might be a reoccurring "problem" here :)
Added bap_thread in broadcast_snk.c for feedback while working on it. Draft to be deleted once feedback is incorperated in the add_bass_to_hap_ha branch.
…o draft_bap_thread
…o draft_bap_thread
Minor edits of review.
This commit adds the suggestions changes to the code. This is still a draft that is waiting that needs internal review. Signed-off-by: Rasmus Moeller <[email protected]>
|
2 participants
Changes included: