Skip to content

Add AsusWRT Devices Connected Sensor#34204

Merged
MartinHjelmare merged 7 commits into
home-assistant:devfrom
timmo001:asuswrt
Apr 15, 2020
Merged

Add AsusWRT Devices Connected Sensor#34204
MartinHjelmare merged 7 commits into
home-assistant:devfrom
timmo001:asuswrt

Conversation

@timmo001
Copy link
Copy Markdown
Member

@timmo001 timmo001 commented Apr 14, 2020

Proposed change

Adds a new sensor with the number of connected devices.

Type of change

  • New feature (which adds functionality to an existing integration)

Example entry for configuration.yaml:

# Example configuration.yaml
asuswrt:
  host: !secret asuswrt_host
  username: !secret asuswrt_username
  password: !secret asuswrt_password
  protocol: ssh
  mode: router
  sensors:
    - devices
    - upload
    - download
    - upload_speed
    - download_speed

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

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

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

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

@probot-home-assistant
Copy link
Copy Markdown

Hey there @kennedyshead, mind taking a look at this pull request as its been labeled with a integration (asuswrt) you are listed as a codeowner for? Thanks!

@timmo001
Copy link
Copy Markdown
Member Author

I did try to add a unique_id, but couldn't find a good candidate. @kennedyshead since it's your library, do you know a way to get the router's mac address or serial number for example?

This can always be a future change however

@timmo001 timmo001 marked this pull request as ready for review April 14, 2020 14:51
@MartinHjelmare MartinHjelmare changed the title AsusWRT - Add Devices Connected Sensor Add AsusWRT Devices Connected Sensor Apr 14, 2020
self._attributes = {
"mac_addresses": mac_addresses,
"ip_addresses": ip_addresses,
"hosts": hosts,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We try to avoid grouping values as attributes. Can we instead make one binary sensor per connected device with device class connectivity?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well we have device_tracker for that so I guess I'll remove the attributes

@timmo001 timmo001 requested a review from MartinHjelmare April 14, 2020 16:43
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codeowner should approve too.

@kennedyshead
Copy link
Copy Markdown
Contributor

kennedyshead commented Apr 15, 2020

This just might be a bad idea...
I will do some tests. There might be a way to build these sensors without doing extra calls?

@kennedyshead
Copy link
Copy Markdown
Contributor

Nah! This will work fine for now.
There will be a 5s cache in aioasuswrt in the future I think... These routers are sensitive in how many commands that run.

btw: Yes I'm 100% sure we can get a unique id for the router if we want to.

@kennedyshead
Copy link
Copy Markdown
Contributor

In short: 🐬

Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please extend the sensor tests to cover the diff. Ie set the "sensors" key in the config with "devices" when setting up the component with async_setup_component and patch the library appropriately.

Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please await hass.async_block_till_done() after setting up the component in the test and assert the state of the entities via the core state machine. We propably need to extend the mocking of the api methods to return appropriate data.

@timmo001 timmo001 requested a review from MartinHjelmare April 15, 2020 14:15
Comment thread tests/components/asuswrt/test_sensor.py Outdated
Comment thread tests/components/asuswrt/test_sensor.py Outdated
Comment thread tests/components/asuswrt/test_sensor.py
Comment thread tests/components/asuswrt/test_sensor.py Outdated
Comment thread tests/components/asuswrt/test_sensor.py Outdated
timmo001 and others added 2 commits April 15, 2020 15:32
Co-Authored-By: Martin Hjelmare <marhje52@gmail.com>
@timmo001 timmo001 requested a review from MartinHjelmare April 15, 2020 15:23
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

@MartinHjelmare MartinHjelmare merged commit e47b548 into home-assistant:dev Apr 15, 2020
@timmo001 timmo001 deleted the asuswrt branch April 15, 2020 15:47
@lock lock Bot locked and limited conversation to collaborators Apr 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants