Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistant distance unit for vl53l0x distance sensor #17021

Closed
10 of 14 tasks
heligan opened this issue Nov 7, 2022 · 15 comments
Closed
10 of 14 tasks

Inconsistant distance unit for vl53l0x distance sensor #17021

heligan opened this issue Nov 7, 2022 · 15 comments
Assignees
Labels
enhancement Type - Enhancement that will be worked on fixed Result - The work on the issue has ended

Comments

@heligan
Copy link

heligan commented Nov 7, 2022

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!

  • Read the Contributing Guide and Policy and the Code of Conduct
  • Searched the problem in issues
  • Searched the problem in discussions
  • Searched the problem in the docs
  • Searched the problem in the chat
  • Device used (e.g., Sonoff Basic): NodeMCU
  • Tasmota binary firmware version number used: tasmota-sensors 12.2
    • Pre-compiled
    • Self-compiled
  • Flashing tools used: esptool.py
  • Provide the output of command: Backlog Template; Module; GPIO 255:
  Configuration output here:

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:
  Rules output here:

  • Provide the output of this command: Status 0:
  STATUS 0 output here:

  • Set weblog to 4 and then, when you experience your issue, provide the output of the Console log:
  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
Copy link
Contributor

sfromis commented Nov 8, 2022

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
Copy link
Collaborator

@barbudor
Copy link
Contributor

barbudor commented Nov 8, 2022

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
Copy link
Collaborator

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
Copy link
Owner

arendst commented Nov 8, 2022

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
Copy link
Contributor

sfromis commented Nov 8, 2022

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 😀

@arendst
Copy link
Owner

arendst commented Nov 8, 2022

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
Copy link
Collaborator

Jason2866 commented Nov 8, 2022

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
Copy link
Owner

arendst commented Nov 8, 2022

We settle for ISO. So cm it is.

arendst added a commit that referenced this issue Nov 8, 2022
Redesign distance sensors HRXL and DYP to use cm instead of mm (#17021)
@sfromis
Copy link
Contributor

sfromis commented Nov 8, 2022

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
Copy link
Owner

arendst commented Nov 8, 2022

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
Copy link
Owner

arendst commented Nov 8, 2022

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

@sfromis
Copy link
Contributor

sfromis commented Nov 8, 2022

Yeah, that was what I meant 😄

arendst added a commit that referenced this issue Nov 8, 2022
Redesign distance sensors VL53LXX, TOF10120, HRXL and DYP to use cm instead of mm (#17021)
@arendst
Copy link
Owner

arendst commented Nov 8, 2022

@heligan pls try latest dev release and report back.

@arendst arendst self-assigned this Nov 8, 2022
@arendst arendst added enhancement Type - Enhancement that will be worked on fixed Result - The work on the issue has ended labels Nov 8, 2022
@heligan
Copy link
Author

heligan commented Nov 8, 2022

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

@heligan heligan closed this as completed Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Type - Enhancement that will be worked on fixed Result - The work on the issue has ended
Projects
None yet
Development

No branches or pull requests

5 participants