<div dir="ltr">Hi, <br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Sep 1, 2015 at 8:30 AM, Peter Hutterer <span dir="ltr"><<a href="mailto:peter.hutterer@who-t.net" target="_blank">peter.hutterer@who-t.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
[...]<br>
but first up, I have to ask: how did you test this, do you have an<br>
implementation higher up in the stack that makes use of this? The API looks<br>
generally ok now, but once I started testing it it didn't turn out too well.<br></blockquote><div><br></div><div>I have a small tool that prints one line per libevdev touch event and printed the additional data. Additionally I used it with mir. But most of the time I used that tool on the various phones.<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
what is the actual range of the major/minor your touchpad devices give you?<br>
my touchscreen here gives me a measly 7-10 device units, translating to less<br>
than 1mm. The orientation works correctly (1 bit), I don't have pressure<br>
here.<br>
double-checking with a magic trackpad shows that the range there is<br>
accurate, so now we need to figure out how commonplace the bogus major/minor<br>
coordinates are.<br></blockquote><div><br></div><div>I usually get values between 5 and 14. And yes since the resolution is usually reported with zero and corrected with 1 those values make not that much sense in mm. When scaled to pixels I believe applying an additional scaling factor would provide more accurate values. But that scaling differs from device to device. But with some time I could come up with correction values.<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
also: it's a bit disappointing that I had to fix the tools myself to<br>
actually print or display this data. this should be part of this patch and<br>
again brings up the question on how you tested this.<br>
adding printfs to the event-debug tool also revealed a bug with the<br>
has_major/has_minor/has_... implementation (see later in this email)<br></blockquote><div><br></div><div>Appologies, I didnt look into the directory and just realize what is there. Next patch will extend to that too.<br><br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span>
> #define DEFAULT_MIDDLE_BUTTON_SCROLL_TIMEOUT ms2us(200)<br>
> +#define DEFAULT_TOUCH_PRESSURE 1<br>
> +#define DEFAULT_TOUCH_ORIENTATION 0<br>
> +#define DEFAULT_TOUCH_MAJOR 0<br>
> +#define DEFAULT_TOUCH_MINOR 0<br>
<br>
</span>make this 0.0 and 1.0 since we're dealing with doubles<br></blockquote><div><br></div><div><br>
> + break;<br>
> + case ABS_MT_TOUCH_MAJOR:<br>
> + current_slot->available_data |= TOUCH_SLOT_DATA_MAJOR;<br>
> + current_slot->area.major = e->value;<br>
<br>
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">the handling of the has_major/minor/orientation isn't correct. if a device<br>
has the axis, you can get the current per-slot value on device startup.<br>
the value remains valid until overwritten by a new value on the device. so<br>
libinput_event_touch_has_major() doesn't tell us whether a specific event<br>
has a major axis, but rather whether a device do the axis at all, i.e. it's<br>
either always true or always false. <br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
the API could be libinput_device_has_... but I think the per-event API is<br>
better. the current implementation is broken though, just add a printf to<br>
event-debug and you'll see how we don't know major/minor until we get an<br>
event for each.<br></blockquote><div><br></div><div>With my touchscreens I get major and minor on the first down. But after looking at the output of the tool again, it seems that the down event does not have those values. Instead it still holds stale values from the last owner of the slot. I am looking into that now. In previous versions of the patch I reset everything when the tracking_id is itself reset to -1. The current code (modulo that bug) assumes that with the mt protocol you will always have valid data for an axis available. There should not be a need to reset individual values. So yes if the touch screen supports a certain axis libinput_touch_event_has_ will indicate 1 on each down or move event even when not changed during that move or down action. <br><br>I have no objection to go back to the previous behavior and reset on ABS_MT_TRACKING_ID = -1, since it wont make a difference to the user - code wise it seems more consistent. <br></div><div><br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
orientation is the same: there's no orientation value for the first touch<br>
point, though after that it keeps being printed (which ironically means that<br>
available_data is never reset).<br>
<div><div><br>
> + break;<br>
> + case ABS_MT_TOUCH_MINOR:<br>
> + current_slot->available_data |= TOUCH_SLOT_DATA_MINOR;<br>
> + current_slot->area.minor = e->value;<br>
> + break;<br>
> + case ABS_MT_ORIENTATION:<br>
> + current_slot->available_data |=<br>
> +<br>
> + struct axis_replacement down_values[] = {<br>
> + {ABS_MT_PRESSURE, input_pressure},<br>
> + {ABS_MT_TOUCH_MAJOR, input_major},<br>
> + {-1, -1}};<br>
> + struct axis_replacement move_values[] = {<br>
> + {ABS_MT_TOUCH_MAJOR, input_major_2},<br>
> + {-1, -1}};<br>
> +<br>
> + const struct input_absinfo *orientation_info;<br>
> + const struct input_absinfo *pressure_info;<br>
> + const struct input_absinfo *x_info;<br>
> + const struct input_absinfo *major_info;<br>
> + double touch_major_scale;<br>
> +<br>
> + double expected_major;<br>
> + double expected_minor;<br>
> + double expected_orientation = 0.0;<br>
> + double expected_pressure;<br>
> +<br>
> + x_info = libevdev_get_abs_info(dev->evdev, ABS_MT_POSITION_X);<br>
> + pressure_info = libevdev_get_abs_info(dev->evdev, ABS_MT_PRESSURE);<br>
> + major_info = libevdev_get_abs_info(dev->evdev, ABS_MT_TOUCH_MAJOR);<br>
> + orientation_info = libevdev_get_abs_info(dev->evdev,<br>
> + ABS_MT_ORIENTATION);<br>
> +<br>
> + if (orientation_info || !pressure_info || !x_info || !major_info ||<br>
<br>
</div></div>do we have a touch screen that doesn't have x_info?<br>
I had to look through the test devices here to make sure we have something<br>
that actually runs the test - I wonder if there's some litest addition we<br>
can do to make sure a test runs fully at least once. we've had the problem<br>
once or twice in the past where a test returned early and the actual test<br>
bits didn't get triggered by any device in the list.<br></blockquote><div><br></div><div>It is hard to resist pointer checking... <br><br>We could override the END_TEST macro, somehow mark the test to be executed next inside litest, and then use code inside the END_TEST macro to indicate that the test completed without bailing out.<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span><br>
> + !libevdev_has_event_code(dev->evdev, EV_ABS, ABS_MT_TOUCH_MAJOR) ||<br>
> + libevdev_has_event_code(dev->evdev, EV_ABS, ABS_MT_TOUCH_MINOR))<br>
> + return;<br>
> +<br>
> + if (exceeds_range(pressure_info, input_pressure) ||<br>
> + exceeds_range(major_info, input_major) ||<br>
> + exceeds_range(major_info, input_major_2)) {<br>
> + fprintf(stderr, "%s does not support the required value ranges",<br>
> + libinput_device_get_name(dev->libinput_device));</span><br></blockquote><div><br></div><div>Wrt ck_abort_msg the intention here and above it so to provide some info that the device was not used for this test instead of failing the test. A ck_warn_msg instead of failure. Then again I could also add another layer of calculation and scale the test input values to the ranges supported by the device. But it already starts to feel wrong that I have put calculations mirroring libinputs behavior inside the test. Doing that would mean adapting the test itself to make it fit to the growing set of devices in the suite. The idea behind those two tests was that the added properties are passed through libinput and exposed to the user of the API, and that if the device indicates changes those will be indicated too. The second test just adds the bevior for default values.<br><br></div><div>The point I am trying to get at is that this problem seems to be an artifact of the test strategy. I believe tests should be as self contained as possible, test one thing, be simple to read, not have their own logic the reader has to interpret, otherwise they themselves might contain failures or errors in the logic might hide errors in the test code. With the current approach there is a part of the input parameters hard coded or configureable inside the device description. A central entity selecting which tests to run on which simulated device (litest and the test registry), and then the test itself. That being said I dont have an easy solution. So for most of the logic I added smaller unit tests that run only internal parts libinput without a simulated device would have been sufficient. Then a simple acceptance test that declares a simulated device for the test and has hard coded input and output values.. To get the unit tests working without a uinput device requires creating interfaces that can be overriden, adding that would bubble upwards through the internals of libinput. <br></div><div><br></div><div>Fixed all the other findings locally. Thanks for the review! <br><br></div><div>regards<br></div><div>Andreas<br></div></div></div></div>