Fix test failures on i386 platform related to floating point issues

git-developer commented 1 year ago

This PR fixes floating point related issues. The issues can be reproduced when building for target platform linux/386.

Logs:

Excerpt from the build log:

#51 0.608 /opt/mqmgateway/source/unittests/exprconv_tests.cpp:122
#51 0.608 ...........................................................................................................................................................................................................................................................................................................
#51 0.608 
#51 0.609 /opt/mqmgateway/source/unittests/exprconv_tests.cpp:136: FAILED:
#51 0.609   REQUIRE( output.getString() == "-1" )
#51 0.609 with expansion:
#51 0.609   "-32768" == "-1"
...
#51 1.373 /opt/mqmgateway/source/unittests/mqtt_named_scalar_conv_tests.cpp:36
#51 1.373 ...........................................................................................................................................................................................................................................................................................................
#51 1.373 
#51 1.373 /opt/mqmgateway/source/unittests/jsonutils.cpp:11: FAILED:
#51 1.373   REQUIRE( d_current == d_expected )
#51 1.373 with expansion:
#51 1.373   {?} == {?}
...
#51 1.666 /opt/mqmgateway/source/unittests/mqtt_state_map_conv_tests.cpp:35
#51 1.666 ...........................................................................................................................................................................................................................................................................................................
#51 1.666 
#51 1.666 /opt/mqmgateway/source/unittests/jsonutils.cpp:11: FAILED:
#51 1.666   REQUIRE( d_current == d_expected )
#51 1.666 with expansion:
#51 1.666   {?} == {?}
#51 1.666

The test failures seem to be caused by casts between double and int. I can't tell why this only happens on i386 platform, though; could be related to different compiler defaults for floating point calculations (just a guess).

Problem 1

exprconv_tests:136: int16_t(regValue) results in -32768 instead of -1. The fix uses the toNumber() function, as all other custom functions do. I also had success with int16_t(static_cast<uint16_t>(regValue)).

Problem 2

The other 2 tests fail because of a diff in the JSON document:

mqtt_state_map_conv_tests:35:

  current.c_str(): {"sensor1":12.339,"sensor2":10.0}
  expected: {"sensor1": 12.34, "sensor2": 10.0}

mqtt_named_scalar_conv_tests:36:
```
  current.c_str(): {"some_name":3245.599}
  expected: { "some_name": 3245.6}
```
This problem can be narrowed down to the dummy variable in ConverterTools::round(); it seems to be caused by casts between double and int.

Conclusion

After struggling with these rounding related issues, I have a suggestion. I think that in ConverterTools:

static double round(double val, int decimal_digits)

should be replaced by

static std::string format(double val, int decimal_digits)

The reason is that there's no proper way to round a floating point variable to an exact value. The converters apply roundings just before creating an MqttValue, finally resulting in a string in the JSON. So in my opinion it's safer to return a string instead of a double as rounding result, to be sure that the value looks exactly as requested.

The only location that uses the rounding result as number is DivideConverter.toModbus(), but the value is stored as int32_t so the effect of rounding looks quite limited to me.

If you agree that rounding should result in a string instead of a double, I will open another PR.

BlackZork commented 1 year ago

I see. I would rather move precision var to MqttValue(double val) constructor and MqttValue::fromDouble and use it when MqttValue::mType is SourceType::DOUBLE and MqttValue::toString is called.

This way we avoid multiple conversions in future when payload_type: binary will be supported.

git-developer commented 1 year ago

That's a good idea. I added a commit here, because a second branch would only produce conflicts and work.

I found out that it's not enough to return a correctly formatted string in getString(), because createConvertedValue() uses getDouble() when writing the value to JSON. One could use rapidjson::Writer.SetMaxDecimalPlaces() to handle this, but this would require exporting the precision outside of MqttValue.

I chose a different approach by rounding the double value when it is set to MqttValue. This results in a double value with the desired precision in JSON, is compatible to the current code base and fixes the issues on the i386 platform.

git-developer commented 1 year ago

The original number-based rounding algorithm works if the int-cast is replaced by std::round, i.e. no string conversions are required for rounding. I changed that accordingly.

BlackZork / mqmgateway