Re: [PATCH weston 21/25] protocol: add weston_touch_calibration

[Peter Hutterer]

On Tue, Apr 24, 2018 at 02:03:41PM +0300, Pekka Paalanen wrote:
> On Wed, 18 Apr 2018 11:30:42 +0300
> Pekka Paalanen  wrote:
> 
> > On Mon, 16 Apr 2018 13:40:44 +1000
> > Peter Hutterer  wrote:
> > 
> > > On Fri, Apr 13, 2018 at 10:51:37AM +0300, Pekka Paalanen wrote:  
> > > >   
> > > > Well, it does touch-downs at least. I think if motion event would end
> > > > up out of range, I'll send cancel event followed by invalid touch.
> > > > Whee, I found use for the cancel event!
> > > 
> > > The cancel event would also be used for the multitouch-case, right?
> > > If a second touch appears before the first one is fully processed, you 
> > > also
> > > need to cancel the first touch. Doesn't even need to be intentional by the
> > > user, could be a palm touch or an accidental touch where they're holding 
> > > the
> > > screen on an edge.  
> > 
> > I hadn't even thought of that, sounds good.
> 
> Actually, I don't have to filter multiple touchpoints in the server.
> The protocol is copied from the wl_touch interface and can handle
> multiple touch points just fine. It can be up to the client to enter a
> denial mode on a second touch down until they are all up.

> Likewise, if the user puts first touch down on a right device, and a
> second touch down on a wrong device which results in a invalid_touch
> event, it can be up to the client to decide whether it accepts the
> first touch or not.
> 
> I'll just have to make the client deal with those.
> 
> Do you see any problem with this?

nope, all good. Sorry, I forgot about the touch ID being in the events...

Cheers,
   Peter
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston 21/25] protocol: add weston_touch_calibration

[Pekka Paalanen]

On Wed, 18 Apr 2018 11:30:42 +0300
Pekka Paalanen  wrote:

> On Mon, 16 Apr 2018 13:40:44 +1000
> Peter Hutterer  wrote:
> 
> > On Fri, Apr 13, 2018 at 10:51:37AM +0300, Pekka Paalanen wrote:  
> > >   
> > > Well, it does touch-downs at least. I think if motion event would end
> > > up out of range, I'll send cancel event followed by invalid touch.
> > > Whee, I found use for the cancel event!
> > 
> > The cancel event would also be used for the multitouch-case, right?
> > If a second touch appears before the first one is fully processed, you also
> > need to cancel the first touch. Doesn't even need to be intentional by the
> > user, could be a palm touch or an accidental touch where they're holding the
> > screen on an edge.  
> 
> I hadn't even thought of that, sounds good.

Actually, I don't have to filter multiple touchpoints in the server.
The protocol is copied from the wl_touch interface and can handle
multiple touch points just fine. It can be up to the client to enter a
denial mode on a second touch down until they are all up.

Likewise, if the user puts first touch down on a right device, and a
second touch down on a wrong device which results in a invalid_touch
event, it can be up to the client to decide whether it accepts the
first touch or not.

I'll just have to make the client deal with those.

Do you see any problem with this?


Thanks,
pq


pgpbJr8kmJ6PH.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston 21/25] protocol: add weston_touch_calibration

[Peter Hutterer]

On Fri, Apr 13, 2018 at 10:51:37AM +0300, Pekka Paalanen wrote:
> On Fri, 13 Apr 2018 14:31:39 +1000
> Peter Hutterer  wrote:
> 
> > On Wed, Apr 11, 2018 at 02:00:49PM +0300, Pekka Paalanen wrote:
> > > On Wed, 11 Apr 2018 10:16:46 +1000
> > > Peter Hutterer  wrote:  
> 
> > > > > > > +  
> > > > > > > +
> > > > > > > +  On creation, this object is tied to a specific touch 
> > > > > > > device. The
> > > > > > > +  server sends a configure event which the client must obey 
> > > > > > > with the
> > > > > > > +  associated wl_surface.
> > > > > > > +
> > > > > > > +  Once the client has committed content to the surface, the 
> > > > > > > server can
> > > > > > > +  grab the touch input device, prevent it from emitting 
> > > > > > > normal touch events,
> > > > > > > +  show the surface on the correct output, and relay input 
> > > > > > > events from the
> > > > > > > +  touch device via this protocol object.
> > > > > > > +
> > > > > > > +  Touch events from other touch devices than the one tied to 
> > > > > > > this object
> > > > > > > +  must generate wrong_touch events on at least touch-down 
> > > > > > > and must not
> > > > > > > +  generate normal or calibration touch events.
> > > > > > > +
> > > > > > > +  At any time, the server can choose to cancel the 
> > > > > > > calibration procedure by
> > > > > > > +  sending the cancel_calibration event. This should also be 
> > > > > > > used if the
> > > > > > > +  touch device disappears or anything else prevents the 
> > > > > > > calibration from
> > > > > > > +  continuing on the server side.
> > > > > > > +
> > > > > > > +  If the wl_surface is destroyed, the server must cancel the 
> > > > > > > calibration.
> > > > > > > +
> > > > > > > +  The touch event coordinates and conversion results are 
> > > > > > > delivered in
> > > > > > > +  calibration units. Calibration units are in the closed 
> > > > > > > interval
> > > > > > > +  [0.0, 1.0] mapped into 32-bit unsigned integers. An 
> > > > > > > integer can be  
> > > > > > 
> > > > > > should probably add what 0.0 and 1.0 represent on each axis, i.e. 
> > > > > > nominal
> > > > > > width and height of the device's sensor. Or is this something we 
> > > > > > need to
> > > > > > hide?
> > > > > 
> > > > > I'm not sure. The underlying assumption is that there is a finite and
> > > > > closed range of values a sensor can output, and that is mapped to [0,
> > > > > 1]. I don't know what nominal width and height are or can the values
> > > > > extend to negative. It might be easier to define the calibration units
> > > > > without defining those first.
> > > > 
> > > > sorry, I used "nominal" before I rewrote to use "sensor", probably made 
> > > > it
> > > > more confusing. On some devices, the sensor is larger than the screen 
> > > > so you
> > > > can get coordinates outside the screen area. This is not a technical
> > > > problem, just a user perception mismatch that doens't need to be 
> > > > exposed and
> > > > after all that's what calibration is about.
> > > > 
> > > > Once you apply calibration though you can get real negative coordinates,
> > > > especially if the device advertises a [n:m] range but then sends events 
> > > > less
> > > > than n. For example, all synaptics touchpads (pre 2014, some after 
> > > > that) do
> > > > that.  
> > > 
> > > But wait, if there are actually devices that report the range [n:m] but
> > > can still send values less than n or greater than m, it means I cannot
> > > transmit those with the encoding set up here, because
> > > libinput_event_touch_get_x_transformed(touch_event, 1) can return
> > > values outside of [0, 1] even when the loaded calibration matrix is
> > > identity. Is that right?  
> > 
> > correct, that can happen.
> > 
> > > Do I need to find an encoding to cope with that, or can I just reject
> > > such touches?
> > > 
> > > I mean, the range [n:m] is in raw input coordinates, before any
> > > normalization or calibration in userspace, right?  
> > 
> > uhm. let me just detail this and you can pick which one was the answer to
> > that question:
> > - the kernel exposes [n:m] axis ranges for ABS_X/Y and the MT equivalents
> > - libinput converts this into mm by default, see 
> > libinput_event_touch_get_x()
> > - libinput converts this to a ranged value [0:N] when calling
> >   libnput_event_touch_get_x_transformed(N). This is what weston uses to get
> >   the screen coordinates.
> > 
> > Both libinput values have the calibration factored in already.
> > 
> > Weston doesn't ever see raw input coordinates. But if you use
> > get_x_transformed, you can get values outside [0:N]. This happens when
> > the [n:m] range is incorrect. Accommodating for some error margin, libinput
> > will print a warning to the log when this happens.
> > 
> > The correct solution would be to warn the user that the 

Re: [PATCH weston 21/25] protocol: add weston_touch_calibration

[Pekka Paalanen]

On Fri, 13 Apr 2018 14:31:39 +1000
Peter Hutterer  wrote:

> On Wed, Apr 11, 2018 at 02:00:49PM +0300, Pekka Paalanen wrote:
> > On Wed, 11 Apr 2018 10:16:46 +1000
> > Peter Hutterer  wrote:  

> > > > > > +  
> > > > > > +
> > > > > > +  On creation, this object is tied to a specific touch device. 
> > > > > > The
> > > > > > +  server sends a configure event which the client must obey 
> > > > > > with the
> > > > > > +  associated wl_surface.
> > > > > > +
> > > > > > +  Once the client has committed content to the surface, the 
> > > > > > server can
> > > > > > +  grab the touch input device, prevent it from emitting normal 
> > > > > > touch events,
> > > > > > +  show the surface on the correct output, and relay input 
> > > > > > events from the
> > > > > > +  touch device via this protocol object.
> > > > > > +
> > > > > > +  Touch events from other touch devices than the one tied to 
> > > > > > this object
> > > > > > +  must generate wrong_touch events on at least touch-down and 
> > > > > > must not
> > > > > > +  generate normal or calibration touch events.
> > > > > > +
> > > > > > +  At any time, the server can choose to cancel the calibration 
> > > > > > procedure by
> > > > > > +  sending the cancel_calibration event. This should also be 
> > > > > > used if the
> > > > > > +  touch device disappears or anything else prevents the 
> > > > > > calibration from
> > > > > > +  continuing on the server side.
> > > > > > +
> > > > > > +  If the wl_surface is destroyed, the server must cancel the 
> > > > > > calibration.
> > > > > > +
> > > > > > +  The touch event coordinates and conversion results are 
> > > > > > delivered in
> > > > > > +  calibration units. Calibration units are in the closed 
> > > > > > interval
> > > > > > +  [0.0, 1.0] mapped into 32-bit unsigned integers. An integer 
> > > > > > can be  
> > > > > 
> > > > > should probably add what 0.0 and 1.0 represent on each axis, i.e. 
> > > > > nominal
> > > > > width and height of the device's sensor. Or is this something we need 
> > > > > to
> > > > > hide?
> > > > 
> > > > I'm not sure. The underlying assumption is that there is a finite and
> > > > closed range of values a sensor can output, and that is mapped to [0,
> > > > 1]. I don't know what nominal width and height are or can the values
> > > > extend to negative. It might be easier to define the calibration units
> > > > without defining those first.
> > > 
> > > sorry, I used "nominal" before I rewrote to use "sensor", probably made it
> > > more confusing. On some devices, the sensor is larger than the screen so 
> > > you
> > > can get coordinates outside the screen area. This is not a technical
> > > problem, just a user perception mismatch that doens't need to be exposed 
> > > and
> > > after all that's what calibration is about.
> > > 
> > > Once you apply calibration though you can get real negative coordinates,
> > > especially if the device advertises a [n:m] range but then sends events 
> > > less
> > > than n. For example, all synaptics touchpads (pre 2014, some after that) 
> > > do
> > > that.  
> > 
> > But wait, if there are actually devices that report the range [n:m] but
> > can still send values less than n or greater than m, it means I cannot
> > transmit those with the encoding set up here, because
> > libinput_event_touch_get_x_transformed(touch_event, 1) can return
> > values outside of [0, 1] even when the loaded calibration matrix is
> > identity. Is that right?  
> 
> correct, that can happen.
> 
> > Do I need to find an encoding to cope with that, or can I just reject
> > such touches?
> > 
> > I mean, the range [n:m] is in raw input coordinates, before any
> > normalization or calibration in userspace, right?  
> 
> uhm. let me just detail this and you can pick which one was the answer to
> that question:
> - the kernel exposes [n:m] axis ranges for ABS_X/Y and the MT equivalents
> - libinput converts this into mm by default, see libinput_event_touch_get_x()
> - libinput converts this to a ranged value [0:N] when calling
>   libnput_event_touch_get_x_transformed(N). This is what weston uses to get
>   the screen coordinates.
> 
> Both libinput values have the calibration factored in already.
> 
> Weston doesn't ever see raw input coordinates. But if you use
> get_x_transformed, you can get values outside [0:N]. This happens when
> the [n:m] range is incorrect. Accommodating for some error margin, libinput
> will print a warning to the log when this happens.
> 
> The correct solution would be to warn the user that the device is garbage
> and make them put a 60-evdev.hwdb quirk in to fix the axis ranges.
> Depending on the device, this could be a generic solution but let's not bet
> on that...
> 
> The user-friendly solution would be to have two layers of mapping. By
> default, you map the libinput 

Re: [PATCH weston 21/25] protocol: add weston_touch_calibration

[Peter Hutterer]

On Wed, Apr 11, 2018 at 02:00:49PM +0300, Pekka Paalanen wrote:
> On Wed, 11 Apr 2018 10:16:46 +1000
> Peter Hutterer  wrote:
> > > > > + An event carries the touch device identification and the 
> > > > > associated
> > > > > + output or head (display connector) name.
> > > > > +
> > > > > + On platforms using udev, the device identification is the udev 
> > > > > DEVPATH.
> > > > 
> > > > do we want to really set this in stone in the protocol? Also, the 
> > > > DEVPATH
> > > > is without /sys, right? Why not make this an open format and let the 
> > > > client
> > > > figure it out. It's not hard to strcmp for /sys or /dev/input/event in 
> > > > the
> > > > client and iirc we have precedence for that in the tablet protocol. or 
> > > > in
> > > > libinput for tablets, can't remember now :)  
> > > 
> > > Yes, I wanted to have a clear, unambgious definition. What udev calls
> > > the DEVPATH is the path without /sys. We can revise this decision
> > > if there ever arises a need for anything else, but it's still a Weston
> > > private protocol for now.
> > > 
> > > When would one use /dev paths?  
> > 
> > *shrug*, not sure in this case, just used that as an example here. Point was
> > more: prefixing with /sys makes it a lot more identifiable than just using
> > the DEVPATH, not least because you can immediately stat that path (or ls, 
> > important
> > for bug reporters).
> > 
> > also, udev_device_new_from_syspath() seems to error out if it doesn't start
> > with "/sys", so any programmatic user would have to pre-pend /sys anway.
> 
> Ok, I can change it to be "DEVPATH prefixed with /sys". Or is SYSPATH a
> thing defined by udev? There is udev_device_get_syspath() but I cannot
> get 'udevadm info' to print that. Originally I wanted to stick to what
> I can see with 'udevadm info'.

I checked the libudev sources and it has:
src/libsystemd/sd-device/sd-device.c:device_set_syspath()

devpath = syspath + STRLEN("/sys");

so DEVPATH is literally the substring of the syspath.

> but I cannot get 'udevadm info' to print that.

this is because udevadm info takes the syspath as input argument to figure
out which device it needs to print. So I suspect it decided not to print it
because you've already given it on the commandline anyway :) Even though any
single device has multiple syspaths that are all symlinked to the
/sys/$DEVPATH node.

Ironically, when you use udevadm info -p $DEVPATH, it also needs to prefix
'/sys' to be able to do anything with the device (see
src/udev/udevadm-info.c)

> > > Btw. are device node and DEVPATH equally racy wrt. device getting
> > > replaced with a different one?  
> > 
> > not 100% sure. Theory is that if you have the DEVPATH you can get the
> > devnode through udev and by then all races should've been resolved. If you
> > get the device node independently, then you're bound to run into races.
> 
> Alright, I'll pay no further thought to device nodes.

Reading this again and thinking about it some more: if you replace a device
node quickly enough, you'll lag behind and run into this issue. So by the
time you get the udev device's devnode the device may have been removed and
the kernel replaced it with another device, re-using the event node. You'll
eventually get the device-removed event from udev but...

In the libinput test suite I work around this by comparing the syspaths, in
the form of:
   devnode = udev_device_get_devnode(dev);
   fd = open(devnode)
   new_dev = udev_device_new_from_devnum(fd)

   strcmp(udev_device_get_syspath(dev), udev_device_get_syspath(new_dev))
   if not equal:
   oops, we've raced ourselves into a corner


I need this because I create and remove hundreds of devices per minute.
I doubt it's needed in a real-world situation though.

> > > We could have rules like if it starts with /sys, remove /sys and you
> > > have the DEVPATH; if it starts with /dev, it's the device node, and so
> > > on, but I don't see the point. So rather than pretending that I cater
> > > for other environments, I just rule them out for now.  
> > 
> > sure, it's fine to just support udev. I just question the utility of DEVPATH
> > in particular. I mean DEVPATH=/devices/rmi4-00/input/input25/event17 on my
> > touchpad, but /sys/class/input/event17 gives me the same udev device. Or
> > /sys/dev/char/.../
> 
> You cannot use any alias in the 'save' or 'create_calibrator' requests
> in any case, Weston does not create a new udev_device from it. It
> simply compares the given device string to what it advertised.
> 
> DEVPATH seemed to be all of unique, well-defined, well-known, and
> easily available and usable. Otherwise I don't care what the device
> string is.

sure, my argument was that using the syspath gives you the same but it's
more flexible when used with other tools.

> > > > > +  
> > > > > +   > > > > +   summary="the touch device identification"/>
> > > > > +   > > > > +   summary="name 

Re: [PATCH weston 21/25] protocol: add weston_touch_calibration

[Pekka Paalanen]

On Wed, 11 Apr 2018 10:16:46 +1000
Peter Hutterer  wrote:

> On Tue, Apr 10, 2018 at 02:01:10PM +0300, Pekka Paalanen wrote:
> > On Mon, 9 Apr 2018 12:54:43 +1000
> > Peter Hutterer  wrote:
> >   
> > > On Fri, Mar 23, 2018 at 02:01:01PM +0200, Pekka Paalanen wrote:  
> > > > From: Pekka Paalanen 
> > > > 
> > > > This is a Wayland protocol extension to allow the calibration of
> > > > touchscreens in Weston.
> > > > 
> > > > See: https://phabricator.freedesktop.org/T7868
> > > > 
> > > > Signed-off-by: Pekka Paalanen 
> > > > ---
> > > >  Makefile.am   |   5 +-
> > > >  protocol/weston-touch-calibration.xml | 320 
> > > > ++
> > > >  2 files changed, 324 insertions(+), 1 deletion(-)
> > > >  create mode 100644 protocol/weston-touch-calibration.xml  
> >   
> > > > +  
> > > > +
> > > > +  This is the global interface for calibrating a touchscreen input
> > > > +  coordinate transformation. It is recommended to make this 
> > > > interface
> > > > +  privileged.
> > > > +
> > > > +  This interface can be used by a client to show a calibration 
> > > > pattern and
> > > > +  receive uncalibrated touch coordinates, facilitating the 
> > > > computation of
> > > > +  a calibration transformation that will align actual touch 
> > > > positions
> > > > +  on screen with their expected coordinates.
> > > > +
> > > > +  Immediately after being bound by a client, the server sends the
> > > > +  touch_device events.
> > > 
> > > s/server/compositor/, in a few more places  
> > 
> > I'm a bit mixed there. I tried to be consistent with "server", but one
> > "compositor" still remained. There are a couple dozen "server".
> > 
> > Is your point that all Wayland spec language should be using
> > "compositor" to talk about the server-side?  
> 
> Yeah, sorry, should've made this clear. 30 years of confusion about X'
> client and server model suggests using 'compositor' and 'client' is
> superior, simply because at least you know one side for sure now :)

I wouldn't be so sure, the misconception that Wayland is a display
server, desktop projects just write their compositors and without
understanding that server=compositor was pretty wide-spread. :-)

At least the client and server model is still the same. Compositor used
to be a client or neither really, and now it is a server, so I could
argue that "compositor" is more confusing than "server". Hence I tend
to talk about a display server nowadays.

But ok, I can follow the precedent and call it "compositor".


> > > > +   An event carries the touch device identification and the 
> > > > associated
> > > > +   output or head (display connector) name.
> > > > +
> > > > +   On platforms using udev, the device identification is the udev 
> > > > DEVPATH.
> > > 
> > > do we want to really set this in stone in the protocol? Also, the DEVPATH
> > > is without /sys, right? Why not make this an open format and let the 
> > > client
> > > figure it out. It's not hard to strcmp for /sys or /dev/input/event in the
> > > client and iirc we have precedence for that in the tablet protocol. or in
> > > libinput for tablets, can't remember now :)  
> > 
> > Yes, I wanted to have a clear, unambgious definition. What udev calls
> > the DEVPATH is the path without /sys. We can revise this decision
> > if there ever arises a need for anything else, but it's still a Weston
> > private protocol for now.
> > 
> > When would one use /dev paths?  
> 
> *shrug*, not sure in this case, just used that as an example here. Point was
> more: prefixing with /sys makes it a lot more identifiable than just using
> the DEVPATH, not least because you can immediately stat that path (or ls, 
> important
> for bug reporters).
> 
> also, udev_device_new_from_syspath() seems to error out if it doesn't start
> with "/sys", so any programmatic user would have to pre-pend /sys anway.

Ok, I can change it to be "DEVPATH prefixed with /sys". Or is SYSPATH a
thing defined by udev? There is udev_device_get_syspath() but I cannot
get 'udevadm info' to print that. Originally I wanted to stick to what
I can see with 'udevadm info'.

> > Btw. are device node and DEVPATH equally racy wrt. device getting
> > replaced with a different one?  
> 
> not 100% sure. Theory is that if you have the DEVPATH you can get the
> devnode through udev and by then all races should've been resolved. If you
> get the device node independently, then you're bound to run into races.

Alright, I'll pay no further thought to device nodes.

> > We could have rules like if it starts with /sys, remove /sys and you
> > have the DEVPATH; if it starts with /dev, it's the device node, and so
> > on, but I don't see the point. So rather than pretending that I cater
> > for other environments, I just rule them out for now.  
> 

Re: [PATCH weston 21/25] protocol: add weston_touch_calibration

[Peter Hutterer]

On Tue, Apr 10, 2018 at 02:01:10PM +0300, Pekka Paalanen wrote:
> On Mon, 9 Apr 2018 12:54:43 +1000
> Peter Hutterer  wrote:
> 
> > On Fri, Mar 23, 2018 at 02:01:01PM +0200, Pekka Paalanen wrote:
> > > From: Pekka Paalanen 
> > > 
> > > This is a Wayland protocol extension to allow the calibration of
> > > touchscreens in Weston.
> > > 
> > > See: https://phabricator.freedesktop.org/T7868
> > > 
> > > Signed-off-by: Pekka Paalanen 
> > > ---
> > >  Makefile.am   |   5 +-
> > >  protocol/weston-touch-calibration.xml | 320 
> > > ++
> > >  2 files changed, 324 insertions(+), 1 deletion(-)
> > >  create mode 100644 protocol/weston-touch-calibration.xml
> 
> > > +  
> > > +
> > > +  This is the global interface for calibrating a touchscreen input
> > > +  coordinate transformation. It is recommended to make this interface
> > > +  privileged.
> > > +
> > > +  This interface can be used by a client to show a calibration 
> > > pattern and
> > > +  receive uncalibrated touch coordinates, facilitating the 
> > > computation of
> > > +  a calibration transformation that will align actual touch positions
> > > +  on screen with their expected coordinates.
> > > +
> > > +  Immediately after being bound by a client, the server sends the
> > > +  touch_device events.  
> > 
> > s/server/compositor/, in a few more places
> 
> I'm a bit mixed there. I tried to be consistent with "server", but one
> "compositor" still remained. There are a couple dozen "server".
> 
> Is your point that all Wayland spec language should be using
> "compositor" to talk about the server-side?

Yeah, sorry, should've made this clear. 30 years of confusion about X'
client and server model suggests using 'compositor' and 'client' is
superior, simply because at least you know one side for sure now :)
 

> > > +
> > > +  
> > > + This gives the calibrator role to the surface and ties it with the
> > > + given touch input device.
> > > +
> > > + The surface must not already have a role, otherwise invalid_surface
> > > + error is raised.
> > > +
> > > + The device string must be one advertised with touch_device event's
> > > + device argument, otherwise invalid_device error is raised.
> > > +
> > > + There must not exist a weston_touch_calibrator protocol object in the
> > > + server already, otherwise already_exists error is raised. This
> > > + limitation is server-wide and not specific to any particular client.  
> > 
> > Personal preference: I find it easier to understand when a sentence is "if
> > blah then error E". As opposed to "if not blah, otherwise E". It also
> > narrows down the error cases a bit better too because you're only describing
> > the error case rather than the not-error case.
> 
> Yup.
> 
> > > +  
> > > +   > > +   summary="the surface to give the role to"/>
> > > +  
> > > +   > > +   summary="a new calibrator object"/>
> > > +
> > > +
> > > +
> > > +  
> > > + This request asks the server to save the calibration data for the
> > > + given touch input device. The server may ignore this request.
> > > +
> > > + The device string must be one advertised with touch_device event's
> > > + device argument, otherwise invalid_device error is raised.
> > > +
> > > + The array must contain exactly six 'float' (the 32-bit floating
> > > + point format used by the C language on the system) numbers. The numbers
> > > + form a libinput style 2-by-3 calibration matrix in row-major order.  
> > 
> > 'libinput-style', i.e. hyphenated. but tbh, no need to mention libinput
> > here, just spell out the matrix:
> > """
> >  For a 3D matrix in the form
> >( a b c )
> >( d e f )
> >( 0 0 1 )
> >  the array must contain the values [a, b, c, d, e, f] with a at index 0.
> > """
> 
> Not only that, but it also needs to define the coordinate spaces where
> it applies, and probably also the ordering with other transformations
> as well. Hence I just punted to "how libinput uses it", because
> everything here is specifically designed for libinput.
> 
> I'm also not sure I can actually lay out a matrix so that it will still
> look ok in generated output and doxygen docs generated from that.

@code and @endcode in doxygen map to  (you can use the html tag
directly too). Or there's @verbatim and @endverbatim.  You can do fancy
things with latex like libinput does, but it makes the source pretty awful
to look at (and introduces extra build dependencies).

> > Much easier than figuring out what "row-major order" means.
> 
> It's a standard term.
> 
> > > +  
> > > +  
> > > +  
> > > +
> > > +
> > > +
> > > +  
> > > + When a client binds to weston_touch_calibration, one touch_device
> > > + event is sent for each touchscreen that is available to be calibrated.
> > > + These events are 

Re: [PATCH weston 21/25] protocol: add weston_touch_calibration

[Pekka Paalanen]

On Mon, 9 Apr 2018 12:54:43 +1000
Peter Hutterer  wrote:

> On Fri, Mar 23, 2018 at 02:01:01PM +0200, Pekka Paalanen wrote:
> > From: Pekka Paalanen 
> > 
> > This is a Wayland protocol extension to allow the calibration of
> > touchscreens in Weston.
> > 
> > See: https://phabricator.freedesktop.org/T7868
> > 
> > Signed-off-by: Pekka Paalanen 
> > ---
> >  Makefile.am   |   5 +-
> >  protocol/weston-touch-calibration.xml | 320 
> > ++
> >  2 files changed, 324 insertions(+), 1 deletion(-)
> >  create mode 100644 protocol/weston-touch-calibration.xml

> > +  
> > +
> > +  This is the global interface for calibrating a touchscreen input
> > +  coordinate transformation. It is recommended to make this interface
> > +  privileged.
> > +
> > +  This interface can be used by a client to show a calibration pattern 
> > and
> > +  receive uncalibrated touch coordinates, facilitating the computation 
> > of
> > +  a calibration transformation that will align actual touch positions
> > +  on screen with their expected coordinates.
> > +
> > +  Immediately after being bound by a client, the server sends the
> > +  touch_device events.  
> 
> s/server/compositor/, in a few more places

I'm a bit mixed there. I tried to be consistent with "server", but one
"compositor" still remained. There are a couple dozen "server".

Is your point that all Wayland spec language should be using
"compositor" to talk about the server-side?


> > +
> > +  
> > +   This gives the calibrator role to the surface and ties it with the
> > +   given touch input device.
> > +
> > +   The surface must not already have a role, otherwise invalid_surface
> > +   error is raised.
> > +
> > +   The device string must be one advertised with touch_device event's
> > +   device argument, otherwise invalid_device error is raised.
> > +
> > +   There must not exist a weston_touch_calibrator protocol object in the
> > +   server already, otherwise already_exists error is raised. This
> > +   limitation is server-wide and not specific to any particular client.  
> 
> Personal preference: I find it easier to understand when a sentence is "if
> blah then error E". As opposed to "if not blah, otherwise E". It also
> narrows down the error cases a bit better too because you're only describing
> the error case rather than the not-error case.

Yup.

> > +  
> > +   > +   summary="the surface to give the role to"/>
> > +  
> > +   > +   summary="a new calibrator object"/>
> > +
> > +
> > +
> > +  
> > +   This request asks the server to save the calibration data for the
> > +   given touch input device. The server may ignore this request.
> > +
> > +   The device string must be one advertised with touch_device event's
> > +   device argument, otherwise invalid_device error is raised.
> > +
> > +   The array must contain exactly six 'float' (the 32-bit floating
> > +   point format used by the C language on the system) numbers. The numbers
> > +   form a libinput style 2-by-3 calibration matrix in row-major order.  
> 
> 'libinput-style', i.e. hyphenated. but tbh, no need to mention libinput
> here, just spell out the matrix:
> """
>  For a 3D matrix in the form
>( a b c )
>( d e f )
>( 0 0 1 )
>  the array must contain the values [a, b, c, d, e, f] with a at index 0.
> """

Not only that, but it also needs to define the coordinate spaces where
it applies, and probably also the ordering with other transformations
as well. Hence I just punted to "how libinput uses it", because
everything here is specifically designed for libinput.

I'm also not sure I can actually lay out a matrix so that it will still
look ok in generated output and doxygen docs generated from that.

> Much easier than figuring out what "row-major order" means.

It's a standard term.

> > +  
> > +  
> > +  
> > +
> > +
> > +
> > +  
> > +   When a client binds to weston_touch_calibration, one touch_device
> > +   event is sent for each touchscreen that is available to be calibrated.
> > +   These events are not sent later even if new touch devices appear.  
> 
> Unclear what "later" means, use "Touch devices added after the initial burst
> of events will not generate a touch_device event".

More like:

Touch devices added in the server will not generate touch_device events
for existing weston_touch_calibration objects.

Or simply:

This is the only time when touch_device events are sent.

> Though arguably - why not? Because it makes things easier to implement?

Yes. There is no event to remove a touch device, and there is no need
for a client to maintain an accurate list of touch devices.

Since there is no event to remove a touch device, there also is no
protocol race between server removing and the client using one.

Re: [PATCH weston 21/25] protocol: add weston_touch_calibration

[Peter Hutterer]

On Fri, Mar 23, 2018 at 02:01:01PM +0200, Pekka Paalanen wrote:
> From: Pekka Paalanen 
> 
> This is a Wayland protocol extension to allow the calibration of
> touchscreens in Weston.
> 
> See: https://phabricator.freedesktop.org/T7868
> 
> Signed-off-by: Pekka Paalanen 
> ---
>  Makefile.am   |   5 +-
>  protocol/weston-touch-calibration.xml | 320 
> ++
>  2 files changed, 324 insertions(+), 1 deletion(-)
>  create mode 100644 protocol/weston-touch-calibration.xml
> 
> diff --git a/Makefile.am b/Makefile.am
> index e028a2a1..5e830777 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -168,7 +168,9 @@ nodist_libweston_@LIBWESTON_MAJOR@_la_SOURCES =   
> \
>   protocol/pointer-constraints-unstable-v1-protocol.c \
>   protocol/pointer-constraints-unstable-v1-server-protocol.h  \
>   protocol/input-timestamps-unstable-v1-protocol.c\
> - protocol/input-timestamps-unstable-v1-server-protocol.h
> + protocol/input-timestamps-unstable-v1-server-protocol.h \
> + protocol/weston-touch-calibration-protocol.c\
> + protocol/weston-touch-calibration-server-protocol.h
>  
>  BUILT_SOURCES += $(nodist_libweston_@LIBWESTON_MAJOR@_la_SOURCES)
>  
> @@ -1534,6 +1536,7 @@ EXTRA_DIST +=   \
>   protocol/weston-screenshooter.xml   \
>   protocol/text-cursor-position.xml   \
>   protocol/weston-test.xml\
> + protocol/weston-touch-calibration.xml   \
>   protocol/ivi-application.xml\
>   protocol/ivi-hmi-controller.xml
>  
> diff --git a/protocol/weston-touch-calibration.xml 
> b/protocol/weston-touch-calibration.xml
> new file mode 100644
> index ..b1e19f9b
> --- /dev/null
> +++ b/protocol/weston-touch-calibration.xml
> @@ -0,0 +1,320 @@
> +
> +
> +
> +  
> +Copyright © 2017 Collabora, Ltd.
> +Copyright © 2017 General Electric Company
> +
> +Permission is hereby granted, free of charge, to any person obtaining a
> +copy of this software and associated documentation files (the 
> "Software"),
> +to deal in the Software without restriction, including without limitation
> +the rights to use, copy, modify, merge, publish, distribute, sublicense,
> +and/or sell copies of the Software, and to permit persons to whom the
> +Software is furnished to do so, subject to the following conditions:
> +
> +The above copyright notice and this permission notice (including the next
> +paragraph) shall be included in all copies or substantial portions of the
> +Software.
> +
> +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS 
> OR
> +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> +THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR 
> OTHER
> +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> +FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> +DEALINGS IN THE SOFTWARE.
> +  
> +
> +  
> +
> +  This is the global interface for calibrating a touchscreen input
> +  coordinate transformation. It is recommended to make this interface
> +  privileged.
> +
> +  This interface can be used by a client to show a calibration pattern 
> and
> +  receive uncalibrated touch coordinates, facilitating the computation of
> +  a calibration transformation that will align actual touch positions
> +  on screen with their expected coordinates.
> +
> +  Immediately after being bound by a client, the server sends the
> +  touch_device events.

s/server/compositor/, in a few more places

> +
> +  The client chooses a touch device from the touch_device events,
> +  creates a wl_surface and then a weston_touch_calibrator for the
> +  wl_surface and the chosen touch device. The client waits for the server
> +  to send a configure event before it starts drawing the first 
> calibration
> +  pattern. After receiving the configure event, the client will iterate
> +  drawing a pattern, getting touch input via weston_touch_calibrator,
> +  and converting pixel coordinates to expected touch coordinates with
> +  weston_touch_calibrator.convert until it has enough correspondences to
> +  compute the calibration transformation or the server cancels the
> +  calibration.
> +
> +  Once the client has successfully computed a new calibration, it can use
> +  weston_touch_calibration.save request to load the new calibration into
> +  the server. The server may take this new calibration into use and may
> +  write it into persistent storage.
> +
> +
> +
> +  
> +   + summary="the