Skip to content

fix(dimsim): wire observe to /color_image without republishing - #3574

Closed
juhasch wants to merge 1 commit into
dimensionalOS:mainfrom
juhasch:fix/dimsim-observe
Closed

fix(dimsim): wire observe to /color_image without republishing#3574
juhasch wants to merge 1 commit into
dimensionalOS:mainfrom
juhasch:fix/dimsim-observe

Conversation

@juhasch

@juhasch juhasch commented Aug 20, 2026

Copy link
Copy Markdown

Summary

  • DimSim already publishes JPEG camera frames on LCM /color_image, but DimSimConnection.video_stream() was an empty subject, so observe() always returned None and the agent thought it was blind.
  • Forward those bus frames into the Go2 video cache, and skip republishing onto /color_image (video_on_bus) so we do not create an echo loop.

Test plan

  • uv run pytest dimos/robot/unitree/test_dimsim_connection.py dimos/robot/unitree/go2/test_connection.py
  • dimos --simulation dimsim --dimsim-scene=apartment run unitree-go2-agentic
  • After the renderer is up, ask the agent what it sees and confirm observe returns a camera frame instead of None

DimSim already publishes camera frames on LCM; the empty video_stream left observe() returning None. Forward those frames into the Go2 cache and skip republish to avoid an echo loop.

Co-authored-by: Cursor <cursoragent@cursor.com>
@github-actions github-actions Bot added the first-time-contributor PR opened by an author who had not previously committed to this repository label Aug 20, 2026
@greptile-apps

greptile-apps Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This change connects DimSim /color_image frames to the Go2 camera stream so observe() can return simulation frames without echoing them back onto the bus.

The frame-forwarding behavior, normal teardown, restart behavior, cached observation, and conditional publication were exercised successfully. One lifecycle problem remains: calling start() twice leaves an earlier camera subscription active after stop(), so a stopped connection can still receive camera frames.

T-Rex validation blocked

The focused Go2 connection test could not be collected because the unitree_webrtc_connect package is unavailable. Installing its normal dependency chain is blocked by the missing host PortAudio library.

Confidence Score: 4/5

Not safe to merge until repeated startup no longer leaves a camera subscription active after shutdown.

A focused executable harness reproduced the post-stop callback and exercised the surrounding forwarding, cleanup, cache, restart, and publication paths. The repository's focused DimSim tests also passed, while the Go2 test module could not be collected because its native audio dependency is unavailable.

Files Needing Attention: dimos/robot/unitree/dimsim_connection.py needs idempotent startup or cleanup of existing subscriptions before replacement. Add regression coverage in dimos/robot/unitree/test_dimsim_connection.py for repeated startup followed by shutdown.

T-Rex T-Rex Logs

What T-Rex did

  • Ran the focused observe-path harness and PyTest to produce a proof for the posted P1 finding and compared the parent baseline output to the changed harness output.
  • Attempted to run the Go2-focused pytest module; collection failed due to unitree_webrtc_connect being unavailable and PortAudio missing, so execution could not proceed in this environment.
  • Ran the harness with changed code and confirmed DOUBLE_START_POST_STOP_CALLBACKS=1, showing a color_image callback delivers a frame after stop following two starts and disproving several related failure modes.

View all artifacts

T-Rex Ran code and verified through T-Rex

Comments Outside Diff (1)

  1. General comment

    P1 Repeated DimSim start leaks a /color_image subscription past stop

    • Bug
      • Calling DimSimConnection.start() twice stores only the second unsubscribe callback. stop() invokes that second callback, but the first /color_image callback remains subscribed and still receives frames after teardown. The executed harness observed DOUBLE_START_POST_STOP_CALLBACKS=1 after start(); start(); stop(); emit(frame).
    • Cause
      • start() unconditionally overwrites _unsubscribe_video (and likewise _unsubscribe_odom) without making startup idempotent or releasing an existing subscription first.
    • Fix
      • Make start() idempotent, or unsubscribe active odom/video callbacks before assigning new ones; add a regression test that calls start(); start(); stop(); and verifies a subsequent /color_image emission produces no callback.

    T-Rex Ran code and verified through T-Rex

Reviews (1): Last reviewed commit: "fix(dimsim): wire observe to /color_imag..." | Re-trigger Greptile

Comment on lines 69 to +70
self._unsubscribe_odom = self._odom_transport.subscribe(self._handle_odom)
self._unsubscribe_video = self._video_transport.subscribe(self._video_subject.on_next)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Repeated startup leaks the camera subscription

A second start() replaces _unsubscribe_video with the newer callback without unsubscribing the first one. stop() therefore removes only the latest subscription, and /color_image frames can still reach the video stream after teardown. Make startup idempotent or dispose existing odom and video subscriptions before assigning replacements, and cover start(); start(); stop(); emit(frame).

Artifacts

Focused executable DimSim observe-path harness source

  • The authored reproducible harness drives parent-versus-PR bus forwarding, lifecycle, cache, conditional-publication, and repeated-start paths, with the takeaway that it directly exercises the reported callback leak.

Parent revision observe-path baseline output

  • The parent-revision run records that no `/color_image` subscription existed and no observe-path frame was received, with the takeaway that the PR introduces the intended forwarding behavior.

Changed DimSim observe-path harness output

  • The PR run proves normal forwarding, stop cleanup, restart behavior, echo prevention, direct publication, and the post-stop callback left by repeated start, with the takeaway that only repeated startup leaks a callback.

Focused DimSim pytest output

  • The repository’s new DimSim focused tests executed with `2 passed`, with the takeaway that its single-start forwarding and normal-stop expectations pass.

View artifacts

T-Rex Ran code and verified through T-Rex

@juhasch

juhasch commented Aug 21, 2026

Copy link
Copy Markdown
Author

PR #3601 has a better, more general fix. Thanks Paul!

@juhasch juhasch closed this Aug 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

first-time-contributor PR opened by an author who had not previously committed to this repository

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant