Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port kobuki keyop to ROS 2. #3

Merged
merged 3 commits into from
Jan 21, 2020

Conversation

clalancette
Copy link
Collaborator

Signed-off-by: Chris Lalancette [email protected]

One important thing to note; there is intentionally no launch files in this PR. That's because launch does not pass stdin through to processes, so there is no way for keyop to access the keyboard. So I didn't even include an example to ensure we don't confuse users.

Signed-off-by: Chris Lalancette <[email protected]>
@clalancette clalancette requested a review from stonier January 20, 2020 13:01
Copy link
Contributor

@stonier stonier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments.

However, what I'm curious about is, without launchers, how we enable a good workflow for providing a convenient means to keyop with a user's own parameters. We're not even using the yaml here.

What would be the best way to ros2 run this node and pass it your own parameters?

Signed-off-by: Chris Lalancette <[email protected]>
@clalancette
Copy link
Collaborator Author

What would be the best way to ros2 run this node and pass it your own parameters?

So, the way I've been testing it is like:

ros2 run kobuki_keyop kobuki_keyop_node --ros-args -p angular_vel_max:=1.3

It's slightly ugly, but I'm not sure how to do better at the moment. I'll also note that this syntax only works on Eloquent; I don't exactly know how to pass it in Dashing without writing it to a config file.

Copy link
Contributor

@stonier stonier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably worth slightly modifying this with it's own command line argument parsing so we can do something like:

ros2 run kobuki_keyop kobuki_keyop_node --parameters kobuki_keyop/config/keyop_params.yaml

so a user can utilise a yaml stored in a vcs / deployed with packages.

Let's put out of scope for this PR though - can you spin up an issue for this?

Copy link
Contributor

@stonier stonier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unblocking this pending:

a) insert the mutex lock in spin if deemed necessary
b) creation of an issue to improve the user workflow with a yaml file

@stonier
Copy link
Contributor

stonier commented Jan 20, 2020

Heading out for a few hours. Feel free to merge this when last tasks have been addressed. I'll create another meta-issue for other remaining work.

@clalancette
Copy link
Collaborator Author

Probably worth slightly modifying this with it's own command line argument parsing so we can do something like:

Oh, I should have mentioned that that already works, both in Dashing and in Eloquent (though with different syntaxes).

Eloquent:

ros2 run kobuki_keyop kobuki_keyop_node --ros-args --params-file config/keyop_params.yaml

Dashing:

ros2 run kobuki_keyop kobuki_keyop_node --ros-args __params:=config/keyop_params.yaml

So I think that task ends up being a documentation one. I'll still open an issue about it, then merge this PR.

@clalancette
Copy link
Collaborator Author

All right, I've opened #5 to fix up the documentation for this package. @stonier it looks like I can't merge on this repository, so please merge this PR up when you get a chance, thanks.

@stonier stonier mentioned this pull request Jan 21, 2020
10 tasks
@stonier stonier merged commit 9ddaa55 into kobuki-base:devel Jan 21, 2020
@clalancette clalancette deleted the keyop-ros2-port branch January 21, 2020 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants