From dc4d5619eae2bb6eebfe2f11ee97f5734a35d731 Mon Sep 17 00:00:00 2001 From: David Sidrane Date: Thu, 14 May 2015 12:36:40 -1000 Subject: [PATCH 1/9] Reduced the amount of memory used by params to only that that is needed Conflicts: src/modules/systemlib/param/param.c --- src/modules/systemlib/param/param.c | 42 ++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 12 deletions(-) diff --git a/src/modules/systemlib/param/param.c b/src/modules/systemlib/param/param.c index 4ec885ab3e..544eeaa39d 100644 --- a/src/modules/systemlib/param/param.c +++ b/src/modules/systemlib/param/param.c @@ -57,6 +57,7 @@ #include "systemlib/param/param.h" #include "systemlib/uthash/utarray.h" #include "systemlib/bson/tinybson.h" +#include #include "uORB/uORB.h" #include "uORB/topics/parameter_update.h" @@ -91,8 +92,23 @@ struct param_wbuf_s { bool unsaved; }; -// XXX this should be param_info_count, but need to work out linking -uint8_t param_changed_storage[(700 / sizeof(uint8_t)) + 1] = {}; + +uint8_t *param_changed_storage = 0; +int size_param_changed_storage_bytes = 0; +const int bits_per_allocation_unit = (sizeof(*param_changed_storage) * 8); + + +static unsigned +get_param_info_count(void) +{ + // Singleton + if (!param_changed_storage) + { + size_param_changed_storage_bytes = (param_info_count / bits_per_allocation_unit ) + 1; + param_changed_storage = zalloc(size_param_changed_storage_bytes); + } + return param_info_count; +} /** flexible array holding modified parameter values */ UT_array *param_values; @@ -140,7 +156,7 @@ param_assert_locked(void) static bool handle_in_range(param_t param) { - return (param < param_info_count); + return (param < get_param_info_count()); } /** @@ -245,15 +261,17 @@ param_find_no_notification(const char *name) unsigned param_count(void) { - return param_info_count; + return get_param_info_count(); } unsigned param_count_used(void) { + // ensure the allocation has been done + get_param_info_count(); unsigned count = 0; - for (unsigned i = 0; i < sizeof(param_changed_storage) / sizeof(param_changed_storage[0]); i++) { - for (unsigned j = 0; j < 8; j++) { + for (unsigned i = 0; i < size_param_changed_storage_bytes; i++) { + for (unsigned j = 0; j < bits_per_allocation_unit; j++) { if (param_changed_storage[i] & (1 << j)) { count++; } @@ -265,7 +283,7 @@ param_count_used(void) param_t param_for_index(unsigned index) { - if (index < param_info_count) { + if (index < get_param_info_count()) return (param_t)index; } @@ -323,7 +341,7 @@ param_get_used_index(param_t param) int count = 0; for (unsigned i = 0; i < (unsigned)param + 1; i++) { - for (unsigned j = 0; j < 8; j++) { + for (unsigned j = 0; j < bits_per_allocation_unit; j++) { if (param_changed_storage[i] & (1 << j)) { if (param_storage_index == i) { @@ -559,8 +577,8 @@ param_used(param_t param) return false; } - unsigned bitindex = param_index - (param_index / sizeof(param_changed_storage[0])); - return param_changed_storage[param_index / sizeof(param_changed_storage[0])] & (1 << bitindex); + unsigned bitindex = param_index - (param_index / bits_per_allocation_unit); + return param_changed_storage[param_index / bits_per_allocation_unit] & (1 << bitindex); } void param_set_used_internal(param_t param) @@ -570,8 +588,8 @@ void param_set_used_internal(param_t param) return; } - unsigned bitindex = param_index - (param_index / sizeof(param_changed_storage[0])); - param_changed_storage[param_index / sizeof(param_changed_storage[0])] |= (1 << bitindex); + unsigned bitindex = param_index - (param_index / bits_per_allocation_unit); + param_changed_storage[param_index / bits_per_allocation_unit] |= (1 << bitindex); } int From 8e9fdc61473f851f8747f379353bd6725e173720 Mon Sep 17 00:00:00 2001 From: David Sidrane Date: Thu, 14 May 2015 16:44:55 -1000 Subject: [PATCH 2/9] Use stdlib's calloc for compaiblity --- src/modules/systemlib/param/param.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/modules/systemlib/param/param.c b/src/modules/systemlib/param/param.c index 544eeaa39d..26568266e7 100644 --- a/src/modules/systemlib/param/param.c +++ b/src/modules/systemlib/param/param.c @@ -44,6 +44,7 @@ #include #include #include +#include #include #include #include @@ -105,7 +106,7 @@ get_param_info_count(void) if (!param_changed_storage) { size_param_changed_storage_bytes = (param_info_count / bits_per_allocation_unit ) + 1; - param_changed_storage = zalloc(size_param_changed_storage_bytes); + param_changed_storage = calloc(size_param_changed_storage_bytes, 1); } return param_info_count; } From 6667e6e078e305df1b739dd70876423aa0455e7f Mon Sep 17 00:00:00 2001 From: David Sidrane Date: Thu, 14 May 2015 18:03:23 -1000 Subject: [PATCH 3/9] Update param.c Not used px4_macros.h anyway --- src/modules/systemlib/param/param.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/modules/systemlib/param/param.c b/src/modules/systemlib/param/param.c index 26568266e7..010f40c883 100644 --- a/src/modules/systemlib/param/param.c +++ b/src/modules/systemlib/param/param.c @@ -58,7 +58,6 @@ #include "systemlib/param/param.h" #include "systemlib/uthash/utarray.h" #include "systemlib/bson/tinybson.h" -#include #include "uORB/uORB.h" #include "uORB/topics/parameter_update.h" From a0af91d05cd801852f08041ddeffa881ae2cd1fb Mon Sep 17 00:00:00 2001 From: David Sidrane Date: Thu, 14 May 2015 18:15:10 -1000 Subject: [PATCH 4/9] Missing Brace --- src/modules/systemlib/param/param.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/modules/systemlib/param/param.c b/src/modules/systemlib/param/param.c index 010f40c883..14e7299e60 100644 --- a/src/modules/systemlib/param/param.c +++ b/src/modules/systemlib/param/param.c @@ -283,7 +283,7 @@ param_count_used(void) param_t param_for_index(unsigned index) { - if (index < get_param_info_count()) + if (index < get_param_info_count()) { return (param_t)index; } From 535eb7dbd954cf8eae984d5b347568f966e5dde8 Mon Sep 17 00:00:00 2001 From: Lorenz Meier Date: Fri, 15 May 2015 09:11:28 +0200 Subject: [PATCH 5/9] param lib: Fix use of array size --- src/modules/systemlib/param/param.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/modules/systemlib/param/param.c b/src/modules/systemlib/param/param.c index 14e7299e60..e86f09ffae 100644 --- a/src/modules/systemlib/param/param.c +++ b/src/modules/systemlib/param/param.c @@ -298,7 +298,7 @@ param_for_used_index(unsigned index) /* walk all params and count */ int count = 0; - for (unsigned i = 0; i < (unsigned)param_info_count + 1; i++) { + for (unsigned i = 0; i < (unsigned)size_param_changed_storage_bytes; i++) { for (unsigned j = 0; j < 8; j++) { if (param_changed_storage[i] & (1 << j)) { From c6bc3153efcd345d411695e449c1a1475f84655a Mon Sep 17 00:00:00 2001 From: David Sidrane Date: Fri, 15 May 2015 03:58:04 -1000 Subject: [PATCH 6/9] Reviewd - fixed indexing that was wrong, code clean up ran astyle --- src/modules/systemlib/param/param.c | 126 +++++++++++++++------------- 1 file changed, 70 insertions(+), 56 deletions(-) diff --git a/src/modules/systemlib/param/param.c b/src/modules/systemlib/param/param.c index e86f09ffae..6de03125ca 100644 --- a/src/modules/systemlib/param/param.c +++ b/src/modules/systemlib/param/param.c @@ -72,13 +72,13 @@ * Array of static parameter info. */ #ifdef _UNIT_TEST - extern struct param_info_s param_array[]; - extern struct param_info_s *param_info_base; - extern struct param_info_s *param_info_limit; +extern struct param_info_s param_array[]; +extern struct param_info_s *param_info_base; +extern struct param_info_s *param_info_limit; #else - extern char __param_start, __param_end; - static const struct param_info_s *param_info_base = (struct param_info_s *) &__param_start; - static const struct param_info_s *param_info_limit = (struct param_info_s *) &__param_end; +extern char __param_start, __param_end; +static const struct param_info_s *param_info_base = (struct param_info_s *) &__param_start; +static const struct param_info_s *param_info_limit = (struct param_info_s *) &__param_end; #endif #define param_info_count ((unsigned)(param_info_limit - param_info_base)) @@ -101,13 +101,13 @@ const int bits_per_allocation_unit = (sizeof(*param_changed_storage) * 8); static unsigned get_param_info_count(void) { - // Singleton - if (!param_changed_storage) - { - size_param_changed_storage_bytes = (param_info_count / bits_per_allocation_unit ) + 1; - param_changed_storage = calloc(size_param_changed_storage_bytes, 1); - } - return param_info_count; + /* Singleton creation of and array of bits to track changed values */ + if (!param_changed_storage) { + size_param_changed_storage_bytes = (param_info_count / bits_per_allocation_unit) + 1; + param_changed_storage = calloc(size_param_changed_storage_bytes, 1); + } + + return param_info_count; } /** flexible array holding modified parameter values */ @@ -170,11 +170,13 @@ param_compare_values(const void *a, const void *b) struct param_wbuf_s *pa = (struct param_wbuf_s *)a; struct param_wbuf_s *pb = (struct param_wbuf_s *)b; - if (pa->param < pb->param) + if (pa->param < pb->param) { return -1; + } - if (pa->param > pb->param) + if (pa->param > pb->param) { return 1; + } return 0; } @@ -187,7 +189,8 @@ param_compare_values(const void *a, const void *b) * NULL if the parameter has not been modified. */ static struct param_wbuf_s * -param_find_changed(param_t param) { +param_find_changed(param_t param) +{ struct param_wbuf_s *s = NULL; param_assert_locked(); @@ -200,8 +203,9 @@ param_find_changed(param_t param) { #else while ((s = (struct param_wbuf_s *)utarray_next(param_values, s)) != NULL) { - if (s->param == param) + if (s->param == param) { break; + } } #endif @@ -238,6 +242,7 @@ param_find_internal(const char *name, bool notification) if (notification) { param_set_used_internal(param); } + return param; } } @@ -267,9 +272,10 @@ param_count(void) unsigned param_count_used(void) { - // ensure the allocation has been done - get_param_info_count(); + // ensure the allocation has been done + get_param_info_count(); unsigned count = 0; + for (unsigned i = 0; i < size_param_changed_storage_bytes; i++) { for (unsigned j = 0; j < bits_per_allocation_unit; j++) { if (param_changed_storage[i] & (1 << j)) { @@ -277,6 +283,7 @@ param_count_used(void) } } } + return count; } @@ -359,10 +366,7 @@ param_get_used_index(param_t param) const char * param_name(param_t param) { - if (handle_in_range(param)) - return param_info_base[param].name; - - return NULL; + return handle_in_range(param) ? param_info_base[param].name : NULL;; } bool @@ -375,29 +379,22 @@ bool param_value_unsaved(param_t param) { static struct param_wbuf_s *s; - s = param_find_changed(param); - - if (s && s->unsaved) - return true; - - return false; + return (s && s->unsaved) ? true : false; } enum param_type_e -param_type(param_t param) -{ - if (handle_in_range(param)) - return param_info_base[param].type; - - return PARAM_TYPE_UNKNOWN; +param_type(param_t param) { + return handle_in_range(param) ? param_info_base[param].type : PARAM_TYPE_UNKNOWN; } size_t param_size(param_t param) { if (handle_in_range(param)) { + switch (param_type(param)) { + case PARAM_TYPE_INT32: case PARAM_TYPE_FLOAT: return 4; @@ -442,8 +439,9 @@ param_get_value_ptr(param_t param) v = ¶m_info_base[param].val; } - if (param_type(param) >= PARAM_TYPE_STRUCT - && param_type(param) <= PARAM_TYPE_STRUCT_MAX) { + if (param_type(param) >= PARAM_TYPE_STRUCT && + param_type(param) <= PARAM_TYPE_STRUCT_MAX) { + result = v->p; } else { @@ -481,8 +479,9 @@ param_set_internal(param_t param, const void *val, bool mark_saved, bool notify_ param_lock(); - if (param_values == NULL) + if (param_values == NULL) { utarray_new(param_values, ¶m_icd); + } if (param_values == NULL) { debug("failed to allocate modified values array"); @@ -512,6 +511,7 @@ param_set_internal(param_t param, const void *val, bool mark_saved, bool notify_ /* update the changed value */ switch (param_type(param)) { + case PARAM_TYPE_INT32: s->val.i = *(int32_t *)val; break; @@ -551,8 +551,9 @@ out: * If we set something, now that we have unlocked, go ahead and advertise that * a thing has been set. */ - if (params_changed && notify_changes) + if (params_changed && notify_changes) { param_notify_changes(); + } return result; } @@ -573,23 +574,25 @@ bool param_used(param_t param) { int param_index = param_get_index(param); + if (param_index < 0) { return false; } - unsigned bitindex = param_index - (param_index / bits_per_allocation_unit); - return param_changed_storage[param_index / bits_per_allocation_unit] & (1 << bitindex); + return param_changed_storage[param_index / bits_per_allocation_unit] & + (1 << param_index % bits_per_allocation_unit); } void param_set_used_internal(param_t param) { int param_index = param_get_index(param); + if (param_index < 0) { return; } - unsigned bitindex = param_index - (param_index / bits_per_allocation_unit); - param_changed_storage[param_index / bits_per_allocation_unit] |= (1 << bitindex); + param_changed_storage[param_index / bits_per_allocation_unit] |= + (1 << param_index % bits_per_allocation_unit); } int @@ -616,8 +619,9 @@ param_reset(param_t param) param_unlock(); - if (s != NULL) + if (s != NULL) { param_notify_changes(); + } return (!param_found); } @@ -640,28 +644,28 @@ param_reset_all(void) } void -param_reset_excludes(const char* excludes[], int num_excludes) +param_reset_excludes(const char *excludes[], int num_excludes) { param_lock(); param_t param; for (param = 0; handle_in_range(param); param++) { - const char* name = param_name(param); + const char *name = param_name(param); bool exclude = false; for (int index = 0; index < num_excludes; index ++) { int len = strlen(excludes[index]); - if((excludes[index][len - 1] == '*' - && strncmp(name, excludes[index], len - 1) == 0) - || strcmp(name, excludes[index]) == 0) { + if ((excludes[index][len - 1] == '*' + && strncmp(name, excludes[index], len - 1) == 0) + || strcmp(name, excludes[index]) == 0) { exclude = true; break; } } - if(!exclude) { + if (!exclude) { param_reset(param); } } @@ -675,14 +679,17 @@ static const char *param_default_file = "/eeprom/parameters"; static char *param_user_file = NULL; int -param_set_default_file(const char* filename) +param_set_default_file(const char *filename) { if (param_user_file != NULL) { free(param_user_file); param_user_file = NULL; } - if (filename) + + if (filename) { param_user_file = strdup(filename); + } + return 0; } @@ -733,6 +740,7 @@ param_load_default(void) warn("open '%s' for reading failed", param_get_default_file()); return -1; } + return 1; } @@ -773,13 +781,16 @@ param_export(int fd, bool only_unsaved) * If we are only saving values changed since last save, and this * one hasn't, then skip it */ - if (only_unsaved && !s->unsaved) + if (only_unsaved && !s->unsaved) { continue; + } s->unsaved = false; /* append the appropriate BSON type object */ + switch (param_type(s->param)) { + case PARAM_TYPE_INT32: param_get(s->param, &i); @@ -823,8 +834,9 @@ param_export(int fd, bool only_unsaved) out: param_unlock(); - if (result == 0) + if (result == 0) { result = bson_encoder_fini(&encoder); + } return result; } @@ -934,8 +946,9 @@ param_import_callback(bson_decoder_t decoder, void *private, bson_node_t node) out: - if (tmp != NULL) + if (tmp != NULL) { free(tmp); + } return result; } @@ -961,8 +974,9 @@ param_import_internal(int fd, bool mark_saved) out: - if (result < 0) + if (result < 0) { debug("BSON error decoding parameters"); + } return result; } From e279e8bb2a87e56d5356fc255d800516c8094cc5 Mon Sep 17 00:00:00 2001 From: Lorenz Meier Date: Sun, 17 May 2015 22:58:52 +0200 Subject: [PATCH 7/9] Fix param changed count logic, speed up logic for unused params --- src/modules/systemlib/param/param.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/modules/systemlib/param/param.c b/src/modules/systemlib/param/param.c index 6de03125ca..f2d5f81b16 100644 --- a/src/modules/systemlib/param/param.c +++ b/src/modules/systemlib/param/param.c @@ -338,20 +338,19 @@ param_get_index(param_t param) int param_get_used_index(param_t param) { - int param_storage_index = param_get_index(param); - - if (param_storage_index < 0) { + /* this tests for out of bounds and does a constant time lookup */ + if (!param_used(param)) { return -1; } - /* walk all params and count */ + /* walk all params and count, now knowing that it has a valid index */ int count = 0; - for (unsigned i = 0; i < (unsigned)param + 1; i++) { - for (unsigned j = 0; j < bits_per_allocation_unit; j++) { + for (unsigned i = 0; i < (unsigned)size_param_changed_storage_bytes; i++) { + for (unsigned j = 0; j < 8; j++) { if (param_changed_storage[i] & (1 << j)) { - if (param_storage_index == i) { + if ((unsigned)param == i * 8 + j) { return count; } From ff4be81976dfe178ed4ddc3ea12c23c00f1dcc18 Mon Sep 17 00:00:00 2001 From: Lorenz Meier Date: Sun, 17 May 2015 23:05:32 +0200 Subject: [PATCH 8/9] Param: do not set a param as used just because its value is non-default. --- src/modules/systemlib/param/param.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/modules/systemlib/param/param.c b/src/modules/systemlib/param/param.c index f2d5f81b16..26696dc0d0 100644 --- a/src/modules/systemlib/param/param.c +++ b/src/modules/systemlib/param/param.c @@ -542,8 +542,6 @@ param_set_internal(param_t param, const void *val, bool mark_saved, bool notify_ } out: - param_set_used_internal(param); - param_unlock(); /* From 2f5e27c18013c1e8c96c4e2266c3a63f774c5962 Mon Sep 17 00:00:00 2001 From: Lorenz Meier Date: Sun, 17 May 2015 23:08:10 +0200 Subject: [PATCH 9/9] param lib: Fix code style --- src/modules/systemlib/param/param.c | 2 +- src/modules/systemlib/param/param.h | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/modules/systemlib/param/param.c b/src/modules/systemlib/param/param.c index 26696dc0d0..910f9da07f 100644 --- a/src/modules/systemlib/param/param.c +++ b/src/modules/systemlib/param/param.c @@ -365,7 +365,7 @@ param_get_used_index(param_t param) const char * param_name(param_t param) { - return handle_in_range(param) ? param_info_base[param].name : NULL;; + return handle_in_range(param) ? param_info_base[param].name : NULL; } bool diff --git a/src/modules/systemlib/param/param.h b/src/modules/systemlib/param/param.h index 9cbe3570b6..07c6dd6bf2 100644 --- a/src/modules/systemlib/param/param.h +++ b/src/modules/systemlib/param/param.h @@ -249,7 +249,7 @@ __EXPORT void param_reset_all(void); * at the end to exclude parameters with a certain prefix. * @param num_excludes The number of excludes provided. */ - __EXPORT void param_reset_excludes(const char* excludes[], int num_excludes); +__EXPORT void param_reset_excludes(const char *excludes[], int num_excludes); /** * Export changed parameters to a file. @@ -306,16 +306,16 @@ __EXPORT void param_foreach(void (*func)(void *arg, param_t param), void *arg, * exist. * @return Zero on success. */ -__EXPORT int param_set_default_file(const char* filename); +__EXPORT int param_set_default_file(const char *filename); /** * Get the default parameter file name. * * @return The path to the current default parameter file; either as - * a result of a call to param_set_default_file, or the + * a result of a call to param_set_default_file, or the * built-in default. */ -__EXPORT const char* param_get_default_file(void); +__EXPORT const char *param_get_default_file(void); /** * Save parameters to the default file.