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

Support multiple parameters callback #457

Merged
merged 1 commit into from Feb 5, 2020
Merged

Conversation

suab321321
Copy link
Contributor

@suab321321 suab321321 commented Nov 9, 2019

@allenh1 now #427 is continued here as my previous repos were a lot messy

@suab321321 suab321321 force-pushed the master branch 5 times, most recently from d85597e to 78cc5e8 Compare Nov 9, 2019
@allenh1
Copy link
Contributor

allenh1 commented Nov 9, 2019

@allenh1 now #427 is continued here as my previous repos were a lot messy

@suab321321 Please refrain from opening multiple pull requests. Instead, It's best to fix the original pull request instead.

rclpy/rclpy/node.py Show resolved Hide resolved
rclpy/rclpy/node.py Outdated Show resolved Hide resolved
rclpy/rclpy/node.py Outdated Show resolved Hide resolved
@suab321321 suab321321 force-pushed the master branch 4 times, most recently from 6c12b68 to b5b8a1e Compare Nov 12, 2019
Copy link
Member

@dirk-thomas dirk-thomas left a comment

Please add tests to cover the new API.

rclpy/rclpy/node.py Outdated Show resolved Hide resolved
rclpy/rclpy/node.py Outdated Show resolved Hide resolved
rclpy/rclpy/node.py Outdated Show resolved Hide resolved
rclpy/rclpy/node.py Outdated Show resolved Hide resolved
rclpy/rclpy/node.py Outdated Show resolved Hide resolved
rclpy/rclpy/node.py Outdated Show resolved Hide resolved
@dirk-thomas dirk-thomas added enhancement New feature or request in progress Actively being worked on (Kanban column) labels Nov 12, 2019
@suab321321 suab321321 force-pushed the master branch 2 times, most recently from c82cc61 to 6ce54c9 Compare Nov 12, 2019
rclpy/rclpy/node.py Outdated Show resolved Hide resolved
rclpy/rclpy/node.py Outdated Show resolved Hide resolved
rclpy/rclpy/node.py Outdated Show resolved Hide resolved
@suab321321 suab321321 force-pushed the master branch 3 times, most recently from 60dc705 to 74bc57c Compare Nov 13, 2019
Copy link
Member

@dirk-thomas dirk-thomas left a comment

