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

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

Merged
merged 7 commits into from
Feb 22, 2020

Conversation

d-a-v
Copy link
Collaborator

@d-a-v d-a-v commented Feb 19, 2020

replaces and closes #7087

(edit: tested)

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

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.

cores/esp8266/core_esp8266_noniso.cpp Outdated Show resolved Hide resolved
@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
Labels
None yet
Projects
None yet
3 participants