Return-Path: MIME-Version: 1.0 In-Reply-To: <1427271541-7509-6-git-send-email-jpawlowski@google.com> References: <1427271541-7509-1-git-send-email-jpawlowski@google.com> <1427271541-7509-6-git-send-email-jpawlowski@google.com> Date: Wed, 25 Mar 2015 22:25:51 -0700 Message-ID: Subject: Re: [PATCH v5 6/8] core: adapter: start proper discovery depending on filter type From: Arman Uguray To: Jakub Pawlowski Cc: BlueZ development Content-Type: text/plain; charset=UTF-8 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Jakub, > On Wed, Mar 25, 2015 at 1:18 AM, Jakub Pawlowski wrote: > This patch implement starting and updating filtered discovery, > depending on kind of filters used by each client. It also removes > iddle timeout for filtered scans, to make sure that this kind of > scan will run continuously. > --- > src/adapter.c | 280 +++++++++++++++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 258 insertions(+), 22 deletions(-) > > diff --git a/src/adapter.c b/src/adapter.c > index 402b94a..f6e0093 100644 > --- a/src/adapter.c > +++ b/src/adapter.c > @@ -92,6 +92,7 @@ > #define SCAN_TYPE_LE ((1 << BDADDR_LE_PUBLIC) | (1 << BDADDR_LE_RANDOM)) > #define SCAN_TYPE_DUAL (SCAN_TYPE_BREDR | SCAN_TYPE_LE) > > +#define HCI_RSSI_INVALID 127 > #define DISTANCE_VAL_INVALID 0x7FFF > #define PATHLOSS_MAX 137 > > @@ -205,6 +206,9 @@ struct btd_adapter { > char *stored_alias; /* stored adapter name alias */ > > bool discovering; /* discovering property state */ > + bool filtered_discovery; /* we are doing filtered discovery */ > + bool no_scan_restart_delay; /* when this flag is set, restart scan > + * without delay */ > uint8_t discovery_type; /* current active discovery type */ > uint8_t discovery_enable; /* discovery enabled/disabled */ > bool discovery_suspended; /* discovery has been suspended */ > @@ -212,6 +216,9 @@ struct btd_adapter { > GSList *set_filter_list; /* list of clients that specified > * filter, but don't scan yet > */ > + /* current discovery filter, if any */ > + struct mgmt_cp_start_service_discovery *current_discovery_filter; > + > GSList *discovery_found; /* list of found devices */ > guint discovery_idle_timeout; /* timeout between discovery runs */ > guint passive_scan_timeout; /* timeout between passive scans */ > @@ -1388,6 +1395,11 @@ static void start_discovery_complete(uint8_t status, uint16_t length, > adapter->discovery_type = rp->type; > adapter->discovery_enable = 0x01; > > + if (adapter->current_discovery_filter) > + adapter->filtered_discovery = true; > + else > + adapter->filtered_discovery = false; > + > if (adapter->discovering) > return; > > @@ -1407,21 +1419,32 @@ static void start_discovery_complete(uint8_t status, uint16_t length, > static gboolean start_discovery_timeout(gpointer user_data) > { > struct btd_adapter *adapter = user_data; > - struct mgmt_cp_start_discovery cp; > + struct mgmt_cp_start_service_discovery *sd_cp; > uint8_t new_type; > > DBG(""); > > adapter->discovery_idle_timeout = 0; > > + /* If we're doing filtered discovery, it must be quickly restarted */ > + adapter->no_scan_restart_delay = !!adapter->current_discovery_filter; > + > + DBG("adapter->current_discovery_filter == %d", > + !!adapter->current_discovery_filter); > + > new_type = get_scan_type(adapter); > > if (adapter->discovery_enable == 0x01) { > + struct mgmt_cp_stop_discovery cp; > + > /* > - * If there is an already running discovery and it has the > - * same type, then just keep it. > + * If we're asked to start regular discovery, and there is an > + * already running regular discovery and it has the same type, > + * then just keep it. > */ > - if (adapter->discovery_type == new_type) { > + if (adapter->current_discovery_filter == NULL && > + adapter->filtered_discovery == false && > + adapter->discovery_type == new_type) { Prefer ! over == NULL, here and elsewhere. > if (adapter->discovering) > return FALSE; > > @@ -1436,20 +1459,43 @@ static gboolean start_discovery_timeout(gpointer user_data) > * queue up a stop discovery command. > * > * This can happen if a passive scanning for Low Energy > - * devices is ongoing. > + * devices is ongoing, or scan type is changed between > + * regular and filtered, or filter was updated. > */ > cp.type = adapter->discovery_type; > - > mgmt_send(adapter->mgmt, MGMT_OP_STOP_DISCOVERY, > adapter->dev_id, sizeof(cp), &cp, > NULL, NULL, NULL); > + > + /* Don't even bother to try to quickly start discovery > + * just after stopping it, it would fail with status > + * MGMT_BUSY. Instead discovering_callback will take > + * care of that. > + */ > + return FALSE; > + > } > > - cp.type = new_type; > + if (adapter->current_discovery_filter == NULL) { > + /* Regular discovery is required */ Don't mix comments/code with declarations. Plus this comment should probably go above the if statement. > + struct mgmt_cp_start_discovery cp; > > - mgmt_send(adapter->mgmt, MGMT_OP_START_DISCOVERY, > + cp.type = new_type; > + mgmt_send(adapter->mgmt, MGMT_OP_START_DISCOVERY, > adapter->dev_id, sizeof(cp), &cp, > start_discovery_complete, adapter, NULL); > + return FALSE; > + } > + > + /* Filtered discovery is required */ > + sd_cp = adapter->current_discovery_filter; > + > + DBG("sending MGMT_OP_START_SERVICE_DISCOVERY %d, %d, %d", > + sd_cp->rssi, sd_cp->type, sd_cp->uuid_count); > + > + mgmt_send(adapter->mgmt, MGMT_OP_START_SERVICE_DISCOVERY, > + adapter->dev_id, sizeof(*sd_cp) + sd_cp->uuid_count * 16, > + sd_cp, start_discovery_complete, adapter, NULL); > > return FALSE; > } > @@ -1561,8 +1607,8 @@ static void discovering_callback(uint16_t index, uint16_t length, > return; > } > > - DBG("hci%u type %u discovering %u", adapter->dev_id, > - ev->type, ev->discovering); > + DBG("hci%u type %u discovering %u method %d", adapter->dev_id, ev->type, > + ev->discovering, adapter->filtered_discovery); > > if (adapter->discovery_enable == ev->discovering) > return; > @@ -1588,7 +1634,10 @@ static void discovering_callback(uint16_t index, uint16_t length, > > switch (adapter->discovery_enable) { > case 0x00: > - trigger_start_discovery(adapter, IDLE_DISCOV_TIMEOUT); > + if (adapter->no_scan_restart_delay) > + trigger_start_discovery(adapter, 0); > + else > + trigger_start_discovery(adapter, IDLE_DISCOV_TIMEOUT); > break; > > case 0x01: > @@ -1596,6 +1645,7 @@ static void discovering_callback(uint16_t index, uint16_t length, > g_source_remove(adapter->discovery_idle_timeout); > adapter->discovery_idle_timeout = 0; > } > + > break; > } > } > @@ -1610,7 +1660,8 @@ static void stop_discovery_complete(uint8_t status, uint16_t length, > if (status == MGMT_STATUS_SUCCESS) { > adapter->discovery_type = 0x00; > adapter->discovery_enable = 0x00; > - > + adapter->filtered_discovery = false; > + adapter->no_scan_restart_delay = false; > adapter->discovering = false; > g_dbus_emit_property_changed(dbus_conn, adapter->path, > ADAPTER_INTERFACE, "Discovering"); > @@ -1661,6 +1712,182 @@ static gboolean remove_temp_devices(gpointer user_data) > return FALSE; > } > > +/* This method merges all adapter filters into one that will be send to kernel. > + * cp_ptr is set to null when regular non-filtered discovery is needed, > + * otherwise it's pointing to filter. Returns 0 on succes, -1 on error > + */ > +static int discovery_filter_to_mgmt_cp(struct btd_adapter *adapter, > + struct mgmt_cp_start_service_discovery **cp_ptr) > +{ > + GSList *l, *m; > + struct mgmt_cp_start_service_discovery *cp; > + int rssi = DISTANCE_VAL_INVALID; > + int uuid_count = 0; > + uint8_t discovery_type = 0; > + uint8_t (*current_uuid)[16]; > + /* empty_uuid variable determines wether there was any filter with no > + * uuids. In this case someone might be looking for all devices in > + * certain proximity, and we need to have empty uuids in kernel filter. > + */ Don't mix in comments and variable declarations if possible. > + bool empty_uuid = false; > + bool has_regular_discovery = false; > + bool has_filtered_discovery = false; > + > + DBG(""); nit: add new line after this > + for (l = adapter->discovery_list; l != NULL; l = g_slist_next(l)) { > + struct watch_client *client = l->data; > + struct discovery_filter *item = client->discovery_filter; > + int curr_uuid_count; > + > + if (item == NULL) { > + has_regular_discovery = true; > + continue; > + } > + > + has_filtered_discovery = true; > + > + discovery_type |= item->type; > + > + /* > + * Rule for merging rssi and pathloss into rssi field of kernel > + * filter is as follow: > + * - if there's any client without proximity filter, then do no > + * proximity filtering, > + * - if all clients specified RSSI, then use lowest value, > + * - if any client specified pathloss, then kernel filter should > + * do no proximity, as kernel can't compute pathloss. We'll do > + * filtering on our own. > + */ > + if (item->rssi == DISTANCE_VAL_INVALID) > + rssi = HCI_RSSI_INVALID; > + else if (rssi != HCI_RSSI_INVALID && rssi >= item->rssi) > + rssi = item->rssi; > + else if (item->pathloss != DISTANCE_VAL_INVALID) > + rssi = HCI_RSSI_INVALID; > + > + curr_uuid_count = g_slist_length(item->uuids); > + > + if (curr_uuid_count == 0) > + empty_uuid = true; > + > + uuid_count += curr_uuid_count; > + } > + > + /* if no proximity filtering is set, disable it */ > + if (rssi == DISTANCE_VAL_INVALID) > + rssi = HCI_RSSI_INVALID; > + > + /* if any of filters had empty uuid, do not filter by uuid */ > + if (empty_uuid == true) > + uuid_count = 0; > + > + if (has_regular_discovery) { > + /* > + * It there is both regular and filtered scan running, then > + * clear whole fitler to report all devices. If tere are only > + * regular scans, run just regular scan. > + */ > + if (has_filtered_discovery) { > + discovery_type = get_scan_type(adapter); > + uuid_count = 0; > + rssi = HCI_RSSI_INVALID; > + } else { > + *cp_ptr = NULL; > + return 0; > + } > + } > + > + cp = g_try_malloc(sizeof(cp) + 16*uuid_count); > + *cp_ptr = cp; > + if (cp == NULL) > + return -1; > + > + cp->type = discovery_type; > + cp->rssi = rssi; > + cp->uuid_count = uuid_count; > + > + /* If filter requires no uuids filter, stop processing here */ > + if (uuid_count == 0) > + return 0; > + > + current_uuid = cp->uuids; > + for (l = adapter->discovery_list; l != NULL; l = g_slist_next(l)) { > + struct watch_client *client = l->data; > + struct discovery_filter *item = client->discovery_filter; > + > + for (m = item->uuids; m != NULL; m = g_slist_next(m)) { > + char *uuid_str = m->data; > + bt_uuid_t uuid, u128; > + uint128_t uint128; > + > + bt_string_to_uuid(&uuid, uuid_str); > + bt_uuid_to_uuid128(&uuid, &u128); > + > + ntoh128((uint128_t *) u128.value.u128.data, &uint128); > + htob128(&uint128, (uint128_t *) current_uuid); > + > + current_uuid++; > + } > + } This function is pretty big and has a lot of nested/extra loops, you could probably make use of helpers and g_slist_foreach. > + return 0; > +} > + > +static bool filters_equal(struct mgmt_cp_start_service_discovery *a, > + struct mgmt_cp_start_service_discovery *b) { > + if (a == NULL && b == NULL) > + return true; > + > + if ((a == NULL && b != NULL) || (a != NULL && b == NULL)) > + return false; > + > + if (a->type != b->type) > + return false; > + > + if (a->rssi != b->rssi) > + return false; > + > + /* > + * When we create mgmt_cp_start_service_discovery structure inside > + * discovery_filter_to_mgmt_cp, we always keep uuids sorted, and > + * unique, so we're safe to compare uuid_count, and uuids like that. > + */ > + if (a->uuid_count != b->uuid_count) > + return false; > + > + if (memcmp(a->uuids, b->uuids, 16 * a->uuid_count) != 0) > + return false; > + > + return true; > +} > + > +static void update_discovery_filter(struct btd_adapter *adapter) > +{ > + struct mgmt_cp_start_service_discovery *sd_cp; > + > + DBG(""); > + > + if (discovery_filter_to_mgmt_cp(adapter, &sd_cp)) { > + error("discovery_filter_to_mgmt_cp returned error"); > + return; > + } > + > + /* > + * If filters are equal, then don't update scan, except for when > + * starting discovery. > + */ > + if (filters_equal(adapter->current_discovery_filter, sd_cp) && > + adapter->discovering != 0) { > + DBG("filters were equal, deciding to not restart the scan."); > + g_free(sd_cp); > + return; > + } > + > + g_free(adapter->current_discovery_filter); > + adapter->current_discovery_filter = sd_cp; > + > + trigger_start_discovery(adapter, 0); > +} > + > static void discovery_destroy(void *user_data) > { > struct watch_client *client = user_data; > @@ -1728,8 +1955,10 @@ static void discovery_disconnect(DBusConnection *conn, void *user_data) > * However in case this is the last client, the discovery in > * the kernel needs to be disabled. > */ > - if (adapter->discovery_list) > + if (adapter->discovery_list) { > + update_discovery_filter(adapter); > return; > + } > > /* > * In the idle phase of a discovery, there is no need to stop it > @@ -1751,7 +1980,8 @@ static void discovery_disconnect(DBusConnection *conn, void *user_data) > stop_discovery_complete, adapter, NULL); > } > > -/* returns true if client was already discovering, false otherwise. *client > +/* > + * Return true if client was already discovering, false otherwise. *client > * will point to discovering client, or client that have pre-set his filter. > */ > static bool get_discovery_client(struct btd_adapter *adapter, > @@ -1806,8 +2036,8 @@ static DBusMessage *start_discovery(DBusConnection *conn, > adapter->set_filter_list = g_slist_remove( > adapter->set_filter_list, client); > adapter->discovery_list = g_slist_prepend( > - adapter->discovery_list, client); > - trigger_start_discovery(adapter, 0); > + adapter->discovery_list, client); > + update_discovery_filter(adapter); > return dbus_message_new_method_return(msg); > } > > @@ -1827,7 +2057,7 @@ static DBusMessage *start_discovery(DBusConnection *conn, > * discovery in idle phase exists, it will be restarted right > * away. > */ > - trigger_start_discovery(adapter, 0); > + update_discovery_filter(adapter); > > return dbus_message_new_method_return(msg); > } > @@ -2033,6 +2263,9 @@ static DBusMessage *set_discovery_filter(DBusConnection *conn, > free_discovery_filter(client->discovery_filter); > client->discovery_filter = discovery_filter; > > + if (is_discovering) > + update_discovery_filter(adapter); > + > if (discovery_filter || is_discovering) > return dbus_message_new_method_return(msg); > > @@ -2090,12 +2323,10 @@ static DBusMessage *stop_discovery(DBusConnection *conn, > */ > g_dbus_remove_watch(dbus_conn, client->watch); > > - /* > - * As long as other discovery clients are still active, just > - * return success. > - */ > - if (adapter->discovery_list) > + if (adapter->discovery_list) { > + update_discovery_filter(adapter); > return dbus_message_new_method_return(msg); > + } > > /* > * In the idle phase of a discovery, there is no need to stop it > @@ -5307,6 +5538,11 @@ static void adapter_stop(struct btd_adapter *adapter) > g_dbus_remove_watch(dbus_conn, client->watch); > } > > + adapter->filtered_discovery = false; > + adapter->no_scan_restart_delay = false; > + g_free(adapter->current_discovery_filter); > + adapter->current_discovery_filter = NULL; > + > adapter->discovering = false; > > while (adapter->connections) { > -- > 2.2.0.rc0.207.ga3a616c > > -- > To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Thanks, Arman