[PATCH v7 0/5] input: Connectability

[PATCH v7 0/5] input: Connectability

[Alex Deymo]

Hi!
It's me again. Let me insist with these input patches again for a good
reason. There is a qualification test (TP/HCE/BV-04-I [Host Initiated
Re-connection]) that requires a Bluetooth host to automatically
re-connect to a HID device when the device goes out of range (and comes
back in range, obviously). The HID 1.1 spec also states when a device
is supossed to accept re-connections from a host and now we export that
via the ReconnectMode property. I extended this patch with a retry timer
that attempts to reconnect to the HID device for a certain amount of
time.

This patch set includes: Patch 1 and 2 are the new ReconnectMode property
and its documentation. Patch 3 is just some details about bool and gboolean.
Bonus: patch 4 includes a memory leak fix; and patch 5 uses all this
thing to automatically reconnect to HID device when its ReconnectMode is
"host" or "any", as required by the test plan.

Coments are welcome, specially for the new patch 5.
Happy coding!
Alex.

Alex Deymo (5):
  input: Documentation for new Input1 interface
  input: Implement the new ReconnectMode Input1 property.
  input: Convert gboolean to bool
  input: Fix memory leak for hidp_connadd_req.
  input: Automatically attempt a reconnect when required.

 doc/input-api.txt       |  32 ++++++++++
 profiles/input/device.c | 166 +++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 195 insertions(+), 3 deletions(-)
 create mode 100644 doc/input-api.txt

-- 
1.8.1.3

[PATCH v7 1/5] input: Documentation for new Input1 interface

[Alex Deymo]

Adds documentation for a new Input1 interface explaining a new
"ReconnectMode" property that exposes the Connectability mode of
a HID device.
---
 doc/input-api.txt | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)
 create mode 100644 doc/input-api.txt

diff --git a/doc/input-api.txt b/doc/input-api.txt
new file mode 100644
index 0000000..67da08b
--- /dev/null
+++ b/doc/input-api.txt
@@ -0,0 +1,32 @@
+BlueZ D-Bus Input API description
+*********************************
+
+Input hierarchy
+===============
+
+Service		org.bluez
+Interface	org.bluez.Input1
+Object path	[variable prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX
+
+Properties	string ReconnectMode [readonly]
+
+			Determines the Connectability mode of the HID device as
+			defined by the HID Profile specification, Section 5.4.2.
+
+			This mode is based in the two properties
+			HIDReconnectInitiate (see Section 5.3.4.6) and
+			HIDNormallyConnectable (see Section 5.3.4.14) which
+			define the following four possible values:
+
+			"none"		Device and host are not required to
+					automatically restore the connection.
+
+			"host"		Bluetooth HID host restores connection.
+
+			"device"	Bluetooth HID device restores
+					connection.
+
+			"any"		Bluetooth HID device shall attempt to
+					restore the lost connection, but
+					Bluetooth HID Host may also restore the
+					connection.
-- 
1.8.1.3

[PATCH v7 2/5] input: Implement the new ReconnectMode Input1 property.

[Alex Deymo]

The "Connectability" of a HID device, as defined by the HID spec,
governs the way the host may and should connect to a HID device or
expect a connection from it. In the comon case of mice and keyboards
the HIDNormallyConnectable is FALSE and HIDReconnectInitiate is TRUE,
since those devices only attempt a connection to the host when they
have some data to transfer. A connection attempt initiated from the
host after the device drops the connection (while still paired) will
result in a Page timeout.

This patch exposes a new property called "ReconnectMode" combining the
those two SDP attributes as shown in the Connectability section of the
HID spec (see section 5.4.2). The property can have one of the following
four values: "None", "Device", "Host", "Any", and is derived from the
SDP cached value on device creation even if the device is off.
---
 profiles/input/device.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)

diff --git a/profiles/input/device.c b/profiles/input/device.c
index 1da9d99..2701c93 100644
--- a/profiles/input/device.c
+++ b/profiles/input/device.c
@@ -54,6 +54,15 @@
 
 #include "sdp-client.h"
 
+#define INPUT_INTERFACE "org.bluez.Input1"
+
+enum reconnect_mode_t {
+	RECONNECT_NONE = 0,
+	RECONNECT_DEVICE,
+	RECONNECT_HOST,
+	RECONNECT_ANY
+};
+
 struct input_device {
 	struct btd_device	*device;
 	char			*path;
@@ -71,6 +80,7 @@ struct input_device {
 	guint			dc_id;
 	gboolean		disable_sdp;
 	char			*name;
+	enum reconnect_mode_t	reconnect_mode;
 };
 
 static GSList *devices = NULL;
@@ -706,6 +716,68 @@ static gboolean is_device_sdp_disable(const sdp_record_t *rec)
 	return data && data->val.uint8;
 }
 
+static enum reconnect_mode_t hid_reconnection_mode(bool reconnect_initiate,
+						bool normally_connectable)
+{
+	if (!reconnect_initiate && !normally_connectable)
+		return RECONNECT_NONE;
+	else if (!reconnect_initiate && normally_connectable)
+		return RECONNECT_HOST;
+	else if (reconnect_initiate && !normally_connectable)
+		return RECONNECT_DEVICE;
+	else /* (reconnect_initiate && normally_connectable) */
+		return RECONNECT_ANY;
+}
+
+static void extract_hid_props(struct input_device *idev,
+					const sdp_record_t *rec)
+{
+	/* Extract HID connectability */
+	bool reconnect_initiate, normally_connectable;
+	sdp_data_t *pdlist;
+
+	/* HIDNormallyConnectable is optional and assumed FALSE
+	* if not present. */
+	pdlist = sdp_data_get(rec, SDP_ATTR_HID_RECONNECT_INITIATE);
+	reconnect_initiate = pdlist ? pdlist->val.uint8 : TRUE;
+
+	pdlist = sdp_data_get(rec, SDP_ATTR_HID_NORMALLY_CONNECTABLE);
+	normally_connectable = pdlist ? pdlist->val.uint8 : FALSE;
+
+	/* Update local values */
+	idev->reconnect_mode =
+		hid_reconnection_mode(reconnect_initiate, normally_connectable);
+}
+
+static const char * const _reconnect_mode_str[] = {
+	"none",
+	"device",
+	"host",
+	"any"
+};
+
+static const char *reconnect_mode_to_string(const enum reconnect_mode_t mode)
+{
+	return _reconnect_mode_str[mode];
+}
+
+static gboolean property_get_reconnect_mode(
+					const GDBusPropertyTable *property,
+					DBusMessageIter *iter, void *data)
+{
+	struct input_device *idev = data;
+	const char *str_mode = reconnect_mode_to_string(idev->reconnect_mode);
+
+	dbus_message_iter_append_basic(iter, DBUS_TYPE_STRING, &str_mode);
+
+	return TRUE;
+}
+
+static const GDBusPropertyTable input_properties[] = {
+	{ "ReconnectMode", "s", property_get_reconnect_mode },
+	{ }
+};
+
 int input_device_register(struct btd_device *device,
 					const char *path, const char *uuid,
 					const sdp_record_t *rec, int timeout)
@@ -723,6 +795,19 @@ int input_device_register(struct btd_device *device,
 	if (!idev)
 		return -EINVAL;
 
+	/* Initialize device properties */
+	extract_hid_props(idev, rec);
+
+	if (g_dbus_register_interface(btd_get_dbus_connection(),
+					idev->path, INPUT_INTERFACE,
+					NULL, NULL,
+					input_properties, idev,
+					NULL) == FALSE) {
+		error("Unable to register %s interface", INPUT_INTERFACE);
+		input_device_free(idev);
+		return -EINVAL;
+	}
+
 	idev->timeout = timeout;
 	idev->uuid = g_strdup(uuid);
 
@@ -761,6 +846,9 @@ int input_device_unregister(const char *path, const char *uuid)
 		return -EBUSY;
 	}
 
+	g_dbus_unregister_interface(btd_get_dbus_connection(),
+						idev->path, INPUT_INTERFACE);
+
 	devices = g_slist_remove(devices, idev);
 	input_device_free(idev);
 
-- 
1.8.1.3

[PATCH v7 3/5] input: Convert gboolean to bool

[Alex Deymo]

Remove some usage of gboolean in favor of bool.
---
 profiles/input/device.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/profiles/input/device.c b/profiles/input/device.c
index 2701c93..13c74cb 100644
--- a/profiles/input/device.c
+++ b/profiles/input/device.c
@@ -78,7 +78,7 @@ struct input_device {
 	int			timeout;
 	struct hidp_connadd_req *req;
 	guint			dc_id;
-	gboolean		disable_sdp;
+	bool			disable_sdp;
 	char			*name;
 	enum reconnect_mode_t	reconnect_mode;
 };
