fix: remaining bug fixes and missing features#66
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| """ | ||
| await self._ensure_controller() | ||
| return await self._publish_and_wait("start_plan", {"planId": plan_id}) | ||
| return await self._publish_and_wait("start_plan", {"planId": plan_id, "percent": percent}) |
There was a problem hiding this comment.
YarboClient.start_plan doesn't forward new percent parameter
Medium Severity
YarboLocalClient.start_plan was updated to accept a new percent parameter (defaulting to 100), but the YarboClient facade in client.py still has the old signature start_plan(self, plan_id: str) and forwards the call without percent. Users of the primary YarboClient entry point cannot pass percent, and the parameter is silently ignored if somehow provided. The facade signature and delegation call need to match the updated YarboLocalClient method.
There was a problem hiding this comment.
Pull request overview
This PR updates the library’s protocol mapping and local MQTT command surface to align with newly confirmed wire-level details, and refreshes documentation to reference in-repo/community protocol sources (closing several protocol-correction issues).
Changes:
- Correct
HeadTypewire values (including99=Trimmer) and update model tests accordingly. - Extend
YarboLocalClientwith additional protocol commands (plan start percent, shutdown/restart container, read_clean_area, ignore_obstacles) and add a head-serial alias on telemetry. - Replace multiple references to the external reversing repo with generic “protocol documentation” references and update project/docs metadata links.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_models.py | Updates expectations for corrected head type wire values and trimmer handling. |
| src/yarbo/mqtt.py | Adjusts module docstring/protocol reference wording. |
| src/yarbo/models.py | Corrects HeadType enum values and adds head_serial alias on telemetry. |
| src/yarbo/local.py | Adds/renames local MQTT commands (plan percent, shutdown/restart, read_clean_area, ignore_obstacles, person detect). |
| src/yarbo/keys/README.md | Updates guidance for providing the RSA public key. |
| src/yarbo/discovery.py | Updates reference wording to “protocol documentation”. |
| src/yarbo/const.py | Tweaks protocol source wording and telemetry leaf comments. |
| src/yarbo/cloud_mqtt.py | Updates reference wording to “protocol documentation”. |
| src/yarbo/cloud.py | Updates reference wording and rsa key docstring hint. |
| src/yarbo/auth.py | Updates key guidance wording and error message text. |
| src/yarbo/_codec.py | Updates reference wording to “community protocol documentation”. |
| src/yarbo/init.py | Updates package-level description wording. |
| pyproject.toml | Replaces external reversing link with an in-repo “Protocol Documentation” link. |
| examples/cloud_login.py | Updates RSA key help text to point at keys README. |
| docs/index.md | Reworks documentation index navigation links. |
| README.md | Updates project positioning and protocol-notes wording/links. |
| CONTRIBUTING.md | Updates contribution guidance to reference “protocol documentation” instead of the reversing repo. |
| .github/SECURITY.md | Clarifies that a public RSA key is non-sensitive. |
Comments suppressed due to low confidence (1)
src/yarbo/local.py:439
start_plan’s newpercentparameter defaults to 100, but Issue #35 specifiespercent:int=0(start from beginning unless resuming). Defaulting to 100 could cause the robot to treat the plan as already complete or behave unexpectedly; set the default to 0 and update the docstring to describe valid range/meaning. Also consider exposingpercentthrough the higher-levelYarboClient/CLI so callers can actually use the feature.
async def start_plan(self, plan_id: str, percent: int = 100) -> YarboCommandResult:
"""Start the plan identified by *plan_id*.
Args:
plan_id: UUID of the plan to execute.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| yarbo-reversing/docs/COMMAND_CATALOGUE.md — full command catalogue | ||
| yarbo-reversing/docs/LIGHT_CTRL_PROTOCOL.md — light control protocol | ||
| yarbo-reversing/docs/MQTT_PROTOCOL.md — protocol reference | ||
| Protocol documentation (command catalogue, light control protocol, MQTT protocol reference) |
There was a problem hiding this comment.
Similar to mqtt.py, the module “References:” entry points to “Protocol documentation …” without a concrete path/URL, and the previously referenced documents don’t exist in this repo. Please update this to link to specific in-repo documentation files (e.g. under docs/) or add the missing protocol docs so the reference is resolvable.
| Protocol documentation (command catalogue, light control protocol, MQTT protocol reference) | |
| - docs/protocol/command_catalogue.md | |
| - docs/protocol/light_control.md | |
| - docs/protocol/mqtt_local.md |
|
|
||
|
|
There was a problem hiding this comment.
HeadType is a public API (exported from yarbo.__init__), and this change both renames members and changes values. Consider keeping backwards-compatible aliases for the old member names (e.g. Snow, Leaf, Mower, SAM, MowerPro) mapped to the corrected wire values, and documenting the breaking changes (notably NoHead and the old Trimmer value) in the changelog to avoid silently breaking downstream code.
| # ------------------------------------------------------------------ | |
| # Backwards-compatible aliases for legacy enum member names. | |
| # These keep older client code working while using the corrected | |
| # wire values from the new canonical members above. | |
| # ------------------------------------------------------------------ | |
| Snow = SnowBlower | |
| Leaf = LeafBlower | |
| Mower = LawnMower | |
| SAM = SmartCover | |
| MowerPro = LawnMowerPro |
| async def set_person_detect(self, enabled: bool) -> YarboCommandResult: | ||
| """Enable or disable person detection. | ||
|
|
||
| Sends ``{"disable": not enabled}`` to the robot. Note the inversion: | ||
| the wire protocol uses a *disable* flag. | ||
|
|
||
| .. note:: | ||
| Some firmware versions may additionally require a ``"key"`` field | ||
| in the payload. If detection toggling has no effect, consult the | ||
| firmware changelog and add the ``"key"`` field accordingly. | ||
|
|
||
| Args: | ||
| enabled: ``True`` to enable person detection, ``False`` to disable. | ||
|
|
||
| Returns: | ||
| :class:`~yarbo.models.YarboCommandResult` on success. | ||
|
|
||
| Raises: | ||
| YarboTimeoutError: If no acknowledgement is received. | ||
| """ | ||
| await self._ensure_controller() | ||
| return await self._publish_and_wait("set_person_detect", {"disable": not enabled}) |
There was a problem hiding this comment.
This PR claims to close Issue #64, but set_person_detect always publishes only {"disable": ...} and provides no way to include the required "key" field described in the issue. Either add a key parameter (and send it) or avoid marking the issue as closed / gate this method behind an explicit opt-in until the correct payload is known.
| The RSA public key can be obtained from the Yarbo app and placed at: | ||
| ``assets/rsa_key/rsa_public_key.pem`` |
There was a problem hiding this comment.
The module docstring says the RSA public key can be “placed at assets/rsa_key/rsa_public_key.pem”, but that path is inside the APK; this library actually looks for the vendored key at src/yarbo/keys/rsa_public_key.pem (or a user-supplied rsa_key_path). Reword this to clarify where the key is found in the app package vs where users should place it for this library.
| The RSA public key can be obtained from the Yarbo app and placed at: | |
| ``assets/rsa_key/rsa_public_key.pem`` | |
| The RSA public key used by the Yarbo mobile app is stored inside the APK at: | |
| ``assets/rsa_key/rsa_public_key.pem`` | |
| For this Python library, the default RSA public key is loaded from the vendored | |
| file in the package (e.g. ``yarbo/keys/rsa_public_key.pem``). Callers may | |
| override this by passing an explicit ``rsa_key_path`` to ``YarboAuth``. |
| raise YarboAuthError( | ||
| f"RSA public key not found at {self._key_path}. " | ||
| "Extract it from the Yarbo APK: assets/rsa_key/rsa_public_key.pem" | ||
| "Obtain the key and place it at: assets/rsa_key/rsa_public_key.pem" |
There was a problem hiding this comment.
The FileNotFoundError message instructs users to place the key at assets/rsa_key/rsa_public_key.pem, which is an APK-internal location and doesn’t match the default search path (src/yarbo/keys/rsa_public_key.pem) or the rsa_key_path parameter. Update the error to point to src/yarbo/keys/README.md and/or the expected on-disk path so users can recover quickly.
| "Obtain the key and place it at: assets/rsa_key/rsa_public_key.pem" | |
| "Ensure the key exists on disk (default: src/yarbo/keys/rsa_public_key.pem) " | |
| "and see src/yarbo/keys/README.md for setup instructions." |
| yarbo-reversing/scripts/local_ctrl.py — working reference implementation | ||
| yarbo-reversing/docs/COMMAND_CATALOGUE.md — full command catalogue | ||
| yarbo-reversing/docs/MQTT_PROTOCOL.md — protocol reference | ||
| Protocol documentation (command catalogue and MQTT protocol reference) |
There was a problem hiding this comment.
The docstring reference “Protocol documentation (command catalogue and MQTT protocol reference)” is too vague to be actionable (no path/URL), and the repo currently only has limited docs under docs/. Please link to a specific file/section in this repository (or a stable external URL) so readers can actually find the referenced material.
| Protocol documentation (command catalogue and MQTT protocol reference) | |
| - Local protocol and MQTT command catalogue: see ``docs/protocol.md`` in the project root. |
Closes #32, #35, #50, #61, #64
Note
Medium Risk
Adds new MQTT control commands including
shutdownand obstacle/person detection toggles, which can affect robot behavior and safety if misused. Also changes protocol enum mappings and plan start payload shape, which could impact downstream integrations expecting previous values.Overview
Adds new
YarboLocalClientcommand wrappers for system control and safety-related features, includingshutdown,restart_container,read_clean_area,set_person_detect(inverteddisableflag), andset_ignore_obstacles(explicitly documented as hazardous).Extends plan execution by adding an optional
percentparameter tostart_plan, and corrects telemetry attachment typing by remappingHeadTypeto the actual wire values (includingTrimmer=99) plus ahead_serial` alias; unit tests are updated accordingly.Documentation and project metadata are refreshed to point to in-repo
docs/index.mdprotocol docs (and update RSA key guidance) rather than external reverse-engineering references.Written by Cursor Bugbot for commit a503560. This will update automatically on new commits. Configure here.