[PATCH libinput] tablet: use the tilt resolution if we have it

[Jason Gerecke]

On 02/17/2016 05:39 PM, Peter Hutterer wrote:
> A nonzero resolution on the tilt axes is units/rad so we can calculate the
> physical min/max based. Uneven min/max ranges are supported.
> 
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> ---
>  src/evdev-tablet.c | 28 +++++++++++++++++++++-------
>  test/tablet.c      | 12 ++++++++----
>  2 files changed, 29 insertions(+), 11 deletions(-)
> 
> diff --git a/src/evdev-tablet.c b/src/evdev-tablet.c
> index 1e5c2cd..226d5bd 100644
> --- a/src/evdev-tablet.c
> +++ b/src/evdev-tablet.c
> @@ -230,17 +230,31 @@ adjust_tilt(const struct input_absinfo *absinfo)
>  	double range = absinfo->maximum - absinfo->minimum;
>  	double value = (absinfo->value - absinfo->minimum) / range;
>  	const int WACOM_MAX_DEGREES = 64;
> +	double max_degrees;
> +
> +	/* If resolution is nonzero, it's in units/radian. But require
> +	 * a min/max less/greater than zero so we can assume 0 is the
> +	 * center */
> +	if (absinfo->resolution != 0 &&
> +	    absinfo->maximum > 0 &&
> +	    absinfo->minimum < 0) {
> +		int range_max = absinfo->value >= 0 ? absinfo->maximum
> +						    : -absinfo->minimum;
> +
> +		max_degrees = 180.0/M_PI * range_max/absinfo->resolution;
> +	} else {
> +		/* Wacom supports physical [-64, 64] degrees, so map to that by
> +		 * default. If other tablets have a different physical range or
> +		 * nonzero physical offsets, they need extra treatment
> +		 * here.
> +		 */
> +		max_degrees = WACOM_MAX_DEGREES;
> +	}
>  
>  	/* Map to the (-1, 1) range */
>  	value = (value * 2) - 1;
>  
> -	/* Wacom supports physical [-64, 64] degrees, so map to that by
> -	 * default. If other tablets have a different physical range or
> -	 * nonzero physical offsets, they need extra treatment
> -	 * here.
> -	 */
> -
> -	return value * WACOM_MAX_DEGREES;
> +	return value * max_degrees;
>  }

This looks correct, if a bit difficult to follow because of the range
normalization (specifically the subtle need for "range_max"). You might
consider having the normalization only occur in the 'else' case so that
the 'if' can be written more straightforwardly:


double value;
const int WACOM_MAX_DEGREES = 64;

if ( /* can use resolution */ ) {
    value = 180.0/M_PI * absinfo->value/absinfo->resolution;
} else {
    /* normalize and scale to +-64 degrees */
}

return value;


That aside, everything looks fine.

Reviewed-by: Jason Gerecke <jason.gerecke at wacom.com>

Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one /
(That is to say, eight) to the two, /
But you can’t take seven from three, /
So you look at the sixty-fours....

>  
>  static inline int32_t
> diff --git a/test/tablet.c b/test/tablet.c
> index c5dc892..ad6ac45 100644
> --- a/test/tablet.c
> +++ b/test/tablet.c
> @@ -3298,7 +3298,8 @@ START_TEST(tilt_x)
>  	ck_assert_double_ge(tx, -52);
>  
>  	ty = libinput_event_tablet_tool_get_tilt_y(tev);
> -	ck_assert_double_eq(ty, -64);
> +	ck_assert_double_ge(ty, -65);
> +	ck_assert_double_lt(ty, -63);
>  
>  	libinput_event_destroy(event);
>  
> @@ -3320,7 +3321,8 @@ START_TEST(tilt_x)
>  		ck_assert_double_le(tx, expected_tx + 2);
>  
>  		ty = libinput_event_tablet_tool_get_tilt_y(tev);
> -		ck_assert_double_eq(ty, -64);
> +		ck_assert_double_ge(ty, -65);
> +		ck_assert_double_lt(ty, -63);
>  
>  		libinput_event_destroy(event);
>  
> @@ -3365,7 +3367,8 @@ START_TEST(tilt_y)
>  	ck_assert_double_ge(ty, -52);
>  
>  	tx = libinput_event_tablet_tool_get_tilt_x(tev);
> -	ck_assert_double_eq(tx, -64);
> +	ck_assert_double_ge(tx, -65);
> +	ck_assert_double_lt(tx, -63);
>  
>  	libinput_event_destroy(event);
>  
> @@ -3387,7 +3390,8 @@ START_TEST(tilt_y)
>  		ck_assert_double_le(ty, expected_ty + 2);
>  
>  		tx = libinput_event_tablet_tool_get_tilt_x(tev);
> -		ck_assert_double_eq(tx, -64);
> +		ck_assert_double_ge(tx, -65);
> +		ck_assert_double_lt(tx, -63);
>  
>  		libinput_event_destroy(event);
>  
>