Add map camera functionality, better config flow, and initial testing coverage

[prairiesnpr]

• Adds support for setting a unique map for each mower. Resolves Map Camera global for all Automowers #409
• Adds support for showing multiple mowers on a single map. Resolves Camera Shows Each Mower Separately #361, resolves Multiple Map Images #364
• Add support for specifying a rotation, for a map to allow for images that aren't oriented North and South. Resolves Only north-south bound maps supported #462, Resolves Lawnmower position on map mirrored along the diagonal between inserted points #441
• Config flow enforces proper data to reduce support questions
• Adds initial unit testing

Coverage Report

Two Mower Map, With Rotation

Single Mower Map, With Rotation

[codecov-commenter]

Codecov Report

❗ No coverage uploaded for pull request base (main@b0cc6e6). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #485   +/-   ##
=======================================
  Coverage        ?   98.93%           
=======================================
  Files           ?       16           
  Lines           ?     1314           
  Branches        ?        0           
=======================================
  Hits            ?     1300           
  Misses          ?       14           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[prairiesnpr]

I should note this also will resize the camera image before passing it back to HA, seems to be a little faster now. In addition, this would restore the ability to configure low energy mode. That option was removed after I had already reworked the configuration. It's working as advertised, so I would vote to leave it in, but if we want to remove it, I can do that.

[Thomas55555]

Okay, that's I huge PR. I try to review it.
But first a question. How do I run the tests? I've never done this before. I've tried this:

(venv) @Thomas55555 ➜ /workspaces/husqvarna_automower/custom_components/husqvarna_automower (pr/prairiesnpr/485) $ pytest
ImportError while loading conftest '/workspaces/husqvarna_automower/custom_components/husqvarna_automower/tests/conftest.py'.
__init__.py:6: in <module>
    from homeassistant.components.application_credentials import DATA_STORAGE
../../../core/homeassistant/components/application_credentials/__init__.py:16: in <module>
    from homeassistant.components import websocket_api
../../../core/homeassistant/components/websocket_api/__init__.py:13: in <module>
    from . import commands, connection, const, decorators, http, messages  # noqa: F401
../../../core/homeassistant/components/websocket_api/commands.py:49: in <module>
    from . import const, decorators, messages
../../../core/homeassistant/components/websocket_api/decorators.py:15: in <module>
    from .connection import ActiveConnection
../../../core/homeassistant/components/websocket_api/connection.py:13: in <module>
    from homeassistant.components.http import current_request
../../../core/homeassistant/components/http/__init__.py:42: in <module>
    from .ban import setup_bans
../../../core/homeassistant/components/http/ban.py:18: in <module>
    from homeassistant.components import persistent_notification
../../../core/homeassistant/components/persistent_notification/__init__.py:206: in <module>
    @websocket_api.websocket_command({vol.Required("type"): "persistent_notification/get"})
E   AttributeError: partially initialized module 'homeassistant.components.websocket_api' has no attribute 'websocket_command' (most likely due to a circular import)
``

[prairiesnpr]

It's absolutely a huge pr, sorry about that, most of its the tests, but I think they'll be worth it. I setup a github action that will run them automatically, if you want to run them manually, I recommend a clean vscode dev container with just this repo, you don't want a core dev container and you can't test directly from windows. From there install the requirements from requirements.txt and then run pytest --asyncio-mode=auto custom_components/husqvarna_automower/tests/ --cov=custom_components.husqvarna_automower --cov-report term-missing -v

[prairiesnpr]

If you decide to merge it, I have the github action setup to upload the coverage results, you would need to register the repo with codecov, codecov.com. It will still pass, just will give a warning that it wasn't able to upload the results.

[Thomas55555]

Hi, I started reviewing and also testing.
First thing was, the zones are here now(still thing it's not right here)... But I gave it a try.
When trying to configure it. There was an error, that at least three coordinates are required. But I entered 3. But didn't work. After entering 4 coordinates, it worked. I could also "save" the zone. But when I assigned it to the mower. I think I have to assign it by clicking on the button of the mower name. It dissapears.

[Thomas55555]

Ah okay. I have to click on "Save zone" first. Didn't got this. I assumed it's already saved, after it's created.

[Thomas55555]

It was even in the documentation, that I have to save it :D
Okay, now I saved a home zone. Assigned it to the mower. If I look on the map, I see the zone, but after clicking on the map again, the map is not available anymore.
And if a don't assign the zone to the mower. I got an okay from the frontend, but in the backend this error:

2023-06-13 15:02:49.347 ERROR (MainThread) [homeassistant] Error doing job: Task exception was never retrieved
Traceback (most recent call last):
  File "/workspaces/husqvarna_automower/core/venv/lib/python3.11/site-packages/homeassistant/helpers/entity_platform.py", line 457, in async_add_entities
    tasks = [
            ^
  File "/workspaces/husqvarna_automower/core/venv/lib/python3.11/site-packages/homeassistant/helpers/entity_platform.py", line 457, in <listcomp>
    tasks = [
            ^
  File "/workspaces/husqvarna_automower/core/config/custom_components/husqvarna_automower/camera.py", line 53, in <genexpr>
    AutomowerCamera(coordinator, idx, entry)
  File "/workspaces/husqvarna_automower/core/config/custom_components/husqvarna_automower/camera.py", line 100, in __init__
    self._overlay_zones()
  File "/workspaces/husqvarna_automower/core/config/custom_components/husqvarna_automower/camera.py", line 159, in _overlay_zones
    if self.mower_id in zone.get(ZONE_MOWERS, []) and zone.get(
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: argument of type 'NoneType' is not iterable

This should be catched in the frontend first.

[prairiesnpr]

Good catch, I'll update the options flow to verify that we ensure a mower was added and that we have a test for it to confirm it's all working.

[Thomas55555]

I should note this also will resize the camera image before passing it back to HA, seems to be a little faster now. In addition, this would restore the ability to configure low energy mode. That option was removed after I had already reworked the configuration. It's working as advertised, so I would vote to leave it in, but if we want to remove it, I can do that.

Yes, it was working. But the problem is, the mower isn't sending en error message when it's in an error mode on the websocket. This only happens when polling. But if we poll only once a day, we have to wait a long time to see, that the mower has an error. So I think it's better, when it's gone again.

[prairiesnpr]

Hi, I started reviewing and also testing. First thing was, the zones are here now(still thing it's not right here)... But I gave it a try. When trying to configure it. There was an error, that at least three coordinates are required. But I entered 3. But didn't work. After entering 4 coordinates, it worked. I could also "save" the zone. But when I assigned it to the mower. I think I have to assign it by clicking on the button of the mower name. It dissapears.

I understand your point on the zone sensor, I would like to keep it for a couple reasons. One, ha isn't going to include polygon zones due to limitations in android/iOS and second, this would sync our codebases, which would allow future PRs to be simpler.

[Thomas55555]

Good catch, I'll update the options flow to verify that we ensure a mower was added and that we have a test for it to confirm it's all working.

And the map is still not working anymore after I clicked on it once, and the zone is gone after that. You should also have a look for that.

[prairiesnpr]

I should note this also will resize the camera image before passing it back to HA, seems to be a little faster now. In addition, this would restore the ability to configure low energy mode. That option was removed after I had already reworked the configuration. It's working as advertised, so I would vote to leave it in, but if we want to remove it, I can do that.

Yes, it was working. But the problem is, the mower isn't sending en error message when it's in an error mode on the websocket. This only happens when polling. But if we poll only once a day, we have to wait a long time to see, that the mower has an error. So I think it's better, when it's gone again.

Should we make it a configurable pull rate, maybe allow a choice in seconds? With a sane min/max.

[Thomas55555]

I also thought about that, or even dynamic polling rates. But I think it's better to keep it as simple as possible for the user and just poll every 300s.

[Thomas55555]

Do you know, why a lot of files are marked as completely changed, but they aren't? e.g. select.py

[prairiesnpr]

I also thought about that, or even dynamic polling rates. But I think it's better to keep it as simple as possible for the user and just poll every 300s.

Cool, I'll remove that logic, it will take just a bit, have to travel for work tomorrow, may not be able to get to it today.

[prairiesnpr]

Do you know, why a lot of files are marked as completely changed, but they aren't? e.g. select.py

I don't understand that, it's clearly not a complete change but git acts like it is.

[prairiesnpr]

Good catch, I'll update the options flow to verify that we ensure a mower was added and that we have a test for it to confirm it's all working.

And the map is still not working anymore after I clicked on it once, and the zone is gone after that. You should also have a look for that.

Can you give me the exact steps to replicate this, just want to make sure I solve it, didn't happen to me when I was testing.

[Thomas55555]

Okay, here are the first changes I suggest. I hope I can extend the review, when I discover something else.
What I really like, are the tests. The make work easier I hope.

[Thomas55555]

Good catch, I'll update the options flow to verify that we ensure a mower was added and that we have a test for it to confirm it's all working.

And the map is still not working anymore after I clicked on it once, and the zone is gone after that. You should also have a look for that.

Can you give me the exact steps to replicate this, just want to make sure I solve it, didn't happen to me when I was testing.

1. Create a zone with 4 coordinates with the name Home
2. Set checkboxes for Show zone on map camera and Haffi (name of my mower)
3. Click submit
4. Click Save Zone
5. Click on Finish
6. The I click on the map and see the map including the zone.
7. I close the map.
8. And after clicking on the map again, online the headline of the map entity appears.

[prairiesnpr]

I think the files showing complete changes is a line ending issue. See https://stackoverflow.com/questions/30752959/git-showing-entire-file-is-modified-instead-of-showing-modified-small-part-of-co

[Thomas55555]

Can this file be deleted:
husqvarna_automower/custom_components/__init__.py ?

[prairiesnpr]

Can this file be deleted: husqvarna_automower/custom_components/__init__.py ?

No, had to add it for test discovery to work.

[prairiesnpr]

@Thomas55555 Take a look at the last commit,this will raise ConfigEntryAuthFailed if data isn't returned from the api. Not certain I like this though, feel like the error should really originate from aioautomower, but this should allow the user to reauth or just reload if it happens again.

[prairiesnpr]

This should address all the comments to this point, we now retain the existing mower selections and pass them to the form.

[Thomas55555]

@Thomas55555 Take a look at the last commit,this will raise ConfigEntryAuthFailed if data isn't returned from the api. Not certain I like this though, feel like the error should really originate from aioautomower, but this should allow the user to reauth or just reload if it happens again.

Please leave it as it was before the PR, because we are not knowing where it's coming from. This may cause unneeded reauths, which we don't want.

[prairiesnpr]

@Thomas55555 Take a look at the last commit,this will raise ConfigEntryAuthFailed if data isn't returned from the api. Not certain I like this though, feel like the error should really originate from aioautomower, but this should allow the user to reauth or just reload if it happens again.

Please leave it as it was before the PR, because we are not knowing where it's coming from. This may cause unneeded reauths, which we don't want.

We could reload the entry? I hate to just leave the integration in a broken state. Another thought might be, that aioautomower should return the last valid data and log the error, if it is a transient issue with the api, it might be resolved the next time we poll the api.

[Thomas55555]

Maybe it was just a coincidence with the invalid credentials issue. And polling again didn't take effect. There was no new data for over 24h.
We can release as beta first, test it and add fixes, if it's really related to that.

[prairiesnpr]

OK, reverted, we no longer raise ConfigEntryAuthFailed if the API returns a 400 error.

[Thomas55555]

Okay, I've tested it now.
It's much better, with saving zones automatically and remembering assignment of the zones to the mower.
Do we still need the button Save Zone now? This could be removed, isn't it?

[prairiesnpr]

Okay, I've tested it now. It's much better, with saving zones automatically and remembering assignment of the zones to the mower. Do we still need the button Save Zone now? This could be removed, isn't it?

We still need something to signal that we are done and end the flow. I think best to keep it, otherwise after we saved a zone we would need to end the flow and start the configuration flow over, could maybe rename it to something else if that helps.

[Thomas55555]

I just clicked on submit and the flow was ended. I think this is sufficient.

[prairiesnpr]

I just clicked on submit and the flow was ended. I think this is sufficient.

Not sure I'm following you here, are we good as is?

[Din-BH]

I just clicked on submit and the flow was ended. I think this is sufficient.

Not sure I'm following you here, are we good as is?

I for one think the extra save zone is just confusing. The zone is already saved. If you need an entry, I suggest you name it "End configuration" and put it last. If you can do without it, even better.

After moving to the latest dev version, trying to update my Mower in config gives me User input malformed: Not a list for dictionary value @ data['additional_mowers']. Also noticed that I can't find a way to add more mowers anywhere - not that I need that, but I guess that was kind of a point to support multiple?

[prairiesnpr]

After moving to the latest dev version, trying to update my Mower in config gives me User input malformed: Not a list for dictionary value @ data['additional_mowers']. Also noticed that I can't find a way to add more mowers anywhere - not that I need that, but I guess that was kind of a point to support multiple?

I had tried to remove it earlier and ended up where submit would repeatedly reload the window if nothing was selected, since there was no user input it thought we were loading it for the first time. Thought that was super confusing, there's probably a way to remove it, it might come to me.

I changed the way we store data, in order to allow us to prefill the multiple choice lists. There was no way to do that with the select helper, had to move to cv.multiple which does support default values. The problem though is this breaks existing config for those of us testing.

Coincidentally, your second question is why you are seeing this, when you set up the camera the last choice is to select additional mowers that should be shown on the same map, since we only have a single mower, the list is empty and not displayed, since we can't show the same mower twice. But since I changed the multiselect, we went from passing the data as a list of dicts to a dict. Think this is the cause of your error, your existing options dict has an empty list, it expects an empty dict. Long story short, delete the integration via the ui then reinstall it and you should be good, that way you will have your options in the correct format.

I didn't bother migrating or catching it, since it applies only to those of us testing, same with the migration, if you had an earlier commit, the migration logic will fail since it expects to migrate from a pre PR config. If that doesn't fix it, please let me know and I'll dig some more. I just opened my dev box which is running the latest commit and had no issues changing the rotation several times.

[Din-BH]

After moving to the latest dev version, trying to update my Mower in config gives me User input malformed: Not a list for dictionary value @ data['additional_mowers']. Also noticed that I can't find a way to add more mowers anywhere - not that I need that, but I guess that was kind of a point to support multiple?
I didn't bother migrating or catching it, since it applies only to those of us testing, same with the migration, if you had an earlier commit, the migration logic will fail since it expects to migrate from a pre PR config. If that doesn't fix it, please let me know and I'll dig some more. I just opened my dev box which is running the latest commit and had no issues changing the rotation several times.

Very good. Just wanted to call it out in case it wasn't this. I thought I might revert to the previous known good when we start to get ready to migrate back in to main, and verify the migration works.

[prairiesnpr]

Very good. Just wanted to call it out in case it wasn't this. I thought I might revert to the previous known good when we start to get ready to migrate back in to main, and verify the migration works.

Great, appreciate all the support in testing. When you write something, you know how it's supposed to work and will think that it's pretty solid until you get other to use it, always amazed at how many things you find that were missed when other people try to use it. Better to discover issue here then when it's an issue in production and I'm getting pinged on a support incident.

Also, side note, I'm very envious of your drone imagery, that looks amazing. Unfortunately, can't justify a drone to make my home assistant look better....

[prairiesnpr]

c60747f addresses comments on the zone menu, still don't have a way to get rid of the option, but this does make it clear that it's a closing step, enables translations for those two options, and ensures that the default option is to finish and close.

[Thomas55555]

Can I merge it?

[prairiesnpr]

Can I merge it?

I'm good with merging, since @Din-BH has been instrumental in helping to test it, let's give him a shot to test the latest changes and if he's good merge it. Hopefully we can get some help with the missing translations, we added a bunch of keys that will need to be translated once we merge.

[Din-BH]

Can I merge it?

I'm good with merging, since @Din-BH has been instrumental in helping to test it, let's give him a shot to test the latest changes and if he's good merge it. Hopefully we can get some help with the missing translations, we added a bunch of keys that will need to be translated once we merge.

Works like a charm. ;) I say go! Great work!