On Tue, Dec 27, 2011 at 6:48 PM, Ping Cheng <pinglinux@xxxxxxxxx> wrote:
> On Wed, Dec 21, 2011 at 8:14 PM, Chris Bagwell <chris@xxxxxxxxxxxxxx> wrote:
>>
>> On Wed, Dec 21, 2011 at 12:43 PM, Ping Cheng <pinglinux@xxxxxxxxx> wrote:
>> > On Sun, Dec 18, 2011 at 6:07 PM, Chris Bagwell <chris@xxxxxxxxxxxxxx> wrote:
>> >>
>> >> On Fri, Dec 16, 2011 at 6:38 PM, Ping Cheng <pinglinux@xxxxxxxxx> wrote:
>> >> > Signed-off-by: Ping Cheng <pingc@xxxxxxxxx>
>> >> > ---
>> >> > drivers/input/tablet/wacom_sys.c | 26 ++++++------
>> >> > drivers/input/tablet/wacom_wac.c | 82
>> >> > +++++++++++++++++++++++++++++++++++++-
>> >> > drivers/input/tablet/wacom_wac.h | 8 ++++
>> >> > 3 files changed, 101 insertions(+), 15 deletions(-)
>> >> >
>> >> > diff --git a/drivers/input/tablet/wacom_sys.c
>> >> > b/drivers/input/tablet/wacom_sys.c
>> >> > index b9736fb..eee06ef 100644
>> >> > --- a/drivers/input/tablet/wacom_sys.c
>> >> > +++ b/drivers/input/tablet/wacom_sys.c
>> >> > @@ -297,6 +297,10 @@ static int wacom_parse_hid(struct usb_interface
>> >> > *intf,
>> >> > features->pktlen
>> >> > = WACOM_PKGLEN_TPC2FG;
>> >> >
>> >> > features->touch_max = 2;
>> >> > }
>> >> > +
>> >> > + if (features->type ==
>> >> > MTSCREEN)
>> >> > + features->pktlen
>> >> > = WACOM_PKGLEN_MTOUCH;
>> >> > +
>> >> > if (features->type ==
>> >> > BAMBOO_PT) {
>> >> > /* need to reset
>> >> > back */
>> >> > features->pktlen
>> >> > = WACOM_PKGLEN_BBTOUCH;
>> >> > @@ -331,18 +335,15 @@ static int wacom_parse_hid(struct usb_interface
>> >> > *intf,
>> >> > case HID_USAGE_Y:
>> >> > if (usage == WCM_DESKTOP) {
>> >> > if (finger) {
>> >> > - features->device_type =
>> >> > BTN_TOOL_FINGER;
>> >> > - if (features->type ==
>> >> > TABLETPC2FG) {
>> >> > - /* need to reset
>> >> > back */
>> >> > - features->pktlen
>> >> > = WACOM_PKGLEN_TPC2FG;
>> >> > + int type =
>> >> > features->type;
>> >> > +
>> >> > + if (type == TABLETPC2FG
>> >> > || type == MTSCREEN) {
>> >> > features->y_max =
>> >> >
>> >> > get_unaligned_le16(&report[i + 3]);
>> >> > features->y_phy =
>> >> >
>> >> > get_unaligned_le16(&report[i + 6]);
>> >> > i += 7;
>> >> > - } else if
>> >> > (features->type == BAMBOO_PT) {
>> >> > - /* need to reset
>> >> > back */
>> >> > - features->pktlen
>> >> > = WACOM_PKGLEN_BBTOUCH;
>> >> > + } else if (type ==
>> >> > BAMBOO_PT) {
>> >> > features->y_phy =
>> >> >
>> >> > get_unaligned_le16(&report[i + 3]);distracted me.
>> >> > features->y_max =
>> >> > @@ -356,10 +357,6 @@ static int wacom_parse_hid(struct usb_interface
>> >> > *intf,
>> >> > i += 4;
>> >> > }
>> >> > } else if (pen) {
>> >> > - /* penabled only accepts
>> >> > exact bytes of data */
>> >> > - if (features->type ==
>> >> > TABLETPC2FG)
>> >> > - features->pktlen
>> >> > = WACOM_PKGLEN_GRAPHIRE;
>> >> > - features->device_type =
>> >> > BTN_TOOL_PEN;
>> >>
>> >> All makes sense. Getting rid of duplicate code since X/Y will always
>> >> be present. Maybe worth a 1 line comment now that its cleaned up so
>> >> someone doesn't let it creep back in.
>> >
>> >
>> > I'll add some comments in v2.
>> >
>> >>
>> >> > features->y_max =
>> >> >
>> >> > get_unaligned_le16(&report[i + 3]);
>> >> > i += 4;
>> >> > @@ -432,7 +429,8 @@ static int wacom_query_tablet_data(struct
>> >> > usb_interface *intf, struct wacom_feat
>> >> >
>> >> > /* ask to report tablet data if it is MT Tablet PC or
>> >> > * not a Tablet PC */
>> >> > - if (features->type == TABLETPC2FG) {
>> >> > + if (((features->type == TABLETPC2FG) || (features->type ==
>> >> > MTSCREEN))
>> >> > + && features->device_type != BTN_TOOL_PEN) {
>> >>
>> >> Glad to see this device_type check. I thought it was probably needed.
>> >>
>> >> I'll have to trust you on this one but its different than Bamboo's.
>> >> The set on Bamboo is on Pen and causes it to start sending pen
>> >> packets. IIR, the touch packets are always sending. So this is
>> >> opposite and enables something related to touch?
>> >
>> >
>> > There are two types' of devices: tabletPC or non-tabletPC. For all tabletPC
>> > devices that support more than one finger/contact, a set call is needed. For
>> > all non-tabletPC devices, a set call is needed. Bamboo is a non-tabletPC
>> > device.
>> >
>> >>
>> >> Assuming you say yes, do you mind changing that to an affirmative
>> >> check for TOUCH so it reads:
>> >
>> >
>> > Well, it does not depend on whether it is touch or pen. It depends on the
>> > type of the device as explained above. Do you still want me to use your
>> > below logic?
>>
>> Let me ask this way. On Bamboo there are 2 interfaces, 1 pen and 1
>> touch. I need to set feature report 2 to a specific value on the pen
>> interface to get useful packets. If I set feature report 2 to
>> something on touch interface, I get an error response back from
>> device. This aligns with HID descriptor because only the pen
>> interface declares that feature report #2.
>>
>> I'm sure its similar for Tablet PC's. Its also writing to feature
>> report #2 but unique data values. What ever the values are, it
>> probably also doesn't make sense to write to both pen interface and
>> touch interface on Tablet PC.
>>
>> If the feature report is only on one of two interfaces I do suggest
>> changing the if() to something like below to make it more obvious ("if
>> (TOUCH)" is more obvious that your looking for specific interface then
>> "if (!PEN)"). If its on both and only 1 needs to be written to then
>> that would probably be a good comment as well.
>
> I see your point. I'll use TOUCH/PEN for the if-statement and add a comment.
>
Thanks.
>> >>
>> >> if (TOUCH)
>> >> /* do TOUCH set's */
>> >> if (PEN)
>> >> /* do PEN set's */
>> >>
>> >>
>> >> > do {
>> >> > rep_data[0] = 3;
>> >> > rep_data[1] = 4;
>> >> > @@ -479,9 +477,9 @@ static int wacom_retrieve_hid_descriptor(struct
>> >> > usb_interface *intf,
>> >> > features->pressure_fuzz = 0;
>> >> > features->distance_fuzz = 0;
>> >> >
>> >> > - /* only Tablet PCs and Bamboo P&T need to retrieve the info */
>> >> > + /* only devices that support touch need to retrieve the info */
>> >> > if ((features->type != TABLETPC) && (features->type !=
>> >> > TABLETPC2FG) &&
>> >> > - (features->type != BAMBOO_PT))
>> >> > + (features->type != BAMBOO_PT) && (features->type !=
>> >> > MTSCREEN))
>> >> > goto out;
>> >> >
>> >> > if (usb_get_extra_descriptor(interface, HID_DEVICET_HID,
>> >> > &hid_desc)) {
>> >> > diff --git a/drivers/input/tablet/wacom_wac.c
>> >> > b/drivers/input/tablet/wacom_wac.c
>> >> > index 8dc185d..1a887a1 100644
>> >> > --- a/drivers/input/tablet/wacom_wac.c
>> >> > +++ b/drivers/input/tablet/wacom_wac.c
>> >> > @@ -729,6 +729,73 @@ static int wacom_intuos_irq(struct wacom_wac
>> >> > *wacom)
>> >> > return 1;
>> >> > }
>> >> >
>> >> > +static int wacom_mt_touch(struct wacom_wac *wacom)
>> >> > +{
>> >> > + struct wacom_features *features = &wacom->features;
>> >> > + struct input_dev *input = wacom->input;
>> >> > + char *data = wacom->data;
>> >> > + struct input_mt_slot *mt;
>> >> > + int i, id = -1, j = 0, k = 4;
>> >> > + int x = 0, y = 0;
>> >> > + int current_num_contacts = data[2];
>> >> > + int contacts_to_send = 0;
>> >> > + bool touch = false;
>> >> > +
>> >> > + /* reset the counter by the first packet */
>> >> > + if (current_num_contacts) {
>> >> > + features->num_contacts = current_num_contacts;
>> >> > + features->num_contacts_left = current_num_contacts;
>> >> > + }
>> >>
>> >> It seems that the saved contact values are only processed in case were
>> >> data[2] indicates no contact counts in packet. I would think that
>> >> packets with no contacts could be ignored (return) unless there is
>> >> some data that must be implied. Why is it not ignored?
>> >
>> >
>> > current_num_contacts is only reported, hence updated, by the first set of
>> > data. Later touch data will be processed as long as number of contacts does
>> > not exceed current_num_contacts.
>> >
>> >>
>> >>
>> >> > +
>> >> > + /* There are at most 5 contacts per packet */
>> >> > + contacts_to_send = min(5, (int)features->num_contacts_left);
>> >>
>> >> Its not obvious to me if data[2] contains touches in packet or total
>> >> touches on screen.
>> >
>> >
>> > Total touches on the screen.
>>
>> OK. I understand now. It would be nice to add that to above comment
>> (the fact that num_contacts_left is total on screen).
>
> num_contacts_left is normally not the total on the screen. It is the
> number of contacts that need to be processed. num_contacts is the
> total on the screen. contacts_to_send is the number of contacts we are
> going to send.
OK. With understanding that data[2] is # of touches in packet and not
total touches on screen then my questions change slightly.
I assume if user put 6 touches a screen and moves around you'd get 2
packets with movement; 1 with data[2] == 5 and 1 with data[2] == 1.
There is this code then:
int current_num_contacts = data[2];
if (current_num_contacts) {
features->num_contacts = current_num_contacts;
features->num_contacts_left = current_num_contacts;
}
So it seems to me that num_contacts can never reach 6 (total touches
on screen). Maybe that needs to be a += or something?
>
>> >>
>> >> > +
>> >> > + for (i = 0; i < contacts_to_send; i++) {
>> >> > + id = le16_to_cpup((__le16 *)&data[k]);
>> >> > +
>> >> > + /* is there an existing slot for this contact? */
>> >> > + for (j = 0; j < features->touch_max; j++) {
>> >> > + mt = &input->mt[j];
>> >> > + if (input_mt_get_value(mt, ABS_MT_TRACKING_ID)
>> >> > == id )
>> >> > + break;
>> >> > + }
>> >> > +
>> >> > + /* no. then find an unused slot to fill */
>> >> > + if (j >= features->touch_max) {
>> >> > + for (j = 0; j < features->touch_max; j++) {
>> >> > + mt = &input->mt[j];
>> >> > + if (input_mt_get_value(mt,
>> >> > ABS_MT_TRACKING_ID) == -1 )
>> >> > + break;
>> >> > + }
>> >> > + }
>> >>
>> >> This may need a little more thought since hid-multitouch doesn't query
>> >> old value. Is there a case were a touch is removed and new touch
>> >> added in same packet? Probably that would never send the -1 to user
>> >> land.
>> >
>> >
>> > Once a touch leaves the screen, a -1 will always be sent to indicate the
>> > touch is out. When a new touch is added, it may use any available packet.
>> > Hence the one used by the just removed touch can also be used. Do I get your
>> > question properly?
>>
>> I'm thinking of this sequence inside 1 packet (probably not easy to
>> produce in the wild):
>>
>> * 1st location in packet shows Finger with ID of 100 as not touching any more.
>> * Loops threw and finds matching ID and sets to -1.
>> * 2nd location in packet shows Finger with ID of 101 as touching. Its new.
>> * Loops threw to find 1st slot with -1 and uses one we just cleared.
>> * Sync window occurs. User only sees Contact ID change from 100 and 101.
>
> Oh, I know where you are. You are in the real world, not in the
> firmware ;). The firmware can not tell if the second finger (101) is
> the same finger as the first one (100) or not. All it can do is:
> report a finger out then a finger in. So, in any moment if there is
> one finger on the screen, it report one finger. If none, it reports
> none. If, by any chance, the second one (101) touches before the first
> one out, we get two finger data before one if out.
>
> Your case won't happen, at least in theory.
OK. I only wanted to bring up the possibility. Depending on firmware
design its possible that never could happen.
>
>> >> > +
>> >> > + touch = data[k - 1] & 0x1;
>> >> > + input_mt_slot(input, j);
>> >> > + if (touch) {
>> >> > + x = le16_to_cpup((__le16 *)&data[k + 6]);
>> >> > + y = le16_to_cpup((__le16 *)&data[k + 8]);
>> >> > +
>> >> > + input_report_abs(input, ABS_MT_POSITION_X, x);
>> >> > + input_report_abs(input, ABS_MT_POSITION_Y, y);
>> >> > + } else
>> >> > + id = -1;
>> >> > +
>> >> > + /* keep the id from firmware since we need
>> >> > + * it to process future events
>> >> > + */
>> >> > + input_report_abs(input, ABS_MT_TRACKING_ID, id);
>> >> > +
>> >> > + k += WACOM_BYTES_PER_MT_PACKET;
>> >> > + }
>> >> > +
>> >> > + input_mt_report_pointer_emulation(input, true);
>> >> > +
>> >> > + features->num_contacts_left -= contacts_to_send;
>> >> > + if (features->num_contacts_left < 0)
>> >> > + features->num_contacts_left = 0;
>> >>
>> >> Same comment as above. Could use a comment on why storing this would be
>> >> needed.
>> >
>> >
>> > If you understood the value of current_num_contacts, it would be easier to
>> > see the need for num_contacts_left. I'll add a comment for
>> > current_num_contacts.
>>
>> I guess I still don't see why its needed or how its being used.
>>
>> Now that I understand that current_num_contacts is total touches, I do
>> understand you need to know when a packet contains 5 full touches and
>> when it contains less than 5. For example if 6 touches then 1st
>> packet could have 5 and next packet could have 1. So you need to know
>> to stop looping after 1. But I do not see how this num_contacts_left
>> helps that. It looks like it would set contacts_to_send to 5 in both
>> packets.
>
> Why do you think contacts_to_send could be 5? If num_contacts_left is
> 1, min(5, (int)features->num_contacts_left) should return 1, which is
> the contacts_to_send. Do I miss something?
I think it stemmed from my misunderstanding of what data[2] contained.
If it contains 1 for 2nd packet with 6th finger then
num_contacts_left would be 1 as well and it would work.
With my new understanding that data[2] is touches in packet (1 to 5)
then it seems all this num_contacts and num_contacts_left is trying to
compute total touches on the screen but then its never used by
anything. I'd get rid of it and just keep the camp of data[2] to <=
5/contacts_to_send part.
Chris
>
> Ping
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
