[PATCH v4 0/2] Input1 Connectability revisited

[PATCH v4 0/2] Input1 Connectability revisited

[Alex Deymo]

New version of this patch using Vinicius idea of ReconnectMode, except that
instead of "passive" and "active" we use "Host" and "Device", since the
the passive/active is relative to what end are you talking about (host/device)
Names are free to change.

Also, this patch set removes the input config file introduced in a previous
version of this patch. This property is readonly after all, and is derived
from the SDP record passed to the Input plugin init (input_device_register).
This SDP record comes from the SDP cache and hid_device_probe ensures it is
not NULL.

Comments are welcomed!

Alex Deymo (2):
  input: Documentation for new Input1 interface
  input: Implement the new ReconnectMode Input1 property.

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

-- 
1.8.1.3

[PATCH v4 1/2] input: Documentation for new Input1 interface

[Alex Deymo]

Adds new documentation for a new Input1 interface explaining a new
"ReconnectionMode" 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..66debae
--- /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 ReconnectionMode [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 v4 2/2] 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 | 89 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 86 insertions(+), 3 deletions(-)

diff --git a/profiles/input/device.c b/profiles/input/device.c
index 1da9d99..33a6f65 100644
--- a/profiles/input/device.c
+++ b/profiles/input/device.c
@@ -54,6 +54,22 @@
 
 #include "sdp-client.h"
 
+#define INPUT_INTERFACE "org.bluez.Input1"
+
+enum reconnect_mode_t {
+	RECONNECT_NONE = 0,
+	RECONNECT_DEVICE,
+	RECONNECT_HOST,
+	RECONNECT_ANY
+};
+
+static const char * const reconnect_value[] = {
+	"None",
+	"Device",
+	"Host",
+	"Any"
+};
+
 struct input_device {
 	struct btd_device	*device;
 	char			*path;
@@ -69,8 +85,9 @@ 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;
 };
 
 static GSList *devices = NULL;
@@ -676,7 +693,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;
@@ -697,7 +714,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;
 
@@ -706,6 +723,56 @@ 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 gboolean property_get_reconnect_mode(
+					const GDBusPropertyTable *property,
+					DBusMessageIter *iter, void *data)
+{
+	struct input_device *idev = data;
+
+	dbus_message_iter_append_basic(iter, DBUS_TYPE_STRING,
+					reconnect_value + idev->reconnect_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 +790,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 +841,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

Re: [PATCH v4 2/2] input: Implement the new ReconnectMode Input1 property.

[Vinicius Costa Gomes]

Hi Alex,

On 14:31 Thu 04 Apr, Alex Deymo wrote:
> 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 | 89 +++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 86 insertions(+), 3 deletions(-)
> 
> diff --git a/profiles/input/device.c b/profiles/input/device.c
> index 1da9d99..33a6f65 100644
> --- a/profiles/input/device.c
> +++ b/profiles/input/device.c
> @@ -54,6 +54,22 @@
>  
>  #include "sdp-client.h"
>  
> +#define INPUT_INTERFACE "org.bluez.Input1"
> +
> +enum reconnect_mode_t {
> +	RECONNECT_NONE = 0,
> +	RECONNECT_DEVICE,
> +	RECONNECT_HOST,
> +	RECONNECT_ANY
> +};
> +
> +static const char * const reconnect_value[] = {
> +	"None",
> +	"Device",
> +	"Host",
> +	"Any"
> +};

I would suggest changing this to a explicit function to make the conversion,
something like 'mode_to_string()'.

> +
>  struct input_device {
>  	struct btd_device	*device;
>  	char			*path;
> @@ -69,8 +85,9 @@ struct input_device {
>  	int			timeout;
>  	struct hidp_connadd_req *req;
>  	guint			dc_id;
> -	gboolean		disable_sdp;
> +	bool			disable_sdp;

And I would put this change in another commit.

>  	char			*name;
> +	enum reconnect_mode_t	reconnect_mode;
>  };
>  
>  static GSList *devices = NULL;
> @@ -676,7 +693,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;
> @@ -697,7 +714,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;
>  
> @@ -706,6 +723,56 @@ 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 gboolean property_get_reconnect_mode(
> +					const GDBusPropertyTable *property,
> +					DBusMessageIter *iter, void *data)
> +{
> +	struct input_device *idev = data;
> +
> +	dbus_message_iter_append_basic(iter, DBUS_TYPE_STRING,
> +					reconnect_value + idev->reconnect_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 +790,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 +841,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
> 


Cheers,
-- 
Vinicius

Re: [PATCH v4 0/2] Input1 Connectability revisited

[Vinicius Costa Gomes]

Hi Alex,

On 14:31 Thu 04 Apr, Alex Deymo wrote:
> New version of this patch using Vinicius idea of ReconnectMode, except that
> instead of "passive" and "active" we use "Host" and "Device", since the
> the passive/active is relative to what end are you talking about (host/device)
> Names are free to change.

Sounds good to me. Just please change to lower case names to maintain
consistency with the rest of the API, see media-api.txt, for example.

> 
> Also, this patch set removes the input config file introduced in a previous
> version of this patch. This property is readonly after all, and is derived
> from the SDP record passed to the Input plugin init (input_device_register).
> This SDP record comes from the SDP cache and hid_device_probe ensures it is
> not NULL.
> 
> Comments are welcomed!
> 
> Alex Deymo (2):
>   input: Documentation for new Input1 interface
>   input: Implement the new ReconnectMode Input1 property.
> 
>  doc/input-api.txt       | 32 ++++++++++++++++++
>  profiles/input/device.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 118 insertions(+), 3 deletions(-)
>  create mode 100644 doc/input-api.txt
> 
> -- 
> 1.8.1.3
> 

Cheers,
-- 
Vinicius

Re: [PATCH v4 0/2] Input1 Connectability revisited

[Alex Deymo]

On Thu, Apr 4, 2013 at 2:44 PM, Vinicius Costa Gomes
<vinicius.gomes@openbossa.org> wrote:
> Sounds good to me. Just please change to lower case names to maintain
> consistency with the rest of the API, see media-api.txt, for example.

Ok. I changed this and the reconnect_mode_to_string() thing. Also
split the part about the gboolean in another commit. For some reason I
though the upper case was the consistent with rest of the API, but now
I see that is not the case for property values.
I'll wait until tomorrow to see if someone else has another comment
over this approach and send all the changes together.
Thanks!
Alex.