2013-03-19 01:05:44

by Alex Deymo

[permalink] [raw]
Subject: [PATCH] Expose HIDNormallyConnectable and HIDReconnectInitiate properties

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 these two HID properties as Device properties, updates
them every time new SDP records are available and persists them in the
device's info file. This patch also shows this information in the
bluetoothctl "info <dev>" command.
For non-HID devices and when no information is available (i.e. a cached
device after upgrade from a version without this patch) both properties
are set to the comon case: HIDNormallyConnectable is FALSE and
HIDReconnectInitiate is TRUE.
---

This patch is a rework of a previous patch named "Add GetCachedServices
to device API". This new attempt exposes only these two properties
related to the "Connectability" of the device.

client/main.c | 2 +
doc/device-api.txt | 22 +++++++++++
src/device.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++++++
src/device.h | 2 +
4 files changed, 138 insertions(+)

diff --git a/client/main.c b/client/main.c
index 9a927a8..bc4be62 100644
--- a/client/main.c
+++ b/client/main.c
@@ -707,6 +707,8 @@ static void cmd_info(const char *arg)
print_property(proxy, "LegacyPairing");
print_uuids(proxy);
print_property(proxy, "Modalias");
+ print_property(proxy, "HIDNormallyConnectable");
+ print_property(proxy, "HIDReconnectInitiate");
}

static void pair_reply(DBusMessage *message, void *user_data)
diff --git a/doc/device-api.txt b/doc/device-api.txt
index 0f916c7..795845c 100644
--- a/doc/device-api.txt
+++ b/doc/device-api.txt
@@ -185,3 +185,25 @@ Properties string Address [readonly]

Received Signal Strength Indicator of the remote
device.
+
+ boolean HIDNormallyConnectable [readonly]
+
+ For HID devices exposes the value of
+ HIDNormallyConnectable as defined by the HID profile
+ specification. If this optional value is not specified
+ by the HID device, the value is asumend as FALSE.
+
+ For devices not supporting the HID profile or if it is
+ unknown if the device supports the HID profile, this
+ property is set to FALSE. You must check the UUIDs
+ property to know if the device supports the HID profile.
+
+ boolean HIDConnectInitiate [readonly]
+
+ For HID devices exposes the value of HIDConnectInitiate
+ as defined by the HID profile specification.
+
+ For devices not supporting the HID profile or if it is
+ unknown if the device supports the HID profile, this
+ property is set to TRUE. You must check the UUIDs
+ property to know if the device supports the HID profile.
diff --git a/src/device.c b/src/device.c
index 3cd7f10..3064b8f 100644
--- a/src/device.c
+++ b/src/device.c
@@ -196,6 +196,8 @@ struct btd_device {
gboolean auto_connect;
gboolean disable_auto_connect;
gboolean general_connect;
+ gboolean hid_normally_connectable;
+ gboolean hid_reconnect_initiate;

bool legacy;
int8_t rssi;
@@ -228,6 +230,8 @@ static gboolean store_device_info_cb(gpointer user_data)
char class[9];
char **uuids = NULL;
gsize length = 0;
+ gboolean has_hid_records;
+ GSList *l;

device->store_id = 0;

@@ -297,6 +301,23 @@ static gboolean store_device_info_cb(gpointer user_data)
g_key_file_set_boolean(key_file, "General", "Blocked",
device->blocked);

+ has_hid_records = FALSE;
+ for (l = device->uuids; l && !has_hid_records; l = g_slist_next(l))
+ has_hid_records = strcmp(l->data, HID_UUID);
+
+ if (has_hid_records) {
+ g_key_file_set_boolean(key_file, "General",
+ "HIDNormallyConnectable",
+ device->hid_normally_connectable);
+ g_key_file_set_boolean(key_file, "General",
+ "HIDReconnectInitiate", device->hid_reconnect_initiate);
+ } else {
+ g_key_file_remove_key(key_file, "General",
+ "HIDNormallyConnectable", NULL);
+ g_key_file_remove_key(key_file, "General",
+ "HIDReconnectInitiate", NULL);
+ }
+
if (device->uuids) {
GSList *l;
int i;
@@ -870,6 +891,31 @@ static gboolean dev_property_get_adapter(const GDBusPropertyTable *property,
return TRUE;
}

+static gboolean dev_property_get_hid_normally_connectable(
+ const GDBusPropertyTable *property,
+ DBusMessageIter *iter, void *data)
+{
+ struct btd_device *device = data;
+
+ dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN,
+ &device->hid_normally_connectable);
+
+ return TRUE;
+}
+
+static gboolean dev_property_get_hid_reconnect_initiate(
+ const GDBusPropertyTable *property,
+ DBusMessageIter *iter, void *data)
+{
+ struct btd_device *device = data;
+
+ dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN,
+ &device->hid_reconnect_initiate);
+
+ return TRUE;
+}
+
+
static void profile_remove(gpointer data, gpointer user_data)
{
struct btd_profile *profile = data;
@@ -1615,6 +1661,10 @@ static const GDBusPropertyTable device_properties[] = {
{ "Modalias", "s", dev_property_get_modalias, NULL,
dev_property_exists_modalias },
{ "Adapter", "o", dev_property_get_adapter },
+ { "HIDNormallyConnectable", "b",
+ dev_property_get_hid_normally_connectable },
+ { "HIDReconnectInitiate", "b",
+ dev_property_get_hid_reconnect_initiate },
{ }
};

@@ -1748,6 +1798,7 @@ static void load_info(struct btd_device *device, const char *local,
char **techno, **t;
gboolean bredr = FALSE;
gboolean le = FALSE;
+ GError *err = NULL;

/* Load device name from storage info file, if that fails fall back to
* the cache.
@@ -1830,6 +1881,23 @@ next:
if (blocked)
device_block(device, FALSE);

+ /* Load HID connectivity properties */
+ device->hid_normally_connectable = g_key_file_get_boolean(key_file,
+ "General", "HIDNormallyConnectable", &err);
+ if (err) {
+ device->hid_normally_connectable = FALSE;
+ g_error_free(err);
+ err = NULL;
+ }
+
+ device->hid_reconnect_initiate = g_key_file_get_boolean(key_file,
+ "General", "HIDReconnectInitiate", &err);
+ if (err) {
+ device->hid_reconnect_initiate = TRUE;
+ g_error_free(err);
+ err = NULL;
+ }
+
/* Load device profile list */
uuids = g_key_file_get_string_list(key_file, "General", "Services",
NULL, NULL);
@@ -2045,6 +2113,11 @@ struct btd_device *device_create(struct btd_adapter *adapter,
g_free(str);
}

+ /* In absense of futher information we expose the comon values for HID
+ * devices. */
+ device->hid_normally_connectable = FALSE;
+ device->hid_reconnect_initiate = TRUE;
+
return device;
}

@@ -2611,6 +2684,24 @@ static void update_bredr_services(struct browse_req *req, sdp_list_t *recs)
product, version);
}

