feat(w1): add version-aware V025 support - #438
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Dexforce W1 robot integration to be version-aware, introducing V025 support via a centralized W1VersionSpec source-of-truth. It removes the previously supported industrial-arm variant (arm_kind) and updates assembly, solver TCP handling, tests, examples, and documentation accordingly.
Changes:
- Added
W1VersionSpec(specs.py) to define per-revision URDF layout, kinematics constants, sensor transforms, and the version-owned EEF/TCP offset (notably the V025 +12 mm Z offset). - Removed
arm_kindthroughout W1 configs/tests/examples and unified W1 arm handling under a single arm model. - Updated assets and docs to include the unified V025 archive (
dexforce_w1/v025/w1.zip) and clarified how the version-owned offset composes with EEF attachments and TCP.
Reviewed changes
Copilot reviewed 25 out of 27 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/sim/solvers/test_srs_solver.py | Drops arm_kind from solver tests; uses unified arm URDF path. |
| tests/sim/objects/test_robot.py | Updates robot test configs to remove arm_kind. |
| tests/sim/objects/test_robot_cfg.py | Updates roundtrip tests, adds tests for unknown-field rejection and V025 EEF/TCP offset composition. |
| scripts/tutorials/gym/modular_env.py | Updates tutorial config to remove arm_kind. |
| examples/sim/utility/workspace_analyzer/analyze_plane_workspace.py | Updates example W1 config to remove arm_kind. |
| examples/sim/utility/workspace_analyzer/analyze_joint_workspace.py | Updates example W1 config to remove arm_kind. |
| examples/sim/utility/workspace_analyzer/analyze_cartesian_workspace.py | Updates example W1 config to remove arm_kind. |
| examples/sim/scene/scene_demo.py | Updates scene demo W1 config to remove arm_kind. |
| examples/sim/robot/dexforce_w1.py | Updates robot example to remove arm_kind. |
| examples/sim/gizmo/gizmo_scene.py | Switches to the unified V021 URDF path instead of the removed industrial-variant asset path. |
| examples/sim/demo/grasp_cup_to_caffe.py | Adds marker visualization for initial kinematics; (currently includes an interactive debugger breakpoint). |
| embodichain/lab/sim/utility/cfg_utils.py | Adjusts comments around merge behavior now that arm_kind is removed from variant fields. |
| embodichain/lab/sim/robots/dexforce_w1/utils.py | Refactors URDF assembly + solver config builders to be version-spec-driven (no arm_kind), applies version-derived TCP. |
| embodichain/lab/sim/robots/dexforce_w1/types.py | Adds V025 and simplifies component types (LEFT_ARM/RIGHT_ARM); removes DexforceW1ArmKind. |
| embodichain/lab/sim/robots/dexforce_w1/specs.py | New: defines W1VersionSpec, local asset override resolution, and per-version calibration/transforms. |
| embodichain/lab/sim/robots/dexforce_w1/params.py | Refactors arm kinematics params to be version-driven (via W1VersionSpec), removing arm_kind. |
| embodichain/lab/sim/robots/dexforce_w1/cfg.py | Updates DexforceW1Cfg to be version/component-version driven, composes user EEF/TCP overrides with the version offset, and removes arm_kind. |
| embodichain/lab/sim/cfg.py | Updates URDFCfg name_case docstring to match the default behavior. |
| embodichain/data/assets/w1_assets.py | Adds DexforceW1V025 dataset and removes industrial-variant dataset assets; renames arm component dataset classes. |
| docs/source/tutorial/modular_env.rst | Updates tutorial narrative to remove arm-type mention. |
| docs/source/resources/robot/dexforce_w1.md | Updates W1 robot docs for V025, unified assets, and explains the version-owned EEF/TCP offset model. |
| docs/source/overview/sim/solvers/srs_solver.md | Updates solver docs to remove arm_kind from the W1 example. |
| docs/source/guides/add_robot.rst | Updates guidance to remove arm_kind from variant-field examples. |
| docs/source/features/workspace_analyzer/workspace_analyzer.md | Updates workspace analyzer docs to remove arm_kind usage. |
| agent_context/topics/robot-system/robot-system.md | Updates agent context docs to reflect the removal of arm_kind from DexforceW1Cfg. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🟡 Not ready to approve
It introduces a confirmed unused import in an updated example and a broken from_dict version parsing path that will reject common version string inputs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (2)
examples/sim/demo/grasp_cup_to_caffe.py:34
MarkerCfgis imported but never used in this script (the only occurrence is in the import list). This can fail lint/CI checks and should be removed, or the code should be updated to actually use it.
from embodichain.lab.sim.cfg import (
RenderCfg,
LightCfg,
MarkerCfg,
JointDrivePropertiesCfg,
embodichain/lab/sim/robots/dexforce_w1/params.py:159
W1ArmKineParams.from_dictcurrently parses string versions viaDexforceW1Version[data["version"]], which only works for enum member names (e.g."V021") and fails for the value strings used elsewhere in configs (e.g."v021","v025"). Parsing should be consistent with the rest of the codebase (value-based, case-insensitive).
version = (
DexforceW1Version[data["version"]]
if isinstance(data.get("version"), str)
else data.get("version", DexforceW1Version.V021)
)
- Files reviewed: 25/27 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
There are confirmed runtime-edge-case failures in W1 override composition (transform/tcp can be None) and a documented local-asset env-var feature (EMBODICHAIN_W1_V025_ROOT) that is not implemented in the code.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (6)
embodichain/lab/sim/robots/dexforce_w1/cfg.py:119
- _compose_user_eef_overrides assumes a user-supplied hand component always has a 4x4 transform, but URDFCfg docs allow omitting it / setting it to None. If a caller provides a left_hand/right_hand override without a transform (or with transform=None), this will raise when compose_eef_attach_xpos tries to validate the shape.
component = self.urdf_cfg.components.get(component_name)
if component is None:
continue
spec = get_w1_version_spec(self._arm_version(arm_side))
component["transform"] = spec.compose_eef_attach_xpos(
arm_side, component["transform"]
)
embodichain/lab/sim/robots/dexforce_w1/cfg.py:57
- DexforceW1Cfg.component_versions is typed as a bare
dict | None, which loses the key/value contract used throughout this PR (DexforceW1Type -> DexforceW1Version). Tightening this helps static analysis and makes the config API clearer.
version: DexforceW1Version = DexforceW1Version.V021
with_default_eef: bool = True
component_versions: dict | None = None
embodichain/lab/sim/robots/dexforce_w1/cfg.py:133
- When composing the version offset into user-overridden solver TCPs, the code does not guard against tcp=None. If a caller sets tcp to None (to request the default), this will fail in compose_eef_attach_xpos; better to skip composition unless a concrete 4x4 TCP is present.
for arm_side in DexforceW1ArmSide:
part_name = f"{arm_side.value}_arm"
part_override = solver_cfg.get(part_name)
if not isinstance(part_override, dict) or "tcp" not in part_override:
continue
solver = self.solver_cfg.get(part_name)
if solver is None:
continue
spec = get_w1_version_spec(self._arm_version(arm_side))
solver.tcp = spec.compose_eef_attach_xpos(arm_side, solver.tcp)
embodichain/lab/sim/robots/dexforce_w1/utils.py:570
- build_dexforce_w1_cfg annotates solver_cfg as dict[DexforceW1Type, SolverCfg], but the function actually stores it into cfg.solver_cfg where keys are control-part strings (e.g. "left_arm", "full_body"). This type mismatch can mislead callers and breaks static typing.
component_versions: dict[DexforceW1Type, DexforceW1Version] | None = None,
solver_cfg: dict[DexforceW1Type, SolverCfg] | None = None,
) -> "DexforceW1Cfg":
embodichain/data/assets/w1_assets.py:133
- The PR description says V025 supports local loading via
EMBODICHAIN_W1_V025_ROOT, but that environment variable is not referenced anywhere in the codebase after these changes. As-is, V025 appears to be discoverable only via the standard dataset download/cache roots (e.g. EMBODICHAIN_DATA_ROOT).
def __init__(self, data_root: str = None):
data_descriptor = o3d.data.DataDescriptor(
os.path.join(
EMBODICHAIN_DOWNLOAD_PREFIX,
w1_assets,
"v025",
"w1.zip",
),
"a983814a05b20ba12fce02883cfd1d7e",
embodichain/lab/sim/robots/dexforce_w1/params.py:56
- post_init assigns NumPy arrays into d_list/link_lengths/rotation_directions, but these fields are declared as
list[...]above. This internal contract mismatch confuses type checkers/IDEs and makes it unclear these are computed fields.
spec = get_w1_version_spec(self.version)
self.d_list = np.asarray(spec.arm_d_list, dtype=float)
self.link_lengths = np.array(
- Files reviewed: 22/24 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
The overall direction looks good: centralizing version assets, kinematics, and EEF/TCP calibration into the spec layer makes the main path clearer. The line-by-line suggestions below focus on config round-trip, state consistency of the public builder, and fail-fast behavior when adding new versions/EEFs. The first two issues have already been reproduced on the current head; I recommend fixing them and adding regression tests before merging.
| component_type = DexforceW1Type(component_type) | ||
| if not isinstance(version, DexforceW1Version): | ||
| version = DexforceW1Version(version.lower()) | ||
| normalized[component_type] = version |
There was a problem hiding this comment.
Blocking: component_versions currently cannot round-trip. The key is normalized to DexforceW1Type here, but RobotCfg.to_dict() serializes dict keys with str(k), so {"left_arm": "v025"} becomes {"DexforceW1Type.LEFT_ARM": "v025"}; on the next from_dict(), the DexforceW1Type(component_type) call on the line above raises ValueError. I recommend explicitly using the enum's .value when serializing (or making the base serializer handle enum keys correctly), and adding a round-trip test that includes component_versions.
| @@ -733,11 +684,19 @@ def build_dexforce_w1_cfg( | |||
| from embodichain.lab.sim.robots.dexforce_w1.cfg import DexforceW1Cfg | |||
|
|
|||
| cfg = DexforceW1Cfg() | |||
There was a problem hiding this comment.
Blocking: this public builder does not encode the build options back into the config object. For example, with include_hand=False, the returned object's with_default_eef is still True; arm_sides, include_head/eyes/wrist_cameras, and the hand type/version have no corresponding fields either. In practice, after build_dexforce_w1_cfg(include_hand=False).to_dict() and reading it back via DexforceW1Cfg.from_dict(), the left and right hands reappear; a config that builds only the left arm also gets the right arm back after the round-trip. I recommend modeling these options as first-class fields of DexforceW1Cfg and making _build_defaults() the single construction entry point; at minimum, sync with_default_eef and add a builder round-trip test.
| if isinstance(data.get("arm_kind"), str) | ||
| else data.get("arm_kind") | ||
| ) | ||
| version = ( |
There was a problem hiding this comment.
This parses by enum member name, so "V025" works, but "v025" — used by other config entry points and the docs — raises KeyError. I recommend providing a single parse_w1_version() (case policy, enum/value/name all handled in one place) shared by cfg, builder, spec, and kinematics, and covering W1ArmKineParams.from_dict({"arm_side": "LEFT", "version": "v025"}).
| arm_d_list=_SHARED_D_LIST, | ||
| arm_base_z=0.1025, | ||
| default_eef_attach_xpos=_V021_DEFAULT_EEF_ATTACH_XPOS, | ||
| solver_tcp=_DEFAULT_TCP, |
There was a problem hiding this comment.
Although W1VersionSpec is frozen=True, the dicts passed in here are mutable, and V021/V022/V025 share the same _DEFAULT_TCP instance. If any caller mutates spec.solver_tcp[...], it simultaneously pollutes the other versions and all subsequent configs, which contradicts the expectation of a "read-only single source of truth." I recommend using immutable mappings / fully tuple-ized data, or creating an independent defensive copy per spec and not exposing the internal containers directly.
| continue | ||
| for brand in DexforceW1HandBrand: | ||
| for side in DexforceW1ArmSide: | ||
| self._urdf_rel_paths[(brand, side, version)] = self._urdf_rel_paths[ |
There was a problem hiding this comment.
This loop silently maps every future W1 revision to the V021 hand asset: as soon as a new version is added to the enum, forgetting to confirm/register a hand asset will not fail-fast. At the same time, DexforceW1Version is used both as the whole-robot revision and as the external EEF version, coupling two separate dimensions. I recommend explicitly listing the supported aliases and raising on missing ones; longer term, W1Revision and HandVersion/HandSpec could be split apart.
| spec = get_w1_version_spec(self._arm_version(arm_side)) | ||
| solver.tcp = spec.compose_eef_attach_xpos(arm_side, solver.tcp) | ||
|
|
||
| def to_dict(self): |
There was a problem hiding this comment.
This introduces a W1-specific serialization protocol for derived state, but agent_context/topics/robot-system/robot-system.md — modified in this same PR — still states that a robot config's from_dict must keep the three-line template and to_dict must be inherited. The implementation and the maintenance rules now contradict each other, which makes incorrect refactoring easy down the line. I recommend at least syncing the agent context to spell out the W1 exception and the raw/final transform semantics; also add round-trip tests for V025 custom EEF/TCP, component_versions, and builder configs.
Description
Add version-aware Dexforce W1 support for V021, V022, and V025.
Key changes:
W1VersionSpecas the source of truth for versioned assets, kinematics, sensors, EEF attachment, and TCP configuration.arm_kind; W1 now uses the anthropomorphic arm model directly.Asset paths:
No new Python dependencies are required.
Type of change
arm_kindconfigurations are removed)Screenshots
Kazam_screencast_00020.mp4
Testing
Checklist
black .command to format the code base.