Hi Pauli,
On Mon, Aug 11, 2025 at 12:54 PM Pauli Virtanen <pav@xxxxxx> wrote:
>
> Hi Luiz,
>
> ma, 2025-08-11 kello 10:38 -0400, Luiz Augusto von Dentz kirjoitti:
> > Hi Pauli,
> >
> > On Sun, Aug 10, 2025 at 2:18 PM Pauli Virtanen <pav@xxxxxx> wrote:
> > >
> > > When detaching BAP session, the session is removed from the global list,
> > > and streams do not go through normal state change cleanup, so local
> > > endpoint states are not cleaned up. This causes problems as ASE may be
> > > in busy state even though there is no stream.
> >
> > Why wouldn't they go through the normal state change cleanup though,
> > the stream shall be set to idle and then affect the endpoint state as
> > well or we do have something preventing that to happen? Or the
> > local_eps are not used as stream->ep for some reason?
>
> What I mean above, is that design in bt_bap_detach() appears to be to
> not call the usual state change callbacks from there.
>
> bap is first removed from sessions list, so bt_bap_ref_safe(bap) ==
> NULL, so that stream_set_state() only calls ops->detach(), which does
> not change ep state.
Ok, that explains it, bu then perhaps the likes of bap_ucast_detach
shall be cleanup the ep state once it does ep->stream = NULL, that
said if it is an unexpected disconnect Id cache the codec
configuration so upon reconnection that should restore the stream
faster, if the stream has been released then it should be in idle
already.
> For remote EPs (iow as BAP Client), we will re-read their state when
> attaching the session next time, so state cleanup on detach is not
> necessary. (We also probably should not in general be updating remote
> EP states ourselves, but leave it to remote via GATT.)
>
> For local EPs, we don't reset state on detach or attach, so they stay
> what they were.
Ok, this is a bug, on disconnect we either should return it to codec
config (cache) or idle (non-cache).
> I think we (as BAP Server) are not supposed to persist the local ASE
> state over session detach, e.g. remote disappears and ATT disconnects,
> and then reconnects later.
>
> We could reset the endpoint state in stream_set_state() if there is no
> session, if that sounds better, instead of doing it in bt_bap_detach()
> like here.
>
> IIUC stream_set_state() is supposed to be called only for streams to
> local endpoints, but not 100% sure if it's so in practice (re:
> broadcast or stream_io_disconnected).
>
> > >
> > > Fix by resetting all ASE to IDLE state after detaching all streams.
> > > ---
> > > src/shared/bap.c | 9 +++++++++
> > > 1 file changed, 9 insertions(+)
> > >
> > > diff --git a/src/shared/bap.c b/src/shared/bap.c
> > > index ed5c322b4..d4beb9818 100644
> > > --- a/src/shared/bap.c
> > > +++ b/src/shared/bap.c
> > > @@ -5664,6 +5664,14 @@ static void stream_foreach_detach(void *data, void *user_data)
> > > stream_set_state(stream, BT_BAP_STREAM_STATE_IDLE);
> > > }
> > >
> > > +static void ep_foreach_detach(void *data, void *user_data)
> > > +{
> > > + struct bt_bap_endpoint *ep = data;
> > > +
> > > + ep->state = BT_ASCS_ASE_STATE_IDLE;
> > > + ep->old_state = BT_ASCS_ASE_STATE_IDLE;
> > > +}
> > > +
> > > static void bap_req_detach(void *data)
> > > {
> > > struct bt_bap_req *req = data;
> > > @@ -5696,6 +5704,7 @@ void bt_bap_detach(struct bt_bap *bap)
> > > bap->att = NULL;
> > >
> > > queue_foreach(bap->streams, stream_foreach_detach, bap);
> > > + queue_foreach(bap->local_eps, ep_foreach_detach, bap);
> >
> > This sounds more like a workaround though, the stream_foreach_detach
> > should have cleaned up all existing streams and their endpoints, we
> > could perhaps print a message if the ep->state is not idle then it
> > means something is not quite right.
> >
> > > queue_foreach(bap_cbs, bap_detached, bap);
> > > }
> > >
> > > --
> > > 2.50.1
> > >
> > >
> >
--
Luiz Augusto von Dentz
