Hi Luiz,
ma, 2025-03-17 kello 14:55 -0400, Luiz Augusto von Dentz kirjoitti:
> Hi Pauli,
>
> On Sat, Mar 1, 2025 at 11:27 AM Pauli Virtanen <pav@xxxxxx> wrote:
> >
> > la, 2025-03-01 kello 17:57 +0200, Pauli Virtanen kirjoitti:
> > > Upper layer as Unicast Client needs to be able to destroy streams when
> > > it wants to reconfigure endpoints.
> > >
> > > This does not currently work right, because of Server issued
> > > Releasing->Config (caching) state transitions, which currently cause
> > > streams never enter Idle (so they are never destroyed).
> > >
> > > Fix this by considering Releasing->Config as Releasing->Idle->Config.
> > > Also do not make new streams from cached config data as Unicast Client,
> > > and leave all stream configuration to upper layer.
>
> Not sure I follow you here, how can we consider it idle even for
> cached config data? If we don't keep the stream there won't be a
> MediaTransport representing it either so it won't even be possible to
> know from the upper layer there is something already configured, so I
> really think we should have an option to reconfigure at MediaTransport
> layer rather than trying to hide it somehow.
The idea is that in Client role, the upper layer does not care whether
an ASE is configured or idle. It has to (re)configure it before it
wants to use it. The cost of this is that you are sometimes doing some
Config Codec that wouldn't be strictly needed, but it is harmless.
The problem here was that that linking of CIS gets broken by the
configured streams. That said, now I see it might be addressed just by
clearing the stream QoS and linking state when Releasing (QoS is then
invalid anyway so it needs to be cleared).
> > This change also implies the following, so that reconfiguring multi-ASE
> > configurations works right:
> >
> > shared/bap: ucast client streams always use a free ASE
> >
> > Because upper layer controls Unicast Client streams, bt_bap_stream_new()
> > should for unicast always allocate an unused ASE.
> >
> > diff --git a/src/shared/bap.c b/src/shared/bap.c
> > index 4f44db07a..a85169009 100644
> > --- a/src/shared/bap.c
> > +++ b/src/shared/bap.c
> > @@ -5836,29 +5836,6 @@ static bool find_ep_unused(const void *data, const void *user_data)
> > return true;
> > }
> >
> > -static bool find_ep_pacs(const void *data, const void *user_data)
> > -{
> > - const struct bt_bap_endpoint *ep = data;
> > - const struct match_pac *match = user_data;
> > -
> > - if (!ep->stream)
> > - return false;
> > -
> > - if (ep->stream->lpac != match->lpac)
> > - return false;
> > -
> > - if (ep->stream->rpac != match->rpac)
> > - return false;
> > -
> > - switch (ep->state) {
> > - case BT_BAP_STREAM_STATE_CONFIG:
> > - case BT_BAP_STREAM_STATE_QOS:
> > - return true;
> > - }
> > -
> > - return false;
> > -}
> > -
> > static bool find_ep_source(const void *data, const void *user_data)
> > {
> > const struct bt_bap_endpoint *ep = data;
> > @@ -6053,15 +6030,11 @@ static struct bt_bap_stream *bap_ucast_stream_new(struct bt_bap *bap,
> > match.lpac = lpac;
> > match.rpac = rpac;
> >
> > - /* Check for existing stream */
> > - ep = queue_find(bap->remote_eps, find_ep_pacs, &match);
> > + /* Find unused ASE */
> > + ep = queue_find(bap->remote_eps, find_ep_unused, &match);
>
> And if there aren't any ASE left then what? I'd go with the design the
> spec suggests that stream can be reconfigured for QoS Configuration
> state down to Idle, the only hard part is figuring out if a stream
> state affects another, etc.
>
> > if (!ep) {
> > - /* Check for unused ASE */
> > - ep = queue_find(bap->remote_eps, find_ep_unused, &match);
> > - if (!ep) {
> > - DBG(bap, "Unable to find unused ASE");
> > - return NULL;
> > - }
> > + DBG(bap, "Unable to find unused ASE");
> > + return NULL;
> > }
> >
> > stream = ep->stream;
> >
> >
> >
> > > ---
> > > src/shared/bap.c | 32 ++++++++++++++++++++++++++++++--
> > > 1 file changed, 30 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/shared/bap.c b/src/shared/bap.c
> > > index 54c6e8629..4f44db07a 100644
> > > --- a/src/shared/bap.c
> > > +++ b/src/shared/bap.c
> > > @@ -1363,6 +1363,31 @@ static void bap_stream_state_changed(struct bt_bap_stream *stream)
> > > struct bt_bap *bap = stream->bap;
> > > const struct queue_entry *entry;
> > >
> > > + switch (stream->ep->old_state) {
> > > + case BT_ASCS_ASE_STATE_RELEASING:
> > > + /* After Releasing, Server may either transition to Config or
> > > + * Idle. Our Unicast Client streams shall be considered
> > > + * destroyed after Releasing, so that upper layer can control
> > > + * stream creation. Make the lifecycle management simpler by
> > > + * making sure the streams are destroyed by always emitting Idle
> > > + * to upper layer after Releasing, even if the remote ASE did
> > > + * not go through that state.
> > > + */
> > > + if (stream->client &&
> > > + stream->ep->state != BT_ASCS_ASE_STATE_IDLE &&
> > > + (stream->lpac->type & (BT_BAP_SINK |
> > > + BT_BAP_SOURCE))) {
> > > + struct bt_bap_endpoint *ep = stream->ep;
> > > + uint8_t state = ep->state;
> > > +
> > > + ep->state = BT_ASCS_ASE_STATE_IDLE;
> > > + bap_stream_state_changed(stream);
> > > + ep->state = state;
> > > + return;
> > > + }
> > > + break;
> > > + }
> > > +
> > > /* Pre notification updates */
> > > switch (stream->ep->state) {
> > > case BT_ASCS_ASE_STATE_IDLE:
> > > @@ -4851,7 +4876,8 @@ static void ep_status_config(struct bt_bap *bap, struct bt_bap_endpoint *ep,
> > > }
> > >
> > > /* Any previously applied codec configuration may be cached by the
> > > - * server.
> > > + * server. However, all Unicast Client stream creation shall be left to
> > > + * the upper layer.
> > > */
> > > if (!ep->stream) {
> > > struct match_pac match;
> > > @@ -4866,7 +4892,9 @@ static void ep_status_config(struct bt_bap *bap, struct bt_bap_endpoint *ep,
> > > if (!match.lpac || !match.rpac)
> > > return;
> > >
> > > - bap_stream_new(bap, ep, match.lpac, match.rpac, NULL, true);
> > > + if (!(match.lpac->type & (BT_BAP_SINK | BT_BAP_SOURCE)))
> > > + bap_stream_new(bap, ep, match.lpac, match.rpac,
> > > + NULL, true);
> > > }
> > >
> > > if (!ep->stream)
> >
> >
>
>
--
Pauli Virtanen
