Hi Luiz， > [ EXTERNAL EMAIL ] > > Hi Yang, > > On Tue, Jan 7, 2025 at 8:50 PM Yang Li <Yang.Li@xxxxxxxxxxx> wrote: >> Hi Luiz & Pauli, >> >> ASCS v1.0.1 §5.9 introduces two operation processes for the server to >> handle the released state: >> ---------------- >> If the server wants to cache a codec configuration: >> - Transition the ASE to the Codec Configured state and write a value of >> 0x01 (Codec Configured) to the ASE_State field >> - Write the configured values or the server’s preferred values for the >> Codec_ID, Codec_Specific_Configuration_Length, and >> Codec_Specific_Configuration parameters to the matching >> Additional_ASE_Parameters fields defined in Table 4.3 >> <https://www.bluetooth.com/wp-content/uploads/Files/Specification/HTML/23166-ASCS-html5/out/en/index-en.html#UUID-8e961f8e-98b7-23b8-53d7-59cc88762fc0_N1661460117035>. >> - Write the server’s preferred values for the remaining >> Additional_ASE_Parameters fields defined in Table 4.3 >> <https://www.bluetooth.com/wp-content/uploads/Files/Specification/HTML/23166-ASCS-html5/out/en/index-en.html#UUID-8e961f8e-98b7-23b8-53d7-59cc88762fc0_N1661460117035>. >> >> If the server does not want to cache a codec configuration: >> - Transition the ASE to the Idle state and write a value of 0x00 (Idle) >> to the ASE_State field. >> - Delete any Additional_ASE_Parameters fields present. >> ---------------- >> https://www.bluetooth.com/wp-content/uploads/Files/Specification/HTML/23166-ASCS-html5/out/en/index-en.html#UUID-9d5221b2-eadd-1bde-09d4-5b3bb9e6d7b8 >> Currently BlueZ only uses non-cached operation, It seems that the >> Android platform is not compatible with the non-cached Codec >> Configuration scenario, >> I raised an issue when testing playback and pause using a Pixel phone. >> https://github.com/bluez/bluez/issues/1053 >> So I tried modifying the code to use a cached Codec Configuration >> (referencing the process for Pixel + Redmi Buds 5Pro headphones). >> I believe there are two issues that need to be confirmed: >> - Does BlueZ need to support both operations and how to decide which one >> to use? (cached or non-cached); >> - Whether the Android platform will be compatible with non-cached in the >> future. >> >>> [ EXTERNAL EMAIL ] >>> >>> Hi Yang, >>> >>> On Sun, Jan 5, 2025 at 9:50 PM Yang Li via B4 Relay >>> <devnull+yang.li.amlogic.com@xxxxxxxxxx> wrote: >>>> From: Yang Li <yang.li@xxxxxxxxxxx> >>>> >>>> CIS sink need caching the Codec Configured when releasing by Pixel, >>>> state machine is releasing -> Codec. If streamming -> idle, CIS sink >>>> was silent after resume music. >>> You need to work on the commit message, perhaps quote the spec if >>> there is a description of the state transition. >> Well, I got it. >>>> Signed-off-by: Yang Li <yang.li@xxxxxxxxxxx> >>>> --- >>>> src/shared/bap.c | 43 +++++++++++++++++++++++++++++++------------ >>>> 1 file changed, 31 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/src/shared/bap.c b/src/shared/bap.c >>>> index 167501282..a7f5dec92 100644 >>>> --- a/src/shared/bap.c >>>> +++ b/src/shared/bap.c >>>> @@ -1063,6 +1063,28 @@ static void stream_notify_metadata(struct bt_bap_stream *stream) >>>> free(status); >>>> } >>>> >>>> +static void stream_notify_release(struct bt_bap_stream *stream) >>>> +{ >>>> + struct bt_bap_endpoint *ep = stream->ep; >>>> + struct bt_ascs_ase_status *status; >>>> + size_t len; >>>> + >>>> + DBG(stream->bap, "stream %p", stream); >>>> + >>>> + len = sizeof(*status); >>>> + status = malloc(len); >>> Just use a stack variable instead of using malloc. >> I will do it. >> >> I just referred to other function flows, like stream_notify_metadata() >> >>>> + >>>> + memset(status, 0, len); >>>> + status->id = ep->id; >>>> + ep->state = BT_BAP_STREAM_STATE_RELEASING; >>>> + status->state = ep->state; >>>> + >>>> + gatt_db_attribute_notify(ep->attr, (void *) status, len, >>>> + bt_bap_get_att(stream->bap)); >>>> + >>>> + free(status); >>>> +} >>>> + >>>> static struct bt_bap *bt_bap_ref_safe(struct bt_bap *bap) >>>> { >>>> if (!bap || !bap->ref_count || !queue_find(sessions, NULL, bap)) >>>> @@ -1634,7 +1656,7 @@ static bool stream_notify_state(void *data) >>>> struct bt_bap_stream *stream = data; >>>> struct bt_bap_endpoint *ep = stream->ep; >>>> >>>> - DBG(stream->bap, "stream %p", stream); >>>> + DBG(stream->bap, "stream %p status %d", stream, ep->state); >>>> >>>> if (stream->state_id) { >>>> timeout_remove(stream->state_id); >>>> @@ -1655,6 +1677,9 @@ static bool stream_notify_state(void *data) >>>> case BT_ASCS_ASE_STATE_DISABLING: >>>> stream_notify_metadata(stream); >>>> break; >>>> + case BT_ASCS_ASE_STATE_RELEASING: >>>> + stream_notify_release(stream); >>> Ok, I see where this is going, but the spec doesn't actually mandate >>> to send releasing or caching the codec configuration: >>> >>> https://www.bluetooth.com/wp-content/uploads/Files/Specification/HTML/23166-ASCS-html5/out/en/index-en.html#UUID-c37600a3-4541-1926-2f13-eb29057e41d5_N1661459513418 >>> >>> Perhaps you are saying that we need to send Releasing at least? There >>> is also the thing that I don't understand is why would someone send >>> release command and get rid of QoS/CIG while keeping the Codec >>> Configuration? >> I think the client requires the server to notify the current state as >> released. > There is no released state, only idle, that said the spec clearly > state that we need to transition to Releasing (0x06) before moving to > Idle (0x00), which is something we are not currently doing, anyway we > need to properly document this and perhaps we need to add a timer in > case the remote doesn't disconnect the ISO link we should probably > cleanup ourselves other we would be stuck in releasing state forever. Well，I understand. >>>> + break; >>>> } >>>> >>>> return false; >>>> @@ -1936,9 +1961,7 @@ static uint8_t stream_disable(struct bt_bap_stream *stream, struct iovec *rsp) >>>> /* Sink can autonomously transit to QOS while source needs to go to >>>> * Disabling until BT_ASCS_STOP is received. >>>> */ >>>> - if (stream->ep->dir == BT_BAP_SINK) >>>> - stream_set_state(stream, BT_BAP_STREAM_STATE_QOS); >>>> - else >>>> + if (stream->ep->dir == BT_BAP_SOURCE) >>> Don't think this is correct, why are you taking away the setting to >>> QoS like it is documented? >> This is also what I am confused about. Why do we need to set state=QoS >> in stream_disable()? > Disable is different from the Release command, it is used to 'pause' > the stream but keep the QoS settings, but the server still needs to > transition back to QoS, and no I'm not guessing here the state machine > does really require these state transitions. I got it. > >> From the ASE state machine, the ASE state should stay in Codec >> Configured after Released, and switch to QoS state when Client >> configures QoS. >> >> https://www.bluetooth.com/wp-content/uploads/Files/Specification/HTML/23166-ASCS-html5/out/en/index-en.html#UUID-363eeef6-abc5-6f54-cfa6-09fe4451fd15 > Sounds like you are confusing Disable with Release again here, over > each line connecting the states there is the operations, transition > from Releasing to Codec Configuration is clearly _not_ mandatory, in > fact I think for embedded devices they most like want to save memory > when a stream is released, so Id only really recommend caching the > Codec Configuration in case the connection is lost or something like > that, not when the remote peer intentionally release. Yes, I agree. I retested the original code and analyzed logcat, and found that its MediaController had an exception. I have submitted the issue on the issuetracker. It seems that Android needs to solve the idle problem head-on. https://issuetracker.google.com/issues/388956587 >>>> stream_set_state(stream, BT_BAP_STREAM_STATE_DISABLING); >>>> >>>> return 0; >>>> @@ -2068,17 +2091,11 @@ static unsigned int bap_ucast_metadata(struct bt_bap_stream *stream, >>>> >>>> static uint8_t stream_release(struct bt_bap_stream *stream, struct iovec *rsp) >>>> { >>>> - struct bt_bap_pac *pac; >>>> - >>>> DBG(stream->bap, "stream %p", stream); >>>> >>>> ascs_ase_rsp_success(rsp, stream->ep->id); >>>> >>>> - pac = stream->lpac; >>>> - if (pac->ops && pac->ops->clear) >>>> - pac->ops->clear(stream, pac->user_data); >>> Hmm, I think we do depend on clear to be called to tell the upper >>> stack the transport is being released, or you did test this with the >>> likes of pipewire and found that is not really required? >> If clear is executed, CIS will be disconnected automatically. I compared >> it with the pixel+Buds5 headphones, and it was the mobile phone that >> disconnected the CIS, so in the cached Codec Configuration operation >> process, there is no need to disconnect the CIS. > Fair enough, that said I don't think this behavior is invalid either, > and like I pointed above we should probably trigger a timer if the > remote doesn't cleanup the CIS on releasing. > >>>> - stream_set_state(stream, BT_BAP_STREAM_STATE_IDLE); >>>> + stream_set_state(stream, BT_BAP_STREAM_STATE_RELEASING); >>>> >>>> return 0; >>>> } >>>> @@ -6172,8 +6189,10 @@ static bool stream_io_disconnected(struct io *io, void *user_data) >>>> >>>> DBG(stream->bap, "stream %p io disconnected", stream); >>>> >>>> - bt_bap_stream_set_io(stream, -1); >>>> + if (stream->ep->state == BT_ASCS_ASE_STATE_RELEASING) >>>> + stream_set_state(stream, BT_BAP_STREAM_STATE_CONFIG); >>>> >>>> + bt_bap_stream_set_io(stream, -1); >>>> return false; >>>> } >>>> >>>> >>>> --- >>>> base-commit: dfb1ffdc95a00bc06d81a75c11ab5ad2e24d37bf >>>> change-id: 20250106-upstream-1ec9ae96cda4 >>>> >>>> Best regards, >>>> -- >>>> Yang Li <yang.li@xxxxxxxxxxx> >>>> >>>> >>>> >>> -- >>> Luiz Augusto von Dentz >> > > -- > Luiz Augusto von Dentz
