static_transform_publisher argument order

[Stapelzeiger]

Bug report

Required Info:

• Operating System:
  • any
• Installation type:
  • binaries
• Version or commit hash:
  • Foxy patch 1
• DDS implementation:
  • any
• Client library (if applicable):
  • N/A

Steps to reproduce issue

run the following:

# ros2 run tf2_ros static_transform_publisher
A command line utility for manually sending a transform.
Usage: static_transform_publisher x y z qx qy qz qw frame_id child_frame_id
OR
Usage: static_transform_publisher x y z yaw pitch roll frame_id child_frame_id
# ros2 run tf2_ros static_transform_publisher 0 0 0  0.1 0 0 map child_yaw

Expected behavior

child frame is rotated in yaw:

# ros2 run tf2_ros tf2_echo map child_yaw
At time 0.0
- Translation: [0.000, 0.000, 0.000]
- Rotation: in Quaternion [0.000, 0.000, 0.050, 0.999]
...

Actual behavior

child frame is rotated in roll:

# ros2 run tf2_ros tf2_echo map child_yaw
At time 0.0
- Translation: [0.000, 0.000, 0.000]
- Rotation: in Quaternion [0.050, 0.000, 0.000, 0.999]
...

Additional information

The problem is in the change in command line argument order, introduced in #182, specifically this line
I'd be happy to do a PR, however I'm unsure what the actual argument order should be:

• The ROS2 tf2 tutorial has the [roll, pitch, yaw] argument order.
• The tool itself advertises [yaw, pitch, roll] source
• The static_transform_publisher in classical ROS uses the [yaw pitch roll] sequence ROS wiki

I'm leaning towards keeping [yaw pitch roll] and reversing the change in order introduced in #182

To be clear, this only refers to the order of the command line arguments. The rotation sequence is always body fixed yaw, followed by pitch, followed by roll or equivalently fixed axis roll, followed by pitch, followed by yaw.

Lastly, I discovered this bug after update from Eloquent to Foxy. (My static transforms were suddenly all wrong and my robot attempted suicide). I suggest to document this change in command line argument order in the Foxy release notes.

[allenh1]

The problem is in the change in command line argument order, introduced in #182

Yeah, that is definitely what happened. This change was first released for Foxy, so it wouldn't have appeared in Eloquent. Apologies for almost killing your bot 🙃.

This was completely unintentional, so I made a pull request (see #294) to fix it.

In the meantime, you might try using the component version (which was the main feature I meant to introduce with that PR). For the component version, you would put this in your launchfile:

container = ComposableNodeContainer(
    name='static_tf_container',
    package='rclcpp_components',
    executable='component_container',
    node_namespace='',
    composable_node_descriptions=[
        ComposableNode(
            package='tf2_ros',
            plugin='tf2_ros::StaticTransformBroadcasterNode',
            name='your_tf_node_name',
            namespace='',
            parameters=[{
                '/rotation/z': 0.050
            }]
        ),
        # other static transform nodes
    ]
)

[Stapelzeiger]

Great. Thanks. I'll open a PR for the tf2 tutorial to match this.

[clalancette]

Thanks for reporting this bug. I'm sorry I missed it in the review.

Unfortunately, this puts us in a really unfortunate spot, since we are going to be inconsistent no matter what we do. The options
as I see them:

1. Fix Foxy to be like Eloquent and Dashing. This gets us consistency, but we risk breaking anyone who has already started to port to Foxy.
2. Declare the new argument order to be correct, and leave Foxy alone. This means that people porting from Eloquent and earlier will run into the same problem that @Stapelzeiger did. We can document it, though.
3. Rewrite the argument parsing in static_transform_publisher completely to take --roll 0.0 --pitch 0.0 --yaw 0.0 options. We could also backport this to Foxy and recommend it going forward. The big advantage there is that it becomes completely unambiguous which argument is for which axis.

There may be other options that I'm not thinking of. What do people think about these options?

[allenh1]

I'm sorry I missed it in the review.

Honestly, I think this is a testament to how unintuitive the original ordering is... After @Stapelzeiger reported this issue, I remembered pretty quickly how many times this bit me in ROS 1.

I think this ordering of the arguments is an artifact of an older tf/tf2 implementations which used SetYPR (which, if memory serves me, is deprecated/removed as of quite a while ago -- @tfoote please correct me if I'm wrong).

As such, I think we should move towards your second option (declaring the current order of the options to be correct from here on). I lean that way because it would break the behavior currently happening in the Foxy release. That said, at a very minimum, the CLI usage should be updated.

Regardless, I do apologize to anyone to whom this patch has given a headache.

[Stapelzeiger]

Option 2 might bring more confusion in the future, since Dashing and Eloquent are still using the previous order and most importantly so does ROS1 where a lot (most?) of people are still stuck and will be porting their robots from in the future.
People don't expect argument orders to change (especially if the arguments are 6 numbers in a row).

Honestly, I think this is a testament to how unintuitive the original ordering is...

I don't see the order of option 2 more intuitive than option 1. If you are thinking in body fixed (intrinsic) rotations the sequence it is yaw-pitch-roll (matching arguments order of option 1), if you think in terms of static rotation axes (extrinsic) it is roll-pitch-yaw (matching arguments order of option 2). Both are equivalent and either can be more intuitive depending the use case.

I think option 3 is probably the best as it will eliminate possible confusion in the future.

[allenh1]

I don't see the order of option 2 more intuitive than option 1.

This could just be my perspective, if people are more familiar with the YPR ordering then let's certainly do that.

I figured it might be useful to consider the ordering found within REP-103 in this discussion as well.

Euler angles are generally discouraged due to having 24 'valid' conventions with different domains using different conventions by default.

Thinking on it further, however, I support both 2 and 3, but lean towards 2 because it is already released. Option 3 would certainly break things uniformly, and thus would call attention to it (it would crash instead of just behave in a potentially unexpected way).

Maybe a solution would be to add --rpy and --ypr arguments and add a warning message if neither is specified (then in the next distro we can just switch to RPY or revert to YPR, whichever is most desired).

[clalancette]

Maybe a solution would be to add --rpy and --ypr arguments and add a warning message if neither is specified (then in the next distro we can just switch to RPY or revert to YPR, whichever is most desired).

Yeah, my intention here would be to roll it out a little more slowly.

That is, we would implement the --roll, --pitch, --yaw options in addition to what is already there (there's some details to be figured out about what happens if both the old-style and new-style are specified, but we can work through it). Then we mark the "old" command-line options as deprecated (with a warning) for Galactic. Then post-Galactic, we remove the now deprecated ones. We can also backport the addition of the options to Foxy, minus the deprecation warnings.

@allenh1 Do you have any time to work on a plan like this?

I like this plan, but it still leaves the problem of what to do about the order of command-line options in Foxy (since we are going to have a deprecation period no matter what we do). I'll bring it up with the rest of the ROS 2 team to get other opinions.

[allenh1]

@allenh1 Do you have any time to work on a plan like this?

Yes, I'll start on it once you and the ROS 2 team reach a consensus.

[clalancette]

Yes, I'll start on it once you and the ROS 2 team reach a consensus

All right, the consensus is to go back to the ordering that was in Dashing and Eloquent. This also preserves the format that was used in ROS 1, so it should make ports easier. @Stapelzeiger, sorry to say that you'll have to change your code again, but I think this is the best way forward overall.

With that said, I'm going to go review #294 . Once that is approved and merged, I'll open a backport PR for Foxy, and we'll announce the change (back) with the next Foxy sync.

@allenh1 If you have time to work on it, adding in the more specific options sounds like a good thing to start (targeting the ros2 branch). I think we've had two separate proposals:

1. My initial proposal, which would add command-line arguments of --roll, --yaw, --pitch, --qx, --qy, --qz, --qw.
2. Your proposal, which would add command-line arguments of --rpy, --quat.

The first one is slightly more flexible in that you can specify the arguments in any order, but it is more verbose. It's also harder to check that they are mutually exclusive (i.e. what if the user specifies --roll 0.5 --qw 1.0?). The second one is more compact, but still relies on the arguments being specified in the correct order within each option. I prefer the first option, but I'm willing to be convinced that the second one is fine too. @allenh1 @Stapelzeiger , any opinions?

[allenh1]

Nope, all good from me 👍

[Stapelzeiger]

I like option 1, but no strong preference.

I would very much like to see a --roll_deg family of options as well for angles in degrees.

[clalancette]

FYI, I've opened up #295 to track the ongoing work on this issue.