Improve floating point conversions. #423

sbertin-telular · 2019-02-14T16:27:45Z

Provides an improved range of conversions and handles scientific
notation.

Updates the tests.

Ignores warning about comparing floating point numbers for equality.
This is intentionally used to detect a NaN.

Signed-off-by: Scott Bertin [email protected]

sbertin-telular · 2019-02-15T12:48:56Z

Don't merge yet. Further testing showed It is trying to print too many digits in some cases.

sbertin-telular · 2019-02-15T13:25:06Z

Should be good now.

dnav · 2019-02-19T17:10:06Z

Hi Scott,

Thing is scientific notation is not supported in LwM2M text format whereas it is in JSON formats. So could it be possible for utils_floatToText() to have a parameter enabling or disabling the scientific notation ?

From the LwM2M 1.1 TS Core, appendix C:

Float: Represented as an ASCII signed numeric representation.
For example, we use a floating point number with the significand of 6.667, a base of 10 and the exponent of -11. This represents the number 6.667e-11 in scientific notation and will be represented as "0.00000000006667" as an ASCII string.

dnav · 2019-02-19T17:13:27Z

core/utils.c

    }
-
-    if (decPart <= 1 + FLT_EPSILON)
+    else if (data > DBL_MAX )


Shouldn't this never be true ?

DBL_MAX is the largest finite double value. Infinity will be larger. Infinity needs special handling here or later loops will never terminate.

sbertin-telular · 2019-02-19T18:04:16Z

Thing is scientific notation is not supported in LwM2M text format whereas it is in JSON formats. So could it be possible for utils_floatToText() to have a parameter enabling or disabling the scientific notation ?

I forgot about text format when doing this. My previous attempt (pull request #419) would be more appropriate for text and this is better for JSON. The two approaches are very different and I think having different functions would be more appropriate. What do you think about reopening #419 and moving this approach to new functions in json.c? We could close this pull request and I'll make a new one for that.

sbertin-telular · 2019-02-21T21:14:15Z

I made it work with a parameter.

sbernard31 · 2020-11-03T15:16:29Z

As requested, #482 (comment).

We should begin to integrate current open PR in master.
Regarding @sbertin-telular , this one seems to be the first one to integrate.

@sbertin-telular, could you please rebase this without merge commit (eventually if you think this makes sense you can squash all commit in one).

Then I will integrate it in master.

sbertin-telular · 2020-11-04T13:26:33Z

Rebased and squashed.

sbernard31 · 2020-11-04T14:16:02Z

@rettichschnidi, do you approve this ?

rettichschnidi

First try :)

My findings are not substantial, and most of them clearly have their roots from the way existing code looks. This and given that the current master can not be built without failing tests, I will not insist on any of those findings getting taken care of now. I am also happy contributing some overall improvements (also addressing my findings here) once the biggest PRs (aka @sbertin-telular's LWM2M 1.1 branch) got merged.

rettichschnidi · 2020-11-04T02:02:03Z

tests/CMakeLists.txt

@@ -15,6 +15,7 @@ add_definitions(-pedantic -Wall -Wextra -Wfloat-equal -Wshadow -Wpointer-arith -

 include_directories (${WAKAAMA_SOURCES_DIR} ${SHARED_INCLUDE_DIRS})
 set_source_files_properties(${WAKAAMA_SOURCES_DIR}/senml_json.c PROPERTIES COMPILE_FLAGS -Wno-float-equal)
+set_source_files_properties(${WAKAAMA_SOURCES_DIR}/utils.c PROPERTIES COMPILE_FLAGS -Wno-float-equal)


Given that Wakaama is either based on C99 or C11 (does not compile with C90), I'd use isnan(data) instead of data != data (and #include <math.h>) in utils.c instead of disabling this warning.

(The line above, not changed in this PR, has the same issue)

Not compiling for C90 might be a bug. See #232.

Created #494 to start a discussion about this topic.

Eventually and If you both agree we can let's this point aside for this PR.
I guess maybe #494 could be long discussion and I would not to block the PRs integration for this.
I feel that integrating all the pending work(PRs) is still the priority.

Thinking about this more I remove my objection. Things like uint8_t are already not part of C90 and are used everywhere.

Disabling the warning for senml_json.c is still necessary as there is an explicit check for equality to 0.

rettichschnidi · 2020-11-04T20:29:48Z

core/json.c

@@ -898,7 +901,7 @@ static int prv_serializeValue(lwm2m_data_t * tlvP,
        return -1;
    }

-    return head;
+    return (int)head;


I'd rather have the return size changed to ssize_t, but given the amount of changes this implies, including interface breaking changes, i think I am very fine doing this another time.

rettichschnidi · 2020-11-04T20:36:33Z

core/senml_json.c

        if (!res) return -1;
+        /* Error if inf or nan */
+        if (buffer[head] != '-' && (buffer[head] < '0' || buffer[head] > '9')) return -1;


Suggested change

if (buffer[head] != '-' && (buffer[head] < '0' || buffer[head] > '9')) return -1;

if (buffer[head] != '-' && !isdigit(buffer[head])) return -1;

Plus #include <ctype.h> on top of the file

rettichschnidi · 2020-11-04T20:37:12Z

core/senml_json.c

        if (!res) return -1;
+        /* Error if inf or nan */
+        if (buffer[head] != '-' && (buffer[head] < '0' || buffer[head] > '9')) return -1;
+        if (res > 1 && buffer[head] == '-' && (buffer[head+1] < '0' || buffer[head+1] > '9')) return -1;


Suggested change

if (res > 1 && buffer[head] == '-' && (buffer[head+1] < '0' || buffer[head+1] > '9')) return -1;

if (res > 1 && buffer[head] == '-' && !isdigit(buffer[head+1])) return -1;

(Plus #include <ctype.h> on top of the file)

rettichschnidi · 2020-11-04T20:45:15Z

tests/convert_numbers_test.c

@@ -32,8 +32,9 @@ int64_t ints[]={12, -114 , 1 , 134 , 43243 , 0, -215025, INT64_MIN, INT64_MAX};
 const char* ints_text[] = {"12","-114","1", "134", "43243","0","-215025", "-9223372036854775808", "9223372036854775807"};
 uint64_t uints[]={12, 1 , 134 , 43243 , 0, UINT64_MAX};
 const char* uints_text[] = {"12","1", "134", "43243","0","18446744073709551615"};
-double floats[]={12, -114 , -30 , 1.02 , 134.000235 , 0.43243 , 0, -21.5025, -0.0925, 0.98765, 6.667e-11, FLT_MIN, FLT_MAX, DBL_MIN, DBL_MAX};
-const char* floats_text[] = {"12.0","-114.0","-30.0", "1.02", "134.000235","0.43243","0.0","-21.5025","-0.0925","0.98765", "0.00000000006667", "0.00000000000000000000000000000000000001175494", "340282346638528859811704183484516925440.0", "0.00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000002225073858", "179769313486231570814527423731704356798070567525844996598917476803157260780028538760589558632766878171540458953514382464234321326889464182768467546703537516986049910576551282076245490090389328944075868508455133942304583236903222948165808559332123348274797826204144723168738177180919299881250404026184124858368.0"};
+double floats[]={12, -114 , -30 , 1.02 , 134.000235 , 0.43243 , 0, -21.5025, -0.0925, 0.98765, 6.667e-11, 56.789, -52.0006, FLT_MIN, FLT_MAX, DBL_MIN, DBL_MAX};


Suggested change

double floats[]={12, -114 , -30 , 1.02 , 134.000235 , 0.43243 , 0, -21.5025, -0.0925, 0.98765, 6.667e-11, 56.789, -52.0006, FLT_MIN, FLT_MAX, DBL_MIN, DBL_MAX};

static const double floats[]={12, -114 , -30 , 1.02 , 134.000235 , 0.43243 , 0, -21.5025, -0.0925, 0.98765, 6.667e-11, 56.789, -52.0006, FLT_MIN, FLT_MAX, DBL_MIN, DBL_MAX};

Same (static, const) applies to all other variables declared in this file.

rettichschnidi · 2020-11-04T20:50:19Z

tests/convert_numbers_test.c

@@ -94,7 +95,7 @@ static void test_utils_textToFloat(void)
        double res;
        int converted;

-        converted = utils_textToFloat((uint8_t*)floats_text[i], strlen(floats_text[i]), &res);
+        converted = utils_textToFloat((uint8_t*)floats_text[i], strlen(floats_text[i]), &res, false);


Suggested change

converted = utils_textToFloat((uint8_t*)floats_text[i], strlen(floats_text[i]), &res, false);

converted = utils_textToFloat((const uint8_t*)floats_text[i], strlen(floats_text[i]), &res, false);

Same goes for all other calls to utils_textToFloat()/utils_textToUInt()/utils_textToInt().

rettichschnidi · 2020-11-04T20:51:03Z

tests/convert_numbers_test.c

+        double res;
+        int converted;
+
+        converted = utils_textToFloat((uint8_t*)floats_exponential[i], strlen(floats_exponential[i]), &res, true);


Suggested change

converted = utils_textToFloat((uint8_t*)floats_exponential[i], strlen(floats_exponential[i]), &res, true);

converted = utils_textToFloat((const uint8_t*)floats_exponential[i], strlen(floats_exponential[i]), &res, true);

rettichschnidi · 2020-11-04T21:04:47Z

tests/convert_numbers_test.c

+        CU_ASSERT(converted);
+        if (converted)
+        {
+            CU_ASSERT_DOUBLE_EQUAL(res, floats[i], floats[i]/1000000.0);


Where does the decision to allow a relative error of 1/1'000'000 come from? A comment about its origin would be helpful here, even when it is just a decision taken when implementing.

rettichschnidi · 2020-11-04T21:15:48Z

tests/convert_numbers_test.c


        CU_ASSERT(len);
        if (len)
        {
            compareLen = (int)strlen(floats_text[i]);
-            if (compareLen > DBL_DIG
+            if (compareLen > DBL_DIG - 1


I am having a hard time understanding what should get achieved in this block. Would be great to have some comment here.

rettichschnidi · 2020-11-04T21:58:16Z

tests/convert_numbers_test.c

+    CU_ASSERT_NOT_EQUAL(val, 0);
+    len = utils_floatToText(val, (uint8_t*)res, 6, true);
+    CU_ASSERT(len == 3);
+    if(len)


Outside of the loops (like the for above), I'd rather use CU_ASSERT_FATAL instead of this if/else/i++ boilerplate.

The problem with CU_ASSERT_FATAL is that it prevents later tests from running if it fails. I prefer to know everything that is broken rather than just the first thing. It can help with deciding how to fix it.

Having just one such assertion per test function would help with that. Of course, this would imply another kind of boiler plate... Since the existing code base does not do it, I am OK with leaving it as it is and doing some rework later on, on the whole code base.

sbertin-telular · 2020-11-06T13:33:57Z

I forgot to sign off on the commits. I'll do a rebase & squash again and make sure that is signed off.

rettichschnidi · 2020-11-06T17:14:14Z

core/json.c

+        if (!res) return -1;
+        /* Error if inf or nan */
+        if (buffer[head] != '-' && (buffer[head] < '0' || buffer[head] > '9')) return -1;
+        if (res > 1 && buffer[head] == '-' && (buffer[head+1] < '0' || buffer[head+1] > '9')) return -1;


Using isdigit()/#include <ctype.h> makes sense here too.

rettichschnidi · 2020-11-06T17:14:19Z

core/json.c

+        res = utils_floatToText(value, buffer + head, bufferLen - head, true);
+        if (!res) return -1;
+        /* Error if inf or nan */
+        if (buffer[head] != '-' && (buffer[head] < '0' || buffer[head] > '9')) return -1;


Using isdigit()/#include <ctype.h> makes sense here too.

rettichschnidi

Ignores warning about comparing floating point numbers for equality.
This is intentionally used to detect a NaN.

This part is not true anymore.

rettichschnidi · 2020-11-06T17:40:21Z

core/utils.c

@@ -149,8 +151,11 @@ int utils_textToFloat(const uint8_t * buffer,
        i = 0;
    }

+    /* Must have a decimal digit first after optional sign */
+    if (i >= length || buffer[i] < '0' || buffer[i] > '9') return 0;


Using isdigit()/#include <ctype.h> makes sense here too.

I'll change these as they are new with this pull request, but just grepping for "'0'" shows many more places that isdigit() could be used.

Provides an improved range of conversions and handles scientific notation. Updates the tests. Also includes improvemnets to the tests overall by making file scope constant variables static const and adding const to casts converting from text. Signed-off-by: Scott Bertin <[email protected]>

sbertin-telular · 2020-11-06T18:15:34Z

Rebased, squashed, and fixed commit message.

sbernard31 · 2020-11-09T10:15:34Z

Integrated in master (commit e6c9671)

@sbertin-telular, you maybe forgot to fetch master before to rebase.

dnav reviewed Feb 19, 2019

View reviewed changes

sbertin-telular mentioned this pull request Jul 2, 2019

Support the preferred content format for Bootstrap-Request #443

Closed

sbernard31 mentioned this pull request Nov 3, 2020

🏗 Integrate open PRs 🚧 #486

Closed

rettichschnidi mentioned this pull request Nov 4, 2020

Travsi-ci / any other CI #182

Closed

sbertin-telular force-pushed the float_conversion branch from 2b6ac11 to d1fa528 Compare November 4, 2020 13:20

rettichschnidi reviewed Nov 4, 2020

View reviewed changes

sbertin-telular force-pushed the float_conversion branch from fa3979f to dd43558 Compare November 6, 2020 13:37

rettichschnidi requested changes Nov 6, 2020

View reviewed changes

sbertin-telular force-pushed the float_conversion branch from 0e8c0d3 to 70189fa Compare November 6, 2020 18:14

sbernard31 closed this Nov 9, 2020

sbertin-telular deleted the float_conversion branch November 9, 2020 15:20

	if (buffer[head] != '-' && (buffer[head] < '0' \|\| buffer[head] > '9')) return -1;
	if (buffer[head] != '-' && !isdigit(buffer[head])) return -1;

	if (res > 1 && buffer[head] == '-' && (buffer[head+1] < '0' \|\| buffer[head+1] > '9')) return -1;
	if (res > 1 && buffer[head] == '-' && !isdigit(buffer[head+1])) return -1;

	double floats[]={12, -114 , -30 , 1.02 , 134.000235 , 0.43243 , 0, -21.5025, -0.0925, 0.98765, 6.667e-11, 56.789, -52.0006, FLT_MIN, FLT_MAX, DBL_MIN, DBL_MAX};
	static const double floats[]={12, -114 , -30 , 1.02 , 134.000235 , 0.43243 , 0, -21.5025, -0.0925, 0.98765, 6.667e-11, 56.789, -52.0006, FLT_MIN, FLT_MAX, DBL_MIN, DBL_MAX};

	converted = utils_textToFloat((uint8_t*)floats_text[i], strlen(floats_text[i]), &res, false);
	converted = utils_textToFloat((const uint8_t*)floats_text[i], strlen(floats_text[i]), &res, false);

Improve floating point conversions. #423

Improve floating point conversions. #423

Conversation

sbertin-telular commented Feb 14, 2019

sbertin-telular commented Feb 15, 2019

sbertin-telular commented Feb 15, 2019

dnav commented Feb 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbertin-telular commented Feb 19, 2019

sbertin-telular commented Feb 21, 2019

sbernard31 commented Nov 3, 2020

sbertin-telular commented Nov 4, 2020

sbernard31 commented Nov 4, 2020

rettichschnidi left a comment • edited Loading

Choose a reason for hiding this comment

rettichschnidi Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbertin-telular commented Nov 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rettichschnidi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbertin-telular commented Nov 6, 2020

sbernard31 commented Nov 9, 2020

rettichschnidi left a comment •

edited

Loading

rettichschnidi Nov 4, 2020 •

edited

Loading

sbertin-telular commented Nov 6, 2020 •

edited

Loading