Dubious code in printFloat(double number, uint8_t digits)

Open
#172 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
5/5
Estimated time
Over a week
Newbie friendliness
25/100
Issue type
Refactor
Clarity
Needs clarification
Activity status
Stale
Tech stack
cpp
Domain
embedded-iot

Research direction

Start by reviewing cores/arduino/Print.cpp, especially printFloat and the linked range-check code. Clarify the desired formatting, precision parameter, float-versus-double behavior, and whether %g and %e are in scope before changing the implementation. Done requires agreement on those options and testing beyond the limited testing described.

Written by the indexing model from the issue text.

Description

bug

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);
    }
}
Dominant language
C++
Stars
306
Forks
150
PR merge metrics
No merged PRs in 30d

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from arduino/ArduinoCore-API

All issues in arduino/ArduinoCore-API

Similar issues

More C++ issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.