Hi Andrei,
On Thu, Feb 8, 2024 at 6:25 AM Andrei Istodorescu
<andrei.istodorescu@xxxxxxx> wrote:
>
> Hi Luiz,
>
> > -----Original Message-----
> > From: Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>
> > Sent: Wednesday, February 7, 2024 4:42 PM
> > To: Andrei Istodorescu <andrei.istodorescu@xxxxxxx>
> > Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Mihai-Octavian Urzica <mihai-
> > octavian.urzica@xxxxxxx>; Silviu Florian Barbulescu
> > <silviu.barbulescu@xxxxxxx>; Vlad Pruteanu <vlad.pruteanu@xxxxxxx>;
> > Iulia Tanasescu <iulia.tanasescu@xxxxxxx>
> > Subject: [EXT] Re: [PATCH BlueZ 3/3] bap: Do PA Sync for each BAP Broadcast
> > source discovered
> >
> > 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 Andrei,
> >
> > On Wed, Feb 7, 2024 at 7:23 AM Andrei Istodorescu
> > <andrei.istodorescu@xxxxxxx> wrote:
> > >
> > > After discovering a BAP Broadcast Source do a short PA sync first to
> > > learn the BASE. After discovering the BASE check how many BISes are
> > > matching the sink capabilities and create endpoints for them. Allow
> > > user to configure one endpoint using "SetConfiguration" causing BIG
> > > synchronization to the corresponding BIS; also this results in
> > > creating a stream and the corresponding transport.
> > > ---
> > > profiles/audio/bap.c | 324
> > > +++++++++++++++----------------------------
> > > 1 file changed, 110 insertions(+), 214 deletions(-)
> > >
> > > diff --git a/profiles/audio/bap.c b/profiles/audio/bap.c index
> > > 88c93127bea0..8646eae2ed20 100644
> > > --- a/profiles/audio/bap.c
> > > +++ b/profiles/audio/bap.c
> > > @@ -105,6 +105,44 @@ struct bap_data {
> > > void *user_data;
> > > };
> > >
> > > +/* Structure holding the parameters for periodic train and BIG
> > > + * synchronization
> > > + */
> > > +static struct bt_iso_qos bap_sink_sync_parameters = {
> > > + .bcast = {
> > > + .big = BT_ISO_QOS_BIG_UNSET,
> > > + .bis = BT_ISO_QOS_BIS_UNSET,
> > > + /* HCI_LE_Periodic_Advertising_Create_Sync */
> > > + .options = 0x00,
> > > + .skip = 0x0000,
> > > + .sync_timeout = 0x4000,
> > > + .sync_cte_type = 0x00,
> > > + /* HCI_LE_BIG_Create_Sync */
> > > + .encryption = 0x00,
> > > + .bcode = {0x00},
> > > + .mse = 0x00,
> > > + .timeout = 0x4000,
> > > + /* to remove from kernel check */
> > > + .sync_factor = 0x07,
> > > + .packing = 0x00,
> > > + .framing = 0x00,
> > > + .in = {
> > > + .interval = 10000,
> > > + .latency = 10,
> > > + .sdu = 40,
> > > + .phy = 0x02,
> > > + .rtn = 2,
> > > + },
> > > + .out = {
> > > + .interval = 10000,
> > > + .latency = 10,
> > > + .sdu = 40,
> > > + .phy = 0x02,
> > > + .rtn = 2,
> > > + }
> > > + }
> > > +};
> >
> > This cannot be global, it needs to be stored on a per device basis so it doesn't
> > get overwritten.
>
> I will submit an update for this.
>
> >
> > > static struct queue *sessions;
> > >
> > > static bool bap_data_set_user_data(struct bap_data *data, void
> > > *user_data) @@ -422,113 +460,6 @@ static int
> > parse_array(DBusMessageIter *iter, struct iovec *iov)
> > > return 0;
> > > }
> > >
> > > -static bool parse_base(void *data, size_t len, util_debug_func_t func,
> > > - uint32_t *presDelay, uint8_t *numSubgroups, uint8_t *numBis,
> > > - struct bt_bap_codec *codec, struct iovec **caps,
> > > - struct iovec **meta)
> > > -{
> > > - struct iovec iov = {
> > > - .iov_base = data,
> > > - .iov_len = len,
> > > - };
> > > -
> > > - uint8_t capsLen, metaLen;
> > > - struct iovec cc;
> > > - struct iovec metadata;
> > > -
> > > - if (presDelay) {
> > > - if (!util_iov_pull_le24(&iov, presDelay))
> > > - return false;
> > > - util_debug(func, NULL, "PresentationDelay %d", *presDelay);
> > > - }
> > > -
> > > - if (numSubgroups) {
> > > - if (!util_iov_pull_u8(&iov, numSubgroups))
> > > - return false;
> > > - util_debug(func, NULL, "NumSubgroups %d", *numSubgroups);
> > > - }
> > > -
> > > - if (numBis) {
> > > - if (!util_iov_pull_u8(&iov, numBis))
> > > - return false;
> > > - util_debug(func, NULL, "NumBis %d", *numBis);
> > > - }
> > > -
> > > - if (codec) {
> > > - codec = util_iov_pull_mem(&iov, sizeof(*codec));
> > > - if (!codec)
> > > - return false;
> > > - util_debug(func, NULL, "%s: ID %d CID 0x%2.2x VID 0x%2.2x",
> > > - "Codec", codec->id, codec->cid, codec->vid);
> > > - }
> > > -
> > > - if (!util_iov_pull_u8(&iov, &capsLen))
> > > - return false;
> > > - util_debug(func, NULL, "CC Len %d", capsLen);
> > > -
> > > - if (!capsLen)
> > > - return false;
> > > -
> > > - cc.iov_len = capsLen;
> > > - cc.iov_base = util_iov_pull_mem(&iov, capsLen);
> > > - if (!cc.iov_base)
> > > - return false;
> > > -
> > > - if (caps) {
> > > - if (*caps)
> > > - util_iov_free(*caps, 1);
> > > -
> > > - *caps = util_iov_dup(&cc, 1);
> > > - }
> > > -
> > > - for (int i = 0; capsLen > 1; i++) {
> > > - struct bt_ltv *ltv = util_iov_pull_mem(&cc, sizeof(*ltv));
> > > - uint8_t *caps;
> > > -
> > > - if (!ltv) {
> > > - util_debug(func, NULL, "Unable to parse %s",
> > > - "Capabilities");
> > > - return false;
> > > - }
> > > -
> > > - util_debug(func, NULL, "%s #%u: len %u type %u",
> > > - "CC", i, ltv->len, ltv->type);
> > > -
> > > - caps = util_iov_pull_mem(&cc, ltv->len - 1);
> > > - if (!caps) {
> > > - util_debug(func, NULL, "Unable to parse %s",
> > > - "CC");
> > > - return false;
> > > - }
> > > - util_hexdump(' ', caps, ltv->len - 1, func, NULL);
> > > -
> > > - capsLen -= (ltv->len + 1);
> > > - }
> > > -
> > > - if (!util_iov_pull_u8(&iov, &metaLen))
> > > - return false;
> > > - util_debug(func, NULL, "Metadata Len %d", metaLen);
> > > -
> > > - if (!metaLen)
> > > - return false;
> > > -
> > > - metadata.iov_len = metaLen;
> > > - metadata.iov_base = util_iov_pull_mem(&iov, metaLen);
> > > - if (!metadata.iov_base)
> > > - return false;
> > > -
> > > - if (meta) {
> > > - if (*meta)
> > > - util_iov_free(*meta, 1);
> > > -
> > > - *meta = util_iov_dup(&metadata, 1);
> > > - }
> > > -
> > > - util_hexdump(' ', metadata.iov_base, metaLen, func, NULL);
> > > -
> > > - return true;
> > > -}
> > > -
> > > static int parse_io_qos(const char *key, int var, DBusMessageIter *iter,
> > > struct bt_bap_io_qos *qos) { @@
> > > -954,6 +885,17 @@ static DBusMessage
> > *set_configuration(DBusConnection *conn, DBusMessage *msg,
> > > return btd_error_invalid_args(msg);
> > > }
> > >
> > > + /* For BAP Broadcast Sink, the capabilities and metadata are coming
> > > + * from the source's BIS, which are present in the remote PAC
> > > + */
> > > + if (bt_bap_pac_get_type(ep->lpac) == BT_BAP_BCAST_SINK) {
> > > + util_iov_free(setup->caps, 1);
> > > + setup->caps = util_iov_dup(bt_bap_pac_get_data(ep->rpac), 1);
> > > + util_iov_free(setup->metadata, 1);
> > > + setup->metadata = util_iov_dup(
> > > + bt_bap_pac_get_metadata(ep->rpac), 1);
> > > + }
> > > +
> > > setup->stream = bt_bap_stream_new(ep->data->bap, ep->lpac, ep-
> > >rpac,
> > > &setup->qos,
> > > setup->caps);
> > >
> > > @@ -977,95 +919,27 @@ static DBusMessage
> > *set_configuration(DBusConnection *conn, DBusMessage *msg,
> > > break;
> > > case BT_BAP_STREAM_TYPE_BCAST:
> > > /* No message sent over the air for broadcast */
> > > - if (bt_bap_pac_get_type(ep->lpac) == BT_BAP_BCAST_SINK)
> > > - setup->msg = dbus_message_ref(msg);
> > > - else {
> > > + if (bt_bap_pac_get_type(ep->lpac) ==
> > > + BT_BAP_BCAST_SOURCE)
> > > setup->base = bt_bap_stream_get_base(setup->stream);
> > > - setup->id = 0;
> > > }
> > > + setup->id = 0;
> > >
> > > if (ep->data->service)
> > > service_set_connecting(ep->data->service);
> > >
> > > return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
> > > - }
> > >
> > > return NULL;
> > > }
> > >
> > > -static void update_bcast_qos(struct bt_iso_qos *qos,
> > > - struct bt_bap_qos *bap_qos)
> > > -{
> > > - bap_qos->bcast.big = qos->bcast.big;
> > > - bap_qos->bcast.bis = qos->bcast.bis;
> > > - bap_qos->bcast.sync_factor = qos->bcast.sync_factor;
> > > - bap_qos->bcast.packing = qos->bcast.packing;
> > > - bap_qos->bcast.framing = qos->bcast.framing;
> > > - bap_qos->bcast.encryption = qos->bcast.encryption;
> > > - bap_qos->bcast.options = qos->bcast.options;
> > > - bap_qos->bcast.skip = qos->bcast.skip;
> > > - bap_qos->bcast.sync_timeout = qos->bcast.sync_timeout;
> > > - bap_qos->bcast.sync_cte_type = qos->bcast.sync_cte_type;
> > > - bap_qos->bcast.mse = qos->bcast.mse;
> > > - bap_qos->bcast.timeout = qos->bcast.timeout;
> > > - bap_qos->bcast.io_qos.interval = qos->bcast.in.interval;
> > > - bap_qos->bcast.io_qos.latency = qos->bcast.in.latency;
> > > - bap_qos->bcast.io_qos.phy = qos->bcast.in.phy;
> > > - bap_qos->bcast.io_qos.sdu = qos->bcast.in.sdu;
> > > - bap_qos->bcast.io_qos.rtn = qos->bcast.in.rtn;
> > > - if (!bap_qos->bcast.bcode)
> > > - bap_qos->bcast.bcode = new0(struct iovec, 1);
> > > - util_iov_memcpy(bap_qos->bcast.bcode, qos->bcast.bcode,
> > > - sizeof(qos->bcast.bcode));
> > > -}
> > > -
> > > static void iso_bcast_confirm_cb(GIOChannel *io, GError *err, void
> > > *user_data) {
> > > struct bap_setup *setup = user_data;
> > > - struct bap_ep *ep = setup->ep;
> > > - struct bap_data *data = ep->data;
> > > - struct bt_iso_qos qos;
> > > - struct bt_iso_base base;
> > > - char address[18];
> > > int fd;
> > > - struct iovec *base_io;
> > > - uint32_t presDelay;
> > > - uint8_t numSubgroups;
> > > - uint8_t numBis;
> > > - struct bt_bap_codec codec;
> > > -
> > > - bt_io_get(io, &err,
> > > - BT_IO_OPT_DEST, address,
> > > - BT_IO_OPT_QOS, &qos,
> > > - BT_IO_OPT_BASE, &base,
> > > - BT_IO_OPT_INVALID);
> > > - if (err) {
> > > - error("%s", err->message);
> > > - g_error_free(err);
> > > - goto drop;
> > > - }
> > >
> > > - g_io_channel_ref(io);
> > > - btd_service_connecting_complete(data->service, 0);
> > > - DBG("BCAST ISO: sync with %s (BIG 0x%02x BIS 0x%02x)",
> > > - address, qos.bcast.big, qos.bcast.bis);
> > > -
> > > - update_bcast_qos(&qos, &setup->qos);
> > > -
> > > - base_io = new0(struct iovec, 1);
> > > - util_iov_memcpy(base_io, base.base, base.base_len);
> > > -
> > > - parse_base(base_io->iov_base, base_io->iov_len, bap_debug,
> > > - &presDelay, &numSubgroups, &numBis,
> > > - &codec, &setup->caps, &setup->metadata);
> > > -
> > > - /* Update pac with BASE information */
> > > - bt_bap_update_bcast_source(ep->rpac, &codec, setup->caps,
> > > - setup->metadata);
> > > - setup->id = bt_bap_stream_config(setup->stream, &setup->qos,
> > > - setup->caps, NULL, NULL);
> > > -
> > > - bt_bap_stream_set_user_data(setup->stream, ep->path);
> > > + /* listen channel is not needed anymore */
> > > + g_io_channel_unref(setup->io);
> > > + setup->io = NULL;
> > >
> > > fd = g_io_channel_unix_get_fd(io);
> > >
> > > @@ -1074,26 +948,43 @@ static void iso_bcast_confirm_cb(GIOChannel
> > *io, GError *err, void *user_data)
> > > g_io_channel_set_close_on_unref(io, FALSE);
> > > return;
> > > }
> > > -
> > > -
> > > - return;
> > > -
> > > -drop:
> > > - g_io_channel_shutdown(io, TRUE, NULL);
> > > -
> > > }
> > >
> > > static void iso_pa_sync_confirm_cb(GIOChannel *io, void *user_data)
> > > {
> > > GError *err = NULL;
> > > + struct bap_data *data = user_data;
> > > + struct bt_iso_base base;
> > > + struct bt_bap_base base_s;
> > > + struct bt_iso_qos qos;
> > >
> > > - if (!bt_io_bcast_accept(io, iso_bcast_confirm_cb,
> > > - user_data, NULL, &err, BT_IO_OPT_INVALID)) {
> > > - error("bt_io_bcast_accept: %s", err->message);
> > > + btd_service_connecting_complete(data->service, 0);
> > > +
> > > + bt_io_get(io, &err,
> > > + BT_IO_OPT_BASE, &base,
> > > + BT_IO_OPT_QOS, &qos,
> > > + BT_IO_OPT_INVALID);
> > > + if (err) {
> > > + error("%s", err->message);
> > > g_error_free(err);
> > > g_io_channel_shutdown(io, TRUE, NULL);
> > > + return;
> > > }
> > >
> > > + /* The PA Sync channel becomes the new listen_io.
> > > + * It will be later used to listen for a BIS io.
> > > + */
> > > + g_io_channel_unref(data->listen_io);
> > > + data->listen_io = io;
> > > + g_io_channel_ref(io);
> > > +
> > > + /* Analyze received BASE data and create remote media endpoints for
> > each
> > > + * matching BIS
> > > + */
> > > + base_s.subgroups = queue_new();
> > > + bt_bap_parse_base(data->bap, base.base, base.base_len,
> > bap_debug,
> > > + &base_s);
> > > + queue_foreach(base_s.subgroups, bt_bap_parse_bis, NULL);
> > > }
> > >
> > > static bool match_data_bap_data(const void *data, const void
> > > *match_data) @@ -1934,12 +1825,11 @@ static void setup_listen_io(struct
> > bap_data *data, struct bt_bap_stream *stream,
> > > data->listen_io = io;
> > > }
> > >
> > > -static void setup_listen_io_broadcast(struct bap_data *data,
> > > +static void setup_accept_io_broadcast(struct bap_data *data,
> > > struct bap_setup *setup,
> > > struct bt_bap_stream *stream,
> > > struct bt_iso_qos *qos) {
> > > - GIOChannel *io;
> > > GError *err = NULL;
> > > struct sockaddr_iso_bc iso_bc_addr;
> > >
> > > @@ -1951,29 +1841,18 @@ static void setup_listen_io_broadcast(struct
> > > bap_data *data,
> > >
> > > DBG("stream %p", stream);
> > >
> > > - /* If IO already set skip creating it again */
> > > - if (bt_bap_stream_get_io(stream) || data->listen_io)
> > > - return;
> > > -
> > > - io = bt_io_listen(NULL, iso_pa_sync_confirm_cb, setup, NULL, &err,
> > > - BT_IO_OPT_SOURCE_BDADDR,
> > > - btd_adapter_get_address(data->adapter),
> > > - BT_IO_OPT_DEST_BDADDR,
> > > - device_get_address(data->device),
> > > - BT_IO_OPT_DEST_TYPE,
> > > - btd_device_get_bdaddr_type(data->device),
> > > - BT_IO_OPT_MODE, BT_IO_MODE_ISO,
> > > - BT_IO_OPT_QOS, &qos->bcast,
> > > - BT_IO_OPT_ISO_BC_NUM_BIS, iso_bc_addr.bc_num_bis,
> > > - BT_IO_OPT_ISO_BC_BIS, iso_bc_addr.bc_bis,
> > > - BT_IO_OPT_INVALID);
> > > - if (!io) {
> > > - error("%s", err->message);
> > > + if (!bt_io_bcast_accept(data->listen_io,
> > > + iso_bcast_confirm_cb,
> > > + setup, NULL, &err,
> > > + BT_IO_OPT_ISO_BC_NUM_BIS,
> > > + iso_bc_addr.bc_num_bis, BT_IO_OPT_ISO_BC_BIS,
> > > + iso_bc_addr.bc_bis, BT_IO_OPT_INVALID)) {
> > > + error("bt_io_bcast_accept: %s", err->message);
> > > g_error_free(err);
> > > }
> > > - setup->io = io;
> > > - data->listen_io = io;
> > >
> > > + setup->io = data->listen_io;
> > > + data->listen_io = NULL;
> > > }
> > > static void setup_create_ucast_io(struct bap_data *data,
> > > struct bap_setup *setup, @@
> > > -2037,7 +1916,7 @@ done:
> > > if (bt_bap_pac_get_type(setup->ep->lpac) ==
> > BT_BAP_BCAST_SOURCE)
> > > setup_connect_io_broadcast(data, setup, stream, &iso_qos);
> > > else
> > > - setup_listen_io_broadcast(data, setup, stream, &iso_qos);
> > > + setup_accept_io_broadcast(data, setup, stream,
> > > + &iso_qos);
> > > }
> > >
> > > static void setup_create_io(struct bap_data *data, struct bap_setup
> > > *setup, @@ -2422,6 +2301,7 @@ static int bap_bcast_probe(struct
> > btd_service *service)
> > > struct btd_gatt_database *database =
> > btd_adapter_get_database(adapter);
> > > struct bap_data *data = btd_service_get_user_data(service);
> > > char addr[18];
> > > + GError *err = NULL;
> > >
> > > ba2str(device_get_address(device), addr);
> > >
> > > @@ -2465,7 +2345,23 @@ static int bap_bcast_probe(struct btd_service
> > > *service)
> > >
> > > bt_bap_set_user_data(data->bap, service);
> > >
> > > - bt_bap_new_bcast_source(data->bap, device_get_path(device));
> > > + DBG("Create PA sync with this source");
> > > + data->listen_io = bt_io_listen(NULL, iso_pa_sync_confirm_cb, data,
> > > + NULL, &err,
> > > + BT_IO_OPT_SOURCE_BDADDR,
> > > + btd_adapter_get_address(data->adapter),
> > > + BT_IO_OPT_DEST_BDADDR,
> > > + device_get_address(data->device),
> > > + BT_IO_OPT_DEST_TYPE,
> > > + btd_device_get_bdaddr_type(data->device),
> > > + BT_IO_OPT_MODE, BT_IO_MODE_ISO,
> > > + BT_IO_OPT_QOS, &bap_sink_sync_parameters,
> > > + BT_IO_OPT_INVALID);
> > > + if (!data->listen_io) {
> > > + error("%s", err->message);
> > > + g_error_free(err);
> > > + }
> >
> > I really doubt this will work in a crowded environment, it seems we would be
> > doing several PA Sync in parallel, one for each announcement found, which
> > not only would overwrite the QOS but also I don't think controller are capable
> > of doing multiple PA Sync like that so we might need to serialize the process
> > of doing short lived PA Syncs to enumerate the BASE.
> >
> > Usually we have dealt with the serialization using an idle timer which can
> > then check services that need to be resolved, once a service is being
> > resolved then the timer shall be stopped, we restart the time everytime
> > something needs to be resolved.
> >
>
> bap_bcast_probe starts a PA Sync controller procedure for each new Broadcast
> source seen.
> This can take several milliseconds with physical controller and runs in an instant
> with the emulator. The problem that can arrive is that another Broadcast source
> gets probed and another bt_io_listen is executed before the previous one is
> completed. In this scenario the controller will return an error for no resources
> and the second PA sync will fail, but the one in progress will complete successfully.
> Given the timings for this to happen, can I submit a different patch for this
> issue, or shall I fix it in this one?
Yes, you can choose to fix it later by adding a TODO comment, that
said I'd fix the QoS being a static global.
> > > +
> > > return 0;
> > > }
> > >
> > > --
> > > 2.40.1
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz
--
Luiz Augusto von Dentz
