* [BlueZ PATCH v6 2/6] adv_monitor: Implement Adv matching based on stored monitors
2020-10-07 0:14 [BlueZ PATCH v6 1/6] adv_monitor: Implement RSSI Filter logic for background scanning Miao-chen Chou
@ 2020-10-07 0:14 ` Miao-chen Chou
2020-10-07 6:21 ` Luiz Augusto von Dentz
2020-10-07 0:14 ` [BlueZ PATCH v6 3/6] adapter: Clear all Adv monitors upon bring-up Miao-chen Chou
` (5 subsequent siblings)
6 siblings, 1 reply; 15+ messages in thread
From: Miao-chen Chou @ 2020-10-07 0:14 UTC (permalink / raw)
To: Bluetooth Kernel Mailing List
Cc: Luiz Augusto von Dentz, Howard Chung,
chromeos-bluetooth-upstreaming, Alain Michaud, Marcel Holtmann,
Manish Mandlik, Miao-chen Chou, Abhishek Pandit-Subedi
This implements create an entry point in adapter to start the matching of
Adv based on all monitors and invoke the RSSI tracking for Adv reporting.
Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Reviewed-by: Alain Michaud <alainm@chromium.org>
Reviewed-by: Manish Mandlik <mmandlik@chromium.org>
---
Changes in v6:
- Fix the termination condition of AD data paring and remove unnecessary
length check
Changes in v5:
- Remove unittest helper functions
Changes in v3:
- Remove unused variables
- Fix signature of queue_find()
src/adapter.c | 35 ++++++--
src/adv_monitor.c | 219 ++++++++++++++++++++++++++++++++++++++++------
src/adv_monitor.h | 12 +++
3 files changed, 234 insertions(+), 32 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index 6d0114a6b..fdd9b3808 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -6601,6 +6601,15 @@ static void update_found_devices(struct btd_adapter *adapter,
bool name_known, discoverable;
char addr[18];
bool duplicate = false;
+ GSList *matched_monitors;
+
+ /* During the background scanning, update the device only when the data
+ * match at least one Adv monitor
+ */
+ matched_monitors = btd_adv_monitor_content_filter(
+ adapter->adv_monitor_manager, data, data_len);
+ if (!adapter->discovering && !matched_monitors)
+ return;
memset(&eir_data, 0, sizeof(eir_data));
eir_parse(&eir_data, data, data_len);
@@ -6646,18 +6655,22 @@ static void update_found_devices(struct btd_adapter *adapter,
device_store_cached_name(dev, eir_data.name);
/*
- * Only skip devices that are not connected, are temporary and there
- * is no active discovery session ongoing.
+ * Only skip devices that are not connected, are temporary, and there
+ * is no active discovery session ongoing and no matched Adv monitors
*/
- if (!btd_device_is_connected(dev) && (device_is_temporary(dev) &&
- !adapter->discovery_list)) {
+ if (!btd_device_is_connected(dev) &&
+ (device_is_temporary(dev) && !adapter->discovery_list) &&
+ !matched_monitors) {
eir_data_free(&eir_data);
return;
}
- /* Don't continue if not discoverable or if filter don't match */
- if (!discoverable || (adapter->filtered_discovery &&
- !is_filter_match(adapter->discovery_list, &eir_data, rssi))) {
+ /* If there is no matched Adv monitors, don't continue if not
+ * discoverable or if active discovery filter don't match.
+ */
+ if (!matched_monitors && (!discoverable ||
+ (adapter->filtered_discovery && !is_filter_match(
+ adapter->discovery_list, &eir_data, rssi)))) {
eir_data_free(&eir_data);
return;
}
@@ -6714,6 +6727,14 @@ static void update_found_devices(struct btd_adapter *adapter,
eir_data_free(&eir_data);
+ /* After the device is updated, notify the matched Adv monitors */
+ if (matched_monitors) {
+ btd_adv_monitor_notify_monitors(adapter->adv_monitor_manager,
+ dev, rssi, matched_monitors);
+ g_slist_free(matched_monitors);
+ matched_monitors = NULL;
+ }
+
/*
* Only if at least one client has requested discovery, maintain
* list of found devices and name confirming for legacy devices.
diff --git a/src/adv_monitor.c b/src/adv_monitor.c
index 31ed30a00..fcb127cd4 100644
--- a/src/adv_monitor.c
+++ b/src/adv_monitor.c
@@ -29,15 +29,12 @@
#include "device.h"
#include "log.h"
#include "src/error.h"
-#include "src/shared/ad.h"
#include "src/shared/mgmt.h"
#include "src/shared/queue.h"
#include "src/shared/util.h"
#include "adv_monitor.h"
-static void monitor_device_free(void *data);
-
#define ADV_MONITOR_INTERFACE "org.bluez.AdvertisementMonitor1"
#define ADV_MONITOR_MGR_INTERFACE "org.bluez.AdvertisementMonitorManager1"
@@ -84,7 +81,7 @@ enum monitor_state {
MONITOR_STATE_HONORED, /* Accepted by kernel */
};
-struct pattern {
+struct btd_adv_monitor_pattern {
uint8_t ad_type;
uint8_t offset;
uint8_t length;
@@ -133,6 +130,23 @@ struct app_match_data {
const char *path;
};
+struct adv_content_filter_info {
+ uint8_t eir_len;
+ const uint8_t *eir;
+
+ bool matched; /* Intermediate state per monitor */
+ GSList *matched_monitors; /* List of matched monitors */
+};
+
+struct adv_rssi_filter_info {
+ struct btd_device *device;
+ int8_t rssi;
+};
+
+static void monitor_device_free(void *data);
+static void adv_monitor_filter_rssi(struct adv_monitor *monitor,
+ struct btd_device *device, int8_t rssi);
+
const struct adv_monitor_type {
enum monitor_type type;
const char *name;
@@ -155,7 +169,7 @@ static void app_reply_msg(struct adv_monitor_app *app, DBusMessage *reply)
/* Frees a pattern */
static void pattern_free(void *data)
{
- struct pattern *pattern = data;
+ struct btd_adv_monitor_pattern *pattern = data;
if (!pattern)
return;
@@ -435,6 +449,36 @@ failed:
return false;
}
+/* Allocates and initiates a pattern with the given content */
+static struct btd_adv_monitor_pattern *pattern_create(
+ uint8_t ad_type, uint8_t offset, uint8_t length, const uint8_t *value)
+{
+ struct btd_adv_monitor_pattern *pattern;
+
+ if (offset > BT_AD_MAX_DATA_LEN - 1)
+ return NULL;
+
+ if ((ad_type > BT_AD_3D_INFO_DATA &&
+ ad_type != BT_AD_MANUFACTURER_DATA) ||
+ ad_type < BT_AD_FLAGS) {
+ return NULL;
+ }
+
+ if (!value || !length || offset + length > BT_AD_MAX_DATA_LEN)
+ return NULL;
+
+ pattern = new0(struct btd_adv_monitor_pattern, 1);
+ if (!pattern)
+ return NULL;
+
+ pattern->ad_type = ad_type;
+ pattern->offset = offset;
+ pattern->length = length;
+ memcpy(pattern->value, value, pattern->length);
+
+ return pattern;
+}
+
/* Retrieves Patterns from the remote Adv Monitor object, verifies the values
* and update the local Adv Monitor
*/
@@ -464,7 +508,7 @@ static bool parse_patterns(struct adv_monitor *monitor, const char *path)
int value_len;
uint8_t *value;
uint8_t offset, ad_type;
- struct pattern *pattern;
+ struct btd_adv_monitor_pattern *pattern;
DBusMessageIter struct_iter, value_iter;
dbus_message_iter_recurse(&array_iter, &struct_iter);
@@ -496,28 +540,10 @@ static bool parse_patterns(struct adv_monitor *monitor, const char *path)
dbus_message_iter_get_fixed_array(&value_iter, &value,
&value_len);
- // Verify the values
- if (offset > BT_AD_MAX_DATA_LEN - 1)
- goto failed;
-
- if ((ad_type > BT_AD_3D_INFO_DATA &&
- ad_type != BT_AD_MANUFACTURER_DATA) ||
- ad_type < BT_AD_FLAGS) {
- goto failed;
- }
-
- if (!value || value_len <= 0 || value_len > BT_AD_MAX_DATA_LEN)
- goto failed;
-
- pattern = new0(struct pattern, 1);
+ pattern = pattern_create(ad_type, offset, value_len, value);
if (!pattern)
goto failed;
- pattern->ad_type = ad_type;
- pattern->offset = offset;
- pattern->length = value_len;
- memcpy(pattern->value, value, pattern->length);
-
queue_push_tail(monitor->patterns, pattern);
dbus_message_iter_next(&array_iter);
@@ -952,6 +978,149 @@ void btd_adv_monitor_manager_destroy(struct btd_adv_monitor_manager *manager)
manager_destroy(manager);
}
+static bool pattern_match(const uint8_t *eir, uint8_t eir_len,
+ const struct btd_adv_monitor_pattern *pattern)
+{
+ const uint8_t *data;
+ uint8_t idx = 0;
+ uint8_t field_len, data_len, data_type;
+
+ while (idx < eir_len - 1) {
+ field_len = eir[0];
+
+ /* Check for the end of EIR */
+ if (field_len == 0)
+ break;
+
+ idx += field_len + 1;
+
+ /* Do not continue filtering if got incorrect length */
+ if (idx > eir_len)
+ break;
+
+ data = &eir[2];
+ data_type = eir[1];
+ data_len = field_len - 1;
+
+ eir += field_len + 1;
+
+ if (data_type != pattern->ad_type)
+ continue;
+
+ if (data_len < pattern->offset + pattern->length)
+ continue;
+
+ if (!memcmp(data + pattern->offset, pattern->value,
+ pattern->length))
+ return true;
+ }
+
+ return false;
+}
+
+/* Processes the content matching based on a pattern */
+static void adv_match_per_pattern(void *data, void *user_data)
+{
+ struct btd_adv_monitor_pattern *pattern = data;
+ struct adv_content_filter_info *info = user_data;
+
+ if (!pattern || info->matched)
+ return;
+
+ info->matched = pattern_match(info->eir, info->eir_len, pattern);
+}
+
+/* Processes the content matching based pattern(s) of a monitor */
+static void adv_match_per_monitor(void *data, void *user_data)
+{
+ struct adv_monitor *monitor = data;
+ struct adv_content_filter_info *info = user_data;
+
+ if (!monitor && monitor->state != MONITOR_STATE_HONORED)
+ return;
+
+ /* Reset the intermediate matched status */
+ info->matched = false;
+
+ if (monitor->type == MONITOR_TYPE_OR_PATTERNS) {
+ queue_foreach(monitor->patterns, adv_match_per_pattern, info);
+ if (info->matched)
+ goto matched;
+ }
+
+ return;
+
+matched:
+ info->matched_monitors = g_slist_prepend(info->matched_monitors,
+ monitor);
+}
+
+/* Processes the content matching for the monitor(s) of an app */
+static void adv_match_per_app(void *data, void *user_data)
+{
+ struct adv_monitor_app *app = data;
+
+ if (!app)
+ return;
+
+ queue_foreach(app->monitors, adv_match_per_monitor, user_data);
+}
+
+/* Processes the content matching for every app without RSSI filtering and
+ * notifying monitors. The caller is responsible of releasing the memory of the
+ * list but not the data.
+ * Returns the list of monitors whose content match eir.
+ */
+GSList *btd_adv_monitor_content_filter(struct btd_adv_monitor_manager *manager,
+ const uint8_t *eir, uint8_t eir_len)
+{
+ struct adv_content_filter_info info;
+
+ if (!manager || !eir || !eir_len)
+ return NULL;
+
+ info.eir_len = eir_len;
+ info.eir = eir;
+ info.matched_monitors = NULL;
+
+ queue_foreach(manager->apps, adv_match_per_app, &info);
+
+ return info.matched_monitors;
+}
+
+/* Wraps adv_monitor_filter_rssi() to processes the content-matched monitor with
+ * RSSI filtering and notifies it on device found/lost event
+ */
+static void monitor_filter_rssi(gpointer a, gpointer b)
+{
+ struct adv_monitor *monitor = a;
+ struct adv_rssi_filter_info *info = b;
+
+ if (!monitor || !info)
+ return;
+
+ adv_monitor_filter_rssi(monitor, info->device, info->rssi);
+}
+
+/* Processes every content-matched monitor with RSSI filtering and notifies on
+ * device found/lost event. The caller is responsible of releasing the memory
+ * of matched_monitors list but not its data.
+ */
+void btd_adv_monitor_notify_monitors(struct btd_adv_monitor_manager *manager,
+ struct btd_device *device, int8_t rssi,
+ GSList *matched_monitors)
+{
+ struct adv_rssi_filter_info info;
+
+ if (!manager || !device || !matched_monitors)
+ return;
+
+ info.device = device;
+ info.rssi = rssi;
+
+ g_slist_foreach(matched_monitors, monitor_filter_rssi, &info);
+}
+
/* Matches a device based on btd_device object */
static bool monitor_device_match(const void *a, const void *b)
{
diff --git a/src/adv_monitor.h b/src/adv_monitor.h
index 13d5d7282..e2482e11e 100644
--- a/src/adv_monitor.h
+++ b/src/adv_monitor.h
@@ -11,16 +11,28 @@
#ifndef __ADV_MONITOR_H
#define __ADV_MONITOR_H
+#include <glib.h>
+
+#include "src/shared/ad.h"
+
struct mgmt;
struct btd_device;
struct btd_adapter;
struct btd_adv_monitor_manager;
+struct btd_adv_monitor_pattern;
struct btd_adv_monitor_manager *btd_adv_monitor_manager_create(
struct btd_adapter *adapter,
struct mgmt *mgmt);
void btd_adv_monitor_manager_destroy(struct btd_adv_monitor_manager *manager);
+GSList *btd_adv_monitor_content_filter(struct btd_adv_monitor_manager *manager,
+ const uint8_t *eir, uint8_t eir_len);
+
+void btd_adv_monitor_notify_monitors(struct btd_adv_monitor_manager *manager,
+ struct btd_device *device, int8_t rssi,
+ GSList *matched_monitors);
+
void btd_adv_monitor_device_remove(struct btd_adv_monitor_manager *manager,
struct btd_device *device);
--
2.26.2
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [BlueZ PATCH v6 2/6] adv_monitor: Implement Adv matching based on stored monitors
2020-10-07 0:14 ` [BlueZ PATCH v6 2/6] adv_monitor: Implement Adv matching based on stored monitors Miao-chen Chou
@ 2020-10-07 6:21 ` Luiz Augusto von Dentz
2020-10-12 21:21 ` Miao-chen Chou
0 siblings, 1 reply; 15+ messages in thread
From: Luiz Augusto von Dentz @ 2020-10-07 6:21 UTC (permalink / raw)
To: Miao-chen Chou
Cc: Bluetooth Kernel Mailing List, Luiz Augusto von Dentz,
Howard Chung, ChromeOS Bluetooth Upstreaming, Alain Michaud,
Marcel Holtmann, Manish Mandlik, Abhishek Pandit-Subedi
Hi Miao,
On Tue, Oct 6, 2020 at 5:17 PM Miao-chen Chou <mcchou@chromium.org> wrote:
>
> This implements create an entry point in adapter to start the matching of
> Adv based on all monitors and invoke the RSSI tracking for Adv reporting.
>
> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> Reviewed-by: Alain Michaud <alainm@chromium.org>
> Reviewed-by: Manish Mandlik <mmandlik@chromium.org>
> ---
>
> Changes in v6:
> - Fix the termination condition of AD data paring and remove unnecessary
> length check
>
> Changes in v5:
> - Remove unittest helper functions
>
> Changes in v3:
> - Remove unused variables
> - Fix signature of queue_find()
>
> src/adapter.c | 35 ++++++--
> src/adv_monitor.c | 219 ++++++++++++++++++++++++++++++++++++++++------
> src/adv_monitor.h | 12 +++
> 3 files changed, 234 insertions(+), 32 deletions(-)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index 6d0114a6b..fdd9b3808 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -6601,6 +6601,15 @@ static void update_found_devices(struct btd_adapter *adapter,
> bool name_known, discoverable;
> char addr[18];
> bool duplicate = false;
> + GSList *matched_monitors;
> +
> + /* During the background scanning, update the device only when the data
> + * match at least one Adv monitor
> + */
> + matched_monitors = btd_adv_monitor_content_filter(
> + adapter->adv_monitor_manager, data, data_len);
> + if (!adapter->discovering && !matched_monitors)
> + return;
>
> memset(&eir_data, 0, sizeof(eir_data));
> eir_parse(&eir_data, data, data_len);
> @@ -6646,18 +6655,22 @@ static void update_found_devices(struct btd_adapter *adapter,
> device_store_cached_name(dev, eir_data.name);
>
> /*
> - * Only skip devices that are not connected, are temporary and there
> - * is no active discovery session ongoing.
> + * Only skip devices that are not connected, are temporary, and there
> + * is no active discovery session ongoing and no matched Adv monitors
> */
> - if (!btd_device_is_connected(dev) && (device_is_temporary(dev) &&
> - !adapter->discovery_list)) {
> + if (!btd_device_is_connected(dev) &&
> + (device_is_temporary(dev) && !adapter->discovery_list) &&
> + !matched_monitors) {
> eir_data_free(&eir_data);
> return;
> }
>
> - /* Don't continue if not discoverable or if filter don't match */
> - if (!discoverable || (adapter->filtered_discovery &&
> - !is_filter_match(adapter->discovery_list, &eir_data, rssi))) {
> + /* If there is no matched Adv monitors, don't continue if not
> + * discoverable or if active discovery filter don't match.
> + */
> + if (!matched_monitors && (!discoverable ||
> + (adapter->filtered_discovery && !is_filter_match(
> + adapter->discovery_list, &eir_data, rssi)))) {
> eir_data_free(&eir_data);
> return;
> }
> @@ -6714,6 +6727,14 @@ static void update_found_devices(struct btd_adapter *adapter,
>
> eir_data_free(&eir_data);
>
> + /* After the device is updated, notify the matched Adv monitors */
> + if (matched_monitors) {
> + btd_adv_monitor_notify_monitors(adapter->adv_monitor_manager,
> + dev, rssi, matched_monitors);
> + g_slist_free(matched_monitors);
> + matched_monitors = NULL;
> + }
> +
> /*
> * Only if at least one client has requested discovery, maintain
> * list of found devices and name confirming for legacy devices.
> diff --git a/src/adv_monitor.c b/src/adv_monitor.c
> index 31ed30a00..fcb127cd4 100644
> --- a/src/adv_monitor.c
> +++ b/src/adv_monitor.c
> @@ -29,15 +29,12 @@
> #include "device.h"
> #include "log.h"
> #include "src/error.h"
> -#include "src/shared/ad.h"
> #include "src/shared/mgmt.h"
> #include "src/shared/queue.h"
> #include "src/shared/util.h"
>
> #include "adv_monitor.h"
>
> -static void monitor_device_free(void *data);
> -
> #define ADV_MONITOR_INTERFACE "org.bluez.AdvertisementMonitor1"
> #define ADV_MONITOR_MGR_INTERFACE "org.bluez.AdvertisementMonitorManager1"
>
> @@ -84,7 +81,7 @@ enum monitor_state {
> MONITOR_STATE_HONORED, /* Accepted by kernel */
> };
>
> -struct pattern {
> +struct btd_adv_monitor_pattern {
> uint8_t ad_type;
> uint8_t offset;
> uint8_t length;
> @@ -133,6 +130,23 @@ struct app_match_data {
> const char *path;
> };
>
> +struct adv_content_filter_info {
> + uint8_t eir_len;
> + const uint8_t *eir;
> +
> + bool matched; /* Intermediate state per monitor */
> + GSList *matched_monitors; /* List of matched monitors */
Please use struct queue on new code.
> +};
> +
> +struct adv_rssi_filter_info {
> + struct btd_device *device;
> + int8_t rssi;
> +};
> +
> +static void monitor_device_free(void *data);
> +static void adv_monitor_filter_rssi(struct adv_monitor *monitor,
> + struct btd_device *device, int8_t rssi);
> +
> const struct adv_monitor_type {
> enum monitor_type type;
> const char *name;
> @@ -155,7 +169,7 @@ static void app_reply_msg(struct adv_monitor_app *app, DBusMessage *reply)
> /* Frees a pattern */
> static void pattern_free(void *data)
> {
> - struct pattern *pattern = data;
> + struct btd_adv_monitor_pattern *pattern = data;
>
> if (!pattern)
> return;
> @@ -435,6 +449,36 @@ failed:
> return false;
> }
>
> +/* Allocates and initiates a pattern with the given content */
> +static struct btd_adv_monitor_pattern *pattern_create(
> + uint8_t ad_type, uint8_t offset, uint8_t length, const uint8_t *value)
> +{
> + struct btd_adv_monitor_pattern *pattern;
> +
> + if (offset > BT_AD_MAX_DATA_LEN - 1)
> + return NULL;
> +
> + if ((ad_type > BT_AD_3D_INFO_DATA &&
> + ad_type != BT_AD_MANUFACTURER_DATA) ||
> + ad_type < BT_AD_FLAGS) {
> + return NULL;
> + }
> +
> + if (!value || !length || offset + length > BT_AD_MAX_DATA_LEN)
> + return NULL;
> +
> + pattern = new0(struct btd_adv_monitor_pattern, 1);
> + if (!pattern)
> + return NULL;
> +
> + pattern->ad_type = ad_type;
> + pattern->offset = offset;
> + pattern->length = length;
> + memcpy(pattern->value, value, pattern->length);
I wonder why you didn't add pattern matching into bt_ad directly, that
should make it simpler to unit test such feature and enable using in
other tools as well.
> + return pattern;
> +}
> +
> /* Retrieves Patterns from the remote Adv Monitor object, verifies the values
> * and update the local Adv Monitor
> */
> @@ -464,7 +508,7 @@ static bool parse_patterns(struct adv_monitor *monitor, const char *path)
> int value_len;
> uint8_t *value;
> uint8_t offset, ad_type;
> - struct pattern *pattern;
> + struct btd_adv_monitor_pattern *pattern;
> DBusMessageIter struct_iter, value_iter;
>
> dbus_message_iter_recurse(&array_iter, &struct_iter);
> @@ -496,28 +540,10 @@ static bool parse_patterns(struct adv_monitor *monitor, const char *path)
> dbus_message_iter_get_fixed_array(&value_iter, &value,
> &value_len);
>
> - // Verify the values
> - if (offset > BT_AD_MAX_DATA_LEN - 1)
> - goto failed;
> -
> - if ((ad_type > BT_AD_3D_INFO_DATA &&
> - ad_type != BT_AD_MANUFACTURER_DATA) ||
> - ad_type < BT_AD_FLAGS) {
> - goto failed;
> - }
> -
> - if (!value || value_len <= 0 || value_len > BT_AD_MAX_DATA_LEN)
> - goto failed;
> -
> - pattern = new0(struct pattern, 1);
> + pattern = pattern_create(ad_type, offset, value_len, value);
> if (!pattern)
> goto failed;
>
> - pattern->ad_type = ad_type;
> - pattern->offset = offset;
> - pattern->length = value_len;
> - memcpy(pattern->value, value, pattern->length);
> -
> queue_push_tail(monitor->patterns, pattern);
>
> dbus_message_iter_next(&array_iter);
> @@ -952,6 +978,149 @@ void btd_adv_monitor_manager_destroy(struct btd_adv_monitor_manager *manager)
> manager_destroy(manager);
> }
>
> +static bool pattern_match(const uint8_t *eir, uint8_t eir_len,
> + const struct btd_adv_monitor_pattern *pattern)
> +{
> + const uint8_t *data;
> + uint8_t idx = 0;
> + uint8_t field_len, data_len, data_type;
> +
> + while (idx < eir_len - 1) {
> + field_len = eir[0];
> +
> + /* Check for the end of EIR */
> + if (field_len == 0)
> + break;
> +
> + idx += field_len + 1;
> +
> + /* Do not continue filtering if got incorrect length */
> + if (idx > eir_len)
> + break;
> +
> + data = &eir[2];
> + data_type = eir[1];
> + data_len = field_len - 1;
> +
> + eir += field_len + 1;
> +
> + if (data_type != pattern->ad_type)
> + continue;
> +
> + if (data_len < pattern->offset + pattern->length)
> + continue;
> +
> + if (!memcmp(data + pattern->offset, pattern->value,
> + pattern->length))
> + return true;
> + }
Perhaps something like:
struct bt_ad *bt_ad_new_with_data(uint8_t *data, uint8_t len);
and
bool bt_ad_has_pattern(struct bt_ad *ad, const struct bt_ad_pattern *pattern);
> + return false;
> +}
> +
> +/* Processes the content matching based on a pattern */
> +static void adv_match_per_pattern(void *data, void *user_data)
> +{
> + struct btd_adv_monitor_pattern *pattern = data;
> + struct adv_content_filter_info *info = user_data;
> +
> + if (!pattern || info->matched)
> + return;
> +
> + info->matched = pattern_match(info->eir, info->eir_len, pattern);
> +}
> +
> +/* Processes the content matching based pattern(s) of a monitor */
> +static void adv_match_per_monitor(void *data, void *user_data)
> +{
> + struct adv_monitor *monitor = data;
> + struct adv_content_filter_info *info = user_data;
> +
> + if (!monitor && monitor->state != MONITOR_STATE_HONORED)
> + return;
> +
> + /* Reset the intermediate matched status */
> + info->matched = false;
> +
> + if (monitor->type == MONITOR_TYPE_OR_PATTERNS) {
> + queue_foreach(monitor->patterns, adv_match_per_pattern, info);
> + if (info->matched)
> + goto matched;
> + }
> +
> + return;
> +
> +matched:
> + info->matched_monitors = g_slist_prepend(info->matched_monitors,
> + monitor);
> +}
> +
> +/* Processes the content matching for the monitor(s) of an app */
> +static void adv_match_per_app(void *data, void *user_data)
> +{
> + struct adv_monitor_app *app = data;
> +
> + if (!app)
> + return;
> +
> + queue_foreach(app->monitors, adv_match_per_monitor, user_data);
> +}
> +
> +/* Processes the content matching for every app without RSSI filtering and
> + * notifying monitors. The caller is responsible of releasing the memory of the
> + * list but not the data.
> + * Returns the list of monitors whose content match eir.
> + */
> +GSList *btd_adv_monitor_content_filter(struct btd_adv_monitor_manager *manager,
> + const uint8_t *eir, uint8_t eir_len)
> +{
> + struct adv_content_filter_info info;
> +
> + if (!manager || !eir || !eir_len)
> + return NULL;
> +
> + info.eir_len = eir_len;
> + info.eir = eir;
> + info.matched_monitors = NULL;
> +
> + queue_foreach(manager->apps, adv_match_per_app, &info);
> +
> + return info.matched_monitors;
> +}
> +
> +/* Wraps adv_monitor_filter_rssi() to processes the content-matched monitor with
> + * RSSI filtering and notifies it on device found/lost event
> + */
> +static void monitor_filter_rssi(gpointer a, gpointer b)
> +{
> + struct adv_monitor *monitor = a;
> + struct adv_rssi_filter_info *info = b;
> +
> + if (!monitor || !info)
> + return;
> +
> + adv_monitor_filter_rssi(monitor, info->device, info->rssi);
> +}
> +
> +/* Processes every content-matched monitor with RSSI filtering and notifies on
> + * device found/lost event. The caller is responsible of releasing the memory
> + * of matched_monitors list but not its data.
> + */
> +void btd_adv_monitor_notify_monitors(struct btd_adv_monitor_manager *manager,
> + struct btd_device *device, int8_t rssi,
> + GSList *matched_monitors)
> +{
> + struct adv_rssi_filter_info info;
> +
> + if (!manager || !device || !matched_monitors)
> + return;
> +
> + info.device = device;
> + info.rssi = rssi;
> +
> + g_slist_foreach(matched_monitors, monitor_filter_rssi, &info);
> +}
> +
> /* Matches a device based on btd_device object */
> static bool monitor_device_match(const void *a, const void *b)
> {
> diff --git a/src/adv_monitor.h b/src/adv_monitor.h
> index 13d5d7282..e2482e11e 100644
> --- a/src/adv_monitor.h
> +++ b/src/adv_monitor.h
> @@ -11,16 +11,28 @@
> #ifndef __ADV_MONITOR_H
> #define __ADV_MONITOR_H
>
> +#include <glib.h>
> +
> +#include "src/shared/ad.h"
> +
> struct mgmt;
> struct btd_device;
> struct btd_adapter;
> struct btd_adv_monitor_manager;
> +struct btd_adv_monitor_pattern;
>
> struct btd_adv_monitor_manager *btd_adv_monitor_manager_create(
> struct btd_adapter *adapter,
> struct mgmt *mgmt);
> void btd_adv_monitor_manager_destroy(struct btd_adv_monitor_manager *manager);
>
> +GSList *btd_adv_monitor_content_filter(struct btd_adv_monitor_manager *manager,
> + const uint8_t *eir, uint8_t eir_len);
> +
> +void btd_adv_monitor_notify_monitors(struct btd_adv_monitor_manager *manager,
> + struct btd_device *device, int8_t rssi,
> + GSList *matched_monitors);
> +
> void btd_adv_monitor_device_remove(struct btd_adv_monitor_manager *manager,
> struct btd_device *device);
>
> --
> 2.26.2
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [BlueZ PATCH v6 2/6] adv_monitor: Implement Adv matching based on stored monitors
2020-10-07 6:21 ` Luiz Augusto von Dentz
@ 2020-10-12 21:21 ` Miao-chen Chou
2020-10-13 23:45 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 15+ messages in thread
From: Miao-chen Chou @ 2020-10-12 21:21 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: Bluetooth Kernel Mailing List, Luiz Augusto von Dentz,
Howard Chung, ChromeOS Bluetooth Upstreaming, Alain Michaud,
Marcel Holtmann, Manish Mandlik, Abhishek Pandit-Subedi
Hi Luiz,
I did think of adding pattern to bt_ad at the beginning, but here are
reasons why I ended up with hosting the definition of pattern in
adv_monitor.
(1) Pattern is specific to monitoring purpose. An advertisement
should not include patterns as its fields due to that fact that a
pattern hosts an offset. So I wasn't sure about its justification to
be placed in shared/ad. But if you foresee that it would be reused,
then I am more than happy to add it to shared/ad.
(2) Introducing helpers as you suggested below indeed make it more
unittestable. However, it also implied that EIR data (this is in fact
AD data) needs to be parsed into a new bt_ad for pattern comparison,
and I didn't see an obvious benefit of converting EIR data into a
bt_ad just for the comparison.
Maybe we can add a struct bt_ad_pattern in shared/ad.h and introduce
the following two functions. What do you think?
struct bt_ad_pattern *bt_ad_pattern_new(uint8_t type, size_t offset,
size_t len, const void *data);
/* |data| is one single AD data field so that we can avoid converting
EIR data to bt_ad */
bool bt_ad_pattern bt_ad_pattern_match(struct bt_ad_pattern *pattern,
void *data, size_t len);
On Tue, Oct 6, 2020 at 11:21 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Miao,
>
> On Tue, Oct 6, 2020 at 5:17 PM Miao-chen Chou <mcchou@chromium.org> wrote:
> >
> > This implements create an entry point in adapter to start the matching of
> > Adv based on all monitors and invoke the RSSI tracking for Adv reporting.
> >
> > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > Reviewed-by: Alain Michaud <alainm@chromium.org>
> > Reviewed-by: Manish Mandlik <mmandlik@chromium.org>
> > ---
> >
> > Changes in v6:
> > - Fix the termination condition of AD data paring and remove unnecessary
> > length check
> >
> > Changes in v5:
> > - Remove unittest helper functions
> >
> > Changes in v3:
> > - Remove unused variables
> > - Fix signature of queue_find()
> >
> > src/adapter.c | 35 ++++++--
> > src/adv_monitor.c | 219 ++++++++++++++++++++++++++++++++++++++++------
> > src/adv_monitor.h | 12 +++
> > 3 files changed, 234 insertions(+), 32 deletions(-)
> >
> > diff --git a/src/adapter.c b/src/adapter.c
> > index 6d0114a6b..fdd9b3808 100644
> > --- a/src/adapter.c
> > +++ b/src/adapter.c
> > @@ -6601,6 +6601,15 @@ static void update_found_devices(struct btd_adapter *adapter,
> > bool name_known, discoverable;
> > char addr[18];
> > bool duplicate = false;
> > + GSList *matched_monitors;
> > +
> > + /* During the background scanning, update the device only when the data
> > + * match at least one Adv monitor
> > + */
> > + matched_monitors = btd_adv_monitor_content_filter(
> > + adapter->adv_monitor_manager, data, data_len);
> > + if (!adapter->discovering && !matched_monitors)
> > + return;
> >
> > memset(&eir_data, 0, sizeof(eir_data));
> > eir_parse(&eir_data, data, data_len);
> > @@ -6646,18 +6655,22 @@ static void update_found_devices(struct btd_adapter *adapter,
> > device_store_cached_name(dev, eir_data.name);
> >
> > /*
> > - * Only skip devices that are not connected, are temporary and there
> > - * is no active discovery session ongoing.
> > + * Only skip devices that are not connected, are temporary, and there
> > + * is no active discovery session ongoing and no matched Adv monitors
> > */
> > - if (!btd_device_is_connected(dev) && (device_is_temporary(dev) &&
> > - !adapter->discovery_list)) {
> > + if (!btd_device_is_connected(dev) &&
> > + (device_is_temporary(dev) && !adapter->discovery_list) &&
> > + !matched_monitors) {
> > eir_data_free(&eir_data);
> > return;
> > }
> >
> > - /* Don't continue if not discoverable or if filter don't match */
> > - if (!discoverable || (adapter->filtered_discovery &&
> > - !is_filter_match(adapter->discovery_list, &eir_data, rssi))) {
> > + /* If there is no matched Adv monitors, don't continue if not
> > + * discoverable or if active discovery filter don't match.
> > + */
> > + if (!matched_monitors && (!discoverable ||
> > + (adapter->filtered_discovery && !is_filter_match(
> > + adapter->discovery_list, &eir_data, rssi)))) {
> > eir_data_free(&eir_data);
> > return;
> > }
> > @@ -6714,6 +6727,14 @@ static void update_found_devices(struct btd_adapter *adapter,
> >
> > eir_data_free(&eir_data);
> >
> > + /* After the device is updated, notify the matched Adv monitors */
> > + if (matched_monitors) {
> > + btd_adv_monitor_notify_monitors(adapter->adv_monitor_manager,
> > + dev, rssi, matched_monitors);
> > + g_slist_free(matched_monitors);
> > + matched_monitors = NULL;
> > + }
> > +
> > /*
> > * Only if at least one client has requested discovery, maintain
> > * list of found devices and name confirming for legacy devices.
> > diff --git a/src/adv_monitor.c b/src/adv_monitor.c
> > index 31ed30a00..fcb127cd4 100644
> > --- a/src/adv_monitor.c
> > +++ b/src/adv_monitor.c
> > @@ -29,15 +29,12 @@
> > #include "device.h"
> > #include "log.h"
> > #include "src/error.h"
> > -#include "src/shared/ad.h"
> > #include "src/shared/mgmt.h"
> > #include "src/shared/queue.h"
> > #include "src/shared/util.h"
> >
> > #include "adv_monitor.h"
> >
> > -static void monitor_device_free(void *data);
> > -
> > #define ADV_MONITOR_INTERFACE "org.bluez.AdvertisementMonitor1"
> > #define ADV_MONITOR_MGR_INTERFACE "org.bluez.AdvertisementMonitorManager1"
> >
> > @@ -84,7 +81,7 @@ enum monitor_state {
> > MONITOR_STATE_HONORED, /* Accepted by kernel */
> > };
> >
> > -struct pattern {
> > +struct btd_adv_monitor_pattern {
> > uint8_t ad_type;
> > uint8_t offset;
> > uint8_t length;
> > @@ -133,6 +130,23 @@ struct app_match_data {
> > const char *path;
> > };
> >
> > +struct adv_content_filter_info {
> > + uint8_t eir_len;
> > + const uint8_t *eir;
> > +
> > + bool matched; /* Intermediate state per monitor */
> > + GSList *matched_monitors; /* List of matched monitors */
>
> Please use struct queue on new code.
Addressed in my local v7.
>
> > +};
> > +
> > +struct adv_rssi_filter_info {
> > + struct btd_device *device;
> > + int8_t rssi;
> > +};
> > +
> > +static void monitor_device_free(void *data);
> > +static void adv_monitor_filter_rssi(struct adv_monitor *monitor,
> > + struct btd_device *device, int8_t rssi);
> > +
> > const struct adv_monitor_type {
> > enum monitor_type type;
> > const char *name;
> > @@ -155,7 +169,7 @@ static void app_reply_msg(struct adv_monitor_app *app, DBusMessage *reply)
> > /* Frees a pattern */
> > static void pattern_free(void *data)
> > {
> > - struct pattern *pattern = data;
> > + struct btd_adv_monitor_pattern *pattern = data;
> >
> > if (!pattern)
> > return;
> > @@ -435,6 +449,36 @@ failed:
> > return false;
> > }
> >
> > +/* Allocates and initiates a pattern with the given content */
> > +static struct btd_adv_monitor_pattern *pattern_create(
> > + uint8_t ad_type, uint8_t offset, uint8_t length, const uint8_t *value)
> > +{
> > + struct btd_adv_monitor_pattern *pattern;
> > +
> > + if (offset > BT_AD_MAX_DATA_LEN - 1)
> > + return NULL;
> > +
> > + if ((ad_type > BT_AD_3D_INFO_DATA &&
> > + ad_type != BT_AD_MANUFACTURER_DATA) ||
> > + ad_type < BT_AD_FLAGS) {
> > + return NULL;
> > + }
> > +
> > + if (!value || !length || offset + length > BT_AD_MAX_DATA_LEN)
> > + return NULL;
> > +
> > + pattern = new0(struct btd_adv_monitor_pattern, 1);
> > + if (!pattern)
> > + return NULL;
> > +
> > + pattern->ad_type = ad_type;
> > + pattern->offset = offset;
> > + pattern->length = length;
> > + memcpy(pattern->value, value, pattern->length);
>
> I wonder why you didn't add pattern matching into bt_ad directly, that
> should make it simpler to unit test such feature and enable using in
> other tools as well.
>
Please see my reply above.
> > + return pattern;
> > +}
> > +
> > /* Retrieves Patterns from the remote Adv Monitor object, verifies the values
> > * and update the local Adv Monitor
> > */
> > @@ -464,7 +508,7 @@ static bool parse_patterns(struct adv_monitor *monitor, const char *path)
> > int value_len;
> > uint8_t *value;
> > uint8_t offset, ad_type;
> > - struct pattern *pattern;
> > + struct btd_adv_monitor_pattern *pattern;
> > DBusMessageIter struct_iter, value_iter;
> >
> > dbus_message_iter_recurse(&array_iter, &struct_iter);
> > @@ -496,28 +540,10 @@ static bool parse_patterns(struct adv_monitor *monitor, const char *path)
> > dbus_message_iter_get_fixed_array(&value_iter, &value,
> > &value_len);
> >
> > - // Verify the values
> > - if (offset > BT_AD_MAX_DATA_LEN - 1)
> > - goto failed;
> > -
> > - if ((ad_type > BT_AD_3D_INFO_DATA &&
> > - ad_type != BT_AD_MANUFACTURER_DATA) ||
> > - ad_type < BT_AD_FLAGS) {
> > - goto failed;
> > - }
> > -
> > - if (!value || value_len <= 0 || value_len > BT_AD_MAX_DATA_LEN)
> > - goto failed;
> > -
> > - pattern = new0(struct pattern, 1);
> > + pattern = pattern_create(ad_type, offset, value_len, value);
> > if (!pattern)
> > goto failed;
> >
> > - pattern->ad_type = ad_type;
> > - pattern->offset = offset;
> > - pattern->length = value_len;
> > - memcpy(pattern->value, value, pattern->length);
> > -
> > queue_push_tail(monitor->patterns, pattern);
> >
> > dbus_message_iter_next(&array_iter);
> > @@ -952,6 +978,149 @@ void btd_adv_monitor_manager_destroy(struct btd_adv_monitor_manager *manager)
> > manager_destroy(manager);
> > }
> >
> > +static bool pattern_match(const uint8_t *eir, uint8_t eir_len,
> > + const struct btd_adv_monitor_pattern *pattern)
> > +{
> > + const uint8_t *data;
> > + uint8_t idx = 0;
> > + uint8_t field_len, data_len, data_type;
> > +
> > + while (idx < eir_len - 1) {
> > + field_len = eir[0];
> > +
> > + /* Check for the end of EIR */
> > + if (field_len == 0)
> > + break;
> > +
> > + idx += field_len + 1;
> > +
> > + /* Do not continue filtering if got incorrect length */
> > + if (idx > eir_len)
> > + break;
> > +
> > + data = &eir[2];
> > + data_type = eir[1];
> > + data_len = field_len - 1;
> > +
> > + eir += field_len + 1;
> > +
> > + if (data_type != pattern->ad_type)
> > + continue;
> > +
> > + if (data_len < pattern->offset + pattern->length)
> > + continue;
> > +
> > + if (!memcmp(data + pattern->offset, pattern->value,
> > + pattern->length))
> > + return true;
> > + }
>
> Perhaps something like:
>
> struct bt_ad *bt_ad_new_with_data(uint8_t *data, uint8_t len);
>
> and
>
> bool bt_ad_has_pattern(struct bt_ad *ad, const struct bt_ad_pattern *pattern);
>
Please see my reply above.
> > + return false;
> > +}
> > +
> > +/* Processes the content matching based on a pattern */
> > +static void adv_match_per_pattern(void *data, void *user_data)
> > +{
> > + struct btd_adv_monitor_pattern *pattern = data;
> > + struct adv_content_filter_info *info = user_data;
> > +
> > + if (!pattern || info->matched)
> > + return;
> > +
> > + info->matched = pattern_match(info->eir, info->eir_len, pattern);
> > +}
> > +
> > +/* Processes the content matching based pattern(s) of a monitor */
> > +static void adv_match_per_monitor(void *data, void *user_data)
> > +{
> > + struct adv_monitor *monitor = data;
> > + struct adv_content_filter_info *info = user_data;
> > +
> > + if (!monitor && monitor->state != MONITOR_STATE_HONORED)
> > + return;
> > +
> > + /* Reset the intermediate matched status */
> > + info->matched = false;
> > +
> > + if (monitor->type == MONITOR_TYPE_OR_PATTERNS) {
> > + queue_foreach(monitor->patterns, adv_match_per_pattern, info);
> > + if (info->matched)
> > + goto matched;
> > + }
> > +
> > + return;
> > +
> > +matched:
> > + info->matched_monitors = g_slist_prepend(info->matched_monitors,
> > + monitor);
> > +}
> > +
> > +/* Processes the content matching for the monitor(s) of an app */
> > +static void adv_match_per_app(void *data, void *user_data)
> > +{
> > + struct adv_monitor_app *app = data;
> > +
> > + if (!app)
> > + return;
> > +
> > + queue_foreach(app->monitors, adv_match_per_monitor, user_data);
> > +}
> > +
> > +/* Processes the content matching for every app without RSSI filtering and
> > + * notifying monitors. The caller is responsible of releasing the memory of the
> > + * list but not the data.
> > + * Returns the list of monitors whose content match eir.
> > + */
> > +GSList *btd_adv_monitor_content_filter(struct btd_adv_monitor_manager *manager,
> > + const uint8_t *eir, uint8_t eir_len)
> > +{
> > + struct adv_content_filter_info info;
> > +
> > + if (!manager || !eir || !eir_len)
> > + return NULL;
> > +
> > + info.eir_len = eir_len;
> > + info.eir = eir;
> > + info.matched_monitors = NULL;
> > +
> > + queue_foreach(manager->apps, adv_match_per_app, &info);
> > +
> > + return info.matched_monitors;
> > +}
> > +
> > +/* Wraps adv_monitor_filter_rssi() to processes the content-matched monitor with
> > + * RSSI filtering and notifies it on device found/lost event
> > + */
> > +static void monitor_filter_rssi(gpointer a, gpointer b)
> > +{
> > + struct adv_monitor *monitor = a;
> > + struct adv_rssi_filter_info *info = b;
> > +
> > + if (!monitor || !info)
> > + return;
> > +
> > + adv_monitor_filter_rssi(monitor, info->device, info->rssi);
> > +}
> > +
> > +/* Processes every content-matched monitor with RSSI filtering and notifies on
> > + * device found/lost event. The caller is responsible of releasing the memory
> > + * of matched_monitors list but not its data.
> > + */
> > +void btd_adv_monitor_notify_monitors(struct btd_adv_monitor_manager *manager,
> > + struct btd_device *device, int8_t rssi,
> > + GSList *matched_monitors)
> > +{
> > + struct adv_rssi_filter_info info;
> > +
> > + if (!manager || !device || !matched_monitors)
> > + return;
> > +
> > + info.device = device;
> > + info.rssi = rssi;
> > +
> > + g_slist_foreach(matched_monitors, monitor_filter_rssi, &info);
> > +}
> > +
> > /* Matches a device based on btd_device object */
> > static bool monitor_device_match(const void *a, const void *b)
> > {
> > diff --git a/src/adv_monitor.h b/src/adv_monitor.h
> > index 13d5d7282..e2482e11e 100644
> > --- a/src/adv_monitor.h
> > +++ b/src/adv_monitor.h
> > @@ -11,16 +11,28 @@
> > #ifndef __ADV_MONITOR_H
> > #define __ADV_MONITOR_H
> >
> > +#include <glib.h>
> > +
> > +#include "src/shared/ad.h"
> > +
> > struct mgmt;
> > struct btd_device;
> > struct btd_adapter;
> > struct btd_adv_monitor_manager;
> > +struct btd_adv_monitor_pattern;
> >
> > struct btd_adv_monitor_manager *btd_adv_monitor_manager_create(
> > struct btd_adapter *adapter,
> > struct mgmt *mgmt);
> > void btd_adv_monitor_manager_destroy(struct btd_adv_monitor_manager *manager);
> >
> > +GSList *btd_adv_monitor_content_filter(struct btd_adv_monitor_manager *manager,
> > + const uint8_t *eir, uint8_t eir_len);
> > +
> > +void btd_adv_monitor_notify_monitors(struct btd_adv_monitor_manager *manager,
> > + struct btd_device *device, int8_t rssi,
> > + GSList *matched_monitors);
> > +
> > void btd_adv_monitor_device_remove(struct btd_adv_monitor_manager *manager,
> > struct btd_device *device);
> >
> > --
> > 2.26.2
> >
>
>
> --
> Luiz Augusto von Dentz
Thanks,
Miao
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [BlueZ PATCH v6 2/6] adv_monitor: Implement Adv matching based on stored monitors
2020-10-12 21:21 ` Miao-chen Chou
@ 2020-10-13 23:45 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 15+ messages in thread
From: Luiz Augusto von Dentz @ 2020-10-13 23:45 UTC (permalink / raw)
To: Miao-chen Chou
Cc: Bluetooth Kernel Mailing List, Luiz Augusto von Dentz,
Howard Chung, ChromeOS Bluetooth Upstreaming, Alain Michaud,
Marcel Holtmann, Manish Mandlik, Abhishek Pandit-Subedi
Hi Miao-chen,
On Mon, Oct 12, 2020 at 2:21 PM Miao-chen Chou <mcchou@chromium.org> wrote:
>
> Hi Luiz,
>
> I did think of adding pattern to bt_ad at the beginning, but here are
> reasons why I ended up with hosting the definition of pattern in
> adv_monitor.
> (1) Pattern is specific to monitoring purpose. An advertisement
> should not include patterns as its fields due to that fact that a
> pattern hosts an offset. So I wasn't sure about its justification to
> be placed in shared/ad. But if you foresee that it would be reused,
> then I am more than happy to add it to shared/ad.
> (2) Introducing helpers as you suggested below indeed make it more
> unittestable. However, it also implied that EIR data (this is in fact
> AD data) needs to be parsed into a new bt_ad for pattern comparison,
> and I didn't see an obvious benefit of converting EIR data into a
> bt_ad just for the comparison.
Regarding (1) like I said you will need another struct to store the
offset and length for doing the pattern matching but otherwise it is
pretty similar, regarding (2) there is actually a real benefit that we
could in the future drop the eir_ helpers and unit test just the bt_ad
including pattern matching as well.
> Maybe we can add a struct bt_ad_pattern in shared/ad.h and introduce
> the following two functions. What do you think?
>
> struct bt_ad_pattern *bt_ad_pattern_new(uint8_t type, size_t offset,
> size_t len, const void *data);
Not sure I follow you here, that doesn't seem to be about the parsing
of the AD but the creation of an object for matching, not sure we need
that since you can probably just pass a stack variable directly, in
any case you would need a free as well.
> /* |data| is one single AD data field so that we can avoid converting
> EIR data to bt_ad */
> bool bt_ad_pattern bt_ad_pattern_match(struct bt_ad_pattern *pattern,
> void *data, size_t len);
You may have more than one entry of the same type, anyway if we do
have something like bt_ad_new_with_data that means we can pass it
around and not have to reparse it when doing custom filtering. We
might as well have a callback that does get called every time there is
a match, also it may be possible to pass all the pattern at once in
which case we can pass the pattern which matched along with the data
thus having the call it multiple times for each monitor.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [BlueZ PATCH v6 3/6] adapter: Clear all Adv monitors upon bring-up
2020-10-07 0:14 [BlueZ PATCH v6 1/6] adv_monitor: Implement RSSI Filter logic for background scanning Miao-chen Chou
2020-10-07 0:14 ` [BlueZ PATCH v6 2/6] adv_monitor: Implement Adv matching based on stored monitors Miao-chen Chou
@ 2020-10-07 0:14 ` Miao-chen Chou
2020-10-07 0:14 ` [BlueZ PATCH v6 4/6] adv_monitor: Implement Add Adv Patterns Monitor cmd handler Miao-chen Chou
` (4 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: Miao-chen Chou @ 2020-10-07 0:14 UTC (permalink / raw)
To: Bluetooth Kernel Mailing List
Cc: Luiz Augusto von Dentz, Howard Chung,
chromeos-bluetooth-upstreaming, Alain Michaud, Marcel Holtmann,
Manish Mandlik, Miao-chen Chou
This clears all Adv monitors upon daemon bring-up by issuing
MGMT_OP_REMOVE_ADV_MONITOR command with monitor_handle 0.
The following test was performed:
- Add an Adv Monitor using btmgmt, restart bluetoothd and observe the
monitor got removed.
Reviewed-by: Alain Michaud <alainm@chromium.org>
Reviewed-by: Manish Mandlik <mmandlik@chromium.org>
Reviewed-by: Howard Chung <howardchung@google.com>
---
(no changes since v1)
src/adapter.c | 39 +++++++++++++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)
diff --git a/src/adapter.c b/src/adapter.c
index fdd9b3808..c7179fc2e 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -9500,6 +9500,43 @@ failed:
btd_adapter_unref(adapter);
}
+static void reset_adv_monitors_complete(uint8_t status, uint16_t length,
+ const void *param, void *user_data)
+{
+ const struct mgmt_rp_remove_adv_monitor *rp = param;
+
+ if (status != MGMT_STATUS_SUCCESS) {
+ error("Failed to reset Adv Monitors: %s (0x%02x)",
+ mgmt_errstr(status), status);
+ return;
+ }
+
+ if (length < sizeof(*rp)) {
+ error("Wrong size of remove Adv Monitor response for reset "
+ "all Adv Monitors");
+ return;
+ }
+
+ DBG("Removed all Adv Monitors");
+}
+
+static void reset_adv_monitors(uint16_t index)
+{
+ struct mgmt_cp_remove_adv_monitor cp;
+
+ DBG("sending remove Adv Monitor command with handle 0");
+
+ /* Handle 0 indicates to remove all */
+ cp.monitor_handle = 0;
+ if (mgmt_send(mgmt_master, MGMT_OP_REMOVE_ADV_MONITOR, index,
+ sizeof(cp), &cp, reset_adv_monitors_complete, NULL,
+ NULL) > 0) {
+ return;
+ }
+
+ error("Failed to reset Adv Monitors");
+}
+
static void index_added(uint16_t index, uint16_t length, const void *param,
void *user_data)
{
@@ -9514,6 +9551,8 @@ static void index_added(uint16_t index, uint16_t length, const void *param,
return;
}
+ reset_adv_monitors(index);
+
adapter = btd_adapter_new(index);
if (!adapter) {
btd_error(index,
--
2.26.2
^ permalink raw reply related [flat|nested] 15+ messages in thread* [BlueZ PATCH v6 4/6] adv_monitor: Implement Add Adv Patterns Monitor cmd handler
2020-10-07 0:14 [BlueZ PATCH v6 1/6] adv_monitor: Implement RSSI Filter logic for background scanning Miao-chen Chou
2020-10-07 0:14 ` [BlueZ PATCH v6 2/6] adv_monitor: Implement Adv matching based on stored monitors Miao-chen Chou
2020-10-07 0:14 ` [BlueZ PATCH v6 3/6] adapter: Clear all Adv monitors upon bring-up Miao-chen Chou
@ 2020-10-07 0:14 ` Miao-chen Chou
2020-10-07 6:26 ` Luiz Augusto von Dentz
2020-10-07 0:14 ` [BlueZ PATCH v6 5/6] adv_monitor: Fix return type of RegisterMonitor() method Miao-chen Chou
` (3 subsequent siblings)
6 siblings, 1 reply; 15+ messages in thread
From: Miao-chen Chou @ 2020-10-07 0:14 UTC (permalink / raw)
To: Bluetooth Kernel Mailing List
Cc: Luiz Augusto von Dentz, Howard Chung,
chromeos-bluetooth-upstreaming, Alain Michaud, Marcel Holtmann,
Manish Mandlik, Miao-chen Chou
From: Howard Chung <howardchung@google.com>
- Send the MGMT_OP command to kernel upon registration of a Adv patterns
monitor.
- Call Activate() or Release() to client depending on the reply from
kernel
the call through syslog
Reviewed-by: Alain Michaud <alainm@chromium.org>
Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
Reviewed-by: Manish Mandlik <mmandlik@chromium.org>
---
(no changes since v1)
src/adv_monitor.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 66 insertions(+), 1 deletion(-)
diff --git a/src/adv_monitor.c b/src/adv_monitor.c
index fcb127cd4..582cc9a46 100644
--- a/src/adv_monitor.c
+++ b/src/adv_monitor.c
@@ -588,11 +588,59 @@ done:
return monitor->state != MONITOR_STATE_FAILED;
}
+/* Handles the callback of Add Adv Patterns Monitor command */
+static void add_adv_patterns_monitor_cb(uint8_t status, uint16_t length,
+ const void *param, void *user_data)
+{
+ const struct mgmt_rp_add_adv_patterns_monitor *rp = param;
+ struct adv_monitor *monitor = user_data;
+ uint16_t adapter_id = monitor->app->manager->adapter_id;
+
+ if (status != MGMT_STATUS_SUCCESS || !param) {
+ btd_error(adapter_id, "Failed to Add Adv Patterns Monitor "
+ "with status 0x%02x", status);
+ monitor_release(monitor, NULL);
+ return;
+ }
+
+ if (length < sizeof(*rp)) {
+ btd_error(adapter_id, "Wrong size of Add Adv Patterns Monitor "
+ "response");
+ monitor_release(monitor, NULL);
+ return;
+ }
+
+ monitor->state = MONITOR_STATE_HONORED;
+
+ DBG("Calling Activate() on Adv Monitor of owner %s at path %s",
+ monitor->app->owner, monitor->path);
+
+ g_dbus_proxy_method_call(monitor->proxy, "Activate", NULL, NULL, NULL,
+ NULL);
+
+ DBG("Adv Monitor with handle:0x%04x added",
+ le16_to_cpu(rp->monitor_handle));
+}
+
+static void monitor_copy_patterns(void *data, void *user_data)
+{
+ struct btd_adv_monitor_pattern *pattern = data;
+ struct mgmt_cp_add_adv_monitor *cp = user_data;
+
+ if (!pattern)
+ return;
+
+ memcpy(cp->patterns + cp->pattern_count, pattern, sizeof(*pattern));
+ cp->pattern_count++;
+}
+
/* Handles an Adv Monitor D-Bus proxy added event */
static void monitor_proxy_added_cb(GDBusProxy *proxy, void *user_data)
{
struct adv_monitor *monitor;
struct adv_monitor_app *app = user_data;
+ struct mgmt_cp_add_adv_monitor *cp = NULL;
+ uint8_t pattern_count, cp_len;
uint16_t adapter_id = app->manager->adapter_id;
const char *path = g_dbus_proxy_get_path(proxy);
const char *iface = g_dbus_proxy_get_interface(proxy);
@@ -625,7 +673,24 @@ static void monitor_proxy_added_cb(GDBusProxy *proxy, void *user_data)
queue_push_tail(app->monitors, monitor);
+ pattern_count = queue_length(monitor->patterns);
+ cp_len = sizeof(struct mgmt_cp_add_adv_monitor) +
+ pattern_count * sizeof(struct mgmt_adv_pattern);
+
+ cp = malloc0(cp_len);
+ queue_foreach(monitor->patterns, monitor_copy_patterns, cp);
+
+ if (!mgmt_send(app->manager->mgmt, MGMT_OP_ADD_ADV_PATTERNS_MONITOR,
+ adapter_id, cp_len, cp, add_adv_patterns_monitor_cb,
+ monitor, NULL)) {
+ error("Unable to send Add Adv Patterns Monitor command");
+ goto done;
+ }
+
DBG("Adv Monitor allocated for the object at path %s", path);
+
+done:
+ free(cp);
}
/* Handles the removal of an Adv Monitor D-Bus proxy */
@@ -1036,7 +1101,7 @@ static void adv_match_per_monitor(void *data, void *user_data)
struct adv_monitor *monitor = data;
struct adv_content_filter_info *info = user_data;
- if (!monitor && monitor->state != MONITOR_STATE_HONORED)
+ if (!monitor || monitor->state != MONITOR_STATE_HONORED)
return;
/* Reset the intermediate matched status */
--
2.26.2
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [BlueZ PATCH v6 4/6] adv_monitor: Implement Add Adv Patterns Monitor cmd handler
2020-10-07 0:14 ` [BlueZ PATCH v6 4/6] adv_monitor: Implement Add Adv Patterns Monitor cmd handler Miao-chen Chou
@ 2020-10-07 6:26 ` Luiz Augusto von Dentz
2020-10-12 21:35 ` Miao-chen Chou
0 siblings, 1 reply; 15+ messages in thread
From: Luiz Augusto von Dentz @ 2020-10-07 6:26 UTC (permalink / raw)
To: Miao-chen Chou
Cc: Bluetooth Kernel Mailing List, Luiz Augusto von Dentz,
Howard Chung, ChromeOS Bluetooth Upstreaming, Alain Michaud,
Marcel Holtmann, Manish Mandlik
Hi Miao,
On Tue, Oct 6, 2020 at 5:17 PM Miao-chen Chou <mcchou@chromium.org> wrote:
>
> From: Howard Chung <howardchung@google.com>
>
> - Send the MGMT_OP command to kernel upon registration of a Adv patterns
> monitor.
> - Call Activate() or Release() to client depending on the reply from
> kernel
>
> the call through syslog
>
> Reviewed-by: Alain Michaud <alainm@chromium.org>
> Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> Reviewed-by: Manish Mandlik <mmandlik@chromium.org>
> ---
>
> (no changes since v1)
>
> src/adv_monitor.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 66 insertions(+), 1 deletion(-)
>
> diff --git a/src/adv_monitor.c b/src/adv_monitor.c
> index fcb127cd4..582cc9a46 100644
> --- a/src/adv_monitor.c
> +++ b/src/adv_monitor.c
> @@ -588,11 +588,59 @@ done:
> return monitor->state != MONITOR_STATE_FAILED;
> }
>
> +/* Handles the callback of Add Adv Patterns Monitor command */
> +static void add_adv_patterns_monitor_cb(uint8_t status, uint16_t length,
> + const void *param, void *user_data)
> +{
> + const struct mgmt_rp_add_adv_patterns_monitor *rp = param;
> + struct adv_monitor *monitor = user_data;
> + uint16_t adapter_id = monitor->app->manager->adapter_id;
> +
> + if (status != MGMT_STATUS_SUCCESS || !param) {
> + btd_error(adapter_id, "Failed to Add Adv Patterns Monitor "
> + "with status 0x%02x", status);
> + monitor_release(monitor, NULL);
> + return;
> + }
> +
> + if (length < sizeof(*rp)) {
> + btd_error(adapter_id, "Wrong size of Add Adv Patterns Monitor "
> + "response");
> + monitor_release(monitor, NULL);
> + return;
> + }
> +
> + monitor->state = MONITOR_STATE_HONORED;
I would reword this state to ACTIVE instead of HONORED as it seems
more consistent.
> + DBG("Calling Activate() on Adv Monitor of owner %s at path %s",
> + monitor->app->owner, monitor->path);
> +
> + g_dbus_proxy_method_call(monitor->proxy, "Activate", NULL, NULL, NULL,
> + NULL);
> +
> + DBG("Adv Monitor with handle:0x%04x added",
> + le16_to_cpu(rp->monitor_handle));
> +}
> +
> +static void monitor_copy_patterns(void *data, void *user_data)
> +{
> + struct btd_adv_monitor_pattern *pattern = data;
> + struct mgmt_cp_add_adv_monitor *cp = user_data;
> +
> + if (!pattern)
> + return;
> +
> + memcpy(cp->patterns + cp->pattern_count, pattern, sizeof(*pattern));
> + cp->pattern_count++;
> +}
> +
> /* Handles an Adv Monitor D-Bus proxy added event */
> static void monitor_proxy_added_cb(GDBusProxy *proxy, void *user_data)
> {
> struct adv_monitor *monitor;
> struct adv_monitor_app *app = user_data;
> + struct mgmt_cp_add_adv_monitor *cp = NULL;
> + uint8_t pattern_count, cp_len;
> uint16_t adapter_id = app->manager->adapter_id;
> const char *path = g_dbus_proxy_get_path(proxy);
> const char *iface = g_dbus_proxy_get_interface(proxy);
> @@ -625,7 +673,24 @@ static void monitor_proxy_added_cb(GDBusProxy *proxy, void *user_data)
>
> queue_push_tail(app->monitors, monitor);
>
> + pattern_count = queue_length(monitor->patterns);
> + cp_len = sizeof(struct mgmt_cp_add_adv_monitor) +
> + pattern_count * sizeof(struct mgmt_adv_pattern);
> +
> + cp = malloc0(cp_len);
> + queue_foreach(monitor->patterns, monitor_copy_patterns, cp);
> +
> + if (!mgmt_send(app->manager->mgmt, MGMT_OP_ADD_ADV_PATTERNS_MONITOR,
> + adapter_id, cp_len, cp, add_adv_patterns_monitor_cb,
> + monitor, NULL)) {
> + error("Unable to send Add Adv Patterns Monitor command");
> + goto done;
> + }
> +
> DBG("Adv Monitor allocated for the object at path %s", path);
> +
> +done:
> + free(cp);
> }
>
> /* Handles the removal of an Adv Monitor D-Bus proxy */
> @@ -1036,7 +1101,7 @@ static void adv_match_per_monitor(void *data, void *user_data)
> struct adv_monitor *monitor = data;
> struct adv_content_filter_info *info = user_data;
>
> - if (!monitor && monitor->state != MONITOR_STATE_HONORED)
> + if (!monitor || monitor->state != MONITOR_STATE_HONORED)
> return;
>
> /* Reset the intermediate matched status */
> --
> 2.26.2
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [BlueZ PATCH v6 4/6] adv_monitor: Implement Add Adv Patterns Monitor cmd handler
2020-10-07 6:26 ` Luiz Augusto von Dentz
@ 2020-10-12 21:35 ` Miao-chen Chou
0 siblings, 0 replies; 15+ messages in thread
From: Miao-chen Chou @ 2020-10-12 21:35 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: Bluetooth Kernel Mailing List, Luiz Augusto von Dentz,
Howard Chung, ChromeOS Bluetooth Upstreaming, Alain Michaud,
Marcel Holtmann, Manish Mandlik
Hi Luiz,
On Tue, Oct 6, 2020 at 11:26 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Miao,
>
> On Tue, Oct 6, 2020 at 5:17 PM Miao-chen Chou <mcchou@chromium.org> wrote:
> >
> > From: Howard Chung <howardchung@google.com>
> >
> > - Send the MGMT_OP command to kernel upon registration of a Adv patterns
> > monitor.
> > - Call Activate() or Release() to client depending on the reply from
> > kernel
> >
> > the call through syslog
> >
> > Reviewed-by: Alain Michaud <alainm@chromium.org>
> > Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> > Reviewed-by: Manish Mandlik <mmandlik@chromium.org>
> > ---
> >
> > (no changes since v1)
> >
> > src/adv_monitor.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 66 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/adv_monitor.c b/src/adv_monitor.c
> > index fcb127cd4..582cc9a46 100644
> > --- a/src/adv_monitor.c
> > +++ b/src/adv_monitor.c
> > @@ -588,11 +588,59 @@ done:
> > return monitor->state != MONITOR_STATE_FAILED;
> > }
> >
> > +/* Handles the callback of Add Adv Patterns Monitor command */
> > +static void add_adv_patterns_monitor_cb(uint8_t status, uint16_t length,
> > + const void *param, void *user_data)
> > +{
> > + const struct mgmt_rp_add_adv_patterns_monitor *rp = param;
> > + struct adv_monitor *monitor = user_data;
> > + uint16_t adapter_id = monitor->app->manager->adapter_id;
> > +
> > + if (status != MGMT_STATUS_SUCCESS || !param) {
> > + btd_error(adapter_id, "Failed to Add Adv Patterns Monitor "
> > + "with status 0x%02x", status);
> > + monitor_release(monitor, NULL);
> > + return;
> > + }
> > +
> > + if (length < sizeof(*rp)) {
> > + btd_error(adapter_id, "Wrong size of Add Adv Patterns Monitor "
> > + "response");
> > + monitor_release(monitor, NULL);
> > + return;
> > + }
> > +
> > + monitor->state = MONITOR_STATE_HONORED;
>
> I would reword this state to ACTIVE instead of HONORED as it seems
> more consistent.
>
Addressed in my local v7. I will send this after we settle down the
discussion on the other thread.
> > + DBG("Calling Activate() on Adv Monitor of owner %s at path %s",
> > + monitor->app->owner, monitor->path);
> > +
> > + g_dbus_proxy_method_call(monitor->proxy, "Activate", NULL, NULL, NULL,
> > + NULL);
> > +
> > + DBG("Adv Monitor with handle:0x%04x added",
> > + le16_to_cpu(rp->monitor_handle));
> > +}
> > +
> > +static void monitor_copy_patterns(void *data, void *user_data)
> > +{
> > + struct btd_adv_monitor_pattern *pattern = data;
> > + struct mgmt_cp_add_adv_monitor *cp = user_data;
> > +
> > + if (!pattern)
> > + return;
> > +
> > + memcpy(cp->patterns + cp->pattern_count, pattern, sizeof(*pattern));
> > + cp->pattern_count++;
> > +}
> > +
> > /* Handles an Adv Monitor D-Bus proxy added event */
> > static void monitor_proxy_added_cb(GDBusProxy *proxy, void *user_data)
> > {
> > struct adv_monitor *monitor;
> > struct adv_monitor_app *app = user_data;
> > + struct mgmt_cp_add_adv_monitor *cp = NULL;
> > + uint8_t pattern_count, cp_len;
> > uint16_t adapter_id = app->manager->adapter_id;
> > const char *path = g_dbus_proxy_get_path(proxy);
> > const char *iface = g_dbus_proxy_get_interface(proxy);
> > @@ -625,7 +673,24 @@ static void monitor_proxy_added_cb(GDBusProxy *proxy, void *user_data)
> >
> > queue_push_tail(app->monitors, monitor);
> >
> > + pattern_count = queue_length(monitor->patterns);
> > + cp_len = sizeof(struct mgmt_cp_add_adv_monitor) +
> > + pattern_count * sizeof(struct mgmt_adv_pattern);
> > +
> > + cp = malloc0(cp_len);
> > + queue_foreach(monitor->patterns, monitor_copy_patterns, cp);
> > +
> > + if (!mgmt_send(app->manager->mgmt, MGMT_OP_ADD_ADV_PATTERNS_MONITOR,
> > + adapter_id, cp_len, cp, add_adv_patterns_monitor_cb,
> > + monitor, NULL)) {
> > + error("Unable to send Add Adv Patterns Monitor command");
> > + goto done;
> > + }
> > +
> > DBG("Adv Monitor allocated for the object at path %s", path);
> > +
> > +done:
> > + free(cp);
> > }
> >
> > /* Handles the removal of an Adv Monitor D-Bus proxy */
> > @@ -1036,7 +1101,7 @@ static void adv_match_per_monitor(void *data, void *user_data)
> > struct adv_monitor *monitor = data;
> > struct adv_content_filter_info *info = user_data;
> >
> > - if (!monitor && monitor->state != MONITOR_STATE_HONORED)
> > + if (!monitor || monitor->state != MONITOR_STATE_HONORED)
> > return;
> >
> > /* Reset the intermediate matched status */
> > --
> > 2.26.2
> >
>
>
> --
> Luiz Augusto von Dentz
Regards,
Miao
^ permalink raw reply [flat|nested] 15+ messages in thread
* [BlueZ PATCH v6 5/6] adv_monitor: Fix return type of RegisterMonitor() method
2020-10-07 0:14 [BlueZ PATCH v6 1/6] adv_monitor: Implement RSSI Filter logic for background scanning Miao-chen Chou
` (2 preceding siblings ...)
2020-10-07 0:14 ` [BlueZ PATCH v6 4/6] adv_monitor: Implement Add Adv Patterns Monitor cmd handler Miao-chen Chou
@ 2020-10-07 0:14 ` Miao-chen Chou
2020-10-07 0:14 ` [BlueZ PATCH v6 6/6] adv_monitor: Issue Remove Adv Monitor mgmt call Miao-chen Chou
` (2 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: Miao-chen Chou @ 2020-10-07 0:14 UTC (permalink / raw)
To: Bluetooth Kernel Mailing List
Cc: Luiz Augusto von Dentz, Howard Chung,
chromeos-bluetooth-upstreaming, Alain Michaud, Marcel Holtmann,
Manish Mandlik, Miao-chen Chou
This modifies the D-Bus call return type to be asynchronous for
RegisterMonitor() method call.
The following test was performed:
- Enter bluetoothctl, exit the console and re-enter the console without
AlreadyExist error for RegisterMonitor() upon bring-up of the console.
Reviewed-by: Howard Chung <howardchung@google.com>
Reviewed-by: Manish Mandlik <mmandlik@chromium.org>
---
(no changes since v1)
src/adv_monitor.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/adv_monitor.c b/src/adv_monitor.c
index 582cc9a46..33edbf00c 100644
--- a/src/adv_monitor.c
+++ b/src/adv_monitor.c
@@ -737,6 +737,8 @@ static struct adv_monitor_app *app_create(DBusConnection *conn,
app->monitors = queue_new();
+ app->reg = dbus_message_ref(msg);
+
g_dbus_client_set_disconnect_watch(app->client, app_disconnect_cb, app);
/* Note that any property changes on a monitor object would not affect
@@ -748,8 +750,6 @@ static struct adv_monitor_app *app_create(DBusConnection *conn,
g_dbus_client_set_ready_watch(app->client, app_ready_cb, app);
- app->reg = dbus_message_ref(msg);
-
return app;
}
@@ -843,7 +843,7 @@ static DBusMessage *unregister_monitor(DBusConnection *conn,
}
static const GDBusMethodTable adv_monitor_methods[] = {
- { GDBUS_EXPERIMENTAL_METHOD("RegisterMonitor",
+ { GDBUS_EXPERIMENTAL_ASYNC_METHOD("RegisterMonitor",
GDBUS_ARGS({ "application", "o" }),
NULL, register_monitor) },
{ GDBUS_EXPERIMENTAL_ASYNC_METHOD("UnregisterMonitor",
--
2.26.2
^ permalink raw reply related [flat|nested] 15+ messages in thread* [BlueZ PATCH v6 6/6] adv_monitor: Issue Remove Adv Monitor mgmt call
2020-10-07 0:14 [BlueZ PATCH v6 1/6] adv_monitor: Implement RSSI Filter logic for background scanning Miao-chen Chou
` (3 preceding siblings ...)
2020-10-07 0:14 ` [BlueZ PATCH v6 5/6] adv_monitor: Fix return type of RegisterMonitor() method Miao-chen Chou
@ 2020-10-07 0:14 ` Miao-chen Chou
2020-10-07 1:13 ` [BlueZ,v6,1/6] adv_monitor: Implement RSSI Filter logic for background scanning bluez.test.bot
2020-10-07 6:07 ` [BlueZ PATCH v6 1/6] " Luiz Augusto von Dentz
6 siblings, 0 replies; 15+ messages in thread
From: Miao-chen Chou @ 2020-10-07 0:14 UTC (permalink / raw)
To: Bluetooth Kernel Mailing List
Cc: Luiz Augusto von Dentz, Howard Chung,
chromeos-bluetooth-upstreaming, Alain Michaud, Marcel Holtmann,
Manish Mandlik
From: Alain Michaud <alainm@chromium.org>
This calls Remove Adv Monitor command to kernel and handles the callback
during a monitor removal initiated by a D-Bus client. This also
registers callback for getting notified on Adv Monitor Removed event, so
that the Adv monitor manager can invalidate the monitor by calling
Release() on its proxy.
The following tests were performed.
- In bluetoothctl console, add a monitor and remove the monitor by its
index and verify the removal in both the output of btmgmt and syslog.
- In bluetoothctl console, add a monitor, remove the monitor via
btmgmt and verify the removal in syslog.
Reviewed-by: Howard Chung <howardchung@google.com>
Reviewed-by: Alain Michaud <alainm@chromium.org>
---
(no changes since v4)
Changes in v4:
- Fix build error
Changes in v3:
- Fix const qualifier of a pointer
src/adv_monitor.c | 132 +++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 124 insertions(+), 8 deletions(-)
diff --git a/src/adv_monitor.c b/src/adv_monitor.c
index 33edbf00c..08adc7fa4 100644
--- a/src/adv_monitor.c
+++ b/src/adv_monitor.c
@@ -79,6 +79,7 @@ enum monitor_state {
MONITOR_STATE_FAILED, /* Failed to be init'ed */
MONITOR_STATE_INITED, /* Init'ed but not yet sent to kernel */
MONITOR_STATE_HONORED, /* Accepted by kernel */
+ MONITOR_STATE_REMOVING, /* Removing from kernel */
};
struct btd_adv_monitor_pattern {
@@ -94,6 +95,7 @@ struct adv_monitor {
char *path;
enum monitor_state state; /* MONITOR_STATE_* */
+ uint16_t monitor_handle; /* Kernel Monitor Handle */
int8_t high_rssi; /* High RSSI threshold */
uint16_t high_rssi_timeout; /* High RSSI threshold timeout */
@@ -610,6 +612,7 @@ static void add_adv_patterns_monitor_cb(uint8_t status, uint16_t length,
return;
}
+ monitor->monitor_handle = le16_to_cpu(rp->monitor_handle);
monitor->state = MONITOR_STATE_HONORED;
DBG("Calling Activate() on Adv Monitor of owner %s at path %s",
@@ -618,8 +621,7 @@ static void add_adv_patterns_monitor_cb(uint8_t status, uint16_t length,
g_dbus_proxy_method_call(monitor->proxy, "Activate", NULL, NULL, NULL,
NULL);
- DBG("Adv Monitor with handle:0x%04x added",
- le16_to_cpu(rp->monitor_handle));
+ DBG("Adv monitor with handle:0x%04x added", monitor->monitor_handle);
}
static void monitor_copy_patterns(void *data, void *user_data)
@@ -693,20 +695,77 @@ done:
free(cp);
}
+/* Handles the callback of Remove Adv Monitor command */
+static void remove_adv_monitor_cb(uint8_t status, uint16_t length,
+ const void *param, void *user_data)
+{
+ struct adv_monitor *monitor = user_data;
+ const struct mgmt_rp_remove_adv_monitor *rp = param;
+ uint16_t adapter_id = monitor->app->manager->adapter_id;
+
+ if (status != MGMT_STATUS_SUCCESS || !param) {
+ btd_error(adapter_id, "Failed to Remove Adv Monitor with "
+ "status 0x%02x", status);
+ goto done;
+ }
+
+ if (length < sizeof(*rp)) {
+ btd_error(adapter_id, "Wrong size of Remove Adv Monitor "
+ "response");
+ goto done;
+ }
+
+done:
+ queue_remove(monitor->app->monitors, monitor);
+
+ DBG("Adv Monitor removed with handle:0x%04x, path %s",
+ monitor->monitor_handle, monitor->path);
+
+ monitor_free(monitor);
+}
+
+
/* Handles the removal of an Adv Monitor D-Bus proxy */
static void monitor_proxy_removed_cb(GDBusProxy *proxy, void *user_data)
{
struct adv_monitor *monitor;
+ struct mgmt_cp_remove_adv_monitor cp;
struct adv_monitor_app *app = user_data;
+ uint16_t adapter_id = app->manager->adapter_id;
- monitor = queue_remove_if(app->monitors, monitor_match, proxy);
- if (monitor) {
- DBG("Adv Monitor removed for the object at path %s",
- monitor->path);
+ monitor = queue_find(app->monitors, monitor_match, proxy);
- /* The object was gone, so we don't need to call Release() */
- monitor_free(monitor);
+ /* A monitor removed event from kernel can remove a monitor and notify
+ * the app on Release() where this callback can be invoked, so we
+ * simply skip here.
+ */
+ if (!monitor)
+ return;
+
+ if (monitor->state != MONITOR_STATE_HONORED)
+ goto done;
+
+ monitor->state = MONITOR_STATE_REMOVING;
+
+ cp.monitor_handle = cpu_to_le16(monitor->monitor_handle);
+
+ if (!mgmt_send(app->manager->mgmt, MGMT_OP_REMOVE_ADV_MONITOR,
+ adapter_id, sizeof(cp), &cp, remove_adv_monitor_cb,
+ monitor, NULL)) {
+ btd_error(adapter_id, "Unable to send Remove Advt Monitor "
+ "command");
+ goto done;
}
+
+ return;
+
+done:
+ queue_remove(app->monitors, monitor);
+
+ DBG("Adv Monitor removed in state %02x with path %s", monitor->state,
+ monitor->path);
+
+ monitor_free(monitor);
}
/* Creates an app object, initiates it and sets D-Bus event handlers */
@@ -915,6 +974,59 @@ static const GDBusPropertyTable adv_monitor_properties[] = {
{ }
};
+/* Matches a monitor based on its handle */
+static bool removed_monitor_match(const void *data, const void *user_data)
+{
+ const uint16_t *handle = user_data;
+ const struct adv_monitor *monitor = data;
+
+ if (!data || !handle)
+ return false;
+
+ return monitor->monitor_handle == *handle;
+}
+
+/* Remove the matched monitor and reports the removal to the app */
+static void app_remove_monitor(void *data, void *user_data)
+{
+ struct adv_monitor_app *app = data;
+ struct adv_monitor *monitor;
+
+ monitor = queue_find(app->monitors, removed_monitor_match, user_data);
+ if (monitor) {
+ if (monitor->state == MONITOR_STATE_HONORED)
+ monitor_release(monitor, NULL);
+
+ queue_remove(app->monitors, monitor);
+
+ DBG("Adv Monitor at path %s removed", monitor->path);
+
+ monitor_free(monitor);
+ }
+}
+
+/* Processes Adv Monitor removed event from kernel */
+static void adv_monitor_removed_callback(uint16_t index, uint16_t length,
+ const void *param, void *user_data)
+{
+ struct btd_adv_monitor_manager *manager = user_data;
+ const struct mgmt_ev_adv_monitor_removed *ev = param;
+ uint16_t handle = ev->monitor_handle;
+ const uint16_t adapter_id = manager->adapter_id;
+
+ if (length < sizeof(*ev)) {
+ btd_error(adapter_id, "Wrong size of Adv Monitor Removed "
+ "event");
+ return;
+ }
+
+ /* Traverse the apps to find the monitor */
+ queue_foreach(manager->apps, app_remove_monitor, &handle);
+
+ DBG("Adv Monitor removed event with handle 0x%04x processed",
+ ev->monitor_handle);
+}
+
/* Allocates a manager object */
static struct btd_adv_monitor_manager *manager_new(
struct btd_adapter *adapter,
@@ -934,6 +1046,10 @@ static struct btd_adv_monitor_manager *manager_new(
manager->adapter_id = btd_adapter_get_index(adapter);
manager->apps = queue_new();
+ mgmt_register(manager->mgmt, MGMT_EV_ADV_MONITOR_REMOVED,
+ manager->adapter_id, adv_monitor_removed_callback,
+ manager, NULL);
+
return manager;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 15+ messages in thread* RE: [BlueZ,v6,1/6] adv_monitor: Implement RSSI Filter logic for background scanning
2020-10-07 0:14 [BlueZ PATCH v6 1/6] adv_monitor: Implement RSSI Filter logic for background scanning Miao-chen Chou
` (4 preceding siblings ...)
2020-10-07 0:14 ` [BlueZ PATCH v6 6/6] adv_monitor: Issue Remove Adv Monitor mgmt call Miao-chen Chou
@ 2020-10-07 1:13 ` bluez.test.bot
2020-10-07 6:07 ` [BlueZ PATCH v6 1/6] " Luiz Augusto von Dentz
6 siblings, 0 replies; 15+ messages in thread
From: bluez.test.bot @ 2020-10-07 1:13 UTC (permalink / raw)
To: linux-bluetooth, mcchou
[-- Attachment #1: Type: text/plain, Size: 557 bytes --]
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=360563
---Test result---
##############################
Test: CheckPatch - PASS
##############################
Test: CheckGitLint - PASS
##############################
Test: CheckBuild - PASS
##############################
Test: MakeCheck - PASS
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [BlueZ PATCH v6 1/6] adv_monitor: Implement RSSI Filter logic for background scanning
2020-10-07 0:14 [BlueZ PATCH v6 1/6] adv_monitor: Implement RSSI Filter logic for background scanning Miao-chen Chou
` (5 preceding siblings ...)
2020-10-07 1:13 ` [BlueZ,v6,1/6] adv_monitor: Implement RSSI Filter logic for background scanning bluez.test.bot
@ 2020-10-07 6:07 ` Luiz Augusto von Dentz
2020-10-12 21:21 ` Miao-chen Chou
6 siblings, 1 reply; 15+ messages in thread
From: Luiz Augusto von Dentz @ 2020-10-07 6:07 UTC (permalink / raw)
To: Miao-chen Chou
Cc: Bluetooth Kernel Mailing List, Luiz Augusto von Dentz,
Howard Chung, ChromeOS Bluetooth Upstreaming, Alain Michaud,
Marcel Holtmann, Manish Mandlik, Manish Mandlik,
Abhishek Pandit-Subedi
Hi Miao,
On Tue, Oct 6, 2020 at 5:17 PM Miao-chen Chou <mcchou@chromium.org> wrote:
>
> From: Manish Mandlik <mmandlik@google.com>
>
> This patch implements the RSSI Filter logic for background scanning.
>
> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> Reviewed-by: Alain Michaud <alainm@chromium.org>
> Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> Reviewed-by: Howard Chung <howardchung@google.com>
> ---
>
> (no changes since v5)
>
> Changes in v5:
> - Remove the use of unit test in commit message
>
> Changes in v3:
> - Fix commit message
>
> doc/advertisement-monitor-api.txt | 5 +
> src/adapter.c | 1 +
> src/adv_monitor.c | 286 +++++++++++++++++++++++++++++-
> src/adv_monitor.h | 4 +
> 4 files changed, 292 insertions(+), 4 deletions(-)
>
> diff --git a/doc/advertisement-monitor-api.txt b/doc/advertisement-monitor-api.txt
> index e09b6fd25..92c8ffc38 100644
> --- a/doc/advertisement-monitor-api.txt
> +++ b/doc/advertisement-monitor-api.txt
> @@ -70,6 +70,11 @@ Properties string Type [read-only]
> dBm indicates unset. The valid range of a timer is 1 to
> 300 seconds while 0 indicates unset.
>
> + If the peer device advertising interval is greater than the
> + HighRSSIThresholdTimer, the device will never be found. Similarly,
> + if it is greater than LowRSSIThresholdTimer, the device will be
> + considered as lost. Consider configuring these values accordingly.
> +
> array{(uint8, uint8, array{byte})} Patterns [read-only, optional]
>
> If Type is set to 0x01, this must exist and has at least
> diff --git a/src/adapter.c b/src/adapter.c
> index c0053000a..6d0114a6b 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -1214,6 +1214,7 @@ void btd_adapter_remove_device(struct btd_adapter *adapter,
> adapter->connect_list = g_slist_remove(adapter->connect_list, dev);
>
> adapter->devices = g_slist_remove(adapter->devices, dev);
> + btd_adv_monitor_device_remove(adapter->adv_monitor_manager, dev);
>
> adapter->discovery_found = g_slist_remove(adapter->discovery_found,
> dev);
> diff --git a/src/adv_monitor.c b/src/adv_monitor.c
> index e441a5566..31ed30a00 100644
> --- a/src/adv_monitor.c
> +++ b/src/adv_monitor.c
> @@ -26,6 +26,7 @@
>
> #include "adapter.h"
> #include "dbus-common.h"
> +#include "device.h"
> #include "log.h"
> #include "src/error.h"
> #include "src/shared/ad.h"
> @@ -35,6 +36,8 @@
>
> #include "adv_monitor.h"
>
> +static void monitor_device_free(void *data);
> +
> #define ADV_MONITOR_INTERFACE "org.bluez.AdvertisementMonitor1"
> #define ADV_MONITOR_MGR_INTERFACE "org.bluez.AdvertisementMonitorManager1"
>
> @@ -95,15 +98,36 @@ struct adv_monitor {
>
> enum monitor_state state; /* MONITOR_STATE_* */
>
> - int8_t high_rssi; /* high RSSI threshold */
> - uint16_t high_rssi_timeout; /* high RSSI threshold timeout */
> - int8_t low_rssi; /* low RSSI threshold */
> - uint16_t low_rssi_timeout; /* low RSSI threshold timeout */
> + int8_t high_rssi; /* High RSSI threshold */
> + uint16_t high_rssi_timeout; /* High RSSI threshold timeout */
> + int8_t low_rssi; /* Low RSSI threshold */
> + uint16_t low_rssi_timeout; /* Low RSSI threshold timeout */
> + struct queue *devices; /* List of adv_monitor_device objects */
>
> enum monitor_type type; /* MONITOR_TYPE_* */
> struct queue *patterns;
> };
>
> +/* Some data like last_seen, timer/timeout values need to be maintained
> + * per device. struct adv_monitor_device maintains such data.
> + */
> +struct adv_monitor_device {
> + struct adv_monitor *monitor;
> + struct btd_device *device;
> +
> + time_t high_rssi_first_seen; /* Start time when RSSI climbs above
> + * the high RSSI threshold
> + */
> + time_t low_rssi_first_seen; /* Start time when RSSI drops below
> + * the low RSSI threshold
> + */
> + time_t last_seen; /* Time when last Adv was received */
> + bool device_found; /* State of the device - lost/found */
> + guint device_lost_timer; /* Timer to track if the device goes
> + * offline/out-of-range
> + */
I guess we could just drop the device_ term from the last 2 fields, it
should be implicit from the object itself that is already called
device.
> +};
> +
> struct app_match_data {
> const char *owner;
> const char *path;
> @@ -150,6 +174,9 @@ static void monitor_free(void *data)
> g_dbus_proxy_unref(monitor->proxy);
> g_free(monitor->path);
>
> + queue_destroy(monitor->devices, monitor_device_free);
> + monitor->devices = NULL;
> +
> queue_destroy(monitor->patterns, pattern_free);
>
> free(monitor);
> @@ -248,6 +275,7 @@ static struct adv_monitor *monitor_new(struct adv_monitor_app *app,
> monitor->high_rssi_timeout = ADV_MONITOR_UNSET_TIMER;
> monitor->low_rssi = ADV_MONITOR_UNSET_RSSI;
> monitor->low_rssi_timeout = ADV_MONITOR_UNSET_TIMER;
> + monitor->devices = queue_new();
>
> monitor->type = MONITOR_TYPE_NONE;
> monitor->patterns = NULL;
> @@ -923,3 +951,253 @@ void btd_adv_monitor_manager_destroy(struct btd_adv_monitor_manager *manager)
>
> manager_destroy(manager);
> }
> +
> +/* Matches a device based on btd_device object */
> +static bool monitor_device_match(const void *a, const void *b)
> +{
> + const struct adv_monitor_device *dev = a;
> + const struct btd_device *device = b;
> +
> + if (!dev)
> + return false;
Checks like the above may hide bugs where NULL objects are being added
to the list, if that is happening then we probably need to fix it.
> + if (dev->device != device)
> + return false;
> +
> + return true;
> +}
> +
> +/* Frees a monitor device object */
> +static void monitor_device_free(void *data)
> +{
> + struct adv_monitor_device *dev = data;
> +
> + if (!dev)
> + return;
Ditto.
> + if (dev->device_lost_timer) {
> + g_source_remove(dev->device_lost_timer);
> + dev->device_lost_timer = 0;
> + }
> +
> + dev->monitor = NULL;
> + dev->device = NULL;
> +
> + g_free(dev);
> +}
> +
> +/* Removes a device from monitor->devices list */
> +static void remove_device_from_monitor(void *data, void *user_data)
> +{
> + struct adv_monitor *monitor = data;
> + struct btd_device *device = user_data;
> + struct adv_monitor_device *dev = NULL;
> +
> + if (!monitor)
> + return;
Ditto.
> + dev = queue_remove_if(monitor->devices, monitor_device_match, device);
> + if (dev) {
> + DBG("Device removed from the Adv Monitor at path %s",
> + monitor->path);
> + monitor_device_free(dev);
> + }
> +}
> +
> +/* Removes a device from every monitor in an app */
> +static void remove_device_from_app(void *data, void *user_data)
> +{
> + struct adv_monitor_app *app = data;
> + struct btd_device *device = user_data;
> +
> + if (!app)
> + return;
Ditto.
> + queue_foreach(app->monitors, remove_device_from_monitor, device);
> +}
> +
> +/* Removes a device from every monitor in all apps */
> +void btd_adv_monitor_device_remove(struct btd_adv_monitor_manager *manager,
> + struct btd_device *device)
> +{
> + if (!manager || !device)
> + return;
> +
> + queue_foreach(manager->apps, remove_device_from_app, device);
> +}
> +
> +/* Creates a device object to track the per-device information */
> +static struct adv_monitor_device *monitor_device_create(
> + struct adv_monitor *monitor,
> + struct btd_device *device)
> +{
> + struct adv_monitor_device *dev = NULL;
> +
> + dev = g_try_malloc0(sizeof(struct adv_monitor_device));
Please use new0 on new code.
> + if (!dev)
> + return NULL;
> +
> + dev->monitor = monitor;
> + dev->device = device;
> +
> + queue_push_tail(monitor->devices, dev);
> +
> + return dev;
> +}
> +
> +/* Includes found/lost device's object path into the dbus message */
> +static void report_device_state_setup(DBusMessageIter *iter, void *user_data)
> +{
> + const char *path = device_get_path(user_data);
> +
> + dbus_message_iter_append_basic(iter, DBUS_TYPE_OBJECT_PATH, &path);
> +}
> +
> +/* Handles a situation where the device goes offline/out-of-range */
> +static gboolean handle_device_lost_timeout(gpointer user_data)
> +{
> + struct adv_monitor_device *dev = user_data;
> + struct adv_monitor *monitor = dev->monitor;
> + time_t curr_time = time(NULL);
> +
> + DBG("Device Lost timeout triggered for device %p "
> + "for the Adv Monitor at path %s", dev->device, monitor->path);
> +
> + dev->device_lost_timer = 0;
> +
> + if (dev->device_found && dev->last_seen) {
> + /* We were tracking for the Low RSSI filter. Check if there is
> + * any Adv received after the timeout function is invoked.
> + * If not, report the Device Lost event.
> + */
> + if (difftime(curr_time, dev->last_seen) >=
> + monitor->low_rssi_timeout) {
> + dev->device_found = false;
> +
> + DBG("Calling DeviceLost() on Adv Monitor of owner %s "
> + "at path %s", monitor->app->owner, monitor->path);
> +
> + g_dbus_proxy_method_call(monitor->proxy, "DeviceLost",
> + report_device_state_setup,
> + NULL, dev->device, NULL);
> + }
> + }
> +
> + return FALSE;
> +}
> +
> +/* Filters an Adv based on its RSSI value */
> +static void adv_monitor_filter_rssi(struct adv_monitor *monitor,
> + struct btd_device *device, int8_t rssi)
> +{
> + struct adv_monitor_device *dev = NULL;
> + time_t curr_time = time(NULL);
> + uint16_t adapter_id = monitor->app->manager->adapter_id;
> +
> + /* If the RSSI thresholds and timeouts are not specified, report the
> + * DeviceFound() event without tracking for the RSSI as the Adv has
> + * already matched the pattern filter.
> + */
> + if (monitor->high_rssi == ADV_MONITOR_UNSET_RSSI &&
> + monitor->low_rssi == ADV_MONITOR_UNSET_RSSI &&
> + monitor->high_rssi_timeout == ADV_MONITOR_UNSET_TIMER &&
> + monitor->low_rssi_timeout == ADV_MONITOR_UNSET_TIMER) {
> + DBG("Calling DeviceFound() on Adv Monitor of owner %s "
> + "at path %s", monitor->app->owner, monitor->path);
> +
> + g_dbus_proxy_method_call(monitor->proxy, "DeviceFound",
> + report_device_state_setup, NULL,
> + device, NULL);
> +
> + return;
> + }
> +
> + dev = queue_find(monitor->devices, monitor_device_match, device);
> + if (!dev)
> + dev = monitor_device_create(monitor, device);
There seems to be missing indentation here as the following if
statement should be nested otherwise the dev point is always evaluated
twice which is not optimal.
> + if (!dev) {
> + btd_error(adapter_id, "Failed to create Adv Monitor "
> + "device object.");
> + return;
> + }
Id put the code above into a function e.g. monitor_device_get so
whenever you need to find + create logic you can just use it.
> +
> + if (dev->device_lost_timer) {
> + g_source_remove(dev->device_lost_timer);
> + dev->device_lost_timer = 0;
> + }
> +
> + /* Reset the timings of found/lost if a device has been offline for
> + * longer than the high/low timeouts.
> + */
> + if (dev->last_seen) {
> + if (difftime(curr_time, dev->last_seen) >
> + monitor->high_rssi_timeout) {
> + dev->high_rssi_first_seen = 0;
> + }
> +
> + if (difftime(curr_time, dev->last_seen) >
> + monitor->low_rssi_timeout) {
> + dev->low_rssi_first_seen = 0;
> + }
> + }
> + dev->last_seen = curr_time;
> +
> + /* Check for the found devices (if the device is not already found) */
> + if (!dev->device_found && rssi > monitor->high_rssi) {
> + if (dev->high_rssi_first_seen) {
> + if (difftime(curr_time, dev->high_rssi_first_seen) >=
> + monitor->high_rssi_timeout) {
> + dev->device_found = true;
> +
> + DBG("Calling DeviceFound() on Adv Monitor "
> + "of owner %s at path %s",
> + monitor->app->owner, monitor->path);
> +
> + g_dbus_proxy_method_call(
> + monitor->proxy, "DeviceFound",
> + report_device_state_setup, NULL,
> + dev->device, NULL);
> + }
> + } else {
> + dev->high_rssi_first_seen = curr_time;
> + }
> + } else {
> + dev->high_rssi_first_seen = 0;
> + }
> +
> + /* Check for the lost devices (only if the device is already found, as
> + * it doesn't make any sense to report the Device Lost event if the
> + * device is not found yet)
> + */
> + if (dev->device_found && rssi < monitor->low_rssi) {
> + if (dev->low_rssi_first_seen) {
> + if (difftime(curr_time, dev->low_rssi_first_seen) >=
> + monitor->low_rssi_timeout) {
> + dev->device_found = false;
> +
> + DBG("Calling DeviceLost() on Adv Monitor "
> + "of owner %s at path %s",
> + monitor->app->owner, monitor->path);
> +
> + g_dbus_proxy_method_call(
> + monitor->proxy, "DeviceLost",
> + report_device_state_setup, NULL,
> + dev->device, NULL);
> + }
> + } else {
> + dev->low_rssi_first_seen = curr_time;
> + }
> + } else {
> + dev->low_rssi_first_seen = 0;
> + }
> +
> + /* Setup a timer to track if the device goes offline/out-of-range, only
> + * if we are tracking for the Low RSSI Threshold. If we are tracking
> + * the High RSSI Threshold, nothing needs to be done.
> + */
> + if (dev->device_found) {
> + dev->device_lost_timer =
> + g_timeout_add_seconds(monitor->low_rssi_timeout,
> + handle_device_lost_timeout, dev);
> + }
> +}
> diff --git a/src/adv_monitor.h b/src/adv_monitor.h
> index 5cb372217..13d5d7282 100644
> --- a/src/adv_monitor.h
> +++ b/src/adv_monitor.h
> @@ -12,6 +12,7 @@
> #define __ADV_MONITOR_H
>
> struct mgmt;
> +struct btd_device;
> struct btd_adapter;
> struct btd_adv_monitor_manager;
>
> @@ -20,4 +21,7 @@ struct btd_adv_monitor_manager *btd_adv_monitor_manager_create(
> struct mgmt *mgmt);
> void btd_adv_monitor_manager_destroy(struct btd_adv_monitor_manager *manager);
>
> +void btd_adv_monitor_device_remove(struct btd_adv_monitor_manager *manager,
> + struct btd_device *device);
> +
> #endif /* __ADV_MONITOR_H */
> --
> 2.26.2
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [BlueZ PATCH v6 1/6] adv_monitor: Implement RSSI Filter logic for background scanning
2020-10-07 6:07 ` [BlueZ PATCH v6 1/6] " Luiz Augusto von Dentz
@ 2020-10-12 21:21 ` Miao-chen Chou
2020-10-13 23:17 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 15+ messages in thread
From: Miao-chen Chou @ 2020-10-12 21:21 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: Bluetooth Kernel Mailing List, Luiz Augusto von Dentz,
Howard Chung, ChromeOS Bluetooth Upstreaming, Alain Michaud,
Marcel Holtmann, Manish Mandlik, Manish Mandlik,
Abhishek Pandit-Subedi
Hi Luiz,
All the following changes were addressed in my local v7. I will wait
for your feedback on the other thread and send as a series.
On Tue, Oct 6, 2020 at 11:07 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Miao,
>
> On Tue, Oct 6, 2020 at 5:17 PM Miao-chen Chou <mcchou@chromium.org> wrote:
> >
> > From: Manish Mandlik <mmandlik@google.com>
> >
> > This patch implements the RSSI Filter logic for background scanning.
> >
> > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > Reviewed-by: Alain Michaud <alainm@chromium.org>
> > Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> > Reviewed-by: Howard Chung <howardchung@google.com>
> > ---
> >
> > (no changes since v5)
> >
> > Changes in v5:
> > - Remove the use of unit test in commit message
> >
> > Changes in v3:
> > - Fix commit message
> >
> > doc/advertisement-monitor-api.txt | 5 +
> > src/adapter.c | 1 +
> > src/adv_monitor.c | 286 +++++++++++++++++++++++++++++-
> > src/adv_monitor.h | 4 +
> > 4 files changed, 292 insertions(+), 4 deletions(-)
> >
> > diff --git a/doc/advertisement-monitor-api.txt b/doc/advertisement-monitor-api.txt
> > index e09b6fd25..92c8ffc38 100644
> > --- a/doc/advertisement-monitor-api.txt
> > +++ b/doc/advertisement-monitor-api.txt
> > @@ -70,6 +70,11 @@ Properties string Type [read-only]
> > dBm indicates unset. The valid range of a timer is 1 to
> > 300 seconds while 0 indicates unset.
> >
> > + If the peer device advertising interval is greater than the
> > + HighRSSIThresholdTimer, the device will never be found. Similarly,
> > + if it is greater than LowRSSIThresholdTimer, the device will be
> > + considered as lost. Consider configuring these values accordingly.
> > +
> > array{(uint8, uint8, array{byte})} Patterns [read-only, optional]
> >
> > If Type is set to 0x01, this must exist and has at least
> > diff --git a/src/adapter.c b/src/adapter.c
> > index c0053000a..6d0114a6b 100644
> > --- a/src/adapter.c
> > +++ b/src/adapter.c
> > @@ -1214,6 +1214,7 @@ void btd_adapter_remove_device(struct btd_adapter *adapter,
> > adapter->connect_list = g_slist_remove(adapter->connect_list, dev);
> >
> > adapter->devices = g_slist_remove(adapter->devices, dev);
> > + btd_adv_monitor_device_remove(adapter->adv_monitor_manager, dev);
> >
> > adapter->discovery_found = g_slist_remove(adapter->discovery_found,
> > dev);
> > diff --git a/src/adv_monitor.c b/src/adv_monitor.c
> > index e441a5566..31ed30a00 100644
> > --- a/src/adv_monitor.c
> > +++ b/src/adv_monitor.c
> > @@ -26,6 +26,7 @@
> >
> > #include "adapter.h"
> > #include "dbus-common.h"
> > +#include "device.h"
> > #include "log.h"
> > #include "src/error.h"
> > #include "src/shared/ad.h"
> > @@ -35,6 +36,8 @@
> >
> > #include "adv_monitor.h"
> >
> > +static void monitor_device_free(void *data);
> > +
> > #define ADV_MONITOR_INTERFACE "org.bluez.AdvertisementMonitor1"
> > #define ADV_MONITOR_MGR_INTERFACE "org.bluez.AdvertisementMonitorManager1"
> >
> > @@ -95,15 +98,36 @@ struct adv_monitor {
> >
> > enum monitor_state state; /* MONITOR_STATE_* */
> >
> > - int8_t high_rssi; /* high RSSI threshold */
> > - uint16_t high_rssi_timeout; /* high RSSI threshold timeout */
> > - int8_t low_rssi; /* low RSSI threshold */
> > - uint16_t low_rssi_timeout; /* low RSSI threshold timeout */
> > + int8_t high_rssi; /* High RSSI threshold */
> > + uint16_t high_rssi_timeout; /* High RSSI threshold timeout */
> > + int8_t low_rssi; /* Low RSSI threshold */
> > + uint16_t low_rssi_timeout; /* Low RSSI threshold timeout */
> > + struct queue *devices; /* List of adv_monitor_device objects */
> >
> > enum monitor_type type; /* MONITOR_TYPE_* */
> > struct queue *patterns;
> > };
> >
> > +/* Some data like last_seen, timer/timeout values need to be maintained
> > + * per device. struct adv_monitor_device maintains such data.
> > + */
> > +struct adv_monitor_device {
> > + struct adv_monitor *monitor;
> > + struct btd_device *device;
> > +
> > + time_t high_rssi_first_seen; /* Start time when RSSI climbs above
> > + * the high RSSI threshold
> > + */
> > + time_t low_rssi_first_seen; /* Start time when RSSI drops below
> > + * the low RSSI threshold
> > + */
> > + time_t last_seen; /* Time when last Adv was received */
> > + bool device_found; /* State of the device - lost/found */
> > + guint device_lost_timer; /* Timer to track if the device goes
> > + * offline/out-of-range
> > + */
>
> I guess we could just drop the device_ term from the last 2 fields, it
> should be implicit from the object itself that is already called
> device.
Done.
>
> > +};
> > +
> > struct app_match_data {
> > const char *owner;
> > const char *path;
> > @@ -150,6 +174,9 @@ static void monitor_free(void *data)
> > g_dbus_proxy_unref(monitor->proxy);
> > g_free(monitor->path);
> >
> > + queue_destroy(monitor->devices, monitor_device_free);
> > + monitor->devices = NULL;
> > +
> > queue_destroy(monitor->patterns, pattern_free);
> >
> > free(monitor);
> > @@ -248,6 +275,7 @@ static struct adv_monitor *monitor_new(struct adv_monitor_app *app,
> > monitor->high_rssi_timeout = ADV_MONITOR_UNSET_TIMER;
> > monitor->low_rssi = ADV_MONITOR_UNSET_RSSI;
> > monitor->low_rssi_timeout = ADV_MONITOR_UNSET_TIMER;
> > + monitor->devices = queue_new();
> >
> > monitor->type = MONITOR_TYPE_NONE;
> > monitor->patterns = NULL;
> > @@ -923,3 +951,253 @@ void btd_adv_monitor_manager_destroy(struct btd_adv_monitor_manager *manager)
> >
> > manager_destroy(manager);
> > }
> > +
> > +/* Matches a device based on btd_device object */
> > +static bool monitor_device_match(const void *a, const void *b)
> > +{
> > + const struct adv_monitor_device *dev = a;
> > + const struct btd_device *device = b;
> > +
> > + if (!dev)
> > + return false;
>
> Checks like the above may hide bugs where NULL objects are being added
> to the list, if that is happening then we probably need to fix it.
Added error logs here and elsewhere.
>
> > + if (dev->device != device)
> > + return false;
> > +
> > + return true;
> > +}
> > +
> > +/* Frees a monitor device object */
> > +static void monitor_device_free(void *data)
> > +{
> > + struct adv_monitor_device *dev = data;
> > +
> > + if (!dev)
> > + return;
>
> Ditto.
Done.
>
> > + if (dev->device_lost_timer) {
> > + g_source_remove(dev->device_lost_timer);
> > + dev->device_lost_timer = 0;
> > + }
> > +
> > + dev->monitor = NULL;
> > + dev->device = NULL;
> > +
> > + g_free(dev);
> > +}
> > +
> > +/* Removes a device from monitor->devices list */
> > +static void remove_device_from_monitor(void *data, void *user_data)
> > +{
> > + struct adv_monitor *monitor = data;
> > + struct btd_device *device = user_data;
> > + struct adv_monitor_device *dev = NULL;
> > +
> > + if (!monitor)
> > + return;
>
> Ditto.
Done.
>
> > + dev = queue_remove_if(monitor->devices, monitor_device_match, device);
> > + if (dev) {
> > + DBG("Device removed from the Adv Monitor at path %s",
> > + monitor->path);
> > + monitor_device_free(dev);
> > + }
> > +}
> > +
> > +/* Removes a device from every monitor in an app */
> > +static void remove_device_from_app(void *data, void *user_data)
> > +{
> > + struct adv_monitor_app *app = data;
> > + struct btd_device *device = user_data;
> > +
> > + if (!app)
> > + return;
>
> Ditto.
>
> > + queue_foreach(app->monitors, remove_device_from_monitor, device);
> > +}
> > +
> > +/* Removes a device from every monitor in all apps */
> > +void btd_adv_monitor_device_remove(struct btd_adv_monitor_manager *manager,
> > + struct btd_device *device)
> > +{
> > + if (!manager || !device)
> > + return;
> > +
> > + queue_foreach(manager->apps, remove_device_from_app, device);
> > +}
> > +
> > +/* Creates a device object to track the per-device information */
> > +static struct adv_monitor_device *monitor_device_create(
> > + struct adv_monitor *monitor,
> > + struct btd_device *device)
> > +{
> > + struct adv_monitor_device *dev = NULL;
> > +
> > + dev = g_try_malloc0(sizeof(struct adv_monitor_device));
>
> Please use new0 on new code.
Done.
>
> > + if (!dev)
> > + return NULL;
> > +
> > + dev->monitor = monitor;
> > + dev->device = device;
> > +
> > + queue_push_tail(monitor->devices, dev);
> > +
> > + return dev;
> > +}
> > +
> > +/* Includes found/lost device's object path into the dbus message */
> > +static void report_device_state_setup(DBusMessageIter *iter, void *user_data)
> > +{
> > + const char *path = device_get_path(user_data);
> > +
> > + dbus_message_iter_append_basic(iter, DBUS_TYPE_OBJECT_PATH, &path);
> > +}
> > +
> > +/* Handles a situation where the device goes offline/out-of-range */
> > +static gboolean handle_device_lost_timeout(gpointer user_data)
> > +{
> > + struct adv_monitor_device *dev = user_data;
> > + struct adv_monitor *monitor = dev->monitor;
> > + time_t curr_time = time(NULL);
> > +
> > + DBG("Device Lost timeout triggered for device %p "
> > + "for the Adv Monitor at path %s", dev->device, monitor->path);
> > +
> > + dev->device_lost_timer = 0;
> > +
> > + if (dev->device_found && dev->last_seen) {
> > + /* We were tracking for the Low RSSI filter. Check if there is
> > + * any Adv received after the timeout function is invoked.
> > + * If not, report the Device Lost event.
> > + */
> > + if (difftime(curr_time, dev->last_seen) >=
> > + monitor->low_rssi_timeout) {
> > + dev->device_found = false;
> > +
> > + DBG("Calling DeviceLost() on Adv Monitor of owner %s "
> > + "at path %s", monitor->app->owner, monitor->path);
> > +
> > + g_dbus_proxy_method_call(monitor->proxy, "DeviceLost",
> > + report_device_state_setup,
> > + NULL, dev->device, NULL);
> > + }
> > + }
> > +
> > + return FALSE;
> > +}
> > +
> > +/* Filters an Adv based on its RSSI value */
> > +static void adv_monitor_filter_rssi(struct adv_monitor *monitor,
> > + struct btd_device *device, int8_t rssi)
> > +{
> > + struct adv_monitor_device *dev = NULL;
> > + time_t curr_time = time(NULL);
> > + uint16_t adapter_id = monitor->app->manager->adapter_id;
> > +
> > + /* If the RSSI thresholds and timeouts are not specified, report the
> > + * DeviceFound() event without tracking for the RSSI as the Adv has
> > + * already matched the pattern filter.
> > + */
> > + if (monitor->high_rssi == ADV_MONITOR_UNSET_RSSI &&
> > + monitor->low_rssi == ADV_MONITOR_UNSET_RSSI &&
> > + monitor->high_rssi_timeout == ADV_MONITOR_UNSET_TIMER &&
> > + monitor->low_rssi_timeout == ADV_MONITOR_UNSET_TIMER) {
> > + DBG("Calling DeviceFound() on Adv Monitor of owner %s "
> > + "at path %s", monitor->app->owner, monitor->path);
> > +
> > + g_dbus_proxy_method_call(monitor->proxy, "DeviceFound",
> > + report_device_state_setup, NULL,
> > + device, NULL);
> > +
> > + return;
> > + }
> > +
> > + dev = queue_find(monitor->devices, monitor_device_match, device);
> > + if (!dev)
> > + dev = monitor_device_create(monitor, device);
>
> There seems to be missing indentation here as the following if
> statement should be nested otherwise the dev point is always evaluated
> twice which is not optimal.
Done.
>
> > + if (!dev) {
> > + btd_error(adapter_id, "Failed to create Adv Monitor "
> > + "device object.");
> > + return;
> > + }
>
> Id put the code above into a function e.g. monitor_device_get so
> whenever you need to find + create logic you can just use it.
This is the only case of find + create logic, so I guess that the
author intentionally leaves it as it is.
>
> > +
> > + if (dev->device_lost_timer) {
> > + g_source_remove(dev->device_lost_timer);
> > + dev->device_lost_timer = 0;
> > + }
> > +
> > + /* Reset the timings of found/lost if a device has been offline for
> > + * longer than the high/low timeouts.
> > + */
> > + if (dev->last_seen) {
> > + if (difftime(curr_time, dev->last_seen) >
> > + monitor->high_rssi_timeout) {
> > + dev->high_rssi_first_seen = 0;
> > + }
> > +
> > + if (difftime(curr_time, dev->last_seen) >
> > + monitor->low_rssi_timeout) {
> > + dev->low_rssi_first_seen = 0;
> > + }
> > + }
> > + dev->last_seen = curr_time;
> > +
> > + /* Check for the found devices (if the device is not already found) */
> > + if (!dev->device_found && rssi > monitor->high_rssi) {
> > + if (dev->high_rssi_first_seen) {
> > + if (difftime(curr_time, dev->high_rssi_first_seen) >=
> > + monitor->high_rssi_timeout) {
> > + dev->device_found = true;
> > +
> > + DBG("Calling DeviceFound() on Adv Monitor "
> > + "of owner %s at path %s",
> > + monitor->app->owner, monitor->path);
> > +
> > + g_dbus_proxy_method_call(
> > + monitor->proxy, "DeviceFound",
> > + report_device_state_setup, NULL,
> > + dev->device, NULL);
> > + }
> > + } else {
> > + dev->high_rssi_first_seen = curr_time;
> > + }
> > + } else {
> > + dev->high_rssi_first_seen = 0;
> > + }
> > +
> > + /* Check for the lost devices (only if the device is already found, as
> > + * it doesn't make any sense to report the Device Lost event if the
> > + * device is not found yet)
> > + */
> > + if (dev->device_found && rssi < monitor->low_rssi) {
> > + if (dev->low_rssi_first_seen) {
> > + if (difftime(curr_time, dev->low_rssi_first_seen) >=
> > + monitor->low_rssi_timeout) {
> > + dev->device_found = false;
> > +
> > + DBG("Calling DeviceLost() on Adv Monitor "
> > + "of owner %s at path %s",
> > + monitor->app->owner, monitor->path);
> > +
> > + g_dbus_proxy_method_call(
> > + monitor->proxy, "DeviceLost",
> > + report_device_state_setup, NULL,
> > + dev->device, NULL);
> > + }
> > + } else {
> > + dev->low_rssi_first_seen = curr_time;
> > + }
> > + } else {
> > + dev->low_rssi_first_seen = 0;
> > + }
> > +
> > + /* Setup a timer to track if the device goes offline/out-of-range, only
> > + * if we are tracking for the Low RSSI Threshold. If we are tracking
> > + * the High RSSI Threshold, nothing needs to be done.
> > + */
> > + if (dev->device_found) {
> > + dev->device_lost_timer =
> > + g_timeout_add_seconds(monitor->low_rssi_timeout,
> > + handle_device_lost_timeout, dev);
> > + }
> > +}
> > diff --git a/src/adv_monitor.h b/src/adv_monitor.h
> > index 5cb372217..13d5d7282 100644
> > --- a/src/adv_monitor.h
> > +++ b/src/adv_monitor.h
> > @@ -12,6 +12,7 @@
> > #define __ADV_MONITOR_H
> >
> > struct mgmt;
> > +struct btd_device;
> > struct btd_adapter;
> > struct btd_adv_monitor_manager;
> >
> > @@ -20,4 +21,7 @@ struct btd_adv_monitor_manager *btd_adv_monitor_manager_create(
> > struct mgmt *mgmt);
> > void btd_adv_monitor_manager_destroy(struct btd_adv_monitor_manager *manager);
> >
> > +void btd_adv_monitor_device_remove(struct btd_adv_monitor_manager *manager,
> > + struct btd_device *device);
> > +
> > #endif /* __ADV_MONITOR_H */
> > --
> > 2.26.2
> >
>
>
> --
> Luiz Augusto von Dentz
Regards,
Miao
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [BlueZ PATCH v6 1/6] adv_monitor: Implement RSSI Filter logic for background scanning
2020-10-12 21:21 ` Miao-chen Chou
@ 2020-10-13 23:17 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 15+ messages in thread
From: Luiz Augusto von Dentz @ 2020-10-13 23:17 UTC (permalink / raw)
To: Miao-chen Chou
Cc: Bluetooth Kernel Mailing List, Luiz Augusto von Dentz,
Howard Chung, ChromeOS Bluetooth Upstreaming, Alain Michaud,
Marcel Holtmann, Manish Mandlik, Manish Mandlik,
Abhishek Pandit-Subedi
Hi Miao-chen,
On Mon, Oct 12, 2020 at 2:21 PM Miao-chen Chou <mcchou@chromium.org> wrote:
>
> Hi Luiz,
>
> All the following changes were addressed in my local v7. I will wait
> for your feedback on the other thread and send as a series.
>
> On Tue, Oct 6, 2020 at 11:07 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Miao,
> >
> > On Tue, Oct 6, 2020 at 5:17 PM Miao-chen Chou <mcchou@chromium.org> wrote:
> > >
> > > From: Manish Mandlik <mmandlik@google.com>
> > >
> > > This patch implements the RSSI Filter logic for background scanning.
> > >
> > > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > Reviewed-by: Alain Michaud <alainm@chromium.org>
> > > Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> > > Reviewed-by: Howard Chung <howardchung@google.com>
> > > ---
> > >
> > > (no changes since v5)
> > >
> > > Changes in v5:
> > > - Remove the use of unit test in commit message
> > >
> > > Changes in v3:
> > > - Fix commit message
> > >
> > > doc/advertisement-monitor-api.txt | 5 +
> > > src/adapter.c | 1 +
> > > src/adv_monitor.c | 286 +++++++++++++++++++++++++++++-
> > > src/adv_monitor.h | 4 +
> > > 4 files changed, 292 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/doc/advertisement-monitor-api.txt b/doc/advertisement-monitor-api.txt
> > > index e09b6fd25..92c8ffc38 100644
> > > --- a/doc/advertisement-monitor-api.txt
> > > +++ b/doc/advertisement-monitor-api.txt
> > > @@ -70,6 +70,11 @@ Properties string Type [read-only]
> > > dBm indicates unset. The valid range of a timer is 1 to
> > > 300 seconds while 0 indicates unset.
> > >
> > > + If the peer device advertising interval is greater than the
> > > + HighRSSIThresholdTimer, the device will never be found. Similarly,
> > > + if it is greater than LowRSSIThresholdTimer, the device will be
> > > + considered as lost. Consider configuring these values accordingly.
> > > +
> > > array{(uint8, uint8, array{byte})} Patterns [read-only, optional]
> > >
> > > If Type is set to 0x01, this must exist and has at least
> > > diff --git a/src/adapter.c b/src/adapter.c
> > > index c0053000a..6d0114a6b 100644
> > > --- a/src/adapter.c
> > > +++ b/src/adapter.c
> > > @@ -1214,6 +1214,7 @@ void btd_adapter_remove_device(struct btd_adapter *adapter,
> > > adapter->connect_list = g_slist_remove(adapter->connect_list, dev);
> > >
> > > adapter->devices = g_slist_remove(adapter->devices, dev);
> > > + btd_adv_monitor_device_remove(adapter->adv_monitor_manager, dev);
> > >
> > > adapter->discovery_found = g_slist_remove(adapter->discovery_found,
> > > dev);
> > > diff --git a/src/adv_monitor.c b/src/adv_monitor.c
> > > index e441a5566..31ed30a00 100644
> > > --- a/src/adv_monitor.c
> > > +++ b/src/adv_monitor.c
> > > @@ -26,6 +26,7 @@
> > >
> > > #include "adapter.h"
> > > #include "dbus-common.h"
> > > +#include "device.h"
> > > #include "log.h"
> > > #include "src/error.h"
> > > #include "src/shared/ad.h"
> > > @@ -35,6 +36,8 @@
> > >
> > > #include "adv_monitor.h"
> > >
> > > +static void monitor_device_free(void *data);
> > > +
> > > #define ADV_MONITOR_INTERFACE "org.bluez.AdvertisementMonitor1"
> > > #define ADV_MONITOR_MGR_INTERFACE "org.bluez.AdvertisementMonitorManager1"
> > >
> > > @@ -95,15 +98,36 @@ struct adv_monitor {
> > >
> > > enum monitor_state state; /* MONITOR_STATE_* */
> > >
> > > - int8_t high_rssi; /* high RSSI threshold */
> > > - uint16_t high_rssi_timeout; /* high RSSI threshold timeout */
> > > - int8_t low_rssi; /* low RSSI threshold */
> > > - uint16_t low_rssi_timeout; /* low RSSI threshold timeout */
> > > + int8_t high_rssi; /* High RSSI threshold */
> > > + uint16_t high_rssi_timeout; /* High RSSI threshold timeout */
> > > + int8_t low_rssi; /* Low RSSI threshold */
> > > + uint16_t low_rssi_timeout; /* Low RSSI threshold timeout */
> > > + struct queue *devices; /* List of adv_monitor_device objects */
> > >
> > > enum monitor_type type; /* MONITOR_TYPE_* */
> > > struct queue *patterns;
> > > };
> > >
> > > +/* Some data like last_seen, timer/timeout values need to be maintained
> > > + * per device. struct adv_monitor_device maintains such data.
> > > + */
> > > +struct adv_monitor_device {
> > > + struct adv_monitor *monitor;
> > > + struct btd_device *device;
> > > +
> > > + time_t high_rssi_first_seen; /* Start time when RSSI climbs above
> > > + * the high RSSI threshold
> > > + */
> > > + time_t low_rssi_first_seen; /* Start time when RSSI drops below
> > > + * the low RSSI threshold
> > > + */
> > > + time_t last_seen; /* Time when last Adv was received */
> > > + bool device_found; /* State of the device - lost/found */
> > > + guint device_lost_timer; /* Timer to track if the device goes
> > > + * offline/out-of-range
> > > + */
> >
> > I guess we could just drop the device_ term from the last 2 fields, it
> > should be implicit from the object itself that is already called
> > device.
> Done.
>
> >
> > > +};
> > > +
> > > struct app_match_data {
> > > const char *owner;
> > > const char *path;
> > > @@ -150,6 +174,9 @@ static void monitor_free(void *data)
> > > g_dbus_proxy_unref(monitor->proxy);
> > > g_free(monitor->path);
> > >
> > > + queue_destroy(monitor->devices, monitor_device_free);
> > > + monitor->devices = NULL;
> > > +
> > > queue_destroy(monitor->patterns, pattern_free);
> > >
> > > free(monitor);
> > > @@ -248,6 +275,7 @@ static struct adv_monitor *monitor_new(struct adv_monitor_app *app,
> > > monitor->high_rssi_timeout = ADV_MONITOR_UNSET_TIMER;
> > > monitor->low_rssi = ADV_MONITOR_UNSET_RSSI;
> > > monitor->low_rssi_timeout = ADV_MONITOR_UNSET_TIMER;
> > > + monitor->devices = queue_new();
> > >
> > > monitor->type = MONITOR_TYPE_NONE;
> > > monitor->patterns = NULL;
> > > @@ -923,3 +951,253 @@ void btd_adv_monitor_manager_destroy(struct btd_adv_monitor_manager *manager)
> > >
> > > manager_destroy(manager);
> > > }
> > > +
> > > +/* Matches a device based on btd_device object */
> > > +static bool monitor_device_match(const void *a, const void *b)
> > > +{
> > > + const struct adv_monitor_device *dev = a;
> > > + const struct btd_device *device = b;
> > > +
> > > + if (!dev)
> > > + return false;
> >
> > Checks like the above may hide bugs where NULL objects are being added
> > to the list, if that is happening then we probably need to fix it.
> Added error logs here and elsewhere.
>
> >
> > > + if (dev->device != device)
> > > + return false;
> > > +
> > > + return true;
> > > +}
> > > +
> > > +/* Frees a monitor device object */
> > > +static void monitor_device_free(void *data)
> > > +{
> > > + struct adv_monitor_device *dev = data;
> > > +
> > > + if (!dev)
> > > + return;
> >
> > Ditto.
> Done.
>
> >
> > > + if (dev->device_lost_timer) {
> > > + g_source_remove(dev->device_lost_timer);
> > > + dev->device_lost_timer = 0;
> > > + }
> > > +
> > > + dev->monitor = NULL;
> > > + dev->device = NULL;
> > > +
> > > + g_free(dev);
> > > +}
> > > +
> > > +/* Removes a device from monitor->devices list */
> > > +static void remove_device_from_monitor(void *data, void *user_data)
> > > +{
> > > + struct adv_monitor *monitor = data;
> > > + struct btd_device *device = user_data;
> > > + struct adv_monitor_device *dev = NULL;
> > > +
> > > + if (!monitor)
> > > + return;
> >
> > Ditto.
> Done.
>
> >
> > > + dev = queue_remove_if(monitor->devices, monitor_device_match, device);
> > > + if (dev) {
> > > + DBG("Device removed from the Adv Monitor at path %s",
> > > + monitor->path);
> > > + monitor_device_free(dev);
> > > + }
> > > +}
> > > +
> > > +/* Removes a device from every monitor in an app */
> > > +static void remove_device_from_app(void *data, void *user_data)
> > > +{
> > > + struct adv_monitor_app *app = data;
> > > + struct btd_device *device = user_data;
> > > +
> > > + if (!app)
> > > + return;
> >
> > Ditto.
>
> >
> > > + queue_foreach(app->monitors, remove_device_from_monitor, device);
> > > +}
> > > +
> > > +/* Removes a device from every monitor in all apps */
> > > +void btd_adv_monitor_device_remove(struct btd_adv_monitor_manager *manager,
> > > + struct btd_device *device)
> > > +{
> > > + if (!manager || !device)
> > > + return;
> > > +
> > > + queue_foreach(manager->apps, remove_device_from_app, device);
> > > +}
> > > +
> > > +/* Creates a device object to track the per-device information */
> > > +static struct adv_monitor_device *monitor_device_create(
> > > + struct adv_monitor *monitor,
> > > + struct btd_device *device)
> > > +{
> > > + struct adv_monitor_device *dev = NULL;
> > > +
> > > + dev = g_try_malloc0(sizeof(struct adv_monitor_device));
> >
> > Please use new0 on new code.
> Done.
>
> >
> > > + if (!dev)
> > > + return NULL;
> > > +
> > > + dev->monitor = monitor;
> > > + dev->device = device;
> > > +
> > > + queue_push_tail(monitor->devices, dev);
> > > +
> > > + return dev;
> > > +}
> > > +
> > > +/* Includes found/lost device's object path into the dbus message */
> > > +static void report_device_state_setup(DBusMessageIter *iter, void *user_data)
> > > +{
> > > + const char *path = device_get_path(user_data);
> > > +
> > > + dbus_message_iter_append_basic(iter, DBUS_TYPE_OBJECT_PATH, &path);
> > > +}
> > > +
> > > +/* Handles a situation where the device goes offline/out-of-range */
> > > +static gboolean handle_device_lost_timeout(gpointer user_data)
> > > +{
> > > + struct adv_monitor_device *dev = user_data;
> > > + struct adv_monitor *monitor = dev->monitor;
> > > + time_t curr_time = time(NULL);
> > > +
> > > + DBG("Device Lost timeout triggered for device %p "
> > > + "for the Adv Monitor at path %s", dev->device, monitor->path);
> > > +
> > > + dev->device_lost_timer = 0;
> > > +
> > > + if (dev->device_found && dev->last_seen) {
> > > + /* We were tracking for the Low RSSI filter. Check if there is
> > > + * any Adv received after the timeout function is invoked.
> > > + * If not, report the Device Lost event.
> > > + */
> > > + if (difftime(curr_time, dev->last_seen) >=
> > > + monitor->low_rssi_timeout) {
> > > + dev->device_found = false;
> > > +
> > > + DBG("Calling DeviceLost() on Adv Monitor of owner %s "
> > > + "at path %s", monitor->app->owner, monitor->path);
> > > +
> > > + g_dbus_proxy_method_call(monitor->proxy, "DeviceLost",
> > > + report_device_state_setup,
> > > + NULL, dev->device, NULL);
> > > + }
> > > + }
> > > +
> > > + return FALSE;
> > > +}
> > > +
> > > +/* Filters an Adv based on its RSSI value */
> > > +static void adv_monitor_filter_rssi(struct adv_monitor *monitor,
> > > + struct btd_device *device, int8_t rssi)
> > > +{
> > > + struct adv_monitor_device *dev = NULL;
> > > + time_t curr_time = time(NULL);
> > > + uint16_t adapter_id = monitor->app->manager->adapter_id;
> > > +
> > > + /* If the RSSI thresholds and timeouts are not specified, report the
> > > + * DeviceFound() event without tracking for the RSSI as the Adv has
> > > + * already matched the pattern filter.
> > > + */
> > > + if (monitor->high_rssi == ADV_MONITOR_UNSET_RSSI &&
> > > + monitor->low_rssi == ADV_MONITOR_UNSET_RSSI &&
> > > + monitor->high_rssi_timeout == ADV_MONITOR_UNSET_TIMER &&
> > > + monitor->low_rssi_timeout == ADV_MONITOR_UNSET_TIMER) {
> > > + DBG("Calling DeviceFound() on Adv Monitor of owner %s "
> > > + "at path %s", monitor->app->owner, monitor->path);
> > > +
> > > + g_dbus_proxy_method_call(monitor->proxy, "DeviceFound",
> > > + report_device_state_setup, NULL,
> > > + device, NULL);
> > > +
> > > + return;
> > > + }
> > > +
> > > + dev = queue_find(monitor->devices, monitor_device_match, device);
> > > + if (!dev)
> > > + dev = monitor_device_create(monitor, device);
> >
> > There seems to be missing indentation here as the following if
> > statement should be nested otherwise the dev point is always evaluated
> > twice which is not optimal.
> Done.
>
> >
> > > + if (!dev) {
> > > + btd_error(adapter_id, "Failed to create Adv Monitor "
> > > + "device object.");
> > > + return;
> > > + }
> >
> > Id put the code above into a function e.g. monitor_device_get so
> > whenever you need to find + create logic you can just use it.
> This is the only case of find + create logic, so I guess that the
> author intentionally leaves it as it is.
Fair enough, I thought Ive seen some other users of the find + create
logic but if it is just one let leave at that.
>
> >
> > > +
> > > + if (dev->device_lost_timer) {
> > > + g_source_remove(dev->device_lost_timer);
> > > + dev->device_lost_timer = 0;
> > > + }
> > > +
> > > + /* Reset the timings of found/lost if a device has been offline for
> > > + * longer than the high/low timeouts.
> > > + */
> > > + if (dev->last_seen) {
> > > + if (difftime(curr_time, dev->last_seen) >
> > > + monitor->high_rssi_timeout) {
> > > + dev->high_rssi_first_seen = 0;
> > > + }
> > > +
> > > + if (difftime(curr_time, dev->last_seen) >
> > > + monitor->low_rssi_timeout) {
> > > + dev->low_rssi_first_seen = 0;
> > > + }
> > > + }
> > > + dev->last_seen = curr_time;
> > > +
> > > + /* Check for the found devices (if the device is not already found) */
> > > + if (!dev->device_found && rssi > monitor->high_rssi) {
> > > + if (dev->high_rssi_first_seen) {
> > > + if (difftime(curr_time, dev->high_rssi_first_seen) >=
> > > + monitor->high_rssi_timeout) {
> > > + dev->device_found = true;
> > > +
> > > + DBG("Calling DeviceFound() on Adv Monitor "
> > > + "of owner %s at path %s",
> > > + monitor->app->owner, monitor->path);
> > > +
> > > + g_dbus_proxy_method_call(
> > > + monitor->proxy, "DeviceFound",
> > > + report_device_state_setup, NULL,
> > > + dev->device, NULL);
> > > + }
> > > + } else {
> > > + dev->high_rssi_first_seen = curr_time;
> > > + }
> > > + } else {
> > > + dev->high_rssi_first_seen = 0;
> > > + }
> > > +
> > > + /* Check for the lost devices (only if the device is already found, as
> > > + * it doesn't make any sense to report the Device Lost event if the
> > > + * device is not found yet)
> > > + */
> > > + if (dev->device_found && rssi < monitor->low_rssi) {
> > > + if (dev->low_rssi_first_seen) {
> > > + if (difftime(curr_time, dev->low_rssi_first_seen) >=
> > > + monitor->low_rssi_timeout) {
> > > + dev->device_found = false;
> > > +
> > > + DBG("Calling DeviceLost() on Adv Monitor "
> > > + "of owner %s at path %s",
> > > + monitor->app->owner, monitor->path);
> > > +
> > > + g_dbus_proxy_method_call(
> > > + monitor->proxy, "DeviceLost",
> > > + report_device_state_setup, NULL,
> > > + dev->device, NULL);
> > > + }
> > > + } else {
> > > + dev->low_rssi_first_seen = curr_time;
> > > + }
> > > + } else {
> > > + dev->low_rssi_first_seen = 0;
> > > + }
> > > +
> > > + /* Setup a timer to track if the device goes offline/out-of-range, only
> > > + * if we are tracking for the Low RSSI Threshold. If we are tracking
> > > + * the High RSSI Threshold, nothing needs to be done.
> > > + */
> > > + if (dev->device_found) {
> > > + dev->device_lost_timer =
> > > + g_timeout_add_seconds(monitor->low_rssi_timeout,
> > > + handle_device_lost_timeout, dev);
> > > + }
> > > +}
> > > diff --git a/src/adv_monitor.h b/src/adv_monitor.h
> > > index 5cb372217..13d5d7282 100644
> > > --- a/src/adv_monitor.h
> > > +++ b/src/adv_monitor.h
> > > @@ -12,6 +12,7 @@
> > > #define __ADV_MONITOR_H
> > >
> > > struct mgmt;
> > > +struct btd_device;
> > > struct btd_adapter;
> > > struct btd_adv_monitor_manager;
> > >
> > > @@ -20,4 +21,7 @@ struct btd_adv_monitor_manager *btd_adv_monitor_manager_create(
> > > struct mgmt *mgmt);
> > > void btd_adv_monitor_manager_destroy(struct btd_adv_monitor_manager *manager);
> > >
> > > +void btd_adv_monitor_device_remove(struct btd_adv_monitor_manager *manager,
> > > + struct btd_device *device);
> > > +
> > > #endif /* __ADV_MONITOR_H */
> > > --
> > > 2.26.2
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz
>
> Regards,
> Miao
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 15+ messages in thread