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

Bug fixes in main branch #362

Merged
merged 6 commits into from
Feb 23, 2021
Merged

Conversation

JafarAbdi
Copy link
Contributor

Description

Please explain the changes you made, including a reference to the related issue if applicable

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

@henningkayser henningkayser self-requested a review February 9, 2021 15:08
@codecov
Copy link

codecov bot commented Feb 9, 2021

Codecov Report

Merging #362 (9fc33ee) into main (c38b13e) will increase coverage by 0.01%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #362      +/-   ##
==========================================
+ Coverage   53.23%   53.24%   +0.01%     
==========================================
  Files         209      209              
  Lines       18856    18849       -7     
==========================================
- Hits        10037    10034       -3     
+ Misses       8819     8815       -4     
Impacted Files Coverage Δ
...c/default_capabilities/tf_publisher_capability.cpp 0.00% <0.00%> (ø)
...eit_core/robot_trajectory/src/robot_trajectory.cpp 34.08% <40.00%> (-0.41%) ⬇️
...ontroller_manager/action_based_controller_handle.h 61.02% <100.00%> (-0.64%) ⬇️
...nning_scene_monitor/src/planning_scene_monitor.cpp 56.37% <0.00%> (+0.16%) ⬆️

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 c38b13e...6a820b0. Read the comment docs.

Copy link
Member

@tylerjw tylerjw left a comment

Choose a reason for hiding this comment

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

Please update the description to describe what you are changing here.

Copy link
Member

@tylerjw tylerjw left a comment

Choose a reason for hiding this comment

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

new changes look good

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.

Just a question, then +1

@@ -258,27 +255,24 @@ void RobotTrajectory::getRobotTrajectoryMsg(moveit_msgs::msg::RobotTrajectory& t
if (!onedof.empty())
{
trajectory.joint_trajectory.header.frame_id = robot_model_->getModelFrame();
trajectory.joint_trajectory.header.stamp = stamp;
trajectory.joint_trajectory.header.stamp = rclcpp::Time(0, 0, RCL_ROS_TIME);
Copy link
Member

Choose a reason for hiding this comment

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

Looking at the documentation for rclcpp::Time, it looks like the default is RCL_SYSTEM_TIME. What's the reason for something different here?

Copy link
Member

@henningkayser henningkayser Feb 17, 2021

Choose a reason for hiding this comment

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

RCL_ROS_TIME is default for node time as it should be able to support simulation and is therefore mostly used for messages. It's also the equivalent for the old ros::Time while system time uses the system clock directly. See https://design.ros2.org/articles/clock_and_time.html for background on time sources.

@henningkayser henningkayser merged commit 97dd653 into moveit:main Feb 23, 2021
MikeWrock pushed a commit to MikeWrock/moveit2 that referenced this pull request Aug 15, 2022
some mentioned ~/ws_moveit2/ and some ~/ws_moveit/
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.

None yet

4 participants