arendst / Tasmota

Alternative firmware for ESP8266 and ESP32 based devices with easy configuration using webUI, OTA updates, automation using timers or rules, expandability and entirely local control over MQTT, HTTP, Serial or KNX. Full documentation at
https://tasmota.github.io/docs
GNU General Public License v3.0
22.19k stars 4.81k forks source link

Inconsistant distance unit for vl53l0x distance sensor #17021

Closed heligan closed 1 year ago

heligan commented 2 years ago

PROBLEM DESCRIPTION

I am using a vl53l0x distance sensor with latest tamota firmware (12.2). However, I noticed an issue when the sensor data is published to home assistant. Basically the tasmota web UI shows xxx mm

Screen Shot 2022-11-04 at 9 36 18 PM

while the data published to HA says xxx cm (10x larger).

Screen Shot 2022-11-04 at 9 36 48 PM

I did check some of the tasmota code. It seems the auto discover part uses cm by default:

https://github.com/arendst/Tasmota/blob/development/tasmota/tasmota_xdrv_driver/xdrv_12_home_assistant.ino#L49

however, different distance sensor code uses their own interpretation of the value. e.g. vl53l0x uses a uint16_t to represent a mm-based distance

https://github.com/arendst/Tasmota/blob/development/tasmota/tasmota_xsns_sensor/xsns_45_vl53l0x.ino#L79

SR04 sensor translates the raw data into a float-value to represent a cm-based distance

https://github.com/arendst/Tasmota/blob/development/tasmota/tasmota_xsns_sensor/xsns_22_sr04.ino#L45

Not sure how this confusion populates into later stages (e.g. web UI or mqtt or HA) but this definitely shows up when it gets published to HA side. I can see temperature sensors usually have a temp unit attached but it doesn't seem to be the same with distance sensors.

Any idea how to address this issue?

REQUESTED INFORMATION

Make sure your have performed every step and checked the applicable boxes before submitting your issue. Thank you!

00:49:17.995 CMD: Backlog Template; Module; GPIO 255 00:49:18.043 MQT: stat/garage_mini/RESULT = {"NAME":"ZTMini","GPIO":[1,1,1,1,608,640,1,1,1,1184,160,1,1,4704],"FLAG":0,"BASE":18} 00:49:18.265 MQT: stat/garage_mini/RESULT = {"Module":{"0":"ZTMini"}} 00:49:18.522 MQT: stat/garage_mini/RESULT = {"GPIO0":{"0":"None"},"GPIO1":{"0":"None"},"GPIO2":{"0":"None"},"GPIO3":{"0":"None"},"GPIO4":{"608":"I2C SCL"},"GPIO5":{"640":"I2C SDA"},"GPIO9":{"0":"None"},"GPIO10":{"0":"None"},"GPIO12":{"0":"None"},"GPIO13":{"1184":"DHT11"},"GPIO14":{"160":"Switch1"},"GPIO15":{"0":"None"},"GPIO16":{"0":"None"},"GPIO17":{"4704":"ADC Input"}}

- [ ] If using rules, provide the output of this command: `Backlog Rule1; Rule2; Rule3`:
```lua
  Rules output here:
- [ ] Set `weblog` to 4 and then, when you experience your issue, provide the output of the Console log:
```lua
  Console output here:

TO REPRODUCE

Steps to reproduce the behavior:

just try a vl53l0x sensor and publish the data to home assistant.

EXPECTED BEHAVIOUR

A clear and concise description of what you expected to happen.

They should use the same unit.

SCREENSHOTS

If applicable, add screenshots to help explain your problem.

see description.

ADDITIONAL CONTEXT

Add any other context about the problem here.

(Please, remember to close the issue when the problem has been addressed)

sfromis commented 1 year ago

The sensor payload only has units included for values where you have an option to select what unit you want, like C/F.

But yeah, as things are now, Tasmota does not have options to bridge the difference in what default units the drivers are using. Simplest might be an option

Jason2866 commented 1 year ago

https://github.com/arendst/Tasmota/blob/development/tasmota/tasmota_xdrv_driver/xdrv_12_home_assistant.ino#L49 is not used anymore for announcing devices to HA.

barbudor commented 1 year ago

As of today, only 1 unit of a given physical value can be pushed and there is no consensus if a distance driver should use either mm or km as either are valid depending on the use case

It could be easy to modify VL53Lxx and SR04 drivers to include a Unit in their respective payload but that would not mean that HA would be able to take it into account without improvement in the HaTasmota plugin. Happy to do that if this can help (the tasmota part of course, not the HA :))

Jason2866 commented 1 year ago

https://github.com/emontnemery/hatasmota/blob/master/hatasmota/sensor.py#L116 looks like HA assumes distance are in "cm" Adding Units is not common in Tasmota. Maybe there is a non breaking change possible to announce all distances in Tassmota Discovery only in "cm"?

arendst commented 1 year ago

Why not change all distance drivers to cm. Makes it a breaking change with regards to values for some drivers but then it's standard on all of them.

I'm in favour.

sfromis commented 1 year ago

I'm also sympathetic towards a standard distance unit to be used, like cm. Maybe the unit of mm should only be for rain then, where a couple of sensors are using it.

As we do have C/F selection for temperature, someone may want something similar for distance in cm/inch :grinning:

arendst commented 1 year ago

I'll start checking the impact on changing all D_JSON_DISTANCE values to cm now.

EDIT: first scan shows VL53L0x, VL53L1x, AS3935 and TOF10120 are affected. EDIT2: also HRXL and DYP are affected which report range instead of distance.

Jason2866 commented 1 year ago

As we do have C/F selection for temperature, someone may want something similar for distance in cm/inch 😀

Nooooo, noooooo What next? Gallons?

arendst commented 1 year ago

We settle for ISO. So cm it is.

sfromis commented 1 year ago

Since those two drivers are still using uint variables (instead of float), and a divide by 10 was added, looks like the resolution is now worse, no longer able to report distance differences less than 1 cm.

arendst commented 1 year ago

Correct. The datasheets for both sensors even mentions that distances below 30 cm need to disregarded!

I don't think distances below 1cm make any sense at all.

arendst commented 1 year ago

But you triggered another issue. I'll change to float adding the decimal resolution.

sfromis commented 1 year ago

Yeah, that was what I meant :smile:

arendst commented 1 year ago

@heligan pls try latest dev release and report back.

heligan commented 1 year ago

Thanks Theo for the quick fix and great discussion everyone. I verified the fix is working properly. Both sides use cm-based distance now.

Screen Shot 2022-11-08 at 12 30 54 PM