tf_listener and lookup_transform in roslibrust

[lucasw]

It needs additional features, testing, and cleanup but I made a roslibrust version of rustros_tf/tf_rosrust: https://github.com/lucasw/rustros_tf/tree/roslibrust, additional description is here: smilerobotics/tf_rosrust#55

There's also a node (in addition to echo.rs) that uses it here: https://github.com/lucasw/tf_demo/blob/old_tf_to_new_tf/src/bin/tf2tf.rs - you can look up any tf then publish the relationship as a new transform with any parent child desired. The python and C++ versions have additional features to modify parts of the found transform, zero out rotations or translations on any axis- I'll add that in later.

Having the equivalent of dynamic reconfigure (actually dynamic-dynamic reconfigure) would be nice but I can live without it, I can have a workflow of using the python or C++ version initially for experimentation then swapping in the rust node when the parameters are set. A replacement wouldn't need to use ros parameters/topics/services just a lightweight way to tell the node to change parameter values with the possibility of a gui front end in another process.

It looks comparable in performance to the C++ version so far.

Any suggestions are very welcome.

[Carter12s]

Very cool!! This is an awesome example to have of some real world usage of roslibrust outside of the silo I've been using it for and shows some areas where we have substantial work to continue to improve the usability and API of the crate. If you have additional perspectives on usability I'd love to hear them. Below are some of my thoughts upon reviewing the code:

1. We are causing users a ton of pain with the namespace and leading slash stuff. The original rosbridge implementation this crate was built on was very lenient on what it took in as a node name or topic name, and just passed them on to rosbridge. We introduced some checks and special types when we did the ROS1 native implementation, but they aren't rolled out inside the crate well yet. Providing a better builder API for creating a valid resource name (similar to PathBuf) and standardizing on that throughout the crate would be a big win.

This is pain we should be able to remove:

let full_node_name = &format!("/{ns}/tf2tf").replace("//", "/");

2. I personally hate ros1's use of environment variables and dearly wish they had made different design choices. I decided to make roslibrust not respect the ROS environment variables by default as I consider that "non-rust like behavior". However, it is an annoyance our users have to work around. NodeHandle should probably have a "::new_from_env" constructor that looks up ROS_MASTER_URI for the user (possibly also parsing command line arguments for node name?)

3. It is really hard to figure out how to deal with errors correctly...

let _dynamic_subscriber = nh.subscribe::<TFMessage>("/tf", 100).await.unwrap();
let _static_subscriber = nh.subscribe::<TFMessage>("/tf_static", 100).await.unwrap();

The rust side of me flinches at these unwraps, but frankly that is probably a more sane thing to do than most C++ ROS nodes do when communication to the ROS master fails. It shouldn't be hard to build systems with robust reconnection logic, but ROS's general style for C++ and Python has people not think about this, and you can make a really good case for that.

Dealing with complex networking scenarios where things are going in and out of communication makes code fundamentally harder to write, and probably shouldn't be what "example code shows". But I do want the API of the crate to lead to the Rust "pit of success", as it stands I think the vast majority of users will end up just calling .unwrap() on the fact that an subscribe() call can fail...

I'm toying with a lot of ideas of how to make handling the possible errors that can happen with ROS communication internally to the crate, and making more of the external API infallible, but hard to do this in ways that I can be confident will work for everybody.

I'd like to steer the API / Behavior of roslibrust towards:

• A NodeHandle level operation (subscribe, advertise, etc.) never fail, they are always queued, and the crate will continuously retrying those operations under the hood if the fail (unless the future is canceled).
• .next() and .publish() should never fail but be retried internally and call_service() should only fail if the underlying service itself reports failure.
• NodeHandle should have a method like ".register_error_handler()" that provides access to a stream of "Node Level" errors like loss of communication with RosMaster, individual TCP Sockets going bad, timeouts, or serialization errors in the underlying protocols.

This API is slightly worse in some objective ways, as not handling the errors at the callsite of the triggering event means you lose context about what was happening. An example of this would be having a node where in multiple places you are publishing to the same topic, but it is only a serious problem when failures occur at specific locations.

However, I think this API would make users of the crate significantly more likely to implement good error handling in their nodes.... Sorry long rant.

4. The chrono TimeDelta workaround you had to do is a PITA and I've had to work around it as well. We have some stub code for providing conversions to and from chrono types, but haven't followed up on that work. I think it should be an obvious win to have the fundamental time type of roslibrust's message generation be based on chrono.

5. We need to provide support for parameters and dynamic reconfigure!

6. Tiny style nit on your example code, but I've been working to break the habit of using tokio::select! as it tend to lead to some giant functions with too much nesting. Takes a decent amount of work to refactor code around using, and I wouldn't use it in an example as I'd be afraid that I'm leading people to write an entire Node inside one select! statement.

[lucasw]

What's a good non-select pattern to use for multiple subscribers feeding data into something else that needs use the updates and publish regularly- spawn a thread for each one, and channels between them?

[Carter12s]

Yeah I think getting used to using tokio::spawn() to create tasks / actors for things, and then connecting those actors together a combination of channels or Arc<Mutex> is generally the right way to steer people.

If you are going to use tokio::select!, I try to put as little code as possible actually in the branch arms as the code gets unreadable quickly otherwise. Additionally, select! cancels the other branch arms when one arm completes which can be really un-intuitive if you've not dealt with futures that aren't cancel safe before. For example I often end up using read_exact() within tokio io which is NOT cancel safe: https://docs.rs/tokio/latest/tokio/io/trait.AsyncReadExt.html#method.read_exact

[Carter12s]

I also said it at the top of my previous message, but I'll say it again. Thank you!