Skip to content

Rework Mikrotik device scanning following Unifi#27484

Merged
balloob merged 25 commits into
home-assistant:devfrom
engrbm87:mikortik-integration
Jan 30, 2020
Merged

Rework Mikrotik device scanning following Unifi#27484
balloob merged 25 commits into
home-assistant:devfrom
engrbm87:mikortik-integration

Conversation

@engrbm87
Copy link
Copy Markdown
Contributor

@engrbm87 engrbm87 commented Oct 11, 2019

Breaking Change:

Mikrotik Integration can now be configured through config flow. It also supports importing from configuration.yaml (refer to the docs to update your configuration). Mikrotik integration now uses the entity registry for managing devices (known_devices.yaml will not be used anymore).

Description:

  • This PR reworks the Mikrotik integration to follow Unifi implementation and add config flow support.
  • If capsman or wireless interface is detected it will scan it and add/update devices status automatically. Otherwise it will fallback to DHCP list as source of devices.
  • The following options are included:
    • Force DHCP: Force using DHCP for scanning devices
    • ARP ping: Ping devices that have an active lease but are not in the wireless list. (Default is False)
    • Consider home interval: A device will change state to away if not detected after the set period. (Default: 300 seconds)

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#11927

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

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. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@engrbm87 engrbm87 changed the title rework device scanning, add tests Rework Mikrotik device scanning following Unifi Oct 11, 2019
@engrbm87
Copy link
Copy Markdown
Contributor Author

@MartinHjelmare I made a new pull request because I somehow messed up the commits in the previous one (#27093) . I have improved the device scanning algorithm and added tests.
Please take a look and let me know your comments

@engrbm87
Copy link
Copy Markdown
Contributor Author

Hi @MartinHjelmare , any chance you can look at this PR and let me know your comments.
Thanks

@MartinHjelmare
Copy link
Copy Markdown
Member

Sorry, this week is busy for me. I'll try to find some time.

@velaar
Copy link
Copy Markdown

velaar commented Nov 12, 2019

Testing this Mikrotik integration for about a week now. Two concerns:

  • It doesn’t seem to handle disabled entities properly
  • consider_home doesn’t seem to work as expected. Or should we use discover_interval instead?

@engrbm87
Copy link
Copy Markdown
Contributor Author

Testing this Mikrotik integration for about a week now. Two concerns:

* It doesn’t seem to handle disabled entities properly

* consider_home doesn’t seem to work as expected. Or should we use discover_interval instead?

Can you provide more details:
What is happening to disabled entities?
Are you testing with wireless or wired devices?
How much did you set the consider_home value and after how long is it setting the device as away?

@velaar
Copy link
Copy Markdown

velaar commented Nov 13, 2019

Can you provide more details:
What is happening to disabled entities?
Are you testing with wireless or wired devices?

I am testing with wireless devices.

2019-11-10 21:39:53 WARNING (MainThread) [homeassistant.helpers.entity] Entity device_tracker.hs110 is incorrectly being triggered for updates while it is disabled. This is a bug in the mikrotik integration.
2019-11-10 21:40:09 WARNING (MainThread) [homeassistant.helpers.entity] Entity device_tracker.harmonyhub is incorrectly being triggered for updates while it is disabled. This is a bug in the mikrotik integration.
2019-11-10 21:40:25 WARNING (MainThread) [homeassistant.helpers.entity] Entity device_tracker.vlads_ipad_pro is incorrectly being triggered for updates while it is disabled. This is a bug in the mikrotik integration.
2019-11-10 21:42:31 WARNING (MainThread) [homeassistant.helpers.entity] Entity device_tracker.vladimiplewatch is incorrectly being triggered for updates while it is disabled. This is a bug in the mikrotik integration.

How much did you set the consider_home value and after how long is it setting the device as away?

This one cleared up by itself. It is possible I had an inconsistent wifi connection. Sorry, my bad.

@engrbm87
Copy link
Copy Markdown
Contributor Author

@velaar , Thanks for the feedback and testing. Updating disabled entities can be fixed I will update the code.
Can you please confirm that the functionality of the scanning is working as expected. Any concerns?

@velaar
Copy link
Copy Markdown

velaar commented Nov 18, 2019

@engrbm87 scanning functionality works as expected for me. Thank you for fixing the disabled entities. Wish there was an easy way of managing entities but this is obviously out of scope for this integration.

@engrbm87
Copy link
Copy Markdown
Contributor Author

@engrbm87 scanning functionality works as expected for me. Thank you for fixing the disabled entities. Wish there was an easy way of managing entities but this is obviously out of scope for this integration.

ًWhat do you mean "easy way"?

@engrbm87
Copy link
Copy Markdown
Contributor Author

Hello @balloob . For this PR I am setting the system option enable_newly_added_entities using an option in the config flow initial setup. Is that OK or how shall we let the user decide if they want to enable new entities by default or enable manually later?

@tjorim
Copy link
Copy Markdown
Contributor

tjorim commented Dec 21, 2019

@engrbm87 Could you please rebase this PR?
Also, while at it, feel free to pin the librouteros version to 3.0.0 (that release removed the ConnectionError exception).

Comment thread homeassistant/components/mikrotik/__init__.py Outdated
Comment thread homeassistant/components/mikrotik/hub.py Outdated
Comment thread homeassistant/components/mikrotik/hub.py Outdated
Comment thread homeassistant/components/mikrotik/hub.py Outdated
Comment thread homeassistant/components/mikrotik/hub.py Outdated
Comment thread tests/components/mikrotik/test_config_flow.py Outdated
Comment thread tests/components/mikrotik/test_device_tracker.py Outdated
Comment thread tests/components/mikrotik/test_hub.py Outdated
Comment thread tests/components/mikrotik/test_hub.py Outdated
Comment thread tests/components/mikrotik/test_hub.py
Comment thread tests/components/mikrotik/test_hub.py Outdated
Comment thread tests/components/mikrotik/test_hub.py Outdated
Comment thread tests/components/mikrotik/test_config_flow.py Outdated
Comment thread tests/components/mikrotik/test_hub.py Outdated
@MartinHjelmare
Copy link
Copy Markdown
Member

MartinHjelmare commented Jan 10, 2020

The coverage is below 95% in the device tracker and hub modules. It's ok to exclude these from coverage calculation for now.

https://codecov.io/gh/home-assistant/home-assistant/pull/27484/diff

@tjorim
Copy link
Copy Markdown
Contributor

tjorim commented Jan 25, 2020

@engrbm87 Looks great! There are some new merge conflicts because of #30800 and #30810.

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.

See the hue config flow tests for good examples of config flow tests.

Comment thread tests/components/mikrotik/test_config_flow.py Outdated
Comment thread tests/components/mikrotik/test_config_flow.py Outdated
Comment thread tests/components/mikrotik/test_config_flow.py Outdated
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.

Looks good!

@MartinHjelmare
Copy link
Copy Markdown
Member

Is this a breaking change? It's not clear from the breaking change paragraph.

@frenck
Copy link
Copy Markdown
Member

frenck commented Jan 30, 2020

@MartinHjelmare Yes, it is.

@MartinHjelmare
Copy link
Copy Markdown
Member

@engrbm87 please update the breaking change paragraph to mention what is breaking and what the user needs to do to cope with the breaking change.

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.

8 participants