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 #7087

Closed
wants to merge 9 commits into from

Conversation

d-a-v
Copy link
Collaborator

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

Replaced by #7093 (which updates only 1 file, not 23 :)

This PR restores dtosrf() only when floats are disabled in printf.
boards.txt builder adds a new define -DNOPRINTFLOAT in this case which enables dtostrf compilation.
A small fix has been added for the rounding issue, which I believe is generic enough, but will always be a subject for discussion.
Some multiplications have been removed from the algorithm too.

fixes #7073 with solution 6
updates #7068
fixes #7043

dtostrf(1045.5, 0) now returns 1046.

@mcspr @arendst Can you test this ?

@d-a-v d-a-v changed the title dtostrf: restore when floats are disabld in printf/scanf dtostrf: restore when floats are disabled in printf/scanf Feb 16, 2020
@d-a-v d-a-v changed the title dtostrf: restore when floats are disabled in printf/scanf restore dtostrf when floats are disabled in printf/scanf + round fix Feb 16, 2020
@arendst
Copy link

arendst commented Feb 16, 2020

Did some tests:

  • Compiled your PR without changing other parameters results in empty dtostrf responses where normally a valid float would have been printed. Code size 507256 bytes.
  • Compiled your PR with additional -DNOPRINTFLOAT and without pio/strip_floats.py results in valid dtostrf response. Code size 522816. This test doesn't make sense with regard to your PR as floats are enabled anyway.
  • Compiled your PR with additional -DNOPRINTFLOAT and with pio/strip_floats.py results in valid dtostrf response. Code size 507800.

So your PR works but I have to add define -DNOPRINTFLOAT to get a valid output of dtostrf while still being able to save 15k code space.

@d-a-v
Copy link
Collaborator Author

d-a-v commented Feb 16, 2020

d-a-v approved this pull request.

Copy link
Collaborator

@mcspr mcspr left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -40,11 +41,94 @@ char* ultoa(unsigned long value, char* result, int base) {
return utoa((unsigned int)value, result, base);
}

#ifdef NOPRINTFLOAT
Copy link
Collaborator

@mcspr mcspr Feb 16, 2020

Choose a reason for hiding this comment

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

NO_PRINTF_FLOAT?
NO_FLOAT_PRINTF?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Dogpiling on this (sorry). Adding new defines normally ends up breaking PIO or other folks.

Would it be possible, through whatever magic the -u _printf_float uses, to somehow make the proper version be linked-in?

I could also drag this into the newlib if that would be needed to make it work "seamlessly" with the existing infra/flags.

Copy link
Collaborator

@mcspr mcspr Feb 18, 2020

Choose a reason for hiding this comment

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

I thought of it as something like the existing NO_GLOBAL_... defines
PIO user btw, only change that would be required is adding env.Append(CXXFLAGS=["NO_PRINTF_FLOAT"]) to the user build script (or, modifying platformio.ini build_flags=... variable, which is also file that is controlled by user), Core files don't need to change

Re 'magic', it's just a simple != NULL:
https://github.com/earlephilhower/newlib-xtensa/blob/b326aa447a1e5b33d6ed7d164aa2c59eb3754b9f/newlib/libc/stdio/nano-vfprintf.c#L651-L653

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, then could it be as simple as:

char * dtostrf(double number, signed char width, unsigned char prec, char *s) {
    if (!_printf_float) {
        return __dtostrf(number, width, char prec, s);
    } else {
      char fmt[32];
      sprintf(fmt, "%%%d.%df", width, prec);
      sprintf(s, fmt, number);
      return s;
  }
}

static char * __dtostrf(double number, signed char width, unsigned char prec, char *s) {
...all the @d-a-v fixed code here...
}

This way the existing flag which will be eval'd a compile time will either cause the dtostrf guts to be omitted and use printf, or vice-versa.

No flag changes, no tool flow changes. Assuming it works...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

great ! 👍 thanks @mcspr @earlephilhower

}

#else // !NOPRINTFLOAT

char * dtostrf(double number, signed char width, unsigned char prec, char *s) {
char fmt[32];
Copy link
Collaborator

Choose a reason for hiding this comment

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

As I mentioned in the issue, does this need to have a safeguard such as asm(".global _printf_float");?
ref https://sourceware.org/binutils/docs/as/Global.html#Global

.global makes the symbol visible to ld.

earlephilhower and others added 2 commits February 16, 2020 09:17
Corrected stack start and end in stack_thunk_dump_stack().
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.

Thanks for un-borking my borkage! You went the extra mile and optimized for all cases, which is awesome.

Can we make it transparent, though? You're the weak-linkage expert on the team, I think. :)

@@ -40,11 +41,94 @@ char* ultoa(unsigned long value, char* result, int base) {
return utoa((unsigned int)value, result, base);
}

#ifdef NOPRINTFLOAT
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dogpiling on this (sorry). Adding new defines normally ends up breaking PIO or other folks.

Would it be possible, through whatever magic the -u _printf_float uses, to somehow make the proper version be linked-in?

I could also drag this into the newlib if that would be needed to make it work "seamlessly" with the existing infra/flags.

…log limitation and other fixes (esp8266#6887)

* upstream lwIP is now downloaded by a makefile, not subsubmoduled

* lwip2: upstream lwIP not sub-sub-modules anymore
lwip2: Allow IPv4 and IPv6 DNS and SNTP server configured via DHCP to co-exist (patch against upstream)

* lwip2: enable tcp-listen-with-backlog feature

* lwip2 submodule update:
- enable more efficient chksum algorithm thanks to Richard Allen
- enable tcp listener with backlog

* more comments, fix backlog management, fix API
* move default value definition in .cpp
because one must not believe it can be redefined before including WiFiServer.h

* improved backlog handling, it is no more a breaking change
* configTime(tzsec,dstsec,): fix UTC/local management
This PR also remove dead code since probably newlib updates
The NTP-TZ-DST example is also updated
* restore sntp_set_timezone_in_seconds()
fixes esp8266#6678
* +configTzTime()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants