@clalancette do you think it would be worth using this in the static_transform_publisher_program.cpp, or leave that as is?

Definitely need to add some tests still.

@clalancette / @tfoote Could one of you please spin up CI for me?

I saw there are already tests for static transform publisher, and just refactored the existing one to use the component. Tests pass locally, at least.

Let me know what you think!

@tfoote since these are new files, should I license them as Apache-2.0?

For consistency with the code base please license them under BSD-3 clause. Otherwise you end up requiring double due diligence for the package.

I have't had a chance to read through it yet, but kicking off CI to see the results.

• Linux
• Linux-aarch64
• macOS
• Windows

I haven't had a chance to read through it yet, but kicking off CI to see the results.

@tfoote thanks, found an unfortunate mistake in my use of the logger macros.

Fixed now, sorry I failed to notice that previously.

From here:

We may have to think about using parameters instead here.

My concern with parameters is how that would be done in a system with several of these components. If they all use something like "linear_x" for the name, how does one keep the parameters from getting clobbered by other static transform setups?

I'm really not sure how to support the generic component usage.

My concern with parameters is how that would be done in a system with several of these components. If they all use something like "linear_x" for the name, how does one keep the parameters from getting clobbered by other static transform setups?

The individual nodes would have to be named differently or namespaced differently, which would solve that problem. Even though we don't currently enforce it, you really want to do that anyway, since there is currently no other way to uniquely identify a node.

The individual nodes would have to be named differently or namespaced differently, which would solve that problem

So I should probably take the name parameter and append it so it would look like this?

/[name]/translation/x
/[name]/translation/y
/[name]/translation/z
/[name]/rotation/x
/[name]/rotation/y
/[name]/rotation/z
/[name]/rotation/w

So I should probably take the name parameter and append it so it would look like this?

Honestly, I'd get rid of the name parameter altogether. This is no different than if you try to run multiple of any kind of node; you need to uniquely name them one way or another.

Honestly, I'd get rid of the name parameter altogether

Okay, so, to make sure I understand, you want the above but without the leading /name, which would be remapped by the user.

@clalancette I have not declared the parameters yet, but I'd like you to take a look at things to make sure we're still on the same page if you have a minute.

@clalancette I have not declared the parameters yet, but I'd like you to take a look at things to make sure we're still on the same page if you have a minute.

So I do like the declare_parameters; that's nice.

The ability to dynamically change the parameters on the fly is neat, but I actually don't think it belongs here. That is, this is the static transform broadcaster, so being able to change it on the fly gives me cognitive dissonance. It also makes it more complicated for an unclear use-case. So I'd suggest not installing the parameter_callback at all, and just declaring the parameters at constructor time.

The ability to dynamically change the parameters on the fly is neat, but I actually don't think it belongs here.

Yeah, that's a good point.

So I'd suggest not installing the parameter_callback at all, and just declaring the parameters at constructor time.

As I'm frankly a bit out of the loop here, I'm not quite sure what the "proper" way to do that would be. Is this still get_parameter or is there a new mechanism? Maybe this?

As I'm frankly a bit out of the loop here, I'm not quite sure what the "proper" way to do that would be. Is this still get_parameter or is there a new mechanism? Maybe this?

Personally I would just take your declare_parameter lines (lines 57 - 65), move them to the top of the constructor, and mark them as "read-only" (something like https://github.com/ros2/rclcpp/blob/2716d3e81ed1bfd8b1afe8d07bdeda7d3a4ea61b/rclcpp/test/test_node.cpp#L315). That way the parameters are still introspectable, but can't be changed.

Personally I would just take your declare_parameter lines (lines 57 - 65), move them to the top of the constructor, and mark them as "read-only"

Done! Tested and working (at least locally).

@tfoote / @clalancette please review and or spin up CI for me (when you are able)!

@allenh1 Why the changes in 15a8df9? I'm pretty sure the node was already being installed into lib, and it is not a library so doesn't need to be exported.

@allenh1 Why the changes in 15a8df9? I'm pretty sure the node was already being installed into lib, and it is not a library so doesn't need to be exported.

@clalancette it was being installed into lib, but I wanted to use the component in a different package, which could not find the library. On windows, I could find the .dll file, but I could not find the .lib file.

Is this an invalid use case for components? If so I can just drop that commit.

@clalancette it was being installed into lib, but I wanted to use the component in a different package, which could not find the library. On windows, I could find the .dll file, but I could not find the .lib file.

No, sorry, I was just getting confused with the names. Sorry about that. That change is fine.

@clalancette While we're waiting on the branch, I'd like to bring up an observation from using this.

I get a warning in the console when I use multiple instances of this component due to the name of the node being duplicated.

Should I add a static counter to the class that will increment when the constructor is called, then append the counter to the name of the node? I think that makes the node names look rather silly, but would solve this issue.

Does this have to do with the TODO about anonymizing the name of the node?

@clalancette / @tfoote rebased on top of ros2 -- should CI be run on this one more time?

@clalancette since the eloquent branch is now available and this is rebased on the tip of ros2, is there anything else I should wait for before hitting that squash and merge?

Maybe one more round of CI since I'm paranoid (which you'll need to start for me, of couse ;) ).

@allenh1 Now that we are past freeze, we can reconsider this one.

First, do you mind squashing all of this development into a single commit? It's pretty messy to look at (a lot of that is my fault, but we may as well fix it now).

Second, I think your point about the name of the node is a really good one. Yes, that is part of what anonymizing the name would buy us. We should think about how to fix it before we merge this, as that will bite people pretty easily.

The problem with adding a static counter to the class is that it only helps for this process. If you launch two processes, and they each have a static_transform_publisher in them, they'll still overlap. One way to go about this (without true anonymous name support) is to generate a UUID for the node and suffix that onto the name. See https://github.com/ros2/rclcpp/blob/e494b3efada0258845e41ca866c5baadc8c9a83c/rclcpp_action/src/client.cpp#L370 , where we do something similar for actions. I'll propose we do something similar for now, and we can always update it later to do something smarter if we get true anonymous node support.

It's pretty messy to look at (a lot of that is my fault, but we may as well fix it now).

It was quite a bit more iteration than I was also anticipating, haha. Squashed. Now we can pretend we just got it right a lot earlier on ;).

The problem with adding a static counter to the class is that it only helps for this process

That's a valid point.

One way to go about this (without true anonymous name support) is to generate a UUID for the node and suffix that onto the name.

I also think this is the best approach (at least for the time being). I'll get to that soon.

@clalancette could you spin up CI for me please?

@clalancette could you spin up CI for me please?

Sure! Here you go:

• Linux
• Linux-aarch64
• macOS
• Windows

Sure! Here you go

@clalancette Hm... Seems something is not quite right. I ran colcon test --packages-select tf2_ros locally, then colcon test-result and got the following on my machine:

G:\dashing_ws>colcon test-result
c:\python37\lib\site-packages\colcon_cmake\test_result\ctest.py:62: DeprecationWarning: This method will be removed in future versions.  Use 'list(elem)' or iteration over elem instead.
  children = root.getchildren()
Summary: 42 tests, 0 errors, 0 failures, 0 skipped

Did I need to run a different test?

@clalancette Hm... Seems something is not quite right. I ran colcon test --packages-select tf2_ros locally, then colcon test-result and got the following on my machine:

I'm not sure why you are getting a different result than CI. However, I will point out that the tests that failed in CI here also failed on the nightlies: https://ci.ros2.org/view/nightly/job/nightly_linux_debug/1398/#showFailuresLink . So it's not the fault of this PR.

At this point, I'm pretty good with this. I'll let it sit until Monday (I'm pretty busy today anyway), but if I don't get other opinions by then I'll merge it.

I'll let it sit until Monday (I'm pretty busy today anyway), but if I don't get other opinions by then I'll merge it.

🎉 Thanks!

I'm not sure why you are getting a different result than CI.

Me either... Just a classic "works on my machine" I suppose.