mike-matera/FastPID

Appying bit shift to possibly negative value.

TBAMax opened this issue · 4 comments

TBAMax commented

In the step() function in the end there is:
// Remove the integer scaling factor.
int16_t rval = out >> PARAM_SHIFT;
out variable type here is: int64_t out
When applying shift to the negative integer, the sign bit gets shifted also and may mess up the result.
So when the limits are set to allow also for negative values, what happens? Or are only postitive values allowed, and I missed that? Seems to be no check done for that in the code to allow only positive values.

mike-matera commented

Hi! I think you're right! I can't believe I didn't test for this issue. Let me take a look and see if I can make a test and fix the bug.

TBAMax commented

Hi! It is possible that compiler already takes this into account, and there is no issue (this is most likely). I have been thinking about this meanwhile.

  • 👍1
mike-matera commented

I wrote some new tests. According to the documentation doing this in C/C++ causes undefined behavior. It seems that on my x86-64 machine that behavior is an arithmetic shift that makes negative numbers come out okay. I don't have an easy way to test this on Arduino right now. Do you?

mike-matera commented

I've done some more reasearch. The C++ standard states:

For negative a, the value of a >> b is implementation-defined (in most implementations, this performs arithmetic right shift, so that the result remains negative).

So I went looking for any information about GCC. What I found came from a mailing list post that states:

For right shifts, if the type is signed, then we use an arithmetic shift; if the type is unsigned then we use a logical.

Since the out variable is declared as an int64_t, a signed type, I think the bit shift operator will work as expected as long as you're using GCC. This squares with my observations compling the test harness on GCC for x86-64. The Arduino compiler uses GCC so I think this holds with any of the ATMega chips as well as the ARM cores. I'm not sure about others.

Thank you for pointing this out! I'm closing the bug for now because I think that even though it's somewhat bad code it's fast and it works on GCC platforms.