Skip to content

restore dtostrf when floats are disabled in printf/scanf + round fix#7093

Merged
devyte merged 7 commits intoesp8266:masterfrom
d-a-v:dtostrfix2
Feb 22, 2020
Merged

restore dtostrf when floats are disabled in printf/scanf + round fix#7093
devyte merged 7 commits intoesp8266:masterfrom
d-a-v:dtostrfix2

Conversation

@d-a-v
Copy link
Collaborator

@d-a-v d-a-v commented Feb 19, 2020 •
edited
Loading

replaces and closes #7087

(edit: tested)

fixes #7073 with solution 6
updates #7068
fixes #7043
(no need for -DNOPRINTFLOAT)

@d-a-v d-a-v mentioned this pull request Feb 19, 2020
Closed
@d-a-v d-a-v requested a review from earlephilhower February 19, 2020 23:02
earlephilhower
earlephilhower approved these changes Feb 20, 2020
View reviewed changes
Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

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

Looks like the if (function pointer) is not optimized. Both branches are present in all builds (float and non-float). That gives a 554-byte function in both cases. In normal mode, ~500 of those bytes are never used (the __dtostrf inlined code), and in non-float mode ~50 of those aren't used (the printf() branch).

I think this may be the best we can hope for, without adding a new #define like your original PR, so I'm approving as-is. If someone else has any ideas, I'd be interested...

@d-a-v d-a-v requested a review from devyte February 20, 2020 06:21
@d-a-v
Copy link
Collaborator Author

d-a-v commented Feb 20, 2020

I am too interested in another solution because I see only the use of the define to not have the real implementation when printf-float is available. That's because the symbol presence cannot be known at compile time.

@earlephilhower with a patch to newlib, we could condition linking of _prinf_float the very same way we do with stack in sys space when WPS is not used. No more -u printf... nor any define. A simple call to a function could disable printf/floats at link time.

devyte
devyte requested changes Feb 20, 2020
View reviewed changes
@earlephilhower earlephilhower added this to the 2.7.0 milestone Feb 20, 2020
@d-a-v
Copy link
Collaborator Author

d-a-v commented Feb 21, 2020

diff from original dtostrf is

index b39edbb2..c742904d 100644
--- a/cores/esp8266/core_esp8266_noniso.cpp
+++ b/cores/esp8266/core_esp8266_noniso.cpp
@@ -27,6 +27,7 @@
 #include <stdbool.h>
 #include <stdint.h>
 #include <math.h>
+#include <limits>
 #include "stdlib_noniso.h"
 
 extern "C" {
@@ -77,11 +78,15 @@ char * dtostrf(double number, signed char width, unsigned char prec, char *s) {
     // Figure out how big our number really is
     double tenpow = 1.0;
     int digitcount = 1;
-    while (number >= 10.0 * tenpow) {
-        tenpow *= 10.0;
+    double nextpow;
+    while (number >= (nextpow = (10.0 * tenpow))) {
+        tenpow = nextpow;
         digitcount++;
     }
 
+    // minimal compensation for possible lack of precision (#7087 addition)
+    number *= 1 + std::numeric_limits<decltype(number)>::epsilon();
+
     number /= tenpow;
     fillme -= digitcount;

@devyte devyte merged commit cd56dc0 into esp8266:master Feb 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@earlephilhower earlephilhower earlephilhower approved these changes

@devyte devyte devyte approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.7.0

Development

Successfully merging this pull request may close these issues.

dtostrf/Print(float) inconsistent value/methods, downstream use [BUG]: Rounding Values when Using String Type Cast

3 participants

@d-a-v @earlephilhower @devyte