Re: hidp_output_raw_report, HID_OUTPUT_REPORT and Sixaxis

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.

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 <redacted>
---
 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

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 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 <redacted>
---
 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

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 <redacted>

Applied, thanks Antonio.

-- 
Jiri Kosina
SUSE Labs, Novell Inc.