merbanan / rtl_433

Program to decode radio transmissions from devices on the ISM bands (and other frequencies)
GNU General Public License v2.0
6.07k stars 1.31k forks source link

Round computed value to remove many floating point digits in rtl_433_mqtt_hass.py #2208

Closed joanwa closed 1 year ago

joanwa commented 1 year ago

I use rtl_433_mqtt_hass.py in home assistant to read sensor data from a weather station and always end up having values like

4.77778 °C for temperature or 227.10001 mm for the rain sensor.

The rounding could be done in home assistant, or here in rtl_433_mqtt_hass.py. I think here makes more sense in the mappings' value template, since we know the unit and decimal points which make sense for them. e.g. 1 decimal for temperature, and perhaps also one for the rain sensor in mm

merbanan commented 1 year ago

Well this is something that should be solved in the presentation layer. Floating point representation in computers have this "feature". If the storage type is float rounding will probably not help.

joanwa commented 1 year ago

I mean

    "temperature_C": {
        "device_type": "sensor",
        "object_suffix": "T",
        "config": {
            "device_class": "temperature",
            "name": "Temperature",
            "unit_of_measurement": "°C",
            "value_template": "{{ value|float }}",
            "state_class": "measurement"
        }
    }   

and

 "rain_mm": {
        "device_type": "sensor",
        "object_suffix": "RT",
        "config": {
            "name": "Rain Total",
            "unit_of_measurement": "mm",
            "value_template": "{{ value|float }}",
            "state_class": "total_increasing"
        }
    }

could use the same rounding as rain_in already does:

    "rain_in": {
        "device_type": "sensor",
        "object_suffix": "RT",
        "config": {
            "name": "Rain Total",
            "unit_of_measurement": "mm",
            "value_template": "{{ (float(value|float) * 25.4) | round(2) }}",
            "state_class": "total_increasing"
        }
    }
zuckschwerdt commented 1 year ago

As @merbanan said we are doing the best we can to transmit float values, formatting nice values is always up to some display logic.

That change to the value_template is exactly where this should be applied :) PR welcome.

joanwa commented 1 year ago

great, PR is out: https://github.com/merbanan/rtl_433/pull/2209

kuenkin commented 1 year ago

Which order of decimals makes sense could be related more to the precision of the sensor. For instance, with this change inovalley rain meter with a rain counter of 0.45 mm goes to 0.5 mm for no real reason...

zuckschwerdt commented 1 year ago

I was going to say that people might want 2 decimals, but then it's just for display and no decimals would still be okay. For the Inovalley-kw9015b that 0.45 mm is the step width, so you will never be off, just rounded sensibly.

Rounding ``` 0.45 -> 0.5 0.9 -> 0.9 1.35 -> 1.4 1.8 -> 1.8 2.25 -> 2.3 2.7 -> 2.7 3.15 -> 3.2 3.6 -> 3.6 4.05 -> 4.1 4.5 -> 4.5 ```
kuenkin commented 1 year ago

But, If I want to know how much rain was the last two days and, those days the rain was just a step each day, the global sum will be 1 mm instead of 0.9mm, or am I wrong? And in the same way, if each day of one month there is just one step of rain, the global sum displayed will be 15 mm instead of 13.5 mm, what would be a 14% of error, don't think that rounding is so sensible for a rain meter where the values are cumulative, just my thoughts... Anyway, thank you very much for this program and let the rounding be done in home assistant depending on the personal preferences. By the way, none of my sensors, with good reception, shows strange values with many decimal places. I got that kind of values with sensors that are far away from my reception point. Mainly neighbour's sensors.

zuckschwerdt commented 1 year ago

You might be right on the rounding error, depending where the aggregation is done. Is HA the datastore for your weather info? I never investigated that, are there long term statistics in HA? I dump all my values in a time series database then view, even multi-year, in Grafana.

kuenkin commented 1 year ago

Yes, HA is my datastore as it support LTS: https://data.home-assistant.io/docs/statistics/

zuckschwerdt commented 1 year ago

@joanwa we should round to 2 decimals then. I guess everything up to maybe 4 decimals will still look the same anyway, right?

joanwa commented 1 year ago

just for the rain sensor or also the temperature sensor? All weather stations I've come across, e.g. switchdoc labs outdoor and indoor sensors, ecowitt Wittboy specify a sensor resolution of 0.1 K.

Re rain sensors WeatherSense Wireless WeatherRack2 state a resolution between 0.3mm and 1mm (mechanical sensor) https://shop.ecowitt.com/products/wittboy states 0.1mm with an electrical sensor

therefore I thought one digit for all is sufficient. Happy to bump the rain sensor to 2 digits, even though I find it hard to believe that any mechanical rain sensor can be that sensitive.

zuckschwerdt commented 1 year ago

It's more about the conversion factor than actual resolution. And just the rain values are of concern here as those are aggregated and not an absolute value like temperature, humidity and such. Just rain values need the higher precision.

knarrff commented 1 year ago

Just to add my two cents: Even if the accuracy of a given sensor is not that high, say 0.1 degrees for a thermometer, its resolution might be a lot higher and also interesting despite the lower accuracy. Take devices that use a BME280 for example. It's temperature accuracy is quite bad: +-1 degree, but its resolution is 0.01 degrees, and here I would be interested in all those digits. Think of external calibration or maybe I am not interested in the absolute value but the relative changes only.

Now, this is not one of my actual use cases - all of my rtl_433-relevant sensors do send fewer digits. However, the range of devices is huge and I don't have an overview of all the ones supported by rtl_433 now or in the future. This is not a problem with 'base' rtl_433 either, since the change was just in one of the examples. I just wanted to bring this up in case it ever comes up with 'base' rtl_433.

zuckschwerdt commented 1 year ago

@knarrff good point. Totally agree on the BME280, I can see if a window two rooms away was opened precisely. And that's the use-case here as the absolute temperature is off by up to 3 degrees ;)

Here it comes down to ask/answer the question: Can we, for all users, assume that temperature values going into HA are meant for a general overview on a dashboard only? I'm leaning towards a clear "yes", I'm not aware of any fancy e.g. "window-open" tracking/alerting, but I don't really use HA.

kuenkin commented 1 year ago

In my case, and for many people in HA, I think, not only for an overview but also to track such changes and get alerts/notifications or do actions, which is the magic of HA....so I think that assumption not always true. And for dashboard purposes, HA already does the rounding by itself...

zuckschwerdt commented 1 year ago

@kuenkin most prominent case first: do you want to track 1/100 degree (precision) changes and have/know of supported sensors that transmit those?

HA already does the rounding by itself...

But what were those outputs in the first post? Should we maybe just round off the 5th decimal? Float accuracy should work well with 4 decimals.

gdt commented 1 year ago

I don't think we can assume "dashboard for humans only". There could be all sorts of consumers. I find 0.01 degree (actual sensor resolution) useful occasionally in HA, when I as a human look at graphs. I would want want to round to 0.1. Whether there is code to notice these things now, I don't know, but it's anything that can read/interpret the HA database.

I do not see HA rounding values in an entity card when the sensor value_json doesn't have rounding.

What i see this as really being about is not turning something that is natively 0.01F or 0.01C resolution into values with 10 digits after the decimal. Doing this right in a systematic way requires metadata on resolution. But, it might be that rounding temps to 0.01 degree or 0.001 degree is good enough to remove most of the annoyance and safe enough.

Beyond this issue, I'd like to think about rounding to not too many when doing the native temperature conversions. From the science viewpoint, writing values with excess figures not supported by anything isn't good, but converting back should get you were you started from. That sort of argues for 0.001 in temp, given that 0.01 temp sensors are normal and I haven't seen one that has resolution of 0.001 yet (in any context that approaches rtl_433 or maker parts).

zuckschwerdt commented 1 year ago

For the MQTT and JSON output we already round to 5 decimals (and truncate zeros), but that still leaves conversion or representation errors as shown in the first post (4.77778, 227.10001) https://github.com/merbanan/rtl_433/blob/2e3032d2805937111ac44151ddc58b61cf015cdc/src/output_mqtt.c#L405-L419 https://github.com/merbanan/rtl_433/blob/063294f308efe57f38e0c2fea87fc2d549ab5c3a/src/data.c#L513-L530

gdt commented 1 year ago

I don't think it is good to truncate trailing zeros. It makes the precision appear to vary. I find a display that says 72.9, 73, 73.1 to be very strange.

5 is pretty much as good as 4. Fixing it requires carrying precision from the driver.

If all sensors were metric and people only used metric this would be easier :-)

zuckschwerdt commented 1 year ago

display that says 72.9, 73, 73.1 to be very strange.

Very true, esp. JSON needs that as type hint, and indeed we do keep the first decimal ;)

gdt commented 1 year ago

73.09, 73.1, 73.11 is also odd. I don't think 0 should be treated specially.

zuckschwerdt commented 1 year ago

We expect a consumer (esp. JSON) to read the value (it's not a quoted string) as float number -> trailing zeros can't be represented there. With MQTT value topics the payload is always a string, but I would expect consumers to also parse there?

gdt commented 1 year ago

I see your point. This is all unfortunately messy and I'm not seeing a fix short of carrying precision as metadata.

kuenkin commented 1 year ago

@kuenkin most prominent case first: do you want to track 1/100 degree (precision) changes and have/know of supported sensors that transmit those?

Well not really need that precision of thousandths but, don't know if other people does. You and knarrff already reported an useful case of that accuracy even when the sensor could be offset, so really don't know what to say....

knarrff commented 1 year ago

Can we, for all users, assume that temperature values going into HA are meant for a general overview on a dashboard only? I'm leaning towards a clear "yes"

You may be right with HA and dashboards. But then, who are we to decide what people use HA dashboards for. But the maybe here more important thing is: that does not really matter for rtl_433. As far as I understand it, rtl_433 has one job: to decode messages over the air and to transmit the contained information further on for whatever use the user intents to use it. As such, it should not be its job to alter that information, especially not on the basis of assumptions that may, but also may not be true.

However, I also think that while there are cases when precision of the passed-on float should not be rounded, there are also cases when it would be ok. This (original) bug report gives both examples:

4.77778 °C for temperature or 227.10001mm

The value "4.77778" was probably created due to a unit conversion. There is no good way to round that value to a "nicer", but still as accurate float in any reasonable way, at least not without information about the original resolution of that sensor. At least the code in question here does not have that info - maybe the decoder could - there is only so much you can put in a few bits.

The value "227.10001" on the other hand was quite likely generated by the necessarily imprecise representation of "227.1" in a floating point number. I don't see a reason against "rounding" this to "227.1". In theory, of course also here, the same argument would apply: how should the python script know that the precision of that sensor isn't that high? It would be harder to make that case, but coming back to the issue at hand here:

The job of rtl_433 is data decoding and forwarding. I agree with @merbanan:

this is something that should be solved in the presentation layer.

I don't see rtl_433 to be part of the presentation layer.

I am also sorry to have stirred up this again so much. Rounding within an example mqtt script is not too bad. I don't think that it sets a good example, but it is only an example script and not the core of the tool. rtl_433 can send the data directly to mqtt, and as long as there nothing is rounded, I remain happy.

Also and before I forget: a BIG thanks and shoutout to the developers, maintainers and all the other volunteers here!

zuckschwerdt commented 1 year ago

sorry to have stirred up this again so much.

It always takes different views and use-cases to get a good perspective on even minor technical things. And if people take time to layout out their needs or view on things I always want to hear that. If that drives a healthy discussion even better.

We might need to move this "example" script to it's own repo, to round it off wih some docs, maybe container-build and own development pace.

gdt commented 1 year ago

This issue has become inactive. I think where we have landed is:

I think the way forward is to write down the plan, and then deviations if any can be fixed with PRs and issues if necessary.

Does that sound like a fair summary?

knarrff commented 1 year ago

If rtL433 proper starts using "a reasonable number of digits", users should be able to disable that, because what is "reasonable" is debateable, as we can see in this report.

zuckschwerdt commented 1 year ago

Look for the print_double functions, e.g. the JSON output uses "%.3f" https://github.com/merbanan/rtl_433/blob/master/src/output_file.c#L98 and the MQTT output for not too big/small nubmers uses "%.5f" with trailing zeros removed https://github.com/merbanan/rtl_433/blob/master/src/output_mqtt.c#L421 currently.

gdt commented 1 year ago

@knarrff You are quoting me out of context. I said avoiding precision that is obviously goofy, which is quite different from using just enough for some opinion about what's enough. No one has suggested that we need micro-degrees of temperature vs millidegrees. We aren't anywhere near whether 0.01 or 0.1 is adequate. And we have started -- and nobody has complained that I have seen.

gdt commented 1 year ago

I reviewed the mqtt_hass code and it is mostly ok. I'm going to call this done and open a new issue that is limited in scope.