+ /* Extract HID connectability */
+ if (bt_uuid_strcmp(profile_uuid, HID_UUID) == 0) {
+ bool hid_nc, hid_ri;
+ sdp_data_t *pdlist;
+
+ /* HIDNormallyConnectable is optional and assumed FALSE
+ * if not present. */
+ pdlist = sdp_data_get(rec,
+ SDP_ATTR_HID_NORMALLY_CONNECTABLE);
+ hid_nc = pdlist ? pdlist->val.uint8 : FALSE;
+
+ pdlist = sdp_data_get(rec,
+ SDP_ATTR_HID_RECONNECT_INITIATE);
+ hid_ri = pdlist ? pdlist->val.uint8 : TRUE;
+
+ btd_device_set_hid_props(device, hid_nc, hid_ri);
+ }
+
/* Check for duplicates */
if (sdp_list_find(req->records, rec, rec_cmp)) {
g_free(profile_uuid);
@@ -4281,6 +4372,27 @@ void btd_device_set_pnpid(struct btd_device *device, uint16_t source,
store_device_info(device);
}

+void btd_device_set_hid_props(struct btd_device *device,
+ gboolean normally_connectable, gboolean reconnect_initiate)
+{
+ gboolean update_nc, update_ri;
+
+ update_nc = device->hid_normally_connectable != normally_connectable;
+ device->hid_normally_connectable = normally_connectable;
+ if (update_nc)
+ g_dbus_emit_property_changed(dbus_conn, device->path,
+ DEVICE_INTERFACE, "HIDNormallyConnectable");
+
+ update_ri = device->hid_reconnect_initiate != reconnect_initiate;
+ device->hid_reconnect_initiate = reconnect_initiate;
+ if (update_ri)
+ g_dbus_emit_property_changed(dbus_conn, device->path,
+ DEVICE_INTERFACE, "HIDReconnectInitiate");
+
+ if (update_nc || update_ri)
+ store_device_info(device);
+}
+
void btd_device_init(void)
{
dbus_conn = btd_get_dbus_connection();
diff --git a/src/device.h b/src/device.h
index d072015..1442e0a 100644
--- a/src/device.h
+++ b/src/device.h
@@ -108,6 +108,8 @@ int device_unblock(struct btd_device *device, gboolean silent,
gboolean update_only);
void btd_device_set_pnpid(struct btd_device *device, uint16_t source,
uint16_t vendor, uint16_t product, uint16_t version);
+void btd_device_set_hid_props(struct btd_device *device,
+ gboolean normally_connectable, gboolean reconnect_initiate);

int device_connect_le(struct btd_device *dev);

--
1.8.1.3



2013-03-20 15:47:16

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH] Expose HIDNormallyConnectable and HIDReconnectInitiate properties

Hi Alex,

> 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 these two HID properties as Device properties, updates
> them every time new SDP records are available and persists them in the
> device's info file. This patch also shows this information in the
> bluetoothctl "info <dev>" command.
> For non-HID devices and when no information is available (i.e. a cached
> device after upgrade from a version without this patch) both properties
> are set to the comon case: HIDNormallyConnectable is FALSE and
> HIDReconnectInitiate is TRUE.
> ---
>
> This patch is a rework of a previous patch named "Add GetCachedServices
> to device API". This new attempt exposes only these two properties
> related to the "Connectability" of the device.

I am not a huge fan of making these part of the Device1 interface. They should be part of an Input1 interface on the same object path. And it should be only present for HID devices. Making this available for everybody is a bit against the idea of keeping it simple.

We do not have an Input1 interface right now, but that does not stop the input plugin from creating this.

Regards

Marcel


2013-04-04 19:52:06

by Vinicius Costa Gomes

[permalink] [raw]
Subject: Re: [PATCH v3 1/2] input: Documentation for new Input1 interface

Hi Alex,

On 12:13 Thu 04 Apr, Alex Deymo wrote:
> On Wed, Apr 3, 2013 at 9:43 PM, Vinicius Costa Gomes
> <[email protected]> wrote:
> > Just for fun, the only idea that I could come up with is: "ReconnectionMode"
> > or "Mode" with two values: "active" (the remote is not normally connectable,
> > but is able to initiate connections), "passive" (the remote is not able to
> > initiate connections but is normally connectable) and "dual".
> >
> > Alex, does something like this cover the use cases that you have in mind?
> >
> Hi!
> The HID profile spec [1], page 77, Table 5.7, defines the four values
> as valid (True/False for each property). I don't know any device that
> has both values in False, and it looks kind of weird to have to click
> some sort of hidden connect button in a input only HID device every
> time you want to reconnect since you can do data-driven reconnections,
> but well, the specs allows that option. The important thing here is
> that these four values are meaningful to the user interface and we
> should export them in some way, because you have to notify the user to
> do actions like "please, move your mouse to reconnect" or "please,
> click the connect button in your mouse to reconnect" or similar
> messages in situations where the host can't simply reconnect to the
> device with user interaction.
>
> I agree that the spec names aren't the best ones, but at least are the
> standard names defined in the spec. Exporting two boolean properties
> with different names than the spec will simply add more noise. Combine
> both properties in one "ReconnectionMode" looks better to me since
> both variables are coupled in the mentioned table, but it will have
> the four values ("none", "active", "passive", "dual").

Yeah, even if it doesn't make much sense we should handle the "none" case.

>
> Other option:
> "ReconnectInitiator": "none", "device", "host", "both"

I still like "ReconnectionMode" slightly better, but no strong feelings about
it. And in this case I would rename "both" to "any".

>
> Tell me which one do you like the most.
> Cheers,
> Alex.
>
> [1] https://www.bluetooth.org/docman/handlers/downloaddoc.ashx?doc_id=246761
>
>
> > Cheers,
> > --
> > Vinicius


Cheers,
--
Vinicius

2013-04-04 19:13:39

by Alex Deymo

[permalink] [raw]
Subject: Re: [PATCH v3 1/2] input: Documentation for new Input1 interface

On Wed, Apr 3, 2013 at 9:43 PM, Vinicius Costa Gomes
<[email protected]> wrote:
> Just for fun, the only idea that I could come up with is: "ReconnectionMode"
> or "Mode" with two values: "active" (the remote is not normally connectable,
> but is able to initiate connections), "passive" (the remote is not able to
> initiate connections but is normally connectable) and "dual".
>
> Alex, does something like this cover the use cases that you have in mind?
>
Hi!
The HID profile spec [1], page 77, Table 5.7, defines the four values
as valid (True/False for each property). I don't know any device that
has both values in False, and it looks kind of weird to have to click
some sort of hidden connect button in a input only HID device every
time you want to reconnect since you can do data-driven reconnections,
but well, the specs allows that option. The important thing here is
that these four values are meaningful to the user interface and we
should export them in some way, because you have to notify the user to
do actions like "please, move your mouse to reconnect" or "please,
click the connect button in your mouse to reconnect" or similar
messages in situations where the host can't simply reconnect to the
device with user interaction.

I agree that the spec names aren't the best ones, but at least are the
standard names defined in the spec. Exporting two boolean properties
with different names than the spec will simply add more noise. Combine
both properties in one "ReconnectionMode" looks better to me since
both variables are coupled in the mentioned table, but it will have
the four values ("none", "active", "passive", "dual").

Other option:
"ReconnectInitiator": "none", "device", "host", "both"

Tell me which one do you like the most.
Cheers,
Alex.

[1] https://www.bluetooth.org/docman/handlers/downloaddoc.ashx?doc_id=246761


> Cheers,
> --
> Vinicius

2013-04-04 04:43:32

by Vinicius Costa Gomes

[permalink] [raw]
Subject: Re: [PATCH v3 1/2] input: Documentation for new Input1 interface

Hi,


On 20:48 Wed 03 Apr, Marcel Holtmann wrote:
> Hi Alex,
>
> > Adds new documentation for a new Input1 interface exposing two properties.
> > ---
> > doc/input-api.txt | 21 +++++++++++++++++++++
> > 1 file changed, 21 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..44bbde6
> > --- /dev/null
> > +++ b/doc/input-api.txt
> > @@ -0,0 +1,21 @@
> > +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 boolean NormallyConnectable [readonly]
> > +
> > + Exposes the value of HIDNormallyConnectable as defined
> > + by the HID profile specification. If this optional value
> > + is not specified by the HID device, the value is asumend
> > + as FALSE.
> > +
> > + boolean ConnectInitiate [readonly]
> > +
> > + Exposes the value of ConnectInitiate as defined by the
> > + HID profile specification.
>
> I am not 100% comfortable with the property names yet. Especially the ConnectInitiate sounds badly to me. Does anybody have any ideas?

Just for fun, the only idea that I could come up with is: "ReconnectionMode"
or "Mode" with two values: "active" (the remote is not normally connectable,
but is able to initiate connections), "passive" (the remote is not able to
initiate connections but is normally connectable) and "dual".

Alex, does something like this cover the use cases that you have in mind?

>
> Regards
>
> Marcel
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html


Cheers,
--
Vinicius

2013-04-04 03:48:20

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH v3 1/2] input: Documentation for new Input1 interface

