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

[Peter Hutterer]

On Mon, Feb 22, 2016 at 05:58:22PM -0800, Jason Gerecke wrote:
> 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>

amended as requested, thanks!

Cheers,
   Peter

> 
> >  
> >  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);
> >  
> >