Re: [PATCHv3] hog: Use HoG device name as uHID input device name

Re: [PATCHv3] hog: Use HoG device name as uHID input device name

[Petri Gynther]

Any comments on this patch?

In uHID kernel driver, uhid_dev_create() does:
  ...
  strncpy(hid->name, ev->u.create.name, 127);
  hid->name[127] = 0;
  strncpy(hid->phys, ev->u.create.phys, 63);
  hid->phys[63] = 0;
  strncpy(hid->uniq, ev->u.create.uniq, 63);
  hid->uniq[63] = 0;

Also, BlueZ already does this with HID input devices.
input_device_new() copies Bluetooth device name into idev->name. And
then, hidp_add_connection() copies idev->name to req->name and passes
it to kernel. In kernel, hidp_setup_hid() populates hid->name,
hid->phys, and hid->uniq.

On Wed, Jan 29, 2014 at 7:55 PM, Petri Gynther <pgynther@google.com> wrote:
> If HoG BLE device name is known, use it when creating uHID input device.
> Pass adapter and device addresses to uHID as well.
> ---
>  profiles/input/hog.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/profiles/input/hog.c b/profiles/input/hog.c
> index ded6303..030dc91 100644
> --- a/profiles/input/hog.c
> +++ b/profiles/input/hog.c
> @@ -392,7 +392,15 @@ static void report_map_read_cb(guint8 status, const guint8 *pdu, guint16 plen,
>         /* create uHID device */
>         memset(&ev, 0, sizeof(ev));
>         ev.type = UHID_CREATE;
> -       strcpy((char *) ev.u.create.name, "bluez-hog-device");
> +       if (device_name_known(hogdev->device)) {
> +               device_get_name(hogdev->device, (char *) ev.u.create.name,
> +                               sizeof(ev.u.create.name) - 1);
> +       } else {
> +               strcpy((char *) ev.u.create.name, "bluez-hog-device");
> +       }
> +       ba2str(btd_adapter_get_address(device_get_adapter(hogdev->device)),
> +               (char *) ev.u.create.phys);
> +       ba2str(device_get_address(hogdev->device), (char *) ev.u.create.uniq);
>         ev.u.create.vendor = vendor;
>         ev.u.create.product = product;
>         ev.u.create.version = version;
> --
> 1.8.5.3
>

Re: [PATCHv3] hog: Use HoG device name as uHID input device name

[Petri Gynther]

Hi Johan,

On Thu, Feb 13, 2014 at 1:19 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Petri,
>
> On Wed, Jan 29, 2014, Petri Gynther wrote:
>> If HoG BLE device name is known, use it when creating uHID input device.
>> Pass adapter and device addresses to uHID as well.
>
> Could you perhaps elaborate a bit what exactly the "uniq" and "phys"
> members are in the request (possibly with a reference to the uHID
> documentation). That would make it easier to verify that the content
> you're adding to them makes sense.
>

>From linux/include/linux/hid.h:
struct hid_device {
  ...
  char name[128];    /* Device name */
  char phys[64];        /* Device physical location */
  char uniq[64];         /* Device unique identifier (serial #) */

So, for a Bluetooth HID or HoG device, BT adapter MAC address is good
fit for "phys" and device MAC address is good for "uniq". This is what
hidp module in kernel already does. But, we can be more descriptive
here if we want. These values are readable in sysfs under
/sys/devices/virtual/input/<input-device>.

>> +     if (device_name_known(hogdev->device)) {
>> +             device_get_name(hogdev->device, (char *) ev.u.create.name,
>> +                             sizeof(ev.u.create.name) - 1);
>> +     } else {
>> +             strcpy((char *) ev.u.create.name, "bluez-hog-device");
>> +     }
>
> We don't use { } for single line branches, so you can drop these here.
>

I'll fix this.

>> +     ba2str(btd_adapter_get_address(device_get_adapter(hogdev->device)),
>> +             (char *) ev.u.create.phys);
>
> This is getting a bit messy with the nested function calls. I'd add
> separate adapter variable here to make this a bit more readable.
>
> Also note our coding style wrt split lines: indent the continuation
> lines as much as you can (with tabs) as long as it's under 80 chars.
>
> Also, it might be good to split this up into two patches since you've
> essentially got two independent improvements here. One is making sure
> the name member contains something more useful and the other is adding
> some content to the uniq and phys members.

I'll add name, phys, and uniq variables in struct hog_device, so that
they are easy to pass to uHID when it is time to create the uHID
device.

>
> Johan

Re: [PATCHv3] hog: Use HoG device name as uHID input device name

[Johan Hedberg]

Hi Petri,

On Wed, Jan 29, 2014, Petri Gynther wrote:
> If HoG BLE device name is known, use it when creating uHID input device.
> Pass adapter and device addresses to uHID as well.

Could you perhaps elaborate a bit what exactly the "uniq" and "phys"
members are in the request (possibly with a reference to the uHID
documentation). That would make it easier to verify that the content
you're adding to them makes sense.

> +	if (device_name_known(hogdev->device)) {
> +		device_get_name(hogdev->device, (char *) ev.u.create.name,
> +				sizeof(ev.u.create.name) - 1);
> +	} else {
> +		strcpy((char *) ev.u.create.name, "bluez-hog-device");
> +	}

We don't use { } for single line branches, so you can drop these here.

> +	ba2str(btd_adapter_get_address(device_get_adapter(hogdev->device)),
> +		(char *) ev.u.create.phys);

This is getting a bit messy with the nested function calls. I'd add
separate adapter variable here to make this a bit more readable.

Also note our coding style wrt split lines: indent the continuation
lines as much as you can (with tabs) as long as it's under 80 chars.

Also, it might be good to split this up into two patches since you've
essentially got two independent improvements here. One is making sure
the name member contains something more useful and the other is adding
some content to the uniq and phys members.

Johan

[PATCHv3] hog: Use HoG device name as uHID input device name

[Petri Gynther]

If HoG BLE device name is known, use it when creating uHID input device.
Pass adapter and device addresses to uHID as well.
---
 profiles/input/hog.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/profiles/input/hog.c b/profiles/input/hog.c
index ded6303..030dc91 100644
--- a/profiles/input/hog.c
+++ b/profiles/input/hog.c
@@ -392,7 +392,15 @@ static void report_map_read_cb(guint8 status, const guint8 *pdu, guint16 plen,
 	/* create uHID device */
 	memset(&ev, 0, sizeof(ev));
 	ev.type = UHID_CREATE;
-	strcpy((char *) ev.u.create.name, "bluez-hog-device");
+	if (device_name_known(hogdev->device)) {
+		device_get_name(hogdev->device, (char *) ev.u.create.name,
+				sizeof(ev.u.create.name) - 1);
+	} else {
+		strcpy((char *) ev.u.create.name, "bluez-hog-device");
+	}
+	ba2str(btd_adapter_get_address(device_get_adapter(hogdev->device)),
+		(char *) ev.u.create.phys);
+	ba2str(device_get_address(hogdev->device), (char *) ev.u.create.uniq);
 	ev.u.create.vendor = vendor;
 	ev.u.create.product = product;
 	ev.u.create.version = version;
-- 
1.8.5.3