[PATCH libinput 1/3] Add a timer subsystem

[Peter Hutterer]

On Wed, Jun 04, 2014 at 05:26:34PM +0200, Hans de Goede wrote:
> Currently we are using DIY timers in the touchpad softbutton and tap handling
> code, and at least the softbutton code gets its wrong. It uses one timer-fd
> per touchpad to set a timeout per touch, which means that if a timeout is
> set for 100ms from now for touch 1, and then a timeout gets set 50 ms later
> for 200 ms from now, then the timeout for touch 1 will come 150 ms too late.
> 
> This commits adds a proper timer subsystem so that we've one place to deal
> with timer handling, and so that we can only get it wrong (well hopefully
> we get it right) in one place.
> 
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> ---

[...]

> +static void
> +libinput_timer_handler(void *data)
> +{
> +	struct libinput *libinput = data;
> +	struct libinput_timer *timer, *tmp;
> +	struct timespec ts;
> +	uint64_t now;
> +	int r;
> +
> +	r = clock_gettime(CLOCK_MONOTONIC, &ts);
> +	if (r) {
> +		log_error("clock_gettime error: %s\n", strerror(errno));
> +		return;
> +	}
> +
> +	now = ts.tv_sec * 1000ULL + ts.tv_nsec / 1000000;
> +
> +	list_for_each_safe(timer, tmp, &libinput->timer.list, list) {
> +		if (timer->expire <= now) {
> +			/* Clear the timer before calling timer_func,
> +			   as timer_func may re-arm it */
> +			libinput_timer_cancel(timer);
> +			timer->timer_func(now, timer->timer_func_data);

would it make sense to pass the timer itself into the timer func, for the
use-cases where the func handles multiple timers and only needs to e.g.
print and re-arm that timer?

this is an internal API so we can add it when needed later.
 
> +		}
> +	}
> +}
> +
> +int
> +libinput_timer_subsys_init(struct libinput *libinput)
> +{
> +	libinput->timer.fd = timerfd_create(CLOCK_MONOTONIC, TFD_CLOEXEC);
> +	if (libinput->timer.fd < 0)
> +		return -1;
> +
> +	list_init(&libinput->timer.list);
> +
> +	libinput->timer.source = libinput_add_fd(libinput,
> +						 libinput->timer.fd,
> +						 libinput_timer_handler,
> +						 libinput);
> +	if (!libinput->timer.source) {
> +		close(libinput->timer.fd);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +void
> +libinput_timer_subsys_exit(struct libinput *libinput)
> +{
> +	/* All timer uses should have destroyed their timers now */

typo: "users"

> +struct libinput_timer {
> +       struct libinput *libinput;
> +       struct list list;
> +       uint64_t expire;

do me a favour and add a comment here and in 
libinput_timer_set that this is in abstime, not reltime.

> +       void (*timer_func)(uint64_t now, void *timer_func_data);
> +       void *timer_func_data;
> +};


series:
Reviewed-by: Peter Hutterer <peter.hutterer at who-t.net>
with Jonas' comments and the above addressed.

two other things that you may or may not want to add because they help
debugging but require some more effort:
- put a log_bug_libinput() into libinput_timer_set if the timer is <
  current time
- put a log_bug_libinput() into libinput_timer_set if the timer is
  more than say 5s into the future. this is arbitrary but a 
  good safety guard that can save on some debugging time.

Cheers,
   Peter