Re: [PATCH BlueZ v1 2/3] client/player: Add interface menu to configure MICP-MICS during PTS testing.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Mahesh,

On Thu, Aug 3, 2023 at 12:20 AM Mahesh Talewad <mahesh.talewad@xxxxxxx> wrote:
>
> - Includes implementations required for PTS testing for MICS and MICP.
> - Interface given in bluetoothctl/player menu for sending MICS and MICP
> commands while excecuting PTS test cases. Tested all MICP and MICS PTS
> test cases[LE] and all are passed.
> - Added flag - MICP_MICS_PTS_FLAG in configure.ac which enable/disable
> PTS testing related code during compilation. uncomment this flag in
> configure.ac inorder to enable PTS testing related code and comment
> it for disable. By default this flag is disabled.

We could perhaps have a more generic solution that the testing
interfaces with a testing flag, which we can perhaps enable at
runtime, that said in the past we took a different approach of having
a dedicated tool for testing protocols (e.g. avtest), but I think I
would rather do this with bluetoothctl nowadays.

> - Spec implementation/PTS testing:
> MICS - MICS_v1.0.pdf
> MICP - MICP_v1.0.pdf
> PTS Testing MICS: MICS.TS.p0ed2.pdf
> PTS Testing MICP: MICP.TS.p3.pdf
> ---
>  client/main.c     |  12 ++++
>  client/player.c   | 164 ++++++++++++++++++++++++++++++++++++++++++++++
>  client/player.h   |   4 ++
>  configure.ac      |   3 +
>  src/adapter.c     |  87 ++++++++++++++++++++++++
>  src/shared/micp.c | 122 ++++++++++++++++++++++++++++++++++
>  src/shared/micp.h |   8 +++
>  7 files changed, 400 insertions(+)
>
> diff --git a/client/main.c b/client/main.c
> index 0eac5bdf5..d7c735d19 100644
> --- a/client/main.c
> +++ b/client/main.c
> @@ -413,6 +413,10 @@ static struct adapter *adapter_new(GDBusProxy *proxy)
>         if (!default_ctrl)
>                 default_ctrl = adapter;
>
> +#ifdef MICP_MICS_PTS_FLAG
> +       mics_set_proxy((void *)adapter);
> +#endif /* MICP_MICS_PTS_FLAG */

At the client side we should probably do a runtime detection, by
checking if a specific interface is supported, rather than depend on a
compilation flag since you may have the client and daemon coming from
different packages.

