Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200819121048.BlueZ.v1.1.I3a57ea1eb3e3f5b87abc44ea478fc83817627ffc@changeid>
 <20200819121048.BlueZ.v1.2.I045d6f668c141c6b834ba6b31fc81618c0a7b8e8@changeid>
In-Reply-To: <20200819121048.BlueZ.v1.2.I045d6f668c141c6b834ba6b31fc81618c0a7b8e8@changeid>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Tue, 8 Sep 2020 10:06:06 -0700
Message-ID: <CABBYNZLeqA=i_JdcJUD-Q-6p_8DtMpmmW=gg4yvQrJ77jRG4sw@mail.gmail.com>
Subject: Re: [BlueZ PATCH v1 2/4] client: Implement more interfaces of ADV
 monitor in bluetoothctl
To:     Howard Chung <howardchung@google.com>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        Alain Michaud <alainm@chromium.org>,
        Miao-chen Chou <mcchou@chromium.org>,
        Manish Mandlik <mmandlik@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Howard,

On Tue, Aug 18, 2020 at 9:15 PM Howard Chung <howardchung@google.com> wrote:
>
> This patch creates a submenu in bluetoothctl and implements several
> commands.
>
> new commands:
> [bluetooth]# menu advmon
> [bluetooth]# add-pattern-monitor or_patterns -10,10,-30,5 1,2,ab0011
> Advertisement Monitor 0 added
> [bluetooth]# get-pattern-monitor all
> Advertisement Monitor 0
>         path: /org/bluez/adv_monitor_app/0
>         type: or_patterns
>         rssi:
>                 high threshold: -10
>                 high threshold timer: 10
>                 low threshold: -30
>                 low threshold timer: 5
>         pattern 1:
>                 start position: 1
>                 AD data type: 2
>                 content: ab0011
> [bluetooth]# get-supported-info
> Supported Features:
> Supported Moniter Types: or_patterns
> [bluetooth]# remove-pattern-monitor 0
> Monitor 0 deleted
>
> Signed-off-by: Howard Chung <howardchung@google.com>
> ---
>
>  client/advertisement_monitor.c | 328 ++++++++++++++++++++++++++++++++-
>  client/advertisement_monitor.h |   4 +
>  client/main.c                  |  70 +++++++
>  3 files changed, 399 insertions(+), 3 deletions(-)
>
> diff --git a/client/advertisement_monitor.c b/client/advertisement_monitor.c
> index bd2309537..ec8f23711 100644
> --- a/client/advertisement_monitor.c
> +++ b/client/advertisement_monitor.c
> @@ -29,6 +29,7 @@
>  #include <string.h>
>
>  #include "gdbus/gdbus.h"
> +#include "src/shared/ad.h"
>  #include "src/shared/util.h"
>  #include "src/shared/shell.h"
>  #include "advertisement_monitor.h"
> @@ -36,6 +37,27 @@
>  #define ADV_MONITOR_APP_PATH "/org/bluez/adv_monitor_app"
>  #define ADV_MONITOR_INTERFACE "org.bluez.AdvertisementMonitor1"
>
> +struct rssi_setting {
> +       int16_t high_threshold;
> +       uint16_t high_timer;
> +       int16_t low_threshold;
> +       uint16_t low_timer;
> +};
> +
> +struct pattern {
> +       uint8_t start_pos;
> +       uint8_t ad_data_type;
> +       uint8_t content_len;
> +       uint8_t content[BT_AD_MAX_DATA_LEN];
> +};
> +
> +struct adv_monitor {
> +       uint8_t idx;
> +       char *type;
> +       struct rssi_setting *rssi;
> +       GSList *patterns;
> +};
> +
>  struct adv_monitor_manager {
>         GSList *supported_types;
>         GSList *supported_features;
> @@ -43,6 +65,9 @@ struct adv_monitor_manager {
>         gboolean app_registered;
>  } manager = { NULL, NULL, NULL, FALSE };
>
> +static uint8_t adv_mon_idx;
> +static GSList *adv_mons;
> +
>  static void set_supported_list(GSList **list, DBusMessageIter *iter)
>  {
>         char *str;
> @@ -138,7 +163,10 @@ static void unregister_reply(DBusMessage *message, void *user_data)
>
>  void adv_monitor_register_app(DBusConnection *conn)
>  {
> -       if (manager.supported_types == NULL || manager.app_registered == TRUE ||
> +       if (manager.app_registered == TRUE) {
> +               bt_shell_printf("Advertisement Monitor already registered\n");
> +               return bt_shell_noninteractive_quit(EXIT_FAILURE);
> +       } else if (manager.supported_types == NULL ||
>                 g_dbus_proxy_method_call(manager.proxy, "RegisterMonitor",
>                                         register_setup, register_reply,
>                                         NULL, NULL) == FALSE) {
> @@ -150,8 +178,10 @@ void adv_monitor_register_app(DBusConnection *conn)
>
>  void adv_monitor_unregister_app(DBusConnection *conn)
>  {
> -       if (manager.app_registered == FALSE ||
> -               g_dbus_proxy_method_call(manager.proxy, "UnregisterMonitor",
> +       if (manager.app_registered == FALSE) {
> +               bt_shell_printf("Advertisement Monitor not registered\n");
> +               return bt_shell_noninteractive_quit(EXIT_FAILURE);
> +       } else if (g_dbus_proxy_method_call(manager.proxy, "UnregisterMonitor",
>                                         unregister_setup, unregister_reply,
>                                         NULL, NULL) == FALSE) {
>                 bt_shell_printf("Failed to unregister Advertisement Monitor\n");
> @@ -159,3 +189,295 @@ void adv_monitor_unregister_app(DBusConnection *conn)
>         }
>         manager.app_registered = FALSE;
>  }
> +
> +static void free_pattern(void *user_data)
> +{
> +       struct pattern *p = user_data;
> +
> +       g_free(p);
> +}
> +
> +static void free_adv_monitor(void *user_data)
> +{
> +       struct adv_monitor *adv_monitor = user_data;
> +
> +       g_free(adv_monitor->type);
> +       g_free(adv_monitor->rssi);
> +       g_slist_free_full(adv_monitor->patterns, free_pattern);
> +       g_free(adv_monitor);
> +}
> +
> +static uint8_t str2bytearray(char *str, uint8_t *arr)
> +{
> +       int idx, len = strlen(str), arr_len = 0;
> +
> +       if (len%2 != 0)
> +               return 0;
> +
> +       for (idx = 0; idx < len; idx += 2) {
> +               if (sscanf(str+idx, "%2hhx", &arr[arr_len++]) < 1)
> +                       return 0;
> +       }
> +       return arr_len;
> +}
> +
> +static struct rssi_setting *parse_rssi(char *rssi_str)
> +{
> +       struct rssi_setting *rssi;
> +       int16_t high_threshold, low_threshold;
> +       uint16_t high_timer, low_timer;
> +
> +       if (sscanf(rssi_str, "%hd,%hu,%hd,%hu", &high_threshold, &high_timer,
> +                                       &low_threshold, &low_timer) < 4)
> +               return NULL;
> +
> +       rssi = g_malloc0(sizeof(struct rssi_setting));
> +
> +       if (!rssi) {
> +               bt_shell_printf("Failed to allocate rssi_setting");
> +               bt_shell_noninteractive_quit(EXIT_FAILURE);
> +               return NULL;
> +       }
> +
> +       rssi->high_threshold = high_threshold;
> +       rssi->high_timer = high_timer;
> +       rssi->low_threshold = low_threshold;
> +       rssi->low_timer = low_timer;
> +
> +       return rssi;
> +}
> +
> +static struct pattern *parse_pattern(char *pattern)
> +{
> +       uint8_t start_pos, ad_data_type;
> +       char content_str[BT_AD_MAX_DATA_LEN];
> +       struct pattern *pat;
> +
> +       if (sscanf(pattern, "%hhu,%hhu,%s", &start_pos, &ad_data_type,
> +                                                       content_str) < 3)
> +               return NULL;
> +
> +       pat = g_malloc0(sizeof(struct pattern));
> +
> +       if (!pat) {
> +               bt_shell_printf("Failed to allocate pattern");
> +               bt_shell_noninteractive_quit(EXIT_FAILURE);
> +               return NULL;
> +       }
> +
> +       pat->start_pos = start_pos;
> +       pat->ad_data_type = ad_data_type;
> +       pat->content_len = str2bytearray(content_str, pat->content);
> +       if (pat->content_len == 0) {
> +               free_pattern(pat);
> +               return NULL;
> +       }
> +
> +       return pat;
> +}
> +
> +static GSList *parse_patterns(char *pattern_list[], int num)
> +{
> +       GSList *patterns = NULL;
> +       int cnt;
> +
> +       if (num == 0) {
> +               bt_shell_printf("No pattern provided\n");
> +               return NULL;
> +       }
> +
> +       for (cnt = 0; cnt < num; cnt++) {
> +               struct pattern *pattern;
> +
> +               pattern = parse_pattern(pattern_list[cnt]);
> +               if (pattern == NULL) {
> +                       g_slist_free_full(patterns, free_pattern);
> +                       return NULL;
> +               }
> +               patterns = g_slist_append(patterns, pattern);
> +       }
> +
> +       return patterns;
> +}
> +
> +static gint cmp_adv_monitor_with_idx(gconstpointer a, gconstpointer b)
> +{
> +       const struct adv_monitor *adv_monitor = a;
> +       uint8_t idx = *(uint8_t *)b;
> +
> +       return adv_monitor->idx != idx;
> +}
> +
> +static struct adv_monitor *find_adv_monitor_with_idx(uint8_t monitor_idx)
> +{
> +       GSList *list;
> +
> +       list = g_slist_find_custom(adv_mons, &monitor_idx,
> +                                               cmp_adv_monitor_with_idx);
> +
> +       if (list)
> +               return (struct adv_monitor *)list->data;
> +       return NULL;
> +}
> +
> +static void print_bytearray(char *prefix, uint8_t *arr, uint8_t len)
> +{
> +       int idx;
> +
> +       bt_shell_printf("%s", prefix);
> +       for (idx = 0; idx < len; idx++)
> +               bt_shell_printf("%02hhx", arr[idx]);
> +       bt_shell_printf("\n");
> +}
> +
> +static void print_adv_monitor(struct adv_monitor *adv_monitor)
> +{
> +       GSList *l;
> +
> +       bt_shell_printf("Advertisement Monitor %d\n", adv_monitor->idx);
> +       bt_shell_printf("\ttype: %s\n", adv_monitor->type);
> +       if (adv_monitor->rssi) {
> +               bt_shell_printf("\trssi:\n");
> +               bt_shell_printf("\t\thigh threshold: %hd\n",
> +                                       adv_monitor->rssi->high_threshold);
> +               bt_shell_printf("\t\thigh threshold timer: %hu\n",
> +                                       adv_monitor->rssi->high_timer);
> +               bt_shell_printf("\t\tlow threshold: %hd\n",
> +                                       adv_monitor->rssi->low_threshold);
> +               bt_shell_printf("\t\tlow threshold timer: %hu\n",
> +                                       adv_monitor->rssi->low_timer);
> +       }
> +
> +       if (adv_monitor->patterns) {
> +               int idx = 1;
> +
> +               for (l = adv_monitor->patterns; l; l = g_slist_next(l), idx++) {
> +                       struct pattern *pattern = l->data;
> +
> +                       bt_shell_printf("\tpattern %d:\n", idx);
> +                       bt_shell_printf("\t\tstart position: %hhu\n",
> +                                                       pattern->start_pos);
> +                       bt_shell_printf("\t\tAD data type: %hhu\n",
> +                                                       pattern->ad_data_type);
> +                       print_bytearray("\t\tcontent: ", pattern->content,
> +                                                       pattern->content_len);
> +               }
> +       }
> +}
> +
> +void adv_monitor_add_monitor(DBusConnection *conn, int argc, char *argv[])
> +{
> +       struct adv_monitor *adv_monitor;
> +       struct rssi_setting *rssi;
> +       GSList *patterns = NULL;
> +       char *type;
> +
> +       if (g_slist_length(adv_mons) >= UINT8_MAX) {
> +               bt_shell_printf("Number of advertisement monitor exceeds "
> +                               "the limit");
> +               return;
> +       }
> +
> +       while (find_adv_monitor_with_idx(adv_mon_idx))
> +               adv_mon_idx += 1;
> +
> +       type = argv[1];
> +
> +       if (strcmp(argv[2], "-") == 0)
> +               rssi = NULL;
> +       else {
> +               rssi = parse_rssi(argv[2]);
> +               if (rssi == NULL) {
> +                       bt_shell_printf("RSSIThresholdAndTimers malformed\n");
> +                       return;
> +               }
> +       }
> +
> +       patterns = parse_patterns(argv+3, argc-3);
> +       if (patterns == NULL) {
> +               bt_shell_printf("pattern-list malformed\n");
> +               return;
> +       }
> +
> +       adv_monitor = g_malloc0(sizeof(struct adv_monitor));
> +
> +       if (!adv_monitor) {
> +               bt_shell_printf("Failed to allocate adv_monitor");
> +               return bt_shell_noninteractive_quit(EXIT_FAILURE);
> +       }
> +
> +       adv_monitor->idx = adv_mon_idx;
> +       adv_monitor->type = g_strdup(type);
> +       adv_monitor->rssi = rssi;
> +       adv_monitor->patterns = patterns;
> +
> +       adv_mons = g_slist_append(adv_mons, adv_monitor);
> +       bt_shell_printf("Advertisement Monitor %d added\n", adv_monitor->idx);
> +}
> +
> +void adv_monitor_print_monitor(DBusConnection *conn, int monitor_idx)
> +{
> +       struct adv_monitor *adv_monitor;
> +       GSList *l;
> +
> +       if (monitor_idx < 0) {
> +               for (l = adv_mons; l; l = g_slist_next(l)) {
> +                       adv_monitor = l->data;
> +                       print_adv_monitor(adv_monitor);
> +               }
> +               return;
> +       }
> +
> +       adv_monitor = find_adv_monitor_with_idx(monitor_idx);
> +
> +       if (adv_monitor == NULL) {
> +               bt_shell_printf("Can't find monitor with index %d\n",
> +                                                               monitor_idx);
> +               return;
> +       }
> +
> +       print_adv_monitor(adv_monitor);
> +}
> +
> +void adv_monitor_remove_monitor(DBusConnection *conn, int monitor_idx)
> +{
> +       struct adv_monitor *adv_monitor;
> +
> +       if (monitor_idx < 0) {
> +               g_slist_free_full(g_steal_pointer(&adv_mons), free_adv_monitor);
> +               return;
> +       }
> +
> +       adv_monitor = find_adv_monitor_with_idx(monitor_idx);
> +       if (adv_monitor == NULL) {
> +               bt_shell_printf("Can't find monitor with index %d\n",
> +                                                               monitor_idx);
> +               return;
> +       }
> +
> +       adv_mons = g_slist_remove(adv_mons, adv_monitor);
> +       free_adv_monitor(adv_monitor);
> +       bt_shell_printf("Monitor %d deleted\n", monitor_idx);
> +}
> +
> +static void print_supported_list(GSList *list)
> +{
> +       GSList *iter;
> +
> +       for (iter = list; iter; iter = g_slist_next(iter)) {
> +               char *data = iter->data;
> +
> +               printf(" %s", data);
> +       }
> +}
> +
> +void adv_monitor_get_supported_info(void)
> +{
> +       bt_shell_printf("Supported Features:");
> +       print_supported_list(manager.supported_features);
> +       bt_shell_printf("\n");
> +
> +       bt_shell_printf("Supported Moniter Types:");
> +       print_supported_list(manager.supported_types);
> +       bt_shell_printf("\n");
> +}
> diff --git a/client/advertisement_monitor.h b/client/advertisement_monitor.h
> index 77b0b62c6..f2a0caf77 100644
> --- a/client/advertisement_monitor.h
> +++ b/client/advertisement_monitor.h
> @@ -21,3 +21,7 @@ void adv_monitor_add_manager(DBusConnection *conn, GDBusProxy *proxy);
>  void adv_monitor_remove_manager(DBusConnection *conn);
>  void adv_monitor_register_app(DBusConnection *conn);
>  void adv_monitor_unregister_app(DBusConnection *conn);
> +void adv_monitor_add_monitor(DBusConnection *conn, int argc, char *argv[]);
> +void adv_monitor_print_monitor(DBusConnection *conn, int monitor_idx);
> +void adv_monitor_remove_monitor(DBusConnection *conn, int monitor_idx);
> +void adv_monitor_get_supported_info(void);
> diff --git a/client/main.c b/client/main.c
> index 7ddd13aa0..2b63ee62a 100644
> --- a/client/main.c
> +++ b/client/main.c
> @@ -2686,6 +2686,53 @@ static void cmd_ad_clear(int argc, char *argv[])
>                 return bt_shell_noninteractive_quit(EXIT_FAILURE);
>  }
>
> +static void print_add_monitor_usage(void)
> +{
> +       bt_shell_usage();
> +       bt_shell_printf("RSSIThresholdAndTimers format:\n"
> +                       "\t<high-rssi>,<high-timeout>,<low-rssi>,<low-timeout>\n"
> +                       "\tor single '-' for not using RSSI as filter\n");
> +       bt_shell_printf("pattern format:\n"
> +                       "\t<start_position>,<ad_data_type>,<content_of_pattern>\n");
> +       bt_shell_printf("e.g.\n"
> +                       "\tadd-pattern-monitor or_patterns -10,10,-20,20 1,2,01ab55\n");

btshell parameters are space separated not comma.

> +}
> +
> +static void cmd_adv_monitor_add_monitor(int argc, char *argv[])
> +{
> +       if (argc < 3)
> +               print_add_monitor_usage();
> +       else
> +               adv_monitor_add_monitor(dbus_conn, argc, argv);
> +}
> +
> +static void cmd_adv_monitor_print_monitor(int argc, char *argv[])
> +{
> +       int monitor_idx;
> +
> +       if (strcmp(argv[1], "all") == 0)
> +               monitor_idx = -1;
> +       else
> +               monitor_idx = atoi(argv[1]);
> +       adv_monitor_print_monitor(dbus_conn, monitor_idx);
> +}
> +
> +static void cmd_adv_monitor_remove_monitor(int argc, char *argv[])
> +{
> +       int monitor_idx;
> +
> +       if (strcmp(argv[1], "all") == 0)
> +               monitor_idx = -1;
> +       else
> +               monitor_idx = atoi(argv[1]);
> +       adv_monitor_remove_monitor(dbus_conn, monitor_idx);
> +}
> +
> +static void cmd_adv_monitor_get_supported_info(int argc, char *argv[])
> +{
> +       adv_monitor_get_supported_info();
> +}
> +
>  static const struct bt_shell_menu advertise_menu = {
>         .name = "advertise",
>         .desc = "Advertise Options Submenu",
> @@ -2722,6 +2769,28 @@ static const struct bt_shell_menu advertise_menu = {
>         { } },
>  };
>
> +static const struct bt_shell_menu advertise_monitor_menu = {
> +       .name = "advmon",

I'd use monitor instead of advmon here.

> +       .desc = "Advertisement Monitor Options Submenu",
> +       .entries = {
> +       { "add-pattern-monitor", "<type-of-monitor/help> "
> +                               "[RSSIThresholdAndTimers] "
> +                               "[patterns=pattern1 pattern2 ...]",
> +                               cmd_adv_monitor_add_monitor,
> +                               "Add pattern monitor" },
> +       { "get-pattern-monitor", "<monitor-id/all>",
> +                               cmd_adv_monitor_print_monitor,
> +                               "Get advertisement monitor" },
> +       { "remove-pattern-monitor", "<monitor-id/all>",
> +                               cmd_adv_monitor_remove_monitor,
> +                               "Remove advertisement monitor" },
> +       { "get-supported-info", NULL,
> +                               cmd_adv_monitor_get_supported_info,
> +                               "Get advertisement manager supported "
> +                               "features and supported monitor types" },

We could probably drop the monitor from the commands here which should
leave us at:

add-pattern
get-pattern
remote-pattern
supported-info

> +       { } },
> +};
> +
>  static const struct bt_shell_menu scan_menu = {
>         .name = "scan",
>         .desc = "Scan Options Submenu",
> @@ -2897,6 +2966,7 @@ int main(int argc, char *argv[])
>         bt_shell_init(argc, argv, &opt);
>         bt_shell_set_menu(&main_menu);
>         bt_shell_add_submenu(&advertise_menu);
> +       bt_shell_add_submenu(&advertise_monitor_menu);
>         bt_shell_add_submenu(&scan_menu);
>         bt_shell_add_submenu(&gatt_menu);
>         bt_shell_set_prompt(PROMPT_OFF);
> --
> 2.28.0.220.ged08abb693-goog
>


-- 
Luiz Augusto von Dentz