Buffer server service

[tonynajjar]

This is a rough attempt at exposing a service in the buffer server as requested in #445

[tonynajjar]

@clalancette any update here? I think this would be a significant performance boost for a lot of big projects using many TF listeners

[clalancette]

@tonynajjar I'm sorry for the extremely long time to get back to review this.

Overall, I like the concept of making this transition. I've left a bunch of fixes inline, and I also have some high-level comments:

1. I think we should print a warning when the user uses the BufferServer action the first time. That should give them some warning that this will eventually be changing.
2. I think there should be a way to disable the action server. I'll suggest maybe a constructor parameter enable_lookup_action that defaults to true. When we remove the action server in K-Turtle, we can set that to false by default. I'm also open to other ideas here.

[tonynajjar]

I think we should print a warning when the user uses the BufferServer action the first time. That should give them some warning that this will eventually be changing.

I would simply add a RCLCPP_WARN_ONCE in the goalCB, what do you think?

I think there should be a way to disable the action server. I'll suggest maybe a constructor parameter enable_lookup_action that defaults to true. When we remove the action server in K-Turtle, we can set that to false by default. I'm also open to other ideas here.

Can you elaborate why this is necessary? When we remove the action server in K-Turtle there won't be anything to disable, or am I misunderstanding you?

[clalancette]

I've left some more fixes inline.

I think there should be a way to disable the action server. I'll suggest maybe a constructor parameter enable_lookup_action that defaults to true. When we remove the action server in K-Turtle, we can set that to false by default. I'm also open to other ideas here.

Can you elaborate why this is necessary? When we remove the action server in K-Turtle there won't be anything to disable, or am I misunderstanding you?

Regarding this question, what I was thinking about here was that creating both an action server and a service server is putting additional load on the network (particularly discovery load). For those users that know that they only want to use the new service-based interface, they could disable the action one to save themselves some discovery traffic. That said, I could be convinced that we don't need to do this.

I also realized that we should update the BufferClient to use the new service interface by default. That way most users won't notice the change at all.

[clalancette]

Suggested change

	RCLCPP_WARN_ONCE(logger_, "You are using the deprecated action interface of the tf2_ros::BufferServer. \
	Please use the service interface instead.");
	RCLCPP_WARN_ONCE(
	logger_,
	"You are using the deprecated action interface of the tf2_ros::BufferServer. "
	"Please use the service interface instead.");

[tonynajjar]

Regarding this question, what I was thinking about here was that creating both an action server and a service server is putting additional load on the network (particularly discovery load). For those users that know that they only want to use the new service-based interface, they could disable the action one to save themselves some discovery traffic. That said, I could be convinced that we don't need to do this.

I'm still a bit confused about the need of keeping both. Here's what I propose:

In this PR, we completely replace the action interface with a service interface for both server and clients without breaking anything for most users using them "normally".

What do you think? Once this is decided I can proceed with buffer clients, tests, etc...

[clalancette]

In this PR, we completely replace the action interface with a service interface for both server and clients without breaking anything for most users using them "normally".

It's hard to say. I don't know how many users directly connect to the action server here without using the BufferClient. If that number is ~0, then we can do this. But I generally don't like to pull the rug out from users who still might be using the old interfaces.

[tonynajjar]

My view is that the intended way to use the BufferServer was together with a BufferClient so there was never a guarantee that the underlying action server will remain the same. Having an official tutorial would have helped justify this point but unfortunately I can't find any.

I can ask on the Discourse if anyone uses the action server directly if that helps. In any case, I've given my thoughts, I'd appreciate if you can come to a decision so I can implement it, as I still have only the next 2 weeks to work on contributing back this refactoring that we did internally.

[clalancette]

I can ask on the Discourse if anyone uses the action server directly if that helps. In any case, I've given my thoughts, I'd appreciate if you can come to a decision so I can implement it, as I still have only the next 2 weeks to work on contributing back this refactoring that we did internally.

The more I look at this, the more confused I get.

I downloaded all of the releases source code for both Humble and Rolling. And within it, I can't find a single reference to BufferServer, BufferClient, or LookupTransform that isn't from this repository. That seems to suggest that not only are there no direct callers to the action, that there are no users of this code at all.

What am I missing here? Where is this code used, and how is it having extra overhead if there are no callers to it anywhere that I can find?

[tonynajjar]

Very interesting....... We use it extensively in our codebase to avoid each node having its own Buffer as this in itself has overhead. I guess this makes sense mostly when you have a project with many nodes needing access to transforms.
Only a speculation but I guess released packages do not have this need as they are not "example application packages". Therefore the creation of a transform listener/Buffer doesn't have much overhead. Also there is no official tutorial on how to use the buffer server/client so it won't be that popular.
One more thing, if a released package uses the buffer client, it adds a requirement for the user to launch a buffer server; it's simpler to create a TF listener and be self sufficient.

The combination of these reasons might justify why you can't find it. It doesn't mean it's not useful for projects with many tf-hungry nodes!

[gavanderhoorn]

Just to add a 👍 to @tonynajjar's comment:

We use it extensively in our codebase to avoid each node having its own Buffer as this in itself has overhead

adding listeners 'everywhere' results in a significant amount of overhead which can be avoided by using a single buffer_server instance. Especially if a node only really needs to occasionally lookup a transform.