Skip to content

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

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

Closed
wants to merge 9 commits into from
84 changes: 84 additions & 0 deletions cores/esp8266/core_esp8266_noniso.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include <stdbool.h>
#include <stdint.h>
#include <math.h>
#include <limits>
#include "stdlib_noniso.h"

extern "C" {
Expand All @@ -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


char * dtostrf(double number, signed char width, unsigned char prec, char *s) {
bool negative = false;

if (isnan(number)) {
strcpy(s, "nan");
return s;
}
if (isinf(number)) {
strcpy(s, "inf");
return s;
}

char* out = s;

int fillme = width; // how many cells to fill for the integer part
if (prec > 0) {
fillme -= (prec+1);
}

// Handle negative numbers
if (number < 0.0) {
negative = true;
fillme--;
number = -number;
}

// Round correctly so that print(1.999, 2) prints as "2.00"
// I optimized out most of the divisions
double rounding = 2.0;
for (uint8_t i = 0; i < prec; ++i)
rounding *= 10.0;
rounding = 1.0 / rounding;

number += rounding;

// Figure out how big our number really is
double tenpow = 1.0;
int digitcount = 1;
double nextpow;
while (number >= (nextpow = (10.0 * tenpow))) {
tenpow = nextpow;
digitcount++;
}

// minimal compensation for possible lack of precision
number *= 1 + std::numeric_limits<decltype(number)>::epsilon();

number /= tenpow;
fillme -= digitcount;

// Pad unused cells with spaces
while (fillme-- > 0) {
*out++ = ' ';
}

// Handle negative sign
if (negative) *out++ = '-';

// Print the digits, and if necessary, the decimal point
digitcount += prec;
int8_t digit = 0;
while (digitcount-- > 0) {
digit = (int8_t)number;
if (digit > 9) digit = 9; // insurance
*out++ = (char)('0' | digit);
if ((digitcount == prec) && (prec > 0)) {
*out++ = '.';
}
number -= digit;
number *= 10.0;
}

// make sure the string is terminated
*out = 0;
return s;
}

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

sprintf(fmt, "%%%d.%df", width, prec);
sprintf(s, fmt, number);
return s;
}

#endif // !NOPRINTFLOAT

};
5 changes: 3 additions & 2 deletions platform.txt
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ build.stdcpp_lib=-lstdc++
build.stdcpp_level=-std=gnu++11

build.float=-u _printf_float -u _scanf_float
build.floatflags=
build.led=

# default SDK for all boards
Expand All @@ -53,7 +54,7 @@ compiler.libc.path={runtime.platform.path}/tools/sdk/libc/xtensa-lx106-elf
compiler.cpreprocessor.flags=-D__ets__ -DICACHE_FLASH -U__STRICT_ANSI__ "-I{compiler.sdk.path}/include" "-I{compiler.sdk.path}/{build.lwip_include}" "-I{compiler.libc.path}/include" "-I{build.path}/core"

compiler.c.cmd=xtensa-lx106-elf-gcc
compiler.c.flags=-c {compiler.warning_flags} -Os -g -Wpointer-arith -Wno-implicit-function-declaration -Wl,-EL -fno-inline-functions -nostdlib -mlongcalls -mtext-section-literals -falign-functions=4 -MMD -std=gnu99 -ffunction-sections -fdata-sections {build.exception_flags} {build.sslflags}
compiler.c.flags=-c {compiler.warning_flags} -Os -g -Wpointer-arith -Wno-implicit-function-declaration -Wl,-EL -fno-inline-functions -nostdlib -mlongcalls -mtext-section-literals -falign-functions=4 -MMD -std=gnu99 -ffunction-sections -fdata-sections {build.exception_flags} {build.sslflags} {build.floatflags}

compiler.S.cmd=xtensa-lx106-elf-gcc
compiler.S.flags=-c -g -x assembler-with-cpp -MMD -mlongcalls
Expand All @@ -64,7 +65,7 @@ compiler.c.elf.cmd=xtensa-lx106-elf-gcc
compiler.c.elf.libs=-lhal -lphy -lpp -lnet80211 {build.lwip_lib} -lwpa -lcrypto -lmain -lwps -lbearssl -laxtls -lespnow -lsmartconfig -lairkiss -lwpa2 {build.stdcpp_lib} -lm -lc -lgcc

compiler.cpp.cmd=xtensa-lx106-elf-g++
compiler.cpp.flags=-c {compiler.warning_flags} -Os -g -mlongcalls -mtext-section-literals -fno-rtti -falign-functions=4 {build.stdcpp_level} -MMD -ffunction-sections -fdata-sections {build.exception_flags} {build.sslflags}
compiler.cpp.flags=-c {compiler.warning_flags} -Os -g -mlongcalls -mtext-section-literals -fno-rtti -falign-functions=4 {build.stdcpp_level} -MMD -ffunction-sections -fdata-sections {build.exception_flags} {build.sslflags} {build.floatflags}

compiler.as.cmd=xtensa-lx106-elf-as

Expand Down
1 change: 1 addition & 0 deletions tools/boards.txt.py
Original file line number Diff line number Diff line change
Expand Up @@ -1588,6 +1588,7 @@ def all_boards ():

if nofloat:
print(id + '.build.float=')
print(id + '.build.floatflags=-DNOPRINTFLOAT')

print('')

Expand Down