Fix spline point velocity and acceleration precision - #559
Open
s4lmon wants to merge 9 commits into
Open
Conversation
s4lmon
commented
Aug 26, 2026
s4lmon
commented
Aug 26, 2026
Member
|
Hi @s4lmon |
There was a problem hiding this comment.
Pull request overview
Improves spline velocity/acceleration wire precision to prevent quantization-induced controller faults.
Changes:
- Adds
MULT_VEL_ACC = 1e8with range validation. - Updates URScript decoding and driver templating.
- Expands protocol documentation and tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
include/ur_client_library/control/trajectory_point_interface.h |
Defines the multiplier and range limit. |
src/control/trajectory_point_interface.cpp |
Validates and encodes spline velocity/acceleration. |
src/ur/ur_driver.cpp |
Injects the multiplier into scripts. |
resources/external_control.urscript |
Decodes spline fields using the new scale. |
doc/architecture/trajectory_point_interface.rst |
Documents scaling and limits. |
tests/test_trajectory_point_interface.cpp |
Tests precision, encoding, and range validation. |
tests/test_script_reader.cpp |
Updates script-template test data. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+528
to
+533
| EXPECT_EQ(send_vel[0], ((double)received_velocities[0]) / traj_point_interface_->MULT_VEL_ACC); | ||
| EXPECT_EQ(send_vel[1], ((double)received_velocities[1]) / traj_point_interface_->MULT_VEL_ACC); | ||
| EXPECT_EQ(send_vel[2], ((double)received_velocities[2]) / traj_point_interface_->MULT_VEL_ACC); | ||
| EXPECT_EQ(send_vel[3], ((double)received_velocities[3]) / traj_point_interface_->MULT_VEL_ACC); | ||
| EXPECT_EQ(send_vel[4], ((double)received_velocities[4]) / traj_point_interface_->MULT_VEL_ACC); | ||
| EXPECT_EQ(send_vel[5], ((double)received_velocities[5]) / traj_point_interface_->MULT_VEL_ACC); |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #559 +/- ##
==========================================
- Coverage 80.51% 80.20% -0.32%
==========================================
Files 116 116
Lines 6976 6995 +19
Branches 3083 3095 +12
==========================================
- Hits 5617 5610 -7
- Misses 980 1002 +22
- Partials 379 383 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
Comment on lines
+345
to
+348
| void TrajectoryPointInterface::setVelAccMultiplier(const int32_t multiplier) | ||
| { | ||
| mult_vel_acc_ = multiplier; | ||
| } |
urfeex
requested changes
Aug 31, 2026
urfeex
left a comment
urfeex
left a comment
Member
There was a problem hiding this comment.
Could you please add a test for a legacy script that it falls back to using MULT_JOINTSTATE?
| (spline_primitive->target_accelerations.has_value() && | ||
| !within_range(spline_primitive->target_accelerations.value()))) | ||
| { | ||
| URCL_LOG_ERROR("Spline point velocity or acceleration out of range. Maximum allowed magnitude is %f.", |
Member
There was a problem hiding this comment.
Printing the actual value might be helpful to narrow it down.
Comment on lines
+345
to
+348
| void TrajectoryPointInterface::setVelAccMultiplier(const int32_t multiplier) | ||
| { | ||
| mult_vel_acc_ = multiplier; | ||
| } |
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
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.
The spline vel/acc are quantised at
1e-6byMULT_JOINTSTATE, so near-zero accelerations reconstruct as jagged profiles. On our UR15 we notice many current compensation errors, that make the arm unusable with the ROS 2 driver.We have this fix that is working, implementing the same strategy as #482.
We implement a finer dedicated multiplier (
MULT_VEL_ACC = 1e8) and an explicit range guard (~21.47 rad/s(²)), analagous to max goal time check.This is backwards compatible. We achieve this by only setting the range guard if the modern encoding is used.
UrDriverdetects whether the control script defines the new multiplier via{{VEL_ACC_REPLACE}}. If undefined, we fall back to legacyMULT_JOINTSTATEencoding with a warning.Credit to Toni Divic for discovering this bug.
Note
Medium Risk
Changes real-time trajectory wire encoding and robot script decoding for splines; mismatch between library and custom scripts without
{{VEL_ACC_REPLACE}}keeps legacy behavior but with reduced precision.Overview
Spline trajectory points now encode per-sample velocities and accelerations with a dedicated
MULT_VEL_ACC(1e8) instead ofMULT_JOINTSTATE(1e6), so near-zero values survive the int32 wire format and acceleration profiles stay smooth.external_control.urscriptdecodes those fields withMULT_velaccfrom a new{{VEL_ACC_REPLACE}}placeholder; docs describe limits (~21.47 rad/s²) and rejection of out-of-range values.TrajectoryPointInterfaceapplies the finer multiplier only for SPLINE points (positions still use joint-state scaling), exposessetVelAccMultiplier, and validates spline vel/acc magnitude against the active multiplier.ScriptReaderrecords which placeholders were substituted (isVariableRegistered) soUrDriverpicks modern vs legacy encoding after loading the control script—warning and falling back toMULT_JOINTSTATEwhen the placeholder is missing. Tests cover near-zero roundtrip, range checks, legacy multiplier, and variable registry behavior.Reviewed by Cursor Bugbot for commit 892841f. Bugbot is set up for automated code reviews on this repo. Configure here.