Hi,
another piece in the Sixaxis jigsaw:
in commit d4bfa033ed84e0ae446eff445d107ffd5ee78df3 support for setting
different report types was added to hidp, however in my Sixaxis
experiments setting leds (sending and output report) was not working
until I made this change:
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index b68a608..0c443b7 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -402,7 +402,7 @@ static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data, s
report_type = HIDP_TRANS_SET_REPORT | HIDP_DATA_RTYPE_FEATURE;
break;
case HID_OUTPUT_REPORT:
- report_type = HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT;
+ report_type = HIDP_TRANS_SET_REPORT | HIDP_DATA_RTYPE_OUPUT;
break;
default:
return -EINVAL;
Is it only the Sixaxis which needs the output report as a SET_REPORT
operation, or the change above is an actual fix?
I don't know bluetooth at all, sorry.
In case this is a sixaxis specific behavior then I guess I'll be
overriding hidp_output_raw_report() in hid-sony.c just like I did for the
usbhid counterpart.
Thanks,
Antonio
--
Antonio Ospite
http://ao2.it
PGP public key ID: 0x4553B001
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?
On Tue, 30 Nov 2010 14:54:05 +0100
Antonio Ospite <[email protected]> wrote:
> Hi,
>
> another piece in the Sixaxis jigsaw:
> in commit d4bfa033ed84e0ae446eff445d107ffd5ee78df3 support for setting
> different report types was added to hidp, however in my Sixaxis
> experiments setting leds (sending and output report) was not working
sending _an_ output report.
Regards,
Antonio
--
Antonio Ospite
http://ao2.it
PGP public key ID: 0x4553B001
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?
On Mon, 21 Feb 2011, Gustavo F. Padovan wrote:
> > > > The current implementation of hidp_output_raw_report() relies only on
> > > > the Control channel even for Output reports, and the BT HID
> > > > specification [1] does not mention using the DATA message for Output
> > > > reports on the Control channel (see section 7.9.1 and also Figure 11:
> > > > SET_ Flow Chart), so let us just use SET_REPORT.
> > > >
> > > > This also fixes sending Output reports to some devices (like Sony
> > > > Sixaxis) which are not able to handle DATA messages on the Control
> > > > channel.
> > > >
> > > > Ideally hidp_output_raw_report() could be improved to use this scheme:
> > > > Feature Report -- SET_REPORT on the Control channel
> > > > Output Report -- DATA on the Interrupt channel
> > > > for more efficiency, but as said above, right now only the Control
> > > > channel is used.
> > > >
> > > > [1] http://www.bluetooth.com/Specification%20Documents/HID_SPEC_V10.pdf
> > > >
> > > > case HID_OUTPUT_REPORT:
> > > > - report_type = HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT;
> > > > + report_type = HIDP_TRANS_SET_REPORT | HIDP_DATA_RTYPE_OUPUT;
> > > >
> > >
> > > I think this is right. Section 7.4[.0] says that SET_ and GET_ requests return
> > > with HANDSHAKE. Section 7.4.9 says that DATA does _not_ return a HANDSHAKE. My
> > > patch to hidp_output_raw_report() relies on getting a HANDSHAKE back, so it
> > > wouldn't have worked with BT devices that take output reports. Since I don't
> > > have any that do, I couldn't test it. (And it was like that when I got here :)
> > > )
> > >
> > > For the whole set:
> > > Acked-by: Alan Ott <[email protected]>
> >
> > Agreed.
> >
> > As an author of the original code, I agree with the change. But as it is
> > in net/bluetooth, I'd at least have Acked-by from some of the Bluetooth
> > folks before I take it through my tree.
> >
> > Marcel? Gustavo?
>
> Acked-by: Gustavo F. Padovan <[email protected]>
Perfect, applied, thanks.
--
Jiri Kosina
SUSE Labs, Novell Inc.
Hi Jiri,
* Jiri Kosina <[email protected]> [2011-02-21 13:50:40 +0100]:
> On Sun, 20 Feb 2011, Alan Ott wrote:
>
> > On 02/20/2011 12:26 PM, Antonio Ospite wrote:
> > > The current implementation of hidp_output_raw_report() relies only on
> > > the Control channel even for Output reports, and the BT HID
> > > specification [1] does not mention using the DATA message for Output
> > > reports on the Control channel (see section 7.9.1 and also Figure 11:
> > > SET_ Flow Chart), so let us just use SET_REPORT.
> > >
> > > This also fixes sending Output reports to some devices (like Sony
> > > Sixaxis) which are not able to handle DATA messages on the Control
> > > channel.
> > >
> > > Ideally hidp_output_raw_report() could be improved to use this scheme:
> > > Feature Report -- SET_REPORT on the Control channel
> > > Output Report -- DATA on the Interrupt channel
> > > for more efficiency, but as said above, right now only the Control
> > > channel is used.
> > >
> > > [1] http://www.bluetooth.com/Specification%20Documents/HID_SPEC_V10.pdf
> > >
> > > case HID_OUTPUT_REPORT:
> > > - report_type = HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT;
> > > + report_type = HIDP_TRANS_SET_REPORT | HIDP_DATA_RTYPE_OUPUT;
> > >
> >
> > I think this is right. Section 7.4[.0] says that SET_ and GET_ requests return
> > with HANDSHAKE. Section 7.4.9 says that DATA does _not_ return a HANDSHAKE. My
> > patch to hidp_output_raw_report() relies on getting a HANDSHAKE back, so it
> > wouldn't have worked with BT devices that take output reports. Since I don't
> > have any that do, I couldn't test it. (And it was like that when I got here :)
> > )
> >
> > For the whole set:
> > Acked-by: Alan Ott <[email protected]>
>
> Agreed.
>
> As an author of the original code, I agree with the change. But as it is
> in net/bluetooth, I'd at least have Acked-by from some of the Bluetooth
> folks before I take it through my tree.
>
> Marcel? Gustavo?
Acked-by: Gustavo F. Padovan <[email protected]>
--
Gustavo F. Padovan
http://profusion.mobi
On Sun, 20 Feb 2011, Alan Ott wrote:
> On 02/20/2011 12:26 PM, Antonio Ospite wrote:
> > The current implementation of hidp_output_raw_report() relies only on
> > the Control channel even for Output reports, and the BT HID
> > specification [1] does not mention using the DATA message for Output
> > reports on the Control channel (see section 7.9.1 and also Figure 11:
> > SET_ Flow Chart), so let us just use SET_REPORT.
> >
> > This also fixes sending Output reports to some devices (like Sony
> > Sixaxis) which are not able to handle DATA messages on the Control
> > channel.
> >
> > Ideally hidp_output_raw_report() could be improved to use this scheme:
> > Feature Report -- SET_REPORT on the Control channel
> > Output Report -- DATA on the Interrupt channel
> > for more efficiency, but as said above, right now only the Control
> > channel is used.
> >
> > [1] http://www.bluetooth.com/Specification%20Documents/HID_SPEC_V10.pdf
> >
> > case HID_OUTPUT_REPORT:
> > - report_type = HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT;
> > + report_type = HIDP_TRANS_SET_REPORT | HIDP_DATA_RTYPE_OUPUT;
> >
>
> I think this is right. Section 7.4[.0] says that SET_ and GET_ requests return
> with HANDSHAKE. Section 7.4.9 says that DATA does _not_ return a HANDSHAKE. My
> patch to hidp_output_raw_report() relies on getting a HANDSHAKE back, so it
> wouldn't have worked with BT devices that take output reports. Since I don't
> have any that do, I couldn't test it. (And it was like that when I got here :)
> )
>
> For the whole set:
> Acked-by: Alan Ott <[email protected]>
Agreed.
As an author of the original code, I agree with the change. But as it is
in net/bluetooth, I'd at least have Acked-by from some of the Bluetooth
folks before I take it through my tree.
Marcel? Gustavo?
Thanks,
--
Jiri Kosina
SUSE Labs, Novell Inc.
On Sun, 20 Feb 2011, Antonio Ospite wrote:
> The Sixaxis does not want the report_id as part of the data packet in
> Output reports, so we have to discard buf[0] when sending the actual
> control message.
>
> Add also some documentation about that and about why
> hdev->hid_output_raw_report needs to be overridden.
>
> Signed-off-by: Antonio Ospite <[email protected]>
Applied, thanks Antonio.
> ---
> drivers/hid/hid-sony.c | 20 ++++++++++++++++++++
> 1 files changed, 20 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index 68d7b36..93819a0 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -46,6 +46,16 @@ static __u8 *sony_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> return rdesc;
> }
>
> +/*
> + * The Sony Sixaxis does not handle HID Output Reports on the Interrupt EP
> + * like it should according to usbhid/hid-core.c::usbhid_output_raw_report()
> + * so we need to override that forcing HID Output Reports on the Control EP.
> + *
> + * There is also another issue about HID Output Reports via USB, the Sixaxis
> + * does not want the report_id as part of the data packet, so we have to
> + * discard buf[0] when sending the actual control message, even for numbered
> + * reports, humpf!
> + */
> static int sixaxis_usb_output_raw_report(struct hid_device *hid, __u8 *buf,
> size_t count, unsigned char report_type)
> {
> @@ -55,6 +65,12 @@ static int sixaxis_usb_output_raw_report(struct hid_device *hid, __u8 *buf,
> int report_id = buf[0];
> int ret;
>
> + if (report_type == HID_OUTPUT_REPORT) {
> + /* Don't send the Report ID */
> + buf++;
> + count--;
> + }
> +
> ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
> HID_REQ_SET_REPORT,
> USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> @@ -62,6 +78,10 @@ static int sixaxis_usb_output_raw_report(struct hid_device *hid, __u8 *buf,
> interface->desc.bInterfaceNumber, buf, count,
> USB_CTRL_SET_TIMEOUT);
>
> + /* Count also the Report ID, in case of an Output report. */
> + if (ret > 0 && report_type == HID_OUTPUT_REPORT)
> + ret++;
> +
> return ret;
> }
>
> --
> 1.7.4.1
>
--
Jiri Kosina
SUSE Labs, Novell Inc.
On 02/20/2011 12:26 PM, Antonio Ospite wrote:
> The current implementation of hidp_output_raw_report() relies only on
> the Control channel even for Output reports, and the BT HID
> specification [1] does not mention using the DATA message for Output
> reports on the Control channel (see section 7.9.1 and also Figure 11:
> SET_ Flow Chart), so let us just use SET_REPORT.
>
> This also fixes sending Output reports to some devices (like Sony
> Sixaxis) which are not able to handle DATA messages on the Control
> channel.
>
> Ideally hidp_output_raw_report() could be improved to use this scheme:
> Feature Report -- SET_REPORT on the Control channel
> Output Report -- DATA on the Interrupt channel
> for more efficiency, but as said above, right now only the Control
> channel is used.
>
> [1] http://www.bluetooth.com/Specification%20Documents/HID_SPEC_V10.pdf
>
> case HID_OUTPUT_REPORT:
> - report_type = HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT;
> + report_type = HIDP_TRANS_SET_REPORT | HIDP_DATA_RTYPE_OUPUT;
>
I think this is right. Section 7.4[.0] says that SET_ and GET_ requests
return with HANDSHAKE. Section 7.4.9 says that DATA does _not_ return a
HANDSHAKE. My patch to hidp_output_raw_report() relies on getting a
HANDSHAKE back, so it wouldn't have worked with BT devices that take
output reports. Since I don't have any that do, I couldn't test it. (And
it was like that when I got here :) )
For the whole set:
Acked-by: Alan Ott <[email protected]>
Alan.
The Sixaxis does not want the report_id as part of the data packet in
Output reports, so we have to discard buf[0] when sending the actual
control message.
Add also some documentation about that and about why
hdev->hid_output_raw_report needs to be overridden.
Signed-off-by: Antonio Ospite <[email protected]>
---
drivers/hid/hid-sony.c | 20 ++++++++++++++++++++
1 files changed, 20 insertions(+), 0 deletions(-)
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 68d7b36..93819a0 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -46,6 +46,16 @@ static __u8 *sony_report_fixup(struct hid_device *hdev, __u8 *rdesc,
return rdesc;
}
+/*
+ * The Sony Sixaxis does not handle HID Output Reports on the Interrupt EP
+ * like it should according to usbhid/hid-core.c::usbhid_output_raw_report()
+ * so we need to override that forcing HID Output Reports on the Control EP.
+ *
+ * There is also another issue about HID Output Reports via USB, the Sixaxis
+ * does not want the report_id as part of the data packet, so we have to
+ * discard buf[0] when sending the actual control message, even for numbered
+ * reports, humpf!
+ */
static int sixaxis_usb_output_raw_report(struct hid_device *hid, __u8 *buf,
size_t count, unsigned char report_type)
{
@@ -55,6 +65,12 @@ static int sixaxis_usb_output_raw_report(struct hid_device *hid, __u8 *buf,
int report_id = buf[0];
int ret;
+ if (report_type == HID_OUTPUT_REPORT) {
+ /* Don't send the Report ID */
+ buf++;
+ count--;
+ }
+
ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
HID_REQ_SET_REPORT,
USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
@@ -62,6 +78,10 @@ static int sixaxis_usb_output_raw_report(struct hid_device *hid, __u8 *buf,
interface->desc.bInterfaceNumber, buf, count,
USB_CTRL_SET_TIMEOUT);
+ /* Count also the Report ID, in case of an Output report. */
+ if (ret > 0 && report_type == HID_OUTPUT_REPORT)
+ ret++;
+
return ret;
}
--
1.7.4.1
Hi,
an Output report is used to set LEDs on the Sony Sixaxis controller, but
the device is quite picky about how it wants the Output report to be
sent, so these patches attempt to overcome the Sixaxis deficiencies.
The first patch is OK to apply, even for 2.6.38 I think.
For the second one I'd really like an actual test with other devices
supporting HID Output reports over BT, I don't want to break anything
even if the change looks sane as per the BT HID specification.
Something equivalent to the second patch should be implemented as an
override in hid-sony.c as well, in order to be protected from future
improvements to hidp_output_raw_report(), but for that to work I need to
make hidp_send_ctrl_message() public and I don't know if you'll like
that, we'll see in a patch to come.
Antonio Ospite (2):
hid-sony.c: Fix sending Output reports to the Sixaxis
bt hidp: send Output reports using SET_REPORT on the Control channel
drivers/hid/hid-sony.c | 20 ++++++++++++++++++++
net/bluetooth/hidp/core.c | 2 +-
2 files changed, 21 insertions(+), 1 deletions(-)
FYI, this is how I am setting leds on the Sixaxis, the same code works
with hidraw via usb and bluetooth after the patches above are applied:
#include <stdio.h>
#include <stdlib.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
#define LED_1 (0x01 << 1)
#define LED_2 (0x01 << 2)
#define LED_3 (0x01 << 3)
#define LED_4 (0x01 << 4)
void set_leds(int fd, unsigned char led_status[4])
{
int ret;
unsigned char change_leds[] = {
0x01,
0x00, 0x00, 0x00, 0x00, 0x00, // rumble values TBD.
0x00, 0x00, 0x00, 0x00, 0x1e,
0xff, 0x27, 0x10, 0x00, 0x32, // LED 4
0xff, 0x27, 0x10, 0x00, 0x32, // LED 3
0xff, 0x27, 0x10, 0x00, 0x32, // LED 2
0xff, 0x27, 0x10, 0x00, 0x32, // LED 1
0x00, 0x00, 0x00, 0x00, 0x00,
};
int led = 0;
if (led_status[0])
led |= LED_1;
if (led_status[1])
led |= LED_2;
if (led_status[2])
led |= LED_3;
if (led_status[3])
led |= LED_4;
printf("led: 0x%02x\n", led);
change_leds[10] = led;
ret = write(fd, change_leds, sizeof(change_leds));
if (ret < (ssize_t) sizeof(change_leds)) {
close(fd);
perror("Unable to write to hidraw device");
exit(EXIT_FAILURE);
}
}
int main(int argc, char *argv[])
{
int fd;
unsigned char led_status[4] = { 1, 0, 0, 0 };
if (argc != 2) {
fprintf(stderr, "usage: %s </dev/hidrawX>\n", argv[0]);
exit(1);
}
fd = open(argv[1], O_RDWR);
if (fd < 0) {
perror("open");
exit(1);
}
set_leds(fd, led_status);
return 0;
}
--
Antonio Ospite
http://ao2.it
PGP public key ID: 0x4553B001
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?
The current implementation of hidp_output_raw_report() relies only on
the Control channel even for Output reports, and the BT HID
specification [1] does not mention using the DATA message for Output
reports on the Control channel (see section 7.9.1 and also Figure 11:
SET_ Flow Chart), so let us just use SET_REPORT.
This also fixes sending Output reports to some devices (like Sony
Sixaxis) which are not able to handle DATA messages on the Control
channel.
Ideally hidp_output_raw_report() could be improved to use this scheme:
Feature Report -- SET_REPORT on the Control channel
Output Report -- DATA on the Interrupt channel
for more efficiency, but as said above, right now only the Control
channel is used.
[1] http://www.bluetooth.com/Specification%20Documents/HID_SPEC_V10.pdf
Signed-off-by: Antonio Ospite <[email protected]>
---
net/bluetooth/hidp/core.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 6df8ea1..3c036b0 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -405,7 +405,7 @@ static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data, s
report_type = HIDP_TRANS_SET_REPORT | HIDP_DATA_RTYPE_FEATURE;
break;
case HID_OUTPUT_REPORT:
- report_type = HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT;
+ report_type = HIDP_TRANS_SET_REPORT | HIDP_DATA_RTYPE_OUPUT;
break;
default:
return -EINVAL;
--
1.7.4.1
On Thu, 17 Feb 2011, Antonio Ospite wrote:
> However, even if my patch gets is, Output reports on the Sixaxis will
> stop working again as soon as someone implements the behavior you are
> suggesting, that is:
> Feature Report -> SET_REPORT on the Control channel
> Output Report -> DATA on the Interrupt channel
>
> So I am going to submit the hidp_output_raw_report() override in
> hid-sony.c as well, does this sound OK?
That makes sense to me.
Could you please repost all the patches that are currently being discussed
are flying around in the air?
I feel I haven't been CCed on all of them, so I don't have the complete
picture.
Thanks,
--
Jiri Kosina
SUSE Labs, Novell Inc.
On Tue, 30 Nov 2010 18:40:03 +0100
"[email protected]" <[email protected]> wrote:
Pascal, you were replying to linux-input only so some of the original
recipients missed your comments.
> Antonio Ospite wrote:
> > +++ b/net/bluetooth/hidp/core.c
> > case HID_OUTPUT_REPORT:
> > - report_type = HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT;
> > + report_type = HIDP_TRANS_SET_REPORT | HIDP_DATA_RTYPE_OUPUT;
>
> > Is it only the Sixaxis which needs the output report as a SET_REPORT
> > operation, or the change above is an actual fix?
>
> My understanding of the Bluetooth HID Profile specification [1],
> section 7.4.9, is that on the control channel DATA is only for
> responding to an incoming GET_REPORT. Since Linux is the HID
> Host, it will probably never need to reply to a GET_REPORT.
> So your patch looks good.
>
OK, I managed to study [1] a little bit, so yes, I also think my patch
is acceptable right now, but just because as you note in another
message hidp_output_raw_report() is relying only on the control channel.
In section 7.9.1 there's no mention of DATA for Output reports on the
Control channel (see also Figure 11: SET_ Flow Chart).
So I am resending it, it shouldn't hurt.
> Note that usbhid_output_raw_report() seems to send OUTPUT
> reports as USB interrupt messages and FEATURE reports as
> USB SET_REPORT control messages, which makes sense too.
> It would be nice if hidraw behaved the same over USB and
> over Bluetooth. Unfortunately I don't think you can drive
> the sixaxis leds with DATA on the Bluetooth interrupt channel.
>
However, even if my patch gets is, Output reports on the Sixaxis will
stop working again as soon as someone implements the behavior you are
suggesting, that is:
Feature Report -> SET_REPORT on the Control channel
Output Report -> DATA on the Interrupt channel
So I am going to submit the hidp_output_raw_report() override in
hid-sony.c as well, does this sound OK?
>
> [1] http://www.bluetooth.com/Specification%20Documents/HID_SPEC_V10.pdf
>
Regards,
Antonio
--
Antonio Ospite
http://ao2.it
PGP public key ID: 0x4553B001
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?