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 <vlad.pruteanu@xxxxxxx> wrote:
> >
> > Hello Luiz,
> >
> >
> > > -----Original Message-----
> > > From: Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>
> > > Sent: Monday, March 25, 2024 6:04 PM
> > > To: Vlad Pruteanu <vlad.pruteanu@xxxxxxx>
> > > Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Mihai-Octavian Urzica <mihai-
> > > octavian.urzica@xxxxxxx>; Silviu Florian Barbulescu
> > > <silviu.barbulescu@xxxxxxx>; Iulia Tanasescu <iulia.tanasescu@xxxxxxx>;
> > > Andrei Istodorescu <andrei.istodorescu@xxxxxxx>
> > > 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
> > > <vlad.pruteanu@xxxxxxx> 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
>
>
>
