Re: [PATCH v3] HID: wacom: generic: add 5 tablet touch keys

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Jan 06 2017 or thereabouts, Peter Hutterer wrote:
> On Wed, Jan 04, 2017 at 10:27:15AM +0100, Benjamin Tissoires wrote:
> > On Jan 04 2017 or thereabouts, Peter Hutterer wrote:
> > > On Tue, Jan 03, 2017 at 02:33:36PM -0800, Ping Cheng wrote:
> > > > Hi Dmitry,
> > > > 
> > > > On Tue, Jan 3, 2017 at 1:30 AM, Benjamin Tissoires
> > > > <benjamin.tissoires@xxxxxxxxxx> wrote:
> > > > > On Dec 22 2016 or thereabouts, Dmitry Torokhov wrote:
> > > > >> On Mon, Dec 19, 2016 at 11:30:13AM +0100, Jiri Kosina wrote:
> > > > >> > On Fri, 16 Dec 2016, Ping Cheng wrote:
> > > > >> >
> > > > >> > > Wacom Cintiq Pro [1] is a series of display tablets. They support
> > > > >> > > 5 touch keys on the tablet. Those keys represent specific functions.
> > > > >> > > They turn display off; bring up OSD; bring up On Screen Keyboard;
> > > > >> > > bring up desktop control panel; and turn touch on/off.
> > > > >> > >
> > > > >> > > The most left touch key, that turns display on/off, is controlled by
> > > > >> > > firmware. When the display is on, the mode is set. Otherwise, the
> > > > >> > > mode is off. So, it works like a switch. That's why we introduced a
> > > > >> > > new switch: SW_INDIRECT. The switch defauts to INDIRECT instead of DIRECT
> > > > >> > > was a request from useland, more specifically Gnome, developers.
> > > > >> > >
> > > > >> > > Other four touch keys are true software keys. We use the existing
> > > > >> > > KEY_BUTTONCONFIG and KEY_CONTROLPANEL for OSD and control panel. But,
> > > > >> > > we have to request two new keys: KEY_ONSCREEN_KEYBOARD and KEY_MUTE_DEVICE
> > > > >> > > since none of the existing keys support those two actions.
> > > > >> > >
> > > > >> > > [1] http://www.wacom.com/en-us/products/pen-displays/wacom-cintiq-pro
> > > > >> > >
> > > > >> > > Signed-off-by: Ping Cheng <ping.cheng@xxxxxxxxx>
> > > > >> > > ---
> > > > >> > > v3: Since no one has followed up with v2, let's add some more comments for
> > > > >> > > SW_INDIRECT so we keep the offlist decision public ;).
> > > > >> >
> > > > >> > [ ... snip ... ]
> > > > >> >
> > > > >> > > diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
> > > > >> > > index d6d071f..32ef894 100644
> > > > >> > > --- a/include/uapi/linux/input-event-codes.h
> > > > >> > > +++ b/include/uapi/linux/input-event-codes.h
> > > > >> > > @@ -641,6 +641,9 @@
> > > > >> > >   * e.g. teletext or data broadcast application (MHEG, MHP, HbbTV, etc.)
> > > > >> > >   */
> > > > >> > >  #define KEY_DATA                 0x275
> > > > >> > > +/* same as SW_MUTE_DEVICE but triggered by a key */
> > > > >> > > +#define KEY_MUTE_DEVICE          0x278
> > > > >> > > +#define KEY_ONSCREEN_KEYBOARD            0x279
> > > > >> > >
> > > > >> > >  #define BTN_TRIGGER_HAPPY                0x2c0
> > > > >> > >  #define BTN_TRIGGER_HAPPY1               0x2c0
> > > > >> > > @@ -781,7 +784,8 @@
> > > > >> > >  #define SW_LINEIN_INSERT 0x0d  /* set = inserted */
> > > > >> > >  #define SW_MUTE_DEVICE           0x0e  /* set = device disabled */
> > > > >> > >  #define SW_PEN_INSERTED          0x0f  /* set = pen inserted */
> > > > >> > > -#define SW_MAX                   0x0f
> > > > >> > > +#define SW_INDIRECT              0x10  /* set = not a direct input device */
> > > > >> > > +#define SW_MAX                   0x1f
> > > > >> >
> > > > >> > I'd like to have explicit Ack from Dmitry on these ... Dmitry?
> > > > >>
> > > > >> Sorry for the delay, but I think adding SW_INDIRECT is actually wrong.
> > > > >>
> > > > >> What Wacom folks seem to need is way to re-classfiy the device (i.e.
> > > > >> update its properties) and let userspace know somehow. We can't keep
> > > > >> adding SW_INDIRECT and SW_NOTPOINTER and SW_NOTBUTTONPAD and so forth.
> > > > >> We however have EV_SYN/SYN_CONFIG that we may use to let userspace know
> > > > >> that device configuration changed and that userspace needs to
> > > > >> interrogate the device again. We can emit this code both when we change
> > > > >> properties as well as when we change ABS limits and changing keymaps and
> > > > >> so forth.
> > > > 
> > > > By "to interrogate the device again",  do you mean once we report
> > > > EV_SYN/SYN_CONFIG, userspace needs to reinitialize the device?
> > > > 
> > > > If not, how do we tell userspace which feature/info has been changed?
> > > > 
> > > > >>
> > > > >> Benjamin, Peter, any opinion on the above?
> > > > >
> > > > > Oooh, so that's the purpose of this event :) (the documentation says
> > > > > "TBD"). I am fine with that. We would need to adapt the documentation in
> > > > > Documentation/input/event-codes.txt and make sure the handlers are
> > > > > properly adapted too (*maybe* add a SYN_DROP event to empty the queue of
> > > > > the events in userspace).
> > > > 
> > > > Can you update the "TBD" in Documentation/input/event-codes.txt so we
> > > > have an agreed description to follow?
> > > 
> > > Here's a first attempt:
> > > 
> > >  * SYN_CONFIG:
> > >    - Used to indicate that the device's static description has changed. This
> > 
> > Should we indicate the obvious fact that this is only after the device has
> > been registered and presented to userspace? I can imagine drivers adding
> > those notifications after each declaration while preparing the input
> > node :)
> 
> added, see below
> 
> > 
> > >      can happen when
> > >      - at least one event type or code has been added or removed, or
> > >      - at least one device property has been added or removed, or
> > >      - at least one absolute axis has changed properties, or
> > >      - the keycode has changed, or
> > >      - the name, id, phys or unique identifier of the device has changed,
> > >      A client should re-query the device once a SYN_CONFIG has been received
> > >      as if the device was newly added. SYN_CONFIG does not indicate which
> > >      information changed, a client is required to re-read the full 
> > >      state. A SYN_CONFIG may be triggered even by types/codes/... that the
> > >      client is not aware of, i.e. the state before and after may look
> > >      identical to any given client.
> > >      Dynamic values such as key state, switch state, absolute axis value,
> > >      force feedback, etc. do not trigger a SYN_CONFIG. A SYN_CONFIG may
> > >      trigger a SYN_DROPPED to terminate ongoing event sequences.
> > 
> > I think we should enforce this last sentence a little bit more:
> > "A SYN_CONFIG is basically a reset of the device so it should be
> > considered as a SYN_DROPPED from the client perspective. For backward
> > compatibility with clients not supporting SYN_CONFIG, a SYN_DROPPED
> > event is sent right before SYN_CONFIG."
> 
> Are we planning to force a SYN_DROPPED every time? Even when the event queue
> is empty? I think it wouldn't do much other than keep the client busy. 
> 
> Come to think of it, there's a chance that this *reduces* backwards
> compatibility. Until a SYN_DROPPED occurs, a client has no reason to ever
> check bits again. But re-fetching all states during SYN_DROPPED is normal,
> so if one axis disappears, the client may now get confused.

Ouch, very much ouch.

Maybe SYN_CONFIG should be limited to only props, axis ranges and new
axis?

> 
> > Also, maybe we should add a warning about the udev properties:
> > "If a SYN_CONFIG allows to change the static description of a device, it
> > should be used with care. Some userspace pieces (udev) rely on the
> > device creation to tag it properly. Completely changing the device type
> > from a mouse to a touchpad is better handled through the destruction and
> > then creation of a new input node than relying on SYN_CONFIG"
> > 
> > (please fix typos/grammar/jibberish)
> > 
> > > 
> > > We should also add a blurb to the EVIOC*MASK ioctls about SYN_CONFIG.
> > 
> > Saying that it can't be masked and that it will invalidate the current
> > masks? Or this is the responsibility of the client?
> 
> I would just leave the masks as-is, less work and more predictability. This
> means that a client has to re-apply them on SYN_CONFIG to avoid events from
> new axes (if any) but that seems like the most straightforward approach. We
> already allow masking out axes a device doesn't have, so invalidating
> doesn't really give us any benefits. 
> 
> Ok, let's have a v2 draft:
> 
> * SYN_CONFIG:
>   - Used to indicate that the device's static description has changed, after
>     the device has been registered and presented to userspace.
>     This can happen when
>     - at least one event type or code has been added or removed, or
>     - at least one device property has been added or removed, or
>     - at least one absolute axis has changed properties, or
>     - the keycode has changed, or
>     - the name, id, phys or unique identifier of the device has changed,
>     Do not send this event during the initial device initialization process,
>     this event is exclusively for changes at runtime.
> 
>     A client should re-query the device once a SYN_CONFIG has been received
>     as if the device was newly added. SYN_CONFIG does not indicate which
>     information changed, a client is required to re-read the full 
>     state. A SYN_CONFIG may be triggered even by types/codes/... that the
>     client is not aware of, i.e. the state before and after may look
>     identical to any given client.
> 
>     Dynamic values such as key state, switch state, absolute axis value,
>     force feedback, etc. do not trigger a SYN_CONFIG. A SYN_CONFIG may
>     trigger a SYN_DROPPED to terminate ongoing event sequences.
> 
>     SYN_CONFIG should be used with extreme care. There is no guarantee that
>     a SYN_CONFIG event is handled correctly by a client, or even noticed.
>     For example, udev relies on the original device description only to
>     assign a device type. If a SYN_CONFIG were to change the device
>     sufficiently to warrant a new device type tag, this may never be
>     detected. Changing a device type (e.g. from mouse to touchpad) is better
>     handled through the destruction and recreation of the input node.

I like it. Besides the legacy breakage we are talking about above.

Cheers,
Benjamin

> 
> Cheers,
>    Peter
>  
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux