From a5cdff06d5630a769e9f6b1306ca16fc10f0857c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beat=20K=C3=BCng?= Date: Fri, 24 Mar 2017 16:59:51 +0100 Subject: [PATCH] param: implement rate-limited autosave - add a saving delay of 300ms - save at most once every 2 seconds --- msg/parameter_update.msg | 4 +- .../systemlib/flashparams/flashparams.c | 2 +- .../systemlib/flashparams/flashparams.h | 2 +- src/modules/systemlib/param/param.c | 117 ++++++++++++++---- src/modules/systemlib/param/param.h | 10 -- src/modules/systemlib/param/param_shmem.c | 117 ++++++++++++++---- src/systemcmds/param/param.c | 4 +- 7 files changed, 194 insertions(+), 62 deletions(-) diff --git a/msg/parameter_update.msg b/msg/parameter_update.msg index 3fa04a7657..0fc1e74370 100644 --- a/msg/parameter_update.msg +++ b/msg/parameter_update.msg @@ -1 +1,3 @@ -bool saved # wether the change has already been saved to disk +# This message is used to notify the system about one or more parameter changes + +uint32 dummy # unused diff --git a/src/modules/systemlib/flashparams/flashparams.c b/src/modules/systemlib/flashparams/flashparams.c index 6f32bb2731..bea2327656 100644 --- a/src/modules/systemlib/flashparams/flashparams.c +++ b/src/modules/systemlib/flashparams/flashparams.c @@ -281,7 +281,7 @@ param_import_callback(bson_decoder_t decoder, void *private, bson_node_t node) goto out; } - if (param_set_external(param, v, state->mark_saved, true, false)) { + if (param_set_external(param, v, state->mark_saved, true)) { debug("error setting value for '%s'", node->name); goto out; diff --git a/src/modules/systemlib/flashparams/flashparams.h b/src/modules/systemlib/flashparams/flashparams.h index 195a18fce7..64588057a8 100644 --- a/src/modules/systemlib/flashparams/flashparams.h +++ b/src/modules/systemlib/flashparams/flashparams.h @@ -61,7 +61,7 @@ __BEGIN_DECLS #define FLASH_PARAMS_EXPOSE __EXPORT __EXPORT extern UT_array *param_values; -__EXPORT int param_set_external(param_t param, const void *val, bool mark_saved, bool notify_changes, bool is_saved); +__EXPORT int param_set_external(param_t param, const void *val, bool mark_saved, bool notify_changes); __EXPORT const void *param_get_value_ptr_external(param_t param); /* The interface hooks to the Flash based storage. The caller is responsible for locking */ diff --git a/src/modules/systemlib/param/param.c b/src/modules/systemlib/param/param.c index 7ac9087800..0a0709cfe7 100644 --- a/src/modules/systemlib/param/param.c +++ b/src/modules/systemlib/param/param.c @@ -45,7 +45,6 @@ #include #include #include -#include #include #include #include @@ -94,6 +93,13 @@ #define PARAM_CLOSE close #endif +#include +/* autosaving variables */ +static hrt_abstime last_autosave_timestamp = 0; +struct work_s autosave_work; +static bool autosave_scheduled = false; +static bool autosave_disabled = false; + /** * Array of static parameter info. */ @@ -283,12 +289,12 @@ param_find_changed(param_t param) } static void -_param_notify_changes(bool is_saved) +_param_notify_changes(void) { #if !defined(PARAM_NO_ORB) struct parameter_update_s pup = { .timestamp = hrt_absolute_time(), - .saved = is_saved + .dummy = 0 }; /* @@ -308,7 +314,7 @@ _param_notify_changes(bool is_saved) void param_notify_changes(void) { - _param_notify_changes(false); + _param_notify_changes(); } param_t @@ -579,8 +585,62 @@ param_get(param_t param, void *val) return result; } +/** + * worker callback method to save the parameters + * @param arg unused + */ +static void autosave_worker(void *arg) +{ + bool disabled = false; + + param_lock_writer(); + last_autosave_timestamp = hrt_absolute_time(); + autosave_scheduled = false; + disabled = autosave_disabled; + param_unlock_writer(); + + if (disabled) { + return; + } + + PX4_DEBUG("Autosaving params"); + int ret = param_save_default(); + + if (ret != 0) { + PX4_ERR("param save failed (%i)", ret); + } +} + +/** + * Automatically save the parameters after a timeout and limited rate. + * + * This needs to be called with the writer lock held (it's not necessary that it's the writer lock, but it + * needs to be the same lock as autosave_worker() and param_control_autosave() use). + */ +static void param_autosave(void) +{ + if (autosave_scheduled || autosave_disabled) { + return; + } + + // wait at least 300ms before saving, because: + // - tasks often call param_set() for multiple params, so this avoids unnecessary save calls + // - the logger stores changed params. He gets notified on a param change via uORB and then + // looks at all unsaved params. + hrt_abstime delay = 300 * 1000; + + const hrt_abstime rate_limit = 2000 * 1000; // rate-limit saving to 2 seconds + hrt_abstime last_save_elapsed = hrt_elapsed_time(&last_autosave_timestamp); + + if (last_save_elapsed < rate_limit && rate_limit > last_save_elapsed + delay) { + delay = rate_limit - last_save_elapsed; + } + + autosave_scheduled = true; + work_queue(LPWORK, &autosave_work, (worker_t)&autosave_worker, NULL, USEC2TICK(delay)); +} static int -param_set_internal(param_t param, const void *val, bool mark_saved, bool notify_changes, bool is_saved) +param_set_internal(param_t param, const void *val, bool mark_saved, bool notify_changes) { int result = -1; bool params_changed = false; @@ -651,6 +711,10 @@ param_set_internal(param_t param, const void *val, bool mark_saved, bool notify_ s->unsaved = !mark_saved; result = 0; + + if (!mark_saved) { // this is false when importing parameters + param_autosave(); + } } out: @@ -661,16 +725,16 @@ out: * a thing has been set. */ if (params_changed && notify_changes) { - _param_notify_changes(is_saved); + _param_notify_changes(); } return result; } #if defined(FLASH_BASED_PARAMS) -int param_set_external(param_t param, const void *val, bool mark_saved, bool notify_changes, bool is_saved) +int param_set_external(param_t param, const void *val, bool mark_saved, bool notify_changes) { - return param_set_internal(param, val, mark_saved, notify_changes, is_saved); + return param_set_internal(param, val, mark_saved, notify_changes); } const void *param_get_value_ptr_external(param_t param) @@ -682,19 +746,13 @@ const void *param_get_value_ptr_external(param_t param) int param_set(param_t param, const void *val) { - return param_set_internal(param, val, false, true, false); -} - -int -param_set_no_autosave(param_t param, const void *val) -{ - return param_set_internal(param, val, false, true, true); + return param_set_internal(param, val, false, true); } int param_set_no_notification(param_t param, const void *val) { - return param_set_internal(param, val, false, false, false); + return param_set_internal(param, val, false, false); } bool @@ -745,17 +803,18 @@ param_reset(param_t param) param_found = true; } + param_autosave(); + param_unlock_writer(); if (s != NULL) { - _param_notify_changes(false); + _param_notify_changes(); } return (!param_found); } - -void -param_reset_all(void) +static void +param_reset_all_internal(bool auto_save) { param_lock_writer(); @@ -766,9 +825,19 @@ param_reset_all(void) /* mark as reset / deleted */ param_values = NULL; + if (auto_save) { + param_autosave(); + } + param_unlock_writer(); - _param_notify_changes(false); + _param_notify_changes(); +} + +void +param_reset_all(void) +{ + param_reset_all_internal(true); } void @@ -796,7 +865,7 @@ param_reset_excludes(const char *excludes[], int num_excludes) } } - _param_notify_changes(false); + _param_notify_changes(); } static const char *param_default_file = PX4_ROOTFSDIR"/eeprom/parameters"; @@ -1132,7 +1201,7 @@ param_import_callback(bson_decoder_t decoder, void *private, bson_node_t node) goto out; } - if (param_set_internal(param, v, state->mark_saved, true, false)) { + if (param_set_internal(param, v, state->mark_saved, true)) { debug("error setting value for '%s'", node->name); goto out; } @@ -1205,7 +1274,7 @@ param_import(int fd) int param_load(int fd) { - param_reset_all(); + param_reset_all_internal(false); return param_import_internal(fd, true); } diff --git a/src/modules/systemlib/param/param.h b/src/modules/systemlib/param/param.h index 61230aab56..09889b2ef8 100644 --- a/src/modules/systemlib/param/param.h +++ b/src/modules/systemlib/param/param.h @@ -248,16 +248,6 @@ __EXPORT int param_get(param_t param, void *val); */ __EXPORT int param_set(param_t param, const void *val); -/** - * Set the value of a parameter, but do not trigger an auto-save - * - * @param param A handle returned by param_find or passed by param_foreach. - * @param val The value to set; assumed to point to a variable of the parameter type. - * For structures, the pointer is assumed to point to a structure to be copied. - * @return Zero if the parameter's value could be set from a scalar, nonzero otherwise. - */ -__EXPORT int param_set_no_autosave(param_t param, const void *val); - /** * Set the value of a parameter, but do not notify the system about the change. * diff --git a/src/modules/systemlib/param/param_shmem.c b/src/modules/systemlib/param/param_shmem.c index 9dfe1bd0cc..ab4aecae25 100644 --- a/src/modules/systemlib/param/param_shmem.c +++ b/src/modules/systemlib/param/param_shmem.c @@ -79,6 +79,13 @@ #endif #define PARAM_CLOSE close +#include +/* autosaving variables */ +static hrt_abstime last_autosave_timestamp = 0; +struct work_s autosave_work; +static bool autosave_scheduled = false; +static bool autosave_disabled = false; + /** * Array of static parameter info. */ @@ -125,7 +132,7 @@ extern void update_to_shmem(param_t param, union param_value_u value); extern int update_from_shmem(param_t param, union param_value_u *value); extern void update_index_from_shmem(void); -static int param_set_internal(param_t param, const void *val, bool mark_saved, bool notify_changes, bool is_saved); +static int param_set_internal(param_t param, const void *val, bool mark_saved, bool notify_changes); unsigned char set_called_from_get = 0; static int param_import_done = @@ -259,9 +266,9 @@ param_find_changed(param_t param) } static void -_param_notify_changes(bool is_saved) +_param_notify_changes(void) { - struct parameter_update_s pup = { .timestamp = hrt_absolute_time(), .saved = is_saved }; + struct parameter_update_s pup = { .timestamp = hrt_absolute_time(), .dummy = 0 }; /* * If we don't have a handle to our topic, create one now; otherwise @@ -278,7 +285,7 @@ _param_notify_changes(bool is_saved) void param_notify_changes(void) { - _param_notify_changes(true); + _param_notify_changes(); } @@ -545,7 +552,7 @@ param_get(param_t param, void *val) if (update_from_shmem(param, &value)) { set_called_from_get = 1; - param_set_internal(param, &value, true, false, false); + param_set_internal(param, &value, true, false); set_called_from_get = 0; } @@ -578,8 +585,63 @@ param_get(param_t param, void *val) return result; } + +/** + * worker callback method to save the parameters + * @param arg unused + */ +static void autosave_worker(void *arg) +{ + bool disabled = false; + + param_lock(); + last_autosave_timestamp = hrt_absolute_time(); + autosave_scheduled = false; + disabled = autosave_disabled; + param_unlock(); + + if (disabled) { + return; + } + + PX4_DEBUG("Autosaving params"); + int ret = param_save_default(); + + if (ret != 0) { + PX4_ERR("param save failed (%i)", ret); + } +} + +/** + * Automatically save the parameters after a timeout and limited rate. + * + * This needs to be called with the writer lock held (it's not necessary that it's the writer lock, but it + * needs to be the same lock as autosave_worker() and param_control_autosave() use). + */ +static void param_autosave(void) +{ + if (autosave_scheduled || autosave_disabled) { + return; + } + + // wait at least 300ms before saving, because: + // - tasks often call param_set() for multiple params, so this avoids unnecessary save calls + // - the logger stores changed params. He gets notified on a param change via uORB and then + // looks at all unsaved params. + hrt_abstime delay = 300 * 1000; + + const hrt_abstime rate_limit = 2000 * 1000; // rate-limit saving to 2 seconds + hrt_abstime last_save_elapsed = hrt_elapsed_time(&last_autosave_timestamp); + + if (last_save_elapsed < rate_limit && rate_limit > last_save_elapsed + delay) { + delay = rate_limit - last_save_elapsed; + } + + autosave_scheduled = true; + work_queue(LPWORK, &autosave_work, (worker_t)&autosave_worker, NULL, USEC2TICK(delay)); +} static int -param_set_internal(param_t param, const void *val, bool mark_saved, bool notify_changes, bool is_saved) +param_set_internal(param_t param, const void *val, bool mark_saved, bool notify_changes) { int result = -1; bool params_changed = false; @@ -656,6 +718,10 @@ param_set_internal(param_t param, const void *val, bool mark_saved, bool notify_ s->unsaved = !mark_saved; params_changed = true; result = 0; + + if (!mark_saved) { // this is false when importing parameters + param_autosave(); + } } out: @@ -669,7 +735,7 @@ out: if (!param_import_done) { notify_changes = 0; } if (params_changed && notify_changes) { - _param_notify_changes(is_saved); + _param_notify_changes(); } if (result == 0 && !set_called_from_get) { @@ -698,19 +764,13 @@ out: int param_set(param_t param, const void *val) { - return param_set_internal(param, val, false, true, false); -} - -int -param_set_no_autosave(param_t param, const void *val) -{ - return param_set_internal(param, val, false, true, true); + return param_set_internal(param, val, false, true); } int param_set_no_notification(param_t param, const void *val) { - return param_set_internal(param, val, false, false, false); + return param_set_internal(param, val, false, false); } bool @@ -763,17 +823,18 @@ param_reset(param_t param) param_found = true; } + param_autosave(); + param_unlock(); if (s != NULL) { - _param_notify_changes(false); + _param_notify_changes(); } return (!param_found); } - -void -param_reset_all(void) +static void +param_reset_all_internal(bool auto_save) { param_lock(); @@ -784,9 +845,19 @@ param_reset_all(void) /* mark as reset / deleted */ param_values = NULL; + if (auto_save) { + param_autosave(); + } + param_unlock(); - _param_notify_changes(false); + _param_notify_changes(); +} + +void +param_reset_all(void) +{ + param_reset_all_internal(true); } void @@ -814,7 +885,7 @@ param_reset_excludes(const char *excludes[], int num_excludes) } } - _param_notify_changes(false); + _param_notify_changes(); } #ifdef __PX4_QURT @@ -1145,7 +1216,7 @@ param_import_callback(bson_decoder_t decoder, void *private, bson_node_t node) goto out; } - if (param_set_internal(param, v, state->mark_saved, true, false)) { + if (param_set_internal(param, v, state->mark_saved, true)) { PX4_DEBUG("error setting value for '%s'", node->name); goto out; } @@ -1204,7 +1275,7 @@ param_import(int fd) int param_load(int fd) { - param_reset_all(); + param_reset_all_internal(false); return param_import_internal(fd, true); } diff --git a/src/systemcmds/param/param.c b/src/systemcmds/param/param.c index 934fb48043..75065121f2 100644 --- a/src/systemcmds/param/param.c +++ b/src/systemcmds/param/param.c @@ -531,7 +531,7 @@ do_set(const char *name, const char *val, bool fail_on_not_found) param_value_unsaved(param) ? '*' : (param_value_is_default(param) ? ' ' : '+'), param_name(param)); PARAM_PRINT("curr: %ld", (long)i); - param_set_no_autosave(param, &newval); + param_set(param, &newval); PARAM_PRINT(" -> new: %ld\n", (long)newval); } } @@ -553,7 +553,7 @@ do_set(const char *name, const char *val, bool fail_on_not_found) param_value_unsaved(param) ? '*' : (param_value_is_default(param) ? ' ' : '+'), param_name(param)); PARAM_PRINT("curr: %4.4f", (double)f); - param_set_no_autosave(param, &newval); + param_set(param, &newval); PARAM_PRINT(" -> new: %4.4f\n", (double)newval); }