[PATCH wayland-protocols] Add the tablet protocol

[Peter Hutterer]

Hi Daniel,

Thanks for the review. I'll reply to a few select pieces, anything skipped
is something I'll just fold into the next diff.

On Mon, Nov 16, 2015 at 11:21:36AM +0000, Daniel Stone wrote:
> Peter, Stephen,
> 
> On 6 November 2015 at 04:24, Peter Hutterer <peter.hutterer at who-t.net> wrote:
> > This is the revamped version of the tablet protocol for graphics tablets
> > (e.g. Wacom tablets). Too many changes from the last version (a year ago or
> > so), so I won't detail them, best to look at it with fresh eyes.
> 
> Thanks a lot for doing this! Looks really good to me, and I think
> getting it in wayland-protocols would be good.
> 
> Do you have other implementations, e.g. GTK+/Mutter? If so, a pointer
> (ha) to those would be great.
> 
> Obligatory nitpicks follow:
> 
> > +<?xml version="1.0" encoding="UTF-8"?>
> > +<protocol name="wayland_tablet_unstable_v1">
> > +  <description summary="Wayland protocol for graphics tablets">
> > +    This description provides a high-level overview of the interplay between
> > +    the interfaces defined this protocol. For details, see the protocol
> > +    specification.
> > +
> > +    More than one tablet may exist, and device-specifics matter. Tablets are
> > +    not represented by a single virtual device like wl_pointer. A client
> > +    binds to the tablet manager object which is just a proxy object. From
> > +    that, the client requests wp_tablet_manager.get_tablet_seat(wl_seat)
> > +    and that returns the actual interface that has all the tablets. With
> > +    this indirection, we can avoid merging wp_tablet into the actual wayland
> > +    protocol, a long-term benefit.
> 
> Yes, it turns out the wl_seat.get_* model was probably a pretty bad
> anti-pattern. Oh well. To avoid too much object proliferation though,
> how about something like:
> <interface name="wp_tablet_manager">
>   <request name="add_seat">
>     <description summary="Add seat of interest">
>       Add a seat of interest to this tablet manager. The client will
> receive events for all tablets currently on this seat, as well as
> tablets added to this seat in future.
>     </description>
>     <arg name="seat" type="object" interface="wl_seat"/>
>   </request>
>   [inline wp_tablet_seat here ...]
> </interface>
> 
> Then you can just bin wp_tablet_seat.
> 
> Not a pattern we've previously employed, but I think it's a good one
> that avoids one extra step of indirection, as well as making it a bit
> easier for clients to do the right thing.
> 
> > +    The wp_tablet_seat sends a "tablet added" for each tablet connected.
> > +    That event is followed by descriptive events about the hardware;
> > +    currently that includes events for name, vid/pid and
> > +    internal/external/display type, and a wp_tablet.path event that
> > +    describes a local path. This path can be used to uniquely identify a
> > +    tablet, or get more information through libwacom.  Emulated or nested
> > +    tablets can skip any of those, e.g. a virtual tablet may not have a
> > +    vid/pid. The sequence of descriptive events is terminated by a
> > +    wp_tablet.done event to signal that a client may now finalize any
> > +    initialization for that tablet.
> 
> Is VID/PID in USB space? (Answers in diff form preferred.)
> 
> > +    Two special events (that don't exist in X) are down and up. They signal
> > +    "tip touching the surface". For tablets without real proximity
> > +    detection, the sequence is: proximity_in, motion, down, frame.
> > +
> > +    When the tool leaves proximity, a proximity_out event is sent, followed
> > +    by button release events for any button still down. This signals to
> > +    the client that the buttons were held when the tool left proximity.
> > +    Those events are all sent within the same frame.
> 
> Can we please enforce a strict bracketing of down/up occurring in
> matched pairs inside of proximity_in/out? So, the total event stream
> would be:
>   - proximity_in
>     - motion
>     - down
>       - motion/motion/button/...
>     - up
>   - proximity_out
> 
> I assume the implementation does this, but it would be nice to have
> encoded in the protocol.

the original plan was to have the proximity out event sent before the button
event iff the tool goes out of proximity with the button still pressed. the
frame events (similar to SYN_REPORT) group them together, so as long as you
process those correctly you still know the state. so the order would be,
and I'm using button event here rather than the tip event because that is
actually likely to happen:

- proximity_in
- motion
- frame
- button down
- frame
- proximity out
- button up
- frame

The above signals that the button was still down during proximity.
but you're right, it be easier to pair them symmetrically and any sensible
client that handles frame events correctly will do exactly the right thing.
So the above would turn into:

- proximity_in
- motion
- frame
- button down
- frame
- button up
- proximity out
- frame

I'll reword that.
 
> > +    If the tool moves out of the surface but stays in proximity (i.e.
> > +    between windows), compositor-specific grab policies apply. This usually
> > +    means that the proximity-out is delayed until all buttons are released.
> 
> If no buttons are down, we'd _expect_ to just get a proximity_out
> immediately, right? It almost seems like we'd want a subtype argument
> here, enumerating what happened: did the user lift the tool entirely
> (actually out of proximity), or is the tablet still in proximity but
> just wandered off to another surface?
> 
> The doc for proximity_out also doesn't contradict this, but reading
> the two together is a bit jarring:
> > + [wp_tablet_tool1::proximity_out]
> > +       When the tablet tool leaves proximity of the tablet, button release
> > +       events are sent for each button that was held down at the time of
> > +       leaving proximity. These events are sent in the same wp_tablet.frame
> > +       of the proximity_out event.
> 
> It'd be nice to have discussion of the two cases - genuinely leaving
> proximity vs. retaining proximity but leaving surface focus -
> together.

iirc we had that at some point early on and decided to drop the separate
enter/leave event. I can't think of a good use-case where you need to know
the difference between enter/leave vs proximity in/out. 

The physical hardware's distance detection is fairly unpredictable so it's
quite common to leave physical proximity when moving across a tablet (i.e.
one window to the other).

if you have a good case here why we need to differ the two I'm all ears, I
just haven't been able to come up with one yet.

> > +    Moving a tool physically from one tablet to the other has no real effect
> > +    on the protocol, since we already have the tool object from the "tool
> > +    added" event. All the information is already there and the proximity_in
> > +    is all a client needs to reconstruct what happened.
> 
> ... also (up+)proximity_out from the old tablet.
> 
> > +  <interface name="zwp_tablet_tool1" version="1">
> > +    <description summary="a physical tablet tool">
> > +      An unique object that represents a physical tool that has been, or is
> > +      currently in use with a tablet in this seat. Each wp_tablet_tool
> > +      object stays valid until the client destroys it; the compositor
> > +      reuses the wp_tablet_tool object to indicate that the object's
> > +      respective physical tool has come into proximity of a tablet again.
> > +      A client must not call wp_tablet_tool.destroy until it receives a
> > +      wp_tablet_tool.release event.
> > +
> > +      A wp_tablet_tool object's uniqueness depends on the tablet's ability
> > +      to report serial numbers. If the tablet doesn't support this
> > +      capability, then the tool cannot be guaranteed to be unique.
> 
> 'Unique' is a little confusing - perhaps 'a one-to-one mapping between
> tool objects and physical tools' or similar. Also if we're not
> providing some kind of tablet caps event, that you can't query this.
> It's also pretty contradictory with the first sentence. :) A
> 'generally' there would be enough to weasel out of it.

that's just a mixup in wording. the object is unique either way, whether
it's mapped to the same physical tool depends on the serial id. I'll just
reword that.

> > +    <request name="set_cursor">
> > +      <description summary="set the tablet tool's surface">
> > +       Sets the surface of the cursor used for this tool on the given
> > +       tablet. The surface is only shown when this tool is in proximity of
> > +       this tablet. If the surface is NULL, the pointer image is hidden
> > +       completely.
> > +
> > +       The parameters hotspot_x and hotspot_y define the position of the
> > +       pointer surface relative to the pointer location. Its top-left corner
> > +       is always at (x, y) - (hotspot_x, hotspot_y), where (x, y) are the
> > +       coordinates of the pointer location, in surface local coordinates.
> 
> (Something something transforms on the cursor surface ...)
> 
> > +    <request name="destroy" type="destructor">
> > +      <description summary="destroy the tool object">
> > +       This destroys the client's resource for this tool object.
> > +
> > +       A client must not issue this request until it receives a
> > +       wp_tablet_tool.remove event.
> 
> Egads. What happens when a client no longer wants to know about tablet
> events (e.g. you have an embedded drawing program in a browser but
> then close the tab): it just has to ignore the stream of events
> forever? A bit hostile.

you can drop the tablet manager and re-bind. I think filtering specific
devices at the protocol level is a bit of a niche usecase and better handled
in the toolkit.
 
> > +    <enum name="type">
> > +      <description summary="a physical tool type">
> > +       Describes the physical type of a tool. The physical type of a tool
> > +       generally defines its base usage.
> > +
> > +       The mouse tool represents a mouse-shaped tool that is not a relative
> > +       device but bound to the tablet's surface, providing absolute
> > +       coordinates.
> > +
> > +       The lens tool is a mouse-shaped tool with an attached lens to
> > +       provide precision focus.
> > +      </description>
> > +      <entry name="pen" value="0x140" summary="Pen"/>
> > +      <entry name="eraser" value="0x141" summary="Eraser"/>
> > +      <entry name="brush" value="0x142" summary="Brush"/>
> > +      <entry name="pencil" value="0x143" summary="Pencil"/>
> > +      <entry name="airbrush" value="0x144" summary="Airbrush"/>
> > +      <entry name="finger" value="0x145" summary="Finger"/>
> > +      <entry name="mouse" value="0x146" summary="Mouse"/>
> > +      <entry name="lens" value="0x147" summary="Lens"/>
> > +    </enum>
> 
> *raises eyebrow* - magic numbers from Wacom protocol? (Fine if they are.)

linux/input.h, actually (BTN_TOOL_PEN, etc.), not that it really matters, we
could use 1-7 just as well :)

> > +    <event name="serial_id">
> > +      <description summary="unique serial number of the tool">
> > +       If the tool can be identified by a unique 64-bit serial number, this
> > +       event notifies the client of the serial number.
> > +
> > +       If multiple tablets are available in the same seat and the tool is
> > +       uniquely identifiable by the serial number, that tool may move
> > +       between tablets.
> > +
> > +       Otherwise, if the tool has no serial number and this event is
> > +       missing, the tool is tied to the tablet it first comes into
> > +       proximity with. Even if the physical tool is used on multiple
> > +       tablets, separate wp_tablet_tool objects will be created, one per
> > +       tablet.
> > +
> > +       This event is sent in the initial burst of events before the
> > +       wp_tablet_tool.done event.
> > +      </description>
> > +      <arg name="serial_id_msb" type="uint" summary="the unique serial number of the tool, most significant bits"/>
> > +      <arg name="serial_id_lsb" type="uint" summary="the unique serial number of the tool, least significant bits"/>
> > +    </event>
> 
> Would be really nice to not use 'serial' in the name, though using it
> in the description absolutely makes sense. Just want to avoid
> confusion with serial-as-place-in-event-stream-identifier ...

hwserial? it's referred to as serial number in most other instances, so I'm
out of ideas on how else to name it. I'm aware of the clash (hence
"serial_id") but unsure how to avoid it.

