| Message ID | 20121114195852.GA14840@polaris.bitmath.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
Hi Henrik, On 11/14/2012 08:58 PM, Henrik Rydberg wrote: > Hi Benjamin, > >> Computes the device timestamp according to the specification. >> It also ensures that if the time between two events is greater >> than MAX_TIMESTAMP_INTERVAL, the timestamp will be reset. >> >> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com> >> --- > > This was not what I envisioned from the discussion yesterday, I was > probably too vague. Firstly, the absolute time interval checked seems > to be 500 ms, which is arbitrary. Second, the wrap should probably use Not exactly. The time interval was 5s, near enough the minimum requirement for this field (6.5535 s). > (logical_maximum + 1). Third, we are still not sure whether we should > take the 'time = 0 means reset' logic literally, I think it needs to > be tested. Again, the device I have never does any reset at the first touch, it just wraps the counter. The problem is that when there is no event, we now that no events occurred since the previous timestamp, modulus 6.5535 seconds. > > In light of this, I would like to suggest the patch below instead. It > should follow the current interpretation of the definition to the > letter. I imagine very few devices will actually do anything but wrap > the counter, but that remains to be seen. In that case, we could > simplify the code further, since we will have no hope of detecting > large intervals properly anyways. > > Thanks, > Henrik > > From 448808dd988e96d58b79148292fde147935305ab Mon Sep 17 00:00:00 2001 > From: Henrik Rydberg <rydberg@euromail.se> > Date: Wed, 14 Nov 2012 20:53:17 +0100 > Subject: [PATCH] HID: hid-multitouch: send the device timestamp when present > > Compute and relay the device timestamp when present. The counter > value range varies between devices, but the MSC_TIMESTAMP event > is always 32 bits long. A value of zero means the time since the > previous event is unknown. > > Signed-off-by: Henrik Rydberg <rydberg@euromail.se> > --- > drivers/hid/hid-multitouch.c | 36 +++++++++++++++++++++++++++++++++--- > include/linux/hid.h | 1 + > 2 files changed, 34 insertions(+), 3 deletions(-) > > diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c > index 61543c0..586f9c6 100644 > --- a/drivers/hid/hid-multitouch.c > +++ b/drivers/hid/hid-multitouch.c > @@ -98,6 +98,8 @@ struct mt_device { > bool serial_maybe; /* need to check for serial protocol */ > bool curvalid; /* is the current contact valid? */ > unsigned mt_flags; /* flags to pass to input-mt */ > + unsigned dev_time; /* device scan time in units of 100 us */ > + unsigned timestamp; /* the timestamp to be sent */ > }; > > /* classes of device behavior */ > @@ -458,12 +460,19 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, > mt_store_field(usage, td, hi); > td->last_field_index = field->index; > return 1; > + case HID_DG_SCANTIME: > + hid_map_usage(hi, usage, bit, max, > + EV_MSC, MSC_TIMESTAMP); > + set_bit(MSC_TIMESTAMP, hi->input->mscbit); > + td->last_field_index = field->index; > + return 1; > case HID_DG_CONTACTCOUNT: > td->last_field_index = field->index; > return 1; > case HID_DG_CONTACTMAX: > - /* we don't set td->last_slot_field as contactcount and > - * contact max are global to the report */ > + /* we don't set td->last_slot_field as scan time, > + * contactcount and contact max are global to the > + * report */ > td->last_field_index = field->index; > return -1; > case HID_DG_TOUCH: > @@ -492,7 +501,8 @@ static int mt_input_mapped(struct hid_device *hdev, struct hid_input *hi, > struct hid_field *field, struct hid_usage *usage, > unsigned long **bit, int *max) > { > - if (usage->type == EV_KEY || usage->type == EV_ABS) > + if (usage->type == EV_KEY || usage->type == EV_ABS || > + usage->type == EV_MSC) > set_bit(usage->type, hi->input->evbit); > > return -1; > @@ -571,10 +581,27 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input) > static void mt_sync_frame(struct mt_device *td, struct input_dev *input) > { > input_mt_sync_frame(input); > + input_event(input, EV_MSC, MSC_TIMESTAMP, td->timestamp); > input_sync(input); > td->num_received = 0; > } > > +static void mt_compute_timestamp(struct mt_device *td, struct hid_field *field, > + unsigned value) > +{ > + unsigned dt; > + > + if (value) { > + dt = (value - td->dev_time) % (field->logical_maximum + 1); The curious thing is that this is not working on my kernel: this is the output of the following printk: printk(KERN_ERR "%s timestamp=%d value=%d dt=%d field->logical_maximum=%d !td->timestamp=%d\n", __func__, td->timestamp, value, dt, field->logical_maximum, !td->timestamp); Nov 14 22:09:55 localhost kernel: [ 1288.821899] mt_compute_timestamp timestamp=6535900 value=65359 dt=93 field->logical_maximum=65535 !td->timestamp=0 Nov 14 22:09:55 localhost kernel: [ 1288.831938] mt_compute_timestamp timestamp=6545200 value=65452 dt=93 field->logical_maximum=65535 !td->timestamp=0 Nov 14 22:09:55 localhost kernel: [ 1288.841852] mt_compute_timestamp timestamp=900 value=9 dt=-65443 field->logical_maximum=65535 !td->timestamp=0 Nov 14 22:09:55 localhost kernel: [ 1288.850852] mt_compute_timestamp timestamp=10300 value=103 dt=94 field->logical_maximum=65535 !td->timestamp=0 Nov 14 22:09:55 localhost kernel: [ 1288.860839] mt_compute_timestamp timestamp=19600 value=196 dt=93 field->logical_maximum=65535 !td->timestamp=0 I know I cheated when I put "dt=%d" for an unsigned, but the fact is that the counter rolls back... However, his construction works: static void mt_compute_timestamp(struct mt_device *td, struct hid_field *field, __s32 value) { __s32 dt; if (value) { dt = value - td->dev_time; if (dt < 0) dt += field->logical_maximum + 1; td->timestamp += 100 * dt; } else { td->timestamp = 0; } td->dev_time = value; } > + td->timestamp += 100 * dt; > + td->timestamp += !td->timestamp; I don't understand the meaning of adding !td->timestamp. :/ > + } else { > + td->timestamp = 0; My particular device never falls into this case... So I never reset the timestamp. This is problematic because we compute the timestamp on an unsigned, and the struct input_absinfo sends __s32... I'm afraid we will have some troubles after a long use of the device. Anyway, many thanks for the review of this whole patchset! Cheers, Benjamin > + } > + > + td->dev_time = value; > +} > + > static int mt_event(struct hid_device *hid, struct hid_field *field, > struct hid_usage *usage, __s32 value) > { > @@ -622,6 +649,9 @@ static int mt_event(struct hid_device *hid, struct hid_field *field, > case HID_DG_HEIGHT: > td->curdata.h = value; > break; > + case HID_DG_SCANTIME: > + mt_compute_timestamp(td, field, value); > + break; > case HID_DG_CONTACTCOUNT: > /* > * Includes multi-packet support where subsequent > diff --git a/include/linux/hid.h b/include/linux/hid.h > index d2c42dd..eb7ec6c 100644 > --- a/include/linux/hid.h > +++ b/include/linux/hid.h > @@ -242,6 +242,7 @@ struct hid_item { > #define HID_DG_DEVICEINDEX 0x000d0053 > #define HID_DG_CONTACTCOUNT 0x000d0054 > #define HID_DG_CONTACTMAX 0x000d0055 > +#define HID_DG_SCANTIME 0x000d0056 > > /* > * HID report types --- Ouch! HID spec says 1 2 3! > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Benjamin, > > This was not what I envisioned from the discussion yesterday, I was > > probably too vague. Firstly, the absolute time interval checked seems > > to be 500 ms, which is arbitrary. Second, the wrap should probably use > > Not exactly. The time interval was 5s, near enough the minimum requirement for this field (6.5535 s). Sorry about that, but the argument remains; it should depend on logical_maximum. > > (logical_maximum + 1). Third, we are still not sure whether we should > > take the 'time = 0 means reset' logic literally, I think it needs to > > be tested. > > Again, the device I have never does any reset at the first touch, it just wraps the counter. > The problem is that when there is no event, we now that no events occurred since the previous timestamp, modulus 6.5535 seconds. So the very first win8 device we test already diverges from the win8 specification, how nice. Ok, you have conviced me that comparing to a proper time makes sense. However, I am not happy about using a separate time for this, given that we will fill in the event time later in the chain. In addition, it would make perfect sense to extend the validity of the hardware time with the event time for longer intervals. The relative error only makes a difference on milisecond intervals. A patch that seamlessly extends the validity of the hardware time, ideally using the event time, seems like a viable solution. > > +static void mt_compute_timestamp(struct mt_device *td, struct hid_field *field, > > + unsigned value) > > +{ > > + unsigned dt; > > + > > + if (value) { > > + dt = (value - td->dev_time) % (field->logical_maximum + 1); > > The curious thing is that this is not working on my kernel: Oups, I suspect (value - td->dev_time) needs to be explicitly converted to unsigned. > I don't understand the meaning of adding !td->timestamp. :/ With the definition that timestamp == 0 means an unknown interval, we do not want to send that value by accident. Thanks, Henrik -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Henrik, On Fri, Nov 16, 2012 at 9:09 PM, Henrik Rydberg <rydberg@euromail.se> wrote: > Hi Benjamin, > >> > This was not what I envisioned from the discussion yesterday, I was >> > probably too vague. Firstly, the absolute time interval checked seems >> > to be 500 ms, which is arbitrary. Second, the wrap should probably use >> >> Not exactly. The time interval was 5s, near enough the minimum requirement for this field (6.5535 s). > > Sorry about that, but the argument remains; it should depend on logical_maximum. I must be tired when I read your mail, I only understand now what you meant by logical_maximum in your first mail... sorry. So, totally agree, we could make this kind of thing depending on logical_max. > >> > (logical_maximum + 1). Third, we are still not sure whether we should >> > take the 'time = 0 means reset' logic literally, I think it needs to >> > be tested. >> >> Again, the device I have never does any reset at the first touch, it just wraps the counter. >> The problem is that when there is no event, we now that no events occurred since the previous timestamp, modulus 6.5535 seconds. > > So the very first win8 device we test already diverges from the win8 > specification, how nice. hehe... well, the scan time is not as critical as can be the other fields. Anyway, it's fun :) > > Ok, you have conviced me that comparing to a proper time makes > sense. However, I am not happy about using a separate time for this, > given that we will fill in the event time later in the > chain. that may explains the delta I observed from the kernel time and the device time. > > In addition, it would make perfect sense to extend the validity of the > hardware time with the event time for longer intervals. The relative > error only makes a difference on milisecond intervals. > > A patch that seamlessly extends the validity of the hardware time, > ideally using the event time, seems like a viable solution. Just to be sure: When I receive scan_time, I increment timestamp with the device value. When not, I find out the number of counter wrap I missed with the kernel timer (jiffies) to get the actual device time. Is that ok for you? > >> > +static void mt_compute_timestamp(struct mt_device *td, struct hid_field *field, >> > + unsigned value) >> > +{ >> > + unsigned dt; >> > + >> > + if (value) { >> > + dt = (value - td->dev_time) % (field->logical_maximum + 1); >> >> The curious thing is that this is not working on my kernel: > > Oups, I suspect (value - td->dev_time) needs to be explicitly converted to unsigned. I can't remember if I tried this one, but I'll try to make it one line in the next version. > >> I don't understand the meaning of adding !td->timestamp. :/ > > With the definition that timestamp == 0 means an unknown interval, we > do not want to send that value by accident. ok, makes sense. Cheers, Benjamin > > Thanks, > Henrik -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Benjamin, > > In addition, it would make perfect sense to extend the validity of the > > hardware time with the event time for longer intervals. The relative > > error only makes a difference on milisecond intervals. > > > > A patch that seamlessly extends the validity of the hardware time, > > ideally using the event time, seems like a viable solution. > > Just to be sure: > When I receive scan_time, I increment timestamp with the device value. > When not, I find out the number of counter wrap I missed with the > kernel timer (jiffies) to get the actual device time. > > Is that ok for you? If the device has no scan time, then no scan time is reported, of course. If the scan time delta _and_ the actual time delta are both below logical_max (say), then increase the counter with the scan time delta. Else, if the actual time delta is below 2^31, increase the counter with the actual time delta. Else set the counter to zero. This ought to give us the sought-after accuracy for small times, the sought-after extension for intermediate times, and a clear definition of unknown time. Cheers, Henrik -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> below logical_max (say),
That was meant to read (logical_max / 2).
Henrik
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guys, well, I'm not very satisfied with this patch. I first thought it was a good idea but I can see now several cons: 1. Henrik would like to partially base the time spent between two events when the device wraps on the *event* time. This is a great idea for consistency, but I'm afraid I won't be able to implement it because this time is computed *after* we call input_event and is only used by evdev. Thus, I still need to add an other clock and some differences may occur. 2. the user space (at least X) will not use it before a long time: they already have the time of the event and it will not add that much consistency. 3. it will wake up the whole input chain when fingers are present but no moves occurs on the digitizer: the only events we get are MSC_TIMESTAMP and EV_SYN and the user-space will be awaken just for that. 4. MSC_TIMESTAMP does not have an abs_max value, thus we are forced to compute sth consistent in the kernel that can be forwarded to the user space. So, I propose not to include this feature, and eventually reverting the patch that introduced MSC_TIMESTAMP as it's useless if we don't use it right now. Jiri, Dmitry, Henrik, are ok with that? Cheers, Benjamin On Tue, Nov 20, 2012 at 9:54 PM, Henrik Rydberg <rydberg@euromail.se> wrote: >> below logical_max (say), > > That was meant to read (logical_max / 2). > > Henrik -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Benjamin, > well, I'm not very satisfied with this patch. I first thought it was a > good idea but I can see now several cons: > 1. Henrik would like to partially base the time spent between two > events when the device wraps on the *event* time. This is a great idea > for consistency, but I'm afraid I won't be able to implement it > because this time is computed *after* we call input_event and is only > used by evdev. Thus, I still need to add an other clock and some > differences may occur. Alternatively, device times need to become part of input core. > 2. the user space (at least X) will not use it before a long time: > they already have the time of the event and it will not add that much > consistency. Ok. > 3. it will wake up the whole input chain when fingers are present but > no moves occurs on the digitizer: the only events we get are > MSC_TIMESTAMP and EV_SYN and the user-space will be awaken just for > that. Good point, and a second argument for including this in the input core. > 4. MSC_TIMESTAMP does not have an abs_max value, thus we are forced to > compute sth consistent in the kernel that can be forwarded to the user > space. Granted, but we do not really lose anything by doing so. > So, I propose not to include this feature, and eventually reverting > the patch that introduced MSC_TIMESTAMP as it's useless if we don't > use it right now. > > Jiri, Dmitry, Henrik, are ok with that? I think it is fine to postpone the patch, but based on the comments above, I do not think we need to revert the input definition. Thanks, Henrik -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index 61543c0..586f9c6 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -98,6 +98,8 @@ struct mt_device { bool serial_maybe; /* need to check for serial protocol */ bool curvalid; /* is the current contact valid? */ unsigned mt_flags; /* flags to pass to input-mt */ + unsigned dev_time; /* device scan time in units of 100 us */ + unsigned timestamp; /* the timestamp to be sent */ }; /* classes of device behavior */ @@ -458,12 +460,19 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, mt_store_field(usage, td, hi); td->last_field_index = field->index; return 1; + case HID_DG_SCANTIME: + hid_map_usage(hi, usage, bit, max, + EV_MSC, MSC_TIMESTAMP); + set_bit(MSC_TIMESTAMP, hi->input->mscbit); + td->last_field_index = field->index; + return 1; case HID_DG_CONTACTCOUNT: td->last_field_index = field->index; return 1; case HID_DG_CONTACTMAX: - /* we don't set td->last_slot_field as contactcount and - * contact max are global to the report */ + /* we don't set td->last_slot_field as scan time, + * contactcount and contact max are global to the + * report */ td->last_field_index = field->index; return -1; case HID_DG_TOUCH: @@ -492,7 +501,8 @@ static int mt_input_mapped(struct hid_device *hdev, struct hid_input *hi, struct hid_field *field, struct hid_usage *usage, unsigned long **bit, int *max) { - if (usage->type == EV_KEY || usage->type == EV_ABS) + if (usage->type == EV_KEY || usage->type == EV_ABS || + usage->type == EV_MSC) set_bit(usage->type, hi->input->evbit); return -1; @@ -571,10 +581,27 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input) static void mt_sync_frame(struct mt_device *td, struct input_dev *input) { input_mt_sync_frame(input); + input_event(input, EV_MSC, MSC_TIMESTAMP, td->timestamp); input_sync(input); td->num_received = 0; } +static void mt_compute_timestamp(struct mt_device *td, struct hid_field *field, + unsigned value) +{ + unsigned dt; + + if (value) { + dt = (value - td->dev_time) % (field->logical_maximum + 1); + td->timestamp += 100 * dt; + td->timestamp += !td->timestamp; + } else { + td->timestamp = 0; + } + + td->dev_time = value; +} + static int mt_event(struct hid_device *hid, struct hid_field *field, struct hid_usage *usage, __s32 value) { @@ -622,6 +649,9 @@ static int mt_event(struct hid_device *hid, struct hid_field *field, case HID_DG_HEIGHT: td->curdata.h = value; break; + case HID_DG_SCANTIME: + mt_compute_timestamp(td, field, value); + break; case HID_DG_CONTACTCOUNT: /* * Includes multi-packet support where subsequent diff --git a/include/linux/hid.h b/include/linux/hid.h index d2c42dd..eb7ec6c 100644 --- a/include/linux/hid.h +++ b/include/linux/hid.h @@ -242,6 +242,7 @@ struct hid_item { #define HID_DG_DEVICEINDEX 0x000d0053 #define HID_DG_CONTACTCOUNT 0x000d0054 #define HID_DG_CONTACTMAX 0x000d0055 +#define HID_DG_SCANTIME 0x000d0056 /* * HID report types --- Ouch! HID spec says 1 2 3!