Added Zyxel Keenetic NDMS2 based routers support for device tracking by foxel · Pull Request #9315 · home-assistant/core

foxel · 2017-09-06T09:24:17Z

Description:

Pull request in home-assistant.github.io with documentation: home-assistant/home-assistant.io#3373

Example entry for `configuration.yaml` (if applicable):

device_tracker:
  - platform: keenetic_ndms2
    host: !secret router_ip
    username: !secret router_username
    password: !secret router_password

MartinHjelmare

I think you can remove the two options CONF_HOME_INTERVAL and CONF_EXCLUDE. See below for all comments.

You also need to fix the linting errors, see the travis build log. Also add a line for this platform in .coveragerc to remove the platform from the coveralls build.

MartinHjelmare · 2017-09-12T15:25:22Z

+    vol.Required(CONF_USERNAME): cv.string,
+    vol.Required(CONF_PASSWORD): cv.string,
+    vol.Required(CONF_INTERFACE, default=DEFAULT_INTERFACE): cv.string,
+    vol.Required(CONF_HOME_INTERVAL, default=0): cv.positive_int,


There's already a global option consider_home to handle stale devices. You don't need to add this for this platform.

Looked into it. Looks like so

MartinHjelmare · 2017-09-12T15:26:33Z

+    vol.Required(CONF_PASSWORD): cv.string,
+    vol.Required(CONF_INTERFACE, default=DEFAULT_INTERFACE): cv.string,
+    vol.Required(CONF_HOME_INTERVAL, default=0): cv.positive_int,
+    vol.Optional(CONF_EXCLUDE, default=[]):


You seem to have modeled this platform after the NMAP device tracker. That one is a special case which requires the CONF_EXCLUDE option. I don't think you need that option for this platform.

At least i use it to exclude some devices, e.g. IoT switches and so on from tracking.

There is already the track boolean option in the known_devices.yaml for this purpose. You can also set the global option track_new_devices to false in the config if you don't want to track new devices.

MartinHjelmare · 2017-09-12T15:28:26Z

+import logging
+
+import requests
+from collections import namedtuple


Please move this line to the builtin group above.

MartinHjelmare · 2017-09-12T15:29:05Z

+from homeassistant.components.device_tracker import (
+    DOMAIN, PLATFORM_SCHEMA, DeviceScanner)
+from homeassistant.const import (
+    CONF_HOST, CONF_USERNAME, CONF_PASSWORD


Please sort the imported names alphabetically 🔡.

MartinHjelmare · 2017-09-12T16:19:24Z

+                self._username, self._password
+            ))
+        except requests.exceptions.Timeout:
+            _LOGGER.exception(


Log with error is enough.

MartinHjelmare · 2017-09-12T16:19:47Z

+                "Connection to the router timed out at URL %s", self._url)
+            return False
+        if res.status_code != 200:
+            _LOGGER.exception(


Error or warning.

MartinHjelmare · 2017-09-12T16:20:21Z

+            result = res.json()
+        except ValueError:
+            # If json decoder could not parse the response
+            _LOGGER.exception("Failed to parse response from router")


MartinHjelmare · 2017-09-14T08:24:08Z

See my comment: #9315 (comment).

MartinHjelmare

Clean the remaining things and this is good to go.

MartinHjelmare · 2017-09-14T16:45:47Z

+    return scanner if scanner.success_init else None
+
+
+Device = namedtuple('Device', ['mac', 'name', 'ip', 'last_update'])


You can remove ip and last_update as they are now not used anymore.

MartinHjelmare · 2017-09-14T17:04:35Z

+            return False
+
+        # parsing response
+        now = dt_util.now()


MartinHjelmare · 2017-09-14T17:04:43Z

+                continue
+
+            name = info.get('name')
+            last_results.append(Device(mac.upper(), name, ipv4, now))


MartinHjelmare · 2017-09-14T17:05:50Z

+            if info.get('interface') != self._interface:
+                continue
+            mac = info.get('mac')
+            ipv4 = info.get('ip')


MartinHjelmare · 2017-09-14T17:05:56Z

+            mac = info.get('mac')
+            ipv4 = info.get('ip')
+            # No address = no item :)
+            if mac is None or ipv4 is None:


MartinHjelmare · 2017-09-14T17:08:10Z

+import voluptuous as vol
+
+import homeassistant.helpers.config_validation as cv
+import homeassistant.util.dt as dt_util


foxel · 2017-09-14T23:53:51Z

@MartinHjelmare, thanks for the catch

MartinHjelmare

Link to a docs PR in the description and we can merge this.

Added Zyxel Keenetic NDMS2 based routers support for device tracking

64a9940

homeassistant added cla-signed new-platform platform: device_tracker.keenetic_ndms2 labels Sep 6, 2017

MartinHjelmare requested changes Sep 12, 2017

View reviewed changes

Review feedback

ce04005

homeassistant added the cla-signed label Sep 13, 2017

balloobbot added the platform label Sep 13, 2017

Review feedback+

96c796d

homeassistant added the cla-signed label Sep 14, 2017

MartinHjelmare requested changes Sep 14, 2017

View reviewed changes

MartinHjelmare reviewed Sep 14, 2017

View reviewed changes

Review feedback: removed unneeded code

980c614

MartinHjelmare approved these changes Sep 15, 2017

View reviewed changes

foxel mentioned this pull request Sep 15, 2017

Added keenetic NDMS2 docs home-assistant/home-assistant.io#3373

Merged

MartinHjelmare merged commit 78bb0da into home-assistant:dev Sep 16, 2017

balloob mentioned this pull request Sep 22, 2017

0.54 #9532

Merged

home-assistant locked and limited conversation to collaborators Mar 3, 2018

ghost added integration: keenetic_ndms2 and removed platform: device_tracker.keenetic_ndms2 labels Mar 21, 2019

foxel deleted the keenetik-ndms2 branch November 14, 2019 14:37

		return scanner if scanner.success_init else None


		Device = namedtuple('Device', ['mac', 'name', 'ip', 'last_update'])

Uh oh!

Conversation

foxel commented Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Uh oh!

MartinHjelmare left a comment

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!

MartinHjelmare commented Sep 14, 2017

Uh oh!

MartinHjelmare left a comment

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!

foxel commented Sep 14, 2017

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

foxel commented Sep 6, 2017 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):