From 01fcef9b9dd54eb79ffc00e672a2ce41b03fef12 Mon Sep 17 00:00:00 2001 From: Lyude Date: Tue, 11 Oct 2016 14:01:49 -0400 Subject: [PATCH] Introduce new profile-oriented API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Almost all of the devices we've worked with here are the same: they have some sort of profile on them, and if they don't (which is quite rare) it's rather easy to simulate one in the driver. In fact basically all of the device drivers in ratbag just update the profile and rewrite it on calls to the write_resolution_dpi() and write_button() calls. This is annoying since profile writes can take a good bit of time, and we're wasting a lot of time with redundant profile writes that way. During XDC this year, we came up with an idea that makes more sense: a profile oriented API. Instead of having multiple entrypoints in the API to writing and updating the profile we just cache all of the profile changes until ratbag_device_commit_profiles() is called, at which point we do a single set of writes for each profile and button that changed. This has a lot of advantages: - Where the profile is and is not written out to the device is now explicit. - Changing attributes of each profile won't result in any sort of delay up until the library user commits the profiles, allowing us to avoid hanging the application every time a simple attribute is changed. - We can cut down on workarounds in drivers to cater to the previous API. Since this commit doesn't handle converting all of the device drivers over to the new API, we also add a temporary wrapper for devices that haven't yet been converted to this new API so that they're still usable. Once all of the drivers have been converted to the new API, the wrapper will be removed along with the internal driver API calls. Signed-off-by: Lyude Changes since v1: - has_changed→dirty - Change namespace to LIBRATBAG_0.5.0 Changes since v2: - Remove unneeded note about not setting dirty bit - commit_profiles->commit - Make FIXMEs more obvious - Minor styling nitpicks - Split up capability check fixes into seperate patch --- src/libratbag-private.h | 37 +++++++--- src/libratbag.c | 158 ++++++++++++++++++++++++++++------------ src/libratbag.h | 12 +++ src/libratbag.sym | 3 +- tools/ratbag-command.c | 4 + 5 files changed, 157 insertions(+), 57 deletions(-) diff --git a/src/libratbag-private.h b/src/libratbag-private.h index adabbbbd..668ffe66 100644 --- a/src/libratbag-private.h +++ b/src/libratbag-private.h @@ -130,6 +130,18 @@ struct ratbag_driver { */ void (*remove)(struct ratbag_device *device); + /** + * Callback called when the driver should write any profiles that + * were modified back to the device. + * + * Both profile and button structs have a dirty variable that can + * be used to tell whether or not they've actually changed since + * the last commit. In order to reduce the amount of time + * committing takes, drivers should use this information to avoid + * writing back profiles and buttons that haven't actually changed. + */ + int (*commit)(struct ratbag_device *device); + /** * Callback called when a read profile is requested by the * caller of the library. @@ -141,9 +153,10 @@ struct ratbag_driver { */ void (*read_profile)(struct ratbag_profile *profile, unsigned int index); - /** - * Here, the driver should actually write the profile to the - * device. + /* + * FIXME: This function is deprecated and should not be removed. Once + * we've updated all the device drivers to stop using it we'll remove + * it. Look at commit() instead. */ int (*write_profile)(struct ratbag_profile *profile); @@ -168,18 +181,18 @@ struct ratbag_driver { */ void (*read_button)(struct ratbag_button *button); - /** - * For the given button, store in the profile and in the device - * the given struct ratbag_button. + /* + * FIXME: This function is deprecated and should not be removed. Once + * we've updated all the device drivers to stop using it we'll remove + * it. Look at commit() instead. */ int (*write_button)(struct ratbag_button *button, const struct ratbag_button_action *action); - /** - * For the given profile, overwrite the current resolution - * of the sensor expressed in DPI, and commit it to the hardware. - * - * Mandatory if the driver exports RATBAG_DEVICE_CAP_SWITCHABLE_RESOLUTION. + /* + * FIXME: This function is deprecated and should not be removed. Once + * we've updated all the device drivers to stop using it we'll remove + * it. Look at commit() instead. */ int (*write_resolution_dpi)(struct ratbag_resolution *resolution, int dpi_x, int dpi_y); @@ -218,6 +231,7 @@ struct ratbag_profile { } resolution; bool is_active; /**< profile is the currently active one */ + bool dirty; /**< profile changed since last commit */ }; #define BUTTON_ACTION_NONE \ @@ -277,6 +291,7 @@ struct ratbag_button { enum ratbag_button_type type; struct ratbag_button_action action; uint32_t action_caps; + bool dirty; /* changed since last commit to device */ }; static inline void diff --git a/src/libratbag.c b/src/libratbag.c index 5a07ecbd..fdeb14c4 100644 --- a/src/libratbag.c +++ b/src/libratbag.c @@ -749,6 +749,87 @@ ratbag_device_has_capability(const struct ratbag_device *device, return !!(device->capabilities & (1UL << cap)); } +/* FIXME: This is a temporary fix for all of the drivers that have yet to be + * converted to the new profile-oriented API. Once all of the drivers have been + * converted, this code should be removed. + */ +static enum ratbag_error_code +ratbag_old_write_profile(struct ratbag_device *device) +{ + struct ratbag_profile *profile; + struct ratbag_button *button; + struct ratbag_resolution *resolution; + int rc; + unsigned int i; + + assert(device->driver->write_profile); + + list_for_each(profile, &device->profiles, link) { + if (!profile->dirty) + continue; + + rc = device->driver->write_profile(profile); + if (rc) + return RATBAG_ERROR_DEVICE; + + if (device->driver->write_resolution_dpi) { + for (i = 0; i < profile->resolution.num_modes; i++) { + resolution = &profile->resolution.modes[i]; + + rc = device->driver->write_resolution_dpi( + resolution, resolution->dpi_x, + resolution->dpi_y); + if (rc) + return RATBAG_ERROR_DEVICE; + } + } + + if (device->driver->write_button) { + list_for_each(button, &profile->buttons, link) { + struct ratbag_button_action action = + button->action; + + if (!button->dirty) + continue; + + rc = device->driver->write_button(button, + &action); + if (rc) + return RATBAG_ERROR_DEVICE; + } + } + } + + return RATBAG_SUCCESS; +} + +LIBRATBAG_EXPORT enum ratbag_error_code +ratbag_device_commit(struct ratbag_device *device) +{ + struct ratbag_profile *profile; + struct ratbag_button *button; + int rc; + + if (!device->driver->commit) { + rc = ratbag_old_write_profile(device); + if (rc != RATBAG_SUCCESS) + return rc; + } else { + rc = device->driver->commit(device); + if (rc) + return RATBAG_ERROR_DEVICE; + } + + list_for_each(profile, &device->profiles, link) { + profile->dirty = false; + + list_for_each(button, &profile->buttons, link) + button->dirty = false; + } + + return RATBAG_SUCCESS; +} + LIBRATBAG_EXPORT enum ratbag_error_code ratbag_profile_set_active(struct ratbag_profile *profile) { @@ -759,11 +840,6 @@ ratbag_profile_set_active(struct ratbag_profile *profile) if (!ratbag_device_has_capability(device, RATBAG_DEVICE_CAP_SWITCHABLE_PROFILE)) return RATBAG_ERROR_CAPABILITY; - assert(device->driver->write_profile); - rc = device->driver->write_profile(profile); - if (rc) - return RATBAG_ERROR_DEVICE; - assert(device->driver->set_active_profile); rc = device->driver->set_active_profile(device, profile->index); if (rc) @@ -843,15 +919,12 @@ ratbag_resolution_set_dpi(struct ratbag_resolution *resolution, unsigned int dpi) { struct ratbag_profile *profile = resolution->profile; - int rc; resolution->dpi_x = dpi; resolution->dpi_y = dpi; + profile->dirty = true; - assert(profile->device->driver->write_resolution_dpi); - rc = profile->device->driver->write_resolution_dpi(resolution, dpi, dpi); - - return rc == 0 ? RATBAG_SUCCESS : RATBAG_ERROR_DEVICE; + return RATBAG_SUCCESS; } LIBRATBAG_EXPORT enum ratbag_error_code @@ -859,7 +932,6 @@ ratbag_resolution_set_dpi_xy(struct ratbag_resolution *resolution, unsigned int x, unsigned int y) { struct ratbag_profile *profile = resolution->profile; - int rc; if (!ratbag_resolution_has_capability(resolution, RATBAG_RESOLUTION_CAP_SEPARATE_XY_RESOLUTION)) @@ -868,18 +940,21 @@ ratbag_resolution_set_dpi_xy(struct ratbag_resolution *resolution, if ((x == 0 && y != 0) || (x != 0 && y == 0)) return RATBAG_ERROR_VALUE; - assert(profile->device->driver->write_resolution_dpi); - rc = profile->device->driver->write_resolution_dpi(resolution, x, y); - return rc == 0 ? RATBAG_SUCCESS : RATBAG_ERROR_DEVICE; + resolution->dpi_x = x; + resolution->dpi_y = y; + profile->dirty = true; + + return RATBAG_SUCCESS; } LIBRATBAG_EXPORT enum ratbag_error_code ratbag_resolution_set_report_rate(struct ratbag_resolution *resolution, unsigned int hz) { + resolution->profile->dirty = true; resolution->hz = hz; - /* FIXME: call into the driver */ - return RATBAG_ERROR_IMPLEMENTATION; + + return RATBAG_SUCCESS; } LIBRATBAG_EXPORT int @@ -930,9 +1005,10 @@ ratbag_resolution_is_default(const struct ratbag_resolution *resolution) LIBRATBAG_EXPORT enum ratbag_error_code ratbag_resolution_set_default(struct ratbag_resolution *resolution) { + resolution->profile->dirty = true; resolution->is_default = true; - /* FIXME: call into the driver */ - return RATBAG_ERROR_IMPLEMENTATION; + + return RATBAG_SUCCESS; } LIBRATBAG_EXPORT struct ratbag_button* @@ -1000,7 +1076,6 @@ LIBRATBAG_EXPORT enum ratbag_error_code ratbag_button_set_button(struct ratbag_button *button, unsigned int btn) { struct ratbag_button_action action = {0}; - int rc; if (!ratbag_device_has_capability(button->profile->device, RATBAG_DEVICE_CAP_BUTTON_KEY)) @@ -1009,12 +1084,11 @@ ratbag_button_set_button(struct ratbag_button *button, unsigned int btn) action.type = RATBAG_BUTTON_ACTION_TYPE_BUTTON; action.action.button = btn; - rc = button->profile->device->driver->write_button(button, &action); + button->action = action; + button->dirty = true; + button->profile->dirty = true; - if (rc == 0) - button->action = action; - - return rc == 0 ? RATBAG_SUCCESS : RATBAG_ERROR_DEVICE; + return RATBAG_SUCCESS; } LIBRATBAG_EXPORT enum ratbag_button_action_special @@ -1031,7 +1105,6 @@ ratbag_button_set_special(struct ratbag_button *button, enum ratbag_button_action_special act) { struct ratbag_button_action action = {0}; - int rc; /* FIXME: range checks */ @@ -1042,12 +1115,11 @@ ratbag_button_set_special(struct ratbag_button *button, action.type = RATBAG_BUTTON_ACTION_TYPE_SPECIAL; action.action.special = act; - rc = button->profile->device->driver->write_button(button, &action); + button->action = action; + button->dirty = true; + button->profile->dirty = true; - if (rc == 0) - button->action = action; - - return rc == 0 ? RATBAG_SUCCESS : RATBAG_ERROR_DEVICE; + return RATBAG_SUCCESS; } LIBRATBAG_EXPORT unsigned int @@ -1070,7 +1142,6 @@ ratbag_button_set_key(struct ratbag_button *button, size_t sz) { struct ratbag_button_action action = {0}; - int rc; /* FIXME: range checks */ @@ -1082,31 +1153,30 @@ ratbag_button_set_key(struct ratbag_button *button, action.type = RATBAG_BUTTON_ACTION_TYPE_KEY; action.action.key.key = key; - rc = button->profile->device->driver->write_button(button, &action); - if (rc == 0) - button->action = action; + button->action = action; + button->dirty = true; + button->profile->dirty = true; - return rc == 0 ? RATBAG_SUCCESS : RATBAG_ERROR_DEVICE; + return RATBAG_SUCCESS; } LIBRATBAG_EXPORT enum ratbag_error_code ratbag_button_disable(struct ratbag_button *button) { struct ratbag_button_action action; - int rc; if (!ratbag_device_has_capability(button->profile->device, RATBAG_DEVICE_CAP_BUTTON_KEY)) return RATBAG_ERROR_CAPABILITY; action.type = RATBAG_BUTTON_ACTION_TYPE_NONE; - rc = button->profile->device->driver->write_button(button, &action); - if (rc == 0) - button->action = action; + button->action = action; + button->dirty = true; + button->profile->dirty = true; - return rc == 0 ? RATBAG_SUCCESS : RATBAG_ERROR_DEVICE; + return RATBAG_SUCCESS; } LIBRATBAG_EXPORT struct ratbag_button * @@ -1245,17 +1315,15 @@ LIBRATBAG_EXPORT enum ratbag_error_code ratbag_button_set_macro(struct ratbag_button *button, const struct ratbag_button_macro *macro) { - int rc; - if (!ratbag_device_has_capability(button->profile->device, RATBAG_DEVICE_CAP_BUTTON_MACROS)) return RATBAG_ERROR_CAPABILITY; ratbag_button_copy_macro(button, macro); + button->dirty = true; + button->profile->dirty = true; - rc = button->profile->device->driver->write_button(button, &button->action); - - return rc == 0 ? RATBAG_SUCCESS : RATBAG_ERROR_DEVICE; + return RATBAG_SUCCESS; } LIBRATBAG_EXPORT enum ratbag_error_code diff --git a/src/libratbag.h b/src/libratbag.h index 21f4b122..d04a7d5e 100644 --- a/src/libratbag.h +++ b/src/libratbag.h @@ -537,6 +537,18 @@ int ratbag_device_has_capability(const struct ratbag_device *device, enum ratbag_device_capability cap); +/** + * @ingroup device + * + * Write any changes to the device. Depending on the device, this may take + * a couple of seconds. + * + * @param device A previously initialized ratbag device + * @return 0 on success or an error code otherwise + */ +enum ratbag_error_code +ratbag_device_commit(struct ratbag_device *device); + /** * @ingroup device * diff --git a/src/libratbag.sym b/src/libratbag.sym index b076bae8..8fd781a8 100644 --- a/src/libratbag.sym +++ b/src/libratbag.sym @@ -1,6 +1,6 @@ /* in alphabetical order! */ -LIBRATBAG_0.3.0 { +LIBRATBAG_0.5.0 { global: ratbag_create_context; ratbag_button_disable; @@ -28,6 +28,7 @@ global: ratbag_button_set_special; ratbag_button_set_user_data; ratbag_button_unref; + ratbag_device_commit; ratbag_device_get_name; ratbag_device_get_num_buttons; ratbag_device_get_num_profiles; diff --git a/tools/ratbag-command.c b/tools/ratbag-command.c index f780bb42..25d9b988 100644 --- a/tools/ratbag-command.c +++ b/tools/ratbag-command.c @@ -1701,6 +1701,10 @@ main(int argc, char **argv) ratbag, &options, argc, argv); + + if (options.device) + rc = ratbag_device_commit(options.device); + out: ratbag_resolution_unref(options.resolution); ratbag_button_unref(options.button);