[RFC libinput 1/2] Add a "buttonset" interface for button-only devices

[Peter Hutterer]

On Fri, Feb 13, 2015 at 09:34:52AM +0100, Hans de Goede wrote:
> Hi Peter,
> 
> On 13-02-15 08:42, Peter Hutterer wrote:
> >On 13/02/2015 14:32 , Peter Hutterer wrote:
> >>On Thu, Feb 12, 2015 at 12:37:12PM +0100, Hans de Goede wrote:
> >>>Hi,
> >>>
> >>>On 12-02-15 08:33, Peter Hutterer wrote:
> >>>>A generic interface for devices that provide buttons and axes, but don't
> >>>>control the pointer. This caters for the Pad part of Wacom graphics tablets
> >>>>but could eventually also deal with remote controls, 3D mice and other
> >>>>devices.
> >>>>
> >>>>This patch adds a new interface "buttonset" with a new capability and two
> >>>>events AXIS and BUTTON. The interface is largely modelled after the
> >>>>tablet interface, but with different axes and a few different behaviors.
> >>>>The button events do the obvious, axis events provide 5 hooks:
> >>>>
> >>>>libinput_event_buttonset_get_axis_value() -> axis value in physical dimensions
> >>>>libinput_event_buttonset_get_axis_value_transformed()
> >>>>libinput_event_buttonset_get_axis_delta() -> axis delta in physical dimensions
> >>>>libinput_event_buttonset_get_axis_delta_transformed()
> >>>>libinput_event_buttonset_get_axis_delta_discrete() -> axis delta in clicks
> >>>>
> >>>>The latter is similar after the scroll click counts.
> >>>>
> >>>>Currently implemented are two ring and two strip axes. The axis use depends a
> >>>>lot on the device and is hard to predict. e.g. strips have apparently been
> >>>>used for physical positioning (like a sliding actuator) whereas the wheel
> >>>>usually only cares about deltas. The above set should cater for the common
> >>>>use-cases.
> >>>>
> >>>>Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> >>>
> >>><snip>
> >>>
> >>>>diff --git a/src/libinput.h b/src/libinput.h
> >>>>index a8146c0..d777730 100644
> >>>>--- a/src/libinput.h
> >>>>+++ b/src/libinput.h
> >>>>@@ -55,7 +55,8 @@ enum libinput_device_capability {
> >>>>  	LIBINPUT_DEVICE_CAP_KEYBOARD = 0,
> >>>>  	LIBINPUT_DEVICE_CAP_POINTER = 1,
> >>>>  	LIBINPUT_DEVICE_CAP_TOUCH = 2,
> >>>>-	LIBINPUT_DEVICE_CAP_TABLET = 3
> >>>>+	LIBINPUT_DEVICE_CAP_TABLET = 3,
> >>>>+	LIBINPUT_DEVICE_CAP_BUTTONSET = 4
> >>>>  };
> >>>>
> >>>>  /**
> >>>>@@ -148,6 +149,34 @@ enum libinput_tablet_axis {
> >>>>  /**
> >>>>   * @ingroup device
> >>>>   *
> >>>>+ * Available axis types for a buttonset device. It must have the @ref
> >>>>+ * LIBINPUT_DEVICE_CAP_BUTTONSET capability.
> >>>>+ */
> >>>>+enum libinput_buttonset_axis {
> >>>>+	LIBINPUT_BUTTONSET_AXIS_RING = 1,
> >>>>+	LIBINPUT_BUTTONSET_AXIS_RING2 = 2,
> >>>>+	LIBINPUT_BUTTONSET_AXIS_STRIP = 3,
> >>>>+	LIBINPUT_BUTTONSET_AXIS_STRIP2 = 4,
> >>>>+};
> >>>
> >>>Hmm, I thought that the idea was in the end to be able to use the
> >>>button set interface with any random input device, with any random
> >>>amount of inputs? Limiting things to just 2 of each type sounds wrong
> >>>in that light, I think it would be better to just number axis 0 - #
> >>>without looking at their type, and add a separate type field, which
> >>>can be: RING, STRIP or UNKNOWN. Axis should probably also have flags
> >>>to indicate if they generate abs events, deltas or both.
> >>
> >>hmm, good point, I honestly didn't think of that. I did a quick edit of the
> >>header file to see what the API would look like, see the diff below.
> >>The changes are axis from the enum to uint32_t and a few extra calls:
> >>     libinput_event_buttonset_has_absolute_value(event, axisnumber)
> >>     libinput_device_buttonset_get_num_axes(device)
> >>     libinput_device_buttonset_get_axis_type(device, axisnumber)
> >>
> >>The latter two take a device. I'm not sure I like the API more though, on
> >>the caller side it becomes a lot less expressive: before every call into the
> >>API would carry enough information to know what you're getting, now it's
> >>just an arbitrary number. It's more flexible this way though, that's for
> >>sure.
> >>
> >>As for an UNKNOWN axis: I'd rather avoid this altogether. if we don't know
> >>what an axis is we should ignore it since we can't normalize the events or
> >>even guarantee that any event sequence is correct.
> >>
> >>Thinking aloud: and that "no unknown axes" approach is also why I still like
> >>the original diff: it guarantees that any axis we export is known to
> >>libinput. We don't use the axis type as mask (in the API) so we have a _lot_
> >>of axes we can add eventually. I don't think having, eventually,
> >>LIBINPUT_BUTTONSET_AXIS_STRIP14 is the worst of
> >>all things. We could group them if we really wanted to as well, similar to
> >>the events, i.e.
> >>  LIBINPUT_BUTTONSET_AXIS_RING = 1
> >>  LIBINPUT_BUTTONSET_AXIS_RING2,
> >>  ... empty space
> >>  LIBINPUT_BUTTONSET_AXIS_STRIP = 50,
> >>  ...
> >>
> >>So that some procedural looping through a specific axis type is possible. If
> >>we ever have a device with more than 50 axis of the same type, libinput is
> >>probably not the best piece of code to handle it anyway :)
> >
> >After a bit more thinking about this, I started liking this idea more
> >and more. The only change to the original patch would be spacing out the
> >enum, with calls to the API becoming both well defined and free-form and
> >forward-compatible.
> >
> >If we know there are devices that use a specific axis we can add them to
> >the enum (RING2 for example), otherwise with the proper spacing all
> >other axes are still available:
> >
> >To get the first ring value:
> >libinput_event_buttonset_get_axis_value(
> >          event,
> >          LIBINPUT_BUTTONSET_AXIS_RING);
> >
> >To get the 14th ring value:
> >
> >libinput_event_buttonset_get_axis_value(
> >          event,
> >          LIBINPUT_BUTTONSET_AXIS_RING + 13);
> >
> >If we make the type spacing even, you can query all *potential* axes of
> >one type with:
> >   for (i = AXIS_RING; i < AXIS_RING + AXIS_TYPE_COUNT; i++)
> >        ....
> >
> >I think that's a lot better than having just numbered axes, it adds the
> >a lot of semantics that are otherwise difficult to get.
> 
> Ok, this (spacing the enums out) was actually my first idea too, but I wasn't
> sure it would be generic enough, thinking more about it I think it should work
> fine. Would be good to add a
> 
> int
> libinput_device_get_buttonset_axis_count(struct libinput_device *device, libinput_buttonset_axis axis);
> 
> Or some such so that API users who want to know how many (if any) of a type
> of axis there are in a device with buttonset capability can do so with needing to
> do a loop and call has_axis all the time. Our internal API may still use the
> loop for all I care, I just think we need something more API user friendly :)

ok, I've played around with the ideas in this thread and discussed it with
Benjamin this morning. Short summary: I think we should go with the original
patch, with an optional extension for numbered axes later.

I tried the enum spacing first, providing an API where  we have
   LIBINPUT_BUTTONSET_AXIS_TYPE_COUNT = 512
   LIBINPUT_BUTTONSET_AXIS_RING = 512
   LIBINPUT_BUTTONSET_AXIS_STRIP = 1024

so the caller can use code like
   libinput_event_buttonset_get_axis_value(event,
                                   LIBINPUT_BUTTONSET_AXIS_RING + 2);
to get the third axis. switching event-debug showed the issues with this
approach: to effectively go through the axes, the client needs two loops,
one for the type one for the number and mask the enum into a type and a
number component, something that's prone to bugs.

Next attempt was to split value and axis number explicitly in the API:
   libinput_event_buttonset_get_axis_value(event,
                                           LIBINPUT_BUTTONSET_AXIS_RING,
                                           2);
Better than the above as it's less error-prone.
It still doesn't remove the above issue though, nested loops everywhere to
access the type and the number.
event-debug is a special case in that it doesn't care about the content and
cares more about listing/printing everything. However I suspect that any
generic toolkit will require the same.

[Internally both changes are a bit of a nightmare as both would require some
rewriting, but that's solveable]

So the question is now: what does this gain us? better handling of truly
generic devices with random axes. Which leads into the next question: what
are random axes?
The above approach is over-engineered because there's a group of axes
that only exists once. I don't think there's a device with two X axes on the
same device for example. Likewise, there are axes that have more use being
semantically labelled than just numbered. The ring axes are a good example,
the current RING and RING2 naming is bad, we should name it RING_LEFT,
RING_RIGHT instead.

Other than say a mix table we'll likely find semantic naming for a majority
of the device. For the other devices, we can use a hybrid approach:
leave the current API to use enums but in the future, when we require more
flexible axes or devices with multiple identical axes we can add calls like:
   libinput_device_buttonset_has_axis_by_offset(event, axis_number);
   libinput_device_buttonset_get_axis_type_by_offset(event, axis_number);
   libinput_event_buttonset_get_axis_value_by_offset(event,
                                                     axis_number);

etc. this would require a duplication of the buttonset API with a by_offset
prefix but is otherwise straightforward and leaves the most expressive API
as the default one.

We also discussed making an axis an object (struct libinput_buttonset_axis)
and providing a get_axis(enum) and a get_axis_by_offset(number) call. That
object could've then be used in the API. I don't think it's a good idea to
do this, it adds complexity (ref, unref, userdata) and effectively reduces the
API to magic numbers/variable names again.

so long story short, I think we should go with the enum after all and leave
numbered/offset axes for the future when we need it.

> And I do still believe we need an UNKNOWN axis, at XDC we were talking about these
> button boxes people could buy to use with midi-apps, which were just a bunch
> of dials, I don't want to do a database of those, so for such a device we
> should just end up saying this is a buttonbox with UNKNOWN axis, which when
> absolute must be used with get_transformed only, and the deltas are in an
> unknown unit, but we can cross that bridge when we get there.

once the axis is labelled once, it effectively becomes ABI. if we label
something as unknown and then fix it in a later version of libinput we will
break things. so we should hide anything we don't know enough about.
Nothing stops us from adding a LINEAR_ACTUATOR, MIDI_DIAL, etc. though.

Cheers,
   Peter