This patch set first refactors the sixaxis plugin to more easily support
multiple controllers with different protocols.
Secondly support for pairing the Dualshock 4 controller over USB is added.
Last, the PID of the new DS4 controller is added into the input device special
case handler which allows this controller to be used.
Juha Kuikka (4):
sixaxis: Re-organize functions for refactoring
sixaxis: Refactor to add device-specific functions
sixaxis: Add support for pairing DS4 over USB
Add new DS4 controller PID into special case handler
plugins/sixaxis.c | 293 ++++++++++++++++++++++++++++++++----------------
profiles/input/server.c | 6 +-
2 files changed, 202 insertions(+), 97 deletions(-)
--
2.7.4
On Wed, 2016-12-28 at 16:32 -0800, 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. This new
> PID
> matches with the model number CUH-ZCT2U.
> ---
> profiles/input/server.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/profiles/input/server.c b/profiles/input/server.c
> index eb3fcf8..3576f2b 100644
> --- a/profiles/input/server.c
> +++ b/profiles/input/server.c
> @@ -135,8 +135,10 @@ static bool dev_is_sixaxis(const bdaddr_t *src,
> const bdaddr_t *dst)
> if (vid == 0x054c && pid == 0x0268)
> return true;
>
> - /* DualShock 4 */
> - if (vid == 0x054c && pid == 0x05c4)
> + /* DualShock 4 CUH-ZCT1U (PID 0x05c4)
> + * DualShock 4 CUH-ZCT2U (PID 0x09cc) (slim/pro)
> + */
> + if (vid == 0x054c && (pid == 0x05c4 || pid == 0x09cc))
> return true;
>
> /* Navigation Controller */
Might be nice to have the struct you currently have in the sixaxis
plugin in a shared header, so we don't need to open code this function.
For example:
typedef enum {
CABLE_PAIRING_SIXAXIS,
CABLE_PAIRING_DS4
} CablePairingType;
static struct {
int vid;
int pid;
CablePairingType type;
} cable_pairing_devices[] = {
...
};
The sixaxis plugin would know which functions to use from the type, the
input/server.c code would loop over the array to see whether it's a
cable pairing.
Looks fine otherwise.
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 set up the
interval device and tell the controller that we are the master and to
connect to us.
Actual bonding happens on first connection per usual. Note that this
requires an agent to confirm the request.
The DS4 allows setting the link key over USB so true bonding could
happen over that channel. However currently there is no API in
bluetoothd to allow for this.
This is a convenience feature for people who use the same controllers
with both their PS4 and a Linux gaming system. This feature makes
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 so it would
mimic that functionality as well.
This patch is based on DS4 supprt for sixpair tool by t0xicCode:
https://github.com/t0xicCode/sixpair/commit/975c38cb6cd61a2f0be350714f0f64cef5f0432c
I merely translated from raw libusb to hid operations.
---
plugins/sixaxis.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 84 insertions(+)
diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
index 99f4d0b..58e6c47 100644
--- a/plugins/sixaxis.c
+++ b/plugins/sixaxis.c
@@ -392,6 +392,70 @@ static int sixaxis_post_connect(int fd, struct udev_device *udevice)
g_io_channel_unref(io);
}
+static int ds4_get_device_bdaddr(int fd, bdaddr_t *bdaddr)
+{
+ uint8_t buf[7];
+ int ret;
+
+ memset(buf, 0, sizeof(buf));
+
+ buf[0] = 0x81;
+
+ ret = ioctl(fd, HIDIOCGFEATURE(sizeof(buf)), buf);
+ if (ret < 0) {
+ error("ds4: failed to read device address (%s)",
+ strerror(errno));
+ return ret;
+ }
+
+ /* address is little-endian on DS4 */
+ bacpy(bdaddr, (bdaddr_t*) (buf + 1));
+
+ return 0;
+}
+
+static int ds4_get_master_bdaddr(int fd, bdaddr_t *bdaddr)
+{
+ uint8_t buf[16];
+ int ret;
+
+ memset(buf, 0, sizeof(buf));
+
+ buf[0] = 0x12;
+
+ ret = ioctl(fd, HIDIOCGFEATURE(sizeof(buf)), buf);
+ if (ret < 0) {
+ error("ds4: failed to read master address (%s)",
+ strerror(errno));
+ return ret;
+ }
+
+ /* address is little-endian on DS4 */
+ bacpy(bdaddr, (bdaddr_t*) (buf + 10));
+
+ return 0;
+}
+
+static int ds4_set_master_bdaddr(int fd, const bdaddr_t *bdaddr)
+{
+ uint8_t buf[23];
+ int ret;
+
+ buf[0] = 0x13;
+ bacpy((bdaddr_t*) (buf + 1), bdaddr);
+ /* TODO: we could put the key here but
+ there is no way to force a re-loading
+ of link keys to the kernel from here. */
+ memset(buf + 7, 0, 16);
+
+ ret = ioctl(fd, HIDIOCSFEATURE(sizeof(buf)), buf);
+ if (ret < 0)
+ error("ds4: failed to write master address (%s)",
+ strerror(errno));
+
+ return ret;
+}
+
static const struct {
const char *name;
uint16_t source;
@@ -425,6 +489,26 @@ static const struct {
.set_master_bdaddr = sixaxis_set_master_bdaddr,
.post_connect = sixaxis_post_connect,
},
+ {
+ .name = "Wireless Controller",
+ .source = 0x0002,
+ .vid = 0x054c,
+ .pid = 0x05c4,
+ .version = 0x0001,
+ .get_device_bdaddr = ds4_get_device_bdaddr,
+ .get_master_bdaddr = ds4_get_master_bdaddr,
+ .set_master_bdaddr = ds4_set_master_bdaddr,
+ },
+ {
+ .name = "Wireless Controller",
+ .source = 0x0002,
+ .vid = 0x054c,
+ .pid = 0x09cc,
+ .version = 0x0001,
+ .get_device_bdaddr = ds4_get_device_bdaddr,
+ .get_master_bdaddr = ds4_get_master_bdaddr,
+ .set_master_bdaddr = ds4_set_master_bdaddr,
+ },
};
static bool setup_device(int fd, int index, struct btd_adapter *adapter)
--
2.7.4
In preparation for adding support for DS4 controller, refactor
address access methods per device type.
Also move the sixaxis controller LED setup into a "post_connect"
function.
---
plugins/sixaxis.c | 111 ++++++++++++++++++++++++++++++++----------------------
1 file changed, 65 insertions(+), 46 deletions(-)
diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
index d62c6db..99f4d0b 100644
--- a/plugins/sixaxis.c
+++ b/plugins/sixaxis.c
@@ -48,29 +48,6 @@
#include "src/log.h"
#include "src/shared/util.h"
-static const struct {
- const char *name;
- uint16_t source;
- uint16_t vid;
- uint16_t pid;
- uint16_t version;
-} devices[] = {
- {
- .name = "PLAYSTATION(R)3 Controller",
- .source = 0x0002,
- .vid = 0x054c,
- .pid = 0x0268,
- .version = 0x0000,
- },
- {
- .name = "Navigation Controller",
- .source = 0x0002,
- .vid = 0x054c,
- .pid = 0x042f,
- .version = 0x0000,
- },
-};
-
struct leds_data {
char *syspath_prefix;
uint8_t bitmap;
@@ -86,7 +63,7 @@ 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 int sixaxis_get_device_bdaddr(int fd, bdaddr_t *bdaddr)
{
uint8_t buf[18];
int ret;
@@ -107,7 +84,7 @@ static int get_device_bdaddr(int fd, bdaddr_t *bdaddr)
return 0;
}
-static int get_master_bdaddr(int fd, bdaddr_t *bdaddr)
+static int sixaxis_get_master_bdaddr(int fd, bdaddr_t *bdaddr)
{
uint8_t buf[8];
int ret;
@@ -128,7 +105,7 @@ static int get_master_bdaddr(int fd, bdaddr_t *bdaddr)
return 0;
}
-static int set_master_bdaddr(int fd, const bdaddr_t *bdaddr)
+static int sixaxis_set_master_bdaddr(int fd, const bdaddr_t *bdaddr)
{
uint8_t buf[8];
int ret;
@@ -146,7 +123,7 @@ static int set_master_bdaddr(int fd, const bdaddr_t *bdaddr)
return ret;
}
-static uint8_t calc_leds_bitmap(int number)
+static uint8_t sixaxis_calc_leds_bitmap(int number)
{
uint8_t bitmap = 0;
@@ -164,7 +141,7 @@ static uint8_t calc_leds_bitmap(int number)
return bitmap;
}
-static void set_leds_hidraw(int fd, uint8_t leds_bitmap)
+static void sixaxis_set_leds_hidraw(int fd, uint8_t leds_bitmap)
{
/*
* the total time the led is active (0xff means forever)
@@ -199,7 +176,7 @@ static void set_leds_hidraw(int fd, uint8_t leds_bitmap)
error("sixaxis: failed to set LEDS (%d bytes written)", ret);
}
-static bool set_leds_sysfs(struct leds_data *data)
+static bool sixaxis_set_leds_sysfs(struct leds_data *data)
{
int i;
@@ -233,7 +210,7 @@ static bool set_leds_sysfs(struct leds_data *data)
return true;
}
-static gboolean setup_leds(GIOChannel *channel, GIOCondition cond,
+static gboolean sixaxis_setup_leds(GIOChannel *channel, GIOCondition cond,
gpointer user_data)
{
struct leds_data *data = user_data;
@@ -244,9 +221,9 @@ static gboolean setup_leds(GIOChannel *channel, GIOCondition cond,
if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
goto out;
- if(!set_leds_sysfs(data)) {
+ if(!sixaxis_set_leds_sysfs(data)) {
int fd = g_io_channel_unix_get_fd(channel);
- set_leds_hidraw(fd, data->bitmap);
+ sixaxis_set_leds_hidraw(fd, data->bitmap);
}
out:
@@ -374,7 +351,7 @@ out:
return syspath_prefix;
}
-static struct leds_data *get_leds_data(struct udev_device *udevice)
+static struct leds_data *sixaxis_get_leds_data(struct udev_device *udevice)
{
struct leds_data *data;
int number;
@@ -386,7 +363,7 @@ static struct leds_data *get_leds_data(struct udev_device *udevice)
if (!data)
return NULL;
- data->bitmap = calc_leds_bitmap(number);
+ data->bitmap = sixaxis_calc_leds_bitmap(number);
if (data->bitmap == 0) {
leds_data_destroy(data);
return NULL;
@@ -401,6 +378,55 @@ static struct leds_data *get_leds_data(struct udev_device *udevice)
return data;
}
+static int sixaxis_post_connect(int fd, struct udev_device *udevice)
+{
+ GIOChannel *io;
+
+ io = g_io_channel_unix_new(fd);
+
+ /* wait for events before setting leds */
+ g_io_add_watch(io, G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
+ sixaxis_setup_leds, sixaxis_get_leds_data(udevice));
+
+ g_io_channel_set_close_on_unref(io, TRUE);
+ g_io_channel_unref(io);
+}
+
+static const struct {
+ const char *name;
+ uint16_t source;
+ uint16_t vid;
+ uint16_t pid;
+ uint16_t version;
+ int (*get_device_bdaddr)(int fd, bdaddr_t *bdaddr);
+ int (*get_master_bdaddr)(int fd, bdaddr_t *bdaddr);
+ int (*set_master_bdaddr)(int fd, const bdaddr_t *bdaddr);
+ int (*post_connect)(int fd, struct udev_device *udevice); /* optional */
+} devices[] = {
+ {
+ .name = "PLAYSTATION(R)3 Controller",
+ .source = 0x0002,
+ .vid = 0x054c,
+ .pid = 0x0268,
+ .version = 0x0000,
+ .get_device_bdaddr = sixaxis_get_device_bdaddr,
+ .get_master_bdaddr = sixaxis_get_master_bdaddr,
+ .set_master_bdaddr = sixaxis_set_master_bdaddr,
+ .post_connect = sixaxis_post_connect,
+ },
+ {
+ .name = "Navigation Controller",
+ .source = 0x0002,
+ .vid = 0x054c,
+ .pid = 0x042f,
+ .version = 0x0000,
+ .get_device_bdaddr = sixaxis_get_device_bdaddr,
+ .get_master_bdaddr = sixaxis_get_master_bdaddr,
+ .set_master_bdaddr = sixaxis_set_master_bdaddr,
+ .post_connect = sixaxis_post_connect,
+ },
+};
+
static bool setup_device(int fd, int index, struct btd_adapter *adapter)
{
char device_addr[18], master_addr[18], adapter_addr[18];
@@ -408,10 +434,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 (devices[index].get_device_bdaddr(fd, &device_bdaddr) < 0)
return false;
- if (get_master_bdaddr(fd, &master_bdaddr) < 0)
+ if (devices[index].get_master_bdaddr(fd, &master_bdaddr) < 0)
return false;
/* This can happen if controller was plugged while already connected
@@ -425,7 +451,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 (devices[index].set_master_bdaddr(fd, adapter_bdaddr) < 0)
return false;
}
@@ -481,7 +507,6 @@ static int get_supported_device(struct udev_device *udevice, uint16_t *bus)
static void device_added(struct udev_device *udevice)
{
struct btd_adapter *adapter;
- GIOChannel *io;
uint16_t bus;
int index;
int fd;
@@ -502,8 +527,6 @@ static void device_added(struct udev_device *udevice)
if (fd < 0)
return;
- io = g_io_channel_unix_new(fd);
-
switch (bus) {
case BUS_USB:
if (!setup_device(fd, index, adapter))
@@ -511,18 +534,14 @@ static void device_added(struct udev_device *udevice)
/* fall through */
case BUS_BLUETOOTH:
- /* wait for events before setting leds */
- g_io_add_watch(io, G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
- setup_leds, get_leds_data(udevice));
+ if (devices[index].post_connect)
+ devices[index].post_connect(fd, udevice);
break;
default:
DBG("uknown bus type (%u)", bus);
break;
}
-
- g_io_channel_set_close_on_unref(io, TRUE);
- g_io_channel_unref(io);
}
static gboolean monitor_watch(GIOChannel *source, GIOCondition condition,
--
2.7.4
No functional changes, only moving functions around in file.
---
plugins/sixaxis.c | 104 +++++++++++++++++++++++++++---------------------------
1 file changed, 52 insertions(+), 52 deletions(-)
diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
index fcc93bc..d62c6db 100644
--- a/plugins/sixaxis.c
+++ b/plugins/sixaxis.c
@@ -255,58 +255,6 @@ out:
return FALSE;
}
-static bool setup_device(int fd, int index, struct btd_adapter *adapter)
-{
- char device_addr[18], master_addr[18], adapter_addr[18];
- bdaddr_t device_bdaddr, master_bdaddr;
- const bdaddr_t *adapter_bdaddr;
- struct btd_device *device;
-
- if (get_device_bdaddr(fd, &device_bdaddr) < 0)
- return false;
-
- if (get_master_bdaddr(fd, &master_bdaddr) < 0)
- return false;
-
- /* This can happen if controller was plugged while already connected
- * eg. to charge up battery.
- * Don't set LEDs in that case, hence return false */
- device = btd_adapter_find_device(adapter, &device_bdaddr,
- BDADDR_BREDR);
- if (device && btd_device_is_connected(device))
- return false;
-
- adapter_bdaddr = btd_adapter_get_address(adapter);
-
- if (bacmp(adapter_bdaddr, &master_bdaddr)) {
- if (set_master_bdaddr(fd, adapter_bdaddr) < 0)
- return false;
- }
-
- ba2str(&device_bdaddr, device_addr);
- ba2str(&master_bdaddr, master_addr);
- ba2str(adapter_bdaddr, adapter_addr);
- DBG("remote %s old_master %s new_master %s",
- device_addr, master_addr, adapter_addr);
-
- device = btd_adapter_get_device(adapter, &device_bdaddr, BDADDR_BREDR);
-
- if (g_slist_find_custom(btd_device_get_uuids(device), HID_UUID,
- (GCompareFunc)strcasecmp)) {
- DBG("device %s already known, skipping", device_addr);
- return true;
- }
-
- info("sixaxis: setting up new device");
-
- btd_device_device_set_name(device, devices[index].name);
- btd_device_set_pnpid(device, devices[index].source, devices[index].vid,
- devices[index].pid, devices[index].version);
- btd_device_set_temporary(device, false);
-
- return true;
-}
-
static int get_js_number(struct udev_device *udevice)
{
struct udev_list_entry *devices, *dev_list_entry;
@@ -453,6 +401,58 @@ static struct leds_data *get_leds_data(struct udev_device *udevice)
return data;
}
+static bool setup_device(int fd, int index, struct btd_adapter *adapter)
+{
+ char device_addr[18], master_addr[18], adapter_addr[18];
+ bdaddr_t device_bdaddr, master_bdaddr;
+ const bdaddr_t *adapter_bdaddr;
+ struct btd_device *device;
+
+ if (get_device_bdaddr(fd, &device_bdaddr) < 0)
+ return false;
+
+ if (get_master_bdaddr(fd, &master_bdaddr) < 0)
+ return false;
+
+ /* This can happen if controller was plugged while already connected
+ * eg. to charge up battery.
+ * Don't set LEDs in that case, hence return false */
+ device = btd_adapter_find_device(adapter, &device_bdaddr,
+ BDADDR_BREDR);
+ if (device && btd_device_is_connected(device))
+ return false;
+
+ adapter_bdaddr = btd_adapter_get_address(adapter);
+
+ if (bacmp(adapter_bdaddr, &master_bdaddr)) {
+ if (set_master_bdaddr(fd, adapter_bdaddr) < 0)
+ return false;
+ }
+
+ ba2str(&device_bdaddr, device_addr);
+ ba2str(&master_bdaddr, master_addr);
+ ba2str(adapter_bdaddr, adapter_addr);
+ DBG("remote %s old_master %s new_master %s",
+ device_addr, master_addr, adapter_addr);
+
+ device = btd_adapter_get_device(adapter, &device_bdaddr, BDADDR_BREDR);
+
+ if (g_slist_find_custom(btd_device_get_uuids(device), HID_UUID,
+ (GCompareFunc)strcasecmp)) {
+ DBG("device %s already known, skipping", device_addr);
+ return true;
+ }
+
+ info("sixaxis: setting up new device");
+
+ btd_device_device_set_name(device, devices[index].name);
+ btd_device_set_pnpid(device, devices[index].source, devices[index].vid,
+ devices[index].pid, devices[index].version);
+ btd_device_set_temporary(device, false);
+
+ return true;
+}
+
static int get_supported_device(struct udev_device *udevice, uint16_t *bus)
{
struct udev_device *hid_parent;
--
2.7.4
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. This new PID
matches with the model number CUH-ZCT2U.
---
profiles/input/server.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/profiles/input/server.c b/profiles/input/server.c
index eb3fcf8..3576f2b 100644
--- a/profiles/input/server.c
+++ b/profiles/input/server.c
@@ -135,8 +135,10 @@ static bool dev_is_sixaxis(const bdaddr_t *src, const bdaddr_t *dst)
if (vid == 0x054c && pid == 0x0268)
return true;
- /* DualShock 4 */
- if (vid == 0x054c && pid == 0x05c4)
+ /* DualShock 4 CUH-ZCT1U (PID 0x05c4)
+ * DualShock 4 CUH-ZCT2U (PID 0x09cc) (slim/pro)
+ */
+ if (vid == 0x054c && (pid == 0x05c4 || pid == 0x09cc))
return true;
/* Navigation Controller */
--
2.7.4
On Thu, 2017-01-05 at 11:17 -0800, Juha Kuikka wrote:
> On Thu, Jan 5, 2017 at 3:17 AM, Bastien Nocera <[email protected]>
> wrote:
> > On Wed, 2017-01-04 at 11:18 -0800, Juha Kuikka wrote:
> > > Hi Bastien,
> > >
> > > On Thu, Dec 29, 2016 at 3:48 AM, Bastien Nocera <[email protected]
> > > t>
> > > wrote:
> > > > On Wed, 2016-12-28 at 16:32 -0800, 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.
> > > > > This
> > > > > new
> > > > > PID
> > > > > matches with the model number CUH-ZCT2U.
> > > > > ---
> > > > > profiles/input/server.c | 6 ++++--
> > > > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/profiles/input/server.c
> > > > > b/profiles/input/server.c
> > > > > index eb3fcf8..3576f2b 100644
> > > > > --- a/profiles/input/server.c
> > > > > +++ b/profiles/input/server.c
> > > > > @@ -135,8 +135,10 @@ static bool dev_is_sixaxis(const
> > > > > bdaddr_t
> > > > > *src,
> > > > > const bdaddr_t *dst)
> > > > > if (vid == 0x054c && pid == 0x0268)
> > > > > return true;
> > > > >
> > > > > - /* DualShock 4 */
> > > > > - if (vid == 0x054c && pid == 0x05c4)
> > > > > + /* DualShock 4 CUH-ZCT1U (PID 0x05c4)
> > > > > + * DualShock 4 CUH-ZCT2U (PID 0x09cc) (slim/pro)
> > > > > + */
> > > > > + if (vid == 0x054c && (pid == 0x05c4 || pid == 0x09cc))
> > > > > return true;
> > > > >
> > > > > /* Navigation Controller */
> > > >
> > > > Might be nice to have the struct you currently have in the
> > > > sixaxis
> > > > plugin in a shared header, so we don't need to open code this
> > > > function.
> > > > For example:
> > > >
> > > > typedef enum {
> > > > CABLE_PAIRING_SIXAXIS,
> > > > CABLE_PAIRING_DS4
> > > > } CablePairingType;
> > > >
> > > > static struct {
> > > > int vid;
> > > > int pid;
> > > > CablePairingType type;
> > > > } cable_pairing_devices[] = {
> > > > ...
> > > > };
> > > >
> > >
> > > I agree, having the pertinent VID/PID in one place only would be
> > > a
> > > good idea. One small issue I see here is that it would
> > > unnecessarily
> > > expose internal information from sixaxis plugin.
> >
> > Which internal information? I don't think you need to export any
> > information from the sixaxis plugin to make this work.
> >
> > > > The sixaxis plugin would know which functions to use from the
> > > > type,
> > > > the
> > > > input/server.c code would loop over the array to see whether
> > > > it's a
> > > > cable pairing.
> > >
> > > Having a type field instead of the function pointers would make
> > > for a
> > > less clear implementation in sixaxis.c. My previous patch had an
> > > analogous mechanism for detecting controller type and changing
> > > behavior accordingly and I got some comments about a function
> > > pointer
> > > approach being preferred.
> >
> > No, I'm not asking you to replace that. Inside the sixaxis plugin
> > you
> > used to do:
> > set_master_bdaddr()
> > {
> > if (is_ds4()) {
> > ...
> > } else if (is_sixaxis()) {
> > ...
> > }
> > }
> >
> > Here, I'm saying that when you're doing your search you should
> > have:
> > setup_device()
> > {
> > if (is_ds4())
> > device->funcs = ds4_device_funcs;
> > else if (is_sixaxis()
> > device->funcs = sixaxis_device_funcs;
> > }
> >
> > Or you could also use another array for that:
> > static struct {
> > DeviceType type;
> > SetupFunc setup_func;
> > PostSetupFunc post_setup_func;
> > } functions[] = {
> > CABLE_PAIRING_TYPE_DS4, ds4_setup_func, ...
> >
> > And set that up in setup_device().
>
> Thank you for the clarification. I will revise the patches with that
> in mind.
>
> Do you have a suggestion on where the cable_pairing_devices[] and API
> to read it should go? It cannot be in the sixaxis since it may not be
> built in.
In a separate .h file, not sure in which directory. Probably
profiles/input as that's always compiled-in?
> I am thinking of an API in the vein of:
> int btd_find_cable_pairing_type(int vid, int pid);
I don't think you need to have API or code in the shared .h file, just
a shared array.
> Which would return the CABLE_PAIRING_TYPE_* enum or
> CABLE_PAIRING_TYPE_UNKNOWN (-1) on error.
>
> >
> > > I saw some patches floating around concerning a generalized cable
> > > pairing mechanism. Perhaps this change should be part of that?
> >
> > I don't think we need something more generic at this point.
>
> Cheers,
> - Juha
> --
> 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
On Thu, Jan 5, 2017 at 3:17 AM, Bastien Nocera <[email protected]> wrote:
> On Wed, 2017-01-04 at 11:18 -0800, Juha Kuikka wrote:
>> Hi Bastien,
>>
>> On Thu, Dec 29, 2016 at 3:48 AM, Bastien Nocera <[email protected]>
>> wrote:
>> > On Wed, 2016-12-28 at 16:32 -0800, 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. This
>> > > new
>> > > PID
>> > > matches with the model number CUH-ZCT2U.
>> > > ---
>> > > profiles/input/server.c | 6 ++++--
>> > > 1 file changed, 4 insertions(+), 2 deletions(-)
>> > >
>> > > diff --git a/profiles/input/server.c b/profiles/input/server.c
>> > > index eb3fcf8..3576f2b 100644
>> > > --- a/profiles/input/server.c
>> > > +++ b/profiles/input/server.c
>> > > @@ -135,8 +135,10 @@ static bool dev_is_sixaxis(const bdaddr_t
>> > > *src,
>> > > const bdaddr_t *dst)
>> > > if (vid == 0x054c && pid == 0x0268)
>> > > return true;
>> > >
>> > > - /* DualShock 4 */
>> > > - if (vid == 0x054c && pid == 0x05c4)
>> > > + /* DualShock 4 CUH-ZCT1U (PID 0x05c4)
>> > > + * DualShock 4 CUH-ZCT2U (PID 0x09cc) (slim/pro)
>> > > + */
>> > > + if (vid == 0x054c && (pid == 0x05c4 || pid == 0x09cc))
>> > > return true;
>> > >
>> > > /* Navigation Controller */
>> >
>> > Might be nice to have the struct you currently have in the sixaxis
>> > plugin in a shared header, so we don't need to open code this
>> > function.
>> > For example:
>> >
>> > typedef enum {
>> > CABLE_PAIRING_SIXAXIS,
>> > CABLE_PAIRING_DS4
>> > } CablePairingType;
>> >
>> > static struct {
>> > int vid;
>> > int pid;
>> > CablePairingType type;
>> > } cable_pairing_devices[] = {
>> > ...
>> > };
>> >
>>
>> I agree, having the pertinent VID/PID in one place only would be a
>> good idea. One small issue I see here is that it would unnecessarily
>> expose internal information from sixaxis plugin.
>
> Which internal information? I don't think you need to export any
> information from the sixaxis plugin to make this work.
>
>> > The sixaxis plugin would know which functions to use from the type,
>> > the
>> > input/server.c code would loop over the array to see whether it's a
>> > cable pairing.
>>
>> Having a type field instead of the function pointers would make for a
>> less clear implementation in sixaxis.c. My previous patch had an
>> analogous mechanism for detecting controller type and changing
>> behavior accordingly and I got some comments about a function pointer
>> approach being preferred.
>
> No, I'm not asking you to replace that. Inside the sixaxis plugin you
> used to do:
> set_master_bdaddr()
> {
> if (is_ds4()) {
> ...
> } else if (is_sixaxis()) {
> ...
> }
> }
>
> Here, I'm saying that when you're doing your search you should have:
> setup_device()
> {
> if (is_ds4())
> device->funcs = ds4_device_funcs;
> else if (is_sixaxis()
> device->funcs = sixaxis_device_funcs;
> }
>
> Or you could also use another array for that:
> static struct {
> DeviceType type;
> SetupFunc setup_func;
> PostSetupFunc post_setup_func;
> } functions[] = {
> CABLE_PAIRING_TYPE_DS4, ds4_setup_func, ...
>
> And set that up in setup_device().
Thank you for the clarification. I will revise the patches with that in mind.
Do you have a suggestion on where the cable_pairing_devices[] and API
to read it should go? It cannot be in the sixaxis since it may not be
built in.
I am thinking of an API in the vein of:
int btd_find_cable_pairing_type(int vid, int pid);
Which would return the CABLE_PAIRING_TYPE_* enum or
CABLE_PAIRING_TYPE_UNKNOWN (-1) on error.
>
>> I saw some patches floating around concerning a generalized cable
>> pairing mechanism. Perhaps this change should be part of that?
>
> I don't think we need something more generic at this point.
Cheers,
- Juha
On Wed, 2017-01-04 at 11:18 -0800, Juha Kuikka wrote:
> Hi Bastien,
>
> On Thu, Dec 29, 2016 at 3:48 AM, Bastien Nocera <[email protected]>
> wrote:
> > On Wed, 2016-12-28 at 16:32 -0800, 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. This
> > > new
> > > PID
> > > matches with the model number CUH-ZCT2U.
> > > ---
> > > profiles/input/server.c | 6 ++++--
> > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/profiles/input/server.c b/profiles/input/server.c
> > > index eb3fcf8..3576f2b 100644
> > > --- a/profiles/input/server.c
> > > +++ b/profiles/input/server.c
> > > @@ -135,8 +135,10 @@ static bool dev_is_sixaxis(const bdaddr_t
> > > *src,
> > > const bdaddr_t *dst)
> > > if (vid == 0x054c && pid == 0x0268)
> > > return true;
> > >
> > > - /* DualShock 4 */
> > > - if (vid == 0x054c && pid == 0x05c4)
> > > + /* DualShock 4 CUH-ZCT1U (PID 0x05c4)
> > > + * DualShock 4 CUH-ZCT2U (PID 0x09cc) (slim/pro)
> > > + */
> > > + if (vid == 0x054c && (pid == 0x05c4 || pid == 0x09cc))
> > > return true;
> > >
> > > /* Navigation Controller */
> >
> > Might be nice to have the struct you currently have in the sixaxis
> > plugin in a shared header, so we don't need to open code this
> > function.
> > For example:
> >
> > typedef enum {
> > CABLE_PAIRING_SIXAXIS,
> > CABLE_PAIRING_DS4
> > } CablePairingType;
> >
> > static struct {
> > int vid;
> > int pid;
> > CablePairingType type;
> > } cable_pairing_devices[] = {
> > ...
> > };
> >
>
> I agree, having the pertinent VID/PID in one place only would be a
> good idea. One small issue I see here is that it would unnecessarily
> expose internal information from sixaxis plugin.
Which internal information? I don't think you need to export any
information from the sixaxis plugin to make this work.
> > The sixaxis plugin would know which functions to use from the type,
> > the
> > input/server.c code would loop over the array to see whether it's a
> > cable pairing.
>
> Having a type field instead of the function pointers would make for a
> less clear implementation in sixaxis.c. My previous patch had an
> analogous mechanism for detecting controller type and changing
> behavior accordingly and I got some comments about a function pointer
> approach being preferred.
No, I'm not asking you to replace that. Inside the sixaxis plugin you
used to do:
set_master_bdaddr()
{
if (is_ds4()) {
...
} else if (is_sixaxis()) {
...
}
}
Here, I'm saying that when you're doing your search you should have:
setup_device()
{
if (is_ds4())
device->funcs = ds4_device_funcs;
else if (is_sixaxis()
device->funcs = sixaxis_device_funcs;
}
Or you could also use another array for that:
static struct {
DeviceType type;
SetupFunc setup_func;
PostSetupFunc post_setup_func;
} functions[] = {
CABLE_PAIRING_TYPE_DS4, ds4_setup_func, ...
And set that up in setup_device().
> I saw some patches floating around concerning a generalized cable
> pairing mechanism. Perhaps this change should be part of that?
I don't think we need something more generic at this point.
Hi Bastien,
On Thu, Dec 29, 2016 at 3:48 AM, Bastien Nocera <[email protected]> wrote:
> On Wed, 2016-12-28 at 16:32 -0800, 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. This new
>> PID
>> matches with the model number CUH-ZCT2U.
>> ---
>> profiles/input/server.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/profiles/input/server.c b/profiles/input/server.c
>> index eb3fcf8..3576f2b 100644
>> --- a/profiles/input/server.c
>> +++ b/profiles/input/server.c
>> @@ -135,8 +135,10 @@ static bool dev_is_sixaxis(const bdaddr_t *src,
>> const bdaddr_t *dst)
>> if (vid == 0x054c && pid == 0x0268)
>> return true;
>>
>> - /* DualShock 4 */
>> - if (vid == 0x054c && pid == 0x05c4)
>> + /* DualShock 4 CUH-ZCT1U (PID 0x05c4)
>> + * DualShock 4 CUH-ZCT2U (PID 0x09cc) (slim/pro)
>> + */
>> + if (vid == 0x054c && (pid == 0x05c4 || pid == 0x09cc))
>> return true;
>>
>> /* Navigation Controller */
>
> Might be nice to have the struct you currently have in the sixaxis
> plugin in a shared header, so we don't need to open code this function.
> For example:
>
> typedef enum {
> CABLE_PAIRING_SIXAXIS,
> CABLE_PAIRING_DS4
> } CablePairingType;
>
> static struct {
> int vid;
> int pid;
> CablePairingType type;
> } cable_pairing_devices[] = {
> ...
> };
>
I agree, having the pertinent VID/PID in one place only would be a
good idea. One small issue I see here is that it would unnecessarily
expose internal information from sixaxis plugin.
> The sixaxis plugin would know which functions to use from the type, the
> input/server.c code would loop over the array to see whether it's a
> cable pairing.
Having a type field instead of the function pointers would make for a
less clear implementation in sixaxis.c. My previous patch had an
analogous mechanism for detecting controller type and changing
behavior accordingly and I got some comments about a function pointer
approach being preferred.
I saw some patches floating around concerning a generalized cable
pairing mechanism. Perhaps this change should be part of that?
>
> Looks fine otherwise.
- Juha
On Wed, 2016-12-28 at 16:32 -0800, Juha Kuikka wrote:
> This patch set first refactors the sixaxis plugin to more easily
> support
> multiple controllers with different protocols.
>
> Secondly support for pairing the Dualshock 4 controller over USB is
> added.
>
> Last, the PID of the new DS4 controller is added into the input
> device special
> case handler which allows this controller to be used.
I've redone this patchset on top of my "cable pairing authentication"
patches:
https://github.com/hadess/bluez/commits/ds4-cable-pairing
I wanted people to test it out, go through review comments, and fix any
major problems before sending it out to the list.
The cable pairing authentication needs an agent that supports it. If
you have GNOME installed, you're in luck. The cable pairing
authentication has been supported since gnome-bluetooth 3.18.0.
It was implemented in this patch, if you want to try and add support in
one of the tools around:
https://git.gnome.org/browse/gnome-bluetooth/commit/?id=2efefd456b5ae3617e618e8d76ca3a121b83570e
TODO items:
- go through original (Juha's) patchset review comments
- add cancellation for cable pairing when USB is disconnected
- tests tests tests
Comments welcome!