Possible division by zero in tilt calculations fix for issue 469

universam1 / iSpindel

electronic Hydrometer

http://www.ispindel.de

Other

827 stars 322 forks source link

Possible division by zero in tilt calculations fix for issue 469 #470

Closed pppedrillo closed 3 years ago

pppedrillo commented 3 years ago

Fix for issue https://github.com/universam1/iSpindel/issues/469

MarcoCLA commented 3 years ago

The refactor of the code I agree with. But why would you want to return 0 if no angle could be determined? If it is clearly a hardware failure then it should not return an angle. A check could be done at boot as part of the initAccel().

pppedrillo commented 3 years ago

Sure we can return anything in this case - the value doesnt make sense anyway. But "0" is easy to spot - eg user will rotate iSpindel and see tilt is still "0" (not a "NaN" like it is now). It was like that in the version up to 6.3 I believe, before this "improved" formula with came in - tilt was zero in case of hw failure.

ErikdBr commented 3 years ago

Sure we can return anything in this case - the value doesnt make sense anyway. But "0" is easy to spot - eg user will rotate iSpindel and see tilt is still "0" (not a "NaN" like it is now). It was like that in the version up to 6.3 I believe, before this "improved" formula with came in - tilt was zero in case of hw failure.

It was indeed zero up to 6.3-ish. Zero is better then NaN.

MarcoCLA commented 3 years ago

In my opinion returning NaN (or something else which is not a valid value) is better in case of a hardware failure and can be easily detected at boot. An angle of 0 is technically a valid angle and can occur. A acceleration of 0 on all three axis (which causes the error) not. If the choice is made to return 0, will the temperature sensor code be changed to be in line with the accelgyro? Then that one should also just return 0 if no sensor is found (currently the iSpindel will just not continue booting).

It does not really matter which of the two implementations is chosen, but I think they should be the same, so the two choices:

Stop booting if a problem with the temp/accel sensor is detected
Just return 0 if the sensor is problematic.

pppedrillo commented 3 years ago

Then that one should also just return 0 if no sensor is found (currently the iSpindel will just not continue booting).

I completely agree and frankly couldn't understand from the very beginning why iSpindel is treating missing or not working temp sensor in that specific way.

iSpindel can work without accel/gyro as an overcomplicated thermometer. iSpindel can work without temp probe as a hydrometer. While it can sounds weird, both usecases make sense to me.