Re: [PATCH libinput 0/6] Add dynamic devices to the path backend
On Fri, Feb 07, 2014 at 08:09:21AM +1000, Peter Hutterer wrote: On Thu, Feb 06, 2014 at 10:23:57PM +0100, Jonas Ådahl wrote: On Thu, Feb 06, 2014 at 02:13:04PM +1000, Peter Hutterer wrote: This patchset revamps the path backend to allow for more than one path-based device per context. I thought the initial approach of having one context per device is sufficient but there are a few use-cases that can really only be solved by having libinput control all devices. A common example is disabling the touchpad while typing, or making the trackstick buttons on the Lenovo T440s useful. So for my little libinput-based xorg driver [1] I need to have a shared context between the various devices added. The change looks bigger than it is, it largely just replicates what the udev-seat backend already does anyway. Most notable there are a few API changes: - libinput_create_from_path() has been removed, a caller should now use libinput_path_create_context(), then libinput_path_add_device() Why not just libinput_path_create()? I wanted to make it explicit that all this call does is create the context, as opposed to e.g. udev_create_... which adds devices too. I thought libinput_path_create() was clear enough as one didn't pass any device path. Anyway, libinput_path_create_context() is fine as well. I found this to be nicer in the caller code than having libinput_path_create_from_device() and then adding devices. - more devices can be added or removed with libinput_path_add_device and libinput_path_remove_device - to ensure proper namespacing, libinput_create_from_udev is now libinput_udev_create_for_seat() These API changes looks good to me, but should maybe suspend and resume behaviour be documented? Is it required to re-add devices after having resumed? it's not required, the behaviour is that suspend/resume removes and re-adds all devices, or fails if a device cannot be added. exactly the same behavour as the udev seat. I'll expand on the suspend/resume documentation in a separate patch. So far this looks flexible enough for the xorg drivers which have different use-cases than in weston, specifically each device can be disabled and enabled individually. I just call remove/add device when that happens. What's not so nice is a shortcut in libinput_path_add_device(). Instead of just succeeding and letting the caller handle the DEVICE_ADDED event, it returns the struct libinput_device directly. At least within the xorg driver context I found it too hard to work with the events (long description on request) but the short summary is that I'd need to cache any events before DEVICE_ADDED and use timers to re-trigger the read once I have the device, aside from the problem of not being able to figure out which device is which based on the events only (we don't expose the devnode to the caller). So I guess you can't just add and then flush and process the queue right there, as the DEVICE_ADDED event will already have been queued when calling libinput_path_add_device()? Not without doing some work to the server, the enable/disable bits are in a quirky order and called from different places than the actual event processing. So without auditing the call paths I can't guarantee that the server is in the correct state for handling events when you get the first couple of events. Returning the device pointer was the less-intrusive but still somewhat-sensible approach :) Not going to argue with that :) It would be a bit awkward anyway to deal with events that way. We could also simply expose the devnode to the caller as well, caching and matching later is not very nice. IMO we should do this anyway, otherwise we'd require the caller to use udev and subsystem guessing* to match it up themselves. At least a devnode is non-ambiguous. Cheers, Peter * probably not much of an issue since we don't deal with serial devices, so we can assume subsystem is always input ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput 0/6] Add dynamic devices to the path backend
On Thu, Feb 06, 2014 at 02:13:04PM +1000, Peter Hutterer wrote: This patchset revamps the path backend to allow for more than one path-based device per context. I thought the initial approach of having one context per device is sufficient but there are a few use-cases that can really only be solved by having libinput control all devices. A common example is disabling the touchpad while typing, or making the trackstick buttons on the Lenovo T440s useful. So for my little libinput-based xorg driver [1] I need to have a shared context between the various devices added. The change looks bigger than it is, it largely just replicates what the udev-seat backend already does anyway. Most notable there are a few API changes: - libinput_create_from_path() has been removed, a caller should now use libinput_path_create_context(), then libinput_path_add_device() Why not just libinput_path_create()? I found this to be nicer in the caller code than having libinput_path_create_from_device() and then adding devices. - more devices can be added or removed with libinput_path_add_device and libinput_path_remove_device - to ensure proper namespacing, libinput_create_from_udev is now libinput_udev_create_for_seat() These API changes looks good to me, but should maybe suspend and resume behaviour be documented? Is it required to re-add devices after having resumed? So far this looks flexible enough for the xorg drivers which have different use-cases than in weston, specifically each device can be disabled and enabled individually. I just call remove/add device when that happens. What's not so nice is a shortcut in libinput_path_add_device(). Instead of just succeeding and letting the caller handle the DEVICE_ADDED event, it returns the struct libinput_device directly. At least within the xorg driver context I found it too hard to work with the events (long description on request) but the short summary is that I'd need to cache any events before DEVICE_ADDED and use timers to re-trigger the read once I have the device, aside from the problem of not being able to figure out which device is which based on the events only (we don't expose the devnode to the caller). So I guess you can't just add and then flush and process the queue right there, as the DEVICE_ADDED event will already have been queued when calling libinput_path_add_device()? We could also simply expose the devnode to the caller as well, caching and matching later is not very nice. On the positive side, the xorg driver seems to be working quite nicely already, though it is rather featureless. Nice :) Cheers, Peter [1] https://github.com/whot/xf86-input-libinput ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput 0/6] Add dynamic devices to the path backend
On Thu, Feb 06, 2014 at 10:23:57PM +0100, Jonas Ådahl wrote: On Thu, Feb 06, 2014 at 02:13:04PM +1000, Peter Hutterer wrote: This patchset revamps the path backend to allow for more than one path-based device per context. I thought the initial approach of having one context per device is sufficient but there are a few use-cases that can really only be solved by having libinput control all devices. A common example is disabling the touchpad while typing, or making the trackstick buttons on the Lenovo T440s useful. So for my little libinput-based xorg driver [1] I need to have a shared context between the various devices added. The change looks bigger than it is, it largely just replicates what the udev-seat backend already does anyway. Most notable there are a few API changes: - libinput_create_from_path() has been removed, a caller should now use libinput_path_create_context(), then libinput_path_add_device() Why not just libinput_path_create()? I wanted to make it explicit that all this call does is create the context, as opposed to e.g. udev_create_... which adds devices too. I found this to be nicer in the caller code than having libinput_path_create_from_device() and then adding devices. - more devices can be added or removed with libinput_path_add_device and libinput_path_remove_device - to ensure proper namespacing, libinput_create_from_udev is now libinput_udev_create_for_seat() These API changes looks good to me, but should maybe suspend and resume behaviour be documented? Is it required to re-add devices after having resumed? it's not required, the behaviour is that suspend/resume removes and re-adds all devices, or fails if a device cannot be added. exactly the same behavour as the udev seat. I'll expand on the suspend/resume documentation in a separate patch. So far this looks flexible enough for the xorg drivers which have different use-cases than in weston, specifically each device can be disabled and enabled individually. I just call remove/add device when that happens. What's not so nice is a shortcut in libinput_path_add_device(). Instead of just succeeding and letting the caller handle the DEVICE_ADDED event, it returns the struct libinput_device directly. At least within the xorg driver context I found it too hard to work with the events (long description on request) but the short summary is that I'd need to cache any events before DEVICE_ADDED and use timers to re-trigger the read once I have the device, aside from the problem of not being able to figure out which device is which based on the events only (we don't expose the devnode to the caller). So I guess you can't just add and then flush and process the queue right there, as the DEVICE_ADDED event will already have been queued when calling libinput_path_add_device()? Not without doing some work to the server, the enable/disable bits are in a quirky order and called from different places than the actual event processing. So without auditing the call paths I can't guarantee that the server is in the correct state for handling events when you get the first couple of events. Returning the device pointer was the less-intrusive but still somewhat-sensible approach :) We could also simply expose the devnode to the caller as well, caching and matching later is not very nice. IMO we should do this anyway, otherwise we'd require the caller to use udev and subsystem guessing* to match it up themselves. At least a devnode is non-ambiguous. Cheers, Peter * probably not much of an issue since we don't deal with serial devices, so we can assume subsystem is always input ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
