Upnp properties by dgomes · Pull Request #8067 · home-assistant/core

dgomes · 2017-06-16T22:57:11Z

This pull request improves the UPnP component with IGD sensors containing bytes sent/received and packets sent/received

mention-bot · 2017-06-16T22:57:12Z

@dgomes, thanks for your PR! By analyzing the history of the files in this pull request, we identified @robbiet480 and @fabaff to be potential reviewers.

houndci-bot · 2017-06-16T22:57:17Z

+
+class IGDSensor(Entity):
+    """Representation of a UPnP IGD sensor."""
+


blank line contains whitespace

fabaff · 2017-06-17T10:27:04Z

The checklist of the PR template covers a bunch of topics which are missing in the PR.

dgomes · 2017-06-17T14:36:12Z

Where can I find the template ? More than willing to cover whats needed

(I've since checked other pull requests and followed the checklist)

In accordance to pull request home-assistant/core#8067

Latest version of miniupnpc is 2.0, but pypi only has 1.9 Fortunately it is enough

balloob · 2017-06-18T05:28:13Z

+    hass.data[DATA_UPNP] = upnp

    upnp.discoverdelay = 200
    upnp.discover()


You cannot do I/O inside a coroutine.

I understood hass.data would be a global dictionary and wouldn't involve any I/O

But I'm moving everything back to setup() and update()

Assigning to data doesn't do I/O. This comment was about upnp.discover() call. Since you changed it back to no longer be a coroutine this comment can be ignored.

balloob · 2017-06-18T05:28:37Z

-    host = base_url.hostname
-    external_port = internal_port = base_url.port
+    unit = config[DOMAIN].get(CONF_UNITS)
+    discovery.load_platform(hass, 'sensor', DOMAIN, {'unit': unit}, config)


You cannot call sync methods inside a coroutine.

I would suggest you change it back to setup and not make it async.

balloob · 2017-06-18T05:29:05Z

+    discovery.load_platform(hass, 'sensor', DOMAIN, {'unit': unit}, config)
+
+    port_mapping = config[DOMAIN].get(CONF_ENABLE_PORT_MAPPING)
+    if port_mapping:


Make it a guard clause:

if not port_mapping: return True base_url = …

balloob · 2017-06-18T05:29:44Z

+
+_LOGGER = logging.getLogger(__name__)
+
+""" sensor_type: [friendly_name, convert_unit, icon] """


Use # for comments. Only use """ for first line in module/class/method to indicate it's a doc string.

balloob · 2017-06-18T05:30:24Z

+    def async_update(self):
+        """Get the latest information from the IGD."""
+        if self.type == "byte_received":
+            self._state = self._upnp.totalbytereceived()


Do these methods do I/O ? If so, this shouldn't be async_update but instead update

balloob · 2017-06-18T05:32:49Z

+from homeassistant.helpers import config_validation as cv
+from homeassistant.helpers import discovery
+
+REQUIREMENTS = ['miniupnpc==1.9']


Will this install miniupnp ?

balloob

You've updated a few places to be async instead of the sync version. However, it seems that there is still I/O being done in those methods. Anything that hits a network interface or talks to disk/network cannot be run inside a coroutine.

fabaff · 2017-06-18T10:53:30Z

Documentation PR: #2832

miniupnpc will do network calls, so this component can’t be moved to coroutine

houndci-bot · 2017-06-18T21:17:48Z

+For more details about this platform, please refer to the documentation at
+https://home-assistant.io/components/sensor.upnp/
+"""
+import asyncio


'asyncio' imported but unused

forgot to remove import ot asyncio

dgomes

I've addressed all the changes

balloob · 2017-06-19T04:32:43Z

Awesome, thanks!

dgomes · 2017-06-19T10:40:15Z

Thanks for the mentoring, looking forward to more contributions.

In accordance to pull request home-assistant/core#8067

* Update to UPnP documentation In accordance to pull request home-assistant/core#8067 * updated with new option * Improved thanks for the comments / review

* make port mapping optional * dependencies + improvements * Added bytes and packets sensors from IGD * flake8 check * new sensor with upnp counters * checks * whitespaces in blank line * requirements update * added sensor.upnp to .coveragerc * downgrade miniupnpc Latest version of miniupnpc is 2.0, but pypi only has 1.9 Fortunately it is enough * revert to non async miniupnpc will do network calls, so this component can’t be moved to coroutine * hof hof forgot to remove import ot asyncio

dgomes added 7 commits June 15, 2017 23:46

make port mapping optional

df6e569

dependencies + improvements

4813a82

Added bytes and packets sensors from IGD

49fba3b

flake8 check

cec02ac

new sensor with upnp counters

60c89b4

checks

985975d

Merge remote-tracking branch 'home-assistant/dev' into upnp_properties

3180c19

homeassistant added integration: upnp new-platform platform: sensor.upnp cla-signed labels Jun 16, 2017

houndci-bot reviewed Jun 16, 2017

View reviewed changes

whitespaces in blank line

49225ad

Merge remote-tracking branch 'home-assistant/dev' into upnp_properties

5a3b63d

balloobbot added component platform labels Jun 17, 2017

dgomes added a commit to dgomes/home-assistant.github.io that referenced this pull request Jun 17, 2017

Update to UPnP documentation

a251f6a

In accordance to pull request home-assistant/core#8067

dgomes mentioned this pull request Jun 17, 2017

Update to UPnP documentation home-assistant/home-assistant.io#2832

Merged

dgomes added 2 commits June 17, 2017 17:29

requirements update

e12ac4e

added sensor.upnp to .coveragerc

9a4f395

homeassistant added the cla-signed label Jun 17, 2017

downgrade miniupnpc

5fc3712

Latest version of miniupnpc is 2.0, but pypi only has 1.9 Fortunately it is enough

balloob reviewed Jun 18, 2017

View reviewed changes

balloob requested changes Jun 18, 2017

View reviewed changes

revert to non async

c127fcc

miniupnpc will do network calls, so this component can’t be moved to coroutine

balloobbot added the requirement label Jun 18, 2017

houndci-bot reviewed Jun 18, 2017

View reviewed changes

hof hof

03c4507

forgot to remove import ot asyncio

homeassistant added the cla-signed label Jun 18, 2017

dgomes commented Jun 18, 2017

View reviewed changes

balloob approved these changes Jun 19, 2017

View reviewed changes

balloob merged commit 83b7914 into home-assistant:dev Jun 19, 2017

fabaff pushed a commit to home-assistant/home-assistant.io that referenced this pull request Jun 19, 2017

Update to UPnP documentation (#2832)

069c17c

In accordance to pull request home-assistant/core#8067

fabaff mentioned this pull request Jun 19, 2017

Add UPNP sensor docs home-assistant/home-assistant.io#2846

Merged

balloob mentioned this pull request Jul 1, 2017

0.48 #8270

Merged

This was referenced Jul 2, 2017

Upnp properties (fix) #8294

Closed

Upnp mapping notification #8303

Merged

balloob pushed a commit to home-assistant/home-assistant.io that referenced this pull request Jul 7, 2017

Upnp update (#2924)

de782ad

* Update to UPnP documentation In accordance to pull request home-assistant/core#8067 * updated with new option * Improved thanks for the comments / review

home-assistant locked and limited conversation to collaborators Oct 20, 2017

ghost removed the platform: sensor.upnp label Mar 21, 2019


		class IGDSensor(Entity):
		"""Representation of a UPnP IGD sensor."""


		_LOGGER = logging.getLogger(__name__)

		""" sensor_type: [friendly_name, convert_unit, icon] """

Uh oh!

Conversation

dgomes commented Jun 16, 2017

Uh oh!

mention-bot commented Jun 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabaff commented Jun 17, 2017

Uh oh!

dgomes commented Jun 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob left a comment

Choose a reason for hiding this comment

Uh oh!

fabaff commented Jun 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgomes left a comment

Choose a reason for hiding this comment

Uh oh!

balloob commented Jun 19, 2017

Uh oh!

dgomes commented Jun 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dgomes commented Jun 17, 2017 •

edited

Loading