Re: Saitek PS1000 gamepad - HID descriptor wonky

Re: Saitek PS1000 gamepad - HID descriptor wonky

[simon]

Hi Andreas,

> I've attached the hid report descriptor in binary and spec format.

As you probably noticed this HID descriptor is a little 'weird'.

First it defines 6 usages
--
        Usage (X),                  ; X (30h, dynamic value)
        Usage (Y),                  ; Y (31h, dynamic value)
        Usage (Z),                  ; Z (32h, dynamic value)
        Usage (Rz),                 ; Rz (35h, dynamic value)
        Usage (Rx),                 ; Rx (33h, dynamic value)
        Usage (Slider),             ; Slider (36h, dynamic value)
--

but then only says they are 5 inputs
--
        Report Count (5),
--

I'm guessing that if you adjust the report count to '6' then your 6th axis
might start working, and possibly the hat-switch and buttons too...

So the next question has to be how committed to your ($15??) game pad are
you, and are you will to go through the pain of building/submitting
patches? It's a great opportunity to put 'kernel dev' on your resume ;-)


I'll have to defer to the other kernel devs....

Q. Is there a way of patching a HID descriptor without writing a driver?


Otherwise you could look at 'drivers/hid/hid-elecom.c' as a template for
your patch.

Good luck, and feel free to ask questions,
Simon.

Re: Saitek PS1000 gamepad - HID descriptor wonky

[Andreas Hübner]

Hi Simon,

> As you probably noticed this HID descriptor is a little 'weird'.
>
> First it defines 6 usages
> [...]
> but then only says they are 5 inputs

Indeed. Actually, the controller only has 5 axes. Two analog sticks (4 axes)
and a throttle that is linked to the trigger buttons. Pressing the left
trigger yields a negative axis value and the right trigger produces
positive values. (I assume it's for controlling the acceleration, etc.)
This axis also has a threshold in the positive and negative range each
triggering a button press.

> So the next question has to be how committed to your ($15??) game pad are
> you, and are you will to go through the pain of building/submitting
> patches? It's a great opportunity to put 'kernel dev' on your resume ;-)

Well, I'm always happy when I can hack some code, so count me in. :)
Usually, the only problem for me is to get a decent setup to start
debugging.

But I guess, I'll have to learn a bit more about HID descriptors before
I can start hacking.

> Otherwise you could look at 'drivers/hid/hid-elecom.c' as a template for
> your patch.

Will take a look at it - thanks!


Andreas

Re: Saitek PS1000 gamepad - HID descriptor wonky

[Andreas Hübner]

Hi Simon!

Let's continue the discussion on the list. I did made some progress
that requires input from the HID maintainers.

> You have to check the rsize, otherwise you might have problems writing
> outside the size of the original descriptor.

That's exactly what I've been doing, or did I miss something?
In fact, I wanted to be absolutely sure and only mess with the descriptor
when the sizes match.  (*rsize == 137)

> Note a D-Pad might be defined as 'Buttons', a 'D-Pad' or as a 'Hat'. You
> need to check out the values change in the HID stream.

It's defined as a 'Hat' with 8 different directions.

> Send me a copy of the original descriptor and I'll have a look.

Should already be on the list. And I was able to identify the culprit so
no need to bother you.

The problem is the following code in hidinput_configure_usage
(drivers/hid/hid-input.c):

	if (field->flags & HID_MAIN_ITEM_CONSTANT)
		goto ignore;

The buttons and the hat are defined with the constant flag enabled.
Now I might be reading the spec wrong, but I'm not so sure that we can
simply skip constant values.
This is the description of the data/constant flag.

    Indicates whether the item is data or a constant
    value. Data indicates the item is defining report
    fields that contain modifiable device data. Constant
    indicates the item is a static read-only field in a
    report and cannot be modified (written) by the
    host.


Well, I need to patch the descriptor nevertheless because it defines a
non-existing sixth axis. Might as well remove the constant flag on the
buttons and hat. However, other devices might be affected as well, so
I'd like to get some input on how to deal with the situation.


Andreas

Re: Saitek PS1000 gamepad - HID descriptor wonky

[simon]

>> You have to check the rsize, otherwise you might have problems writing
>> outside the size of the original descriptor.
>
> That's exactly what I've been doing, or did I miss something?

> In fact, I wanted to be absolutely sure and only mess with the descriptor
> when the sizes match.  (*rsize == 137)

Err.. must have been sleepy when I looked over your code. Don't know
whether '>=' is better than '==', all other modules do this and I wonder
whether the kernel can get a larger descriptor block by some mechanism.


> Should already be on the list. And I was able to identify the culprit so
> no need to bother you.
>
> The problem is the following code in hidinput_configure_usage
> (drivers/hid/hid-input.c):
>
> 	if (field->flags & HID_MAIN_ITEM_CONSTANT)
> 		goto ignore;
>
> The buttons and the hat are defined with the constant flag enabled.
> Now I might be reading the spec wrong, but I'm not so sure that we can
> simply skip constant values.

Quite obviously if the buttons change value in the HID stream then they
are not 'CONSTANT', personally I would patch this part of the HID
descriptor so they are just defined as 'input variable'.

> Well, I need to patch the descriptor nevertheless because it defines a
> non-existing sixth axis. Might as well remove the constant flag on the
> buttons and hat. However, other devices might be affected as well, so
> I'd like to get some input on how to deal with the situation.

Not sure why you would think other devices might be affected. Patching the
HID descriptor should be limited to this USBID pair, and even only active
if the device provided matches the template you use to trigger patching.

Some drivers provide a complete/new descriptor where patching is
complicated, although you gain extra hacker-points if you can achieve the
same with a minimal patch :-)

Simon.

Re: Saitek PS1000 gamepad - HID descriptor wonky

[Andreas Hübner]

On Mon, Dec 12, 2011 at 01:49:59PM -0500, simon@mungewell.org wrote:
> Err.. must have been sleepy when I looked over your code. Don't know
> whether '>=' is better than '==', all other modules do this and I wonder
> whether the kernel can get a larger descriptor block by some mechanism.

Well, this would be kind of strange, wouldn't it?
But I can change it to '>=' if it makes you happy. :)


> Quite obviously if the buttons change value in the HID stream then they
> are not 'CONSTANT', personally I would patch this part of the HID
> descriptor so they are just defined as 'input variable'.

While I personally agree with that interpretation, the spec only states
that constant fields can not be written by the host. (I know, this
doesn't make any sense for an input report.)

Somewhere I read that a constant field with no usage assigned to it
should be treated as padding. But in this case, there is a usage
assigned to.
And this is what I'd actually do in hidinput_configure_usage: only goto
ignore if there is no usage associated with the field.

On the other hand, the gamepad example in the HID Usage Tables doesn't
use the constant flag for buttons.


> Not sure why you would think other devices might be affected. Patching the
> HID descriptor should be limited to this USBID pair, and even only active
> if the device provided matches the template you use to trigger patching.

I was worrying that other devices might declare some non-padding input
fields as constants. So if you fix this problem in the generic part
(i. e. hid-input.c) those devices will also start working correctly.


Andreas

Re: Saitek PS1000 gamepad - HID descriptor wonky

[Andreas Hübner]

I'm finally coming back to this after quite some time.

Tried to find more info regarding the issue with the constant bit on those
input reports, but I wasn't able to come up with anything useful.
Windows just seems to ignore it.

Well, it's probably not worth it to spend anymore time on this, so I
prepared a patch that fixes the report descriptor accordingly.

I tried to follow the kernel patch policy, but since this is my first
patch I might have screwed something up. Just let me know.


Andreas