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

Introduced Iperf3 client sensor #14213

Merged
merged 11 commits into from May 24, 2018

Conversation

Projects
None yet
7 participants
@tchellomello
Contributor

tchellomello commented May 1, 2018

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
    image

  • Upload
    image

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:

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.

@tchellomello tchellomello referenced this pull request May 1, 2018

Merged

Introducing Iperf3 sensor #5282

1 of 2 tasks complete

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

homeassistant/components/sensor/iperf3.py Outdated
hass, self.update, second=config.get(CONF_SECOND),
minute=config.get(CONF_MINUTE), hour=config.get(CONF_HOUR),
day=config.get(CONF_DAY))
@property

This comment has been minimized.

@houndci-bot

houndci-bot May 1, 2018

expected 1 blank line, found 0

homeassistant/components/sensor/iperf3.py Outdated
@property
def service_name(self):
"""Return the service name of the sensor."""
return slugify("{} {}".format('update_iperf3', self.iperf3_client.server))

This comment has been minimized.

@houndci-bot

houndci-bot May 1, 2018

line too long (82 > 79 characters)

homeassistant/components/sensor/iperf3.py Outdated
ATTR_TEST_STATUS = 'Test Status'
ATTR_VERSION = 'Version'
CONF_ATTRIBUTION = 'Data retrived using Iperf3'

This comment has been minimized.

@syssi

syssi May 1, 2018

Member

Typo: retrieved

This comment has been minimized.

@tchellomello

tchellomello May 1, 2018

Contributor

Thanks! 👍

@syssi

This comment has been minimized.

Member

syssi commented May 1, 2018

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

This comment has been minimized.

Contributor

tchellomello commented May 1, 2018

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

homeassistant/components/sensor/iperf3.py Outdated
CONF_MANUAL = 'manual'
CONF_DURATION = 'duration'
CONF_SERVER = 'server'
CONF_PORT = 'port'

This comment has been minimized.

@fabaff

fabaff May 1, 2018

Member

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

homeassistant/components/sensor/iperf3.py Outdated
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):

This comment has been minimized.

@balloob

balloob May 1, 2018

Member

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)

homeassistant/components/sensor/iperf3.py Outdated
sensor.update()
for sensor in dev:
hass.services.register(DOMAIN, sensor.service_name, update)

This comment has been minimized.

@balloob

balloob May 1, 2018

Member

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

homeassistant/components/sensor/iperf3.py Outdated
_LOGGER.error("Iperf3 sensor error: %s", result.error)
self.data = {
'download': STATE_UNKNOWN,

This comment has been minimized.

@balloob

balloob May 1, 2018

Member

Don't use STATE_UNKNOWN, use None instead.

homeassistant/components/sensor/iperf3.py Outdated
ATTR_PROTOCOL: STATE_UNKNOWN,
ATTR_REMOTE_HOST: STATE_UNKNOWN,
ATTR_REMOTE_PORT: STATE_UNKNOWN,
ATTR_VERSION: STATE_UNKNOWN,

This comment has been minimized.

@balloob

balloob May 1, 2018

Member

We should not store static info in the attributes.

homeassistant/components/sensor/iperf3.py Outdated
return ICON
class Iperf3Data(object):

This comment has been minimized.

@balloob

balloob May 1, 2018

Member

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

@tchellomello

This comment has been minimized.

Contributor

tchellomello commented May 16, 2018

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

image

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

This comment has been minimized.

Member

syssi commented May 22, 2018

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

homeassistant/components/sensor/iperf3.py Outdated
self._state = round(self.result.sent_Mbps, 2)
@asyncio.coroutine
def async_added_to_hass(self):

This comment has been minimized.

@balloob

balloob May 23, 2018

Member

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

@tchellomello

This comment has been minimized.

Contributor

tchellomello commented May 24, 2018

@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

syssi approved these changes May 24, 2018

@syssi

This comment has been minimized.

Member

syssi commented May 24, 2018

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

@tchellomello

This comment has been minimized.

Contributor

tchellomello commented May 24, 2018

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

tchellomello added some commits May 1, 2018

@tchellomello tchellomello requested a review from home-assistant/docker as a code owner May 24, 2018

@tchellomello

This comment has been minimized.

Contributor

tchellomello commented May 24, 2018

@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

This comment has been minimized.

Contributor

tchellomello commented May 24, 2018

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

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

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.003%) to 94.004%
Details

@tchellomello tchellomello deleted the tchellomello:iperf_dev branch May 24, 2018

@michaelarnauts

This comment has been minimized.

Contributor

michaelarnauts commented May 24, 2018

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

This comment has been minimized.

Contributor

tchellomello commented May 25, 2018

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

iMicknl added a commit to iMicknl/home-assistant that referenced this pull request May 31, 2018

@balloob balloob referenced this pull request Jun 8, 2018

Merged

0.71.0 #14876

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

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.