Hi Arman,
On Wed, Mar 25, 2015 at 10:25 PM, Arman Uguray <armansito@xxxxxxxxxxxx> wrote:
> Hi Jakub,
>
>> On Wed, Mar 25, 2015 at 1:18 AM, Jakub Pawlowski <jpawlowski@xxxxxxxxxx> 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 !<var> over <var> == NULL, here and elsewhere.
>
fixed in this and following patches
>> 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.
>
fixed
>> + 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.
>
fixed
>> + bool empty_uuid = false;
>> + bool has_regular_discovery = false;
>> + bool has_filtered_discovery = false;
>> +
>> + DBG("");
>
> nit: add new line after this
>
fixed
>> + 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.
>
I added one g_slist_foreach to remove nested loop.
Adding more g_slist_foreach would require adding some structures to
pass data, and it seems like that wouldn't be any more readable.
>> + 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@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> Thanks,
> Arman
Thanks,
Jakub
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
