Introduced Iperf3 client sensor #14213

tchellomello · 2018-05-01T05:51:59Z

Description:

Introducing Iperf3 client sensor. With this sensor is possible to use iperf3 to test your network connection against a local or remote Iperf3 server.

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

Pull request in home-assistant/hassio-build: home-assistant/hassio-build#77

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

sensor:
  - platform: iperf3
    host: 192.168.169.6
    duration: 10 
    monitored_conditions:
      - download
      - upload

Download
Upload

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass

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

Documentation added/updated in home-assistant.github.io

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

New dependencies have been added to the REQUIREMENTS variable ([example][ex-requir]).
New dependencies are only imported inside functions that use them ([example][ex-import]).
New 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.

houndci-bot · 2018-05-01T06:53:13Z

homeassistant/components/sensor/iperf3.py

+                hass, self.update, second=config.get(CONF_SECOND),
+                minute=config.get(CONF_MINUTE), hour=config.get(CONF_HOUR),
+                day=config.get(CONF_DAY))
+    @property


expected 1 blank line, found 0

houndci-bot · 2018-05-01T06:53:13Z

homeassistant/components/sensor/iperf3.py

+    @property
+    def service_name(self):
+        """Return the service name of the sensor."""
+        return slugify("{} {}".format('update_iperf3', self.iperf3_client.server))


line too long (82 > 79 characters)

syssi · 2018-05-01T08:53:18Z

homeassistant/components/sensor/iperf3.py

+ATTR_TEST_STATUS = 'Test Status'
+ATTR_VERSION = 'Version'
+
+CONF_ATTRIBUTION = 'Data retrived using Iperf3'


Typo: retrieved

Thanks! 👍

syssi · 2018-05-01T09:03:10Z

LGTM just the config schema should be improved. What about a list of time periods instead of the bunch of config keys (hour, minute, second)?

tchellomello · 2018-05-01T15:39:37Z

@syssi I decided to follow this format as it is already in use in Fastcom and Speedtest sensors. Maybe we can simplify it and then change in all related components. I'm planning to do it in the future. Thanks for your review.

fabaff · 2018-05-01T18:18:46Z

homeassistant/components/sensor/iperf3.py

+CONF_MANUAL = 'manual'
+CONF_DURATION = 'duration'
+CONF_SERVER = 'server'
+CONF_PORT = 'port'


Can you please check if some of those constants are present already in const.py?

balloob · 2018-05-01T20:02:33Z

homeassistant/components/sensor/iperf3.py

+        vol.All(cv.ensure_list, [vol.All(vol.Coerce(int), vol.Range(0, 59))]),
+    vol.Optional(CONF_MINUTE, default=[0]):
+        vol.All(cv.ensure_list, [vol.All(vol.Coerce(int), vol.Range(0, 59))]),
+    vol.Optional(CONF_HOUR):


Please remove this. For platforms Home Assistant already allows users to override a scan interval by passing scan_interval: <duration> to the platform config. This is defined in PLATFORM_SCHEMA which you extend. To set a default, define a SCAN_INTERVAL = timedelta(minutes=30)

balloob · 2018-05-01T20:03:02Z

homeassistant/components/sensor/iperf3.py

+            sensor.update()
+
+    for sensor in dev:
+        hass.services.register(DOMAIN, sensor.service_name, update)


All services need to be always prefixed with the platform name.

balloob · 2018-05-01T20:04:29Z

homeassistant/components/sensor/iperf3.py

+                    _LOGGER.error("Iperf3 sensor error: %s", result.error)
+
+                    self.data = {
+                        'download': STATE_UNKNOWN,


Don't use STATE_UNKNOWN, use None instead.

balloob · 2018-05-01T20:04:42Z

homeassistant/components/sensor/iperf3.py

+                        ATTR_PROTOCOL: STATE_UNKNOWN,
+                        ATTR_REMOTE_HOST: STATE_UNKNOWN,
+                        ATTR_REMOTE_PORT: STATE_UNKNOWN,
+                        ATTR_VERSION: STATE_UNKNOWN,


We should not store static info in the attributes.

balloob · 2018-05-01T20:05:00Z

homeassistant/components/sensor/iperf3.py

+        return ICON
+
+
+class Iperf3Data(object):


Since there is only 1 per entity, why extract it into its own class?

tchellomello · 2018-05-16T06:02:54Z

@balloob @fabaff guys, PR updated.

Just one observation is to create the service_name I used the slugfy function to use the host to append to the service name. This way if the user has more than 1 instance of the iperf3 sensor, it will allow the user to select which sensor to trigger manually.

I noticed that we have this issue when using the fastdotcom and speedtest components. Only the last instance processed will have its service register. If this approach gets accepted by the team, I'll submit 2 new PR's to fix this behavior on these 2 other platforms too.

Thanks guys

syssi · 2018-05-22T19:26:57Z

If the component has an entity_id (like yours) a service call should look like this:

service: sensor.iperf3_update
entity_id: sensor.iperf3_localhost

If you want to update all iperf3 sensors you could call sensor.iperf3_update without an entity_id. This is an example of the needed service handler: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/light/lifx.py#L222-L234

Please prefix the service name with iperf3. (update_iperf3 -> iperf3_update).

balloob · 2018-05-23T13:11:54Z

homeassistant/components/sensor/iperf3.py

+            self._state = round(self.result.sent_Mbps, 2)
+
+    @asyncio.coroutine
+    def async_added_to_hass(self):


We should not restore state from the database since it can be queried from the device

tchellomello · 2018-05-24T00:22:57Z

@syssi @balloob guys, thanks for the code review.

The service now has the correct name and also supports to pass an entity_id in case the user wants to update just a particular entity. I also removed the state restore option too.

syssi · 2018-05-24T05:57:26Z

@pvizeli This component requires the iperf3 binary > 3.0.6. What's needed to provide this dependency on hass.io?

tchellomello · 2018-05-24T06:23:21Z

@syssi Thanks for your code review. I have actually already a PR ready to be submitted to add support to https://github.com/home-assistant/home-assistant/blob/dev/Dockerfile and I'm working on a separate PR too to HASS.io. I was waiting that to be merged so then I submit the next 2 ones.

…removed state restore

tchellomello · 2018-05-24T06:45:33Z

@syssi Updated this PR to make sure Iperf3 will add support to Home assistant docker image [1]. I'll submit a PR to HASS soon too.

[1] - https://hub.docker.com/r/homeassistant/home-assistant/

tchellomello · 2018-05-24T07:13:36Z

@syssi @pvizeli PR for hassio-build added at home-assistant/hassio-build#77

michaelarnauts · 2018-05-24T21:19:30Z

Actually, if you just need to install a package without compiling or other repositories, you should just add the package in the setup_prereqs file.

The separate scripts are for packages that need more work to install.

tchellomello · 2018-05-25T05:42:51Z

Thanks @michaelarnauts I'll make sure on the next PRs to do it this way! 👍

homeassistant added new-platform platform: sensor.iperf3 labels May 1, 2018

tchellomello mentioned this pull request May 1, 2018

Introducing Iperf3 sensor home-assistant/home-assistant.io#5282

Merged

2 tasks

tchellomello changed the title Introduced Iperf3 client sensor **WIP** Introduced Iperf3 client sensor May 1, 2018

homeassistant added the cla-signed label May 1, 2018

houndci-bot reviewed May 1, 2018

View reviewed changes

syssi reviewed May 1, 2018

View reviewed changes

homeassistant added the cla-signed label May 1, 2018

fabaff reviewed May 1, 2018

View reviewed changes

balloob reviewed May 1, 2018

View reviewed changes

homeassistant added the cla-signed label May 16, 2018

balloob reviewed May 23, 2018

View reviewed changes

tchellomello force-pushed the iperf_dev branch from 4c8324f to 834e2f5 Compare May 24, 2018 00:19

homeassistant added the cla-signed label May 24, 2018

syssi approved these changes May 24, 2018

View reviewed changes

tchellomello added 3 commits May 24, 2018 02:38

Introduced Iperf3 client sensor

a8224ea

Fixed protected-access lint errors

b70aaa3

Fixed houndci lint violations

d021525

tchellomello added 8 commits May 24, 2018 02:38

Fixed typo

d4f0a83

Refactored iperf3 and insert reverse logic

a5a5ff0

Updated update() signature

42fdb91

Make sure entity gets refreshed when service is triggered manually

b9c632a

Makes pylint happy

c438098

Round values

e279544

Refactored iperf3 to support service call filtering by entity_id and …

f5fb6ae

…removed state restore

Adding support to Iperf3 on Homeassistant Docker image

634450b

tchellomello force-pushed the iperf_dev branch from 834e2f5 to 634450b Compare May 24, 2018 06:43

tchellomello requested a review from a team May 24, 2018 06:43

homeassistant added the cla-signed label May 24, 2018

tchellomello mentioned this pull request May 24, 2018

Added iperf3 to homeassistant Docker image home-assistant/hassio-build#77

Merged

syssi merged commit 36da82a into home-assistant:dev May 24, 2018

tchellomello deleted the iperf_dev branch May 24, 2018 07:26

balloob mentioned this pull request Jun 8, 2018

0.71.0 #14876

Merged

girlpunk pushed a commit to girlpunk/home-assistant that referenced this pull request Sep 4, 2018

Add Iperf3 client sensor (home-assistant#14213)

a34bbc8

home-assistant locked and limited conversation to collaborators Sep 5, 2018

ghost added integration: iperf3 and removed platform: sensor.iperf3 labels Mar 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduced Iperf3 client sensor #14213

Introduced Iperf3 client sensor #14213

tchellomello commented May 1, 2018 •

edited

Loading

houndci-bot May 1, 2018

houndci-bot May 1, 2018

syssi May 1, 2018

tchellomello May 1, 2018

syssi commented May 1, 2018

tchellomello commented May 1, 2018

fabaff May 1, 2018

balloob May 1, 2018

balloob May 1, 2018

balloob May 1, 2018

balloob May 1, 2018

balloob May 1, 2018

tchellomello commented May 16, 2018

syssi commented May 22, 2018

balloob May 23, 2018 •

edited

Loading

tchellomello commented May 24, 2018

syssi commented May 24, 2018

tchellomello commented May 24, 2018

tchellomello commented May 24, 2018

tchellomello commented May 24, 2018

michaelarnauts commented May 24, 2018

tchellomello commented May 25, 2018

Introduced Iperf3 client sensor #14213

Introduced Iperf3 client sensor #14213

Conversation

tchellomello commented May 1, 2018 • edited Loading

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syssi commented May 1, 2018

tchellomello commented May 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchellomello commented May 16, 2018

syssi commented May 22, 2018

balloob May 23, 2018 • edited Loading

Choose a reason for hiding this comment

tchellomello commented May 24, 2018

syssi commented May 24, 2018

tchellomello commented May 24, 2018

tchellomello commented May 24, 2018

tchellomello commented May 24, 2018

michaelarnauts commented May 24, 2018

tchellomello commented May 25, 2018

tchellomello commented May 1, 2018 •

edited

Loading

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

balloob May 23, 2018 •

edited

Loading