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 additional sensors and settings to Roborock vacuums #1585

Merged
merged 13 commits into from
Nov 14, 2022

Conversation

starkillerOG
Copy link
Contributor

@starkillerOG starkillerOG commented Nov 9, 2022

Split out from #1543

Add additional sensors and improve the already implemented sensors for the Roborock S7 MaxV.

Split vacuum enums to a separte file to prevent circular imports

Controls added:

  • Mop routing (enum)
  • Mop scrub intensity (enum)
  • Auto dust collection (switch)

@starkillerOG
Copy link
Contributor Author

@rytilahti can you have a look an merge this?
The test failure is just some unrelated error in the server.

Copy link
Owner

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

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

A couple of comments, but all in all this looks good 👍

@starkillerOG
Copy link
Contributor Author

@rytilahti could you test again/merge?
(test failures are unrelated)

@starkillerOG
Copy link
Contributor Author

@rytilahti I think this can be merged right?

@rytilahti
Copy link
Owner

How about the concern regarding removing the entity_category for the time being? We can discuss that later after we have a better view of how that's going to work with the genericmiot

@starkillerOG
Copy link
Contributor Author

I made a PR to your HomeAssistant fork here: rytilahti/home-assistant#4
That has the fixes I made to prevent the
ValueError: Invalid datetime: sensor.rockrobo_vacuum_v1_do_not_disturb_start provides state '22:00:00', which is missing timezone information
And some other errors

@starkillerOG
Copy link
Contributor Author

How about the concern regarding removing the entity_category for the time being? We can discuss that later after we have a better view of how that's going to work with the genericmiot

We can not just set a default since for instance the Battery schould not have a entity category (schould not be diagnostic but a sensor), so we schould somehow properly implement this.

I would rather keep it in this PR, because I now figured out which ones schould have which catogory, otherwise I need to figure it out again later.
In any case something schould be done about it in a future PR, but that can then also change/remove them from the vacuum implementation.

@starkillerOG
Copy link
Contributor Author

@rytilahti could you agree to that?
(If you really insist I will remove the entity category).

@rytilahti
Copy link
Owner

I'm fine with that, thank you @starkillerOG :-) Prior merging this, would you please adjust the PR description to be a bit more descriptive about the changes introduced in this PR for anyone reading the git logs in the future?

@starkillerOG
Copy link
Contributor Author

@rytilahti I updated the PR description, can you merge?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants