#910 chprintf float support for 0 precision

[Simon]

Hello,
I encountered some strange behaviour with chsnprintf float support.
I know that it is meant to be a minimal implementation of snprintf, and it justify the lack of some features.

Here is a sample code and results:

char str[20+1];
double dbl = 12345.123456789;
chsnprintf(str, sizeof(str), "%f", dbl);

"%5f" (default precision of fract part is 6) loss of precision (this is because it use float and not double) too much frac digits
result = 12345.123046875 ; expected result = 12345.123047 (in this case, rounding of last digit) ; fixed result = 12345.123046

"%.1f" ok
result = 12345.1 ; expected result = 12345.1

"%5.0f" (precision of fract part is 0) doesn't work
result = 12345.123046875 ; expected result = 12345 ; fixed result = 12345

"%3.f" (no truncation of integer part, default precision of fract part is 0) doesn't work
result = 12345.123046875 ; expected result = 12345 ; fixed result = 12345

"%9.1f" (right justify) ok
result = ''12345.1 ; expected result = ''12345.1

"%-9.1f$" (left justify) ok
result = 12345.1''$ ; expected result = 12345.1''$

"%+9.1f"(print sign, even when +) not implemented
result = +9.1f ; expected result = '+12345.1

"%#9.0f$"(print decimal point, even when no frac part) not implemented
result = #9.0f$ ; expected result = '''12345.$

"% .1f"(print space if no sign) not implemented
result = '.1f ; expected result = '12345.1

"%09.1f"(leading 0s) ok
result = 0012345.1 ; expected result = 0012345.1

It is easy to implement the support for the 0 precision case, without increasing the size at all:

chibios/os/hal/lib/streams/chprintf.c
fix candidate in chvprintf '.' parse:

    precision = 6; // default precision here
    if (c == '.') {
      precision = 0; // reset precision to 0 here
      while (TRUE) {
        c = *fmt++;
        if (c >= '0' && c <= '9')
          c -= '0';
        else if (c == '*')
          c = va_arg(ap, int);
        else
          break;
        precision *= 10;
        precision += c;
      }
    }

fix candidate in ftoa, add an if:

static char *ftoa(char *p, double num, unsigned long precision)
{
    long l;

    l = (long)num;
    p = long_to_string_with_divisor(p, l, 10, 0);

    if (precision != 0) {
      if (precision > FLOAT_PRECISION)
        precision = FLOAT_PRECISION;
      *p++ = '.';
      precision = pow10[precision - 1];
      l = (long)((num - l) * precision);
      return long_to_string_with_divisor(p, l, 10, precision / 10);
    }
    else
      return p;
}

Also something unrelated: the recent "time API name change" commit (like OSAL_MS2I) should be propagated to most LLD drivers.

Best regards,
Simon

[Simon]

It seems that the
precision = 6; // default precision here
interfere with the way strings are handled below. We do not want all strings to be max 6 length :)

case 's':
      [...]
      if (precision == 0)
        precision = 32767;
      for (p = s; *p && (--precision >= 0); p++)
        ;

The good news is that the 0 precision still works.

precision = 0;
    if (c == '.') {
      precision = 0; // reset precision to 0 here

[Giovanni Di Sirio]

Hi, fixed on all active branches, thanks.

Giovanni

[Simon]

Hi,

The way %s is handled in the printf restrict us to put 6 instead of 0 here

precision = 6; <---------------------------
    if (c == '.') {
      precision = 0;

because the code use the precision = 0 to determine the string maximum size.

case 's':
      [...]
      if (precision == 0) <-----------------------------
        precision = 32767;
      for (p = s; *p && (--precision >= 0); p++)
        ;

If we print with %s, "123456789" it will only print "123456" :(
I saw that later and I couldn't edit the first post.
It is better to undo the change made onprecision = 6;
to let it like before:precision = 0;
The only downside is that it will print like 7 digits instead of 6 in case of %f with unspecified precision, but that not a big deal. :)

Don't forget to modify theftoa function.
I modified it a bit so it returns the integer part of the fp number in case of 0 precision, by default it wasn't.

Best regards,
Simon