[PATCH libinput v3 5/5] libinput: add orientation and size of touch point and pressure to the API

Peter Hutterer peter.hutterer at who-t.net
Wed Jun 24 23:03:50 PDT 2015


On Mon, Jun 22, 2015 at 12:48:08PM +0200, Andreas Pokorny wrote:
> This change adds four new properties to touch events.
> major: diameter of the touch ellipse along the major axis
> minor: diameter perpendicular to major axis
> pressure: a pressure value mapped into the range [0,1]
> orientation: the angle between major and the x axis
> 
> Those values are optionally supported by multi touch drivers, hence
> default values are provided if the information is missing.
> 
> Signed-off-by: Andreas Pokorny <andreas.pokorny at canonical.com>
> ---

almost there, thanks. mostly general nitpicking, the direction this is
heading to certainly looks good.

>  src/evdev.c            | 243 +++++++++++++++++++++++++++++++++++++++++++------
>  src/evdev.h            |  37 ++++++++
>  src/libinput-private.h |  12 ++-
>  src/libinput.c         | 128 +++++++++++++++++++++++++-
>  src/libinput.h         | 194 +++++++++++++++++++++++++++++++++++++++
>  src/libinput.sym       |  10 ++
>  test/touch.c           |  94 +++++++++++++++++++
>  7 files changed, 684 insertions(+), 34 deletions(-)
> 
> diff --git a/src/evdev.c b/src/evdev.c
> index 7e1e5c8..6f88897 100644
> --- a/src/evdev.c
> +++ b/src/evdev.c
> @@ -44,6 +44,11 @@
>  
>  #define DEFAULT_WHEEL_CLICK_ANGLE 15
>  #define DEFAULT_MIDDLE_BUTTON_SCROLL_TIMEOUT 200
> +#define DEFAULT_TOUCH_PRESSURE 1
> +#define DEFAULT_TOUCH_ORIENTATION 0
> +#define DEFAULT_TOUCH_MAJOR 1
> +#define DEFAULT_TOUCH_MINOR 1
> +#define TO_RADIANS M_PI / 180.0

can we make this a fake function?
    #define deg2rad(angle_) (angle_ * M_PI/180.0)
avoids any potential issues with precedence.
if you want to be fancy you can make it an inline with a range check for
[0,360[.

>  
>  enum evdev_key_type {
>  	EVDEV_KEY_TYPE_NONE,
> @@ -244,6 +249,119 @@ evdev_device_transform_y(struct evdev_device *device,
>  	return scale_axis(device->abs.absinfo_y, y, height);
>  }
>  
> +double
> +evdev_device_transform_major(struct evdev_device *device,
> +			     int32_t major,
> +			     int32_t orientation,
> +			     uint32_t width,
> +			     uint32_t height)
> +{
> +	double scale = 1.0;
> +	double x_maximum = device->abs.absinfo_x->maximum;
> +	double x_minimum = device->abs.absinfo_x->minimum;
> +	double y_maximum = device->abs.absinfo_y->maximum;
> +	double y_minimum = device->abs.absinfo_y->minimum;
> +
> +	if (device->abs.absinfo_x->resolution ==
> +	    device->abs.absinfo_y->resolution) {

now I'm just nitpicking, but this is stuff that you can put into tmp.
variables to make the code easier to read. it'll be optimised out anyway, so
having xres/yres float doesn't hurt.

same for adding a w/h to avoid re-calculating the width in multiple
branches.
(see also the patch I've just sent to the list to provide abs.dimensions)


> +		scale = width / (x_maximum - x_minimum + 1);
> +	} else {
> +		double x_scale = (double)width / (x_maximum - x_minimum + 1);
> +		double y_scale = (double)height / (y_maximum - y_minimum + 1);
> +
> +		if (device->abs.absinfo_orientation == NULL) {
> +			scale = (x_scale + y_scale) / 2.0;
> +		}
> +		else {

should be 
    } else {


> +			double angle =
> +				evdev_device_transform_orientation(device,
> +								   orientation);
> +			scale = y_scale*sin(TO_RADIANS*angle) +
> +				x_scale*cos(TO_RADIANS*angle);
> +		}
> +	}
> +
> +	return major * scale;
> +}
> +
> +double
> +evdev_device_transform_minor(struct evdev_device *device,
> +			     int32_t minor,
> +			     int32_t orientation,
> +			     uint32_t width,
> +			     uint32_t height)
> +{
> +	double scale = 1.0;
> +	double x_maximum = device->abs.absinfo_x->maximum;
> +	double x_minimum = device->abs.absinfo_x->minimum;
> +	double y_maximum = device->abs.absinfo_y->maximum;
> +	double y_minimum = device->abs.absinfo_y->minimum;
> +
> +	if (device->abs.absinfo_x->resolution ==
> +	    device->abs.absinfo_y->resolution) {
> +		scale = width / (x_maximum - x_minimum + 1);
> +	} else {
> +		double x_scale = (double)width / (x_maximum - x_minimum + 1);
> +		double y_scale = (double)height / (y_maximum - y_minimum + 1);
> +
> +		if (device->abs.absinfo_orientation == NULL) {
> +			scale = (x_scale + y_scale) / 2.0;
> +		}
> +		else {
> +			double angle =
> +				evdev_device_transform_orientation(device,
> +								   orientation);
> +			scale = y_scale*cos(TO_RADIANS*angle) +
> +				x_scale*sin(TO_RADIANS*angle);
> +		}
> +	}
> +
> +	return minor * scale;
> +}

is this almost the same function or am I missing something obvious? can we
reduce the duplication, the calculation of scale can be shared here, right?

> +
> +double
> +evdev_device_transform_orientation(struct evdev_device *device,
> +				   int32_t orientation)
> +{
> +	const struct input_absinfo *orientation_info =
> +	    device->abs.absinfo_orientation;
> +
> +	/* ABS_MT_ORIENTATION is defined as a clockwise rotation - zero
> +	 * (instead of minimum) is mapped to the y-axis, and maximum is
> +	 * mapped to the x-axis.
> +	 * The following remaps x-axis to zero and y-axis to 90.0. */
> +	if (orientation_info) {
> +		int32_t max_orientation = orientation_info->maximum;
> +		int32_t min_orientation = orientation_info->minimum;
> +
> +		if (max_orientation == 1 && min_orientation == 0)
> +			return orientation == 0 ? 90.0: 0.0;
> +		else
> +			return 90.0 * (max_orientation - (double)orientation) /
> +				max_orientation;
> +	}
> +	else {
> +		return DEFAULT_TOUCH_ORIENTATION;
> +	}
> +}

sorry, mustn't have been clear in one of the previous email - we don't need
to remap if the kernel defines it this way. define touch orientation as
degrees CW off the y axis and we can take it straight from the kernel (minus
mapping to degrees).


> +
> +double
> +evdev_device_transform_pressure(struct evdev_device *device,
> +				int32_t pressure)
> +{
> +	const struct input_absinfo *pressure_info = device->abs.absinfo_pressure;
> +
> +	if (pressure_info) {
> +		double max_pressure = pressure_info->maximum;
> +		double min_pressure = pressure_info->minimum;
> +		return (double)(pressure - min_pressure) /
> +			(max_pressure - min_pressure);

you can use scale_axis() here
is it worth creating a fake absinfo with min 0/max 1 so we can skip the
branch here? maybe not if this is the only place we care about

> +	}
> +	else {
> +		return DEFAULT_TOUCH_PRESSURE;
> +	}
> +}
> +
>  static inline void
>  normalize_delta(struct evdev_device *device,
>  		const struct device_coords *delta,
> @@ -263,8 +381,10 @@ evdev_flush_pending_event(struct evdev_device *device, uint64_t time)
>  	struct libinput_seat *seat = base->seat;
>  	struct normalized_coords accel, unaccel;
>  	struct device_coords point;
> +	struct mt_slot *slot_data;
>  
>  	slot = device->mt.slot;
> +	slot_data = &device->mt.slots[slot];
>  
>  	switch (device->pending_event) {
>  	case EVDEV_NONE:
> @@ -299,7 +419,7 @@ evdev_flush_pending_event(struct evdev_device *device, uint64_t time)
>  		if (!(device->seat_caps & EVDEV_DEVICE_TOUCH))
>  			break;
>  
> -		if (device->mt.slots[slot].seat_slot != -1) {
> +		if (slot_data->seat_slot != -1) {
>  			log_bug_kernel(libinput,
>  				       "%s: Driver sent multiple touch down for the "
>  				       "same slot",
> @@ -308,38 +428,53 @@ evdev_flush_pending_event(struct evdev_device *device, uint64_t time)
>  		}
>  
>  		seat_slot = ffs(~seat->slot_map) - 1;
> -		device->mt.slots[slot].seat_slot = seat_slot;
> +		slot_data->seat_slot = seat_slot;
>  
>  		if (seat_slot == -1)
>  			break;
>  
>  		seat->slot_map |= 1 << seat_slot;
> -		point = device->mt.slots[slot].point;
> +		point = slot_data->point;
>  		transform_absolute(device, &point);
>  
> -		touch_notify_touch_down(base, time, slot, seat_slot,
> -					&point);
> +		touch_notify_touch_down(base,
> +					time,
> +					slot,
> +					seat_slot,
> +					&point,
> +					slot_data->touch_major,
> +					slot_data->touch_minor,
> +					slot_data->orientation,
> +					slot_data->pressure);
>  		break;
>  	case EVDEV_ABSOLUTE_MT_MOTION:
>  		if (!(device->seat_caps & EVDEV_DEVICE_TOUCH))
>  			break;
>  
> -		seat_slot = device->mt.slots[slot].seat_slot;
> -		point = device->mt.slots[slot].point;
> +		seat_slot = slot_data->seat_slot;
> +
> +		point = slot_data->point;
>  
>  		if (seat_slot == -1)
>  			break;
>  
>  		transform_absolute(device, &point);
> -		touch_notify_touch_motion(base, time, slot, seat_slot,
> -					  &point);
> +		touch_notify_touch_motion(base,
> +					  time,
> +					  slot,
> +					  seat_slot,
> +					  &point,
> +					  slot_data->touch_major,
> +					  slot_data->touch_minor,
> +					  slot_data->orientation,
> +					  slot_data->pressure);
>  		break;
>  	case EVDEV_ABSOLUTE_MT_UP:
>  		if (!(device->seat_caps & EVDEV_DEVICE_TOUCH))
>  			break;
>  
> -		seat_slot = device->mt.slots[slot].seat_slot;
> -		device->mt.slots[slot].seat_slot = -1;
> +		seat_slot = slot_data->seat_slot;
> +		slot_data->seat_slot = -1;
>  
>  		if (seat_slot == -1)
>  			break;
> @@ -371,7 +506,15 @@ evdev_flush_pending_event(struct evdev_device *device, uint64_t time)
>  		point = device->abs.point;
>  		transform_absolute(device, &point);
>  
> -		touch_notify_touch_down(base, time, -1, seat_slot, &point);
> +		touch_notify_touch_down(base,
> +					time,
> +					-1,
> +					seat_slot,
> +					&point,
> +					DEFAULT_TOUCH_MAJOR,
> +					DEFAULT_TOUCH_MINOR,
> +					DEFAULT_TOUCH_ORIENTATION,
> +					DEFAULT_TOUCH_PRESSURE);
>  		break;
>  	case EVDEV_ABSOLUTE_MOTION:
>  		point = device->abs.point;
> @@ -383,8 +526,15 @@ evdev_flush_pending_event(struct evdev_device *device, uint64_t time)
>  			if (seat_slot == -1)
>  				break;
>  
> -			touch_notify_touch_motion(base, time, -1, seat_slot,
> -						  &point);
> +			touch_notify_touch_motion(base,
> +						  time,
> +						  -1,
> +						  seat_slot,
> +						  &point,
> +						  DEFAULT_TOUCH_MAJOR,
> +						  DEFAULT_TOUCH_MINOR,
> +						  DEFAULT_TOUCH_ORIENTATION,
> +						  DEFAULT_TOUCH_PRESSURE);
>  		} else if (device->seat_caps & EVDEV_DEVICE_POINTER) {
>  			pointer_notify_motion_absolute(base, time, &point);
>  		}
> @@ -544,8 +694,9 @@ evdev_process_touch(struct evdev_device *device,
>  		    struct input_event *e,
>  		    uint64_t time)
>  {
> -	switch (e->code) {
> -	case ABS_MT_SLOT:
> +	struct mt_slot *current_slot = &device->mt.slots[device->mt.slot];
> +
> +	if (e->code == ABS_MT_SLOT) {
>  		if ((size_t)e->value >= device->mt.slots_len) {
>  			log_bug_libinput(device->base.seat->libinput,
>  					 "%s exceeds slots (%d of %d)\n",
> @@ -556,26 +707,54 @@ evdev_process_touch(struct evdev_device *device,
>  		}
>  		evdev_flush_pending_event(device, time);
>  		device->mt.slot = e->value;
> -		break;
> -	case ABS_MT_TRACKING_ID:
> +	} else if(e->code == ABS_MT_TRACKING_ID) {
>  		if (device->pending_event != EVDEV_NONE &&
>  		    device->pending_event != EVDEV_ABSOLUTE_MT_MOTION)
>  			evdev_flush_pending_event(device, time);
> -		if (e->value >= 0)
> +		if (e->value >= 0) {
>  			device->pending_event = EVDEV_ABSOLUTE_MT_DOWN;
> -		else
> +			current_slot->available_data = TOUCH_SLOT_DATA_NONE;
> +			current_slot->pressure = DEFAULT_TOUCH_PRESSURE;
> +			current_slot->orientation = DEFAULT_TOUCH_ORIENTATION;
> +			current_slot->touch_major = DEFAULT_TOUCH_MAJOR;
> +			current_slot->touch_minor = DEFAULT_TOUCH_MINOR;

no, get this from libevdev, otherwise we may have the wrong value if the
kernel skips it when the value is unchanged to the last event. that's
unlikely enough, but one of the bugs that's painful to debug when it does
happen. see  libevdev_get_slot_value().


> +		} else {
>  			device->pending_event = EVDEV_ABSOLUTE_MT_UP;
> -		break;
> -	case ABS_MT_POSITION_X:
> -		device->mt.slots[device->mt.slot].point.x = e->value;
> -		if (device->pending_event == EVDEV_NONE)
> -			device->pending_event = EVDEV_ABSOLUTE_MT_MOTION;
> -		break;
> -	case ABS_MT_POSITION_Y:
> -		device->mt.slots[device->mt.slot].point.y = e->value;
> -		if (device->pending_event == EVDEV_NONE)
> +		}
> +	} else {
> +		bool needs_wake = true;
> +
> +		switch(e->code) {
> +		case ABS_MT_POSITION_X:
> +			current_slot->point.x = e->value;
> +			break;
> +		case ABS_MT_POSITION_Y:
> +			current_slot->point.y = e->value;
> +			break;
> +		case ABS_MT_TOUCH_MAJOR:
> +			current_slot->available_data |= TOUCH_SLOT_DATA_MAJOR;
> +			current_slot->touch_major = e->value;
> +			if (!(current_slot->available_data & TOUCH_SLOT_DATA_MINOR))
> +				current_slot->touch_minor = e->value;

I think this is something we should do on EV_SYN instead. officially you
can't rely on major coming in before minor.

> +			break;
> +		case ABS_MT_TOUCH_MINOR:
> +			current_slot->available_data |= TOUCH_SLOT_DATA_MINOR;
> +			current_slot->touch_minor = e->value;
> +			break;
> +		case ABS_MT_ORIENTATION:
> +			current_slot->available_data |= TOUCH_SLOT_DATA_ORIENTATION;
> +			current_slot->orientation = e->value;
> +			break;
> +		case ABS_MT_PRESSURE:
> +			current_slot->available_data |= TOUCH_SLOT_DATA_PRESSURE;
> +			current_slot->pressure = e->value;
> +			break;
> +		default:
> +			needs_wake = false;
> +			break;
> +		}
> +		if (needs_wake && device->pending_event == EVDEV_NONE)
>  			device->pending_event = EVDEV_ABSOLUTE_MT_MOTION;
> -		break;
>  	}
>  }
>  
> @@ -1880,6 +2059,10 @@ evdev_configure_device(struct evdev_device *device)
>  			return -1;
>  		}
>  	}
> +	if (libevdev_has_event_code(evdev, EV_ABS, ABS_MT_ORIENTATION))
> +		device->abs.absinfo_orientation = libevdev_get_abs_info(evdev, ABS_MT_ORIENTATION);
> +	if (libevdev_has_event_code(evdev, EV_ABS, ABS_MT_PRESSURE))
> +		device->abs.absinfo_pressure = libevdev_get_abs_info(evdev, ABS_MT_PRESSURE);

libevdev returns NULL when the code doesn't exist, so you can skip the if

>  
>  	if (udev_tags & EVDEV_UDEV_TAG_TOUCHPAD) {
>  		device->dispatch = evdev_mt_touchpad_create(device);
> diff --git a/src/evdev.h b/src/evdev.h
> index 566b0a4..19346ef 100644
> --- a/src/evdev.h
> +++ b/src/evdev.h
> @@ -109,9 +109,23 @@ enum evdev_device_model {
>  	EVDEV_MODEL_ALPS_TOUCHPAD,
>  };
>  
> +/* optional slot data */
> +enum touch_slot_data {
> +	TOUCH_SLOT_DATA_NONE = 0,
> +	TOUCH_SLOT_DATA_MAJOR = (1 << 1),
> +	TOUCH_SLOT_DATA_MINOR = (1 << 2),
> +	TOUCH_SLOT_DATA_ORIENTATION = (1 << 3),
> +	TOUCH_SLOT_DATA_PRESSURE = (1 << 4),
> +};
> +
>  struct mt_slot {
>  	int32_t seat_slot;
>  	struct device_coords point;
> +	enum touch_slot_data available_data;
> +	int32_t touch_major;
> +	int32_t touch_minor;
> +	int32_t orientation;
> +	int32_t pressure;
>  };
>  
>  struct evdev_device {
> @@ -128,6 +142,7 @@ struct evdev_device {
>  	int fd;
>  	struct {
>  		const struct input_absinfo *absinfo_x, *absinfo_y;
> +		const struct input_absinfo *absinfo_pressure, *absinfo_orientation;
>  		int fake_resolution;
>  
>  		struct device_coords point;
> @@ -352,6 +367,28 @@ double
>  evdev_device_transform_y(struct evdev_device *device,
>  			 double y,
>  			 uint32_t height);
> +double
> +evdev_device_transform_major(struct evdev_device *device,
> +			     int32_t major,
> +			     int32_t orientation,
> +			     uint32_t width,
> +			     uint32_t height);
> +
> +double
> +evdev_device_transform_minor(struct evdev_device *device,
> +			     int32_t minor,
> +			     int32_t orientation,
> +			     uint32_t width,
> +			     uint32_t height);
> +
> +double
> +evdev_device_transform_orientation(struct evdev_device *device,
> +				   int32_t orientation);
> +
> +double
> +evdev_device_transform_pressure(struct evdev_device *device,
> +				int32_t pressure);
> +
>  int
>  evdev_device_suspend(struct evdev_device *device);
>  
> diff --git a/src/libinput-private.h b/src/libinput-private.h
> index a7c8838..5a2e2ea 100644
> --- a/src/libinput-private.h
> +++ b/src/libinput-private.h
> @@ -345,14 +345,22 @@ touch_notify_touch_down(struct libinput_device *device,
>  			uint64_t time,
>  			int32_t slot,
>  			int32_t seat_slot,
> -			const struct device_coords *point);
> +			const struct device_coords *point,
> +			int32_t touch_major,
> +			int32_t touch_minor,
> +			int32_t orientation,
> +			int32_t pressure);
>  
  void
>  touch_notify_touch_motion(struct libinput_device *device,
>  			  uint64_t time,
>  			  int32_t slot,
>  			  int32_t seat_slot,
> -			  const struct device_coords *point);
> +			  const struct device_coords *point,
> +			  int32_t touch_major,
> +			  int32_t touch_minor,
> +			  int32_t orientation,
> +			  int32_t pressure);
>  
>  void
>  touch_notify_touch_up(struct libinput_device *device,
> diff --git a/src/libinput.c b/src/libinput.c
> index 7a097c0..aa5902d 100644
> --- a/src/libinput.c
> +++ b/src/libinput.c
> @@ -112,6 +112,10 @@ struct libinput_event_touch {
>  	int32_t slot;
>  	int32_t seat_slot;
>  	struct device_coords point;
> +	int32_t touch_major;
> +	int32_t touch_minor;
> +	int32_t orientation;
> +	int32_t pressure;
>  };
>  
>  static void
> @@ -637,6 +641,110 @@ libinput_event_touch_get_y(struct libinput_event_touch *event)
>  	return evdev_convert_to_mm(device->abs.absinfo_y, event->point.y);
>  }
>  
> +LIBINPUT_EXPORT double
> +libinput_event_touch_get_major(struct libinput_event_touch *event)
> +{
> +	struct evdev_device *device =
> +		(struct evdev_device *) event->base.device;
> +
> +	require_event_type(libinput_event_get_context(&event->base),
> +			   event->base.type,
> +			   0,
> +			   LIBINPUT_EVENT_TOUCH_DOWN,
> +			   LIBINPUT_EVENT_TOUCH_MOTION);
> +
> +	return evdev_convert_to_mm(device->abs.absinfo_x, event->touch_major);

now that I see this: let's make major/minor a struct device_coords, it's a
pair of information anyway and this way we don't have any doubt on what
coordinate format the value is.

> +}
> +
> +LIBINPUT_EXPORT double
> +libinput_event_touch_get_major_transformed(struct libinput_event_touch *event,
> +					   uint32_t width,
> +					   uint32_t height)
> +{
> +	struct evdev_device *device =
> +		(struct evdev_device *) event->base.device;
> +
> +	require_event_type(libinput_event_get_context(&event->base),
> +			   event->base.type,
> +			   0,
> +			   LIBINPUT_EVENT_TOUCH_DOWN,
> +			   LIBINPUT_EVENT_TOUCH_MOTION);
> +
> +	return evdev_device_transform_major(device,
> +					    event->touch_major,
> +					    event->orientation,
> +					    width,
> +					    height);
> +}
> +
> +LIBINPUT_EXPORT double
> +libinput_event_touch_get_minor(struct libinput_event_touch *event)
> +{
> +	struct evdev_device *device =
> +		(struct evdev_device *) event->base.device;
> +
> +	require_event_type(libinput_event_get_context(&event->base),
> +			   event->base.type,
> +			   0,
> +			   LIBINPUT_EVENT_TOUCH_DOWN,
> +			   LIBINPUT_EVENT_TOUCH_MOTION);
> +
> +	return evdev_convert_to_mm(device->abs.absinfo_x, event->touch_minor);
> +}
> +
> +LIBINPUT_EXPORT double
> +libinput_event_touch_get_minor_transformed(struct libinput_event_touch *event,
> +					   uint32_t width,
> +					   uint32_t height)
> +{
> +	struct evdev_device *device =
> +		(struct evdev_device *) event->base.device;
> +
> +	require_event_type(libinput_event_get_context(&event->base),
> +			   event->base.type,
> +			   0,
> +			   LIBINPUT_EVENT_TOUCH_DOWN,
> +			   LIBINPUT_EVENT_TOUCH_MOTION);
> +
> +	return evdev_device_transform_major(device,
> +					    event->touch_major,
> +					    event->orientation,
> +					    width,
> +					    height);
> +}
> +
> +LIBINPUT_EXPORT double
> +libinput_event_touch_get_orientation(struct libinput_event_touch *event)
> +{
> +	struct evdev_device *device =
> +		(struct evdev_device *) event->base.device;
> +
> +	require_event_type(libinput_event_get_context(&event->base),
> +			   event->base.type,
> +			   0,
> +			   LIBINPUT_EVENT_TOUCH_DOWN,
> +			   LIBINPUT_EVENT_TOUCH_MOTION);
> +
> +	return evdev_device_transform_orientation(device,
> +						  event->orientation);
> +}
> +
> +LIBINPUT_EXPORT double
> +libinput_event_touch_get_pressure(struct libinput_event_touch *event)
> +{
> +	struct evdev_device *device =
> +		(struct evdev_device *) event->base.device;
> +
> +	require_event_type(libinput_event_get_context(&event->base),
> +			   event->base.type,
> +			   0,
> +			   LIBINPUT_EVENT_TOUCH_DOWN,
> +			   LIBINPUT_EVENT_TOUCH_MOTION);
> +
> +	return evdev_device_transform_pressure(device,
> +					       event->pressure);
> +}
> +
>  struct libinput_source *
>  libinput_add_fd(struct libinput *libinput,
>  		int fd,
> @@ -1243,7 +1351,11 @@ touch_notify_touch_down(struct libinput_device *device,
>  			uint64_t time,
>  			int32_t slot,
>  			int32_t seat_slot,
> -			const struct device_coords *point)
> +			const struct device_coords *point,
> +			int32_t touch_major,
> +			int32_t touch_minor,
> +			int32_t orientation,
> +			int32_t pressure)
>  {
>  	struct libinput_event_touch *touch_event;
>  
> @@ -1259,6 +1371,10 @@ touch_notify_touch_down(struct libinput_device *device,
>  		.slot = slot,
>  		.seat_slot = seat_slot,
>  		.point = *point,
> +		.touch_major = touch_major,
> +		.touch_minor = touch_minor,
> +		.orientation = orientation,
> +		.pressure = pressure,
>  	};
>  
>  	post_device_event(device, time,
> @@ -1271,7 +1387,11 @@ touch_notify_touch_motion(struct libinput_device *device,
>  			  uint64_t time,
>  			  int32_t slot,
>  			  int32_t seat_slot,
> -			  const struct device_coords *point)
> +			  const struct device_coords *point,
> +			  int32_t touch_major,
> +			  int32_t touch_minor,
> +			  int32_t orientation,
> +			  int32_t pressure)
>  {
>  	struct libinput_event_touch *touch_event;
>  
> @@ -1287,6 +1407,10 @@ touch_notify_touch_motion(struct libinput_device *device,
>  		.slot = slot,
>  		.seat_slot = seat_slot,
>  		.point = *point,
> +		.touch_major = touch_major,
> +		.touch_minor = touch_minor,
> +		.orientation = orientation,
> +		.pressure = pressure,
>  	};
>  
>  	post_device_event(device, time,
> diff --git a/src/libinput.h b/src/libinput.h
> index 0d3200f..d526a27 100644
> --- a/src/libinput.h
> +++ b/src/libinput.h
> @@ -915,6 +915,200 @@ libinput_event_touch_get_y_transformed(struct libinput_event_touch *event,
>  /**
>   * @ingroup event_touch
>   *
> + * @verbatim
> + *                                   screen space
> + *        major
> + *  _______/\________
> + * /                 \
> + *       +++++++
> + *    +++       +++
> + *  ++             ++
> + * +     touch       +
> + * +       ellipse   +   ---> finger pointing direction
> + *  ++             ++
> + *    +++       +++
> + *       +++++++
> + * @endverbatim
> + *
> + * Return the diameter of the major axis of the touch ellipse in mm
> + * This value might not be provided by the device, in that case the value
> + * 1.0 is returned
> + *
> + * For events not of type @ref LIBINPUT_EVENT_TOUCH_DOWN, @ref
> + * LIBINPUT_EVENT_TOUCH_MOTION, this function returns 0.
> + *
> + * @note It is an application bug to call this function for events of type
> + * @ref LIBINPUT_EVENT_TOUCH_DOWN or @ref LIBINPUT_EVENT_TOUCH_MOTION.
> + *
> + * @param event The libinput touch event
> + * @return The current touch major value
> + */
> +double
> +libinput_event_touch_get_major(struct libinput_event_touch *event);
> +
> +/**
> + * @ingroup event_touch
> + *
> + * @verbatim
> + *                                   screen space
> + *        major
> + *  _______/\________
> + * /                 \
> + *       +++++++
> + *    +++       +++
> + *  ++             ++
> + * +     touch       +
> + * +       ellipse   +   ---> finger pointing direction
> + *  ++             ++
> + *    +++       +++
> + *       +++++++
> + * @endverbatim
> + *
> + * Return the diameter of the major axis of the touch ellipse in screen
> + * space This value might not be provided by the device, in that case the

'.' missing, and technically it's not in screen space but transformed to
whatever is passed in. 
(ok, now I see the rest of the doc says screen coords too, so leave it as is
and I'll fix this up later).

> + * value 1.0 is returned

Shouldn't we return 0 if the value is missing? otherwise a caller won't know
if the value is valid or not.

> + *
> + * For events not of type @ref LIBINPUT_EVENT_TOUCH_DOWN, @ref
> + * LIBINPUT_EVENT_TOUCH_MOTION, this function returns 0.
> + *
> + * @note It is an application bug to call this function for events of type
> + * @ref LIBINPUT_EVENT_TOUCH_DOWN or @ref LIBINPUT_EVENT_TOUCH_MOTION.
> + *
> + * @param event The libinput touch event
> + * @param width The current output screen width
> + * @param height The current output screen height
> + * @return The current touch major value
> + */
> +double
> +libinput_event_touch_get_major_transformed(struct libinput_event_touch *event,
> +					   uint32_t width,
> +					   uint32_t height);
> +
> +/**
> + * @ingroup event_touch
> + *
> + * @verbatim
> + *                                     screen space
> + *
> + *       +++++++       -
> + *    +++       +++     \
> + *  ++             ++   |
> + * +    touch        +  =- minor
> + * +      ellipse    +  |        ---> finger pointing direction
> + *  ++             ++   |
> + *    +++       +++     /
> + *       +++++++       -
> + *
> + * @endverbatim
> + *
> + * Return the diameter of the minor axis of the touch ellipse in mm.
> + * This value might not be provided by the device, in this case the value
> + * 1.0 is returned.
> + *
> + * For events not of type @ref LIBINPUT_EVENT_TOUCH_DOWN, @ref
> + * LIBINPUT_EVENT_TOUCH_MOTION, this function returns 0.
> + *
> + * @note It is an application bug to call this function for events of type
> + * @ref LIBINPUT_EVENT_TOUCH_DOWN or @ref LIBINPUT_EVENT_TOUCH_MOTION.
> + *
> + * @param event The libinput touch event
> + * @return The current touch minor value
> + */
> +double
> +libinput_event_touch_get_minor(struct libinput_event_touch *event);
> +
> +/**
> + * @ingroup event_touch
> + *
> + * @verbatim
> + *                                     screen space
> + *
> + *       +++++++       -
> + *    +++       +++     \
> + *  ++             ++   |
> + * +    touch        +  =- minor
> + * +      ellipse    +  |        ---> finger pointing direction
> + *  ++             ++   |
> + *    +++       +++     /
> + *       +++++++       -
> + *
> + * @endverbatim
> + *
> + * Return the diameter of the minor axis of the touch ellipse in screen
> + * space. This value might not be provided by the device, in this case
> + * the value 1.0 is returned.
> + *
> + * For events not of type @ref LIBINPUT_EVENT_TOUCH_DOWN, @ref
> + * LIBINPUT_EVENT_TOUCH_MOTION, this function returns 0.
> + *
> + * @note It is an application bug to call this function for events of type
> + * @ref LIBINPUT_EVENT_TOUCH_DOWN or @ref LIBINPUT_EVENT_TOUCH_MOTION.
> + *
> + * @param event The libinput touch event
> + * @param width The current output screen width
> + * @param height The current output screen height
> + * @return The current touch minor value
> + */
> +double
> +libinput_event_touch_get_minor_transformed(struct libinput_event_touch *event,
> +					   uint32_t width,
> +					   uint32_t height);
> +
> +/**
> + * @ingroup event_touch
> + *
> + * Return the current pressure value touch point normalized to the range
> + * [0,1]. If this value is not provided by the device, it always be 1.0,

typo, "it is always"


> + * which denotes the maximum pressure on a touch point.
> + *
> + * For events not of type @ref LIBINPUT_EVENT_TOUCH_DOWN, @ref
> + * LIBINPUT_EVENT_TOUCH_MOTION, this function returns 0.
> + *
> + * @note It is an application bug to call this function for events of type
> + * @ref LIBINPUT_EVENT_TOUCH_DOWN or @ref LIBINPUT_EVENT_TOUCH_MOTION.
> + *
> + * @param event The libinput touch event
> + * @return The current pressure value
> + */
> +double
> +libinput_event_touch_get_pressure(struct libinput_event_touch *event);
> +
> +/**
> + * @ingroup event_touch
> + * @verbatim
> + *
> + *    major     ∧ y
> + *    axis \    |
> + *          \++ |
> + *         + \ ++
> + *         +  \ |+
> + *          +  \| +
> + *           +  0--+---------------> x
> + *            +     +
> + *             ++   +
> + *               +++
> + *
> + * @endverbatim
> + *
> + * Return the revolution of the major axis of the ellipse away from the X axis

> + * in degrees. 

see my comment above, we should make this off the y axis. In the
tablet/buttonset branch the wording we used was "off the logical north",
since a device may be physically rotated. Guess it's best to copy that
wording .
Maybe add "in the above example, the value would be 315", to avoid any
confusion and we wouldn't want your drawing skills to go to waste ;)

This value might not be measured by the device, or only measured
> + * in coarse steps (e.g only indicating alignment with X or y axis).
> + *
> + * For events not of type @ref LIBINPUT_EVENT_TOUCH_DOWN, @ref
> + * LIBINPUT_EVENT_TOUCH_MOTION, this function returns 0.
> + *
> + * @note It is an application bug to call this function for events of type
> + * @ref LIBINPUT_EVENT_TOUCH_DOWN or @ref LIBINPUT_EVENT_TOUCH_MOTION.
> + *
> + * @param event The libinput touch event
> + * @return The current orientation value
> + */
> +double
> +libinput_event_touch_get_orientation(struct libinput_event_touch *event);
> +
> +/**
> + * @ingroup event_touch
> + *
>   * @return The generic libinput_event of this event
>   */
>  struct libinput_event *
> diff --git a/src/libinput.sym b/src/libinput.sym
> index 9c11174..aebe052 100644
> --- a/src/libinput.sym
> +++ b/src/libinput.sym
> @@ -140,3 +140,13 @@ LIBINPUT_0.15.0 {
>  global:
>  	libinput_device_keyboard_has_key;
>  } LIBINPUT_0.14.0;
> +
> +LIBINPUT_0.19.0 {
> +global:
> +	libinput_event_touch_get_minor;
> +	libinput_event_touch_get_major;
> +	libinput_event_touch_get_minor_transformed;
> +	libinput_event_touch_get_major_transformed;
> +	libinput_event_touch_get_orientation;
> +	libinput_event_touch_get_pressure;
> +} LIBINPUT_0.15.0;
> diff --git a/test/touch.c b/test/touch.c
> index b519613..317d4e9 100644
> --- a/test/touch.c
> +++ b/test/touch.c
> @@ -652,6 +652,94 @@ START_TEST(touch_initial_state)
>  }
>  END_TEST
>  
> +START_TEST(touch_point_properties)
> +{
> +	struct litest_device *dev = litest_current_device();
> +	struct libinput *li = dev->libinput;
> +	struct libinput_event *ev;
> +	struct libinput_event_touch *tev;
> +	struct axis_replacement down_values[] = {
> +		{ABS_MT_PRESSURE, 128},
> +		{ABS_MT_ORIENTATION, 64},
> +		{ABS_MT_TOUCH_MAJOR, 14},
> +		{ABS_MT_TOUCH_MINOR, 8}};
> +	struct axis_replacement move_values[] = {
> +		{ABS_MT_ORIENTATION, 128},
> +		{ABS_MT_TOUCH_MAJOR, 30}};

you have a litest_add_for_device() here but I'd much prefer this 
to be a generic test run for all touch devices. add a
libevdev_has_event_code() check here to return early here for those that
don't match. plus, if you can find another device that has these axis but
with slightly different values that'd be great.

> +
> +
> +	litest_drain_events(li);
> +
> +	litest_touch_down_extended(dev, 0, 5, 95, down_values);
> +
> +	litest_wait_for_event_of_type(li, LIBINPUT_EVENT_TOUCH_DOWN, -1);
> +
> +	ev = libinput_get_event(li);
> +	tev = libinput_event_get_touch_event(ev);

use litest_is_touch_event() here, it checks for a couple of other things for
you

> +
> +	ck_assert_double_eq(libinput_event_touch_get_pressure(tev), 128.0/255.0);

that won't work with a random device, you'll need to use libevdev and
dev->evdev to get the right scale for the axis here.


> +	ck_assert_double_eq(libinput_event_touch_get_orientation(tev), 67.412);
> +	ck_assert_int_eq(libinput_event_touch_get_major(tev), 14);
> +	ck_assert_int_eq(libinput_event_touch_get_minor(tev), 8);

this doesn't look right, even when only run for this device. 14 should only
be if the device has a res of 1u/mm. which is only true for resolution-less
devices.

> +
> +	libinput_event_destroy(ev);
> +
> +	litest_touch_move_extended(dev, 0, 5, 95, move_values);
> +	litest_wait_for_event_of_type(li, LIBINPUT_EVENT_TOUCH_MOTION, -1);
> +
> +	ev = libinput_get_event(li);
> +	tev = libinput_event_get_touch_event(ev);
> +
> +	ck_assert_double_eq(libinput_event_touch_get_pressure(tev), 128.0/255.0);
> +	ck_assert_double_eq(libinput_event_touch_get_orientation(tev), 44.823);
> +	ck_assert_int_eq(libinput_event_touch_get_major(tev), 30);
> +	ck_assert_int_eq(libinput_event_touch_get_minor(tev), 8);
> +
> +	libinput_event_destroy(ev);
> +}
> +END_TEST
> +
> +START_TEST(touch_point_wo_minor_or_orienation)

s/wo/no/ is clearer (imo, anyway)

otherwise, same comments as above.

> +{
> +	struct litest_device *dev = litest_current_device();
> +	struct libinput *li = dev->libinput;
> +	struct libinput_event *ev;
> +	struct libinput_event_touch *tev;
> +	struct axis_replacement down_values[] = {
> +		{ABS_MT_PRESSURE, 128},
> +		{ABS_MT_TOUCH_MAJOR, 14}};
> +	struct axis_replacement move_values[] = {
> +		{ABS_MT_TOUCH_MAJOR, 30}};
> +
> +
> +	litest_drain_events(li);
> +
> +	litest_touch_down_extended(dev, 0, 5, 95, down_values);
> +
> +	litest_wait_for_event_of_type(li, LIBINPUT_EVENT_TOUCH_DOWN, -1);
> +
> +	ev = libinput_get_event(li);
> +	tev = libinput_event_get_touch_event(ev);
> +
> +	ck_assert_double_eq(libinput_event_touch_get_orientation(tev), 0.0);
> +	ck_assert_int_eq(libinput_event_touch_get_major(tev), 14);
> +	ck_assert_int_eq(libinput_event_touch_get_minor(tev), 14);
> +
> +	libinput_event_destroy(ev);
> +
> +	litest_touch_move_extended(dev, 0, 5, 95, move_values);
> +	litest_wait_for_event_of_type(li, LIBINPUT_EVENT_TOUCH_MOTION, -1);
> +
> +	ev = libinput_get_event(li);
> +	tev = libinput_event_get_touch_event(ev);
> +
> +	ck_assert_int_eq(libinput_event_touch_get_major(tev), 30);
> +	ck_assert_int_eq(libinput_event_touch_get_minor(tev), 30);
> +
> +	libinput_event_destroy(ev);
> +}
> +END_TEST
> +
>  void
>  litest_setup_tests(void)
>  {
> @@ -676,6 +764,12 @@ litest_setup_tests(void)
>  	litest_add("touch:protocol a", touch_protocol_a_init, LITEST_PROTOCOL_A, LITEST_ANY);
>  	litest_add("touch:protocol a", touch_protocol_a_touch, LITEST_PROTOCOL_A, LITEST_ANY);
>  	litest_add("touch:protocol a", touch_protocol_a_2fg_touch, LITEST_PROTOCOL_A, LITEST_ANY);
> +	litest_add_for_device("touch:touch point properties",
> +			      touch_point_properties,
> +			      LITEST_GENERIC_MULTITOUCH_SCREEN);
> +	litest_add_for_device("touch:touch point without minor or orientation",
> +			      touch_point_wo_minor_or_orienation,
> +			      LITEST_NEXUS4_TOUCH_SCREEN);

the string is a test suite name to group matching tests. in this case the
best name for both would be "touch:properties" or something like this.

Cheers,
   Peter

>  
>  	litest_add_ranged("touch:state", touch_initial_state, LITEST_TOUCH, LITEST_PROTOCOL_A, &axes);
>  }
> -- 
> 2.1.4
> 


More information about the wayland-devel mailing list