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