[RFC 00/16] Discovery procedure refactoring

[RFC 00/16] Discovery procedure refactoring

[Andre Guedes]

Hi all,

Today, discovery procedure is supported only in hciops. This refactoring has
been done to provide easier implementation of discovery procedure in mgmtops.
Lots of effort would be necessary to implement discovery procedure in mgmtops
because its logic is spread out adapter layer and hciops layer.

The approach this patchset follows is moving all logic related to discovery
procedure from adapter layer to hciops layer.

Future work will be:
        - full support for discovery procedure (BR/EDR, LE-Only, BR/EDR/LE
          devices) in kernel via management interface (today, only BR/EDR is
          supported).
        - name resolving support through mgmt interface (kernel + userspace)

Thanks,
Guedes.

Anderson Briglia (1):
  Implement mgmt start and stop discovery

Andre Guedes (15):
  Add discovery callbacks to btd_adapter_ops
  Replace inquiry/scanning calls by discovery calls
  Add 'discov_state' field to struct dev_info
  Code cleanup event.c
  Remove Periodic Inquiry support in hciops
  Change DiscoverSchedulerInterval default value
  Add 'timeout' param to start_scanning callback
  Refactoring adapter_set_state()
  Remove 'suspend' param from stop_discovery()
  Add extfeatures to struct dev_info
  Implement start_discovery hciops callback
  Remove obsolete code.
  Implement stop_discovery hciops callback
  Remove inquiry and scanning callbacks from btd_adapter_ops
  Remove 'periodic' param from hciops_start_inquiry()

 plugins/hciops.c  |  373 ++++++++++++++++++++++++++++++++++++-----------------
 plugins/mgmtops.c |   35 ++----
 src/adapter.c     |  217 ++++++++++---------------------
 src/adapter.h     |   19 +--
 src/event.c       |   48 +-------
 src/event.h       |    1 -
 src/main.conf     |    4 +-
 7 files changed, 345 insertions(+), 352 deletions(-)

[RFC 01/16] Add discovery callbacks to btd_adapter_ops

[Andre Guedes]

This patch adds start_discovery and stop_discovery callbacks to
struct btd_adapter_ops. These callbacks are responsible for starting
and stoping the discovery procedure. It also creates dummy functions
in hciops and mgmtops which implements those callbacks.
---
 plugins/hciops.c  |   14 ++++++++++++++
 plugins/mgmtops.c |   14 ++++++++++++++
 src/adapter.h     |    2 ++
 3 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/plugins/hciops.c b/plugins/hciops.c
index d1156e2..14c973f 100644
--- a/plugins/hciops.c
+++ b/plugins/hciops.c
@@ -3122,6 +3122,18 @@ static int hciops_cancel_resolve_name(int index, bdaddr_t *bdaddr)
 	return 0;
 }
 
