On Fri, Jan 17, 2014 at 05:54:33PM -0800, Ping Cheng wrote:
> On Thu, Jan 16, 2014 at 8:49 PM, Peter Hutterer
> <[email protected]>wrote:
>
> > On Tue, Jan 14, 2014 at 04:27:01PM -0800, Ping Cheng wrote:
> > > On Mon, Jan 13, 2014 at 1:45 AM, Peter Hutterer <
> > [email protected]>wrote:
> > >
> > > > On Wed, Jan 08, 2014 at 02:50:11PM -0800, Ping Cheng wrote:
> > > > > wcmLinkTouchAndPen only assigned touch to tablet (pen) interface.
> > > > > New Intuos series requires another tool (pad in this case) on the
> > > > > same interface as touch to access touch tool as well. That is, touch
> > > > > tool should be assigned to wcmTouchDevice for its own common struct.
> > > >
> > > > I tried to review this patch but I had a really hard time parsing the
> > above
> > > > and my head is spinning from trying to grasp from the diff. Can you
> > > > rephrase
> > > > what this does, it'll get easer to review then. sorry
> > > >
> > >
> > > Before rephrasing the commit comment, I'd like to explain why we need the
> > > patch.
> > >
> > > This patch is to support patch 2/2 v2. It addresses your suggestion of
> > > removing the for-loop in wcmUpdateHWTouchProperty, to my initial "Support
> > > hardware touch switch" patch.
> > >
> > > For those who have relatively short memories, here is the history. For
> > USB
> > > devices that support both touch and pen, we have two interfaces, one for
> > > touch and the other for pen. So, for each dual input device, we have two
> > > WacomCommon structs. These two stucts do not share tools. In order for
> > pen
> > > tools to tell touch to ignore its events when pen is in prox, we added
> > > routine wcmLinkTouchAndPen to assign touch as wcmTouchDevice to the
> > > WacomCommonPtr of pen. That was good.
> > >
> > > However, we now want to replace priv with something like
> > > common->wcmTouchDevice in wcmUpdateHWTouchProperty. Without assigning
> > touch
> > > to touch interface's common->wcmTouchDevice, driver crashes since
> > > wcmTouchDevice is NULL (unassigned).
> >
> > thanks for the explanation. so if I understand this correctly, you want to
> > have a bi-directional link between the two 'common' devices instead of a
> > single-directional link from tablet to touch. If that's correct please
> > rename wcmTouchDevice to wcmLinkedDevice or something, with a bunch of
> > comments explaining what it links to.
> >
>
> Yes, you get this part right.
>
> Updating function name is not a big deal once we resolve other issues below.
>
>
> > > If you wonder which priv in usbDispatchEvents is receiving SW_MUTE_DEVICE
> > > event, it was PAD tool. Newer Intuos series has PAD and touch tools on
> > the
> > > same interface, same as those Bamboos.
> >
> > hmm, so if the event is received on the pad device, which already has a
> > link
> > to the touch device's common struct why do we need to update this? I must
> > be
> > missing something else here.
> >
>
> Even we know touch is on the same interface with pad (which is not always
> true), the struct common does not give us the pointer to access the touch
> tool. We have to go through the whole list of tools associated with common
> to find touch every time when we need to update TouchSwitchState. So, the
> repeated for-loop can not be avoided unless we find and KEEP it during
> initialization steps in wcmConfig.c.
sorry, what I mean't wasn't the "common" struct but the
common->wcmTouchDevice. That should point to the actual touch device, right?
I don't quite understand yet why something like this isn't sufficient in the
event handling code:
case SW_MUTE:
WacomDevicePtr touch;
if (IsTouch(device))
touch = device;
else
touch = device->common->wcmTouchDevice;
wcmUpdateHWSWitchProperty(touch);
> > > Does this information put your head to a proper position? If not, please
> > > give me your exact position, I'll try my best to locate it ;-).
> >
> > there are more questions:
> > first - there are two loops in wcmLinkTouchAndPen() but you only update the
> > second loop, is that intentional?
>
>
> Yes.
>
> There are no pen and touch devices with different product ids that support
> touch switch yet.
>
>
> > if so, why, and please add a comment to
> > that extent. and if it's not too much hassle imo it's better to update both
> > loops, just because it'll make it easier to understand.
> >
>
> Maybe we want to be proactive for this support?
yes, definitely. it's not something that's obvious when reading it and it
doesn't cost us much to run a quick loop here.
> > second - that second loops is never executed. Either we link and return, or
> > we don't link in which case "device" is NULL and the loop is never entered.
> > do we still need that second for loop?
> >
>
> That is a bug, unrelated to the patch we are discussing. "device =
> xf86FirstLocalDevice();" is needed for the second loop.
>
>
> if the answer is that both need to be updated/fixed, then I'd like you to
> > move the loops into a helper function with a boolean for compare_id.
> > they're
> > identical except for the tablet_id check and so you could do something like
> >
> > if (!wcmLinkDevices(pInfo, TRUE)) /* compare tablet_ids */
> > wcmLinkDevices(pInfo, FALSE); /* ok, try to link without comparing */
> >
> > it'll reduce the code and make it easier to spot bugs like the one
> > mentioned
> > above.
> >
>
> I can do that. The question is, should we consider different product ids
> now?
>
> I think it is a better choice to consider it now. Just need your
> confirmation.
yes, I agree.
>
> (also, I'm not a big fan of loop bodies containing a single if statement,
> > it's usually better to have, in this case, a if (not wacom driver)
> > continue;
> > to enhance readability.
> >
>
> I have no preferences. So, either way, I can update it.
yes please.
Cheers,
Peter
> > > > >
> > > > > The use of IsTablet is necessary to guarantee we link pen and touch
> > > > > tools.
> > > > >
> > > > > Signed-off-by: Ping Cheng <[email protected]>
> > > > > ---
> > > > > src/wcmConfig.c | 21 ++++++++++++++++-----
> > > > > 1 file changed, 16 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/src/wcmConfig.c b/src/wcmConfig.c
> > > > > index 2d19944..6891ca6 100644
> > > > > --- a/src/wcmConfig.c
> > > > > +++ b/src/wcmConfig.c
> > > > > @@ -388,6 +388,11 @@ wcmInitModel(InputInfoPtr pInfo)
> > > > > /**
> > > > > * Link the touch tool to the pen of the same device
> > > > > * so we can arbitrate the events when posting them.
> > > > > + * As a by-product, we also assign the touch tool to
> > > > > + * its own common wcmTouchDevice so other tool on the
> > > > > + * same interface can access it, such as those devices
> > > > > + * that have expresskeys (pad) and touch on the same
> > > > > + * interface like Bamboo and new Intuos series.
> > > > > */
> > > > > static void wcmLinkTouchAndPen(InputInfoPtr pInfo)
> > > > > {
> > > > > @@ -406,18 +411,24 @@ static void wcmLinkTouchAndPen(InputInfoPtr
> > pInfo)
> > > > > tmpcommon = tmppriv->common;
> > > > >
> > > > > /* skip the same tool or already linked
> > devices */
> > > > > - if ((tmppriv == priv) ||
> > tmpcommon->wcmTouchDevice)
> > > > > + if ((tmppriv == priv) ||
> > > > > + (tmpcommon->wcmTouchDevice &&
> > > > IsTablet(tmppriv)))
> > > > > continue;
> > > > >
> > > > > if (tmpcommon->tablet_id == common->tablet_id)
> > > > > {
> > > > > - if (IsTouch(tmppriv) && IsTablet(priv))
> > > > > + if (IsTouch(tmppriv))
> > > > > + {
> > > > > common->wcmTouchDevice =
> > tmppriv;
> > > > > - else if (IsTouch(priv) &&
> > > > IsTablet(tmppriv))
> > > > > + tmpcommon->wcmTouchDevice =
> > > > tmppriv;
> > > > > + } else if (IsTouch(priv))
> > > > > + {
> > > > > tmpcommon->wcmTouchDevice =
> > priv;
> > > > > + common->wcmTouchDevice = priv;
> > > > > + }
> > > > >
> > > > > - if (common->wcmTouchDevice ||
> > > > > -
> > tmpcommon->wcmTouchDevice)
> > > > > + if ((common->wcmTouchDevice &&
> > > > IsTablet(priv)) ||
> > > > > + (tmpcommon->wcmTouchDevice &&
> > > > IsTablet(tmppriv)))
> > > > > {
> > > > > TabletSetFeature(common,
> > > > WCM_PENTOUCH);
> > > > > TabletSetFeature(tmpcommon,
> > > > WCM_PENTOUCH);
> > > > > --
> > > > > 1.8.3.2
> > > > >
> > > >
> >
------------------------------------------------------------------------------
CenturyLink Cloud: The Leader in Enterprise Cloud Services.
Learn Why More Businesses Are Choosing CenturyLink Cloud For
Critical Workloads, Development Environments & Everything In Between.
Get a Quote or Start a Free Trial Today.
http://pubads.g.doubleclick.net/gampad/clk?id=119420431&iu=/4140/ostg.clktrk
_______________________________________________
Linuxwacom-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
