Skip to content

Publish JointState commands for velocity-only and effort-only interfaces#118

Merged
christophfroehlich merged 5 commits into
ros-controls:mainfrom
TedVanderfeen:bugfix/position_publish_only
May 8, 2026
Merged

Publish JointState commands for velocity-only and effort-only interfaces#118
christophfroehlich merged 5 commits into
ros-controls:mainfrom
TedVanderfeen:bugfix/position_publish_only

Conversation

@TedVanderfeen
Copy link
Copy Markdown
Contributor

Description

Fixes the JointStateTopicSystem::write() publish threshold so it considers all supported command interfaces, not just position.

Previously, the rate limiter only accumulated command/state differences for position interfaces. Velocity-only and effort-only hardware configurations could update command values without publishing a sensor_msgs/msg/JointState command message.

This PR updates the threshold check to include:

  • position
  • velocity
  • effort

It also adds regression tests covering position-only, velocity-only, and effort-only command publishing.

Is this user-facing behavior change?

Yes. Users with velocity-only or effort-only command interfaces should now see command messages published on the configured joint_commands_topic when those command values change beyond the trigger threshold.

Did you use Generative AI?

Yes. I used OpenAI Codex to help implement the fix, add regression tests, and run local verification.

Additional Information

Regression check: with the new tests applied but the production fix reverted to the pre-fix implementation, the velocity-only and effort-only publish tests fail because no JointState command message is received:

    [----------] Global test environment tear-down
    [==========] 8 tests from 1 test suite ran. (483 ms total)
    [  PASSED  ] 6 tests.
    [  FAILED  ] 2 tests, listed below:
    [  FAILED  ] TestTopicBasedSystem.topic_based_system_2dof_velocity_only_publishes_commands
    [  FAILED  ] TestTopicBasedSystem.topic_based_system_2dof_effort_only_publishes_commands

With the fix restored, local verification passes:

Summary: 9 tests, 0 errors, 0 failures, 0 skipped

TODOs

To send us a pull request, please:

  • Fork the repository.
  • Modify the source; please focus on the specific change you are contributing. If you also reformat all the code, it will be hard for us to focus on your change.
  • Ensure local tests pass. (colcon test and pre-commit run (requires you to install pre-commit by pip3 install pre-commit)
  • Commit to your fork using clear commit messages.
  • Send a pull request, answering any default questions in the pull request interface.
  • Pay attention to any automated CI failures reported in the pull request, and stay involved in the conversation.

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 82.00000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.65%. Comparing base (81e94e8) to head (e0c7f64).

Files with missing lines Patch % Lines
...test/joint_state_topic_hardware_interface_test.cpp 81.25% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #118      +/-   ##
==========================================
+ Coverage   77.68%   78.65%   +0.97%     
==========================================
  Files           6        6              
  Lines         457      506      +49     
  Branches       46       45       -1     
==========================================
+ Hits          355      398      +43     
- Misses         70       78       +8     
+ Partials       32       30       -2     
Flag Coverage Δ
unittests 78.65% <82.00%> (+0.97%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...rface/src/joint_state_topic_hardware_interface.cpp 63.33% <100.00%> (+2.65%) ⬆️
...test/joint_state_topic_hardware_interface_test.cpp 87.57% <81.25%> (-2.69%) ⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Member

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@TedVanderfeen
Copy link
Copy Markdown
Contributor Author

Thanks @christophfroehlich. Merged latest main back into this branch. Does this need another approval, or is this ready for merge from your side?

@christophfroehlich
Copy link
Copy Markdown
Member

yes, a review by another maintainer would be great

@TedVanderfeen
Copy link
Copy Markdown
Contributor Author

@bmagyar @MarqRazz Could this please be reviewed?

Copy link
Copy Markdown
Collaborator

@MarqRazz MarqRazz left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long to get to this!

@christophfroehlich christophfroehlich merged commit babecd8 into ros-controls:main May 8, 2026
27 of 32 checks passed
@github-project-automation github-project-automation Bot moved this from Needs review to Done in Review triage May 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants