-
Notifications
You must be signed in to change notification settings - Fork 329
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 example of registering custom parameter validation callbacks #273
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me.
result.successful = false; | ||
} else { | ||
RCLCPP_INFO(this->get_logger(), | ||
"parameter '%s' has changed and is now %s", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to quote the second %s
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or use a : %s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I purposefully avoided the quoting as I was afraid it could confuse users into thinking that a string value was set and not an integer.
I went for the 2nd approach in 6109880
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! The only other thing I might look for is an example that uses std::bind
to pass an arbitrary argument into the param_change_callback
along with the parameters
. It is "just" reusing std::bind
, but it is convenient to see the example all put together. However, I won't block this PR for that, it is just a nice-to-have if you feel like adding it in. Thanks.
I think we would need another story to justify passing extra arguments to this callback as currently its only purpose is to check for parity and I have a hard time finding a variable to pass in that would make sense. |
@clalancette Posting new comment instead of editing to sent notification:
I'm open to suggestions for this. |
Nothing really compelling comes to mind, so I'd say go ahead and merge. We can always revisit later. |
Sounds good, thanks for the review! |
fixes ros2/examples#164