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

Add Time of Flight Sensor using VL53L1X #21230

Merged
merged 50 commits into from Mar 8, 2019

Conversation

Projects
None yet
7 participants
@josemotta
Copy link
Contributor

josemotta commented Feb 19, 2019

Description: ToF Sensor using VL53L1X

The Time of Flight sensor uses a invisible laser to measure distance with millimeter resolution.

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#8672

Example entry for configuration.yaml (if applicable):

sensor:
   - platform: tof

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
Show resolved Hide resolved homeassistant/components/sensor/tof.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/tof.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/tof.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/tof.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/tof.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/tof.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/tof.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/tof.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/tof.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/tof.py Outdated

josemotta added some commits Feb 19, 2019

Show resolved Hide resolved homeassistant/components/sensor/tof.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/tof.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/tof.py Outdated

josemotta added some commits Feb 20, 2019

@josemotta josemotta referenced this pull request Feb 21, 2019

Merged

Create sensor.tof #8672

2 of 2 tasks complete

josemotta added some commits Feb 21, 2019

@josemotta

This comment has been minimized.

Copy link
Contributor Author

josemotta commented Feb 22, 2019

To pass travis-ci build tests, the last issue would be to update requirements_all.txt but my environment is Raspberry Pi! Any suggestion that helps me to accomplish that? I cannot clone this fork on the RPI zero.

Python: 3.5.3 TOXENV=lint
******* ERROR
requirements_all.txt is not up to date
Please run script/gen_requirements_all.py

@fabaff
Copy link
Member

fabaff left a comment

Run script/gen_requirements_all.py to fix the issues with requirements_all.txt after adding your requirements to COMMENT_REQUIREMENTS in gen_requirements_all.py.

Show resolved Hide resolved homeassistant/components/sensor/tof.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/tof.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/tof.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/tof.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/tof.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/tof.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/tof.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/tof.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/tof.py Outdated
@josemotta

This comment has been minimized.

Copy link
Contributor Author

josemotta commented Feb 25, 2019

Run script/gen_requirements_all.py to fix the issues with requirements_all.txt after adding your requirements to COMMENT_REQUIREMENTS in gen_requirements_all.py.

In this case, I suppose I should change my current dev machine from Windows10 to Linux. I tried to run it but I got an error from windows/bash, complaining about "ImportError: No module named homeassistant.components".
The option to run it using RPI is not wise, isn´t? Since I would need to clone all the repo inside RPI!

@frenck

This comment has been minimized.

Copy link
Member

frenck commented Feb 25, 2019

⚠️ I was unable to find a documentation PR (matching this code change) in our documentation repository.

🏷 I am adding the docs-missing label until this has been resolved.

@frenck frenck added the docs-missing label Feb 25, 2019

Show resolved Hide resolved homeassistant/components/sensor/tof.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/tof.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/tof.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/tof.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/tof.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/tof.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/tof.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/tof.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/tof.py Outdated

josemotta added some commits Mar 7, 2019

josemotta added some commits Mar 7, 2019

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Looks good!

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Also add the new module to .coveragerc.

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Another thing I forgot:
We should move the sensor platform under a tof or VL53L1X package under homeassistant/components/. We want to name the integration after brands usually. Is VL53L1X better than tof?

@josemotta

This comment has been minimized.

Copy link
Contributor Author

josemotta commented Mar 7, 2019

I suppose to add it to .coveragerc as below, right?

...
    homeassistant/components/sensor/thermoworks_smoke.py
    homeassistant/components/sensor/time_date.py
    homeassistant/components/sensor/tof.py
    homeassistant/components/sensor/torque.py
    homeassistant/components/sensor/trafikverket_weatherstation.py
...

Time of Flight (maybe ToF) is the technology name. VL53L1X is a good name also but this is the component id for the latest STMicroelectronics "tof sensor" after VL53L0X. What happens if a new VL53L2X is coming and VL53L1X is discontinued? Then there would be a couple components or the version would be selected at yaml config in a single "tof" component?

Please clarify/confirm:

move the sensor platform under a tof or VL53L1X package under homeassistant/components/

Should I create a homeassistant/components/tof folder with the sensor.py file inside? anything else? What happens with current homeassistant/components/sensor/tof.py?

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Mar 7, 2019

Ok let's keep tof as integration name for now.

Do git mv on the existing homeassistant/components/sensor/tof.py and move it to homeassistant/components/tof/sensor.py.

.coveragerc and requirements need to be updated after that.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Mar 7, 2019

Put a __init__.py file under the package too, with just a docstring.

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Great!

@MartinHjelmare MartinHjelmare merged commit f705ac6 into home-assistant:dev Mar 8, 2019

4 checks passed

Hound No violations found. Woof!
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.007%) to 92.755%
Details

@wafflebot wafflebot bot removed the in progress label Mar 8, 2019

@josemotta

This comment has been minimized.

Copy link
Contributor Author

josemotta commented Mar 8, 2019

The first PR you never forget. Thanks for your support!

This component was designed for most common use cases, like moving doors and windows. For example, I am now working on a level meter for water tanks using a Rpi0W. But this just scratches the possibilities with VL53L1X, since it can measure at a much bigger rate, up to 50 reading per second, this means a frequency of 50 Hz. How to handle this environment? Pushing instead of polling? Async_update coroutine? Where would I find some docs about this?

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Mar 8, 2019

I'm not sure what you're asking about. Our dev docs are at https://developers.home-assistant.io/.

Please use our discord chat for further discussion. We don't want that in merged PRs. Thanks!

@balloob balloob referenced this pull request Mar 20, 2019

Merged

0.90.0 #22216

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.