Skip to content

fix: 3D joint view drifting out of sync during long teleoperation ses… - #73

Open
ZouzouWP wants to merge 1 commit into
huggingface:mainfrom
ZouzouWP:fix/3d-view-sync
Open

fix: 3D joint view drifting out of sync during long teleoperation ses…#73
ZouzouWP wants to merge 1 commit into
huggingface:mainfrom
ZouzouWP:fix/3d-view-sync

Conversation

@ZouzouWP

Copy link
Copy Markdown

The live 3D arm view in the browser would drift further and further behind the real robot the longer a session ran, and it never recovered on its own — restarting the connection only reset it temporarily.

Root cause

Joint-position frames were pushed onto a FIFO queue and broadcast in order. If the browser tab ever fell even slightly behind the ~30fps producer — a GC pause, a slow render, a backgrounded tab — frames piled up faster than they drained. Every future frame then had to wait behind a growing backlog of stale ones, so the lag only ever grew, never shrank.

flowchart LR
    subgraph before["Before: FIFO queue"]
        P1["Producer ~30fps"] --> Q["Queue\n(grows if consumer is slow)"] --> C1["Consumer"]
    end
    subgraph after["After: latest-wins slot"]
        P2["Producer ~30fps"] -->|overwrites| S["Single slot\n(always newest)"] --> C2["Consumer\n(never behind)"]
    end
Loading

Fix

  • Joint updates now go into a single-slot "latest-wins" buffer instead of the FIFO queue: a new frame overwrites whatever hasn't been sent yet, so a slow consumer skips straight to the most recent pose instead of catching up through a backlog.
  • Broadcast poll interval tightened from 20fps to 30fps to match the teleoperation loop's own rate.
  • Added support for a per-arm urdf_view_zero.json (a zero pose captured by the user against the URDF's own rest pose), which takes priority over the previous hardcoded correction — that one assumed a canonical calibration and was off by ~200° for this arm.

Testing

Measured 17.5ms median WebSocket-to-render latency with zero drift over a long session (previously drifted continuously and never recovered).

…sions

The live 3D arm view in the browser would drift further and further behind the real robot the longer a session ran, and it never recovered on its own — restarting the connection only reset it temporarily.

## Root cause

Joint-position frames were pushed onto a FIFO queue and broadcast in order. If the browser tab ever fell even slightly behind the ~30fps producer — a GC pause, a slow render, a backgrounded tab — frames piled up faster than they drained. Every future frame then had to wait behind a growing backlog of stale ones, so the lag only ever grew, never shrank.

```mermaid
flowchart LR
    subgraph before["Before: FIFO queue"]
        P1["Producer ~30fps"] --> Q["Queue\n(grows if consumer is slow)"] --> C1["Consumer"]
    end
    subgraph after["After: latest-wins slot"]
        P2["Producer ~30fps"] -->|overwrites| S["Single slot\n(always newest)"] --> C2["Consumer\n(never behind)"]
    end
```

## Fix

- Joint updates now go into a single-slot "latest-wins" buffer instead of the FIFO queue: a new frame overwrites whatever hasn't been sent yet, so a slow consumer skips straight to the most recent pose instead of catching up through a backlog.
- Broadcast poll interval tightened from 20fps to 30fps to match the teleoperation loop's own rate.
- Added support for a per-arm `urdf_view_zero.json` (a zero pose captured by the user against the URDF's own rest pose), which takes priority over the previous hardcoded correction — that one assumed a canonical calibration and was off by ~200° for this arm.

## Testing

Measured 17.5ms median WebSocket-to-render latency with zero drift over a long session (previously drifted continuously and never recovered).

@nicolas-rabault nicolas-rabault left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for digging into this properly, the root cause writeup is accurate and the latest-wins slot is the right fix. I confirmed the unbounded queue behaviour and reproduced your reasoning.

The two things blocking merge: ruff format fails on teleoperate.py so the quality check is red, and a joint frame left in the slot when the last client disconnects gets replayed to the next tab that connects.

The urdf_view_zero.json part I'd like to discuss separately. A ~200 degree offset is suspiciously close to one half turn of homing error, so before adding an override file I want to know whether recalibrating the follower at the middle of range pose fixes the view on its own. Happy to merge the latency fix on its own in the meantime if you want to split it out.

Comment thread lelab/server.py
Comment on lines +231 to +235
frame = self._latest_joint_frame
if frame is not None:
self._latest_joint_frame = None
if self.active_connections:
loop.run_until_complete(self._send_to_all_connections(frame))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A frame sitting in this slot when the last client disconnects is never drained, and the next client that connects gets it as its first payload.

disconnect drops the count to zero and calls stop_broadcast_thread, which flips is_running and joins. Whatever is in _latest_joint_frame stays there. When a new client connects, start_broadcast_thread restarts the worker and this block ships the old frame straight away, even if teleoperation ended long ago.

Reproduced against your head commit with a mocked WebSocket:

slot after last client left: {'type': 'joint_update', 'joints': {'Rotation': 9.99}, 'timestamp': 2}
new client first payload:    {'type': 'joint_update', 'joints': {'Rotation': 9.99}, 'timestamp': 2}

In practice: stop teleoperation, close the tab, reopen it, and the 3D arm snaps to a pose the real arm is no longer in, with nothing to correct it until teleoperation starts again.

Clearing the slot in start_broadcast_thread fixes it and keeps the invariant local to the thread lifecycle:

self._latest_joint_frame = None
self.is_running = True
self.broadcast_thread = threading.Thread(target=self._broadcast_worker, daemon=True)

Comment thread lelab/server.py
Comment on lines +228 to +230
# Send the freshest joint frame first (latest-wins slot).
# A frame published between the read and the reset below is
# simply picked up on the next iteration.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment says the opposite of what the code does. A frame published between line 231 and line 233 is not picked up on the next iteration, it is destroyed: the read already returned the previous frame and the unconditional = None overwrites the new one.

The consequence is harmless for a latest-wins slot (the next producer frame is 33ms away and supersedes it anyway), so this is only a matter of the comment being wrong, but the window is not narrow: it spans the whole run_until_complete send. A read and clear in one step under a lock removes it, otherwise please just correct the comment to say the frame is dropped.

Comment thread lelab/server.py

# Get data from queue with a short timeout so joint frames
# are polled frequently enough to stay real-time.
data = self.broadcast_queue.get(timeout=0.02)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional. Now that joint frames bypass the queue entirely, this timeout is what caps the joint frame rate: the worker can only ship one frame per poll, so 0.02 makes the ceiling 50Hz and the thread wakes 50 times a second even when nothing is happening.

A threading.Event set in broadcast_joint_data_sync and waited on here would remove both the magic constant and the busy poll, and let the worker react to a frame the moment it lands. Fine to keep the timeout if you would rather not restructure the loop, but 0.02 deserves a word about where it comes from.

Comment thread lelab/teleoperate.py
Comment on lines +52 to +58
"""Per-arm 3D-view zero captured by the user at the URDF sleep pose.

The hardcoded tick corrections above assume a canonical calibration; real
calibrations vary enough that the 3D view can be wildly offset. If
``calibration/urdf_view_zero.json`` exists (mapping motor name to
``{"zero_deg": float, "sign": ±1}``), it takes priority:
URDF angle = sign * (raw_normalized_deg - zero_deg).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things here.

The docstring says "Per-arm", but the path is a single file at the calibration root, shared by every arm and every saved config. Two followers with different calibrations get the same zero pose, and the file silently applies to whichever one is connected. Either key it by config name (next to the calibration JSON it belongs to) or drop "per-arm" from the wording.

Second, the justification. The comment above says the tick constant holds as long as the user pressed ENTER at the middle of range pose during set_half_turn_homings. A 2048 tick homing error is 180.04 degrees, close to the ~200 degrees in the PR body. Worth ruling out a miscalibrated follower before adding a second source of truth for the zero pose. I put the full question in the review summary.

Comment thread lelab/teleoperate.py
Comment on lines +60 to +63
import json
from pathlib import Path

path = Path.home() / ".cache" / "huggingface" / "lerobot" / "calibration" / "urdf_view_zero.json"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CLAUDE.md asks for shared paths to live in lelab/utils/config.py, not to be rebuilt in feature modules. That file already owns CALIBRATION_BASE_PATH_TELEOP and CALIBRATION_BASE_PATH_ROBOTS and every other ~/.cache/huggingface/lerobot path. Please add the constant there and import it:

# lelab/utils/config.py
URDF_VIEW_ZERO_FILE = os.path.expanduser("~/.cache/huggingface/lerobot/calibration/urdf_view_zero.json")

json is already imported at module level in utils/config.py, and the local import json / from pathlib import Path here can go once the path comes from a constant.

Comment thread lelab/teleoperate.py
data = json.load(f)
return {k: v for k, v in data.items() if isinstance(v, dict) and "zero_deg" in v}
except (OSError, ValueError):
return None

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ruff format wants two blank lines between this function and the module level block that follows, and quality.yml runs pre-commit with the ruff-format hook, so this is a failing check rather than a style preference.

$ ruff format --diff lelab/teleoperate.py
@@ -68,6 +68,7 @@
     except (OSError, ValueError):
         return None

+
 # Global variables for teleoperation state
1 file would be reformatted

ruff format lelab/teleoperate.py fixes it. ruff check is already clean.

Comment thread lelab/teleoperate.py
Comment on lines +109 to +114
view_zero = getattr(get_joint_positions_from_robot, "_view_zero_cache", "unset")
if view_zero == "unset":
view_zero = _load_urdf_view_zero()
get_joint_positions_from_robot._view_zero_cache = view_zero
if view_zero:
logger.info("Using user-captured urdf_view_zero.json for the 3D view")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This caches a None result as hard as a real one, so the first call decides for the whole process. There is no endpoint and no script in this PR that writes urdf_view_zero.json, which means the only way to use the feature is to hand write the file, and if the server has already run one teleoperation session the user has to restart lelab entirely for it to be picked up. That is a rough discovery path for a fix aimed at a visibly broken view.

Loading it once per teleoperation session, in handle_start_teleoperation before the worker starts, gives the same one read per session without the process wide stickiness, and it fits how the rest of this module carries per session state in module globals rather than function attributes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants