UPnP async (#13666)

* moved from miniupnpc to pyupnp-async

* update requirements

* Tests added

* hound

* update requirements_test_all.txt

* update gen_requirements_all.py

* addresses @pvizeli requested changes

* address review comments

homeassistant/components/sensor/upnp.py

@@ -6,38 +6,44 @@ https://home-assistant.io/components/sensor.upnp/
 """
 import logging
 
-from homeassistant.components.upnp import DATA_UPNP, UNITS
+from homeassistant.components.upnp import DATA_UPNP, UNITS, CIC_SERVICE
 from homeassistant.helpers.entity import Entity
 
 _LOGGER = logging.getLogger(__name__)
 
+BYTES_RECEIVED = 1
+BYTES_SENT = 2
+PACKETS_RECEIVED = 3
+PACKETS_SENT = 4
+
 # sensor_type: [friendly_name, convert_unit, icon]
 SENSOR_TYPES = {
-    'byte_received': ['received bytes', True, 'mdi:server-network'],
+    BYTES_RECEIVED: ['received bytes', True, 'mdi:server-network'],
-    'byte_sent': ['sent bytes', True, 'mdi:server-network'],
+    BYTES_SENT: ['sent bytes', True, 'mdi:server-network'],
-    'packets_in': ['packets received', False, 'mdi:server-network'],
+    PACKETS_RECEIVED: ['packets received', False, 'mdi:server-network'],
-    'packets_out': ['packets sent', False, 'mdi:server-network'],
+    PACKETS_SENT: ['packets sent', False, 'mdi:server-network'],
 }
 
 
-def setup_platform(hass, config, add_devices, discovery_info=None):
+async def async_setup_platform(hass, config, add_devices, discovery_info=None):
     """Set up the IGD sensors."""
-    upnp = hass.data[DATA_UPNP]
+    device = hass.data[DATA_UPNP]
+    service = device.find_first_service(CIC_SERVICE)
     unit = discovery_info['unit']
     add_devices([
-        IGDSensor(upnp, t, unit if SENSOR_TYPES[t][1] else None)
+        IGDSensor(service, t, unit if SENSOR_TYPES[t][1] else '#')
         for t in SENSOR_TYPES], True)
 
 
 class IGDSensor(Entity):
     """Representation of a UPnP IGD sensor."""
 
-    def __init__(self, upnp, sensor_type, unit=""):
+    def __init__(self, service, sensor_type, unit=None):
         """Initialize the IGD sensor."""
-        self._upnp = upnp
+        self._service = service
         self.type = sensor_type
         self.unit = unit
-        self.unit_factor = UNITS[unit] if unit is not None else 1
+        self.unit_factor = UNITS[unit] if unit in UNITS else 1
         self._name = 'IGD {}'.format(SENSOR_TYPES[sensor_type][0])
         self._state = None
 
@@ -49,9 +55,9 @@ class IGDSensor(Entity):
     @property
     def state(self):
         """Return the state of the device."""
-        if self._state is None:
+        if self._state:
-            return None
+            return format(float(self._state) / self.unit_factor, '.1f')
-        return format(self._state / self.unit_factor, '.1f')
+        return self._state
 
     @property
     def icon(self):
@@ -63,13 +69,13 @@ class IGDSensor(Entity):
         """Return the unit of measurement of this entity, if any."""
         return self.unit
 
-    def update(self):
+    async def async_update(self):
         """Get the latest information from the IGD."""
-        if self.type == "byte_received":
+        if self.type == BYTES_RECEIVED:
-            self._state = self._upnp.totalbytereceived()
+            self._state = await self._service.get_total_bytes_received()
-        elif self.type == "byte_sent":
+        elif self.type == BYTES_SENT:
-            self._state = self._upnp.totalbytesent()
+            self._state = await self._service.get_total_bytes_sent()
-        elif self.type == "packets_in":
+        elif self.type == PACKETS_RECEIVED:
-            self._state = self._upnp.totalpacketreceived()
+            self._state = await self._service.get_total_packets_received()
-        elif self.type == "packets_out":
+        elif self.type == PACKETS_SENT:
-            self._state = self._upnp.totalpacketsent()
+            self._state = await self._service.get_total_packets_sent()

homeassistant/components/upnp.py

@@ -6,6 +6,7 @@ https://home-assistant.io/components/upnp/
 """
 from ipaddress import ip_address
 import logging
+import asyncio
 
 import voluptuous as vol
 
@@ -14,7 +15,7 @@ from homeassistant.helpers import config_validation as cv
 from homeassistant.helpers import discovery
 from homeassistant.util import get_local_ip
 
-REQUIREMENTS = ['miniupnpc==2.0.2']
+REQUIREMENTS = ['pyupnp-async==0.1.0.1']
 DEPENDENCIES = ['http']
 
 _LOGGER = logging.getLogger(__name__)
@@ -22,7 +23,7 @@ _LOGGER = logging.getLogger(__name__)
 DEPENDENCIES = ['api']
 DOMAIN = 'upnp'
 
-DATA_UPNP = 'UPNP'
+DATA_UPNP = 'upnp_device'
 
 CONF_LOCAL_IP = 'local_ip'
 CONF_ENABLE_PORT_MAPPING = 'port_mapping'
@@ -33,6 +34,11 @@ CONF_HASS = 'hass'
 NOTIFICATION_ID = 'upnp_notification'
 NOTIFICATION_TITLE = 'UPnP Setup'
 
+IGD_DEVICE = 'urn:schemas-upnp-org:device:InternetGatewayDevice:1'
+PPP_SERVICE = 'urn:schemas-upnp-org:service:WANPPPConnection:1'
+IP_SERVICE = 'urn:schemas-upnp-org:service:WANIPConnection:1'
+CIC_SERVICE = 'urn:schemas-upnp-org:service:WANCommonInterfaceConfig:1'
+
 UNITS = {
     "Bytes": 1,
     "KBytes": 1024,
@@ -51,8 +57,7 @@ CONFIG_SCHEMA = vol.Schema({
 }, extra=vol.ALLOW_EXTRA)
 
 
-# pylint: disable=import-error, no-member, broad-except, c-extension-no-member
+async def async_setup(hass, config):
-def setup(hass, config):
     """Register a port mapping for Home Assistant via UPnP."""
     config = config[DOMAIN]
     host = config.get(CONF_LOCAL_IP)
@@ -67,21 +72,35 @@ def setup(hass, config):
             'Unable to determine local IP. Add it to your configuration.')
         return False
 
-    import miniupnpc
+    import pyupnp_async
+    from pyupnp_async.error import UpnpSoapError
 
-    upnp = miniupnpc.UPnP()
+    service = None
-    hass.data[DATA_UPNP] = upnp
+    resp = await pyupnp_async.msearch_first(search_target=IGD_DEVICE)
-
+    if not resp:
-    upnp.discoverdelay = 200
-    upnp.discover()
-    try:
-        upnp.selectigd()
-    except Exception:
-        _LOGGER.exception("Error when attempting to discover an UPnP IGD")
         return False
 
-    unit = config.get(CONF_UNITS)
+    try:
-    discovery.load_platform(hass, 'sensor', DOMAIN, {'unit': unit}, config)
+        device = await resp.get_device()
+        hass.data[DATA_UPNP] = device
+        for _service in device.services:
+            if _service['serviceType'] == PPP_SERVICE:
+                service = device.find_first_service(PPP_SERVICE)
+            if _service['serviceType'] == IP_SERVICE:
+                service = device.find_first_service(IP_SERVICE)
+            if _service['serviceType'] == CIC_SERVICE:
+                unit = config.get(CONF_UNITS)
+                discovery.load_platform(hass, 'sensor',
+                                        DOMAIN,
+                                        {'unit': unit},
+                                        config)
+    except UpnpSoapError as error:
+        _LOGGER.error(error)
+        return False
+
+    if not service:
+        _LOGGER.warning("Could not find any UPnP IGD")
+        return False
 
     port_mapping = config.get(CONF_ENABLE_PORT_MAPPING)
     if not port_mapping:
@@ -98,12 +117,12 @@ def setup(hass, config):
         if internal == CONF_HASS:
             internal = internal_port
         try:
-            upnp.addportmapping(
+            await service.add_port_mapping(internal, external, host, 'TCP',
-                external, 'TCP', host, internal, 'Home Assistant', '')
+                                           desc='Home Assistant')
             registered.append(external)
-        except Exception:
+            _LOGGER.debug("external %s -> %s @ %s", external, internal, host)
-            _LOGGER.exception("UPnP failed to configure port mapping for %s",
+        except UpnpSoapError as error:
-                              external)
+            _LOGGER.error(error)
             hass.components.persistent_notification.create(
                 '<b>ERROR: tcp port {} is already mapped in your router.'
                 '</b><br />Please disable port_mapping in the <i>upnp</i> '
@@ -113,11 +132,13 @@ def setup(hass, config):
                 title=NOTIFICATION_TITLE,
                 notification_id=NOTIFICATION_ID)
 
-    def deregister_port(event):
+    async def deregister_port(event):
         """De-register the UPnP port mapping."""
-        for external in registered:
+        tasks = [service.delete_port_mapping(external, 'TCP')
-            upnp.deleteportmapping(external, 'TCP')
+                 for external in registered]
+        if tasks:
+            await asyncio.wait(tasks)
 
-    hass.bus.listen_once(EVENT_HOMEASSISTANT_STOP, deregister_port)
+    hass.bus.async_listen_once(EVENT_HOMEASSISTANT_STOP, deregister_port)
 
     return True

requirements_all.txt

@@ -517,9 +517,6 @@ mficlient==0.3.0
 # homeassistant.components.sensor.miflora
 miflora==0.3.0
 
-# homeassistant.components.upnp
-miniupnpc==2.0.2
-
 # homeassistant.components.sensor.mopar
 motorparts==1.0.2
 
@@ -1055,6 +1052,9 @@ pytradfri[async]==5.4.2
 # homeassistant.components.device_tracker.unifi
 pyunifi==2.13
 
+# homeassistant.components.upnp
+pyupnp-async==0.1.0.1
+
 # homeassistant.components.keyboard
 # pyuserinput==0.1.11
 

requirements_test_all.txt

@@ -158,6 +158,9 @@ pythonwhois==2.4.3
 # homeassistant.components.device_tracker.unifi
 pyunifi==2.13
 
+# homeassistant.components.upnp
+pyupnp-async==0.1.0.1
+
 # homeassistant.components.notify.html5
 pywebpush==1.6.0
 

script/gen_requirements_all.py

@@ -76,6 +76,7 @@ TEST_REQUIREMENTS = (
     'pyqwikswitch',
     'python-forecastio',
     'pyunifi',
+    'pyupnp-async',
     'pywebpush',
     'restrictedpython',
     'rflink',

tests/components/test_upnp.py

@@ -1,5 +1,4 @@
 """Test the UPNP component."""
-import asyncio
 from collections import OrderedDict
 from unittest.mock import patch, MagicMock
 
@@ -7,15 +6,64 @@ import pytest
 
 from homeassistant.const import EVENT_HOMEASSISTANT_STOP
 from homeassistant.setup import async_setup_component
+from homeassistant.components.upnp import IP_SERVICE, DATA_UPNP
+
+
+class MockService(MagicMock):
+    """Mock upnp IP service."""
+
+    async def add_port_mapping(self, *args, **kwargs):
+        """Original function."""
+        self.mock_add_port_mapping(*args, **kwargs)
+
+    async def delete_port_mapping(self, *args, **kwargs):
+        """Original function."""
+        self.mock_delete_port_mapping(*args, **kwargs)
+
+
+class MockDevice(MagicMock):
+    """Mock upnp device."""
+
+    def find_first_service(self, *args, **kwargs):
+        """Original function."""
+        self._service = MockService()
+        return self._service
+
+    def peep_first_service(self):
+        """Access Mock first service."""
+        return self._service
+
+
+class MockResp(MagicMock):
+    """Mock upnp msearch response."""
+
+    async def get_device(self, *args, **kwargs):
+        """Original function."""
+        device = MockDevice()
+        service = {'serviceType': IP_SERVICE}
+        device.services = [service]
+        return device
 
 
 @pytest.fixture
-def mock_miniupnpc():
+def mock_msearch_first(*args, **kwargs):
-    """Mock miniupnpc."""
+    """Wrapper to async mock function."""
-    mock = MagicMock()
+    async def async_mock_msearch_first(*args, **kwargs):
+        """Mock msearch_first."""
+        return MockResp(*args, **kwargs)
 
-    with patch.dict('sys.modules', {'miniupnpc': mock}):
+    with patch('pyupnp_async.msearch_first', new=async_mock_msearch_first):
-        yield mock.UPnP()
+        yield
+
+
+@pytest.fixture
+def mock_async_exception(*args, **kwargs):
+    """Wrapper to async mock function with exception."""
+    async def async_mock_exception(*args, **kwargs):
+        return Exception
+
+    with patch('pyupnp_async.msearch_first', new=async_mock_exception):
+        yield
 
 
 @pytest.fixture
@@ -26,75 +74,66 @@ def mock_local_ip():
         yield
 
 
-@pytest.fixture(autouse=True)
+async def test_setup_fail_if_no_ip(hass):
-def mock_discovery():
-    """Mock discovery of upnp sensor."""
-    with patch('homeassistant.components.upnp.discovery'):
-        yield
-
-
-@asyncio.coroutine
-def test_setup_fail_if_no_ip(hass):
     """Test setup fails if we can't find a local IP."""
     with patch('homeassistant.components.upnp.get_local_ip',
                return_value='127.0.0.1'):
-        result = yield from async_setup_component(hass, 'upnp', {
+        result = await async_setup_component(hass, 'upnp', {
             'upnp': {}
         })
 
     assert not result
 
 
-@asyncio.coroutine
+async def test_setup_fail_if_cannot_select_igd(hass,
-def test_setup_fail_if_cannot_select_igd(hass, mock_local_ip, mock_miniupnpc):
+                                               mock_local_ip,
+                                               mock_async_exception):
     """Test setup fails if we can't find an UPnP IGD."""
-    mock_miniupnpc.selectigd.side_effect = Exception
+    result = await async_setup_component(hass, 'upnp', {
-
-    result = yield from async_setup_component(hass, 'upnp', {
         'upnp': {}
     })
 
     assert not result
 
 
-@asyncio.coroutine
+async def test_setup_succeeds_if_specify_ip(hass, mock_msearch_first):
-def test_setup_succeeds_if_specify_ip(hass, mock_miniupnpc):
     """Test setup succeeds if we specify IP and can't find a local IP."""
     with patch('homeassistant.components.upnp.get_local_ip',
                return_value='127.0.0.1'):
-        result = yield from async_setup_component(hass, 'upnp', {
+        result = await async_setup_component(hass, 'upnp', {
             'upnp': {
                 'local_ip': '192.168.0.10'
             }
         })
 
     assert result
+    mock_service = hass.data[DATA_UPNP].peep_first_service()
+    assert len(mock_service.mock_add_port_mapping.mock_calls) == 1
+    mock_service.mock_add_port_mapping.assert_called_once_with(
+        8123, 8123, '192.168.0.10', 'TCP', desc='Home Assistant')
 
 
-@asyncio.coroutine
+async def test_no_config_maps_hass_local_to_remote_port(hass,
-def test_no_config_maps_hass_local_to_remote_port(hass, mock_miniupnpc):
+                                                        mock_local_ip,
+                                                        mock_msearch_first):
     """Test by default we map local to remote port."""
-    result = yield from async_setup_component(hass, 'upnp', {
+    result = await async_setup_component(hass, 'upnp', {
-        'upnp': {
+        'upnp': {}
-            'local_ip': '192.168.0.10'
-        }
     })
 
     assert result
-    assert len(mock_miniupnpc.addportmapping.mock_calls) == 1
+    mock_service = hass.data[DATA_UPNP].peep_first_service()
-    external, _, host, internal, _, _ = \
+    assert len(mock_service.mock_add_port_mapping.mock_calls) == 1
-        mock_miniupnpc.addportmapping.mock_calls[0][1]
+    mock_service.mock_add_port_mapping.assert_called_once_with(
-    assert host == '192.168.0.10'
+        8123, 8123, '192.168.0.10', 'TCP', desc='Home Assistant')
-    assert external == 8123
-    assert internal == 8123
 
 
-@asyncio.coroutine
+async def test_map_hass_to_remote_port(hass,
-def test_map_hass_to_remote_port(hass, mock_miniupnpc):
+                                       mock_local_ip,
+                                       mock_msearch_first):
     """Test mapping hass to remote port."""
-    result = yield from async_setup_component(hass, 'upnp', {
+    result = await async_setup_component(hass, 'upnp', {
         'upnp': {
-            'local_ip': '192.168.0.10',
             'ports': {
                 'hass': 1000
             }
@@ -102,41 +141,38 @@ def test_map_hass_to_remote_port(hass, mock_miniupnpc):
     })
 
     assert result
-    assert len(mock_miniupnpc.addportmapping.mock_calls) == 1
+    mock_service = hass.data[DATA_UPNP].peep_first_service()
-    external, _, host, internal, _, _ = \
+    assert len(mock_service.mock_add_port_mapping.mock_calls) == 1
-        mock_miniupnpc.addportmapping.mock_calls[0][1]
+    mock_service.mock_add_port_mapping.assert_called_once_with(
-    assert external == 1000
+        8123, 1000, '192.168.0.10', 'TCP', desc='Home Assistant')
-    assert internal == 8123
 
 
-@asyncio.coroutine
+async def test_map_internal_to_remote_ports(hass,
-def test_map_internal_to_remote_ports(hass, mock_miniupnpc):
+                                            mock_local_ip,
+                                            mock_msearch_first):
     """Test mapping local to remote ports."""
     ports = OrderedDict()
     ports['hass'] = 1000
     ports[1883] = 3883
 
-    result = yield from async_setup_component(hass, 'upnp', {
+    result = await async_setup_component(hass, 'upnp', {
         'upnp': {
-            'local_ip': '192.168.0.10',
             'ports': ports
         }
     })
 
     assert result
-    assert len(mock_miniupnpc.addportmapping.mock_calls) == 2
+    mock_service = hass.data[DATA_UPNP].peep_first_service()
-    external, _, host, internal, _, _ = \
+    assert len(mock_service.mock_add_port_mapping.mock_calls) == 2
-        mock_miniupnpc.addportmapping.mock_calls[0][1]
-    assert external == 1000
-    assert internal == 8123
 
-    external, _, host, internal, _, _ = \
+    mock_service.mock_add_port_mapping.assert_any_call(
-        mock_miniupnpc.addportmapping.mock_calls[1][1]
+        8123, 1000, '192.168.0.10', 'TCP', desc='Home Assistant')
-    assert external == 3883
+    mock_service.mock_add_port_mapping.assert_any_call(
-    assert internal == 1883
+        1883, 3883, '192.168.0.10', 'TCP', desc='Home Assistant')
 
     hass.bus.async_fire(EVENT_HOMEASSISTANT_STOP)
-    yield from hass.async_block_till_done()
+    await hass.async_block_till_done()
-    assert len(mock_miniupnpc.deleteportmapping.mock_calls) == 2
+    assert len(mock_service.mock_delete_port_mapping.mock_calls) == 2
-    assert mock_miniupnpc.deleteportmapping.mock_calls[0][1][0] == 1000
+
-    assert mock_miniupnpc.deleteportmapping.mock_calls[1][1][0] == 3883
+    mock_service.mock_delete_port_mapping.assert_any_call(1000, 'TCP')
+    mock_service.mock_delete_port_mapping.assert_any_call(3883, 'TCP')