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

Add a tutorial for the new ParameterEventHandler class #1123

Merged
merged 7 commits into from
Mar 18, 2021
Merged

Add a tutorial for the new ParameterEventHandler class #1123

merged 7 commits into from
Mar 18, 2021

Conversation

mjeronimo
Copy link
Contributor

@mjeronimo mjeronimo commented Feb 18, 2021

This tutorial depends on the following functionality: ros2/rclcpp#829, which should be integrated before this PR.

@Mergifyio backport rolling

Signed-off-by: Michael Jeronimo [email protected]

@clalancette
Copy link
Contributor

I'm going to mark this one as a "Draft" until the dependency is merged.

@clalancette clalancette marked this pull request as draft February 22, 2021 16:03
@clalancette clalancette changed the base branch from master to rolling March 1, 2021 13:53
@mjeronimo mjeronimo changed the title [WIP] Add a tutorial for the new ParameterEventHandler class Add a tutorial for the new ParameterEventHandler class Mar 4, 2021
@mjeronimo mjeronimo marked this pull request as ready for review March 4, 2021 17:10
@mjeronimo
Copy link
Contributor Author

The required PR, ros2/rclcpp#829, has be merged.

@kscottz
Copy link
Collaborator

kscottz commented Mar 16, 2021

Anything stopping us from getting this merged? If I don't hear anything in the next 48 hours I am going to assume everything is ok.

@clalancette
Copy link
Contributor

Anything stopping us from getting this merged? If I don't hear anything in the next 48 hours I am going to assume everything is ok.

The original PR that added the functionality (ros2/rclcpp#829) was reverted. I'm not sure if its replacement has been merged yet, can you comment @mjeronimo ?

Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

This generally looks good to me 👍

Some suggestions and questions below.

Also, more of a meta-question. Do we have some criteria for what constitutes a "beginner" tutorial versus something else?
Intuitively, I'd say this tutorial is appropriate for beginners, but I was just wondering.

source/Tutorials/Monitoring-For-Parameter-Changes-CPP.rst Outdated Show resolved Hide resolved
source/Tutorials/Monitoring-For-Parameter-Changes-CPP.rst Outdated Show resolved Hide resolved
source/Tutorials/Monitoring-For-Parameter-Changes-CPP.rst Outdated Show resolved Hide resolved
source/Tutorials/Monitoring-For-Parameter-Changes-CPP.rst Outdated Show resolved Hide resolved
source/Tutorials/Monitoring-For-Parameter-Changes-CPP.rst Outdated Show resolved Hide resolved
@clalancette
Copy link
Contributor

Also, more of a meta-question. Do we have some criteria for what constitutes a "beginner" tutorial versus something else?

That's a good question. There's no written criteria for it that I know of.

Intuitively, I'd say this tutorial is appropriate for beginners, but I was just wondering.

My intuition says that this should be an Intermediate tutorial. My reasoning is that a beginner doesn't have to know this information to get started using ROS 2; it's a nice additional benefit when you get into developing your own nodes. But I don't feel really strongly about it.

@mjeronimo
Copy link
Contributor Author

@clalancette The required PR has been merged into rclcpp. I'll address Jacob's comments now.

@mjeronimo
Copy link
Contributor Author

@clalancette @jacobperron Moved this to the Intermediate Tutorials list, per Chris' comments.

@jacobperron
Copy link
Member

My reasoning is that a beginner doesn't have to know this information to get started using ROS 2; it's a nice additional benefit when you get into developing your own nodes

I like that qualifier for being part of the beginner tutorials; tutorials covering stuff that users really should know about when getting started.

Using the new class, ParameterEventsSubscriber.

Signed-off-by: Michael Jeronimo <[email protected]>
Signed-off-by: Michael Jeronimo <[email protected]>
Signed-off-by: Michael Jeronimo <[email protected]>
Signed-off-by: Michael Jeronimo <[email protected]>
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM, two minor comments

source/Tutorials/Monitoring-For-Parameter-Changes-CPP.rst Outdated Show resolved Hide resolved
source/Tutorials/Monitoring-For-Parameter-Changes-CPP.rst Outdated Show resolved Hide resolved
@mjeronimo mjeronimo merged commit 1e18ffd into ros2:rolling Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants