-
-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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
Add Husqvarna Automower integration #100569
Conversation
@Thomas55555 seems to be some issues with the pytests |
I've improved the tests, but they are not passing yet, because I can't handle this in the tests: core/homeassistant/components/husqvarna_automower/__init__.py Lines 29 to 33 in ddf94cd
In the previous PR, we focused on getting the code right, and the tests were blocked in the CI. |
@Thomas55555 just a question, this integration will be missing the calendar and all the other sensors right? |
We need to finish the tests before merging the PR, but it's ok to wait with the tests until the rest of the code is done. |
from homeassistant.helpers import config_entry_oauth2_flow | ||
|
||
|
||
class AsyncConfigEntryAuth(aioautomower.AbstractAuth): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The library client should use this auth instance. It looks unused now. We just instantiate it but then don't use it.
When the library client wants to get an access token to use, ie before every request, it should use async_get_access_token
defined in this class. The library doesn't seem to have been updated to allow this. I recommend reading our library guide on how to structure the library and client:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reworked the library according to the documentation. I actually like this approach.
Everything works fine now except of the websocket connection. It's not possible to establish it and I don't get an error. I've tried very long, but wasn't able to get it working. Could you have a look at the library, please?
https://github.com/Thomas55555/aioautomower/blob/a196d9fbbff1f9fddf65a3eed3d029872ce51a00/aioautomower/session.py#L371
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you should make a loop when connecting the websocket. Just have a connect
method that connects the websocket and stores a reference to it on the client.
Then have another method listen
that fetches incoming messages and iterates over them and handles them. It's this method that the library user should create a background task for.
The benefit of separating the connect
and listen
methods is that we can make sure we can connect the websocket with a timeout before we create the task that should listen for websocket messages. If we can connect the websocket the chances are high that the websocket communication will work. If we fail to connect during the timeout, we want to raise ConfigEntryNotReady
and try again later automatically.
Side notes:
-
It looks like there's a double call to update data callbacks:
https://github.com/Thomas55555/aioautomower/blob/a196d9fbbff1f9fddf65a3eed3d029872ce51a00/aioautomower/session.py#L380-381
https://github.com/Thomas55555/aioautomower/blob/a196d9fbbff1f9fddf65a3eed3d029872ce51a00/aioautomower/session.py#L342 -
I'd remove the schedule immediately parameter. The library user can call the callback themselves if needed.
https://github.com/Thomas55555/aioautomower/blob/a196d9fbbff1f9fddf65a3eed3d029872ce51a00/aioautomower/session.py#L74
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we create a connect
method in the library for the websocket, there should also be a disconnect
method for the websocket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is now a close
method
|
||
async def _async_update_data(self) -> None: | ||
"""Fetch data from Husqvarna.""" | ||
await self.session.connect() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the library. It currently doesn't consistently use the aiohttp client session that is provided when instantiating the abstract auth class. The library should use that provided client session for all requests made. It's ok to create a default aiohttp client session if no session is provided by the library user.
I also suggest dropping or making optional the task that gets the status. The library user can do that. In Home Assistant we can use the update coordinator here to do that.
We can also use the coordinator and set updated data when the websocket callbacks with updated data. There's this coordinator method:
core/homeassistant/helpers/update_coordinator.py
Lines 400 to 402 in 11c4c37
@callback | |
def async_set_updated_data(self, data: _DataT) -> None: | |
"""Manually update data, notify listeners and reset refresh interval.""" |
Then we can remove the callback registration from the entities, and just do that in the coordinator instead. The coordinator will update all coordinator entities when the coordinator is updated.
I also suggest being consistent in the library with exceptions and logging.
- Debug logging is always ok.
- Errors that happen in non background tasks are preferably raised as library specific exceptions and not logged. Let the library user decide how to handle the error and if they want to log it.
- In background tasks raised exceptions are not as useful, unless we make sure that the library user can handle the exception. One way to do that is to let the library user create and handle the task, ie just make a coroutine method that listens for the websocket messages and let the library user create the task for that. This way the library user can easier react to exceptions and decide how to handle them, eg re-connecting with a backoff, like what is done when Home Assistant raises
ConfigEntryNotReady
.
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
tests/components/husqvarna_automower/test_application_credentials.py
Outdated
Show resolved
Hide resolved
tests/components/husqvarna_automower/test_application_credentials.py
Outdated
Show resolved
Hide resolved
In the beginning yes. Later I want to add additional platforms. |
) | ||
if not self.ws_connected: | ||
await self.mowersession.connect() | ||
self.mowersession.register_data_callback( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this just stores the callback reference, we probably don't need to do it after every connect call, but just once for the coordinator. Or is the callback unregistered if the connection is lost?
await setup_entity(hass) | ||
coordinator = hass.data[DOMAIN]["automower_test"] | ||
lawn_mower = AutomowerLawnMowerEntity(mower.data[0], coordinator) | ||
coordinator: AutomowerDataUpdateCoordinator = hass.data[DOMAIN]["automower_test"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't interact directly with the coordinator or any entities in tests. Those are integrations details. Set up the integration while patching the client library appropriately and assert core state.
https://developers.home-assistant.io/docs/development_testing#writing-tests-for-integrations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed some of the tests in this file. Can you tell me, if this is going in the right direction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I added some comments below.
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
I'm hoping we can finish this PR at some point. |
I was quite busy the last months. But I was working on the library the last days. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests are mowing in the right direction. 👍
resume_schedule=AsyncMock(), | ||
), | ||
), patch( | ||
"homeassistant.helpers.config_entry_oauth2_flow.async_get_config_entry_implementation", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't need to patch all these details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it isn't patched, there are errors like that: FAILED tests/components/husqvarna_automower/test_lawn_mower.py::test_lawn_mower_commands - TypeError: <AsyncMock name='async_get_config_entry_implementation().async_refresh_token()' id='139843651950352'>
I don't know another way to avoid them.
* add translations to exceptions * Make errors more user understandable * make command not ternary operator * removed non-user facing exceptions * Add user facing exceptions and code coverage * add match * fix linting * Apply suggestions from code review Co-authored-by: Franck Nijhof <frenck@frenck.nl> --------- Co-authored-by: Franck Nijhof <frenck@frenck.nl>
* Add cloud tts entity * Test test_login_view_missing_entity * Fix pipeline iteration for migration * Update tests * Make migration more strict * Fix docstring
…108034) * Add helper function to update and reload config entry to config flow * Use async_create_task * Remove await * Reload only when update & add task name * Rename function
* Cleanup Discovergy config flow * Make use of the helper function
Co-authored-by: Erik <erik@montnemery.com> Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
…M Fritz!Tools (#108667) prevent dictionary changed size during iteration
* Use snapshot testing in Airly sensor * Apply suggestions from code review Co-authored-by: Robert Resch <robert@resch.dev> * Fix tests --------- Co-authored-by: Robert Resch <robert@resch.dev>
* Add icon translations to Airly * Fix test * Fix tests
* Fixing foot warmer timer bug * Fixing bug where temperature wasnt assigned to number entity causing tests to fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @Thomas55555,
When attempting to inspect the commits of your pull request for CLA signature status among all authors we encountered commit(s) which were not linked to a GitHub account, thus not allowing us to determine their status(es).
The commits that are missing a linked GitHub account are the following:
-
6b597ac2607dc679b7950f29377cc9fc631bf27d
- This commit has something that looks like an email address (thomas.protzner@gmai.com). Maybe try linking that to GitHub?. -
2d59d507e5025d964459b2105d06cd4f60fe75ec
- This commit has something that looks like an email address (thomas.protzner@gmai.com). Maybe try linking that to GitHub?.
Unfortunately, we are unable to accept this pull request until this situation is corrected.
Here are your options:
-
If you had an email address set for the commit that simply wasn't linked to your GitHub account you can link that email now and it will retroactively apply to your commits. The simplest way to do this is to click the link to one of the above commits and look for a blue question mark in a blue circle in the top left. Hovering over that bubble will show you what email address you used. Clicking on that button will take you to your email address settings on GitHub. Just add the email address on that page and you're all set. GitHub has more information about this option in their help center.
-
If you didn't use an email address at all, it was an invalid email, or it's one you can't link to your GitHub, you will need to change the authorship information of the commit and your global Git settings so this doesn't happen again going forward. GitHub provides some great instructions on how to change your authorship information in their help center.
- If you only made a single commit you should be able to run
(substituting "Author Name" and "
git commit --amend --author="Author Name <email@address.com>"
email@address.com
" for your actual information) to set the authorship information. - If you made more than one commit and the commit with the missing authorship information is not the most recent one you have two options:
- You can re-create all commits missing authorship information. This is going to be the easiest solution for developers that aren't extremely confident in their Git and command line skills.
- You can use this script that GitHub provides to rewrite history. Please note: this should be used only if you are very confident in your abilities and understand its impacts.
- Whichever method you choose, I will come by to re-check the pull request once you push the fixes to this branch.
- If you only made a single commit you should be able to run
We apologize for this inconvenience, especially since it usually bites new contributors to Home Assistant. We hope you understand the need for us to protect ourselves and the great community we all have built legally. The best thing to come out of this is that you only need to fix this once and it benefits the entire Home Assistant and GitHub community.
Thanks, I look forward to checking this PR again soon! ❤️
* Add time to charge and type checking * Revert drive_state_shift_state change * Use original name * Use function instead of lambda * Update homeassistant/components/tessie/sensor.py Co-authored-by: Jan-Philipp Benecke <github@bnck.me> * Fix callback * Avoid having to test None * Go back to if * Use minutes instead of hours --------- Co-authored-by: Jan-Philipp Benecke <github@bnck.me>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @Thomas55555,
When attempting to inspect the commits of your pull request for CLA signature status among all authors we encountered commit(s) which were not linked to a GitHub account, thus not allowing us to determine their status(es).
The commits that are missing a linked GitHub account are the following:
-
6b597ac2607dc679b7950f29377cc9fc631bf27d
- This commit has something that looks like an email address (thomas.protzner@gmai.com). Maybe try linking that to GitHub?. -
2d59d507e5025d964459b2105d06cd4f60fe75ec
- This commit has something that looks like an email address (thomas.protzner@gmai.com). Maybe try linking that to GitHub?.
Unfortunately, we are unable to accept this pull request until this situation is corrected.
Here are your options:
-
If you had an email address set for the commit that simply wasn't linked to your GitHub account you can link that email now and it will retroactively apply to your commits. The simplest way to do this is to click the link to one of the above commits and look for a blue question mark in a blue circle in the top left. Hovering over that bubble will show you what email address you used. Clicking on that button will take you to your email address settings on GitHub. Just add the email address on that page and you're all set. GitHub has more information about this option in their help center.
-
If you didn't use an email address at all, it was an invalid email, or it's one you can't link to your GitHub, you will need to change the authorship information of the commit and your global Git settings so this doesn't happen again going forward. GitHub provides some great instructions on how to change your authorship information in their help center.
- If you only made a single commit you should be able to run
(substituting "Author Name" and "
git commit --amend --author="Author Name <email@address.com>"
email@address.com
" for your actual information) to set the authorship information. - If you made more than one commit and the commit with the missing authorship information is not the most recent one you have two options:
- You can re-create all commits missing authorship information. This is going to be the easiest solution for developers that aren't extremely confident in their Git and command line skills.
- You can use this script that GitHub provides to rewrite history. Please note: this should be used only if you are very confident in your abilities and understand its impacts.
- Whichever method you choose, I will come by to re-check the pull request once you push the fixes to this branch.
- If you only made a single commit you should be able to run
We apologize for this inconvenience, especially since it usually bites new contributors to Home Assistant. We hope you understand the need for us to protect ourselves and the great community we all have built legally. The best thing to come out of this is that you only need to fix this once and it benefits the entire Home Assistant and GitHub community.
Thanks, I look forward to checking this PR again soon! ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @Thomas55555,
When attempting to inspect the commits of your pull request for CLA signature status among all authors we encountered commit(s) which were not linked to a GitHub account, thus not allowing us to determine their status(es).
The commits that are missing a linked GitHub account are the following:
-
6b597ac2607dc679b7950f29377cc9fc631bf27d
- This commit has something that looks like an email address (thomas.protzner@gmai.com). Maybe try linking that to GitHub?. -
2d59d507e5025d964459b2105d06cd4f60fe75ec
- This commit has something that looks like an email address (thomas.protzner@gmai.com). Maybe try linking that to GitHub?.
Unfortunately, we are unable to accept this pull request until this situation is corrected.
Here are your options:
-
If you had an email address set for the commit that simply wasn't linked to your GitHub account you can link that email now and it will retroactively apply to your commits. The simplest way to do this is to click the link to one of the above commits and look for a blue question mark in a blue circle in the top left. Hovering over that bubble will show you what email address you used. Clicking on that button will take you to your email address settings on GitHub. Just add the email address on that page and you're all set. GitHub has more information about this option in their help center.
-
If you didn't use an email address at all, it was an invalid email, or it's one you can't link to your GitHub, you will need to change the authorship information of the commit and your global Git settings so this doesn't happen again going forward. GitHub provides some great instructions on how to change your authorship information in their help center.
- If you only made a single commit you should be able to run
(substituting "Author Name" and "
git commit --amend --author="Author Name <email@address.com>"
email@address.com
" for your actual information) to set the authorship information. - If you made more than one commit and the commit with the missing authorship information is not the most recent one you have two options:
- You can re-create all commits missing authorship information. This is going to be the easiest solution for developers that aren't extremely confident in their Git and command line skills.
- You can use this script that GitHub provides to rewrite history. Please note: this should be used only if you are very confident in your abilities and understand its impacts.
- Whichever method you choose, I will come by to re-check the pull request once you push the fixes to this branch.
- If you only made a single commit you should be able to run
We apologize for this inconvenience, especially since it usually bites new contributors to Home Assistant. We hope you understand the need for us to protect ourselves and the great community we all have built legally. The best thing to come out of this is that you only need to fix this once and it benefits the entire Home Assistant and GitHub community.
Thanks, I look forward to checking this PR again soon! ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @Thomas55555,
When attempting to inspect the commits of your pull request for CLA signature status among all authors we encountered commit(s) which were not linked to a GitHub account, thus not allowing us to determine their status(es).
The commits that are missing a linked GitHub account are the following:
-
6b597ac2607dc679b7950f29377cc9fc631bf27d
- This commit has something that looks like an email address (thomas.protzner@gmai.com). Maybe try linking that to GitHub?. -
2d59d507e5025d964459b2105d06cd4f60fe75ec
- This commit has something that looks like an email address (thomas.protzner@gmai.com). Maybe try linking that to GitHub?.
Unfortunately, we are unable to accept this pull request until this situation is corrected.
Here are your options:
-
If you had an email address set for the commit that simply wasn't linked to your GitHub account you can link that email now and it will retroactively apply to your commits. The simplest way to do this is to click the link to one of the above commits and look for a blue question mark in a blue circle in the top left. Hovering over that bubble will show you what email address you used. Clicking on that button will take you to your email address settings on GitHub. Just add the email address on that page and you're all set. GitHub has more information about this option in their help center.
-
If you didn't use an email address at all, it was an invalid email, or it's one you can't link to your GitHub, you will need to change the authorship information of the commit and your global Git settings so this doesn't happen again going forward. GitHub provides some great instructions on how to change your authorship information in their help center.
- If you only made a single commit you should be able to run
(substituting "Author Name" and "
git commit --amend --author="Author Name <email@address.com>"
email@address.com
" for your actual information) to set the authorship information. - If you made more than one commit and the commit with the missing authorship information is not the most recent one you have two options:
- You can re-create all commits missing authorship information. This is going to be the easiest solution for developers that aren't extremely confident in their Git and command line skills.
- You can use this script that GitHub provides to rewrite history. Please note: this should be used only if you are very confident in your abilities and understand its impacts.
- Whichever method you choose, I will come by to re-check the pull request once you push the fixes to this branch.
- If you only made a single commit you should be able to run
We apologize for this inconvenience, especially since it usually bites new contributors to Home Assistant. We hope you understand the need for us to protect ourselves and the great community we all have built legally. The best thing to come out of this is that you only need to fix this once and it benefits the entire Home Assistant and GitHub community.
Thanks, I look forward to checking this PR again soon! ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @Thomas55555,
When attempting to inspect the commits of your pull request for CLA signature status among all authors we encountered commit(s) which were not linked to a GitHub account, thus not allowing us to determine their status(es).
The commits that are missing a linked GitHub account are the following:
-
6b597ac2607dc679b7950f29377cc9fc631bf27d
- This commit has something that looks like an email address (thomas.protzner@gmai.com). Maybe try linking that to GitHub?. -
2d59d507e5025d964459b2105d06cd4f60fe75ec
- This commit has something that looks like an email address (thomas.protzner@gmai.com). Maybe try linking that to GitHub?.
Unfortunately, we are unable to accept this pull request until this situation is corrected.
Here are your options:
-
If you had an email address set for the commit that simply wasn't linked to your GitHub account you can link that email now and it will retroactively apply to your commits. The simplest way to do this is to click the link to one of the above commits and look for a blue question mark in a blue circle in the top left. Hovering over that bubble will show you what email address you used. Clicking on that button will take you to your email address settings on GitHub. Just add the email address on that page and you're all set. GitHub has more information about this option in their help center.
-
If you didn't use an email address at all, it was an invalid email, or it's one you can't link to your GitHub, you will need to change the authorship information of the commit and your global Git settings so this doesn't happen again going forward. GitHub provides some great instructions on how to change your authorship information in their help center.
- If you only made a single commit you should be able to run
(substituting "Author Name" and "
git commit --amend --author="Author Name <email@address.com>"
email@address.com
" for your actual information) to set the authorship information. - If you made more than one commit and the commit with the missing authorship information is not the most recent one you have two options:
- You can re-create all commits missing authorship information. This is going to be the easiest solution for developers that aren't extremely confident in their Git and command line skills.
- You can use this script that GitHub provides to rewrite history. Please note: this should be used only if you are very confident in your abilities and understand its impacts.
- Whichever method you choose, I will come by to re-check the pull request once you push the fixes to this branch.
- If you only made a single commit you should be able to run
We apologize for this inconvenience, especially since it usually bites new contributors to Home Assistant. We hope you understand the need for us to protect ourselves and the great community we all have built legally. The best thing to come out of this is that you only need to fix this once and it benefits the entire Home Assistant and GitHub community.
Thanks, I look forward to checking this PR again soon! ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @Thomas55555,
When attempting to inspect the commits of your pull request for CLA signature status among all authors we encountered commit(s) which were not linked to a GitHub account, thus not allowing us to determine their status(es).
The commits that are missing a linked GitHub account are the following:
-
6b597ac2607dc679b7950f29377cc9fc631bf27d
- This commit has something that looks like an email address (thomas.protzner@gmai.com). Maybe try linking that to GitHub?. -
2d59d507e5025d964459b2105d06cd4f60fe75ec
- This commit has something that looks like an email address (thomas.protzner@gmai.com). Maybe try linking that to GitHub?. -
e0fda6ef79613e4d5a9518cb747d8144d29a5317
- This commit has something that looks like an email address (thomas.protzner@gmai.com). Maybe try linking that to GitHub?.
Unfortunately, we are unable to accept this pull request until this situation is corrected.
Here are your options:
-
If you had an email address set for the commit that simply wasn't linked to your GitHub account you can link that email now and it will retroactively apply to your commits. The simplest way to do this is to click the link to one of the above commits and look for a blue question mark in a blue circle in the top left. Hovering over that bubble will show you what email address you used. Clicking on that button will take you to your email address settings on GitHub. Just add the email address on that page and you're all set. GitHub has more information about this option in their help center.
-
If you didn't use an email address at all, it was an invalid email, or it's one you can't link to your GitHub, you will need to change the authorship information of the commit and your global Git settings so this doesn't happen again going forward. GitHub provides some great instructions on how to change your authorship information in their help center.
- If you only made a single commit you should be able to run
(substituting "Author Name" and "
git commit --amend --author="Author Name <email@address.com>"
email@address.com
" for your actual information) to set the authorship information. - If you made more than one commit and the commit with the missing authorship information is not the most recent one you have two options:
- You can re-create all commits missing authorship information. This is going to be the easiest solution for developers that aren't extremely confident in their Git and command line skills.
- You can use this script that GitHub provides to rewrite history. Please note: this should be used only if you are very confident in your abilities and understand its impacts.
- Whichever method you choose, I will come by to re-check the pull request once you push the fixes to this branch.
- If you only made a single commit you should be able to run
We apologize for this inconvenience, especially since it usually bites new contributors to Home Assistant. We hope you understand the need for us to protect ourselves and the great community we all have built legally. The best thing to come out of this is that you only need to fix this once and it benefits the entire Home Assistant and GitHub community.
Thanks, I look forward to checking this PR again soon! ❤️
Breaking change
Proposed change
Add Husqvarna Automower integration. This was previously a custom integration. As there is the
lawn_mower
entity now, I thought it's a good moment to integrate it in Home Assistant.Replaces messed up #99776
Type of change
Additional information
Checklist
black --fast homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.To help with the load of incoming pull requests: