Small potential issue with disarming

[ZacharyTaylor]

This may be a small nit-pick but when going through the PX4 code I encountered a line that to me has the potential to be a safety issue: #define NAN_VALUE (0.0f/0.0f)
https://github.com/PX4/Firmware/blob/master/src/modules/px4iofirmware/mixer.cpp#L69

Here a NAN is created by dividing by zero, however this is not guaranteed to create a NAN and is undefined behaviour, as the standard states

C++11 5.6.4 - The binary / operator yields the quotient, and the binary % operator yields the remainder from the division of the first expression by the second. If the second operand of / or % is zero the behavior is undefined.

This could become an issue as this value is used to prevent the motor turning on when the system is disarmed
https://github.com/PX4/Firmware/blob/master/src/modules/px4iofirmware/mixer.cpp#L482

While unlikely, if a compiler were to set this value to a finite number it would still be compliant with the C++ standard but would result in motors running when the system is disarmed.

This is not an issue if IEEE754 floating point numbers are used as there 0.0f/0.0f is explicitly defined as NAN, however unless I missed it the only check performed for this is in the libuavcan module and it is not always enabled.

A possible solution would be a compile time assert on std::numeric_limits<float>::is_iec559. It would also make sense to me to define NAN_VALUE as std::numeric_limits<float>::quiet_NaN()

[dagar]

Small nit-picks are always appreciated.

We should try using std::numeric_limits on NuttX, otherwise what about the NAN macro? I believe it's a GNU extension, but that should be fine.

[dagar]

Pull request - #9280

[dagar]

@ZacharyTaylor while we're on the subject do you have any thoughts for safe floating point usage across the code base? Currently we have -funsafe-math-optimizations enabled (https://github.com/PX4/Firmware/blob/master/cmake/common/px4_base.cmake#L364). This is historical, and to be honest I don't know how much thought went into it.

Question: Should we consider going to full -ffast-math, or the other extreme for full IEEE754 compliance?

• -funsafe-math-optimizations
  • This mode enables optimizations that allow arbitrary reassociations and transformations with no accuracy guarantees. It also does not try to preserve the sign of zeros.
• -fno-errno-math
  • disables setting of the errno variable as required by C89/C99 on calling math library routines
• -ffast-math flag
  • enables -fno-trapping-math, -funsafe-math-optimizations, -ffinite-math-only, -fno-errno-math, -fno-signaling-nans, -fno-rounding-math, -fcx-limited-range and -fno-signed-zeros. Each of these flags violates IEEE in a different way

Reference: GCC Floating Point Math https://gcc.gnu.org/wiki/FloatingPointMath

[dagar]

Disable math-error in #9281.

[ZacharyTaylor]

Thanks a lot for the quick reply and pull request I will try give it a good look through either tomorrow or Thursday.

I am by no means an expert on compiler optimizations or floating point numbers. However, I feel that the current practice of setting an output to nan to disable it may cause some potential issues with the optimization flag -ffinite-math-only which -ffast-math applies. For example with this flags enabled I believe gcc and clang both erroneously give that nan - nan = 0. This ability for a nan to be transformed to a finite value under these optimization gives the potential for the system to run the motors while disarmed. I see these situations as quite unlikely, but as this has the potential to cause injuries I personally would err on the side of caution