Adjust services supported by litterrobot vacuum #95788

emontnemery · 2023-07-03T16:25:59Z

Proposed change

Add support for vacuum.stop and deprecate the implementation of services vacuum.turn_on and vacuum.turn_off in litterrobot vacuum

Background:

Services vacuum.turn_on and vacuum.turn_off are supported by the deprecated base class VacuumEntity, not by the base class StateVacuumEntity which is implemented by litterrobot.

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

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

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 Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

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

Documentation added/updated for www.home-assistant.io

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.
Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

home-assistant · 2023-07-03T16:26:05Z

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

Code owner commands

Code owners of litterrobot 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 litterrobot Removes the current integration label and assignees on the pull request, add the integration domain after the command.

emontnemery · 2023-07-04T09:35:09Z

homeassistant/components/litterrobot/vacuum.py

+            is_fixable=False,
+            severity=ir.IssueSeverity.WARNING,
+            translation_key="service_deprecation_turn_off",
+        )

    async def async_start(self) -> None:
        """Start a clean cycle."""
        await self.robot.start_cleaning()


What happens if self.robot.set_power_status(False) has been called, will this still work, or do we need to also call self.robot.set_power_status(True) here?

self.robot.set_power_status(True) would need to be called first if the robot is off, otherwise nothing will happen.

OK, I updated the code to call self.robot.set_power_status(True)

homeassistant/components/litterrobot/vacuum.py

emontnemery · 2023-07-04T15:05:46Z

@natekspencer @tkdrob I have no way to test the changes, could you review this, in particular my concern here: #95788 (comment) ?

gjohansson-ST · 2023-07-28T07:57:23Z

@emontnemery conflicts needs to be resolved

emontnemery · 2023-08-16T10:25:57Z

@natekspencer Would it be possible for you to test the changes?

tests/components/litterrobot/test_vacuum.py

homeassistant/components/litterrobot/strings.json

natekspencer · 2023-08-18T16:21:05Z

@natekspencer Would it be possible for you to test the changes?

As far as functionality goes, this works.

I'm debating if this approach makes sense from the end user perspective, however. Prior to this deprecation, the controls matched the capabilities of the Whisker app (i.e. you can start a clean cycle and turn off/on the robot). The Whisker app doesn't have the notion of stopping a cycle, but it can be done by powering off the robot, which is how it is implemented here. If the robot is off, you can't just start a cycle, but have to turn the robot on. However, turning the robot on automatically starts a clean cycle. This is handled more gracefully by always turning on the robot in the start command here, which I personally think is better. So the discrepancy is really in the stop/turn off semantics.

If I'm a user, do I understand that stopping a clean cycle will turn the robot off? Also, if I want to turn the robot off for another reason, can I just issue a stop command? Right now that works, but will that be deprecated in the future such that you can't stop a cycle if it isn't running?

emontnemery · 2023-10-09T08:09:36Z

I'm debating if this approach makes sense from the end user perspective, however. Prior to this deprecation, the controls matched the capabilities of the Whisker app (i.e. you can start a clean cycle and turn off/on the robot). The Whisker app doesn't have the notion of stopping a cycle, but it can be done by powering off the robot, which is how it is implemented here.

As explained in the background for the PR, the problem is that services vacuum.turn_on and vacuum.turn_off are supported by the deprecated base class VacuumEntity, not by the base class StateVacuumEntity which is implemented by litterrobot. StateVacuumEntity instead supports vacuum.start and vacuum.stop.

Integrations need to implement the entity model defined by Home Assistant, this often means wording won't be the same in an official app and Home Assistant.

There's some more background to this change here: https://developers.home-assistant.io/blog/2023/07/10/vacuum-updates

MartinHjelmare

Thanks!

natekspencer · 2023-10-09T14:07:22Z

As explained in the background for the PR, the problem is that services vacuum.turn_on and vacuum.turn_off are supported by the deprecated base class VacuumEntity, not by the base class StateVacuumEntity which is implemented by litterrobot. StateVacuumEntity instead supports vacuum.start and vacuum.stop.

Integrations need to implement the entity model defined by Home Assistant, this often means wording won't be the same in an official app and Home Assistant.

There's some more background to this change here: https://developers.home-assistant.io/blog/2023/07/10/vacuum-updates

@emontnemery, sorry, I think I misrepresented what I was trying to say. I understand the reason for the changes and am not arguing that. What I was trying to comment on specifically was the stop functionally actually turning off the robot versus creating a separate switch entity to control power on/off.

emontnemery · 2023-10-10T14:53:57Z

Oh, I see @natekspencer, I misunderstood then. If you want to change the behavior like that, it's fine to change the behavior in a follow-up PR.

home-assistant bot added cla-signed integration: litterrobot small-pr PRs with less than 30 lines. Quality Scale: No score labels Jul 3, 2023

emontnemery mentioned this pull request Jul 3, 2023

Remove unsupported vacuum service handlers #95787

Merged

20 tasks

emontnemery marked this pull request as draft July 4, 2023 09:01

emontnemery force-pushed the litterrobot_vacuum_tweak branch from f99d8fc to 070e78d Compare July 4, 2023 09:33

emontnemery changed the title ~~Remove unsupported services from litterrobot vacuum~~ Adjust services supported by litterrobot vacuum Jul 4, 2023

emontnemery commented Jul 4, 2023

View reviewed changes

emontnemery marked this pull request as ready for review July 4, 2023 09:35

MartinHjelmare reviewed Jul 4, 2023

View reviewed changes

homeassistant/components/litterrobot/vacuum.py Outdated Show resolved Hide resolved

MartinHjelmare added the deprecation Indicates a breaking change to happen in the future label Jul 4, 2023

emontnemery mentioned this pull request Jul 4, 2023

Revert "Remove unsupported services from tuya vacuum" #95845

Merged

frenck added the smash Indicator this PR is close to finish for merging or closing label Jul 5, 2023

emontnemery closed this Jul 11, 2023

emontnemery reopened this Jul 11, 2023

edenhaus marked this pull request as draft August 4, 2023 07:18

emontnemery added 8 commits August 16, 2023 12:22

Update supported features

dcb8e56

Add support for vacuum.stop

617c8bf

Raise issue when vacuum.turn_on or turn_off is called

60701df

Mark issues as fixable

06e1342

Improve test coverage

37c975c

Fix tests

34dbc1a

Correct issue

986eeb0

Call set_power_status(True) before start_cleaning()

d16601b

emontnemery force-pushed the litterrobot_vacuum_tweak branch from 03bfac8 to d16601b Compare August 16, 2023 10:25

emontnemery marked this pull request as ready for review August 16, 2023 10:28

MartinHjelmare reviewed Aug 16, 2023

View reviewed changes

tests/components/litterrobot/test_vacuum.py Outdated Show resolved Hide resolved

homeassistant/components/litterrobot/strings.json Outdated Show resolved Hide resolved

Address review comments

5892062

MartinHjelmare approved these changes Oct 9, 2023

View reviewed changes

MartinHjelmare merged commit 1f122eb into dev Oct 9, 2023
21 checks passed

MartinHjelmare deleted the litterrobot_vacuum_tweak branch October 9, 2023 08:56

github-actions bot locked and limited conversation to collaborators Oct 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust services supported by litterrobot vacuum #95788

Adjust services supported by litterrobot vacuum #95788

emontnemery commented Jul 3, 2023 •

edited

home-assistant bot commented Jul 3, 2023

emontnemery Jul 4, 2023 •

edited

natekspencer Jul 30, 2023

emontnemery Aug 16, 2023

emontnemery commented Jul 4, 2023

gjohansson-ST commented Jul 28, 2023

emontnemery commented Aug 16, 2023

natekspencer commented Aug 18, 2023 •

edited

emontnemery commented Oct 9, 2023

MartinHjelmare left a comment

natekspencer commented Oct 9, 2023 •

edited

emontnemery commented Oct 10, 2023

Adjust services supported by litterrobot vacuum #95788

Adjust services supported by litterrobot vacuum #95788

Conversation

emontnemery commented Jul 3, 2023 • edited

Proposed change

Background:

Type of change

Additional information

Checklist

home-assistant bot commented Jul 3, 2023

emontnemery Jul 4, 2023 • edited

Choose a reason for hiding this comment

natekspencer Jul 30, 2023

Choose a reason for hiding this comment

emontnemery Aug 16, 2023

Choose a reason for hiding this comment

emontnemery commented Jul 4, 2023

gjohansson-ST commented Jul 28, 2023

emontnemery commented Aug 16, 2023

natekspencer commented Aug 18, 2023 • edited

emontnemery commented Oct 9, 2023

MartinHjelmare left a comment

Choose a reason for hiding this comment

natekspencer commented Oct 9, 2023 • edited

emontnemery commented Oct 10, 2023

emontnemery commented Jul 3, 2023 •

edited

emontnemery Jul 4, 2023 •

edited

natekspencer commented Aug 18, 2023 •

edited

natekspencer commented Oct 9, 2023 •

edited