On Thu, Jan 23, 2014 at 10:45 PM, Peter Hutterer
<peter.hutte...@who-t.net>wrote:
> On Wed, Jan 22, 2014 at 08:28:23PM -0800, Ping Cheng wrote:
> > On Tue, Jan 21, 2014 at 8:01 PM, Peter Hutterer
> > <peter.hutte...@who-t.net<javascript:_e({}, 'cvml',
> > 'peter.hutte...@who-t.net');>
> > > wrote:
> >
> > > 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 <javascript:_e({}, 'cvml',
> > > '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 <javascript:_e({}, 'cvml',
> > > '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.
> >
> >
> > I knew what you meant. That was exactly where you missed the "pointer".
> >
> >
> > > That should point to the actual touch device, right?
> >
> >
> > It depends. Don't forget there are two common structs here: one for pen;
> > one for touch. The original code only assigned touch pointer to the pen
> > common struct's wcmTouchDevice. The wcmTouchDevice of touch common struct
> > wasn't updated, which would stay at its initial value = null(0).
> >
> >
> > > 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;
> >
> >
> > Since we are on touch interface here, this common is not the one for pen.
> > So, you can not access wcmTouchDevice from this common without my updated
> > code. That was the main reason I had to make a patch before calling
> > wcmUpdateHWSWitchProperty.
>
> I know, but that's what the condition is for. If we get the SW_MUTE on the
> touch device we already have the device and we can simply update it. If we
> get it on the pad (which shares the common with the pen) the wcmTouchDevice
> should be set and give us the device.
>
> So in my view we have 4 devices that roughly look like this (best viewed in
> monospace):
>
> pen \
> eraser - common(1)
> pad / \
> wcmTouchDevice
> \
> touch - common(2) - wcmTouchDevice == NULL
>
I see where you got lost. You assumed pad is on pen device, which is true
for Intuos Pro and Cintiq series. But it is not true for Bamboo and new
Intuos series.
now depending on where we read in the SW_MUTE we need to get to the touch
> device. if we read it from common(1) then we have wcmTouchDevice pointing
> to
> it, if we read it from common(2), well, we have the touch device right
> there.
> that should be enough to post the property update which only lives on the
> touch device anyway.
>
> the only problem with the above would be if pad and touch is on the same
> physical device, but can this even happen with the SW_MUTE devices?
>
Unfortunately, pad and touch are on the same device for Intuos series.
> Sometime, we have to step back to see the whole picture :-).
> >
> > I'll make a new version if you follow what I said above.
>
> I still don't understand it fully but I don't want to hold up the fix
> either, so feel free to go ahead with the patch. Mabye I'll get it then :)
>
Updated patchset coming soon.
Thank you.
Ping
> Cheers,
> Peter
>
> > > 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<javascript:_e({},
> > > 'cvml', '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