>         return adapter;
>  }
>
> @@ -892,6 +896,10 @@ static void cmd_show(int argc, char *argv[])
>                 }
>         }
>
> +#ifdef MICP_MICS_PTS_FLAG
> +       mics_set_proxy((void *)adapter);
> +#endif /*MICP_MICS_PTS_FLAG*/
> +
>         if (!g_dbus_proxy_get_property(adapter->proxy, "Address", &iter))
>                 return bt_shell_noninteractive_quit(EXIT_FAILURE);
>
> @@ -951,6 +959,10 @@ static void cmd_select(int argc, char *argv[])
>                 return bt_shell_noninteractive_quit(EXIT_FAILURE);
>         }
>
> +#ifdef MICP_MICS_PTS_FLAG
> +       mics_set_proxy((void *)adapter);
> +#endif /*MICP_MICS_PTS_FLAG*/
> +
>         if (default_ctrl && default_ctrl->proxy == adapter->proxy)
>                 return bt_shell_noninteractive_quit(EXIT_SUCCESS);
>
> diff --git a/client/player.c b/client/player.c
> index e5084967a..2e48025e8 100644
> --- a/client/player.c
> +++ b/client/player.c
> @@ -596,6 +596,153 @@ static void cmd_show_item(int argc, char *argv[])
>         return bt_shell_noninteractive_quit(EXIT_SUCCESS);
>  }
>
> +#ifdef MICP_MICS_PTS_FLAG
> +struct mics_adapter {
> +       GDBusProxy *proxy;
> +};
> +static struct mics_adapter *mics_default_ctrl;
> +void mics_set_proxy(void *proxy)
> +{
> +       mics_default_ctrl = (struct mics_adapter *)proxy;
> +       if (mics_default_ctrl == NULL) {
> +               bt_shell_printf("mics_default_ctrl is NULL\n");
> +               return;
> +       }
> +}
> +static gboolean parse_argument(int argc, char *argv[], const char **arg_table,
> +                                       const char *msg, dbus_bool_t *value,
> +                                       const char **option)
> +{
> +       const char **opt;
> +
> +       if (!strcmp(argv[1], "help")) {
> +               for (opt = arg_table; opt && *opt; opt++)
> +                       bt_shell_printf("%s\n", *opt);
> +               bt_shell_noninteractive_quit(EXIT_SUCCESS);
> +               return FALSE;
> +       }
> +
> +       if (!strcmp(argv[1], "on") || !strcmp(argv[1], "yes")) {
> +               *value = TRUE;
> +               if (option)
> +                       *option = "";
> +               return TRUE;
> +       }
> +
> +       if (!strcmp(argv[1], "off") || !strcmp(argv[1], "no")) {
> +               *value = FALSE;
> +               return TRUE;
> +       }
> +
> +       for (opt = arg_table; opt && *opt; opt++) {
> +               if (strcmp(argv[1], *opt) == 0) {
> +                       *value = TRUE;
> +                       *option = *opt;
> +                       return TRUE;
> +               }
> +       }
> +
> +       bt_shell_printf("Invalid argument %s\n", argv[1]);
> +       return FALSE;
> +}
> +
> +static void cmd_set_mute_state(int argc, char *argv[])
> +{
> +       dbus_bool_t mute_state;
> +       char *str;
> +
> +       if (!parse_argument(argc, argv, NULL, NULL, &mute_state, NULL))
> +               return bt_shell_noninteractive_quit(EXIT_FAILURE);
> +
> +       str = g_strdup_printf("mics %s", mute_state == TRUE ? "on" : "off");
> +
> +       if (g_dbus_proxy_set_property_basic(mics_default_ctrl->proxy, "mics",
> +                                       DBUS_TYPE_BOOLEAN, &mute_state,
> +                                       generic_callback, str, g_free) == TRUE)
> +               return;
> +       g_free(str);
> +
> +       return bt_shell_noninteractive_quit(EXIT_FAILURE);
> +}
> +
> +static void cmd_enable_disable_mute_state(int argc, char *argv[])
> +{
> +       dbus_bool_t mute_state;
> +       char *str;
> +
> +       if (!parse_argument(argc, argv, NULL, NULL, &mute_state, NULL))
> +               return bt_shell_noninteractive_quit(EXIT_FAILURE);
> +
> +       str = g_strdup_printf("mics %s", mute_state == TRUE ? "on" : "off");
> +
> +       if (g_dbus_proxy_set_property_basic(mics_default_ctrl->proxy,
> +                               "mics_state", DBUS_TYPE_BOOLEAN, &mute_state,
> +                               generic_callback, str, g_free) == TRUE)
> +               return;
> +       g_free(str);
> +
> +       return bt_shell_noninteractive_quit(EXIT_FAILURE);
> +}
> +
> +static void cmd_micp_discover_mute(int argc, char *argv[])
> +{
> +       dbus_bool_t mute_state = 0;
> +       char *str;
> +
> +
> +       if (!parse_argument(argc, argv, NULL, NULL, &mute_state, NULL))
> +               return bt_shell_noninteractive_quit(EXIT_FAILURE);
> +
> +       str = g_strdup_printf("mics %s", mute_state == TRUE ? "on" : "off");
> +
> +       if (g_dbus_proxy_set_property_basic(mics_default_ctrl->proxy,
> +                               "micp_disc", DBUS_TYPE_BOOLEAN, &mute_state,
> +                               generic_callback, str, g_free) == TRUE)
> +               return;
> +       g_free(str);
> +
> +       return bt_shell_noninteractive_quit(EXIT_FAILURE);
> +}
> +
> +static void cmd_enable_read_mute_state(int argc, char *argv[])
> +{
> +       char *endptr = NULL;
> +       int handle;
> +
> +       handle = strtol(argv[1], &endptr, 0);
> +       if (!endptr || *endptr != '\0' || handle > UINT16_MAX) {
> +               bt_shell_printf("Invalid argument: %s\n", argv[1]);
> +               return bt_shell_noninteractive_quit(EXIT_FAILURE);
> +       }
> +       bt_shell_printf("%s: %x\n", __func__, handle);
> +       if (g_dbus_proxy_set_property_basic(mics_default_ctrl->proxy,
> +                               "micp_read_char", DBUS_TYPE_UINT16, &handle,
> +                               generic_callback, NULL, NULL) == TRUE)
> +               return;
> +
> +       return bt_shell_noninteractive_quit(EXIT_FAILURE);
> +}
> +
> +static void cmd_enable_write_mute_state(int argc, char *argv[])
> +{
> +       char *endptr = NULL;
> +       int handle;
> +
> +       handle = strtol(argv[1], &endptr, 0);
> +       if (!endptr || *endptr != '\0' || handle > UINT16_MAX) {
> +               bt_shell_printf("Invalid argument: %s\n", argv[1]);
> +               return bt_shell_noninteractive_quit(EXIT_FAILURE);
> +       }
> +       bt_shell_printf("%s : %x\n", __func__, handle);
> +       if (g_dbus_proxy_set_property_basic(mics_default_ctrl->proxy,
> +                               "micp_write_char", DBUS_TYPE_UINT16, &handle,
> +                               generic_callback, NULL, NULL) == TRUE)
> +               return;
> +
> +       return bt_shell_noninteractive_quit(EXIT_FAILURE);
> +}
> +#endif /* MICP_MICS_PTS_FLAG */
> +
>  static void cmd_show(int argc, char *argv[])
>  {
>         GDBusProxy *proxy;
> @@ -969,6 +1116,23 @@ static const struct bt_shell_menu player_menu = {
>                                                         item_generator},
>         { "show-item",   "<item>",    cmd_show_item, "Show item information",
>                                                         item_generator},
> +#ifdef MICP_MICS_PTS_FLAG
> +       { "mics_mute",     "<on/off>", cmd_set_mute_state,
> +                                       "Set Mics Mute state to on / off",
> +                                                       NULL },
> +       { "mics_state",     "<on/off>", cmd_enable_disable_mute_state,
> +                                       "Set Mics Mute state to on[enable] / off[disable]",
> +                                                       NULL },
> +       { "micp_discover",     "<on/off>", cmd_micp_discover_mute,
> +                                       "discover Mute Characteristic",
> +                                                       NULL },
> +       { "micp_read",     "<handle>", cmd_enable_read_mute_state,
> +                                       "Read Mute Characteristic",
> +                                                       NULL },
> +       { "micp_write",     "<handle>", cmd_enable_write_mute_state,
> +                                       "Write Mute Characteristic",
> +                                                       NULL },
> +#endif /* MICP_MICS_PTS_FLAG */
>         {} },
>  };
>
> diff --git a/client/player.h b/client/player.h
> index e7778cb1e..316090721 100644
> --- a/client/player.h
> +++ b/client/player.h
> @@ -10,3 +10,7 @@
>
>  void player_add_submenu(void);
>  void player_remove_submenu(void);
> +
> +#ifdef MICP_MICS_PTS_FLAG
> +void mics_set_proxy(void *proxy);
> +#endif /*MICP_MICS_PTS_FLAG*/
> diff --git a/configure.ac b/configure.ac
> index 9a8856380..a190d9168 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -215,6 +215,9 @@ AC_ARG_ENABLE(micp, AS_HELP_STRING([--disable-micp],
>                 [disable MICP profile]), [enable_micp=${enableval}])
>  AM_CONDITIONAL(MICP, test "${enable_micp}" != "no")
>
> +#AC_DEFINE(MICP_MICS_PTS_FLAG, 1,
> +#      [Enable/Disable PTS related code changes in MICP and MICS])
> +
>  AC_ARG_ENABLE(csip, AS_HELP_STRING([--disable-csip],
>                 [disable CSIP profile]), [enable_csip=${enableval}])
>  AM_CONDITIONAL(CSIP, test "${enable_csip}" != "no")
> diff --git a/src/adapter.c b/src/adapter.c
> index 2679d4302..89f6d76f4 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -68,6 +68,10 @@
>  #include "eir.h"
>  #include "battery.h"
>
> +#ifdef MICP_MICS_PTS_FLAG
> +#include "src/shared/micp.h"
> +#endif /*MICP_MICS_PTS_FLAG*/
> +
>  #define MODE_OFF               0x00
>  #define MODE_CONNECTABLE       0x01
>  #define MODE_DISCOVERABLE      0x02
> @@ -3333,6 +3337,82 @@ static void property_set_pairable(const GDBusPropertyTable *property,
>         property_set_mode(adapter, MGMT_SETTING_BONDABLE, iter, id);
>  }
>
> +#ifdef MICP_MICS_PTS_FLAG

The code below definitely _doesn't_ belong to the daemon core.

> +static void property_set_mute_state(const GDBusPropertyTable *property,
> +                               DBusMessageIter *iter,
> +                               GDBusPendingPropertySet id, void *user_data)
> +{
> +       dbus_bool_t enable;
> +
> +       dbus_message_iter_get_basic(iter, &enable);
> +       DBG("SET %s: %d\n", __func__, enable);
> +       mics_change_mute_state(enable);
> +       g_dbus_pending_property_success(id);
> +}
> +
> +static void property_mute_enable_disable(const GDBusPropertyTable *propert,
> +                               DBusMessageIter *iter,
> +                               GDBusPendingPropertySet id, void *user_data)
> +{
> +       dbus_bool_t enable;
> +
> +       dbus_message_iter_get_basic(iter, &enable);
> +       DBG("%s: %d\n", __func__, enable);
> +       mics_enable_disable_mute(enable);
> +       g_dbus_pending_property_success(id);
> +}
> +
> +static void property_micp_discover_mute(const GDBusPropertyTable *propert,
> +                               DBusMessageIter *iter,
> +                               GDBusPendingPropertySet id, void *user_data)
> +{
> +       dbus_bool_t enable;
> +
> +       dbus_message_iter_get_basic(iter, &enable);
> +       DBG("%s : %d\n", __func__, enable);
> +       micp_discover_mute_char();
> +       g_dbus_pending_property_success(id);
> +}
> +
> +static void property_micp_read_mute(const GDBusPropertyTable *propert,
> +                               DBusMessageIter *iter,
> +                               GDBusPendingPropertySet id, void *user_data)
> +{
> +       uint16_t handle;
> +
> +       if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_UINT16) {
> +               g_dbus_pending_property_error(id,
> +                               ERROR_INTERFACE ".InvalidArguments",
> +                               "Expected UINT16");
> +               return;
> +       }
> +       dbus_message_iter_get_basic(iter, &handle);
> +       DBG("%s : %x\n", __func__, handle);
> +
> +       mics_mute_char_read(handle);
> +       g_dbus_pending_property_success(id);
> +}
> +
> +static void property_micp_write_mute(const GDBusPropertyTable *propert,
> +                               DBusMessageIter *iter,
> +                               GDBusPendingPropertySet id, void *user_data)
> +{
> +       uint16_t handle;
> +
> +       if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_UINT16) {
> +               g_dbus_pending_property_error(id,
> +                               ERROR_INTERFACE ".InvalidArguments",
> +                               "Expected UINT16");
> +               return;
> +       }
> +       dbus_message_iter_get_basic(iter, &handle);
> +       DBG("%s : %x\n", __func__, handle);
> +
> +       micp_char_write_value(handle);
> +       g_dbus_pending_property_success(id);
> +}
> +#endif /*MICP_MICS_PTS_FLAG*/
> +
>  static gboolean property_get_pairable_timeout(
>                                         const GDBusPropertyTable *property,
>                                         DBusMessageIter *iter, void *user_data)
> @@ -3886,6 +3966,13 @@ static const GDBusPropertyTable adapter_properties[] = {
>         { "DiscoverableTimeout", "u", property_get_discoverable_timeout,
>                                         property_set_discoverable_timeout },
>         { "Pairable", "b", property_get_pairable, property_set_pairable },
> +#ifdef MICP_MICS_PTS_FLAG
> +       { "mics", "b", NULL, property_set_mute_state },
> +       { "mics_state", "b", NULL, property_mute_enable_disable },
> +       { "micp_disc", "b", NULL, property_micp_discover_mute },
> +       { "micp_read_char", "q", NULL, property_micp_read_mute },
> +       { "micp_write_char", "q", NULL, property_micp_write_mute },

Perhaps rather than doing this via profile specific interface we could
check the runtime flag and expose the adapter GATT database, then at
bluetoothctl one could select the attributes, since you probably are
doing this to enable testing the procedures without having to depend
on things like MediaTransport.Volume which depends on BAP to setup a
stream first which I guess PTS doesn't utilize currently?

Anyway, tapping on GATT procedures directly shall cover all testing
when the upper profile uses GATT procedures, that way we don't have to
keep adding new interfaces for testing with PTS, anyway I would
recommend you split these changes to a different set after we have
done merging the initial code for MICP.

> +#endif /*MICP_MICS_PTS_FLAG*/
>         { "PairableTimeout", "u", property_get_pairable_timeout,
>                                         property_set_pairable_timeout },
>         { "Discovering", "b", property_get_discovering },
> diff --git a/src/shared/micp.c b/src/shared/micp.c
> index 25ffa6940..c5b814d98 100644
> --- a/src/shared/micp.c
> +++ b/src/shared/micp.c
> @@ -6,6 +6,10 @@
>   *  Copyright (C) 2023  NXP Semiconductors. All rights reserved.
>   *
>   */
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
>  #define _GNU_SOURCE
>  #include <inttypes.h>
>  #include <string.h>
> @@ -74,6 +78,11 @@ struct bt_micp {
>         void *user_data;
>  };
>
> +#ifdef MICP_MICS_PTS_FLAG
> +struct bt_mics *pts_mics;
> +struct bt_micp *pts_micp;
> +#endif /*MICP_MICS_PTS_FLAG*/
> +
>  static struct queue *micp_db;
>  static struct queue *micp_cbs;
>  static struct queue *sessions;
> @@ -532,6 +541,9 @@ static struct bt_micp_db *micp_db_new(struct gatt_db *db)
>         mdb->mics = mics_new(db);
>         mdb->mics->mdb = mdb;
>
> +#ifdef MICP_MICS_PTS_FLAG
> +       pts_mics = mdb->mics;
> +#endif /*MICP_MICS_PTS_FLAG*/
>         queue_push_tail(micp_db, mdb);
>
>         return mdb;
> @@ -783,6 +795,10 @@ static void foreach_mics_char(struct gatt_db_attribute *attr, void *user_data)
>         bt_uuid_t uuid, uuid_mute;
>         struct bt_mics *mics;
>
> +#ifdef MICP_MICS_PTS_FLAG
> +       pts_micp = micp;
> +#endif /*MICP_MICS_PTS_FLAG*/
> +
>         if (!gatt_db_attribute_get_char_data(attr, NULL, &value_handle,
>                         NULL, NULL, &uuid))
>                 return;
> @@ -920,3 +936,109 @@ bool bt_micp_attach(struct bt_micp *micp, struct bt_gatt_client *client)
>                                                 micp);
>         return true;
>  }
> +
> +#ifdef MICP_MICS_PTS_FLAG
> +void mics_change_mute_state(bool state)
> +{
> +       if (pts_micp == NULL)
> +               return;
> +
> +       DBG(pts_micp, "%s: %d", __func__, state);
> +       state == true ? mics_muted(pts_mics, pts_micp, 0) :
> +                                       mics_not_muted(pts_mics, pts_micp, 0);
> +}
> +
> +static uint8_t mics_mute_enable_disable(struct bt_mics *mics, uint8_t state)
> +{
> +       uint8_t *mute_state;
> +
> +       mute_state = mdb_get_mute_state(mics->mdb);
> +
> +       *mute_state = state;
> +
> +       return 0;
> +}
> +
> +void mics_enable_disable_mute(bool state)
> +{
> +       state == true ? mics_mute_enable_disable(pts_mics, MICS_MUTED) :
> +                       mics_mute_enable_disable(pts_mics, MICS_DISABLED);
> +}
> +
> +static void micp_char_search_cb(bool success, uint8_t att_ecode,
> +                                               struct bt_gatt_result *result,
> +                                               void *user_data)
> +{
> +       DBG(pts_micp, "micp_char_search_cb");
> +
> +}
> +
> +static void micp_foreach_mics_service(struct gatt_db_attribute *attr,
> +                                                       void *user_data)
> +{
> +       uint16_t start, end;
> +       bool primary;
> +       bt_uuid_t uuid;
> +       struct bt_gatt_request *gatt_ret;
> +       struct bt_att *micp_att;
> +       struct bt_micp *micp = user_data;
> +       struct bt_mics *mics = micp_get_mics(micp);
> +
> +       if (!gatt_db_attribute_get_service_data(attr, &start, &end, &primary,
> +               &uuid)) {
> +               DBG(micp, "%s: ERR! gatt_db_attribute_get_service_data\n",
> +                       __func__);
> +               return;
> +
> +       }
> +       micp_att = bt_micp_get_att(micp);
> +       gatt_ret = bt_gatt_discover_characteristics(micp_att, start, end,
> +                                       micp_char_search_cb, NULL, NULL);
> +
> +       if (gatt_ret)
> +               DBG(micp, "MICP GATT DISCOVER START\n");
> +       else
> +               DBG(micp, "MICP GATT DISCOVER FAILED\n");
> +
> +       mics->service = attr;
> +
> +       gatt_db_service_set_claimed(attr, true);
> +       gatt_db_service_foreach_char(attr, foreach_mics_char, micp);
> +}
> +
> +void micp_discover_mute_char(void)
> +{
> +       bt_uuid_t uuid;
> +
> +       bt_uuid16_create(&uuid, MICS_UUID);
> +       gatt_db_foreach_service(pts_micp->ldb->db, &uuid,
> +                                       micp_foreach_mics_service, pts_micp);
> +}
> +
> +void mics_mute_char_read(uint16_t handle)
> +{
> +       DBG(pts_micp, "%s. handle: %x\n", __func__, handle);
> +       micp_read_value(pts_micp, handle, read_mute_state, pts_micp);
> +}
> +
> +static void micp_write_cb(bool success, uint8_t att_ecode, void *user_data)
> +{
> +       if (success)
> +               DBG(pts_micp, "MICP Write successful\n");
> +       else
> +               DBG(pts_micp, "\nWrite failed: 0x%02x\n", att_ecode);
> +}
> +
> +void micp_char_write_value(uint16_t handle)
> +{
> +       const uint8_t value = 0x01;
> +
> +       if (!pts_micp->client) {
> +               DBG(pts_micp, "%s: pts_micp->client is NULL", __func__);
> +               return;
> +       }
> +       bt_gatt_client_write_value(pts_micp->client, handle, &value, 0x01,
> +                       micp_write_cb, NULL, NULL);
> +
> +}
> +#endif /*MICP_MICS_PTS_FLAG*/
> diff --git a/src/shared/micp.h b/src/shared/micp.h
> index b307ac9f4..4a9807ed4 100644
> --- a/src/shared/micp.h
> +++ b/src/shared/micp.h
> @@ -44,3 +44,11 @@ bool bt_micp_ready_unregister(struct bt_micp *micp, unsigned int id);
>
>  bool bt_micp_unregister(unsigned int id);
>  struct bt_micp *bt_micp_new(struct gatt_db *ldb, struct gatt_db *rdb);
> +
> +#ifdef MICP_MICS_PTS_FLAG
> +void mics_change_mute_state(bool state);
> +void mics_enable_disable_mute(bool state);
> +void micp_discover_mute_char(void);
> +void mics_mute_char_read(uint16_t handle);
> +void micp_char_write_value(uint16_t handle);
> +#endif /*MICP_MICS_PTS_FLAG*/
> --
> 2.34.1
>


-- 
Luiz Augusto von Dentz




[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux