Rename ServoServer to ServoNode #649

tylerjw · 2021-08-30T23:04:39Z

Description

Until we figure out how we want to change the API to prevent this sort of bug this makes servo error much nicer in the constructor of ServoCalcs.

This also includes a fix for a clang-tidy error I hadn't seen before. I wonder if our clang-tidy changed since the last time someone made a code change to servo.

Updated tutorials: moveit/moveit2_tutorials#109

Checklist

Required by CI: Code is auto formatted using clang-format
While waiting for someone to review your request, please help review another open pull request to support the maintainers
Make PR to update tutorials.
Migration note

codecov · 2021-08-31T00:21:56Z

Codecov Report

Merging #649 (9de7fdc) into main (bf39dca) will not change coverage.
The diff coverage is 81.82%.

❗ Current head 9de7fdc differs from pull request most recent head 3bf38cb. Consider uploading reports for the commit 3bf38cb to get more accurate results

@@           Coverage Diff           @@
##             main     #649   +/-   ##
=======================================
  Coverage   54.18%   54.18%           
=======================================
  Files         192      192           
  Lines       20187    20187           
=======================================
  Hits        10936    10936           
  Misses       9251     9251

Impacted Files	Coverage Δ
moveit_ros/moveit_servo/src/servo_node.cpp	`78.95% <81.82%> (ø)`

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 bf39dca...3bf38cb. Read the comment docs.

henningkayser · 2021-08-31T22:04:39Z

moveit_ros/moveit_servo/src/teleop_demo/joystick_servo_example.cpp

@@ -49,8 +49,8 @@

 // We'll just set up parameters here
 const std::string JOY_TOPIC = "/joy";
-const std::string TWIST_TOPIC = "/servo_server/delta_twist_cmds";
-const std::string JOINT_TOPIC = "/servo_server/delta_joint_cmds";
+const std::string TWIST_TOPIC = "/servo_component/delta_twist_cmds";


