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.
Vlad Pruteanu (1):
client/player: Fix transport.send command's transfer of packets
client/player.c | 34 +++++++++++++++++++++++++++-------
1 file changed, 27 insertions(+), 7 deletions(-)
--
2.39.2
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
Hi Vlad,
On Mon, Mar 25, 2024 at 10:40 AM Vlad Pruteanu <[email protected]> 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 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.
Please add some output samples showing the wrong interval ends up being used.
> ---
> 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;
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 = 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
>
--
Luiz Augusto von Dentz
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
Hello Luiz,
> -----Original Message-----
> From: Luiz Augusto von Dentz <[email protected]>
> Sent: Monday, March 25, 2024 6:04 PM
> To: Vlad Pruteanu <[email protected]>
> Cc: [email protected]; Mihai-Octavian Urzica <mihai-
> [email protected]>; Silviu Florian Barbulescu
> <[email protected]>; Iulia Tanasescu <[email protected]>;
> Andrei Istodorescu <[email protected]>
> Subject: [EXT] Re: [PATCH BlueZ 1/1] client/player: Fix transport.send
> command's transfer of packets
>
> 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 Vlad,
>
> On Mon, Mar 25, 2024 at 10:40 AM Vlad Pruteanu
> <[email protected]> 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 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.
>
> Please add some output samples showing the wrong interval ends up being
> used.
>
Below you can find what I consider to a relevant excerpt taken from btmon log:
...
< HCI Command: LE Set Connected Isochronous Group Parameters (0x08|0x0062)
CIG ID: 0x00
Central to Peripheral SDU Interval: 10000 us (0x002710)
Peripheral to Central SDU Interval: 10000 us (0x002710)
SCA: 201 - 500 ppm (0x00)
Packing: Sequential (0x00)
Framing: Unframed (0x00)
Central to Peripheral Maximum Latency: 10 ms (0x000a)
Peripheral to Central Maximum Latency: 10 ms (0x000a)
Number of CIS: 1
CIS ID: 0x00
Central to Peripheral Maximum SDU Size: 40
Peripheral to Central Maximum SDU Size: 0
Central to Peripheral PHY: LE 2M (0x02)
Peripheral to Central PHY: LE 2M (0x02)
Central to Peripheral Retransmission attempts: 0x02
Peripheral to Central Retransmission attempts: 0x00
...
> HCI Event: Command Complete (0x0e)
LE Set Connected Isochronous Group Parameters (0x08|0x0062) ncmd 1
Status: Success (0x00)
CIG ID: 0x00
Number of Handles: 1
Connection Handle #0: 144
...
< HCI Command: LE Create Connected Isochronous Stream (0x08|0x0064)
Number of CIS: 1
CIS Handle: 144
ACL Handle: 128
...
> HCI Event: LE Meta Event (0x3e)
LE Connected Isochronous Stream Established (0x19)
Status: Success (0x00)
Connection Handle: 144
CIG Synchronization Delay: 1992 us (0x0007c8)
CIS Synchronization Delay: 1992 us (0x0007c8)
Central to Peripheral Latency: 1992 us (0x0007c8)
Peripheral to Central Latency: 1992 us (0x0007c8)
Central to Peripheral PHY: LE 2M (0x02)
Peripheral to Central PHY: LE 2M (0x02)
Number of Subevents: 3
Central to Peripheral Burst Number: 1
Peripheral to Central Burst Number: 0
Central to Peripheral Flush Timeout: 1
Peripheral to Central Flush Timeout: 1
Central to Peripheral MTU: 40
Peripheral to Central MTU: 0
ISO Interval: 10.00 msec (0x0008)
...
bluetoothd[349393]: < ISO Data TX: Handle 144 flags 0x02
> HCI Event: Number of Completed Packets (0x13)
Num handles: 1
Handle: 144 Address: A0:CD:F3:78:04:0A (Murata Manufacturing Co., Ltd.)
Count: 1
#470: len 44 (27 Kb/s)
Latency: 13 msec (13-13 msec ~13 msec)
The Maximum Latency is correctly set to 10ms in the LE Set Connected Isochronous Group Parameters command, and the controller reports the final latency as being 1992 us. This is the value that (after conversion to ms) qos.ucast.out.latency takes in the current implementation. num is defined as num = (qos.ucast.out.latency * 1000 / qos.ucast.out.interval), with interval being the ISO interval, 10000us and therefore num will be 0 in this case. As you can see the first packet is always sent, but after that none can be seen. Instead, the Host should send 1 packet every time this function is called. The timer related change below ensures that this function is triggered at every SDU interval. So, in the end, the Host will send 1 packet every SDU interval.
> > ---
> > 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;
>
> 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?
>
The main change that I made here is the value of the interval for the timer.
Before, it used to be set to qos.ucast.out.latency and I propose to change
it to qos.ucast.out.interval, so that the Host will send 1SDU packet on
every ISO Interval. I included the ucast/bcast qos update since I was
already updating this line of code. That is, I didn't want to send the patch
with "qos->ucast.out.interval" without differentiating between bcast and
ucast since it would've been wrong.
> > + 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
> >
>
>
> --
> Luiz Augusto von Dentz
Hi Vlad,
On Tue, Mar 26, 2024 at 11:21 AM Vlad Pruteanu <[email protected]> wrote:
>
> Hello Luiz,
>
>
> > -----Original Message-----
> > From: Luiz Augusto von Dentz <[email protected]>
> > Sent: Monday, March 25, 2024 6:04 PM
> > To: Vlad Pruteanu <[email protected]>
> > Cc: [email protected]; Mihai-Octavian Urzica <mihai-
> > [email protected]>; Silviu Florian Barbulescu
> > <[email protected]>; Iulia Tanasescu <[email protected]>;
> > Andrei Istodorescu <[email protected]>
> > Subject: [EXT] Re: [PATCH BlueZ 1/1] client/player: Fix transport.send
> > command's transfer of packets
> >
> > 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 Vlad,
> >
> > On Mon, Mar 25, 2024 at 10:40 AM Vlad Pruteanu
> > <[email protected]> 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 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.
> >
> > Please add some output samples showing the wrong interval ends up being
> > used.
> >
> Below you can find what I consider to a relevant excerpt taken from btmon log:
> ...
> < HCI Command: LE Set Connected Isochronous Group Parameters (0x08|0x0062)
> CIG ID: 0x00
> Central to Peripheral SDU Interval: 10000 us (0x002710)
> Peripheral to Central SDU Interval: 10000 us (0x002710)
> SCA: 201 - 500 ppm (0x00)
> Packing: Sequential (0x00)
> Framing: Unframed (0x00)
> Central to Peripheral Maximum Latency: 10 ms (0x000a)
> Peripheral to Central Maximum Latency: 10 ms (0x000a)
> Number of CIS: 1
> CIS ID: 0x00
> Central to Peripheral Maximum SDU Size: 40
> Peripheral to Central Maximum SDU Size: 0
> Central to Peripheral PHY: LE 2M (0x02)
> Peripheral to Central PHY: LE 2M (0x02)
> Central to Peripheral Retransmission attempts: 0x02
> Peripheral to Central Retransmission attempts: 0x00
> ...
> > HCI Event: Command Complete (0x0e)
> LE Set Connected Isochronous Group Parameters (0x08|0x0062) ncmd 1
> Status: Success (0x00)
> CIG ID: 0x00
> Number of Handles: 1
> Connection Handle #0: 144
> ...
> < HCI Command: LE Create Connected Isochronous Stream (0x08|0x0064)
> Number of CIS: 1
> CIS Handle: 144
> ACL Handle: 128
> ...
> > HCI Event: LE Meta Event (0x3e)
> LE Connected Isochronous Stream Established (0x19)
> Status: Success (0x00)
> Connection Handle: 144
> CIG Synchronization Delay: 1992 us (0x0007c8)
> CIS Synchronization Delay: 1992 us (0x0007c8)
> Central to Peripheral Latency: 1992 us (0x0007c8)
> Peripheral to Central Latency: 1992 us (0x0007c8)
> Central to Peripheral PHY: LE 2M (0x02)
> Peripheral to Central PHY: LE 2M (0x02)
> Number of Subevents: 3
> Central to Peripheral Burst Number: 1
> Peripheral to Central Burst Number: 0
> Central to Peripheral Flush Timeout: 1
> Peripheral to Central Flush Timeout: 1
> Central to Peripheral MTU: 40
> Peripheral to Central MTU: 0
> ISO Interval: 10.00 msec (0x0008)
> ...
> bluetoothd[349393]: < ISO Data TX: Handle 144 flags 0x02
> > HCI Event: Number of Completed Packets (0x13)
> Num handles: 1
> Handle: 144 Address: A0:CD:F3:78:04:0A (Murata Manufacturing Co., Ltd.)
> Count: 1
> #470: len 44 (27 Kb/s)
> Latency: 13 msec (13-13 msec ~13 msec)
>
> The Maximum Latency is correctly set to 10ms in the LE Set Connected Isochronous Group Parameters command, and the controller reports the final latency as being 1992 us. This is the value that (after conversion to ms) qos.ucast.out.latency takes in the current implementation. num is defined as num = (qos.ucast.out.latency * 1000 / qos.ucast.out.interval), with interval being the ISO interval, 10000us and therefore num will be 0 in this case. As you can see the first packet is always sent, but after that none can be seen. Instead, the Host should send 1 packet every time this function is called. The timer related change below ensures that this function is triggered at every SDU interval. So, in the end, the Host will send 1 packet every SDU interval.
> > > ---
> > > 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;
> >
> > 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?
> >
> The main change that I made here is the value of the interval for the timer.
> Before, it used to be set to qos.ucast.out.latency and I propose to change
> it to qos.ucast.out.interval, so that the Host will send 1SDU packet on
> every ISO Interval. I included the ucast/bcast qos update since I was
> already updating this line of code. That is, I didn't want to send the patch
> with "qos->ucast.out.interval" without differentiating between bcast and
> ucast since it would've been wrong.
Hmm if I recall correctly this code used to be sending a packet per
interval but we run into problems because the interval is rather short
and OS itself would take time to transfer the packet from userspace to
socket and then to driver, etc, so I suggest we round up the value if
it ends up being 0.
> > > + 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
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz
--
Luiz Augusto von Dentz
Hi,
ti, 2024-03-26 kello 11:33 -0400, Luiz Augusto von Dentz kirjoitti:
> Hi Vlad,
>
> On Tue, Mar 26, 2024 at 11:21 AM Vlad Pruteanu <[email protected]> wrote:
> >
> > Hello Luiz,
> >
> >
> > > -----Original Message-----
> > > From: Luiz Augusto von Dentz <[email protected]>
> > > Sent: Monday, March 25, 2024 6:04 PM
> > > To: Vlad Pruteanu <[email protected]>
> > > Cc: [email protected]; Mihai-Octavian Urzica <mihai-
> > > [email protected]>; Silviu Florian Barbulescu
> > > <[email protected]>; Iulia Tanasescu <[email protected]>;
> > > Andrei Istodorescu <[email protected]>
> > > Subject: [EXT] Re: [PATCH BlueZ 1/1] client/player: Fix transport.send
> > > command's transfer of packets
> > >
> > > 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 Vlad,
> > >
> > > On Mon, Mar 25, 2024 at 10:40 AM Vlad Pruteanu
> > > <[email protected]> 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 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.
> > >
> > > Please add some output samples showing the wrong interval ends up being
> > > used.
> > >
> > Below you can find what I consider to a relevant excerpt taken from btmon log:
> > ...
> > < HCI Command: LE Set Connected Isochronous Group Parameters (0x08|0x0062)
> > CIG ID: 0x00
> > Central to Peripheral SDU Interval: 10000 us (0x002710)
> > Peripheral to Central SDU Interval: 10000 us (0x002710)
> > SCA: 201 - 500 ppm (0x00)
> > Packing: Sequential (0x00)
> > Framing: Unframed (0x00)
> > Central to Peripheral Maximum Latency: 10 ms (0x000a)
> > Peripheral to Central Maximum Latency: 10 ms (0x000a)
> > Number of CIS: 1
> > CIS ID: 0x00
> > Central to Peripheral Maximum SDU Size: 40
> > Peripheral to Central Maximum SDU Size: 0
> > Central to Peripheral PHY: LE 2M (0x02)
> > Peripheral to Central PHY: LE 2M (0x02)
> > Central to Peripheral Retransmission attempts: 0x02
> > Peripheral to Central Retransmission attempts: 0x00
> > ...
> > > HCI Event: Command Complete (0x0e)
> > LE Set Connected Isochronous Group Parameters (0x08|0x0062) ncmd 1
> > Status: Success (0x00)
> > CIG ID: 0x00
> > Number of Handles: 1
> > Connection Handle #0: 144
> > ...
> > < HCI Command: LE Create Connected Isochronous Stream (0x08|0x0064)
> > Number of CIS: 1
> > CIS Handle: 144
> > ACL Handle: 128
> > ...
> > > HCI Event: LE Meta Event (0x3e)
> > LE Connected Isochronous Stream Established (0x19)
> > Status: Success (0x00)
> > Connection Handle: 144
> > CIG Synchronization Delay: 1992 us (0x0007c8)
> > CIS Synchronization Delay: 1992 us (0x0007c8)
> > Central to Peripheral Latency: 1992 us (0x0007c8)
> > Peripheral to Central Latency: 1992 us (0x0007c8)
> > Central to Peripheral PHY: LE 2M (0x02)
> > Peripheral to Central PHY: LE 2M (0x02)
> > Number of Subevents: 3
> > Central to Peripheral Burst Number: 1
> > Peripheral to Central Burst Number: 0
> > Central to Peripheral Flush Timeout: 1
> > Peripheral to Central Flush Timeout: 1
> > Central to Peripheral MTU: 40
> > Peripheral to Central MTU: 0
> > ISO Interval: 10.00 msec (0x0008)
> > ...
> > bluetoothd[349393]: < ISO Data TX: Handle 144 flags 0x02
> > > HCI Event: Number of Completed Packets (0x13)
> > Num handles: 1
> > Handle: 144 Address: A0:CD:F3:78:04:0A (Murata Manufacturing Co., Ltd.)
> > Count: 1
> > #470: len 44 (27 Kb/s)
> > Latency: 13 msec (13-13 msec ~13 msec)
> >
> > The Maximum Latency is correctly set to 10ms in the LE Set Connected Isochronous Group Parameters command, and the controller reports the final latency as being 1992 us. This is the value that (after conversion to ms) qos.ucast.out.latency takes in the current implementation. num is defined as num = (qos.ucast.out.latency * 1000 / qos.ucast.out.interval), with interval being the ISO interval, 10000us and therefore num will be 0 in this case. As you can see the first packet is always sent, but after that none can be seen. Instead, the Host should send 1 packet every time this function is called. The timer related change below ensures that this function is triggered at every SDU interval. So, in the end, the Host will send 1 packet every SDU interval.
> > > > ---
> > > > 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;
> > >
> > > 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?
> > >
> > The main change that I made here is the value of the interval for the timer.
> > Before, it used to be set to qos.ucast.out.latency and I propose to change
> > it to qos.ucast.out.interval, so that the Host will send 1SDU packet on
> > every ISO Interval. I included the ucast/bcast qos update since I was
> > already updating this line of code. That is, I didn't want to send the patch
> > with "qos->ucast.out.interval" without differentiating between bcast and
> > ucast since it would've been wrong.
>
> Hmm if I recall correctly this code used to be sending a packet per
> interval but we run into problems because the interval is rather short
> and OS itself would take time to transfer the packet from userspace to
> socket and then to driver, etc, so I suggest we round up the value if
> it ends up being 0.
ISO_Interval is not necessarily equal to the SDU_Interval, for unframed
it may be an integer multiple of it, and for framed I guess it can be
incommensurate. See Core v5.4 Vol 6 Part G Sec 2.
There might be similar confusion also in the
MediaTransport.QoS.Interval property, which IIRC for Client is
SDU_Interval and for Server is ISO_Interval.
***
The code here probably should not use ISO_Interval for anything, since
this is Host -> ISOAL data flow.
Instead, IIUC, it should determine SDU_Interval from the codec
parameters and for LC3 it is the frame duration. Then it should (on
average) send one SDU per SDU_Interval.
Or, send N*SDU every N*SDU_Interval for some N>=1. But since it's using
timerfd intervals here for the timing, I'm not that sure it is really
necessary as the average data rate is then maintained even if some
wakeups are late and the controller flow control should smooth it out.
>
> > > > + 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
> > > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
>
>
>
Hi Pauli,
On Mon, Apr 1, 2024 at 11:05 AM Pauli Virtanen <[email protected]> wrote:
>
> Hi,
>
> ti, 2024-03-26 kello 11:33 -0400, Luiz Augusto von Dentz kirjoitti:
> > Hi Vlad,
> >
> > On Tue, Mar 26, 2024 at 11:21 AM Vlad Pruteanu <[email protected]> wrote:
> > >
> > > Hello Luiz,
> > >
> > >
> > > > -----Original Message-----
> > > > From: Luiz Augusto von Dentz <[email protected]>
> > > > Sent: Monday, March 25, 2024 6:04 PM
> > > > To: Vlad Pruteanu <[email protected]>
> > > > Cc: [email protected]; Mihai-Octavian Urzica <mihai-
> > > > [email protected]>; Silviu Florian Barbulescu
> > > > <[email protected]>; Iulia Tanasescu <[email protected]>;
> > > > Andrei Istodorescu <[email protected]>
> > > > Subject: [EXT] Re: [PATCH BlueZ 1/1] client/player: Fix transport.send
> > > > command's transfer of packets
> > > >
> > > > 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 Vlad,
> > > >
> > > > On Mon, Mar 25, 2024 at 10:40 AM Vlad Pruteanu
> > > > <[email protected]> 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 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.
> > > >
> > > > Please add some output samples showing the wrong interval ends up being
> > > > used.
> > > >
> > > Below you can find what I consider to a relevant excerpt taken from btmon log:
> > > ...
> > > < HCI Command: LE Set Connected Isochronous Group Parameters (0x08|0x0062)
> > > CIG ID: 0x00
> > > Central to Peripheral SDU Interval: 10000 us (0x002710)
> > > Peripheral to Central SDU Interval: 10000 us (0x002710)
> > > SCA: 201 - 500 ppm (0x00)
> > > Packing: Sequential (0x00)
> > > Framing: Unframed (0x00)
> > > Central to Peripheral Maximum Latency: 10 ms (0x000a)
> > > Peripheral to Central Maximum Latency: 10 ms (0x000a)
> > > Number of CIS: 1
> > > CIS ID: 0x00
> > > Central to Peripheral Maximum SDU Size: 40
> > > Peripheral to Central Maximum SDU Size: 0
> > > Central to Peripheral PHY: LE 2M (0x02)
> > > Peripheral to Central PHY: LE 2M (0x02)
> > > Central to Peripheral Retransmission attempts: 0x02
> > > Peripheral to Central Retransmission attempts: 0x00
> > > ...
> > > > HCI Event: Command Complete (0x0e)
> > > LE Set Connected Isochronous Group Parameters (0x08|0x0062) ncmd 1
> > > Status: Success (0x00)
> > > CIG ID: 0x00
> > > Number of Handles: 1
> > > Connection Handle #0: 144
> > > ...
> > > < HCI Command: LE Create Connected Isochronous Stream (0x08|0x0064)
> > > Number of CIS: 1
> > > CIS Handle: 144
> > > ACL Handle: 128
> > > ...
> > > > HCI Event: LE Meta Event (0x3e)
> > > LE Connected Isochronous Stream Established (0x19)
> > > Status: Success (0x00)
> > > Connection Handle: 144
> > > CIG Synchronization Delay: 1992 us (0x0007c8)
> > > CIS Synchronization Delay: 1992 us (0x0007c8)
> > > Central to Peripheral Latency: 1992 us (0x0007c8)
> > > Peripheral to Central Latency: 1992 us (0x0007c8)
> > > Central to Peripheral PHY: LE 2M (0x02)
> > > Peripheral to Central PHY: LE 2M (0x02)
> > > Number of Subevents: 3
> > > Central to Peripheral Burst Number: 1
> > > Peripheral to Central Burst Number: 0
> > > Central to Peripheral Flush Timeout: 1
> > > Peripheral to Central Flush Timeout: 1
> > > Central to Peripheral MTU: 40
> > > Peripheral to Central MTU: 0
> > > ISO Interval: 10.00 msec (0x0008)
> > > ...
> > > bluetoothd[349393]: < ISO Data TX: Handle 144 flags 0x02
> > > > HCI Event: Number of Completed Packets (0x13)
> > > Num handles: 1
> > > Handle: 144 Address: A0:CD:F3:78:04:0A (Murata Manufacturing Co., Ltd.)
> > > Count: 1
> > > #470: len 44 (27 Kb/s)
> > > Latency: 13 msec (13-13 msec ~13 msec)
> > >
> > > The Maximum Latency is correctly set to 10ms in the LE Set Connected Isochronous Group Parameters command, and the controller reports the final latency as being 1992 us. This is the value that (after conversion to ms) qos.ucast.out.latency takes in the current implementation. num is defined as num = (qos.ucast.out.latency * 1000 / qos.ucast.out.interval), with interval being the ISO interval, 10000us and therefore num will be 0 in this case. As you can see the first packet is always sent, but after that none can be seen. Instead, the Host should send 1 packet every time this function is called. The timer related change below ensures that this function is triggered at every SDU interval. So, in the end, the Host will send 1 packet every SDU interval.
> > > > > ---
> > > > > 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;
> > > >
> > > > 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?
> > > >
> > > The main change that I made here is the value of the interval for the timer.
> > > Before, it used to be set to qos.ucast.out.latency and I propose to change
> > > it to qos.ucast.out.interval, so that the Host will send 1SDU packet on
> > > every ISO Interval. I included the ucast/bcast qos update since I was
> > > already updating this line of code. That is, I didn't want to send the patch
> > > with "qos->ucast.out.interval" without differentiating between bcast and
> > > ucast since it would've been wrong.
> >
> > Hmm if I recall correctly this code used to be sending a packet per
> > interval but we run into problems because the interval is rather short
> > and OS itself would take time to transfer the packet from userspace to
> > socket and then to driver, etc, so I suggest we round up the value if
> > it ends up being 0.
>
> ISO_Interval is not necessarily equal to the SDU_Interval, for unframed
> it may be an integer multiple of it, and for framed I guess it can be
> incommensurate. See Core v5.4 Vol 6 Part G Sec 2.
>
> There might be similar confusion also in the
> MediaTransport.QoS.Interval property, which IIRC for Client is
> SDU_Interval and for Server is ISO_Interval.
>
> ***
>
> The code here probably should not use ISO_Interval for anything, since
> this is Host -> ISOAL data flow.
>
> Instead, IIUC, it should determine SDU_Interval from the codec
> parameters and for LC3 it is the frame duration. Then it should (on
> average) send one SDU per SDU_Interval.
>
> Or, send N*SDU every N*SDU_Interval for some N>=1. But since it's using
> timerfd intervals here for the timing, I'm not that sure it is really
> necessary as the average data rate is then maintained even if some
> wakeups are late and the controller flow control should smooth it out.
Well afaik there is no flow control when using unframed, so the
controller will not smooth anything it will just take whatever there
is in its buffers and send each at its own interval, so if we don't
want to risk being late and thus cause no data at each interval we
better pre-load them, which is what we have been doing since the first
set of packets is sent immediately, after establishing the connection,
and then we send at each latency, we could perhaps just send on
POLLOUT though so we use the Number of Complete Packets aka. TX
Complete to control the flow that way we don't have to do any timing
calculations to know when we need to send the next packet, the problem
is that normally POLLOUT is generated when there are socket buffers
not really controller buffers, so afaik we would be buffering a lot
more than expected.
> >
> > > > > + 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
> > > > >
> > > >
> > > >
> > > > --
> > > > Luiz Augusto von Dentz
> >
> >
> >
>
--
Luiz Augusto von Dentz
Hi Luiz,
ma, 2024-04-01 kello 12:00 -0400, Luiz Augusto von Dentz kirjoitti:
> Hi Pauli,
>
> On Mon, Apr 1, 2024 at 11:05 AM Pauli Virtanen <[email protected]> wrote:
> >
> > Hi,
> >
> > ti, 2024-03-26 kello 11:33 -0400, Luiz Augusto von Dentz kirjoitti:
> > > Hi Vlad,
> > >
> > > On Tue, Mar 26, 2024 at 11:21 AM Vlad Pruteanu <[email protected]> wrote:
> > > >
> > > > Hello Luiz,
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Luiz Augusto von Dentz <[email protected]>
> > > > > Sent: Monday, March 25, 2024 6:04 PM
> > > > > To: Vlad Pruteanu <[email protected]>
> > > > > Cc: [email protected]; Mihai-Octavian Urzica <mihai-
> > > > > [email protected]>; Silviu Florian Barbulescu
> > > > > <[email protected]>; Iulia Tanasescu <[email protected]>;
> > > > > Andrei Istodorescu <[email protected]>
> > > > > Subject: [EXT] Re: [PATCH BlueZ 1/1] client/player: Fix transport.send
> > > > > command's transfer of packets
> > > > >
> > > > > 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 Vlad,
> > > > >
> > > > > On Mon, Mar 25, 2024 at 10:40 AM Vlad Pruteanu
> > > > > <[email protected]> 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 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.
> > > > >
> > > > > Please add some output samples showing the wrong interval ends up being
> > > > > used.
> > > > >
> > > > Below you can find what I consider to a relevant excerpt taken from btmon log:
> > > > ...
> > > > < HCI Command: LE Set Connected Isochronous Group Parameters (0x08|0x0062)
> > > > CIG ID: 0x00
> > > > Central to Peripheral SDU Interval: 10000 us (0x002710)
> > > > Peripheral to Central SDU Interval: 10000 us (0x002710)
> > > > SCA: 201 - 500 ppm (0x00)
> > > > Packing: Sequential (0x00)
> > > > Framing: Unframed (0x00)
> > > > Central to Peripheral Maximum Latency: 10 ms (0x000a)
> > > > Peripheral to Central Maximum Latency: 10 ms (0x000a)
> > > > Number of CIS: 1
> > > > CIS ID: 0x00
> > > > Central to Peripheral Maximum SDU Size: 40
> > > > Peripheral to Central Maximum SDU Size: 0
> > > > Central to Peripheral PHY: LE 2M (0x02)
> > > > Peripheral to Central PHY: LE 2M (0x02)
> > > > Central to Peripheral Retransmission attempts: 0x02
> > > > Peripheral to Central Retransmission attempts: 0x00
> > > > ...
> > > > > HCI Event: Command Complete (0x0e)
> > > > LE Set Connected Isochronous Group Parameters (0x08|0x0062) ncmd 1
> > > > Status: Success (0x00)
> > > > CIG ID: 0x00
> > > > Number of Handles: 1
> > > > Connection Handle #0: 144
> > > > ...
> > > > < HCI Command: LE Create Connected Isochronous Stream (0x08|0x0064)
> > > > Number of CIS: 1
> > > > CIS Handle: 144
> > > > ACL Handle: 128
> > > > ...
> > > > > HCI Event: LE Meta Event (0x3e)
> > > > LE Connected Isochronous Stream Established (0x19)
> > > > Status: Success (0x00)
> > > > Connection Handle: 144
> > > > CIG Synchronization Delay: 1992 us (0x0007c8)
> > > > CIS Synchronization Delay: 1992 us (0x0007c8)
> > > > Central to Peripheral Latency: 1992 us (0x0007c8)
> > > > Peripheral to Central Latency: 1992 us (0x0007c8)
> > > > Central to Peripheral PHY: LE 2M (0x02)
> > > > Peripheral to Central PHY: LE 2M (0x02)
> > > > Number of Subevents: 3
> > > > Central to Peripheral Burst Number: 1
> > > > Peripheral to Central Burst Number: 0
> > > > Central to Peripheral Flush Timeout: 1
> > > > Peripheral to Central Flush Timeout: 1
> > > > Central to Peripheral MTU: 40
> > > > Peripheral to Central MTU: 0
> > > > ISO Interval: 10.00 msec (0x0008)
> > > > ...
> > > > bluetoothd[349393]: < ISO Data TX: Handle 144 flags 0x02
> > > > > HCI Event: Number of Completed Packets (0x13)
> > > > Num handles: 1
> > > > Handle: 144 Address: A0:CD:F3:78:04:0A (Murata Manufacturing Co., Ltd.)
> > > > Count: 1
> > > > #470: len 44 (27 Kb/s)
> > > > Latency: 13 msec (13-13 msec ~13 msec)
> > > >
> > > > The Maximum Latency is correctly set to 10ms in the LE Set Connected Isochronous Group Parameters command, and the controller reports the final latency as being 1992 us. This is the value that (after conversion to ms) qos.ucast.out.latency takes in the current implementation. num is defined as num = (qos.ucast.out.latency * 1000 / qos.ucast.out.interval), with interval being the ISO interval, 10000us and therefore num will be 0 in this case. As you can see the first packet is always sent, but after that none can be seen. Instead, the Host should send 1 packet every time this function is called. The timer related change below ensures that this function is triggered at every SDU interval. So, in the end, the Host will send 1 packet every SDU interval.
> > > > > > ---
> > > > > > 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;
> > > > >
> > > > > 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?
> > > > >
> > > > The main change that I made here is the value of the interval for the timer.
> > > > Before, it used to be set to qos.ucast.out.latency and I propose to change
> > > > it to qos.ucast.out.interval, so that the Host will send 1SDU packet on
> > > > every ISO Interval. I included the ucast/bcast qos update since I was
> > > > already updating this line of code. That is, I didn't want to send the patch
> > > > with "qos->ucast.out.interval" without differentiating between bcast and
> > > > ucast since it would've been wrong.
> > >
> > > Hmm if I recall correctly this code used to be sending a packet per
> > > interval but we run into problems because the interval is rather short
> > > and OS itself would take time to transfer the packet from userspace to
> > > socket and then to driver, etc, so I suggest we round up the value if
> > > it ends up being 0.
> >
> > ISO_Interval is not necessarily equal to the SDU_Interval, for unframed
> > it may be an integer multiple of it, and for framed I guess it can be
> > incommensurate. See Core v5.4 Vol 6 Part G Sec 2.
> >
> > There might be similar confusion also in the
> > MediaTransport.QoS.Interval property, which IIRC for Client is
> > SDU_Interval and for Server is ISO_Interval.
> >
> > ***
> >
> > The code here probably should not use ISO_Interval for anything, since
> > this is Host -> ISOAL data flow.
> >
> > Instead, IIUC, it should determine SDU_Interval from the codec
> > parameters and for LC3 it is the frame duration. Then it should (on
> > average) send one SDU per SDU_Interval.
> >
> > Or, send N*SDU every N*SDU_Interval for some N>=1. But since it's using
> > timerfd intervals here for the timing, I'm not that sure it is really
> > necessary as the average data rate is then maintained even if some
> > wakeups are late and the controller flow control should smooth it out.
>
> Well afaik there is no flow control when using unframed, so the
> controller will not smooth anything it will just take whatever there
> is in its buffers and send each at its own interval, so if we don't
> want to risk being late and thus cause no data at each interval we
> better pre-load them, which is what we have been doing since the first
> set of packets is sent immediately, after establishing the connection,
> and then we send at each latency, we could perhaps just send on
> POLLOUT though so we use the Number of Complete Packets aka. TX
> Complete to control the flow that way we don't have to do any timing
> calculations to know when we need to send the next packet, the problem
> is that normally POLLOUT is generated when there are socket buffers
> not really controller buffers, so afaik we would be buffering a lot
> more than expected.
So the flow control behavior observed is that if a packet is late,
controller does not drop it but uses it for the next interval. Each
miss then increases the latency until you reach a value where it's
larger than your userspace jitter, and you no longer should run out.
I don't see sending N*SDU each N*SDU_Interval changing this: after
N*SDU_Interval you are in a situation where socket/controller queue is
again empty, and late wakeup causes a miss.
To preload, I think you'd need to send M > N packets on the first timer
round, and then you'd have M-N packets of latency buffer in the queues.
>
> > >
> > > > > > + 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
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Luiz Augusto von Dentz
> > >
> > >
> > >
> >
>
>