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.
> { "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.
> - 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.
> "[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