Should we just use the /servo/* namespace?

agree that probably makes sense or /moveit_servo ?

👍 to /servo/*

henningkayser · 2021-08-31T22:06:47Z

moveit_ros/moveit_servo/include/moveit_servo/servo_component.h

@@ -45,10 +45,10 @@

 namespace moveit_servo
 {
-class ServoServer : public rclcpp::Node
+class ServoComponent : public rclcpp::Node


What are the implications of calling this class Component? Isn't it a node? So ServoNode makes most sense to me, which in turn can be a component node (exported, no class/file needed) or a node executable ServoNodeExecutable.

Changes here should be reviewed along with #621 (Add standalone executable for Servo server node). In fact, maybe this PR should be closed and we just make all of the naming changes as a separate commit in that PR.

I think ServoNodeComponent (class name) and servo_component_node.cpp make good sense for this class / file.

The executable that Joe added in #621 could be called ServoMain or ServoStandaloneNode. It just instantiates this class in main.

+1 to ServoNode. In my mind we create a node and then allow launching it in a component container.

tylerjw · 2021-09-17T14:21:00Z

I renamed servo_component to servo_node and servo_server_node to servo_node_main.

henningkayser · 2021-09-30T11:11:59Z

@tylerjw one thing before merging. Could you provide deprecated headers and typedefs for the old API? Basically like this, only from the original headers not like I did back then.

tylerjw · 2021-10-03T15:33:28Z

@AndyZe I believe this is ready for review. I included a deprecated header so existing code should continue to build and produce a warning. There is no way for me to deprecate the change in the name for the binaries (the component node plugin) so launch files that depend on the old ServoServer node name will stop working after this.

moveit_ros/moveit_servo/include/moveit_servo/servo_server.h

AndyZe

A bit of text still needs to be replaced. Approved assuming that happens.

moveit_ros/moveit_servo/launch/servo_example.launch.py:            #     name="servo_server",
moveit_ros/moveit_servo/launch/servo_example.launch.py:        executable="servo_server_node",

AndyZe · 2021-10-13T18:36:04Z

It looks like this PR along with some of the other recent changes (e.g. launch file shuffling) have broken the Servo tutorial. I'd like to put the brakes on more large Servo changes until the tutorial gets fixed.

tylerjw · 2021-10-13T19:36:42Z

It looks like this PR along with some of the other recent changes (e.g. launch file shuffling) have broken the Servo tutorial. I'd like to put the brakes on more large Servo changes until the tutorial gets fixed.

The tutorial on the website or the tutorial in the main branch of the tutorial repo? The website has changed to track a new foxy branch. That website is based on a foxy install of moveit.

AndyZe · 2021-10-13T19:38:18Z

main branch, built from source

tylerjw · 2021-10-13T19:39:48Z

The internet is that eventually, we'll have different versions of the tutorials for each version of ros/moveit but for now we want the tutorial website to be as beginner-friendly and as stable as possible. Long term we will have multiple branches of the tutorial website deploy to different versions of the website, but until then we changed the website to only deploy from the foxy branch of moveit2_tutorials and be focused on beginners who install moveit from the foxy release.

For the main branch (of the tutorials and moveit), to use that you'll need to build it locally (including the website) to even access it and we do need a process for making sure it is working before putting it on the web which we will do at some point before Humble.

tylerjw · 2021-10-13T19:41:41Z

This tutorial: http://moveit2_tutorials.picknik.ai/doc/realtime_servo/realtime_servo_tutorial.html

Does not work with source installs of moveit unless you are building the foxy version of moveit. Eventually, it'll be moved to a URL that looks like this:

moveit2_tutorials.foxy.picknik.ai

and the main branch will be deployed to:

moveit2_tutorials.rolling.picknik.ai

tylerjw · 2021-10-13T19:42:39Z

We do need to include a migration guide from foxy eventually also for all the major changes to things like servo from foxy.

AndyZe · 2021-10-13T19:56:46Z

Yep, the plan for the tutorials makes sense. We should try to keep the main tutorials branch up to date with Rolling, though. Partly my fault for approving this PR without testing it.

tylerjw · 2021-10-13T20:03:54Z

Yep, the plan for the tutorials makes sense. We should try to keep the main tutorials branch up to date with Rolling, though. Partly my fault for approving this PR without testing it.

I can make the PR to update the rolling tutorials. We do need to create a migration guide to include with the Humble release of tutorials to make it easy for people to migrate from Foxy to Humble.

tylerjw requested a review from AndyZe August 30, 2021 23:04

tylerjw force-pushed the servo_component_rename branch from 73bc8eb to b7064a8 Compare August 30, 2021 23:06

tylerjw mentioned this pull request Aug 30, 2021

Rename ServoServer, update Readme moveit/moveit2_tutorials#109

Merged

2 tasks

tylerjw force-pushed the servo_component_rename branch 3 times, most recently from 905cc96 to 1a40885 Compare August 30, 2021 23:20

tylerjw mentioned this pull request Aug 30, 2021

Add standalone executable for Servo server node #621

Merged

6 tasks

tylerjw force-pushed the servo_component_rename branch from 1a40885 to e95b7b5 Compare August 30, 2021 23:25

henningkayser reviewed Aug 31, 2021

View reviewed changes

henningkayser added this to In Progress in Galactic/Rolling - 2.3.0 - September 10 Sep 7, 2021

henningkayser moved this from In Progress to Review in progress in Galactic/Rolling - 2.3.0 - September 10 Sep 21, 2021

henningkayser approved these changes Sep 29, 2021

View reviewed changes

tylerjw force-pushed the servo_component_rename branch from 411cc95 to 7e643e4 Compare October 1, 2021 20:09

tylerjw changed the title ~~Rename ServoServer to ServerComponent~~ Rename ServoServer to ServoNode Oct 1, 2021

JafarAbdi approved these changes Oct 4, 2021

View reviewed changes

moveit_ros/moveit_servo/include/moveit_servo/servo_server.h Show resolved Hide resolved

AndyZe approved these changes Oct 4, 2021

View reviewed changes

Rename ServoServer to ServerNode

3bf38cb

tylerjw force-pushed the servo_component_rename branch from 9de7fdc to 3bf38cb Compare October 4, 2021 16:19

tylerjw merged commit 69b59b9 into moveit:main Oct 4, 2021

Galactic/Rolling - 2.3.0 - September 10 automation moved this from Review in progress to Done Oct 4, 2021

tylerjw deleted the servo_component_rename branch October 4, 2021 18:02

AndyZe mentioned this pull request Oct 15, 2021

Fix the servo executable name #746

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename ServoServer to ServoNode #649

Rename ServoServer to ServoNode #649

tylerjw commented Aug 30, 2021 •

edited

Loading

codecov bot commented Aug 31, 2021 •

edited

Loading

henningkayser Aug 31, 2021

tylerjw Aug 31, 2021

AndyZe Aug 31, 2021

henningkayser Aug 31, 2021

AndyZe Aug 31, 2021 •

edited

Loading

schornakj Sep 7, 2021

tylerjw commented Sep 17, 2021

henningkayser commented Sep 30, 2021 •

edited

Loading

tylerjw commented Oct 3, 2021

AndyZe left a comment

AndyZe commented Oct 13, 2021

tylerjw commented Oct 13, 2021

AndyZe commented Oct 13, 2021

tylerjw commented Oct 13, 2021 •

edited

Loading

tylerjw commented Oct 13, 2021

tylerjw commented Oct 13, 2021

AndyZe commented Oct 13, 2021

tylerjw commented Oct 13, 2021

Rename ServoServer to ServoNode #649

Rename ServoServer to ServoNode #649

Conversation

tylerjw commented Aug 30, 2021 • edited Loading

Description

Checklist

codecov bot commented Aug 31, 2021 • edited Loading

Codecov Report

henningkayser Aug 31, 2021

Choose a reason for hiding this comment

tylerjw Aug 31, 2021

Choose a reason for hiding this comment

AndyZe Aug 31, 2021

Choose a reason for hiding this comment

henningkayser Aug 31, 2021

Choose a reason for hiding this comment

AndyZe Aug 31, 2021 • edited Loading

Choose a reason for hiding this comment

schornakj Sep 7, 2021

Choose a reason for hiding this comment

tylerjw commented Sep 17, 2021

henningkayser commented Sep 30, 2021 • edited Loading

tylerjw commented Oct 3, 2021

AndyZe left a comment

Choose a reason for hiding this comment

AndyZe commented Oct 13, 2021

tylerjw commented Oct 13, 2021

AndyZe commented Oct 13, 2021

tylerjw commented Oct 13, 2021 • edited Loading

tylerjw commented Oct 13, 2021

tylerjw commented Oct 13, 2021

AndyZe commented Oct 13, 2021

tylerjw commented Oct 13, 2021

tylerjw commented Aug 30, 2021 •

edited

Loading

codecov bot commented Aug 31, 2021 •

edited

Loading

AndyZe Aug 31, 2021 •

edited

Loading

henningkayser commented Sep 30, 2021 •

edited

Loading

tylerjw commented Oct 13, 2021 •

edited

Loading