@@ -686,7 +686,7 @@ int input_device_disconnect(struct btd_device *dev, struct btd_profile *profile)
 
 static struct input_device *input_device_new(struct btd_device *device,
 				const char *path, const uint32_t handle,
-				gboolean disable_sdp)
+				bool disable_sdp)
 {
 	struct btd_adapter *adapter = device_get_adapter(device);
 	struct input_device *idev;
@@ -707,7 +707,7 @@ static struct input_device *input_device_new(struct btd_device *device,
 	return idev;
 }
 
-static gboolean is_device_sdp_disable(const sdp_record_t *rec)
+static bool is_device_sdp_disable(const sdp_record_t *rec)
 {
 	sdp_data_t *data;
 
-- 
1.8.1.3

[PATCH v7 4/5] input: Fix memory leak for hidp_connadd_req.

[Alex Deymo]

If the struct input_device is destroyed while a hidp_connad_req
is pending, the input_device will be destroyed leaking the associated
memory.

==30790== 492 (168 direct, 324 indirect) bytes in 1 blocks are definitely lost in loss record 199 of 216
==30790==    at 0x4C29DB4: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==30790==    by 0x4E7FAE0: g_malloc0 (gmem.c:189)
==30790==    by 0x4378E4: hidp_add_connection (device.c:365)
==30790==    by 0x438055: input_device_connected (device.c:525)
==30790==    by 0x4380DA: interrupt_connect_cb (device.c:548)
==30790==    by 0x443508: connect_cb (btio.c:230)
==30790==    by 0x4E79D52: g_main_context_dispatch (gmain.c:2539)
==30790==    by 0x4E7A09F: g_main_context_iterate.isra.23 (gmain.c:3146)
==30790==    by 0x4E7A499: g_main_loop_run (gmain.c:3340)
==30790==    by 0x448CCC: main (main.c:583)
---
 profiles/input/device.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/profiles/input/device.c b/profiles/input/device.c
index 13c74cb..6ffc199 100644
--- a/profiles/input/device.c
+++ b/profiles/input/device.c
@@ -121,6 +121,11 @@ static void input_device_free(struct input_device *idev)
 	if (idev->ctrl_io)
 		g_io_channel_unref(idev->ctrl_io);
 
+	if (idev->req) {
+		g_free(idev->req->rd_data);
+		g_free(idev->req);
+	}
+
 	g_free(idev->uuid);
 
 	g_free(idev);
-- 
1.8.1.3

[PATCH v7 5/5] input: Automatically attempt a reconnect when required.

[Alex Deymo]

The HID 1.1 spec requires a host to attempt a reconnection when a
HID device goes out of range and comes back. There is test (see
TP/HCE/BV-04-I [Host Initiated Re-connection]) to ensure that the
host initiates the connection when that happens.

This patch adds a reconnection timer for HID devices trying to
reconnect every 30s for a maximum of 3 minutes after the connection
to a HID device with a ReconnectMode of "host" or "any" is closed.
---
 profiles/input/device.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/profiles/input/device.c b/profiles/input/device.c
index 6ffc199..4af95d8 100644
--- a/profiles/input/device.c
+++ b/profiles/input/device.c
@@ -81,10 +81,15 @@ struct input_device {
 	bool			disable_sdp;
 	char			*name;
 	enum reconnect_mode_t	reconnect_mode;
+	guint			reconnect_timer;
+	uint32_t		reconnect_attempt;
 };
 
 static GSList *devices = NULL;
 
+static void input_device_enter_reconnect_mode(struct input_device *idev);
+static const char *reconnect_mode_to_string(const enum reconnect_mode_t mode);
+
 static struct input_device *find_device_by_path(GSList *list, const char *path)
 {
 	for (; list; list = list->next) {
@@ -126,6 +131,9 @@ static void input_device_free(struct input_device *idev)
 		g_free(idev->req);
 	}
 
+	if (idev->reconnect_timer > 0)
+		g_source_remove(idev->reconnect_timer);
+
 	g_free(idev->uuid);
 
 	g_free(idev);
@@ -160,6 +168,9 @@ static gboolean intr_watch_cb(GIOChannel *chan, GIOCondition cond, gpointer data
 	if (idev->ctrl_io && !(cond & G_IO_NVAL))
 		g_io_channel_shutdown(idev->ctrl_io, TRUE, NULL);
 
+	/* Enter the auto-reconnect mode if needed */
+	input_device_enter_reconnect_mode(idev);
+
 	return FALSE;
 }
 
@@ -654,6 +665,62 @@ static int dev_connect(struct input_device *idev)
 	return -EIO;
 }
 
+static gboolean input_device_auto_reconnect(gpointer user_data)
+{
+	struct input_device *idev = user_data;
+
+	DBG("path=%s, attempt=%d", idev->path, idev->reconnect_attempt);
+
+	/* Stop the recurrent reconnection attempts if the device is reconnected
+	 * or is marked for removal. */
+	if (device_is_temporary(idev->device) ||
+					device_is_connected(idev->device))
+		return FALSE;
+
+	/* Only attempt an auto-reconnect for at most 3 minutes (6 * 30s). */
+	if (idev->reconnect_attempt >= 6)
+		return FALSE;
+
+	/* Check if the profile is already connected. */
+	if (idev->ctrl_io)
+		return FALSE;
+
+	if (is_connected(idev))
+		return FALSE;
+
+	idev->reconnect_attempt++;
+	dev_connect(idev);
+
+	return TRUE;
+}
+
+static void input_device_enter_reconnect_mode(struct input_device *idev)
+{
+	DBG("path=%s reconnect_mode=%s", idev->path,
+				reconnect_mode_to_string(idev->reconnect_mode));
+
+	/* Only attempt an auto-reconnect when the device is required to accept
+	 * reconnections from the host. */
+	if (idev->reconnect_mode != RECONNECT_ANY &&
+				idev->reconnect_mode != RECONNECT_HOST)
+		return;
+
+	/* If the device is temporary we are not required to reconnect with the
+	 * device. This is likely the case of a removing device. */
+	if (device_is_temporary(idev->device) ||
+					device_is_connected(idev->device))
+		return;
+
+	if (idev->reconnect_timer > 0)
+		g_source_remove(idev->reconnect_timer);
+
+	DBG("registering auto-reconnect");
+	idev->reconnect_attempt = 0;
+	idev->reconnect_timer = g_timeout_add_seconds(30,
+					input_device_auto_reconnect, idev);
+
+}
+
 int input_device_connect(struct btd_device *dev, struct btd_profile *profile)
 {
 	struct input_device *idev;
-- 
1.8.1.3

Re: [PATCH v7 0/5] input: Connectability

[Marcel Holtmann]

Hi Alex,

> It's me again. Let me insist with these input patches again for a good
> reason. There is a qualification test (TP/HCE/BV-04-I [Host Initiated
> Re-connection]) that requires a Bluetooth host to automatically
> re-connect to a HID device when the device goes out of range (and comes
> back in range, obviously). The HID 1.1 spec also states when a device
> is supossed to accept re-connections from a host and now we export that
> via the ReconnectMode property. I extended this patch with a retry timer
> that attempts to reconnect to the HID device for a certain amount of
> time.
> 
> This patch set includes: Patch 1 and 2 are the new ReconnectMode property
> and its documentation. Patch 3 is just some details about bool and gboolean.
> Bonus: patch 4 includes a memory leak fix; and patch 5 uses all this
> thing to automatically reconnect to HID device when its ReconnectMode is
> "host" or "any", as required by the test plan.

I am fine with these patches, but I leave the final say to Johan.

However I do have a general question, do we need the new D-Bus API if we do an auto-reconnect handling? If so, do we need to indicate that the we are currently in auto-reconnect mode and/or cancel it when connecting attempts via other means happen.

Regards

Marcel

Re: [PATCH v7 0/5] input: Connectability

[Alex Deymo]

On Wed, Apr 17, 2013 at 1:20 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> However I do have a general question, do we need the new D-Bus API if we do an auto-reconnect handling? If so, do we need to indicate that the we are currently in auto-reconnect mode and/or cancel it when connecting attempts via other means happen.

We still need the new D-Bus API to let the UI know that it can't
reconnect to the device because the device is not supposed to normally
accept connections (ReconnectMode is "device" (or "none" if such a
device exists)). This allows the UI to instruct the user to go and
move the mouse to reconnect or press that hidden "connect" button in
his device to reconnect it if the device had lost the pairing. If the
ReconnectMode is "device", the meaning of Device1.Connected property
changes a bit, since the fact that my mouse is disconnected right now
doesn't mean that it is not working as intended or that it should be
connected.

The auto-reconnect is implied and required by the Bluetooth HID spec
for example in cases where ReconnectMode is "host". I don't think we
should indicate that we are currently following the spec. The
auto-reconnect attempts are canceled when the device is reconnected
for some reason (because of this auto-reconnect or not) or when the
device is marked for deletion, even if the removal is during the
auto-reconnect process.

Regards,
Alex.

Re: [PATCH v7 0/5] input: Connectability

[Alex Deymo]

On Wed, Apr 17, 2013 at 1:37 PM, Alex Deymo <deymo@chromium.org> wrote:
> On Wed, Apr 17, 2013 at 1:20 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
>> However I do have a general question, do we need the new D-Bus API if we do an auto-reconnect handling? If so, do we need to indicate that the we are currently in auto-reconnect mode and/or cancel it when connecting attempts via other means happen.
>
> We still need the new D-Bus API to let the UI know that it can't
> reconnect to the device because the device is not supposed to normally
> accept connections (ReconnectMode is "device" (or "none" if such a
> device exists)). This allows the UI to instruct the user to go and
> move the mouse to reconnect or press that hidden "connect" button in
> his device to reconnect it if the device had lost the pairing. If the
> ReconnectMode is "device", the meaning of Device1.Connected property
> changes a bit, since the fact that my mouse is disconnected right now
> doesn't mean that it is not working as intended or that it should be
> connected.
>
> The auto-reconnect is implied and required by the Bluetooth HID spec
> for example in cases where ReconnectMode is "host". I don't think we
> should indicate that we are currently following the spec. The
> auto-reconnect attempts are canceled when the device is reconnected
> for some reason (because of this auto-reconnect or not) or when the
> device is marked for deletion, even if the removal is during the
> auto-reconnect process.

Ping ?

Re: [PATCH v7 0/5] input: Connectability

[Marcel Holtmann]

Hi Alex,

>> However I do have a general question, do we need the new D-Bus API if we do an auto-reconnect handling? If so, do we need to indicate that the we are currently in auto-reconnect mode and/or cancel it when connecting attempts via other means happen.
> 
> We still need the new D-Bus API to let the UI know that it can't
> reconnect to the device because the device is not supposed to normally
> accept connections (ReconnectMode is "device" (or "none" if such a
> device exists)). This allows the UI to instruct the user to go and
> move the mouse to reconnect or press that hidden "connect" button in
> his device to reconnect it if the device had lost the pairing. If the
> ReconnectMode is "device", the meaning of Device1.Connected property
> changes a bit, since the fact that my mouse is disconnected right now
> doesn't mean that it is not working as intended or that it should be
> connected.

I might have should answered this email with an acknowledged and understood comment. Sorry about that.

My original comment still stands. I am fine with this patch set, but leave it up to Johan to make the final to apply it.

> The auto-reconnect is implied and required by the Bluetooth HID spec
> for example in cases where ReconnectMode is "host". I don't think we
> should indicate that we are currently following the spec. The
> auto-reconnect attempts are canceled when the device is reconnected
> for some reason (because of this auto-reconnect or not) or when the
> device is marked for deletion, even if the removal is during the
> auto-reconnect process.

Sounds good.

Regards

Marcel

Re: [PATCH v7 0/5] input: Connectability

[Johan Hedberg]

Hi,

On Mon, Apr 22, 2013, Marcel Holtmann wrote:
> >> However I do have a general question, do we need the new D-Bus API
> >> if we do an auto-reconnect handling? If so, do we need to indicate
> >> that the we are currently in auto-reconnect mode and/or cancel it
> >> when connecting attempts via other means happen.
> > 
> > We still need the new D-Bus API to let the UI know that it can't
> > reconnect to the device because the device is not supposed to normally
> > accept connections (ReconnectMode is "device" (or "none" if such a
> > device exists)). This allows the UI to instruct the user to go and
> > move the mouse to reconnect or press that hidden "connect" button in
> > his device to reconnect it if the device had lost the pairing. If the
> > ReconnectMode is "device", the meaning of Device1.Connected property
> > changes a bit, since the fact that my mouse is disconnected right now
> > doesn't mean that it is not working as intended or that it should be
> > connected.
> 
> I might have should answered this email with an acknowledged and
> understood comment. Sorry about that.
> 
> My original comment still stands. I am fine with this patch set, but
> leave it up to Johan to make the final to apply it.

No major issues from my side either. All patches in the set have now
been applied.

Johan