Introduce rounding

[Okarss]

The impact on footprint and performance is tiny and the code turned out to be relatively simple. In my opinion a mathematical correctness is definitely worth for such a minor sacrifice.

[charlesnicholson]

Whoa, cool, thanks for the patch! I'll study it today sometime, but at first glance it seems reasonable. Could I ask you for some unit tests that confirm the behavior works as expected please? Additionally, if you're interested, feel free to add yourself to the contributors file!

[charlesnicholson]

BTW I think I just set up nanoprintf to automatically run the PR actions for all contributors, so hopefully I won't need to manually click "yes yes please run the actions" every time you push.

[Okarss]

/tests/unit_ftoa_rev.cc : REQUIRE(npf_ftoa_rev(buf, 0.f, 0, &frac_bytes) == 2);
This test obviously fails because the third parameter has been changed to a pointer. I'll fix this one.

/tests/mpaland-conformance/paland.cc:592 : require_conform("30343.140", "%.3f", 30343.1415354);
The actual float value is 30343.140625, therefore with truncation the expected output is "30343.140", but with rounding it should be "30343.141". Should I fix it and create a PR for that nanoprintf-paland-conformance submodule also?

While developing I just tested these cases manually:

// Rounding
npf_snprintf(..., "%f",   12.8954); // "12.895400"
npf_snprintf(..., "%.5f", 12.8954); // "12.89540"
npf_snprintf(..., "%.4f", 12.8954); // "12.8954"
npf_snprintf(..., "%.3f", 12.8954); // "12.895"
npf_snprintf(..., "%.2f", 12.8954); // "12.90"
npf_snprintf(..., "%.1f", 12.8954); // "12.9"
npf_snprintf(..., "%.f",  12.8954); // "13"

// Limits of the implemented float precision
npf_snprintf(..., "%.8f", 1./3.); // "0.33333334"
npf_snprintf(..., "%.8f", 2./3.); // "0.66666668"

// Limits of the implemented float range
npf_snprintf(..., "%f", 18446742974197923840.); // "18446742974197923840.000000"
npf_snprintf(..., "%f", 18446744073709551616.); // "oor"

I'll look into how to add those as an additional unit tests.

By the way, for analyzing floating point values and limits, I used this tool:
https://www.h-schmidt.net/FloatConverter/IEEE754.html

[charlesnicholson]

Re:

// Limits of the implemented float precision
npf_snprintf(..., "%.8f", 1./3.); // "0.33333334"
npf_snprintf(..., "%.8f", 2./3.); // "0.66666668"

I don't yet understand why those would round up, is that intentional?

[charlesnicholson]

I can handle the paland stuff if you like, it's not set up for collaborative development- i just push directly to main over there once a local branch works against the PR i have open in this repo...

[Okarss]

Reviewed all of the float related tests in the paland.cc file and readjusted the expected values for the affected ones.
paland.cc.txt
rounding.patch

I don't yet understand why those would round up, is that intentional?

Those are not rounded but just truncated at the limit of the precision. The actual single float values stored are:

• 0.3333333432674407958984375
• 0.666666686534881591796875

Because of NPF_MAX_FRACTION_DEC_DIGITS = 8 the current implementation of conversion code prints (reversed) "33333334" and "66666668" respectively and the further printing code adds additional zeros for higher "precision" output, when necessary. As rounding needs "the next digit", the last digit at the configured precision depth will always be truncated, not rounded. With the current configuration it's the 8th digit.

Thinking more about the limits... Mathematically the resolution of the 32-bit float is log10(2^24) = 7.2 decimal digits. As it's >7, it makes sense for the 8th digit to be properly rounded, leaving the 9th digit truncated. Setting NPF_MAX_FRACTION_DEC_DIGITS = 9 would also change a few more test cases like this one:

The actual float value: 42.89522552490234375
The old: require_conform("42.895225520000", "%.12f", 42.89522387654321);
The new: require_conform("42.895225524000", "%.12f", 42.89522387654321);

I guess that's the mathematically right way... Should I go forward and readjust for 9 digits?

[charlesnicholson]

I applied your patch over on the mpaland-conformance branch, and pushed it to a remote with the name rounding. If you retarget the submodule SHA to rounding and push we'll see the CI results. Once we get a clean bill of health, I'll merge the rounding branch to main and we can retarget again. It's a bit of a clunky workflow, sorry.

Thanks for the explanation about 1/3 etc. I think it makes sense to have the max fraction decimal digits to equal 9 as well, your reasoning seems right to me...

Also, thanks for all of the work on this patch!

[charlesnicholson]

I just merged your changes to the mpaland-conformance branch; they're now in main! One last change, just bumping the submodule SHA to main and we're good to merge!

[Okarss]

I'll go through the test cases and create a patch for 9 digit version. If that doesn't create unexpected issues, I guess we should add it to this PR?

[charlesnicholson]

Sure, sounds good to me.

[Okarss]

There it is: rounding9.patch
Will do testing for this the same way? If yes, then I will wait for a submodule patch and then push it together with the NPF_MAX_FRACTION_DEC_DIGITS value update.

[charlesnicholson]

Applied and pushed your rounding9 patch to a rounding9 branch on mpaland-conformance.

[charlesnicholson]

Looks good, I'll merge the mpaland PR and then you can bump this (again), and then .... merge?

[charlesnicholson]

mpaland is merged, you can bump the sha to main whenever you like.

[charlesnicholson]

This looks good to me, is there anything else you want to get into this PR or shall I merge it? Thanks again for the contribution!

[Okarss]

It seems that this PR is complete. Now looking at the code I have some ideas for a potential improvement for both precision and rounding, but at this point those are just rough ideas. In short - the npf_fsplit_abs() is multiplying up the uint64_t fraction and then the npf_ftoa_rev() is dividing it down to get the digits. Combining/avoiding that could save the CPU cycles significantly, probably reduce code size and open up a possibility for a completely dynamic and "unlimited" precision, eliminating the NPF_MAX_FRACTION_DEC_DIGITS altogether. But there are many nuances to think through and such changes would be more intrusive. So definitely should not be a part of this PR.

Thanks to you also for a nice printf library implementation! As the Newlib implementation is unusable, I looked for alternatives for some MCU projects. For a device based on STM32L432 I chose this one, because it is compact and supports float values. But, as I need a proper rounding in that project, I looked if I can implement it myself.

[charlesnicholson]

Thanks for the kind words! I would be very interested in such a PR if you felt motivated. The floating-point portion of nanoprintf is definitely the weakest part of it, in my opinion. It feels like there are many size optimizations lurking there, as you mention.