Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] logging: Add simple float numbers support #18921

Closed
wants to merge 1 commit into from

Conversation

nordic-krch
Copy link
Contributor

Added macros which allow printing float number. It is splitting float
into 3 arguments: sign, interger and residuum.

Method requires custom macro being used instead of format specifier and
macro for converting float argument.

Partially fixes #18351.

Example of logging float numer:

LOG_INF("value:"LOGF" is printed as float", LOGF_ARG(-0.112));

will result in: value:-0.1120 is printed as float

I know that it's not very convenient but allows float logging. Supporting %f is (as mentioned by @pabigot ) rather complex.

FYI: this approach is taken from nRF5 SDK where similar logger exists with similar limitation.

Let me know what do you think about it. If it is accepted i'll extend sample and document it.

Signed-off-by: Krzysztof Chruscinski krzysztof.chruscinski@nordicsemi.no

Added macros which allow printing float number. It is splitting float
into 3 arguments: sign, interger and residuum.

Method requires custom macro being used instead of format specifier and
macro for converting float argument.

Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>
@nordic-krch nordic-krch added area: Logging RFC Request For Comments: want input from the community labels Sep 5, 2019
@nordic-krch
Copy link
Contributor Author

fyi @deadsy

@zephyrbot zephyrbot added the area: API Changes to public APIs label Sep 5, 2019
@pizi-nordic
Copy link
Collaborator

Why not use regular %f and pass float as the argument?

@pabigot
Copy link
Collaborator

pabigot commented Sep 5, 2019

Why not use regular %f and pass float as the argument?

Because that doesn't work in C. A float passed as an argument is promoted to double, just as a u8_t is promoted to int.

Copy link
Collaborator

@pabigot pabigot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meh.

Technically this provides a solution, though it's ugly and limited and dirty (really, a macro that expands to three parameters is...I just don't have words). I wouldn't accept this without a fairly extensive test of the conversion code alone. What does it do with NAN? Or really small or really large values?

The ideal case would be to support %f properly. Given that there's no single implementation of *printf formatting for Zephyr use in logging/printk/libc across toolchains that's not going to happen.

A better solution overall would be to say that if you need to log a floating point number, you're responsible for converting it to ASCIIZ first.

I'd consider a path based on something like gcvt, even if we have to add an optional implementation of it to minimal libc. You'd need a helper macro to declare a buffer that's of the correct size for a given number of digits, because that value is not obvious.

Then I'd step back and say no, just tell people to use snprintf.

@nordic-krch
Copy link
Contributor Author

it's ugly and limited and dirty

i agree with that, just ported something we had.

Converting to string might be a better option.

@nordic-krch nordic-krch closed this Sep 5, 2019
@deadsy
Copy link

deadsy commented Sep 5, 2019

The ideal case would be to support %f properly. Given that there's no single implementation of *printf formatting for Zephyr use in logging/printk/libc across toolchains that's not going to happen.

If the printf code doesn't handle float printing, that's one thing- but as I understood it the core problem was that the logging code can't handle the float/double promotion involved with the varargs.

The snprintf alternative works of course, but is very clunky:

char tmp[32];
snprintf(tmp, sizeof(tmp), "%f", val);
LOG_DBG("here it is %s", log_strdup(tmp));

This starts to feel a bit silly after the first couple of times.

@pabigot
Copy link
Collaborator

pabigot commented Sep 5, 2019

@deadsy I think a better description of the core problem is that the logging infrastructure can't accurately determine the space requirements of the variable argument list that it needs to capture. Since that length varies based on the underlying *printf handling of format length modifiers, which is at least libc-dependent and possibly dependent on compilation flags and platform, it's not a simple problem to solve.

You could abstract to a local wrapper that returns a reference to a static buffer, unless you have to worry about thread safety.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: Logging RFC Request For Comments: want input from the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

logging: 32 bit float values don't work.
5 participants