On Thu, Dec 22, 2016 at 2:24 AM, Antonio Ospite <ao2@xxxxxx> wrote: > On Wed, 21 Dec 2016 21:20:53 -0800 > Juha Kuikka <juha.kuikka@xxxxxxxxxxx> 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@xxxxxxxxx> > Roderick Colenbrander <roderick.colenbrander@xxxxxxxx> > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
