diff mbox series

[BlueZ,3/3] sco-tester: check sent SCO data is received at bthost

Message ID 7cc0c544b498337e6591610be16e9361687e0196.1741190102.git.pav@iki.fi (mailing list archive)
State Accepted
Commit 87d4f95dc31208863b93d22fd035f049297a10ca
Headers show
Series [BlueZ,1/3] btdev: pass sent SCO data to bthost | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success

Commit Message

Pauli Virtanen March 5, 2025, 3:58 p.m. UTC 
When sending data, also check that the data is received by bthost.
---
 tools/sco-tester.c | 47 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 45 insertions(+), 2 deletions(-)

Comments

Luiz Augusto von Dentz March 5, 2025, 6:58 p.m. UTC | #1 
Hi Pauli,

On Wed, Mar 5, 2025 at 10:58 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> When sending data, also check that the data is received by bthost.
> ---
>  tools/sco-tester.c | 47 ++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 45 insertions(+), 2 deletions(-)
>
> diff --git a/tools/sco-tester.c b/tools/sco-tester.c
> index 7f37ca5cf..9886481ff 100644
> --- a/tools/sco-tester.c
> +++ b/tools/sco-tester.c
> @@ -318,10 +318,51 @@ static void client_connectable_complete(uint16_t opcode, uint8_t status,
>                 tester_setup_complete();
>  }
>
> +static void bthost_recv_data(const void *buf, uint16_t len, uint8_t status,
> +                                                               void *user_data)
> +{
> +       struct test_data *data = user_data;
> +       const struct sco_client_data *scodata = data->test_data;

I had to add the following change in order to pass with these changes:

+       /* Ignore empty packet as that is used to confirm NOCP is being
+        * generated.
+        */
+       if (!len)
+               return;
+

Now I wonder if this may be a problem since it does show up as a data
packet even though it is empty, I guess for the purpose of HFP
streaming it shouldn't cause problems but if someone attempts to do
packet based checks like this it may stop working.

> +       --data->step;
> +
> +       tester_print("Client received %u bytes of data", len);
> +
> +       if (scodata->send_data && (scodata->data_len != len ||
> +                       memcmp(scodata->send_data, buf, len)))
> +               tester_test_failed();
> +       else if (!data->step)
> +               tester_test_passed();
> +}
> +
> +static void bthost_sco_disconnected(void *user_data)
> +{
> +       struct test_data *data = user_data;
> +
> +       tester_print("SCO handle 0x%04x disconnected", data->handle);
> +
> +       data->handle = 0x0000;
> +}
> +
> +static void sco_new_conn(uint16_t handle, void *user_data)
> +{
> +       struct test_data *data = user_data;
> +       struct bthost *host;
> +
> +       tester_print("New client connection with handle 0x%04x", handle);
> +
> +       data->handle = handle;
> +
> +       host = hciemu_client_get_host(data->hciemu);
> +       bthost_add_sco_hook(host, data->handle, bthost_recv_data, data,
> +                               bthost_sco_disconnected);
> +}
> +
>  static void setup_powered_callback(uint8_t status, uint16_t length,
>                                         const void *param, void *user_data)
>  {
>         struct test_data *data = tester_get_data();
> +       const struct sco_client_data *scodata = data->test_data;
>         struct bthost *bthost;
>
>         if (status != MGMT_STATUS_SUCCESS) {
> @@ -334,6 +375,9 @@ static void setup_powered_callback(uint8_t status, uint16_t length,
>         bthost = hciemu_client_get_host(data->hciemu);
>         bthost_set_cmd_complete_cb(bthost, client_connectable_complete, data);
>         bthost_write_scan_enable(bthost, 0x03);
> +
> +       if (scodata && scodata->send_data)
> +               bthost_set_sco_cb(bthost, sco_new_conn, data);
>  }
>
>  static void setup_powered(const void *test_data)
> @@ -740,8 +784,6 @@ static gboolean sco_connect_cb(GIOChannel *io, GIOCondition cond,
>                 ssize_t ret = 0;
>                 unsigned int count;
>
> -               data->step = 0;
> -
>                 sco_tx_timestamping(data, io);
>
>                 tester_print("Writing %u*%u bytes of data",
> @@ -751,6 +793,7 @@ static gboolean sco_connect_cb(GIOChannel *io, GIOCondition cond,
>                         ret = write(sk, scodata->send_data, scodata->data_len);
>                         if (scodata->data_len != ret)
>                                 break;
> +                       data->step++;
>                 }
>                 if (scodata->data_len != ret) {
>                         tester_warn("Failed to write %u bytes: %zu %s (%d)",
> --
> 2.48.1
>
>
Pauli Virtanen March 6, 2025, 7:37 p.m. UTC | #2 
Hi,

ke, 2025-03-05 kello 13:58 -0500, Luiz Augusto von Dentz kirjoitti:
> On Wed, Mar 5, 2025 at 10:58 AM Pauli Virtanen <pav@iki.fi> wrote:
> > 
> > When sending data, also check that the data is received by bthost.
> > ---
> >  tools/sco-tester.c | 47 ++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 45 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/sco-tester.c b/tools/sco-tester.c
> > index 7f37ca5cf..9886481ff 100644
> > --- a/tools/sco-tester.c
> > +++ b/tools/sco-tester.c
> > @@ -318,10 +318,51 @@ static void client_connectable_complete(uint16_t opcode, uint8_t status,
> >                 tester_setup_complete();
> >  }
> > 
> > +static void bthost_recv_data(const void *buf, uint16_t len, uint8_t status,
> > +                                                               void *user_data)
> > +{
> > +       struct test_data *data = user_data;
> > +       const struct sco_client_data *scodata = data->test_data;
> 
> I had to add the following change in order to pass with these changes:
> 
> +       /* Ignore empty packet as that is used to confirm NOCP is being
> +        * generated.
> +        */
> +       if (!len)
> +               return;
> +
> 
> Now I wonder if this may be a problem since it does show up as a data
> packet even though it is empty, I guess for the purpose of HFP
> streaming it shouldn't cause problems but if someone attempts to do
> packet based checks like this it may stop working.

It's probably a change in behavior if the controller interprets the
zero-length packet as an instruction to send zero-filled SCO packet
(with valid CRC etc) over the air. I'm not sure what the controllers do
here.

The timer causes a small hiccup at startup, but that is only once per
controller index added. Since then the controller is not spec-
compliant, maybe it is OK and there could be a quirk...

> 
> > +       --data->step;
> > +
> > +       tester_print("Client received %u bytes of data", len);
> > +
> > +       if (scodata->send_data && (scodata->data_len != len ||
> > +                       memcmp(scodata->send_data, buf, len)))
> > +               tester_test_failed();
> > +       else if (!data->step)
> > +               tester_test_passed();
> > +}
> > +
> > +static void bthost_sco_disconnected(void *user_data)
> > +{
> > +       struct test_data *data = user_data;
> > +
> > +       tester_print("SCO handle 0x%04x disconnected", data->handle);
> > +
> > +       data->handle = 0x0000;
> > +}
> > +
> > +static void sco_new_conn(uint16_t handle, void *user_data)
> > +{
> > +       struct test_data *data = user_data;
> > +       struct bthost *host;
> > +
> > +       tester_print("New client connection with handle 0x%04x", handle);
> > +
> > +       data->handle = handle;
> > +
> > +       host = hciemu_client_get_host(data->hciemu);
> > +       bthost_add_sco_hook(host, data->handle, bthost_recv_data, data,
> > +                               bthost_sco_disconnected);
> > +}
> > +
> >  static void setup_powered_callback(uint8_t status, uint16_t length,
> >                                         const void *param, void *user_data)
> >  {
> >         struct test_data *data = tester_get_data();
> > +       const struct sco_client_data *scodata = data->test_data;
> >         struct bthost *bthost;
> > 
> >         if (status != MGMT_STATUS_SUCCESS) {
> > @@ -334,6 +375,9 @@ static void setup_powered_callback(uint8_t status, uint16_t length,
> >         bthost = hciemu_client_get_host(data->hciemu);
> >         bthost_set_cmd_complete_cb(bthost, client_connectable_complete, data);
> >         bthost_write_scan_enable(bthost, 0x03);
> > +
> > +       if (scodata && scodata->send_data)
> > +               bthost_set_sco_cb(bthost, sco_new_conn, data);
> >  }
> > 
> >  static void setup_powered(const void *test_data)
> > @@ -740,8 +784,6 @@ static gboolean sco_connect_cb(GIOChannel *io, GIOCondition cond,
> >                 ssize_t ret = 0;
> >                 unsigned int count;
> > 
> > -               data->step = 0;
> > -
> >                 sco_tx_timestamping(data, io);
> > 
> >                 tester_print("Writing %u*%u bytes of data",
> > @@ -751,6 +793,7 @@ static gboolean sco_connect_cb(GIOChannel *io, GIOCondition cond,
> >                         ret = write(sk, scodata->send_data, scodata->data_len);
> >                         if (scodata->data_len != ret)
> >                                 break;
> > +                       data->step++;
> >                 }
> >                 if (scodata->data_len != ret) {
> >                         tester_warn("Failed to write %u bytes: %zu %s (%d)",
> > --
> > 2.48.1
> > 
> > 
> 
>
Luiz Augusto von Dentz March 6, 2025, 8:32 p.m. UTC | #3 
Hi Pauli,

On Thu, Mar 6, 2025 at 2:37 PM Pauli Virtanen <pav@iki.fi> wrote:
>
> Hi,
>
> ke, 2025-03-05 kello 13:58 -0500, Luiz Augusto von Dentz kirjoitti:
> > On Wed, Mar 5, 2025 at 10:58 AM Pauli Virtanen <pav@iki.fi> wrote:
> > >
> > > When sending data, also check that the data is received by bthost.
> > > ---
> > >  tools/sco-tester.c | 47 ++++++++++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 45 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tools/sco-tester.c b/tools/sco-tester.c
> > > index 7f37ca5cf..9886481ff 100644
> > > --- a/tools/sco-tester.c
> > > +++ b/tools/sco-tester.c
> > > @@ -318,10 +318,51 @@ static void client_connectable_complete(uint16_t opcode, uint8_t status,
> > >                 tester_setup_complete();
> > >  }
> > >
> > > +static void bthost_recv_data(const void *buf, uint16_t len, uint8_t status,
> > > +                                                               void *user_data)
> > > +{
> > > +       struct test_data *data = user_data;
> > > +       const struct sco_client_data *scodata = data->test_data;
> >
> > I had to add the following change in order to pass with these changes:
> >
> > +       /* Ignore empty packet as that is used to confirm NOCP is being
> > +        * generated.
> > +        */
> > +       if (!len)
> > +               return;
> > +
> >
> > Now I wonder if this may be a problem since it does show up as a data
> > packet even though it is empty, I guess for the purpose of HFP
> > streaming it shouldn't cause problems but if someone attempts to do
> > packet based checks like this it may stop working.
>
> It's probably a change in behavior if the controller interprets the
> zero-length packet as an instruction to send zero-filled SCO packet
> (with valid CRC etc) over the air. I'm not sure what the controllers do
> here.
>
> The timer causes a small hiccup at startup, but that is only once per
> controller index added. Since then the controller is not spec-
> compliant, maybe it is OK and there could be a quirk...

Yeah, that said we could perhaps add a quirk to mark Sync Flow Control
as supported, rather than adding one for saying it is broken, that way
we can enable it only in the controllers that are known to work
otherwise there is always a chance that controllers don't behave as
expected.

> >
> > > +       --data->step;
> > > +
> > > +       tester_print("Client received %u bytes of data", len);
> > > +
> > > +       if (scodata->send_data && (scodata->data_len != len ||
> > > +                       memcmp(scodata->send_data, buf, len)))
> > > +               tester_test_failed();
> > > +       else if (!data->step)
> > > +               tester_test_passed();
> > > +}
> > > +
> > > +static void bthost_sco_disconnected(void *user_data)
> > > +{
> > > +       struct test_data *data = user_data;
> > > +
> > > +       tester_print("SCO handle 0x%04x disconnected", data->handle);
> > > +
> > > +       data->handle = 0x0000;
> > > +}
> > > +
> > > +static void sco_new_conn(uint16_t handle, void *user_data)
> > > +{
> > > +       struct test_data *data = user_data;
> > > +       struct bthost *host;
> > > +
> > > +       tester_print("New client connection with handle 0x%04x", handle);
> > > +
> > > +       data->handle = handle;
> > > +
> > > +       host = hciemu_client_get_host(data->hciemu);
> > > +       bthost_add_sco_hook(host, data->handle, bthost_recv_data, data,
> > > +                               bthost_sco_disconnected);
> > > +}
> > > +
> > >  static void setup_powered_callback(uint8_t status, uint16_t length,
> > >                                         const void *param, void *user_data)
> > >  {
> > >         struct test_data *data = tester_get_data();
> > > +       const struct sco_client_data *scodata = data->test_data;
> > >         struct bthost *bthost;
> > >
> > >         if (status != MGMT_STATUS_SUCCESS) {
> > > @@ -334,6 +375,9 @@ static void setup_powered_callback(uint8_t status, uint16_t length,
> > >         bthost = hciemu_client_get_host(data->hciemu);
> > >         bthost_set_cmd_complete_cb(bthost, client_connectable_complete, data);
> > >         bthost_write_scan_enable(bthost, 0x03);
> > > +
> > > +       if (scodata && scodata->send_data)
> > > +               bthost_set_sco_cb(bthost, sco_new_conn, data);
> > >  }
> > >
> > >  static void setup_powered(const void *test_data)
> > > @@ -740,8 +784,6 @@ static gboolean sco_connect_cb(GIOChannel *io, GIOCondition cond,
> > >                 ssize_t ret = 0;
> > >                 unsigned int count;
> > >
> > > -               data->step = 0;
> > > -
> > >                 sco_tx_timestamping(data, io);
> > >
> > >                 tester_print("Writing %u*%u bytes of data",
> > > @@ -751,6 +793,7 @@ static gboolean sco_connect_cb(GIOChannel *io, GIOCondition cond,
> > >                         ret = write(sk, scodata->send_data, scodata->data_len);
> > >                         if (scodata->data_len != ret)
> > >                                 break;
> > > +                       data->step++;
> > >                 }
> > >                 if (scodata->data_len != ret) {
> > >                         tester_warn("Failed to write %u bytes: %zu %s (%d)",
> > > --
> > > 2.48.1
> > >
> > >
> >
> >
>
diff mbox series

Patch

diff --git a/tools/sco-tester.c b/tools/sco-tester.c
index 7f37ca5cf..9886481ff 100644
--- a/tools/sco-tester.c
+++ b/tools/sco-tester.c
@@ -318,10 +318,51 @@  static void client_connectable_complete(uint16_t opcode, uint8_t status,
 		tester_setup_complete();
 }
 
+static void bthost_recv_data(const void *buf, uint16_t len, uint8_t status,
+								void *user_data)
+{
+	struct test_data *data = user_data;
+	const struct sco_client_data *scodata = data->test_data;
+
+	--data->step;
+
+	tester_print("Client received %u bytes of data", len);
+
+	if (scodata->send_data && (scodata->data_len != len ||
+			memcmp(scodata->send_data, buf, len)))
+		tester_test_failed();
+	else if (!data->step)
+		tester_test_passed();
+}
+
+static void bthost_sco_disconnected(void *user_data)
+{
+	struct test_data *data = user_data;
+
+	tester_print("SCO handle 0x%04x disconnected", data->handle);
+
+	data->handle = 0x0000;
+}
+
+static void sco_new_conn(uint16_t handle, void *user_data)
+{
+	struct test_data *data = user_data;
+	struct bthost *host;
+
+	tester_print("New client connection with handle 0x%04x", handle);
+
+	data->handle = handle;
+
+	host = hciemu_client_get_host(data->hciemu);
+	bthost_add_sco_hook(host, data->handle, bthost_recv_data, data,
+				bthost_sco_disconnected);
+}
+
 static void setup_powered_callback(uint8_t status, uint16_t length,
 					const void *param, void *user_data)
 {
 	struct test_data *data = tester_get_data();
+	const struct sco_client_data *scodata = data->test_data;
 	struct bthost *bthost;
 
 	if (status != MGMT_STATUS_SUCCESS) {
@@ -334,6 +375,9 @@  static void setup_powered_callback(uint8_t status, uint16_t length,
 	bthost = hciemu_client_get_host(data->hciemu);
 	bthost_set_cmd_complete_cb(bthost, client_connectable_complete, data);
 	bthost_write_scan_enable(bthost, 0x03);
+
+	if (scodata && scodata->send_data)
+		bthost_set_sco_cb(bthost, sco_new_conn, data);
 }
 
 static void setup_powered(const void *test_data)
@@ -740,8 +784,6 @@  static gboolean sco_connect_cb(GIOChannel *io, GIOCondition cond,
 		ssize_t ret = 0;
 		unsigned int count;
 
-		data->step = 0;
-
 		sco_tx_timestamping(data, io);
 
 		tester_print("Writing %u*%u bytes of data",
@@ -751,6 +793,7 @@  static gboolean sco_connect_cb(GIOChannel *io, GIOCondition cond,
 			ret = write(sk, scodata->send_data, scodata->data_len);
 			if (scodata->data_len != ret)
 				break;
+			data->step++;
 		}
 		if (scodata->data_len != ret) {
 			tester_warn("Failed to write %u bytes: %zu %s (%d)",