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 a service to get maps for Roborock #111478

Merged
merged 6 commits into from
Apr 8, 2024

Conversation

Lash-L
Copy link
Contributor

@Lash-L Lash-L commented Feb 26, 2024

Breaking change

Proposed change

Right now to get room information for Roborock vacuums, there are a lot of steps and it is hard for most users to follow. This streamlines the process.

This allows you to get the maps of your device - including the map id, map name, and the corresponding rooms that are within that map.

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

To help with the load of incoming pull requests:

@home-assistant
Copy link

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

Code owner commands

Code owners of roborock 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 roborock Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

@standardtoaster
Copy link

I’m looking at a similar feature for iRobot vacs, and it also looks like starting a clean for a room is non standard and kind of clunky, but supported by most popular vacs. Did you consider expanding the base vacuum component?

I’m looking at getting started on something a little generic by adding a sensor for the rooms, and then a base method / service called like clean_rooms or something.

@Olen
Copy link
Contributor

Olen commented Mar 26, 2024

If you plan on making this more generic, there really should be a service clean_room, or maybe start should be extended to have an optional segment_id parameter as well.

@gjohansson-ST
Copy link
Member

I'm a bit hesitant about the service name as in the PR description you only refer to rooms.
Should it instead be get_rooms and possibly only return the rooms dict to simplify the returned data?

@Lash-L
Copy link
Contributor Author

Lash-L commented Mar 31, 2024

I'm a bit hesitant about the service name as in the PR description you only refer to rooms. Should it instead be get_rooms and possibly only return the rooms dict to simplify the returned data?

What about get_map_information? Or something similar? I think the other data is helpful to be involved and good to relate map id with the corresponding rooms. Willing to changes to just get rooms though if you think that would be best.

If you plan on making this more generic, there really should be a service clean_room, or maybe start should be extended to have an optional segment_id parameter as well.

I’m looking at a similar feature for iRobot vacs, and it also looks like starting a clean for a room is non standard and kind of clunky, but supported by most popular vacs. Did you consider expanding the base vacuum component?

I’m looking at getting started on something a little generic by adding a sensor for the rooms, and then a base method / service called like clean_rooms or something.

home-assistant/architecture#913

home-assistant/architecture#950

Both of those relate to more official solutions, but arch changes are a bit more slow moving and this is semi-specialized. It could probably be replaced with real builtin functionality eventually, but I have had users ask for this for months, so I would rather add this and then depreciate it years down the line if real support was added that could replace this.

@standardtoaster
Copy link

Thanks for the context! Blocking this change was not at all my intent.

I think there is more middle ground than those changes above that isn't a big architectural change but allows easier access to metadata from these units - bit I think it's something that those features would build upon.

I'm trying to get a prototype up that I'm somewhat happy with for people to pick apart, happy to CC you.

@Lash-L
Copy link
Contributor Author

Lash-L commented Mar 31, 2024

Thanks for the context! Blocking this change was not at all my intent.

You're all good - I did not think that is what you were doing! I just wanted to give more context.

I think there is more middle ground than those changes above that isn't a big architectural change but allows easier access to metadata from these units - bit I think it's something that those features would build upon.

I'm trying to get a prototype up that I'm somewhat happy with for people to pick apart, happy to CC you.

Please do! I know Eden was also planning on doing some big work on vacuums soon: #112431 (comment) He is a core dev and more easily able to align to the core team's mission. Perhaps a channel should be made on discord for all interested parties. (But also I'm not sending this to deter you from doing anything, just giving some extra info!

@gjohansson-ST
Copy link
Member

I'm a bit hesitant about the service name as in the PR description you only refer to rooms. Should it instead be get_rooms and possibly only return the rooms dict to simplify the returned data?

What about get_map_information? Or something similar? I think the other data is helpful to be involved and good to relate map id with the corresponding rooms. Willing to changes to just get rooms though if you think that would be best.

I think it's probably fine with get_maps in that case so then please update the PR title and description to better reflect this as we are not getting only rooms.

@Lash-L Lash-L changed the title Add a service to get rooms for Roborock Add a service to get maps for Roborock Apr 6, 2024
Copy link
Member

@gjohansson-ST gjohansson-ST left a comment

Choose a reason for hiding this comment

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

I think this looks good and I also gave it a test run with my own pet. Much easier than before to get the room info.

Some specific questions but the code looks good

@home-assistant home-assistant bot marked this pull request as draft April 6, 2024 21:33
@home-assistant
Copy link

home-assistant bot commented Apr 6, 2024

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.

@Lash-L Lash-L marked this pull request as ready for review April 6, 2024 22:54
Copy link
Member

@gjohansson-ST gjohansson-ST left a comment

Choose a reason for hiding this comment

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

Thanks @Lash-L 👍

@gjohansson-ST gjohansson-ST merged commit 5ef4207 into home-assistant:dev Apr 8, 2024
24 checks passed
@Lash-L
Copy link
Contributor Author

Lash-L commented Apr 8, 2024

Thanks @gjohansson-ST !

@github-actions github-actions bot locked and limited conversation to collaborators Apr 9, 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.

Roborock Integration not getting room names Unable to get room map numbers for Roborock S6
4 participants