Rounding errors in statusvalues

[tomkooij]

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

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)

[tomkooij]

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

[arjenhiemstra]

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)

[TD-er]

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.

[tomkooij]

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

https://github.com/bblanchon/ArduinoJson/issues/1724#issuecomment-1059849539

[arjenhiemstra]

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.

2. to change these lines: https://github.com/arjenhiemstra/ithowifi/blob/8ce7b8725617c57b0e2d161c740b633f975fce34/software/NRG_itho_wifi/main/IthoSystem.cpp#L301-L303 to doubles again. These would bring back the settings values to the correct values again.

[TD-er]

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)

[tomkooij]

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

[arjenhiemstra]

Thanks for trying! Will look into further

[arjenhiemstra]

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:

https://github.com/arjenhiemstra/ithowifi/blob/8ce7b8725617c57b0e2d161c740b633f975fce34/software/NRG_itho_wifi/main/IthoSystem.cpp#L301-L303

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

my oh my @tomkooij, its even worse, sorry: https://github.com/arjenhiemstra/ithowifi/issues/142#issuecomment-1454741692

[arjenhiemstra]

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

and I think I now what causes it: https://github.com/arjenhiemstra/ithowifi/blob/6f5a113a1b836c3d70584b624efd02469f3d7a05/software/NRG_itho_wifi/main/generic_functions.cpp#L463-L466

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

[tomkooij]

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

Thanks!