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

[Moveit_servo] Move and rename jogger #223

Closed

Conversation

AdamPettinger
Copy link
Contributor

Description

I moved and renamed the jogger: moveit_experimental/moveit_jog_arm -> moveit_ros/moveit_servo. In addition to the file paths and names, I renamed some of the internals (namespaces, project/package names, JogArm interface class -> Servo interface class, etc). This PR overall looks like a ton of changes, but I tried to break it down so each commit is a logical step that only modifies file names, or changed the files insides so that no commit in itself had a tremendous amount of diffs.

Also kind of weird to test, but I copied the new moveit_servo package to a catkin workspace and built it there successfully. I am currently working on getting this to build with ROS2, so the COLCON_IGNORE will stay for a bit longer

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@AdamPettinger AdamPettinger changed the title Move and rename jogger [Moveit_servo] Move and rename jogger Jun 23, 2020
@codecov
Copy link

codecov bot commented Jun 23, 2020

Codecov Report

Merging #223 into master will decrease coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #223      +/-   ##
==========================================
- Coverage   47.43%   47.35%   -0.08%     
==========================================
  Files         143      143              
  Lines       13347    13347              
==========================================
- Hits         6331     6321      -10     
- Misses       7016     7026      +10     
Impacted Files Coverage Δ
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp 66.22% <0.00%> (-4.45%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2ff4b9...48272b3. Read the comment docs.

Copy link
Member

@AndyZe AndyZe left a comment

Choose a reason for hiding this comment

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

I cloned the repo and grepped for jog. It looks like many other replacements need to be made. Especially in the /test folder. Were you ignoring tests on purpose for now?

I'll send you a message on how to find/replace recursively, that should make this a lot faster.

@@ -84,11 +84,11 @@ target_link_libraries(cpp_interface_example
## ROS message-based node ##
############################

add_executable(jog_server
src/jog_server.cpp
add_executable(servo_server
Copy link
Member

Choose a reason for hiding this comment

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

I guess servo_server is a bit strange to say, but I'm OK with it

@AdamPettinger
Copy link
Contributor Author

"jog" does still exist in quite a lot of places. There are definitely some places that need to be changed in the tests (e.g. def test_jog_arm_halt_msg(node)), and I will make those changes

Then throughout the package there are some potential changes that are less obvious. Some examples:

  • Are we going to refer to it as joint jogging and Cartesian jogging, or joint servoing and Cartesian servoing?
  • "the jogger" is in a lot of comments, what replaces that?
  • Changing JogCalcs -> ServoCalcs? Discussed offline and I think making this change is good
  • Parameters in the examples: keep as delta_jog_cmds, or replace with what? delta_servo_cmds?

@rhaschke
Copy link
Contributor

Didn't we want to rename this package in both ROS1 and ROS2?

@AndyZe
Copy link
Member

AndyZe commented Jun 25, 2020

Didn't we want to rename this package in both ROS1 and ROS2?

Sorry I missed the meeting yesterday. After a few weeks of no jog discussion, I thought it was safe to skip. Wrong.

I don't want to give Adam more menial tasks, nor distract him from the main goal. If you feel strongly that it should get renamed in ROS1, I can do that. Do you feel strongly?

@AndyZe
Copy link
Member

AndyZe commented Jun 25, 2020

* Are we going to refer to it as joint jogging and Cartesian jogging, or joint servoing and Cartesian servoing?

I think we should purge "jog" as much as possible. So, joint servoing and Cartesian servoing.

* "the jogger" is in a lot of comments, what replaces that?

"the servo server"? "the servo node"?

* Changing JogCalcs -> ServoCalcs? Discussed offline and I think making this change is good

yep, i think so too

* Parameters in the examples: keep as `delta_jog_cmds`, or replace with what? `delta_servo_cmds`?

Don't we have delta_joint_cmds and delta_jog_cmds? I think it should be delta_joint_cmds and delta_cartesian_cmds

@AdamPettinger
Copy link
Contributor Author

I think these sound good and what I was leaning towards

So, joint servoing and Cartesian servoing.

"the servo node"

I think it should be delta_joint_cmds and delta_cartesian_cmds

But I think I will do delta_joint_cmds and delta_twist_cmds

@v4hn
Copy link
Contributor

v4hn commented Jun 25, 2020

Do you feel strongly?

Talk to Henning about porting from MoveIt1 to MoveIt2. We really want that name to change everywhere, otherwise porting between versions becomes even more of a nightmare!

@AdamPettinger
Copy link
Contributor Author

New commits to address @AndyZe's comments.

Testing this work in a catkin workspace, so it should work OK in ROS1 if we decide to go that route

@AdamPettinger
Copy link
Contributor Author

Closing this now. With the rename done in ROS1 moveit/moveit#2165, we don't need to do it here, and should be able to sync those rename changes to moveit2 after #227 is merged

@AdamPettinger AdamPettinger deleted the move_and_rename_jogger branch August 24, 2020 17:17
MikeWrock pushed a commit to MikeWrock/moveit2 that referenced this pull request Aug 15, 2022
* Updated servo tutorial for MoveIt servo parameters update

* Add moveit2 to the repos file

Co-authored-by: Tyler Weaver <tylerjw@gmail.com>
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