On Fri, Oct 4, 2013 at 1:12 PM, Jason Gerecke <killert...@gmail.com> wrote:
> On Fri, Oct 4, 2013 at 12:33 PM, Ping Cheng <pingli...@gmail.com> wrote:
>> On Fri, Oct 4, 2013 at 11:53 AM, Jason Gerecke <killert...@gmail.com> wrote:
>>> On Thu, Oct 3, 2013 at 5:18 PM, Ping Cheng <pingli...@gmail.com> wrote:
>>>> On touch enabled devices, we miss pad events when nothing touches
>>>> tablet.
>>>>
>>>
>>> This commit message needs to be expanded. I have no idea what what the
>>> underlying problem is, or how this patch fixes it.
>>
>> When we worked on something for too long, we would think everything is
>> so clear and easy to understand ;).
>>
>>> The first hunk
>>> makes sense (setting PAD_ID shouldn't be limited to just tablets that
>>> use generic BTN_n events), but the second hunk doesn't look like it is
>>> doing much of anything (it now always clears the dirty flag on touch
>>> devices, but that shouldn't have any practical effect...)
>>
>> Clear dirty bit is only a by-product. It is the /* walk through all
>> channels */ that makes a difference. With the old logic, for generic
>> devices that support touch and expresskeys (such as Bamboo), when
>> wcmTouch is 0 and we get expresskeys along with touch events, we'll
>> never be able to get into the for-loop.
>>
>> if (ds->device_type != TOUCH_ID || common->wcmTouch)
>>
>> will be false since (ds->device_type == TOUCH_ID) and (common->wcmTouch ==
>> 0).
>>
>> I'll update the commit comments if above explanation satisfies you.
>>
>> Ping
>>
>
> But that shouldn't make a difference. The call to wcmEvent is guarded
> by `if (ds->device_type != TOUCH_ID || common->wcmTouch)` both before
> and after this patch. If touch is disabled and expresskeys come
> through the TOUCH_ID device, then we *still* never send an event.
>
> The reason this patch works is because of the first hunk: the second
> hunk should do nothing. As long as we properly set `ds->device_type =
> PAD_ID` for buttons from a generic device, then the old logic in the
> second hunk should work correctly.
But, with the old logic, ds->device_type never changes. It stays at
TOUCH_ID for the whole loop. In fact, we never get into the loop. We
checked only once up front, which was the root cause.
Ping
>>>> src/wcmUSB.c | 28 ++++++++++++++--------------
>>>> 1 file changed, 14 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/src/wcmUSB.c b/src/wcmUSB.c
>>>> index eaaf854..634eb1c 100644
>>>> --- a/src/wcmUSB.c
>>>> +++ b/src/wcmUSB.c
>>>> @@ -1464,11 +1464,12 @@ static void usbParseBTNEvent(WacomCommonPtr common,
>>>> }
>>>> if (nkeys >= usbdata->npadkeys)
>>>> change = 0;
>>>> - else if (!ds->device_type) /* expresskey pressed
>>>> at startup */
>>>> - ds->device_type = PAD_ID;
>>>> }
>>>>
>>>> channel->dirty |= change;
>>>> +
>>>> + if (!ds->device_type && channel->dirty) /* expresskey pressed at
>>>> startup or missing type */
>>>> + ds->device_type = PAD_ID;
>>>> }
>>>>
>>>> /**
>>>> @@ -1601,7 +1602,7 @@ static Bool usbIsTabletToolInProx(int device_type,
>>>> int proximity)
>>>>
>>>> static void usbDispatchEvents(InputInfoPtr pInfo)
>>>> {
>>>> - int i;
>>>> + int i, c;
>>>> WacomDeviceState *ds;
>>>> struct input_event* event;
>>>> WacomDevicePtr priv = (WacomDevicePtr)pInfo->private;
>>>> @@ -1722,17 +1723,16 @@ static void usbDispatchEvents(InputInfoPtr pInfo)
>>>> if (!ds->proximity)
>>>> private->wcmLastToolSerial = 0;
>>>>
>>>> - /* don't send touch event when touch isn't enabled */
>>>> - if (ds->device_type != TOUCH_ID || common->wcmTouch)
>>>> - {
>>>> - int c;
>>>> - for (c = 0; c < MAX_CHANNELS; c++) {
>>>> - DBG(10, common, "Checking if channel %d is
>>>> dirty...\n", c);
>>>> - if (common->wcmChannel[c].dirty) {
>>>> - DBG(10, common, "Dirty flag set on channel
>>>> %d; sending event.\n", c);
>>>> - common->wcmChannel[c].dirty = FALSE;
>>>> - wcmEvent(common, c,
>>>> &common->wcmChannel[c].work);
>>>> - }
>>>> + for (c = 0; c < MAX_CHANNELS; c++) {
>>>> + ds = &common->wcmChannel[c].work;
>>>> +
>>>> + /* walk through all channels */
>>>> + if (common->wcmChannel[c].dirty) {
>>>> + DBG(10, common, "Dirty flag set on channel %d;
>>>> sending event.\n", c);
>>>> + common->wcmChannel[c].dirty = FALSE;
>>>> + /* don't send touch event when touch isn't enabled
>>>> */
>>>> + if (ds->device_type != TOUCH_ID ||
>>>> common->wcmTouch)
>>>> + wcmEvent(common, c, ds);
>>>> }
>>>> }
>>>> }
>>>> --
>>>> 1.8.1.2
>>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>> October Webinars: Code for Performance
>>>> Free Intel webinars can help you accelerate application performance.
>>>> Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most
>>>> from
>>>> the latest Intel processors and coprocessors. See abstracts and register >
>>>> http://pubads.g.doubleclick.net/gampad/clk?id=60134791&iu=/4140/ostg.clktrk
>>>> _______________________________________________
>>>> Linuxwacom-devel mailing list
>>>> Linuxwacom-devel@lists.sourceforge.net
>>>> https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance.
Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from
the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60134791&iu=/4140/ostg.clktrk
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
