hidp_output_raw_report, HID_OUTPUT_REPORT and Sixaxis

hidp_output_raw_report, HID_OUTPUT_REPORT and Sixaxis

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 1404 bytes --]

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?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: hidp_output_raw_report, HID_OUTPUT_REPORT and Sixaxis

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 643 bytes --]

On Tue, 30 Nov 2010 14:54:05 +0100
Antonio Ospite <ospite@studenti.unina.it> 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?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: hidp_output_raw_report, HID_OUTPUT_REPORT and Sixaxis

[pascal]

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.

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.

Pascal

[1] http://www.bluetooth.com/Specification%20Documents/HID_SPEC_V10.pdf

Re: hidp_output_raw_report, HID_OUTPUT_REPORT and Sixaxis

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 1977 bytes --]

On Tue, 30 Nov 2010 18:40:03 +0100
"pascal@pabr.org" <pascal@pabr.org> wrote:

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

Hi Pascal,

I'll check with the specification you pointed at, and try to come up
with a meaningful commit message then.

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

Well, but that does not work for sixaxis, it only accepts output
reports on the control endpoint, look at a recent hid-sony.c
overriding usbhid_output_raw_report().

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

The non-standard USB behavior made me think that the patch proposed
could not be an actual fix but another non-standard sixaxis quirk; as I
said, I'll need to check this assumption against the document in [1].

> Pascal
> 
> [1] http://www.bluetooth.com/Specification%20Documents/HID_SPEC_V10.pdf
> 

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?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: hidp_output_raw_report, HID_OUTPUT_REPORT and Sixaxis

[pascal]

Antonio Ospite wrote:
> The non-standard USB behavior made me think that the patch proposed
> could not be an actual fix but another non-standard sixaxis quirk;

That sounds right; I hadn't noticed hid-sony.c overriding
usbhid_output_raw_report().  Maybe it should simply override
hidp_output_raw_report() as well.

And maybe the default hidp_output_raw_report() should be
patched to behave like the default usbhid_output_raw_report(),
for the sake of consistency.

I am not sure you will find a final answer in the HID spec, but
the default usbhid_output_raw_report() certainly makes sense:
Send OUTPUT reports as interrupt messages because this is more
efficient e.g. for streaming reports to force-feedback devices;
and use the control channel only when the spec leaves no other
choice, i.e. for other report types.

Of course there is also the option of adding a specific function
in struct hid_device for transactions on the control channel.
That would be a good opportunity to enforce section 7.9.1
("no more than one outstanding request on the control channel")
and to implement callbacks for handshake results; but currently
nobody needs these features AFAIK.

Pascal

Re: hidp_output_raw_report, HID_OUTPUT_REPORT and Sixaxis

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 2367 bytes --]

On Tue, 30 Nov 2010 18:40:03 +0100
"pascal-dXI0m6hRz7k@public.gmane.org" <pascal-dXI0m6hRz7k@public.gmane.org> 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?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: hidp_output_raw_report, HID_OUTPUT_REPORT and Sixaxis

[Jiri Kosina]

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.

[PATCH 0/2] Fix sending Output reports to the Sony Sixaxis

[Antonio Ospite]

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?

[PATCH 1/2] hid-sony.c: Fix sending Output reports to the Sixaxis

[Antonio Ospite]

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 <ospite@studenti.unina.it>
---
 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

[PATCH 2/2] bt hidp: send Output reports using SET_REPORT on the Control channel

[Antonio Ospite]

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 <ospite@studenti.unina.it>
---
 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

Re: [PATCH 2/2] bt hidp: send Output reports using SET_REPORT on the Control channel

[Alan Ott]

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 <alan-yzvJWuRpmD1zbRFIqnYvSA@public.gmane.org>

Alan.

Re: [PATCH 1/2] hid-sony.c: Fix sending Output reports to the Sixaxis

[Jiri Kosina]

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 <ospite@studenti.unina.it>

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.

Re: [PATCH 2/2] bt hidp: send Output reports using SET_REPORT on the Control channel

[Jiri Kosina]

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 <alan@signal11.us>

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.

Re: [PATCH 2/2] bt hidp: send Output reports using SET_REPORT on the Control channel

[Gustavo F. Padovan]

Hi Jiri,

* Jiri Kosina <jkosina@suse.cz> [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 <alan@signal11.us>
> 
> 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 <padovan@profusion.mobi>

-- 
Gustavo F. Padovan
http://profusion.mobi

Re: [PATCH 2/2] bt hidp: send Output reports using SET_REPORT on the Control channel

[Jiri Kosina]

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 <alan-yzvJWuRpmD1zbRFIqnYvSA@public.gmane.org>
> > 
> > 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 <padovan-Y3ZbgMPKUGA34EUeqzHoZw@public.gmane.org>

Perfect, applied, thanks.

-- 
Jiri Kosina
SUSE Labs, Novell Inc.