On Mon, 23 Apr 2012, Henrik Rydberg wrote:
> Hi Jiri,
>
> Here is the third version of the extension to the device-driver
> matching mechanism. AFAICT, there are no outstanding issues. I think
> we are getting close. :-)
I have now updated the 'might-rebase/autoloading' branch of git.hid tree
(this is a rebasing branch, so simple pull will not work for you as a
fast-forward, you'll have to delete and repull the branch).
It's currently in a shape I would be okay to pushing out for -next and
scheduling for next merge window, and I will do so tomorrow if nothing
else pops up during testing or reviews.
Thanks everybody, especially Henrik of course, for excellent work on this.
--
Jiri Kosina
SUSE Labs
Hi Jiri,
> I have now updated the 'might-rebase/autoloading' branch of git.hid tree
> (this is a rebasing branch, so simple pull will not work for you as a
> fast-forward, you'll have to delete and repull the branch).
>
> It's currently in a shape I would be okay to pushing out for -next and
> scheduling for next merge window, and I will do so tomorrow if nothing
> else pops up during testing or reviews.
The branch looks good in this end.
Thanks,
Henrik
On Tue, May 1, 2012 at 1:03 PM, Jiri Kosina <[email protected]> wrote:
> On Mon, 23 Apr 2012, Henrik Rydberg wrote:
>
>> Hi Jiri,
>>
>> Here is the third version of the extension to the device-driver
>> matching mechanism. AFAICT, there are no outstanding issues. I think
>> we are getting close. :-)
>
> I have now updated the 'might-rebase/autoloading' branch of git.hid tree
> (this is a rebasing branch, so simple pull will not work for you as a
> fast-forward, you'll have to delete and repull the branch).
>
> It's currently in a shape I would be okay to pushing out for -next and
> scheduling for next merge window, and I will do so tomorrow if nothing
> else pops up during testing or reviews.
Hi Jiri,
Well, I'm back in my lab this morning, and I'm testing your branch
against the different devices we have here. I'm observing a strange
problem: when connecting some devices, the first touch is always at
0,0. If I rmmod and insmod hid_multitouch it seems to work after....
I'm investigating this bug now. I'll tell you when I'm done. I still
don't now if it's related to Henrik's changes or 3.4-rc4.
Cheers,
Benjamin
>
> Thanks everybody, especially Henrik of course, for excellent work on this.
>
> --
> Jiri Kosina
> SUSE Labs
On Wed, 2 May 2012, Benjamin Tissoires wrote:
> > It's currently in a shape I would be okay to pushing out for -next and
> > scheduling for next merge window, and I will do so tomorrow if nothing
> > else pops up during testing or reviews.
>
> Hi Jiri,
>
> Well, I'm back in my lab this morning, and I'm testing your branch
> against the different devices we have here. I'm observing a strange
> problem: when connecting some devices, the first touch is always at
> 0,0. If I rmmod and insmod hid_multitouch it seems to work after....
>
> I'm investigating this bug now. I'll tell you when I'm done. I still
> don't now if it's related to Henrik's changes or 3.4-rc4.
I see, thanks a lot for the heads-up, I will be waiting for your analysis
of this.
I have already renamed the branch to 'device-groups' and merged it into
branch that goes into next, so please send me any followup patches on top
of that queue now.
Thanks,
--
Jiri Kosina
SUSE Labs
On Wed, May 2, 2012 at 10:45 AM, Jiri Kosina <[email protected]> wrote:
> On Wed, 2 May 2012, Benjamin Tissoires wrote:
>
>> > It's currently in a shape I would be okay to pushing out for -next and
>> > scheduling for next merge window, and I will do so tomorrow if nothing
>> > else pops up during testing or reviews.
>>
>> Hi Jiri,
>>
>> Well, I'm back in my lab this morning, and I'm testing your branch
>> against the different devices we have here. I'm observing a strange
>> problem: when connecting some devices, the first touch is always at
>> 0,0. If I rmmod and insmod hid_multitouch it seems to work after....
>>
>> I'm investigating this bug now. I'll tell you when I'm done. I still
>> don't now if it's related to Henrik's changes or 3.4-rc4.
>
> I see, thanks a lot for the heads-up, I will be waiting for your analysis
> of this.
>
> I have already renamed the branch to 'device-groups' and merged it into
> branch that goes into next, so please send me any followup patches on top
> of that queue now.
Ok, I think I understood: nothing related to Henrik's changes. The bug
is also present in the upstream kernel.
In the function set_last_slot_field, I was doing a:
test_bit(usage->hid, hi->input->absbit);
for instance, 0x10030 (X hid field) in the absbit bit field which
contains only ABS_CNT (0x40) bits.... That's why it was a random bug.
Now I understood why Ubuntu complained about this test ;-)
I won't have the time to submit the patch right now, I have to give a
lecture this afternoon. Maybe this evening or tomorrow.
Cheers,
Benjamin
>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs
On Wed, May 2, 2012 at 12:55 PM, Benjamin Tissoires
<[email protected]> wrote:
> On Wed, May 2, 2012 at 10:45 AM, Jiri Kosina <[email protected]> wrote:
>> On Wed, 2 May 2012, Benjamin Tissoires wrote:
>>
>>> > It's currently in a shape I would be okay to pushing out for -next and
>>> > scheduling for next merge window, and I will do so tomorrow if nothing
>>> > else pops up during testing or reviews.
>>>
>>> Hi Jiri,
>>>
>>> Well, I'm back in my lab this morning, and I'm testing your branch
>>> against the different devices we have here. I'm observing a strange
>>> problem: when connecting some devices, the first touch is always at
>>> 0,0. If I rmmod and insmod hid_multitouch it seems to work after....
>>>
>>> I'm investigating this bug now. I'll tell you when I'm done. I still
>>> don't now if it's related to Henrik's changes or 3.4-rc4.
>>
>> I see, thanks a lot for the heads-up, I will be waiting for your analysis
>> of this.
>>
>> I have already renamed the branch to 'device-groups' and merged it into
>> branch that goes into next, so please send me any followup patches on top
>> of that queue now.
>
> Ok, I think I understood: nothing related to Henrik's changes. The bug
> is also present in the upstream kernel.
> In the function set_last_slot_field, I was doing a:
> test_bit(usage->hid, hi->input->absbit);
>
> for instance, 0x10030 (X hid field) in the absbit bit field which
> contains only ABS_CNT (0x40) bits.... That's why it was a random bug.
> Now I understood why Ubuntu complained about this test ;-)
>
> I won't have the time to submit the patch right now, I have to give a
> lecture this afternoon. Maybe this evening or tomorrow.
>
> Cheers,
> Benjamin
Hi guys,
I'm currently on the bug fix I told you earlier. However, I found a
more problematic bug in the hid_groups functionality.
Some device, like the Perixx peripad, present several interfaces
(mouse, keyboard and multitouch).
The hid groups functionality detects the HID field Contact ID, and
then forwards all interfaces to hid-multitouch. The point is that
hid-multitouch does not know how to handle mice and keyboards, and
then fails handling the interfaces of the device.
This particular device is then fully broken (as anyone pumped the events).
I also noticed the same problem (but less problematic) with cypress
panels: it presents different vendor interfaces and they are handled
by hid-multitouch.
Sorry for spotting this that late,
Benjamin
>
>>
>> Thanks,
>>
>> --
>> Jiri Kosina
>> SUSE Labs
> I'm currently on the bug fix I told you earlier. However, I found a
> more problematic bug in the hid_groups functionality.
>
> Some device, like the Perixx peripad, present several interfaces
> (mouse, keyboard and multitouch).
> The hid groups functionality detects the HID field Contact ID, and
> then forwards all interfaces to hid-multitouch. The point is that
> hid-multitouch does not know how to handle mice and keyboards, and
> then fails handling the interfaces of the device.
I am a bit unclear as to which devices this applies to, but I see two
possible solutions:
1) Add the devices in question back to the have_special_drivers list.
2) Add the interface type to the group descision, which should
probably be done anyway. I have a patch in the pipe that, will send it
later today.
> This particular device is then fully broken (as anyone pumped the events).
> I also noticed the same problem (but less problematic) with cypress
> panels: it presents different vendor interfaces and they are handled
> by hid-multitouch.
It would be great if you could test soution 1) before on a device -
something seems wrong if those interfaces were handled by hid-generic
before, but before getting the logic straight, it does not hurt to
try. :-)
Thanks,
Henrik
On Thu, May 3, 2012 at 2:23 PM, Henrik Rydberg <[email protected]> wrote:
>> I'm currently on the bug fix I told you earlier. However, I found a
>> more problematic bug in the hid_groups functionality.
>>
>> Some device, like the Perixx peripad, present several interfaces
>> (mouse, keyboard and multitouch).
>> The hid groups functionality detects the HID field Contact ID, and
>> then forwards all interfaces to hid-multitouch. The point is that
>> hid-multitouch does not know how to handle mice and keyboards, and
>> then fails handling the interfaces of the device.
>
> I am a bit unclear as to which devices this applies to, but I see two
> possible solutions:
>
> 1) Add the devices in question back to the have_special_drivers list.
Well... The device presents valid mouse and keyboard interface that
should be handled by hid-generic.
The behavior of this particular device is the following:
- when 1 finger is in use, then it sends events over the mouse interface
- when 2 fingers are present, it sends events over the multitouch interface
- when you physically trigger the switch mode button, a keyboard
appears and it sends key events over the keyboard interface, and
eventually mouse events if you press the "mouse" key.... ;-)
This crap is all inherited by the fact that Microsoft do not want to
handle indirect touch, and the device maker found this solution to
counter this.
To sum up, adding it to the have_special_drivers driver list won't
work as we need part of the device to be handled by hid-generic.
>
> 2) Add the interface type to the group descision, which should
> probably be done anyway. I have a patch in the pipe that, will send it
> later today.
A simpler solution consists in adding the macros HID_USB_MT_DEVICE(v,
p) and HID_BLUETOOTH_MT_DEVICE(v, p) as you had introduced in a
earlier patch (I don't know why it disappeared).
The problem came out because:
- hid-multitouch registered the triplet BUS_USB / VID / PID.
- For each interface, it asks udev (or the kernel) which driver to
use, and whatever .group was, it was always hid-multitouch that came
out.
So it's just safer to specify the group for all multitouch devices.
Cheers,
Benjamin
>
>> This particular device is then fully broken (as anyone pumped the events).
>> I also noticed the same problem (but less problematic) with cypress
>> panels: it presents different vendor interfaces and they are handled
>> by hid-multitouch.
>
> It would be great if you could test soution 1) before on a device -
> something seems wrong if those interfaces were handled by hid-generic
> before, but before getting the logic straight, it does not hurt to
> try. :-)
>
> Thanks,
> Henrik
> > 1) Add the devices in question back to the have_special_drivers list.
>
> Well... The device presents valid mouse and keyboard interface that
> should be handled by hid-generic.
> The behavior of this particular device is the following:
> - when 1 finger is in use, then it sends events over the mouse interface
> - when 2 fingers are present, it sends events over the multitouch interface
> - when you physically trigger the switch mode button, a keyboard
> appears and it sends key events over the keyboard interface, and
> eventually mouse events if you press the "mouse" key.... ;-)
>
> This crap is all inherited by the fact that Microsoft do not want to
> handle indirect touch, and the device maker found this solution to
> counter this.
>
> To sum up, adding it to the have_special_drivers driver list won't
> work as we need part of the device to be handled by hid-generic.
So was this particular device never listed in have_special_drivers?
> > 2) Add the interface type to the group descision, which should
> > probably be done anyway. I have a patch in the pipe that, will send it
> > later today.
>
> A simpler solution consists in adding the macros HID_USB_MT_DEVICE(v,
> p) and HID_BLUETOOTH_MT_DEVICE(v, p) as you had introduced in a
> earlier patch (I don't know why it disappeared).
No, the specific entries in the hid-multitouch device list matches any
group, so those defines were simplified away in the second version.
> The problem came out because:
> - hid-multitouch registered the triplet BUS_USB / VID / PID.
> - For each interface, it asks udev (or the kernel) which driver to
> use, and whatever .group was, it was always hid-multitouch that came
> out.
>
> So it's just safer to specify the group for all multitouch devices.
This is still confusing. I thought the real problem was that the
non-mt interfaces do not match hid-generic. Solution 2) should take
care of that. What I don't understand is how those other interfaces
came to be handled by hid-generic before this patch, unless this
device was never listed in have_special_driver.
Are we talking about USB_DEVICE_ID_TOPSEED2_PERIPAD_701 here?
Henrik
On Thu, May 3, 2012 at 3:19 PM, Henrik Rydberg <[email protected]> wrote:
>> > 1) Add the devices in question back to the have_special_drivers list.
>>
>> Well... The device presents valid mouse and keyboard interface that
>> should be handled by hid-generic.
>> The behavior of this particular device is the following:
>> - when 1 finger is in use, then it sends events over the mouse interface
>> - when 2 fingers are present, it sends events over the multitouch interface
>> - when you physically trigger the switch mode button, a keyboard
>> appears and it sends key events over the keyboard interface, and
>> eventually mouse events if you press the "mouse" key.... ;-)
>>
>> This crap is all inherited by the fact that Microsoft do not want to
>> handle indirect touch, and the device maker found this solution to
>> counter this.
>>
>> To sum up, adding it to the have_special_drivers driver list won't
>> work as we need part of the device to be handled by hid-generic.
>
> So was this particular device never listed in have_special_drivers?
No, and that's the way it should (not being part of have_special_driver).
>
>> > 2) Add the interface type to the group descision, which should
>> > probably be done anyway. I have a patch in the pipe that, will send it
>> > later today.
>>
>> A simpler solution consists in adding the macros HID_USB_MT_DEVICE(v,
>> p) and HID_BLUETOOTH_MT_DEVICE(v, p) as you had introduced in a
>> earlier patch (I don't know why it disappeared).
>
> No, the specific entries in the hid-multitouch device list matches any
> group, so those defines were simplified away in the second version.
disagree: a device can present several interface (because it has
several "devices") and only those presenting Contact ID can and should
be handled by hid-multitouch.
Cypress for instance presents one interface for the multitouch layer,
and one other for specific controls that are seen as a keyboard.
However, in this particular case, I'm not sure we want to show this
interface to the end user.... ;-)
>
>> The problem came out because:
>> - hid-multitouch registered the triplet BUS_USB / VID / PID.
>> - For each interface, it asks udev (or the kernel) which driver to
>> use, and whatever .group was, it was always hid-multitouch that came
>> out.
>>
>> So it's just safer to specify the group for all multitouch devices.
>
> This is still confusing. I thought the real problem was that the
> non-mt interfaces do not match hid-generic. Solution 2) should take
> care of that. What I don't understand is how those other interfaces
> came to be handled by hid-generic before this patch, unless this
> device was never listed in have_special_driver.
The think is that they do match hid-generic (they get the group
HID_GROUP_GENERIC).
However they also match hid-multitouch (as hid-multitouch does not ask
for a particular group). So, if hid-multitouch is loaded __before__
hid-generic, it will be given the device whatever the match with
hid-generic.
And again, yes it was never listed in have_special_driver.
>
> Are we talking about USB_DEVICE_ID_TOPSEED2_PERIPAD_701 here?
yep
- For consistency, I'd rather specifying the group for any devices.
This because hid-multitouch can not handle other interfaces than
multitouch one. Though the catchall is interesting in the sense that
it may help us to hide unwanted interfaces.
- For backward compatibility, we should adapt each device (currently,
I only spotted this particular one) to decide if we need to catch the
group or not.
Jiri, any thought?
Benjamin
>
> Henrik
> >> > 2) Add the interface type to the group descision, which should
> >> > probably be done anyway. I have a patch in the pipe that, will send it
> >> > later today.
> >>
> >> A simpler solution consists in adding the macros HID_USB_MT_DEVICE(v,
> >> p) and HID_BLUETOOTH_MT_DEVICE(v, p) as you had introduced in a
> >> earlier patch (I don't know why it disappeared).
> >
> > No, the specific entries in the hid-multitouch device list matches any
> > group, so those defines were simplified away in the second version.
>
> disagree: a device can present several interface (because it has
> several "devices") and only those presenting Contact ID can and should
> be handled by hid-multitouch.
Obviously this is only a problem for the devices with mixed
interfaces, but for those, solution 2) together with specifying the
group as you suggest should work. We can definitely change all devices
in the list, it just was not necessary before (or so I thought).
> The think is that they do match hid-generic (they get the group
> HID_GROUP_GENERIC).
> However they also match hid-multitouch (as hid-multitouch does not ask
> for a particular group). So, if hid-multitouch is loaded __before__
> hid-generic, it will be given the device whatever the match with
> hid-generic.
I suppose what you describe here is how it was working before the
device groups. Ok, I see what to do now. I will be back shortly with a
patch which should make USB_DEVICE_ID_TOPSEED2_PERIPAD_701 work, and
let's take it from there.
Thanks for testing,
Henrik
On Thu, May 3, 2012 at 3:54 PM, Henrik Rydberg <[email protected]> wrote:
>> >> > 2) Add the interface type to the group descision, which should
>> >> > probably be done anyway. I have a patch in the pipe that, will send it
>> >> > later today.
>> >>
>> >> A simpler solution consists in adding the macros HID_USB_MT_DEVICE(v,
>> >> p) and HID_BLUETOOTH_MT_DEVICE(v, p) as you had introduced in a
>> >> earlier patch (I don't know why it disappeared).
>> >
>> > No, the specific entries in the hid-multitouch device list matches any
>> > group, so those defines were simplified away in the second version.
>>
>> disagree: a device can present several interface (because it has
>> several "devices") and only those presenting Contact ID can and should
>> be handled by hid-multitouch.
>
> Obviously this is only a problem for the devices with mixed
> interfaces, but for those, solution 2) together with specifying the
> group as you suggest should work. We can definitely change all devices
> in the list, it just was not necessary before (or so I thought).
>
>> The think is that they do match hid-generic (they get the group
>> HID_GROUP_GENERIC).
>> However they also match hid-multitouch (as hid-multitouch does not ask
>> for a particular group). So, if hid-multitouch is loaded __before__
>> hid-generic, it will be given the device whatever the match with
>> hid-generic.
>
> I suppose what you describe here is how it was working before the
> device groups.
Just to be clear: no, this last paragraph occurs after device groups.
Before, it was working as hid-generic catch the device first, then
released it while looking at the report descriptors. Now, it's purely
dependent on which driver is loaded first for this particaular case.
Benjamin
> Ok, I see what to do now. I will be back shortly with a
> patch which should make USB_DEVICE_ID_TOPSEED2_PERIPAD_701 work, and
> let's take it from there.
>
> Thanks for testing,
> Henrik
> > I suppose what you describe here is how it was working before the
> > device groups.
>
> Just to be clear: no, this last paragraph occurs after device groups.
> Before, it was working as hid-generic catch the device first, then
> released it while looking at the report descriptors. Now, it's purely
> dependent on which driver is loaded first for this particaular case.
Thanks for the clarification, and I agree. It also means you are
completely right, it will suffice to change those devices from the
wildcard group to the multitouch group. I don't mind switching them
all.
Thanks,
Henrik