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

Fix device host discovery in config flow #52

Merged
merged 1 commit into from
Jun 19, 2021
Merged

Fix device host discovery in config flow #52

merged 1 commit into from
Jun 19, 2021

Conversation

pfrybar
Copy link
Contributor

@pfrybar pfrybar commented May 21, 2021

I setup my device with host discovery, but at some point the IP address of the device changed and HA lost connectivity. Looking through the code, the devices support discovery in async_setup_entry but only if the host was not set during the config flow.

Turns out the discovered host is being returned from _async_get_entry_data and saved to the config entry data. This small PR fixes that issue and will cause the host to be auto discovered.

@@ -214,6 +214,7 @@ async def _async_get_entry_data(
) -> Optional[str]:
"""Try connect and return config entry data."""
device = get_device(serial, credential, device_type)
saved_host = host
Copy link
Owner

Choose a reason for hiding this comment

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

Can you explain why this fixes the problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

During the config setup, if you leave the Host field empty it will get passed into this method as None. When this method discovers the host, it overrides the host variable because of the nonlocal host (line 224) and the config data that is returned will have a hard-coded host value. Instead of returning the discovered host, this change will return the passed in host value (None in this case).

Later on during the entry setup the code checks if the Host value is set - https://github.com/shenxn/ha-dyson/blob/main/custom_components/dyson_local/__init__.py#L107. If this value is None then the host will be discovered automatically which is what we want.

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for the explanation. Can you add some comments here since it's a little confusing.

Copy link
Owner

@shenxn shenxn left a comment

Choose a reason for hiding this comment

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

LGTM with nits

@@ -214,6 +214,7 @@ async def _async_get_entry_data(
) -> Optional[str]:
"""Try connect and return config entry data."""
device = get_device(serial, credential, device_type)
saved_host = host
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for the explanation. Can you add some comments here since it's a little confusing.

@shenxn shenxn changed the base branch from main to dev June 19, 2021 03:15
@shenxn shenxn merged commit 5c8e77d into shenxn:dev Jun 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants