Navigation Menu

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

Jump to bottom

Add config flow and device registry to fritzbox integration #31240

Merged
merged 56 commits into from Apr 20, 2020
Merged

Add config flow and device registry to fritzbox integration #31240

merged 56 commits into from Apr 20, 2020

Conversation

escoand
Copy link
Contributor

@escoand escoand commented Jan 28, 2020 •
edited

Breaking change

YAML configuration has been deprecated and should no longer be used, it will be removed in one of the next releases.

Proposed change

Add config flow and device registry to fritzbox integration.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml
fritzbox:
  devices:
    - password: YOUR_PASSWORD

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.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@project-bot project-bot bot added this to Needs review in Dev Jan 28, 2020
@escoand
Copy link
Contributor Author

escoand commented Jan 28, 2020 •
edited

Points I'm not sure about:

  1. It's possible to power on/off a PowerLine adapter by itself and also by a central router device. So it's possible to have the same switch at multiple devices. So HA is complaining about non-unique IDs.
  2. I would like to remove the devices layer in the configuration - it doesn't make much sense to me. Is there a reason it was ever added?
  3. I dislike the handling of connecting to a device in __init__ and pass the object to hass.data. What is the best practice for working with shared objects?
  4. The fritzdect integration has the same focus but less functionality. Furthermore the used pypi module fritzhome is not supported anymore, in contrast to pyfritzhome used by this integration. Shouldn't we drop fritzdect?

@pvizeli
Copy link
Member

pvizeli commented Jan 30, 2020

Yes, drop fritzdect and let say we replace it with this PR to become one fritzbox integration.

@MartinHjelmare
Copy link
Member

MartinHjelmare commented Jan 30, 2020 •
edited

Please don't do everything in this PR. This PR should only focus on the config flow addition for this integration.

Further PRs are welcome where we replace the fritzdect integration.

@MartinHjelmare
Copy link
Member

MartinHjelmare commented Jan 30, 2020 •
edited

Regarding 3., that's the best practice, ie to connect to the device in the component module and store it in hass.data, when more than one platform relies on the same device instance.

@escoand
Copy link
Contributor Author

escoand commented Jan 30, 2020

Further PRs are welcome where we replace the fritzdetct integration.

The drop of fritzdect doesn't belong to this PR directly. It could be done at any time as it's already today not needed anymore.

@escoand
Copy link
Contributor Author

escoand commented Jan 31, 2020 •
edited

It's possible to power on/off a PowerLine adapter by itself and also by a central router device. So it's possible to have the same switch at multiple devices. So HA is complaining about non-unique IDs.

@MartinHjelmare @pvizeli And hints here?

@escoand
Copy link
Contributor Author

escoand commented Jan 31, 2020

Further PRs are welcome where we replace the fritzdetct integration.

The drop of fritzdect doesn't belong to this PR directly. It could be done at any time as it's already today not needed anymore.

see #31359

@MartinHjelmare
Copy link
Member

Why would the same entity be represented by different devices?

It sounds like there are two devices that interact and the first device can be actuated by the second device. We will still only represent the first device with a single entity. It doesn't matter for the entity how the first device is actuated.

@escoand
Copy link
Contributor Author

escoand commented Feb 1, 2020 •
edited

AVM has a so called Mesh functionality. All their products can be controlled from every device's web ui. This is the reason why every configured integration can see and control every device.
Yes your right, the entity is only shown once but on setup HA is complaining about non-unique ids on 2nd, 3rd, ... setup of the same entity.

@MartinHjelmare
Copy link
Member

The same entity is not allowed to be set up more than once.

@escoand
Copy link
Contributor Author

escoand commented Feb 1, 2020

That's why I'm asking. ;-)
Where and how is the best way to check this in a thread-safe manner.

@MartinHjelmare
Copy link
Member

MartinHjelmare commented Feb 1, 2020 •
edited

Store the ids of the devices or entities and make sure to not set up the same id more than once.

Usually a set is good to store unique items. If the container needs to be accessed more than once or from different module, store it in hass.data.

@escoand
Copy link
Contributor Author

escoand commented Feb 1, 2020

OK, I thought there is maybe a built-in helper function.

@escoand
Copy link
Contributor Author

escoand commented Apr 18, 2020

@MartinHjelmare Would like to set the quality scale but I'm unsure which is right.
There is only the one failing test, then it's ready for merge.

@escoand escoand mentioned this pull request Apr 18, 2020
Closed
@MartinHjelmare
Copy link
Member

We shouldn't try to squeeze everything into this PR. Just make sure tests are good and things are working. Then we merge.

MartinHjelmare
MartinHjelmare approved these changes Apr 19, 2020
View reviewed changes
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.

Looks good!

@MartinHjelmare
Copy link
Member

Please update the breaking change paragraph in the PR description so we can copy it directly to the release notes. Briefly describe what changed and what the users need to do to cope with the breaking change.

@TheZoker
Copy link
Contributor

I just tested the latest version of this PR as a custom integration. I removed all config items and recreated them in the integrations panel and I get the following issue in the second I create them:

2020-04-20 01:40:08 ERROR (MainThread) [homeassistant.config] Invalid config for [fritzbox]: [homeassistant] is an invalid option for [fritzbox]. Check: fritzbox->homeassistant. (See /config/configuration.yaml, line 0). Please check the docs at https://www.home-assistant.io/integrations/fritzbox
2020-04-20 01:40:08 ERROR (MainThread) [homeassistant.setup] Setup failed for fritzbox: Invalid config.

When I remove the integration and restart HA, the error is gone. When I add the integration again, the error is in my log.

MartinHjelmare
MartinHjelmare requested changes Apr 19, 2020
View reviewed changes
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.

Thanks for the report. I think this should fix that.

homeassistant/components/fritzbox/__init__.py Outdated Show resolved Hide resolved
Dev automation moved this from Reviewer approved to Review in progress Apr 19, 2020
MartinHjelmare
MartinHjelmare approved these changes Apr 20, 2020
View reviewed changes
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.

Can be merged when build passes.

Dev automation moved this from Review in progress to Reviewer approved Apr 20, 2020
@escoand
Copy link
Contributor Author

escoand commented Apr 20, 2020

@MartinHjelmare Thanks for the fix and all your reviews.

@MartinHjelmare MartinHjelmare merged commit c87ecf0 into home-assistant:dev Apr 20, 2020
Dev automation moved this from Reviewer approved to Done Apr 20, 2020
@escoand escoand deleted the fritzbox_config_flow branch April 20, 2020 17:50
@lock lock bot locked and limited conversation to collaborators Apr 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@TheZoker TheZoker TheZoker left review comments

@balloob balloob balloob left review comments

@MartinHjelmare MartinHjelmare MartinHjelmare approved these changes

Assignees
No one assigned
Labels
breaking-change cla-signed integration: fritzbox
Projects
Dev
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@escoand @pvizeli @MartinHjelmare @TheZoker @balloob @frenck @homeassistant