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

Update to sense component to fully be async #21698

Merged
merged 3 commits into from Mar 12, 2019

Conversation

Projects
None yet
8 participants
@kbickar
Copy link
Contributor

commented Mar 6, 2019

Description:

Updated the sense component to take advantage of asyncio functionality.

The big change is in the sense library with a new class for HA to import:
https://github.com/scottbonline/sense/blob/stable/sense_energy/asyncsenseable.py

Related issue (if applicable): fixes #20844

@kbickar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

Side Note: This is my first time working with asyncio and while it is working without issue on my installation, I'd appreciate if anyone with more experience with it could review

@Maaniac

This comment has been minimized.

Copy link

commented Mar 7, 2019

Binary sensors appear to be working but i'm getting an config error for the sensor component.

@kbickar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

What's the config error you're getting?

@Maaniac

This comment has been minimized.

Copy link

commented Mar 7, 2019

Unable to prepare setup for platform sense.sensor: Platform not found. Also, getting an unable to find platform sense. Search path was limited to path of component: custom_components.

@kbickar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

Did you install it as a custom component or as a normal one? Sounds like it can't see the sensor.py file

@Maaniac

This comment has been minimized.

Copy link

commented Mar 7, 2019

Running Hass.io so I don't have access to the files. I created a folder called sense under custom components and placed all three of the python files in there. Maybe because this is a main component it does not work.

@Maaniac

This comment has been minimized.

Copy link

commented Mar 7, 2019

On a separate note would you consider adding an option to split up the sensor from the binary sensor component? Or add an exclude entities to filter out sensors we don't want to import?

@kbickar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

I'm not sure what you mean by split up the sensor from the binary sensor. By defining the platform, the sensors are added. Defining the sensor separately would be a little awkward as it has no configurable options. There used to be a "monitored_conditions" config, but it was recommended that be removed by reviewers. Having said that, it can be a little annoying to have all the devices show up as new binary sensors (including new ones), so some option to reduce that might be helpful

@tylerterzigni

This comment has been minimized.

Copy link

commented Mar 8, 2019

Unable to prepare setup for platform sense.sensor: Platform not found. Also, getting an unable to find platform sense. Search path was limited to path of component: custom_components.

I am also getting the Unable to prepare setup for platform sense.sensor: Platform not found.

I also am getting the following:

Error loading homeassistant.components.sense.sensor. Make sure all dependencies are installed
Traceback (most recent call last):
File "/home/stanley/homeassistant/lib/python3.6/site-packages/homeassistant/loader.py", line 166, in _load_file
module = importlib.import_module(path)
File "/usr/lib/python3.6/importlib/init.py", line 126, in import_module
return _bootstrap._gcd_import(name[level:], package, level)
File "", line 994, in _gcd_import
File "", line 971, in _find_and_load
File "", line 955, in _find_and_load_unlocked
File "", line 665, in _load_unlocked
File "", line 678, in exec_module
File "", line 219, in _call_with_frames_removed
File "/home/stanley/homeassistant/lib/python3.6/site-packages/homeassistant/components/sense/sensor.py", line 6, in
from homeassistant.const import POWER_WATT
ImportError: cannot import name 'POWER_WATT'

as well as this:
Unable to find platform sense. Search path was limited to path of component: homeassistant.components

@kbickar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

That's a new constant on the dev branch (added in #21570) - looks like it hasn't made it to a release yet, so you'll need to install the consts file manually.

I will try installing as a custom component to see if I can replicate the issue with it not finding the sensor in that case

@tylerterzigni

This comment has been minimized.

Copy link

commented Mar 8, 2019

Any chance you can explain or point in the direction of how to install the consts file manually? I am a newb!

@kbickar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

Simple way would be download: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/const.py and put it in /home/stanley/homeassistant/lib/python3.6/site-packages/homeassistant/

@AndyRPH

This comment has been minimized.

Copy link

commented Mar 9, 2019

Looking to test this in hassio too, so I dropped the three files into

config/custom_components/sensor/sense/

And now get a slightly different error:

Update for binary_sensor.oven fails
Traceback (most recent call last):
File "/usr/local/lib/python3.7/site-packages/homeassistant/helpers/entity.py", line 220, in async_update_ha_state
await self.async_device_update()
File "/usr/local/lib/python3.7/site-packages/homeassistant/helpers/entity.py", line 348, in async_device_update
await self.hass.async_add_executor_job(self.update)
File "/usr/local/lib/python3.7/concurrent/futures/thread.py", line 57, in run
result = self.fn(*self.args, **self.kwargs)
File "/usr/local/lib/python3.7/site-packages/homeassistant/components/sense/binary_sensor.py", line 110, in update
self._data.get_realtime()
File "/config/deps/lib/python3.7/site-packages/sense_energy/sense_api.py", line 84, in get_realtime
self._realtime = websocket.get_realtime(url, self.wss_timeout)
File "/config/deps/lib/python3.7/site-packages/sense_energy/ws_async.py", line 24, in get_realtime
asyncio.get_event_loop().run_until_complete(
File "/usr/local/lib/python3.7/asyncio/events.py", line 644, in get_event_loop
% threading.current_thread().name)
RuntimeError: There is no current event loop in thread 'SyncWorker_4'.

Is there more to testing it than that?

@kbickar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2019

Yes, there's a major update to the sense energy library that this component depends on. From the stacktrace it looks like you have 0.60 installed, not the new 0.70

@AndyRPH

This comment has been minimized.

Copy link

commented Mar 9, 2019

@kbickar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2019

I don't think it'll update the package unless you run the updater and it's part of the install. Having two conflicting versions would probably cause issues.

@AndyRPH

This comment has been minimized.

Copy link

commented Mar 10, 2019

@kbickar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2019

The built in updater will only update the release version. I don't run hass.io, but it looks like there's a custom deps add on that you might be able to use to install the new version of the package

@pvizeli pvizeli merged commit 62df6cb into home-assistant:dev Mar 12, 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 remained the same at 92.757%
Details

@ghost ghost removed the in progress label Mar 12, 2019

@MartinHjelmare
Copy link
Member

left a comment

Please open a new PR where we can address the comments.

"""Set up the Sense binary sensor."""
if discovery_info is None:

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Mar 12, 2019

Member

We should keep this guard clause. It's needed to only act on setup via discovery.

load_platform(hass, 'sensor', DOMAIN, {}, config)
load_platform(hass, 'binary_sensor', DOMAIN, {}, config)
hass.async_create_task(
async_load_platform(hass, 'sensor', DOMAIN, None, config))

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Mar 12, 2019

Member

Pass empty dict as discovery_info. See below.

"""Set up the Sense sensor."""
if discovery_info is None:

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Mar 12, 2019

Member

See above.

@kbickar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

Please open a new PR where we can address the comments.
@MartinHjelmare
Created #22014 to address comments

@kbickar kbickar deleted the kbickar:sense-async-1 branch Mar 13, 2019

@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.