More Intuitive CLI for Static Transform Publisher #392

allenh1 · 2021-03-10T15:24:44Z

Opening for visibility. Few TODO's left in line that I'll get to in a bit, but this fixes #295.

Signed-off-by: Hunter L. Allen hunter.allen@ghostrobotics.io

allenh1 · 2021-03-11T14:36:21Z

RPY now supported, but need to add --roll-deg and the like still.

clalancette · 2021-03-11T14:39:00Z

tf2_ros/src/static_transform_broadcaster_program.cpp

+  std::unordered_map<std::string, std::string> ret;
+  /* collect from [exe] --option1 value --option2 value ... --optionN value */
+  for (size_t x = 1; x < args.size(); x += 2) {
+    ret.emplace(std::move(args[x]), std::move(args[x + 1]));
+  }


What happens here if the user passes an incorrect command line, like:

--x --y 0

Ah, that's a good catch. Would likely crash.

clalancette · 2021-03-11T14:40:43Z

tf2_ros/src/static_transform_broadcaster_program.cpp

+    rotation = _get_rotation(arg_map);
+    translation = _get_translation(arg_map);


What happens if the user supplies arguments that we don't understand, like --foo?

I think this is actually immune to that. It should set to the identity transform.

That said, it should probably error, since that's not a valid argument.

tf2_ros/src/static_transform_broadcaster_program.cpp

tfoote · 2021-03-11T20:23:21Z

For simplicity I'd suggest that instead of adding a bunch of extra logic to this command line parsing that we consider making a new executable with a new name which can take the new argument styles. This will ensure that we maintain the existing CLI and we can just reuse the existing implementation with a new command line parsing.

One simple alternative that gives a lot of power would be to actually make this into a python tool that has all the bells and whistles turned on, that then executes the less user friendly one under the hood. This would make the command line parsing much less error prone. And users who don't want python on their system can still use the less user friendly C++ version.

clalancette · 2021-03-11T22:53:45Z

For simplicity I'd suggest that instead of adding a bunch of extra logic to this command line parsing that we consider making a new executable with a new name which can take the new argument styles. This will ensure that we maintain the existing CLI and we can just reuse the existing implementation with a new command line parsing.

The thing is, we still need most of the logic to parse the command-line anyway. Adding a second executable only removes the backwards compatibility logic, and I don't expect that to be a whole lot of code anyway. To me, the benefit of keeping the current executable name, and being able to (eventually) remove the old arguments outweighs the downside of carrying a bit of extra logic around.

I guess the command-line parsing becomes much easier if we switch the whole thing to Python. But the thing is that we've gone a long way towards separating C++ and Python packages in geometry2, and I'd like to not go back on that. So we'd have to move the new executable to tf2_ros_py, and then it is harder for users to migrate and discover, etc. I just don't think that parsing the command-line arguments in C++ is going to be that hard, and so I don't think it warrants a big hammer here.

TLDR: my opinion is that we should continue down the path of parsing the command-line arguments in the current executable, and add (just a small bit more) logic to deal with the backwards-compatible arguments.

tfoote · 2021-03-12T01:18:25Z

Ok, If we were to switch it to be python based it would likely move to tf2_tools or the like to avoid the dependency.

clalancette · 2021-03-12T15:33:55Z

Ok, If we were to switch it to be python based it would likely move to tf2_tools or the like to avoid the dependency.

Yes, that's a good point.

It seems to me that we have 3 options here:

Rewrite this utility to be in Python. This has some obvious code maintenance benefits. The downside is that it is disruptive to users on two axes: they need to change the package that the utility comes from, and they need to change the command-line arguments to the new syntax.
Create a new executable in C++ that only takes the new arguments. This gets rid of some back-compat logic, but still requires that we write the command-line parsing ourselves. This is disruptive to users on two axes: they need to change the executable they are running, and they need to change the command-line arguments to the new syntax.
Put all of the command-line parsing and back-compat right into the existing executable. This is disruptive to users on one axis: they need to change the command-line arguments to the new syntax.

Option 3 is the least disruptive to users; option 1 is easier long-term maintenance (2 seems to be the worst of both worlds, so I won't consider it further). Hard to choose :).

@allenh1 You are the one actually working on this, so any thoughts from you on this?

allenh1 · 2021-03-12T17:14:08Z

@allenh1 You are the one actually working on this, so any thoughts from you on this?

Hm. I think @tfoote makes a really good point here. I am not opposed to option 1 here at all.

I'm really not a fan of option 2. I can see that being a real maintenance headache in the future where we have the component version, the standalone executable, and the other standalone executable for the same thing. That's probably overkill, imo.

As for option 3, it's definitely a lot of code for setting arguments, but I'm much faster with my C++ than my Python to be honest. And I feel like after printing the deprecation warning for the old style in Galactic, it can be removed completely in H-turtle.

So my preference is definitely 3, but probably just because it's all C++. I am fine with 1 if that's what we want instead.

clalancette · 2021-03-12T19:25:53Z

@allenh1 I'll leave the final decision to you. I'm slightly more in favor of 3 than 1, just because of the relative ease of transition. Tully seems to be more in favor of 1 than 3, because of the maintenance. You are doing the work, so you can do what you feel is best and be the tie-breaker :).

If you do decide to go with 1, remember that in addition to the new Python executable we'll want a small update to the existing static_transform_broadcaster_program to say This utility is deprecated, use <new_executable> now.

allenh1 · 2021-03-12T19:34:11Z

If you do decide to go with 3, remember that in addition to the new Python executable

Do you mean 1? Just to be clear...

clalancette · 2021-03-12T19:43:44Z

Do you mean 1? Just to be clear...

Yes! Sorry. Will update the comment.

clalancette · 2021-09-16T17:24:00Z

@allenh1 I spent a bit of time here and reimplemented this to take care of all of the issues earlier identified, as well as adding support for both new-style and old-style arguments (the latter print a warning). I think this now fulfills all of the requirements we were looking for; can you take a look at the code and let me know what you think about it?

allenh1 · 2021-09-26T14:16:16Z

I think this now fulfills all of the requirements we were looking for; can you take a look at the code and let me know what you think about it?

@clalancette looks good to me. Thanks for finishing this up for me.

clalancette · 2021-09-27T12:36:28Z

@ahcorde When you get a chance, can you do a review of this? Thanks.

clalancette

This looks good to me, but since I rewrote substantial portions of it I'll wait on @ahcorde before merging it. I'll run CI on it in the meantime.

clalancette · 2021-10-04T22:08:25Z

CI:

Linux
Linux-aarch64
macOS
Windows

clalancette · 2021-10-05T11:38:39Z

Fixed some warnings on Windows and rebased. Here's another try at CI:

Linux
Linux-aarch64
macOS
Windows

clalancette · 2021-10-11T13:28:25Z

CI:

Linux
Linux-aarch64
macOS
Windows

ahcorde

just a mirnor style comment, otherwise LGTM

tf2_ros/src/static_transform_broadcaster_program.cpp

Signed-off-by: Hunter L. Allen <hunter.allen@ghostrobotics.io>

In this approach, we still allow the old command-line style, but it prints a warning and is deprecated. The new style is to use flags (like --x, --qx, etc), so that it is clear to the user which axes they are overriding. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette · 2021-10-12T19:06:36Z

CI:

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette · 2021-10-13T00:53:29Z

One more CI to take the test warnings into account:

Linux
Linux-aarch64
macOS
Windows
Windows Debug

clalancette · 2021-10-13T12:12:35Z

CI is green, approved, so merging. Thanks for the initial code @allenh1 , and thanks for the review @ahcorde .

allenh1 added the in progress Actively being worked on (Kanban column) label Mar 10, 2021

allenh1 force-pushed the 295-more-intuitive-command-line-arguments branch from 2b28c36 to df2315b Compare March 11, 2021 14:34

allenh1 requested a review from clalancette March 11, 2021 14:34

allenh1 added in review Waiting for review (Kanban column) in progress Actively being worked on (Kanban column) and removed in progress Actively being worked on (Kanban column) in review Waiting for review (Kanban column) labels Mar 11, 2021

clalancette requested changes Mar 11, 2021

View reviewed changes

clalancette self-assigned this Apr 1, 2021

clalancette added the more-information-needed Further information is required label Apr 1, 2021

clalancette force-pushed the 295-more-intuitive-command-line-arguments branch from df2315b to d45244f Compare September 16, 2021 17:22

clalancette removed in progress Actively being worked on (Kanban column) more-information-needed Further information is required labels Sep 24, 2021

clalancette approved these changes Oct 4, 2021

View reviewed changes

clalancette force-pushed the 295-more-intuitive-command-line-arguments branch 2 times, most recently from 9cf4e53 to c38fab3 Compare October 11, 2021 13:26

clalancette force-pushed the 295-more-intuitive-command-line-arguments branch from c38fab3 to eca4cf4 Compare October 11, 2021 18:31

ahcorde approved these changes Oct 12, 2021

View reviewed changes

tf2_ros/src/static_transform_broadcaster_program.cpp Outdated Show resolved Hide resolved

allenh1 and others added 3 commits October 12, 2021 19:05

Start implementing new argument parsing

b5bc1d6

Signed-off-by: Hunter L. Allen <hunter.allen@ghostrobotics.io>

Fixes from review.

e2d83c2

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette force-pushed the 295-more-intuitive-command-line-arguments branch from eca4cf4 to e2d83c2 Compare October 12, 2021 19:05

Fixes to get rid of warnings in tests.

6c64938

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette merged commit 254748f into ros2 Oct 13, 2021

delete-merged-branch bot deleted the 295-more-intuitive-command-line-arguments branch October 13, 2021 12:13

clalancette mentioned this pull request Apr 29, 2022

ROS 2 Humble needed release notes ros2/ros2_documentation#2446

Closed

87 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More Intuitive CLI for Static Transform Publisher #392

More Intuitive CLI for Static Transform Publisher #392

allenh1 commented Mar 10, 2021 •

edited by clalancette

Loading

allenh1 commented Mar 11, 2021

clalancette Mar 11, 2021

allenh1 Mar 11, 2021

clalancette Mar 11, 2021

allenh1 Mar 11, 2021

tfoote commented Mar 11, 2021

clalancette commented Mar 11, 2021

tfoote commented Mar 12, 2021

clalancette commented Mar 12, 2021

allenh1 commented Mar 12, 2021 •

edited

Loading

clalancette commented Mar 12, 2021 •

edited

Loading

allenh1 commented Mar 12, 2021

clalancette commented Mar 12, 2021

clalancette commented Sep 16, 2021

allenh1 commented Sep 26, 2021

clalancette commented Sep 27, 2021

clalancette left a comment

clalancette commented Oct 4, 2021

clalancette commented Oct 5, 2021 •

edited

Loading

clalancette commented Oct 11, 2021

ahcorde left a comment

clalancette commented Oct 12, 2021

clalancette commented Oct 13, 2021 •

edited

Loading

clalancette commented Oct 13, 2021

		rotation = _get_rotation(arg_map);
		translation = _get_translation(arg_map);

More Intuitive CLI for Static Transform Publisher #392

More Intuitive CLI for Static Transform Publisher #392

Conversation

allenh1 commented Mar 10, 2021 • edited by clalancette Loading

allenh1 commented Mar 11, 2021

clalancette Mar 11, 2021

Choose a reason for hiding this comment

allenh1 Mar 11, 2021

Choose a reason for hiding this comment

clalancette Mar 11, 2021

Choose a reason for hiding this comment

allenh1 Mar 11, 2021

Choose a reason for hiding this comment

tfoote commented Mar 11, 2021

clalancette commented Mar 11, 2021

tfoote commented Mar 12, 2021

clalancette commented Mar 12, 2021

allenh1 commented Mar 12, 2021 • edited Loading

clalancette commented Mar 12, 2021 • edited Loading

allenh1 commented Mar 12, 2021

clalancette commented Mar 12, 2021

clalancette commented Sep 16, 2021

allenh1 commented Sep 26, 2021

clalancette commented Sep 27, 2021

clalancette left a comment

Choose a reason for hiding this comment

clalancette commented Oct 4, 2021

clalancette commented Oct 5, 2021 • edited Loading

clalancette commented Oct 11, 2021

ahcorde left a comment

Choose a reason for hiding this comment

clalancette commented Oct 12, 2021

clalancette commented Oct 13, 2021 • edited Loading

clalancette commented Oct 13, 2021

allenh1 commented Mar 10, 2021 •

edited by clalancette

Loading

allenh1 commented Mar 12, 2021 •

edited

Loading

clalancette commented Mar 12, 2021 •

edited

Loading

clalancette commented Oct 5, 2021 •

edited

Loading

clalancette commented Oct 13, 2021 •

edited

Loading