[PATCH 0/3] Dualshock4 controller enhancements

[PATCH 0/3] Dualshock4 controller enhancements

[Juha Kuikka]

This patch set contains enhancements for the Dualshock4 controller.

There are now two Dualshock4 PIDs out there, add second one to
special case handler.

Modify sixaxis plugin to add support for pairing DS4 controllers over USB.
The HID report also allows for the link key to be set but since there is
currently no API to set it from the plugins, this has not been implemented.

Because of this the DS4 will connect to the bluez host automatically but
the bonding still has to happen over the bluetooth connection as normal.

Also set connected controllers "paired" by default. This allows for hands-free
connection to bluez host.

Juha Kuikka (3):
  Add new DS4 controller PID into special case handler
  sixaxis: Add support for pairing DS4 over USB
  sixaxis: Set created devices trusted by default

 plugins/sixaxis.c       | 87 +++++++++++++++++++++++++++++++++++++------------
 profiles/input/server.c |  2 +-
 2 files changed, 68 insertions(+), 21 deletions(-)

-- 
1.9.1

[PATCH 1/3] Add new DS4 controller PID into special case handler

[Juha Kuikka]

There is a special path for various game controllers where they connect
to the hid service before bluetoothd knows what they are.

This patch adds another PID for the Dualshock4 controller.
---
 profiles/input/server.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/profiles/input/server.c b/profiles/input/server.c
index eb3fcf8..61f084a 100644
--- a/profiles/input/server.c
+++ b/profiles/input/server.c
@@ -136,7 +136,7 @@ static bool dev_is_sixaxis(const bdaddr_t *src, const bdaddr_t *dst)
 		return true;
 
 	/* DualShock 4 */
-	if (vid == 0x054c && pid == 0x05c4)
+	if (vid == 0x054c && (pid == 0x05c4 || pid == 0x09cc))
 		return true;
 
 	/* Navigation Controller */
-- 
1.9.1

[PATCH 2/3] sixaxis: Add support for pairing DS4 over USB

[Juha Kuikka]

This patch adds support for "pairing" a Dualshock4 controller over USB
into the sixaxis plugin.

Pairing is in quotes because we cannot do real bonding due to lack of
API of setting link keys from the plugin so instead we just tell the
controller that we are the master and to connect to us.

Actual bonding happens on first connection per usual.

This patch is based on information from sixpair tool.
---
 plugins/sixaxis.c | 86 ++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 66 insertions(+), 20 deletions(-)

diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
index fcc93bc..1fb2091 100644
--- a/plugins/sixaxis.c
+++ b/plugins/sixaxis.c
@@ -69,6 +69,20 @@ static const struct {
 		.pid = 0x042f,
 		.version = 0x0000,
 	},
+	{
+		.name = "Wireless Controller",
+		.source = 0x0002,
+		.vid = 0x054c,
+		.pid = 0x05c4,
+		.version = 0x0001,
+	},
+	{
+		.name = "Wireless Controller",
+		.source = 0x0002,
+		.vid = 0x054c,
+		.pid = 0x09cc,
+		.version = 0x0001,
+	},
 };
 
 struct leds_data {
@@ -86,57 +100,89 @@ static struct udev *ctx = NULL;
 static struct udev_monitor *monitor = NULL;
 static guint watch_id = 0;
 
-static int get_device_bdaddr(int fd, bdaddr_t *bdaddr)
+static bool is_dualshock4(int index)
+{
+	return devices[index].pid == 0x05c4 || devices[index].pid == 0x09cc;
+}
+
+static int get_device_bdaddr(int fd, int index, bdaddr_t *bdaddr)
 {
 	uint8_t buf[18];
-	int ret;
+	int ret, report_length;
 
 	memset(buf, 0, sizeof(buf));
 
-	buf[0] = 0xf2;
+	if (is_dualshock4(index)) {
+		report_length = 7;
+		buf[0] = 0x81;
+	} else {
+		report_length = 18;
+		buf[0] = 0xf2;
+	}
 
-	ret = ioctl(fd, HIDIOCGFEATURE(sizeof(buf)), buf);
+	ret = ioctl(fd, HIDIOCGFEATURE(report_length), buf);
 	if (ret < 0) {
 		error("sixaxis: failed to read device address (%s)",
 							strerror(errno));
 		return ret;
 	}
 
-	baswap(bdaddr, (bdaddr_t *) (buf + 4));
+	if (is_dualshock4(index))
+		memcpy(bdaddr->b, buf + 1, 6); // little-endian on DS4
+	else
+		baswap(bdaddr, (bdaddr_t *) (buf + 4));
 
 	return 0;
 }
 
-static int get_master_bdaddr(int fd, bdaddr_t *bdaddr)
+static int get_master_bdaddr(int fd, int index, bdaddr_t *bdaddr)
 {
-	uint8_t buf[8];
-	int ret;
+	uint8_t buf[16];
+	int ret, report_length;
 
 	memset(buf, 0, sizeof(buf));
 
-	buf[0] = 0xf5;
+	if (is_dualshock4(index)) {
+		report_length = 16;
+		buf[0] = 0x12;
+	} else {
+		report_length = 8;
+		buf[0] = 0xf5;
+	}
 
-	ret = ioctl(fd, HIDIOCGFEATURE(sizeof(buf)), buf);
+	ret = ioctl(fd, HIDIOCGFEATURE(report_length), buf);
 	if (ret < 0) {
 		error("sixaxis: failed to read master address (%s)",
 							strerror(errno));
 		return ret;
 	}
 
-	baswap(bdaddr, (bdaddr_t *) (buf + 2));
+	if (is_dualshock4(index))
+		memcpy(bdaddr->b, buf + 10, 6); // little-endian on DS4
+	else
+		baswap(bdaddr, (bdaddr_t *) (buf + 2));
 
 	return 0;
 }
 
-static int set_master_bdaddr(int fd, const bdaddr_t *bdaddr)
+static int set_master_bdaddr(int fd, int index, const bdaddr_t *bdaddr)
 {
-	uint8_t buf[8];
-	int ret;
+	uint8_t buf[23];
+	int ret, report_length;
 
-	buf[0] = 0xf5;
-	buf[1] = 0x01;
+	if (is_dualshock4(index)) {
+		report_length = 23;
 
-	baswap((bdaddr_t *) (buf + 2), bdaddr);
+		buf[0] = 0x13;
+		memcpy(buf + 1, bdaddr->b, 6);
+		memset(buf + 7, 0, 16); /* TODO: we could put the key here but there is no way to force a re-loading of link keys to the kernel */
+	} else {
+		report_length = 8;
+
+		buf[0] = 0xf5;
+		buf[1] = 0x01;
+		baswap((bdaddr_t *) (buf + 2), bdaddr);
+	}
 
 	ret = ioctl(fd, HIDIOCSFEATURE(sizeof(buf)), buf);
 	if (ret < 0)
@@ -262,10 +308,10 @@ static bool setup_device(int fd, int index, struct btd_adapter *adapter)
 	const bdaddr_t *adapter_bdaddr;
 	struct btd_device *device;
 
-	if (get_device_bdaddr(fd, &device_bdaddr) < 0)
+	if (get_device_bdaddr(fd, index, &device_bdaddr) < 0)
 		return false;
 
-	if (get_master_bdaddr(fd, &master_bdaddr) < 0)
+	if (get_master_bdaddr(fd, index, &master_bdaddr) < 0)
 		return false;
 
 	/* This can happen if controller was plugged while already connected
@@ -279,7 +325,7 @@ static bool setup_device(int fd, int index, struct btd_adapter *adapter)
 	adapter_bdaddr = btd_adapter_get_address(adapter);
 
 	if (bacmp(adapter_bdaddr, &master_bdaddr)) {
-		if (set_master_bdaddr(fd, adapter_bdaddr) < 0)
+		if (set_master_bdaddr(fd, index, adapter_bdaddr) < 0)
 			return false;
 	}
 
-- 
1.9.1

[PATCH 3/3] sixaxis: Set created devices trusted by default

[Juha Kuikka]

If user has enabled the sixaxis plugin and plugs a controller in,
it can be assumed they also want the controllers to be able to connect
automatically.
---
 plugins/sixaxis.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
index 1fb2091..ba96b54 100644
--- a/plugins/sixaxis.c
+++ b/plugins/sixaxis.c
@@ -349,6 +349,7 @@ static bool setup_device(int fd, int index, struct btd_adapter *adapter)
 	btd_device_set_pnpid(device, devices[index].source, devices[index].vid,
 				devices[index].pid, devices[index].version);
 	btd_device_set_temporary(device, false);
+	btd_device_set_trusted(device, true);
 
 	return true;
 }
-- 
1.9.1

Re: [PATCH 2/3] sixaxis: Add support for pairing DS4 over USB

[Antonio Ospite]

On Wed, 21 Dec 2016 21:20:53 -0800
Juha Kuikka <juha.kuikka@synapse.com> wrote:

> This patch adds support for "pairing" a Dualshock4 controller over USB
> into the sixaxis plugin.
>
> Pairing is in quotes because we cannot do real bonding due to lack of
> API of setting link keys from the plugin so instead we just tell the
> controller that we are the master and to connect to us.
>

Hi Juha,

I do not have a DS4, so it is not immediately clear to me what the
difference is between having or not having this USB "pairing" with DS4
controllers.

Could you please elaborate a little more? Thanks.
I thought people were using DS4s via BT just fine without it.

And what about setup_leds() for the DS4?  Will it still be called when
the DS4 shows up via BT?

Also consider CCing these developers when you send a possible next
iteration, they worked on the DS4 kernel driver and could be interested:

Frank Praznik <frank.praznik@gmail.com>
Roderick Colenbrander <roderick.colenbrander@sony.com>

Maybe mention BlueZ in the title of the cover letter when you do that,
to make clearer to them that the changes are not about the kernel
driver.

Some comments inlined below.

> Actual bonding happens on first connection per usual.
> 
> This patch is based on information from sixpair tool.

Is there an official link to that?

> ---
>  plugins/sixaxis.c | 86 ++++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 66 insertions(+), 20 deletions(-)
> 
> diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
> index fcc93bc..1fb2091 100644
> --- a/plugins/sixaxis.c
> +++ b/plugins/sixaxis.c
> @@ -69,6 +69,20 @@ static const struct {
>  		.pid = 0x042f,
>  		.version = 0x0000,
>  	},
> +	{
> +		.name = "Wireless Controller",
> +		.source = 0x0002,
> +		.vid = 0x054c,
> +		.pid = 0x05c4,
> +		.version = 0x0001,

If these devices have a different pairing mechanism maybe we can set
some function pointers here, and use a "callback" approach to handle it.

This would avoid having a lot of if...else in the functions below.

Something like:
		get_device_bdaddr = ds4_get_device_bdaddr,
		get_master_bdaddr = ds4_get_master_bdaddr,
		set_device_bdaddr = ds4_set_device_bdaddr,
> +	},
> +	{
> +		.name = "Wireless Controller",
> +		.source = 0x0002,
> +		.vid = 0x054c,
> +		.pid = 0x09cc,
> +		.version = 0x0001,
> +	},
>  };
>  
>  struct leds_data {
> @@ -86,57 +100,89 @@ static struct udev *ctx = NULL;
>  static struct udev_monitor *monitor = NULL;
>  static guint watch_id = 0;
>  
> -static int get_device_bdaddr(int fd, bdaddr_t *bdaddr)

This function will just become sixaxis_get_device_bdaddr, and a new
function called ds4_get_device_bdaddr will implement the DS4 paring
which uses a different feature report, with a different size and
structure.

This looks cleaner to me.

> +static bool is_dualshock4(int index)
> +{
> +	return devices[index].pid == 0x05c4 || devices[index].pid == 0x09cc;
> +}

this will not be needed,

> +
> +static int get_device_bdaddr(int fd, int index, bdaddr_t *bdaddr)

and neither changing the function parameters.

>  {
>  	uint8_t buf[18];
> -	int ret;
> +	int ret, report_length;
>  
>  	memset(buf, 0, sizeof(buf));
>  
> -	buf[0] = 0xf2;
> +	if (is_dualshock4(index)) {
> +		report_length = 7;
> +		buf[0] = 0x81;
> +	} else {
> +		report_length = 18;
> +		buf[0] = 0xf2;
> +	}
>  
> -	ret = ioctl(fd, HIDIOCGFEATURE(sizeof(buf)), buf);
> +	ret = ioctl(fd, HIDIOCGFEATURE(report_length), buf);
>  	if (ret < 0) {
>  		error("sixaxis: failed to read device address (%s)",
>  							strerror(errno));
>  		return ret;
>  	}
>  
> -	baswap(bdaddr, (bdaddr_t *) (buf + 4));
> +	if (is_dualshock4(index))
> +		memcpy(bdaddr->b, buf + 1, 6); // little-endian on DS4
> +	else
> +		baswap(bdaddr, (bdaddr_t *) (buf + 4));
>  
>  	return 0;
>  }
>  
> -static int get_master_bdaddr(int fd, bdaddr_t *bdaddr)
> +static int get_master_bdaddr(int fd, int index, bdaddr_t *bdaddr)

ditto.

>  {
> -	uint8_t buf[8];
> -	int ret;
> +	uint8_t buf[16];
> +	int ret, report_length;
>  
>  	memset(buf, 0, sizeof(buf));
>  
> -	buf[0] = 0xf5;
> +	if (is_dualshock4(index)) {
> +		report_length = 16;
> +		buf[0] = 0x12;
> +	} else {
> +		report_length = 8;
> +		buf[0] = 0xf5;
> +	}
>  
> -	ret = ioctl(fd, HIDIOCGFEATURE(sizeof(buf)), buf);
> +	ret = ioctl(fd, HIDIOCGFEATURE(report_length), buf);
>  	if (ret < 0) {
>  		error("sixaxis: failed to read master address (%s)",
>  							strerror(errno));
>  		return ret;
>  	}
>  
> -	baswap(bdaddr, (bdaddr_t *) (buf + 2));
> +	if (is_dualshock4(index))
> +		memcpy(bdaddr->b, buf + 10, 6); // little-endian on DS4
> +	else
> +		baswap(bdaddr, (bdaddr_t *) (buf + 2));
>  
>  	return 0;
>  }
>  
> -static int set_master_bdaddr(int fd, const bdaddr_t *bdaddr)
> +static int set_master_bdaddr(int fd, int index, const bdaddr_t *bdaddr)

ditto.

>  {
> -	uint8_t buf[8];
> -	int ret;
> +	uint8_t buf[23];
> +	int ret, report_length;
>  
> -	buf[0] = 0xf5;
> -	buf[1] = 0x01;
> +	if (is_dualshock4(index)) {
> +		report_length = 23;
>  
> -	baswap((bdaddr_t *) (buf + 2), bdaddr);
> +		buf[0] = 0x13;
> +		memcpy(buf + 1, bdaddr->b, 6);
> +		memset(buf + 7, 0, 16); /* TODO: we could put the key here but there is no way to force a re-loading of link keys to the kernel */
> +	} else {
> +		report_length = 8;
> +
> +		buf[0] = 0xf5;
> +		buf[1] = 0x01;
> +		baswap((bdaddr_t *) (buf + 2), bdaddr);
> +	}
>  
>  	ret = ioctl(fd, HIDIOCSFEATURE(sizeof(buf)), buf);
>  	if (ret < 0)
> @@ -262,10 +308,10 @@ static bool setup_device(int fd, int index, struct btd_adapter *adapter)
>  	const bdaddr_t *adapter_bdaddr;
>  	struct btd_device *device;
>  
> -	if (get_device_bdaddr(fd, &device_bdaddr) < 0)
> +	if (get_device_bdaddr(fd, index, &device_bdaddr) < 0)
>  		return false;
>  
> -	if (get_master_bdaddr(fd, &master_bdaddr) < 0)
> +	if (get_master_bdaddr(fd, index, &master_bdaddr) < 0)
>  		return false;
>

Here the function pointers will be used, like:
	if (devices[index].get_device_bdaddr(fd, &device_bdaddr) < 0)
	...
	if (devices[index].get_master_bdaddr(fd, &master_bdaddr) < 0)

>  	/* This can happen if controller was plugged while already connected
> @@ -279,7 +325,7 @@ static bool setup_device(int fd, int index, struct btd_adapter *adapter)
>  	adapter_bdaddr = btd_adapter_get_address(adapter);
>  
>  	if (bacmp(adapter_bdaddr, &master_bdaddr)) {
> -		if (set_master_bdaddr(fd, adapter_bdaddr) < 0)
> +		if (set_master_bdaddr(fd, index, adapter_bdaddr) < 0)

and here too.

>  			return false;
>  	}
>  

If this approach sounds OK, send a preparatory patch which adds the
needed fields to the global struct and renames the sixaxis functions
maybe moving them before the definition of the devices[] array, and then
do the DS4 changes to add the new devices and callbacks.

Thanks,
   Antonio

-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: [PATCH 2/3] sixaxis: Add support for pairing DS4 over USB

[Juha Kuikka]

On Thu, Dec 22, 2016 at 2:24 AM, Antonio Ospite <ao2@ao2.it> wrote:
> On Wed, 21 Dec 2016 21:20:53 -0800
> Juha Kuikka <juha.kuikka@synapse.com> wrote:
>
>> This patch adds support for "pairing" a Dualshock4 controller over USB
>> into the sixaxis plugin.
>>
>> Pairing is in quotes because we cannot do real bonding due to lack of
>> API of setting link keys from the plugin so instead we just tell the
>> controller that we are the master and to connect to us.
>>
>
> Hi Juha,

Hi Antonio,

>
> I do not have a DS4, so it is not immediately clear to me what the
> difference is between having or not having this USB "pairing" with DS4
> controllers.
>
> Could you please elaborate a little more? Thanks.
> I thought people were using DS4s via BT just fine without it.

Certainly!

This is a convenience feature. Users often use the same controllers
with both their PS4 and a Linux gaming system. This feature would make
switching a controller back-and-forth between the systems more
convenient as there would be no need for keyboard and mouse to go
through the BT discovery and pairing process every time they change
systems, instead the user could just plug it in.

This is exactly how the PS4 pairs/bonds the controllers
(http://manuals.playstation.net/document/en/ps4/basic/usecontroller.html)
so it would mimic that functionality as well.

>
> And what about setup_leds() for the DS4?  Will it still be called when
> the DS4 shows up via BT?

Yes. The DS4 kernel driver exposes the LEDs. I will review this path as well.

> Also consider CCing these developers when you send a possible next
> iteration, they worked on the DS4 kernel driver and could be interested:
>
> Frank Praznik <frank.praznik@gmail.com>
> Roderick Colenbrander <roderick.colenbrander@sony.com>
>
> Maybe mention BlueZ in the title of the cover letter when you do that,
> to make clearer to them that the changes are not about the kernel
> driver.

Thank you for the suggestion, I will.

> Some comments inlined below.
>
>> Actual bonding happens on first connection per usual.
>>
>> This patch is based on information from sixpair tool.
>
> Is there an official link to that?

There are several links but I am not certain which one is the
original. I will dig around for this.
>
>> ---
>>  plugins/sixaxis.c | 86 ++++++++++++++++++++++++++++++++++++++++++-------------
>>  1 file changed, 66 insertions(+), 20 deletions(-)
>>
>> diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
>> index fcc93bc..1fb2091 100644
>> --- a/plugins/sixaxis.c
>> +++ b/plugins/sixaxis.c
>> @@ -69,6 +69,20 @@ static const struct {
>>               .pid = 0x042f,
>>               .version = 0x0000,
>>       },
>> +     {
>> +             .name = "Wireless Controller",
>> +             .source = 0x0002,
>> +             .vid = 0x054c,
>> +             .pid = 0x05c4,
>> +             .version = 0x0001,
>
> If these devices have a different pairing mechanism maybe we can set
> some function pointers here, and use a "callback" approach to handle it.
>
> This would avoid having a lot of if...else in the functions below.
>
> Something like:
>                 get_device_bdaddr = ds4_get_device_bdaddr,
>                 get_master_bdaddr = ds4_get_master_bdaddr,
>                 set_device_bdaddr = ds4_set_device_bdaddr,
>> +     },
>> +     {
>> +             .name = "Wireless Controller",
>> +             .source = 0x0002,
>> +             .vid = 0x054c,
>> +             .pid = 0x09cc,
>> +             .version = 0x0001,
>> +     },
>>  };
>>
>>  struct leds_data {
>> @@ -86,57 +100,89 @@ static struct udev *ctx = NULL;
>>  static struct udev_monitor *monitor = NULL;
>>  static guint watch_id = 0;
>>
>> -static int get_device_bdaddr(int fd, bdaddr_t *bdaddr)
>
> This function will just become sixaxis_get_device_bdaddr, and a new
> function called ds4_get_device_bdaddr will implement the DS4 paring
> which uses a different feature report, with a different size and
> structure.
>
> This looks cleaner to me.

I agree. I will refactor this.

>
>> +static bool is_dualshock4(int index)
>> +{
>> +     return devices[index].pid == 0x05c4 || devices[index].pid == 0x09cc;
>> +}
>
> this will not be needed,

Right.
>
>> +
>> +static int get_device_bdaddr(int fd, int index, bdaddr_t *bdaddr)
>
> and neither changing the function parameters.
>
>>  {
>>       uint8_t buf[18];
>> -     int ret;
>> +     int ret, report_length;
>>
>>       memset(buf, 0, sizeof(buf));
>>
>> -     buf[0] = 0xf2;
>> +     if (is_dualshock4(index)) {
>> +             report_length = 7;
>> +             buf[0] = 0x81;
>> +     } else {
>> +             report_length = 18;
>> +             buf[0] = 0xf2;
>> +     }
>>
>> -     ret = ioctl(fd, HIDIOCGFEATURE(sizeof(buf)), buf);
>> +     ret = ioctl(fd, HIDIOCGFEATURE(report_length), buf);
>>       if (ret < 0) {
>>               error("sixaxis: failed to read device address (%s)",
>>                                                       strerror(errno));
>>               return ret;
>>       }
>>
>> -     baswap(bdaddr, (bdaddr_t *) (buf + 4));
>> +     if (is_dualshock4(index))
>> +             memcpy(bdaddr->b, buf + 1, 6); // little-endian on DS4
>> +     else
>> +             baswap(bdaddr, (bdaddr_t *) (buf + 4));
>>
>>       return 0;
>>  }
>>
>> -static int get_master_bdaddr(int fd, bdaddr_t *bdaddr)
>> +static int get_master_bdaddr(int fd, int index, bdaddr_t *bdaddr)
>
> ditto.
>
>>  {
>> -     uint8_t buf[8];
>> -     int ret;
>> +     uint8_t buf[16];
>> +     int ret, report_length;
>>
>>       memset(buf, 0, sizeof(buf));
>>
>> -     buf[0] = 0xf5;
>> +     if (is_dualshock4(index)) {
>> +             report_length = 16;
>> +             buf[0] = 0x12;
>> +     } else {
>> +             report_length = 8;
>> +             buf[0] = 0xf5;
>> +     }
>>
>> -     ret = ioctl(fd, HIDIOCGFEATURE(sizeof(buf)), buf);
>> +     ret = ioctl(fd, HIDIOCGFEATURE(report_length), buf);
>>       if (ret < 0) {
>>               error("sixaxis: failed to read master address (%s)",
>>                                                       strerror(errno));
>>               return ret;
>>       }
>>
>> -     baswap(bdaddr, (bdaddr_t *) (buf + 2));
>> +     if (is_dualshock4(index))
>> +             memcpy(bdaddr->b, buf + 10, 6); // little-endian on DS4
>> +     else
>> +             baswap(bdaddr, (bdaddr_t *) (buf + 2));
>>
>>       return 0;
>>  }
>>
>> -static int set_master_bdaddr(int fd, const bdaddr_t *bdaddr)
>> +static int set_master_bdaddr(int fd, int index, const bdaddr_t *bdaddr)
>
> ditto.
>
>>  {
>> -     uint8_t buf[8];
>> -     int ret;
>> +     uint8_t buf[23];
>> +     int ret, report_length;
>>
>> -     buf[0] = 0xf5;
>> -     buf[1] = 0x01;
>> +     if (is_dualshock4(index)) {
>> +             report_length = 23;
>>
>> -     baswap((bdaddr_t *) (buf + 2), bdaddr);
>> +             buf[0] = 0x13;
>> +             memcpy(buf + 1, bdaddr->b, 6);
>> +             memset(buf + 7, 0, 16); /* TODO: we could put the key here but there is no way to force a re-loading of link keys to the kernel */

Here is an interesting possibility, we could pass the link key here so
the controller would be fully bonded after the USB connection but
currently there is no API to pass that key back to the core.

Do you think this would this be something worth looking into?

>> +     } else {
>> +             report_length = 8;
>> +
>> +             buf[0] = 0xf5;
>> +             buf[1] = 0x01;
>> +             baswap((bdaddr_t *) (buf + 2), bdaddr);
>> +     }
>>
>>       ret = ioctl(fd, HIDIOCSFEATURE(sizeof(buf)), buf);
>>       if (ret < 0)
>> @@ -262,10 +308,10 @@ static bool setup_device(int fd, int index, struct btd_adapter *adapter)
>>       const bdaddr_t *adapter_bdaddr;
>>       struct btd_device *device;
>>
>> -     if (get_device_bdaddr(fd, &device_bdaddr) < 0)
>> +     if (get_device_bdaddr(fd, index, &device_bdaddr) < 0)
>>               return false;
>>
>> -     if (get_master_bdaddr(fd, &master_bdaddr) < 0)
>> +     if (get_master_bdaddr(fd, index, &master_bdaddr) < 0)
>>               return false;
>>
>
> Here the function pointers will be used, like:
>         if (devices[index].get_device_bdaddr(fd, &device_bdaddr) < 0)
>         ...
>         if (devices[index].get_master_bdaddr(fd, &master_bdaddr) < 0)
>
>>       /* This can happen if controller was plugged while already connected
>> @@ -279,7 +325,7 @@ static bool setup_device(int fd, int index, struct btd_adapter *adapter)
>>       adapter_bdaddr = btd_adapter_get_address(adapter);
>>
>>       if (bacmp(adapter_bdaddr, &master_bdaddr)) {
>> -             if (set_master_bdaddr(fd, adapter_bdaddr) < 0)
>> +             if (set_master_bdaddr(fd, index, adapter_bdaddr) < 0)
>
> and here too.
>
>>                       return false;
>>       }
>>
>
> If this approach sounds OK, send a preparatory patch which adds the
> needed fields to the global struct and renames the sixaxis functions
> maybe moving them before the definition of the devices[] array, and then
> do the DS4 changes to add the new devices and callbacks.

Will do! Thank you for the review and suggestions.

Cheers,
 - Juha

Re: [PATCH 1/3] Add new DS4 controller PID into special case handler

[Szymon Janc]

Hi Juha,

On Wednesday, 21 December 2016 21:20:52 CET Juha Kuikka wrote:
> There is a special path for various game controllers where they connect
> to the hid service before bluetoothd knows what they are.
> 
> This patch adds another PID for the Dualshock4 controller.
> ---
>  profiles/input/server.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/profiles/input/server.c b/profiles/input/server.c
> index eb3fcf8..61f084a 100644
> --- a/profiles/input/server.c
> +++ b/profiles/input/server.c
> @@ -136,7 +136,7 @@ static bool dev_is_sixaxis(const bdaddr_t *src, const
> bdaddr_t *dst) return true;
> 
>  	/* DualShock 4 */
> -	if (vid == 0x054c && pid == 0x05c4)
> +	if (vid == 0x054c && (pid == 0x05c4 || pid == 0x09cc))
>  		return true;

Is this ID for new DS4 sold with PS4 pro? if so please add a comment.

> 
>  	/* Navigation Controller */


-- 
pozdrawiam
Szymon Janc

Re: [PATCH 3/3] sixaxis: Set created devices trusted by default

[Szymon Janc]

Hi Juha,

On Wednesday, 21 December 2016 21:20:54 CET Juha Kuikka wrote:
> If user has enabled the sixaxis plugin and plugs a controller in,
> it can be assumed they also want the controllers to be able to connect
> automatically.
> ---
>  plugins/sixaxis.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
> index 1fb2091..ba96b54 100644
> --- a/plugins/sixaxis.c
> +++ b/plugins/sixaxis.c
> @@ -349,6 +349,7 @@ static bool setup_device(int fd, int index, struct
> btd_adapter *adapter) btd_device_set_pnpid(device, devices[index].source,
> devices[index].vid, devices[index].pid, devices[index].version);
>  	btd_device_set_temporary(device, false);
> +	btd_device_set_trusted(device, true);
> 
>  	return true;
>  }

This was already NACKed in the past for security reasons (mostly due to 
possibility of fake device being trusted).


-- 
pozdrawiam
Szymon Janc

Re: [PATCH 2/3] sixaxis: Add support for pairing DS4 over USB

[Szymon Janc]

Hi Juha,

On Wednesday, 21 December 2016 21:20:53 CET Juha Kuikka wrote:
> This patch adds support for "pairing" a Dualshock4 controller over USB
> into the sixaxis plugin.
> 
> Pairing is in quotes because we cannot do real bonding due to lack of
> API of setting link keys from the plugin so instead we just tell the
> controller that we are the master and to connect to us.
> 
> Actual bonding happens on first connection per usual.
> 
> This patch is based on information from sixpair tool.
> ---
>  plugins/sixaxis.c | 86
> ++++++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 66
> insertions(+), 20 deletions(-)
> 
> diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
> index fcc93bc..1fb2091 100644
> --- a/plugins/sixaxis.c
> +++ b/plugins/sixaxis.c
> @@ -69,6 +69,20 @@ static const struct {
>  		.pid = 0x042f,
>  		.version = 0x0000,
>  	},
> +	{
> +		.name = "Wireless Controller",
> +		.source = 0x0002,
> +		.vid = 0x054c,
> +		.pid = 0x05c4,
> +		.version = 0x0001,
> +	},
> +	{
> +		.name = "Wireless Controller",
> +		.source = 0x0002,
> +		.vid = 0x054c,
> +		.pid = 0x09cc,
> +		.version = 0x0001,
> +	},
>  };
> 
>  struct leds_data {
> @@ -86,57 +100,89 @@ static struct udev *ctx = NULL;
>  static struct udev_monitor *monitor = NULL;
>  static guint watch_id = 0;
> 
> -static int get_device_bdaddr(int fd, bdaddr_t *bdaddr)
> +static bool is_dualshock4(int index)
> +{
> +	return devices[index].pid == 0x05c4 || devices[index].pid == 0x09cc;
> +}
> +
> +static int get_device_bdaddr(int fd, int index, bdaddr_t *bdaddr)
>  {

While passing index isn't that bad for differentiating pads it would be great 
to have dedicated get/set callbacks in devices[] structures. AFAIR there are 
some chinese DS3 counterfeits that don't work currently with sixaxis plugin 
that could possibly use custom calbacks.

What do you think about such approach?

>  	uint8_t buf[18];
> -	int ret;
> +	int ret, report_length;
> 
>  	memset(buf, 0, sizeof(buf));
> 
> -	buf[0] = 0xf2;
> +	if (is_dualshock4(index)) {
> +		report_length = 7;
> +		buf[0] = 0x81;
> +	} else {
> +		report_length = 18;
> +		buf[0] = 0xf2;
> +	}
> 
> -	ret = ioctl(fd, HIDIOCGFEATURE(sizeof(buf)), buf);
> +	ret = ioctl(fd, HIDIOCGFEATURE(report_length), buf);
>  	if (ret < 0) {
>  		error("sixaxis: failed to read device address (%s)",
>  							strerror(errno));
>  		return ret;
>  	}
> 
> -	baswap(bdaddr, (bdaddr_t *) (buf + 4));
> +	if (is_dualshock4(index))
> +		memcpy(bdaddr->b, buf + 1, 6); // little-endian on DS4
> +	else
> +		baswap(bdaddr, (bdaddr_t *) (buf + 4));
> 
>  	return 0;
>  }
> 
> -static int get_master_bdaddr(int fd, bdaddr_t *bdaddr)
> +static int get_master_bdaddr(int fd, int index, bdaddr_t *bdaddr)
>  {
> -	uint8_t buf[8];
> -	int ret;
> +	uint8_t buf[16];
> +	int ret, report_length;
> 
>  	memset(buf, 0, sizeof(buf));
> 
> -	buf[0] = 0xf5;
> +	if (is_dualshock4(index)) {
> +		report_length = 16;
> +		buf[0] = 0x12;
> +	} else {
> +		report_length = 8;
> +		buf[0] = 0xf5;
> +	}
> 
> -	ret = ioctl(fd, HIDIOCGFEATURE(sizeof(buf)), buf);
> +	ret = ioctl(fd, HIDIOCGFEATURE(report_length), buf);
>  	if (ret < 0) {
>  		error("sixaxis: failed to read master address (%s)",
>  							strerror(errno));
>  		return ret;
>  	}
> 
> -	baswap(bdaddr, (bdaddr_t *) (buf + 2));
> +	if (is_dualshock4(index))
> +		memcpy(bdaddr->b, buf + 10, 6); // little-endian on DS4

I'd prefer use of bacpy(). Also please use /* */ style comments.

> +	else
> +		baswap(bdaddr, (bdaddr_t *) (buf + 2));
> 
>  	return 0;
>  }
> 
> -static int set_master_bdaddr(int fd, const bdaddr_t *bdaddr)
> +static int set_master_bdaddr(int fd, int index, const bdaddr_t *bdaddr)
>  {
> -	uint8_t buf[8];
> -	int ret;
> +	uint8_t buf[23];
> +	int ret, report_length;
> 
> -	buf[0] = 0xf5;
> -	buf[1] = 0x01;
> +	if (is_dualshock4(index)) {
> +		report_length = 23;
> 
> -	baswap((bdaddr_t *) (buf + 2), bdaddr);
> +		buf[0] = 0x13;
> +		memcpy(buf + 1, bdaddr->b, 6);
> +		memset(buf + 7, 0, 16); /* TODO: we could put the key here but there 
is
> no way to force a re-loading of link keys to the kernel */ +	} else {

80 characters limit.

> +		report_length = 8;
> +
> +		buf[0] = 0xf5;
> +		buf[1] = 0x01;
> +		baswap((bdaddr_t *) (buf + 2), bdaddr);
> +	}
> 
>  	ret = ioctl(fd, HIDIOCSFEATURE(sizeof(buf)), buf);
>  	if (ret < 0)
> @@ -262,10 +308,10 @@ static bool setup_device(int fd, int index, struct
> btd_adapter *adapter) const bdaddr_t *adapter_bdaddr;
>  	struct btd_device *device;
> 
> -	if (get_device_bdaddr(fd, &device_bdaddr) < 0)
> +	if (get_device_bdaddr(fd, index, &device_bdaddr) < 0)
>  		return false;
> 
> -	if (get_master_bdaddr(fd, &master_bdaddr) < 0)
> +	if (get_master_bdaddr(fd, index, &master_bdaddr) < 0)
>  		return false;
> 
>  	/* This can happen if controller was plugged while already connected
> @@ -279,7 +325,7 @@ static bool setup_device(int fd, int index, struct
> btd_adapter *adapter) adapter_bdaddr = btd_adapter_get_address(adapter);
> 
>  	if (bacmp(adapter_bdaddr, &master_bdaddr)) {
> -		if (set_master_bdaddr(fd, adapter_bdaddr) < 0)
> +		if (set_master_bdaddr(fd, index, adapter_bdaddr) < 0)
>  			return false;
>  	}


-- 
pozdrawiam
Szymon Janc

Re: [PATCH 3/3] sixaxis: Set created devices trusted by default

[Bastien Nocera]

On Wed, 2016-12-28 at 17:07 +0100, Szymon Janc wrote:
> Hi Juha,
> 
> On Wednesday, 21 December 2016 21:20:54 CET Juha Kuikka wrote:
> > If user has enabled the sixaxis plugin and plugs a controller in,
> > it can be assumed they also want the controllers to be able to
> > connect
> > automatically.
> > ---
> >  plugins/sixaxis.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
> > index 1fb2091..ba96b54 100644
> > --- a/plugins/sixaxis.c
> > +++ b/plugins/sixaxis.c
> > @@ -349,6 +349,7 @@ static bool setup_device(int fd, int index,
> > struct
> > btd_adapter *adapter) btd_device_set_pnpid(device,
> > devices[index].source,
> > devices[index].vid, devices[index].pid, devices[index].version);
> >  	btd_device_set_temporary(device, false);
> > +	btd_device_set_trusted(device, true);
> > 
> >  	return true;
> >  }
> 
> This was already NACKed in the past for security reasons (mostly due
> to 
> possibility of fake device being trusted).

Juha, you could take a look at a way of getting this cleaned up and
merged:
https://www.spinics.net/lists/linux-bluetooth/msg62970.html
https://www.spinics.net/lists/linux-bluetooth/msg62967.html

I couldn't get a straight answer about the patch, as the developers
themselves seemed to disagree on how to solve the problem.

Cheers