Add scene support to roborock #137203
Add scene support to roborock #137203
Conversation
|
Hey there @Lash-L, @allenporter, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
@Lash-L this is still pending the release of the feature, and not yet tested, but I would appreciate your initial review |
|
I don't think buttons are the right way to surface scenes. Are scene entities a fit for this? |
No, because ha scenes are for reproducing states of several entities. Here the name is misleading, the app calls it routine or program and it just fires a custom predefined program of the vacuum |
Allen is a core maintainer and I'm going to defer to him for any entity recommendations or final decisions in that regard. I can do a look through of all roborock logic after an entity is cemented. The dependency bump will have to split into a separate PR |
Sticking to the first part: I don't think buttons are the right way to surface this. |
regevbr
changed the title
|
Changed to scene. |
|
upgrade pr - #137244 |
|
Validated locally that it works :-) |
|
@Lash-L ready for review |
|
Added tests to fully cover new logic and created PR to update the docs |
| _LOGGER = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class RoborockRestApi: |
There was a problem hiding this comment.
I don't think this class is needed. It looks like the API for RoborockApiClient is incorrect and it should take these as constructor arguments like the other clients do.
There was a problem hiding this comment.
Can you please further explain what you mean and what you need fixing?
I guess you are talking about the fact that a function requires to get the user data as param instead of having it in the constructor?
Even if that was the case, we would still want to wrap the functionality in our own class.
There was a problem hiding this comment.
Can you please further explain what you mean and what you need fixing?
I'm saying make it look how the other APIs look, rather than a new style.
I'm saying (1) remove this class (2) update the class in the upstream library to take these constructors arguments.
I guess you are talking about the fact that a function requires to get the user data as param instead of having it in the constructor?
That's part of it.
Even if that was the case, we would still want to wrap the functionality in our own class.
Why? The other APIs don't have this and call the api directly. I don't see why this is special.
There was a problem hiding this comment.
update the class in the upstream library to take these constructors arguments
The RoborockApiClient should not take these arguments. As the RoborockApiClient is used for finding out what devices exist in the first place, whereas these parameters imply that you already know what devices you have.
Perhaps the coordinator could just have the api included like is currently done and these functions can live on the coordinator? Not sure - but I also don't think a RoborockRestAPI class needs to exist.
Perhaps the api client could be modified to store the user data once already called? But you would still need to specify the device duid - which would be present in the coordinator already.
The RoborockApiClient (could maybe be renamed to be RoborockWebClient to make things clearer?) is one client for all devices, whereas our other clients are one client for one device
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
| ), | ||
| ) | ||
| for coordinator, scenes in zip( | ||
| config_entry.runtime_data.v1, scene_lists, strict=False |
There was a problem hiding this comment.
My understanding is that strict=False implies its ok for the lists to not have the same length. That should not happen. You added this which wasn't in the example i had -- can you remove or help me understand why this could possibly be OK? It would just introduce bugs if it were true, no?
There was a problem hiding this comment.
This was added by automatically by Ruff and I failed to notice it. I chnaged it to True
Yes Im aware, that is why I do a rebase so commit history is kept and should not affect the "changes since last review" methodology. In any case I need to rebase again now cause there are conflicts with recent changes to his integration that were already emerged. How do you want me to proceed with it? |
|
Personally @regevbr - I just hit the resolve conflicts button in github or 'update branch' button in github. |
That is what i did |
resolved |
|
@regevbr we decided your original proposal to use buttons was correct. However, given this was late in the release cycle we needed to revert so we can target 2025.4 with this. Are you open to sending another PR to add back to button? If you're annoyed at me understandable :) and i can do it but also wanted your name to be on the feature. |
Proposed change
Add ability to start scenes as added in Python-roborock/python-roborock#317
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: