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

Rounding to number of decimals in to_chars or to_string? #340

Closed
elcojacobs opened this issue Dec 11, 2018 · 12 comments
Closed

Rounding to number of decimals in to_chars or to_string? #340

elcojacobs opened this issue Dec 11, 2018 · 12 comments

Comments

@elcojacobs
Copy link

I'm using a safe elastic fixed point, with the following template:

template <uint8_t I, uint8_t F, class Narrowest>
using safe_elastic_fixed_point = cnl::fixed_point<
    cnl::overflow_integer<
        cnl::elastic_integer<
            I + F,
            Narrowest>,
        cnl::saturated_overflow_tag>,
    -F>;

To show some values on a display, I would like to convert the fixed point number to a string with a fixed number of decimals. For example: 20.45697 should be displayed as 20.5.
The to_string and to_char functions seem to give a string representation of the maximum precision.

Is there a simple way to do this with CNL? Can I wrap the value in a rounding integer before using to_string or something similar?

PS Thanks for the awesome library John. I'm about to release a new brewing control platform and all the fixed point math is now using your library.

johnmcfarlane added a commit that referenced this issue Dec 11, 2018
- to_chars(fixed_point) doesn't round to nearest
@johnmcfarlane
Copy link
Owner

Hi Elco and thanks for the kind words. I'm glad to hear you find CNL useful.

Alas, my to_string implementation is currently pretty minimal. It's barely good enough to print truncated values in typical cases. Rounding won't work as to_chars converts the value to native rounding mode first. Fixing this may not be completely trivial due to the interplay between decimal and binary. Also, there are fiddly situations where, e.g. 9.999 gets rounded up and subsequently prints a very different value: 10.00.

I've written a failing test for now but don't think it will be a quick fix. In the meantime, all I can suggest is that you resort to floating-point for displaying the result.

(Note that while you don't get correct rounding, you can truncate the value passed to to_chars by passing in a smaller buffer as show in the failing test.)

@elcojacobs
Copy link
Author

Thanks John,

For the time being, I have implemented a simple workaround that counts the number of digits required for the desired number of decimals and adds a rounder so truncating gives the correct rounded result.
I indeed already noticed that passing a smaller buffer results in truncating, so I just needed to adjust the value a bit before calling the cnl::to_chars.

Here is my simple implementation, in case anyone else has the same requirements.

using temp_t = safe_elastic_fixed_point<11, 12, int32_t>;

void
to_chars(const temp_t& t, char* buf, uint8_t len, uint8_t decimals)
{
    auto digits = decimals + 2;
    auto rounder = temp_t(0.5);

    for (; decimals > 0; decimals--) {
        rounder = rounder / 10;
    }

    temp_t rounded;
    if (t >= temp_t(0)) {
        rounded = t + rounder;
        auto temporary = rounded;
        while (temporary >= temp_t(10)) {
            temporary = temporary / 10;
            ++digits;
        }
    } else {
        rounded = t - rounder;
        auto temporary = rounded;
        while (temporary <= temp_t(-10)) {
            temporary = temporary / 10;
            ++digits;
        }
        digits += 1; // for minus sign
    }

    std::memset(buf, 0, len);
    if (digits > len) {
        digits = len;
    }

    cnl::to_chars(buf, &buf[digits], rounded);
}

It can probably be improved upon quite a bit, but it works well enough for now.
I don't use any floats or doubles in the entire codebase and I'd like to keep it that way. Floats to string isn't cheap in code size either.

@elcojacobs
Copy link
Author

A small change is needed to compile on arm embedded:

using reduced_precision_t = safe_elastic_fixed_point<11, 10, int32_t>;
cnl::to_chars(buf, &buf[digits], reduced_precision_t(rounded));

Otherwise to_chars wants to use a 66 bit wide type, which fails.

@elcojacobs
Copy link
Author

elcojacobs commented Dec 17, 2018

The implementation above still caused soft floating point operators to be included in the build, so I ended up writing this much simpler implementation:

std::string
to_string_dec(const fp12_t& t, uint8_t decimals)
{
    // shift to right number of digits
    auto shifted = t;
    for (uint8_t i = decimals; i > 0; --i) {
        shifted = shifted * 10;
    }

    // ensure correct rounding
    auto rounder = fp12_t(0.5);
    shifted = t >= fp12_t(0) ? shifted + rounder : shifted - rounder;

    // convert to int
    auto intValue = int32_t(shifted);

    // convert to string
    auto s = std::string();
    s = s << intValue;

    // prefix zeros for values smaller than 1
    uint8_t insertPos = (intValue < 0) ? 1 : 0; // handle minus sign
    uint8_t minChars = decimals + insertPos + 1;
    if (minChars > s.size()) {
        s.insert(insertPos, minChars - s.size(), '0');
    }

    // insert decimal point
    s.insert(s.end() - decimals, '.');
    return s;
}

I'm not using std::to_string because it is not available om my arm compiler. I have the << operator overloaded and use sprintf, which is already included in de build by some libraries that depend on it.

std::string&
operator<<(std::string& lh, const int32_t& in)
{
    char temp[33];
    snprintf(temp, 33, "%" PRId32, in);
    lh += std::string(temp);
    return lh;
}

std::string&
operator<<(std::string& lh, const uint32_t& in)
{
    char temp[33];
    snprintf(temp, 33, "%" PRIu32, in);
    lh += std::string(temp);
    return lh;
}

My binary is free of floating point operations again :)

(edit: handle leading zeros for values smaller than 1)

@elcojacobs
Copy link
Author

With the recent changes to to_chars, I have been able to simplify my to_string function that keeps trailing zeros and rounds correctly to this:

std::string
to_string_dec(const fp12_t& t, uint8_t decimals)
{
    static constexpr fp12_t rounders[5] = {cnl::fraction<>(1, 2),
                                           cnl::fraction<>(1, 20),
                                           cnl::fraction<>(1, 200),
                                           cnl::fraction<>(1, 2000),
                                           cnl::fraction<>(1, 20000)};

    if (decimals > 4) {
        decimals = 4;
    }

    auto rounder = (t >= 0) ? rounders[decimals] : -rounders[decimals];
    auto rounded = t + rounder;

    auto digits = decimals + ((rounded < 0) ? 3 : 2);
    auto temporary = abs(rounded);
    while (temporary >= decltype(temporary)(10)) {
        temporary = temporary / 10;
        ++digits;
    }

    char buf[16] = {0};

    cnl::to_chars(buf, buf + digits, rounded);

    return std::string(buf);
}

To work around to_string not being available on ARM I define it before including CNL:

#pragma once

// workaround for std::to_string not being available on arm

#include <sstream>
namespace std {

template <typename T>
std::string
to_string(const T& n)
{
    std::ostringstream s;
    s << n;
    return s.str();
}
}

#include "../cnl/include/cnl/num_traits.h"
#include "../cnl/include/cnl/static_number.h"

#include <cstdint>
#include <type_traits>

template <
    int IntegerDigits,
    int FractionalDigits,
    class Narrowest>
using saturated_elastic_fixed_point = cnl::static_number<
    IntegerDigits + FractionalDigits, -FractionalDigits, cnl::native_rounding_tag, cnl::saturated_overflow_tag, Narrowest>;

using fp12_t = saturated_elastic_fixed_point<11, 12, std::int32_t>;

std::string
to_string_dec(const fp12_t& t, uint8_t decimals);

@elcojacobs
Copy link
Author

While the code above works and compiles, the usage of native_rounding_tag causes floating point instructions to be used, which increases the code size on an embedded platform by an unacceptable amount.
Using CNL allows us to not use floating point math in the entire application. To reintroduce it just for rounding during string conversion is a no go.

Disassembly showing the use of __aeabi_dmul, multiply double * double:

