On Fri, Jan 17, 2014 at 05:54:33PM -0800, Ping Cheng wrote:
> On Thu, Jan 16, 2014 at 8:49 PM, Peter Hutterer 
> <peter.hutte...@who-t.net>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 <
> > peter.hutte...@who-t.net>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 <pi...@wacom.com>
> > > > > ---
> > > > >  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
Linuxwacom-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel

Reply via email to