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. > >> + 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()? 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 >> 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. > >> - 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
