On 6/28/2010 4:37 PM, Henrik Rydberg wrote:
> Joonyoung Shim wrote:
>> On 6/25/2010 11:08 PM, Henrik Rydberg wrote:
>>> Hi Joonyoung,
>>>
>>> some follow-up comments on the MT events.
>>>
>>>> +static void qt602240_input_report(struct qt602240_data *data, int single_id)
>>>> +{
>>>> + struct qt602240_finger *finger = data->finger;
>>>> + struct input_dev *input_dev = data->input_dev;
>>>> + int finger_num = 0;
>>>> + int id;
>>>> +
>>>> + for (id = 0; id < QT602240_MAX_FINGER; id++) {
>>>> + if (!finger[id].status)
>>>> + continue;
>>>> +
>>>> + input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR,
>>>> + finger[id].area);
>>>> +
>>>> + if (finger[id].status == QT602240_RELEASE)
>>>> + finger[id].status = 0;
>>>> + else {
>>>> + input_report_abs(input_dev, ABS_MT_POSITION_X,
>>>> + finger[id].x);
>>>> + input_report_abs(input_dev, ABS_MT_POSITION_Y,
>>>> + finger[id].y);
>>>> + finger_num++;
>>>> + }
>>>> +
>>>> + input_mt_sync(input_dev);
>>>> + }
>>>> +
>>>> + input_report_key(input_dev, BTN_TOUCH, finger_num > 0);
>>>> +
>>>> + if (finger[single_id].status != QT602240_RELEASE) {
>>>> + input_report_abs(input_dev, ABS_X, finger[single_id].x);
>>>> + input_report_abs(input_dev, ABS_Y, finger[single_id].y);
>>>> + }
>>>> +
>>>> + input_sync(input_dev);
>>>> +}
>>> The problem still persists here. I will try to explain in code form instead:
>>>
>>> for (id = 0; id < QT602240_MAX_FINGER; id++) {
>>> if (!finger[id].status)
>>> continue;
>>>
>>> input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR,
>>> finger[id].status != QT602240_RELEASE ? finger[id].area : 0);
>>> input_report_abs(input_dev, ABS_MT_POSITION_X,
>>> finger[id].x);
>>> input_report_abs(input_dev, ABS_MT_POSITION_Y,
>>> finger[id].y);
>> Hmm, is it OK to report ABS_MT_POSITION_X/Y when releases?
>
> Yes. The position should be the position where the finger left the surface. It
> is different from the ABS_X/Y case simply because the type A protocol exchanges
> data statelessly.
>
I see, but i have something wondering at your document.
This is your patch of "Document the MT event slot protocol"
+Protocol Example A
+------------------
+
+Here is what a minimal event sequence for a two-contact touch would look
+like for a type A device:
+
+ ABS_MT_POSITION_X x[0]
+ ABS_MT_POSITION_Y y[0]
+ SYN_MT_REPORT
+ ABS_MT_POSITION_X x[1]
+ ABS_MT_POSITION_Y y[1]
+ SYN_MT_REPORT
+ SYN_REPORT
+The sequence after moving one of the contacts looks exactly the same; the
+raw data for all present contacts are sent between every synchronization
+with SYN_REPORT.
-Usage
------
+Here is the sequence after lifting the first contact:
+
+ ABS_MT_POSITION_X x[1]
+ ABS_MT_POSITION_Y y[1]
+ SYN_MT_REPORT
+ SYN_REPORT
+
+And here is the sequence after lifting the second contact:
+
+ SYN_MT_REPORT
+ SYN_REPORT
+
Here, there is no reporting for ABS_MT_POSITION_X/Y event, because that
is the last contact?
Then, the coordinates of the first contact are x[1] and y[1], right? If
yes, it is some confusing, i think they are x[0] and y[0].
>>> input_mt_sync(input_dev);
>>>
>>> if (finger[id].status == QT602240_RELEASE)
>>> finger[id].status = 0;
>>> else
>>> finger_num++;
>>> }
>>>
>>> Regarding the single_id, I can understand the need for it, but the logic for a
>>> single touch is slightly confusing. If single_id is to be interpreted as the
>>> contact currently being tracked as the single pointer, and that single_id
>>> changes as the number of fingers on the pad is reduced, until there is only one
>>> left, then it would still be clearer logically to do something like this:
>>>
>>> if (finger_num > 0) {
>>> input_report_key(input_dev, BTN_TOUCH, 1);
>>> input_report_abs(input_dev, ABS_X, finger[single_id].x);
>>> input_report_abs(input_dev, ABS_Y, finger[single_id].y);
>>> } else {
>>> input_report_key(input_dev, BTN_TOUCH, 0);
>>> }
>>> input_sync(input_dev);
>>>
>>> Would it not?
>>>
>> There is a case which fingers more than one are touched and current
>> status of single_id finger is release, then this will report ABS_X/Y
>> events even if it is the release event.
>
> I see. And you want BTN_TOUCH to follow the logic for the single touch? I think
> that is the main issue here. We can have _one_ of the following definitions, but
> not both:
>
> 1. input_report_key(input_dev, BTN_TOUCH, finger_num > 0);
>
OK, i will use this. This was original code.
> 2. input_report_key(input_dev, BTN_TOUCH,
> finger[single_id].status != QT602240_RELEASE);
>
> If you use the latter, there should be another event to denote the finger_num ==
> 0 case. This line at the end should do it:
>
> if (finger_num == 0)
> input_mt_sync(input_dev);
>
I don't know why this needs?
Here, there are fixed codes. How about do you think?
static void qt602240_input_report(struct qt602240_data *data, int single_id)
{
struct qt602240_finger *finger = data->finger;
struct input_dev *input_dev = data->input_dev;
int status = finger[single_id].status;
int finger_num = 0;
int id;
for (id = 0; id < QT602240_MAX_FINGER; id++) {
if (!finger[id].status)
continue;
input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR,
finger[id].status != QT602240_RELEASE ?
finger[id].area : 0);
input_report_abs(input_dev, ABS_MT_POSITION_X,
finger[id].x);
input_report_abs(input_dev, ABS_MT_POSITION_Y,
finger[id].y);
input_mt_sync(input_dev);
if (finger[id].status == QT602240_RELEASE)
finger[id].status = 0;
else
finger_num++;
}
input_report_key(input_dev, BTN_TOUCH, finger_num > 0);
if (status != QT602240_RELEASE) {
input_report_abs(input_dev, ABS_X, finger[single_id].x);
input_report_abs(input_dev, ABS_Y, finger[single_id].y);
}
input_sync(input_dev);
}
Thanks.
--
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
