Implement Ito v1 local teleoperation loop - #4
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33d533c562
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this.pilotInput.rateHz = sessionConfig.pilotInputRateHz; | ||
| this.pilotInput.start(); |
There was a problem hiding this comment.
After a successful acquisition this starts the pilot-input loop, but the app never creates a PilotInputPeer or SplatBatchPeer (repo-wide search only finds those classes in their definitions/tests), so DataChannelPilotInputTransport.canSend() remains false and no Splat Batch channel is attached. In a real pilot session the UI will show an active session, but the robot receives no controls and the visual stream never arrives/stales out.
Useful? React with 👍 / 👎.
| else: | ||
| LOGGER.info("Ito Droid ignoring unsupported message type %s", message_type) |
There was a problem hiding this comment.
For an Ito Droid session the server relays the client's pilotInput negotiation as a webrtc.offer, but this dispatcher only handles session start/end messages and drops every other type. Once the client sends the pilot-input offer, the physical driver just logs it as unsupported and never attaches the data-channel receiver, so the servo control loop cannot receive pilot snapshots.
Useful? React with 👍 / 👎.
| yaw = snapshot.get("headsetYawRadians") | ||
| if not isinstance(yaw, (int, float)): | ||
| raise ValueError("Pilot Input Snapshot requires numeric headsetYawRadians") |
There was a problem hiding this comment.
The Pilot Client and both WebRTC decoders use the protocol field headsetYawRad, but the controller reads headsetYawRadians here. Any decoded client snapshot passed into receive_pilot_input_snapshot() is therefore rejected and _latest_snapshot never updates, leaving the camera-pan command neutral/held despite valid pilot input.
Useful? React with 👍 / 👎.
| if not isinstance(payload.get("controllers"), dict): | ||
| raise ValueError("snapshot requires controllers") |
There was a problem hiding this comment.
The browser snapshot generator serializes controllers as an array of input sources, while this mock WebRTC decoder requires a dict. When the real Pilot Client drives the Mock Robot over the pilotInput data channel, every snapshot with controller state is rejected before logging; the e2e test misses this because it hand-crafts a dict instead of using the client payload shape.
Useful? React with 👍 / 👎.
| future: asyncio.Future[dict[str, Any]] = asyncio.get_running_loop().create_future() | ||
| self._pending_requests[request["messageId"]] = future | ||
| try: | ||
| await driver.websocket.send(pack_envelope(request)) |
There was a problem hiding this comment.
If the driver disconnects after being listed Available but before this send completes, websockets raises ConnectionClosed out of _request_driver_session_start. That bypasses _release_failed_acquisition(), so the pilot never receives a session.acquire.result and the robot stays occupied until endpoint cleanup instead of being immediately released as an acquisition failure.
Useful? React with 👍 / 👎.
| <meta name="viewport" content="width=device-width, initial-scale=1" /> | ||
| <title>Ito Pilot</title> | ||
| <link rel="stylesheet" href="./styles.css" /> | ||
| <script src="https://aframe.io/releases/1.5.0/aframe.min.js"></script> |
There was a problem hiding this comment.
The Docker/nginx pilot client serves the repo's static files, but this page still loads A-Frame from an external CDN. In the trusted/private or offline robot-network deployments described for local and physical testing, that leaves AFRAME undefined before main.js runs, so the Enter VR launch surface and all in-VR UI never initialize.
Useful? React with 👍 / 👎.
| if session and session.driver_connection is None: | ||
| session.driver_connection = state | ||
| state.session_id = session.session_id | ||
| session.note_endpoint_present() |
There was a problem hiding this comment.
When a robot driver's WebSocket drops during an active session, both current drivers clear their local session in run_once cleanup, but this branch merely reattaches the new connection and clears the missing-endpoint timer without sending driver.session.start again. After a driver reconnect, the server believes the session is healthy and suppresses cleanup while relaying live-path messages to a driver that no longer has the session active.
Useful? React with 👍 / 👎.
| serverUrl: this.settings.serverUrl, | ||
| requestTimeoutMs: this.settings.requestTimeoutMs, | ||
| }); | ||
| this.control.addEventListener("sessionended", (event) => this.handleSessionEnded(event.detail)); |
There was a problem hiding this comment.
ItoControlClient emits closed/error, but the app only listens for sessionended here. If the pilot's WebSocket drops during an active session, the VR UI and pilot loop remain in the active state without sending a resume connection.hello or recreating WebRTC peers, so transient network loss strands the session until the user restarts the page or server cleanup fires.
Useful? React with 👍 / 👎.
| ) | ||
| self.sessions[session_id] = session | ||
|
|
||
| start_result = await self._request_driver_session_start(record.connection, robot_id, session_id, session_config) |
There was a problem hiding this comment.
This driver-start request is awaited while still inside the single _acquisition_lock. When one robot's driver is slow or times out, pilots trying to acquire unrelated available robots are blocked behind that wait for the full request timeout even though only the selected robot needed to be reserved.
Useful? React with 👍 / 👎.
| return | ||
|
|
||
| value = start_result["payload"].get("value", {}) | ||
| if value.get("sessionId") != session_id: |
There was a problem hiding this comment.
A third-party driver can send a protocol-valid driver.session.start.result with ok: true but a non-map or null value; then value.get(...) raises, aborting the acquisition coroutine and leaving the pilot without a result. Treating that response as an invalid-session start failure would keep the reservation cleanup path reliable for malformed driver responses.
Useful? React with 👍 / 👎.
1 participant
Summary
Verification
pytest -q→ 41 passed, 1 skipped in the Hermes runtime/tmp/ito-test-venv/bin/python -m pytest -qwith aiortc/PyAV installed → 42 passedcd client && npm test→ 15 passedpodman build -f server/Dockerfile -t ito-server:test .→ passedpodman build -f drivers/mock-robot/Dockerfile -t ito-mock-robot:test .→ passedpodman build -f drivers/ito-droid/Dockerfile -t ito-droid:test .→ passedito-server:test, connected over WebSocket, sentconnection.hello, received successfulconnection.hello.resultStill intentionally open