Add optional rate-limiting retargeters for bare position-servo followers (SO-101 class)#727
Add optional rate-limiting retargeters for bare position-servo followers (SO-101 class)#727johnnynunez wants to merge 5 commits into
Conversation
…followers Low-cost followers like the SO-101 track whatever position command reaches the bus at full servo torque, with no controller-side handling of anomalous targets: one bad frame (IK jump, tracking glitch, leader-stream hiccup, operator flick) becomes a full-speed slew that can strip gears or stall motors on real hardware. Add two drop-in harness nodes that bound per-frame command velocity without changing the stream contract: - EePoseRateLimiter: clamps linear [m/s] and angular [rad/s] velocity of an absolute 7-D ee_pose stream (same contract as SO101ClutchRetargeter / Se3AbsRetargeter, inserts before the reorderer). - JointRateLimiter: clamps per-joint velocity of a name-keyed joint_targets group (same contract as JointStateRetargeter's joint mode), with optional per-joint overrides. Both latch on the first valid frame after construction/reset (engage-time placement stays owned by the upstream clutch/align logic), clamp against the last emitted command, derive dt from graph real_time_ns clamped to [min_dt, max_dt] so a pipeline stall cannot authorize a teleport, hold last on dropped frames, and re-latch on reset. Below the limit they are exact pass-throughs (no smoothing lag). Covered by sim-free unit tests (pure clamp math + compute-level behavior, mirroring test_so101_retargeters.py patterns). Signed-off-by: johnnynunez <johnnynuca14@gmail.com>
|
📝 Docs preview is not auto-deployed for fork PRs. A maintainer with write access to |
📝 WalkthroughWalkthroughThis PR adds a new rate-limiting safety-harness module ( Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Pipeline
participant EePoseRateLimiter
participant JointRateLimiter
participant GraphTime
Pipeline->>GraphTime: read real_time_ns
Pipeline->>EePoseRateLimiter: compute(ee_pose input, context)
EePoseRateLimiter->>EePoseRateLimiter: _clamped_dt(prev_time, real_time_ns)
EePoseRateLimiter->>EePoseRateLimiter: clamp position/orientation step
EePoseRateLimiter-->>Pipeline: clamped ee_pose
Pipeline->>JointRateLimiter: compute(joint_targets input, context)
JointRateLimiter->>JointRateLimiter: _clamped_dt(prev_time, real_time_ns)
JointRateLimiter->>JointRateLimiter: clamp per-joint delta
JointRateLimiter-->>Pipeline: clamped joint_targets
Estimated code review effortEstimated code review effort: 4 (Complex) | ~60 minutes Suggested labelsSuggested labels: enhancement, needs-review Suggested reviewersSuggested reviewers: retargeting-engine maintainers 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/retargeters/rate_limiter.py`:
- Around line 301-338: The pose handling in the rate limiter path should reject
non-finite EE positions before latching or clamping, since a malformed position
can be stored as the baseline or propagate NaNs through _clamp_position_step.
Update the pose-processing logic in the rate-limiter flow around
_quat_normalize, _clamp_position_step, and the self._last_pose latch so that any
input with NaN/Inf in pose[:3] is treated as invalid and does not update the
emitted pose or internal state. Preserve the existing degenerate-orientation
handling, but add a finite-position guard before both the first-frame latch and
the later position-limiting branch.
- Around line 120-131: Update __post_init__ in the rate limiter dataclass to
reject non-finite configuration values before the existing positivity checks:
validate max_linear_velocity, max_angular_velocity, max_joint_velocity, each
entry in joint_velocity_overrides, and the dt bounds using a finiteness check so
inf and NaN are not accepted. Keep the current ordering constraints for min_dt,
nominal_dt, and max_dt, but ensure any non-finite value raises ValueError with a
clear message from __post_init__.
- Around line 426-451: Guard joint targets against NaN in the rate limiting
path: the current `np.clip` flow in the target update logic can preserve invalid
values and then store them in `_last_targets`, which poisons future outputs.
Update the frame-handling code in the target limiter method that builds
`targets` and `limited` so it detects or filters non-finite joint values before
applying the clamp, and only writes valid values into `_last_targets` and `out`;
if any target is invalid, keep the previous state or substitute a safe fallback
instead of propagating it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 74e5b0dc-690c-4bc9-8ebd-f0a4b603eb11
📒 Files selected for processing (3)
src/core/retargeting_engine_tests/python/test_rate_limiter.pysrc/retargeters/__init__.pysrc/retargeters/rate_limiter.py
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Clamping alone still walks the arm toward an anomalous target at the velocity limit. When the anomaly is an IK divergence or tracking teleport (observed: raw joint commands flipping ~100 deg in one frame), that motion should not be executed at all. Add optional reject_linear/angular/joint_velocity thresholds to RateLimiterConfig: a frame whose input velocity (vs the last accepted input, over the clamped dt) exceeds the threshold is refused outright -- the limiter holds the last emitted command and does not advance its baseline. A single bad joint rejects the whole joint frame. After max_consecutive_rejections consecutive anomalous frames (default 30, ~1 s at 30 FPS) the input is treated as a persistent new target and re-accepted, still clamped, so rejection cannot deadlock the pipeline; None holds indefinitely. Thresholds must be >= the corresponding clamp limits; rejection is disabled by default (existing behavior unchanged). Covered by 20 new unit tests including glitch-recovery, whole-frame rejection on a single bad joint, cap re-acceptance, and config validation; mutation-checked against disabled detection in both nodes. Signed-off-by: Johnny <johnnync13@gmail.com>
What
Adds two optional retargeter nodes under
src/retargeters/that govern commands before they reach a follower robot:JointRateLimiter— per-joint velocity clamp (with per-joint overrides, e.g. a faster gripper) against the last emitted command.EePoseRateLimiter— end-effector pose clamp: linear velocity [m/s] + geodesic angular velocity [rad/s] (double-cover safe).RateLimiterConfig— shared config (limits,min_dt/max_dtclamping, per-joint overrides, rejection thresholds).Plus
src/core/retargeting_engine_tests/python/test_rate_limiter.py(sim-free unit tests, same pattern astest_so101_retargeters.py).Both nodes now implement a three-band governor:
max_*_velocityis slewed toward the target at the limit.reject_linear_velocity/reject_angular_velocity/reject_joint_velocityis a discontinuity (IK divergence, tracking teleport, stream catch-up step), not motion to follow, and is not executed at all: the limiter holds the last emitted command and does not creep toward the anomaly. For the joint limiter, a single joint over the threshold rejects the whole frame — joints move together, and executing the sane joints of an insane frame still produces an insane configuration.Rejection anti-deadlock: after
max_consecutive_rejectionsconsecutive anomalous frames (default 30, ~1 s at 30 FPS) the input is treated as a persistent new target — a legitimate regime change, e.g. the leader really did move during a stream outage — and re-accepted, still clamped by band 2.Noneholds indefinitely until the input returns within the envelope or a reset arrives. Rejection is disabled by default (reject_* = None), so existing clamp-only behavior is unchanged; config validation enforcesreject_* >= max_*(a rejection threshold below the clamp limit would refuse motion the clamp band is meant to allow).Why
Follower arms built on bare position servos (SO-101 Feetech bus and similar) execute whatever position reaches the bus at full torque — there is no controller-side velocity/accel envelope. Any anomalous retargeting frame becomes a full-torque slew:
This class of failure physically damaged a gear train on one of our SO-101 followers during XR teleop (context: [Slack thread — SO101 harness MR into Isaac Teleop, Lior Ben Horin]).
Clamping alone bounds the damage but still walks the arm toward the anomaly at the velocity limit — a ±100° IK flip at a 90 °/s clamp is 90 °/s of confident motion toward a garbage target for as long as the divergence lasts. The rejection tier closes that gap: a frame that asks for e.g. 3000 °/s is not evidence of intent, it is evidence of malfunction, and the correct amount of motion to execute toward it is zero.
Design notes
dtis clamped to[min_dt, max_dt]— a multi-second pipeline stall does not authorize a multi-second-sized step on the next frame.Testing
test_rate_limiter.pyincl. 20 new rejection/validation tests + existingtest_so101_retargeters.py+test_joint_state_retargeter.py) against the locally built wheel._is_anomalousin the EE limiter fails 5 tests; disabling the joint-frame rejection predicate fails 4 tests; removing the position clamp or the joint clip fails the corresponding clamp tests.isaac_teleop_to_so101example wrappingJointRateLimiterbetweendevice.compute()androbot.send_action()): anomalous IK frames (±100° raw flips) were clamped to ≤3°/frame steps at 90 °/s; normal teleop is pass-through.SKIP=check-copyright-year pre-commitclean; DCO signed.