> 
> > +    <enum name="capability">
> > +      <description summary="capability flags for a tool">
> > +       Describes extra capabilities on a tablet.
> > +
> > +       Any tool must provide x and y values, extra axes are
> > +       device-specific.
> > +      </description>
> > +      <entry name="tilt" value="1" summary="Tilt axes"/>
> > +      <entry name="pressure" value="2" summary="Pressure axis"/>
> > +      <entry name="distance" value="3" summary="Distance axis"/>
> > +    </enum>
> > +
> > +    <event name="capability">
> > +      <description summary="tool capability notification">
> > +       This event notifies the client of any capabilities of this tool,
> > +       beyond the main set of x/y axes and tip up/down detection.
> > +
> > +       One event is sent for each extra capability available on this tool.
> > +
> > +       This event is sent in the initial burst of events before the
> > +       wp_tablet_tool.done event.
> > +      </description>
> > +      <arg name="capability" type="uint" summary="the capability"/>
> > +    </event>
> 
> Bitmask?

tbh, the way we send events, I don't see any specific benefit a bitmask
would give us. we could send a single capability event with a bitmask but
tbh it's less flexible and virtually the same work in the client.

> > +    <event name="down">
> > +      <description summary="tablet tool is making contact">
> > +       Sent whenever the tablet tool comes in contact with the surface of the
> > +       tablet. If the tablet tool moves out of a region while in contact with
> > +       the surface of the tablet, the client owning said region will receive a
> > +       wp_tablet::up event, followed by a wp_tablet::proximity_out event and a
> > +       wp_tablet::frame event.
> > +      </description>
> > +      <arg name="serial" type="uint"/>
> > +    </event>
> 
> Inconsistent use of protocol::event here, with protocol.event
> elsewhere in the docs.
> 
> >+    <event name="distance">
> >+      <description summary="distance change event">
> >+       Sent whenever the distance axis on a tool changes. The value of this
> >+       event is normalized to a value between 0 and 65535.
> >+      </description>
> >+      <arg name="distance" type="uint" summary="The current distance value"/>
> >+    </event>
> 
> Units?
> 
> > +    <enum name="button_state">
> > +      <description summary="physical button state">
> > +       Describes the physical state of a button which provoked the button event
> > +      </description>
> > +      <entry name="released" value="0" summary="button is not pressed"/>
> > +      <entry name="pressed" value="1" summary="button is pressed"/>
> > +    </enum>
> 
> Imperative is better here: 'button pressed', 'button released'. Also
> happens to match the doc in the button event itself. ;)
> 
> > +    <event name="button">
> > +      <description summary="button event">
> > +       Sent whenever a button on the stylus is pressed or released.
> > +      </description>
> > +
> > +      <arg name="serial" type="uint"/>
> > +      <arg name="button" type="uint" summary="The button whose state has changed"/>
> > +      <arg name="state" type="uint" summary="Whether the button was pressed or released"/>
> > +    </event>
> 
> > +    <event name="id">
> > +      <description summary="tablet device vid/pid">
> > +       This event is sent in the initial burst of events before the
> > +       wp_tablet.done event.
> > +      </description>
> > +      <arg name="vid" type="uint" summary="vendor id"/>
> > +      <arg name="pid" type="uint" summary="product id"/>
> > +    </event>
> 
> VID/PID space ... ?
> 
> > +    <enum name="type">
> > +      <description summary="tablet type">
> > +       Describes the type of tablet
> > +      </description>
> 
> Full stop
> 
> > +      <entry name="external" value="0" summary="The tablet is an external tablet, such as an Intuos"/>
> > +      <entry name="internal" value="1" summary="The tablet is a built-in tablet, usually in a laptop"/>
> > +      <entry name="display" value="2" summary="The tablet is a display tablet, such as a Cintiq"/>
> > +    </enum>
> > +
> > +    <event name="type">
> > +      <description summary="the tablet type">
> > +       This event is sent in the initial burst of events before the
> > +       wp_tablet.done event.
> > +      </description>
> > +      <arg name="type" type="uint" summary="the tablet type (internal, external, ...)"/>
> > +    </event>
> 
> Are these two not redundant?

fwiw, the type event/enum has been dropped since (see Carlos' comments elsewhere)
 
> > +    <event name="path">
> > +      <description summary="path to the device">
> > +       A system-specific device path that indicates which device is behind
> > +       this wp_tablet. This information may be used to gather additional
> > +       information about the device, e.g. through libwacom.
> > +
> > +       A device may have more than one device path, if so, multiple
> > +       wp_tablet.path events are sent. A device may be emulated and not
> > +       have a device path, in that case this event will not be sent.
> > +
> > +       The format of the path is unspecified, it may be a device node, a
> > +       sysfs path, or some other identifier. It is up to the client to
> > +       identify the string provided.
> > +
> > +       This event is sent in the initial burst of events before the
> > +       wp_tablet.done event.
> > +      </description>
> > +      <arg name="path" type="string" summary="path to local device"/>
> > +    </event>
> 
> Mm, device path isn't really a good idea with logind. Should just be
> sysfs path or nothing, I think.

I specifically left this open because we can't predict what it'll be (see
the third paragraph). A client will have to strcmp for /sys or /dev, but I
think that's still better than dictating one specific path type.

> Anyway, none of these are particularly fundamental to the protocol;
> the biggest change is the wp_tablet_seat thing, but given that's not a
> pattern we've previously used, it could go either way. So, with as
> many or few of these comments addressed as you feel makes sense:
> Reviewed-by: Daniel Stone <daniels at collabora.com>

thanks, much appreciated

Cheers,
   Peter