using native_rounding_type = set_rounding_t<decltype(value), _impl::native_rounding_tag>;
        auto const& native_rounding_value = static_cast<native_rounding_type>(value);
 80a41a6:	9700      	str	r7, [sp, #0]
 80a41a8:	f011 fd2a 	bl	80b5c00 <__aeabi_i2d>
 80a41ac:	2200      	movs	r2, #0
 80a41ae:	4b1d      	ldr	r3, [pc, #116]	; (80a4224 <_Z13to_string_decB5cxx11RKN3cnl11fixed_pointINS_16rounding_integerINS_16overflow_integerINS_15elastic_integerILi23ENS_5_impl12wide_integerILi31ElEEEENS_22saturated_overflow_tagEEENS4_19native_rounding_tagEEELin12ELi2EEEh+0xdc>)
 80a41b0:	f011 fd8c 	bl	80b5ccc <__aeabi_dmul>
 80a41b4:	2200      	movs	r2, #0
 80a41b6:	2300      	movs	r3, #0
 80a41b8:	f012 f818 	bl	80b61ec <__aeabi_dcmpgt>
 80a41bc:	b118      	cbz	r0, 80a41c6 <_Z13to_string_decB5cxx11RKN3cnl11fixed_pointINS_16rounding_integerINS_16overflow_integerINS_15elastic_integerILi23ENS_5_impl12wide_integerILi31ElEEEENS_22saturated_overflow_tagEEENS4_19native_rounding_tagEEELin12ELi2EEEh+0x7e>
                    // +ve
                    return to_chars_positive(first, last, value);

@johnmcfarlane
Copy link
Owner

johnmcfarlane commented Apr 27, 2019 via email

@elcojacobs
Copy link
Author

Just seems to not be implemented by the standard library included with arm-none-eabi-gcc. I am using an old version though (5.3) because the platform I use (particle device-os) is still not compatible with newer and better compilers. It is annoying and I have tried to help them to migrate to a newer gcc but without much success so far. Not a real problem though, because including the small template function is a workaround to make the compiler happy and is not actually included in the final binary.

@johnmcfarlane
Copy link
Owner

I get that a lot from embedded developers: the tools always seem to be out of date. One last question: are you able to program in C++14?

@elcojacobs
Copy link
Author

Yes, I use C++14. The original makefiles from particle have std=c++11 even, but I modified the makefiles to use C++14. And I think that's even pretty cutting edge for embedded, unfortunately. Some embedded devs are even stuck in C99.

I'd rather use the latest arm-none-eabi-gcc, because compiler warnings and errors have improved so much, but the dependencies/framework don't officially support it.
I compile almost all of the embedded code with latest gcc to run in simulation for unit and integration testing though (Catch2 for unit). Very rarely debug on target and when developing, by the time I flash the actual hardware I am 99% sure it will work.

We recently switched from Travis to Azure and are quite happy with that. Runs our tests and builds docker images to compile, simulate and flash the hardware. Firmware repo is here:
https://github.com/brewblox/brewblox-firmware

Most of the CNL usage is in /lib, actual application in /app/brewblox.

@elcojacobs
Copy link
Author

elcojacobs commented Nov 21, 2019

Adding for documentation for others:
arm 5.3 seems to be happy with just a forward declaration:

#ifdef __arm__
#include <string>

// forward declare std::to_string. Arm gcc 5.3 compiler cannot find it in headers and cnl has references to it in headers
namespace std {
template <typename T>
string to_string(T);
}
#endif

#define CNL_USE_INT128 false
#define CNL_RELEASE true // prevents using stdio, which is not included when linking

#include "../cnl/include/cnl/num_traits.h"
#include "../cnl/include/cnl/static_number.h"

@johnmcfarlane
Copy link
Owner

I'm going to close this issue now. I'd be very interested in further information -- possibly in the form of new issues that break down the topics raised in this thread:

  • Is there a way to test certain features are float-free that I can put into a CI task? In that way, I could more easily fix the issue and ensure it remains fixed. Ideally, is there a compiler flag which turns the problematic floating-point use into a hard compiler error?
  • Sometimes, it is necessary to compare a scaled_integer to 0.0 in order to avoid an illegal shift. This is tracked in Cannot always compare scaled_integer to 0 #799. Is that case a problem? I'd like to take another stab at fixing it.
  • Assuming you ever get to test on C++20, have recent changes to to_chars made the situation better/worse?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants