[PATCH BlueZ v2 0/1] client/player: Fix transport.send command's transfer of packets

[PATCH BlueZ v2 0/1] client/player: Fix transport.send command's transfer of packets

[Vlad Pruteanu]

The transport.send command sends a number num of packets at intervals of
transport latency.

Num is defined as qos.ucast.out.latency * 1000 / qos.ucast.out.interval.

Since this latency could be smaller than the SDU interval, the resulting
num could be 0, causing the file transfer to stop after the first packet.
In this case num will be set to 1 so that at least 1 packet is always sent.

It the transport send timer is set to a value smaller than that of SDU
interval, the available buffers for ISO Data will eventually become full.
Thus, if a packet can't be sent due to resource temporarily being
unavailable decrease the fd offset so that next time the same packet will
be sent.

This patch is a temporary fix until the appropriate solution that uses
number of completed packets to control the flow of ISO Data packets is
implemented.

Since both Unicast and Broadcast scenarios use the same transport functions
differentiate between the 2 cases when accessing the qos structures to get
the transport latency.

Vlad Pruteanu (1):
  client/player: Fix transport.send command's transfer of packets

 client/player.c | 55 +++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 49 insertions(+), 6 deletions(-)

-- 
2.40.1

[PATCH BlueZ v2 1/1] client/player: Fix transport.send command's transfer of packets

[Vlad Pruteanu]

The transport.send command sends a number num of packets at intervals of
transport latency.

Num is defined as qos.ucast.out.latency * 1000 / qos.ucast.out.interval.

Since this latency could be smaller than the SDU interval, the resulting
num could be 0, causing the file transfer to stop after the first packet.
In this case num will be set to 1 so that at least 1 packet is always sent.

It the transport send timer is set to a value smaller than that of SDU
interval, the available buffers for ISO Data will eventually become full.
Thus, if a packet can't be sent due to resource temporarily being
unavailable decrease the fd offset so that next time the same packet will
be sent.

This patch is a temporary fix until the appropriate solution that uses
number of completed packets to control the flow of ISO Data packets is
implemented.

Since both Unicast and Broadcast scenarios use the same transport functions
differentiate between the 2 cases when accessing the qos structures to get
the transport latency.
---
 client/player.c | 55 +++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 49 insertions(+), 6 deletions(-)

diff --git a/client/player.c b/client/player.c
index 1f56bfd27..ca169e58f 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"
@@ -4972,11 +4973,23 @@ static int transport_send_seq(struct transport *transport, int fd, uint32_t num)
 		}
 
 		ret = send(transport->sk, buf, ret, 0);
+		/* If send failed due to the resource being temporarily
+		 * unavailable the controller's ISO data buffers are
+		 * full. Try sending the same packet next time.
+		 */
 		if (ret <= 0) {
-			bt_shell_printf("send failed: %s (%d)",
+			if (errno == EAGAIN) {
+				/* Decrease the fd's offset so that the same
+				 * packet is sent next time.
+				 */
+				offset = lseek(fd, -transport->mtu[1],
+								SEEK_CUR);
+			} else {
+				bt_shell_printf("send failed: %s (%d)",
 							strerror(errno), errno);
-			free(buf);
-			return -errno;
+				free(buf);
+				return -errno;
+			}
 		}
 
 		elapsed_time(!transport->seq, &secs, &nsecs);
@@ -5033,7 +5046,15 @@ static bool transport_timer_read(struct io *io, void *user_data)
 
 	/* num of packets = latency (ms) / interval (us) */
 	num = (qos.ucast.out.latency * 1000 / qos.ucast.out.interval);
-
+	if (num < 1)
+		/* The latency could be smaller than the interval resulting in
+		 * num being 0. If this is the case, set it to 1 so that packets
+		 * will still be sent.
+		 */
+		num = 1;
+	/* TODO: replace this timer based implementation with one that
+	 * uses the number of completed packets reports.
+	 */
 	ret = transport_send_seq(transport, transport->fd, num);
 	if (ret < 0) {
 		bt_shell_printf("Unable to send: %s (%d)\n",
@@ -5052,6 +5073,8 @@ static bool transport_timer_read(struct io *io, void *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,8 +5091,28 @@ static int transport_send(struct transport *transport, int fd,
 		return -errno;
 
 	memset(&ts, 0, sizeof(ts));
-	ts.it_value.tv_nsec = qos->ucast.out.latency * 1000000;
-	ts.it_interval.tv_nsec = 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's 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 = sizeof(addr);
+
+	if (getpeername(transport->sk, &addr, &optlen) < 0)
+		return -errno;
+
+	if (!(bacmp(&addr.iso_bdaddr, BDADDR_ANY))) {
+		/* Interval is measured in ms, multiply by 1000000 to get ns */
+		ts.it_value.tv_nsec = qos->bcast.out.latency * 1000000;
+		ts.it_interval.tv_nsec = qos->bcast.out.latency * 1000000;
+	} else {
+		/* Interval is measured in ms, multiply by 1000000 to get ns */
+		ts.it_value.tv_nsec = qos->ucast.out.latency * 1000000;
+		ts.it_interval.tv_nsec = qos->ucast.out.latency * 1000000;
+	}
 
 	if (timerfd_settime(timer_fd, TFD_TIMER_ABSTIME, &ts, NULL) < 0)
 		return -errno;
-- 
2.40.1

Re: [PATCH BlueZ v2 1/1] client/player: Fix transport.send command's transfer of packets

[Luiz Augusto von Dentz]

Hi Vlad,

On Fri, Apr 19, 2024 at 10:58 AM Vlad Pruteanu <vlad.pruteanu@nxp.com> wrote:
>
> The transport.send command sends a number num of packets at intervals of
> transport latency.
>
> Num is defined as qos.ucast.out.latency * 1000 / qos.ucast.out.interval.
>
> Since this latency could be smaller than the SDU interval, the resulting
> num could be 0, causing the file transfer to stop after the first packet.
> In this case num will be set to 1 so that at least 1 packet is always sent.
>
> It the transport send timer is set to a value smaller than that of SDU
> interval, the available buffers for ISO Data will eventually become full.
> Thus, if a packet can't be sent due to resource temporarily being
> unavailable decrease the fd offset so that next time the same packet will
> be sent.
>
> This patch is a temporary fix until the appropriate solution that uses
> number of completed packets to control the flow of ISO Data packets is
> implemented.
>
> Since both Unicast and Broadcast scenarios use the same transport functions
> differentiate between the 2 cases when accessing the qos structures to get
> the transport latency.
> ---
>  client/player.c | 55 +++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 49 insertions(+), 6 deletions(-)
>
> diff --git a/client/player.c b/client/player.c
> index 1f56bfd27..ca169e58f 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"
> @@ -4972,11 +4973,23 @@ static int transport_send_seq(struct transport *transport, int fd, uint32_t num)
>                 }
>
>                 ret = send(transport->sk, buf, ret, 0);
> +               /* If send failed due to the resource being temporarily
> +                * unavailable the controller's ISO data buffers are
> +                * full. Try sending the same packet next time.
> +                */
>                 if (ret <= 0) {
> -                       bt_shell_printf("send failed: %s (%d)",
> +                       if (errno == EAGAIN) {
> +                               /* Decrease the fd's offset so that the same
> +                                * packet is sent next time.
> +                                */
> +                               offset = lseek(fd, -transport->mtu[1],
> +                                                               SEEK_CUR);

Not really sure why you think this is a good idea, ISO already has
retransmission support and if that is failing then there is no reason
to retry here, beside this could loop causing the same data to be
retried forever.

> +                       } else {
> +                               bt_shell_printf("send failed: %s (%d)",
>                                                         strerror(errno), errno);
> -                       free(buf);
> -                       return -errno;
> +                               free(buf);
> +                               return -errno;
> +                       }
>                 }
>
>                 elapsed_time(!transport->seq, &secs, &nsecs);
> @@ -5033,7 +5046,15 @@ static bool transport_timer_read(struct io *io, void *user_data)
>
>         /* num of packets = latency (ms) / interval (us) */
>         num = (qos.ucast.out.latency * 1000 / qos.ucast.out.interval);
> -
> +       if (num < 1)
> +               /* The latency could be smaller than the interval resulting in
> +                * num being 0. If this is the case, set it to 1 so that packets
> +                * will still be sent.
> +                */
> +               num = 1;

Perhaps we should be looking into rounding closest sort of logic.

> +       /* TODO: replace this timer based implementation with one that
> +        * uses the number of completed packets reports.
> +        */
>         ret = transport_send_seq(transport, transport->fd, num);
>         if (ret < 0) {
>                 bt_shell_printf("Unable to send: %s (%d)\n",
> @@ -5052,6 +5073,8 @@ static bool transport_timer_read(struct io *io, void *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,8 +5091,28 @@ static int transport_send(struct transport *transport, int fd,
>                 return -errno;
>
>         memset(&ts, 0, sizeof(ts));
> -       ts.it_value.tv_nsec = qos->ucast.out.latency * 1000000;
> -       ts.it_interval.tv_nsec = 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's 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 = sizeof(addr);
> +
> +       if (getpeername(transport->sk, &addr, &optlen) < 0)
> +               return -errno;
> +
> +       if (!(bacmp(&addr.iso_bdaddr, BDADDR_ANY))) {
> +               /* Interval is measured in ms, multiply by 1000000 to get ns */
> +               ts.it_value.tv_nsec = qos->bcast.out.latency * 1000000;
> +               ts.it_interval.tv_nsec = qos->bcast.out.latency * 1000000;
> +       } else {
> +               /* Interval is measured in ms, multiply by 1000000 to get ns */
> +               ts.it_value.tv_nsec = qos->ucast.out.latency * 1000000;
> +               ts.it_interval.tv_nsec = qos->ucast.out.latency * 1000000;
> +       }

This is a different fix, please send it as a separate patch.

>         if (timerfd_settime(timer_fd, TFD_TIMER_ABSTIME, &ts, NULL) < 0)
>                 return -errno;
> --
> 2.40.1
>


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ v2 1/1] client/player: Fix transport.send command's transfer of packets

[Luiz Augusto von Dentz]

Hi Pauli,

On Fri, Apr 19, 2024 at 11:24 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Vlad,
>
> On Fri, Apr 19, 2024 at 10:58 AM Vlad Pruteanu <vlad.pruteanu@nxp.com> wrote:
> >
> > The transport.send command sends a number num of packets at intervals of
> > transport latency.
> >
> > Num is defined as qos.ucast.out.latency * 1000 / qos.ucast.out.interval.
> >
> > Since this latency could be smaller than the SDU interval, the resulting
> > num could be 0, causing the file transfer to stop after the first packet.
> > In this case num will be set to 1 so that at least 1 packet is always sent.
> >
> > It the transport send timer is set to a value smaller than that of SDU
> > interval, the available buffers for ISO Data will eventually become full.
> > Thus, if a packet can't be sent due to resource temporarily being
> > unavailable decrease the fd offset so that next time the same packet will
> > be sent.
> >
> > This patch is a temporary fix until the appropriate solution that uses
> > number of completed packets to control the flow of ISO Data packets is
> > implemented.
> >
> > Since both Unicast and Broadcast scenarios use the same transport functions
> > differentiate between the 2 cases when accessing the qos structures to get
> > the transport latency.
> > ---
> >  client/player.c | 55 +++++++++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 49 insertions(+), 6 deletions(-)
> >
> > diff --git a/client/player.c b/client/player.c
> > index 1f56bfd27..ca169e58f 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"
> > @@ -4972,11 +4973,23 @@ static int transport_send_seq(struct transport *transport, int fd, uint32_t num)
> >                 }
> >
> >                 ret = send(transport->sk, buf, ret, 0);
> > +               /* If send failed due to the resource being temporarily
> > +                * unavailable the controller's ISO data buffers are
> > +                * full. Try sending the same packet next time.
> > +                */
> >                 if (ret <= 0) {
> > -                       bt_shell_printf("send failed: %s (%d)",
> > +                       if (errno == EAGAIN) {
> > +                               /* Decrease the fd's offset so that the same
> > +                                * packet is sent next time.
> > +                                */
> > +                               offset = lseek(fd, -transport->mtu[1],
> > +                                                               SEEK_CUR);
>
> Not really sure why you think this is a good idea, ISO already has
> retransmission support and if that is failing then there is no reason
> to retry here, beside this could loop causing the same data to be
> retried forever.
>
> > +                       } else {
> > +                               bt_shell_printf("send failed: %s (%d)",
> >                                                         strerror(errno), errno);
> > -                       free(buf);
> > -                       return -errno;
> > +                               free(buf);
> > +                               return -errno;
> > +                       }
> >                 }
> >
> >                 elapsed_time(!transport->seq, &secs, &nsecs);
> > @@ -5033,7 +5046,15 @@ static bool transport_timer_read(struct io *io, void *user_data)
> >
> >         /* num of packets = latency (ms) / interval (us) */
> >         num = (qos.ucast.out.latency * 1000 / qos.ucast.out.interval);
> > -
> > +       if (num < 1)
> > +               /* The latency could be smaller than the interval resulting in
> > +                * num being 0. If this is the case, set it to 1 so that packets
> > +                * will still be sent.
> > +                */
> > +               num = 1;
>
> Perhaps we should be looking into rounding closest sort of logic.
>
> > +       /* TODO: replace this timer based implementation with one that
> > +        * uses the number of completed packets reports.
> > +        */

Regarding this TODO item, Im planning to introduce something like the
following to io.h:

+bool io_set_tx_complete_handler(struct io *io, io_tx_complete_func_t callback,
+                               int flags, int pool_interval,
+                               void *user_data, io_destroy_func_t destroy);

The problem is that if we do schedule new packets on tx_complete
callback that doesn't account for the time it takes to process such
event, so over time this will accumulate and at some point we could
perhaps miss an interval, @Pauli Virtanen or perhaps you are not
really doing the flow control based on the TX timestamp/complete? That
perhaps depends if the controller is generating the events as soon as
the packet is submitted or at the exact moment of the event interval,
in any case the general idea is that we keep the controller buffers
full as much as possible to prevent missing intervals.

> >         ret = transport_send_seq(transport, transport->fd, num);
> >         if (ret < 0) {
> >                 bt_shell_printf("Unable to send: %s (%d)\n",
> > @@ -5052,6 +5073,8 @@ static bool transport_timer_read(struct io *io, void *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,8 +5091,28 @@ static int transport_send(struct transport *transport, int fd,
> >                 return -errno;
> >
> >         memset(&ts, 0, sizeof(ts));
> > -       ts.it_value.tv_nsec = qos->ucast.out.latency * 1000000;
> > -       ts.it_interval.tv_nsec = 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's 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 = sizeof(addr);
> > +
> > +       if (getpeername(transport->sk, &addr, &optlen) < 0)
> > +               return -errno;
> > +
> > +       if (!(bacmp(&addr.iso_bdaddr, BDADDR_ANY))) {
> > +               /* Interval is measured in ms, multiply by 1000000 to get ns */
> > +               ts.it_value.tv_nsec = qos->bcast.out.latency * 1000000;
> > +               ts.it_interval.tv_nsec = qos->bcast.out.latency * 1000000;
> > +       } else {
> > +               /* Interval is measured in ms, multiply by 1000000 to get ns */
> > +               ts.it_value.tv_nsec = qos->ucast.out.latency * 1000000;
> > +               ts.it_interval.tv_nsec = qos->ucast.out.latency * 1000000;
> > +       }
>
> This is a different fix, please send it as a separate patch.
>
> >         if (timerfd_settime(timer_fd, TFD_TIMER_ABSTIME, &ts, NULL) < 0)
> >                 return -errno;
> > --
> > 2.40.1
> >
>
>
> --
> Luiz Augusto von Dentz



-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ v2 1/1] client/player: Fix transport.send command's transfer of packets

[Pauli Virtanen]

Hi,

pe, 2024-04-19 kello 14:16 -0400, Luiz Augusto von Dentz kirjoitti:
> Hi Pauli,
> 
> On Fri, Apr 19, 2024 at 11:24 AM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> > 
> > Hi Vlad,
> > 
> > On Fri, Apr 19, 2024 at 10:58 AM Vlad Pruteanu <vlad.pruteanu@nxp.com> wrote:
> > > 
> > > The transport.send command sends a number num of packets at intervals of
> > > transport latency.
> > > 
> > > Num is defined as qos.ucast.out.latency * 1000 / qos.ucast.out.interval.
> > > 
> > > Since this latency could be smaller than the SDU interval, the resulting
> > > num could be 0, causing the file transfer to stop after the first packet.
> > > In this case num will be set to 1 so that at least 1 packet is always sent.
> > > 
> > > It the transport send timer is set to a value smaller than that of SDU
> > > interval, the available buffers for ISO Data will eventually become full.
> > > Thus, if a packet can't be sent due to resource temporarily being
> > > unavailable decrease the fd offset so that next time the same packet will
> > > be sent.
> > > 
> > > This patch is a temporary fix until the appropriate solution that uses
> > > number of completed packets to control the flow of ISO Data packets is
> > > implemented.
> > > 
> > > Since both Unicast and Broadcast scenarios use the same transport functions
> > > differentiate between the 2 cases when accessing the qos structures to get
> > > the transport latency.
> > > ---
> > >  client/player.c | 55 +++++++++++++++++++++++++++++++++++++++++++------
> > >  1 file changed, 49 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/client/player.c b/client/player.c
> > > index 1f56bfd27..ca169e58f 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"
> > > @@ -4972,11 +4973,23 @@ static int transport_send_seq(struct transport *transport, int fd, uint32_t num)
> > >                 }
> > > 
> > >                 ret = send(transport->sk, buf, ret, 0);
> > > +               /* If send failed due to the resource being temporarily
> > > +                * unavailable the controller's ISO data buffers are
> > > +                * full. Try sending the same packet next time.
> > > +                */
> > >                 if (ret <= 0) {
> > > -                       bt_shell_printf("send failed: %s (%d)",
> > > +                       if (errno == EAGAIN) {
> > > +                               /* Decrease the fd's offset so that the same
> > > +                                * packet is sent next time.
> > > +                                */
> > > +                               offset = lseek(fd, -transport->mtu[1],
> > > +                                                               SEEK_CUR);
> > 
> > Not really sure why you think this is a good idea, ISO already has
> > retransmission support and if that is failing then there is no reason
> > to retry here, beside this could loop causing the same data to be
> > retried forever.
> > 
> > > +                       } else {
> > > +                               bt_shell_printf("send failed: %s (%d)",
> > >                                                         strerror(errno), errno);
> > > -                       free(buf);
> > > -                       return -errno;
> > > +                               free(buf);
> > > +                               return -errno;
> > > +                       }
> > >                 }
> > > 
> > >                 elapsed_time(!transport->seq, &secs, &nsecs);
> > > @@ -5033,7 +5046,15 @@ static bool transport_timer_read(struct io *io, void *user_data)
> > > 
> > >         /* num of packets = latency (ms) / interval (us) */
> > >         num = (qos.ucast.out.latency * 1000 / qos.ucast.out.interval);
> > > -
> > > +       if (num < 1)
> > > +               /* The latency could be smaller than the interval resulting in
> > > +                * num being 0. If this is the case, set it to 1 so that packets
> > > +                * will still be sent.
> > > +                */
> > > +               num = 1;
> > 
> > Perhaps we should be looking into rounding closest sort of logic.
> > 
> > > +       /* TODO: replace this timer based implementation with one that
> > > +        * uses the number of completed packets reports.
> > > +        */
> 
> Regarding this TODO item, Im planning to introduce something like the
> following to io.h:
> 
> +bool io_set_tx_complete_handler(struct io *io, io_tx_complete_func_t callback,
> +                               int flags, int pool_interval,
> +                               void *user_data, io_destroy_func_t destroy);
> 
> The problem is that if we do schedule new packets on tx_complete
> callback that doesn't account for the time it takes to process such
> event, so over time this will accumulate and at some point we could
> perhaps miss an interval, @Pauli Virtanen or perhaps you are not
> really doing the flow control based on the TX timestamp/complete? That
> perhaps depends if the controller is generating the events as soon as
> the packet is submitted or at the exact moment of the event interval,
> in any case the general idea is that we keep the controller buffers
> full as much as possible to prevent missing intervals.

Pipewire is currently not using the TX timestamps for flow control like
that.

The running min--max range of sendmsg-to-NCP event latency of each CIS
is monitored, and if they are not in sync then packets are dropped to
realign the CIS. Similarly, packets are dropped if latencies become
larger than a defined threshold.

Otherwise, one sendmsg() per CIS is done on every SDU_Interval based on
a timer. This is from rt prio thread, so there's less timing jitter on
our side. Earlier I experimented with explicitly pushing more data to
keep buffers filled but this did not seem to really matter.

One probably could try to do something smarter here, e.g. indeed trying
to use the TX timestamp events to track how many packets are
uncompleted, and then try to keep the number at a constant.

However, I don't see the Core specification saying much about the HCI
ISO flow control. E.g. it's not so clear you can queue SDUs for next
SDU_Intervals if you are not providing time stamps:

BLUETOOTH CORE SPECIFICATION Version 5.4 | Vol 6, Part G page 3079

3.3 TIME STAMP FOR SDU
...
When an HCI ISO Data packet sent by the Host does not contain a
Time_Stamp or the Time_Stamp value is not based on the Controller's
clock, the Controller should determine the CIS or BIS event to be used
to transmit the SDU contained in that packet based on the time of
arrival of that packet.
...

> 
> > >         ret = transport_send_seq(transport, transport->fd, num);
> > >         if (ret < 0) {
> > >                 bt_shell_printf("Unable to send: %s (%d)\n",
> > > @@ -5052,6 +5073,8 @@ static bool transport_timer_read(struct io *io, void *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,8 +5091,28 @@ static int transport_send(struct transport *transport, int fd,
> > >                 return -errno;
> > > 
> > >         memset(&ts, 0, sizeof(ts));
> > > -       ts.it_value.tv_nsec = qos->ucast.out.latency * 1000000;
> > > -       ts.it_interval.tv_nsec = 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's 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 = sizeof(addr);
> > > +
> > > +       if (getpeername(transport->sk, &addr, &optlen) < 0)
> > > +               return -errno;
> > > +
> > > +       if (!(bacmp(&addr.iso_bdaddr, BDADDR_ANY))) {
> > > +               /* Interval is measured in ms, multiply by 1000000 to get ns */
> > > +               ts.it_value.tv_nsec = qos->bcast.out.latency * 1000000;
> > > +               ts.it_interval.tv_nsec = qos->bcast.out.latency * 1000000;
> > > +       } else {
> > > +               /* Interval is measured in ms, multiply by 1000000 to get ns */
> > > +               ts.it_value.tv_nsec = qos->ucast.out.latency * 1000000;
> > > +               ts.it_interval.tv_nsec = qos->ucast.out.latency * 1000000;
> > > +       }
> > 
> > This is a different fix, please send it as a separate patch.
> > 
> > >         if (timerfd_settime(timer_fd, TFD_TIMER_ABSTIME, &ts, NULL) < 0)
> > >                 return -errno;
> > > --
> > > 2.40.1
> > > 
> > 
> > 
> > --
> > Luiz Augusto von Dentz
> 
> 
> 

-- 
Pauli Virtanen

RE: client/player: Fix transport.send command's transfer of packets

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1279 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=846161

---Test result---

Test Summary:
CheckPatch                    PASS      0.41 seconds
GitLint                       PASS      0.26 seconds
BuildEll                      PASS      24.53 seconds
BluezMake                     PASS      1712.85 seconds
MakeCheck                     PASS      13.17 seconds
MakeDistcheck                 PASS      176.46 seconds
CheckValgrind                 PASS      250.02 seconds
CheckSmatch                   PASS      353.84 seconds
bluezmakeextell               PASS      118.77 seconds
IncrementalBuild              PASS      1478.60 seconds
ScanBuild                     WARNING   1002.10 seconds

Details
##############################
Test: ScanBuild - WARNING
Desc: Run Scan Build
Output:
client/player.c:4985:5: warning: Value stored to 'offset' is never read
                                offset = lseek(fd, -transport->mtu[1],
                                ^        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.



---
Regards,
Linux Bluetooth

[PATCH BlueZ 1/1] client/player: Fix transport.send command's transfer of packets

[Vlad Pruteanu]

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 presets,
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.
---
 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, void *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, void *user_data)
 		return false;
 	}
 
-	/* num of packets = latency (ms) / interval (us) */
-	num = (qos.ucast.out.latency * 1000 / qos.ucast.out.interval);
-
-	ret = transport_send_seq(transport, transport->fd, num);
+	ret = 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, void *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 *transport, int fd,
 		return -errno;
 
 	memset(&ts, 0, sizeof(ts));
-	ts.it_value.tv_nsec = qos->ucast.out.latency * 1000000;
-	ts.it_interval.tv_nsec = 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 = sizeof(addr);
+
+	if (getpeername(transport->sk, &addr, &optlen) < 0)
+		return -errno;
+
+	if (!(bacmp(&addr.iso_bdaddr, BDADDR_ANY))) {
+		/* Interval is measured in us, multiply by 1000 to get ns */
+		ts.it_value.tv_nsec = qos->bcast.out.interval * 1000;
+		ts.it_interval.tv_nsec = qos->bcast.out.interval * 1000;
+	} else {
+		/* Interval is measured in us, multiply by 1000 to get ns */
+		ts.it_value.tv_nsec = qos->ucast.out.interval * 1000;
+		ts.it_interval.tv_nsec = qos->ucast.out.interval * 1000;
+
+	}
 	if (timerfd_settime(timer_fd, TFD_TIMER_ABSTIME, &ts, NULL) < 0)
 		return -errno;
 
-- 
2.39.2

RE: client/player: Fix transport.send command's transfer of packets

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 948 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=837924

---Test result---

Test Summary:
CheckPatch                    PASS      0.34 seconds
GitLint                       PASS      0.23 seconds
BuildEll                      PASS      24.69 seconds
BluezMake                     PASS      1710.49 seconds
MakeCheck                     PASS      13.72 seconds
MakeDistcheck                 PASS      173.11 seconds
CheckValgrind                 PASS      247.65 seconds
CheckSmatch                   PASS      344.68 seconds
bluezmakeextell               PASS      116.81 seconds
IncrementalBuild              PASS      1507.39 seconds
ScanBuild                     PASS      988.72 seconds



---
Regards,
Linux Bluetooth