Add function to get non-ROS arguments #210

Nizerlak · 2022-06-27T21:19:49Z

Hi guys,
This is my first PR in this repo.
I faced an issue during creating this PR. In order to obtain non-ROS args, the input args (passed from console on node startup) has to be known, which is possible only during Context initialization (new function). My solution was to get those non-ROS arguments and store in new field of Context - non_ros_arguments - which could be accessed by getter function. Unfortunately it generated conflict with lib.rs. I made a temporary fix, but I think that, a design of Node and/or Context should be revised to resolve this conflict.

Nizerlak · 2022-06-28T21:31:00Z

Clone implementation for Context would resolve described problem. Also it could make mentioned lib.rs fragment more elegant (call for clone instead of building struct with calls to struct's fields clone anyway).

jhdcs

I have a few safety concerns, especially the use of Vec::from_raw_parts... Could you please take a look at the suggested changes and address them?

Otherwise, this looks like a good start! Hopefully I haven't missed anything. And thank you for your contribution!!

rclrs/src/context.rs

nnmm · 2022-07-02T12:11:20Z

@Nizerlak Thanks for the PR, and you're right about the issue in lib.rs. That is a bit hacky and needs to be refactored.

@jhdcs @esteve I think the cleanest solution is to add a &Context argument to the spin functions. Are you fine with that?

esteve · 2022-07-02T12:34:15Z

@jhdcs @esteve I think the cleanest solution is to add a &Context argument to the spin functions. Are you fine with that?

I'd void that as much a possible, after all, Nodes have a reference to Context, so adding a Context is redundant. How does rclcpp tackle this?

nnmm · 2022-07-02T13:02:59Z

I'd void that as much a possible, after all, Nodes have a reference to Context, so adding a Context is redundant. How does rclcpp tackle this?

rclcpp uses the implicit global context.

Nodes don't have a reference to Context but only to the rcl context_t. If we want them to have a full Context, we need to make Context::new return an Arc or possibly Arc<Mutex<_>> – that's also possible but a little ugly/inefficient.

A third way is to have a private version of WaitSet::new() that takes an rcl_context_t instead of a &Context. Then we don't need a full Context inside of lib.rs. Thinking about it, that's probably the best way.

Nizerlak · 2022-07-02T13:23:06Z

@jhdcs @esteve I think the cleanest solution is to add a &Context argument to the spin functions. Are you fine with that?

I'd void that as much a possible, after all, Nodes have a reference to Context, so adding a Context is redundant. How does rclcpp tackle this?

So maybe refactor WaitSet to receive in new rcl context mutex? Here it is unpacking inner mutex from context anyway so here it could be passed directly without wrapping with Context.

In rclcpp i can see that wrapped Context is passed, but it doesn't hold information about unparsed argumets.

The only way they're dealing with unprased arguments is throwing an exception here in case of bad ROS arguments (equivalent of ours #167). In order to get non-ROS arguments they retrieve arguments structure from Node and call rcl function directly here. Maybe we should also utilize this approach and deal with non-ROS arguments independently from Context creation.

Nizerlak · 2022-07-02T14:05:29Z

Also, I have a question about workflow in this repo. While applying fixes to this PR shall I

make fix commits
or
rebase with corresponding commits edition and force push?

nnmm · 2022-07-02T14:12:36Z

Also, I have a question about workflow in this repo. While applying fixes to this PR shall I
1. make fix commits
   or

2. rebase with corresponding commits edition and force push?

Good question, we should document this. I think it's fine to force-push in the very early stages (e.g. you noticed that CI isn't passing), but generally fix commits are a little easier for reviewers. We squash commits when merging anyway.

nnmm · 2022-07-02T14:16:39Z

So maybe refactor WaitSet to receive in new rcl context mutex? Here it is unpacking inner mutex from context anyway so here it could be passed directly without wrapping with Context.

That is basically what I'm suggesting with my comment "A third way is to have a private version of WaitSet::new() that takes an rcl_context_t instead of a &Context." We will need two versions: pub new() like it is now, and new_from_rcl_context() (not pub) that takes rcl_context_t. Public APIs should not have the rcl types in it. new() can then unpack the rcl_context and call new_from_rcl_context().

Nizerlak · 2022-07-04T17:44:02Z

I've implemented another approach. non_ros_arguments is member function of Context and should be called with program input arguments. Possible problem may occur, when user calls this function with wrong input_args for some reason. However, problem with holding arguments as Context member is resolved.

Nizerlak · 2022-07-04T18:20:02Z

Also, I've generalized getting rcl_arguments, which may be useful while resolving #167, since identical steps to obtain ROS unparsed arguments would be performed as for non-ROS ones.

jhdcs

Sorry for the delay - it was a long weekend over here.

jhdcs · 2022-07-05T12:20:43Z

rclrs/src/rcl_utils.rs

+        rcl_args: *const rcl_arguments_t,
+        allocator: rcl_allocator_t,
+        out_ptr: *mut *mut ::std::os::raw::c_int,
+    ) -> rcl_ret_t;


Is it possible/desirable to have this return a Result instead? Akin to how we use .ok()? for most of our calls to rcl?

It was intended to be just a wrapper around corresponding rcl_argument_get... and rcl_argument_get_count..., so i left the interface repeated.

rclrs/src/rcl_utils.rs

jhdcs · 2022-07-05T12:33:06Z

rclrs/src/rcl_utils.rs

+        // SAFETY: No preconditions for next 2 functions.
+        let allocator = rcutils_get_default_allocator();
+        allocator.deallocate.unwrap()(indices_ptr as *mut c_void, null_mut());


Suggested change

// SAFETY: No preconditions for next 2 functions.

let allocator = rcutils_get_default_allocator();

allocator.deallocate.unwrap()(indices_ptr as *mut c_void, null_mut());

// SAFETY: No preconditions for this function.

let allocator = rcutils_get_default_allocator();

// SAFETY: No preconditions for this function.

allocator.deallocate.unwrap()(indices_ptr as *mut c_void, null_mut());

Additionally, is there a way to re-write this to avoid the unwrap()?

I don't see a reason why unwrap could return None here, since allocator is retrieved by rcutils_get_default_allocator() just above. I could add another error type to RclrsError like I suggested here, but still I don't see a reason for that.

jhdcs · 2022-07-05T12:33:36Z

rclrs/src/rcl_utils.rs

+            // and is allocated with size equal to one returned by rcl_arguments_get_count_unparsed.
+            // SAFETY: No preconditions for this function.
+            let index = *(indices_ptr.add(i));
+            non_ros_args.push(args.get(index as usize).unwrap().clone());


Is there a way to re-write this to avoid the unwrap()?

Another way would be to add new error type to RclrsError, like IndexOutOfRange and raise it here if get returns None. What do you think about that?

rclrs/src/rcl_utils.rs

nnmm · 2022-07-05T12:45:41Z

I've implemented another approach. non_ros_arguments is member function of Context and should be called with program input arguments. Possible problem may occur, when user calls this function with wrong input_args for some reason. However, problem with holding arguments as Context member is resolved.

That's also a possibility, but I actually think our idea of constructing the wait set with an rcl_context_t is better, since (1) the end result is more user-friendly and (2) we'll probably run into the same problem of wanting to store something in Context soon again, so why not fix it properly now. @esteve @jhdcs let me know if you don't agree.

esteve · 2022-07-05T13:00:10Z

It seems to be that we're running in circles just to avoid what rclcpp did (an implicit context), it's not the cleanest solution, that's true, but all this going back and forth just in a way shows that perhaps rclcpp's solution is worth considering after all.

jhdcs · 2022-07-05T13:10:07Z

It seems to be that we're running in circles just to avoid what rclcpp did (an implicit context), it's not the cleanest solution, that's true, but all this going back and forth just in a way shows that perhaps rclcpp's solution is worth considering after all.

Perhaps we should discuss this in a separate issue? I foresee the potential for a very long discussion on the merits, and I don't want to hold up @Nizerlak's PR if it can be avoided. Especially if we're fundamentally restructuring rclrs underneath him from that discussion.

The work coming from that discussion should be a separate PR.

nnmm · 2022-07-05T13:57:56Z

It seems to be that we're running in circles just to avoid what rclcpp did (an implicit context), it's not the cleanest solution, that's true, but all this going back and forth just in a way shows that perhaps rclcpp's solution is worth considering after all.

I'm not against considering it in principle, but (1) we'd have the same problem here if we had a global context (since we'd still want the context instance associated with the node to be used in the wait set – it's the same in rclcpp) and (2) it's quite easy to fix.

esteve · 2022-07-06T15:14:33Z

@Nizerlak thank you so much for iterating with us. Here's an idea that we've discussed with @jhdcs and @nnmm, let us know what you think of it. How about a free function that just extracts the arguments from env::args()? Its usage would be like the following:

fn main() {
    let non_ros_args = rclrs::extract_non_ros_args(env::args());
    // do rclrs stuff, create Context, create Node, etc.
}

Most of the time, once the command line arguments have been parsed, there's no need to access them again. This has the advantage that we're not storing anything for the entire lifetime of a Node or a Context, just get the arguments we're interested in, and then execute the rest of the logic of our program. With a solution like this, we could even parse arguments and only initialize rclrs constructs after all has been processed by clap, and not before (e.g. ensure that all arguments are correct before doing anything). What do you think?

Nizerlak · 2022-07-07T13:15:21Z

I like that idea @esteve. With this approach there's no problem when user passes different arguments while calling Context::new and Context::non_ros_arguments and detaches non-ROS things (which are non-ROS args obviously) from ROS things - Context. I'll apply this suggestion as soon as possible.

rclrs/src/lib.rs

nnmm · 2022-07-11T13:05:44Z

@Nizerlak Is this PR ready for a review?

Nizerlak · 2022-07-11T13:12:49Z

Yes, I forgot to mention. Sorry for that.

nnmm · 2022-07-11T13:49:33Z

rclrs/src/lib.rs

+    if let Err(err) = ret {
+        // SAFETY: No preconditions for this function
+        unsafe {
+            rcl_arguments_fini(&mut rcl_arguments).ok()?;


I think it's safe to rely on this being done by rcl if rcl_parse_arguments() fails: https://github.com/ros2/rcl/blob/4eccc3cbe65f5b58981f22efaf612a65731cb2f0/rcl/src/rcl/arguments.c#L670

I.e. please remove this call.

rclrs/src/rcl_utils.rs

jhdcs

Just a question on how things are organized.

jhdcs · 2022-07-22T17:58:37Z

rclrs/src/rcl_utils.rs

+/// `rcl_arguments_get_count_unparsed_ros` -> `rcl_arguments_get_count_ros`
+/// ...
+pub(crate) fn get_rcl_arguments(
+    rcl_get_count: unsafe extern "C" fn(*const rcl_arguments_t) -> std::os::raw::c_int,


Is there a reason that we're passing these functions around? Couldn't we get much the same effect by having multiple outer functions that use the different API calls, and wrapping the common portions together in a different function?

Honestly I don't see how to wrap this common portions to separate functions. They're just too small and linked tightly to te rest. Maybe wrapping it into macro woud solve this, but it's a bit nasty and seems like overkill.
Current approach is on @nnmm request, so if there's still something wrong with it please agree on one version, since I'd like to start resolving other issues not being attached to this one for so long :/

Ah, I had missed that. My apologies.

Yes, that PR has gone on quite long, and I agree with @Nizerlak that no more common functions can be factored out.

Addressed by author

nnmm · 2022-07-23T06:09:07Z

I'm really happy with the code quality in this PR overall, most of my comments relate to the SAFETY comments which seem like they maybe seemed a bit pointless to you :) I encourage you to just be a bit creative with these, and write down notable comments about usage of this function even if there is nothing strictly safety-related to be said about it.

One request: Could you move everything into an arguments.rs module that is pub used by lib.rs?

And sorry about the long review time – the next one will be faster.

rclrs/src/rcl_utils.rs

nnmm · 2022-07-23T05:34:15Z

rclrs/src/lib.rs

+    if let Err(err) = ret {
+        // SAFETY: No preconditions for this function
+        unsafe {
+            rcl_arguments_fini(&mut rcl_arguments).ok()?;


I.e. please remove this call.

rclrs/src/rcl_utils.rs

rclrs/src/lib.rs

nnmm · 2022-07-23T06:03:08Z

rclrs/src/rcl_utils.rs

+/// `rcl_arguments_get_count_unparsed_ros` -> `rcl_arguments_get_count_ros`
+/// ...
+pub(crate) fn get_rcl_arguments(
+    rcl_get_count: unsafe extern "C" fn(*const rcl_arguments_t) -> std::os::raw::c_int,


Yes, that PR has gone on quite long, and I agree with @Nizerlak that no more common functions can be factored out.

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

rclrs/src/arguments.rs

nnmm · 2022-07-23T14:35:47Z

Thanks for making all the changes! Could you just also move the code from rcl_utils.rs into arguments.rs?

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

…arguments

nnmm

I think we're just one cargo fmt away from merging it.

nnmm

LGTM, thank you. I'm looking forward to what you'll tackle next :-)

Nizerlak changed the title ~~Add function to get non-ROS arguments #147~~ Add function to get non-ROS arguments Jun 27, 2022

jhdcs requested review from nnmm, esteve and jhdcs July 1, 2022 15:23

jhdcs requested changes Jul 1, 2022

View reviewed changes

Nizerlak mentioned this pull request Jul 3, 2022

Fix memory leaks #213

Closed

Nizerlak requested a review from jhdcs July 5, 2022 12:01

jhdcs requested changes Jul 5, 2022

View reviewed changes

Nizerlak commented Jul 8, 2022

View reviewed changes

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

Nizerlak requested a review from jhdcs July 9, 2022 07:45

nnmm reviewed Jul 11, 2022

View reviewed changes

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

UT repair

9898089

jhdcs previously requested changes Jul 22, 2022

View reviewed changes

jhdcs self-requested a review July 22, 2022 19:49

nnmm reviewed Jul 23, 2022

View reviewed changes

Nizerlak and others added 9 commits July 23, 2022 12:30

fix safety comment

d37a98c

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

fix safety comment

852470a

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

Update rclrs/src/rcl_utils.rs

33713da

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

Update rclrs/src/lib.rs

d6c9256

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

small fixes

8a54cce

unwrap instead of error

25020dc

fix cstring_args collecting

7e323b1

move functionality to separate module

277cf79

cleanup

407ffc5

Nizerlak requested a review from nnmm July 23, 2022 11:42

nnmm reviewed Jul 23, 2022

View reviewed changes

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

Nizerlak and others added 3 commits July 24, 2022 13:42

Improve doc

4b67972

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

move content from rcl_utils.rs to arguments.rs

4a448d5

Merge remote-tracking branch 'origin/non-ros-arguments' into non-ros-…

565d61e

…arguments

Nizerlak requested a review from nnmm July 24, 2022 11:54

nnmm previously approved these changes Jul 24, 2022

View reviewed changes

format fix

aa496aa

Nizerlak dismissed nnmm’s stale review via aa496aa July 24, 2022 14:42

Nizerlak requested a review from nnmm July 24, 2022 14:43

nnmm approved these changes Jul 24, 2022

View reviewed changes

nnmm merged commit 81e2ccf into ros2-rust:main Jul 24, 2022

Nizerlak mentioned this pull request Jul 24, 2022

Add function to get non-ROS arguments #147

Closed

esteve mentioned this pull request Sep 27, 2022

Add notes on unsafe comments, client library consistency, and reviewing to contributing.md #252

Merged

Add function to get non-ROS arguments #210

Add function to get non-ROS arguments #210

Conversation

Nizerlak commented Jun 27, 2022 • edited Loading

Nizerlak commented Jun 28, 2022 • edited Loading

jhdcs left a comment

Choose a reason for hiding this comment

nnmm commented Jul 2, 2022

esteve commented Jul 2, 2022

nnmm commented Jul 2, 2022 • edited Loading

Nizerlak commented Jul 2, 2022 • edited Loading

Nizerlak commented Jul 2, 2022

nnmm commented Jul 2, 2022 • edited Loading

nnmm commented Jul 2, 2022

Nizerlak commented Jul 4, 2022

Nizerlak commented Jul 4, 2022

jhdcs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nnmm commented Jul 5, 2022

esteve commented Jul 5, 2022

jhdcs commented Jul 5, 2022

nnmm commented Jul 5, 2022

esteve commented Jul 6, 2022

Nizerlak commented Jul 7, 2022

nnmm commented Jul 11, 2022

Nizerlak commented Jul 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhdcs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nnmm commented Jul 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nnmm commented Jul 23, 2022

nnmm left a comment

Choose a reason for hiding this comment

nnmm left a comment

Choose a reason for hiding this comment

Nizerlak commented Jun 27, 2022 •

edited

Loading

Nizerlak commented Jun 28, 2022 •

edited

Loading

nnmm commented Jul 2, 2022 •

edited

Loading

Nizerlak commented Jul 2, 2022 •

edited

Loading

nnmm commented Jul 2, 2022 •

edited

Loading