home-assistant / core

:house_with_garden: Open source home automation that puts local control and privacy first.
https://www.home-assistant.io
Apache License 2.0
68.78k stars 28.11k forks source link

Add config flow to Genius hub #116173

Open GeoffAtHome opened 1 week ago

GeoffAtHome commented 1 week ago

Breaking change

Validation of MAC address has been removed.

Proposed change

Added config flow to integration including tests for config flow.

Type of change

Added configure flow.

Additional information

Checklist

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

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

To help with the load of incoming pull requests:

home-assistant[bot] commented 1 week ago

Hey there @manzanotti, mind taking a look at this pull request as it has been labeled with an integration (geniushub) you are listed as a code owner for? Thanks!

Code owner commands Code owners of `geniushub` can trigger bot actions by commenting: - `@home-assistant close` Closes the pull request. - `@home-assistant rename Awesome new title` Renames the pull request. - `@home-assistant reopen` Reopen the pull request. - `@home-assistant unassign geniushub` Removes the current integration label and assignees on the pull request, add the integration domain after the command. - `@home-assistant add-label needs-more-information` Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request. - `@home-assistant remove-label needs-more-information` Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.
GeoffAtHome commented 1 week ago

@manzanotti please review the config flow to ensure the UI makes sense. I tried to follow the same steps as in the documentation but the first choice may need improving.

Open to suggestions as how to validate the optional MAC address is really a MAC address. A similar test for testing validating a local IP address before calling validate_input.

manzanotti commented 1 week ago

@manzanotti please review the config flow to ensure the UI makes sense. I tried to follow the same steps as in the documentation but the first choice may need improving.

Open to suggestions as how to validate the optional MAC address is really a MAC address. A similar test for testing validating a local IP address before calling validate_input.

Should be able to take a look on Sunday. I look forward to it, I've wanted to put a UI together for ages, just haven't had time!

I would prefer a single commit with everything in it though, I must admit. (I would do a soft reset 5 back from HEAD, then create a single commit, rather than squashing the commits down, but that's my preference).

GeoffAtHome commented 1 week ago

@manzanotti please review the config flow to ensure the UI makes sense. I tried to follow the same steps as in the documentation but the first choice may need improving. Open to suggestions as how to validate the optional MAC address is really a MAC address. A similar test for testing validating a local IP address before calling validate_input.

Should be able to take a look on Sunday. I look forward to it, I've wanted to put a UI together for ages, just haven't had time!

I would prefer a single commit with everything in it though, I must admit. (I would do a soft reset 5 back from HEAD, then create a single commit, rather than squashing the commits down, but that's my preference).

Thanks. Not sure what you mean by soft reset.

Also, for another PR, rethink devices vs entities. I think TRVs should be represented as a device. The same for sensors and smart plugs.

manzanotti commented 6 days ago

Thanks. Not sure what you mean by soft reset.

Also, for another PR, rethink devices vs entities. I think TRVs should be represented as a device. The same for sensors and smart plugs.

Apologies, I had to spend all my free time yesterday setting up my Pi again, couldn't connect to HA on it. Hope to have time to look tomorrow evening.

git reset --soft HEAD~8

This will take you back 8 commits, but leaves all your file changes in place. Then you can create a new commit, and all your changes will be in that one single commit. Whilst this is perfectly safe, you can always create a new branch from the current one, come back to this branch, then do the soft reset. Then your original branch with 8 commits is still available.

home-assistant[bot] commented 6 days ago

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:

Learn more about our pull request process.

GeoffAtHome commented 4 days ago

Thanks. Not sure what you mean by soft reset. Also, for another PR, rethink devices vs entities. I think TRVs should be represented as a device. The same for sensors and smart plugs.

Apologies, I had to spend all my free time yesterday setting up my Pi again, couldn't connect to HA on it. Hope to have time to look tomorrow evening.

git reset --soft HEAD~8

This will take you back 8 commits, but leaves all your file changes in place. Then you can create a new commit, and all your changes will be in that one single commit. Whilst this is perfectly safe, you can always create a new branch from the current one, come back to this branch, then do the soft reset. Then your original branch with 8 commits is still available.

Hmm - not sure what I have done but I appear to have lost all my work.

joostlek commented 4 days ago

it's still on github, so as long as you don't (force) push you haven't lost anything

GeoffAtHome commented 4 days ago

Appears that I only lost one change to one file.

joostlek commented 4 days ago

I am also wondering, is there a reason why the cloud API specifies a MAC address and doesn't just fetch all devices?

GeoffAtHome commented 4 days ago

Need to catch adding the same device twice with config flow. The device isn't really added twice but it throws multiple errors. Must be a better way to handle this? Any suggestions?

joostlek commented 4 days ago

Can you add the device both via cloud and local at the same time?

GeoffAtHome commented 4 days ago

Can you add the device both via cloud and local at the same time?

I've never tried. That.... the device is keyed of the MAC address which is obtained by the API so in reality no. @manzanotti have you tried adding it local and by cloud?

Calling self._async_abort_entries_match(user_input) at the appropriate points catches the duplicate. But will need to check, tomorrow, with local and cloud.

joostlek commented 4 days ago

I'm asking because we can also use the mac as unique_id.

With the cloud API, can you get all devices without specifying on our side which devices we want?

So what I am imagining is a dual entry type setup. So whenever you chose cloud, the relation would be 1 config entry == 1 cloud account == n devices. When you chose local, 1 config entry == 1 device.

In the cloud option, we need to find a good candidate for an unique_id. This can be some kind of user_id that the API returns, or the username (in case this can't be changed). The unique_id for the local one would be the mac address.

And yes, this requires a bit of a refactor and isn't completely aligned with the scope of only adding a config flow, but since we have to import everything, might as well make sure we only import the data we need so future users can also do a quick setup. (Imagine having 5 devices and having to enter your username and password 5 times. Would work. Isn't very user friendly. If we can simplify this, that'd be a big plus)

joostlek commented 4 days ago

But again, I don't own any geniushub devices, let me know what you think and if this would work. Why/why not?

GeoffAtHome commented 3 days ago

But again, I don't own any geniushub devices, let me know what you think and if this would work. Why/why not?

OK I have tried to add the same device twice. Once using V1 API and again with V3 API. Bottom line is the second device is added with no entities.

A MAC address is required for V1 (Cloud) API but I need to do more investigation into the geniushubclient library. Also, when a device is deleted the instance of the client is not unloaded.

This is all going beyond the scope of this PR.

joostlek commented 3 days ago

Yes it doesn't completely fit the scope, but I see this also as a way to lay a good foundation for future improvements. It's easier to adapt the code now to only import cloud credentials once and then just work from there, instead of having a config entry for every device connected via the cloud and then merging them again.

So I hope you agree that if we can improve the user friendliness, we should do that. And I'm also offering help, since I would also like to see this PR succeed (since I like to see really nice tidy and neat integrations). So I am just trying to do a due dilligence on the path we should take to make sure the integration is user friendly and fit for the future.

GeoffAtHome commented 3 days ago

Yes it doesn't completely fit the scope, but I see this also as a way to lay a good foundation for future improvements. It's easier to adapt the code now to only import cloud credentials once and then just work from there, instead of having a config entry for every device connected via the cloud and then merging them again.

So I hope you agree that if we can improve the user friendliness, we should do that. And I'm also offering help, since I would also like to see this PR succeed (since I like to see really nice tidy and neat integrations). So I am just trying to do a due dilligence on the path we should take to make sure the integration is user friendly and fit for the future.

@joostlek thanks for your input and feedback.

I need to give this some though as I am having trouble testing the V1 API (Cloud) as the service I am connecting is returning: 2024-05-02 11:58:07.605 ERROR (MainThread) [homeassistant.components.geniushub] Setup failed, check your configuration, 503, message='Service Temporarily Unavailable', url=URL('https://my.geniushub.co.uk/v1/issues')

When the service is available all is good. When unable to setup is completely wrong.

Would love to know how to display device info and diagnostic info.

My set up is: 1 hub creates 79 entities. These are for Climate, TRV and Sensors. The hub probably should appear as a device. Probably each physical device should be a device with multiple entities.

@manzanotti please can you test the V1 connection. Maybe it's my account.

joostlek commented 3 days ago

devices and such are amazing and we can add them in a later PR. But now you say, does the mac you have to put in is for the bridge only?

Also, I am currently working a lot on my thesis, so I may be a bit slow to respond or review.

GeoffAtHome commented 2 days ago

This error: 2024-05-02 11:58:07.605 ERROR (MainThread) [homeassistant.components.geniushub] Setup failed, check your configuration, 503, message='Service Temporarily Unavailable', url=URL('https://my.geniushub.co.uk/v1/issues')

Is being caused by setting up a GeniusHub in validate_input. Removing setting up the GeniusHub from validate_input hides this problem and is more inline with how the original code worked before this PR.

This is only true for the V1 API. V3 API is fine.

Advantage of calling GeniusHub in validate_input is errors in the configuration are returned to the user immediately.

Is there an alternative way to validate the configuration without setting up a GeniusHub? More for me to investigate. Only set up a GeniusHub for V1 API? Less than idea but better than what we have at the moment.

It looks like the geniushubclient only support a single instance and does not lend itself to reconfiguration. My python isn't great but I cannot see any option to destroy a GeniusHub object and instantiate a new one. Will `del do the job?

If it were possible to add more that on `GeniusHub in YAML how would that be represented? Again, my YAML maybe lacking.

Hope to spend a little more time later today on this but failing that I won't be able to look at this for a few weeks as I am travelling again.

BTW: The solar eclipse in Texas was awesome!

joostlek commented 2 days ago

If you indeed can reproduce that the call only works when you have 1 entry at a time, it's probably something in the library (or integration) that is setting something to a shared value. This can either be like a global variable or a dict declared on class level instead of instance level

manzanotti commented 2 days ago

This error: 2024-05-02 11:58:07.605 ERROR (MainThread) [homeassistant.components.geniushub] Setup failed, check your configuration, 503, message='Service Temporarily Unavailable', url=URL('https://my.geniushub.co.uk/v1/issues')

Is being caused by setting up a GeniusHub in validate_input. Removing setting up the GeniusHub from validate_input hides this problem and is more inline with how the original code worked before this PR.

This is only true for the V1 API. V3 API is fine.

Advantage of calling GeniusHub in validate_input is errors in the configuration are returned to the user immediately.

Is there an alternative way to validate the configuration without setting up a GeniusHub? More for me to investigate. Only set up a GeniusHub for V1 API? Less than idea but better than what we have at the moment.

It looks like the geniushubclient only support a single instance and does not lend itself to reconfiguration. My python isn't great but I cannot see any option to destroy a GeniusHub object and instantiate a new one. Will `del do the job?

If it were possible to add more that on `GeniusHub in YAML how would that be represented? Again, my YAML maybe lacking.

Hope to spend a little more time later today on this but failing that I won't be able to look at this for a few weeks as I am travelling again.

BTW: The solar eclipse in Texas was awesome!

It probably requires one client per hub, rather than re-using one client between multiple hubs.

If you can't work on this for a few weeks, I will do my best to clone your branch and see what I can do.

joostlek commented 2 days ago

If you guys are up to it we could create a discord group to discuss about it

manzanotti commented 2 days ago

If you guys are up to it we could create a discord group to discuss about it

There is a gitter room for it, if that helps?

https://app.gitter.im/#/room/#geniushub-client_community:gitter.im