Looks like Peter beat me to this submission. Ping's patch with this part
got committed at same time as original email.
Maybe I'll rework the small parts that were mean to improve readability.
Replies below.
On Wed, Jun 2, 2010 at 7:35 PM, Ping Cheng <pingli...@gmail.com> wrote:
> Hi Chris,
>
> Thank you for the patch. Please see my comments inline.
>
> Ping
>
> On Tue, Jun 1, 2010 at 7:18 PM, <ch...@cnpbagwell.com> wrote:
> > From: Chris Bagwell <ch...@cnpbagwell.com>
> >
> > usbChooseChannel() supported in most cases 1 input and 1 pad to
> > be concurrently in proximity. DUALINPUT V5 devices (Intuos 1 & 2)
> > could handle a combination 2 inputs and 1 pad, up to max of 2 in
> > proximity at 1 time though.
> >
> > This change ports over an update from linuxwacom to allow
> > 2 input channels on V4 devices as well as 1 pad to be
> > concurrently in proximity. It gives DUALINPUT V5 a total
> > of 3 as a side affect.
> >
> > This change is required to expose multitouch input to any
> > pending Gesture logic. This is namely for Bamboo P&T and
> > Tablet PC's.
> >
> > Patch also replaces a few magic numbers with #defines to improve
> > understanding.
> >
> > Signed-off-by: Chris Bagwell <ch...@cnpbagwell.com>
> > ---
> > src/wcmUSB.c | 34 ++++++++++++++++++++--------------
> > src/xf86WacomDefs.h | 8 ++++++--
> > 2 files changed, 26 insertions(+), 16 deletions(-)
> >
> > diff --git a/src/wcmUSB.c b/src/wcmUSB.c
> > index f0321ee..b804aa9 100644
> > --- a/src/wcmUSB.c
> > +++ b/src/wcmUSB.c
> > @@ -697,13 +697,17 @@ static int usbChooseChannel(WacomCommonPtr common,
> int serial)
> >
> > if (common->wcmProtocolLevel == 4)
> > {
> > - /* Protocol 4 doesn't support tool serial numbers */
> > - if (serial == 0xf0)
> > - channel = 1;
> > - else
> > - channel = 0;
>
> This is pretty much for backward compatibility support since there was
> no serial number reported from the kernel for V4 devices. How about we
> change the comment to:
>
> /* default to channel one if serial number is not
> received */
>
Sounds good. The main thing I did was make the backwards compatible part a
statement of its own up front instead of else of next if() statement so its
clearer serial==0 not something kernel is sending.
BTW, even latest linuxwacom has cases we are not sending MSC_SERIAL for V4
devices. Perhaps we should add that in all places for consistency?
>
> > + /* V4 devices imply serial #1 if not sent */
> > + if (!serial)
> > + serial = 1;
> > +
> > + if (serial == SINGLEINPUT_PAD_SERIAL)
> > + channel = PAD_CHANNEL;
> > + else if (serial < MAX_CHANNELS)
> > + channel = serial-1;
> > }
> > - else if (serial) /* serial number should never be 0 for V5
> devices */
> > + /* serial number should never be 0 for V5 devices */
> > + else if (serial)
> > {
> > /* dual input is supported */
> > if (TabletHasFeature(common, WCM_DUALINPUT))
> > @@ -712,7 +716,7 @@ static int usbChooseChannel(WacomCommonPtr common,
> int serial)
> > for (i=0; i<MAX_CHANNELS; ++i)
> > {
> > if (common->wcmChannel[i].work.proximity
> &&
> > -
> common->wcmChannel[i].work.serial_num == serial)
> > + common->wcmChannel[i].work.serial_num
> == serial)
> > {
> > channel = i;
> > break;
> > @@ -732,14 +736,16 @@ static int usbChooseChannel(WacomCommonPtr common,
> int serial)
> > }
> > }
> > }
> > - else /* one transducer plus expresskey (pad) is
> supported */
> > + else
> > {
> > - if (serial == -1) /* pad */
> > - channel = 1;
> > - else if ( (common->wcmChannel[0].work.proximity
> && /* existing transducer */
> > -
> (common->wcmChannel[0].work.serial_num == serial)) ||
> > -
> !common->wcmChannel[0].work.proximity ) /* new transducer */
> > - channel = 0;
> > + /* One transducer plus expresskey (pad) is
> supported.
> > + * Second dual input is not used. */
> > + if (serial == DUALINPUT_PAD_SERIAL)
> > + channel = PAD_CHANNEL;
>
> This line makes me feel we want to split the pen-only devices from the
> pen and touch ones. The middle channel, in this case, is wasted all
> the time for this type of devices. Plus, there are only two fingers
> for now. What happens if we have more than 2 fingers? We would leave
> a bigger gap between channel=0 and PAD_CHANNEL (the last one) for this
> type of devices.
>
> The linuxwacom solution was kind of a workaround to meet the deadline.
> For xf86-input-wacom, I think we want to do it right. Basically, I'd
> like to keep the pen-only flow unchanged. Only update the ones that
> support touch. Let me know if you get confused.
>
Let me see if I understand you correctly. Your requesting that for pen-only
devices (this section of code), you want to keep using channel 0 for the pen
and channel 1 for pad and leave channel 2 doing nothing. That is apposed to
my change above were channel 0 is for pen and channel 2 is for pad with
channel 1 doing nothing.
I wanted to make sure your not requesting to make size of wcmChannel[]
dynamic and never allow unused entries.
I'm fine with old array locations. My main intent is to add some #define's
so magical values of 0 and 1 are not used. That means I'll need to define
something like DUALINPUT_PAD_CHANNEL and SINGLEINPUT_PAD_CHANNEL. Anyone
have better name suggestions?
>
> > + else if ((common->wcmChannel[0].work.proximity &&
> /* existing transducer */
> > + (common->wcmChannel[0].work.serial_num
> == serial)) ||
> > + !common->wcmChannel[0].work.proximity )
> /* new transducer */
> > + channel = FIRST_INPUT_CHANNEL;
> > }
> > }
> >
> > diff --git a/src/xf86WacomDefs.h b/src/xf86WacomDefs.h
> > index 3d3c176..cbd890c 100644
> > --- a/src/xf86WacomDefs.h
> > +++ b/src/xf86WacomDefs.h
> > @@ -359,8 +359,12 @@ extern WacomDeviceClass gWacomISDV4Device;
> >
> > #define DEVICE_ISDV4 0x000C
> >
> > -#define MAX_CHANNELS 2
> > -#define MAX_FINGERS 2
> > +#define MAX_FINGERS 2
> > +#define MAX_CHANNELS MAX_FINGERS+1
> > +#define FIRST_INPUT_CHANNEL 0
> > +#define PAD_CHANNEL 2
>
> The idea was to make pad channel always the last one. It would be more
> future proofing if we:
>
> #define PAD_CHANNEL MAX_CHANNELS-1
>
Good idea. Thank you.
Building on comment from above though, what do you think about something
like:
#define MAX_SINGLEINPUT_CHANNELS 2
#define MAX_MULTIINPUT_CHANNELS MAX_FINGERS+1
#define MAX_CHANNELS MAX_DUALINPUT_CHANNELS
#define SINGLEINPUT_PAD_CHANNEL 1
#define MULTIINPUT_PAD_CHANNEL MAX_MULTIINPUT_CHANNELS-1
This way a couple of the "for(x=0; x < MAX_CHANNELS; x++)" style loops can
be customized to multi vs. single input modes as well.
>
> > +#define DUALINPUT_PAD_SERIAL -1
> > +#define SINGLEINPUT_PAD_SERIAL 0xf0
> >
> > struct _WacomCommonRec
> > {
> > --
> > 1.7.0.1
> >
> >
> >
> ------------------------------------------------------------------------------
> >
> > _______________________________________________
> > Linuxwacom-devel mailing list
> > Linuxwacom-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
> >
>
------------------------------------------------------------------------------
ThinkGeek and WIRED's GeekDad team up for the Ultimate
GeekDad Father's Day Giveaway. ONE MASSIVE PRIZE to the
lucky parental unit. See the prize list and enter to win:
http://p.sf.net/sfu/thinkgeek-promo
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
