Hi Silviu,
On Thu, May 23, 2024 at 6:46 AM Silviu Florian Barbulescu
<silviu.barbulescu@xxxxxxx> wrote:
>
> Hi Luiz,
>
> > -----Original Message-----
> > From: Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>
> > Sent: Monday, May 20, 2024 7:40 PM
> > To: Silviu Florian Barbulescu <silviu.barbulescu@xxxxxxx>
> > Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Mihai-Octavian Urzica <mihai-
> > octavian.urzica@xxxxxxx>; Vlad Pruteanu <vlad.pruteanu@xxxxxxx>; Andrei
> > Istodorescu <andrei.istodorescu@xxxxxxx>; Iulia Tanasescu
> > <iulia.tanasescu@xxxxxxx>
> > Subject: [EXT] Re: [PATCH BlueZ 1/2] shared/bap: Update BAP Broadcast Source
> > state machine
> >
> > Caution: This is an external email. Please take care when clicking links or
> > opening attachments. When in doubt, report the message using the 'Report this
> > email' button
> >
> >
> > Hi Silviu,
> >
> > On Mon, May 20, 2024 at 12:28 PM Silviu Florian Barbulescu
> > <silviu.barbulescu@xxxxxxx> wrote:
> > >
> > > Update BAP Broadcast Source state machine states to use BAP define
> > > states for source Idle, Config, Streaming, and an intermediary state enabling.
> >
> > Not really following, what is an intermediary state? Or do you mean internal
> > state? And more importantly why would we need an internal state like that?
> >
>
> Hi Luiz we use the ENABLING state to know when a transport linked to a stream
> has been acquired by a process and in the case of a BIG with one BIS stream goes
> in the ENABLING state waiting for the response from the kernel that the BIG has
> been created so it can go to the streaming state.
> For the case of a BIG with multiple BISes, the BIG is created when all BISes
> are acquired. So we use the ENABLING state to verify that all transports
> attached to that streams form BIG have been acquired so we can create the BIG.
> If we don’t use this internal state we can use a flag in the stream structure
> to be informed that the transport attached to the stream is in the acquiring
> procedure, but this would not look good, as we will have a flag intercalated
> with the stream state machine.
Fair enough, please add this information as a comment so in the future
we know why we are using enabling for broadcast.
> > > Updated test-bap too.
> > >
> > > ---
> > > src/shared/bap.c | 39 ++++++++++++++++++++-------------------
> > > unit/test-bap.c | 4 ++--
> > > 2 files changed, 22 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/src/shared/bap.c b/src/shared/bap.c index
> > > 6572ef1d1..639149520 100644
> > > --- a/src/shared/bap.c
> > > +++ b/src/shared/bap.c
> > > @@ -1361,14 +1361,6 @@ static void ep_config_cb(struct bt_bap_stream
> > *stream, int err)
> > > if (err)
> > > return;
> > >
> > > - if (bt_bap_stream_get_type(stream) == BT_BAP_STREAM_TYPE_BCAST)
> > {
> > > - if (bt_bap_stream_io_dir(stream) == BT_BAP_BCAST_SINK)
> > > - stream_set_state(stream, BT_BAP_STREAM_STATE_QOS);
> > > - else if (bt_bap_stream_io_dir(stream) == BT_BAP_BCAST_SOURCE)
> > > - stream_set_state(stream, BT_BAP_STREAM_STATE_CONFIG);
> > > - return;
> > > - }
> > > -
> > > stream_set_state(stream, BT_BAP_STREAM_STATE_CONFIG); }
> > >
> > > @@ -1759,6 +1751,15 @@ static unsigned int bap_stream_metadata(struct
> > bt_bap_stream *stream,
> > > return req->id;
> > > }
> > >
> > > +static unsigned int bap_bcast_qos(struct bt_bap_stream *stream,
> > > + struct bt_bap_qos *data,
> > > + bt_bap_stream_func_t func,
> > > + void *user_data) {
> > > + stream->qos = *data;
> > > + return 1;
> > > +}
> > > +
> > > static unsigned int bap_bcast_config(struct bt_bap_stream *stream,
> > > struct bt_bap_qos *qos, struct iovec *data,
> > > bt_bap_stream_func_t func, void
> > > *user_data) @@ -2071,7 +2072,7 @@ static unsigned int
> > bap_bcast_get_state(struct bt_bap_stream *stream)
> > > return stream->state;
> > > }
> > >
> > > -static unsigned int bap_bcast_enable(struct bt_bap_stream *stream,
> > > +static unsigned int bap_bcast_sink_enable(struct bt_bap_stream
> > > +*stream,
> > > bool enable_links, struct iovec *data,
> > > bt_bap_stream_func_t func,
> > > void *user_data) @@ -2081,22
> > > +2082,21 @@ static unsigned int bap_bcast_enable(struct bt_bap_stream
> > *stream,
> > > return 1;
> > > }
> > >
> > > -static unsigned int bap_bcast_start(struct bt_bap_stream *stream,
> > > +static unsigned int bap_bcast_src_enable(struct bt_bap_stream *stream,
> > > + bool enable_links, struct
> > > +iovec *data,
> > > bt_bap_stream_func_t func,
> > > void *user_data) {
> > > - stream_set_state(stream, BT_BAP_STREAM_STATE_STREAMING);
> > > + stream_set_state(stream, BT_BAP_STREAM_STATE_ENABLING);
> > >
> > > return 1;
> > > }
> > >
> > > -static unsigned int bap_bcast_sink_disable(struct bt_bap_stream *stream,
> > > - bool disable_links,
> > > +static unsigned int bap_bcast_start(struct bt_bap_stream *stream,
> > > bt_bap_stream_func_t func,
> > > void *user_data)
> > > {
> > > - bap_stream_io_detach(stream);
> > > - stream_set_state(stream, BT_BAP_STREAM_STATE_CONFIG);
> > > + stream_set_state(stream, BT_BAP_STREAM_STATE_STREAMING);
> > >
> > > return 1;
> > > }
> > > @@ -2106,7 +2106,8 @@ static unsigned int bap_bcast_disable(struct
> > bt_bap_stream *stream,
> > > bt_bap_stream_func_t func,
> > > void *user_data)
> > > {
> > > - stream_set_state(stream, BT_BAP_STREAM_STATE_DISABLING);
> > > + bap_stream_io_detach(stream);
> > > + stream_set_state(stream, BT_BAP_STREAM_STATE_CONFIG);
> > >
> > > return 1;
> > > }
> > > @@ -2205,14 +2206,14 @@ static const struct bt_bap_stream_ops
> > stream_ops[] = {
> > > bap_ucast_release, bap_ucast_detach),
> > > STREAM_OPS(BT_BAP_BCAST_SINK, bap_bcast_set_state,
> > > bap_bcast_get_state,
> > > - bap_bcast_config, NULL, bap_bcast_enable,
> > > - bap_bcast_start, bap_bcast_sink_disable, NULL,
> > > + bap_bcast_config, NULL, bap_bcast_sink_enable,
> > > + bap_bcast_start, bap_bcast_disable, NULL,
> > > bap_bcast_metadata, bap_bcast_sink_get_dir,
> > > bap_bcast_get_location,
> > > bap_bcast_release, bap_bcast_sink_detach),
> > > STREAM_OPS(BT_BAP_BCAST_SOURCE, bap_bcast_set_state,
> > > bap_bcast_get_state,
> > > - bap_bcast_config, NULL, bap_bcast_enable,
> > > + bap_bcast_config, bap_bcast_qos, bap_bcast_src_enable,
> > > bap_bcast_start, bap_bcast_disable, NULL,
> > > bap_bcast_metadata, bap_bcast_src_get_dir,
> > > bap_bcast_get_location,
> > > diff --git a/unit/test-bap.c b/unit/test-bap.c
> > > index 46ee0e4e5..10f9e348c 100644
> > > --- a/unit/test-bap.c
> > > +++ b/unit/test-bap.c
> > > @@ -547,10 +547,10 @@ static void bsrc_state(struct bt_bap_stream
> > *stream, uint8_t old_state,
> > > struct test_data *data = user_data;
> > >
> > > switch (new_state) {
> > > - case BT_BAP_STREAM_STATE_QOS:
> > > + case BT_BAP_STREAM_STATE_CONFIG:
> > > bt_bap_stream_enable(stream, true, NULL, NULL, NULL);
> > > break;
> > > - case BT_BAP_STREAM_STATE_CONFIG:
> > > + case BT_BAP_STREAM_STATE_ENABLING:
> > > data->base = bt_bap_stream_get_base(stream);
> > >
> > > g_assert(data->base);
> > > --
> > > 2.40.1
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz
>
--
Luiz Augusto von Dentz