+static int hciops_start_discovery(int index)
+{
+	DBG("index %d", index);
+	return -ENOSYS;
+}
+
+static int hciops_stop_discovery(int index)
+{
+	DBG("index %d", index);
+	return -ENOSYS;
+}
+
 static int hciops_fast_connectable(int index, gboolean enable)
 {
 	struct dev_info *dev = &devs[index];
@@ -3671,6 +3683,8 @@ static struct btd_adapter_ops hci_ops = {
 	.set_discoverable = hciops_set_discoverable,
 	.set_pairable = hciops_set_pairable,
 	.set_limited_discoverable = hciops_set_limited_discoverable,
+	.start_discovery = hciops_start_discovery,
+	.stop_discovery = hciops_stop_discovery,
 	.start_inquiry = hciops_start_inquiry,
 	.stop_inquiry = hciops_stop_inquiry,
 	.start_scanning = hciops_start_scanning,
diff --git a/plugins/mgmtops.c b/plugins/mgmtops.c
index a8a7ac2..ac180f9 100644
--- a/plugins/mgmtops.c
+++ b/plugins/mgmtops.c
@@ -1588,6 +1588,18 @@ static int mgmt_set_limited_discoverable(int index, gboolean limited)
 	return -ENOSYS;
 }
 
+static int mgmt_start_discovery(int index)
+{
+	DBG("index %d", index);
+	return -ENOSYS;
+}
+
+static int mgmt_stop_discovery(int index)
+{
+	DBG("index %d", index);
+	return -ENOSYS;
+}
+
 static int mgmt_start_inquiry(int index, uint8_t length, gboolean periodic)
 {
 	struct mgmt_hdr hdr;
@@ -2026,6 +2038,8 @@ static struct btd_adapter_ops mgmt_ops = {
 	.set_discoverable = mgmt_set_discoverable,
 	.set_pairable = mgmt_set_pairable,
 	.set_limited_discoverable = mgmt_set_limited_discoverable,
+	.start_discovery = mgmt_start_discovery,
+	.stop_discovery = mgmt_stop_discovery,
 	.start_inquiry = mgmt_start_inquiry,
 	.stop_inquiry = mgmt_stop_inquiry,
 	.start_scanning = mgmt_start_scanning,
diff --git a/src/adapter.h b/src/adapter.h
index 7cc7c02..1a5c687 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -187,6 +187,8 @@ struct btd_adapter_ops {
 	int (*set_discoverable) (int index, gboolean discoverable);
 	int (*set_pairable) (int index, gboolean pairable);
 	int (*set_limited_discoverable) (int index, gboolean limited);
+	int (*start_discovery) (int index);
+	int (*stop_discovery) (int index);
 	int (*start_inquiry) (int index, uint8_t length, gboolean periodic);
 	int (*stop_inquiry) (int index);
 	int (*start_scanning) (int index);
-- 
1.7.1

[RFC 02/16] Replace inquiry/scanning calls by discovery calls

[Andre Guedes]

This patch replaces calls to btd_adapter_ops callbacks start_inquiry,
start_scanning, stop_inquiry and stop_scanning by start_discovery and
stop_discovery.
---
 src/adapter.c |   23 ++---------------------
 1 files changed, 2 insertions(+), 21 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index 8dbd62c..1dfa9a6 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -726,10 +726,7 @@ static void stop_discovery(struct btd_adapter *adapter, gboolean suspend)
 		return;
 	}
 
-	if (adapter->state & STATE_LE_SCAN)
-		adapter_ops->stop_scanning(adapter->dev_id);
-	else
-		adapter_ops->stop_inquiry(adapter->dev_id);
+	adapter_ops->stop_discovery(adapter->dev_id);
 }
 
 static void session_remove(struct session_req *req)
@@ -1218,8 +1215,6 @@ static gboolean stop_scanning(gpointer user_data)
 
 static int start_discovery(struct btd_adapter *adapter)
 {
-	int type;
-
 	/* Do not start if suspended */
 	if (adapter->state & STATE_SUSPENDED)
 		return 0;
@@ -1230,21 +1225,7 @@ static int start_discovery(struct btd_adapter *adapter)
 
 	pending_remote_name_cancel(adapter);
 
-	type = adapter_get_discover_type(adapter) & ~DISC_RESOLVNAME;
-
-	switch (type) {
-	case DISC_STDINQ:
-	case DISC_INTERLEAVE:
-		return adapter_ops->start_inquiry(adapter->dev_id,
-							0x08, FALSE);
-	case DISC_PINQ:
-		return adapter_ops->start_inquiry(adapter->dev_id,
-							0x08, TRUE);
-	case DISC_LE:
-		return adapter_ops->start_scanning(adapter->dev_id);
-	default:
-		return -EINVAL;
-	}
+	return adapter_ops->start_discovery(adapter->dev_id);
 }
 
 static gboolean discovery_cb(gpointer user_data)
-- 
1.7.1

[RFC 03/16] Add 'discov_state' field to struct dev_info

[Andre Guedes]

We need to track the current discovering state (HALTED, INQUIRY or SCAN)
of the adapter in hciops layer during the discovery procedure. This will
help us to properly update the state of btd_adapter and emit the property
"Discovering" correctly.
---
 plugins/hciops.c |   90 +++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 85 insertions(+), 5 deletions(-)

diff --git a/plugins/hciops.c b/plugins/hciops.c
index 14c973f..dfd00b1 100644
--- a/plugins/hciops.c
+++ b/plugins/hciops.c
@@ -52,6 +52,10 @@
 #include "manager.h"
 #include "oob.h"
 
+#define DISCOV_HALTED 0
+#define DISCOV_INQ 1
+#define DISCOV_SCAN 2
+
 static int child_pipe[2] = { -1, -1 };
 
 static guint child_io_id = 0;
@@ -102,6 +106,8 @@ static struct dev_info {
 
 	int8_t tx_power;
 
+	int discov_state;
+
 	uint32_t current_cod;
 	uint32_t wanted_cod;
 	uint32_t pending_cod;
@@ -135,6 +141,34 @@ static struct dev_info {
 	GSList *connections;
 } *devs = NULL;
 
+static inline int get_state(int index)
+{
+	struct dev_info *dev = &devs[index];
+
+	return dev->discov_state;
+}
+
+static void set_state(int index, int state)
+{
+	struct dev_info *dev = &devs[index];
+
+	if (dev->discov_state == state)
+		return;
+
+	dev->discov_state = state;
+
+	DBG("hci%d: new state %d", index, dev->discov_state);
+
+	switch (dev->discov_state) {
+	case DISCOV_HALTED:
+		break;
+	case DISCOV_INQ:
+		break;
+	case DISCOV_SCAN:
+		break;
+	}
+}
+
 static int ignore_device(struct hci_dev_info *di)
 {
 	return hci_test_bit(HCI_RAW, &di->flags) || di->type >> 4 != HCI_BREDR;
@@ -153,6 +187,7 @@ static struct dev_info *init_dev_info(int index, int sk, gboolean registered,
 	dev->registered = registered;
 	dev->already_up = already_up;
 	dev->io_capability = 0x03; /* No Input No Output */
+	dev->discov_state = DISCOV_HALTED;
 
 	return dev;
 }
@@ -1713,14 +1748,23 @@ static void read_bd_addr_complete(int index, read_bd_addr_rp *rp)
 		init_adapter(index);
 }
 
+static inline void cs_inquiry_evt(int index, uint8_t status)
+{
+	if (status) {
+		error("Inquiry Failed with status 0x%02x", status);
+		return;
+	}
+
+	set_state(index, DISCOV_INQ);
+}
+
 static inline void cmd_status(int index, void *ptr)
 {
-	struct dev_info *dev = &devs[index];
 	evt_cmd_status *evt = ptr;
 	uint16_t opcode = btohs(evt->opcode);
 
 	if (opcode == cmd_opcode_pack(OGF_LINK_CTL, OCF_INQUIRY))
-		start_inquiry(&dev->bdaddr, evt->status, FALSE);
+		cs_inquiry_evt(index, evt->status);
 }
 
 static void read_scan_complete(int index, uint8_t status, void *ptr)
@@ -1841,6 +1885,42 @@ static void read_local_oob_data_complete(int index, uint8_t status,
 		oob_read_local_data_complete(adapter, rp->hash, rp->randomizer);
 }
 
+static inline void inquiry_complete_evt(int index, uint8_t status)
+{
+	if (status) {
+		error("Inquiry Failed with status 0x%02x", status);
+		return;
+	}
+
+	set_state(index, DISCOV_HALTED);
+}
+
+static inline void cc_inquiry_cancel(int index, uint8_t status)
+{
+	if (status) {
+		error("Inquiry Cancel Failed with status 0x%02x", status);
+		return;
+	}
+
+	set_state(index, DISCOV_HALTED);
+}
+
+static inline void cc_le_set_scan_enable(int index, uint8_t status)
+{
+	int state;
+
+	if (status) {
+		error("LE Set Scan Enable Failed with status 0x%02x", status);
+		return;
+	}
+
+	state = get_state(index);
+	if (state == DISCOV_SCAN)
+		set_state(index, DISCOV_HALTED);
+	else
+		set_state(index, DISCOV_SCAN);
+}
+
 static inline void cmd_complete(int index, void *ptr)
 {
 	struct dev_info *dev = &devs[index];
@@ -1872,13 +1952,13 @@ static inline void cmd_complete(int index, void *ptr)
 		inquiry_complete(&dev->bdaddr, status, TRUE);
 		break;
 	case cmd_opcode_pack(OGF_LINK_CTL, OCF_INQUIRY_CANCEL):
-		inquiry_complete(&dev->bdaddr, status, FALSE);
+		cc_inquiry_cancel(index, status);
 		break;
 	case cmd_opcode_pack(OGF_HOST_CTL, OCF_WRITE_LE_HOST_SUPPORTED):
 		write_le_host_complete(index, status);
 		break;
 	case cmd_opcode_pack(OGF_LE_CTL, OCF_LE_SET_SCAN_ENABLE):
-		btd_event_le_set_scan_enable_complete(&dev->bdaddr, status);
+		cc_le_set_scan_enable(index, status);
 		break;
 	case cmd_opcode_pack(OGF_HOST_CTL, OCF_CHANGE_LOCAL_NAME):
 		if (!status)
@@ -2373,7 +2453,7 @@ static gboolean io_security_event(GIOChannel *chan, GIOCondition cond,
 
 	case EVT_INQUIRY_COMPLETE:
 		evt = (evt_cmd_status *) ptr;
-		inquiry_complete(&dev->bdaddr, evt->status, FALSE);
+		inquiry_complete_evt(index, evt->status);
 		break;
 
 	case EVT_INQUIRY_RESULT:
-- 
1.7.1

[RFC 04/16] Code cleanup event.c

[Andre Guedes]

Remove btd_event_le_set_scan_enable_complete() since it is not
used anymore.

The adapter's state is updated in cc_le_set_scan_enable() which
handles LE Set Scan Enable command complete events.
---
 src/event.c |   27 ---------------------------
 src/event.h |    1 -
 2 files changed, 0 insertions(+), 28 deletions(-)

diff --git a/src/event.c b/src/event.c
index 5373e33..7feec1f 100644
--- a/src/event.c
+++ b/src/event.c
@@ -690,33 +690,6 @@ void btd_event_disconn_complete(bdaddr_t *local, bdaddr_t *peer)
 
 /* Section reserved to device HCI callbacks */
 
-void btd_event_le_set_scan_enable_complete(bdaddr_t *local, uint8_t status)
-{
-	struct btd_adapter *adapter;
-	int state;
-
-	adapter = manager_find_adapter(local);
-	if (!adapter) {
-		error("No matching adapter found");
-		return;
-	}
-
-	if (status) {
-		error("Can't enable/disable LE scan");
-		return;
-	}
-
-	state = adapter_get_state(adapter);
-
-	/* Enabling or disabling ? */
-	if (state & STATE_LE_SCAN)
-		state &= ~STATE_LE_SCAN;
-	else
-		state |= STATE_LE_SCAN;
-
-	adapter_set_state(adapter, state);
-}
-
 void btd_event_returned_link_key(bdaddr_t *local, bdaddr_t *peer)
 {
 	struct btd_adapter *adapter;
diff --git a/src/event.h b/src/event.h
index 765390a..005d8a7 100644
--- a/src/event.h
+++ b/src/event.h
@@ -35,7 +35,6 @@ void btd_event_disconn_complete(bdaddr_t *local, bdaddr_t *peer);
 void btd_event_bonding_complete(bdaddr_t *local, bdaddr_t *peer,
 							uint8_t status);
 void btd_event_simple_pairing_complete(bdaddr_t *local, bdaddr_t *peer, uint8_t status);
-void btd_event_le_set_scan_enable_complete(bdaddr_t *local, uint8_t status);
 void btd_event_returned_link_key(bdaddr_t *local, bdaddr_t *peer);
 int btd_event_user_confirm(bdaddr_t *sba, bdaddr_t *dba, uint32_t passkey);
 int btd_event_user_passkey(bdaddr_t *sba, bdaddr_t *dba);
-- 
1.7.1

[RFC 05/16] Remove Periodic Inquiry support in hciops

[Andre Guedes]

Periodic Inquiry is no longer supported in hciops since the discovery
procedure will use Standard Inquiry only.

External tools which request Periodic Inquiry will not change the
adapter's state.
---
 plugins/hciops.c |  112 +++++++----------------------------------------------
 src/event.c      |   13 +------
 2 files changed, 16 insertions(+), 109 deletions(-)

diff --git a/plugins/hciops.c b/plugins/hciops.c
index dfd00b1..81a0f29 100644
--- a/plugins/hciops.c
+++ b/plugins/hciops.c
@@ -507,24 +507,13 @@ static void start_adapter(int index)
 static int hciops_stop_inquiry(int index)
 {
 	struct dev_info *dev = &devs[index];
-	struct hci_dev_info di;
-	int err;
 
 	DBG("hci%d", index);
 
-	if (hci_devinfo(index, &di) < 0)
+	if (hci_send_cmd(dev->sk, OGF_LINK_CTL, OCF_INQUIRY_CANCEL, 0, 0) < 0)
 		return -errno;
 
-	if (hci_test_bit(HCI_INQUIRY, &di.flags))
-		err = hci_send_cmd(dev->sk, OGF_LINK_CTL,
-						OCF_INQUIRY_CANCEL, 0, 0);
-	else
-		err = hci_send_cmd(dev->sk, OGF_LINK_CTL,
-					OCF_EXIT_PERIODIC_INQUIRY, 0, 0);
-	if (err < 0)
-		err = -errno;
-
-	return err;
+	return 0;
 }
 
 static gboolean init_adapter(int index)
@@ -1306,56 +1295,6 @@ reject:
 	hci_send_cmd(dev->sk, OGF_LINK_CTL, OCF_PIN_CODE_NEG_REPLY, 6, dba);
 }
 
-static void start_inquiry(bdaddr_t *local, uint8_t status, gboolean periodic)
-{
-	struct btd_adapter *adapter;
-	int state;
-
-	/* Don't send the signal if the cmd failed */
-	if (status) {
-		error("Inquiry Failed with status 0x%02x", status);
-		return;
-	}
-
-	adapter = manager_find_adapter(local);
-	if (!adapter) {
-		error("Unable to find matching adapter");
-		return;
-	}
-
-	state = adapter_get_state(adapter);
-
-	if (periodic)
-		state |= STATE_PINQ;
-	else
-		state |= STATE_STDINQ;
-
-	adapter_set_state(adapter, state);
-}
-
-static void inquiry_complete(bdaddr_t *local, uint8_t status,
-							gboolean periodic)
-{
-	struct btd_adapter *adapter;
-	int state;
-
-	/* Don't send the signal if the cmd failed */
-	if (status) {
-		error("Inquiry Failed with status 0x%02x", status);
-		return;
-	}
-
-	adapter = manager_find_adapter(local);
-	if (!adapter) {
-		error("Unable to find matching adapter");
-		return;
-	}
-
-	state = adapter_get_state(adapter);
-	state &= ~(STATE_STDINQ | STATE_PINQ);
-	adapter_set_state(adapter, state);
-}
-
 static inline void remote_features_notify(int index, void *ptr)
 {
 	struct dev_info *dev = &devs[index];
@@ -1945,12 +1884,6 @@ static inline void cmd_complete(int index, void *ptr)
 		ptr += sizeof(evt_cmd_complete);
 		read_bd_addr_complete(index, ptr);
 		break;
-	case cmd_opcode_pack(OGF_LINK_CTL, OCF_PERIODIC_INQUIRY):
-		start_inquiry(&dev->bdaddr, status, TRUE);
-		break;
-	case cmd_opcode_pack(OGF_LINK_CTL, OCF_EXIT_PERIODIC_INQUIRY):
-		inquiry_complete(&dev->bdaddr, status, TRUE);
-		break;
 	case cmd_opcode_pack(OGF_LINK_CTL, OCF_INQUIRY_CANCEL):
 		cc_inquiry_cancel(index, status);
 		break;
@@ -2043,6 +1976,10 @@ static inline void inquiry_result(int index, int plen, void *ptr)
 	uint8_t num = *(uint8_t *) ptr++;
 	int i;
 
+	/* Skip if it is not in Inquiry state */
+	if (get_state(index) != DISCOV_INQ)
+		return;
+
 	for (i = 0; i < num; i++) {
 		inquiry_info *info = ptr;
 		uint32_t class = info->dev_class[0] |
@@ -3060,39 +2997,20 @@ static int hciops_start_inquiry(int index, uint8_t length, gboolean periodic)
 {
 	struct dev_info *dev = &devs[index];
 	uint8_t lap[3] = { 0x33, 0x8b, 0x9e };
-	int err;
+	inquiry_cp inq_cp;
 
 	DBG("hci%d length %u periodic %d", index, length, periodic);
 
-	if (periodic) {
-		periodic_inquiry_cp cp;
-
-		memset(&cp, 0, sizeof(cp));
-		memcpy(&cp.lap, lap, 3);
-		cp.max_period = htobs(24);
-		cp.min_period = htobs(16);
-		cp.length  = length;
-		cp.num_rsp = 0x00;
-
-		err = hci_send_cmd(dev->sk, OGF_LINK_CTL,
-						OCF_PERIODIC_INQUIRY,
-						PERIODIC_INQUIRY_CP_SIZE, &cp);
-	} else {
-		inquiry_cp inq_cp;
-
-		memset(&inq_cp, 0, sizeof(inq_cp));
-		memcpy(&inq_cp.lap, lap, 3);
-		inq_cp.length = length;
-		inq_cp.num_rsp = 0x00;
+	memset(&inq_cp, 0, sizeof(inq_cp));
+	memcpy(&inq_cp.lap, lap, 3);
+	inq_cp.length = length;
+	inq_cp.num_rsp = 0x00;
 
-		err = hci_send_cmd(dev->sk, OGF_LINK_CTL,
-					OCF_INQUIRY, INQUIRY_CP_SIZE, &inq_cp);
-	}
-
-	if (err < 0)
-		err = -errno;
+	if (hci_send_cmd(dev->sk, OGF_LINK_CTL,
+			OCF_INQUIRY, INQUIRY_CP_SIZE, &inq_cp) < 0)
+		return -errno;
 
-	return err;
+	return 0;
 }
 
 static int le_set_scan_enable(int index, uint8_t enable)
diff --git a/src/event.c b/src/event.c
index 7feec1f..b04220a 100644
--- a/src/event.c
+++ b/src/event.c
@@ -435,7 +435,7 @@ void btd_event_device_found(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
 	char local_addr[18], peer_addr[18], *alias, *name;
 	name_status_t name_status;
 	struct eir_data eir_data;
-	int state, err;
+	int err;
 	dbus_bool_t legacy;
 	unsigned char features[8];
 	const char *dev_name;
@@ -455,17 +455,6 @@ void btd_event_device_found(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
 	if (data)
 		write_remote_eir(local, peer, data);
 
-	/*
-	 * Workaround to identify periodic inquiry: inquiry complete event is
-	 * sent after each window, however there isn't an event to indicate the
-	 * beginning of a new periodic inquiry window.
-	 */
-	state = adapter_get_state(adapter);
-	if (!(state & (STATE_STDINQ | STATE_LE_SCAN | STATE_PINQ))) {
-		state |= STATE_PINQ;
-		adapter_set_state(adapter, state);
-	}
-
 	/* the inquiry result can be triggered by NON D-Bus client */
 	if (adapter_get_discover_type(adapter) & DISC_RESOLVNAME &&
 				adapter_has_discov_sessions(adapter))
-- 
1.7.1

Re: [RFC 05/16] Remove Periodic Inquiry support in hciops

[Luiz Augusto von Dentz]

Hi Andre,

On Sat, Apr 30, 2011 at 3:27 AM, Andre Guedes
<andre.guedes@openbossa.org> wrote:
> Periodic Inquiry is no longer supported in hciops since the discovery
> procedure will use Standard Inquiry only.
>
> External tools which request Periodic Inquiry will not change the
> adapter's state.
> ---
>  plugins/hciops.c |  112 +++++++----------------------------------------------
>  src/event.c      |   13 +------
>  2 files changed, 16 insertions(+), 109 deletions(-)
>
> diff --git a/plugins/hciops.c b/plugins/hciops.c
> index dfd00b1..81a0f29 100644
> --- a/plugins/hciops.c
> +++ b/plugins/hciops.c
> @@ -507,24 +507,13 @@ static void start_adapter(int index)
>  static int hciops_stop_inquiry(int index)
>  {
>        struct dev_info *dev = &devs[index];
> -       struct hci_dev_info di;
> -       int err;
>
>        DBG("hci%d", index);
>
> -       if (hci_devinfo(index, &di) < 0)
> +       if (hci_send_cmd(dev->sk, OGF_LINK_CTL, OCF_INQUIRY_CANCEL, 0, 0) < 0)
>                return -errno;
>
> -       if (hci_test_bit(HCI_INQUIRY, &di.flags))
> -               err = hci_send_cmd(dev->sk, OGF_LINK_CTL,
> -                                               OCF_INQUIRY_CANCEL, 0, 0);
> -       else
> -               err = hci_send_cmd(dev->sk, OGF_LINK_CTL,
> -                                       OCF_EXIT_PERIODIC_INQUIRY, 0, 0);
> -       if (err < 0)
> -               err = -errno;
> -
> -       return err;
> +       return 0;
>  }
>
>  static gboolean init_adapter(int index)
> @@ -1306,56 +1295,6 @@ reject:
>        hci_send_cmd(dev->sk, OGF_LINK_CTL, OCF_PIN_CODE_NEG_REPLY, 6, dba);
>  }
>
> -static void start_inquiry(bdaddr_t *local, uint8_t status, gboolean periodic)
> -{
> -       struct btd_adapter *adapter;
> -       int state;
> -
> -       /* Don't send the signal if the cmd failed */
> -       if (status) {
> -               error("Inquiry Failed with status 0x%02x", status);
> -               return;
> -       }
> -
> -       adapter = manager_find_adapter(local);
> -       if (!adapter) {
> -               error("Unable to find matching adapter");
> -               return;
> -       }
> -
> -       state = adapter_get_state(adapter);
> -
> -       if (periodic)
> -               state |= STATE_PINQ;
> -       else
> -               state |= STATE_STDINQ;
> -
> -       adapter_set_state(adapter, state);
> -}
> -
> -static void inquiry_complete(bdaddr_t *local, uint8_t status,
> -                                                       gboolean periodic)
> -{
> -       struct btd_adapter *adapter;
> -       int state;
> -
> -       /* Don't send the signal if the cmd failed */
> -       if (status) {
> -               error("Inquiry Failed with status 0x%02x", status);
> -               return;
> -       }
> -
> -       adapter = manager_find_adapter(local);
> -       if (!adapter) {
> -               error("Unable to find matching adapter");
> -               return;
> -       }
> -
> -       state = adapter_get_state(adapter);
> -       state &= ~(STATE_STDINQ | STATE_PINQ);
> -       adapter_set_state(adapter, state);
> -}
> -
>  static inline void remote_features_notify(int index, void *ptr)
>  {
>        struct dev_info *dev = &devs[index];
> @@ -1945,12 +1884,6 @@ static inline void cmd_complete(int index, void *ptr)
>                ptr += sizeof(evt_cmd_complete);
>                read_bd_addr_complete(index, ptr);
>                break;
> -       case cmd_opcode_pack(OGF_LINK_CTL, OCF_PERIODIC_INQUIRY):
> -               start_inquiry(&dev->bdaddr, status, TRUE);
> -               break;
> -       case cmd_opcode_pack(OGF_LINK_CTL, OCF_EXIT_PERIODIC_INQUIRY):
> -               inquiry_complete(&dev->bdaddr, status, TRUE);
> -               break;
>        case cmd_opcode_pack(OGF_LINK_CTL, OCF_INQUIRY_CANCEL):
>                cc_inquiry_cancel(index, status);
>                break;
> @@ -2043,6 +1976,10 @@ static inline void inquiry_result(int index, int plen, void *ptr)
>        uint8_t num = *(uint8_t *) ptr++;
>        int i;
>
> +       /* Skip if it is not in Inquiry state */
> +       if (get_state(index) != DISCOV_INQ)
> +               return;
> +
>        for (i = 0; i < num; i++) {
>                inquiry_info *info = ptr;
>                uint32_t class = info->dev_class[0] |
> @@ -3060,39 +2997,20 @@ static int hciops_start_inquiry(int index, uint8_t length, gboolean periodic)
>  {
>        struct dev_info *dev = &devs[index];
>        uint8_t lap[3] = { 0x33, 0x8b, 0x9e };
> -       int err;
> +       inquiry_cp inq_cp;
>
>        DBG("hci%d length %u periodic %d", index, length, periodic);
>
> -       if (periodic) {
> -               periodic_inquiry_cp cp;
> -
> -               memset(&cp, 0, sizeof(cp));
> -               memcpy(&cp.lap, lap, 3);
> -               cp.max_period = htobs(24);
> -               cp.min_period = htobs(16);
> -               cp.length  = length;
> -               cp.num_rsp = 0x00;
> -
> -               err = hci_send_cmd(dev->sk, OGF_LINK_CTL,
> -                                               OCF_PERIODIC_INQUIRY,
> -                                               PERIODIC_INQUIRY_CP_SIZE, &cp);
> -       } else {
> -               inquiry_cp inq_cp;
> -
> -               memset(&inq_cp, 0, sizeof(inq_cp));
> -               memcpy(&inq_cp.lap, lap, 3);
> -               inq_cp.length = length;
> -               inq_cp.num_rsp = 0x00;
> +       memset(&inq_cp, 0, sizeof(inq_cp));
> +       memcpy(&inq_cp.lap, lap, 3);
> +       inq_cp.length = length;
> +       inq_cp.num_rsp = 0x00;
>
> -               err = hci_send_cmd(dev->sk, OGF_LINK_CTL,
> -                                       OCF_INQUIRY, INQUIRY_CP_SIZE, &inq_cp);
> -       }
> -
> -       if (err < 0)
> -               err = -errno;
> +       if (hci_send_cmd(dev->sk, OGF_LINK_CTL,
> +                       OCF_INQUIRY, INQUIRY_CP_SIZE, &inq_cp) < 0)
> +               return -errno;
>
> -       return err;
> +       return 0;
>  }
>
>  static int le_set_scan_enable(int index, uint8_t enable)
> diff --git a/src/event.c b/src/event.c
> index 7feec1f..b04220a 100644
> --- a/src/event.c
> +++ b/src/event.c
> @@ -435,7 +435,7 @@ void btd_event_device_found(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
>        char local_addr[18], peer_addr[18], *alias, *name;
>        name_status_t name_status;
>        struct eir_data eir_data;
> -       int state, err;
> +       int err;
>        dbus_bool_t legacy;
>        unsigned char features[8];
>        const char *dev_name;
> @@ -455,17 +455,6 @@ void btd_event_device_found(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
>        if (data)
>                write_remote_eir(local, peer, data);
>
> -       /*
> -        * Workaround to identify periodic inquiry: inquiry complete event is
> -        * sent after each window, however there isn't an event to indicate the
> -        * beginning of a new periodic inquiry window.
> -        */
> -       state = adapter_get_state(adapter);
> -       if (!(state & (STATE_STDINQ | STATE_LE_SCAN | STATE_PINQ))) {
> -               state |= STATE_PINQ;
> -               adapter_set_state(adapter, state);
> -       }
> -
>        /* the inquiry result can be triggered by NON D-Bus client */
>        if (adapter_get_discover_type(adapter) & DISC_RESOLVNAME &&
>                                adapter_has_discov_sessions(adapter))
> --
> 1.7.1
>

What exactly is reason to remove periodic inquiry? I see the point for
dual mode devices, but for regular BD/EDR its very useful since the
controller takes care of the scheduling and we don't have to send
another command each round. Also we need to be able to detect if pinq
was activate by the user somehow, since it may cause some bugs which
will be very to debug since you are removing the state tracking.

-- 
Luiz Augusto von Dentz
Computer Engineer

Re: [RFC 05/16] Remove Periodic Inquiry support in hciops

[Andre Guedes]

Hi Luiz,

On Mon, May 2, 2011 at 4:38 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Andre,
>
> On Sat, Apr 30, 2011 at 3:27 AM, Andre Guedes
> <andre.guedes@openbossa.org> wrote:
>> Periodic Inquiry is no longer supported in hciops since the discovery
>> procedure will use Standard Inquiry only.
>>
>> External tools which request Periodic Inquiry will not change the
>> adapter's state.
>> ---
>>  plugins/hciops.c |  112 +++++++----------------------------------------------
>>  src/event.c      |   13 +------
>>  2 files changed, 16 insertions(+), 109 deletions(-)
>>
>> diff --git a/plugins/hciops.c b/plugins/hciops.c
>> index dfd00b1..81a0f29 100644
>> --- a/plugins/hciops.c
>> +++ b/plugins/hciops.c
>> @@ -507,24 +507,13 @@ static void start_adapter(int index)
>>  static int hciops_stop_inquiry(int index)
>>  {
>>        struct dev_info *dev = &devs[index];
>> -       struct hci_dev_info di;
>> -       int err;
>>
>>        DBG("hci%d", index);
>>
>> -       if (hci_devinfo(index, &di) < 0)
>> +       if (hci_send_cmd(dev->sk, OGF_LINK_CTL, OCF_INQUIRY_CANCEL, 0, 0) < 0)
>>                return -errno;
>>
>> -       if (hci_test_bit(HCI_INQUIRY, &di.flags))
>> -               err = hci_send_cmd(dev->sk, OGF_LINK_CTL,
>> -                                               OCF_INQUIRY_CANCEL, 0, 0);
>> -       else
>> -               err = hci_send_cmd(dev->sk, OGF_LINK_CTL,
>> -                                       OCF_EXIT_PERIODIC_INQUIRY, 0, 0);
>> -       if (err < 0)
>> -               err = -errno;
>> -
>> -       return err;
>> +       return 0;
>>  }
>>
>>  static gboolean init_adapter(int index)
>> @@ -1306,56 +1295,6 @@ reject:
>>        hci_send_cmd(dev->sk, OGF_LINK_CTL, OCF_PIN_CODE_NEG_REPLY, 6, dba);
>>  }
>>
>> -static void start_inquiry(bdaddr_t *local, uint8_t status, gboolean periodic)
>> -{
>> -       struct btd_adapter *adapter;
>> -       int state;
>> -
>> -       /* Don't send the signal if the cmd failed */
>> -       if (status) {
>> -               error("Inquiry Failed with status 0x%02x", status);
>> -               return;
>> -       }
>> -
>> -       adapter = manager_find_adapter(local);
>> -       if (!adapter) {
>> -               error("Unable to find matching adapter");
>> -               return;
>> -       }
>> -
>> -       state = adapter_get_state(adapter);
>> -
>> -       if (periodic)
>> -               state |= STATE_PINQ;
>> -       else
>> -               state |= STATE_STDINQ;
>> -
>> -       adapter_set_state(adapter, state);
>> -}
>> -
>> -static void inquiry_complete(bdaddr_t *local, uint8_t status,
>> -                                                       gboolean periodic)
>> -{
>> -       struct btd_adapter *adapter;
>> -       int state;
>> -
>> -       /* Don't send the signal if the cmd failed */
>> -       if (status) {
>> -               error("Inquiry Failed with status 0x%02x", status);
>> -               return;
>> -       }
>> -
>> -       adapter = manager_find_adapter(local);
>> -       if (!adapter) {
>> -               error("Unable to find matching adapter");
>> -               return;
>> -       }
>> -
>> -       state = adapter_get_state(adapter);
>> -       state &= ~(STATE_STDINQ | STATE_PINQ);
>> -       adapter_set_state(adapter, state);
>> -}
>> -
>>  static inline void remote_features_notify(int index, void *ptr)
>>  {
>>        struct dev_info *dev = &devs[index];
>> @@ -1945,12 +1884,6 @@ static inline void cmd_complete(int index, void *ptr)
>>                ptr += sizeof(evt_cmd_complete);
>>                read_bd_addr_complete(index, ptr);
>>                break;
>> -       case cmd_opcode_pack(OGF_LINK_CTL, OCF_PERIODIC_INQUIRY):
>> -               start_inquiry(&dev->bdaddr, status, TRUE);
>> -               break;
>> -       case cmd_opcode_pack(OGF_LINK_CTL, OCF_EXIT_PERIODIC_INQUIRY):
>> -               inquiry_complete(&dev->bdaddr, status, TRUE);
>> -               break;
>>        case cmd_opcode_pack(OGF_LINK_CTL, OCF_INQUIRY_CANCEL):
>>                cc_inquiry_cancel(index, status);
>>                break;
>> @@ -2043,6 +1976,10 @@ static inline void inquiry_result(int index, int plen, void *ptr)
>>        uint8_t num = *(uint8_t *) ptr++;
>>        int i;
>>
>> +       /* Skip if it is not in Inquiry state */
>> +       if (get_state(index) != DISCOV_INQ)
>> +               return;
>> +
>>        for (i = 0; i < num; i++) {
>>                inquiry_info *info = ptr;
>>                uint32_t class = info->dev_class[0] |
>> @@ -3060,39 +2997,20 @@ static int hciops_start_inquiry(int index, uint8_t length, gboolean periodic)
>>  {
>>        struct dev_info *dev = &devs[index];
>>        uint8_t lap[3] = { 0x33, 0x8b, 0x9e };
>> -       int err;
>> +       inquiry_cp inq_cp;
>>
>>        DBG("hci%d length %u periodic %d", index, length, periodic);
>>
>> -       if (periodic) {
>> -               periodic_inquiry_cp cp;
>> -
>> -               memset(&cp, 0, sizeof(cp));
>> -               memcpy(&cp.lap, lap, 3);
>> -               cp.max_period = htobs(24);
>> -               cp.min_period = htobs(16);
>> -               cp.length  = length;
>> -               cp.num_rsp = 0x00;
>> -
>> -               err = hci_send_cmd(dev->sk, OGF_LINK_CTL,
>> -                                               OCF_PERIODIC_INQUIRY,
>> -                                               PERIODIC_INQUIRY_CP_SIZE, &cp);
>> -       } else {
>> -               inquiry_cp inq_cp;
>> -
>> -               memset(&inq_cp, 0, sizeof(inq_cp));
>> -               memcpy(&inq_cp.lap, lap, 3);
>> -               inq_cp.length = length;
>> -               inq_cp.num_rsp = 0x00;
>> +       memset(&inq_cp, 0, sizeof(inq_cp));
>> +       memcpy(&inq_cp.lap, lap, 3);
>> +       inq_cp.length = length;
>> +       inq_cp.num_rsp = 0x00;
>>
>> -               err = hci_send_cmd(dev->sk, OGF_LINK_CTL,
>> -                                       OCF_INQUIRY, INQUIRY_CP_SIZE, &inq_cp);
>> -       }
>> -
>> -       if (err < 0)
>> -               err = -errno;
>> +       if (hci_send_cmd(dev->sk, OGF_LINK_CTL,
>> +                       OCF_INQUIRY, INQUIRY_CP_SIZE, &inq_cp) < 0)
>> +               return -errno;
>>
>> -       return err;
>> +       return 0;
>>  }
>>
>>  static int le_set_scan_enable(int index, uint8_t enable)
>> diff --git a/src/event.c b/src/event.c
>> index 7feec1f..b04220a 100644
>> --- a/src/event.c
>> +++ b/src/event.c
>> @@ -435,7 +435,7 @@ void btd_event_device_found(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
>>        char local_addr[18], peer_addr[18], *alias, *name;
>>        name_status_t name_status;
>>        struct eir_data eir_data;
>> -       int state, err;
>> +       int err;
>>        dbus_bool_t legacy;
>>        unsigned char features[8];
>>        const char *dev_name;
>> @@ -455,17 +455,6 @@ void btd_event_device_found(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
>>        if (data)
>>                write_remote_eir(local, peer, data);
>>
>> -       /*
>> -        * Workaround to identify periodic inquiry: inquiry complete event is
>> -        * sent after each window, however there isn't an event to indicate the
>> -        * beginning of a new periodic inquiry window.
>> -        */
>> -       state = adapter_get_state(adapter);
>> -       if (!(state & (STATE_STDINQ | STATE_LE_SCAN | STATE_PINQ))) {
>> -               state |= STATE_PINQ;
>> -               adapter_set_state(adapter, state);
>> -       }
>> -
>>        /* the inquiry result can be triggered by NON D-Bus client */
>>        if (adapter_get_discover_type(adapter) & DISC_RESOLVNAME &&
>>                                adapter_has_discov_sessions(adapter))
>> --
>> 1.7.1
>>
>
> What exactly is reason to remove periodic inquiry? I see the point for
> dual mode devices, but for regular BD/EDR its very useful since the
> controller takes care of the scheduling and we don't have to send
> another command each round. Also we need to be able to detect if pinq
> was activate by the user somehow, since it may cause some bugs which
> will be very to debug since you are removing the state tracking.
>

The reason is, until now, we're not able to detect correctly when the controller
has started an periodic inquiry. What we had before was a workaround in inquiry
result event handler to set the adapter state to PINQ. This workaround is not
correct because the adapter may be carrying out a discovery procedure and its
state is IDLE. This situation occurs when there is no device in range (no
inquiry result event is generated, consequently, adapter's state will be not set
to PINQ).

So, due to that issue and the fact using periodic inquiry is not mandatory to
implement the discovery procedure described in Core spec, we decided to
remove the periodic inquiry support by now.

Additionally, as I said in the patch description, periodic inquiry commands sent
by the user somehow (and the events it generates) will not change the adapter
state since they are ignored by hciops.

> --
> Luiz Augusto von Dentz
> Computer Engineer
>

--
Andre Guedes.

[RFC 06/16] Change DiscoverSchedulerInterval default value

[Andre Guedes]

This patch changes the default value of "DiscoverSchedulerInterval"
from 0 to 30 seconds.

Since Periodic Inquiry is not supported anymore, the option
"DiscoverSchedulerInterval" in main.conf must be updated. As
a first attempt, it was chosen a 30 seconds interval.
---
 src/main.conf |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/main.conf b/src/main.conf
index c03f135..8cd132f 100644
--- a/src/main.conf
+++ b/src/main.conf
@@ -27,8 +27,8 @@ PairableTimeout = 0
 PageTimeout = 8192
 
 # Discover scheduler interval used in Adapter.DiscoverDevices
-# The value is in seconds. Defaults is 0 to use controller scheduler.
-DiscoverSchedulerInterval = 0
+# The value is in seconds. Defaults is 30.
+DiscoverSchedulerInterval = 30
 
 # What value should be assumed for the adapter Powered property when
 # SetProperty(Powered, ...) hasn't been called yet. Defaults to true
-- 
1.7.1

Re: [RFC 06/16] Change DiscoverSchedulerInterval default value

[Luiz Augusto von Dentz]

Hi Andre,

On Sat, Apr 30, 2011 at 3:27 AM, Andre Guedes
<andre.guedes@openbossa.org> wrote:
> This patch changes the default value of "DiscoverSchedulerInterval"
> from 0 to 30 seconds.
>
> Since Periodic Inquiry is not supported anymore, the option
> "DiscoverSchedulerInterval" in main.conf must be updated. As
> a first attempt, it was chosen a 30 seconds interval.
> ---
>  src/main.conf |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/main.conf b/src/main.conf
> index c03f135..8cd132f 100644
> --- a/src/main.conf
> +++ b/src/main.conf
> @@ -27,8 +27,8 @@ PairableTimeout = 0
>  PageTimeout = 8192
>
>  # Discover scheduler interval used in Adapter.DiscoverDevices
> -# The value is in seconds. Defaults is 0 to use controller scheduler.
> -DiscoverSchedulerInterval = 0
> +# The value is in seconds. Defaults is 30.
> +DiscoverSchedulerInterval = 30
>
>  # What value should be assumed for the adapter Powered property when
>  # SetProperty(Powered, ...) hasn't been called yet. Defaults to true

Im not sure how you came up with 30 sec, IMO it is too long and
doesn't specify how long we inquiry/scan/resolve names.

-- 
Luiz Augusto von Dentz
Computer Engineer

Re: [RFC 06/16] Change DiscoverSchedulerInterval default value

[Andre Guedes]

Hi Luiz,

On Mon, May 2, 2011 at 4:48 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Andre,
>
> On Sat, Apr 30, 2011 at 3:27 AM, Andre Guedes
> <andre.guedes@openbossa.org> wrote:
>> This patch changes the default value of "DiscoverSchedulerInterval"
>> from 0 to 30 seconds.
>>
>> Since Periodic Inquiry is not supported anymore, the option
>> "DiscoverSchedulerInterval" in main.conf must be updated. As
>> a first attempt, it was chosen a 30 seconds interval.
>> ---
>>  src/main.conf |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/main.conf b/src/main.conf
>> index c03f135..8cd132f 100644
>> --- a/src/main.conf
>> +++ b/src/main.conf
>> @@ -27,8 +27,8 @@ PairableTimeout = 0
>>  PageTimeout = 8192
>>
>>  # Discover scheduler interval used in Adapter.DiscoverDevices
>> -# The value is in seconds. Defaults is 0 to use controller scheduler.
>> -DiscoverSchedulerInterval = 0
>> +# The value is in seconds. Defaults is 30.
>> +DiscoverSchedulerInterval = 30
>>
>>  # What value should be assumed for the adapter Powered property when
>>  # SetProperty(Powered, ...) hasn't been called yet. Defaults to true
>
> Im not sure how you came up with 30 sec, IMO it is too long and
> doesn't specify how long we inquiry/scan/resolve names.
>

30 sec was a reasonable time we firstly thought. But as I said, it is
just a first
attempt. How long you think it is a good interval value?

Yes. It doesn't specify how long the whole discovery process takes. It specifies
how long the adapter is gonna wait in IDLE state before trigger the a new
discovery procedure. We changed this behavior because it would become very
tricky to predict how long the name resolving is gonna take since it depends on
the number of devices found during discovery.

I think this is not clear in the patch description, so I rewrite it. Thanks.

> --
> Luiz Augusto von Dentz
> Computer Engineer
>

--
Andre Guedes

[RFC 07/16] Add 'timeout' param to start_scanning callback

[Andre Guedes]

This patch adds a timeout parameter to start_scanning callback in
btd_adatper. The parameter should be used to stop scanning  after
'timeout' milliseconds.
---
 plugins/hciops.c  |   38 ++++++++++++++++++++++++++++++++++++--
 plugins/mgmtops.c |    2 +-
 src/adapter.c     |    2 +-
 src/adapter.h     |    2 +-
 4 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/plugins/hciops.c b/plugins/hciops.c
index 81a0f29..61ae404 100644
--- a/plugins/hciops.c
+++ b/plugins/hciops.c
@@ -139,6 +139,8 @@ static struct dev_info {
 	GSList *uuids;
 
 	GSList *connections;
+
+	guint stop_scan_id;
 } *devs = NULL;
 
 static inline int get_state(int index)
@@ -2303,6 +2305,9 @@ static void stop_hci_dev(int index)
 	if (dev->watch_id > 0)
 		g_source_remove(dev->watch_id);
 
+	if (dev->stop_scan_id > 0)
+		g_source_remove(dev->stop_scan_id);
+
 	if (dev->io != NULL)
 		g_io_channel_unref(dev->io);
 
@@ -3031,10 +3036,25 @@ static int le_set_scan_enable(int index, uint8_t enable)
 	return 0;
 }
 
-static int hciops_start_scanning(int index)
+static gboolean stop_le_scan_cb(gpointer user_data)
+{
+	struct dev_info *dev = user_data;
+	int err;
+
+	err = le_set_scan_enable(dev->id, 0);
+	if (err < 0)
+		return TRUE;
+
+	dev->stop_scan_id = 0;
+
+	return FALSE;
+}
+
+static int hciops_start_scanning(int index, int timeout)
 {
 	struct dev_info *dev = &devs[index];
 	le_set_scan_parameters_cp cp;
+	int err;
 
 	DBG("hci%d", index);
 
@@ -3051,13 +3071,27 @@ static int hciops_start_scanning(int index)
 				LE_SET_SCAN_PARAMETERS_CP_SIZE, &cp) < 0)
 		return -errno;
 
-	return le_set_scan_enable(index, 1);
+	err = le_set_scan_enable(index, 1);
+	if (err < 0)
+		return err;
+
+	/* Schedule a le scan disable in 'timeout' milliseconds */
+	dev->stop_scan_id = g_timeout_add(timeout, stop_le_scan_cb, dev);
+
+	return 0;
 }
 
 static int hciops_stop_scanning(int index)
 {
+	struct dev_info *dev = &devs[index];
+
 	DBG("hci%d", index);
 
+	if (dev->stop_scan_id > 0) {
+		g_source_remove(dev->stop_scan_id);
+		dev->stop_scan_id = 0;
+	}
+
 	return le_set_scan_enable(index, 0);
 }
 
diff --git a/plugins/mgmtops.c b/plugins/mgmtops.c
index ac180f9..9fa195d 100644
--- a/plugins/mgmtops.c
+++ b/plugins/mgmtops.c
@@ -1632,7 +1632,7 @@ static int mgmt_stop_inquiry(int index)
 	return 0;
 }
 
-static int mgmt_start_scanning(int index)
+static int mgmt_start_scanning(int index, int timeout)
 {
 	DBG("index %d", index);
 	return -ENOSYS;
diff --git a/src/adapter.c b/src/adapter.c
index 1dfa9a6..6c29992 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2807,7 +2807,7 @@ void adapter_set_state(struct btd_adapter *adapter, int state)
 		 */
 		if (adapter->disc_sessions && (type & DISC_INTERLEAVE) &&
 						(previous & STATE_STDINQ)) {
-			adapter_ops->start_scanning(adapter->dev_id);
+			adapter_ops->start_scanning(adapter->dev_id, 0);
 			return;
 		}
 		/* BR/EDR only: inquiry finished */
diff --git a/src/adapter.h b/src/adapter.h
index 1a5c687..bb1abe8 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -191,7 +191,7 @@ struct btd_adapter_ops {
 	int (*stop_discovery) (int index);
 	int (*start_inquiry) (int index, uint8_t length, gboolean periodic);
 	int (*stop_inquiry) (int index);
-	int (*start_scanning) (int index);
+	int (*start_scanning) (int index, int timeout);
 	int (*stop_scanning) (int index);
 
 	int (*resolve_name) (int index, bdaddr_t *bdaddr);
-- 
1.7.1

[RFC 08/16] Refactoring adapter_set_state()

[Andre Guedes]

This patch implements a new state machine for struct btd_adapter.

The adapter_set_state() function was completely rewritten since its
logic does not apply anymore. The whole logic of interleaved discovery
procedure before implemented in adapter_set_state() should be
implemented at hciops and mgmtops layers.

At the adapter layer, it is not important to track what is the current
state (inquiring or scanning) during the discovery session. All the
adapter layer cares about is if it is performing the discovery or not.
Therefore, the adapter states STATE_STDINQ, STATE_PINQ and
STATE_LE_SCAN were replaced by a new state called STATE_DISCOV.

Additionally, because there is no point in implementing states
as a bitmask, all adapter states were implemented using integers
instead of a bitmask.
---
 plugins/mgmtops.c |   11 ++---
 src/adapter.c     |  147 +++++++++++++++++++++++------------------------------
 src/adapter.h     |   10 ++--
 src/event.c       |    5 +--
 4 files changed, 73 insertions(+), 100 deletions(-)

diff --git a/plugins/mgmtops.c b/plugins/mgmtops.c
index 9fa195d..e4eac81 100644
--- a/plugins/mgmtops.c
+++ b/plugins/mgmtops.c
@@ -1374,13 +1374,10 @@ static void mgmt_discovering(int sk, uint16_t index, void *buf, size_t len)
 	if (!adapter)
 		return;
 
-	state = adapter_get_state(adapter);
-
-	if (ev->val) {
-		if (!(state & (STATE_STDINQ | STATE_LE_SCAN | STATE_PINQ)))
-			state |= STATE_PINQ;
-	} else
-		state &= ~(STATE_STDINQ | STATE_PINQ);
+	if (ev->val)
+		state = STATE_DISCOV;
+	else
+		state = STATE_IDLE;
 
 	adapter_set_state(adapter, state);
 }
diff --git a/src/adapter.c b/src/adapter.c
index 6c29992..6a2a8c1 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -59,9 +59,6 @@
 #include "attrib-server.h"
 #include "att.h"
 
-/* Interleaved discovery window: 5.12 sec */
-#define GAP_INTER_DISCOV_WIN		5120
-
 /* Flags Descriptions */
 #define EIR_LIM_DISC                0x01 /* LE Limited Discoverable Mode */
 #define EIR_GEN_DISC                0x02 /* LE General Discoverable Mode */
@@ -266,11 +263,13 @@ static int pending_remote_name_cancel(struct btd_adapter *adapter)
 	if (!dev) /* no pending request */
 		return -ENODATA;
 
-	adapter->state &= ~STATE_RESOLVNAME;
 	err = adapter_ops->cancel_resolve_name(adapter->dev_id, &dev->bdaddr);
 	if (err < 0)
 		error("Remote name cancel failed: %s(%d)",
 						strerror(errno), errno);
+
+	adapter_set_state(adapter, STATE_IDLE);
+
 	return err;
 }
 
@@ -280,7 +279,7 @@ int adapter_resolve_names(struct btd_adapter *adapter)
 	int err;
 
 	/* Do not attempt to resolve more names if on suspended state */
-	if (adapter->state & STATE_SUSPENDED)
+	if (adapter->state == STATE_SUSPENDED)
 		return 0;
 
 	memset(&match, 0, sizeof(struct remote_dev_info));
@@ -715,8 +714,8 @@ static void stop_discovery(struct btd_adapter *adapter, gboolean suspend)
 
 	/* Reset if suspended, otherwise remove timer (software scheduler)
 	   or request inquiry to stop */
-	if (adapter->state & STATE_SUSPENDED) {
-		adapter->state &= ~STATE_SUSPENDED;
+	if (adapter->state == STATE_SUSPENDED) {
+		adapter_set_state(adapter, STATE_IDLE);
 		return;
 	}
 
@@ -1204,23 +1203,14 @@ struct btd_device *adapter_get_device(DBusConnection *conn,
 						DEVICE_TYPE_BREDR);
 }
 
-static gboolean stop_scanning(gpointer user_data)
-{
-	struct btd_adapter *adapter = user_data;
-
-	adapter_ops->stop_scanning(adapter->dev_id);
-
-	return FALSE;
-}
-
 static int start_discovery(struct btd_adapter *adapter)
 {
 	/* Do not start if suspended */
-	if (adapter->state & STATE_SUSPENDED)
+	if (adapter->state == STATE_SUSPENDED)
 		return 0;
 
 	/* Postpone discovery if still resolving names */
-	if (adapter->state & STATE_RESOLVNAME)
+	if (adapter->state == STATE_RESOLVNAME)
 		return -EINPROGRESS;
 
 	pending_remote_name_cancel(adapter);
@@ -1368,7 +1358,7 @@ static DBusMessage *get_properties(DBusConnection *conn,
 				DBUS_TYPE_UINT32, &adapter->pairable_timeout);
 
 
-	if (adapter->state & (STATE_PINQ | STATE_STDINQ | STATE_LE_SCAN))
+	if (adapter->state == STATE_DISCOV)
 		value = TRUE;
 	else
 		value = FALSE;
@@ -2761,80 +2751,76 @@ void adapter_get_address(struct btd_adapter *adapter, bdaddr_t *bdaddr)
 	bacpy(bdaddr, &adapter->bdaddr);
 }
 
+static inline void suspend_discovery(struct btd_adapter *adapter)
+{
+	if (adapter->state != STATE_SUSPENDED)
+		return;
+
+	if (adapter->oor_devices) {
+		g_slist_free(adapter->oor_devices);
+		adapter->oor_devices = NULL;
+	}
+
+	if (adapter->scheduler_id) {
+		g_source_remove(adapter->scheduler_id);
+		adapter->scheduler_id = 0;
+	}
+
+	adapter_ops->stop_discovery(adapter->dev_id);
+}
+
+static inline void resolve_names(struct btd_adapter *adapter)
+{
+	int err;
+
+	if (adapter->state != STATE_RESOLVNAME)
+		return;
+
+	err = adapter_resolve_names(adapter);
+	if (err < 0)
+		adapter_set_state(adapter, STATE_IDLE);
+}
+
 void adapter_set_state(struct btd_adapter *adapter, int state)
 {
 	const char *path = adapter->path;
 	gboolean discov_active;
-	int previous, type;
 
 	if (adapter->state == state)
 		return;
 
-	previous = adapter->state;
 	adapter->state = state;
 
-	type = adapter_get_discover_type(adapter);
+	DBG("hci%d: new state %d", adapter->dev_id, adapter->state);
 
-	switch (state) {
-	case STATE_STDINQ:
-	case STATE_PINQ:
-		discov_active = TRUE;
+	switch (adapter->state) {
+	case STATE_IDLE:
+		update_oor_devices(adapter);
 
-		/* Started a new session while resolving names ? */
-		if (previous & STATE_RESOLVNAME)
-			return;
+		discov_active = FALSE;
+		emit_property_changed(connection, path,
+					ADAPTER_INTERFACE, "Discovering",
+					DBUS_TYPE_BOOLEAN, &discov_active);
+
+		if (adapter_has_discov_sessions(adapter)) {
+			adapter->scheduler_id = g_timeout_add_seconds(
+						main_opts.discov_interval,
+						discovery_cb, adapter);
+		}
 		break;
-	case STATE_LE_SCAN:
+	case STATE_DISCOV:
 		discov_active = TRUE;
-
-		if (!adapter->disc_sessions)
-			break;
-
-		/* Stop scanning after TGAP(100)/2 */
-		adapter->stop_discov_id = g_timeout_add(GAP_INTER_DISCOV_WIN,
-							stop_scanning,
-							adapter);
-
-		/* For dual mode: don't send "Discovering = TRUE" (twice) */
-		if (bredr_capable(adapter) == TRUE)
-			return;
-
+		emit_property_changed(connection, path,
+					ADAPTER_INTERFACE, "Discovering",
+					DBUS_TYPE_BOOLEAN, &discov_active);
 		break;
-	case STATE_IDLE:
-		/*
-		 * Interleave: from inquiry to scanning. Interleave is not
-		 * applicable to requests triggered by external applications.
-		 */
-		if (adapter->disc_sessions && (type & DISC_INTERLEAVE) &&
-						(previous & STATE_STDINQ)) {
-			adapter_ops->start_scanning(adapter->dev_id, 0);
-			return;
-		}
-		/* BR/EDR only: inquiry finished */
-		discov_active = FALSE;
+	case STATE_RESOLVNAME:
+		resolve_names(adapter);
 		break;
-	default:
-		discov_active = FALSE;
+	case STATE_SUSPENDED:
+		suspend_discovery(adapter);
 		break;
 	}
-
-	if (discov_active == FALSE) {
-		if (type & DISC_RESOLVNAME) {
-			if (adapter_resolve_names(adapter) == 0) {
-				adapter->state |= STATE_RESOLVNAME;
-				return;
-			}
-		}
-
-		update_oor_devices(adapter);
-	} else if (adapter->disc_sessions && main_opts.discov_interval)
-			adapter->scheduler_id = g_timeout_add_seconds(
-						main_opts.discov_interval,
-						discovery_cb, adapter);
-
-	emit_property_changed(connection, path,
-				ADAPTER_INTERFACE, "Discovering",
-				DBUS_TYPE_BOOLEAN, &discov_active);
 }
 
 int adapter_get_state(struct btd_adapter *adapter)
@@ -3274,24 +3260,19 @@ gboolean adapter_has_discov_sessions(struct btd_adapter *adapter)
 void adapter_suspend_discovery(struct btd_adapter *adapter)
 {
 	if (adapter->disc_sessions == NULL ||
-			adapter->state & STATE_SUSPENDED)
+			adapter->state == STATE_SUSPENDED)
 		return;
 
 	DBG("Suspending discovery");
 
-	stop_discovery(adapter, TRUE);
-	adapter->state |= STATE_SUSPENDED;
+	adapter_set_state(adapter, STATE_SUSPENDED);
 }
 
 void adapter_resume_discovery(struct btd_adapter *adapter)
 {
-	if (adapter->disc_sessions == NULL)
-		return;
-
 	DBG("Resuming discovery");
 
-	adapter->state &= ~STATE_SUSPENDED;
-	start_discovery(adapter);
+	adapter_set_state(adapter, STATE_IDLE);
 }
 
 int btd_register_adapter_driver(struct btd_adapter_driver *driver)
diff --git a/src/adapter.h b/src/adapter.h
index bb1abe8..a408a92 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -37,12 +37,10 @@
 #define MODE_UNKNOWN		0xff
 
 /* Discover states */
-#define STATE_IDLE		0x00
-#define STATE_LE_SCAN		0x01
-#define STATE_STDINQ		0x02
-#define STATE_PINQ		0x04
-#define STATE_RESOLVNAME	0x08
-#define STATE_SUSPENDED		0x10
+#define STATE_IDLE		0
+#define STATE_DISCOV		1
+#define STATE_RESOLVNAME	2
+#define STATE_SUSPENDED		3
 
 /* Supported host/controller discover type */
 #define DISC_LE			0x01
diff --git a/src/event.c b/src/event.c
index b04220a..bf6f4e3 100644
--- a/src/event.c
+++ b/src/event.c
@@ -559,7 +559,6 @@ void btd_event_remote_name(bdaddr_t *local, bdaddr_t *peer, uint8_t status,
 {
 	struct btd_adapter *adapter;
 	char srcaddr[18], dstaddr[18];
-	int state;
 	struct btd_device *device;
 	struct remote_dev_info match, *dev_info;
 
@@ -604,9 +603,7 @@ proceed:
 	if (adapter_resolve_names(adapter) == 0)
 		return;
 
-	state = adapter_get_state(adapter);
-	state &= ~STATE_RESOLVNAME;
-	adapter_set_state(adapter, state);
+	adapter_set_state(adapter, STATE_IDLE);
 }
 
 int btd_event_link_key_notify(bdaddr_t *local, bdaddr_t *peer,
-- 
1.7.1

[RFC 09/16] Remove 'suspend' param from stop_discovery()

[Andre Guedes]

This patch removes 'suspend' parameter from stop_discovery() in adapter.c.

This parameter is useless since new a function (suspend_discovery) was
created to suspend discovery sessions.
---
 src/adapter.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index 6a2a8c1..0279494 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -700,12 +700,11 @@ static GSList *remove_bredr(GSList *all)
 	return le;
 }
 
-static void stop_discovery(struct btd_adapter *adapter, gboolean suspend)
+static void stop_discovery(struct btd_adapter *adapter)
 {
 	pending_remote_name_cancel(adapter);
 
-	if (suspend == FALSE)
-		adapter->found_devices = remove_bredr(adapter->found_devices);
+	adapter->found_devices = remove_bredr(adapter->found_devices);
 
 	if (adapter->oor_devices) {
 		g_slist_free(adapter->oor_devices);
@@ -762,7 +761,7 @@ static void session_remove(struct session_req *req)
 
 		DBG("Stopping discovery");
 
-		stop_discovery(adapter, FALSE);
+		stop_discovery(adapter);
 	}
 }
 
@@ -2537,7 +2536,7 @@ int btd_adapter_stop(struct btd_adapter *adapter)
 	/* check pending requests */
 	reply_pending_requests(adapter);
 
-	stop_discovery(adapter, FALSE);
+	stop_discovery(adapter);
 
 	if (adapter->disc_sessions) {
 		g_slist_foreach(adapter->disc_sessions, (GFunc) session_free,
-- 
1.7.1

Re: [RFC 09/16] Remove 'suspend' param from stop_discovery()

[Luiz Augusto von Dentz]

Hi Andre,

On Sat, Apr 30, 2011 at 3:27 AM, Andre Guedes
<andre.guedes@openbossa.org> wrote:
> This patch removes 'suspend' parameter from stop_discovery() in adapter.c.
>
> This parameter is useless since new a function (suspend_discovery) was
> created to suspend discovery sessions.
> ---
>  src/adapter.c |    9 ++++-----
>  1 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index 6a2a8c1..0279494 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -700,12 +700,11 @@ static GSList *remove_bredr(GSList *all)
>        return le;
>  }
>
> -static void stop_discovery(struct btd_adapter *adapter, gboolean suspend)
> +static void stop_discovery(struct btd_adapter *adapter)
>  {
>        pending_remote_name_cancel(adapter);
>
> -       if (suspend == FALSE)
> -               adapter->found_devices = remove_bredr(adapter->found_devices);
> +       adapter->found_devices = remove_bredr(adapter->found_devices);
>
>        if (adapter->oor_devices) {
>                g_slist_free(adapter->oor_devices);
> @@ -762,7 +761,7 @@ static void session_remove(struct session_req *req)
>
>                DBG("Stopping discovery");
>
> -               stop_discovery(adapter, FALSE);
> +               stop_discovery(adapter);
>        }
>  }
>
> @@ -2537,7 +2536,7 @@ int btd_adapter_stop(struct btd_adapter *adapter)
>        /* check pending requests */
>        reply_pending_requests(adapter);
>
> -       stop_discovery(adapter, FALSE);
> +       stop_discovery(adapter);
>
>        if (adapter->disc_sessions) {
>                g_slist_foreach(adapter->disc_sessions, (GFunc) session_free,
> --

What happened with stop_discovery called within suspend_discovery? We
need to avoid cleaning up the list of found devices otherwise we won't
be able to send DeviceDisappeared properly when discovery got resume.

-- 
Luiz Augusto von Dentz
Computer Engineer

Re: [RFC 09/16] Remove 'suspend' param from stop_discovery()

[Andre Guedes]

Hi Luiz,

On Mon, May 2, 2011 at 5:42 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Andre,
>
> On Sat, Apr 30, 2011 at 3:27 AM, Andre Guedes
> <andre.guedes@openbossa.org> wrote:
>> This patch removes 'suspend' parameter from stop_discovery() in adapter.c.
>>
>> This parameter is useless since new a function (suspend_discovery) was
>> created to suspend discovery sessions.
>> ---
>>  src/adapter.c |    9 ++++-----
>>  1 files changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/adapter.c b/src/adapter.c
>> index 6a2a8c1..0279494 100644
>> --- a/src/adapter.c
>> +++ b/src/adapter.c
>> @@ -700,12 +700,11 @@ static GSList *remove_bredr(GSList *all)
>>        return le;
>>  }
>>
>> -static void stop_discovery(struct btd_adapter *adapter, gboolean suspend)
>> +static void stop_discovery(struct btd_adapter *adapter)
>>  {
>>        pending_remote_name_cancel(adapter);
>>
>> -       if (suspend == FALSE)
>> -               adapter->found_devices = remove_bredr(adapter->found_devices);
>> +       adapter->found_devices = remove_bredr(adapter->found_devices);
>>
>>        if (adapter->oor_devices) {
>>                g_slist_free(adapter->oor_devices);
>> @@ -762,7 +761,7 @@ static void session_remove(struct session_req *req)
>>
>>                DBG("Stopping discovery");
>>
>> -               stop_discovery(adapter, FALSE);
>> +               stop_discovery(adapter);
>>        }
>>  }
>>
>> @@ -2537,7 +2536,7 @@ int btd_adapter_stop(struct btd_adapter *adapter)
>>        /* check pending requests */
>>        reply_pending_requests(adapter);
>>
>> -       stop_discovery(adapter, FALSE);
>> +       stop_discovery(adapter);
>>
>>        if (adapter->disc_sessions) {
>>                g_slist_foreach(adapter->disc_sessions, (GFunc) session_free,
>> --
>
> What happened with stop_discovery called within suspend_discovery? We
> need to avoid cleaning up the list of found devices otherwise we won't
> be able to send DeviceDisappeared properly when discovery got resume.
>

At patch 08/16 I create a new function to suspend the discovery procedure
(see suspend_discovery() in adapter.c). It was done because using the
same function to perform stop or suspend discovery started to get a little
bit confusing.

> --
> Luiz Augusto von Dentz
> Computer Engineer
>

--
Andre Guedes

[RFC 10/16] Add extfeatures to struct dev_info

[Andre Guedes]

The extfeatures field will be used by hciops layer to know if the
adapter is LE capable.
---
 plugins/hciops.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/plugins/hciops.c b/plugins/hciops.c
index 61ae404..965e9de 100644
--- a/plugins/hciops.c
+++ b/plugins/hciops.c
@@ -102,6 +102,7 @@ static struct dev_info {
 	char name[249];
 	uint8_t eir[HCI_MAX_EIR_LENGTH];
 	uint8_t features[8];
+	uint8_t extfeatures[8];
 	uint8_t ssp_mode;
 
 	int8_t tx_power;
@@ -1648,6 +1649,7 @@ static void read_local_ext_features_complete(int index,
 				const read_local_ext_features_rp *rp)
 {
 	struct btd_adapter *adapter;
+	struct dev_info *dev = &devs[index];
 
 	DBG("hci%d status %u", index, rp->status);
 
@@ -1664,6 +1666,7 @@ static void read_local_ext_features_complete(int index,
 	if (rp->page_num != 1)
 		return;
 
+	memcpy(dev->extfeatures, rp->features, sizeof(dev->extfeatures));
 	btd_adapter_update_local_ext_features(adapter, rp->features);
 }
 
-- 
1.7.1

[RFC 11/16] Implement start_discovery hciops callback

[Andre Guedes]

---
 plugins/hciops.c |   98 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 94 insertions(+), 4 deletions(-)

diff --git a/plugins/hciops.c b/plugins/hciops.c
index 965e9de..23e8915 100644
--- a/plugins/hciops.c
+++ b/plugins/hciops.c
@@ -56,11 +56,26 @@
 #define DISCOV_INQ 1
 #define DISCOV_SCAN 2
 
+#define TIMEOUT_BR_LE_SCAN 5120 /* TGAP(100)/2 */
+#define TIMEOUT_LE_SCAN 10240 /* TGAP(gen_disc_scan_min) */
+
+#define LENGTH_BR_INQ 0x08
+#define LENGTH_BR_LE_INQ 0x04
+
+static int hciops_start_scanning(int index, int timeout);
+
 static int child_pipe[2] = { -1, -1 };
 
 static guint child_io_id = 0;
 static guint ctl_io_id = 0;
 
+enum adapter_type {
+	BR_EDR,
+	LE_ONLY,
+	BR_EDR_LE,
+	UNKNOWN,
+};
+
 /* Commands sent by kernel on starting an adapter */
 enum {
 	PENDING_BDADDR,
@@ -151,27 +166,74 @@ static inline int get_state(int index)
 	return dev->discov_state;
 }
 
+static inline gboolean is_resolvname_enabled(void)
+{
+	return main_opts.name_resolv ? TRUE : FALSE;
+}
+
 static void set_state(int index, int state)
 {
+	struct btd_adapter *adapter;
 	struct dev_info *dev = &devs[index];
 
 	if (dev->discov_state == state)
 		return;
 
+	adapter = manager_find_adapter_by_id(index);
+	if (!adapter) {
+		error("No matching adapter found");
+		return;
+	}
+
 	dev->discov_state = state;
 
 	DBG("hci%d: new state %d", index, dev->discov_state);
 
 	switch (dev->discov_state) {
 	case DISCOV_HALTED:
+		if (adapter_get_state(adapter) == STATE_SUSPENDED)
+			return;
+
+		if (is_resolvname_enabled() &&
+					adapter_has_discov_sessions(adapter))
+			adapter_set_state(adapter, STATE_RESOLVNAME);
+		else
+			adapter_set_state(adapter, STATE_IDLE);
 		break;
 	case DISCOV_INQ:
-		break;
 	case DISCOV_SCAN:
+		adapter_set_state(adapter, STATE_DISCOV);
 		break;
 	}
 }
 
+static inline gboolean is_le_capable(int index)
+{
+	struct dev_info *dev = &devs[index];
+
+	return (main_opts.le && dev->features[4] & LMP_LE &&
+			dev->extfeatures[0] & LMP_HOST_LE) ? TRUE : FALSE;
+}
+
+static inline gboolean is_bredr_capable(int index)
+{
+	struct dev_info *dev = &devs[index];
+
+	return (dev->features[4] & LMP_NO_BREDR) == 0 ? TRUE : FALSE;
+}
+
+static int get_adapter_type(int index)
+{
+	if (is_le_capable(index) && is_bredr_capable(index))
+		return BR_EDR_LE;
+	else if (is_le_capable(index))
+		return LE_ONLY;
+	else if (is_bredr_capable(index))
+		return BR_EDR;
+
+	return UNKNOWN;
+}
+
 static int ignore_device(struct hci_dev_info *di)
 {
 	return hci_test_bit(HCI_RAW, &di->flags) || di->type >> 4 != HCI_BREDR;
@@ -1831,12 +1893,30 @@ static void read_local_oob_data_complete(int index, uint8_t status,
 
 static inline void inquiry_complete_evt(int index, uint8_t status)
 {
+	int adapter_type;
+	struct btd_adapter *adapter;
+
 	if (status) {
 		error("Inquiry Failed with status 0x%02x", status);
 		return;
 	}
 
-	set_state(index, DISCOV_HALTED);
+	adapter = manager_find_adapter_by_id(index);
+	if (!adapter) {
+		error("No matching adapter found");
+		return;
+	}
+
+	adapter_type = get_adapter_type(index);
+
+	if (adapter_type == BR_EDR_LE &&
+					adapter_has_discov_sessions(adapter)) {
+		int err = hciops_start_scanning(index, TIMEOUT_BR_LE_SCAN);
+		if (err < 0)
+			set_state(index, DISCOV_HALTED);
+	} else {
+		set_state(index, DISCOV_HALTED);
+	}
 }
 
 static inline void cc_inquiry_cancel(int index, uint8_t status)
@@ -3159,8 +3239,18 @@ static int hciops_cancel_resolve_name(int index, bdaddr_t *bdaddr)
 
 static int hciops_start_discovery(int index)
 {
-	DBG("index %d", index);
-	return -ENOSYS;
+	int adapter_type = get_adapter_type(index);
+
+	switch (adapter_type) {
+	case BR_EDR_LE:
+		return hciops_start_inquiry(index, LENGTH_BR_LE_INQ, FALSE);
+	case BR_EDR:
+		return hciops_start_inquiry(index, LENGTH_BR_INQ, FALSE);
+	case LE_ONLY:
+		return hciops_start_scanning(index, TIMEOUT_LE_SCAN);
+	default:
+		return -EINVAL;
+	}
 }
 
 static int hciops_stop_discovery(int index)
-- 
1.7.1

[RFC 12/16] Remove obsolete code.

[Andre Guedes]

adapter_get_discover_type() has become obsolete. This patch removes it and
do the proper adaptation.
---
 plugins/hciops.c |    8 --------
 src/adapter.c    |   38 --------------------------------------
 src/adapter.h    |    3 ---
 src/event.c      |    3 +--
 4 files changed, 1 insertions(+), 51 deletions(-)

diff --git a/plugins/hciops.c b/plugins/hciops.c
index 23e8915..8653ab5 100644
--- a/plugins/hciops.c
+++ b/plugins/hciops.c
@@ -1710,7 +1710,6 @@ static void read_simple_pairing_mode_complete(int index, void *ptr)
 static void read_local_ext_features_complete(int index,
 				const read_local_ext_features_rp *rp)
 {
-	struct btd_adapter *adapter;
 	struct dev_info *dev = &devs[index];
 
 	DBG("hci%d status %u", index, rp->status);
@@ -1718,18 +1717,11 @@ static void read_local_ext_features_complete(int index,
 	if (rp->status)
 		return;
 
-	adapter = manager_find_adapter_by_id(index);
-	if (!adapter) {
-		error("No matching adapter found");
-		return;
-	}
-
 	/* Local Extended feature page number is 1 */
 	if (rp->page_num != 1)
 		return;
 
 	memcpy(dev->extfeatures, rp->features, sizeof(dev->extfeatures));
-	btd_adapter_update_local_ext_features(adapter, rp->features);
 }
 
 static void read_bd_addr_complete(int index, read_bd_addr_rp *rp)
diff --git a/src/adapter.c b/src/adapter.c
index 0279494..5de8775 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -136,7 +136,6 @@ struct btd_adapter {
 	sdp_list_t *services;		/* Services associated to adapter */
 
 	uint8_t  features[8];
-	uint8_t  extfeatures[8];
 
 	gboolean pairable;		/* pairable state */
 	gboolean initialized;
@@ -2336,37 +2335,6 @@ static void update_oor_devices(struct btd_adapter *adapter)
 	adapter->oor_devices =  g_slist_copy(adapter->found_devices);
 }
 
-static gboolean bredr_capable(struct btd_adapter *adapter)
-{
-	return (adapter->features[4] & LMP_NO_BREDR) == 0 ? TRUE : FALSE;
-}
-
-static gboolean le_capable(struct btd_adapter *adapter)
-{
-	return (adapter->features[4] & LMP_LE &&
-			adapter->extfeatures[0] & LMP_HOST_LE) ? TRUE : FALSE;
-}
-
-int adapter_get_discover_type(struct btd_adapter *adapter)
-{
-	gboolean le, bredr;
-	int type;
-
-	le = le_capable(adapter);
-	bredr = bredr_capable(adapter);
-
-	if (main_opts.le && le)
-		type = bredr ? DISC_INTERLEAVE : DISC_LE;
-	else
-		type = main_opts.discov_interval ? DISC_STDINQ :
-							DISC_PINQ;
-
-	if (main_opts.name_resolv)
-		type |= DISC_RESOLVNAME;
-
-	return type;
-}
-
 void btd_adapter_get_mode(struct btd_adapter *adapter, uint8_t *mode,
 					uint8_t *on_mode, gboolean *pairable)
 {
@@ -3640,12 +3608,6 @@ int btd_adapter_passkey_reply(struct btd_adapter *adapter, bdaddr_t *bdaddr,
 	return adapter_ops->passkey_reply(adapter->dev_id, bdaddr, passkey);
 }
 
-void btd_adapter_update_local_ext_features(struct btd_adapter *adapter,
-						const uint8_t *features)
-{
-	memcpy(adapter->extfeatures, features, 8);
-}
-
 int btd_adapter_encrypt_link(struct btd_adapter *adapter, bdaddr_t *bdaddr,
 					bt_hci_result_t cb, gpointer user_data)
 {
diff --git a/src/adapter.h b/src/adapter.h
index a408a92..1db9225 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -264,9 +264,6 @@ int btd_adapter_confirm_reply(struct btd_adapter *adapter, bdaddr_t *bdaddr,
 int btd_adapter_passkey_reply(struct btd_adapter *adapter, bdaddr_t *bdaddr,
 							uint32_t passkey);
 
-void btd_adapter_update_local_ext_features(struct btd_adapter *adapter,
-						const uint8_t *features);
-
 int btd_adapter_encrypt_link(struct btd_adapter *adapter, bdaddr_t *bdaddr,
 				bt_hci_result_t cb, gpointer user_data);
 
diff --git a/src/event.c b/src/event.c
index bf6f4e3..b223448 100644
--- a/src/event.c
+++ b/src/event.c
@@ -456,8 +456,7 @@ void btd_event_device_found(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
 		write_remote_eir(local, peer, data);
 
 	/* the inquiry result can be triggered by NON D-Bus client */
-	if (adapter_get_discover_type(adapter) & DISC_RESOLVNAME &&
-				adapter_has_discov_sessions(adapter))
+	if (main_opts.name_resolv && adapter_has_discov_sessions(adapter))
 		name_status = NAME_REQUIRED;
 	else
 		name_status = NAME_NOT_REQUIRED;
-- 
1.7.1

[RFC 13/16] Implement stop_discovery hciops callback

[Andre Guedes]

---
 plugins/hciops.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/plugins/hciops.c b/plugins/hciops.c
index 8653ab5..c3a28b9 100644
--- a/plugins/hciops.c
+++ b/plugins/hciops.c
@@ -3247,8 +3247,18 @@ static int hciops_start_discovery(int index)
 
 static int hciops_stop_discovery(int index)
 {
+	struct dev_info *dev = &devs[index];
+
 	DBG("index %d", index);
-	return -ENOSYS;
+
+	switch (dev->discov_state) {
+	case DISCOV_INQ:
+		return hciops_stop_inquiry(index);
+	case DISCOV_SCAN:
+		return hciops_stop_scanning(index);
+	default:
+		return -EINVAL;
+	}
 }
 
 static int hciops_fast_connectable(int index, gboolean enable)
-- 
1.7.1

[RFC 14/16] Implement mgmt start and stop discovery

[Andre Guedes]

From: Anderson Briglia <anderson.briglia@openbossa.org>

This patch implements mgmt_start_discovery and mgmt_stop_discovery
callbacks for mgmtops.
---
 plugins/mgmtops.c |   24 ++++++++++++++++++++++--
 1 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/plugins/mgmtops.c b/plugins/mgmtops.c
index e4eac81..a7a7d67 100644
--- a/plugins/mgmtops.c
+++ b/plugins/mgmtops.c
@@ -1587,14 +1587,34 @@ static int mgmt_set_limited_discoverable(int index, gboolean limited)
 
 static int mgmt_start_discovery(int index)
 {
+	struct mgmt_hdr hdr;
+
 	DBG("index %d", index);
-	return -ENOSYS;
+
+	memset(&hdr, 0, sizeof(hdr));
+	hdr.opcode = htobs(MGMT_OP_START_DISCOVERY);
+	hdr.index = htobs(index);
+
+	if (write(mgmt_sock, &hdr, sizeof(hdr)) < 0)
+		return -errno;
+
+	return 0;
 }
 
 static int mgmt_stop_discovery(int index)
 {
+	struct mgmt_hdr hdr;
+
 	DBG("index %d", index);
-	return -ENOSYS;
+
+	memset(&hdr, 0, sizeof(hdr));
+	hdr.opcode = htobs(MGMT_OP_STOP_DISCOVERY);
+	hdr.index = htobs(index);
+
+	if (write(mgmt_sock, &hdr, sizeof(hdr)) < 0)
+		return -errno;
+
+	return 0;
 }
 
 static int mgmt_start_inquiry(int index, uint8_t length, gboolean periodic)
-- 
1.7.1

[RFC 15/16] Remove inquiry and scanning callbacks from btd_adapter_ops

[Andre Guedes]

The discovery procedure is implemented in discovery callbacks
(start_discovery and stop_discovery). We don't need inquiry and
scanning specifics callbacks in btd_adapter_ops anymore.
---
 plugins/hciops.c  |    4 ----
 plugins/mgmtops.c |   48 ------------------------------------------------
 src/adapter.h     |    4 ----
 3 files changed, 0 insertions(+), 56 deletions(-)

diff --git a/plugins/hciops.c b/plugins/hciops.c
index c3a28b9..db3ca84 100644
--- a/plugins/hciops.c
+++ b/plugins/hciops.c
@@ -3812,10 +3812,6 @@ static struct btd_adapter_ops hci_ops = {
 	.set_limited_discoverable = hciops_set_limited_discoverable,
 	.start_discovery = hciops_start_discovery,
 	.stop_discovery = hciops_stop_discovery,
-	.start_inquiry = hciops_start_inquiry,
-	.stop_inquiry = hciops_stop_inquiry,
-	.start_scanning = hciops_start_scanning,
-	.stop_scanning = hciops_stop_scanning,
 	.resolve_name = hciops_resolve_name,
 	.cancel_resolve_name = hciops_cancel_resolve_name,
 	.set_name = hciops_set_name,
diff --git a/plugins/mgmtops.c b/plugins/mgmtops.c
index a7a7d67..9e1cfac 100644
--- a/plugins/mgmtops.c
+++ b/plugins/mgmtops.c
@@ -1617,50 +1617,6 @@ static int mgmt_stop_discovery(int index)
 	return 0;
 }
 
-static int mgmt_start_inquiry(int index, uint8_t length, gboolean periodic)
-{
-	struct mgmt_hdr hdr;
-
-	DBG("index %d length %u periodic %d", index, length, periodic);
-
-	memset(&hdr, 0, sizeof(hdr));
-	hdr.opcode = htobs(MGMT_OP_START_DISCOVERY);
-	hdr.index = htobs(index);
-
-	if (write(mgmt_sock, &hdr, sizeof(hdr)) < 0)
-		return -errno;
-
-	return 0;
-}
-
-static int mgmt_stop_inquiry(int index)
-{
-	struct mgmt_hdr hdr;
-
-	DBG("index %d", index);
-
-	memset(&hdr, 0, sizeof(hdr));
-	hdr.opcode = htobs(MGMT_OP_STOP_DISCOVERY);
-	hdr.index = htobs(index);
-
-	if (write(mgmt_sock, &hdr, sizeof(hdr)) < 0)
-		return -errno;
-
-	return 0;
-}
-
-static int mgmt_start_scanning(int index, int timeout)
-{
-	DBG("index %d", index);
-	return -ENOSYS;
-}
-
-static int mgmt_stop_scanning(int index)
-{
-	DBG("index %d", index);
-	return -ENOSYS;
-}
-
 static int mgmt_resolve_name(int index, bdaddr_t *bdaddr)
 {
 	char addr[18];
@@ -2057,10 +2013,6 @@ static struct btd_adapter_ops mgmt_ops = {
 	.set_limited_discoverable = mgmt_set_limited_discoverable,
 	.start_discovery = mgmt_start_discovery,
 	.stop_discovery = mgmt_stop_discovery,
-	.start_inquiry = mgmt_start_inquiry,
-	.stop_inquiry = mgmt_stop_inquiry,
-	.start_scanning = mgmt_start_scanning,
-	.stop_scanning = mgmt_stop_scanning,
 	.resolve_name = mgmt_resolve_name,
 	.cancel_resolve_name = mgmt_cancel_resolve_name,
 	.set_name = mgmt_set_name,
diff --git a/src/adapter.h b/src/adapter.h
index 1db9225..4e15c17 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -187,10 +187,6 @@ struct btd_adapter_ops {
 	int (*set_limited_discoverable) (int index, gboolean limited);
 	int (*start_discovery) (int index);
 	int (*stop_discovery) (int index);
-	int (*start_inquiry) (int index, uint8_t length, gboolean periodic);
-	int (*stop_inquiry) (int index);
-	int (*start_scanning) (int index, int timeout);
-	int (*stop_scanning) (int index);
 
 	int (*resolve_name) (int index, bdaddr_t *bdaddr);
 	int (*cancel_resolve_name) (int index, bdaddr_t *bdaddr);
-- 
1.7.1

[RFC 16/16] Remove 'periodic' param from hciops_start_inquiry()

[Andre Guedes]

The 'periodic' parameter from hciops_start_inquiry() is useless.
This patch removes it.
---
 plugins/hciops.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/plugins/hciops.c b/plugins/hciops.c
index db3ca84..2c49e35 100644
--- a/plugins/hciops.c
+++ b/plugins/hciops.c
@@ -3073,13 +3073,13 @@ static int hciops_set_dev_class(int index, uint8_t major, uint8_t minor)
 	return err;
 }
 
-static int hciops_start_inquiry(int index, uint8_t length, gboolean periodic)
+static int hciops_start_inquiry(int index, uint8_t length)
 {
 	struct dev_info *dev = &devs[index];
 	uint8_t lap[3] = { 0x33, 0x8b, 0x9e };
 	inquiry_cp inq_cp;
 
-	DBG("hci%d length %u periodic %d", index, length, periodic);
+	DBG("hci%d length %u", index, length);
 
 	memset(&inq_cp, 0, sizeof(inq_cp));
 	memcpy(&inq_cp.lap, lap, 3);
@@ -3235,9 +3235,9 @@ static int hciops_start_discovery(int index)
 
 	switch (adapter_type) {
 	case BR_EDR_LE:
-		return hciops_start_inquiry(index, LENGTH_BR_LE_INQ, FALSE);
+		return hciops_start_inquiry(index, LENGTH_BR_LE_INQ);
 	case BR_EDR:
-		return hciops_start_inquiry(index, LENGTH_BR_INQ, FALSE);
+		return hciops_start_inquiry(index, LENGTH_BR_INQ);
 	case LE_ONLY:
 		return hciops_start_scanning(index, TIMEOUT_LE_SCAN);
 	default:
-- 
1.7.1

Re: [RFC 00/16] Discovery procedure refactoring

[Luiz Augusto von Dentz]

Hi Andre,

On Sat, Apr 30, 2011 at 3:27 AM, Andre Guedes
<andre.guedes@openbossa.org> wrote:
> Hi all,
>
> Today, discovery procedure is supported only in hciops. This refactoring has
> been done to provide easier implementation of discovery procedure in mgmtops.
> Lots of effort would be necessary to implement discovery procedure in mgmtops
> because its logic is spread out adapter layer and hciops layer.
>
> The approach this patchset follows is moving all logic related to discovery
> procedure from adapter layer to hciops layer.
>
> Future work will be:
>        - full support for discovery procedure (BR/EDR, LE-Only, BR/EDR/LE
>          devices) in kernel via management interface (today, only BR/EDR is
>          supported).
>        - name resolving support through mgmt interface (kernel + userspace)
>
> Thanks,
> Guedes.
>
> Anderson Briglia (1):
>  Implement mgmt start and stop discovery
>
> Andre Guedes (15):
>  Add discovery callbacks to btd_adapter_ops
>  Replace inquiry/scanning calls by discovery calls
>  Add 'discov_state' field to struct dev_info
>  Code cleanup event.c
>  Remove Periodic Inquiry support in hciops
>  Change DiscoverSchedulerInterval default value
>  Add 'timeout' param to start_scanning callback
>  Refactoring adapter_set_state()
>  Remove 'suspend' param from stop_discovery()
>  Add extfeatures to struct dev_info
>  Implement start_discovery hciops callback
>  Remove obsolete code.
>  Implement stop_discovery hciops callback
>  Remove inquiry and scanning callbacks from btd_adapter_ops
>  Remove 'periodic' param from hciops_start_inquiry()
>
>  plugins/hciops.c  |  373 ++++++++++++++++++++++++++++++++++++-----------------
>  plugins/mgmtops.c |   35 ++----
>  src/adapter.c     |  217 ++++++++++---------------------
>  src/adapter.h     |   19 +--
>  src/event.c       |   48 +-------
>  src/event.h       |    1 -
>  src/main.conf     |    4 +-
>  7 files changed, 345 insertions(+), 352 deletions(-)

How do we solve the problem with command failing to restart
inquiring/scanning, with pinq we only need to send the command once
and if it fails we can signal the error back to the client, but if we
are sending commands every round they may fail (buggy hardware) and
currently we have no way to notify the client if discovery session
failed after it has been started.

Im afraid because of dual mode adapters we may need to change the
D-Bus API in the future and have the client to specify somehow on
StartDiscovery if LE only, BR/EDR only or both like connman does with
RequestScan, otherwise it may take too long (each cycle taking 20-30
seconds) to discover devices because it wasting time scanning or
resolving names in the wrong technology.

-- 
Luiz Augusto von Dentz
Computer Engineer

Re: [RFC 00/16] Discovery procedure refactoring

[Anderson Lizardo]

Hi Luiz,

On Mon, May 2, 2011 at 4:39 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Im afraid because of dual mode adapters we may need to change the
> D-Bus API in the future and have the client to specify somehow on
> StartDiscovery if LE only, BR/EDR only or both like connman does with
> RequestScan, otherwise it may take too long (each cycle taking 20-30
> seconds) to discover devices because it wasting time scanning or
> resolving names in the wrong technology.

On dual mode devices, there is a specific procedure (defined by the
Core spec) for discovery, called "interleaved discovery" where BR/EDR
and LE discovery procedures are alternated at specific times. It
cannot just do BR/EDR or LE, unless we want to "fake" a single mode
adapter.

In summary, if the adapter is to work as a compliant dual mode device,
it shall use the interleaved discovery procedure. BR/EDR only or LE
only discovery would only be acceptable for single mode adapters. We
could as well have configuration options to "simulate" single mode on
dual mode adapters.

HTH,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

Re: [RFC 00/16] Discovery procedure refactoring

[Andre Guedes]

Hi Luiz,

On Mon, May 2, 2011 at 5:39 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Andre,
>
> On Sat, Apr 30, 2011 at 3:27 AM, Andre Guedes
> <andre.guedes@openbossa.org> wrote:
>> Hi all,
>>
>> Today, discovery procedure is supported only in hciops. This refactoring has
>> been done to provide easier implementation of discovery procedure in mgmtops.
>> Lots of effort would be necessary to implement discovery procedure in mgmtops
>> because its logic is spread out adapter layer and hciops layer.
>>
>> The approach this patchset follows is moving all logic related to discovery
>> procedure from adapter layer to hciops layer.
>>
>> Future work will be:
>>        - full support for discovery procedure (BR/EDR, LE-Only, BR/EDR/LE
>>          devices) in kernel via management interface (today, only BR/EDR is
>>          supported).
>>        - name resolving support through mgmt interface (kernel + userspace)
>>
>> Thanks,
>> Guedes.
>>
>> Anderson Briglia (1):
>>  Implement mgmt start and stop discovery
>>
>> Andre Guedes (15):
>>  Add discovery callbacks to btd_adapter_ops
>>  Replace inquiry/scanning calls by discovery calls
>>  Add 'discov_state' field to struct dev_info
>>  Code cleanup event.c
>>  Remove Periodic Inquiry support in hciops
>>  Change DiscoverSchedulerInterval default value
>>  Add 'timeout' param to start_scanning callback
>>  Refactoring adapter_set_state()
>>  Remove 'suspend' param from stop_discovery()
>>  Add extfeatures to struct dev_info
>>  Implement start_discovery hciops callback
>>  Remove obsolete code.
>>  Implement stop_discovery hciops callback
>>  Remove inquiry and scanning callbacks from btd_adapter_ops
>>  Remove 'periodic' param from hciops_start_inquiry()
>>
>>  plugins/hciops.c  |  373 ++++++++++++++++++++++++++++++++++++-----------------
>>  plugins/mgmtops.c |   35 ++----
>>  src/adapter.c     |  217 ++++++++++---------------------
>>  src/adapter.h     |   19 +--
>>  src/event.c       |   48 +-------
>>  src/event.h       |    1 -
>>  src/main.conf     |    4 +-
>>  7 files changed, 345 insertions(+), 352 deletions(-)
>
> How do we solve the problem with command failing to restart
> inquiring/scanning, with pinq we only need to send the command once
> and if it fails we can signal the error back to the client, but if we
> are sending commands every round they may fail (buggy hardware) and
> currently we have no way to notify the client if discovery session
> failed after it has been started.
>

I think this problem already exists if we change "DiscoverSchedulerInterval"
option to a value different from zero. So, there is no regression. The problem
you reported is masked by using PINQ as a default configuration in main.conf.

But anyway, you're right, we need to come up with some mechanism to
signal the client an error occurred during the discovery procedure.

> Im afraid because of dual mode adapters we may need to change the
> D-Bus API in the future and have the client to specify somehow on
> StartDiscovery if LE only, BR/EDR only or both like connman does with
> RequestScan, otherwise it may take too long (each cycle taking 20-30
> seconds) to discover devices because it wasting time scanning or
> resolving names in the wrong technology.
>
> --
> Luiz Augusto von Dentz
> Computer Engineer
>

The purpose of this patchset is refactoring discovery procedure related code.
It doesn't aim to fix known bugs.

--
Andre Guedes.

Re: [RFC 00/16] Discovery procedure refactoring

[Johan Hedberg]

Hi André,

On Fri, Apr 29, 2011, Andre Guedes wrote:
> Hi all,
> 
> Today, discovery procedure is supported only in hciops. This refactoring has
> been done to provide easier implementation of discovery procedure in mgmtops.
> Lots of effort would be necessary to implement discovery procedure in mgmtops
> because its logic is spread out adapter layer and hciops layer.
> 
> The approach this patchset follows is moving all logic related to discovery
> procedure from adapter layer to hciops layer.
> 
> Future work will be:
>         - full support for discovery procedure (BR/EDR, LE-Only, BR/EDR/LE
>           devices) in kernel via management interface (today, only BR/EDR is
>           supported).
>         - name resolving support through mgmt interface (kernel + userspace)
> 
> Thanks,
> Guedes.
> 
> Anderson Briglia (1):
>   Implement mgmt start and stop discovery
> 
> Andre Guedes (15):
>   Add discovery callbacks to btd_adapter_ops
>   Replace inquiry/scanning calls by discovery calls
>   Add 'discov_state' field to struct dev_info
>   Code cleanup event.c
>   Remove Periodic Inquiry support in hciops
>   Change DiscoverSchedulerInterval default value
>   Add 'timeout' param to start_scanning callback
>   Refactoring adapter_set_state()
>   Remove 'suspend' param from stop_discovery()
>   Add extfeatures to struct dev_info
>   Implement start_discovery hciops callback
>   Remove obsolete code.
>   Implement stop_discovery hciops callback
>   Remove inquiry and scanning callbacks from btd_adapter_ops
>   Remove 'periodic' param from hciops_start_inquiry()
> 
>  plugins/hciops.c  |  373 ++++++++++++++++++++++++++++++++++++-----------------
>  plugins/mgmtops.c |   35 ++----
>  src/adapter.c     |  217 ++++++++++---------------------
>  src/adapter.h     |   19 +--
>  src/event.c       |   48 +-------
>  src/event.h       |    1 -
>  src/main.conf     |    4 +-
>  7 files changed, 345 insertions(+), 352 deletions(-)

All patches in this set have been pushed upstream. Thanks.

There seems to be at least one race condition that'd need fixing which I
can see with test-discovery: if the D-Bus client that requested
discovery exits like test-discovery does it can occur between sending
the HCI_Inquiry command but before the cmd_status or first inquiry event
arrives. In this case the adapter state doesn't seem to be yet updated
and so the ongoing inquiry doesn't get canceled even if it should. You
should be able to see this simply by running hcidump together with
test-discovery e.g. on your laptop.

Johan

Re: [RFC 00/16] Discovery procedure refactoring

[Andre Guedes]

Hi Johan,

Yes, I see the race condition you talking about. Actually, the same happens if
you try LE Set Scan Enable command.

The hciops state machine always transits at event handlers. It means that
this approach leaves us a window between the command and its status
event.

If you call stop_discovery when the window is "open" (the race condition) the
discovery procedure will be stopped, but it will still do _one_ inquiry or
scan operation. So, the penality the race condition brings is performing _one_
inquiry or scan before stopping.

One solution to this problem could be adding extra states to hciops state
machine to represent PENDING INQ, PENDING SCAN, CANCEL INQ
and CANCEL SCAN states.

Right now, we're quite busy supporting discovery procedure in mgmt interface.
Since this is a minor issue, we'll work on this as soon as we conclude mgmt
discovery support.

Anyway, if you prefer I can send you a one-line patch keeping the old behavior.
The old implementation suffers from the same race condition, but only for
LE Set Scan Enable command. Additionally, it has a weird behavior since it
may send a inquiry cancel command even if the adapter is in IDLE state. Also,
it may send a inquiry cancel command before receiving the inquiry command
status event what does not follow the spec, see inquiry cancel command
description: "The command should only be issued after the Inquiry command has
been issued, a Command Status event has been received for the Inquiry command,
and before the Inquiry Complete event occurs".

Thanks,

Andre Guedes.

On Thu, May 5, 2011 at 5:26 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi André,
>
> On Fri, Apr 29, 2011, Andre Guedes wrote:
>> Hi all,
>>
>> Today, discovery procedure is supported only in hciops. This refactoring has
>> been done to provide easier implementation of discovery procedure in mgmtops.
>> Lots of effort would be necessary to implement discovery procedure in mgmtops
>> because its logic is spread out adapter layer and hciops layer.
>>
>> The approach this patchset follows is moving all logic related to discovery
>> procedure from adapter layer to hciops layer.
>>
>> Future work will be:
>>         - full support for discovery procedure (BR/EDR, LE-Only, BR/EDR/LE
>>           devices) in kernel via management interface (today, only BR/EDR is
>>           supported).
>>         - name resolving support through mgmt interface (kernel + userspace)
>>
>> Thanks,
>> Guedes.
>>
>> Anderson Briglia (1):
>>   Implement mgmt start and stop discovery
>>
>> Andre Guedes (15):
>>   Add discovery callbacks to btd_adapter_ops
>>   Replace inquiry/scanning calls by discovery calls
>>   Add 'discov_state' field to struct dev_info
>>   Code cleanup event.c
>>   Remove Periodic Inquiry support in hciops
>>   Change DiscoverSchedulerInterval default value
>>   Add 'timeout' param to start_scanning callback
>>   Refactoring adapter_set_state()
>>   Remove 'suspend' param from stop_discovery()
>>   Add extfeatures to struct dev_info
>>   Implement start_discovery hciops callback
>>   Remove obsolete code.
>>   Implement stop_discovery hciops callback
>>   Remove inquiry and scanning callbacks from btd_adapter_ops
>>   Remove 'periodic' param from hciops_start_inquiry()
>>
>>  plugins/hciops.c  |  373 ++++++++++++++++++++++++++++++++++++-----------------
>>  plugins/mgmtops.c |   35 ++----
>>  src/adapter.c     |  217 ++++++++++---------------------
>>  src/adapter.h     |   19 +--
>>  src/event.c       |   48 +-------
>>  src/event.h       |    1 -
>>  src/main.conf     |    4 +-
>>  7 files changed, 345 insertions(+), 352 deletions(-)
>
> All patches in this set have been pushed upstream. Thanks.
>
> There seems to be at least one race condition that'd need fixing which I
> can see with test-discovery: if the D-Bus client that requested
> discovery exits like test-discovery does it can occur between sending
> the HCI_Inquiry command but before the cmd_status or first inquiry event
> arrives. In this case the adapter state doesn't seem to be yet updated
> and so the ongoing inquiry doesn't get canceled even if it should. You
> should be able to see this simply by running hcidump together with
> test-discovery e.g. on your laptop.
>
> Johan
>