Skip to content

Add multiplier support to ForceTorqueSensorBroadcaster #1647

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

edxmorgan
Copy link
Contributor

@edxmorgan edxmorgan commented Apr 18, 2025

Summary:
This PR introduces a new multiplier parameter set for the force_torque_sensor_broadcaster controller. Users can now specify per–axis scaling factors for both force and torque readings, applied after the existing offset logic. This feature enables dynamic unit conversions or gain adjustments without modifying downstream code.

Changes:

  • Extended the auto‑generated Params to include multiplier.force.{x,y,z} and multiplier.torque.{x,y,z}
  • Implemented apply_sensor_multiplier(...) and invoked it in the publish loop immediately after offset application
  • Added a new gtest (SensorName_Publish_Success_with_Multipliers) mirroring the offset test to verify correct scaling in both the published message and exported state interfaces

Pipeline Status:
✅ All existing and new tests pass locally (colcon test --packages-select force_torque_sensor_broadcaster)
pre-commit run shows no formatting errors

Testing:

  • Unit test covers positive, negative, and zero multipliers
  • Manual verification in a running ROS 2 system confirms expected behavior

Signed-off-by: Edward <papaacquah17@gmail.com>
Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution.

I'm interested in the motivation for this change?, can you please tell us some scenarios where we would be needing this? or a hardware that this is needed.

Thank you

Copy link
Contributor

@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.

Thanks for the contribution. The changes look fine, under reserve of the doubts of Sai if this his necessary at all.

@edxmorgan , as it seems that you are using the FTS broadcaster: Could you please have a look at #559 and leave a review there?

Copy link

codecov bot commented Apr 19, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.23%. Comparing base (39c8ad3) to head (914ab5b).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1647      +/-   ##
==========================================
+ Coverage   85.19%   85.23%   +0.03%     
==========================================
  Files         127      127              
  Lines       12006    12038      +32     
  Branches     1010     1011       +1     
==========================================
+ Hits        10229    10261      +32     
  Misses       1460     1460              
  Partials      317      317              
Flag Coverage Δ
unittests 85.23% <100.00%> (+0.03%) ⬆️

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

Files with missing lines Coverage Δ
...roadcaster/src/force_torque_sensor_broadcaster.cpp 90.90% <100.00%> (+0.64%) ⬆️
...ster/test/test_force_torque_sensor_broadcaster.cpp 98.17% <100.00%> (+0.31%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@edxmorgan
Copy link
Contributor Author

edxmorgan commented Apr 19, 2025

@saikishor @christophfroehlich thank you for considering this contribution. my initial motive for this implementation was to allow for forces/torque frame‐axis adjustment in my visualization: my sensor’s physical axes don’t line up with RViz frame (axes inverted),this resolved it by simply setting a ±1 scale multiplier on the broadcaster to flip or scale each axis, keeping TF tree and visuals correct without messy remapping or creating new state interface for the purpose.

Another use is pure unit conversion—many F/T sensors publish in hardware‑specific units, and with per‑axis multipliers you can turn those raw values into Newtons or Newton‑meters directly via ROS parameters, no C++ changes or extra nodes needed.

@saikishor
Copy link
Member

@saikishor @christophfroehlich thank you for considering this contribution. my initial motive for this implementation was to allow for forces/torque frame‐axis adjustment in my visualization: my sensor’s physical axes don’t line up with RViz frame (axes inverted),this resolved it by simply setting a ±1 scale multiplier on the broadcaster to flip or scale each axis, keeping TF tree and visuals correct without messy remapping or creating new state interface for the purpose.

Another use is pure unit conversion—many F/T sensors publish in hardware‑specific units, and with per‑axis multipliers you can turn those raw values into Newtons or Newton‑meters directly via ROS parameters, no C++ changes or extra nodes needed.

I understand, but I have this feeling that the responsibility of this should belong to the hardware component but not the controller. @christophfroehlich What do you think?. Changing the direction is also normal use case in the controller.

The problem would be, if any controller tries to get the FT sensor data directly from the interfaces, it would be completely inconsistent right?, Ofcourse, you can use the exported interfaces from broadcaster, but then the data would be at low frequency.

@edxmorgan
Copy link
Contributor Author

I understand, but I have this feeling that the responsibility of this should belong to the hardware component but not the controller. @christophfroehlich What do you think?. Changing the direction is also normal use case in the controller.

The problem would be, if any controller tries to get the FT sensor data directly from the interfaces, it would be completely inconsistent right?, Ofcourse, you can use the exported interfaces from broadcaster, but then the data would be at low frequency.

@saikishor You’re right that, in an ideal world, hardware drivers would hand over fully calibrated data. Since this broadcaster already supports per‑axis offsets, adding multipliers was the most natural extension—now it handles both offset and scale directly via ROS parameters. Those two transformations alone cover the vast majority of use cases (unit conversion, axis flipping) without any C++ changes or driver rebuilds.

If this approach feels too “hacky,” I’m happy to explore alternative designs—just let me know what you’d prefer.

…e_sensor_broadcaster/add_multiplier
Copy link
Contributor

@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.

I fixed the clang-format linting errors for you, please install pre-commit before your next PR ;)

saikishor
saikishor previously approved these changes May 17, 2025
@christophfroehlich christophfroehlich dismissed stale reviews from saikishor and themself via 32d0e5d May 17, 2025 13:59
Copy link
Contributor

@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.

Just added release notes

@christophfroehlich christophfroehlich merged commit b06cd33 into ros-controls:master May 17, 2025
19 of 26 checks passed
mergify bot pushed a commit that referenced this pull request May 17, 2025
Signed-off-by: Edward <papaacquah17@gmail.com>
(cherry picked from commit b06cd33)

# Conflicts:
#	doc/release_notes.rst
christophfroehlich pushed a commit that referenced this pull request May 17, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
… (#1686)

Co-authored-by: edward.ix <edmorgangh@gmail.com>
nitin2606 pushed a commit to nitin2606/ros2_controllers that referenced this pull request May 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants