Parse URDF file from ros_robot class by mhpanah · Pull Request #144 · ros-navigation/navigation2

mhpanah · 2018-10-05T22:12:36Z

Open and parse URDF file from ros_robot class. Once the URDF file is parsed, the robot description can be obtained from "model_".

Currently getCurrentPose, getCurrentVelocity, getRobotName, and sendVelocity methods have been implemented.

getCurrentVelocity method gets odometry msg which contains both twist and pose. getCurrentPose method gets pose msg which is estimated from localization module. To make this less confusing, I will open another PR to rename the getCurrentVelocity method to getOdometry and also rename the getCurrentPose to getCurrentEstimatedPose.

TODO(mhpanah):

getFootPrint()
robot types and SN or ID
Add unit and integration test
Be able to configure which topic name to subscribe to

mjeronimo · 2018-10-05T22:38:52Z

src/libs/nav2_robot/include/nav2_robot/ros_robot.hpp


  void enterSafeState() override;

+  urdf::Model model_;


This should be a protected member variable. There should be public methods used to access the data held in the model. For example, you could add a getFootprint() method that other code could use to get the robot footprint.

Update: I see you mentioned that the footprint is in the yaml file. Can we add to the URDF to include it there?

Agreed.
I'm looking into footprint to see what we can do.

mjeronimo · 2018-10-05T22:40:33Z

src/libs/nav2_robot/include/nav2_robot/ros_robot.hpp

 protected:
  rclcpp::Node * node_;
-
+  const char * urdf_file_;


I'd prefer to use std::string instead of char *. You can always get the char * using urdf_file_.c_str().

mjeronimo · 2018-10-05T22:41:55Z

src/libs/nav2_robot/src/ros_robot.cpp

@@ -23,6 +24,17 @@ RosRobot::RosRobot(rclcpp::Node * node)
 : node_(node), initial_pose_received_(false)
 {
  // Open and parser the URDF file


Just noticed a typo here that you could fix while you're in this file ("parser" should be "parse").

mjeronimo · 2018-10-05T23:14:27Z

src/navigation/nav2_simple_navigator/src/main.cpp


 int main(int argc, char ** argv)
 {
+  try {


I don't want to diverge this file from all of the other main.cpp's. If we want to catch exceptions at the top, we should do it for all of them. Let's discuss as a group.

Yes, we should discuss this. Actually Me and Carl were thinking to bring this up on Monday for discussion.

mjeronimo · 2018-10-05T23:14:58Z

src/navigation/nav2_simple_navigator/src/main.cpp

 int main(int argc, char ** argv)
 {
+  try {
  rclcpp::init(argc, argv);


Indentation isn't right here.

mjeronimo · 2018-10-05T23:15:31Z

src/navigation/nav2_simple_navigator/src/main.cpp

  rclcpp::shutdown();

  return 0;
+  } catch(std::exception& e) {


There's no return statement when catching the exception. Main will exit without a return statement.

mjeronimo · 2018-10-11T21:17:33Z

src/libs/nav2_robot/include/nav2_robot/ros_robot.hpp

  void enterSafeState() override;

-  geometry_msgs::msg::PoseWithCovarianceStamped::SharedPtr getCurrentPose();
+  bool getCurrentPose(geometry_msgs::msg::PoseWithCovarianceStamped::SharedPtr & robot_pose);


@mhpanah @crdelsey
What's the rationale to prefer a success/failure return code in this case versus an exception? We should be consistent with our error handling mechanisms, if possible, and I thought we were preferring exceptions. That is, we'd introduce specific exception types instead of using the generic ones, such as runtime_error.

mjeronimo · 2018-10-11T21:21:22Z

src/navigation/nav2_simple_navigator/src/main.cpp

+    return 0;
+  } catch (std::exception & e) {
+    RCLCPP_ERROR(rclcpp::get_logger("simple_navigator"), e.what());
+    return 0;


Don't think we should return 0 on a failure.

That's right. :)

mjeronimo · 2018-10-11T21:24:48Z

src/navigation/nav2_simple_navigator/src/main.cpp

-  return 0;
+    return 0;
+  } catch (std::exception & e) {
+    RCLCPP_ERROR(rclcpp::get_logger("simple_navigator"), e.what());


I don't recommend using the logger name of the simple navigator module here as a string literal. This introduces a dependency on the name defined somewhere else (the name is duplicated and prone to getting out of sync). It also isn't good form, in my opinion, to write to other module's loggers. I think we should use the name of this module since it caught the exception. Perhaps this means that we do still need the logging at the source throwing the exception so that it appears in that module's log.

Per our discussion, I will remove handling the exception for now to be consistent with other main files.

mjeronimo · 2018-10-12T20:16:47Z

src/libs/nav2_robot/src/ros_robot.cpp

+RosRobot::getFootPrint()
+{
+}
+// TODO(mhpanah): modify this method name and implementation to inclue robot types and Serial # (ID)


Typo: "inclue" -> include

mjeronimo

Remove the unimplemented getFootprint method and fix the typo.

mkhansenbot

A few small changes needed, and the turtlebot urdf and yaml files need to be removed.

mkhansenbot · 2018-10-12T21:47:42Z

src/libs/nav2_robot/include/nav2_robot/ros_robot.hpp

-  geometry_msgs::msg::PoseWithCovarianceStamped::SharedPtr getCurrentPose();
+  bool getCurrentPose(geometry_msgs::msg::PoseWithCovarianceStamped::SharedPtr & robot_pose);
+  bool getCurrentVelocity(nav_msgs::msg::Odometry::SharedPtr & robot_velocity);
+  // TODO(mhpanah): implement getFootPrint method


Please file an issue for any TODO's

Opened #163 to address all the TODO's.

mkhansenbot · 2018-10-12T21:54:59Z

src/libs/nav2_robot/src/ros_robot.cpp

 RosRobot::onPoseReceived(const geometry_msgs::msg::PoseWithCovarianceStamped::SharedPtr msg)
 {
-  RCLCPP_INFO(node_->get_logger(), "RosRobot::onPoseReceved");
+  RCLCPP_INFO(node_->get_logger(), "RosRobot::onPoseReceived");


Can we change the RCLCPP_INFO messages here to something useful, like "Pose Received" or change this to a RCLCPP_DEBUG message

I think we should we just remove this message or change it to RCLCPP_DEBUG.

mkhansenbot · 2018-10-12T21:58:29Z

src/libs/nav2_robot/src/ros_robot.cpp

+RosRobot::getCurrentVelocity(nav_msgs::msg::Odometry::SharedPtr & robot_velocity)
+{
+  if (!initial_odom_received_) {
+   RCLCPP_INFO(node_->get_logger(), "initial odom not received yet");


Can we improve this message to something like "Can't return current velocity: Initial odometry not yet received". Also, should this be an RCLCPP_WARN() instead of an INFO, or this OK if it occurs?

Agreed. This should be RCLCPP_WARN. Will fix it.

mkhansenbot · 2018-10-12T22:00:26Z

src/libs/nav2_robot/urdf/turtlebot2.yaml

@@ -0,0 +1,29 @@
+robot:


Turtlebot files (urdf and yaml) shouldn't be in this repo. If they're needed for testing they should be pulled in at launch time from their packages.

src/navigation/nav2_simple_navigator/src/simple_navigator.cpp

mkhansenbot

I think you have extra quotes in one of the strings, and you need to rebase because it's showing merge conflicts due to other PRs having been merged.

src/libs/nav2_robot/src/ros_robot.cpp

src/navigation/nav2_simple_navigator/src/simple_navigator.cpp

mkhansenbot

Looks good now 👍

* added to the BT XML list * tilte underline * added to the full navigation plugin list * Migration

mhpanah added the nav2_robot label Oct 5, 2018

mhpanah added this to the October 2018 milestone Oct 5, 2018

mhpanah self-assigned this Oct 5, 2018

mhpanah requested review from a user, bpwilcox, mjeronimo and mkhansenbot October 5, 2018 22:12

mjeronimo suggested changes Oct 5, 2018

View reviewed changes

mjeronimo reviewed Oct 5, 2018

View reviewed changes

mhpanah closed this Oct 10, 2018

mhpanah reopened this Oct 11, 2018

mjeronimo reviewed Oct 11, 2018

View reviewed changes

mhpanah force-pushed the robot branch from b1826e2 to 1e4bb71 Compare October 12, 2018 00:12

mjeronimo reviewed Oct 12, 2018

View reviewed changes

mjeronimo approved these changes Oct 12, 2018

View reviewed changes

mkhansenbot requested changes Oct 12, 2018

View reviewed changes

mhpanah force-pushed the robot branch from 1e4bb71 to 0e81c84 Compare October 12, 2018 22:44

mkhansenbot reviewed Oct 15, 2018

View reviewed changes

src/libs/nav2_robot/src/ros_robot.cpp Show resolved Hide resolved

src/navigation/nav2_simple_navigator/src/simple_navigator.cpp Show resolved Hide resolved

mkhansenbot approved these changes Oct 15, 2018

View reviewed changes

mhpanah added 7 commits October 15, 2018 15:39

be able to parse urdf file from ros_robot class

31f8386

fix uncrustify

82865e6

addressing reviewers comments

6d89710

added odometry subscription and publishing velocity functionality

8c76ee6

added getVelocity, getRobotname, and sendVelocity

a1d9879

fixed format

45ddc42

changed get methods to return boolean

af33c92

mhpanah added 2 commits October 15, 2018 15:39

address reviewer comments

916cd06

removed urdf and yaml files

539dc49

mhpanah force-pushed the robot branch from 7dd3448 to 539dc49 Compare October 15, 2018 22:41

mhpanah merged commit d0a2f6e into ros-navigation:master Oct 15, 2018

mhpanah mentioned this pull request Oct 16, 2018

Define Robot class and interface #22

Closed

mini-1235 pushed a commit to mini-1235/navigation2 that referenced this pull request Feb 5, 2025

Docs for PR ros-navigation#2204 (ros-navigation#144)

41e5b46

* added to the BT XML list * tilte underline * added to the full navigation plugin list * Migration

Forsyth-Creations pushed a commit to Forsyth-Creations/navigation2 that referenced this pull request Feb 19, 2025

Fix industrial CI (ros-navigation#144)

b2200e1

Conversation

mhpanah commented Oct 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjeronimo Oct 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjeronimo left a comment

Choose a reason for hiding this comment

Uh oh!

mkhansenbot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mkhansenbot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mkhansenbot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

mhpanah commented Oct 5, 2018 •

edited

Loading

mjeronimo Oct 5, 2018 •

edited

Loading