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.

Sign up for GitHub

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

Jump to bottom

Demo for pre and post set parameter callback support #565

Merged
merged 25 commits into from
Oct 13, 2022
Merged

Demo for pre and post set parameter callback support #565

merged 25 commits into from
Oct 13, 2022

Conversation

deepanshubansal01
Copy link
Contributor

@deepanshubansal01 deepanshubansal01 commented Jun 6, 2022 •
edited
Loading

Related to ros2/rclcpp#1947 and ros2/rclpy#966

Signed-off-by: deepanshu deepanshubansal01@gmail.com

@deepanshubansal01 deepanshubansal01 mentioned this pull request Jun 6, 2022
Merged
jacobperron
jacobperron reviewed Jun 6, 2022
View reviewed changes
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.

I've done a first pass; thanks for adding a demo for the new feature!

demo_nodes_cpp/src/parameters/test_param_logic.cpp Outdated Show resolved Hide resolved
demo_nodes_cpp/src/parameters/test_param_logic.cpp Outdated Show resolved Hide resolved
demo_nodes_cpp/src/parameters/test_param_logic.cpp Outdated Show resolved Hide resolved
demo_nodes_cpp/src/parameters/test_param_logic.cpp Outdated Show resolved Hide resolved
demo_nodes_cpp/src/parameters/test_param_logic.cpp Outdated Show resolved Hide resolved
demo_nodes_cpp/src/parameters/test_param_logic.cpp Outdated Show resolved Hide resolved
demo_nodes_cpp/src/parameters/test_param_logic.cpp Outdated Show resolved Hide resolved
demo_nodes_cpp/src/parameters/test_param_logic.cpp Outdated Show resolved Hide resolved
demo_nodes_cpp/src/parameters/test_param_logic.cpp Outdated Show resolved Hide resolved
demo_nodes_cpp/src/parameters/test_param_logic.cpp Outdated Show resolved Hide resolved
@deepanshubansal01 deepanshubansal01 self-assigned this Jun 6, 2022
@deepanshubansal01 deepanshubansal01 marked this pull request as draft June 6, 2022 19:56
@deepanshubansal01 deepanshubansal01 marked this pull request as ready for review June 23, 2022 23:18
@audrow audrow changed the base branch from master to rolling June 28, 2022 14:18
audrow
audrow reviewed Jul 6, 2022
View reviewed changes
Copy link
Member

@audrow audrow left a comment

Choose a reason for hiding this comment

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

The code looks alright to me, but I think the files need to be checked for formatting. You can try running ament_cpplint or ament_cppcheck on your machine.

demo_nodes_cpp/CMakeLists.txt Outdated Show resolved Hide resolved
demo_nodes_cpp/src/parameters/set_parameters_callback.cpp Outdated Show resolved Hide resolved
@deepanshubansal01 deepanshubansal01 changed the title test local parameter callback support Dome for pre and post set parameter callback support Jul 6, 2022
@deepanshubansal01 deepanshubansal01 changed the title Dome for pre and post set parameter callback support Demo for pre and post set parameter callback support Jul 6, 2022
@deepanshubansal01 deepanshubansal01 mentioned this pull request Jul 13, 2022
Merged
fujitatomoya
fujitatomoya approved these changes Jul 18, 2022
View reviewed changes
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

looks good to me

@deepanshubansal01
Copy link
Contributor Author

@adityapande-1995 @jacobperron should we merge this now, since the rclpy and rclcpp PR's have been merged already.

@fujitatomoya
Copy link
Collaborator

@deepanshubansal01 lgtm, can you address the conflicts?

@deepanshubansal01
Copy link
Contributor Author

@deepanshubansal01 lgtm, can you address the conflicts?

Will do.

clalancette
clalancette requested changes Oct 6, 2022
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I've left a couple of stylistic points to consider.

demo_nodes_cpp/src/parameters/set_parameters_callback.cpp Outdated Show resolved Hide resolved
demo_nodes_cpp/src/parameters/set_parameters_callback.cpp Outdated Show resolved Hide resolved
demo_nodes_py/demo_nodes_py/parameters/set_parameters_callback.py Outdated Show resolved Hide resolved
@deepanshubansal01
Copy link
Contributor Author

Seems like it's good to go, should I squash and merge now?

@clalancette
Copy link
Contributor

Seems like it's good to go, should I squash and merge now?

We still need approval and to run CI on it. I'll take another look, though I may not get back to it today.

@adityapande-1995
Copy link
Contributor

CI :

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@deepanshubansal01 @clalancette
test local parameter callback support
48cf397 
Signed-off-by: deepanshu <deepanshubansal01@gmail.com>
@deepanshubansal01 @clalancette
refactoring of the post set parameter callback test node
7fd1662 
Signed-off-by: deepanshu <deepanshubansal01@gmail.com>
@deepanshubansal01 @clalancette
cleanup
e94b1ab 
Signed-off-by: deepanshu <deepanshubansal01@gmail.com>
@deepanshubansal01 @clalancette
logging
eae14b7 
Signed-off-by: deepanshu <deepanshubansal01@gmail.com>
@deepanshubansal01 @clalancette
logging
329105f 
Signed-off-by: deepanshu <deepanshubansal01@gmail.com>
@deepanshubansal01 @clalancette
logging
23d5c03 
Signed-off-by: deepanshu <deepanshubansal01@gmail.com>
@deepanshubansal01 @clalancette
remove validation callback
fae73e3 
Signed-off-by: deepanshu <deepanshubansal01@gmail.com>
@deepanshubansal01 @clalancette
add_pre_set_parameter_callback demo
aa7cab5 
Signed-off-by: deepanshu <deepanshubansal01@gmail.com>
@deepanshubansal01 @clalancette
renaming based on new architecture
395e2c2 
Signed-off-by: deepanshu <deepanshubansal01@gmail.com>
@deepanshubansal01 @clalancette
fix linting and formatting
5529271 
Signed-off-by: deepanshu <deepanshubansal01@gmail.com>
@deepanshubansal01 @clalancette
formatting
01b3ac2 
Signed-off-by: deepanshu <deepanshubansal01@gmail.com>
@deepanshubansal01 @clalancette
fix formatting
a561c80 
Signed-off-by: deepanshu <deepanshubansal01@gmail.com>
@deepanshubansal01 @clalancette
formatting
b53bb27 
Signed-off-by: deepanshu <deepanshubansal01@gmail.com>
@deepanshubansal01 @clalancette
formatting
5b8d77c 
Signed-off-by: deepanshu <deepanshubansal01@gmail.com>
@deepanshubansal01 @clalancette
ament_uncrustify
56a1a34 
Signed-off-by: deepanshu <deepanshubansal01@gmail.com>
@deepanshubansal01 @clalancette
add demo for pre and post set param callback
0d7367a 
Signed-off-by: deepanshu <deepanshubansal01@gmail.com>
@deepanshubansal01 @clalancette
simplify the cpp demo for callback
5554e41 
Signed-off-by: deepanshu <deepanshubansal01@gmail.com>
@deepanshubansal01 @clalancette
solve linting errors
0f85134 
Signed-off-by: deepanshu <deepanshubansal01@gmail.com>
@deepanshubansal01 @clalancette
update rclpy demo
d0f84af 
Signed-off-by: deepanshu <deepanshubansal01@gmail.com>
@deepanshubansal01 @clalancette
changes after CI run
5478325 
Signed-off-by: deepanshu <deepanshubansal01@gmail.com>
@deepanshubansal01 @clalancette
single quoatation instead of double
ec3fcb3 
Signed-off-by: deepanshu <deepanshubansal01@gmail.com>
@deepanshubansal01 @clalancette
formatting
49a4219 
Signed-off-by: deepanshu <deepanshubansal01@gmail.com>
@deepanshubansal01 @clalancette
code review
435bd3a 
Signed-off-by: deepanshu <deepanshubansal01@gmail.com>
@deepanshubansal01 @clalancette
simplify code and solve linting errors
7078cbf 
Signed-off-by: deepanshu <deepanshubansal01@gmail.com>
@clalancette clalancette force-pushed the deepanshu/local-param-changed-callback branch from b94bc55 to 7078cbf Compare October 12, 2022 19:07
@clalancette
Copy link
Contributor

clalancette commented Oct 12, 2022 •
edited
Loading

CI failed here because this needed another rebase. Done now, here is CI again:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@deepanshubansal01
Copy link
Contributor Author

deepanshubansal01 commented Oct 13, 2022 •
edited
Loading

Seems like CI is passing now. Can I squash merge now or wait for more pending reviewers. Thanks!

@clalancette clalancette merged commit 25f6176 into rolling Oct 13, 2022
@delete-merged-branch delete-merged-branch bot deleted the deepanshu/local-param-changed-callback branch October 13, 2022 12:28
clalancette added a commit that referenced this pull request Oct 13, 2022
@clalancette
Revert "Demo for pre and post set parameter callback support (#565)"
7bcdd02 
This reverts commit 25f6176.
cardboardcode pushed a commit to cardboardcode/demos that referenced this pull request Jan 23, 2023
@deepanshubansal01 @cardboardcode
Demo for pre and post set parameter callback support (ros2#565)
de5295d 
* local parameter callback support

Signed-off-by: deepanshu <deepanshubansal01@gmail.com>
cardboardcode pushed a commit to cardboardcode/demos that referenced this pull request Jan 23, 2023
@deepanshubansal01 @cardboardcode
Demo for pre and post set parameter callback support (ros2#565)
fd65651 
* local parameter callback support

Signed-off-by: deepanshu <deepanshubansal01@gmail.com>
Signed-off-by: Bey Hao Yun <beyhy94@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@audrow audrow audrow left review comments

@clalancette clalancette clalancette approved these changes

@jacobperron jacobperron Awaiting requested review from jacobperron

@adityapande-1995 adityapande-1995 Awaiting requested review from adityapande-1995 adityapande-1995 is a code owner

@fujitatomoya fujitatomoya Awaiting requested review from fujitatomoya

Assignees

@deepanshubansal01 deepanshubansal01

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@deepanshubansal01 @fujitatomoya @clalancette @adityapande-1995 @jacobperron @audrow