Dubious code in printFloat(double number, uint8_t digits)

[MalcolmBoura]

A function named printFloat that prints a double is unfortunate naming!

https://github.com/arduino/ArduinoCore-avr/blob/2ff00ae7d4e85fa422b7918ee12baf56a1f3006e/cores/arduino/Print.cpp#L229

The comment indicates a problem. It works but empirical constants are not good practice!

The range check is needed in order to prevent the integer part of the float overflowing in https://github.com/arduino/ArduinoCore-avr/blob/2ff00ae7d4e85fa422b7918ee12baf56a1f3006e/cores/arduino/Print.cpp#L247 unsigned long is not necessarily 32 bits so the numeric literal is non-portable. The constant is actually the unsigned long equivalent of INT_MAX - 1. (The -1 is to allow for rounding but further thought needed to be certain of that).

I have a fix which avoids the need for int_part so the problem no longer arises. It also makes the implementation of %g and %e formats relatively trivial.

• Some testing but far from thorough.
• Uses sig figs for %f format instead of decimal places but that can be changed.
• Precision as a parameter not implemented.
• Consideration should be given to printFloat(float f, struct PrintOptions *options)
• It prints a float but that is more than adequate for most purposes.

I would welcome feedback on the various options before I do anything further.

#define F_LARGE 6 /* maximum exponent for F format */
#define F_SMALL -3 /* minimum exponent for F format */ 
#define E_SIG_FIG 4 /* significant figures for E format */
#define F_SIG_FIG 6

// Buffer size. The +1 allows for the rounding digit.
#if E_SIG_FIG > F_SIG_FIG
#define SIZE (E_SIG_FIG + 1)
#else
#define SIZE (F_SIG_FIG + 1)
#endif

void printFloat(float f) {  
    int8_t d;
    char buffer[SIZE];
    bool negative;
    bool Eformat = false;
    uint8_t start = 1; // index of first digit leaving room for a carry from the rounding
    uint8_t dp;     // index of first digit after the decimal point
    uint8_t finish; // index of extra digit used for rounding
    int8_t i;       // loop counter (must be signed)

    if (isnan(f)) { print("nan"); return; }
    if (isinf(f)) { print("inf"); return; }

    negative = f < 0.0;
    if (negative) { f = -f; print('-'); }

    int8_t exponent = 0;
    while (f >= 10.0) { exponent++; f /= 10.0; }
    while (f <   1.0) { exponent--; f *= 10.0; }

    // need one more digit for use when rounding
    if (exponent > F_LARGE || exponent < F_SMALL) {
        // E format
        finish = start + E_SIG_FIG;
        dp = 1;
        Eformat = true;
    }
    else {
        // F format
        finish = start + F_SIG_FIG;
        dp = exponent + 1;
    }

    // store the digit chars into the buffer
    for (uint8_t i = start; i <= finish; i++) {
        d = (uint8_t)f;
        buffer[i] = '0' + d;
        f -= d;
        // Could check for f==0 here and save some multiplies but larger code size
        f *= 10.;
    }

    // rounding
    if (buffer[finish] >= '5') {
        i = finish - 1;
        buffer[0] = '0';
        while (buffer[i] == '9') {
           buffer[i] = '0';
           i--;
        }
        buffer[i]++;
    }    

    if (buffer[0] == '1') {
        start = 0; // there was a carry from the rounding
    }
    else {
        dp++;
    }

    if (exponent >= 0) { // positive exponent
        for ( i = start; i < dp; i++) {
            print(buffer[i]);
        }
        print('.');
        for ( i = dp; i < finish; i++) {
            print(buffer[i]);
        }
    }
    else { // negative exponent
        if (Eformat) {
            print(buffer[start]);
            print('.');
            for ( i = dp; i < finish; i++) {
                print(buffer[i]);
            }
        }
        else { // F format
            print("0.");
            for (i = 1; i < -exponent; i++) {
                print('0');
            }
            for (i = start; i < finish; i++) {
                print(buffer[i]);
            }
        }
    }

    if (Eformat) {
        print('E');
        print(exponent);
    }
}

[matthijskooijman]

You've reported this on the AVR core, whose code does not really need to be portable. In particular, long is always 32-bit and float is identical to double there. You are right that using named constants over magic numbers is a good idea.

However, the ArduinoCore-API repo has this exact same code and that repo is intended to be portable (also intended to replace the AVR-specific code in the AVR repository), so I agree that this should be fixed there.

I will transfer this issue to the ArduinoCore-API repo, where I think it would be better placed.

I have not looked at the implementation in detail, I'll leave that to others.

[MalcolmBoura]

It has occurred to me that the conversion from the literal constant to float in order to carry out the comparison may result in a value a few hundred lower than MAX_INT being needed. All very messy as it depends on how that is implemented. I don't recall it being defined anywhere. Anyway, it would be better to use something human friendly like 1E9.