Added support for clients and services #146

esteve · 2022-05-02T13:42:46Z

This PR adds support for clients and services. Includes a very primitive implementation of a Future which will surely evolve once we support the executor API (see #126).

i-am-chauhan · 2022-05-12T05:02:04Z

Hi @esteve , My name is Shubham Chauhan. My team is using ros2 in our project. We are considering to use rust client for writing ros2 nodes. Currently, we are using CPP client. So I was looking into this repo. Although we found out it doesn't support services and clients yet. But as I can see, that will be supported once this PR has been merged.

So could you tell us when this will be merged in the master and be officially supported? And if we can help you with anything, please let us know.

nnmm · 2022-05-29T11:54:30Z

@esteve Could you rebase this onto master? Also, what design decisions did you make along the way?

nnmm · 2022-05-30T23:10:26Z

@esteve Could you rebase this onto master? Also, what design decisions did you make along the way?

Actually, nevermind with rebasing the whole thing - I think it would be better to open separate PRs for message generation and service (without client) first.

esteve · 2022-06-02T11:18:00Z

@nnmm I've rebased anyway, I'd rather get all the changes in, generating clients and services by itself won't be of much use.

esteve · 2022-06-02T11:19:28Z

@i-am-chauhan this should be almost ready for merging. In any case, your team can help by trying out this branch (or the rest of ros2-rust), we always welcome feedback!

nnmm · 2022-06-02T11:21:42Z

@esteve I was thinking that splitting it up into PRs would make it easier to review. This PR will gain many comments otherwise, since it's so large, which could get hard to keep an overview of. You can also make the new modules private if you don't want to have the partial features exposed on master.

esteve · 2022-06-02T12:47:38Z

@nnmm I see what you mean and I'd agree, but given that messages are in sync with the API, splitting them wouldn't help much, so I prefer to keep both in the same PR to make sure that any changes in either side match.

nnmm · 2022-06-02T16:02:56Z

Great to see this PR moving forward :)

rclrs/src/future.rs

rosidl_generator_rs/resource/srv.rs.em

rosidl_runtime_rs/src/traits.rs

i-am-chauhan · 2022-06-13T10:46:16Z

Hi @esteve , couple of days ago, I was trying to write some service nodes using this. Things were working then. But when I upgraded to latest of this branch, It's not compiling. And I can see there is lot of changes being made here. So I guess I'll have to wait till it's a bit stable.

esteve · 2022-06-13T10:53:19Z

@i-am-chauhan the latest commit should fix all the compilation issues, there will be more internal cleanups, but I don't expect the API to change much. Could you give it a try again? Thanks!

mxgrey

Apologies for the long delay in reviewing this. I finally had some spare time to look it over.

Getting services into ros2-rust is super exciting! I have some feedback on the callback signatures which loosely amounts to "I wish rclcpp had done it this way from the start, so maybe we can have ros2-rust do things in a 'better' (subjective, I know) way from the start".

If my suggestions sound interesting I'll be happy to try making the changes and submit a PR that targets this one.

rclrs/src/node/client.rs

rclrs/src/node/service.rs

esteve · 2022-06-15T09:57:06Z

@mxgrey

have some feedback on the callback signatures which loosely amounts to "I wish rclcpp had done it this way from the start, so maybe we can have ros2-rust do things in a 'better' (subjective, I know) way from the start".

As the guy who implemented services in rclcpp (ros2/rclcpp#7), I actually agree with you 😄 Unfortunately at the time, we couldn't change the API too much from ROS1, we were already in deep trouble with a part of the community by proposing a new rewrite of ROS. Having said that, I'd like for ros2-rust not to deviate too much from rclcpp and rclpy, but at the same time take advantage of the possibilities that Rust offers.

This PR is meant to be a first stake at clients and services, with the bare minimum implemented to make it useful for users and to finally make a first release of ros2-rust on crates.io So I'd prefer any extra features go in a follow-up PR (like for example asynchronouse services via deferred callbacks, which I think would be super useful, don't get me wrong).

mxgrey · 2022-06-15T10:41:10Z

Makes sense, I'll be happy to do a follow-up PR in the future 👍

esteve · 2022-07-05T11:49:35Z

All feedback addressed, thanks everyone! Please have another look.

If there any improvements, but are not super urgent, please open follow up tickets, I'd like to merge this soon and wrap up a new release.

nnmm · 2022-07-05T12:04:57Z

All feedback addressed, thanks everyone! Please have another look.

If there any improvements, but are not super urgent, please open follow up tickets, I'd like to merge this soon and wrap up a new release.

Great! I agree we're really close to merging this. I commented on two threads where I think changes are needed.

esteve · 2022-07-05T12:19:29Z

@nnmm thanks for your feedback! I've applied the // change for the comments. Let's open a ticket to revisit the Fn / FnMut discussion.

nnmm · 2022-07-05T13:07:30Z

Could you also add clients/services it to the list of features in README.md?

jhdcs

Just a quick question about a comment.

rosidl_generator_rs/resource/srv.rs.em

i-am-chauhan · 2022-07-05T15:41:15Z

@i-am-chauhan

Hi @esteve , I can add this feature in client and give you the PR if you want.

That sounds really cool, thanks! It won't be included in this PR since we want to make a first release of rclrs soon and will wait until this PR is merged for that.

However, not to discourage you, but adding a function to wait for a service to be available implies adding support for accessing the ROS graph, which requires many parts to be implemented (e.g. guard conditions, most likely a refactor of the executor API, etc.)

I'll be happy to guide you through the process, so feel free to start working on it and I'll give you feedback.

Hi @esteve , I have created a PR for this with minimal changes. Please have a look and let me know if something needs to change.

examples/minimal_client_service/Cargo.toml

nnmm

Did my final review, only some nitpicks remaining.

examples/minimal_client_service/package.xml

examples/minimal_client_service/Cargo.toml

rclrs/src/node/client.rs

rclrs/src/node/service.rs

nnmm · 2022-07-05T17:00:14Z

rclrs/src/node/client.rs

+            err,
+            s: topic.into(),
+        })?;
+        let rcl_node = { &mut *node.rcl_node_mtx.lock() };


Suggested change

let rcl_node = { &mut *node.rcl_node_mtx.lock() };

let rcl_node = &mut *node.rcl_node_mtx.lock();

I can't find the thread, but this was changed based on @jhdcs 's suggestion to not lock the Node's mutex for the entirety of the function and only for the handle instead. We should revisit the other handles in the codebase to make sure we're not locking too much.

I agree with the idea, but I believe as long as you hold the mutable reference, the mutex stays locked.

nnmm · 2022-07-05T17:07:48Z

rclrs/src/node/client.rs

+            }
+            Err(e) => return Err(e),
+        };
+        let requests = &mut *self.requests.lock();


No need to change anything, just a remark: The &mut * to convert the MutexGuard into a reference is mostly just needed when passing something to an rcl function, here and in a few other places we can also write self.futures.lock().remove().

rclrs/src/node/client.rs

rclrs/src/node/service.rs

Co-authored-by: Nikolai Morin <[email protected]>

nnmm

🚀🚀🚀🚀

nnmm · 2022-07-08T10:51:43Z

@esteve Don't you want to merge it? :)

esteve force-pushed the client-services branch 3 times, most recently from 27c535f to 7c6944a Compare May 2, 2022 14:12

esteve force-pushed the client-services branch 2 times, most recently from 8120cec to d85d298 Compare June 2, 2022 11:16

esteve force-pushed the client-services branch from d85d298 to 655dad6 Compare June 2, 2022 11:21

esteve force-pushed the client-services branch from 655dad6 to 84057df Compare June 2, 2022 12:40

esteve force-pushed the client-services branch 3 times, most recently from 575cd40 to 24eab94 Compare June 2, 2022 15:09

nnmm reviewed Jun 3, 2022

View reviewed changes

rclrs/src/future.rs Outdated Show resolved Hide resolved

nnmm reviewed Jun 3, 2022

View reviewed changes

rosidl_generator_rs/resource/srv.rs.em Show resolved Hide resolved

nnmm reviewed Jun 3, 2022

View reviewed changes

rosidl_runtime_rs/src/traits.rs Outdated Show resolved Hide resolved

esteve force-pushed the client-services branch 3 times, most recently from c0f2251 to 7765b3e Compare June 13, 2022 10:01

mxgrey reviewed Jun 15, 2022

View reviewed changes

rclrs/src/node/client.rs Outdated Show resolved Hide resolved

rclrs/src/node/service.rs Outdated Show resolved Hide resolved

Added support for clients and services

e744ca1

Added comment about why calling unwrap is safe

34bdc08

esteve added 2 commits July 5, 2022 14:12

Removed map_err

679b028

Use double slashes for comments for take_* functions

4191195

esteve added 2 commits July 5, 2022 14:30

Fix docstrings

e149424

Simplify unwrap_or call

e1b02d2

jhdcs reviewed Jul 5, 2022

View reviewed changes

rosidl_generator_rs/resource/srv.rs.em Show resolved Hide resolved

nnmm reviewed Jul 5, 2022

View reviewed changes

examples/minimal_client_service/Cargo.toml Outdated Show resolved Hide resolved

nnmm reviewed Jul 5, 2022

View reviewed changes

esteve and others added 6 commits July 6, 2022 12:06

Apply suggestions from code review

1c5064b

Co-authored-by: Nikolai Morin <[email protected]>

Removed dependency

d085a24

Removed dependency

a87318e

Removed dependency

051b828

Ignore result

bf61831

Added clients and services to the list of supported features

b59bb9a

nnmm previously approved these changes Jul 6, 2022

View reviewed changes

jhdcs previously approved these changes Jul 6, 2022

View reviewed changes

Added comment to service struct

51141b4

esteve dismissed stale reviews from jhdcs and nnmm via 51141b4 July 6, 2022 13:01

nnmm approved these changes Jul 6, 2022

View reviewed changes

jhdcs approved these changes Jul 6, 2022

View reviewed changes

esteve merged commit 58dd04a into main Jul 8, 2022

delete-merged-branch bot deleted the client-services branch July 8, 2022 12:43

Tacha-S mentioned this pull request Oct 20, 2022

Add support for actions #244

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for clients and services #146

Added support for clients and services #146

esteve commented May 2, 2022 •

edited

Loading

i-am-chauhan commented May 12, 2022 •

edited

Loading

nnmm commented May 29, 2022

nnmm commented May 30, 2022

esteve commented Jun 2, 2022

esteve commented Jun 2, 2022

nnmm commented Jun 2, 2022 •

edited

Loading

esteve commented Jun 2, 2022

nnmm commented Jun 2, 2022

i-am-chauhan commented Jun 13, 2022 •

edited

Loading

esteve commented Jun 13, 2022

mxgrey left a comment

esteve commented Jun 15, 2022

mxgrey commented Jun 15, 2022

esteve commented Jul 5, 2022

nnmm commented Jul 5, 2022

esteve commented Jul 5, 2022

nnmm commented Jul 5, 2022

jhdcs left a comment

i-am-chauhan commented Jul 5, 2022 •

edited

Loading

nnmm left a comment

nnmm Jul 5, 2022

esteve Jul 6, 2022

nnmm Jul 6, 2022

nnmm Jul 5, 2022

nnmm left a comment

nnmm commented Jul 8, 2022

	let rcl_node = { &mut *node.rcl_node_mtx.lock() };
	let rcl_node = &mut *node.rcl_node_mtx.lock();

Added support for clients and services #146

Added support for clients and services #146

Conversation

esteve commented May 2, 2022 • edited Loading

i-am-chauhan commented May 12, 2022 • edited Loading

nnmm commented May 29, 2022

nnmm commented May 30, 2022

esteve commented Jun 2, 2022

esteve commented Jun 2, 2022

nnmm commented Jun 2, 2022 • edited Loading

esteve commented Jun 2, 2022

nnmm commented Jun 2, 2022

i-am-chauhan commented Jun 13, 2022 • edited Loading

esteve commented Jun 13, 2022

mxgrey left a comment

Choose a reason for hiding this comment

esteve commented Jun 15, 2022

mxgrey commented Jun 15, 2022

esteve commented Jul 5, 2022

nnmm commented Jul 5, 2022

esteve commented Jul 5, 2022

nnmm commented Jul 5, 2022

jhdcs left a comment

Choose a reason for hiding this comment

i-am-chauhan commented Jul 5, 2022 • edited Loading

nnmm left a comment

Choose a reason for hiding this comment

nnmm Jul 5, 2022

Choose a reason for hiding this comment

esteve Jul 6, 2022

Choose a reason for hiding this comment

nnmm Jul 6, 2022

Choose a reason for hiding this comment

nnmm Jul 5, 2022

Choose a reason for hiding this comment

nnmm left a comment

Choose a reason for hiding this comment

nnmm commented Jul 8, 2022

esteve commented May 2, 2022 •

edited

Loading

i-am-chauhan commented May 12, 2022 •

edited

Loading

nnmm commented Jun 2, 2022 •

edited

Loading

i-am-chauhan commented Jun 13, 2022 •

edited

Loading

i-am-chauhan commented Jul 5, 2022 •

edited

Loading