Skip to content
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

Modernize Huawei LTE #26675

Merged
merged 39 commits into from
Oct 24, 2019
Merged

Modernize Huawei LTE #26675

merged 39 commits into from
Oct 24, 2019

Conversation

scop
Copy link
Member

@scop scop commented Sep 16, 2019

Breaking Change:

Configuration has been consolidated below huawei_lte.

Device tracker no longer uses known_devices.yaml but entity registry.

Description:

Modernize Huawei LTE quite a bit, config entry support, unauthenticated access.

Related issue (if applicable): fixes #23819

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

Example entry for configuration.yaml (if applicable):

huawei_lte:
  - url: http://192.168.100.1/
    username: admin
    password: something

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.

homeassistant/components/huawei_lte/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/huawei_lte/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/huawei_lte/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/huawei_lte/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/huawei_lte/device_tracker.py Outdated Show resolved Hide resolved
homeassistant/components/huawei_lte/device_tracker.py Outdated Show resolved Hide resolved
homeassistant/components/huawei_lte/switch.py Outdated Show resolved Hide resolved
@scop scop changed the title Modernize Huawei LTE WIP: Modernize Huawei LTE Sep 22, 2019
scop added 4 commits October 5, 2019 20:01
- config entry support, with override support from huawei_lte platform in YAML
- device tracker entity registry support
- refactor for easier addition of more features
- internal code cleanups
No longer needed, because pretty much all keys from supported
categories are exposed as sensors.

Closes #23819
@scop scop requested a review from a team as a code owner October 5, 2019 17:01
@scop
Copy link
Member Author

scop commented Oct 5, 2019

So, here's a work in progress rework of the whole shebang, using entities disabled on discover, and entity enablement. Appears to work in principle, but I'm seeing some issues (see the "Work in progress" section).

@scop
Copy link
Member Author

scop commented Oct 5, 2019

Also, the passing of system_options to disable new entities when creating the config entry requires #27044 to be applied first.

@balloob
Copy link
Member

balloob commented Oct 5, 2019

If I go to entity registry and enable some disabled entities, after a while I receive these errors in the logs (and the whole component stops working). Dunno why this happens, or how to prevent it.

That happens when Home Assistant does the reload. Apparently your integration does not deal with reloads well. You need to make sure that when an entry is unloaded, you unload and detach all things that you set up. You should not return from that method until it is all set up. So don't create task but await the forward unload calls.

@balloob
Copy link
Member

balloob commented Oct 5, 2019

So when you re-enable an entity, Home Assistant doesn't know where that entity is, nor can it instantiate it. So what happens is that we wait up to 30 seconds after the last change and then reload the integration.

https://github.com/home-assistant/home-assistant/blob/5c01dd483fc6bf2d15848b7f4ebcc99fb45bb705/homeassistant/config_entries.py#L823-L831

Copy link
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.

Generally looks good I think. Beware of async vs sync context.

homeassistant/components/huawei_lte/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/huawei_lte/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/huawei_lte/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/huawei_lte/device_tracker.py Outdated Show resolved Hide resolved
homeassistant/components/huawei_lte/device_tracker.py Outdated Show resolved Hide resolved
homeassistant/components/huawei_lte/device_tracker.py Outdated Show resolved Hide resolved
tests/components/huawei_lte/test_config_flow.py Outdated Show resolved Hide resolved
@scop scop mentioned this pull request Oct 7, 2019
9 tasks
@scop
Copy link
Member Author

scop commented Oct 7, 2019

Thanks a lot for the assistance, I'll address rest of the comments a bit later.

@scop
Copy link
Member Author

scop commented Oct 11, 2019

Ok, with this batch of commits, stuff appears to work fine. Reload woes were because I was inadvertently using a mutable default defaultdict for all router instances (filed a couple of related pylint RFE's right after that), and there were some other less significant ones here and there.

I believe this is pretty much in a shape for inclusion now, still pending #27044 or similar, and the _better_snakecase should probably be turned to a general utility functions so other things can take advantage of it.

@scop
Copy link
Member Author

scop commented Oct 13, 2019

Way to set system_options tweaked, and new hopefully less intrusive PR for it filed as #27612.

@scop scop changed the title WIP: Modernize Huawei LTE Modernize Huawei LTE Oct 16, 2019
@scop
Copy link
Member Author

scop commented Oct 16, 2019

Tweaked to enable a small known subset of sensors by default, no longer requires #27612.

@scop
Copy link
Member Author

scop commented Oct 19, 2019

Should be good to go now.

@scop
Copy link
Member Author

scop commented Oct 19, 2019

Re better snakecase: okunishinishi/python-stringcase#18

Copy link
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.

Nice work!

@scop scop merged commit fc09702 into home-assistant:dev Oct 24, 2019
@scop scop deleted the huawei-lte-modernize branch October 24, 2019 16:32
@lock lock bot locked and limited conversation to collaborators Oct 25, 2019
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.

Components logger doesn't respect default logger level
5 participants