Repository navigation
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
go2 replay realtime (arm64)
Details
| Benchmark suite | Current: 6d4ea6e | Previous: 5bc57a0 | Ratio |
|---|---|---|---|
peak memory |
1736.574 MB |
1776.91 MB |
0.98 |
peak threads |
375 threads |
376 threads |
1.00 |
network (transport) |
2725.84 MB |
2730.972 MB |
1.00 |
disk write |
1.566 MB |
1.59 MB |
0.98 |
instructions |
137.175 G |
138.52 G |
0.99 |
This comment was automatically generated by workflow using github-action-benchmark.
 [High impact] The PR changes shared command routing used by arm and mobile-robot control workflows. Do not merge yet: xArm commands can still fight each other, and joint-feedback recovery can still restore old holds. Findings
SummaryUnifies connection commands around named joints and their declared interfaces.
Reviews (4) · Last reviewed commit: "fix(control): stop velocity-only joints ..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #5016 +/- ##
==========================================
+ Coverage 83.82% 83.93% +0.11%
==========================================
Files 1866 1868 +2
Lines 182008 182436 +428
Branches 14987 15002 +15
==========================================
+ Hits 152575 153136 +561
+ Misses 25467 25428 -39
+ Partials 3966 3872 -94
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 6 files with indirect coverage changes 🚀 New features to boost your workflow:
|
… hold Greptile review on #5016: - P1: OperatorHoldTask discovered axes only in the joint-state snapshot, which never contains a connection robot's base axes (they are commanded speeds, not read positions), so a running velocity task kept driving the base through a hold, including after an e-stop clear. The coordinator now names every connection base axis to the hold (on start and on add_task), and the hold zeroes them like an adapter base's velocity joints. - P2: the base-timeout test slept 0.1s against a 0.2s reading timeout, so a paused runner stopped the command entirely and the zeroed twist never arrived (seen on macOS CI). The test now advances the task clock (perf_counter, the source of state.t_now) past the task timeout by hand and re-reports the base's feedback right before each tick, so only the task command expires. - P2: the routing test drove an arm named mock and a base named go2, never exercising one robot's <robot>/base split and merge. A new test gives one robot joint ports and cmd_vel and asserts both commands leave in the same tick; another covers the route-level mode-conflict guard, which was the patch's only uncovered code. Regressions: the hold zeroes a running base and keeps it after an e-stop clear (connection level), and unit tests cover set_base_axes with and without an e-stop.
TomCC7
changed the title
| velocity_key = joint.command_keys.get(Interface.VELOCITY) | ||
| if mode is ControlMode.VELOCITY and velocity_key is not None: | ||
| values[velocity_key] = 0.0 | ||
| self._holds.pop(joint.name, None) | ||
| continue | ||
| hold = self._hold(joint, latest) | ||
| if hold is not None: | ||
| values[joint.command_keys[Interface.POSITION]] = hold | ||
| elif velocity_key is not None: | ||
| values[velocity_key] = 0.0 |
There was a problem hiding this comment.
Jogging one xArm joint now leaves the other joints on position hold. That sends both position_command and velocity_command in the same tick. But XArmConnection.write() switches the whole arm between position and velocity modes, and each write fills every arm joint. The messages overwrite each other and repeatedly switch modes.
Preserve the xArm's shared-mode constraint, or translate the complete command into one supported arm-wide write.
There was a problem hiding this comment.
Fixed in 6d4ea6e. The concrete xArm config now declares one shared arm interface: position-only by default, or explicitly velocity-only for jogging; mixed/dual arm declarations are rejected. The gripper remains position-only. Generic arbitration has no arm/base special case. The regression sends a single velocity winner through ConnectionSource and the real command port, verifies a single velocity frame with zero-speed peers, no servo overwrite and no repeated SDK mode switches. xArm connection tests passed under hash seeds 0, 1 and 2 (23 cases per seed); the final affected suite passed 307 cases.
| return len(self._reported) == len(self._needed) and all( | ||
| now - self._port_receipts[port] <= self._timeout for port in self._needed |
There was a problem hiding this comment.
The new per-port timeout stops commands when joint_state goes quiet while imu keeps arriving. Recovery still checks only the last message from any port. The live IMU prevents _resumed from being set, so returning joint feedback makes command() reuse old position holds instead of taking the newly measured positions. The robot can move back toward an old target after the outage.
Reset holds and required readings after any required port expires, using the same freshness check that stopped commands.
Knowledge Base Used: Robot control platform
There was a problem hiding this comment.
Fixed in 6d4ea6e. Recovery now checks the required ports' own receipt ages, not the last message from any port. Any expired required port clears cached required readings and marks holds for recapture. The regression keeps IMU live across a joint-state outage, returns partial then complete joint feedback, and verifies the old 0.5 position target is replaced by the newly measured 0.8 hold. Included in the final 307 passing affected tests.
| joint_commands: tuple[Interface, ...] = () | ||
| pd: bool = False | ||
| base_range: Twist | None = None | ||
| joint_commands: dict[str, tuple[Interface, ...]] = {} |
There was a problem hiding this comment.
The new joint_commands dictionary breaks R1ProConnectionConfig in PR #5017, which still declares a tuple. The revised limits() calls .keys() on that tuple and fails before connecting. PRs #5019 and #5084 retain the same declaration. They also still use the removed base converters and old coordinator configuration.
Migrate those dependent callers alongside this contract change, or keep a compatible path until they are updated.
There was a problem hiding this comment.
Checked the actual published descendants: #5017 at b61686c, #5019 at 3920bf8, and #5084 at f03b8b2 all declare joint_commands as a dictionary (upper-body positions, base velocities) and their coordinator blueprints use connections=. The connection/blueprint files do not reference the removed virtual-coordinate converters. No legacy shim was added. 6d4ea6e additionally migrates the Piper connection and new Piper/xArm blueprints from main; their actual CI-failing cases now pass locally, with targeted mypy clean. Only #5016 was rebased/pushed; the descendants still need a separate parent synchronization.
There was a problem hiding this comment.
Thanks for checking the published descendants. You’re right—the current R1 Pro declarations use dictionary-based joint_commands, and the changes in #5016 migrate its Piper connection and coordinator blueprints as well. My startup-break warning was based on stale branch state; no legacy shim is needed, and I retract that finding. The descendants’ separate parent synchronization remains follow-up coordination, not a blocker in #5016.
… hold Greptile review on #5016: - P1: OperatorHoldTask discovered axes only in the joint-state snapshot, which never contains a connection robot's base axes (they are commanded speeds, not read positions), so a running velocity task kept driving the base through a hold, including after an e-stop clear. The coordinator now names every connection base axis to the hold (on start and on add_task), and the hold zeroes them like an adapter base's velocity joints. - P2: the base-timeout test slept 0.1s against a 0.2s reading timeout, so a paused runner stopped the command entirely and the zeroed twist never arrived (seen on macOS CI). The test now advances the task clock (perf_counter, the source of state.t_now) past the task timeout by hand and re-reports the base's feedback right before each tick, so only the task command expires. - P2: the routing test drove an arm named mock and a base named go2, never exercising one robot's <robot>/base split and merge. A new test gives one robot joint ports and cmd_vel and asserts both commands leave in the same tick; another covers the route-level mode-conflict guard, which was the patch's only uncovered code. Regressions: the hold zeroes a running base and keeps it after an e-stop clear (connection level), and unit tests cover set_base_axes with and without an e-stop.
8f86fbc to
a1502e7
Compare
github-actions
Bot
added
the
ready-to-merge
| eligible = { | ||
| name | ||
| for name, interfaces in state.joints.joint_command_interfaces.items() | ||
| if Interface.POSITION in interfaces |
There was a problem hiding this comment.
The operator hold should stop every joint, including speed-only ones.
We send the base zero speed how its implemented right now. with this change nav or teleop can keep driving the base while a hold is on
There was a problem hiding this comment.
Fixed in d7763c0. OperatorHoldTask now freezes measured positions on position-capable joints and sends zero velocity on velocity-only joints, selected from declared interfaces rather than base names. joint_names scopes both groups. Mixed-mode outputs use ordinary per-joint arbitration; no coordinator hold special case. Regression checks cover ongoing velocity input, E-stop/clear remaining held until acknowledgement, and scoped exclusions. 141 targeted tests passed; 57 hold/connection tests independently rerun; targeted production mypy and Ruff passed. No CI or hardware checks.
There was a problem hiding this comment.
The requested d7763c0 hold behavior is preserved in the rebased head 6d4ea6e: position-capable joints freeze, velocity-only joints receive zero speed, explicit scope restricts both, and E-stop clear does not replay stale motion. The README contradiction is now fixed too. Hold/coordinator/connection regressions are included in the final 307 passing affected tests. No human threads were auto-resolved.
|
@greptile review |
github-actions
Bot
removed
the
ready-to-merge
| `operator_hold` (priority 100) automatically freezes every joint that accepts | ||
| position commands at its measured position and waits for a person. Velocity-only | ||
| joints remain visible to other tasks but are not claimed by the hold, even when | ||
| they report positions. `joint_names` optionally restricts the eligible subset. | ||
| It is not an e-stop: the e-stop cuts motion; the hold keeps position. |
There was a problem hiding this comment.
operator_hold now claims velocity-only joints and sends zero speed, but this section still says those joints are not claimed. That gives operators the wrong expectation about whether the base can move during a hold. Update the description to say that the hold stops velocity-only joints unless joint_names excludes them.
| `operator_hold` (priority 100) automatically freezes every joint that accepts | |
| position commands at its measured position and waits for a person. Velocity-only | |
| joints remain visible to other tasks but are not claimed by the hold, even when | |
| they report positions. `joint_names` optionally restricts the eligible subset. | |
| It is not an e-stop: the e-stop cuts motion; the hold keeps position. | |
| `operator_hold` (priority 100) automatically freezes every joint that accepts | |
| position commands at its measured position and waits for a person. Velocity-only | |
| joints are claimed at the same priority and commanded to zero speed, even when | |
| they report positions. `joint_names` optionally restricts both groups. | |
| It is not an e-stop: the e-stop cuts motion; the hold keeps commanding these targets. |
There was a problem hiding this comment.
Fixed in 6d4ea6e. The README now says operator_hold claims velocity-only joints at the same priority and commands zero speed, and that joint_names restricts both position and velocity groups. This matches the preserved hold implementation; the final 307 passing affected tests include hold and scoped-hold coverage.
| return Pose(position=offset_position, orientation=target_pose.orientation) | ||
|
|
||
|
|
||
| def twist_to_values(twist: Twist, joints: tuple[str, str, str], *, yaw: float) -> dict[str, float]: |
There was a problem hiding this comment.
These changes can now be deleted
There was a problem hiding this comment.
Removed in 6d4ea6e: twist_to_values, twist_from_values, pose_to_values, pose_from_values and their private planar helpers, plus tests that only exercised those unused helpers. A repository-wide reference audit found no production callers. Kept twist_to_numpy and the navigation controllers' independently used world-to-body conversion. Navigation, conversion and affected control/connection tests are included in the final 307 passing tests.
…coordinator A robot behind a ConnectionModule with a cmd_vel port had its base told 0 on every axis every tick, so no task could drive it. Now its base axes (<robot>/base/vx, vy, wz) are registered like an adapter base's virtual joints: tasks claim and win them in arbitration, the tick loop routes them to their own per-robot entry so a velocity task on the base never mode-conflicts with a position task on the joints, and ConnectionSource fills each axis with the winner's speed or 0 when idle or stopped. The coordinator's twist_command mapping covers connection robots too, so the same teleop/nav publishers drive an adapter base or a connection base unchanged. Estop semantics are unchanged: commands stop, the connection module's own deadman halts the robot. Arm-only coordinators and legacy adapter stacks are untouched: a robot without a cmd_vel port has no base axes, and adapters never take the new route.
Follow-up verification for the connection base winners: a velocity task or twist drives vx, vy and wz each non-zero through to the robot's cmd_vel port; two velocity tasks on the same base arbitrate by priority and the loser resumes when the winner stops; and a task whose stream goes quiet zeroes every axis, as an adapter base's does.
The velocity-task timeout can be observed by a later tick than the sleep(0.1) boundary on slow or loaded runners (seen on macOS CI), so keep ticking until the zeroed command shows up.
… hold Greptile review on #5016: - P1: OperatorHoldTask discovered axes only in the joint-state snapshot, which never contains a connection robot's base axes (they are commanded speeds, not read positions), so a running velocity task kept driving the base through a hold, including after an e-stop clear. The coordinator now names every connection base axis to the hold (on start and on add_task), and the hold zeroes them like an adapter base's velocity joints. - P2: the base-timeout test slept 0.1s against a 0.2s reading timeout, so a paused runner stopped the command entirely and the zeroed twist never arrived (seen on macOS CI). The test now advances the task clock (perf_counter, the source of state.t_now) past the task timeout by hand and re-reports the base's feedback right before each tick, so only the task command expires. - P2: the routing test drove an arm named mock and a base named go2, never exercising one robot's <robot>/base split and merge. A new test gives one robot joint ports and cmd_vel and asserts both commands leave in the same tick; another covers the route-level mode-conflict guard, which was the patch's only uncovered code. Regressions: the hold zeroes a running base and keeps it after an e-stop clear (connection level), and unit tests cover set_base_axes with and without an e-stop.
The task clock (perf_counter) was already patched, but the connection source stamps feedback receipts and checks their freshness with the real monotonic clock, so a scheduling pause longer than reading_timeout_s between the last report and the tick could still stale the feedback and drop the zeroing command. Patch time.monotonic with its own frozen clock too, and keep a real 0.25s pause between the report and the tick to prove a slow runner cannot stale the feedback: with the monotonic patch removed the strict assertions fail again.
d7763c0 to
6d4ea6e
Compare
Overview
Unify connection control around named joints and supported interfaces. First layer of #5017 → #5019 → #5084.
Changes
joint_namesrestricts both groups. Update the hold documentation accordingly.Rebase and validation
Head:
6d4ea6e92245816d0c59396d302cd36d0e236a09, rebased onto main5bc57a07087d427cbbdd84281523c5c1e970ff0c. All 17 original commits were replayed from the previous main boundary; the original head is backed up locally.Implementer validation at this head:
CI=1.git diff --checkpassed.Hardware behavior/latency, the full test suite, and post-push CI are not verified. No new CI polling was started. Existing dependency deprecation/resource warnings remain.
Only this root PR was rewritten. The published R1 Pro descendants already use dictionary declarations and
connections=, but still need a separate parent synchronization after this root rebase; their branches were not changed.