Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-2754-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240325144031.335354-1-vlad.pruteanu@nxp.com> <20240325144031.335354-2-vlad.pruteanu@nxp.com>
In-Reply-To: <20240325144031.335354-2-vlad.pruteanu@nxp.com>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Mon, 25 Mar 2024 12:03:56 -0400
Message-ID: <CABBYNZLHb3dRVQ5KxNCorOp2cJWjAwknCU9--19UScFgfMJ7bA@mail.gmail.com>
Subject: Re: [PATCH BlueZ 1/1] client/player: Fix transport.send command's
 transfer of packets
To: Vlad Pruteanu <vlad.pruteanu@nxp.com>
Cc: linux-bluetooth@vger.kernel.org, mihai-octavian.urzica@nxp.com, 
	silviu.barbulescu@nxp.com, iulia.tanasescu@nxp.com, 
	andrei.istodorescu@nxp.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Vlad,

On Mon, Mar 25, 2024 at 10:40=E2=80=AFAM Vlad Pruteanu <vlad.pruteanu@nxp.c=
om> wrote:
>
> The transport.send command sends a number num of packets at intervals
> specified by the transport latency reported by the CIS Establsihed event.
> Num is defined as qos.ucast.out.latency * 1000 / qos.ucast.out.interval.
> Since this latency could be smaller than the SDU interval for some preset=
s,
> the resulting num would be 0, causing the file transfer to stop after the
> first packet. Instead, one packet should be sent at SDU interval distance
> apart.

Please add some output samples showing the wrong interval ends up being use=
d.

> ---
>  client/player.c | 34 +++++++++++++++++++++++++++-------
>  1 file changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/client/player.c b/client/player.c
> index 1f56bfd27..f579d9904 100644
> --- a/client/player.c
> +++ b/client/player.c
> @@ -34,6 +34,7 @@
>
>  #include "lib/bluetooth.h"
>  #include "lib/uuid.h"
> +#include "lib/iso.h"
>
>  #include "profiles/audio/a2dp-codecs.h"
>  #include "src/shared/lc3.h"
> @@ -5007,7 +5008,6 @@ static bool transport_timer_read(struct io *io, voi=
d *user_data)
>         struct bt_iso_qos qos;
>         socklen_t len;
>         int ret, fd;
> -       uint32_t num;
>         uint64_t exp;
>
>         if (transport->fd < 0)
> @@ -5031,10 +5031,7 @@ static bool transport_timer_read(struct io *io, vo=
id *user_data)
>                 return false;
>         }
>
> -       /* num of packets =3D latency (ms) / interval (us) */
> -       num =3D (qos.ucast.out.latency * 1000 / qos.ucast.out.interval);
> -
> -       ret =3D transport_send_seq(transport, transport->fd, num);
> +       ret =3D transport_send_seq(transport, transport->fd, 1);
>         if (ret < 0) {
>                 bt_shell_printf("Unable to send: %s (%d)\n",
>                                         strerror(-ret), ret);
> @@ -5052,6 +5049,8 @@ static bool transport_timer_read(struct io *io, voi=
d *user_data)
>  static int transport_send(struct transport *transport, int fd,
>                                         struct bt_iso_qos *qos)
>  {
> +       struct sockaddr_iso addr;
> +       socklen_t optlen;
>         struct itimerspec ts;
>         int timer_fd;
>
> @@ -5068,9 +5067,30 @@ static int transport_send(struct transport *transp=
ort, int fd,
>                 return -errno;
>
>         memset(&ts, 0, sizeof(ts));
> -       ts.it_value.tv_nsec =3D qos->ucast.out.latency * 1000000;
> -       ts.it_interval.tv_nsec =3D qos->ucast.out.latency * 1000000;
>
> +       /* Need to know if the transport on which data is sent is
> +        * broadcast or unicast so that the correct qos structure
> +        * can be accessed. At this point in code there no other
> +        * way of knowing this besides checking the peer address.
> +        * Broadcast will use BDADDR_ANY, while Unicast will use
> +        * the connected peer's actual address.
> +        */
> +       memset(&addr, 0, sizeof(addr));
> +       optlen =3D sizeof(addr);
> +
> +       if (getpeername(transport->sk, &addr, &optlen) < 0)
> +               return -errno;

The description seems to be suggesting there is a rounding error, but
the code below only deals with the fact we didn't use proper fields
for broadcast, so is it really fixing what is the patch description or
something else?

> +       if (!(bacmp(&addr.iso_bdaddr, BDADDR_ANY))) {
> +               /* Interval is measured in us, multiply by 1000 to get ns=
 */
> +               ts.it_value.tv_nsec =3D qos->bcast.out.interval * 1000;
> +               ts.it_interval.tv_nsec =3D qos->bcast.out.interval * 1000=
;
> +       } else {
> +               /* Interval is measured in us, multiply by 1000 to get ns=
 */
> +               ts.it_value.tv_nsec =3D qos->ucast.out.interval * 1000;
> +               ts.it_interval.tv_nsec =3D qos->ucast.out.interval * 1000=
;
> +
> +       }
>         if (timerfd_settime(timer_fd, TFD_TIMER_ABSTIME, &ts, NULL) < 0)
>                 return -errno;
>
> --
> 2.39.2
>


--=20
Luiz Augusto von Dentz