As mentioned before (#457 (review)) please add test coverage for the newly introduced API.

callback: Callable[[List[Parameter]], SetParametersResult]
) -> None:
"""
Add a callback in front to the list of callback.
Copy link
Member

@dirk-thomas dirk-thomas Nov 14, 2019

Choose a reason for hiding this comment

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

@ivanpauno Can you comment why the C++ implementation inserts new callbacks before the already registered ones?

rclpy/rclpy/node.py Outdated Show resolved Hide resolved
rclpy/rclpy/node.py Outdated Show resolved Hide resolved
rclpy/rclpy/node.py Outdated Show resolved Hide resolved
@suab321321 suab321321 force-pushed the master branch 4 times, most recently from 83230e3 to 1f0d44f Compare Nov 16, 2019
@suab321321
Copy link
Contributor Author

suab321321 commented Feb 3, 2020

@jacobperron sir?

@suab321321
Copy link
Contributor Author

suab321321 commented Feb 3, 2020

@jacobperron sir now is it okay?

@jacobperron
Copy link
Member

jacobperron commented Feb 3, 2020

The same test is still failing for me as I commented in #457 (comment)

Can you reproduce the test failure?

I am running the following commands to build and test this code:

colcon build --packages-up-to rclpy
colcon test --packages-select rclpy --ctest-args -R test_node --event-handlers console_direct+

The second command will limit the test to test_node.py and give you more detailed output about the failure.

Please fix the test.

@allenh1
Copy link
Contributor

allenh1 commented Feb 3, 2020

The command git rebase HEAD~41 --signoff should reapply the last 41 commits on the current branch that is checked out, but with the "--signoff" option.

I think this inadvertently caused a large amount of older commits to get signed by @suab321321...

here we discovered that the first commit on this branch was waiting on @dirk-thomas's signoff, causing the DCO failure.

This branch is in a rather odd place... way too many changes and way too many commits... They should resolve after a squash merge though, right?

@dirk-thomas
Copy link
Member

dirk-thomas commented Feb 3, 2020

The PR is only being considered for merging once it has a "clean" commit list. That means only list commits related to the change proposed in this PR.

@allenh1
Copy link
Contributor

allenh1 commented Feb 4, 2020

The PR is only being considered for merging once it has a "clean" commit list.

Yeah, I think that's wise.

Maybe git cherry-pick can salvage this one?

My suggestion here would be:

  1. check out a temporary branch off the current ROS 2 master
  2. git cherry-pick --signoff each individual commit on suab321321:master
  3. Once you are 100% sure it is correct do a git branch -D for suab321321:master, and replace that branch with the temporary one
  4. git push --force-with-lease the new master branch created by @suab321321 with the patches applied on top of the current master

Does this sound like a good approach? Just thinking out loud here.

@suab321321
Copy link
Contributor Author

suab321321 commented Feb 4, 2020

@allenh1 sir now my commits are passing DCO checking..is this needed now?

@suab321321
Copy link
Contributor Author

suab321321 commented Feb 4, 2020

@jacobperron sir now it is passing :)
Screenshot from 2020-02-05 00-51-08

@allenh1
Copy link
Contributor

allenh1 commented Feb 4, 2020

@allenh1 sir now my commits are passing DCO checking..is this needed now?

@dirk-thomas has answered this already:

The PR is only being considered for merging once it has a "clean" commit list. That means only list commits related to the change proposed in this PR.

What I was recommending is a potential way to get this branch to the state required for merging. It doesn't feel right to me to use cherry-pick for this though, to be honest.

Anybody have any ideas for fixing this?

Copy link
Member

@jacobperron jacobperron left a comment

I've left some more comments reviewing the test code again.

rclpy/test/test_node.py Outdated Show resolved Hide resolved
rclpy/test/test_node.py Outdated Show resolved Hide resolved
rclpy/test/test_node.py Outdated Show resolved Hide resolved
rclpy/test/test_node.py Show resolved Hide resolved
rclpy/test/test_node.py Outdated Show resolved Hide resolved
rclpy/test/test_node.py Show resolved Hide resolved
rclpy/test/test_node.py Show resolved Hide resolved
rclpy/test/test_node.py Outdated Show resolved Hide resolved
rclpy/test/test_node.py Show resolved Hide resolved

self.node.remove_on_set_parameters_callback(self.reject_parameter_callback_1)

# Now the setting its value again.
Copy link
Member

@jacobperron jacobperron Feb 4, 2020

Choose a reason for hiding this comment

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

This suggestion is still valid.

@jacobperron
Copy link
Member

jacobperron commented Feb 4, 2020

Anybody have any ideas for fixing this?

It might be easier to start a fresh branch off of master, copy the files with changes there, and do a single commit. After verifying the changes are the same as upstream, you can force push to this branch. I can do this after my latest comments are addressed.

@allenh1
Copy link
Contributor

allenh1 commented Feb 4, 2020

Anybody have any ideas for fixing this?

It might be easier to start a fresh branch off of master, copy the files with changes there, and do a single commit.

Yeah, that's probably the cleanest fix. 👍

@suab321321 suab321321 force-pushed the master branch 2 times, most recently from 604a186 to 926364e Compare Feb 5, 2020
@suab321321
Copy link
Contributor Author

suab321321 commented Feb 5, 2020

@jacobperron sir is it good now?

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: AbhinavSingh <singhabhinav9051571833@gmail.com>
Copy link
Member

@jacobperron jacobperron left a comment

@suab321321 The code looks good to me 👍 I've cleaned up the commit history and will trigger CI now:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
  • Windows-container Build Status (experimental, can be ignored)

rclpy/test/test_node.py Outdated Show resolved Hide resolved
Copy link
Member

@jacobperron jacobperron left a comment

Thanks for the addition @suab321321!

@jacobperron jacobperron merged commit 822f194 into ros2:master Feb 5, 2020
1 check passed
@jacobperron jacobperron removed the in progress Actively being worked on (Kanban column) label Feb 5, 2020
jacobperron added a commit that referenced this pull request Feb 6, 2020
Replaced by new API that supports multiple callbacks introduced in #457.
Replaced references to the old API with the new API.
Left tests for the old API that should be removed or updated when we remove
the deprecated API.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@suab321321
Copy link
Contributor Author

suab321321 commented Feb 6, 2020

thanks @jacobperron sir for merging and @allenh1 sir thank you so much for the guidance :)

jacobperron added a commit that referenced this pull request Mar 10, 2020
Replaced by new API that supports multiple callbacks introduced in #457.
Replaced references to the old API with the new API.
Left tests for the old API that should be removed or updated when we remove
the deprecated API.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants