json: improve handling of JSON floating-point types

[cjwinklhofer]

Improve parsing and serializing of 'float' and 'double'

Up to now, the handling of type float was offloaded to the users of the JSON utility, with the token JSON_TOK_FLOAT, which is still possible.

Improve handling of floating point types and support the types 'float' and 'double' in a built-in way so that they can be directly parsed into variables (of type float or double) and are also directly serialized as a JSON number.

The types are serialized in the shortest representation, either as adecimal number or in scientific notation:

• float (with JSON_TOK_FLOAT_FP): encoded with maximal 9 digits
• double (with JSON_TOK_DOUBLE_FP): encoded with maximal 16 digits
• NaN, Infinity, -Infinity: encoded and decoded as: {"nan_val":NaN,"inf_pos":Infinity,"inf_neg":-Infinity}

Enable the floating point functionality with the Kconfig option: JSON_LIBRARY_FP_SUPPORT=y. It requires a libc implementation with support for floating point functions: strtof(), strtod(), isnan() and isinf().

Fixes: #59412

Remark

This pull-request emerged from the pull-request #86800. Thanks @EricNRS!

[github-actions]

Hello @cjwinklhofer, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

[EricNRS]

Thank for the PR! Everything looks good to me, just some very minor comments.

Suggest minor edit for the commit message in 5809b83:

json: improve parsing and serializing of 'float' and 'double'
Up to now, the handling of type float was uploaded to the users of the
JSON utility, with the token JSON_TOK_OPAQUE_FLOAT.
...

I would suggest changing it to:

json: improve parsing and serializing of 'float' and 'double'
Up to now, the handling of type float was offloaded to the users of the
JSON utility, with the token JSON_TOK_OPAQUE_FLOAT (renamed from JSON_TOK_FLOAT).
...

[EricNRS]

Instead of using a magic number, you may want to use sizeof("-3.40282347e+38") (which would be 16-characters consisting of 15 characters plus a null). The compiler will evaluate that at compile time and just replace it with the number and it has the benefit of showing your sizing intentions better.

Same comment for double.

[cjwinklhofer]

Hi @EricNRS,

thanks for the review! I adapted the commit message (also added a note for the requirements) and replaced the magic-numbers for the buffer size.

Thanks
Christoph

[EricNRS]

@cjwinklhofer Code looks good to me. Just one minor change of uploaded -> offloaded in the commit message:

Up to now, the handling of type float was uploaded offloaded to the users of the
JSON utility, with the token JSON_TOK_OPAQUE_FLOAT (renamed from
JSON_TOK_FLOAT)

@ofirshe Could you do a quick once-over of this change since you reported the original issue?

[andyross]

I do note this now creates a hard dependency between the json code and the libc strtod(), which may not be small (the presence of printk floating point generation support is already kconfig'able). Have you looked at code size effects? Basically there are a lot of size-constrained deployments that might want json but not to pay for floating point.

[EricNRS]

I do note this now creates a hard dependency between the json code and the libc strtod(), which may not be small (the presence of printk floating point generation support is already kconfig'able). Have you looked at code size effects? Basically there are a lot of size-constrained deployments that might want json but not to pay for floating point.

@andyross - would that be CONFIG_REQUIRES_FLOAT_PRINTF or is there a different configuration item that indicates availability of floating point string operations? CONFIG_CBPRINTF_FP_SUPPORT also comes to mind as IS_ENABLED(CONFIG_CBPRINTF_FP_SUPPORT) is commonly sprinkled throughout the logging code.

[cjwinklhofer]

I do note this now creates a hard dependency between the json code and the libc strtod(), which may not be small (the presence of printk floating point generation support is already kconfig'able). Have you looked at code size effects? Basically there are a lot of size-constrained deployments that might want json but not to pay for floating point.

Thanks, yes that is a good point! I did a quick comparison with the 'helloworld' sample on a STM32F3 board, the flash size reported by the build output increases from 18428 Bytes to 27676 Bytes, with the floating-point changes (CONFIG_CBPRINTF_FP_SUPPORT and usage strtod from picolib).

Moreover, it will not compile when built with minimal libc, due to the missing strtof and strtod definitions:

CONFIG_MINIMAL_LIBC=y
CONFIG_EXTERNAL_LIBC=n

Rework of PR

I will rework this PR and make the floating-point handling in the JSON module optional. Probably by adding a new Kconfig option, similar to CONFIG_CBPRINTF_FP_SUPPORT:
CONFIG_JSON_LIBRARY_FP_SUPPORT=y|n (default n)

By default, the JSON module will work as before (float handling offloaded to the client), with the benefit that no migration is required (the JSON_TOK_OPAQUE_FLOAT will disappear) and FP support can be enabled on demand (utilizing snprintk with %g and strtod).

[EricNRS]

By default, the JSON module will work as before (float handling offloaded to the client), with the benefit that no migration is required (the JSON_TOK_OPAQUE_FLOAT will disappear) and FP support can be enabled on demand (utilizing snprintk with %g and strtod).

The issue with that approach is that if you are using both the LWM2M library and other code that requires floating point, then the LWM2M library would have an issue again. Another option would be to keep the original JSON_TOK_FLOAT definition and implementation and then add JSON_TOK_FLOAT_FP and JSON_TOK_DOUBLE_FP for the new functionality gated by CONFIG_JSON_LIBRARY_FP_SUPPORT. That should keep everyone happy.

[cjwinklhofer]

Encoding and decoding support is now only included on demand with the Kconfig option 'CONFIG_JSON_LIBRARY_FP_SUPPORT'.
In addition, it uses separate token names for the built-in floating-point handling, as @EricNRS suggested, thanks! Hence, manual float parsing (JSON_TOK_FLOAT) is still possible and existing users do not need to change something.

[EricNRS]

The change looks great. Thank you!

[cjwinklhofer]

There was an issue with my mail address: the author (mail) and the sign-off did not match, hence the new push.

[cjwinklhofer]

One additional remark for NaN, Infinity and -Infinity: Currently, this implementation returns an EINVAL, which I would like to change and also allow them, like in other JSON parsers (Python JSON or boost-json). A serialized JSON output would look like:

{ "nan_val": NaN, "inf_pos": Infinity, "inf_neg": -Infinity }

I would use this as the default behavior, instead of the EINVAL error - probably it makes sense to add a Kconfig option but I am a bit skeptical if this makes things more complicated than needed.

Best
Christoph

[EricNRS]

One additional remark for NaN, Infinity and -Infinity: Currently, this implementation returns an EINVAL, which I would like to change and also allow them, like in other JSON parsers (Python JSON or boost-json). A serialized JSON output would look like:

{ "nan_val": NaN, "inf_pos": Infinity, "inf_neg": -Infinity }

I would use this as the default behavior, instead of the EINVAL error - probably it makes sense to add a Kconfig option but I am a bit skeptical if this makes things more complicated than needed.

I think that is fine to add as the default behavior and no reason to add a Kconfig for it. Once those changes are done, I will test here on my branch and then we can rattle some cages to get this change merged.

[cjwinklhofer]

Thanks @EricNRS! I added the NaN and Infinity serialization and updated also the description of the pull-request.

[d3zd3z]

I think this now looks good. However, I think we should probably have tests to at least exercise the behavior across numeric types (given js's float-only specification).

[EricNRS]

I think this now looks good. However, I think we should probably have tests to at least exercise the behavior across numeric types (given js's float-only specification).

Just to clarify, are you just asking to just test the float and double parsing with some integers (0, -0, 1234, -1234, etc)?

[EricNRS]

I retested for my use case using doubles and everything works as expected.

[EricNRS]

Note that I get a warning of buf being uninitialized when CONFIG_JSON_LIBRARY_FP_SUPPORT=n. Looks like it is a false positive. The easiest fix may be to add str[0] = '\0'; in print_double() at line 1231.

zephyr/lib/utils/json.c: In function 'double_encode':
zephyr/lib/utils/json.c:1274:16: warning: 'buf' may be used uninitialized [-Wmaybe-uninitialized]
 1271 |         return append_bytes(buf, (size_t)ret, data);

Same warning for fload_encode() as well.

[cjwinklhofer]

Thanks, I also think that it is a false positive, will add the explicit initialization. It occurs with CONFIG_NO_OPITIMIZATIONS=n and CONFIG_DEBUG=y.

[cjwinklhofer]

I think this now looks good. However, I think we should probably have tests to at least exercise the behavior across numeric types (given js's float-only specification).

Thanks! I will add a test to parse JSON numbers in different formats.

[cjwinklhofer]

I had to rebase this branch to the current main, due to a CI error:

ERROR: Could not open requirements file: [Errno 2] No such file or directory: 'scripts/requirements-actions.txt'

[d3zd3z]

I think this now looks good. However, I think we should probably have tests to at least exercise the behavior across numeric types (given js's float-only specification).

Just to clarify, are you just asking to just test the float and double parsing with some integers (0, -0, 1234, -1234, etc)?

As well as parsing integers that are given as integer-valued floating point numbers. (1234.0). JS is weird. Obviously, this won't work if floating point is not enabled, but this is allowed by json.

[cjwinklhofer]

I think this now looks good. However, I think we should probably have tests to at least exercise the behavior across numeric types (given js's float-only specification).

Just to clarify, are you just asking to just test the float and double parsing with some integers (0, -0, 1234, -1234, etc)?

As well as parsing integers that are given as integer-valued floating point numbers. (1234.0). JS is weird. Obviously, this won't work if floating point is not enabled, but this is allowed by json.

Sorry, did not got it: You mean adding additional tests for the integer types (JSON_TOK_NUMBER, JSON_TOK_UINT64 and JSON_TOK_INT64) - parsing a JSON number in float format (1234.0) into an integer-type, which would result in an EINVAL?

[EricNRS]

As well as parsing integers that are given as integer-valued floating point numbers. (1234.0). JS is weird. Obviously, this won't work if floating point is not enabled, but this is allowed by json.

We should open another issue for that since this PR is just for issue #59412 which is for floating-point support.

Currently, if the format is specified as an integer type (e.g. JSON_TOK_NUMBER), then the parser will return -EINVAL if the value has a decimal point. That seems to work reasonably well and I agree it is not optimal due to JSON not having integer types. However, the question eventually becomes, what is the expected behavior? Should the value be truncated, rounded, or an error thrown? For what it is worth, some other JSON parsing libraries side-step the issue by using a double for parsing all number types and leave it up to the application to application code to cast it to an integer type, but as @andyross pointed out, that increases the code size too much for some smaller micros.

[d3zd3z]

Also, parsing as double prevents a json user from being able to use the full range of a 64-bit integer. I think it is reasonable to not support numbers with decimals as integers.

[andrew-gillan]

@cjwinklhofer and all the reviewers, thanks for this PR and #87580!