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

Add work area switch for Husqvarna Automower #126376

Merged
merged 13 commits into from
Sep 24, 2024

Conversation

Thomas55555
Copy link
Contributor

@Thomas55555 Thomas55555 commented Sep 21, 2024

Breaking change

Proposed change

Add a switch for each work area to enable and disable it.
As this ist the second work area control entity a generic WorkAreaControlEntity is introduced.
Also the deletion of not needed work area entitites is moved from number.py to sensor.py

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)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

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
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format 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.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

Comment on lines 158 to 176
class WorkAreaControlEntity(AutomowerControlEntity):
"""Replies available when the mower is connected."""

def __init__(
self,
mower_id: str,
coordinator: AutomowerDataUpdateCoordinator,
work_area_id: int,
) -> None:
"""Initialize AutomowerEntity."""
super().__init__(mower_id, coordinator)
self.work_area_id = work_area_id

@property
def work_area_attributes(self) -> WorkArea:
"""Get the mower attributes of the current mower."""
if TYPE_CHECKING:
assert self.mower_attributes.work_areas is not None
return self.mower_attributes.work_areas[self.work_area_id]
Copy link
Member

Choose a reason for hiding this comment

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

Also wondering, should we check if self.work_area_id is in self.mower_attributes.work_areas in the available property? so if its not there we render the entity unavailable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, if I understood that right. Do you mean, a case were one of the work areas is deleted by the user in the mower and HA ist not restarted, yet?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, in that case, this would raise an error I think, so should we add a

@property
def available(self) -> bool:
    return super().available and self.work_area_id in self.mower_attributes.work_areas

And maybe we can move the automatic work area creation and deletion to runtime, which is awesome (but can be daunting, so its fine to move to a followup)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have an good example for that?

Copy link
Member

Choose a reason for hiding this comment

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

I use this logic in withings sensor platform (but it has added complexity).

In essence what is needed is:

  1. Add listener to the coordinator in async_setup_entry of the relevant platforms
  2. Keep a list (or a set for easy adding and subtracting of identifiers) of identifiers of (in this case) work areas you have added
  3. In the listener you compare the new data with the old data, and if you find old ones you call the entity registry to delete the entities (airgradient has this in the mutable entities like number) and if you find new ones, you do a call to async_add_entities

tedee also is a nice example, but they have the logic added in the coordinator, which might not fit well

homeassistant/components/husqvarna_automower/entity.py Outdated Show resolved Hide resolved
homeassistant/components/husqvarna_automower/entity.py Outdated Show resolved Hide resolved
Comment on lines 184 to +189
@property
def translation_key(self) -> str:
"""Return the translation key of the work area."""
return self.entity_description.translation_key_fn(self.work_area_id)
return self.entity_description.translation_key_fn(
self.work_area_id, self.entity_description.key
)
Copy link
Member

Choose a reason for hiding this comment

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

Is this dynamic? Or should we set this to _attr_translation_key in the constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's dynamic. There is one work area which is always there. I called it my_lawn. This is translated with My lawn. All other work areas are named by the user and are not translated.

Copy link
Member

Choose a reason for hiding this comment

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

With dynamic I mean, can it change on runtime? Or will it be set for the full runtime of the entity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And for the work areas which are not translated and set by the user, the entity name would change if the users changes the work area name in the mower.

Copy link
Member

Choose a reason for hiding this comment

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

But then we should set translation key in the constructor and the name in the property

Copy link
Contributor Author

@Thomas55555 Thomas55555 Sep 22, 2024

Choose a reason for hiding this comment

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

With dynamic I mean, can it change on runtime? Or will it be set for the full runtime of the entity?

Maybe it would be better, if it only changes on reload. Because if a work area name is changed, the user would see it, but if a work area was added, the user wouldn't see the change. I think that could be confusing?

Copy link
Member

Choose a reason for hiding this comment

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

There are ways you can add those entities on runtime, I think its a good user experience if HA follows the app within reasonable time

homeassistant/components/husqvarna_automower/strings.json Outdated Show resolved Hide resolved
@home-assistant
Copy link

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

Learn more about our pull request process.

@home-assistant home-assistant bot marked this pull request as draft September 22, 2024 14:12
@Thomas55555 Thomas55555 marked this pull request as ready for review September 22, 2024 16:04
@joostlek joostlek merged commit dc77b2d into home-assistant:dev Sep 24, 2024
30 checks passed
@Thomas55555 Thomas55555 deleted the Add-work-area-switch branch September 24, 2024 17:17
@github-actions github-actions bot locked and limited conversation to collaborators Sep 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants