docs: remove all reverse engineering references, clean documentation#65
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 missing new percent parameter passthrough
High Severity
The start_plan method on YarboLocalClient gained a new percent parameter, but the YarboClient wrapper in client.py (line 211) still has the old signature start_plan(self, plan_id: str) and calls self._local.start_plan(plan_id) without forwarding percent. Since YarboClient is the primary entry point documented in the README, users of that class can never pass the percent parameter. The local client will always receive the default 100, which also silently changes the wire payload for all existing callers by adding a "percent": 100 field.
| YarboTimeoutError: If no acknowledgement is received. | ||
| """ | ||
| await self._ensure_controller() | ||
| return await self._publish_and_wait("ignore_obstacles", {"state": 1 if state else 0}) |
There was a problem hiding this comment.
New methods missing from primary YarboClient wrapper
High Severity
Five new public methods — shutdown, restart_container, read_clean_area, set_person_detect, and set_ignore_obstacles — were added to YarboLocalClient but have no corresponding delegation wrappers in YarboClient (client.py). Since YarboClient is the documented primary entry point and every other YarboLocalClient method has a wrapper there, users of YarboClient cannot access any of these new features.
There was a problem hiding this comment.
Pull request overview
This PR primarily updates repository documentation and in-code docstrings to remove reverse-engineering language and replace it with more neutral “community/project/protocol” terminology. It also includes a few functional additions to the local client API.
Changes:
- Replace reverse-engineering references across docs/docstrings with community/protocol documentation language and update external links/URLs.
- Update RSA key guidance in docs/examples and tweak related error/help text.
- Extend
YarboLocalClientwith additional command wrappers and add apercentfield to thestart_planpayload.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/yarbo/mqtt.py | Docstring wording and reference cleanup. |
| src/yarbo/local.py | Docstring cleanup plus functional API changes (plan start payload + new command methods). |
| src/yarbo/keys/README.md | Updated key acquisition wording; removed APK extraction steps. |
| src/yarbo/discovery.py | Reference wording cleanup. |
| src/yarbo/const.py | “Sources” wording cleanup in module docstring/comments. |
| src/yarbo/cloud_mqtt.py | Reference wording cleanup. |
| src/yarbo/cloud.py | Reference wording cleanup and constructor doc tweak. |
| src/yarbo/auth.py | Reference wording cleanup and updated key-related messaging. |
| src/yarbo/_codec.py | Reference wording cleanup. |
| src/yarbo/init.py | Package description wording cleanup. |
| pyproject.toml | Replace URL entry with “Protocol Documentation” link to in-repo docs. |
| examples/cloud_login.py | Update key requirement/help text to point to keys README. |
| docs/index.md | Remove external protocol-doc links; add repo/PSYarbo links. |
| README.md | Rewrite/cleanup RE language; adjust related-project links and protocol docs pointer. |
| CONTRIBUTING.md | Update language around protocol knowledge sources and links. |
| .github/SECURITY.md | Minor wording tweak regarding the public RSA key. |
Comments suppressed due to low confidence (1)
src/yarbo/auth.py:89
_default_key_path's warning/docstring referencessrc/yarbo/keys/..., which is a repo-relative path and won't match an installed package layout. Consider updating the wording to refer to the package-relative path (yarbo/keys/rsa_public_key.pem) and keepsrc/...only as a contributor note if needed.
.. warning::
The vendored key at ``src/yarbo/keys/rsa_public_key.pem`` is a
**placeholder** — cloud auth will fail until it is replaced with
the real key (see keys/README.md for instructions).
See ``src/yarbo/keys/README.md`` for extraction instructions, or
supply the real key path via ``rsa_key_path`` at construction time.
"""
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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" | ||
| ) from exc |
There was a problem hiding this comment.
The FileNotFoundError message says "Obtain the key and place it at: assets/rsa_key/rsa_public_key.pem". assets/... is the location inside the Yarbo app package, not a path the library reads from; this is likely to confuse users. Suggest changing the message to (a) point at the expected PEM path (rsa_key_path or the vendored yarbo/keys/rsa_public_key.pem) and (b) reference keys/README.md for where to obtain it from the app.
| async def shutdown(self) -> YarboCommandResult: | ||
| """Shut down the robot. | ||
|
|
||
| Returns: | ||
| :class:`~yarbo.models.YarboCommandResult` on success. | ||
|
|
||
| Raises: | ||
| YarboTimeoutError: If no acknowledgement is received. | ||
| """ | ||
| await self._ensure_controller() | ||
| return await self._publish_and_wait("shutdown", {}) | ||
|
|
||
| async def restart_container(self) -> YarboCommandResult: | ||
| """Restart the software container on the robot. | ||
|
|
||
| Returns: | ||
| :class:`~yarbo.models.YarboCommandResult` on success. | ||
|
|
||
| Raises: | ||
| YarboTimeoutError: If no acknowledgement is received. | ||
| """ | ||
| await self._ensure_controller() | ||
| return await self._publish_and_wait("restart_container", {}) | ||
|
|
||
| # ------------------------------------------------------------------ | ||
| # Area management | ||
| # ------------------------------------------------------------------ | ||
|
|
||
| async def read_clean_area(self) -> YarboCommandResult: | ||
| """Request the list of saved clean areas from the robot. | ||
|
|
||
| Returns: | ||
| :class:`~yarbo.models.YarboCommandResult` on success. | ||
|
|
||
| Raises: | ||
| YarboTimeoutError: If no acknowledgement is received. | ||
| """ | ||
| await self._ensure_controller() | ||
| return await self._publish_and_wait("read_clean_area", {}) |
There was a problem hiding this comment.
New shutdown, restart_container, and read_clean_area commands are added here but there are no corresponding tests verifying the publish command names/payloads (similar command wrappers in this module are covered in tests/test_local.py). Adding small unit tests would help prevent regressions in these high-impact actions.
| 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}) | ||
|
|
||
| async def set_ignore_obstacles(self, state: bool) -> YarboCommandResult: | ||
| """Enable or disable obstacle detection bypass. | ||
|
|
||
| .. warning:: | ||
| **Safety hazard.** Disabling obstacle detection allows the robot | ||
| to continue moving even when obstacles are detected. Only use this | ||
| in controlled environments where you are certain there are no | ||
| people, animals, or objects in the robot's path. | ||
|
|
||
| Args: | ||
| state: ``True`` to ignore obstacles (bypass detection), | ||
| ``False`` to restore normal obstacle detection. | ||
|
|
||
| Returns: | ||
| :class:`~yarbo.models.YarboCommandResult` on success. | ||
|
|
||
| Raises: | ||
| YarboTimeoutError: If no acknowledgement is received. | ||
| """ | ||
| await self._ensure_controller() | ||
| return await self._publish_and_wait("ignore_obstacles", {"state": 1 if state else 0}) |
There was a problem hiding this comment.
New safety-related methods set_person_detect and set_ignore_obstacles are introduced without test coverage. Please add unit tests that assert the exact payload shape (including the disable inversion and the 1/0 encoding) so behavior stays stable across refactors.
| - [Light control protocol](https://github.com/markus-lassfolk/yarbo-reversing/blob/main/docs/LIGHT_CTRL_PROTOCOL.md) | ||
| - [API endpoints](https://github.com/markus-lassfolk/yarbo-reversing/blob/main/docs/API_ENDPOINTS.md) | ||
| - [MQTT protocol reference](https://github.com/markus-lassfolk/yarbo-reversing/blob/main/docs/MQTT_PROTOCOL.md) | ||
| See [protocol documentation](docs/index.md) for additional protocol details. |
There was a problem hiding this comment.
The link text suggests docs/index.md contains protocol details, but docs/index.md currently only has general navigation/quick start and no protocol-specific references. Either add/restore protocol documentation links/content there, or adjust this sentence so it doesn't imply protocol docs exist at that location.
| See [protocol documentation](docs/index.md) for additional protocol details. | |
| See the [documentation](docs/index.md) for additional details and usage information. |
| Documentation = "https://github.com/markus-lassfolk/python-yarbo#readme" | ||
| Changelog = "https://github.com/markus-lassfolk/python-yarbo/blob/main/CHANGELOG.md" | ||
| "yarbo-reversing" = "https://github.com/markus-lassfolk/yarbo-reversing" | ||
| "Protocol Documentation" = "https://github.com/markus-lassfolk/python-yarbo/blob/main/docs/index.md" |
There was a problem hiding this comment.
[project.urls] adds a "Protocol Documentation" link pointing to docs/index.md, but that page doesn't currently contain protocol documentation (only general docs). Consider either pointing this URL at the actual protocol docs location (if any) or renaming/removing the entry to avoid misleading package metadata.
| "Protocol Documentation" = "https://github.com/markus-lassfolk/python-yarbo/blob/main/docs/index.md" |
| - [PSYarbo PowerShell module](https://github.com/markus-lassfolk/PSYarbo) | ||
| - [CHANGELOG](../CHANGELOG.md) | ||
| - [CONTRIBUTING](../CONTRIBUTING.md) | ||
|
|
There was a problem hiding this comment.
docs/index.md removed the protocol-detail links (command catalogue, light control protocol, API endpoints), but other docs/metadata now refer to this page as "protocol documentation". Either reintroduce protocol-detail navigation here (possibly pointing to in-repo docs) or update those references so users don't lose the protocol entry point.
| ## Protocol documentation | |
| This page is the entry point for the Yarbo protocol documentation. For low-level details about the MQTT and cloud protocols, see: | |
| - [Command catalogue](protocol/command-catalogue.md) | |
| - [Light control protocol](protocol/light-control.md) | |
| - [Cloud API endpoints](protocol/cloud-api.md) |
| 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 docstring says the RSA public key can be "placed at" assets/rsa_key/rsa_public_key.pem, which reads like a destination path. That path is inside the mobile app package; consider rewording to clarify it's the location within the app package (and point to src/yarbo/keys/README.md for how the library expects the PEM to be provided).
| 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 can be obtained from the Yarbo mobile app; within the app | |
| package it is stored at ``assets/rsa_key/rsa_public_key.pem``. See | |
| ``src/yarbo/keys/README.md`` for how this library expects the PEM to be | |
| provided. |
| The actual public key can be obtained from the Yarbo app package and placed at: | ||
|
|
||
| ``` | ||
| assets/rsa_key/rsa_public_key.pem | ||
| ``` | ||
|
|
||
| ### Extract from APK | ||
|
|
||
| ```bash | ||
| # 1. Download the Yarbo APK (from Google Play via apkpure or your device) | ||
| # 2. Unzip it (APKs are ZIP archives) | ||
| unzip yarbo.apk -d yarbo_unpacked | ||
|
|
||
| # 3. Copy the key | ||
| cp yarbo_unpacked/assets/rsa_key/rsa_public_key.pem /path/to/python-yarbo/src/yarbo/keys/ | ||
| src/yarbo/keys/rsa_public_key.pem | ||
| ``` |
There was a problem hiding this comment.
src/yarbo/keys/rsa_public_key.pem is a repo-relative path; for users installing from PyPI there is no src/ directory. Consider either (1) documenting the installed path (.../site-packages/yarbo/keys/rsa_public_key.pem) if you want to support vendoring, or (2) recommending the runtime rsa_key_path approach as the primary method and framing the vendoring path as "when working in the repository".
| @@ -448,7 +445,7 @@ async def start_plan(self, plan_id: str) -> YarboCommandResult: | |||
| YarboTimeoutError: If no acknowledgement is received. | |||
| """ | |||
| 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.
start_plan gained a percent parameter but the docstring still documents only plan_id, and the new parameter isn't surfaced via YarboClient.start_plan(...) (it still takes only plan_id). Either document and plumb percent through the public client/CLI, or drop it here to keep the API consistent.
| # ------------------------------------------------------------------ | ||
| # System control | ||
| # ------------------------------------------------------------------ | ||
|
|
||
| async def shutdown(self) -> YarboCommandResult: | ||
| """Shut down the robot. | ||
|
|
||
| Returns: | ||
| :class:`~yarbo.models.YarboCommandResult` on success. | ||
|
|
||
| Raises: | ||
| YarboTimeoutError: If no acknowledgement is received. | ||
| """ | ||
| await self._ensure_controller() | ||
| return await self._publish_and_wait("shutdown", {}) | ||
|
|
||
| async def restart_container(self) -> YarboCommandResult: | ||
| """Restart the software container on the robot. | ||
|
|
||
| Returns: | ||
| :class:`~yarbo.models.YarboCommandResult` on success. | ||
|
|
||
| Raises: | ||
| YarboTimeoutError: If no acknowledgement is received. | ||
| """ | ||
| await self._ensure_controller() | ||
| return await self._publish_and_wait("restart_container", {}) | ||
|
|
There was a problem hiding this comment.
PR title/description describe documentation-only changes, but this file also changes behavior/API (adds percent to start_plan and introduces multiple new command methods). Consider splitting the functional changes into a separate PR or at least updating the PR description/changelog to reflect the new public API surface.
Summary
Removes all reverse engineering language from the entire repository and replaces with professional, community-project language.
Changes
Verification
Closes #7
Note
Medium Risk
Mostly documentation/link rewrites, but it also changes the local control surface by adding new MQTT commands (including shutdown/restart and safety-related toggles) and extending
start_planpayloads, which could affect robot behavior if used incorrectly.Overview
Rewords project/docs to remove reverse-engineering framing and instead describe a community-developed protocol effort, while updating references to point at in-repo
docs/index.mdand adjusting key guidance (README, SECURITY, CONTRIBUTING, module docstrings,pyproject.tomlURLs, andexamples/cloud_login.py).Extends
YarboLocalClientwith additional MQTT command wrappers:start_plannow includes an optionalpercentfield, and new methods add system/area/safety controls (shutdown,restart_container,read_clean_area,set_person_detect,set_ignore_obstacles).Written by Cursor Bugbot for commit cd9840b. This will update automatically on new commits. Configure here.