Repository navigation
feat: use individual module connections - #2049
Closed
paul-nechifor wants to merge 2 commits into
Closed
paul-nechifor wants to merge 2 commits into
paul-nechifor wants to merge 2 commits into
Conversation
Contributor
Greptile SummaryThis PR refactors the Unitree connection layer from a single monolithic class into per-backend modules (
Confidence Score: 3/5Several real-hardware paths remain broken: camera-info threads never exit so teardown always times out; the WebRTC handshake blocks forever on connection failure; and Go2FleetConnection silently loses fleet behaviour when with_backend() is called. The registry and Blueprint.with_backend() machinery is clean and well-tested. However, three confirmed defects from the prior review remain unaddressed: the infinite camera-info loop preventing clean shutdown, the indefinitely-blocking WebRTC handshake, and the fleet-connection tag-inheritance problem. Together these make the production robot paths unreliable. dimos/robot/unitree/go2/connection_webrtc.py, connection_replay.py, connection_dimsim.py (infinite camera-info loops); dimos/robot/unitree/webrtc_session.py (blocking handshake); dimos/robot/unitree/go2/fleet_connection.py (inherited _connection_tag); dimos/robot/unitree/g1/init.py (incompatible stream surfaces) Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as dimos.py CLI
participant GC as GlobalConfig
participant ABP as all_blueprints
participant CR as connection_registry
participant WB as with_backend()
participant SC as _check_stream_parity
participant KC as _check_kwargs_compat
CLI->>GC: "update(**cli_config_overrides)"
CLI->>ABP: "autoconnect(*robot_types)"
ABP-->>CLI: Blueprint (with tagged atoms)
CLI->>GC: effective_simulator
GC-->>CLI: mujoco / replay / None
alt backend is not None
CLI->>WB: blueprint.with_backend(backend)
loop each atom in blueprint.blueprints
WB->>WB: getattr(atom.module, _connection_tag)
alt "tag.backend != requested"
WB->>CR: get_connection(tag.robot, backend)
CR-->>WB: target class
WB->>SC: _check_stream_parity(old, target, atom)
SC->>SC: compare name+direction sets only
WB->>KC: _check_kwargs_compat(target, atom.kwargs)
KC->>KC: check model_fields vs kwargs keys
WB->>WB: BlueprintAtom.create(target, atom.kwargs)
end
end
WB-->>CLI: new Blueprint
end
CLI->>CLI: blueprint.start()
Reviews (9): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
paul-nechifor
force-pushed
the
paul/feat/simulation-modules
branch
from
May 12, 2026 04:29
bab5f66 to
7647f65
Compare
paul-nechifor
force-pushed
the
paul/feat/simulation-modules
branch
3 times, most recently
from
May 12, 2026 05:30
72080c3 to
f82d270
Compare
paul-nechifor
force-pushed
the
paul/feat/simulation-modules
branch
from
May 19, 2026 05:08
f82d270 to
53f9af1
Compare
leshy
reviewed
May 20, 2026
| # fully populated by the time `Blueprint.with_backend(...)` runs. The Mujoco | ||
| # variant defers its mujoco-engine import to instance __init__, so registering | ||
| # it here is cheap. | ||
| from dimos.robot.unitree.go2 import ( |
Contributor
Author
There was a problem hiding this comment.
We did, but I need them here for the same reason we banned them: to import more than is needed.
This solution requires importing all of the connections in order to register them in the registry.
…modules
Resolved conflicts:
- dimos/robot/unitree/connection.py: deleted (per PR); ported time_is_now
helper and its application to lidar/odom/video streams into
Go2WebRtcConnection (was in unitree/connection.py on main).
- dimos/robot/unitree/go2/connection.py: deleted (per PR); ported
dedicated_worker=True onto Go2WebRtcConnection and main's YAML-based
camera_info loading into dimos/robot/unitree/go2/camera.py.
- dimos/robot/unitree/go2/blueprints/basic/unitree_go2_basic.py: kept PR's
Go2WebRtcConnection import; adopted main's commented-out
ClockSyncConfigurator (sensor timestamps now derived on reception).
- dimos/robot/all_blueprints.py: dropped foxglove-bridge (removed on main)
and obsolete g1-connection entries (classes no longer exist on PR);
verified by pytest dimos/robot/test_all_blueprints_generation.py.
- docs/usage/{data,sensor}_streams/storage_replay.md: updated
connection.py doclinks to point to new connection_replay.py.
Contributor
Author
|
Needs to be redone. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Closes DIM-XXX
Solution
How to Test
Contributor License Agreement