Hi Yun-hao,
On Tue, Sep 8, 2020 at 9:01 PM Yun-hao Chung <howardchung@xxxxxxxxxx> wrote:
>
> Hi Luiz,
>
> Thanks for the comments.
>
>
> On Wed, Sep 9, 2020 at 1:06 AM Luiz Augusto von Dentz
> <luiz.dentz@xxxxxxxxx> wrote:
> >
> > Hi Howard,
> >
> > On Tue, Aug 18, 2020 at 9:15 PM Howard Chung <howardchung@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > >
> > > 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.
>
> The reason we use commas is because RSSIThresholdAndTimers is an
> all-or-none property, it's clearer for users to either input all the
> four parameters or a single '-' to leave it unset.
> For a similar reason, a valid pattern requires all of the three
> parameters provided. If we want to avoid comma for maintaining
> consistency, we could have command usage like:
>
> add-pattern or_patterns -10 10 -20 20 1 2 01ab55 // single content
> with RSSI filter
> add-pattern or_patterns - 1 2 01ab55 // single
> content without RSSI filter
>
> Let me know if this is OK with you and other suggestions are welcome.
Actually it might be better to have good defaults on these or have
something like:
<rssi-range=low,high> then parse the input to check if comma has been
passed otherwise we can assume only the high (or low if that value
means better signal) rssi has been passed.
<timeout=low,high> same as above.
Also I would consider merging the type into the command itself:
add-or-pattern-rssi -10,-20 10,1 01ab55
add-or-pattern 1 2 01ab55
That way the bt_shell parsing of the command description can actually
detect if enough parameters have been given since we can't really have
short of overloading command with different arguments as it would make
it rather confusing to use since there are no types to detect which
version the user is trying to use.
> >
> > > +}
> > > +
> > > +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.
>
> Will address these in the next patch. Thanks!
>
> >
> > > + .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
--
Luiz Augusto von Dentz
