-
Notifications
You must be signed in to change notification settings - Fork 430
Add support for positional target frame arguments for transform wrench node #2040
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
base: master
Are you sure you want to change the base?
Add support for positional target frame arguments for transform wrench node #2040
Conversation
thedevmystic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phenomenal job on adding this features. Appreciate it, @vedh1234
| The wrench transformer can be launched with target frames passed directly as positional arguments: | ||
|
|
||
| .. code-block:: bash | ||
| ros2 run force_torque_sensor_broadcaster wrench_transformer_node | ||
| ros2 run force_torque_sensor_broadcaster wrench_transformer_node frame1 frame2 | ||
| Target frames may also be set via the ``target_frames`` parameter: | ||
|
|
||
| .. code-block:: bash | ||
| ros2 run force_torque_sensor_broadcaster wrench_transformer_node \ | ||
| --ros-args -p target_frames:="['frame1','frame2']" | ||
| Positional arguments override the parameter value when both are provided. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest why don't we keep the standalone executable comment too, and introduce tf2_echo frames option as an "option". So that previous usage of the controller can be still used. And the previous documentation is still valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The node cannot run without specifying target_frames (it throws an initialization error). I saw the tf2_echo doc which also mentions frame usage, that's why I thought of adding it and removing the older comment. So do you think it's still valid to keep the standalone executable comment or we are supposed to add what successfully runs ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the older implementation of force_torque_sensor_broadcaster can run without specifying the tf_echo argument.
Then, I think we shouldn't throw an error if it is missing.
We should introduce tf_echo as an "option", not a requirement. To keep the backward compatibility, when tf_echo wasn't introduced.
We should make it optional, by providing a default value when none is provided, but currently it is a requirement. This will break the usage of old existing users.
Am I missing something, if so, feel free to inform me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification!
Just to confirm — the previous implementation of wrench_transformer_node also required target_frames to be non-empty. The parameter had a not_empty<> validator, so launching the node without specifying target_frames already resulted in a parameter validation error.
So the node was never runnable without frames — only the documentation implied it.
This PR doesn’t change that requirement, it just adds an additional way (positional args) to provide the same frames. The parameter-based approach is still fully supported and backward compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarification.
But this means the not_empty<> is incorrect, as the original documentation explicitly said, it can be used as an standalone executable, without specifying the target frames. As also target frames is initialized as empty list (default value). I think it is a long run inconsistency.
Because, something that is forbidden to be empty, can't have a default value.
Orginal documentation:
The package provides a standalone ROS 2 node wrench_transformer_node that transforms wrench messages published by the ForceTorqueSensorBroadcaster controller to different target frames using TF2. This is useful when applications need force/torque data in coordinate frames other than the sensor frame.
The node subscribes to wrench messages from the broadcaster (either raw or filtered) and publishes transformed versions to separate topics for each target frame.
Usage
The wrench transformer node can be launched as a standalone executable:
ros2 run force_torque_sensor_broadcaster wrench_transformer_nodeAs we can see, we don't need to specify target frames, do we?
So, I think wrench_transformer_node was intended to be used without explicit target_frames. And tf_echo is a option, not an requirement.
So, i think we should remove the validation, as default value is provided, if it is empty.
force_torque_wrench_transformer:
target_frames: {
type: string_array,
default_value: [],
description: "...",
# validation: {
# not_empty<>: null
# }
}What do you say, @vedh1234?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what would be the usage of the wrench_transformer_node without target frames? that was not the original intention. The current state of the documentation fixes this, thanks!
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2040 +/- ##
==========================================
+ Coverage 84.91% 84.93% +0.01%
==========================================
Files 148 148
Lines 14367 14389 +22
Branches 1230 1233 +3
==========================================
+ Hits 12200 12221 +21
Misses 1740 1740
- Partials 427 428 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
christophfroehlich
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot, LGTM!
| The wrench transformer can be launched with target frames passed directly as positional arguments: | ||
|
|
||
| .. code-block:: bash | ||
| ros2 run force_torque_sensor_broadcaster wrench_transformer_node | ||
| ros2 run force_torque_sensor_broadcaster wrench_transformer_node frame1 frame2 | ||
| Target frames may also be set via the ``target_frames`` parameter: | ||
|
|
||
| .. code-block:: bash | ||
| ros2 run force_torque_sensor_broadcaster wrench_transformer_node \ | ||
| --ros-args -p target_frames:="['frame1','frame2']" | ||
| Positional arguments override the parameter value when both are provided. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what would be the usage of the wrench_transformer_node without target frames? that was not the original intention. The current state of the documentation fixes this, thanks!
This PR adds support for passing target frames as positional CLI arguments to
wrench_transformer_node, similar totf2_echo. ( Issue #2027 ) Positional arguments override the existingtarget_framesparameter when both are provided.Includes: