On Fri, 15 Jan 2021 at 02:42, Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
>
> Hi Archie,
>
> On Wed, Jan 13, 2021 at 11:45 PM Archie Pusaka <apusaka@xxxxxxxxxx> wrote:
> >
> > From: Archie Pusaka <apusaka@xxxxxxxxxxxx>
> >
> > Using the new opcode MGMT_OP_ADD_ADV_PATTERNS_MONITOR_RSSI to
> > monitor advertisement according to some RSSI criteria.
> >
> > Reviewed-by: Yun-Hao Chung <howardchung@xxxxxxxxxx>
> > ---
> >
> > Changes in v3:
> > * split the struct RSSIThresholdsAndTimers
> >
> > client/adv_monitor.c | 162 ++++++++++++++++++++++++++-----------------
> > client/adv_monitor.h | 1 +
> > client/main.c | 29 ++++----
> > 3 files changed, 113 insertions(+), 79 deletions(-)
> >
> > diff --git a/client/adv_monitor.c b/client/adv_monitor.c
> > index f62e9f4442..37faf1edfa 100644
> > --- a/client/adv_monitor.c
> > +++ b/client/adv_monitor.c
> > @@ -30,9 +30,10 @@
> >
> > struct rssi_setting {
> > int16_t high_threshold;
> > - uint16_t high_timer;
> > + uint16_t high_timeout;
> > int16_t low_threshold;
> > - uint16_t low_timer;
> > + uint16_t low_timeout;
> > + uint16_t sampling_period;
> > };
> >
> > struct pattern {
> > @@ -131,24 +132,58 @@ static gboolean get_type(const GDBusPropertyTable *property,
> > return TRUE;
> > }
> >
> > -static gboolean get_rssi(const GDBusPropertyTable *property,
> > +static gboolean get_low_threshold(const GDBusPropertyTable *property,
> > DBusMessageIter *iter, void *user_data)
> > {
> > struct adv_monitor *adv_monitor = user_data;
> > struct rssi_setting *rssi = adv_monitor->rssi;
> > - DBusMessageIter data_iter;
> >
> > - dbus_message_iter_open_container(iter, DBUS_TYPE_STRUCT,
> > - NULL, &data_iter);
> > - dbus_message_iter_append_basic(&data_iter, DBUS_TYPE_INT16,
> > - &rssi->high_threshold);
> > - dbus_message_iter_append_basic(&data_iter, DBUS_TYPE_UINT16,
> > - &rssi->high_timer);
> > - dbus_message_iter_append_basic(&data_iter, DBUS_TYPE_INT16,
> > + dbus_message_iter_append_basic(iter, DBUS_TYPE_INT16,
> > &rssi->low_threshold);
> > - dbus_message_iter_append_basic(&data_iter, DBUS_TYPE_UINT16,
> > - &rssi->low_timer);
> > - dbus_message_iter_close_container(iter, &data_iter);
> > + return TRUE;
> > +}
> > +
> > +static gboolean get_high_threshold(const GDBusPropertyTable *property,
> > + DBusMessageIter *iter, void *user_data)
> > +{
> > + struct adv_monitor *adv_monitor = user_data;
> > + struct rssi_setting *rssi = adv_monitor->rssi;
> > +
> > + dbus_message_iter_append_basic(iter, DBUS_TYPE_INT16,
> > + &rssi->high_threshold);
> > + return TRUE;
> > +}
> > +
> > +static gboolean get_low_timeout(const GDBusPropertyTable *property,
> > + DBusMessageIter *iter, void *user_data)
> > +{
> > + struct adv_monitor *adv_monitor = user_data;
> > + struct rssi_setting *rssi = adv_monitor->rssi;
> > +
> > + dbus_message_iter_append_basic(iter, DBUS_TYPE_UINT16,
> > + &rssi->low_timeout);
> > + return TRUE;
> > +}
> > +
> > +static gboolean get_high_timeout(const GDBusPropertyTable *property,
> > + DBusMessageIter *iter, void *user_data)
> > +{
> > + struct adv_monitor *adv_monitor = user_data;
> > + struct rssi_setting *rssi = adv_monitor->rssi;
> > +
> > + dbus_message_iter_append_basic(iter, DBUS_TYPE_UINT16,
> > + &rssi->high_timeout);
> > + return TRUE;
> > +}
> > +
> > +static gboolean get_sampling_period(const GDBusPropertyTable *property,
> > + DBusMessageIter *iter, void *user_data)
> > +{
> > + struct adv_monitor *adv_monitor = user_data;
> > + struct rssi_setting *rssi = adv_monitor->rssi;
> > +
> > + dbus_message_iter_append_basic(iter, DBUS_TYPE_UINT16,
> > + &rssi->sampling_period);
> > return TRUE;
> > }
> >
> > @@ -212,7 +247,11 @@ static gboolean pattern_exists(const GDBusPropertyTable *property, void *data)
> >
> > static const GDBusPropertyTable adv_monitor_props[] = {
> > { "Type", "s", get_type },
> > - { "RSSIThresholdsAndTimers", "(nqnq)", get_rssi, NULL, rssi_exists },
> > + { "RSSILowThreshold", "n", get_low_threshold, NULL, rssi_exists },
> > + { "RSSIHighThreshold", "n", get_high_threshold, NULL, rssi_exists },
> > + { "RSSILowTimeout", "q", get_low_timeout, NULL, rssi_exists },
> > + { "RSSIHighTimeout", "q", get_high_timeout, NULL, rssi_exists },
> > + { "RSSISamplingPeriod", "q", get_sampling_period, NULL, rssi_exists },
>
> Interesting, is the SamplingPeriod new? It seems we are missing the
> documentation changes of the split.
Yes, it is new. The purpose is to make it compatible with the add
monitor MGMT command. Also yes, I forgot to update the doc. Will do so
in v4.
>
> > { "Patterns", "a(yyay)", get_patterns, NULL, pattern_exists },
> > { }
> > };
> > @@ -376,56 +415,51 @@ static uint8_t str2bytearray(char *str, uint8_t *arr)
> > return arr_len;
> > }
> >
> > -static void parse_rssi_value_pair(char *value_pair, int *low, int *high)
> > -{
> > - char *val1, *val2;
> > - bool flag = value_pair[0] == ',';
> > -
> > - val1 = strtok(value_pair, ",");
> > -
> > - if (!val1)
> > - return;
> > -
> > - val2 = strtok(NULL, ",");
> > -
> > - if (!val2) {
> > - if (!flag)
> > - *low = atoi(val1);
> > - else
> > - *high = atoi(val1);
> > - } else {
> > - *low = atoi(val1);
> > - *high = atoi(val2);
> > - }
> > -}
> > -
> > -static struct rssi_setting *parse_rssi(char *range, char *timeout)
> > +static struct rssi_setting *parse_rssi(char *params)
> > {
> > struct rssi_setting *rssi;
> > - int high_threshold, low_threshold, high_timer, low_timer;
> > -
> > - high_threshold = RSSI_DEFAULT_HIGH_THRESHOLD;
> > - low_threshold = RSSI_DEFAULT_LOW_THRESHOLD;
> > - high_timer = RSSI_DEFAULT_HIGH_TIMEOUT;
> > - low_timer = RSSI_DEFAULT_LOW_TIMEOUT;
> > + char *split, *endptr;
> > + int i;
> > + int values[5] = {RSSI_DEFAULT_LOW_THRESHOLD,
> > + RSSI_DEFAULT_HIGH_THRESHOLD,
> > + RSSI_DEFAULT_LOW_TIMEOUT,
> > + RSSI_DEFAULT_HIGH_TIMEOUT,
> > + RSSI_DEFAULT_SAMPLING_PERIOD};
> > +
> > + for (i = 0; i < 5; i++) {
> > + if (!params) /* Params too short */
> > + goto bad_format;
> > +
> > + split = strsep(¶ms, ",");
> > + if (*split != '\0') {
> > + values[i] = strtol(split, &endptr, 0);
> > + if (*endptr != '\0') /* Conversion failed */
> > + goto bad_format;
> > + } /* Otherwise no parsing needed - use default */
> > + }
>
> You might want to consider taking these parameters separately in the
> command itself, that way we don't have to reparse the string parameter
> as they would be split in argc/argv by bt_shell.
Done in v4
>
> > - parse_rssi_value_pair(range, &low_threshold, &high_threshold);
> > - parse_rssi_value_pair(timeout, &low_timer, &high_timer);
> > + if (params) /* There are trailing unused params */
> > + goto bad_format;
> >
> > rssi = g_malloc0(sizeof(struct rssi_setting));
> > -
> > if (!rssi) {
> > - bt_shell_printf("Failed to allocate rssi_setting");
> > + bt_shell_printf("Failed to allocate rssi_setting\n");
> > 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;
> > + rssi->low_threshold = values[0];
> > + rssi->high_threshold = values[1];
> > + rssi->low_timeout = values[2];
> > + rssi->high_timeout = values[3];
> > + rssi->sampling_period = values[4];
> >
> > return rssi;
> > +
> > +bad_format:
> > + bt_shell_printf("Failed to parse RSSI\n");
> > + bt_shell_noninteractive_quit(EXIT_FAILURE);
> > + return NULL;
> > }
> >
> > static struct pattern *parse_pattern(char *parameter_list[])
> > @@ -435,7 +469,7 @@ static struct pattern *parse_pattern(char *parameter_list[])
> > pat = g_malloc0(sizeof(struct pattern));
> >
> > if (!pat) {
> > - bt_shell_printf("Failed to allocate pattern");
> > + bt_shell_printf("Failed to allocate pattern\n");
> > bt_shell_noninteractive_quit(EXIT_FAILURE);
> > return NULL;
> > }
> > @@ -531,12 +565,14 @@ static void print_adv_monitor(struct adv_monitor *adv_monitor)
> > 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\thigh threshold timeout: %hu\n",
> > + adv_monitor->rssi->high_timeout);
> > 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);
> > + bt_shell_printf("\t\tlow threshold timeout: %hu\n",
> > + adv_monitor->rssi->low_timeout);
> > + bt_shell_printf("\t\tsampling period: %hu\n",
> > + adv_monitor->rssi->sampling_period);
> > }
> >
> > if (adv_monitor->patterns) {
> > @@ -572,15 +608,15 @@ void adv_monitor_add_monitor(DBusConnection *conn, char *type,
> > while (find_adv_monitor_with_idx(adv_mon_idx))
> > adv_mon_idx += 1;
> >
> > - if (rssi_enabled == FALSE)
> > + if (rssi_enabled == FALSE) {
> > rssi = NULL;
> > - else {
> > - rssi = parse_rssi(argv[1], argv[2]);
> > + } else {
> > + rssi = parse_rssi(argv[1]);
> > if (rssi == NULL)
> > return;
> >
> > - argv += 2;
> > - argc -= 2;
> > + argv += 1;
> > + argc -= 1;
> > }
> >
> > patterns = parse_patterns(argv+1, argc-1);
> > diff --git a/client/adv_monitor.h b/client/adv_monitor.h
> > index dd6f615799..2bdc447265 100644
> > --- a/client/adv_monitor.h
> > +++ b/client/adv_monitor.h
> > @@ -12,6 +12,7 @@
> > #define RSSI_DEFAULT_LOW_THRESHOLD -70
> > #define RSSI_DEFAULT_HIGH_TIMEOUT 10
> > #define RSSI_DEFAULT_LOW_TIMEOUT 5
> > +#define RSSI_DEFAULT_SAMPLING_PERIOD 0
> >
> > void adv_monitor_add_manager(DBusConnection *conn, GDBusProxy *proxy);
> > void adv_monitor_remove_manager(DBusConnection *conn);
> > diff --git a/client/main.c b/client/main.c
> > index 9403f1af6e..5d84e7cd54 100644
> > --- a/client/main.c
> > +++ b/client/main.c
> > @@ -2709,26 +2709,23 @@ static void cmd_ad_clear(int argc, char *argv[])
> >
> > static void print_add_or_pattern_with_rssi_usage(void)
> > {
> > - bt_shell_printf("rssi-range format:\n"
> > - "\t<low-rssi>,<high-rssi>\n"
> > - "\tBoth parameters can be skipped, in that case the\n"
> > - "\tparamter will be set to its pre-defined value\n");
> > - bt_shell_printf("\tPre-defined low-rssi,high-rssi: %d,%d\n",
> > - RSSI_DEFAULT_LOW_THRESHOLD,
> > - RSSI_DEFAULT_HIGH_THRESHOLD);
> > - bt_shell_printf("timeout format:\n"
> > - "\t<low-rssi>,<high-rssi>\n"
> > - "\tBoth parameters can be skipped, in that case the\n"
> > - "\tparamter will be set to its pre-defined value\n");
> > - bt_shell_printf("\tPre-defined low-timeout,high-timeout: %d,%d\n",
> > - RSSI_DEFAULT_LOW_TIMEOUT,
> > + bt_shell_printf("rssi format:\n"
> > + "\t<low-rssi>,<high-rssi>,<low-rssi-timeout>,"
> > + "<high-rssi-timeout>,<sampling>\n"
> > + "\tAll parameters can be skipped, in that case they\n"
> > + "\twill be set to pre-defined values, which are:\n");
> > + bt_shell_printf("\t\tlow-rssi: %d\n", RSSI_DEFAULT_LOW_THRESHOLD);
> > + bt_shell_printf("\t\thigh-rssi: %d\n", RSSI_DEFAULT_HIGH_THRESHOLD);
> > + bt_shell_printf("\t\tlow-rssi-timeout: %d\n", RSSI_DEFAULT_LOW_TIMEOUT);
> > + bt_shell_printf("\t\thigh-rssi-timeout: %d\n",
> > RSSI_DEFAULT_HIGH_TIMEOUT);
> > + bt_shell_printf("\t\tsampling: %d\n", RSSI_DEFAULT_SAMPLING_PERIOD);
> > bt_shell_printf("pattern format:\n"
> > "\t<start_position> <ad_data_type> <content_of_pattern>\n");
> > bt_shell_printf("e.g.\n"
> > - "\tadd-or-pattern-rssi -10, ,10 1 2 01ab55\n");
> > + "\tadd-or-pattern-rssi -10,,,10,0 1 2 01ab55\n");
> > bt_shell_printf("or\n"
> > - "\tadd-or-pattern-rssi -50,-30 , 1 2 01ab55 3 4 23cd66\n");
> > + "\tadd-or-pattern-rssi -50,-30,,, 1 2 01ab55 3 4 23cd66\n");
> > }
> >
> > static void print_add_or_pattern_usage(void)
> > @@ -2826,7 +2823,7 @@ static const struct bt_shell_menu advertise_monitor_menu = {
> > .name = "monitor",
> > .desc = "Advertisement Monitor Options Submenu",
> > .entries = {
> > - { "add-or-pattern-rssi", "<rssi-range=low,high> <timeout=low,high> "
> > + { "add-or-pattern-rssi", "<rssi=low,high,low-time,high-time,sampling> "
>
> Perhaps we should split the rssi setting in multiple commands as well e.g.:
>
> rssi <low> <high>
> timeout <low> <high>
> interval <value>
> add-or-pattern <pattern1> [...]
>
> So when add-or-pattern is used it takes the rssi values if they have
> been, we would probably need to create the instance at first command
> but we only register when add is called, also don't fill the defaults
> just omit them so the daemon fill them in that way if we at some point
> change the defaults we don't have to change it in multiple places.
Done in v4. Please tell me what you think!
>
> > "[patterns=pattern1 pattern2 ...]",
> > cmd_adv_monitor_add_or_monitor_with_rssi,
> > "Add 'or pattern' type monitor with RSSI "
> > --
> > 2.30.0.284.gd98b1dd5eaa7-goog
> >
>
>
> --
> Luiz Augusto von Dentz
Thanks,
Archie
