feat: add Q10 zone, position, and goto support - #908
Conversation
09d3295 to
0ada4c3
Compare
allenporter
left a comment
There was a problem hiding this comment.
Thank you for this great contribution, see my comments below.
|
@allenporter Thanks for the detailed review. I used Codex to help build this, so I want to be transparent about that. I do understand the functional side of the implementation (it actually developed this based on my questions regarding robot positioning and if we could use the zoned cleaning as alternative for the Codex:
Regarding the push updates: on my ss07 running firmware 03.11.24, the map/trace and status updates were streamed reliably during the live goto tests. The robot reached the target and was paused based solely on those updates; we did not use any additional polling or periodic refresh() calls. I was actually quite happy we got this approach to work 😃 There is also a downstream Home Assistant PR using this API: home-assistant/core#177780. You may want to review that as well, particularly to make sure the public API introduced here fits the way Home Assistant consumes it. If the above matches what you have in mind, I’ll have Codex work through the individual comments and update the PR accordingly. Also, if you prefer to take over and implement it yourself, I'd be ok with that as well. As said, I am not a dev, but a Q10 (ss07) owner happy to have my tokens do some useful work so don't hold back on commenting. I'm actually having quite some fun guiding Codex doing this. |
|
I'm fine with you continuing. there is a merge conflict. |
01d1fc5 to
34e3a43
Compare
|
@allenporter This is rebased onto current main and I've worked through and resolved all five review threads. The point API is aligned with #934, and the final safety pass adds explicit confirmed zone ownership before goto may pause. 89 focused tests and all pre-commit hooks pass. GitHub won't let me re-request your review on this fork; the workflow is also waiting for maintainer approval. Ready for another look when you have time. |
allenporter
left a comment
There was a problem hiding this comment.
Thank you for thoroughly working through and addressing all the previous review feedback!
We ran an experimental agentic code review pass on the updated diff:
- The separation of
GotoActioninto a pure state machine cleanly isolates lifecycle decisions from command execution and task management. - Adopting
Q10RoborockPointin common millimetre coordinates provides a consistent, clean consumer-facing API across device families. - Struct packing and zone payload encoding are properly isolated in
b01_q10_protocol.py. - Teardown and cancellation in
VacuumTrait.close()are cleanly hooked intoQ10PropertiesApi.close(). - Full test suite and CI matrix are completely green.
I'll do another human review pass shortly to give it a final look.
(Note: We are experimenting with agentic code review workflows on this repository to help maintainers review and process PRs faster).
Summary
robot_positionin the common Roborock millimetre coordinate spaceGotoActionWhy
Q10 firmware has no V1
app_goto_targetcommand. Goto therefore starts a small zone, pauses within 20 cm of the target, and stops after five minutes only if it still owns that cleaning session. It uses streamed map and status updates without extra polling.The public API uses
Q10RoborockPoint; Q10 trace and vector coordinates remain implementation details. The point model matches the one prepared in #934 so that its follow-up work can build on this PR.Hardware validation
Validated on
roborock.vacuum.ss07firmware 03.11.24. Target(29900, 28650)paused at(30020, 28705), 13.2 cm away, and the robot remained at the destination.Tests
Downstream