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

Allow pausing xiaomi vacuum in all states #20620

Merged
merged 3 commits into from
Feb 6, 2019
Merged

Allow pausing xiaomi vacuum in all states #20620

merged 3 commits into from
Feb 6, 2019

Conversation

pszafer
Copy link
Contributor

@pszafer pszafer commented Jan 31, 2019

I bought Xiaomii vacuum for my parents and found one bug and also reason for one annoying problem.
So two fixes in this PR.

For now user can only pause vacuum while cleaning. I thought that it was some limitation of python miio module, but no. It was component designed that way.

if self.state == STATE_CLEANING:
            await self._try_command(
                "Unable to set start/pause: %s", self._vacuum.pause)

so added STATE_RETURNING to this if.

  1. When new vacuum is connected and no cleaning was performed xiaomi component is crashing.
    Don't know if checking should be added to every attribute update. Waiting for your opinions on it.
    For sure it was crashing on:
self.vacuum_state.clean_time
self.clean_history.count
self.clean_history.total_duration

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

@syssi
Copy link
Member

syssi commented Jan 31, 2019

At least the second issue should be fixed at the underlying library (python-miio): rytilahti/python-miio#457

@pszafer
Copy link
Contributor Author

pszafer commented Jan 31, 2019

ok, so reverted the second issue.

@pszafer
Copy link
Contributor Author

pszafer commented Jan 31, 2019

@MartinHjelmare little out if this PR, but is this possible to get into this if ever? https://github.com/home-assistant/home-assistant/blob/6136ed31f24c921932c705e93be73b0e8935a36d/homeassistant/components/vacuum/xiaomi_miio.py#L301-L303

FAN_SPEEDS are not capitalized and we check capitalized string if it is in dict.

@MartinHjelmare
Copy link
Member

FAN_SPEEDS are capitalized, with first character in capital letter.

@pszafer
Copy link
Contributor Author

pszafer commented Jan 31, 2019

Haha, I lowercased it somehow in my head :)

@rytilahti rytilahti changed the title Fix Xiaomi vacuum Allow pausing xiaomi vacuum in all states Feb 1, 2019
@rytilahti
Copy link
Member

This is intertwined with rytilahti/python-miio#471 -- what happens/what should happen when clicking start again after pausing?

@pszafer
Copy link
Contributor Author

pszafer commented Feb 1, 2019

I don't think is that it is interwined.

About what should happen when clicking start again after pausing I imagine that default behaviour should be resume last state, not clean all.

@rytilahti
Copy link
Member

One problem is that the state gets reverted (most likely) back to idle after some time (see xiaomi_miio.state implementation for an example of this happening with the error handling). Anyway, the backend library should definitely support resuming (unpausing) the state, this will be added as a new command.

The start_pause implementation as it currently is, is flawed. The description for that call is Start, pause or resume a cleaning task. so there needs to be more logic built into it to make it work as expected.

For the time being, could you check what mirobo raw-command get_clean_summary returns so I can shield the accesses to those variables properly?

@pszafer
Copy link
Contributor Author

pszafer commented Feb 4, 2019

Output of command:

Sending cmd get_clean_summary with params []
[340436, 5692697500, 537, [1549201583, 1549190560, 1549107590, 1549100876, 1549053818, 1549052116, 1549051867, 1549018592, 1549011333, 1549010036, 1549009811, 1549008646, 1548928406, 1548920674, 1548919350, 1548919180, 1548917930, 1548917883, 1548917863, 1548917790]]

I think I can resolve conflicts in this PR and it can be merged to just repair first problem:

if self.state == STATE_CLEANING:

@rytilahti what do you think?

@rytilahti
Copy link
Member

At that point the vacuum had cleaned already, so there's no reason for it to crash (like without one, which is likely fixed by my PR in python-miio). Anyway, this change can be merged after the conflict is fixed, there is AFAIK nothing wrong in calling the pause even when the device is already stopped.

@pszafer
Copy link
Contributor Author

pszafer commented Feb 6, 2019

Rebased it. I think it is ready to be merged.

@MartinHjelmare MartinHjelmare merged commit 208f1a4 into home-assistant:dev Feb 6, 2019
@ghost ghost removed the in progress label Feb 6, 2019
bachya pushed a commit to bachya/home-assistant that referenced this pull request Feb 7, 2019
* fix state update when no cleaning is yet performed
allow pause vacuum when returning to base

* revert checking of atttribute updates. Will be fixed in upstream lib.

* remove unnecesarry if on pause_commadn
@balloob balloob mentioned this pull request Feb 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants