arjenhiemstra/ithowifi

Rounding errors in statusvalues

Closed this issue · 11 comments

Describe the bug
Rounding errors in statusvalues. The floats in IthoStatus instead of doubles lead to round off errrors.

To Reproduce
Steps to reproduce the behaviour:
Install master

Expected behaviour
The value 23.06 should be 23.06 not 23.059999xx
The value 13.85 should be 13.85 not 13.85000xxxx

Screenshots
image

Device information

  • Firmware version: master, 2.8.x

I do know how, but just rounding floats to 4 orso digits should solve this.
(Or revert to doubles)

Statusvalues I could just round in HomeAssistant, but in settings it is quite annoying:

image

that's interesting and unexpected! The change from doubles to floats cause these rounding issues? (I cannot check this because I do not have any devices with these kind of values)

Yep sure it can as there are some values which can't be exactly represented as float (or as double)
Only when printing a double, the print function often stops at less decimals than the resolution you can store in a double.
I think the default is 10 decimals for most string conversion functions if no nr of decimals is set.
Given you can store about 18 decimal values in a double, you need to have double values of over 1E9 (and some binary repeating fraction) before you ever encounter these issues.
With floats you can only store ~6 decimals (slightly more).

So you must do the formatting to N decimals yourself.

The problem seems to arise because ArduinoJson uses doubles internally. Set ‘’ARDUINOJSON_USE_DOUBLE’ to 0

bblanchon/ArduinoJson#1724 (comment)

Ok, cool stuff, I vaguely remembered something so I went to my release notes and found this:
"fix: correct rounding issues since ArduinoJSON library update"

history repeating itself but then with a different part of the code...

Ok, I would propose the following:

  1. to keep the structs ithoDeviceStatus and ithoDeviceMeasurements to floats and round the values using the existing round() function to 2 (?) decimals?
    That saves 4 bytes memory per instance of the struct, which on some devies with a lot of status labels can make a substantial difference.

to change these lines:

float cur = 0.0;
float min = 0.0;
float max = 0.0;

to doubles again. These would bring back the settings values to the correct values again.

Also if you have similar code like this, you should add a f next to the "magic" value.

 float max = 0.0f; 

Or else there will be an extra double to float conversion in the code. (not always as the compiler sometimes can detect this behavior and optimize for it)

I just tried master 6f5a113, sadly, it does not work as expected:

image

Thanks for trying! Will look into further

Ok, cool stuff, I vaguely remembered something so I went to my release notes and found this: "fix: correct rounding issues since ArduinoJSON library update"

history repeating itself but then with a different part of the code...

Ok, I would propose the following:

  1. to keep the structs ithoDeviceStatus and ithoDeviceMeasurements to floats and round the values using the existing round() function to 2 (?) decimals?
    That saves 4 bytes memory per instance of the struct, which on some devies with a lot of status labels can make a substantial difference.

to change these lines:

float cur = 0.0;
float min = 0.0;
float max = 0.0;

to doubles again. These would bring back the settings values to the correct values again.

my oh my @tomkooij, its even worse, sorry:
#142 (comment)

I just tried master 6f5a113, sadly, it does not work as expected:

image

and I think I now what causes it:

float round(float value, int precision)
{
float pow10 = pow(10.0, precision);
return static_cast<int>(value * pow10 + 0.5) / pow10;

this has to be changed to double again also of course!!

@arjenhiemstra Yep, that fixed that status values. (setting values are fixed as well, but probably already in the first commit)
image

Thanks!