dtostrf/Print(float) inconsistent value/methods, downstream use

[earlephilhower]

#7068 removed two slightly different (and both slightly wrong) implementations of double to ASCII string in Print::print(float) and dtostrf(a non-std. AVR C addition AFAICT) and replaced it with a call to newlib's sprintf(%f) which is known good.

Unfortunately, as @mcspr noted, there are folks who are building the core w/o float-printf support (printf_float in platform.txt), so the above PR will end up borking their output.

Might need to be labelled as breaking? See
https://github.com/arendst/Tasmota/blob/1598e6227a31be5f27081c5a660d22d21613c0ca/platformio.ini#L83
https://github.com/arendst/Tasmota/blob/development/pio/strip-floats.py
https://github.com/xoseperez/espurna/blob/dev/code/scripts/espurna_utils/float_support.py

The main question now is what is the right thing to do in this case. I suggest it's one of the following:

1. Revert to duplicated code and inconsistent output diffs between Print() and dtostrf() in corner cases. (i.e. undo Use sprintf to output floats in Print/dtostrf #7068). Larger code for most people (3 different versions of float->ascii in ROM), but downstream keeps working.
2. Keep sprintf(%f) and disallow disabling printf_float, requiring changes in downstream projects (ROM will go up ~1K for them, but down ~1KB for most users since the 2 dup'd versions will be expunged)
3. Pick method from Print(float) and replicate it in dtostrf() so they match. Then replace Print(float) with a call to dtostrf(). Smaller ROM for all, both match, but may not match the correct output in some cases.
4. Same as 3, but use the dtostrf() as golden and not Print()s version.

FWIW, looking at the AVC-LIBC implementation, it is closest in logic to number 2 above. They go down to the binary FP level and extract exponents and mantissas (as the AVR ASM level) and work with those parts to actually generate strings.

Any opinions from downstream?

[Makuna]

I would vote for NOT using sprint as the only solution. The reason why, (s)printf implementation is rather large body of code, and one common plan to reduce code size is to avoid it and use the lower level conversion methods, like dtostrf; which now would be a useless solution.

[mcspr]

Would using newlib method in Print and String, but leaving dtostrf as-is be reasonable? When using dtostrf we get what we get. When using print / string there is a correct result, that also can be optionally removed from the build

---

Just comparing local test builds with the master:

float support	bytes
default	606256
strip scanf	605040
strip printf	600800
strip both	599584

[earlephilhower]

Good info in the build sizes, @mcspr

5. Print(float) to use sprintf(), dtostrf to leave as-is. We still have 2 different answers, but only if you use the non-ISO/POSIX utility. Users w/o printf_float cannot use Print(float) in this case (but used to be able to).

I hate keeping the broken bits in, but pragmatically we can isolate it so only folks who explicitly use it for memory savings. It is still kind of a breaking change that way, though.

[earlephilhower]

Internal discussion has ended up liking option 5) the best. Would that pass muster for you, @Makuna @mcspr , anyone else?

If you call dtostrf() you get what you get (we revert to the code that was there, so some rounding issues but close enough for most purposes). If you call Print(float,nnn) you get sprintf("%f"). If you have disabled print_float, then obviously Print(float,nnn) isn't going to work.

[Makuna]

Reasonable compromise.

[mcspr]

Sure, but I am starting to think about subtle bugs that it may cause :/ And I will need to scan some rogue String += with floats in the code... What I am all for is having a way to strip size, I just wonder if that is the correct way.

Also, since Tasmota script was linked, cc @arendst
The version that I tried was based on emon sensor Espurna build, but I also tried building some Tasmota binaries (stat tasmota-sensor.bin) with the latest git and I am seeing slightly different numbers:

HEAD, removed strip-float.py -> 628656 bytes
HEAD, kept strip script -> 613648 bytes (bogus, since nothing actually works b/c dtostrf is a dummy function)
HEAD, removed scanf -> 621808 bytes

---

Keep sprintf(%f) and disallow disabling printf_float, requiring changes in downstream projects (ROM will go up ~1K for them, but down ~1KB for most users since the 2 dup'd versions will be expunged)

@earlephilhower Did you mean something like this?
(spied from https://github.com/arduino/ArduinoCore-samd/blob/master/cores/arduino/avr/dtostrf.c)

--- a/cores/esp8266/core_esp8266_noniso.cpp
+++ b/cores/esp8266/core_esp8266_noniso.cpp
@@ -41,6 +41,7 @@ char* ultoa(unsigned long value, char* result, int base) {
 }

 char * dtostrf(double number, signed char width, unsigned char prec, char *s) {
+    asm(".global _printf_float");
     char fmt[32];
     sprintf(fmt, "%%%d.%df", width, prec);
     sprintf(s, fmt, number);

Implying removing -u _printf_float from ld flags

[arendst]

Just my two cents. If using the Stage version and compiling Tasmota with Zigbee and strip-floats applied the resulting compiled size is 545684.

With strip-floats removed it becomes 560716.

This is consistent with earlier findings on different core versions; adding floats always added over 10k of code.