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.
>
> > 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?
> 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.
(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.
Ping
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
