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. > > > >> + 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. > 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. > > >> 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