Hi Alex,

> Adds new documentation for a new Input1 interface exposing two properties.
> ---
> doc/input-api.txt | 21 +++++++++++++++++++++
> 1 file changed, 21 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..44bbde6
> --- /dev/null
> +++ b/doc/input-api.txt
> @@ -0,0 +1,21 @@
> +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 boolean NormallyConnectable [readonly]
> +
> + Exposes the value of HIDNormallyConnectable as defined
> + by the HID profile specification. If this optional value
> + is not specified by the HID device, the value is asumend
> + as FALSE.
> +
> + boolean ConnectInitiate [readonly]
> +
> + Exposes the value of ConnectInitiate as defined by the
> + HID profile specification.

I am not 100% comfortable with the property names yet. Especially the ConnectInitiate sounds badly to me. Does anybody have any ideas?

Regards

Marcel



2013-04-04 03:42:44

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] input: Expose HIDNormallyConnectable and HIDReconnectInitiate properties

Hi Alex,

> 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 these two HID properties in a new Input1 interface,
> updates them every time new SDP records are available and persists them
> in a new device "input" file.
> ---
> profiles/input/device.c | 170 +++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 168 insertions(+), 2 deletions(-)
>
> diff --git a/profiles/input/device.c b/profiles/input/device.c
> index 1da9d99..a2a87a0 100644
> --- a/profiles/input/device.c
> +++ b/profiles/input/device.c
> @@ -54,6 +54,8 @@
>
> #include "sdp-client.h"
>
> +#define INPUT_INTERFACE "org.bluez.Input1"
> +
> struct input_device {
> struct btd_device *device;
> char *path;
> @@ -69,8 +71,10 @@ struct input_device {
> int timeout;
> struct hidp_connadd_req *req;
> guint dc_id;
> - gboolean disable_sdp;
> + bool disable_sdp;

if you change this one, then you need to change another one in is_device_sdp_disable.

> char *name;
> + bool normally_connectable;
> + bool reconnect_initiate;
> };
>
> static GSList *devices = NULL;
> @@ -116,6 +120,81 @@ static void input_device_free(struct input_device *idev)
> g_free(idev);
> }
>
> +static void input_device_config_filename(struct input_device *idev,
> + char *filename, size_t maxlen)
> +{
> + char adapter_addr[18];
> + char device_addr[18];
> +
> + ba2str(device_get_address(idev->device), device_addr);
> + ba2str(adapter_get_address(device_get_adapter(idev->device)),
> + adapter_addr);
> + snprintf(filename, maxlen, STORAGEDIR "/%s/%s/input", adapter_addr,
> + device_addr);
> + filename[maxlen-1] = '\0';
> +}
> +
> +static void input_device_load_props(struct input_device *idev)
> +{
> + char filename[PATH_MAX + 1];
> + GKeyFile *key_file;
> + GError *err = NULL;
> +
> + input_device_config_filename(idev, filename, PATH_MAX + 1);
> +
> + key_file = g_key_file_new();
> + if (!key_file)
> + return;

Minor nitpick, but put an extra empty line here.

> + g_key_file_load_from_file(key_file, filename, 0, NULL);
> +
> + /* Load HID connectivity properties */
> + idev->normally_connectable = g_key_file_get_boolean(key_file,
> + "Input", "NormallyConnectable", &err);
> + if (err) {
> + idev->normally_connectable = FALSE;
> + g_error_free(err);
> + err = NULL;
> + }
> +
> + idev->reconnect_initiate = g_key_file_get_boolean(key_file,
> + "Input", "ReconnectInitiate", &err);
> + if (err) {
> + idev->reconnect_initiate = TRUE;
> + g_error_free(err);
> + err = NULL;
> + }
> +
> + g_key_file_free(key_file);
> +}
> +
> +static void input_device_store_props(struct input_device *idev)
> +{
> + char filename[PATH_MAX + 1];
> + GKeyFile *key_file;
> + char *str;
> + gsize length = 0;
> +
> + input_device_config_filename(idev, filename, PATH_MAX + 1);
> +
> + key_file = g_key_file_new();
> + if (!key_file)
> + return;

Same here. And just thinking about it, can g_key_file_new() actually fail. Some of the GLib new function will just abort if no memory can be allocated. Can you check please. In the past we added pointless NULL pointer checks since we never checked.

> + g_key_file_load_from_file(key_file, filename, 0, NULL);
> +
> + g_key_file_set_boolean(key_file, "Input",
> + "NormallyConnectable", idev->normally_connectable);
> + g_key_file_set_boolean(key_file, "Input",
> + "ReconnectInitiate", idev->reconnect_initiate);
> +
> + create_file(filename, S_IRUSR | S_IWUSR);
> +
> + str = g_key_file_to_data(key_file, &length, NULL);
> + g_file_set_contents(filename, str, length, NULL);
> + g_free(str);
> +
> + g_key_file_free(key_file);
> +}
> +
> static gboolean intr_watch_cb(GIOChannel *chan, GIOCondition cond, gpointer data)
> {
> struct input_device *idev = data;
> @@ -676,7 +755,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;
> @@ -694,6 +773,8 @@ static struct input_device *input_device_new(struct btd_device *device,
> if (strlen(name) > 0)
> idev->name = g_strdup(name);
>
> + input_device_load_props(idev);
> +
> return idev;
> }
>
> @@ -706,6 +787,76 @@ static gboolean is_device_sdp_disable(const sdp_record_t *rec)
> return data && data->val.uint8;
> }
>
> +static void update_hid_props(struct input_device *idev,
> + gboolean normally_connectable, gboolean reconnect_initiate)

