fix(models): validate addconfiguration() input, fix broken named configs#555
Merged
Conversation
…nfigs BaseRobot.addconfiguration() stored its q argument completely unvalidated, unlike its sibling addconfiguration_attr() (which uses getunit(..., dim=self.n)). A malformed stored config (wrong length, or a 0-d/scalar array) would silently succeed at addconfiguration() time and only crash later, deep in configurations_str() (i.e. whenever someone calls print(robot)), with a confusing IndexError far from the actual mistake. Fixed by validating through getvector(q, self.n), matching the existing q/qd/qdd setter pattern in the same file. This immediately surfaced 5 real, pre-existing bugs it had been silently swallowing: - px150/rx150/rx200: qz was np.zeros(7) but these robots have 8 joints (5 arm + gripper_prop + 2 finger links) — off-by-one, qr already had the correct 8 elements. - Jaco (j2n6s200): qr/qz both hardcoded to 6 elements (arm only), but the robot has 10 joints (6 arm + 2 fingers x [finger, finger_tip] = 4 gripper joints). No <mimic> tag exists in the source xacro, so all 10 are genuinely independent joints as far as the kinematic model goes — extended both configs with 4 trailing zeros (gripper open), matching neither qr nor qz being intended to specify gripper state. - LWR4: qr was np.array(7), a 0-d array holding the scalar 7, not a vector — never usable. Docstring turned out to be copy-pasted verbatim from Puma560.py (identical "qs"/"qn" descriptions) without ever implementing qs/qn, and qr was left as a broken placeholder. Removed qr and the docstring's qs/qn claims entirely rather than inventing unverified "ready"/"nominal" pose values for this DH parameterization — left only the working qz. Verified: full test suite (630 tests) passes, plus explicit checks that each fixed model prints/loads correctly and the 6 other Trossen arms (px100/wx200/wx250/wx250s/vx300/vx300s) already had correct, self-consistent qr/qz lengths. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… on current main The resurrected addconfiguration() validation (previous commit, originally authored 2026-07-05) only covered the 5 broken configs it found at the time. Re-running it against current main's model set surfaces 3 more: - Hyper/Hyper3d: self.qr = np.array(N) is the exact same 0-d-scalar-not- a-vector bug already fixed for LWR4 in the previous commit (np.array(N) holds the scalar N, not an N-element vector). Neither class's own docstring lists qr as a defined configuration (only qz) - same as LWR4, this was dead/broken code, not a documented feature. Removed, matching LWR4's precedent, rather than inventing an unverified pose. - DH.AL5D: all of its DH parameter arrays (a, d, alpha, offset, center_of_mass, moments_of_inertia, joint_limits) are defined with 4 entries, but the link-building loop was `for j in range(3)`, building only 3 links while "home" already had 4 elements - a genuine off-by- one, confirmed against the real 4-DOF Lynxmotion AL5D. Fixed to range(4). Also fixed the References link, which used mismatched quote characters instead of proper reST backtick syntax and so never rendered as a link. Verified: full suite green (620 passed, 72 skipped); AL5D/Hyper/Hyper3d construct, print, and fkine correctly with their corrected joint counts. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #555 +/- ##
=====================================
Coverage 0.00% 0.00%
=====================================
Files 141 141
Lines 13660 13654 -6
=====================================
+ Misses 13660 13654 -6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
1 participant
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.
Summary
BaseRobot.addconfiguration()stored itsqargument completely unvalidated, unlike its siblingaddconfiguration_attr()(which usesgetunit(..., dim=self.n)). A malformed stored config (wrong length, or a 0-d/scalar array) would silently succeed ataddconfiguration()time and only crash later, deep inconfigurations_str()(i.e. whenever someone callsprint(robot)), with a confusingIndexErrorfar from the actual mistake.getvector(q, self.n), matching the existingq/qd/qddsetter pattern in the same file.qzwasnp.zeros(7)but these robots have 8 joints — off-by-one,qralready had the correct 8 elements.qr/qzboth hardcoded to 6 elements (arm only), but the robot has 10 joints (6 arm + 4 gripper). Extended both configs with 4 trailing zeros (gripper open).qr = np.array(N)is a 0-d array holding the scalarN, not anN-element vector — never usable, and not even documented as a valid config in any of the three classes' own docstrings. Removed rather than inventing unverified pose values.range(3), building only 3 links while"home"already had 4 elements — a genuine off-by-one, confirmed against the real 4-DOF Lynxmotion AL5D. Fixed torange(4). Also fixed its reference link, which used mismatched quote characters instead of reST backtick syntax and never rendered as a link.This branch originally predates the
/etspackage split and had gone stale (real merge conflicts against currentmain). Resurrected by cherry-picking the original validation-and-5-fixes commit fresh onto currentmain(applied cleanly), then re-running the model smoke test against currentmain's model set, which surfaced 3 further broken configs beyond the original 5 (Hyper/Hyper3d/AL5D) since the codebase moved on since this was first written.Test plan
TestModelSmoke.test_all_models_constructpasses (all registered DH/URDF/ETS models construct).fkinecorrectly with their corrected joint counts.