On Thu, Jun 3, 2010 at 10:32 AM, Chris Bagwell <ch...@cnpbagwell.com> wrote:
> 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.
That is good.
> 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?
Sure. What are those places? It can be a patch of its own.
>>
>> > + /* 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 think dynamic sizing would be more future proofing. At least, I'd
like to see a dynamic sizing for touch ports. You know the touch and
pen devices have different logical ports. I think we can split the
two type of devices by the logical ports. This would need some major
work in the driver. It might be too much for your time. If you don't
feel the urge to do so, I'll pick it up.
> 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?
Those names are fine for me. Peter may have better ones :). If we
split touch from pen support, the names would be simpler too. What do
you think?
>> > + 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.
No problem. It depends on if we want to work on the names or the code
refactoring first. The names could be different after refactoring, I
think.
>>
>> > +#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
