paramSetFloat() seems to be broken for persistent parameters

[ntamas]

It seems like paramSetFloat() does not work when it is invoked for persistent parameters because the (params[varid.index].type & (~PARAM_CORE)) == PARAM_FLOAT condition does not hold if the PARAM_EXTENDED bit is set.

I worked around it with the following patch, but it's unclear to me whether this is the right thing to do as this would happily attempt to set an int parameter with a float value bit-by-bit; on the other hand, paramSetInt() does the same thing the opposite way (allowing the user to set a float parameter with an int value, again bit-by-bit). Furthermore, both variants allow the user to overwrite parameters marked as read only.

--- a/src/modules/src/param_logic.c
+++ b/src/modules/src/param_logic.c
@@ -581,14 +581,9 @@ void paramSetFloat(paramVarId_t varid, float valuef)
   pk.data[0] = MISC_VALUE_UPDATED;
   pk.data[1] = varid.id & 0xffu;
   pk.data[2] = (varid.id >> 8) & 0xffu;
-  pk.size=3;
-
-  if ((params[varid.index].type & (~PARAM_CORE)) == PARAM_FLOAT) {
-      *(float *)params[varid.index].address = valuef;
+  pk.size = 3;

-      memcpy(&pk.data[2], &valuef, 4);
-      pk.size += 4;
-  }
+  pk.size += paramSet(varid.index, (void *)&valuef);

Ultimately, it should be decided where the type and readonly checks belong; if they do not belong to paramSetInt() and paramSetFloat(), then the patch above would work, but it then allows the user to do all sorts of nasty things to parameters from within an app. If they do belong there, then the checks should be added so my patch is not adequate.

[knmcguire]

thanks @ntamas!

@krichardsson has been working on this lately but as he is on his holiday I'll have @tobbeanton look this

[ntamas]

FYI, it looks like paramGetFloat() is also broken; it mistakenly treats the parameter as integer when the PARAM_EXTENDED bit is set. A quick fix for the problem is this patch:

--- a/src/modules/src/param_logic.c
+++ b/src/modules/src/param_logic.c
@@ -543,7 +543,7 @@ float paramGetFloat(paramVarId_t varid)
 {
   ASSERT(PARAM_VARID_IS_VALID(varid));

-  if ((params[varid.index].type & (~(PARAM_CORE | PARAM_RONLY))) == PARAM_FLOAT)
+  if ((params[varid.index].type & (~(PARAM_CORE | PARAM_RONLY | PARAM_EXTENDED))) == PARAM_FLOAT)
     return *(float *)params[varid.index].address;

   return (float)paramGetInt(varid);

although at this point it would probably be more readable to use PARAM_TYPE_MASK.

[tobbeanton]

Summary, the internal param API has not been tested...

I guess the intention with splitting into Integer and Float functions is to provide a bit of safety, but I don't know the original thinking. I suggest that we fix the functionality and keep a bit of the safety. I can give it a shot.

[tobbeanton]

I now have a suggestion in #1077. It is basically only fixing the bug but still keeping the checks. More work should be done with this after the summer vacations.

[ntamas]

Quick feedback: I tested #1077 and it works great for our use-case, thanks!

[knmcguire]

Tobias is on a holiday now but good to know this fixed your issue!