Skip to content

[RSDK-14385] Emit per-joint velocity and acceleration limits - #227

Open
JohnN193 wants to merge 3 commits into
viam-modules:mainfrom
JohnN193:rsdk-14385-emit-joint-kinematic-limits
Open

[RSDK-14385] Emit per-joint velocity and acceleration limits#227
JohnN193 wants to merge 3 commits into
viam-modules:mainfrom
JohnN193:rsdk-14385-emit-joint-kinematic-limits

Conversation

@JohnN193

@JohnN193 JohnN193 commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Ticket 7 of RSDK-14385. The SVA kinematics schema now carries optional per-joint max_velocity and max_acceleration so the motion service can generate trajectories itself instead of handing arms a path and asking them to build a kinematic chain. This publishes them from our synthesized document.

What changed

  • JointLimits gains max_velocity_deg_per_sec and max_acceleration_deg_per_sec2 as std::optional, because an absent field is how the schema spells unbounded and the shipped kinematics/<model>.json files carry position bounds only.
  • Kinematics::with_kinematic_limits attaches per-joint limits, taking radians (what state_ holds) and converting to the degrees the schema uses.
  • to_sva_json writes each field only when set. load_kinematics reads them back, so a document we emit reparses to what we wrote.
  • state_::create captures the configured limits once, and get_dh_kinematics_json stamps them inside the existing call_once.

What we publish, and one deliberate tradeoff

The values are the configured speed_degs_per_sec and acceleration_degs_per_sec2, already clamped against any max_speed_degs_per_sec / max_acceleration_degs_per_sec2 ceiling, so what we advertise is what an un-overridden move will use. The ceilings are never the effective value: clamp_velocity_limits applies them on the way in, so a configured 30 under a ceiling of 100 runs at 30, and publishing 100 would be a looser bound than the arm honors.

We snapshot at configure time rather than reading the live limits. set_speed_degs_per_sec via DoCommand therefore does not change what GetKinematics reports. That is intentional, not an oversight:

  • The framesystem reads kinematics once when it builds the resource graph and never re-reads it, so live values would not reach the planner anyway.
  • Publishing live values would only make RDK's two read paths disagree with each other, since armplanning.MoveArm fetches per call while the framesystem does not.
  • It would also make the returned bytes change under anything hashing them.
  • Publishing the configured value matches the yaskawa module, so the motion service gets the same meaning from both arms.

A consequence worth knowing: after DoCommand{"set_speed_degs_per_sec": 5} on an arm configured at 60, the document still reports 60 while MoveThrough/ToJointPositions plan against 5.

Testing

  • test_to_sva_json_round_trips_via_parse extended to stamp per-joint-distinct limits (110-160 deg/s, 210-260 deg/s²) so a transposed or broadcast write fails, and to check position bounds survive.
  • test_configured_zero_limits_are_published_as_zero pins the zero behavior in both the parsed optionals and the emitted JSON.
  • test_unstamped_kinematic_limits_are_omitted checks an unstamped document has neither key.

make test green, -Wall -Werror clean, clang-tidy and clang-format clean.

🤖 Generated with Claude Code

The SVA kinematics schema now carries optional `max_velocity` and
`max_acceleration` per joint, so the motion service can generate
trajectories instead of asking arms to turn paths into kinematic chains.
This publishes them from the synthesized document.

`JointLimits` gains the two fields as optionals, since an absent field is
how the schema spells unbounded, and the shipped `kinematics/<model>.json`
files carry position bounds only. `to_sva_json` writes each field only
when it is set. A configured zero is a real limit of zero and is written
as such; only omission means unbounded.

The published values are the configured `speed_degs_per_sec` and
`acceleration_degs_per_sec2`, already clamped against any
`max_speed_degs_per_sec` and `max_acceleration_degs_per_sec2` ceiling, so
what we advertise is what an un-overridden move will use. We capture them
once during `state_::create` rather than reading the live limits, which is
a deliberate tradeoff worth spelling out: `set_speed_degs_per_sec` via
DoCommand moves the live limits for the rest of the session, so after such
a call the document reports a speed the arm will not use. We accept that
because the framesystem reads kinematics once when it builds the resource
graph and never re-reads it, so publishing live values would not reach the
planner anyway. It would only make RDK's two read paths disagree with each
other and make the returned bytes change under anything hashing them.
Publishing the configured value also matches the yaskawa module, so the
motion service gets the same meaning from both arms.

Because the limits are fixed for the lifetime of a `state_`, the document
stays a pure function of the calibration and the configuration, and the
memoized JSON in `cached_kinematics_payload` is untouched.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to focus on this one first rather than the Yaskawa one, because I know this codebase better. Mostly questions at this point. It is a little tricky to review because I keep running into the limits of the current approach and seeing the problems, rather than seeing the gains we make by adopting what is here, despite the rough edges.

Comment thread src/viam/ur/module/ur_arm_model.hpp Outdated
// Returns a copy carrying the given per-joint velocity and acceleration
// limits. Inputs are radians, matching what `URArm::state_` holds; the
// conversion to the degrees the SVA schema uses happens here.
Kinematics with_kinematic_limits(const urcl::vector6d_t& velocity_rad_per_sec, const urcl::vector6d_t& acceleration_rad_per_sec2) const;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Above, these builder style methods are called apply_x, and I recommend continuing that pattern here: apply_kinematic_limits.

Comment on lines +424 to +427
for (std::size_t i = 0; i < k_num_dh_joints; ++i) {
out.limits[i].max_velocity_deg_per_sec = radians_to_degrees(velocity_rad_per_sec[i]);
out.limits[i].max_acceleration_deg_per_sec2 = radians_to_degrees(acceleration_rad_per_sec2[i]);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably, if we have a value for one of these limits from the SVA, that's a true hardware constraint. Should it be possible to increase a limit here? Or should these be capped at what the SVA says the limit really is.

In other words, should we only allow the runtime alteration based on configuration to be a limit down move? Should it be an error to try to increase beyond what the SVA says?

And, what about limits as configured on the robot itself? I believe UR, for instance, offers on-device safety limits on position and velocity. How should those be handled? Should those be involved in our synthesized return?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had intended to only consider the configured values for vel/acc to begin with, rather than having to update all the SVAs. I would rather not modify the actual SVAs just yet, but if its preferred I can try to find the numbers.

I think the short answer is yes, but I would rather save that kind of work for a future kinematics project. When talking with Dan we both felt that whatever we send over the wire should contain both the robot limit and the user's configured limit. That way if another application wants to modify the vel/acc limits they have all of the information to do so.

Def think at some point we should read from the limits configured on the robot to validate against, since we probably should already be doing that. I intended to make followup tickets for both xarm and ur to do so

Comment thread src/viam/ur/module/ur_arm_model.cpp Outdated
joint["axis"] = dh_z_axis;
joint["min"] = limits[i - 1].min_deg;
joint["max"] = limits[i - 1].max_deg;
// Leaving these out is how the schema spells "unbounded", so we

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would such an unbounded velocity or acceleration mean, physically? Should we accept an SVA that constrains only a subset of the joints? I feel like either you know the limits, or you don't.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unbounded would mean that your planner should try to move the arm as fast as possible, hopefully with some other constraint in mind. leaving it unbounded could also work for a simulated environment.

In practice I needed some way to describe an optional state where trajex shouldn't be used for the motion service to grab, as we continue this kinematics work. Basically so arms that still using URDFs or aren't sending kinematic limits won't break in the motion service. the check I added to rdk checks if any of the joints are unset.

For this pr its both impossible to actually send unbounded joints, and in the ur module where the hardware has opinions on how fast motors can move I will go ahead and remove any sort of "unbounded" possibilities.

Comment thread src/viam/ur/module/ur_arm_state.hpp Outdated
Comment on lines +612 to +613
// framesystem reads kinematics once when it builds the resource graph and never re-reads it, so
// publishing live values would not reach the planner anyway; it would only make the two RDK

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are going to need, in the more complete kinematics project, a means to address caching. Consider, for instance, that an arm could be connected to physical arm 1 at a particular IP address, lose connection because that robot arm was taken out of service, and then regain connection to physical arm 2 at that same IP address because it is rotated into service to replace arm 1. The calibration on arm 2 could be different. Without a mechanism to have the arm module signal that kinematics may have changed, the client will be using the wrong one. If you are tracking work items for the next scope, I think it is important to get kinematics cache invalidation on the list.

John Nicholson and others added 2 commits August 24, 2026 14:45
Rename `with_kinematic_limits` to `apply_kinematic_limits` to match the
`apply_calibration` builder next to it.

Stop allowing a document to go out with any joint unconstrained. A UR joint
always has a real speed its motors can do, so there is no unbounded case to
represent here, and RDK's `TrajectoryLimits` is all or nothing regardless:
leave one joint unset and the planner discards timing for the whole arm. So
emitting a partial document is a silent no-op at the far end, and
`to_sva_json` now throws instead of serializing one.

The fields stay optional because that is what a freshly parsed shipped file
honestly contains, none of them carry limits, and because we cannot default
them to zero now that zero is a real limit meaning the joint does not move.
The invariant is on emission rather than on the type.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Make `configured_velocity_limits_` and `configured_acceleration_limits_`
optional. A default-constructed vector is all zeros, and zero is a real
limit meaning the joint does not move, so a snapshot that was never taken
would have been published as an arm configured to stay still.
`get_dh_kinematics_json` now refuses that rather than serving it, which is
the same fail-loud choice `to_sva_json` already makes for a missing limit.

Reject a negative `max_velocity` or `max_acceleration` on parse, matching
what config parsing already does for the same quantities. Zero stays legal.
The parser was previously relying on `apply_kinematic_limits` overwriting
whatever it read, which is true today but not something to depend on.

Cover the two untested branches: the acceleration half of the
refuse-to-serialize guard, and the parse rejections for a present but
non-numeric or negative limit.

Also trim the comments. They had grown to roughly double the module's
comment density, and one block was half again longer than anything else in
the module. The reasoning that belongs in the pull request has moved there.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants