triffid / FiveD_on_Arduino

Rewrite of reprap mendel firmware
http://forums.reprap.org/read.php?147,33082
GNU General Public License v2.0
30 stars 12 forks source link

decfloat_to_int is broken #16

Closed sw closed 13 years ago

sw commented 13 years ago

Since the release branch has been merged into master, the calculation of steps was completely wrong (number of steps was more than 10 times too big).

The problem is with these lines in decfloat_to_int():

int32_t rnew1 = r * (multiplicand / denominator);

int32_t rnew2 = r * (multiplicand % denominator);

r = rnew1 + rnew2;

I couldn't figure out what the intent of these lines was, so I rewrote the function so that it hopefully works in all cases. I also changed the arguments slightly to use a exponent instead of denominator, because this value was one of 1, 10, or 1000 in all cases.

sw commented 13 years ago

I see that Traumflug has already found the error. You still may want to use my function, because it should round correctly and will not overflow.

Traumflug commented 13 years ago

sw,

you're right about the rounding enhancement and about making decfloat_to_int() static.

The changes in the decfloat struct remind me of taking that into the overflow calculations as well (7 bits are plenty for storing up to 5, right?).

Other than that, your algorithm is not only a lot more complex than Jacky2k's, but also introduces a lot more of these expensive multiplications and divides. For a value of 100.000 I count about 9 to 12 divides and 3 to 7 multiplications, which is clearly more than Jacky2k's 4 divisions and 2 multiplications.

FWIW, I had to admit his algorithm is better than all of my countless tackles against this problem as well.

sw commented 13 years ago

I fixed it so the function is more similar to the original, but rounding should be correct in all cases.

triffid commented 13 years ago

pulled, but I merged the two commits into one to clean up all the decfloat_to_int denominator stuff.

thanks for this excellent work :)