Re: [RFC v2 03/10] hid-multitouch: support for PixCir-based panels
From: Benjamin Tissoires
Date: Fri Jan 07 2011 - 10:12:33 EST
Hi Henrik,
I've made the changes and I pushed them on the git of our lab. I
thought it was easier for you to have a look at the changes instead of
resending the whole patch. But v3 will be for this afternoon.
the repo is at:
http://lii-enac.fr/cgi-bin/gitweb.cgi?p=linux-input/enac-drivers.git;a=summary
the changes are on the branch "hid-multitouch-dtor" from "Copyright
notice" to the head ("hid-input: better way of handling
HID_FEATURE_REPORT").
To summarize:
- I used touch_state and seen_in_this_frame -> I hope the semantic is
now clearer. I was able to test it on stantum and cypress device. I
should do the test for pixcir as soon as I can have some time in the
afternoon, and the test against generaltouch should happend in the
beginning of the week.
- I integrated fuzz (please check)
- I re-factorized set_abs (just copied-pasted your code)
- I made other small changes
- Concerning the quirks, I am not very in favor ATM: a flag for
compute slot will infer a switch of 5 cases in the emit_event, and the
idea was to be able to have a constant time access (1) when using
specific function.
The Quirk MT_QUIRK_NOT_SEEN_MEANS_UP is better (it will speed up a
little the code: 1 test against 1 test and 1 affectation) but I don't
think it worse the effort for further device additions.
- The Egalax problem: I am pretty sure that Stéphane took this driver
into account when writing the original patch. BTW I propose to
postpone the problem for 2.6.39.
Cheers,
Benjamin
On Fri, Jan 7, 2011 at 1:23 PM, Henrik Rydberg <rydberg@xxxxxxxxxxx> wrote:
>> > Please provide a bit more information under this config option. The
>> > usual "what should I do", and roughly what devices are supported.
>>
>> Will try, but don't hesitate to send one if you feel in a good mood for writing.
>>
>> I can propose:
>>
>> + ---help---
>> + Generic support for HID multitouch panels.
>> + Currently supported panels are:
>> + - PixCir touchscreen
>> + - Cypress TrueTouch
>> + - 'Sensing Win7-TwoFinger' panel by GeneralTouch
>
> This is fine, but needs the "to compile as a module" etc, examples are
> all around the Kconfig file.
>
>> >> +struct mt_slot {
>> >> + __s32 x, y, p;
>> >> + __s32 contactid; /* the device ContactID assigned to this slot */
>> >> + __u16 trkid; /* the tracking ID that was assigned to this slot */
>> >> + bool valid; /* did we just get valid contact data for this slot? */
>> >> + bool prev_valid;/* was this slot previously valid/active? */
>> >> +};
>> >
>> > The trkid and prev_valid are no longer needed. The touch state seems to be missing.
>>
>> Concerning the trkid, agree.
>> I can assure you that prev_valid is needed by at least the Cypress
>> device. In a sense, it has the same problem than mt protocol A: it
>> does not send the release information except when the last finger has
>> been released. This gymnastic is thus required.
>
> Perhaps I did not explain properly. What is needed is a way to clear
> the touch state of the active slots not yet seen in a touch
> frame. Because of the mixup of semantics around "valid" and "touch",
> it looks more complicated than it really is. My point is that the
> previous frame has nothing to do with it.
>
>>
>> I think that the misunderstanding comes from the name. We have this 2
>> flags (valid and prev_valid) to tell whether the device sent data
>> during this report or the previous one. It's not the same meaning that
>> the touch you're talking about. For Cypress device, those 2 flags are
>> mandatory.
>
> The names are indeed confusing. A valid packet means the incoming
> packet contains information about a slot, and brings updates to the
> touch state and the touch properties. The touch state just needs to be
> set, it is the same logic everytime.
>
>>
>> Maybe introducing data + prev_data + touch (3 flags instead of 2->
>> touch can go into mt_buffer, and data and prev_data only in mt_slot as
>> they are not given by the device) is clearer.
>
> This would be unnecessarily complicated. Either (touch_state,
> seen_in_this_frame), or (touch_state,
> last_frame_revision_of_this_slot) would be enough.
>
>>
>> >
>> >> +
>> >> +struct mt_buffer {
>> >> + __s32 x, y, p;
>> >> + __s32 contactid; /* the device ContactID assigned to this slot */
>> >> +};
>> >
>> > The only different to mt_slot are the valid and touch field, which is
>> > also needed for incoming data. I'd say those should be merged.
>> >
>>
>> Well, the point is that the buffer and the slot have 2 different meanings:
>> one is the incoming data, the other is the processed data. It strikes
>> us to have only one struct as the slot contains extra information for
>> it to be processed.
>
> This is not true. The data that comes via a valid packet is the touch
> state and the property updates, the rest is about handling the
> validity. But let's say you use (touch, revision, props), for
> instance. The incoming valid packet would update curdata.touch and
> curdata.props. When the slot is finished, curdata is copied to the
> right place. At the end of the frame, a device with the
> slots-not-send-in-this-frame-are-considered-unused quirk would reset
> the touch of the slots with slot[i].revision !=
> curdata.revision. Finally, curdata.revision would be incremented.
>
>> >
>> >> + __s8 inputmode; /* InputMode HID feature, -1 if non-existent */
>> >> + __u8 num_received; /* how many contacts we received */
>> >> + __u8 maxcontact; /* expected last contact index */
>> >> + bool curvalid; /* is the current contact valid? */
>> >
>> > This value should probably be a mt_slot struct as well.
>>
>> I was bothering too. Renaming the field (see above) may solve the
>> problem: we have curvalid (or curdata with the name I propose) which
>> is only needed for algorithm reasons, and touch that goes into
>> mt_buffer as it comes from the device.
>
> Looking again, it seems to me that curvalid should really be where it
> is now.
>
>> >
>> >> + struct mt_slot slots[0]; /* first slot */
>> >> +};
>> >> +
>> >> +struct mt_class {
>> >> + int (*compute_slot)(struct mt_device *);
>> >> + __u8 maxcontacts;
>> >> +};
>> >
>> > I imagine maxcontacts could be variable for devices within the same
>> > class. Perhaps it should be a member of the device instead? The
>> > resolution and fuzz could be added here as well.
>>
>> resolution and fuzz: I let you implement it (when adding egalax or
>> 3m). But isn't it something we can't get from the report descriptors?
>
> Resolution, yes, but defaults might still be needed. And maybe
> compute_slot should be a bitmask of quirks instead. So far we would
> have MT_QUIRK_SLOT_IS_CONTACTID and MT_QUIRK_NOT_SEEN_MEANS_UP.
>
>> concerning the device vs. class, currently, we have only seen classes
>> (one or more device sharing the same behavior), but we didn't bother
>> about resolution and fuzz.
>> It would be a shame to have to duplicate the mt_class (or mt_device),
>> one by vendorID/deviceID, as many devices may share the same
>> properties (at least those that have been manufactured by the same
>> company and that behave the same way: cando, stantum, etc...).
>> I also like the concept of default class as it will help people easily
>> adding devices.
>
> The statement comes from observing what seems to happen over time with
> device lists, but sure, it won't really hurt to have the classes.
>
>> >
>> >> +
>> >> +/* classes of device behavior */
>> >> +#define MT_CLS_DEFAULT 0
>> >> +#define MT_CLS_DUAL1 1
>> >> +
>> >> +/*
>> >> + * these device-dependent functions determine what slot corresponds
>> >> + * to a valid contact that was just read.
>> >> + */
>> >> +
>> >> +static int slot_is_contactid(struct mt_device *td)
>> >> +{
>> >> + return td->curdata.contactid;
>> >> +}
>> >> +
>> >> +static int find_slot_from_contactid(struct mt_device *td)
>> >> +{
>> >> + int i;
>> >> + for (i = 0; i < td->mtclass->maxcontacts; ++i) {
>> >> + if (td->slots[i].prev_valid &&
>> >
>> > Why prev_valid? Ought to be valid, right?
>>
>> Because the code resets valid after each sending -> implementation dependent.
>
> The prev_valid and valid are only different because prev_valid is used
> with touch semantics, whereas valid is not and is reset at each frame
> emission. Once we stop mixing fruits, it will all become clear and
> simple.
>
>> > Nice solution to the end-of-data issue. It would be good if the input
>> > setup was abstracted into a function like in hid-egalax, to simplify
>> > further additions.
>>
>> thanks.
>> I was not very happy in making this abstraction for just one line of
>> code. I thought you could do it when adding the additions.
>
> Or you could do it right away - it will simplify the existing
> patch, and make subsequent patches simpler as well.
>
>> >
>> >> + input_mt_init_slots(hi->input,
>> >> + td->mtclass->maxcontacts);
>> >
>> > Maxcontacts should probably take the hid description into account as well.
>>
>> I don't understand your point here
>
> Take 3M as an example. The controller supports up to 60 contacts, but
> different devices may not be needing that many. Having a way to trim
> the number of allocated slots to just the right size for the current
> devices seems like a good idea.
>
>> >
>> > And ABS_PRESSURE.
>>
>> I thought that the mouse emulation was restricted to X and Y.
>
> Look at the psmouse, wacom and generic MT emulation code for counter examples.
>
>> > There are some hid drivers that need to setup fuzz in order to work
>> > properly. We should either add it to hid core or use the same bypass as
>> > in hid-egalax and hid-3m-pct.
>>
>> Can I let you do this in further updates? (for the new devices and
>> those I sent, this works out of the box)
>
> Why not do it right away? It is quite simple, and mostly copy and
> paste from those drivers.
>
>> >> +/*
>> >> + * this function is called when a whole packet has been received and processed,
>> >> + * so that it can decide what to send to the input layer.
>> >> + */
>> >> +static void mt_emit_event(struct mt_device *td, struct input_dev *input)
>> >> +{
>> >> + int i;
>> >> +
>> >> + for (i = 0; i < td->mtclass->maxcontacts; ++i) {
>> >> + struct mt_slot *s = &(td->slots[i]);
>> >> + if (!s->valid) {
>> >> + /*
>> >> + * this slot does not contain useful data,
>> >> + * notify its closure if necessary
>> >> + */
>> >> + if (s->prev_valid) {
>> >> + input_mt_slot(input, i);
>> >> + input_mt_report_slot_state(input,
>> >> + MT_TOOL_FINGER, false);
>> >> + s->prev_valid = false;
>> >> + }
>
> This code will have exactly the same result without the "if
> (s->prev_valid)", since prev_valid is a copy of the last touch
> state. The input core will filter away any duplicate calls. Thus, it
> can all be replaced by
>
> input_mt_slot(input, i);
> input_mt_report_slot_state(input, MT_TOOL_FINGER, s->valid);
>
> Further assuming that valid will change to touch, it is all starting
> to look conceptually correct. We could then prepend this line
>
> if ((device_quirks & MT_QUIRK_NOT_SEEN_MEANS_UP) && s->revision != curdata.revision)
> s->state = false;
>
> and we would have support for the devices you mention.
>
>> >> + continue;
>> >> + }
>> >> + if (!s->prev_valid)
>> >> + s->trkid = td->lasttrkid++;
>> >
>> > Most of the above can be removed.
>>
>> Cypress device does not sends touch information when a touch is
>> released. This piece of code is required for devices that behave the
>> same way.
>
> See the above example.
>
>> >
>> >> +
>> >> + input_mt_slot(input, i);
>> >> + input_mt_report_slot_state(input, MT_TOOL_FINGER, true);
>> >
>> > "true" here should simply be slot->touch state.
>> >
>> >> + input_event(input, EV_ABS, ABS_MT_POSITION_X, s->x);
>> >> + input_event(input, EV_ABS, ABS_MT_POSITION_Y, s->y);
>> >> + input_event(input, EV_ABS, ABS_MT_PRESSURE, s->p);
>> >> + s->prev_valid = true;
>> >> + s->valid = false;
>> >
>> > Invalidating the data of a tracked slots seems wrong. If the device
>> > sends tracked data properly, no special consideration is needed - it
>> > will get cleared when appropriate. Other cases could be dealt with
>> > separately.
>>
>> already discussed (pb in naming the fields I think)
>
> The egalax driver has a different behavior, for instance.
>
>> >> + case HID_DG_TIPSWITCH:
>> >> + td->curvalid = value;
>> >
>> > Most drivers seem to use this as touch state.
>>
>> agree and that is how it is used in the current implementation. We
>> really should change the name.
>
> I believe the curvalid is fine, but it should be updated differently.
> The INRANGE and CONFIDENCE fields seem to do that for some devices,
> yet others rely on the contactcount. The TIPSWITCH field should rather
> update curdata.touch.
>
>> >> + case HID_DG_CONTACTCOUNT:
>> >> + /*
>> >> + * We must not overwrite the previous value (some
>> >> + * devices send one sequence splitted over several
>> >> + * messages)
>> >> + */
>> >> + if (value)
>> >> + td->maxcontact = value - 1;
>> >
>> > Is td->maxcontact ever reset? And why not num_expected or something
>> > instead of maxcontact - odd semantics.
>>
>> maxcontact is not reset (not needed as it is sent in each report).
>> Concerning the name, agree.
>
> You are right, this will work, too.
>
> Thanks,
> Henrik
> --
> 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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/