Can we go with bool here as well.

> +{
> + gboolean update_nc, update_ri;
> + DBusConnection *dbus_conn = btd_get_dbus_connection();
> +
> + update_nc = idev->normally_connectable != normally_connectable;
> + idev->normally_connectable = normally_connectable;
> + if (update_nc)
> + g_dbus_emit_property_changed(dbus_conn, idev->path,
> + DEVICE_INTERFACE, "HIDNormallyConnectable");
> +
> + update_ri = idev->reconnect_initiate != reconnect_initiate;
> + idev->reconnect_initiate = reconnect_initiate;
> + if (update_ri)
> + g_dbus_emit_property_changed(dbus_conn, idev->path,
> + DEVICE_INTERFACE, "HIDReconnectInitiate");
> +
> + if (update_nc || update_ri)
> + input_device_store_props(idev);
> +}
> +
> +static void extract_hid_props(struct input_device *idev,
> + const sdp_record_t *rec)
> +{
> + /* Extract HID connectability */
> + bool hid_nc, hid_ri;
> + sdp_data_t *pdlist;
> +
> + /* HIDNormallyConnectable is optional and assumed FALSE
> + * if not present. */
> + pdlist = sdp_data_get(rec, SDP_ATTR_HID_NORMALLY_CONNECTABLE);
> + hid_nc = pdlist ? pdlist->val.uint8 : FALSE;
> +
> + pdlist = sdp_data_get(rec, SDP_ATTR_HID_RECONNECT_INITIATE);
> + hid_ri = pdlist ? pdlist->val.uint8 : TRUE;
> +
> + update_hid_props(idev, hid_nc, hid_ri);
> +}
> +
> +static gboolean property_get_normally_connectable(
> + const GDBusPropertyTable *property,
> + DBusMessageIter *iter, void *data)
> +{
> + struct input_device *idev = data;
> +
> + dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN,
> + &idev->normally_connectable);

If we want to be 100% safe here, then converting it to dbus_bool_t first is better.

> +
> + return TRUE;
> +}
> +
> +static gboolean property_get_reconnect_initiate(
> + const GDBusPropertyTable *property,
> + DBusMessageIter *iter, void *data)
> +{
> + struct input_device *idev = data;
> +
> + dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN,
> + &idev->reconnect_initiate);

Same here.

> +
> + return TRUE;
> +}
> +
> +static const GDBusPropertyTable input_properties[] = {
> + { "NormallyConnectable", "b", property_get_normally_connectable },
> + { "ReconnectInitiate", "b", property_get_reconnect_initiate },
> + { }
> +};
> +
> int input_device_register(struct btd_device *device,
> const char *path, const char *uuid,
> const sdp_record_t *rec, int timeout)
> @@ -723,6 +874,18 @@ int input_device_register(struct btd_device *device,
> if (!idev)
> return -EINVAL;
>
> + 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;
> + }
> +
> + extract_hid_props(idev, rec);
> +
> idev->timeout = timeout;
> idev->uuid = g_strdup(uuid);
>
> @@ -761,6 +924,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);

Regards

Marcel


2013-04-02 00:40:26

by Alex Deymo

[permalink] [raw]
Subject: [PATCH v3 2/2] input: Expose HIDNormallyConnectable and HIDReconnectInitiate properties

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 these two HID properties in a new Input1 interface,
updates them every time new SDP records are available and persists them
in a new device "input" file.
---
profiles/input/device.c | 170 +++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 168 insertions(+), 2 deletions(-)

diff --git a/profiles/input/device.c b/profiles/input/device.c
index 1da9d99..a2a87a0 100644
--- a/profiles/input/device.c
+++ b/profiles/input/device.c
@@ -54,6 +54,8 @@

#include "sdp-client.h"

+#define INPUT_INTERFACE "org.bluez.Input1"
+
struct input_device {
struct btd_device *device;
char *path;
@@ -69,8 +71,10 @@ struct input_device {
int timeout;
struct hidp_connadd_req *req;
guint dc_id;
- gboolean disable_sdp;
+ bool disable_sdp;
char *name;
+ bool normally_connectable;
+ bool reconnect_initiate;
};

static GSList *devices = NULL;
@@ -116,6 +120,81 @@ static void input_device_free(struct input_device *idev)
g_free(idev);
}

+static void input_device_config_filename(struct input_device *idev,
+ char *filename, size_t maxlen)
+{
+ char adapter_addr[18];
+ char device_addr[18];
+
+ ba2str(device_get_address(idev->device), device_addr);
+ ba2str(adapter_get_address(device_get_adapter(idev->device)),
+ adapter_addr);
+ snprintf(filename, maxlen, STORAGEDIR "/%s/%s/input", adapter_addr,
+ device_addr);
+ filename[maxlen-1] = '\0';
+}
+
+static void input_device_load_props(struct input_device *idev)
+{
+ char filename[PATH_MAX + 1];
+ GKeyFile *key_file;
+ GError *err = NULL;
+
+ input_device_config_filename(idev, filename, PATH_MAX + 1);
+
+ key_file = g_key_file_new();
+ if (!key_file)
+ return;
+ g_key_file_load_from_file(key_file, filename, 0, NULL);
+
+ /* Load HID connectivity properties */
+ idev->normally_connectable = g_key_file_get_boolean(key_file,
+ "Input", "NormallyConnectable", &err);
+ if (err) {
+ idev->normally_connectable = FALSE;
+ g_error_free(err);
+ err = NULL;
+ }
+
+ idev->reconnect_initiate = g_key_file_get_boolean(key_file,
+ "Input", "ReconnectInitiate", &err);
+ if (err) {
+ idev->reconnect_initiate = TRUE;
+ g_error_free(err);
+ err = NULL;
+ }
+
+ g_key_file_free(key_file);
+}
+
+static void input_device_store_props(struct input_device *idev)
+{
+ char filename[PATH_MAX + 1];
+ GKeyFile *key_file;
+ char *str;
+ gsize length = 0;
+
+ input_device_config_filename(idev, filename, PATH_MAX + 1);
+
+ key_file = g_key_file_new();
+ if (!key_file)
+ return;
+ g_key_file_load_from_file(key_file, filename, 0, NULL);
+
+ g_key_file_set_boolean(key_file, "Input",
+ "NormallyConnectable", idev->normally_connectable);
+ g_key_file_set_boolean(key_file, "Input",
+ "ReconnectInitiate", idev->reconnect_initiate);
+
+ create_file(filename, S_IRUSR | S_IWUSR);
+
+ str = g_key_file_to_data(key_file, &length, NULL);
+ g_file_set_contents(filename, str, length, NULL);
+ g_free(str);
+
+ g_key_file_free(key_file);
+}
+
static gboolean intr_watch_cb(GIOChannel *chan, GIOCondition cond, gpointer data)
{
struct input_device *idev = data;
@@ -676,7 +755,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;
@@ -694,6 +773,8 @@ static struct input_device *input_device_new(struct btd_device *device,
if (strlen(name) > 0)
idev->name = g_strdup(name);

+ input_device_load_props(idev);
+
return idev;
}

@@ -706,6 +787,76 @@ static gboolean is_device_sdp_disable(const sdp_record_t *rec)
return data && data->val.uint8;
}

+static void update_hid_props(struct input_device *idev,
+ gboolean normally_connectable, gboolean reconnect_initiate)
+{
+ gboolean update_nc, update_ri;
+ DBusConnection *dbus_conn = btd_get_dbus_connection();
+
+ update_nc = idev->normally_connectable != normally_connectable;
+ idev->normally_connectable = normally_connectable;
+ if (update_nc)
+ g_dbus_emit_property_changed(dbus_conn, idev->path,
+ DEVICE_INTERFACE, "HIDNormallyConnectable");
+
+ update_ri = idev->reconnect_initiate != reconnect_initiate;
+ idev->reconnect_initiate = reconnect_initiate;
+ if (update_ri)
+ g_dbus_emit_property_changed(dbus_conn, idev->path,
+ DEVICE_INTERFACE, "HIDReconnectInitiate");
+
+ if (update_nc || update_ri)
+ input_device_store_props(idev);
+}
+
+static void extract_hid_props(struct input_device *idev,
+ const sdp_record_t *rec)
+{
+ /* Extract HID connectability */
+ bool hid_nc, hid_ri;
+ sdp_data_t *pdlist;
+
+ /* HIDNormallyConnectable is optional and assumed FALSE
+ * if not present. */
+ pdlist = sdp_data_get(rec, SDP_ATTR_HID_NORMALLY_CONNECTABLE);
+ hid_nc = pdlist ? pdlist->val.uint8 : FALSE;
+
+ pdlist = sdp_data_get(rec, SDP_ATTR_HID_RECONNECT_INITIATE);
+ hid_ri = pdlist ? pdlist->val.uint8 : TRUE;
+
+ update_hid_props(idev, hid_nc, hid_ri);
+}
+
+static gboolean property_get_normally_connectable(
+ const GDBusPropertyTable *property,
+ DBusMessageIter *iter, void *data)
+{
+ struct input_device *idev = data;
+
+ dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN,
+ &idev->normally_connectable);
+
+ return TRUE;
+}
+
+static gboolean property_get_reconnect_initiate(
+ const GDBusPropertyTable *property,
+ DBusMessageIter *iter, void *data)
+{
+ struct input_device *idev = data;
+
+ dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN,
+ &idev->reconnect_initiate);
+
+ return TRUE;
+}
+
+static const GDBusPropertyTable input_properties[] = {
+ { "NormallyConnectable", "b", property_get_normally_connectable },
+ { "ReconnectInitiate", "b", property_get_reconnect_initiate },
+ { }
+};
+
int input_device_register(struct btd_device *device,
const char *path, const char *uuid,
const sdp_record_t *rec, int timeout)
@@ -723,6 +874,18 @@ int input_device_register(struct btd_device *device,
if (!idev)
return -EINVAL;

+ 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;
+ }
+
+ extract_hid_props(idev, rec);
+
idev->timeout = timeout;
idev->uuid = g_strdup(uuid);

@@ -761,6 +924,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

2013-04-02 00:40:25

by Alex Deymo

[permalink] [raw]
Subject: [PATCH v3 1/2] input: Documentation for new Input1 interface

Adds new documentation for a new Input1 interface exposing two properties.
---
doc/input-api.txt | 21 +++++++++++++++++++++
1 file changed, 21 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..44bbde6
--- /dev/null
+++ b/doc/input-api.txt
@@ -0,0 +1,21 @@
+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 boolean NormallyConnectable [readonly]
+
+ Exposes the value of HIDNormallyConnectable as defined
+ by the HID profile specification. If this optional value
+ is not specified by the HID device, the value is asumend
+ as FALSE.
+
+ boolean ConnectInitiate [readonly]
+
+ Exposes the value of ConnectInitiate as defined by the
+ HID profile specification.
--
1.8.1.3

2013-04-02 00:40:24

by Alex Deymo

[permalink] [raw]
Subject: [PATCH v3 0/2] HID Connetabillity

Addressed Marcel comments on PATCH v2.
* The two properties are always exposed.
* Persistence file is <adapter>/<device>/input. No global helper is used.
* Doc in a separate commit.
* Logs updated.

Alex Deymo (2):
input: Documentation for new Input1 interface
core: Expose HIDNormallyConnectable and HIDReconnectInitiate
properties

doc/input-api.txt | 21 ++++++
profiles/input/device.c | 170 +++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 189 insertions(+), 2 deletions(-)
create mode 100644 doc/input-api.txt

--
1.8.1.3


2013-04-01 22:54:11

by Alex Deymo

[permalink] [raw]
Subject: Re: [PATCH v2] core: Expose HIDNormallyConnectable and HIDReconnectInitiate properties

Hi,

On Mon, Apr 1, 2013 at 2:48 PM, Marcel Holtmann <[email protected]> wrote:
> lets split doc patches from code patches please.
ok

> Do we want to keep the terms used in the HID spec or come up with a bit more generic property names here?

I think these two properties have a very particular behavior defined
by the HID spec. Using a simpler name (like simply "connectable") will
add more noise.


> We slowly want to move over to using bool instead of gboolean. So I would prefer that any new code starts with this. And for extra points, patches that fix up the old usages are more than welcome.
ok

> Don't we have a general helper for this? We might should. Or should the input plugin store its specific values in its own file?

The %s/%s/info filename is created in 6 different places in the code.
I could add a new helper for this logic instead of having the filename
scheme spread in the code. I don't mind having a separate file for the
input plugin


> I think you need callbacks for the case when the values are not available. Otherwise the GDBus core behaves funny when attempting to get all properties.
Yes, you are right. But, in any case, I'll remove the
hid_props_present since the input interface is created passing a
sdp_record_t, so the properties will be always present. One of the
properties is mandatory (so it should be present in the SDP record)
and the other is optional and assumed as false if not present. Doesn't
make sense to move the HID spec logic to the user/client when the
properties are not present.
Even in the case of an upgrade from bluez 4 the sdp records are
correctly parsed from the old format.

Thanks for the comments!
Alex

2013-04-01 21:48:30

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH v2] core: Expose HIDNormallyConnectable and HIDReconnectInitiate properties

Hi Alex,

> 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 these two HID properties as Device properties, updates
> them every time new SDP records are available and persists them in the
> device's info file. This patch also shows this information in the
> bluetoothctl "info <dev>" command.
> For non-HID devices and when no information is available (i.e. a cached
> device after upgrade from a version without this patch) both properties
> are set to the comon case: HIDNormallyConnectable is FALSE and
> HIDReconnectInitiate is TRUE.
> ---
> doc/input-api.txt | 29 ++++++++
> profiles/input/device.c | 175 ++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 204 insertions(+)
> create mode 100644 doc/input-api.txt

lets split doc patches from code patches please.

> diff --git a/doc/input-api.txt b/doc/input-api.txt
> new file mode 100644
> index 0000000..dffd9fb
> --- /dev/null
> +++ b/doc/input-api.txt
> @@ -0,0 +1,29 @@
> +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 boolean NormallyConnectable [readonly, optional]
> +
> + Exposes the value of HIDNormallyConnectable as defined
> + by the HID profile specification. If this optional value
> + is not specified by the HID device, the value is asumend
> + as FALSE.
> +
> + If the value of the property is unknown, for example
> + upgrading from a previous version of BlueZ, the property
> + is missing.
> +
> + boolean ConnectInitiate [readonly, optional]
> +
> + Exposes the value of ConnectInitiate as defined by the
> + HID profile specification.
> +
> + If the value of the property is unknown, for example
> + upgrading from a previous version of BlueZ, the property
> + is missing.

Do we want to keep the terms used in the HID spec or come up with a bit more generic property names here?

> diff --git a/profiles/input/device.c b/profiles/input/device.c
> index 1da9d99..dd2a3ab 100644
> --- a/profiles/input/device.c
> +++ b/profiles/input/device.c
> @@ -54,6 +54,8 @@
>
> #include "sdp-client.h"
>
> +#define INPUT_INTERFACE "org.bluez.Input1"
> +
> struct input_device {
> struct btd_device *device;
> char *path;
> @@ -71,6 +73,9 @@ struct input_device {
> guint dc_id;
> gboolean disable_sdp;
> char *name;
> + gboolean hid_props_present;
> + gboolean normally_connectable;
> + gboolean reconnect_initiate;
> };

We slowly want to move over to using bool instead of gboolean. So I would prefer that any new code starts with this. And for extra points, patches that fix up the old usages are more than welcome.

> static GSList *devices = NULL;
> @@ -116,6 +121,83 @@ static void input_device_free(struct input_device *idev)
> g_free(idev);
> }
>
> +static void input_device_config_filename(struct input_device *idev,
> + char *filename, size_t maxlen)
> +{
> + char adapter_addr[18];
> + char device_addr[18];
> +
> + ba2str(device_get_address(idev->device), device_addr);
> + ba2str(adapter_get_address(device_get_adapter(idev->device)),
> + adapter_addr);
> + snprintf(filename, maxlen, STORAGEDIR "/%s/%s/info", adapter_addr,
> + device_addr);
> + filename[maxlen-1] = '\0';
> +}

Don't we have a general helper for this? We might should. Or should the input plugin store its specific values in its own file?

> +
> +static void input_device_load_props(struct input_device *idev)
> +{
> + char filename[PATH_MAX + 1];
> + GKeyFile *key_file;
> + GError *err = NULL;
> +
> + input_device_config_filename(idev, filename, PATH_MAX + 1);
> +
> + key_file = g_key_file_new();
> + if (!key_file)
> + return;
> + g_key_file_load_from_file(key_file, filename, 0, NULL);
> +
> + /* Load HID connectivity properties */
> + idev->normally_connectable = g_key_file_get_boolean(key_file,
> + "Input", "NormallyConnectable", &err);
> + if (err) {
> + idev->normally_connectable = FALSE;
> + g_error_free(err);
> + err = NULL;
> + } else
> + idev->hid_props_present = TRUE;
> +
> + idev->reconnect_initiate = g_key_file_get_boolean(key_file,
> + "Input", "ReconnectInitiate", &err);
> + if (err) {
> + idev->reconnect_initiate = TRUE;
> + g_error_free(err);
> + err = NULL;
> + } else
> + idev->hid_props_present = TRUE;
> +
> + g_key_file_free(key_file);
> +}
> +
> +static void input_device_store_props(struct input_device *idev)
> +{
> + char filename[PATH_MAX + 1];
> + GKeyFile *key_file;
> + char *str;
> + gsize length = 0;
> +
> + input_device_config_filename(idev, filename, PATH_MAX + 1);
> +
> + key_file = g_key_file_new();
> + if (!key_file)
> + return;
> + g_key_file_load_from_file(key_file, filename, 0, NULL);
> +
> + g_key_file_set_boolean(key_file, "Input",
> + "NormallyConnectable", idev->normally_connectable);
> + g_key_file_set_boolean(key_file, "Input",
> + "ReconnectInitiate", idev->reconnect_initiate);
> +
> + create_file(filename, S_IRUSR | S_IWUSR);
> +
> + str = g_key_file_to_data(key_file, &length, NULL);
> + g_file_set_contents(filename, str, length, NULL);
> + g_free(str);
> +
> + g_key_file_free(key_file);
> +}
> +
> static gboolean intr_watch_cb(GIOChannel *chan, GIOCondition cond, gpointer data)
> {
> struct input_device *idev = data;
> @@ -694,6 +776,9 @@ static struct input_device *input_device_new(struct btd_device *device,
> if (strlen(name) > 0)
> idev->name = g_strdup(name);
>
> + idev->hid_props_present = FALSE;
> + input_device_load_props(idev);
> +
> return idev;
> }
>
> @@ -706,6 +791,81 @@ static gboolean is_device_sdp_disable(const sdp_record_t *rec)
> return data && data->val.uint8;
> }
>
> +static void update_hid_props(struct input_device *idev,
> + gboolean normally_connectable, gboolean reconnect_initiate)
> +{
> + gboolean update_nc, update_ri;
> + DBusConnection *dbus_conn = btd_get_dbus_connection();
> +
> + update_nc = !idev->hid_props_present ||
> + idev->normally_connectable != normally_connectable;
> + idev->normally_connectable = normally_connectable;
> + if (update_nc)
> + g_dbus_emit_property_changed(dbus_conn, idev->path,
> + DEVICE_INTERFACE, "HIDNormallyConnectable");
> +
> + update_ri = !idev->hid_props_present ||
> + idev->reconnect_initiate != reconnect_initiate;
> + idev->reconnect_initiate = reconnect_initiate;
> + if (update_ri)
> + g_dbus_emit_property_changed(dbus_conn, idev->path,
> + DEVICE_INTERFACE, "HIDReconnectInitiate");
> +
> + idev->hid_props_present = TRUE;
> + if (update_nc || update_ri)
> + input_device_store_props(idev);
> +}
> +
> +static void extract_hid_props(struct input_device *idev,
> + const sdp_record_t *rec)
> +{
> + /* Extract HID connectability */
> + bool hid_nc, hid_ri;
> + sdp_data_t *pdlist;
> +
> + /* HIDNormallyConnectable is optional and assumed FALSE
> + * if not present. */
> + pdlist = sdp_data_get(rec, SDP_ATTR_HID_NORMALLY_CONNECTABLE);
> + hid_nc = pdlist ? pdlist->val.uint8 : FALSE;
> +
> + pdlist = sdp_data_get(rec, SDP_ATTR_HID_RECONNECT_INITIATE);
> + hid_ri = pdlist ? pdlist->val.uint8 : TRUE;
> +
> + update_hid_props(idev, hid_nc, hid_ri);
> +}
> +
> +static gboolean property_get_normally_connectable(
> + const GDBusPropertyTable *property,
> + DBusMessageIter *iter, void *data)
> +{
> + struct input_device *idev = data;
> +
> + if (idev->hid_props_present)
> + dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN,
> + &idev->normally_connectable);
> +
> + return TRUE;
> +}
> +
> +static gboolean property_get_reconnect_initiate(
> + const GDBusPropertyTable *property,
> + DBusMessageIter *iter, void *data)
> +{
> + struct input_device *idev = data;
> +
> + if (idev->hid_props_present)
> + dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN,
> + &idev->reconnect_initiate);
> +
> + return TRUE;
> +}
> +
> +static const GDBusPropertyTable input_properties[] = {
> + { "NormallyConnectable", "b", property_get_normally_connectable },
> + { "ReconnectInitiate", "b", property_get_reconnect_initiate },
> + { }
> +};
> +

I think you need callbacks for the case when the values are not available. Otherwise the GDBus core behaves funny when attempting to get all properties.

> int input_device_register(struct btd_device *device,
> const char *path, const char *uuid,
> const sdp_record_t *rec, int timeout)
> @@ -723,6 +883,18 @@ int input_device_register(struct btd_device *device,
> if (!idev)
> return -EINVAL;
>
> + 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;
> + }
> +
> + extract_hid_props(idev, rec);
> +
> idev->timeout = timeout;
> idev->uuid = g_strdup(uuid);
>
> @@ -761,6 +933,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);

Regards

Marcel


2013-04-01 21:39:51

by Alex Deymo

[permalink] [raw]
Subject: [PATCH v2] core: Expose HIDNormallyConnectable and HIDReconnectInitiate properties

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 these two HID properties as Device properties, updates
them every time new SDP records are available and persists them in the
device's info file. This patch also shows this information in the
bluetoothctl "info <dev>" command.
For non-HID devices and when no information is available (i.e. a cached
device after upgrade from a version without this patch) both properties
are set to the comon case: HIDNormallyConnectable is FALSE and
HIDReconnectInitiate is TRUE.
---
doc/input-api.txt | 29 ++++++++
profiles/input/device.c | 175 ++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 204 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..dffd9fb
--- /dev/null
+++ b/doc/input-api.txt
@@ -0,0 +1,29 @@
+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 boolean NormallyConnectable [readonly, optional]
+
+ Exposes the value of HIDNormallyConnectable as defined
+ by the HID profile specification. If this optional value
+ is not specified by the HID device, the value is asumend
+ as FALSE.
+
+ If the value of the property is unknown, for example
+ upgrading from a previous version of BlueZ, the property
+ is missing.
+
+ boolean ConnectInitiate [readonly, optional]
+
+ Exposes the value of ConnectInitiate as defined by the
+ HID profile specification.
+
+ If the value of the property is unknown, for example
+ upgrading from a previous version of BlueZ, the property
+ is missing.
diff --git a/profiles/input/device.c b/profiles/input/device.c
index 1da9d99..dd2a3ab 100644
--- a/profiles/input/device.c
+++ b/profiles/input/device.c
@@ -54,6 +54,8 @@

#include "sdp-client.h"

+#define INPUT_INTERFACE "org.bluez.Input1"
+
struct input_device {
struct btd_device *device;
char *path;
@@ -71,6 +73,9 @@ struct input_device {
guint dc_id;
gboolean disable_sdp;
char *name;
+ gboolean hid_props_present;
+ gboolean normally_connectable;
+ gboolean reconnect_initiate;
};

static GSList *devices = NULL;
@@ -116,6 +121,83 @@ static void input_device_free(struct input_device *idev)
g_free(idev);
}

+static void input_device_config_filename(struct input_device *idev,
+ char *filename, size_t maxlen)
+{
+ char adapter_addr[18];
+ char device_addr[18];
+
+ ba2str(device_get_address(idev->device), device_addr);
+ ba2str(adapter_get_address(device_get_adapter(idev->device)),
+ adapter_addr);
+ snprintf(filename, maxlen, STORAGEDIR "/%s/%s/info", adapter_addr,
+ device_addr);
+ filename[maxlen-1] = '\0';
+}
+
+static void input_device_load_props(struct input_device *idev)
+{
+ char filename[PATH_MAX + 1];
+ GKeyFile *key_file;
+ GError *err = NULL;
+
+ input_device_config_filename(idev, filename, PATH_MAX + 1);
+
+ key_file = g_key_file_new();
+ if (!key_file)
+ return;
+ g_key_file_load_from_file(key_file, filename, 0, NULL);
+
+ /* Load HID connectivity properties */
+ idev->normally_connectable = g_key_file_get_boolean(key_file,
+ "Input", "NormallyConnectable", &err);
+ if (err) {
+ idev->normally_connectable = FALSE;
+ g_error_free(err);
+ err = NULL;
+ } else
+ idev->hid_props_present = TRUE;
+
+ idev->reconnect_initiate = g_key_file_get_boolean(key_file,
+ "Input", "ReconnectInitiate", &err);
+ if (err) {
+ idev->reconnect_initiate = TRUE;
+ g_error_free(err);
+ err = NULL;
+ } else
+ idev->hid_props_present = TRUE;
+
+ g_key_file_free(key_file);
+}
+
+static void input_device_store_props(struct input_device *idev)
+{
+ char filename[PATH_MAX + 1];
+ GKeyFile *key_file;
+ char *str;
+ gsize length = 0;
+
+ input_device_config_filename(idev, filename, PATH_MAX + 1);
+
+ key_file = g_key_file_new();
+ if (!key_file)
+ return;
+ g_key_file_load_from_file(key_file, filename, 0, NULL);
+
+ g_key_file_set_boolean(key_file, "Input",
+ "NormallyConnectable", idev->normally_connectable);
+ g_key_file_set_boolean(key_file, "Input",
+ "ReconnectInitiate", idev->reconnect_initiate);
+
+ create_file(filename, S_IRUSR | S_IWUSR);
+
+ str = g_key_file_to_data(key_file, &length, NULL);
+ g_file_set_contents(filename, str, length, NULL);
+ g_free(str);
+
+ g_key_file_free(key_file);
+}
+
static gboolean intr_watch_cb(GIOChannel *chan, GIOCondition cond, gpointer data)
{
struct input_device *idev = data;
@@ -694,6 +776,9 @@ static struct input_device *input_device_new(struct btd_device *device,
if (strlen(name) > 0)
idev->name = g_strdup(name);

+ idev->hid_props_present = FALSE;
+ input_device_load_props(idev);
+
return idev;
}

@@ -706,6 +791,81 @@ static gboolean is_device_sdp_disable(const sdp_record_t *rec)
return data && data->val.uint8;
}

+static void update_hid_props(struct input_device *idev,
+ gboolean normally_connectable, gboolean reconnect_initiate)
+{
+ gboolean update_nc, update_ri;
+ DBusConnection *dbus_conn = btd_get_dbus_connection();
+
+ update_nc = !idev->hid_props_present ||
+ idev->normally_connectable != normally_connectable;
+ idev->normally_connectable = normally_connectable;
+ if (update_nc)
+ g_dbus_emit_property_changed(dbus_conn, idev->path,
+ DEVICE_INTERFACE, "HIDNormallyConnectable");
+
+ update_ri = !idev->hid_props_present ||
+ idev->reconnect_initiate != reconnect_initiate;
+ idev->reconnect_initiate = reconnect_initiate;
+ if (update_ri)
+ g_dbus_emit_property_changed(dbus_conn, idev->path,
+ DEVICE_INTERFACE, "HIDReconnectInitiate");
+
+ idev->hid_props_present = TRUE;
+ if (update_nc || update_ri)
+ input_device_store_props(idev);
+}
+
+static void extract_hid_props(struct input_device *idev,
+ const sdp_record_t *rec)
+{
+ /* Extract HID connectability */
+ bool hid_nc, hid_ri;
+ sdp_data_t *pdlist;
+
+ /* HIDNormallyConnectable is optional and assumed FALSE
+ * if not present. */
+ pdlist = sdp_data_get(rec, SDP_ATTR_HID_NORMALLY_CONNECTABLE);
+ hid_nc = pdlist ? pdlist->val.uint8 : FALSE;
+
+ pdlist = sdp_data_get(rec, SDP_ATTR_HID_RECONNECT_INITIATE);
+ hid_ri = pdlist ? pdlist->val.uint8 : TRUE;
+
+ update_hid_props(idev, hid_nc, hid_ri);
+}
+
+static gboolean property_get_normally_connectable(
+ const GDBusPropertyTable *property,
+ DBusMessageIter *iter, void *data)
+{
+ struct input_device *idev = data;
+
+ if (idev->hid_props_present)
+ dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN,
+ &idev->normally_connectable);
+
+ return TRUE;
+}
+
+static gboolean property_get_reconnect_initiate(
+ const GDBusPropertyTable *property,
+ DBusMessageIter *iter, void *data)
+{
+ struct input_device *idev = data;
+
+ if (idev->hid_props_present)
+ dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN,
+ &idev->reconnect_initiate);
+
+ return TRUE;
+}
+
+static const GDBusPropertyTable input_properties[] = {
+ { "NormallyConnectable", "b", property_get_normally_connectable },
+ { "ReconnectInitiate", "b", property_get_reconnect_initiate },
+ { }
+};
+
int input_device_register(struct btd_device *device,
const char *path, const char *uuid,
const sdp_record_t *rec, int timeout)
@@ -723,6 +883,18 @@ int input_device_register(struct btd_device *device,
if (!idev)
return -EINVAL;

+ 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;
+ }
+
+ extract_hid_props(idev, rec);
+
idev->timeout = timeout;
idev->uuid = g_strdup(uuid);

@@ -761,6 +933,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