gbmhunter / MFixedPoint

MFixedPoint is a header-only fixed-point C++ library suitable for fast arithmetic operations on systems which don't have a FPU (e.g. embedded systems).. Suitable for performing computationally intensive operations on a computing platform that does not have a floating-point unit (like most smaller embedded systems, such as Cortex-M3, CortexM0, ATmega, PSoC 5, PSoC 5 LP, PSoC 4, Arduino platforms e.t.c). Common applications include BLDC motor control and image processing. Best performance on a 32-bit or higher architecture (although 8-bit architectures should still be fine).

MIT License

158 stars 32 forks source link

Use BaseType rather than int32_t as input for FpFMultiply #96

Closed TheZoq2 closed 5 years ago

TheZoq2 commented 5 years ago

I think I discovered and fixed a pretty nasty bug in this library. To start off, here are the symptoms:

    mn::MFixedPoint::FpF<int64_t, int64_t, 20> a = -3918.9923265993266;
    mn::MFixedPoint::FpF<int64_t, int64_t, 20> b = -0.0133;

    REQUIRE(round((double) (a*b)) == 52);

Clearly, the result of the above code should be ~52, however, the actual output is 2. If the fractional bit amount is 19, the output is correct.

As far as I can tell, the issue comes from the fact that FpFMultiply uses 32 bit values as inputs even if the BaseType used is 64 bit.

gbmhunter commented 5 years ago

Nice find @TheZoq2 ! Can you please change your MR request so it will merge into develop? I will bring your fix into develop, write some unit tests around this bug fix, and then make a new release to master.

gbmhunter commented 5 years ago

@TheZoq2 I just found out I can change the base branch myself. I will hopefully merge this fix soon!

codecov-io commented 5 years ago

Codecov Report

Merging #96 into develop will not change coverage. The diff coverage is 100%.

@@           Coverage Diff            @@
##           develop      #96   +/-   ##
========================================
  Coverage    92.07%   92.07%           
========================================
  Files            2        2           
  Lines          227      227           
========================================
  Hits           209      209           
  Misses          18       18

Impacted Files	Coverage Δ
include/MFixedPoint/FpF.hpp	`100% <100%> (ø)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 3db2782...44f2573. Read the comment docs.

TheZoq2 commented 5 years ago

Great, thanks! In the mean time, I found another similar issue.

    using Fp = mn::MFixedPoint::FpF<int64_t, int64_t, 25>;
    Fp a = 75;

    REQUIRE(a == 75); // Fails because a == -53

This seems to be caused by the constructors from ints cast the values to int32_t instead of BaseType. My latest commit seems to fix that issue