param: implement rate-limited autosave

- add a saving delay of 300ms
- save at most once every 2 seconds

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

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;

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 */

src/modules/systemlib/param/param.c

@@ -45,7 +45,6 @@
 #include <px4_defines.h>
 #include <px4_posix.h>
 #include <px4_config.h>
-#include <px4_spi.h>
 #include <string.h>
 #include <stdbool.h>
 #include <float.h>
@@ -94,6 +93,13 @@
 #define PARAM_CLOSE	close
 #endif
 
+#include <px4_workqueue.h>
+/* 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);
+	return param_set_internal(param, val, false, true);
-}
-
-int
-param_set_no_autosave(param_t param, const void *val)
-{
-	return param_set_internal(param, val, false, true, 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);
 }
-
+static void
-void
+param_reset_all_internal(bool auto_save)
-param_reset_all(void)
 {
 	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);
 }
 

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.
  *

src/modules/systemlib/param/param_shmem.c

@@ -79,6 +79,13 @@
 #endif
 #define PARAM_CLOSE	close
 
+#include <px4_workqueue.h>
+/* 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);
+	return param_set_internal(param, val, false, true);
-}
-
-int
-param_set_no_autosave(param_t param, const void *val)
-{
-	return param_set_internal(param, val, false, true, 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);
 }
-
+static void
-void
+param_reset_all_internal(bool auto_save)
-param_reset_all(void)
 {
 	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);
 }
 

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