luxonis / depthai-core

DepthAI C++ Library
MIT License
235 stars 127 forks source link

`getIMUFirmwareUpdateStatus()` float return can not be compared equal to "100" #986

Open diablodale opened 8 months ago

diablodale commented 8 months ago

Because floating point arithmetic is different from real number arithmetic. Bottom line: Never use == to compare two floating point numbers. https://isocpp.org/wiki/faq/newbie#floating-point-arith

The return of getIMUFirmwareUpdateStatus() can not be safely compared to "100". Yet that is exactly what the depthai-core docs on that API write. The depthai-core implementation is hidden to users. It is an RPC call.

pimpl->rpcClient->call("getIMUFirmwareUpdateStatus").as<std::tuple<bool, float>>();

This SDK's example is wrong https://github.com/luxonis/depthai-core/blob/eba7400699e69c81f04dc0dc1e2c2b6aafc3b962/examples/IMU/imu_firmware_update.cpp#L43

All these approaches are unsafe and wrong

// updatePercent can be slightly more or less than exactly 100, and the constant 100.0f can be slightly more or less than exactly 100
// therefore all of these percentage comparison are unreliable
if (updateConcluded && updatePercent == 100.0f)
if (updateConcluded && updatePercent == 100.0)
if (updateConcluded && updatePercent >= 100.0f)
if (updateConcluded && updatePercent >= 100.0)
// here the constant is exactly 100 but the variable has the same problems as above, plus now the compiler is going to change/round the operands to do the comparison
if (updateConcluded && updatePercent == 100)
if (updateConcluded && updatePercent >= 100)

The API is flawed. It needs to do something else like:

I see no workaround possible since it is all hidden in a remote RPC call.

moratom commented 8 months ago

@asahtik let's change the percent to an int here please.